From 1c0855b495b83747f43bcaa2b09053532b1508a3 Mon Sep 17 00:00:00 2001 From: lly5044 <504496614@qq.com> Date: Tue, 28 Nov 2017 19:25:14 -0600 Subject: [PATCH 1/4] add response and uri field to RetryableStatusCodeException (#272) * add response and uri field to RetryableStatusCodeException * fix the bug in RetryLoadBalancerInterceptor * wrap throwable regardless --- .../RetryLoadBalancerInterceptor.java | 114 ++++++++++++++---- .../RetryableStatusCodeException.java | 19 +++ .../RetryLoadBalancerInterceptorTest.java | 37 +++++- 3 files changed, 146 insertions(+), 24 deletions(-) diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java index f33ab29f..d59f1103 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java @@ -16,16 +16,23 @@ package org.springframework.cloud.client.loadbalancer; +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; import java.io.IOException; +import java.io.InputStream; import java.net.URI; import org.springframework.cloud.client.ServiceInstance; +import org.springframework.http.HttpHeaders; import org.springframework.http.HttpRequest; +import org.springframework.http.HttpStatus; import org.springframework.http.client.ClientHttpRequestExecution; import org.springframework.http.client.ClientHttpRequestInterceptor; import org.springframework.http.client.ClientHttpResponse; +import org.springframework.retry.RecoveryCallback; import org.springframework.retry.RetryCallback; import org.springframework.retry.RetryContext; +import org.springframework.retry.RetryException; import org.springframework.retry.backoff.BackOffPolicy; import org.springframework.retry.backoff.NoBackOffPolicy; import org.springframework.retry.policy.NeverRetryPolicy; @@ -89,27 +96,90 @@ public class RetryLoadBalancerInterceptor implements ClientHttpRequestIntercepto serviceName)); return template .execute(new RetryCallback() { - @Override - public ClientHttpResponse doWithRetry(RetryContext context) - throws IOException { - ServiceInstance serviceInstance = null; - if (context instanceof LoadBalancedRetryContext) { - LoadBalancedRetryContext lbContext = (LoadBalancedRetryContext) context; - serviceInstance = lbContext.getServiceInstance(); - } - if (serviceInstance == null) { - serviceInstance = loadBalancer.choose(serviceName); - } - ClientHttpResponse response = RetryLoadBalancerInterceptor.this.loadBalancer.execute( - serviceName, serviceInstance, - requestFactory.createRequest(request, body, execution)); - int statusCode = response.getRawStatusCode(); - if(retryPolicy != null && retryPolicy.retryableStatusCode(statusCode)) { - response.close(); - throw new RetryableStatusCodeException(serviceName, statusCode); - } - return response; - } - }); + @Override + public ClientHttpResponse doWithRetry(RetryContext context) + throws IOException { + ServiceInstance serviceInstance = null; + if (context instanceof LoadBalancedRetryContext) { + LoadBalancedRetryContext lbContext = (LoadBalancedRetryContext) context; + serviceInstance = lbContext.getServiceInstance(); + } + if (serviceInstance == null) { + serviceInstance = loadBalancer.choose(serviceName); + } + ClientHttpResponse response = RetryLoadBalancerInterceptor.this.loadBalancer.execute( + serviceName, serviceInstance, + requestFactory.createRequest(request, body, execution)); + int statusCode = response.getRawStatusCode(); + if (retryPolicy != null && retryPolicy.retryableStatusCode(statusCode)) { + ClientHttpResponseWrapper wrapper = new ClientHttpResponseWrapper(response); + wrapper.init(); + throw new RetryableStatusCodeException(serviceName, statusCode, wrapper, null); + } + return response; + } + }, new RecoveryCallback() { + @Override + public ClientHttpResponse recover(RetryContext retryContext) throws Exception { + Throwable lastThrowable = retryContext.getLastThrowable(); + if (lastThrowable != null && lastThrowable instanceof RetryableStatusCodeException) { + RetryableStatusCodeException ex = (RetryableStatusCodeException) lastThrowable; + return (ClientHttpResponse) ex.getResponse(); + } + throw new RetryException("Could not recover", lastThrowable); + } + }); } + + public static class ClientHttpResponseWrapper implements ClientHttpResponse { + + private ClientHttpResponse response; + private InputStream content; + + public ClientHttpResponseWrapper(ClientHttpResponse response) { + this.response = response; + } + + public void init() throws IOException { + InputStream body = response.getBody(); + ByteArrayOutputStream temp = new ByteArrayOutputStream(); + byte[] buffer = new byte[4096]; + int length = 0; + while ((length = body.read(buffer)) != -1) { + temp.write(buffer, 0, length); + } + content = new ByteArrayInputStream(temp.toByteArray()); + response.close(); + } + + @Override + public HttpStatus getStatusCode() throws IOException { + return response.getStatusCode(); + } + + @Override + public int getRawStatusCode() throws IOException { + return response.getRawStatusCode(); + } + + @Override + public String getStatusText() throws IOException { + return response.getStatusText(); + } + + @Override + public void close() { + response.close(); + } + + @Override + public InputStream getBody() throws IOException { + return content; + } + + @Override + public HttpHeaders getHeaders() { + return response.getHeaders(); + } + } } diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryableStatusCodeException.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryableStatusCodeException.java index d5d4b2db..ddce92d8 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryableStatusCodeException.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryableStatusCodeException.java @@ -1,6 +1,7 @@ package org.springframework.cloud.client.loadbalancer; import java.io.IOException; +import java.net.URI; /** * Exception to be thrown when the status code is deemed to be retryable. @@ -10,7 +11,25 @@ public class RetryableStatusCodeException extends IOException { private static final String MESSAGE = "Service %s returned a status code of %d"; + private Object response; + + private URI uri; + public RetryableStatusCodeException(String serviceId, int statusCode) { super(String.format(MESSAGE, serviceId, statusCode)); } + + public RetryableStatusCodeException(String serviceId, int statusCode, Object response, URI uri) { + super(String.format(MESSAGE, serviceId, statusCode)); + this.response = response; + this.uri = uri; + } + + public Object getResponse() { + return response; + } + + public URI getUri() { + return uri; + } } diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptorTest.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptorTest.java index e749c225..8e913df3 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptorTest.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptorTest.java @@ -1,5 +1,6 @@ package org.springframework.cloud.client.loadbalancer; +import java.io.ByteArrayInputStream; import java.io.IOException; import java.io.InputStream; import java.net.URI; @@ -15,6 +16,7 @@ import org.springframework.http.client.ClientHttpRequestExecution; import org.springframework.http.client.ClientHttpResponse; import org.springframework.mock.http.client.MockClientHttpResponse; import org.springframework.retry.RetryContext; +import org.springframework.retry.RetryException; import org.springframework.retry.backoff.BackOffContext; import org.springframework.retry.backoff.BackOffInterruptedException; import org.springframework.retry.backoff.BackOffPolicy; @@ -57,7 +59,7 @@ public class RetryLoadBalancerInterceptorTest { lbProperties = null; } - @Test(expected = IOException.class) + @Test(expected = RetryException.class) public void interceptDisableRetry() throws Throwable { HttpRequest request = mock(HttpRequest.class); when(request.getURI()).thenReturn(new URI("http://foo")); @@ -142,6 +144,7 @@ public class RetryLoadBalancerInterceptorTest { HttpRequest request = mock(HttpRequest.class); when(request.getURI()).thenReturn(new URI("http://foo")); InputStream notFoundStream = mock(InputStream.class); + when(notFoundStream.read(any(byte[].class))).thenReturn(-1); ClientHttpResponse clientHttpResponseNotFound = new MockClientHttpResponse(notFoundStream, HttpStatus.NOT_FOUND); ClientHttpResponse clientHttpResponseOk = new MockClientHttpResponse(new byte[]{}, HttpStatus.OK); LoadBalancedRetryPolicy policy = mock(LoadBalancedRetryPolicy.class); @@ -166,6 +169,36 @@ public class RetryLoadBalancerInterceptorTest { verify(lbRequestFactory, times(2)).createRequest(request, body, execution); } + @Test + public void interceptRetryFailOnStatusCode() throws Throwable { + HttpRequest request = mock(HttpRequest.class); + when(request.getURI()).thenReturn(new URI("http://foo")); + InputStream notFoundStream = new ByteArrayInputStream("foo".getBytes()); + ClientHttpResponse clientHttpResponseNotFound = new MockClientHttpResponse(notFoundStream, HttpStatus.NOT_FOUND); + LoadBalancedRetryPolicy policy = mock(LoadBalancedRetryPolicy.class); + when(policy.retryableStatusCode(eq(HttpStatus.NOT_FOUND.value()))).thenReturn(true); + when(policy.canRetryNextServer(any(LoadBalancedRetryContext.class))).thenReturn(false); + LoadBalancedRetryPolicyFactory lbRetryPolicyFactory = mock(LoadBalancedRetryPolicyFactory.class); + when(lbRetryPolicyFactory.create(eq("foo"), any(ServiceInstanceChooser.class))).thenReturn(policy); + ServiceInstance serviceInstance = mock(ServiceInstance.class); + when(client.choose(eq("foo"))).thenReturn(serviceInstance); + when(client.execute(eq("foo"), eq(serviceInstance), any(LoadBalancerRequest.class))). + thenReturn(clientHttpResponseNotFound); + lbProperties.setEnabled(true); + RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, lbProperties, lbRetryPolicyFactory, lbRequestFactory); + byte[] body = new byte[]{}; + ClientHttpRequestExecution execution = mock(ClientHttpRequestExecution.class); + ClientHttpResponse rsp = interceptor.intercept(request, body, execution); + verify(client, times(1)).execute(eq("foo"), eq(serviceInstance), any(LoadBalancerRequest.class)); + verify(lbRequestFactory, times(1)).createRequest(request, body, execution); + verify(policy, times(2)).canRetryNextServer(any(LoadBalancedRetryContext.class)); + //call twice in a retry attempt + byte[] content = new byte[1024]; + int length = rsp.getBody().read(content); + assertThat(length, is("foo".getBytes().length)); + assertThat(new String(content, 0, length), is("foo")); + } + @Test public void interceptRetry() throws Throwable { HttpRequest request = mock(HttpRequest.class); @@ -193,7 +226,7 @@ public class RetryLoadBalancerInterceptorTest { assertThat(backOffPolicy.getBackoffAttempts(), is(1)); } - @Test(expected = IOException.class) + @Test(expected = RetryException.class) public void interceptFailedRetry() throws Exception { HttpRequest request = mock(HttpRequest.class); when(request.getURI()).thenReturn(new URI("http://foo")); From 26a429c83d50bf0e623e244f763752c6e9ec1671 Mon Sep 17 00:00:00 2001 From: Michael Wirth Date: Sun, 3 Dec 2017 12:07:00 -0200 Subject: [PATCH 2/4] Add SSL support for connection factory with SSL validation (#278) --- ...cheHttpClientConnectionManagerFactory.java | 41 ++++++++------ ...tpClientConnectionManagerFactoryTests.java | 54 +++++++++++++++++-- 2 files changed, 74 insertions(+), 21 deletions(-) diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/commons/httpclient/DefaultApacheHttpClientConnectionManagerFactory.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/commons/httpclient/DefaultApacheHttpClientConnectionManagerFactory.java index 136ce607..6ade938a 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/commons/httpclient/DefaultApacheHttpClientConnectionManagerFactory.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/commons/httpclient/DefaultApacheHttpClientConnectionManagerFactory.java @@ -23,6 +23,7 @@ import org.apache.commons.logging.Log; /** * Default implementation of {@link ApacheHttpClientConnectionManagerFactory}. * @author Ryan Baxter + * @author Michael Wirth */ public class DefaultApacheHttpClientConnectionManagerFactory implements ApacheHttpClientConnectionManagerFactory { @@ -47,24 +48,11 @@ public class DefaultApacheHttpClientConnectionManagerFactory if (disableSslValidation) { try { final SSLContext sslContext = SSLContext.getInstance("SSL"); - sslContext.init(null, new TrustManager[] { new X509TrustManager() { - @Override - public void checkClientTrusted(X509Certificate[] x509Certificates, - String s) throws CertificateException { - } - - @Override - public void checkServerTrusted(X509Certificate[] x509Certificates, - String s) throws CertificateException { - } - - @Override - public X509Certificate[] getAcceptedIssuers() { - return null; - } - } }, new SecureRandom()); + sslContext.init(null, + new TrustManager[] { new DisabledValidationTrustManager()}, + new SecureRandom()); registryBuilder.register(HTTPS_SCHEME, new SSLConnectionSocketFactory( - sslContext, NoopHostnameVerifier.INSTANCE)); + sslContext, NoopHostnameVerifier.INSTANCE)); } catch (NoSuchAlgorithmException e) { LOG.warn("Error creating SSLContext", e); @@ -72,6 +60,8 @@ public class DefaultApacheHttpClientConnectionManagerFactory catch (KeyManagementException e) { LOG.warn("Error creating SSLContext", e); } + } else { + registryBuilder.register("https", SSLConnectionSocketFactory.getSocketFactory()); } final Registry registry = registryBuilder.build(); @@ -82,4 +72,21 @@ public class DefaultApacheHttpClientConnectionManagerFactory return connectionManager; } + + class DisabledValidationTrustManager implements X509TrustManager { + @Override + public void checkClientTrusted(X509Certificate[] x509Certificates, + String s) throws CertificateException { + } + + @Override + public void checkServerTrusted(X509Certificate[] x509Certificates, + String s) throws CertificateException { + } + + @Override + public X509Certificate[] getAcceptedIssuers() { + return null; + } + } } diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/commons/httpclient/DefaultApacheHttpClientConnectionManagerFactoryTests.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/commons/httpclient/DefaultApacheHttpClientConnectionManagerFactoryTests.java index 8f265eab..bab4eff0 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/commons/httpclient/DefaultApacheHttpClientConnectionManagerFactoryTests.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/commons/httpclient/DefaultApacheHttpClientConnectionManagerFactoryTests.java @@ -1,16 +1,26 @@ package org.springframework.cloud.commons.httpclient; -import java.lang.reflect.Field; -import java.util.concurrent.TimeUnit; +import org.apache.http.config.Lookup; import org.apache.http.conn.HttpClientConnectionManager; +import org.apache.http.conn.socket.ConnectionSocketFactory; +import org.apache.http.impl.conn.DefaultHttpClientConnectionOperator; import org.apache.http.impl.conn.PoolingHttpClientConnectionManager; import org.junit.Test; import org.springframework.util.ReflectionUtils; -import static org.junit.Assert.*; +import javax.net.ssl.SSLContextSpi; +import javax.net.ssl.SSLSocketFactory; +import javax.net.ssl.X509TrustManager; +import java.lang.reflect.Field; +import java.util.concurrent.TimeUnit; + +import static org.hamcrest.Matchers.*; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThat; /** * @author Ryan Baxter + * @author Michael Wirth */ public class DefaultApacheHttpClientConnectionManagerFactoryTests { @Test @@ -43,6 +53,42 @@ public class DefaultApacheHttpClientConnectionManagerFactoryTests { assertEquals(TimeUnit.DAYS, timeUnit); } + @Test + public void newConnectionManagerWithSSL() throws Exception { + HttpClientConnectionManager connectionManager = new DefaultApacheHttpClientConnectionManagerFactory() + .newConnectionManager(false, 2, 6); + + Lookup socketFactoryRegistry = getConnectionSocketFactoryLookup( + connectionManager); + assertThat(socketFactoryRegistry.lookup("https"), is(notNullValue())); + assertThat(getX509TrustManager(socketFactoryRegistry).getAcceptedIssuers(), is(notNullValue())); + } + + @Test + public void newConnectionManagerWithDisabledSSLValidation() throws Exception { + HttpClientConnectionManager connectionManager = new DefaultApacheHttpClientConnectionManagerFactory() + .newConnectionManager(true, 2, 6); + + Lookup socketFactoryRegistry = getConnectionSocketFactoryLookup( + connectionManager); + assertThat(socketFactoryRegistry.lookup("https"), is(notNullValue())); + assertThat(getX509TrustManager(socketFactoryRegistry).getAcceptedIssuers(), is(nullValue())); + } + + private Lookup getConnectionSocketFactoryLookup( + HttpClientConnectionManager connectionManager) { + DefaultHttpClientConnectionOperator connectionOperator = getField(connectionManager, "connectionOperator"); + return getField(connectionOperator, "socketFactoryRegistry"); + } + + private X509TrustManager getX509TrustManager( + Lookup socketFactoryRegistry) { + ConnectionSocketFactory connectionSocketFactory = socketFactoryRegistry.lookup("https"); + SSLSocketFactory sslSocketFactory = getField(connectionSocketFactory, "socketfactory"); + SSLContextSpi sslContext = getField(sslSocketFactory, "context"); + return getField(sslContext, "trustManager"); + } + @SuppressWarnings("unchecked") protected T getField(Object target, String name) { Field field = ReflectionUtils.findField(target.getClass(), name); @@ -50,4 +96,4 @@ public class DefaultApacheHttpClientConnectionManagerFactoryTests { Object value = ReflectionUtils.getField(field, target); return (T) value; } -} \ No newline at end of file +} From 20798377dfe7c308f15672578866ba1d0989a91e Mon Sep 17 00:00:00 2001 From: saga Date: Tue, 12 Dec 2017 23:50:02 +0800 Subject: [PATCH 3/4] fix #283 (#286) fix #283 --- .../cloud/client/loadbalancer/LoadBalancerAutoConfiguration.java | 1 + 1 file changed, 1 insertion(+) diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerAutoConfiguration.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerAutoConfiguration.java index 1285abe7..67530c4b 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerAutoConfiguration.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerAutoConfiguration.java @@ -105,6 +105,7 @@ public class LoadBalancerAutoConfiguration { @ConditionalOnClass(RetryTemplate.class) public static class RetryAutoConfiguration { @Bean + @ConditionalOnMissingBean public RetryTemplate retryTemplate() { RetryTemplate template = new RetryTemplate(); template.setThrowLastExceptionOnExhausted(true); From c5382ccfd6144c5f1d9c6c633052f27bbcbd10bd Mon Sep 17 00:00:00 2001 From: Dave Syer Date: Fri, 15 Dec 2017 14:13:41 +0000 Subject: [PATCH 4/4] Refactor ConfigurationPropertiesRebinder a bit It's not super expensive, but it all adds up, and rebinding beans that don't need to be rebound is wasteful. This change should improve things in three places: The ConfigurationPropertiesRebinder has always done a pre-emptive re-bind on startup, which was a bit of a blunt instrument, since all we really wanted as to rebind the beans in the parent context (to the current context's Environment). The rebinding always happened twice for every bean because we explicitly called into the ConfigurationPropertiesPostProcessor directly as well as indirectly through the initialization callbacks. Rebinding in response to an EnvironmentChangeEvent was not able to distinguish between events published locally and by child contexts. The child context always binds the parent's beans anyway, so you don't need to do it twice. --- ...onPropertiesRebinderAutoConfiguration.java | 27 +++++----- .../PropertySourceBootstrapConfiguration.java | 10 ++-- ...ironmentDecryptApplicationInitializer.java | 13 +++-- .../environment/EnvironmentChangeEvent.java | 9 +++- .../environment/EnvironmentManager.java | 4 +- .../ConfigurationPropertiesRebinder.java | 16 +++--- .../context/refresh/ContextRefresher.java | 12 +++-- ...ionPropertiesRebinderIntegrationTests.java | 49 +++++++++++++++---- .../cloud/logging/LoggingRebinderTests.java | 5 +- .../resources/application-config.properties | 1 + .../resources/bootstrap-config.properties | 2 + 11 files changed, 101 insertions(+), 47 deletions(-) create mode 100644 spring-cloud-context/src/test/resources/application-config.properties create mode 100644 spring-cloud-context/src/test/resources/bootstrap-config.properties diff --git a/spring-cloud-context/src/main/java/org/springframework/cloud/autoconfigure/ConfigurationPropertiesRebinderAutoConfiguration.java b/spring-cloud-context/src/main/java/org/springframework/cloud/autoconfigure/ConfigurationPropertiesRebinderAutoConfiguration.java index fae23c21..65c09423 100644 --- a/spring-cloud-context/src/main/java/org/springframework/cloud/autoconfigure/ConfigurationPropertiesRebinderAutoConfiguration.java +++ b/spring-cloud-context/src/main/java/org/springframework/cloud/autoconfigure/ConfigurationPropertiesRebinderAutoConfiguration.java @@ -16,8 +16,6 @@ package org.springframework.cloud.autoconfigure; -import java.util.Collections; - import org.springframework.beans.factory.SmartInitializingSingleton; import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; @@ -25,7 +23,6 @@ import org.springframework.boot.autoconfigure.condition.SearchStrategy; import org.springframework.boot.context.properties.ConfigurationBeanFactoryMetaData; import org.springframework.boot.context.properties.ConfigurationPropertiesBindingPostProcessor; import org.springframework.boot.context.properties.ConfigurationPropertiesBindingPostProcessorRegistrar; -import org.springframework.cloud.context.environment.EnvironmentChangeEvent; import org.springframework.cloud.context.properties.ConfigurationPropertiesBeans; import org.springframework.cloud.context.properties.ConfigurationPropertiesRebinder; import org.springframework.context.ApplicationContext; @@ -64,19 +61,27 @@ public class ConfigurationPropertiesRebinderAutoConfiguration @Bean @ConditionalOnMissingBean(search = SearchStrategy.CURRENT) public ConfigurationPropertiesRebinder configurationPropertiesRebinder( - ConfigurationPropertiesBeans beans, - ConfigurationPropertiesBindingPostProcessor binder) { + ConfigurationPropertiesBeans beans) { ConfigurationPropertiesRebinder rebinder = new ConfigurationPropertiesRebinder( - binder, beans); + beans); return rebinder; } @Override public void afterSingletonsInstantiated() { - // After all beans are initialized send a pre-emptive EnvironmentChangeEvent - // so that anything that needs to rebind gets a chance now (especially for - // beans in the parent context) - this.context.publishEvent( - new EnvironmentChangeEvent(Collections. emptySet())); + // After all beans are initialized explicitly rebind beans from the parent + // so that changes during the initialization of the current context are + // reflected. In particular this can be important when low level services like + // decryption are bootstrapped in the parent, but need to change their + // configuration before the child context is processed. + if (this.context.getParent() != null) { + // TODO: make this optional? (E.g. when creating child contexts that prefer to + // be isolated.) + ConfigurationPropertiesRebinder rebinder = context + .getBean(ConfigurationPropertiesRebinder.class); + for (String name : context.getParent().getBeanDefinitionNames()) { + rebinder.rebind(name); + } + } } } \ No newline at end of file diff --git a/spring-cloud-context/src/main/java/org/springframework/cloud/bootstrap/config/PropertySourceBootstrapConfiguration.java b/spring-cloud-context/src/main/java/org/springframework/cloud/bootstrap/config/PropertySourceBootstrapConfiguration.java index fbbc9ed3..376f3efa 100644 --- a/spring-cloud-context/src/main/java/org/springframework/cloud/bootstrap/config/PropertySourceBootstrapConfiguration.java +++ b/spring-cloud-context/src/main/java/org/springframework/cloud/bootstrap/config/PropertySourceBootstrapConfiguration.java @@ -26,6 +26,7 @@ import java.util.TreeSet; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; + import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.bind.PropertySourcesPropertyValues; import org.springframework.boot.bind.RelaxedDataBinder; @@ -107,7 +108,7 @@ public class PropertySourceBootstrapConfiguration implements } insertPropertySources(propertySources, composite); reinitializeLoggingSystem(environment, logConfig, logFile); - setLogLevels(environment); + setLogLevels(applicationContext, environment); handleIncludedProfiles(environment); } } @@ -139,13 +140,14 @@ public class PropertySourceBootstrapConfiguration implements } } - private void setLogLevels(ConfigurableEnvironment environment) { + private void setLogLevels(ConfigurableApplicationContext applicationContext, + ConfigurableEnvironment environment) { LoggingRebinder rebinder = new LoggingRebinder(); rebinder.setEnvironment(environment); // We can't fire the event in the ApplicationContext here (too early), but we can // create our own listener and poke it (it doesn't need the key changes) - rebinder.onApplicationEvent( - new EnvironmentChangeEvent(Collections. emptySet())); + rebinder.onApplicationEvent(new EnvironmentChangeEvent(applicationContext, + Collections.emptySet())); } private void insertPropertySources(MutablePropertySources propertySources, diff --git a/spring-cloud-context/src/main/java/org/springframework/cloud/bootstrap/encrypt/EnvironmentDecryptApplicationInitializer.java b/spring-cloud-context/src/main/java/org/springframework/cloud/bootstrap/encrypt/EnvironmentDecryptApplicationInitializer.java index 355c661b..ca5bbeeb 100644 --- a/spring-cloud-context/src/main/java/org/springframework/cloud/bootstrap/encrypt/EnvironmentDecryptApplicationInitializer.java +++ b/spring-cloud-context/src/main/java/org/springframework/cloud/bootstrap/encrypt/EnvironmentDecryptApplicationInitializer.java @@ -25,6 +25,7 @@ import java.util.regex.Pattern; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; + import org.springframework.cloud.bootstrap.BootstrapApplicationListener; import org.springframework.cloud.context.environment.EnvironmentChangeEvent; import org.springframework.context.ApplicationContext; @@ -96,8 +97,8 @@ public class EnvironmentDecryptApplicationInitializer implements if (!map.isEmpty()) { // We have some decrypted properties found.addAll(map.keySet()); - insert(applicationContext, - new SystemEnvironmentPropertySource(DECRYPTED_PROPERTY_SOURCE_NAME, map)); + insert(applicationContext, new SystemEnvironmentPropertySource( + DECRYPTED_PROPERTY_SOURCE_NAME, map)); } PropertySource bootstrap = propertySources .get(BootstrapApplicationListener.BOOTSTRAP_PROPERTY_SOURCE_NAME); @@ -115,7 +116,7 @@ public class EnvironmentDecryptApplicationInitializer implements // The parent is actually the bootstrap context, and it is fully // initialized, so we can fire an EnvironmentChangeEvent there to rebind // @ConfigurationProperties, in case they were encrypted. - parent.publishEvent(new EnvironmentChangeEvent(found)); + parent.publishEvent(new EnvironmentChangeEvent(parent, found)); } } @@ -214,8 +215,10 @@ public class EnvironmentDecryptApplicationInitializer implements if (COLLECTION_PROPERTY.matcher(key).matches()) { sourceHasDecryptedCollection = true; } - } else if (COLLECTION_PROPERTY.matcher(key).matches()){ - // put non-ecrypted properties so merging of index properties happens correctly + } + else if (COLLECTION_PROPERTY.matcher(key).matches()) { + // put non-ecrypted properties so merging of index properties + // happens correctly otherCollectionProperties.put(key, value); } } diff --git a/spring-cloud-context/src/main/java/org/springframework/cloud/context/environment/EnvironmentChangeEvent.java b/spring-cloud-context/src/main/java/org/springframework/cloud/context/environment/EnvironmentChangeEvent.java index d0fb5e50..d58e24d8 100644 --- a/spring-cloud-context/src/main/java/org/springframework/cloud/context/environment/EnvironmentChangeEvent.java +++ b/spring-cloud-context/src/main/java/org/springframework/cloud/context/environment/EnvironmentChangeEvent.java @@ -32,10 +32,15 @@ public class EnvironmentChangeEvent extends ApplicationEvent { private Set keys; public EnvironmentChangeEvent(Set keys) { - super(keys); + // Backwards compatible constructor with less utility (practically no use at all) + this(keys, keys); + } + + public EnvironmentChangeEvent(Object context, Set keys) { + super(context); this.keys = keys; } - + /** * @return the keys */ diff --git a/spring-cloud-context/src/main/java/org/springframework/cloud/context/environment/EnvironmentManager.java b/spring-cloud-context/src/main/java/org/springframework/cloud/context/environment/EnvironmentManager.java index d1addd47..12a89dd1 100644 --- a/spring-cloud-context/src/main/java/org/springframework/cloud/context/environment/EnvironmentManager.java +++ b/spring-cloud-context/src/main/java/org/springframework/cloud/context/environment/EnvironmentManager.java @@ -68,7 +68,7 @@ public class EnvironmentManager implements ApplicationEventPublisherAware { Map result = new LinkedHashMap(map); if (!map.isEmpty()) { map.clear(); - publish(new EnvironmentChangeEvent(result.keySet())); + publish(new EnvironmentChangeEvent(publisher, result.keySet())); } return result; } @@ -88,7 +88,7 @@ public class EnvironmentManager implements ApplicationEventPublisherAware { if (!value.equals(environment.getProperty(name))) { map.put(name, value); - publish(new EnvironmentChangeEvent(Collections.singleton(name))); + publish(new EnvironmentChangeEvent(publisher, Collections.singleton(name))); } } diff --git a/spring-cloud-context/src/main/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinder.java b/spring-cloud-context/src/main/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinder.java index 0b156443..e7d6693c 100644 --- a/spring-cloud-context/src/main/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinder.java +++ b/spring-cloud-context/src/main/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinder.java @@ -24,7 +24,6 @@ import org.springframework.aop.framework.Advised; import org.springframework.aop.support.AopUtils; import org.springframework.beans.BeansException; import org.springframework.boot.context.properties.ConfigurationProperties; -import org.springframework.boot.context.properties.ConfigurationPropertiesBindingPostProcessor; import org.springframework.cloud.context.config.annotation.RefreshScope; import org.springframework.cloud.context.environment.EnvironmentChangeEvent; import org.springframework.context.ApplicationContext; @@ -55,16 +54,11 @@ public class ConfigurationPropertiesRebinder private ConfigurationPropertiesBeans beans; - private ConfigurationPropertiesBindingPostProcessor binder; - private ApplicationContext applicationContext; private Map errors = new ConcurrentHashMap<>(); - public ConfigurationPropertiesRebinder( - ConfigurationPropertiesBindingPostProcessor binder, - ConfigurationPropertiesBeans beans) { - this.binder = binder; + public ConfigurationPropertiesRebinder(ConfigurationPropertiesBeans beans) { this.beans = beans; } @@ -102,7 +96,7 @@ public class ConfigurationPropertiesRebinder if (AopUtils.isCglibProxy(bean)) { bean = getTargetObject(bean); } - this.binder.postProcessBeforeInitialization(bean, name); + this.applicationContext.getAutowireCapableBeanFactory().destroyBean(bean); this.applicationContext.getAutowireCapableBeanFactory() .initializeBean(bean, name); return true; @@ -135,7 +129,11 @@ public class ConfigurationPropertiesRebinder @Override public void onApplicationEvent(EnvironmentChangeEvent event) { - rebind(); + if (this.applicationContext.equals(event.getSource()) + // Backwards compatible + || event.getKeys().equals(event.getSource())) { + rebind(); + } } } diff --git a/spring-cloud-context/src/main/java/org/springframework/cloud/context/refresh/ContextRefresher.java b/spring-cloud-context/src/main/java/org/springframework/cloud/context/refresh/ContextRefresher.java index 6dd1f129..da405ac9 100644 --- a/spring-cloud-context/src/main/java/org/springframework/cloud/context/refresh/ContextRefresher.java +++ b/spring-cloud-context/src/main/java/org/springframework/cloud/context/refresh/ContextRefresher.java @@ -34,9 +34,15 @@ public class ContextRefresher { private static final String REFRESH_ARGS_PROPERTY_SOURCE = "refreshArgs"; - private static final String[] DEFAULT_PROPERTY_SOURCES = new String[] { //order matters, cli args aren't first, things get messy + private static final String[] DEFAULT_PROPERTY_SOURCES = new String[] { // order + // matters, + // cli args + // aren't + // first, + // things get + // messy CommandLinePropertySource.COMMAND_LINE_PROPERTY_SOURCE_NAME, - "defaultProperties"}; + "defaultProperties" }; private Set standardSources = new HashSet<>( Arrays.asList(StandardEnvironment.SYSTEM_PROPERTIES_PROPERTY_SOURCE_NAME, @@ -59,7 +65,7 @@ public class ContextRefresher { addConfigFilesToEnvironment(); Set keys = changes(before, extract(this.context.getEnvironment().getPropertySources())).keySet(); - this.context.publishEvent(new EnvironmentChangeEvent(keys)); + this.context.publishEvent(new EnvironmentChangeEvent(context, keys)); this.scope.refreshAll(); return keys; } diff --git a/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderIntegrationTests.java b/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderIntegrationTests.java index 85ab65aa..fc7f3e86 100644 --- a/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderIntegrationTests.java +++ b/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderIntegrationTests.java @@ -19,7 +19,9 @@ import javax.annotation.PostConstruct; import org.junit.Test; import org.junit.runner.RunWith; + import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; import org.springframework.boot.autoconfigure.context.PropertyPlaceholderAutoConfiguration; import org.springframework.boot.context.properties.ConfigurationProperties; import org.springframework.boot.context.properties.EnableConfigurationProperties; @@ -33,17 +35,22 @@ import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Import; import org.springframework.core.env.ConfigurableEnvironment; import org.springframework.test.annotation.DirtiesContext; +import org.springframework.test.context.ActiveProfiles; import org.springframework.test.context.junit4.SpringRunner; import static org.junit.Assert.assertEquals; @RunWith(SpringRunner.class) @SpringBootTest(classes = TestConfiguration.class) +@ActiveProfiles("config") public class ConfigurationPropertiesRebinderIntegrationTests { @Autowired private TestProperties properties; + @Autowired + private ConfigProperties config; + @Autowired private ConfigurationPropertiesRebinder rebinder; @@ -54,44 +61,55 @@ public class ConfigurationPropertiesRebinderIntegrationTests { @DirtiesContext public void testSimpleProperties() throws Exception { assertEquals("Hello scope!", this.properties.getMessage()); - assertEquals(2, this.properties.getCount()); + assertEquals(1, this.properties.getCount()); // Change the dynamic property source... EnvironmentTestUtils.addEnvironment(this.environment, "message:Foo"); // ...but don't refresh, so the bean stays the same: assertEquals("Hello scope!", this.properties.getMessage()); - assertEquals(2, this.properties.getCount()); + assertEquals(1, this.properties.getCount()); + } + + @Test + @DirtiesContext + public void testRefreshInParent() throws Exception { + assertEquals("main", this.config.getName()); + // Change the dynamic property source... + EnvironmentTestUtils.addEnvironment(this.environment, "config.name=foo"); + // ...and then refresh, so the bean is re-initialized: + this.rebinder.rebind(); + assertEquals("foo", this.config.getName()); } @Test @DirtiesContext public void testRefresh() throws Exception { - assertEquals(2, this.properties.getCount()); + assertEquals(1, this.properties.getCount()); assertEquals("Hello scope!", this.properties.getMessage()); // Change the dynamic property source... EnvironmentTestUtils.addEnvironment(this.environment, "message:Foo"); // ...and then refresh, so the bean is re-initialized: this.rebinder.rebind(); assertEquals("Foo", this.properties.getMessage()); - assertEquals(3, this.properties.getCount()); + assertEquals(2, this.properties.getCount()); } @Test @DirtiesContext public void testRefreshByName() throws Exception { - assertEquals(2, this.properties.getCount()); + assertEquals(1, this.properties.getCount()); assertEquals("Hello scope!", this.properties.getMessage()); // Change the dynamic property source... EnvironmentTestUtils.addEnvironment(this.environment, "message:Foo"); // ...and then refresh, so the bean is re-initialized: this.rebinder.rebind("properties"); assertEquals("Foo", this.properties.getMessage()); - assertEquals(3, this.properties.getCount()); + assertEquals(2, this.properties.getCount()); } @Configuration @EnableConfigurationProperties @Import({ RefreshConfiguration.RebinderConfiguration.class, - PropertyPlaceholderAutoConfiguration.class }) + PropertyPlaceholderAutoConfiguration.class }) protected static class TestConfiguration { @Bean @@ -104,8 +122,8 @@ public class ConfigurationPropertiesRebinderIntegrationTests { // Hack out a protected inner class for testing protected static class RefreshConfiguration extends RefreshAutoConfiguration { @Configuration - protected static class RebinderConfiguration extends - ConfigurationPropertiesRebinderAutoConfiguration { + protected static class RebinderConfiguration + extends ConfigurationPropertiesRebinderAutoConfiguration { } } @@ -142,4 +160,17 @@ public class ConfigurationPropertiesRebinderIntegrationTests { } } + @ConfigurationProperties("config") + @ConditionalOnMissingBean(ConfigProperties.class) + public static class ConfigProperties { + private String name; + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } + } } diff --git a/spring-cloud-context/src/test/java/org/springframework/cloud/logging/LoggingRebinderTests.java b/spring-cloud-context/src/test/java/org/springframework/cloud/logging/LoggingRebinderTests.java index cb772bfb..646a29d8 100644 --- a/spring-cloud-context/src/test/java/org/springframework/cloud/logging/LoggingRebinderTests.java +++ b/spring-cloud-context/src/test/java/org/springframework/cloud/logging/LoggingRebinderTests.java @@ -21,6 +21,7 @@ import org.junit.After; import org.junit.Test; import org.slf4j.Logger; import org.slf4j.LoggerFactory; + import org.springframework.boot.logging.LogLevel; import org.springframework.boot.logging.LoggingSystem; import org.springframework.boot.test.util.EnvironmentTestUtils; @@ -52,7 +53,7 @@ public class LoggingRebinderTests { EnvironmentTestUtils.addEnvironment(environment, "logging.level.org.springframework.web=TRACE"); this.rebinder.setEnvironment(environment); - this.rebinder.onApplicationEvent(new EnvironmentChangeEvent( + this.rebinder.onApplicationEvent(new EnvironmentChangeEvent(environment, Collections.singleton("logging.level.org.springframework.web"))); assertTrue(this.logger.isTraceEnabled()); } @@ -64,7 +65,7 @@ public class LoggingRebinderTests { EnvironmentTestUtils.addEnvironment(environment, "logging.level.org.springframework.web=trace"); this.rebinder.setEnvironment(environment); - this.rebinder.onApplicationEvent(new EnvironmentChangeEvent( + this.rebinder.onApplicationEvent(new EnvironmentChangeEvent(environment, Collections.singleton("logging.level.org.springframework.web"))); assertTrue(this.logger.isTraceEnabled()); } diff --git a/spring-cloud-context/src/test/resources/application-config.properties b/spring-cloud-context/src/test/resources/application-config.properties new file mode 100644 index 00000000..d4408d3f --- /dev/null +++ b/spring-cloud-context/src/test/resources/application-config.properties @@ -0,0 +1 @@ +config.name: main diff --git a/spring-cloud-context/src/test/resources/bootstrap-config.properties b/spring-cloud-context/src/test/resources/bootstrap-config.properties new file mode 100644 index 00000000..d61d540a --- /dev/null +++ b/spring-cloud-context/src/test/resources/bootstrap-config.properties @@ -0,0 +1,2 @@ +spring.main.sources: org.springframework.cloud.context.properties.ConfigurationPropertiesRebinderIntegrationTests.ConfigProperties +config.name: parent