Skip to content

Commit

Permalink
feat(gate): copy the MDC to async controller method handler threads (#…
Browse files Browse the repository at this point in the history
…1829)

* refactor(web): remove ExecutorService bean

and construct it locally in ApplicationService since it's only used there.

* 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.

---------

Co-authored-by: Kim Choy <kchoy@salesforce.com>
  • Loading branch information
dbyron-sf and kchoy-sfdc authored Sep 12, 2024
1 parent bd9b281 commit 81b25aa
Show file tree
Hide file tree
Showing 6 changed files with 149 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -90,11 +87,6 @@ class GateConfig {
new RestTemplate()
}

@Bean
ExecutorService executorService() {
Executors.newCachedThreadPool()
}

@Autowired
Registry registry

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -64,6 +67,9 @@ public class GateWebConfig implements WebMvcConfigurer {
@Autowired
ResponseHeaderInterceptorConfigurationProperties responseHeaderInterceptorConfigurationProperties

@Autowired
AsyncTaskExecutor asyncTaskExecutor

@Override
public void addInterceptors(InterceptorRegistry registry) {
registry.addInterceptor(
Expand Down Expand Up @@ -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))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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<>([])
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -48,7 +46,6 @@ class ApplicationServiceSpec extends Specification {
config,
clouddriverSelector,
front50,
Executors.newFixedThreadPool(1),
applicationConfigurationProperties
)
return service
Expand Down Expand Up @@ -240,7 +237,6 @@ class ApplicationServiceSpec extends Specification {
config,
clouddriverSelector,
front50,
Executors.newFixedThreadPool(1),
new ApplicationConfigurationProperties()
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -58,7 +56,6 @@ class ApplicationControllerSpec extends Specification {
new ServiceConfiguration(),
clouddriverSelector,
front50Service,
Executors.newFixedThreadPool(1),
new ApplicationConfigurationProperties()
)
server.start()
Expand Down
Original file line number Diff line number Diff line change
@@ -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<String> 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)));
}
}

0 comments on commit 81b25aa

Please sign in to comment.