diff --git a/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigClientRetryBootstrapper.java b/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigClientRetryBootstrapper.java new file mode 100644 index 00000000..b61a757c --- /dev/null +++ b/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigClientRetryBootstrapper.java @@ -0,0 +1,65 @@ +/* + * Copyright 2013-2020 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.cloud.config.client; + +import org.springframework.boot.BootstrapRegistry; +import org.springframework.boot.Bootstrapper; +import org.springframework.boot.context.properties.bind.Binder; +import org.springframework.cloud.config.client.ConfigServerBootstrapper.LoaderInterceptor; +import org.springframework.retry.support.RetryTemplate; +import org.springframework.util.ClassUtils; + +/** + * Bootstrapper. + * + * @author Marcin Grzejszczak + * @since 3.0.0 + */ +public class ConfigClientRetryBootstrapper implements Bootstrapper { + + static final boolean RETRY_IS_PRESENT = ClassUtils.isPresent("org.springframework.retry.annotation.Retryable", + null); + + @Override + public void intitialize(BootstrapRegistry registry) { + if (!RETRY_IS_PRESENT) { + return; + } + + registry.registerIfAbsent(RetryProperties.class, context -> context.get(Binder.class) + .bind(RetryProperties.PREFIX, RetryProperties.class).orElseGet(RetryProperties::new)); + + registry.registerIfAbsent(RetryTemplate.class, context -> { + RetryProperties properties = context.get(RetryProperties.class); + return RetryTemplate.builder().maxAttempts(properties.getMaxAttempts()).exponentialBackoff( + properties.getInitialInterval(), properties.getMultiplier(), properties.getMaxInterval()).build(); + }); + registry.registerIfAbsent(LoaderInterceptor.class, context -> { + Binder binder = context.get(Binder.class); + boolean failFast = binder.bind(ConfigClientProperties.PREFIX + ".fail-fast", Boolean.class).orElse(false); + if (failFast) { + // if (false) { + RetryTemplate retryTemplate = context.get(RetryTemplate.class); + return loadContext -> retryTemplate.execute(retryContext -> loadContext.getInvocation() + .apply(loadContext.getLoaderContext(), loadContext.getResource())); + } + return null; + }); + + } + +} diff --git a/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServerConfigDataLoader.java b/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServerConfigDataLoader.java index 47dbf308..4dc3e507 100644 --- a/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServerConfigDataLoader.java +++ b/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServerConfigDataLoader.java @@ -78,8 +78,10 @@ public class ConfigServerConfigDataLoader implements ConfigDataLoader { ConfigServerInstanceProvider.Function function = context .get(ConfigServerInstanceProvider.Function.class); - ConfigServerInstanceProvider instanceProvider = new ConfigServerInstanceProvider(function); + + ConfigServerInstanceProvider instanceProvider; + if (ConfigClientRetryBootstrapper.RETRY_IS_PRESENT && retryEnabled) { + log.debug(LogMessage.format("discovery plus retry enabled")); + RetryTemplate retryTemplate = context.get(RetryTemplate.class); + instanceProvider = new ConfigServerInstanceProvider(function) { + @Override + public List getConfigServerInstances(String serviceId) { + return retryTemplate.execute(retryContext -> super.getConfigServerInstances(serviceId)); + } + }; + } + else { + instanceProvider = new ConfigServerInstanceProvider(function); + } instanceProvider.setLog(log); + ConfigClientProperties clientProperties = context.get(ConfigClientProperties.class); ConfigServerInstanceMonitor instanceMonitor = new ConfigServerInstanceMonitor(log, clientProperties, instanceProvider); diff --git a/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServiceBootstrapConfiguration.java b/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServiceBootstrapConfiguration.java index d6100fbd..6c4a38e9 100644 --- a/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServiceBootstrapConfiguration.java +++ b/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServiceBootstrapConfiguration.java @@ -56,8 +56,7 @@ public class ConfigServiceBootstrapConfiguration { @ConditionalOnMissingBean(ConfigServicePropertySourceLocator.class) @ConditionalOnProperty(name = ConfigClientProperties.PREFIX + ".enabled", matchIfMissing = true) public ConfigServicePropertySourceLocator configServicePropertySource(ConfigClientProperties properties) { - ConfigServicePropertySourceLocator locator = new ConfigServicePropertySourceLocator(properties); - return locator; + return new ConfigServicePropertySourceLocator(properties); } @ConditionalOnProperty(ConfigClientProperties.PREFIX + ".fail-fast") diff --git a/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/RetryProperties.java b/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/RetryProperties.java index fe4bfe4f..903f0c8b 100644 --- a/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/RetryProperties.java +++ b/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/RetryProperties.java @@ -22,9 +22,14 @@ import org.springframework.boot.context.properties.ConfigurationProperties; * @author Dave Syer * */ -@ConfigurationProperties("spring.cloud.config.retry") +@ConfigurationProperties(RetryProperties.PREFIX) public class RetryProperties { + /** + * ConfigurationProperties prefix. + */ + public static final String PREFIX = "spring.cloud.config.retry"; + /** * Initial retry interval in milliseconds. */ diff --git a/spring-cloud-config-client/src/main/resources/META-INF/spring.factories b/spring-cloud-config-client/src/main/resources/META-INF/spring.factories index d981fb42..1e5dc9be 100644 --- a/spring-cloud-config-client/src/main/resources/META-INF/spring.factories +++ b/spring-cloud-config-client/src/main/resources/META-INF/spring.factories @@ -13,3 +13,7 @@ org.springframework.cloud.config.client.ConfigServerConfigDataLocationResolver # ConfigData Loaders org.springframework.boot.context.config.ConfigDataLoader=\ org.springframework.cloud.config.client.ConfigServerConfigDataLoader + +# Spring Boot Bootstrappers +org.springframework.boot.Bootstrapper=\ +org.springframework.cloud.config.client.ConfigClientRetryBootstrapper diff --git a/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/DiscoveryClientConfigDataConfigurationNoRetryTests.java b/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/DiscoveryClientConfigDataConfigurationNoRetryTests.java new file mode 100644 index 00000000..9cfe2592 --- /dev/null +++ b/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/DiscoveryClientConfigDataConfigurationNoRetryTests.java @@ -0,0 +1,166 @@ +/* + * Copyright 2013-2019 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.cloud.config.client; + +import java.util.Arrays; +import java.util.Collections; +import java.util.LinkedHashSet; +import java.util.Set; + +import org.junit.After; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mockito; + +import org.springframework.boot.BootstrapRegistry; +import org.springframework.boot.Bootstrapper; +import org.springframework.boot.SpringBootConfiguration; +import org.springframework.boot.autoconfigure.EnableAutoConfiguration; +import org.springframework.boot.builder.SpringApplicationBuilder; +import org.springframework.cloud.client.DefaultServiceInstance; +import org.springframework.cloud.client.ServiceInstance; +import org.springframework.cloud.client.discovery.DiscoveryClient; +import org.springframework.cloud.config.client.ConfigClientProperties.Credentials; +import org.springframework.cloud.test.ClassPathExclusions; +import org.springframework.cloud.test.ModifiedClassPathRunner; +import org.springframework.context.ConfigurableApplicationContext; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.BDDMockito.given; +import static org.mockito.Mockito.verify; +import static org.springframework.cloud.config.client.ConfigClientProperties.Discovery.DEFAULT_CONFIG_SERVER; + +/** + * @author Dave Syer + */ +@RunWith(ModifiedClassPathRunner.class) +@ClassPathExclusions({ "spring-retry-*.jar", "spring-boot-starter-aop-*.jar" }) +public class DiscoveryClientConfigDataConfigurationNoRetryTests { + + protected ConfigurableApplicationContext context; + + protected DiscoveryClient client = Mockito.mock(DiscoveryClient.class); + + protected ServiceInstance info = new DefaultServiceInstance("app:8877", "app", "foo", 8877, false); + + @After + public void close() { + if (this.context != null) { + this.context.close(); + } + } + + @Test + public void shouldFailWithExceptionGetConfigServerInstanceFromDiscoveryClient() throws Exception { + givenDiscoveryClientReturnsNoInfo(); + + assertThatThrownBy(() -> context = setup("spring.cloud.config.discovery.enabled=true", + "spring.cloud.config.fail-fast=true").run()).isInstanceOf(IllegalStateException.class) + .hasMessageContaining("No instances found of configserver"); + } + + @Test + public void shouldFailWithMessageGetConfigServerInstanceFromDiscoveryClient() throws Exception { + givenDiscoveryClientReturnsNoInfo(); + + context = setup("spring.cloud.config.discovery.enabled=true", "spring.cloud.config.fail-fast=false").run(); + + // expectDiscoveryClientConfigServiceBootstrapConfigurationIsSetup(); + expectConfigClientPropertiesHasDefaultConfiguration(); + verifyDiscoveryClientCalledOnce(); + } + + @Test + public void shouldSucceedGetConfigServerInstanceFromDiscoveryClient() throws Exception { + givenDiscoveryClientReturnsInfo(); + + context = setup("spring.cloud.config.discovery.enabled=true", "spring.cloud.config.fail-fast=true").run(); + + // expectDiscoveryClientConfigServiceBootstrapConfigurationIsSetup(); + // expectConfigClientPropertiesHasConfigurationFromEureka(); + verifyDiscoveryClientCalledOnce(); + } + + SpringApplicationBuilder setup(String... env) { + return setup(true, env); + } + + SpringApplicationBuilder setup(boolean addInstanceProvider, String... env) { + SpringApplicationBuilder builder = new SpringApplicationBuilder(TestConfig.class) + .properties(addDefaultEnv(env)); + if (addInstanceProvider) { + builder.addBootstrapper(instanceProviderBootstrapper()); + // ignore actual calls to config server since we're just testing discovery + // client. + builder.addBootstrapper(registry -> registry.register(ConfigServerBootstrapper.LoaderInterceptor.class, + ctx -> loadContext -> null)); + } + return builder.addBootstrapper(registry -> registry.addCloseListener(event -> { + ConfigServerInstanceMonitor monitor = event.getBootstrapContext().get(ConfigServerInstanceMonitor.class); + assertThat(monitor).as("ConfigServerInstanceMonitor was not created when it should").isNotNull(); + })); + } + + protected Bootstrapper instanceProviderBootstrapper() { + return registry -> registry.register(ConfigServerInstanceProvider.Function.class, + BootstrapRegistry.InstanceSupplier.from(() -> this.client::getInstances)); + } + + private String[] addDefaultEnv(String[] env) { + Set set = new LinkedHashSet<>(); + if (env != null && env.length > 0) { + set.addAll(Arrays.asList(env)); + } + set.add("spring.cloud.config.discovery.enabled=true"); + set.add("spring.config.import=optional:configserver:"); + return set.toArray(new String[0]); + } + + void givenDiscoveryClientReturnsInfo() { + givenDiscoveryClientReturnsInfo(this.info); + } + + void givenDiscoveryClientReturnsInfo(ServiceInstance... instances) { + given(this.client.getInstances(DEFAULT_CONFIG_SERVER)).willReturn(Arrays.asList(instances)); + } + + void givenDiscoveryClientReturnsNoInfo() { + given(this.client.getInstances(DEFAULT_CONFIG_SERVER)).willReturn(Collections.emptyList()); + } + + void verifyDiscoveryClientCalledOnce() { + verify(this.client).getInstances(DEFAULT_CONFIG_SERVER); + } + + void expectConfigClientPropertiesHasDefaultConfiguration() { + expectConfigClientPropertiesHasConfiguration("http://localhost:8888"); + } + + void expectConfigClientPropertiesHasConfiguration(final String expectedUri) { + ConfigClientProperties properties = this.context.getBean(ConfigClientProperties.class); + Credentials credentials = properties.getCredentials(0); + assertThat(credentials.getUri()).isEqualTo(expectedUri); + } + + @SpringBootConfiguration + @EnableAutoConfiguration + static class TestConfig { + + } + +} diff --git a/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/DiscoveryClientConfigDataConfigurationTests.java b/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/DiscoveryClientConfigDataConfigurationTests.java index 0bb8694e..156541f3 100644 --- a/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/DiscoveryClientConfigDataConfigurationTests.java +++ b/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/DiscoveryClientConfigDataConfigurationTests.java @@ -22,7 +22,6 @@ import java.util.LinkedHashSet; import java.util.Set; import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.mockito.Mockito; @@ -39,6 +38,7 @@ import org.springframework.cloud.config.client.ConfigClientProperties.Credential import org.springframework.context.ConfigurableApplicationContext; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.assertj.core.api.Assertions.fail; import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.times; @@ -164,7 +164,6 @@ public class DiscoveryClientConfigDataConfigurationTests { } @Test - @Disabled public void shouldRetryAndSucceedGetConfigServerInstanceFromDiscoveryClient() { givenDiscoveryClientReturnsInfoOnThirdTry(); @@ -179,7 +178,6 @@ public class DiscoveryClientConfigDataConfigurationTests { } @Test - @Disabled public void shouldNotRetryIfNotFailFastPropertySet() { givenDiscoveryClientReturnsInfoOnThirdTry(); @@ -191,18 +189,16 @@ public class DiscoveryClientConfigDataConfigurationTests { } @Test - @Disabled public void shouldRetryAndFailWithExceptionGetConfigServerInstanceFromDiscoveryClient() { givenDiscoveryClientReturnsNoInfo(); - // expectNoInstancesOfConfigServerException(); - - context = setup("spring.cloud.config.retry.maxAttempts=3", "spring.cloud.config.retry.initialInterval=10", - "spring.cloud.config.fail-fast=true").run(); + assertThatThrownBy(() -> context = setup("spring.cloud.config.retry.maxAttempts=3", + "spring.cloud.config.retry.initialInterval=10", "spring.cloud.config.fail-fast=true").run()) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("No instances found of configserver"); } @Test - @Disabled public void shouldRetryAndFailWithMessageGetConfigServerInstanceFromDiscoveryClient() { givenDiscoveryClientReturnsNoInfo(); @@ -221,6 +217,10 @@ public class DiscoveryClientConfigDataConfigurationTests { .properties(addDefaultEnv(env)); if (addInstanceProvider) { builder.addBootstrapper(instanceProviderBootstrapper()); + // ignore actual calls to config server since we're just testing discovery + // client. + builder.addBootstrapper(registry -> registry.register(ConfigServerBootstrapper.LoaderInterceptor.class, + ctx -> loadContext -> null)); } return builder.addBootstrapper(registry -> registry.addCloseListener(event -> { ConfigServerInstanceMonitor monitor = event.getBootstrapContext().get(ConfigServerInstanceMonitor.class); diff --git a/spring-cloud-config-sample/pom.xml b/spring-cloud-config-sample/pom.xml index 1f802aa8..e58f87b8 100644 --- a/spring-cloud-config-sample/pom.xml +++ b/spring-cloud-config-sample/pom.xml @@ -52,6 +52,10 @@ junit-vintage-engine test + + org.springframework.retry + spring-retry + diff --git a/spring-cloud-config-sample/src/test/java/sample/ApplicationFailFastTests.java b/spring-cloud-config-sample/src/test/java/sample/ApplicationFailFastTests.java index 723be8e1..6c45ac5d 100644 --- a/spring-cloud-config-sample/src/test/java/sample/ApplicationFailFastTests.java +++ b/spring-cloud-config-sample/src/test/java/sample/ApplicationFailFastTests.java @@ -20,22 +20,27 @@ import org.junit.Test; import org.springframework.boot.builder.SpringApplicationBuilder; -import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.fail; +import static org.assertj.core.api.Assertions.assertThatThrownBy; public class ApplicationFailFastTests { @Test - public void contextFails() { - try { + public void bootstrapContextFails() { + assertThatThrownBy(() -> { new SpringApplicationBuilder().sources(Application.class).run("--spring.config.use-legacy-processing=true", "--server.port=0", "--spring.cloud.config.enabled=true", "--spring.cloud.config.fail-fast=true", "--spring.cloud.config.uri=http://serverhostdoesnotexist:1234"); - fail("failFast option did not produce an exception"); - } - catch (Exception e) { - assertThat(e.getMessage().contains("fail fast")).as("Exception not caused by fail fast").isTrue(); - } + }).as("Exception not caused by fail fast").hasMessageContaining("fail fast"); + } + + @Test + public void configDataContextFails() { + assertThatThrownBy(() -> { + new SpringApplicationBuilder().sources(Application.class).run("--server.port=0", + "--spring.cloud.config.enabled=true", "--spring.cloud.config.fail-fast=true", + "--spring.config.import=optional:configserver:http://serverhostdoesnotexist:1234"); + }).as("Exception not caused by fail fast").hasMessageContaining("fail fast"); + } } diff --git a/spring-cloud-config-sample/src/test/java/sample/ConfigDataRetryIntegrationTests.java b/spring-cloud-config-sample/src/test/java/sample/ConfigDataRetryIntegrationTests.java new file mode 100644 index 00000000..4c6035e3 --- /dev/null +++ b/spring-cloud-config-sample/src/test/java/sample/ConfigDataRetryIntegrationTests.java @@ -0,0 +1,121 @@ +/* + * Copyright 2018-2019 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package sample; + +import java.io.IOException; +import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.runner.RunWith; + +import org.springframework.boot.SpringApplication; +import org.springframework.boot.actuate.autoconfigure.endpoint.web.WebEndpointProperties; +import org.springframework.boot.autoconfigure.EnableAutoConfiguration; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.test.web.client.TestRestTemplate; +import org.springframework.boot.web.server.LocalServerPort; +import org.springframework.cloud.config.server.EnableConfigServer; +import org.springframework.cloud.config.server.test.ConfigServerTestUtils; +import org.springframework.context.ConfigurableApplicationContext; +import org.springframework.context.annotation.Configuration; +import org.springframework.test.context.junit4.SpringRunner; +import org.springframework.util.SocketUtils; +import org.springframework.web.servlet.HandlerInterceptor; +import org.springframework.web.servlet.config.annotation.InterceptorRegistry; +import org.springframework.web.servlet.config.annotation.WebMvcConfigurer; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.springframework.boot.test.context.SpringBootTest.WebEnvironment.RANDOM_PORT; + +@RunWith(SpringRunner.class) +@SpringBootTest(classes = Application.class, + // Normally spring.cloud.config.enabled:true is the default but since we have the + // config server on the classpath we need to set it explicitly + properties = { "spring.application.name=retryapp", "spring.cloud.config.fail-fast=true", + "spring.cloud.config.enabled=true", "spring.config.import=configserver:", + "management.security.enabled=false", "management.endpoints.web.exposure.include=*", + "logging.level.org.springframework.retry=TRACE" }, + webEnvironment = RANDOM_PORT) +public class ConfigDataRetryIntegrationTests { + + private static final String BASE_PATH = new WebEndpointProperties().getBasePath(); + + private static int configPort = SocketUtils.findAvailableTcpPort(); + + private static ConfigurableApplicationContext server; + + @LocalServerPort + private int port; + + @BeforeClass + public static void startConfigServer() throws IOException { + String baseDir = ConfigServerTestUtils.getBaseDirectory("spring-cloud-config-sample"); + String repo = ConfigServerTestUtils.prepareLocalRepo(baseDir, "target/repos", "config-repo", "target/config"); + server = SpringApplication.run(TestConfig.class, "--server.port=" + configPort, "--spring.config.name=server", + "--spring.cloud.config.server.git.uri=" + repo); + + System.setProperty("spring.cloud.config.uri", "http://localhost:" + configPort); + } + + @AfterClass + public static void close() { + System.clearProperty("spring.cloud.config.uri"); + if (server != null) { + server.close(); + } + } + + @Test + @SuppressWarnings("unchecked") + public void contextLoads() { + Map res = new TestRestTemplate().getForObject("http://localhost:" + this.port + BASE_PATH + "/env/info.foo", + Map.class); + assertThat(res).containsKey("propertySources"); + Map property = (Map) res.get("property"); + assertThat(property).containsEntry("value", "bar"); + } + + @Configuration + @EnableAutoConfiguration + @EnableConfigServer + static class TestConfig implements WebMvcConfigurer { + + AtomicInteger count = new AtomicInteger(0); + + // @Override + public void addInterceptors(InterceptorRegistry registry) { + registry.addInterceptor(new HandlerInterceptor() { + @Override + public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) + throws Exception { + if (request.getServletPath().equals("/retryapp/default")) { + return count.incrementAndGet() > 1; + } + return true; + } + }); + } + + } + +}