From 903e6d39c14d2d9333b9d763885f6ba2ae26bdba Mon Sep 17 00:00:00 2001 From: David Byron Date: Sun, 18 Feb 2024 14:21:02 -0800 Subject: [PATCH 01/10] refactor(web): move OkHttpClientComponents.java from src/main/groovy to src/main/java since that's where java files belong --- .../com/netflix/spinnaker/config/OkHttpClientComponents.java | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename kork-web/src/main/{groovy => java}/com/netflix/spinnaker/config/OkHttpClientComponents.java (100%) diff --git a/kork-web/src/main/groovy/com/netflix/spinnaker/config/OkHttpClientComponents.java b/kork-web/src/main/java/com/netflix/spinnaker/config/OkHttpClientComponents.java similarity index 100% rename from kork-web/src/main/groovy/com/netflix/spinnaker/config/OkHttpClientComponents.java rename to kork-web/src/main/java/com/netflix/spinnaker/config/OkHttpClientComponents.java From 8538173cfc3b27303a0490e52d1b2fa1aa28d69f Mon Sep 17 00:00:00 2001 From: David Byron Date: Fri, 19 Jan 2024 20:03:53 -0800 Subject: [PATCH 02/10] test(web): demonstrate behavior of SpinnakerRequestInterceptor --- kork-web/kork-web.gradle | 1 + .../SpinnakerRequestInterceptorTest.java | 139 ++++++++++++++++++ 2 files changed, 140 insertions(+) create mode 100644 kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestInterceptorTest.java diff --git a/kork-web/kork-web.gradle b/kork-web/kork-web.gradle index 313f38191..94bab50ca 100644 --- a/kork-web/kork-web.gradle +++ b/kork-web/kork-web.gradle @@ -43,6 +43,7 @@ dependencies { runtimeOnly "org.hibernate.validator:hibernate-validator" testImplementation project(":kork-test") + testImplementation "com.github.tomakehurst:wiremock-jre8-standalone" testImplementation "ch.qos.logback:logback-classic" testImplementation "ch.qos.logback:logback-core" testImplementation "org.spockframework:spock-core" diff --git a/kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestInterceptorTest.java b/kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestInterceptorTest.java new file mode 100644 index 000000000..3d5a06a95 --- /dev/null +++ b/kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestInterceptorTest.java @@ -0,0 +1,139 @@ +/* + * Copyright 2024 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.okhttp; + +import static com.github.tomakehurst.wiremock.client.WireMock.equalTo; +import static com.github.tomakehurst.wiremock.client.WireMock.get; +import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor; +import static com.github.tomakehurst.wiremock.client.WireMock.ok; +import static com.github.tomakehurst.wiremock.client.WireMock.urlPathEqualTo; +import static com.github.tomakehurst.wiremock.client.WireMock.verify; + +import com.github.tomakehurst.wiremock.client.WireMock; +import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo; +import com.github.tomakehurst.wiremock.junit5.WireMockTest; +import com.netflix.spinnaker.kork.common.Header; +import com.netflix.spinnaker.security.AuthenticatedRequest; +import java.util.Map; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInfo; +import retrofit.RestAdapter; +import retrofit.client.Response; +import retrofit.http.GET; + +@WireMockTest +class SpinnakerRequestInterceptorTest { + + public static final String REQUEST_PATH = "/foo"; + + public static final String TEST_USER = "some-user"; + + public static final String TEST_ACCOUNTS = "some-accounts"; + + public static final String TEST_REQUEST_ID = "some-request-id"; + + public static final Map TEST_SPINNAKER_HEADERS = + Map.of( + Header.USER, + TEST_USER, + Header.ACCOUNTS, + TEST_ACCOUNTS, + Header.REQUEST_ID, + TEST_REQUEST_ID); + + @BeforeEach + void setup(TestInfo testInfo, WireMockRuntimeInfo wmRuntimeInfo) { + System.out.println("--------------- Test " + testInfo.getDisplayName()); + + WireMock wireMock = wmRuntimeInfo.getWireMock(); + + // set up an arbitrary response to avoid 404s, and so it's possible to + // verify the request headers wiremock receives. + wireMock.register(get(REQUEST_PATH).willReturn(ok())); + } + + @AfterEach + void cleanup() { + AuthenticatedRequest.clear(); + } + + @Test + void propagateSpinnakerHeadersFalse(WireMockRuntimeInfo wmRuntimeInfo) { + OkHttpClientConfigurationProperties okHttpClientConfigurationProperties = + new OkHttpClientConfigurationProperties(); + okHttpClientConfigurationProperties.setPropagateSpinnakerHeaders(false); + SpinnakerRequestInterceptor spinnakerRequestInterceptor = + new SpinnakerRequestInterceptor(okHttpClientConfigurationProperties); + + RetrofitService retrofitService = + makeRetrofitService(wmRuntimeInfo, spinnakerRequestInterceptor); + + // Add some spinnaker headers to the MDC + TEST_SPINNAKER_HEADERS.forEach(AuthenticatedRequest::set); + + // Make a request + retrofitService.getRequest(); + + // Verify that wiremock didn't receive the spinnaker headers + TEST_SPINNAKER_HEADERS.forEach( + (Header header, String value) -> { + verify(getRequestedFor(urlPathEqualTo(REQUEST_PATH)).withoutHeader(header.getHeader())); + }); + } + + @Test + void propagateSpinnakerHeadersTrue(WireMockRuntimeInfo wmRuntimeInfo) { + OkHttpClientConfigurationProperties okHttpClientConfigurationProperties = + new OkHttpClientConfigurationProperties(); + okHttpClientConfigurationProperties.setPropagateSpinnakerHeaders(true); + SpinnakerRequestInterceptor spinnakerRequestInterceptor = + new SpinnakerRequestInterceptor(okHttpClientConfigurationProperties); + + RetrofitService retrofitService = + makeRetrofitService(wmRuntimeInfo, spinnakerRequestInterceptor); + + // Add some spinnaker headers to the MDC + TEST_SPINNAKER_HEADERS.forEach(AuthenticatedRequest::set); + + // Make a request + retrofitService.getRequest(); + + // Verify that wiremock received the spinnaker headers + TEST_SPINNAKER_HEADERS.forEach( + (Header header, String value) -> { + verify( + getRequestedFor(urlPathEqualTo(REQUEST_PATH)) + .withHeader(header.getHeader(), equalTo(value))); + }); + } + + private RetrofitService makeRetrofitService( + WireMockRuntimeInfo wmRuntimeInfo, SpinnakerRequestInterceptor spinnakerRequestInterceptor) { + return new RestAdapter.Builder() + .setRequestInterceptor(spinnakerRequestInterceptor) + .setEndpoint(wmRuntimeInfo.getHttpBaseUrl()) + .build() + .create(RetrofitService.class); + } + + interface RetrofitService { + @GET(REQUEST_PATH) + Response getRequest(); + } +} From 2558471b95c4782537aa5b8803cf1e73ef383fc0 Mon Sep 17 00:00:00 2001 From: David Byron Date: Fri, 19 Jan 2024 20:41:31 -0800 Subject: [PATCH 03/10] refactor(web/test): parameterize SpinnakerRequestInterceptorTest to reduce duplication --- kork-web/kork-web.gradle | 1 + .../SpinnakerRequestInterceptorTest.java | 74 ++++++++----------- 2 files changed, 33 insertions(+), 42 deletions(-) diff --git a/kork-web/kork-web.gradle b/kork-web/kork-web.gradle index 94bab50ca..f8eae0fff 100644 --- a/kork-web/kork-web.gradle +++ b/kork-web/kork-web.gradle @@ -46,6 +46,7 @@ dependencies { testImplementation "com.github.tomakehurst:wiremock-jre8-standalone" testImplementation "ch.qos.logback:logback-classic" testImplementation "ch.qos.logback:logback-core" + testImplementation "org.junit.jupiter:junit-jupiter-params" testImplementation "org.spockframework:spock-core" testImplementation "org.spockframework:spock-spring" testImplementation "org.springframework.boot:spring-boot-starter-test" diff --git a/kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestInterceptorTest.java b/kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestInterceptorTest.java index 3d5a06a95..836ca8ea7 100644 --- a/kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestInterceptorTest.java +++ b/kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestInterceptorTest.java @@ -21,23 +21,24 @@ import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor; import static com.github.tomakehurst.wiremock.client.WireMock.ok; import static com.github.tomakehurst.wiremock.client.WireMock.urlPathEqualTo; -import static com.github.tomakehurst.wiremock.client.WireMock.verify; +import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig; -import com.github.tomakehurst.wiremock.client.WireMock; +import com.github.tomakehurst.wiremock.junit5.WireMockExtension; import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo; -import com.github.tomakehurst.wiremock.junit5.WireMockTest; +import com.github.tomakehurst.wiremock.matching.RequestPatternBuilder; import com.netflix.spinnaker.kork.common.Header; import com.netflix.spinnaker.security.AuthenticatedRequest; import java.util.Map; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestInfo; +import org.junit.jupiter.api.extension.RegisterExtension; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import retrofit.RestAdapter; import retrofit.client.Response; import retrofit.http.GET; -@WireMockTest class SpinnakerRequestInterceptorTest { public static final String REQUEST_PATH = "/foo"; @@ -57,15 +58,22 @@ class SpinnakerRequestInterceptorTest { Header.REQUEST_ID, TEST_REQUEST_ID); + /** + * Use this instead of annotating the class with @WireMockTest so there's a WireMock object + * available for parameterized tests. Otherwise the arguments from e.g. ValueSource compete with + * the WireMockRuntimeInfo argument that @WireMockTest provides. + */ + @RegisterExtension + static WireMockExtension wireMock = + WireMockExtension.newInstance().options(wireMockConfig().dynamicPort()).build(); + @BeforeEach void setup(TestInfo testInfo, WireMockRuntimeInfo wmRuntimeInfo) { System.out.println("--------------- Test " + testInfo.getDisplayName()); - WireMock wireMock = wmRuntimeInfo.getWireMock(); - // set up an arbitrary response to avoid 404s, and so it's possible to // verify the request headers wiremock receives. - wireMock.register(get(REQUEST_PATH).willReturn(ok())); + wireMock.stubFor(get(REQUEST_PATH).willReturn(ok())); } @AfterEach @@ -73,40 +81,17 @@ void cleanup() { AuthenticatedRequest.clear(); } - @Test - void propagateSpinnakerHeadersFalse(WireMockRuntimeInfo wmRuntimeInfo) { - OkHttpClientConfigurationProperties okHttpClientConfigurationProperties = - new OkHttpClientConfigurationProperties(); - okHttpClientConfigurationProperties.setPropagateSpinnakerHeaders(false); - SpinnakerRequestInterceptor spinnakerRequestInterceptor = - new SpinnakerRequestInterceptor(okHttpClientConfigurationProperties); - - RetrofitService retrofitService = - makeRetrofitService(wmRuntimeInfo, spinnakerRequestInterceptor); - - // Add some spinnaker headers to the MDC - TEST_SPINNAKER_HEADERS.forEach(AuthenticatedRequest::set); - - // Make a request - retrofitService.getRequest(); - - // Verify that wiremock didn't receive the spinnaker headers - TEST_SPINNAKER_HEADERS.forEach( - (Header header, String value) -> { - verify(getRequestedFor(urlPathEqualTo(REQUEST_PATH)).withoutHeader(header.getHeader())); - }); - } - - @Test - void propagateSpinnakerHeadersTrue(WireMockRuntimeInfo wmRuntimeInfo) { + @ParameterizedTest(name = "propagateSpinnakerHeaders = {0}") + @ValueSource(booleans = {false, true}) + void propagateSpinnakerHeaders(boolean propagateSpinnakerHeaders) { OkHttpClientConfigurationProperties okHttpClientConfigurationProperties = new OkHttpClientConfigurationProperties(); - okHttpClientConfigurationProperties.setPropagateSpinnakerHeaders(true); + okHttpClientConfigurationProperties.setPropagateSpinnakerHeaders(propagateSpinnakerHeaders); SpinnakerRequestInterceptor spinnakerRequestInterceptor = new SpinnakerRequestInterceptor(okHttpClientConfigurationProperties); RetrofitService retrofitService = - makeRetrofitService(wmRuntimeInfo, spinnakerRequestInterceptor); + makeRetrofitService(wireMock.baseUrl(), spinnakerRequestInterceptor); // Add some spinnaker headers to the MDC TEST_SPINNAKER_HEADERS.forEach(AuthenticatedRequest::set); @@ -114,20 +99,25 @@ void propagateSpinnakerHeadersTrue(WireMockRuntimeInfo wmRuntimeInfo) { // Make a request retrofitService.getRequest(); - // Verify that wiremock received the spinnaker headers + // Verify that wiremock did/didn't receive the spinnaker headers as appropriate TEST_SPINNAKER_HEADERS.forEach( (Header header, String value) -> { - verify( - getRequestedFor(urlPathEqualTo(REQUEST_PATH)) - .withHeader(header.getHeader(), equalTo(value))); + RequestPatternBuilder requestPatternBuilder = + getRequestedFor(urlPathEqualTo(REQUEST_PATH)); + if (propagateSpinnakerHeaders) { + requestPatternBuilder.withHeader(header.getHeader(), equalTo(value)); + } else { + requestPatternBuilder.withoutHeader(header.getHeader()); + } + wireMock.verify(requestPatternBuilder); }); } private RetrofitService makeRetrofitService( - WireMockRuntimeInfo wmRuntimeInfo, SpinnakerRequestInterceptor spinnakerRequestInterceptor) { + String baseUrl, SpinnakerRequestInterceptor spinnakerRequestInterceptor) { return new RestAdapter.Builder() .setRequestInterceptor(spinnakerRequestInterceptor) - .setEndpoint(wmRuntimeInfo.getHttpBaseUrl()) + .setEndpoint(baseUrl) .build() .create(RetrofitService.class); } From 96bbc7ba7a3038905d2f954e9b3f9f1a2e186cd2 Mon Sep 17 00:00:00 2001 From: David Byron Date: Fri, 19 Jan 2024 22:00:39 -0800 Subject: [PATCH 04/10] test(web): verify that OkHttpClientComponents creates the expected beans --- .../config/OkHttpClientComponentsTest.java | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 kork-web/src/test/java/com/netflix/spinnaker/config/OkHttpClientComponentsTest.java diff --git a/kork-web/src/test/java/com/netflix/spinnaker/config/OkHttpClientComponentsTest.java b/kork-web/src/test/java/com/netflix/spinnaker/config/OkHttpClientComponentsTest.java new file mode 100644 index 000000000..0975280a3 --- /dev/null +++ b/kork-web/src/test/java/com/netflix/spinnaker/config/OkHttpClientComponentsTest.java @@ -0,0 +1,54 @@ +/* + * Copyright 2024 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.config; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.netflix.spinnaker.okhttp.OkHttp3MetricsInterceptor; +import com.netflix.spinnaker.okhttp.OkHttpMetricsInterceptor; +import com.netflix.spinnaker.okhttp.SpinnakerRequestHeaderInterceptor; +import com.netflix.spinnaker.okhttp.SpinnakerRequestInterceptor; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInfo; +import org.springframework.boot.context.annotation.UserConfigurations; +import org.springframework.boot.task.TaskExecutorBuilder; +import org.springframework.boot.test.context.runner.ApplicationContextRunner; + +class OkHttpClientComponentsTest { + + private final ApplicationContextRunner runner = + new ApplicationContextRunner() + .withBean(TaskExecutorBuilder.class) + .withConfiguration(UserConfigurations.of(OkHttpClientComponents.class)); + + @BeforeEach + void init(TestInfo testInfo) { + System.out.println("--------------- Test " + testInfo.getDisplayName()); + } + + @Test + void verifyValidConfiguration() { + runner.run( + ctx -> { + assertThat(ctx).hasSingleBean(SpinnakerRequestInterceptor.class); + assertThat(ctx).hasSingleBean(SpinnakerRequestHeaderInterceptor.class); + assertThat(ctx).hasSingleBean(OkHttpMetricsInterceptor.class); + assertThat(ctx).hasSingleBean(OkHttp3MetricsInterceptor.class); + }); + } +} From 8f17dfec6aa0d77a7fd5a980defab3b142d7a470 Mon Sep 17 00:00:00 2001 From: David Byron Date: Fri, 19 Jan 2024 21:18:23 -0800 Subject: [PATCH 05/10] refactor(web): decouple SpinnakerRequestInterceptor from OkHttpClientConfigurationProperties to pave the way for multiple SpinnakerRequestInterceptor instances, some configured differently than others. --- .../kork/retrofit/RetrofitServiceProviderTest.kt | 2 -- .../kork/retrofit/Retrofit2ServiceProviderTest.kt | 2 +- .../spinnaker/okhttp/SpinnakerRequestInterceptor.groovy | 8 ++++---- .../netflix/spinnaker/config/OkHttpClientComponents.java | 2 +- .../spinnaker/okhttp/SpinnakerRequestInterceptorTest.java | 5 +---- 5 files changed, 7 insertions(+), 12 deletions(-) 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..bc3f930c7 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 @@ -119,5 +119,3 @@ interface Retrofit1Service { fun getSomething(@Path("user") user: String?, callback: Callback?>?) } - - 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..f3bf68213 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 @@ -102,7 +102,7 @@ private open class TestConfiguration { @Bean open fun spinnakerRequestInterceptor(): SpinnakerRequestInterceptor { - return SpinnakerRequestInterceptor(OkHttpClientConfigurationProperties()) + return SpinnakerRequestInterceptor(true) } @Bean 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 index 0e97ddc69..29e62f257 100644 --- a/kork-web/src/main/groovy/com/netflix/spinnaker/okhttp/SpinnakerRequestInterceptor.groovy +++ b/kork-web/src/main/groovy/com/netflix/spinnaker/okhttp/SpinnakerRequestInterceptor.groovy @@ -20,14 +20,14 @@ import com.netflix.spinnaker.security.AuthenticatedRequest import retrofit.RequestInterceptor class SpinnakerRequestInterceptor implements RequestInterceptor { - private final OkHttpClientConfigurationProperties okHttpClientConfigurationProperties + private final boolean propagateSpinnakerHeaders; - SpinnakerRequestInterceptor(OkHttpClientConfigurationProperties okHttpClientConfigurationProperties) { - this.okHttpClientConfigurationProperties = okHttpClientConfigurationProperties + SpinnakerRequestInterceptor(boolean propagateSpinnakerHeaders) { + this.propagateSpinnakerHeaders = propagateSpinnakerHeaders } void intercept(RequestInterceptor.RequestFacade request) { - if (!okHttpClientConfigurationProperties.propagateSpinnakerHeaders) { + if (!propagateSpinnakerHeaders) { // noop return } diff --git a/kork-web/src/main/java/com/netflix/spinnaker/config/OkHttpClientComponents.java b/kork-web/src/main/java/com/netflix/spinnaker/config/OkHttpClientComponents.java index e187e079d..2363ae5e3 100644 --- a/kork-web/src/main/java/com/netflix/spinnaker/config/OkHttpClientComponents.java +++ b/kork-web/src/main/java/com/netflix/spinnaker/config/OkHttpClientComponents.java @@ -82,7 +82,7 @@ public class OkHttpClientComponents { @Bean public SpinnakerRequestInterceptor spinnakerRequestInterceptor() { - return new SpinnakerRequestInterceptor(clientProperties); + return new SpinnakerRequestInterceptor(clientProperties.getPropagateSpinnakerHeaders()); } @Bean diff --git a/kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestInterceptorTest.java b/kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestInterceptorTest.java index 836ca8ea7..41171dde5 100644 --- a/kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestInterceptorTest.java +++ b/kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestInterceptorTest.java @@ -84,11 +84,8 @@ void cleanup() { @ParameterizedTest(name = "propagateSpinnakerHeaders = {0}") @ValueSource(booleans = {false, true}) void propagateSpinnakerHeaders(boolean propagateSpinnakerHeaders) { - OkHttpClientConfigurationProperties okHttpClientConfigurationProperties = - new OkHttpClientConfigurationProperties(); - okHttpClientConfigurationProperties.setPropagateSpinnakerHeaders(propagateSpinnakerHeaders); SpinnakerRequestInterceptor spinnakerRequestInterceptor = - new SpinnakerRequestInterceptor(okHttpClientConfigurationProperties); + new SpinnakerRequestInterceptor(propagateSpinnakerHeaders); RetrofitService retrofitService = makeRetrofitService(wireMock.baseUrl(), spinnakerRequestInterceptor); From 5db0f8d8f796008f0540a265c307d73bac6f62d6 Mon Sep 17 00:00:00 2001 From: David Byron Date: Fri, 19 Jan 2024 21:35:49 -0800 Subject: [PATCH 06/10] feat(web): add ability to skip X-SPINNAKER-ACCOUNTS in SpinnakerRequestInterceptor for uses of retrofit to communicate outside of spinnaker. Skipping this header avoids possible HTTP 431 response codes where http servers aren't configured to receive large request headers when X-SPINNAKER-ACCOUNTS is large. --- .../okhttp/SpinnakerRequestInterceptor.groovy | 15 ++++++++- .../SpinnakerRequestInterceptorTest.java | 32 +++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) 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 index 29e62f257..10e452deb 100644 --- a/kork-web/src/main/groovy/com/netflix/spinnaker/okhttp/SpinnakerRequestInterceptor.groovy +++ b/kork-web/src/main/groovy/com/netflix/spinnaker/okhttp/SpinnakerRequestInterceptor.groovy @@ -16,14 +16,27 @@ package com.netflix.spinnaker.okhttp +import com.netflix.spinnaker.kork.common.Header import com.netflix.spinnaker.security.AuthenticatedRequest import retrofit.RequestInterceptor class SpinnakerRequestInterceptor implements RequestInterceptor { private final boolean propagateSpinnakerHeaders; + /** + * Don't propagate X-SPINNAKER-ACCOUNTS. Only relevant when propagateSpinnakerHeaders is true. + */ + private final boolean skipAccountsHeader; + SpinnakerRequestInterceptor(boolean propagateSpinnakerHeaders) { this.propagateSpinnakerHeaders = propagateSpinnakerHeaders + this.skipAccountsHeader = false + } + + SpinnakerRequestInterceptor(boolean propagateSpinnakerHeaders, + boolean skipAccountsHeader) { + this.propagateSpinnakerHeaders = propagateSpinnakerHeaders + this.skipAccountsHeader = skipAccountsHeader } void intercept(RequestInterceptor.RequestFacade request) { @@ -33,7 +46,7 @@ class SpinnakerRequestInterceptor implements RequestInterceptor { } AuthenticatedRequest.authenticationHeaders.each { String key, Optional value -> - if (value.present) { + if (value.present && (!skipAccountsHeader || !Header.ACCOUNTS.getHeader().equals(key))) { request.addHeader(key, value.get()) } } diff --git a/kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestInterceptorTest.java b/kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestInterceptorTest.java index 41171dde5..15cb88605 100644 --- a/kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestInterceptorTest.java +++ b/kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestInterceptorTest.java @@ -22,6 +22,7 @@ import static com.github.tomakehurst.wiremock.client.WireMock.ok; import static com.github.tomakehurst.wiremock.client.WireMock.urlPathEqualTo; import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig; +import static org.assertj.core.api.Assertions.assertThat; import com.github.tomakehurst.wiremock.junit5.WireMockExtension; import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo; @@ -31,6 +32,7 @@ import java.util.Map; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestInfo; import org.junit.jupiter.api.extension.RegisterExtension; import org.junit.jupiter.params.ParameterizedTest; @@ -110,6 +112,36 @@ void propagateSpinnakerHeaders(boolean propagateSpinnakerHeaders) { }); } + @Test + void skipAccountHeaders() { + SpinnakerRequestInterceptor spinnakerRequestInterceptor = + new SpinnakerRequestInterceptor( + true /* propagateSpinnakerHeaders */, true /* skipAccountsHeader */); + + RetrofitService retrofitService = + makeRetrofitService(wireMock.baseUrl(), spinnakerRequestInterceptor); + + // Add some spinnaker headers to the MDC, including the accounts header + assertThat(TEST_SPINNAKER_HEADERS).containsKey(Header.ACCOUNTS); + TEST_SPINNAKER_HEADERS.forEach(AuthenticatedRequest::set); + + // Make a request + retrofitService.getRequest(); + + // Verify that wiremock received all spinnaker headers except the accounts header + TEST_SPINNAKER_HEADERS.forEach( + (Header header, String value) -> { + RequestPatternBuilder requestPatternBuilder = + getRequestedFor(urlPathEqualTo(REQUEST_PATH)); + if (Header.ACCOUNTS.equals(header)) { + requestPatternBuilder.withoutHeader(header.getHeader()); + } else { + requestPatternBuilder.withHeader(header.getHeader(), equalTo(value)); + } + wireMock.verify(requestPatternBuilder); + }); + } + private RetrofitService makeRetrofitService( String baseUrl, SpinnakerRequestInterceptor spinnakerRequestInterceptor) { return new RestAdapter.Builder() From 4e311e75c98170c94be53b33a7a7b0a063655b3b Mon Sep 17 00:00:00 2001 From: David Byron Date: Fri, 19 Jan 2024 23:01:25 -0800 Subject: [PATCH 07/10] refactor(web/test): convert SpinnakerRequestHeaderInterceptorTest from groovy to java because the less groovy we've got, the better. This also makes converting the test to use WireMock in preference to mockStatic that much easier. While we're at it, move the test to the same package as SpinnakerRequestHeaderInterceptor: com.netflix.spinnaker.okhttp. --- ...innakerRequestHeaderInterceptorTest.groovy | 57 ------------- ...SpinnakerRequestHeaderInterceptorTest.java | 80 +++++++++++++++++++ 2 files changed, 80 insertions(+), 57 deletions(-) delete mode 100644 kork-web/src/test/groovy/com/netflix/spinnaker/kork/web/interceptors/SpinnakerRequestHeaderInterceptorTest.groovy create mode 100644 kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestHeaderInterceptorTest.java diff --git a/kork-web/src/test/groovy/com/netflix/spinnaker/kork/web/interceptors/SpinnakerRequestHeaderInterceptorTest.groovy b/kork-web/src/test/groovy/com/netflix/spinnaker/kork/web/interceptors/SpinnakerRequestHeaderInterceptorTest.groovy deleted file mode 100644 index 59cce3aba..000000000 --- a/kork-web/src/test/groovy/com/netflix/spinnaker/kork/web/interceptors/SpinnakerRequestHeaderInterceptorTest.groovy +++ /dev/null @@ -1,57 +0,0 @@ -/* - * Copyright 2023 OpsMx, 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.kork.web.interceptors - -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 okhttp3.Interceptor -import okhttp3.Request -import org.mockito.MockedStatic -import org.mockito.Mockito -import spock.lang.Specification - -class SpinnakerRequestHeaderInterceptorTest extends Specification { - - def "request contains authorization header"() { - MockedStatic mockAuthenticatedRequest = Mockito.mockStatic(AuthenticatedRequest.class) - Map> authHeaders = new HashMap>(){} - authHeaders.put(Header.USER.getHeader(), Optional.of("some user")) - authHeaders.put(Header.ACCOUNTS.getHeader(), Optional.of("Some ACCOUNTS")) - def okHttpClientConfigurationProperties = new OkHttpClientConfigurationProperties(); - def headerInterceptor = new SpinnakerRequestHeaderInterceptor(okHttpClientConfigurationProperties) - def chain = Mock(Interceptor.Chain) { - request() >> new Request.Builder() - .url("http://1.1.1.1/heath-check") - .build() - } - - Mockito.when(AuthenticatedRequest.authenticationHeaders) - .thenReturn(authHeaders) - - when: "running the interceptor under test" - headerInterceptor.intercept(chain) - - then: "the expected authorization header is added to the request before proceeding" - 1 * chain.proceed({ Request request -> request.headers(Header.USER.getHeader()) == ["some user"] && - request.headers(Header.ACCOUNTS.getHeader()) == ["Some ACCOUNTS"]}) - - cleanup: - mockAuthenticatedRequest.close() - } -} diff --git a/kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestHeaderInterceptorTest.java b/kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestHeaderInterceptorTest.java new file mode 100644 index 000000000..9b39e682d --- /dev/null +++ b/kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestHeaderInterceptorTest.java @@ -0,0 +1,80 @@ +/* + * Copyright 2023 OpsMx, 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 static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import com.netflix.spinnaker.kork.common.Header; +import com.netflix.spinnaker.security.AuthenticatedRequest; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import okhttp3.Interceptor; +import okhttp3.Request; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; +import org.mockito.MockedStatic; +import org.mockito.Mockito; + +class SpinnakerRequestHeaderInterceptorTest { + + MockedStatic mockAuthenticatedRequest; + + @BeforeEach + void setup() { + mockAuthenticatedRequest = Mockito.mockStatic(AuthenticatedRequest.class); + } + + @AfterEach + void cleanup() { + mockAuthenticatedRequest.close(); + } + + @Test + void requestContainsAuthorizationHeader() throws Exception { + // given + Map> authHeaders = new HashMap>() {}; + authHeaders.put(Header.USER.getHeader(), Optional.of("some user")); + authHeaders.put(Header.ACCOUNTS.getHeader(), Optional.of("Some ACCOUNTS")); + OkHttpClientConfigurationProperties okHttpClientConfigurationProperties = + new OkHttpClientConfigurationProperties(); + SpinnakerRequestHeaderInterceptor headerInterceptor = + new SpinnakerRequestHeaderInterceptor(okHttpClientConfigurationProperties); + Interceptor.Chain chain = mock(Interceptor.Chain.class); + + when(chain.request()) + .thenReturn(new Request.Builder().url("http://1.1.1.1/heath-check").build()); + + when(AuthenticatedRequest.getAuthenticationHeaders()).thenReturn(authHeaders); + + // when: "running the interceptor under test" + headerInterceptor.intercept(chain); + + // then: "the expected authorization header is added to the request before proceeding" + ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(Request.class); + verify(chain).proceed(requestCaptor.capture()); + Request request = requestCaptor.getValue(); + assertThat(request.headers(Header.USER.getHeader())).isEqualTo(List.of("some user")); + assertThat(request.headers(Header.ACCOUNTS.getHeader())).isEqualTo(List.of("Some ACCOUNTS")); + } +} From f6ffeb9fffcad23855c3a387c6cc465edf42bbd0 Mon Sep 17 00:00:00 2001 From: David Byron Date: Fri, 19 Jan 2024 23:35:39 -0800 Subject: [PATCH 08/10] refactor(web/test): convert SpinnakerRequestHeaderInterceptorTest to use wiremock and real requests, instead of static mocking of AuthenticatedRequest since it seems like a more thorough test. Matching the structure of SpinnakerRequestInterceptorTest also seems like a win. --- kork-web/kork-web.gradle | 1 + ...SpinnakerRequestHeaderInterceptorTest.java | 125 +++++++++++++----- .../org.mockito.plugins.MockMaker | 1 - 3 files changed, 90 insertions(+), 37 deletions(-) delete mode 100644 kork-web/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker diff --git a/kork-web/kork-web.gradle b/kork-web/kork-web.gradle index f8eae0fff..55d4c224b 100644 --- a/kork-web/kork-web.gradle +++ b/kork-web/kork-web.gradle @@ -46,6 +46,7 @@ dependencies { testImplementation "com.github.tomakehurst:wiremock-jre8-standalone" testImplementation "ch.qos.logback:logback-classic" testImplementation "ch.qos.logback:logback-core" + testImplementation "com.squareup.retrofit2:retrofit" testImplementation "org.junit.jupiter:junit-jupiter-params" testImplementation "org.spockframework:spock-core" testImplementation "org.spockframework:spock-spring" diff --git a/kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestHeaderInterceptorTest.java b/kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestHeaderInterceptorTest.java index 9b39e682d..31215582d 100644 --- a/kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestHeaderInterceptorTest.java +++ b/kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestHeaderInterceptorTest.java @@ -16,65 +16,118 @@ package com.netflix.spinnaker.okhttp; -import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; +import static com.github.tomakehurst.wiremock.client.WireMock.equalTo; +import static com.github.tomakehurst.wiremock.client.WireMock.get; +import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor; +import static com.github.tomakehurst.wiremock.client.WireMock.ok; +import static com.github.tomakehurst.wiremock.client.WireMock.urlPathEqualTo; +import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig; +import com.github.tomakehurst.wiremock.junit5.WireMockExtension; +import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo; +import com.github.tomakehurst.wiremock.matching.RequestPatternBuilder; import com.netflix.spinnaker.kork.common.Header; import com.netflix.spinnaker.security.AuthenticatedRequest; -import java.util.HashMap; -import java.util.List; import java.util.Map; -import java.util.Optional; -import okhttp3.Interceptor; -import okhttp3.Request; +import okhttp3.OkHttpClient; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; -import org.mockito.ArgumentCaptor; -import org.mockito.MockedStatic; -import org.mockito.Mockito; +import org.junit.jupiter.api.TestInfo; +import org.junit.jupiter.api.extension.RegisterExtension; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import retrofit2.Call; +import retrofit2.Retrofit; +import retrofit2.http.GET; class SpinnakerRequestHeaderInterceptorTest { - MockedStatic mockAuthenticatedRequest; + public static final String REQUEST_PATH = "/foo"; + + public static final String TEST_USER = "some-user"; + + public static final String TEST_ACCOUNTS = "some-accounts"; + + public static final String TEST_REQUEST_ID = "some-request-id"; + + public static final Map TEST_SPINNAKER_HEADERS = + Map.of( + Header.USER, + TEST_USER, + Header.ACCOUNTS, + TEST_ACCOUNTS, + Header.REQUEST_ID, + TEST_REQUEST_ID); + + /** + * Use this instead of annotating the class with @WireMockTest so there's a WireMock object + * available for parameterized tests. Otherwise the arguments from e.g. ValueSource compete with + * the WireMockRuntimeInfo argument that @WireMockTest provides. + */ + @RegisterExtension + static WireMockExtension wireMock = + WireMockExtension.newInstance().options(wireMockConfig().dynamicPort()).build(); @BeforeEach - void setup() { - mockAuthenticatedRequest = Mockito.mockStatic(AuthenticatedRequest.class); + void setup(TestInfo testInfo, WireMockRuntimeInfo wmRuntimeInfo) { + System.out.println("--------------- Test " + testInfo.getDisplayName()); + + // set up an arbitrary response to avoid 404s, and so it's possible to + // verify the request headers wiremock receives. + wireMock.stubFor(get(REQUEST_PATH).willReturn(ok())); } @AfterEach void cleanup() { - mockAuthenticatedRequest.close(); + AuthenticatedRequest.clear(); } - @Test - void requestContainsAuthorizationHeader() throws Exception { - // given - Map> authHeaders = new HashMap>() {}; - authHeaders.put(Header.USER.getHeader(), Optional.of("some user")); - authHeaders.put(Header.ACCOUNTS.getHeader(), Optional.of("Some ACCOUNTS")); + @ParameterizedTest(name = "propagateSpinnakerHeaders = {0}") + @ValueSource(booleans = {false, true}) + void propagateSpinnakerHeaders(boolean propagateSpinnakerHeaders) throws Exception { OkHttpClientConfigurationProperties okHttpClientConfigurationProperties = new OkHttpClientConfigurationProperties(); - SpinnakerRequestHeaderInterceptor headerInterceptor = + okHttpClientConfigurationProperties.setPropagateSpinnakerHeaders(propagateSpinnakerHeaders); + SpinnakerRequestHeaderInterceptor spinnakerRequestHeaderInterceptor = new SpinnakerRequestHeaderInterceptor(okHttpClientConfigurationProperties); - Interceptor.Chain chain = mock(Interceptor.Chain.class); - when(chain.request()) - .thenReturn(new Request.Builder().url("http://1.1.1.1/heath-check").build()); + RetrofitService retrofitService = + makeRetrofitService(wireMock.baseUrl(), spinnakerRequestHeaderInterceptor); - when(AuthenticatedRequest.getAuthenticationHeaders()).thenReturn(authHeaders); + // Add some spinnaker headers to the MDC + TEST_SPINNAKER_HEADERS.forEach(AuthenticatedRequest::set); - // when: "running the interceptor under test" - headerInterceptor.intercept(chain); + // Make a request + retrofitService.getRequest().execute(); + + // Verify that wiremock did/didn't receive the spinnaker headers as appropriate + TEST_SPINNAKER_HEADERS.forEach( + (Header header, String value) -> { + RequestPatternBuilder requestPatternBuilder = + getRequestedFor(urlPathEqualTo(REQUEST_PATH)); + if (propagateSpinnakerHeaders) { + requestPatternBuilder.withHeader(header.getHeader(), equalTo(value)); + } else { + requestPatternBuilder.withoutHeader(header.getHeader()); + } + wireMock.verify(requestPatternBuilder); + }); + } + + private RetrofitService makeRetrofitService( + String baseUrl, SpinnakerRequestHeaderInterceptor spinnakerRequestHeaderInterceptor) { + OkHttpClient okHttpClient = + new OkHttpClient.Builder().addInterceptor(spinnakerRequestHeaderInterceptor).build(); + + return new Retrofit.Builder() + .baseUrl(baseUrl) + .client(okHttpClient) + .build() + .create(RetrofitService.class); + } - // then: "the expected authorization header is added to the request before proceeding" - ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(Request.class); - verify(chain).proceed(requestCaptor.capture()); - Request request = requestCaptor.getValue(); - assertThat(request.headers(Header.USER.getHeader())).isEqualTo(List.of("some user")); - assertThat(request.headers(Header.ACCOUNTS.getHeader())).isEqualTo(List.of("Some ACCOUNTS")); + interface RetrofitService { + @GET(REQUEST_PATH) + Call getRequest(); } } diff --git a/kork-web/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker b/kork-web/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker deleted file mode 100644 index 1f0955d45..000000000 --- a/kork-web/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker +++ /dev/null @@ -1 +0,0 @@ -mock-maker-inline From 14dce34948116f6f00b242db2dfed1e53a813ff2 Mon Sep 17 00:00:00 2001 From: David Byron Date: Fri, 19 Jan 2024 23:41:33 -0800 Subject: [PATCH 09/10] refactor(web): decouple SpinnakerRequestHeaderInterceptor from OkHttpClientConfigurationProperties to pave the way for multiple SpinnakerRequestHeaderInterceptor instances, some configured differently than others. --- .../netflix/spinnaker/config/OkHttpClientComponents.java | 2 +- .../okhttp/SpinnakerRequestHeaderInterceptor.java | 9 ++++----- .../okhttp/SpinnakerRequestHeaderInterceptorTest.java | 5 +---- 3 files changed, 6 insertions(+), 10 deletions(-) diff --git a/kork-web/src/main/java/com/netflix/spinnaker/config/OkHttpClientComponents.java b/kork-web/src/main/java/com/netflix/spinnaker/config/OkHttpClientComponents.java index 2363ae5e3..796bfe144 100644 --- a/kork-web/src/main/java/com/netflix/spinnaker/config/OkHttpClientComponents.java +++ b/kork-web/src/main/java/com/netflix/spinnaker/config/OkHttpClientComponents.java @@ -87,7 +87,7 @@ public SpinnakerRequestInterceptor spinnakerRequestInterceptor() { @Bean public SpinnakerRequestHeaderInterceptor spinnakerRequestHeaderInterceptor() { - return new SpinnakerRequestHeaderInterceptor(clientProperties); + return new SpinnakerRequestHeaderInterceptor(clientProperties.getPropagateSpinnakerHeaders()); } @Bean diff --git a/kork-web/src/main/java/com/netflix/spinnaker/okhttp/SpinnakerRequestHeaderInterceptor.java b/kork-web/src/main/java/com/netflix/spinnaker/okhttp/SpinnakerRequestHeaderInterceptor.java index f10605b02..8dc85ed95 100644 --- a/kork-web/src/main/java/com/netflix/spinnaker/okhttp/SpinnakerRequestHeaderInterceptor.java +++ b/kork-web/src/main/java/com/netflix/spinnaker/okhttp/SpinnakerRequestHeaderInterceptor.java @@ -28,17 +28,16 @@ */ public class SpinnakerRequestHeaderInterceptor implements Interceptor { - private final OkHttpClientConfigurationProperties okHttpClientConfigurationProperties; + private final boolean propagateSpinnakerHeaders; - public SpinnakerRequestHeaderInterceptor( - OkHttpClientConfigurationProperties okHttpClientConfigurationProperties) { - this.okHttpClientConfigurationProperties = okHttpClientConfigurationProperties; + public SpinnakerRequestHeaderInterceptor(boolean propagateSpinnakerHeaders) { + this.propagateSpinnakerHeaders = propagateSpinnakerHeaders; } @Override public Response intercept(Chain chain) throws IOException { Request.Builder builder = chain.request().newBuilder(); - if (!okHttpClientConfigurationProperties.getPropagateSpinnakerHeaders()) { + if (!propagateSpinnakerHeaders) { return chain.proceed(builder.build()); } diff --git a/kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestHeaderInterceptorTest.java b/kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestHeaderInterceptorTest.java index 31215582d..e458cb574 100644 --- a/kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestHeaderInterceptorTest.java +++ b/kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestHeaderInterceptorTest.java @@ -85,11 +85,8 @@ void cleanup() { @ParameterizedTest(name = "propagateSpinnakerHeaders = {0}") @ValueSource(booleans = {false, true}) void propagateSpinnakerHeaders(boolean propagateSpinnakerHeaders) throws Exception { - OkHttpClientConfigurationProperties okHttpClientConfigurationProperties = - new OkHttpClientConfigurationProperties(); - okHttpClientConfigurationProperties.setPropagateSpinnakerHeaders(propagateSpinnakerHeaders); SpinnakerRequestHeaderInterceptor spinnakerRequestHeaderInterceptor = - new SpinnakerRequestHeaderInterceptor(okHttpClientConfigurationProperties); + new SpinnakerRequestHeaderInterceptor(propagateSpinnakerHeaders); RetrofitService retrofitService = makeRetrofitService(wireMock.baseUrl(), spinnakerRequestHeaderInterceptor); From 52c5781e48f8c65898297a3856f9ab2647340dc3 Mon Sep 17 00:00:00 2001 From: David Byron Date: Fri, 19 Jan 2024 23:50:45 -0800 Subject: [PATCH 10/10] feat(web): add ability to skip X-SPINNAKER-ACCOUNTS in SpinnakerRequestHeaderInterceptor for uses of retrofit2 to communicate outside of spinnaker. Skipping this header avoids possible HTTP 431 response codes where http servers aren't configured to receive large request headers when X-SPINNAKER-ACCOUNTS is large. --- .../SpinnakerRequestHeaderInterceptor.java | 14 +++++++- ...SpinnakerRequestHeaderInterceptorTest.java | 32 +++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/kork-web/src/main/java/com/netflix/spinnaker/okhttp/SpinnakerRequestHeaderInterceptor.java b/kork-web/src/main/java/com/netflix/spinnaker/okhttp/SpinnakerRequestHeaderInterceptor.java index 8dc85ed95..350ecb6ee 100644 --- a/kork-web/src/main/java/com/netflix/spinnaker/okhttp/SpinnakerRequestHeaderInterceptor.java +++ b/kork-web/src/main/java/com/netflix/spinnaker/okhttp/SpinnakerRequestHeaderInterceptor.java @@ -16,6 +16,7 @@ package com.netflix.spinnaker.okhttp; +import com.netflix.spinnaker.kork.common.Header; import com.netflix.spinnaker.security.AuthenticatedRequest; import java.io.IOException; import okhttp3.Interceptor; @@ -30,8 +31,18 @@ public class SpinnakerRequestHeaderInterceptor implements Interceptor { private final boolean propagateSpinnakerHeaders; + /** Don't propagate X-SPINNAKER-ACCOUNTS. Only relevant when propagateSpinnakerHeaders is true. */ + private final boolean skipAccountsHeader; + public SpinnakerRequestHeaderInterceptor(boolean propagateSpinnakerHeaders) { this.propagateSpinnakerHeaders = propagateSpinnakerHeaders; + this.skipAccountsHeader = false; + } + + public SpinnakerRequestHeaderInterceptor( + boolean propagateSpinnakerHeaders, boolean skipAccountsHeader) { + this.propagateSpinnakerHeaders = propagateSpinnakerHeaders; + this.skipAccountsHeader = skipAccountsHeader; } @Override @@ -44,7 +55,8 @@ public Response intercept(Chain chain) throws IOException { AuthenticatedRequest.getAuthenticationHeaders() .forEach( (key, value) -> { - if (value.isPresent()) { + if (value.isPresent() + && (!skipAccountsHeader || !Header.ACCOUNTS.getHeader().equals(key))) { builder.addHeader(key, value.get()); } }); diff --git a/kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestHeaderInterceptorTest.java b/kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestHeaderInterceptorTest.java index e458cb574..2d591f954 100644 --- a/kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestHeaderInterceptorTest.java +++ b/kork-web/src/test/java/com/netflix/spinnaker/okhttp/SpinnakerRequestHeaderInterceptorTest.java @@ -22,6 +22,7 @@ import static com.github.tomakehurst.wiremock.client.WireMock.ok; import static com.github.tomakehurst.wiremock.client.WireMock.urlPathEqualTo; import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig; +import static org.assertj.core.api.Assertions.assertThat; import com.github.tomakehurst.wiremock.junit5.WireMockExtension; import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo; @@ -32,6 +33,7 @@ import okhttp3.OkHttpClient; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestInfo; import org.junit.jupiter.api.extension.RegisterExtension; import org.junit.jupiter.params.ParameterizedTest; @@ -111,6 +113,36 @@ void propagateSpinnakerHeaders(boolean propagateSpinnakerHeaders) throws Excepti }); } + @Test + void skipAccountHeaders() throws Exception { + SpinnakerRequestHeaderInterceptor spinnakerRequestHeaderInterceptor = + new SpinnakerRequestHeaderInterceptor( + true /* propagateSpinnakerHeaders */, true /* skipAccountsHeader */); + + RetrofitService retrofitService = + makeRetrofitService(wireMock.baseUrl(), spinnakerRequestHeaderInterceptor); + + // Add some spinnaker headers to the MDC, including the accounts header + assertThat(TEST_SPINNAKER_HEADERS).containsKey(Header.ACCOUNTS); + TEST_SPINNAKER_HEADERS.forEach(AuthenticatedRequest::set); + + // Make a request + retrofitService.getRequest().execute(); + + // Verify that wiremock received all spinnaker headers except the accounts header + TEST_SPINNAKER_HEADERS.forEach( + (Header header, String value) -> { + RequestPatternBuilder requestPatternBuilder = + getRequestedFor(urlPathEqualTo(REQUEST_PATH)); + if (Header.ACCOUNTS.equals(header)) { + requestPatternBuilder.withoutHeader(header.getHeader()); + } else { + requestPatternBuilder.withHeader(header.getHeader(), equalTo(value)); + } + wireMock.verify(requestPatternBuilder); + }); + } + private RetrofitService makeRetrofitService( String baseUrl, SpinnakerRequestHeaderInterceptor spinnakerRequestHeaderInterceptor) { OkHttpClient okHttpClient =