Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(gate): copy the MDC to async controller method handler threads #1829

Merged
merged 2 commits into from
Sep 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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)));
}
}