diff --git a/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/RetrofitServiceFactory.java b/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/RetrofitServiceFactory.java index 40465bec5..2dd37b1fe 100644 --- a/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/RetrofitServiceFactory.java +++ b/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/RetrofitServiceFactory.java @@ -27,7 +27,6 @@ import com.netflix.spinnaker.kork.client.ServiceClientFactory; import com.netflix.spinnaker.retrofit.Slf4jRetrofitLogger; import retrofit.Endpoint; -import retrofit.RequestInterceptor; import retrofit.RestAdapter; import retrofit.converter.JacksonConverter; @@ -36,22 +35,17 @@ class RetrofitServiceFactory implements ServiceClientFactory { private final RestAdapter.LogLevel retrofitLogLevel; private final OkHttpClientProvider clientProvider; - private final RequestInterceptor spinnakerRequestInterceptor; RetrofitServiceFactory( - RestAdapter.LogLevel retrofitLogLevel, - OkHttpClientProvider clientProvider, - RequestInterceptor spinnakerRequestInterceptor) { + RestAdapter.LogLevel retrofitLogLevel, OkHttpClientProvider clientProvider) { this.retrofitLogLevel = retrofitLogLevel; this.clientProvider = clientProvider; - this.spinnakerRequestInterceptor = spinnakerRequestInterceptor; } @Override public T create(Class type, ServiceEndpoint serviceEndpoint, ObjectMapper objectMapper) { Endpoint endpoint = newFixedEndpoint(serviceEndpoint.getBaseUrl()); return new RestAdapter.Builder() - .setRequestInterceptor(spinnakerRequestInterceptor) .setConverter(new JacksonConverter(objectMapper)) .setEndpoint(endpoint) .setClient(new Ok3Client(clientProvider.getClient(serviceEndpoint))) diff --git a/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/RetrofitServiceFactoryAutoConfiguration.java b/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/RetrofitServiceFactoryAutoConfiguration.java index d9a93fb9e..968ad5588 100644 --- a/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/RetrofitServiceFactoryAutoConfiguration.java +++ b/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/RetrofitServiceFactoryAutoConfiguration.java @@ -24,7 +24,6 @@ import org.springframework.context.annotation.Configuration; import org.springframework.core.Ordered; import org.springframework.core.annotation.Order; -import retrofit.RequestInterceptor; import retrofit.RestAdapter; @Configuration @@ -34,10 +33,7 @@ public class RetrofitServiceFactoryAutoConfiguration { @Bean @Order(Ordered.LOWEST_PRECEDENCE) ServiceClientFactory serviceClientFactory( - RestAdapter.LogLevel retrofitLogLevel, - OkHttpClientProvider clientProvider, - RequestInterceptor spinnakerRequestInterceptor) { - return new RetrofitServiceFactory( - retrofitLogLevel, clientProvider, spinnakerRequestInterceptor); + RestAdapter.LogLevel retrofitLogLevel, OkHttpClientProvider clientProvider) { + return new RetrofitServiceFactory(retrofitLogLevel, clientProvider); } } diff --git a/kork-retrofit/src/test/kotlin/com/netflix/spinnaker/kork/retrofit/RetrofitServiceProviderTest.kt b/kork-retrofit/src/test/kotlin/com/netflix/spinnaker/kork/retrofit/RetrofitServiceProviderTest.kt index a6f8e02c0..0c1d83d9e 100644 --- a/kork-retrofit/src/test/kotlin/com/netflix/spinnaker/kork/retrofit/RetrofitServiceProviderTest.kt +++ b/kork-retrofit/src/test/kotlin/com/netflix/spinnaker/kork/retrofit/RetrofitServiceProviderTest.kt @@ -29,10 +29,17 @@ import com.netflix.spinnaker.config.DefaultServiceClientProvider import com.netflix.spinnaker.config.OkHttpClientComponents import com.netflix.spinnaker.kork.client.ServiceClientFactory import com.netflix.spinnaker.kork.client.ServiceClientProvider +import com.netflix.spinnaker.kork.common.Header import com.netflix.spinnaker.okhttp.OkHttpClientConfigurationProperties +import com.netflix.spinnaker.okhttp.SpinnakerRequestHeaderInterceptor +import com.netflix.spinnaker.security.AuthenticatedRequest import dev.minutest.junit.JUnit5Minutests import dev.minutest.rootContext +import io.mockk.every +import io.mockk.mockkStatic import okhttp3.OkHttpClient +import okhttp3.mockwebserver.MockResponse +import okhttp3.mockwebserver.MockWebServer import org.springframework.boot.autoconfigure.AutoConfigurations import org.springframework.boot.autoconfigure.task.TaskExecutionAutoConfiguration import org.springframework.boot.test.context.assertj.AssertableApplicationContext @@ -45,6 +52,9 @@ import retrofit.http.Path import strikt.api.expect import strikt.assertions.isA import strikt.assertions.isEqualTo +import strikt.assertions.isNotNull +import java.util.Optional +import java.util.concurrent.TimeUnit class RetrofitServiceProviderTest : JUnit5Minutests { @@ -78,6 +88,32 @@ class RetrofitServiceProviderTest : JUnit5Minutests { } } + test("auth headers are propagated once") { + mockkStatic(AuthenticatedRequest::class) + every { AuthenticatedRequest.getAuthenticationHeaders() } returns mapOf( + Header.USER.header to Optional.of("user"), + ) + + run { ctx: AssertableApplicationContext -> + val mockWebServer = MockWebServer() + mockWebServer.enqueue(MockResponse().setBody("response")) + + val retrofitClient = ctx.getBean(ServiceClientProvider::class.java).getService( + Retrofit1Service::class.java, + DefaultServiceEndpoint("retrofit1", mockWebServer.url("/").toString()) + ) + + retrofitClient.getSomething("user", null) + val recordedRequest = mockWebServer.takeRequest(5, TimeUnit.SECONDS) + + expect { + that(recordedRequest).isNotNull() + that(recordedRequest!!.headers.values("X-SPINNAKER-USER").size).isEqualTo(1) + } + + mockWebServer.shutdown() + } + } } } @@ -91,8 +127,15 @@ private open class TestConfiguration { HttpTracing.newBuilder(Tracing.newBuilder().build()).build() @Bean - open fun okHttpClient(httpTracing: HttpTracing): OkHttpClient { - return RawOkHttpClientFactory().create(OkHttpClientConfigurationProperties(), emptyList(), httpTracing) + open fun okHttpClient( + httpTracing: HttpTracing, + spinnakerRequestHeaderInterceptor: SpinnakerRequestHeaderInterceptor, + ): OkHttpClient { + return RawOkHttpClientFactory().create( + OkHttpClientConfigurationProperties(), + listOf(spinnakerRequestHeaderInterceptor), + httpTracing, + ) } @Bean @@ -110,7 +153,6 @@ private open class TestConfiguration { serviceClientFactories: List, objectMapper: ObjectMapper): DefaultServiceClientProvider { return DefaultServiceClientProvider(serviceClientFactories, objectMapper) } - } interface Retrofit1Service { diff --git a/kork-retrofit2/src/test/kotlin/com/netflix/spinnaker/kork/retrofit/Retrofit2ServiceProviderTest.kt b/kork-retrofit2/src/test/kotlin/com/netflix/spinnaker/kork/retrofit/Retrofit2ServiceProviderTest.kt index 47b20cb71..52c1b70f5 100644 --- a/kork-retrofit2/src/test/kotlin/com/netflix/spinnaker/kork/retrofit/Retrofit2ServiceProviderTest.kt +++ b/kork-retrofit2/src/test/kotlin/com/netflix/spinnaker/kork/retrofit/Retrofit2ServiceProviderTest.kt @@ -28,7 +28,6 @@ import com.netflix.spinnaker.config.DefaultServiceClientProvider import com.netflix.spinnaker.kork.client.ServiceClientFactory import com.netflix.spinnaker.kork.client.ServiceClientProvider import com.netflix.spinnaker.okhttp.OkHttpClientConfigurationProperties -import com.netflix.spinnaker.okhttp.SpinnakerRequestInterceptor import dev.minutest.junit.JUnit5Minutests import dev.minutest.rootContext import okhttp3.OkHttpClient @@ -100,11 +99,6 @@ private open class TestConfiguration { return OkHttpClientProvider(listOf(DefaultOkHttpClientBuilderProvider(okHttpClient, OkHttpClientConfigurationProperties()))) } - @Bean - open fun spinnakerRequestInterceptor(): SpinnakerRequestInterceptor { - return SpinnakerRequestInterceptor(OkHttpClientConfigurationProperties()) - } - @Bean open fun objectMapper(): ObjectMapper { return ObjectMapper() diff --git a/kork-web/src/main/groovy/com/netflix/spinnaker/config/OkHttpClientComponents.java b/kork-web/src/main/groovy/com/netflix/spinnaker/config/OkHttpClientComponents.java index e187e079d..9018c355f 100644 --- a/kork-web/src/main/groovy/com/netflix/spinnaker/config/OkHttpClientComponents.java +++ b/kork-web/src/main/groovy/com/netflix/spinnaker/config/OkHttpClientComponents.java @@ -30,7 +30,6 @@ import com.netflix.spinnaker.okhttp.OkHttpClientConfigurationProperties; import com.netflix.spinnaker.okhttp.OkHttpMetricsInterceptor; import com.netflix.spinnaker.okhttp.SpinnakerRequestHeaderInterceptor; -import com.netflix.spinnaker.okhttp.SpinnakerRequestInterceptor; import com.netflix.spinnaker.retrofit.Retrofit2ConfigurationProperties; import com.netflix.spinnaker.retrofit.RetrofitConfigurationProperties; import java.io.File; @@ -80,11 +79,6 @@ public class OkHttpClientComponents { private final OkHttpMetricsInterceptorProperties metricsProperties; private final Retrofit2ConfigurationProperties retrofit2Properties; - @Bean - public SpinnakerRequestInterceptor spinnakerRequestInterceptor() { - return new SpinnakerRequestInterceptor(clientProperties); - } - @Bean public SpinnakerRequestHeaderInterceptor spinnakerRequestHeaderInterceptor() { return new SpinnakerRequestHeaderInterceptor(clientProperties); diff --git a/kork-web/src/main/groovy/com/netflix/spinnaker/okhttp/SpinnakerRequestInterceptor.groovy b/kork-web/src/main/groovy/com/netflix/spinnaker/okhttp/SpinnakerRequestInterceptor.groovy deleted file mode 100644 index 0e97ddc69..000000000 --- a/kork-web/src/main/groovy/com/netflix/spinnaker/okhttp/SpinnakerRequestInterceptor.groovy +++ /dev/null @@ -1,41 +0,0 @@ -/* - * Copyright 2016 Netflix, 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.okhttp - -import com.netflix.spinnaker.security.AuthenticatedRequest -import retrofit.RequestInterceptor - -class SpinnakerRequestInterceptor implements RequestInterceptor { - private final OkHttpClientConfigurationProperties okHttpClientConfigurationProperties - - SpinnakerRequestInterceptor(OkHttpClientConfigurationProperties okHttpClientConfigurationProperties) { - this.okHttpClientConfigurationProperties = okHttpClientConfigurationProperties - } - - void intercept(RequestInterceptor.RequestFacade request) { - if (!okHttpClientConfigurationProperties.propagateSpinnakerHeaders) { - // noop - return - } - - AuthenticatedRequest.authenticationHeaders.each { String key, Optional value -> - if (value.present) { - request.addHeader(key, value.get()) - } - } - } -} diff --git a/kork-web/src/main/java/com/netflix/spinnaker/kork/web/context/MdcCopyingAsyncTaskExecutor.java b/kork-web/src/main/java/com/netflix/spinnaker/kork/web/context/MdcCopyingAsyncTaskExecutor.java index a75b902bc..711d2b4c3 100644 --- a/kork-web/src/main/java/com/netflix/spinnaker/kork/web/context/MdcCopyingAsyncTaskExecutor.java +++ b/kork-web/src/main/java/com/netflix/spinnaker/kork/web/context/MdcCopyingAsyncTaskExecutor.java @@ -30,10 +30,11 @@ *

The typical pattern is: * *

AuthenticatedRequestFilter: copies X-SPINNAKER-* incoming request headers into the MDC - * SpinnakerRequestInterceptor: copies X-SPINNAKER-* from the MDC into outgoing request headers + * SpinnakerRequestHeaderInterceptor: copies X-SPINNAKER-* from the MDC into outgoing request + * headers * - *

MdcCopyingAsyncTaskExecutor makes it so SpinnakerRequestInterceptor has something to copy for - * async methods. It also makes it so log messages include X-SPINNAKER-*, specifically + *

MdcCopyingAsyncTaskExecutor makes it so SpinnakerRequestHeaderInterceptor has something to + * copy for async methods. It also makes it so log messages include X-SPINNAKER-*, specifically * X-SPINNAKER-REQUEST-ID and X-SPINNAKER-REQUEST-ID to faciliate troubleshooting. */ public class MdcCopyingAsyncTaskExecutor implements AsyncTaskExecutor {