From 9ab7acaf6f46cff623ad394cbdd2d898d14eafab Mon Sep 17 00:00:00 2001 From: Indraneel Bende Date: Wed, 13 Jun 2018 16:55:39 -0400 Subject: [PATCH] Made RequestReadTimeout and whether to sendState configurable (#1034) Made RequestReadTimeout and whether to send State configurable. --- .../main/asciidoc/spring-cloud-config.adoc | 4 ++ .../config/client/ConfigClientProperties.java | 43 +++++++++++++------ .../ConfigServicePropertySourceLocator.java | 20 ++++++--- ...nfigServicePropertySourceLocatorTests.java | 43 +++++++++++++++++-- 4 files changed, 87 insertions(+), 23 deletions(-) diff --git a/docs/src/main/asciidoc/spring-cloud-config.adoc b/docs/src/main/asciidoc/spring-cloud-config.adoc index 65af7a57..ef74aff8 100644 --- a/docs/src/main/asciidoc/spring-cloud-config.adoc +++ b/docs/src/main/asciidoc/spring-cloud-config.adoc @@ -1203,6 +1203,10 @@ To ensure high availability when you have multiple instances of Config Server de If you use HTTP basic security on your Config Server, it is currently possible to support per-Config Server auth credentials only if you embed the credentials in each URL you specify under the `spring.cloud.config.uri` property. If you use any other kind of security mechanism, you cannot (currently) support per-Config Server authentication and authorization. +=== Configuring Read Timeouts + +If you want to configure read timeout, this can be done by using the property `spring.cloud.config.request-read-timeout`. + === Security If you use HTTP Basic security on the server, clients need to know the password (and username if it is not the default). diff --git a/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigClientProperties.java b/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigClientProperties.java index 0d70fd6c..ef8aaf19 100644 --- a/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigClientProperties.java +++ b/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigClientProperties.java @@ -18,6 +18,7 @@ package org.springframework.cloud.config.client; import java.net.MalformedURLException; import java.net.URL; +import java.util.Arrays; import java.util.HashMap; import java.util.Map; @@ -39,6 +40,7 @@ public class ConfigClientProperties { public static final String PREFIX = "spring.cloud.config"; public static final String TOKEN_HEADER = "X-Config-Token"; public static final String STATE_HEADER = "X-Config-State"; + public static final String AUTHORIZATION = "authorization"; /** * Flag to say that remote configuration is enabled. Default true; @@ -94,9 +96,14 @@ public class ConfigClientProperties { private String token; /** - * Authorization token used by the client to connect to the server. + * timeout on waiting to read data from the Config Server. */ - private String authorization; + private int requestReadTimeout = (60 * 1000 * 3) + 5000; + + /** + * Flag to indicate whether to send state. Default true. + */ + private boolean sendState = true; /** * Additional headers used to create the client request. @@ -198,14 +205,20 @@ public class ConfigClientProperties { this.token = token; } - @DeprecatedConfigurationProperty(reason = "replaced by headers", replacement = "headers") - @Deprecated - public String getAuthorization() { - return this.authorization; + public int getRequestReadTimeout() { + return requestReadTimeout; } - public void setAuthorization(String authorization) { - this.authorization = authorization; + public void setRequestReadTimeout(int requestReadTimeout) { + this.requestReadTimeout = requestReadTimeout; + } + + public boolean isSendState() { + return sendState; + } + + public void setSendState(boolean sendState) { + this.sendState = sendState; } public Map getHeaders() { @@ -351,12 +364,14 @@ public class ConfigClientProperties { @Override public String toString() { - return "ConfigClientProperties [enabled=" + this.enabled + ", profile=" - + this.profile + ", name=" + this.name + ", label=" - + (this.label == null ? "" : this.label) + ", username=" + this.username - + ", password=" + this.password + ", uri=" + this.uri + ", authorization=" - + this.authorization + ", discovery.enabled=" + this.discovery.enabled - + ", failFast=" + this.failFast + ", token=" + this.token + "]"; + return "ConfigClientProperties [enabled=" + enabled + ", profile=" + profile + + ", name=" + name + ", label=" + label + ", username=" + username + + ", password=" + password + ", uri=" + Arrays.toString(uri) + + ", discovery=" + discovery + ", failFast=" + failFast + ", token=" + + token + ", requestReadTimeout=" + requestReadTimeout + ", sendState=" + + sendState + ", headers=" + headers + "]"; } + + } diff --git a/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServicePropertySourceLocator.java b/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServicePropertySourceLocator.java index 5ef23118..176390bb 100644 --- a/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServicePropertySourceLocator.java +++ b/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServicePropertySourceLocator.java @@ -53,6 +53,7 @@ import org.springframework.web.client.RestTemplate; import static org.springframework.cloud.config.client.ConfigClientProperties.STATE_HEADER; import static org.springframework.cloud.config.client.ConfigClientProperties.TOKEN_HEADER; +import static org.springframework.cloud.config.client.ConfigClientProperties.AUTHORIZATION; /** * @author Dave Syer @@ -209,7 +210,7 @@ public class ConfigServicePropertySourceLocator implements PropertySourceLocator if (StringUtils.hasText(token)) { headers.add(TOKEN_HEADER, token); } - if (StringUtils.hasText(state)) { // TODO: opt in to sending state? + if (StringUtils.hasText(state) && properties.isSendState()) { headers.add(STATE_HEADER, state); } @@ -248,11 +249,15 @@ public class ConfigServicePropertySourceLocator implements PropertySourceLocator private RestTemplate getSecureRestTemplate(ConfigClientProperties client) { SimpleClientHttpRequestFactory requestFactory = new SimpleClientHttpRequestFactory(); - requestFactory.setReadTimeout((60 * 1000 * 3) + 5000); // TODO 3m5s, make - // configurable? + if (client.getRequestReadTimeout() < 0) { + throw new IllegalStateException("Invalid Value for Read Timeout set."); + } + requestFactory.setReadTimeout(client.getRequestReadTimeout()); RestTemplate template = new RestTemplate(requestFactory); Map headers = new HashMap<>(client.getHeaders()); - + if (headers.containsKey(AUTHORIZATION)) { + headers.remove(AUTHORIZATION); // To avoid redundant addition of header + } if (!headers.isEmpty()) { template.setInterceptors(Arrays. asList( new GenericRequestHeaderInterceptor(headers))); @@ -263,7 +268,7 @@ public class ConfigServicePropertySourceLocator implements PropertySourceLocator private void addAuthorizationToken(ConfigClientProperties configClientProperties, HttpHeaders httpHeaders, String username, String password) { - String authorization = configClientProperties.getAuthorization(); + String authorization = configClientProperties.getHeaders().get(AUTHORIZATION); if (password != null && authorization != null) { throw new IllegalStateException( @@ -297,5 +302,10 @@ public class ConfigServicePropertySourceLocator implements PropertySourceLocator } return execution.execute(request, body); } + + protected Map getHeaders() { + return headers; + } + } } diff --git a/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/ConfigServicePropertySourceLocatorTests.java b/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/ConfigServicePropertySourceLocatorTests.java index fd76b31b..c5e6021f 100644 --- a/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/ConfigServicePropertySourceLocatorTests.java +++ b/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/ConfigServicePropertySourceLocatorTests.java @@ -1,12 +1,14 @@ package org.springframework.cloud.config.client; import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import java.io.ByteArrayInputStream; import java.net.URI; import java.util.HashMap; +import java.util.Iterator; import java.util.Map; import org.hamcrest.core.IsInstanceOf; @@ -17,6 +19,7 @@ import org.junit.rules.ExpectedException; import org.mockito.Matchers; import org.mockito.Mockito; import org.springframework.boot.test.util.EnvironmentTestUtils; +import org.springframework.cloud.config.client.ConfigServicePropertySourceLocator.GenericRequestHeaderInterceptor; import org.springframework.cloud.config.environment.Environment; import org.springframework.core.env.ConfigurableEnvironment; import org.springframework.core.env.StandardEnvironment; @@ -29,12 +32,16 @@ import org.springframework.http.ResponseEntity; import org.springframework.http.client.ClientHttpRequest; import org.springframework.http.client.ClientHttpRequestExecution; import org.springframework.http.client.ClientHttpRequestFactory; +import org.springframework.http.client.ClientHttpRequestInterceptor; import org.springframework.http.client.ClientHttpResponse; +import org.springframework.http.client.SimpleClientHttpRequestFactory; import org.springframework.mock.http.client.MockClientHttpRequest; import org.springframework.test.util.ReflectionTestUtils; import org.springframework.web.client.HttpServerErrorException; import org.springframework.web.client.RestTemplate; +import static org.springframework.cloud.config.client.ConfigClientProperties.AUTHORIZATION; + public class ConfigServicePropertySourceLocatorTests { @Rule @@ -156,11 +163,10 @@ public class ConfigServicePropertySourceLocatorTests { Mockito.when(requestFactory.createRequest(Mockito.any(URI.class), Mockito.any(HttpMethod.class))).thenReturn(request); ConfigClientProperties defaults = new ConfigClientProperties(this.environment); - // defaults.setUri(new String[] {"http://localhost"); defaults.setFailFast(true); defaults.setUsername("username"); defaults.setPassword("password"); - defaults.setAuthorization("Basic dXNlcm5hbWU6cGFzc3dvcmQNCg=="); + defaults.getHeaders().put(AUTHORIZATION, "Basic dXNlcm5hbWU6cGFzc3dvcmQNCg=="); this.locator = new ConfigServicePropertySourceLocator(defaults); this.expected.expect(IllegalStateException.class); this.expected.expectMessage( @@ -198,7 +204,7 @@ public class ConfigServicePropertySourceLocatorTests { public void shouldAddAuthorizationHeaderWhenAuthorizationSet() { HttpHeaders headers = new HttpHeaders(); ConfigClientProperties defaults = new ConfigClientProperties(this.environment); - defaults.setAuthorization("1234abcd"); + defaults.getHeaders().put(AUTHORIZATION, "Basic dXNlcm5hbWU6cGFzc3dvcmQNCg=="); this.locator = new ConfigServicePropertySourceLocator(defaults); String username = "user"; String password = null; @@ -211,7 +217,7 @@ public class ConfigServicePropertySourceLocatorTests { public void shouldThrowExceptionWhenPasswordAndAuthorizationBothSet() { HttpHeaders headers = new HttpHeaders(); ConfigClientProperties defaults = new ConfigClientProperties(this.environment); - defaults.setAuthorization("1234abcd"); + defaults.getHeaders().put(AUTHORIZATION, "Basic dXNlcm5hbWU6cGFzc3dvcmQNCg=="); this.locator = new ConfigServicePropertySourceLocator(defaults); String username = "user"; String password = "pass"; @@ -221,6 +227,34 @@ public class ConfigServicePropertySourceLocatorTests { headers, username, password); } + @Test + public void shouldThrowExceptionWhenNegativeReadTimeoutSet() { + ConfigClientProperties defaults = new ConfigClientProperties(this.environment); + defaults.setRequestReadTimeout(-1); + this.locator = new ConfigServicePropertySourceLocator(defaults); + this.expected.expect(IllegalStateException.class); + this.expected.expectMessage("Invalid Value for Read Timeout set."); + ReflectionTestUtils.invokeMethod(this.locator, "getSecureRestTemplate", defaults); + } + + @Test + public void checkInterceptorHasNoAuthorizationHeaderPresent() { + ConfigClientProperties defaults = new ConfigClientProperties(this.environment); + defaults.getHeaders().put(AUTHORIZATION, "Basic dXNlcm5hbWU6cGFzc3dvcmQNCg=="); + defaults.getHeaders().put("key", "value"); + this.locator = new ConfigServicePropertySourceLocator(defaults); + RestTemplate restTemplate = ReflectionTestUtils.invokeMethod(this.locator, + "getSecureRestTemplate", defaults); + Iterator iterator = restTemplate.getInterceptors() + .iterator(); + while (iterator.hasNext()) { + GenericRequestHeaderInterceptor genericRequestHeaderInterceptor = (GenericRequestHeaderInterceptor) iterator + .next(); + assertEquals(null, + genericRequestHeaderInterceptor.getHeaders().get(AUTHORIZATION)); + } + } + @SuppressWarnings("unchecked") private void mockRequestResponseWithLabel(ResponseEntity response, String label) { Mockito.when(this.restTemplate.exchange(Mockito.any(String.class), @@ -245,4 +279,5 @@ public class ConfigServicePropertySourceLocatorTests { Mockito.any(Class.class), Matchers.eq(expectedName), Matchers.anyString())).thenReturn(response); } + }