From 8c6740ab71091d3632e60b6dbdd9bf8c20bdc034 Mon Sep 17 00:00:00 2001 From: Dave Syer Date: Wed, 21 Jan 2015 14:40:39 +0000 Subject: [PATCH] Use only ConfigFileApplicationListener in server mini-application In the server we use a SpringApplicationEnvironmentRepository to load the YAML and properties files from git (or locally). It creates a mini SpringApplication so as to faithfully replicate the way the Environment is created. Unfortunately that can have side effects on the server application itself (e.g. setting log levels). In particular if the mini SpringApplication fails to start then the log levels could be left in a "preInitialized" state with all log levels OFF by default. This change ensures that the server logs all errors when loading YAML and properties files, and also that the client logs the error response if it is JSON (as it should be). Fixes gh-66, fixes gh-67 --- .../ConfigServicePropertySourceLocator.java | 26 ++++-- .../config/BootstrapConfigurationTests.java | 27 ++++++ ...nfigServicePropertySourceLocatorTests.java | 92 +++++++++++++++++++ .../test/java/sample/ApplicationTests.java | 11 ++- .../sample/ServerNativeApplicationTests.java | 73 +++++++++++++++ .../src/test/resources/bad.yml | 4 + .../config/server/EnvironmentController.java | 4 +- ...pringApplicationEnvironmentRepository.java | 7 +- .../NativeConfigServerIntegrationTests.java | 58 ++++++++++++ .../VanillaConfigServerIntegrationTests.java | 12 ++- .../src/test/resources/bad.yml | 4 + 11 files changed, 300 insertions(+), 18 deletions(-) create mode 100644 spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/ConfigServicePropertySourceLocatorTests.java create mode 100644 spring-cloud-config-sample/src/test/java/sample/ServerNativeApplicationTests.java create mode 100644 spring-cloud-config-sample/src/test/resources/bad.yml create mode 100644 spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/NativeConfigServerIntegrationTests.java create mode 100644 spring-cloud-config-server/src/test/resources/bad.yml 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 94851342..9dc0bbbd 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 @@ -30,10 +30,12 @@ import org.springframework.core.env.MapPropertySource; import org.springframework.http.HttpEntity; import org.springframework.http.HttpMethod; import org.springframework.http.HttpRequest; +import org.springframework.http.MediaType; import org.springframework.http.client.ClientHttpRequestExecution; import org.springframework.http.client.ClientHttpRequestInterceptor; import org.springframework.http.client.ClientHttpResponse; import org.springframework.security.crypto.codec.Base64; +import org.springframework.web.client.HttpServerErrorException; import org.springframework.web.client.RestTemplate; /** @@ -60,6 +62,8 @@ public class ConfigServicePropertySourceLocator implements PropertySourceLocator CompositePropertySource composite = new CompositePropertySource("configService"); RestTemplate restTemplate = this.restTemplate == null ? getSecureRestTemplate(client) : this.restTemplate; + RuntimeException error = null; + String errorBody = null; try { Environment result = restTemplate.exchange( client.getUri() + "/{name}/{profile}/{label}", HttpMethod.GET, @@ -72,15 +76,23 @@ public class ConfigServicePropertySourceLocator implements PropertySourceLocator } return composite; } - catch (Exception e) { - if (client != null && client.isFailFast()) { - throw new IllegalStateException( - "Could not locate PropertySource. The fail fast property is set, failing", - e); + catch (HttpServerErrorException e) { + error = e; + if (MediaType.APPLICATION_JSON.includes(e.getResponseHeaders().getContentType())) { + errorBody = e.getResponseBodyAsString(); } - logger.error("Could not locate PropertySource: " + e.getMessage()); - return null; } + catch (Exception e) { + error = new IllegalStateException( + "Could not locate PropertySource. The fail fast property is set, failing", + e); + } + if (client != null && client.isFailFast()) { + throw error; + } + logger.error("Could not locate PropertySource: " + + (errorBody == null ? error.getMessage() : errorBody)); + return null; } diff --git a/spring-cloud-config-client/src/test/java/org/springframework/cloud/bootstrap/config/BootstrapConfigurationTests.java b/spring-cloud-config-client/src/test/java/org/springframework/cloud/bootstrap/config/BootstrapConfigurationTests.java index 796e9cd3..49e86987 100644 --- a/spring-cloud-config-client/src/test/java/org/springframework/cloud/bootstrap/config/BootstrapConfigurationTests.java +++ b/spring-cloud-config-client/src/test/java/org/springframework/cloud/bootstrap/config/BootstrapConfigurationTests.java @@ -28,7 +28,9 @@ import java.util.HashMap; import java.util.Map; import org.junit.After; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import org.springframework.boot.builder.SpringApplicationBuilder; import org.springframework.boot.context.properties.ConfigurationProperties; import org.springframework.boot.context.properties.EnableConfigurationProperties; @@ -50,11 +52,15 @@ import org.springframework.core.env.StandardEnvironment; public class BootstrapConfigurationTests { private ConfigurableApplicationContext context; + + @Rule + public ExpectedException expected = ExpectedException.none(); @After public void close() { // Expected.* is bound to the PropertySourceConfiguration below System.clearProperty("expected.name"); + System.clearProperty("expected.fail"); // Used to test system properties override System.clearProperty("bootstrap.foo"); PropertySourceConfiguration.MAP.clear(); @@ -109,6 +115,14 @@ public class BootstrapConfigurationTests { assertNotNull(context.getBean(ConfigClientProperties.class)); } + @Test + public void failsOnPropertySource() { + System.setProperty("expected.fail", "true"); + expected.expectMessage("Planned"); + context = new SpringApplicationBuilder().web(false) + .sources(BareConfiguration.class).run(); + } + @Test public void overrideSystemPropertySourceByDefault() { PropertySourceConfiguration.MAP.put("bootstrap.foo", "bar"); @@ -272,12 +286,17 @@ public class BootstrapConfigurationTests { Collections. singletonMap("bootstrap.foo", "bar")); private String name; + + private boolean fail = false; @Override public PropertySource locate(Environment environment) { if (name != null) { assertEquals(name, environment.getProperty("spring.application.name")); } + if (fail) { + throw new RuntimeException("Planned"); + } return new MapPropertySource("testBootstrap", MAP); } @@ -288,6 +307,14 @@ public class BootstrapConfigurationTests { public void setName(String name) { this.name = name; } + + public boolean isFail() { + return fail; + } + + public void setFail(boolean fail) { + this.fail = fail; + } } } 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 new file mode 100644 index 00000000..5c8c3990 --- /dev/null +++ b/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/ConfigServicePropertySourceLocatorTests.java @@ -0,0 +1,92 @@ +package org.springframework.cloud.config.client; + +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; + +import java.io.ByteArrayInputStream; +import java.net.URI; + +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; +import org.mockito.Matchers; +import org.mockito.Mockito; +import org.springframework.cloud.config.Environment; +import org.springframework.core.env.ConfigurableEnvironment; +import org.springframework.core.env.StandardEnvironment; +import org.springframework.http.HttpEntity; +import org.springframework.http.HttpHeaders; +import org.springframework.http.HttpMethod; +import org.springframework.http.HttpStatus; +import org.springframework.http.MediaType; +import org.springframework.http.ResponseEntity; +import org.springframework.http.client.ClientHttpRequest; +import org.springframework.http.client.ClientHttpRequestFactory; +import org.springframework.http.client.ClientHttpResponse; +import org.springframework.web.client.HttpServerErrorException; +import org.springframework.web.client.RestTemplate; + +public class ConfigServicePropertySourceLocatorTests { + + @Rule + public ExpectedException expected = ExpectedException.none(); + + private ConfigurableEnvironment environment = new StandardEnvironment(); + + private ConfigServicePropertySourceLocator locator = new ConfigServicePropertySourceLocator( + new ConfigClientProperties(environment)); + + private RestTemplate restTemplate = Mockito.mock(RestTemplate.class); + + @Test + public void sunnyDay() { + Environment body = new Environment("app", "master"); + mockRequestResponse(new ResponseEntity(body, HttpStatus.OK)); + locator.setRestTemplate(restTemplate); + assertNotNull(locator.locate(environment)); + } + + @Test + public void failsQuietly() { + mockRequestResponse(new ResponseEntity("Wah!", + HttpStatus.INTERNAL_SERVER_ERROR)); + locator.setRestTemplate(restTemplate); + assertNull(locator.locate(environment)); + } + + @Test + public void failFast() throws Exception { + ClientHttpRequestFactory requestFactory = Mockito + .mock(ClientHttpRequestFactory.class); + ClientHttpRequest request = Mockito.mock(ClientHttpRequest.class); + ClientHttpResponse response = Mockito.mock(ClientHttpResponse.class); + Mockito.when( + requestFactory.createRequest(Mockito.any(URI.class), + Mockito.any(HttpMethod.class))).thenReturn(request); + RestTemplate restTemplate = new RestTemplate(requestFactory); + ConfigClientProperties defaults = new ConfigClientProperties(environment); + defaults.setFailFast(true); + locator = new ConfigServicePropertySourceLocator(defaults); + Mockito.when(request.getHeaders()).thenReturn(new HttpHeaders()); + Mockito.when(request.execute()).thenReturn(response); + HttpHeaders headers = new HttpHeaders(); + headers.setContentType(MediaType.APPLICATION_JSON); + Mockito.when(response.getHeaders()).thenReturn(headers); + Mockito.when(response.getStatusCode()).thenReturn(HttpStatus.INTERNAL_SERVER_ERROR); + Mockito.when(response.getBody()).thenReturn(new ByteArrayInputStream("{}".getBytes())); + locator.setRestTemplate(restTemplate); + expected.expect(HttpServerErrorException.class); + expected.expectMessage("500"); + assertNull(locator.locate(environment)); + } + + @SuppressWarnings("unchecked") + private void mockRequestResponse(ResponseEntity response) { + Mockito.when( + restTemplate.exchange(Mockito.any(String.class), + Mockito.any(HttpMethod.class), Mockito.any(HttpEntity.class), + Mockito.any(Class.class), Matchers.anyString(), + Matchers.anyString(), Matchers.anyString())).thenReturn(response); + } + +} diff --git a/spring-cloud-config-sample/src/test/java/sample/ApplicationTests.java b/spring-cloud-config-sample/src/test/java/sample/ApplicationTests.java index 6b85361d..be9e4fbd 100644 --- a/spring-cloud-config-sample/src/test/java/sample/ApplicationTests.java +++ b/spring-cloud-config-sample/src/test/java/sample/ApplicationTests.java @@ -30,21 +30,26 @@ public class ApplicationTests { @Value("${local.server.port}") private int port; + private static ConfigurableApplicationContext server; + @BeforeClass public static void startConfigServer() throws IOException { String repo = ConfigServerTestUtils.prepareLocalRepo(); - ConfigurableApplicationContext context = SpringApplication.run( + server = SpringApplication.run( org.springframework.cloud.config.server.ConfigServerApplication.class, "--server.port=" + configPort, "--spring.config.name=server", "--spring.cloud.config.server.git.uri=" + repo); - configPort = ((EmbeddedWebApplicationContext) context) + configPort = ((EmbeddedWebApplicationContext) server) .getEmbeddedServletContainer().getPort(); System.setProperty("config.port", "" + configPort); } @AfterClass public static void close() { - System.clearProperty("config.port"); + System.clearProperty("config.port"); + if (server!=null) { + server.close(); + } } @Test diff --git a/spring-cloud-config-sample/src/test/java/sample/ServerNativeApplicationTests.java b/spring-cloud-config-sample/src/test/java/sample/ServerNativeApplicationTests.java new file mode 100644 index 00000000..507fee6c --- /dev/null +++ b/spring-cloud-config-sample/src/test/java/sample/ServerNativeApplicationTests.java @@ -0,0 +1,73 @@ +package sample; + +import static org.junit.Assert.assertTrue; + +import java.io.IOException; +import java.util.Map; + +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.boot.SpringApplication; +import org.springframework.boot.context.embedded.EmbeddedWebApplicationContext; +import org.springframework.boot.test.IntegrationTest; +import org.springframework.boot.test.SpringApplicationConfiguration; +import org.springframework.cloud.config.server.ConfigServerTestUtils; +import org.springframework.context.ConfigurableApplicationContext; +import org.springframework.core.env.ConfigurableEnvironment; +import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; +import org.springframework.test.context.web.WebAppConfiguration; + +@RunWith(SpringJUnit4ClassRunner.class) +@SpringApplicationConfiguration(classes = Application.class) +@IntegrationTest({"server.port:0", "spring.application.name:bad"}) +@WebAppConfiguration +public class ServerNativeApplicationTests { + + private static int configPort = 0; + + @Autowired + private ConfigurableEnvironment environment; + + @Value("${local.server.port}") + private int port; + + private static ConfigurableApplicationContext server; + + @BeforeClass + public static void startConfigServer() throws IOException { + String repo = ConfigServerTestUtils.prepareLocalRepo(); + server = SpringApplication.run( + org.springframework.cloud.config.server.ConfigServerApplication.class, + "--server.port=" + configPort, "--spring.config.name=server", + "--spring.cloud.config.server.git.uri=" + repo, "--spring.profiles.active=native"); + configPort = ((EmbeddedWebApplicationContext) server) + .getEmbeddedServletContainer().getPort(); + System.setProperty("config.port", "" + configPort); + } + + @AfterClass + public static void close() { + System.clearProperty("config.port"); + if (server!=null) { + server.close(); + } + } + + @SuppressWarnings("rawtypes") + @Test + public void contextLoads() { + // The remote config was bad so there is no bootstrap + assertTrue(((Map)environment.getPropertySources().get("bootstrap").getSource()).isEmpty()); + } + + public static void main(String[] args) throws IOException { + configPort = 8888; + startConfigServer(); + SpringApplication.run(Application.class, args); + } + +} diff --git a/spring-cloud-config-sample/src/test/resources/bad.yml b/spring-cloud-config-sample/src/test/resources/bad.yml new file mode 100644 index 00000000..fdd9e04d --- /dev/null +++ b/spring-cloud-config-sample/src/test/resources/bad.yml @@ -0,0 +1,4 @@ +foo: +# whitespace error! + bar: spam + bucket: wham \ No newline at end of file diff --git a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/EnvironmentController.java b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/EnvironmentController.java index 6dae5c09..31172579 100644 --- a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/EnvironmentController.java +++ b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/EnvironmentController.java @@ -29,7 +29,7 @@ import org.yaml.snakeyaml.Yaml; @RestController @RequestMapping("${spring.cloud.config.server.prefix:}") public class EnvironmentController { - + private EnvironmentRepository repository; private EncryptionController encryption; @@ -61,7 +61,7 @@ public class EnvironmentController { } return environment; } - + @RequestMapping("/{name}-{profiles}.properties") public ResponseEntity properties(@PathVariable String name, @PathVariable String profiles) throws IOException { diff --git a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/SpringApplicationEnvironmentRepository.java b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/SpringApplicationEnvironmentRepository.java index b74f4a03..21146a92 100644 --- a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/SpringApplicationEnvironmentRepository.java +++ b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/SpringApplicationEnvironmentRepository.java @@ -22,6 +22,7 @@ import java.util.List; import org.springframework.boot.autoconfigure.PropertyPlaceholderAutoConfiguration; import org.springframework.boot.builder.SpringApplicationBuilder; +import org.springframework.boot.context.config.ConfigFileApplicationListener; import org.springframework.boot.context.properties.ConfigurationProperties; import org.springframework.cloud.config.Environment; import org.springframework.context.ConfigurableApplicationContext; @@ -66,6 +67,10 @@ public class SpringApplicationEnvironmentRepository implements EnvironmentReposi builder.environment(environment); builder.web(false).showBanner(false); String[] args = getArgs(config, label); + // Explicitly set the listeners (to exclude logging listener which would change log + // levels in the caller) + builder.application().setListeners( + Collections.singletonList(new ConfigFileApplicationListener())); ConfigurableApplicationContext context = builder.run(args); environment.getPropertySources().remove("profiles"); try { @@ -121,7 +126,7 @@ public class SpringApplicationEnvironmentRepository implements EnvironmentReposi this.locations = locations; for (int i = 0; i < locations.length; i++) { String location = locations[i]; - if (isDirectory(location)&& !location.endsWith("/")) { + if (isDirectory(location) && !location.endsWith("/")) { location = location + "/"; } locations[i] = location; diff --git a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/NativeConfigServerIntegrationTests.java b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/NativeConfigServerIntegrationTests.java new file mode 100644 index 00000000..b1682b3b --- /dev/null +++ b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/NativeConfigServerIntegrationTests.java @@ -0,0 +1,58 @@ +package org.springframework.cloud.config.server; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; + +import java.io.IOException; + +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.boot.builder.SpringApplicationBuilder; +import org.springframework.boot.test.IntegrationTest; +import org.springframework.boot.test.SpringApplicationConfiguration; +import org.springframework.boot.test.TestRestTemplate; +import org.springframework.cloud.config.Environment; +import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseEntity; +import org.springframework.test.context.ActiveProfiles; +import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; +import org.springframework.test.context.web.WebAppConfiguration; + +@RunWith(SpringJUnit4ClassRunner.class) +@SpringApplicationConfiguration(classes = ConfigServerApplication.class) +@IntegrationTest({ "server.port:0", "spring.config.name:configserver" }) +@WebAppConfiguration +@ActiveProfiles({ "test", "native" }) +public class NativeConfigServerIntegrationTests { + + @Value("${local.server.port}") + private int port; + + @BeforeClass + public static void init() throws IOException{ + ConfigServerTestUtils.prepareLocalRepo(); + } + + @Test + public void contextLoads() { + Environment environment = new TestRestTemplate().getForObject("http://localhost:" + port + "/foo/development/", Environment.class); + assertFalse(environment.getPropertySources().isEmpty()); + assertEquals("overrides", environment.getPropertySources().get(0).getName()); + assertEquals("{spring.cloud.config.enabled=true}", environment.getPropertySources().get(0).getSource().toString()); + } + + @Test + public void badYaml() { + ResponseEntity response = new TestRestTemplate().getForEntity("http://localhost:" + + port + "/bad/default/", String.class); + assertEquals(HttpStatus.INTERNAL_SERVER_ERROR, response.getStatusCode()); + } + + public static void main(String[] args) { + new SpringApplicationBuilder(ConfigServerApplication.class).profiles("native").properties( + "spring.config.name=configserver").run(args); + } + +} diff --git a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/VanillaConfigServerIntegrationTests.java b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/VanillaConfigServerIntegrationTests.java index 8463b9f3..b9c3e09c 100644 --- a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/VanillaConfigServerIntegrationTests.java +++ b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/VanillaConfigServerIntegrationTests.java @@ -23,21 +23,23 @@ import org.springframework.test.context.web.WebAppConfiguration; @WebAppConfiguration @ActiveProfiles("test") public class VanillaConfigServerIntegrationTests { - + @Value("${local.server.port}") private int port; - + @BeforeClass - public static void init() throws IOException{ + public static void init() throws IOException { ConfigServerTestUtils.prepareLocalRepo(); } @Test public void contextLoads() { - Environment environment = new TestRestTemplate().getForObject("http://localhost:" + port + "/foo/development/", Environment.class); + Environment environment = new TestRestTemplate().getForObject("http://localhost:" + + port + "/foo/development/", Environment.class); assertFalse(environment.getPropertySources().isEmpty()); assertEquals("overrides", environment.getPropertySources().get(0).getName()); - assertEquals("{spring.cloud.config.enabled=true}", environment.getPropertySources().get(0).getSource().toString()); + assertEquals("{spring.cloud.config.enabled=true}", environment + .getPropertySources().get(0).getSource().toString()); } } diff --git a/spring-cloud-config-server/src/test/resources/bad.yml b/spring-cloud-config-server/src/test/resources/bad.yml new file mode 100644 index 00000000..fdd9e04d --- /dev/null +++ b/spring-cloud-config-server/src/test/resources/bad.yml @@ -0,0 +1,4 @@ +foo: +# whitespace error! + bar: spam + bucket: wham \ No newline at end of file