fix(headers): avoid retrofit and okhttp adding duplicate headers

With the addition of the okHttp3 request interceptor `SpinnakerRequestHeaderInterceptor` in
#1004 having retrofit1 add `X-SPINNAKER-*` headers is now redundant,
this results in the headers being added twice to all retrofit1 client requests.

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> T create(Class<T> 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)))

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);
  }
 }

kork-retrofit/src/test/kotlin/com/netflix/spinnaker/kork/retrofit/RetrofitServiceProviderTest.kt

@@ -29,22 +29,33 @@ 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
 import org.springframework.boot.test.context.runner.ApplicationContextRunner
 import org.springframework.context.annotation.Bean
 import org.springframework.context.annotation.Configuration
+import org.springframework.context.annotation.Primary
 import retrofit.Callback
 import retrofit.http.GET
 import retrofit.http.Path
 import strikt.api.expect
 import strikt.assertions.isA
 import strikt.assertions.isEqualTo
+import strikt.assertions.isNotNull
+import java.util.*
+import java.util.concurrent.TimeUnit
 
 class RetrofitServiceProviderTest : JUnit5Minutests {
 
@@ -78,6 +89,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 +128,20 @@ private open class TestConfiguration {
  HttpTracing.newBuilder(Tracing.newBuilder().build()).build()
 
  @Bean
- open fun okHttpClient(httpTracing: HttpTracing): OkHttpClient {
- return RawOkHttpClientFactory().create(OkHttpClientConfigurationProperties(), emptyList(), httpTracing)
+ @Primary
+ open fun spinnakerHeaderRequestInterceptor(): SpinnakerRequestHeaderInterceptor =
+ SpinnakerRequestHeaderInterceptor(OkHttpClientConfigurationProperties())
+
+ @Bean
+ open fun okHttpClient(
+ httpTracing: HttpTracing,
+ spinnakerRequestHeaderInterceptor: SpinnakerRequestHeaderInterceptor,
+ ): OkHttpClient {
+ return RawOkHttpClientFactory().create(
+ OkHttpClientConfigurationProperties(),
+ listOf(spinnakerHeaderRequestInterceptor()),
+ httpTracing,
+ )
  }
 
  @Bean
@@ -110,7 +159,6 @@ private open class TestConfiguration {
  serviceClientFactories: List<ServiceClientFactory?>, objectMapper: ObjectMapper): DefaultServiceClientProvider {
  return DefaultServiceClientProvider(serviceClientFactories, objectMapper)
  }
-
 }
 
 interface Retrofit1Service {

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()

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);