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

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

kork-web/src/main/java/com/netflix/spinnaker/kork/web/context/MdcCopyingAsyncTaskExecutor.java

@@ -30,10 +30,11 @@
  * <p>The typical pattern is:
  *
  * <p>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
  *
- * <p>MdcCopyingAsyncTaskExecutor makes it so SpinnakerRequestInterceptor has something to copy for
- * async methods. It also makes it so log messages include X-SPINNAKER-*, specifically
+ * <p>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 {