From 773dda3d5559daf01032d212b3326d12752962f6 Mon Sep 17 00:00:00 2001 From: Madhura Bhave Date: Mon, 8 Jul 2019 15:32:19 -0700 Subject: [PATCH] Log file location should be evaluated just once Using a random value for the logfile name caused the logfile endpoint to return a 404 as the name was resolved from the environment on every request. This commit registers a bean for LogFile which is then used by the logfile endpoint. Fixes gh-17434 --- .../LogFileWebEndpointAutoConfiguration.java | 6 ++-- .../LogFileWebEndpointDocumentationTests.java | 3 +- .../actuate/logging/LogFileWebEndpoint.java | 18 ++++------ .../logging/LogFileWebEndpointTests.java | 14 ++++---- ...LogFileWebEndpointWebIntegrationTests.java | 35 +++++++------------ .../logging/LoggingApplicationListener.java | 18 +++++++--- ...pertiesSampleActuatorApplicationTests.java | 7 ++++ .../application-endpoints.properties | 2 ++ 8 files changed, 55 insertions(+), 48 deletions(-) diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/logging/LogFileWebEndpointAutoConfiguration.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/logging/LogFileWebEndpointAutoConfiguration.java index d4860a4f97e..31d0a7dea46 100644 --- a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/logging/LogFileWebEndpointAutoConfiguration.java +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/logging/LogFileWebEndpointAutoConfiguration.java @@ -16,6 +16,7 @@ package org.springframework.boot.actuate.autoconfigure.logging; +import org.springframework.beans.factory.ObjectProvider; import org.springframework.boot.actuate.autoconfigure.endpoint.condition.ConditionalOnEnabledEndpoint; import org.springframework.boot.actuate.logging.LogFileWebEndpoint; import org.springframework.boot.autoconfigure.EnableAutoConfiguration; @@ -24,6 +25,7 @@ import org.springframework.boot.autoconfigure.condition.ConditionOutcome; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; import org.springframework.boot.autoconfigure.condition.SpringBootCondition; import org.springframework.boot.context.properties.EnableConfigurationProperties; +import org.springframework.boot.logging.LogFile; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.ConditionContext; import org.springframework.context.annotation.Conditional; @@ -52,8 +54,8 @@ public class LogFileWebEndpointAutoConfiguration { @Bean @ConditionalOnMissingBean @Conditional(LogFileCondition.class) - public LogFileWebEndpoint logFileWebEndpoint(Environment environment) { - return new LogFileWebEndpoint(environment, this.properties.getExternalFile()); + public LogFileWebEndpoint logFileWebEndpoint(ObjectProvider logFile) { + return new LogFileWebEndpoint(logFile.getIfAvailable(), this.properties.getExternalFile()); } private static class LogFileCondition extends SpringBootCondition { diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/endpoint/web/documentation/LogFileWebEndpointDocumentationTests.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/endpoint/web/documentation/LogFileWebEndpointDocumentationTests.java index 78442345cb4..5971135cb15 100644 --- a/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/endpoint/web/documentation/LogFileWebEndpointDocumentationTests.java +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/endpoint/web/documentation/LogFileWebEndpointDocumentationTests.java @@ -19,6 +19,7 @@ package org.springframework.boot.actuate.autoconfigure.endpoint.web.documentatio import org.junit.Test; import org.springframework.boot.actuate.logging.LogFileWebEndpoint; +import org.springframework.boot.logging.LogFile; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Import; @@ -56,7 +57,7 @@ public class LogFileWebEndpointDocumentationTests extends MockMvcEndpointDocumen @Bean public LogFileWebEndpoint endpoint(Environment environment) { - return new LogFileWebEndpoint(environment); + return new LogFileWebEndpoint(LogFile.get(environment), null); } } diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/logging/LogFileWebEndpoint.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/logging/LogFileWebEndpoint.java index 1b897a8a157..8c4eb5e4107 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/logging/LogFileWebEndpoint.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/logging/LogFileWebEndpoint.java @@ -25,7 +25,6 @@ import org.springframework.boot.actuate.endpoint.annotation.Endpoint; import org.springframework.boot.actuate.endpoint.annotation.ReadOperation; import org.springframework.boot.actuate.endpoint.web.annotation.WebEndpoint; import org.springframework.boot.logging.LogFile; -import org.springframework.core.env.Environment; import org.springframework.core.io.FileSystemResource; import org.springframework.core.io.Resource; @@ -42,17 +41,13 @@ public class LogFileWebEndpoint { private static final Log logger = LogFactory.getLog(LogFileWebEndpoint.class); - private final Environment environment; - private File externalFile; - public LogFileWebEndpoint(Environment environment, File externalFile) { - this.environment = environment; - this.externalFile = externalFile; - } + private final LogFile logFile; - public LogFileWebEndpoint(Environment environment) { - this(environment, null); + public LogFileWebEndpoint(LogFile logFile, File externalFile) { + this.externalFile = externalFile; + this.logFile = logFile; } @ReadOperation(produces = "text/plain; charset=UTF-8") @@ -68,12 +63,11 @@ public class LogFileWebEndpoint { if (this.externalFile != null) { return new FileSystemResource(this.externalFile); } - LogFile logFile = LogFile.get(this.environment); - if (logFile == null) { + if (this.logFile == null) { logger.debug("Missing 'logging.file' or 'logging.path' properties"); return null; } - return new FileSystemResource(logFile.toString()); + return new FileSystemResource(this.logFile.toString()); } } diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/logging/LogFileWebEndpointTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/logging/LogFileWebEndpointTests.java index d5346493ebb..9cf7cfeba13 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/logging/LogFileWebEndpointTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/logging/LogFileWebEndpointTests.java @@ -25,6 +25,7 @@ import org.junit.Rule; import org.junit.Test; import org.junit.rules.TemporaryFolder; +import org.springframework.boot.logging.LogFile; import org.springframework.core.io.Resource; import org.springframework.mock.env.MockEnvironment; import org.springframework.util.FileCopyUtils; @@ -46,8 +47,6 @@ public class LogFileWebEndpointTests { private final MockEnvironment environment = new MockEnvironment(); - private final LogFileWebEndpoint endpoint = new LogFileWebEndpoint(this.environment); - private File logFile; @Before @@ -58,26 +57,29 @@ public class LogFileWebEndpointTests { @Test public void nullResponseWithoutLogFile() { - assertThat(this.endpoint.logFile()).isNull(); + LogFileWebEndpoint endpoint = new LogFileWebEndpoint(null, null); + assertThat(endpoint.logFile()).isNull(); } @Test public void nullResponseWithMissingLogFile() { this.environment.setProperty("logging.file", "no_test.log"); - assertThat(this.endpoint.logFile()).isNull(); + LogFileWebEndpoint endpoint = new LogFileWebEndpoint(LogFile.get(this.environment), null); + assertThat(endpoint.logFile()).isNull(); } @Test public void resourceResponseWithLogFile() throws Exception { this.environment.setProperty("logging.file", this.logFile.getAbsolutePath()); - Resource resource = this.endpoint.logFile(); + LogFileWebEndpoint endpoint = new LogFileWebEndpoint(LogFile.get(this.environment), null); + Resource resource = endpoint.logFile(); assertThat(resource).isNotNull(); assertThat(StreamUtils.copyToString(resource.getInputStream(), StandardCharsets.UTF_8)).isEqualTo("--TEST--"); } @Test public void resourceResponseWithExternalLogFile() throws Exception { - LogFileWebEndpoint endpoint = new LogFileWebEndpoint(this.environment, this.logFile); + LogFileWebEndpoint endpoint = new LogFileWebEndpoint(null, this.logFile); Resource resource = endpoint.logFile(); assertThat(resource).isNotNull(); assertThat(StreamUtils.copyToString(resource.getInputStream(), StandardCharsets.UTF_8)).isEqualTo("--TEST--"); diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/logging/LogFileWebEndpointWebIntegrationTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/logging/LogFileWebEndpointWebIntegrationTests.java index 220ab3ffbcc..22dd017cedc 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/logging/LogFileWebEndpointWebIntegrationTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/logging/LogFileWebEndpointWebIntegrationTests.java @@ -17,21 +17,19 @@ package org.springframework.boot.actuate.logging; import java.io.File; -import java.io.IOException; -import org.junit.Before; -import org.junit.Rule; +import org.junit.ClassRule; import org.junit.Test; import org.junit.rules.TemporaryFolder; import org.junit.runner.RunWith; import org.springframework.boot.actuate.endpoint.web.test.WebEndpointRunners; -import org.springframework.boot.test.util.TestPropertyValues; +import org.springframework.boot.logging.LogFile; import org.springframework.context.ConfigurableApplicationContext; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; -import org.springframework.core.env.Environment; import org.springframework.http.MediaType; +import org.springframework.mock.env.MockEnvironment; import org.springframework.test.web.reactive.server.WebTestClient; import org.springframework.util.FileCopyUtils; @@ -48,32 +46,19 @@ public class LogFileWebEndpointWebIntegrationTests { private static WebTestClient client; - @Rule - public final TemporaryFolder temp = new TemporaryFolder(); + @ClassRule + public static final TemporaryFolder temp = new TemporaryFolder(); - private File logFile; - - @Before - public void setUp() throws IOException { - this.logFile = this.temp.newFile(); - FileCopyUtils.copy("--TEST--".getBytes(), this.logFile); - } - - @Test - public void getRequestProduces404ResponseWhenLogFileNotFound() { - client.get().uri("/actuator/logfile").exchange().expectStatus().isNotFound(); - } + private static File logFile; @Test public void getRequestProducesResponseWithLogFile() { - TestPropertyValues.of("logging.file:" + this.logFile.getAbsolutePath()).applyTo(context); client.get().uri("/actuator/logfile").exchange().expectStatus().isOk().expectHeader() .contentType("text/plain; charset=UTF-8").expectBody(String.class).isEqualTo("--TEST--"); } @Test public void getRequestThatAcceptsTextPlainProducesResponseWithLogFile() { - TestPropertyValues.of("logging.file:" + this.logFile.getAbsolutePath()).applyTo(context); client.get().uri("/actuator/logfile").accept(MediaType.TEXT_PLAIN).exchange().expectStatus().isOk() .expectHeader().contentType("text/plain; charset=UTF-8").expectBody(String.class).isEqualTo("--TEST--"); } @@ -82,8 +67,12 @@ public class LogFileWebEndpointWebIntegrationTests { static class TestConfiguration { @Bean - public LogFileWebEndpoint logFileEndpoint(Environment environment) { - return new LogFileWebEndpoint(environment); + public LogFileWebEndpoint logFileEndpoint() throws Exception { + logFile = temp.newFile(); + FileCopyUtils.copy("--TEST--".getBytes(), logFile); + MockEnvironment environment = new MockEnvironment(); + environment.setProperty("logging.file", logFile.getAbsolutePath()); + return new LogFileWebEndpoint(LogFile.get(environment), null); } } diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/logging/LoggingApplicationListener.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/logging/LoggingApplicationListener.java index e91d8ee2839..1bd01024018 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/logging/LoggingApplicationListener.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/logging/LoggingApplicationListener.java @@ -121,6 +121,11 @@ public class LoggingApplicationListener implements GenericApplicationListener { */ public static final String LOGGING_SYSTEM_BEAN_NAME = "springBootLoggingSystem"; + /** + * The name of the {@link LogFile} bean. + */ + public static final String LOGFILE_BEAN_NAME = "springBootLogFile"; + private static final Map> DEFAULT_GROUP_LOGGERS; static { MultiValueMap loggers = new LinkedMultiValueMap<>(); @@ -160,6 +165,8 @@ public class LoggingApplicationListener implements GenericApplicationListener { private LoggingSystem loggingSystem; + private LogFile logFile; + private int order = DEFAULT_ORDER; private boolean parseArgs = true; @@ -224,6 +231,9 @@ public class LoggingApplicationListener implements GenericApplicationListener { if (!beanFactory.containsBean(LOGGING_SYSTEM_BEAN_NAME)) { beanFactory.registerSingleton(LOGGING_SYSTEM_BEAN_NAME, this.loggingSystem); } + if (this.logFile != null && !beanFactory.containsBean(LOGFILE_BEAN_NAME)) { + beanFactory.registerSingleton(LOGFILE_BEAN_NAME, this.logFile); + } } private void onContextClosedEvent() { @@ -246,12 +256,12 @@ public class LoggingApplicationListener implements GenericApplicationListener { */ protected void initialize(ConfigurableEnvironment environment, ClassLoader classLoader) { new LoggingSystemProperties(environment).apply(); - LogFile logFile = LogFile.get(environment); - if (logFile != null) { - logFile.applyToSystemProperties(); + this.logFile = LogFile.get(environment); + if (this.logFile != null) { + this.logFile.applyToSystemProperties(); } initializeEarlyLoggingLevel(environment); - initializeSystem(environment, this.loggingSystem, logFile); + initializeSystem(environment, this.loggingSystem, this.logFile); initializeFinalLoggingLevels(environment, this.loggingSystem); registerShutdownHookIfNecessary(environment, this.loggingSystem); } diff --git a/spring-boot-samples/spring-boot-sample-actuator/src/test/java/sample/actuator/EndpointsPropertiesSampleActuatorApplicationTests.java b/spring-boot-samples/spring-boot-sample-actuator/src/test/java/sample/actuator/EndpointsPropertiesSampleActuatorApplicationTests.java index 00ad0aa1ae8..3c3980e6f8e 100644 --- a/spring-boot-samples/spring-boot-sample-actuator/src/test/java/sample/actuator/EndpointsPropertiesSampleActuatorApplicationTests.java +++ b/spring-boot-samples/spring-boot-sample-actuator/src/test/java/sample/actuator/EndpointsPropertiesSampleActuatorApplicationTests.java @@ -66,6 +66,13 @@ public class EndpointsPropertiesSampleActuatorApplicationTests { assertThat(entity.getBody()).contains("\"hello\":\"world\""); } + @Test + public void logfileWithRandomName() { + ResponseEntity entity = this.restTemplate.withBasicAuth("user", getPassword()) + .getForEntity("/admin/logfile", String.class); + assertThat(entity.getStatusCode()).isEqualTo(HttpStatus.OK); + } + private String getPassword() { return "password"; } diff --git a/spring-boot-samples/spring-boot-sample-actuator/src/test/resources/application-endpoints.properties b/spring-boot-samples/spring-boot-sample-actuator/src/test/resources/application-endpoints.properties index dcd010006e5..fab27a09e53 100644 --- a/spring-boot-samples/spring-boot-sample-actuator/src/test/resources/application-endpoints.properties +++ b/spring-boot-samples/spring-boot-sample-actuator/src/test/resources/application-endpoints.properties @@ -1,3 +1,5 @@ server.error.path: /oops management.endpoint.health.show-details: always management.endpoints.web.base-path: /admin +logging.file=./target/${spring.application.instance_id}.log +spring.application.instance_id=${random.value}