diff --git a/pom.xml b/pom.xml index 1c5460fd..a89c4d16 100644 --- a/pom.xml +++ b/pom.xml @@ -49,6 +49,13 @@ pom import + + org.springframework.cloud + spring-cloud-commons + test-jar + test + ${spring-cloud-commons.version} + diff --git a/spring-cloud-config-client/pom.xml b/spring-cloud-config-client/pom.xml index 5817b46b..fdefa944 100644 --- a/spring-cloud-config-client/pom.xml +++ b/spring-cloud-config-client/pom.xml @@ -76,6 +76,12 @@ spring-boot-starter-test test + + org.springframework.cloud + spring-cloud-commons + test-jar + test + diff --git a/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServerInstanceProvider.java b/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServerInstanceProvider.java new file mode 100644 index 00000000..8088b077 --- /dev/null +++ b/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServerInstanceProvider.java @@ -0,0 +1,33 @@ +package org.springframework.cloud.config.client; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.springframework.cloud.client.ServiceInstance; +import org.springframework.cloud.client.discovery.DiscoveryClient; +import org.springframework.retry.annotation.Retryable; + +import java.util.List; + +public class ConfigServerInstanceProvider { + + private static Log logger = LogFactory.getLog(ConfigServerInstanceProvider.class); + private final DiscoveryClient client; + + public ConfigServerInstanceProvider(DiscoveryClient client) { + this.client = client; + } + + @Retryable(interceptor = "configServerRetryInterceptor") + public ServiceInstance getConfigServerInstance(String serviceId) { + logger.debug("Locating configserver (" + serviceId + ") via discovery"); + List instances = this.client.getInstances(serviceId); + if (instances.isEmpty()) { + throw new IllegalStateException( + "No instances found of configserver (" + serviceId + ")"); + } + ServiceInstance instance = instances.get(0); + logger.debug( + "Located configserver (" + serviceId + ") via discovery: " + instance); + return instance; + } +} diff --git a/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/DiscoveryClientConfigServiceBootstrapConfiguration.java b/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/DiscoveryClientConfigServiceBootstrapConfiguration.java index ca70599d..f7ae3f77 100644 --- a/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/DiscoveryClientConfigServiceBootstrapConfiguration.java +++ b/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/DiscoveryClientConfigServiceBootstrapConfiguration.java @@ -16,8 +16,6 @@ package org.springframework.cloud.config.client; -import java.util.List; - import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.springframework.beans.factory.annotation.Autowired; @@ -28,6 +26,7 @@ import org.springframework.cloud.client.discovery.EnableDiscoveryClient; import org.springframework.cloud.client.discovery.event.HeartbeatEvent; import org.springframework.cloud.client.discovery.event.HeartbeatMonitor; import org.springframework.cloud.commons.util.UtilAutoConfiguration; +import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Import; import org.springframework.context.event.ContextRefreshedEvent; @@ -52,10 +51,16 @@ public class DiscoveryClientConfigServiceBootstrapConfiguration { private ConfigClientProperties config; @Autowired - private DiscoveryClient client; + private ConfigServerInstanceProvider instanceProvider; private HeartbeatMonitor monitor = new HeartbeatMonitor(); + @Bean + public ConfigServerInstanceProvider configServerInstanceProvider( + DiscoveryClient discoveryClient) { + return new ConfigServerInstanceProvider(discoveryClient); + } + @EventListener(ContextRefreshedEvent.class) public void startup(ContextRefreshedEvent event) { refresh(); @@ -70,14 +75,9 @@ public class DiscoveryClientConfigServiceBootstrapConfiguration { private void refresh() { try { - logger.debug("Locating configserver via discovery"); String serviceId = this.config.getDiscovery().getServiceId(); - List instances = this.client.getInstances(serviceId); - if (instances.isEmpty()) { - logger.warn("No instances found of configserver (" + serviceId + ")"); - return; - } - ServiceInstance server = instances.get(0); + ServiceInstance server = this.instanceProvider + .getConfigServerInstance(serviceId); String url = getHomePage(server); if (server.getMetadata().containsKey("password")) { String user = server.getMetadata().get("user"); @@ -96,7 +96,12 @@ public class DiscoveryClientConfigServiceBootstrapConfiguration { this.config.setUri(url); } catch (Exception ex) { - logger.warn("Could not locate configserver via discovery", ex); + if (config.isFailFast()) { + throw ex; + } + else { + logger.warn("Could not locate configserver via discovery", ex); + } } } diff --git a/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/BaseDiscoveryClientConfigServiceBootstrapConfigurationTests.java b/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/BaseDiscoveryClientConfigServiceBootstrapConfigurationTests.java new file mode 100644 index 00000000..ebc22a32 --- /dev/null +++ b/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/BaseDiscoveryClientConfigServiceBootstrapConfigurationTests.java @@ -0,0 +1,106 @@ +package org.springframework.cloud.config.client; + +import org.junit.After; +import org.junit.Rule; +import org.junit.rules.ExpectedException; +import org.mockito.Mockito; +import org.springframework.boot.autoconfigure.PropertyPlaceholderAutoConfiguration; +import org.springframework.boot.test.util.EnvironmentTestUtils; +import org.springframework.cloud.client.DefaultServiceInstance; +import org.springframework.cloud.client.ServiceInstance; +import org.springframework.cloud.client.discovery.DiscoveryClient; +import org.springframework.cloud.commons.util.UtilAutoConfiguration; +import org.springframework.context.annotation.AnnotationConfigApplicationContext; + +import java.util.Arrays; +import java.util.Collections; + +import static org.junit.Assert.assertEquals; +import static org.mockito.BDDMockito.given; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.springframework.cloud.config.client.ConfigClientProperties.Discovery.DEFAULT_CONFIG_SERVER; + +public abstract class BaseDiscoveryClientConfigServiceBootstrapConfigurationTests { + + @Rule + public ExpectedException expectedException = ExpectedException.none(); + + protected AnnotationConfigApplicationContext context; + + protected DiscoveryClient client = Mockito.mock(DiscoveryClient.class); + + protected ServiceInstance info = new DefaultServiceInstance("app", "foo", 8877, + false); + + @After + public void close() { + if (this.context != null) { + this.context.close(); + } + } + + void givenDiscoveryClientReturnsNoInfo() { + given(this.client.getInstances(DEFAULT_CONFIG_SERVER)) + .willReturn(Collections. emptyList()); + } + + void givenDiscoveryClientReturnsInfo() { + given(this.client.getInstances(DEFAULT_CONFIG_SERVER)) + .willReturn(Arrays.asList(this.info)); + } + + void givenDiscoveryClientReturnsInfoOnThirdTry() { + given(this.client.getInstances(DEFAULT_CONFIG_SERVER)) + .willReturn(Collections. emptyList()) + .willReturn(Collections. emptyList()) + .willReturn(Arrays.asList(this.info)); + } + + void expectNoInstancesOfConfigServerException() { + expectedException.expect(IllegalStateException.class); + expectedException.expectMessage( + "No instances found of configserver (" + DEFAULT_CONFIG_SERVER + ")"); + } + + void expectDiscoveryClientConfigServiceBootstrapConfigurationIsSetup() { + assertEquals(1, this.context.getBeanNamesForType( + DiscoveryClientConfigServiceBootstrapConfiguration.class).length); + } + + void expectConfigClientPropertiesHasDefaultConfiguration() { + expectConfigClientPropertiesHasConfiguration("http://localhost:8888"); + } + + void expectConfigClientPropertiesHasConfigurationFromEureka() { + expectConfigClientPropertiesHasConfiguration("http://foo:8877/"); + } + + void expectConfigClientPropertiesHasConfiguration(final String expectedUri) { + ConfigClientProperties properties = this.context + .getBean(ConfigClientProperties.class); + assertEquals(expectedUri, properties.getRawUri()); + } + + void verifyDiscoveryClientCalledThreeTimes() { + verify(this.client, times(3)).getInstances(DEFAULT_CONFIG_SERVER); + } + + void verifyDiscoveryClientCalledOnce() { + verify(this.client).getInstances(DEFAULT_CONFIG_SERVER); + } + + void setup(String... env) { + this.context = new AnnotationConfigApplicationContext(); + EnvironmentTestUtils.addEnvironment(this.context, env); + EnvironmentTestUtils.addEnvironment(this.context, "eureka.client.enabled=false"); + this.context.getDefaultListableBeanFactory().registerSingleton("discoveryClient", + this.client); + this.context.register(UtilAutoConfiguration.class, + PropertyPlaceholderAutoConfiguration.class, + DiscoveryClientConfigServiceBootstrapConfiguration.class, + ConfigServiceBootstrapConfiguration.class, ConfigClientProperties.class); + this.context.refresh(); + } + +} diff --git a/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/DiscoveryClientConfigServiceBootstrapConfigurationNoSpringRetryTests.java b/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/DiscoveryClientConfigServiceBootstrapConfigurationNoSpringRetryTests.java new file mode 100644 index 00000000..f57831e3 --- /dev/null +++ b/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/DiscoveryClientConfigServiceBootstrapConfigurationNoSpringRetryTests.java @@ -0,0 +1,50 @@ +package org.springframework.cloud.config.client; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.springframework.cloud.ClassPathExclusions; +import org.springframework.cloud.FilteredClassPathRunner; + +@RunWith(FilteredClassPathRunner.class) +@ClassPathExclusions({ "spring-retry-*.jar", "spring-boot-starter-aop-*.jar" }) +public class DiscoveryClientConfigServiceBootstrapConfigurationNoSpringRetryTests + extends BaseDiscoveryClientConfigServiceBootstrapConfigurationTests { + + @Test + public void shouldFailWithExceptionGetConfigServerInstanceFromDiscoveryClient() + throws Exception { + givenDiscoveryClientReturnsNoInfo(); + + expectNoInstancesOfConfigServerException(); + + setup("spring.cloud.config.discovery.enabled=true", + "spring.cloud.config.failFast=true"); + } + + @Test + public void shouldFailWithMessageGetConfigServerInstanceFromDiscoveryClient() + throws Exception { + givenDiscoveryClientReturnsNoInfo(); + + setup("spring.cloud.config.discovery.enabled=true", + "spring.cloud.config.failFast=false"); + + expectDiscoveryClientConfigServiceBootstrapConfigurationIsSetup(); + expectConfigClientPropertiesHasDefaultConfiguration(); + verifyDiscoveryClientCalledOnce(); + } + + @Test + public void shouldSucceedGetConfigServerInstanceFromDiscoveryClient() + throws Exception { + givenDiscoveryClientReturnsInfo(); + + setup("spring.cloud.config.discovery.enabled=true", + "spring.cloud.config.failFast=true"); + + expectDiscoveryClientConfigServiceBootstrapConfigurationIsSetup(); + expectConfigClientPropertiesHasConfigurationFromEureka(); + verifyDiscoveryClientCalledOnce(); + } + +} \ No newline at end of file diff --git a/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/DiscoveryClientConfigServiceBootstrapConfigurationTests.java b/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/DiscoveryClientConfigServiceBootstrapConfigurationTests.java index 1a793e47..f1c00a61 100644 --- a/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/DiscoveryClientConfigServiceBootstrapConfigurationTests.java +++ b/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/DiscoveryClientConfigServiceBootstrapConfigurationTests.java @@ -16,46 +16,24 @@ package org.springframework.cloud.config.client; -import static org.junit.Assert.assertEquals; -import static org.mockito.BDDMockito.given; -import static org.springframework.cloud.config.client.ConfigClientProperties.Discovery.DEFAULT_CONFIG_SERVER; - -import java.util.Arrays; - -import org.junit.After; import org.junit.Test; -import org.mockito.Mockito; -import org.springframework.boot.autoconfigure.PropertyPlaceholderAutoConfiguration; -import org.springframework.boot.test.util.EnvironmentTestUtils; import org.springframework.cloud.client.DefaultServiceInstance; -import org.springframework.cloud.client.ServiceInstance; import org.springframework.cloud.client.discovery.DiscoveryClient; import org.springframework.cloud.client.discovery.event.HeartbeatEvent; -import org.springframework.cloud.commons.util.UtilAutoConfiguration; import org.springframework.context.annotation.AnnotationConfigApplicationContext; +import static org.junit.Assert.assertEquals; + /** * @author Dave Syer */ -public class DiscoveryClientConfigServiceBootstrapConfigurationTests { - - private AnnotationConfigApplicationContext context; - - private DiscoveryClient client = Mockito.mock(DiscoveryClient.class); - - private ServiceInstance info = new DefaultServiceInstance("app", "foo", 8877, false); - - @After - public void close() { - if (this.context != null) { - this.context.close(); - } - } +public class DiscoveryClientConfigServiceBootstrapConfigurationTests extends BaseDiscoveryClientConfigServiceBootstrapConfigurationTests { @Test public void offByDefault() throws Exception { this.context = new AnnotationConfigApplicationContext( DiscoveryClientConfigServiceBootstrapConfiguration.class); + assertEquals(0, this.context.getBeanNamesForType(DiscoveryClient.class).length); assertEquals(0, this.context.getBeanNamesForType( DiscoveryClientConfigServiceBootstrapConfiguration.class).length); @@ -63,51 +41,49 @@ public class DiscoveryClientConfigServiceBootstrapConfigurationTests { @Test public void onWhenRequested() throws Exception { - given(this.client.getInstances(DEFAULT_CONFIG_SERVER)) - .willReturn(Arrays.asList(this.info)); + givenDiscoveryClientReturnsInfo(); + setup("spring.cloud.config.discovery.enabled=true"); - assertEquals(1, this.context.getBeanNamesForType( - DiscoveryClientConfigServiceBootstrapConfiguration.class).length); - Mockito.verify(this.client).getInstances(DEFAULT_CONFIG_SERVER); - ConfigClientProperties locator = this.context - .getBean(ConfigClientProperties.class); - assertEquals("http://foo:8877/", locator.getRawUri()); + + expectDiscoveryClientConfigServiceBootstrapConfigurationIsSetup(); + verifyDiscoveryClientCalledOnce(); + expectConfigClientPropertiesHasConfigurationFromEureka(); } @Test public void onWhenHeartbeat() throws Exception { setup("spring.cloud.config.discovery.enabled=true"); - assertEquals(1, this.context.getBeanNamesForType( - DiscoveryClientConfigServiceBootstrapConfiguration.class).length); - given(this.client.getInstances(DEFAULT_CONFIG_SERVER)) - .willReturn(Arrays.asList(this.info)); - Mockito.verify(this.client).getInstances(DEFAULT_CONFIG_SERVER); + + expectDiscoveryClientConfigServiceBootstrapConfigurationIsSetup(); + + givenDiscoveryClientReturnsInfo(); + verifyDiscoveryClientCalledOnce(); + context.publishEvent(new HeartbeatEvent(context, "new")); - ConfigClientProperties locator = this.context - .getBean(ConfigClientProperties.class); - assertEquals("http://foo:8877/", locator.getRawUri()); + + expectConfigClientPropertiesHasConfigurationFromEureka(); } @Test public void secureWhenRequested() throws Exception { this.info = new DefaultServiceInstance("app", "foo", 443, true); - given(this.client.getInstances(DEFAULT_CONFIG_SERVER)) - .willReturn(Arrays.asList(this.info)); + givenDiscoveryClientReturnsInfo(); + setup("spring.cloud.config.discovery.enabled=true"); - assertEquals(1, this.context.getBeanNamesForType( - DiscoveryClientConfigServiceBootstrapConfiguration.class).length); - Mockito.verify(this.client).getInstances(DEFAULT_CONFIG_SERVER); - ConfigClientProperties locator = this.context - .getBean(ConfigClientProperties.class); - assertEquals("https://foo:443/", locator.getRawUri()); + + expectDiscoveryClientConfigServiceBootstrapConfigurationIsSetup(); + + verifyDiscoveryClientCalledOnce(); + expectConfigClientPropertiesHasConfiguration("https://foo:443/"); } @Test public void setsPasssword() throws Exception { this.info.getMetadata().put("password", "bar"); - given(this.client.getInstances(DEFAULT_CONFIG_SERVER)) - .willReturn(Arrays.asList(this.info)); + givenDiscoveryClientReturnsInfo(); + setup("spring.cloud.config.discovery.enabled=true"); + ConfigClientProperties locator = this.context .getBean(ConfigClientProperties.class); assertEquals("http://foo:8877/", locator.getRawUri()); @@ -118,25 +94,80 @@ public class DiscoveryClientConfigServiceBootstrapConfigurationTests { @Test public void setsPath() throws Exception { this.info.getMetadata().put("configPath", "/bar"); - given(this.client.getInstances(DEFAULT_CONFIG_SERVER)) - .willReturn(Arrays.asList(this.info)); + givenDiscoveryClientReturnsInfo(); + setup("spring.cloud.config.discovery.enabled=true"); - ConfigClientProperties locator = this.context - .getBean(ConfigClientProperties.class); - assertEquals("http://foo:8877/bar", locator.getRawUri()); + + expectConfigClientPropertiesHasConfiguration("http://foo:8877/bar"); } - private void setup(String... env) { - this.context = new AnnotationConfigApplicationContext(); - EnvironmentTestUtils.addEnvironment(this.context, env); - EnvironmentTestUtils.addEnvironment(this.context, "eureka.client.enabled=false"); - this.context.getDefaultListableBeanFactory().registerSingleton("discoveryClient", - this.client); - this.context.register(UtilAutoConfiguration.class, - PropertyPlaceholderAutoConfiguration.class, - DiscoveryClientConfigServiceBootstrapConfiguration.class, - ConfigClientProperties.class); - this.context.refresh(); + @Test + public void shouldFailGetConfigServerInstanceFromDiscoveryClient() throws Exception { + givenDiscoveryClientReturnsNoInfo(); + + setup("spring.cloud.config.discovery.enabled=true"); + + expectDiscoveryClientConfigServiceBootstrapConfigurationIsSetup(); + verifyDiscoveryClientCalledOnce(); + expectConfigClientPropertiesHasDefaultConfiguration(); + } + + @Test + public void shouldRetryAndSucceedGetConfigServerInstanceFromDiscoveryClient() + throws Exception { + givenDiscoveryClientReturnsInfoOnThirdTry(); + + setup("spring.cloud.config.discovery.enabled=true", + "spring.cloud.config.retry.maxAttempts=3", + "spring.cloud.config.retry.initialInterval=10", + "spring.cloud.config.failFast=true"); + + expectDiscoveryClientConfigServiceBootstrapConfigurationIsSetup(); + verifyDiscoveryClientCalledThreeTimes(); + + context.publishEvent(new HeartbeatEvent(context, "new")); + + expectConfigClientPropertiesHasConfigurationFromEureka(); + } + + @Test + public void shouldNotRetryIfNotFailFastPropertySet() throws Exception { + givenDiscoveryClientReturnsInfoOnThirdTry(); + + setup("spring.cloud.config.discovery.enabled=true", + "spring.cloud.config.retry.maxAttempts=3", + "spring.cloud.config.retry.initialInterval=10"); + + expectDiscoveryClientConfigServiceBootstrapConfigurationIsSetup(); + verifyDiscoveryClientCalledOnce(); + expectConfigClientPropertiesHasDefaultConfiguration(); + } + + @Test + public void shouldRetryAndFailWithExceptionGetConfigServerInstanceFromDiscoveryClient() + throws Exception { + givenDiscoveryClientReturnsNoInfo(); + + expectNoInstancesOfConfigServerException(); + + setup("spring.cloud.config.discovery.enabled=true", + "spring.cloud.config.retry.maxAttempts=3", + "spring.cloud.config.retry.initialInterval=10", + "spring.cloud.config.failFast=true"); + } + + @Test + public void shouldRetryAndFailWithMessageGetConfigServerInstanceFromDiscoveryClient() + throws Exception { + givenDiscoveryClientReturnsNoInfo(); + + setup("spring.cloud.config.discovery.enabled=true", + "spring.cloud.config.retry.maxAttempts=3", + "spring.cloud.config.retry.initialInterval=10", + "spring.cloud.config.failFast=false"); + + expectDiscoveryClientConfigServiceBootstrapConfigurationIsSetup(); + expectConfigClientPropertiesHasDefaultConfiguration(); } }