From 4fee5b4e2ddb01a962b28eaf14e71ac4ed3e96f6 Mon Sep 17 00:00:00 2001 From: Kim Choy Date: Thu, 21 Jul 2022 23:25:56 -0700 Subject: [PATCH 1/2] refactor(web): remove ExecutorService bean and construct it locally in ApplicationService since it's only used there. --- .../com/netflix/spinnaker/gate/config/GateConfig.groovy | 8 -------- .../spinnaker/gate/services/ApplicationService.groovy | 4 ++-- .../netflix/spinnaker/gate/ApplicationServiceSpec.groovy | 4 ---- .../gate/controllers/ApplicationControllerSpec.groovy | 3 --- 4 files changed, 2 insertions(+), 17 deletions(-) diff --git a/gate-web/src/main/groovy/com/netflix/spinnaker/gate/config/GateConfig.groovy b/gate-web/src/main/groovy/com/netflix/spinnaker/gate/config/GateConfig.groovy index 8044cb6249..a31196e3ca 100644 --- a/gate-web/src/main/groovy/com/netflix/spinnaker/gate/config/GateConfig.groovy +++ b/gate-web/src/main/groovy/com/netflix/spinnaker/gate/config/GateConfig.groovy @@ -65,9 +65,6 @@ import org.springframework.util.CollectionUtils import org.springframework.web.client.RestTemplate import retrofit.Endpoint -import java.util.concurrent.ExecutorService -import java.util.concurrent.Executors - import static retrofit.Endpoints.newFixedEndpoint @CompileStatic @@ -90,11 +87,6 @@ class GateConfig { new RestTemplate() } - @Bean - ExecutorService executorService() { - Executors.newCachedThreadPool() - } - @Autowired Registry registry diff --git a/gate-web/src/main/groovy/com/netflix/spinnaker/gate/services/ApplicationService.groovy b/gate-web/src/main/groovy/com/netflix/spinnaker/gate/services/ApplicationService.groovy index 8d1f69b087..f886871ab3 100644 --- a/gate-web/src/main/groovy/com/netflix/spinnaker/gate/services/ApplicationService.groovy +++ b/gate-web/src/main/groovy/com/netflix/spinnaker/gate/services/ApplicationService.groovy @@ -36,6 +36,7 @@ import retrofit.converter.ConversionException import java.util.concurrent.Callable import java.util.concurrent.ExecutionException import java.util.concurrent.ExecutorService +import java.util.concurrent.Executors import java.util.concurrent.Future import java.util.concurrent.atomic.AtomicReference import java.util.stream.Collectors @@ -57,14 +58,13 @@ class ApplicationService { ServiceConfiguration serviceConfiguration, ClouddriverServiceSelector clouddriverServiceSelector, Front50Service front50Service, - ExecutorService executorService, ApplicationConfigurationProperties applicationConfigurationProperties ){ this.serviceConfiguration = serviceConfiguration this.clouddriverServiceSelector = clouddriverServiceSelector this.front50Service = front50Service - this.executorService = executorService this.applicationConfigurationProperties = applicationConfigurationProperties + this.executorService = Executors.newCachedThreadPool() this.allApplicationsCache = new AtomicReference<>([]) } diff --git a/gate-web/src/test/groovy/com/netflix/spinnaker/gate/ApplicationServiceSpec.groovy b/gate-web/src/test/groovy/com/netflix/spinnaker/gate/ApplicationServiceSpec.groovy index df0b097598..a60e078a0d 100644 --- a/gate-web/src/test/groovy/com/netflix/spinnaker/gate/ApplicationServiceSpec.groovy +++ b/gate-web/src/test/groovy/com/netflix/spinnaker/gate/ApplicationServiceSpec.groovy @@ -29,8 +29,6 @@ import retrofit.mime.TypedByteArray import spock.lang.Specification import spock.lang.Unroll -import java.util.concurrent.Executors - class ApplicationServiceSpec extends Specification { def clouddriver = Mock(ClouddriverService) def front50 = Mock(Front50Service) @@ -48,7 +46,6 @@ class ApplicationServiceSpec extends Specification { config, clouddriverSelector, front50, - Executors.newFixedThreadPool(1), applicationConfigurationProperties ) return service @@ -240,7 +237,6 @@ class ApplicationServiceSpec extends Specification { config, clouddriverSelector, front50, - Executors.newFixedThreadPool(1), new ApplicationConfigurationProperties() ) diff --git a/gate-web/src/test/groovy/com/netflix/spinnaker/gate/controllers/ApplicationControllerSpec.groovy b/gate-web/src/test/groovy/com/netflix/spinnaker/gate/controllers/ApplicationControllerSpec.groovy index f73f491c6a..da974d9d3f 100644 --- a/gate-web/src/test/groovy/com/netflix/spinnaker/gate/controllers/ApplicationControllerSpec.groovy +++ b/gate-web/src/test/groovy/com/netflix/spinnaker/gate/controllers/ApplicationControllerSpec.groovy @@ -32,8 +32,6 @@ import org.springframework.web.util.NestedServletException import spock.lang.Specification import spock.lang.Unroll -import java.util.concurrent.Executors - import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get class ApplicationControllerSpec extends Specification { @@ -58,7 +56,6 @@ class ApplicationControllerSpec extends Specification { new ServiceConfiguration(), clouddriverSelector, front50Service, - Executors.newFixedThreadPool(1), new ApplicationConfigurationProperties() ) server.start() From d641205a7adedd873bc566a1d91c9c828cbc773f Mon Sep 17 00:00:00 2001 From: Kim Choy Date: Thu, 21 Jul 2022 23:35:45 -0700 Subject: [PATCH 2/2] feat(gate): copy the MDC to async controller method handler threads so log messages contain more context, and so downstream HTTP calls include X-SPINNAKER-* headers via e.g. SpinnakerRequestInterceptor. --- .../gate/config/GateWebConfig.groovy | 13 +- ...bConfigAuthenticatedRequestFilterTest.java | 135 ++++++++++++++++++ 2 files changed, 147 insertions(+), 1 deletion(-) create mode 100644 gate-web/src/test/java/com/netflix/spinnaker/gate/config/GateWebConfigAuthenticatedRequestFilterTest.java diff --git a/gate-web/src/main/groovy/com/netflix/spinnaker/gate/config/GateWebConfig.groovy b/gate-web/src/main/groovy/com/netflix/spinnaker/gate/config/GateWebConfig.groovy index 4acd511ec0..46c1840244 100644 --- a/gate-web/src/main/groovy/com/netflix/spinnaker/gate/config/GateWebConfig.groovy +++ b/gate-web/src/main/groovy/com/netflix/spinnaker/gate/config/GateWebConfig.groovy @@ -23,19 +23,22 @@ import com.netflix.spinnaker.gate.interceptors.ResponseHeaderInterceptor import com.netflix.spinnaker.gate.interceptors.ResponseHeaderInterceptorConfigurationProperties import com.netflix.spinnaker.gate.retrofit.UpstreamBadRequest import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService -import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; +import com.netflix.spinnaker.kork.web.context.MdcCopyingAsyncTaskExecutor import com.netflix.spinnaker.kork.web.interceptors.MetricsInterceptor import org.springframework.beans.factory.annotation.Autowired import org.springframework.beans.factory.annotation.Value +import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty import org.springframework.boot.context.properties.EnableConfigurationProperties import org.springframework.context.ApplicationContext import org.springframework.context.annotation.Bean import org.springframework.context.annotation.ComponentScan import org.springframework.context.annotation.Configuration +import org.springframework.core.task.AsyncTaskExecutor import org.springframework.http.HttpStatus import org.springframework.web.bind.annotation.ControllerAdvice import org.springframework.web.bind.annotation.ExceptionHandler import org.springframework.web.bind.annotation.ResponseBody +import org.springframework.web.servlet.config.annotation.AsyncSupportConfigurer import org.springframework.web.servlet.config.annotation.ContentNegotiationConfigurer import org.springframework.web.servlet.config.annotation.InterceptorRegistry import org.springframework.web.servlet.config.annotation.WebMvcConfigurer @@ -64,6 +67,9 @@ public class GateWebConfig implements WebMvcConfigurer { @Autowired ResponseHeaderInterceptorConfigurationProperties responseHeaderInterceptorConfigurationProperties + @Autowired + AsyncTaskExecutor asyncTaskExecutor + @Override public void addInterceptors(InterceptorRegistry registry) { registry.addInterceptor( @@ -131,4 +137,9 @@ public class GateWebConfig implements WebMvcConfigurer { void configureContentNegotiation(ContentNegotiationConfigurer configurer) { configurer.favorPathExtension(false) } + + @Override + void configureAsyncSupport(AsyncSupportConfigurer configurer) { + configurer.setTaskExecutor(new MdcCopyingAsyncTaskExecutor(asyncTaskExecutor)) + } } diff --git a/gate-web/src/test/java/com/netflix/spinnaker/gate/config/GateWebConfigAuthenticatedRequestFilterTest.java b/gate-web/src/test/java/com/netflix/spinnaker/gate/config/GateWebConfigAuthenticatedRequestFilterTest.java new file mode 100644 index 0000000000..b52ae58a1d --- /dev/null +++ b/gate-web/src/test/java/com/netflix/spinnaker/gate/config/GateWebConfigAuthenticatedRequestFilterTest.java @@ -0,0 +1,135 @@ +/* + * Copyright 2022 Salesforce, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.netflix.spinnaker.gate.config; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.*; +import static org.springframework.test.web.servlet.result.MockMvcResultHandlers.print; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.request; +import static org.springframework.test.web.servlet.setup.MockMvcBuilders.webAppContextSetup; + +import ch.qos.logback.classic.Level; +import com.netflix.spinnaker.gate.Main; +import com.netflix.spinnaker.kork.common.Header; +import com.netflix.spinnaker.kork.test.log.MemoryAppender; +import com.netflix.spinnaker.security.AuthenticatedRequest; +import java.util.List; +import org.junit.jupiter.api.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Qualifier; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.web.servlet.FilterRegistrationBean; +import org.springframework.test.context.TestPropertySource; +import org.springframework.test.web.servlet.MockMvc; +import org.springframework.test.web.servlet.MvcResult; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.RequestMethod; +import org.springframework.web.bind.annotation.RestController; +import org.springframework.web.context.WebApplicationContext; +import org.springframework.web.servlet.mvc.method.annotation.StreamingResponseBody; + +@SpringBootTest( + classes = {Main.class, GateWebConfigAuthenticatedRequestFilterTest.TestController.class}) +@TestPropertySource(properties = {"spring.config.location=classpath:gate-test.yml"}) +public class GateWebConfigAuthenticatedRequestFilterTest { + + private static final String API_BASE = "/test"; + private static final String API_PATH = "/asyncApi"; + + private static final String LOG_MESSAGE = " logged in async api: "; + + @RestController + @RequestMapping(value = API_BASE) + static class TestController { + private final Logger log = LoggerFactory.getLogger(getClass()); + + @RequestMapping(value = API_PATH, method = RequestMethod.GET) + public StreamingResponseBody asyncApi() { + return outputStream -> { + AuthenticatedRequest.get(Header.USER) + .ifPresent( + v -> { + log.info(Header.USER.name() + LOG_MESSAGE + v); + }); + AuthenticatedRequest.get(Header.APPLICATION) + .ifPresent( + v -> { + log.info(Header.APPLICATION.name() + LOG_MESSAGE + v); + }); + AuthenticatedRequest.get(Header.EXECUTION_TYPE) + .ifPresent( + v -> { + log.info(Header.EXECUTION_TYPE.name() + LOG_MESSAGE + v); + }); + }; + } + } + + @Autowired + @Qualifier("authenticatedRequestFilter") + private FilterRegistrationBean filterRegistrationBean; + + @Autowired private WebApplicationContext webApplicationContext; + + private MockMvc mockMvc; + + @Test + void TestHeaderAvailableInAuthenticatedRequestMDCDuringAsyncApi() throws Exception { + mockMvc = + webAppContextSetup(webApplicationContext) + .addFilters(filterRegistrationBean.getFilter()) + .build(); + + MemoryAppender memoryAppender = new MemoryAppender(TestController.class); + + String testUser = "Test User"; + String testApplication = "Test Application"; + String testExecutionType = "Test Execution Type"; + + MvcResult result = + mockMvc + .perform( + get(API_BASE + API_PATH) + .header(Header.USER.getHeader(), testUser) + .header(Header.APPLICATION.getHeader(), testApplication) + .header(Header.EXECUTION_TYPE.getHeader(), testExecutionType)) + .andExpect(request().asyncStarted()) + .andDo(print()) + .andReturn(); + + mockMvc.perform(asyncDispatch(result)).andDo(print()); + + List messages = memoryAppender.layoutSearch(LOG_MESSAGE, Level.INFO); + String expectedUserLog = Header.USER.name() + LOG_MESSAGE + testUser; + String expectedApplicationLog = Header.APPLICATION.name() + LOG_MESSAGE + testApplication; + String expectedExecutionTypeLog = + Header.EXECUTION_TYPE.name() + LOG_MESSAGE + testExecutionType; + + assertThat(messages.size(), equalTo(3)); + assertThat( + messages, + contains( + containsString(expectedUserLog), + containsString(expectedApplicationLog), + containsString(expectedExecutionTypeLog))); + } +}