From cf1bcaa69088c84ec3734f1ea952d199d11e91d3 Mon Sep 17 00:00:00 2001 From: David Byron <82477955+dbyron-sf@users.noreply.github.com> Date: Tue, 10 Oct 2023 18:27:06 -0400 Subject: [PATCH 01/32] fix(core/test): remove runtime dependency on org.slf4j:slf4j-simple (#1107) since it results in these errors during testing: SLF4J: Class path contains multiple SLF4J bindings. SLF4J: Found binding in [jar:file:/Users/dbyron/.gradle/caches/modules-2/files-2.1/org.slf4j/slf4j-simple/1.7.36/a41f9cfe6faafb2eb83a1c7dd2d0dfd844e2a936/slf4j-simple-1.7.36.jar!/org/slf4j/impl/StaticLoggerBinder.class] SLF4J: Found binding in [jar:file:/Users/dbyron/.gradle/caches/modules-2/files-2.1/ch.qos.logback/logback-classic/1.2.12/d4dee19148dccb177a0736eb2027bd195341da78/logback-classic-1.2.12.jar!/org/slf4j/impl/StaticLoggerBinder.class] SLF4J: See http://www.slf4j.org/codes.html#multiple_bindings for an explanation. SLF4J: Actual binding is of type [org.slf4j.impl.SimpleLoggerFactory] --- kork-core/kork-core.gradle | 1 - 1 file changed, 1 deletion(-) diff --git a/kork-core/kork-core.gradle b/kork-core/kork-core.gradle index 064452767..c3146d1e6 100644 --- a/kork-core/kork-core.gradle +++ b/kork-core/kork-core.gradle @@ -30,6 +30,5 @@ dependencies { testImplementation "org.spockframework:spock-core" testRuntimeOnly "cglib:cglib-nodep" testRuntimeOnly "org.objenesis:objenesis" - testRuntimeOnly "org.slf4j:slf4j-simple" testRuntimeOnly "org.junit.jupiter:junit-jupiter-engine" } From a3a0f60091036eab6bf5afe8d217c37c7cb44fd6 Mon Sep 17 00:00:00 2001 From: Sandesh <30489233+j-sandy@users.noreply.github.com> Date: Wed, 18 Oct 2023 00:52:03 +0530 Subject: [PATCH 02/32] chore(dependency): upgrade to groovy 3.x and related cleanup (#1105) Removed constraints of: org.apache.ant because groovy-ant:3.0.10 (transitively introduced by spring boot 2.5.14) brings ant:1.10.12. https://mvnrepository.com/artifact/org.codehaus.groovy/groovy-ant/3.0.10 org.testng:testng:7.4.0 because groovy-testng:3.0.10 brings testng:7.5 https://mvnrepository.com/artifact/org.codehaus.groovy/groovy-testng/3.0.10 io.rest-assured 4.2.0 --- build.gradle | 6 +-- .../spinnaker-dependencies.gradle | 44 +------------------ 2 files changed, 3 insertions(+), 47 deletions(-) diff --git a/build.gradle b/build.gradle index 39c639391..045fb39d3 100644 --- a/build.gradle +++ b/build.gradle @@ -31,14 +31,12 @@ subprojects { if (it.name != "kork-bom" && it.name != "spinnaker-dependencies") { apply plugin: 'java-library' test { - useJUnitPlatform { - includeEngines "spek2", "junit-vintage", "junit-jupiter" - } + useJUnitPlatform() } dependencies { annotationProcessor(platform(project(":spinnaker-dependencies"))) annotationProcessor("org.springframework.boot:spring-boot-configuration-processor") - testRuntimeOnly("org.junit.vintage:junit-vintage-engine") + testRuntimeOnly "org.junit.jupiter:junit-jupiter-engine" } } } diff --git a/spinnaker-dependencies/spinnaker-dependencies.gradle b/spinnaker-dependencies/spinnaker-dependencies.gradle index fe3a4fbb3..f6d84af94 100644 --- a/spinnaker-dependencies/spinnaker-dependencies.gradle +++ b/spinnaker-dependencies/spinnaker-dependencies.gradle @@ -12,7 +12,6 @@ ext { bouncycastle : "1.70", brave : "5.12.3", gcp : "25.3.0", - groovy : "2.5.15", jooq : "3.13.6", jsch : "0.1.54", jschAgentProxy : "0.0.9", @@ -24,11 +23,6 @@ ext { okhttp : "2.7.5", // CVE-2016-2402 okhttp3 : "4.9.3", openapi : "1.3.9", // this needs to be kept in sync with spring boot as it pulls in the spring-boot-dependencies BOM - //Spring boot 2.5.14 upgrade brings io.rest-assured 4.3.3 as transitive dependency. - //io.rest-assured 4.3.x require groovy 3.0.2. So, pinning the nearest version - //using groovy 2.x. This can be removed with groovy 3.x upgrade. - //[https://github.com/rest-assured/rest-assured/blob/9b683130c93188cabdef850e89d0c9417d847a17/changelog.txt#L200] - restassured : "4.2.0", retrofit : "1.9.0", retrofit2 : "2.8.1", spectator : "1.0.6", @@ -60,14 +54,6 @@ dependencies { // 2.16.0 is subject to CVE-2021-45105. 2.17.0 is subject to CVE-2021-44832, so use >= 2.17.1. api(platform("org.apache.logging.log4j:log4j-bom:2.20.0")) - //Upgrade of spring boot 2.5.x brings groovy 3.x as transitive dependency. - //To avoid transitive upgrade of groovy, pinning it with enforcedPlatform() closure. - //It forces version for internal submodules of kork as well as for all the consumer spinnaker services. - api(enforcedPlatform("org.codehaus.groovy:groovy-bom")){ - version { - strictly "${versions.groovy}" - } - } api(platform("org.jetbrains.kotlinx:kotlinx-coroutines-bom:1.4.3")) //kotlinVersion comes from gradle.properties since we have kotlin code in // this project and need to configure gradle plugins etc. @@ -82,7 +68,7 @@ dependencies { api(platform("com.google.cloud:google-cloud-secretmanager:2.1.7")) api(platform("org.springframework.cloud:spring-cloud-dependencies:${versions.springCloud}")) api(platform("io.strikt:strikt-bom:0.31.0")) - api(platform("org.spockframework:spock-bom:1.3-groovy-2.5")) + api(platform("org.spockframework:spock-bom:2.0-groovy-3.0")) api(platform("com.oracle.oci.sdk:oci-java-sdk-bom:1.5.17")) api(platform("org.testcontainers:testcontainers-bom:1.16.2")) api(platform("io.arrow-kt:arrow-stack:${versions.arrow}")) @@ -170,26 +156,6 @@ dependencies { api("de.huxhorn.sulky:de.huxhorn.sulky.ulid:8.2.0") api("dev.minutest:minutest:1.13.0") api("io.mockk:mockk:1.10.5") - api("io.rest-assured:rest-assured"){ - version { - strictly "${versions.restassured}" - } - } - api("io.rest-assured:rest-assured-common"){ - version { - strictly "${versions.restassured}" - } - } - api("io.rest-assured:json-path"){ - version { - strictly "${versions.restassured}" - } - } - api("io.rest-assured:xml-path"){ - version { - strictly "${versions.restassured}" - } - } api("io.springfox:springfox-swagger-ui:${versions.springfoxSwagger}") api("io.springfox:springfox-swagger2:${versions.springfoxSwagger}") api("io.swagger:swagger-annotations:${versions.swagger}") @@ -246,17 +212,9 @@ dependencies { api("org.springframework.security.oauth.boot:spring-security-oauth2-autoconfigure:2.3.12.RELEASE") api("org.springframework.security.extensions:spring-security-saml-dsl-core:1.0.5.RELEASE") api("org.springframework.security.extensions:spring-security-saml2-core:1.0.9.RELEASE") - api("org.testng:testng:7.4.0") // TODO: remove this with upgrade of spring-boot version to 2.5.0 or with upgrade of groovy-all to 3.0.8 api("org.threeten:threeten-extra:1.0") api("org.apache.tomcat.embed:tomcat-embed-core:${versions.tomcat}") api("org.apache.tomcat.embed:tomcat-embed-el:${versions.tomcat}") api("org.apache.tomcat.embed:tomcat-embed-websocket:${versions.tomcat}") - api('org.apache.ant:ant:1.10.12') { - because "1.9.15 is resolved transitively through Groovy, removes CVE-2021-36374, CVE-2021-36373, CVE-2020-11979, CVE-2020-15250" - } - api('org.apache.ant:ant-launcher:1.10.12'){ - because "1.9.15 is resolved transitively through Groovy, removes CVE-2021-36374, CVE-2021-36373, CVE-2020-11979, CVE-2020-15250" - } - } } From 8433e316bdcc7611c8bd64e5611c84ca7fedebb7 Mon Sep 17 00:00:00 2001 From: Pranav Bhaskaran <67958542+Pranav-b-7@users.noreply.github.com> Date: Fri, 20 Oct 2023 20:49:29 +0530 Subject: [PATCH 03/32] feat(retrofit2): Handled http response bodies for http errors that are not json. (#1104) * feat(retrofit2): handled http response bodies for http errors that are not json . Handled this with the default catch block as the way of handling remains same for all the types of exceptions * test(retrofit2): Test case to verify the logic to handle http response bodies for http errors that are not json * feat(retrofit2): Made changes as per the review comments. Returning null when an exception is thrown while parsing an HTTP error response bodyunder SpinnakerHttpException class * test(retrofit2): Made changes in the test cases as per the review comments. The method name is now modified to : testNonJsonHttpErrorResponse and assertions are modified to verify if the response is returned as null in case of any exceptions * feat(retrofit2): Corrected the javadoc statements for the method getErrorBodyAs in SpinnakerHttpException class and also modified the exception log message to print status code, url and the stack trace to make it more useful * feat(retrofit2): Modified the error log statement to print the http method * test(retrofit2): Added test case to invoke the SpinnakerHttpException constructor with retrofit2 parameters to verify parse failure of non json http error response body * test(retrofit2): Modified the import statements to import only required classes * test(retrofit2): Modified the test case name of NonJsonhttpErrorResponseBody to testSpinnakerHttpExceptionRetrofit2NonJsonHttpErrorResponse * chore(retrofit): results of spotlessApply * Reverted - test(retrofit2): Added test case to invoke the SpinnakerHttpException constructor with retrofit2 parameters to verify parse failure of non json http error response * test(retrofit2): Formatted the code * test(retrofit2): Modified testNonJsonHttpErrorResponse to assert more values like url, response code, reason and message * spotlessApply --------- Co-authored-by: Pranav-b-7 Co-authored-by: David Byron --- .../exceptions/SpinnakerHttpException.java | 17 ++++++++----- .../SpinnakerRetrofit2ErrorHandleTest.java | 25 ++++++++++++++++++- 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerHttpException.java b/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerHttpException.java index a67d561b5..355aea70e 100644 --- a/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerHttpException.java +++ b/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerHttpException.java @@ -18,7 +18,6 @@ import com.google.common.base.Preconditions; import com.netflix.spinnaker.kork.annotations.NullableByDefault; -import java.io.IOException; import java.lang.annotation.Annotation; import java.util.HashMap; import java.util.Map; @@ -219,10 +218,10 @@ public String getReason() { } /** - * HTTP response body converted to specified {@code type}. {@code null} if there is no response. + * HTTP error response body converted to the specified {@code type}. * - * @throws RuntimeException wrapping the underlying IOException if unable to convert the body to - * the specified {@code type}. + * @return null if there's no response or unable to convert the body to the specified {@code + * type}. */ private Map getErrorBodyAs(Retrofit retrofit) { if (retrofit2Response == null) { @@ -233,8 +232,14 @@ private Map getErrorBodyAs(Retrofit retrofit) { retrofit.responseBodyConverter(Map.class, new Annotation[0]); try { return converter.convert(retrofit2Response.errorBody()); - } catch (IOException e) { - throw new RuntimeException(e); + } catch (Exception e) { + log.debug( + "unable to convert response to map ({} {}, {})", + retrofit2Response.raw().request().method(), + retrofit2Response.code(), + retrofit2Response.raw().request().url(), + e); + return null; } } } diff --git a/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofit2ErrorHandleTest.java b/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofit2ErrorHandleTest.java index 75756395b..2da42fb8b 100644 --- a/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofit2ErrorHandleTest.java +++ b/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofit2ErrorHandleTest.java @@ -48,6 +48,8 @@ public class SpinnakerRetrofit2ErrorHandleTest { private static String responseBodyString; + private static String baseUrl = mockWebServer.url("/").toString(); + @BeforeAll public static void setupOnce() throws Exception { @@ -58,7 +60,7 @@ public static void setupOnce() throws Exception { retrofit2Service = new Retrofit.Builder() - .baseUrl(mockWebServer.url("/").toString()) + .baseUrl(baseUrl) .client( new OkHttpClient.Builder() .callTimeout(1, TimeUnit.SECONDS) @@ -193,6 +195,27 @@ void testSpinnakerConversionException() { assertEquals("Failed to process response body", spinnakerConversionException.getMessage()); } + @Test + public void testNonJsonHttpErrorResponse() { + + String invalidJsonTypeResponseBody = "{'errorResponse': 'Failure'"; + int responseCode = HttpStatus.INTERNAL_SERVER_ERROR.value(); + String url = baseUrl + "retrofit2"; + String reason = "Server Error"; + + mockWebServer.enqueue( + new MockResponse().setResponseCode(responseCode).setBody(invalidJsonTypeResponseBody)); + SpinnakerHttpException spinnakerHttpException = + assertThrows(SpinnakerHttpException.class, () -> retrofit2Service.getRetrofit2().execute()); + assertNull(spinnakerHttpException.getResponseBody()); + assertEquals(responseCode, spinnakerHttpException.getResponseCode()); + assertEquals( + "Status: " + responseCode + ", URL: " + url + ", Message: " + reason, + spinnakerHttpException.getMessage()); + assertEquals(url, spinnakerHttpException.getUrl()); + assertEquals(reason, spinnakerHttpException.getReason()); + } + interface DummyWithExecute { void execute(); } From c90aabb693cd54fb4401ec2be403e30f9ed14c51 Mon Sep 17 00:00:00 2001 From: David Byron <82477955+dbyron-sf@users.noreply.github.com> Date: Sun, 29 Oct 2023 19:16:08 -0400 Subject: [PATCH 04/32] feat(retrofit): move url attribute from SpinnakerHttpException to SpinnakerServerException (#1110) * refactor(retrofit/test): cleanup tests in kork-retrofit to use fluent assertions and other small tweaks * test(retrofit): add assertions on SpinnakerHttpException's url to prepare to move url to SpinnakerServerException * refactor(retrofit/test): remove duplicate test from SpinnakerRetrofit2ErrorHandleTest testRetrofitSimpleSpinnakerServerException actually generates a SpinnakerNetworkException and so it's effectively the same test as testRetrofitSimpleSpinnakerNetworkException. * test(retrofit): test behavior when a retrofit endpoint generates a SpinnakerNetworkException * feat(retrofit): move url attribute from SpinnakerHttpException to SpinnakerServerException so it's available to exception handling code that currently has access to the url via RetrofitError. * refactor(retrofit): change Spinnaker*Exception constructors to take a Request instead of a url, to pave the way for storing other attributes of requests (e.g. the http request method) * fix(retrofit): add javadoc, and properly set retryable to false in the new SpinnakerConversionException constructors * refactor(retrofit): use @Getter for url in SpinnakerServerException --- ...rorHandlingExecutorCallAdapterFactory.java | 11 +- .../kork/retrofit/Retrofit2SyncCall.java | 2 +- .../SpinnakerConversionException.java | 24 +++- .../exceptions/SpinnakerHttpException.java | 14 +- .../exceptions/SpinnakerNetworkException.java | 23 +++- .../SpinnakerRetrofitErrorHandler.java | 6 +- .../exceptions/SpinnakerServerException.java | 45 +++++- .../exceptions/Retrofit2SyncCallTest.java | 45 +++--- .../SpinnakerHttpExceptionTest.java | 30 ++-- .../SpinnakerRetrofit2ErrorHandleTest.java | 128 +++++++++--------- .../SpinnakerRetrofitErrorHandlerTest.java | 100 ++++++++------ ...pinnakerRetrofitExceptionHandlersTest.java | 44 +++--- .../SpinnakerServerExceptionTest.java | 68 +++++++--- 13 files changed, 334 insertions(+), 206 deletions(-) diff --git a/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/ErrorHandlingExecutorCallAdapterFactory.java b/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/ErrorHandlingExecutorCallAdapterFactory.java index 81ebacb99..ec3aec12a 100644 --- a/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/ErrorHandlingExecutorCallAdapterFactory.java +++ b/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/ErrorHandlingExecutorCallAdapterFactory.java @@ -152,11 +152,12 @@ public Response execute() { return syncResp; } } catch (JsonProcessingException jpe) { - throw new SpinnakerConversionException("Failed to process response body", jpe); + throw new SpinnakerConversionException( + "Failed to process response body", jpe, delegate.request()); } catch (IOException e) { - throw new SpinnakerNetworkException(e); + throw new SpinnakerNetworkException(e, delegate.request()); } catch (Exception e) { - throw new SpinnakerServerException(e); + throw new SpinnakerServerException(e, delegate.request()); } throw new SpinnakerHttpException(syncResp, retrofit); } @@ -250,11 +251,11 @@ public void onFailure(Call call, final Throwable t) { SpinnakerServerException exception; if (t instanceof IOException) { - exception = new SpinnakerNetworkException(t); + exception = new SpinnakerNetworkException(t, call.request()); } else if (t instanceof SpinnakerHttpException) { exception = (SpinnakerHttpException) t; } else { - exception = new SpinnakerServerException(t); + exception = new SpinnakerServerException(t, call.request()); } final SpinnakerServerException finalException = exception; callbackExecutor.execute( diff --git a/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/Retrofit2SyncCall.java b/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/Retrofit2SyncCall.java index 44d69bcc2..189ef1018 100644 --- a/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/Retrofit2SyncCall.java +++ b/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/Retrofit2SyncCall.java @@ -33,7 +33,7 @@ public static T execute(Call call) { try { return call.execute().body(); } catch (IOException e) { - throw new SpinnakerNetworkException(e); + throw new SpinnakerNetworkException(e, call.request()); } } } diff --git a/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerConversionException.java b/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerConversionException.java index cfd2e3a6b..a8a5bebfa 100644 --- a/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerConversionException.java +++ b/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerConversionException.java @@ -16,14 +16,36 @@ package com.netflix.spinnaker.kork.retrofit.exceptions; +import okhttp3.Request; +import retrofit.RetrofitError; + /** Wraps an exception converting a successful retrofit http response body to its indicated type */ public class SpinnakerConversionException extends SpinnakerServerException { - public SpinnakerConversionException(String message, Throwable cause) { + /** + * Construct a SpinnakerServerException from retrofit2 with a message and cause (e.g. an exception + * converting a response to the specified type). + */ + public SpinnakerConversionException(String message, Throwable cause, Request request) { + super(message, cause, request); + setRetryable(false); + } + + /** + * Construct a SpinnakerConversionException from another SpinnakerConversionException (e.g. via + * newInstance). + */ + public SpinnakerConversionException(String message, SpinnakerConversionException cause) { super(message, cause); setRetryable(false); } + /** Construct a SpinnakerConversionException corresponding to a RetrofitError. */ + public SpinnakerConversionException(RetrofitError e) { + super(e); + setRetryable(false); + } + @Override public SpinnakerConversionException newInstance(String message) { return new SpinnakerConversionException(message, this); diff --git a/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerHttpException.java b/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerHttpException.java index 355aea70e..7a705797f 100644 --- a/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerHttpException.java +++ b/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerHttpException.java @@ -56,8 +56,6 @@ public class SpinnakerHttpException extends SpinnakerServerException { private final Map responseBody; - private final String url; - private final int responseCode; /** @@ -66,6 +64,7 @@ public class SpinnakerHttpException extends SpinnakerServerException { */ private final String reason; + /** Construct a SpinnakerHttpException corresponding to a RetrofitError. */ public SpinnakerHttpException(RetrofitError e) { super(e); @@ -101,7 +100,6 @@ public SpinnakerHttpException(RetrofitError e) { if (responseBody != null) { tmpMessage = (String) responseBody.get("message"); } - url = e.getUrl(); responseCode = response.getStatus(); reason = response.getReason(); rawMessage = tmpMessage != null ? tmpMessage : reason; @@ -113,6 +111,7 @@ public SpinnakerHttpException(RetrofitError e) { */ public SpinnakerHttpException( retrofit2.Response retrofit2Response, retrofit2.Retrofit retrofit) { + super(retrofit2Response.raw().request()); this.response = null; this.retrofit2Response = retrofit2Response; if ((retrofit2Response.code() == HttpStatus.NOT_FOUND.value()) @@ -120,7 +119,6 @@ public SpinnakerHttpException( setRetryable(false); } responseBody = this.getErrorBodyAs(retrofit); - url = retrofit2Response.raw().request().url().toString(); responseCode = retrofit2Response.code(); reason = retrofit2Response.message(); this.rawMessage = @@ -159,7 +157,6 @@ public SpinnakerHttpException(String message, SpinnakerHttpException cause) { this.retrofit2Response = cause.retrofit2Response; rawMessage = null; this.responseBody = cause.responseBody; - this.url = cause.url; this.responseCode = cause.responseCode; this.reason = cause.reason; } @@ -197,7 +194,8 @@ public String getMessage() { return super.getMessage(); } - return String.format("Status: %s, URL: %s, Message: %s", responseCode, url, getRawMessage()); + return String.format( + "Status: %s, URL: %s, Message: %s", responseCode, this.getUrl(), getRawMessage()); } @Override @@ -209,10 +207,6 @@ public Map getResponseBody() { return this.responseBody; } - public String getUrl() { - return this.url; - } - public String getReason() { return this.reason; } diff --git a/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerNetworkException.java b/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerNetworkException.java index a51843d82..febf466e0 100644 --- a/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerNetworkException.java +++ b/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerNetworkException.java @@ -17,19 +17,34 @@ package com.netflix.spinnaker.kork.retrofit.exceptions; import com.netflix.spinnaker.kork.annotations.NonnullByDefault; +import okhttp3.Request; import retrofit.RetrofitError; -/** Wraps an exception of kind {@link RetrofitError.Kind} NETWORK. */ +/** Represents a network error while attempting to execute a retrofit http client request. */ @NonnullByDefault public final class SpinnakerNetworkException extends SpinnakerServerException { - public SpinnakerNetworkException(Throwable cause) { - super(cause); + + /** + * Construct a SpinnakerNetworkException from retrofit2 with a cause (e.g. an exception sending a + * request or processing a response). + */ + public SpinnakerNetworkException(Throwable cause, Request request) { + super(cause, request); } - public SpinnakerNetworkException(String message, Throwable cause) { + /** + * Construct a SpinnakerNetworkException from another SpinnakerNetworkException (e.g. via + * newInstance). + */ + public SpinnakerNetworkException(String message, SpinnakerNetworkException cause) { super(message, cause); } + /** Construct a SpinnakerNetworkException corresponding to a RetrofitError. */ + public SpinnakerNetworkException(RetrofitError e) { + super(e); + } + @Override public SpinnakerNetworkException newInstance(String message) { return new SpinnakerNetworkException(message, this); diff --git a/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofitErrorHandler.java b/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofitErrorHandler.java index 8dace58aa..ca03cb643 100644 --- a/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofitErrorHandler.java +++ b/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofitErrorHandler.java @@ -59,11 +59,11 @@ public Throwable handleError(RetrofitError e) { } return retval; case NETWORK: - return new SpinnakerNetworkException(e.getMessage(), e.getCause()); + return new SpinnakerNetworkException(e); case CONVERSION: - return new SpinnakerConversionException(e.getMessage(), e.getCause()); + return new SpinnakerConversionException(e); default: - return new SpinnakerServerException(e.getMessage(), e.getCause()); + return new SpinnakerServerException(e); } } diff --git a/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerException.java b/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerException.java index 654c164aa..0b2e6ebca 100644 --- a/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerException.java +++ b/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerException.java @@ -18,20 +18,57 @@ import com.netflix.spinnaker.kork.annotations.NonnullByDefault; import com.netflix.spinnaker.kork.exceptions.SpinnakerException; +import lombok.Getter; +import okhttp3.Request; +import retrofit.RetrofitError; /** Represents an error while attempting to execute a retrofit http client request. */ @NonnullByDefault public class SpinnakerServerException extends SpinnakerException { - public SpinnakerServerException(String message, Throwable cause) { - super(message, cause); + @Getter private final String url; + + /** Construct a SpinnakerServerException corresponding to a RetrofitError. */ + public SpinnakerServerException(RetrofitError e) { + super(e.getMessage(), e.getCause()); + url = e.getUrl(); } - public SpinnakerServerException(Throwable cause) { + /** + * Construct a SpinnakerServerException from retrofit2 with no cause (e.g. a non-200 http + * response). + */ + public SpinnakerServerException(Request request) { + super(); + url = request.url().toString(); + } + + /** + * Construct a SpinnakerServerException from retrofit2 with a cause (e.g. an exception sending a + * request or processing a response). + */ + public SpinnakerServerException(Throwable cause, Request request) { super(cause); + this.url = request.url().toString(); } - public SpinnakerServerException() {} + /** + * Construct a SpinnakerServerException from retrofit2 with a message and cause (e.g. an exception + * converting a response to the specified type). + */ + public SpinnakerServerException(String message, Throwable cause, Request request) { + super(message, cause); + this.url = request.url().toString(); + } + + /** + * Construct a SpinnakerServerException from another SpinnakerServerException (e.g. via + * newInstance). + */ + public SpinnakerServerException(String message, SpinnakerServerException cause) { + super(message, cause); + this.url = cause.getUrl(); + } @Override public SpinnakerServerException newInstance(String message) { diff --git a/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/Retrofit2SyncCallTest.java b/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/Retrofit2SyncCallTest.java index c114ea97d..b622734d0 100644 --- a/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/Retrofit2SyncCallTest.java +++ b/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/Retrofit2SyncCallTest.java @@ -16,38 +16,45 @@ package com.netflix.spinnaker.kork.retrofit.exceptions; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.catchThrowableOfType; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import com.netflix.spinnaker.kork.retrofit.Retrofit2SyncCall; import java.io.IOException; +import okhttp3.HttpUrl; +import okhttp3.Request; import org.junit.jupiter.api.Test; -import org.mockito.Mockito; import retrofit2.Call; import retrofit2.Response; -public class Retrofit2SyncCallTest { +class Retrofit2SyncCallTest { @Test - public void testExecuteSuccss() throws IOException { - Call mockcall = Mockito.mock(Call.class); - when(mockcall.execute()).thenReturn(Response.success("testing")); - String execute = Retrofit2SyncCall.execute(mockcall); - assertEquals("testing", execute); + void testExecuteSuccess() throws IOException { + Call mockCall = mock(Call.class); + String responseBody = "testing"; + when(mockCall.execute()).thenReturn(Response.success(responseBody)); + String execute = Retrofit2SyncCall.execute(mockCall); + assertThat(execute).isEqualTo(responseBody); } @Test - public void testExecuteThrowException() throws IOException { - Call mockcall = Mockito.mock(Call.class); + void testExecuteThrowException() throws IOException { + Call mockCall = mock(Call.class); IOException ioException = new IOException("exception test"); - when(mockcall.execute()).thenThrow(ioException); - SpinnakerNetworkException networkEx = - assertThrows( - SpinnakerNetworkException.class, - () -> { - Retrofit2SyncCall.execute(mockcall); - }); - assertEquals(ioException, networkEx.getCause()); + when(mockCall.execute()).thenThrow(ioException); + + HttpUrl url = HttpUrl.parse("http://arbitrary-url"); + Request mockRequest = mock(Request.class); + when(mockCall.request()).thenReturn(mockRequest); + when(mockRequest.url()).thenReturn(url); + + SpinnakerNetworkException thrown = + catchThrowableOfType( + () -> Retrofit2SyncCall.execute(mockCall), SpinnakerNetworkException.class); + assertThat(thrown).hasCause(ioException); + assertThat(thrown.getUrl()).isEqualTo(url.toString()); } } diff --git a/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerHttpExceptionTest.java b/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerHttpExceptionTest.java index 9d2c4774b..bfe9c4b7b 100644 --- a/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerHttpExceptionTest.java +++ b/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerHttpExceptionTest.java @@ -19,9 +19,6 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.catchThrowable; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertTrue; import com.google.gson.Gson; import com.netflix.spinnaker.kork.exceptions.SpinnakerException; @@ -39,11 +36,11 @@ import retrofit2.Retrofit; import retrofit2.converter.jackson.JacksonConverterFactory; -public class SpinnakerHttpExceptionTest { +class SpinnakerHttpExceptionTest { private static final String CUSTOM_MESSAGE = "custom message"; @Test - public void testSpinnakerHttpExceptionFromRetrofitError() { + void testSpinnakerHttpExceptionFromRetrofitError() { String url = "http://localhost"; int statusCode = 200; String message = "arbitrary message"; @@ -69,7 +66,7 @@ public void testSpinnakerHttpExceptionFromRetrofitError() { } @Test - public void testSpinnakerHttpExceptionFromRetrofitException() { + void testSpinnakerHttpExceptionFromRetrofitException() { final String validJsonResponseBodyString = "{\"name\":\"test\"}"; ResponseBody responseBody = ResponseBody.create( @@ -86,19 +83,19 @@ public void testSpinnakerHttpExceptionFromRetrofitException() { assertThat(retrofit2Service.baseUrl().toString()).isEqualTo(url); SpinnakerHttpException notFoundException = new SpinnakerHttpException(response, retrofit2Service); - assertNotNull(notFoundException.getResponseBody()); + assertThat(notFoundException.getResponseBody()).isNotNull(); assertThat(notFoundException.getUrl()).isEqualTo(url); assertThat(notFoundException.getReason()) .isEqualTo("Response.error()"); // set by Response.error Map errorResponseBody = notFoundException.getResponseBody(); - assertEquals(errorResponseBody.get("name"), "test"); - assertEquals(HttpStatus.NOT_FOUND.value(), notFoundException.getResponseCode()); - assertTrue( - notFoundException.getMessage().contains(String.valueOf(HttpStatus.NOT_FOUND.value()))); + assertThat(errorResponseBody.get("name")).isEqualTo("test"); + assertThat(notFoundException.getResponseCode()).isEqualTo(HttpStatus.NOT_FOUND.value()); + assertThat(notFoundException) + .hasMessageContaining(String.valueOf(HttpStatus.NOT_FOUND.value())); } @Test - public void testSpinnakerHttpException_NewInstance() { + void testSpinnakerHttpException_NewInstance() { String url = "http://localhost"; String reason = "reason"; Response response = new Response(url, 200, reason, List.of(), null); @@ -108,10 +105,11 @@ public void testSpinnakerHttpException_NewInstance() { } catch (SpinnakerException e) { SpinnakerException newException = e.newInstance(CUSTOM_MESSAGE); - assertTrue(newException instanceof SpinnakerHttpException); - assertEquals(CUSTOM_MESSAGE, newException.getMessage()); - assertEquals(e, newException.getCause()); - assertEquals(response.getStatus(), ((SpinnakerHttpException) newException).getResponseCode()); + assertThat(newException).isInstanceOf(SpinnakerHttpException.class); + assertThat(newException).hasMessage(CUSTOM_MESSAGE); + assertThat(newException).hasCause(e); + assertThat(((SpinnakerHttpException) newException).getResponseCode()) + .isEqualTo(response.getStatus()); SpinnakerHttpException spinnakerHttpException = (SpinnakerHttpException) newException; assertThat(spinnakerHttpException.getUrl()).isEqualTo(url); assertThat(spinnakerHttpException.getReason()).isEqualTo(reason); diff --git a/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofit2ErrorHandleTest.java b/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofit2ErrorHandleTest.java index 2da42fb8b..eb2101e1e 100644 --- a/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofit2ErrorHandleTest.java +++ b/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofit2ErrorHandleTest.java @@ -16,12 +16,8 @@ package com.netflix.spinnaker.kork.retrofit.exceptions; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertNull; -import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.catchThrowableOfType; import com.fasterxml.jackson.databind.ObjectMapper; import com.netflix.spinnaker.kork.retrofit.ErrorHandlingExecutorCallAdapterFactory; @@ -40,7 +36,7 @@ import retrofit2.Retrofit; import retrofit2.converter.jackson.JacksonConverterFactory; -public class SpinnakerRetrofit2ErrorHandleTest { +class SpinnakerRetrofit2ErrorHandleTest { private static Retrofit2Service retrofit2Service; @@ -51,7 +47,7 @@ public class SpinnakerRetrofit2ErrorHandleTest { private static String baseUrl = mockWebServer.url("/").toString(); @BeforeAll - public static void setupOnce() throws Exception { + static void setupOnce() throws Exception { Map responseBodyMap = new HashMap<>(); responseBodyMap.put("timestamp", "123123123123"); @@ -73,61 +69,66 @@ public static void setupOnce() throws Exception { } @AfterAll - public static void shutdownOnce() throws Exception { + static void shutdownOnce() throws Exception { mockWebServer.shutdown(); } @Test - public void testRetrofitNotFoundIsNotRetryable() { + void testRetrofitNotFoundIsNotRetryable() { mockWebServer.enqueue( new MockResponse() .setResponseCode(HttpStatus.NOT_FOUND.value()) .setBody(responseBodyString)); SpinnakerHttpException notFoundException = - assertThrows(SpinnakerHttpException.class, () -> retrofit2Service.getRetrofit2().execute()); - assertNotNull(notFoundException.getRetryable()); - assertFalse(notFoundException.getRetryable()); + catchThrowableOfType( + () -> retrofit2Service.getRetrofit2().execute(), SpinnakerHttpException.class); + assertThat(notFoundException.getRetryable()).isNotNull(); + assertThat(notFoundException.getRetryable()).isFalse(); + assertThat(notFoundException.getUrl()).isEqualTo(mockWebServer.url("/retrofit2").toString()); } @Test - public void testRetrofitBadRequestIsNotRetryable() { + void testRetrofitBadRequestIsNotRetryable() { mockWebServer.enqueue( new MockResponse() .setResponseCode(HttpStatus.NOT_FOUND.value()) .setBody(responseBodyString)); SpinnakerHttpException spinnakerHttpException = - assertThrows(SpinnakerHttpException.class, () -> retrofit2Service.getRetrofit2().execute()); - assertNotNull(spinnakerHttpException.getRetryable()); - assertFalse(spinnakerHttpException.getRetryable()); + catchThrowableOfType( + () -> retrofit2Service.getRetrofit2().execute(), SpinnakerHttpException.class); + assertThat(spinnakerHttpException.getRetryable()).isNotNull(); + assertThat(spinnakerHttpException.getRetryable()).isFalse(); + assertThat(spinnakerHttpException.getUrl()) + .isEqualTo(mockWebServer.url("/retrofit2").toString()); } @Test - public void testRetrofitOtherClientErrorHasNullRetryable() { + void testRetrofitOtherClientErrorHasNullRetryable() { mockWebServer.enqueue( new MockResponse().setResponseCode(HttpStatus.GONE.value()).setBody(responseBodyString)); SpinnakerHttpException spinnakerHttpException = - assertThrows(SpinnakerHttpException.class, () -> retrofit2Service.getRetrofit2().execute()); - assertNull(spinnakerHttpException.getRetryable()); + catchThrowableOfType( + () -> retrofit2Service.getRetrofit2().execute(), SpinnakerHttpException.class); + assertThat(spinnakerHttpException.getRetryable()).isNull(); + assertThat(spinnakerHttpException.getUrl()) + .isEqualTo(mockWebServer.url("/retrofit2").toString()); } @Test - public void testRetrofitSimpleSpinnakerNetworkException() { + void testRetrofitSimpleSpinnakerNetworkException() { mockWebServer.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.NO_RESPONSE)); - - assertThrows(SpinnakerNetworkException.class, () -> retrofit2Service.getRetrofit2().execute()); - } - - @Test - public void testRetrofitSimpleSpinnakerServerException() { - mockWebServer.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.DISCONNECT_AT_START)); - assertThrows(SpinnakerServerException.class, () -> retrofit2Service.getRetrofit2().execute()); + SpinnakerNetworkException spinnakerNetworkException = + catchThrowableOfType( + () -> retrofit2Service.getRetrofit2().execute(), SpinnakerNetworkException.class); + assertThat(spinnakerNetworkException.getUrl()) + .isEqualTo(mockWebServer.url("/retrofit2").toString()); } @Test - public void testResponseHeadersInException() { + void testResponseHeadersInException() { // Check response headers are retrievable from a SpinnakerHttpException mockWebServer.enqueue( @@ -136,35 +137,35 @@ public void testResponseHeadersInException() { .setBody(responseBodyString) .setHeader("Test", "true")); SpinnakerHttpException spinnakerHttpException = - assertThrows(SpinnakerHttpException.class, () -> retrofit2Service.getRetrofit2().execute()); - assertTrue(spinnakerHttpException.getHeaders().containsKey("Test")); - assertTrue(spinnakerHttpException.getHeaders().get("Test").contains("true")); + catchThrowableOfType( + () -> retrofit2Service.getRetrofit2().execute(), SpinnakerHttpException.class); + assertThat(spinnakerHttpException.getHeaders().containsKey("Test")).isTrue(); + assertThat(spinnakerHttpException.getHeaders().get("Test").contains("true")).isTrue(); + assertThat(spinnakerHttpException.getUrl()) + .isEqualTo(mockWebServer.url("/retrofit2").toString()); } @Test - public void testNotParameterizedException() { - + void testNotParameterizedException() { IllegalArgumentException illegalArgumentException = - assertThrows( - IllegalArgumentException.class, - () -> retrofit2Service.testNotParameterized().execute()); - - assertEquals( - "Call return type must be parameterized as Call or Call", - illegalArgumentException.getCause().getMessage()); + catchThrowableOfType( + () -> retrofit2Service.testNotParameterized().execute(), + IllegalArgumentException.class); + assertThat(illegalArgumentException.getCause().getMessage()) + .isEqualTo("Call return type must be parameterized as Call or Call"); } @Test - public void testWrongReturnTypeException() { + void testWrongReturnTypeException() { IllegalArgumentException illegalArgumentException = - assertThrows( - IllegalArgumentException.class, () -> retrofit2Service.testWrongReturnType().execute()); + catchThrowableOfType( + () -> retrofit2Service.testWrongReturnType().execute(), IllegalArgumentException.class); - assertEquals( - "Unable to create call adapter for interface com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofit2ErrorHandleTest$DummyWithExecute\n" - + " for method Retrofit2Service.testWrongReturnType", - illegalArgumentException.getMessage()); + assertThat(illegalArgumentException) + .hasMessage( + "Unable to create call adapter for interface com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofit2ErrorHandleTest$DummyWithExecute\n" + + " for method Retrofit2Service.testWrongReturnType"); } interface Retrofit2Service { @@ -189,14 +190,17 @@ void testSpinnakerConversionException() { .setBody(invalidJsonTypeResponseBody)); SpinnakerConversionException spinnakerConversionException = - assertThrows( - SpinnakerConversionException.class, () -> retrofit2Service.getRetrofit2().execute()); - - assertEquals("Failed to process response body", spinnakerConversionException.getMessage()); + catchThrowableOfType( + () -> retrofit2Service.getRetrofit2().execute(), SpinnakerConversionException.class); + assertThat(spinnakerConversionException.getRetryable()).isNotNull(); + assertThat(spinnakerConversionException.getRetryable()).isFalse(); + assertThat(spinnakerConversionException).hasMessage("Failed to process response body"); + assertThat(spinnakerConversionException.getUrl()) + .isEqualTo(mockWebServer.url("/retrofit2").toString()); } @Test - public void testNonJsonHttpErrorResponse() { + void testNonJsonHttpErrorResponse() { String invalidJsonTypeResponseBody = "{'errorResponse': 'Failure'"; int responseCode = HttpStatus.INTERNAL_SERVER_ERROR.value(); @@ -206,14 +210,14 @@ public void testNonJsonHttpErrorResponse() { mockWebServer.enqueue( new MockResponse().setResponseCode(responseCode).setBody(invalidJsonTypeResponseBody)); SpinnakerHttpException spinnakerHttpException = - assertThrows(SpinnakerHttpException.class, () -> retrofit2Service.getRetrofit2().execute()); - assertNull(spinnakerHttpException.getResponseBody()); - assertEquals(responseCode, spinnakerHttpException.getResponseCode()); - assertEquals( - "Status: " + responseCode + ", URL: " + url + ", Message: " + reason, - spinnakerHttpException.getMessage()); - assertEquals(url, spinnakerHttpException.getUrl()); - assertEquals(reason, spinnakerHttpException.getReason()); + catchThrowableOfType( + () -> retrofit2Service.getRetrofit2().execute(), SpinnakerHttpException.class); + assertThat(spinnakerHttpException.getResponseBody()).isNull(); + assertThat(spinnakerHttpException.getResponseCode()).isEqualTo(responseCode); + assertThat(spinnakerHttpException) + .hasMessage("Status: " + responseCode + ", URL: " + url + ", Message: " + reason); + assertThat(spinnakerHttpException.getUrl()).isEqualTo(url); + assertThat(spinnakerHttpException.getReason()).isEqualTo(reason); } interface DummyWithExecute { diff --git a/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofitErrorHandlerTest.java b/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofitErrorHandlerTest.java index 405c010b4..2f5556183 100644 --- a/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofitErrorHandlerTest.java +++ b/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofitErrorHandlerTest.java @@ -16,12 +16,9 @@ package com.netflix.spinnaker.kork.retrofit.exceptions; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertNull; -import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; +import static org.assertj.core.api.Assertions.catchThrowableOfType; import com.fasterxml.jackson.databind.DeserializationFeature; import com.fasterxml.jackson.databind.ObjectMapper; @@ -30,6 +27,7 @@ import java.util.Map; import okhttp3.mockwebserver.MockResponse; import okhttp3.mockwebserver.MockWebServer; +import okhttp3.mockwebserver.SocketPolicy; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; @@ -42,14 +40,14 @@ import retrofit.converter.JacksonConverter; import retrofit.http.GET; -public class SpinnakerRetrofitErrorHandlerTest { +class SpinnakerRetrofitErrorHandlerTest { private static RetrofitService retrofitService; private static final MockWebServer mockWebServer = new MockWebServer(); @BeforeAll - public static void setupOnce() throws Exception { + static void setupOnce() throws Exception { mockWebServer.start(); retrofitService = @@ -61,17 +59,24 @@ public static void setupOnce() throws Exception { } @AfterAll - public static void shutdownOnce() throws Exception { + static void shutdownOnce() throws Exception { mockWebServer.shutdown(); } @Test - public void testNotFoundIsNotRetryable() { + void testNotFoundIsNotRetryable() { mockWebServer.enqueue(new MockResponse().setResponseCode(HttpStatus.NOT_FOUND.value())); SpinnakerHttpException notFoundException = - assertThrows(SpinnakerHttpException.class, () -> retrofitService.getFoo()); - assertNotNull(notFoundException.getRetryable()); - assertFalse(notFoundException.getRetryable()); + catchThrowableOfType(() -> retrofitService.getFoo(), SpinnakerHttpException.class); + assertThat(notFoundException.getRetryable()).isNotNull(); + assertThat(notFoundException.getRetryable()).isFalse(); + } + + @Test + void testSpinnakerNetworkException() { + mockWebServer.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.NO_RESPONSE)); + assertThatExceptionOfType(SpinnakerNetworkException.class) + .isThrownBy(() -> retrofitService.getFoo()); } @ParameterizedTest(name = "Deserialize response using {0}") @@ -88,7 +93,7 @@ public void testNotFoundIsNotRetryable() { // is set when building out the RestAdapter @ValueSource( strings = {"Default_GSONConverter", "JacksonConverter", "JacksonConverterWithObjectMapper"}) - public void testResponseWithExtraField(String retrofitConverter) throws Exception { + void testResponseWithExtraField(String retrofitConverter) throws Exception { Map responseBodyMap = new HashMap<>(); responseBodyMap.put("timestamp", "123123123123"); responseBodyMap.put("message", "Not Found error Message"); @@ -131,82 +136,97 @@ public void testResponseWithExtraField(String retrofitConverter) throws Exceptio // "..." // // so make sure we get a SpinnakerHttpException from calling getFoo - assertThrows(SpinnakerHttpException.class, retrofitServiceTestConverter::getFoo); + SpinnakerHttpException spinnakerHttpException = + catchThrowableOfType(retrofitServiceTestConverter::getFoo, SpinnakerHttpException.class); + assertThat(spinnakerHttpException.getUrl()).isEqualTo(mockWebServer.url("/foo").toString()); } @Test - public void testBadRequestIsNotRetryable() { + void testBadRequestIsNotRetryable() { mockWebServer.enqueue(new MockResponse().setResponseCode(HttpStatus.BAD_REQUEST.value())); SpinnakerHttpException spinnakerHttpException = - assertThrows(SpinnakerHttpException.class, () -> retrofitService.getFoo()); - assertNotNull(spinnakerHttpException.getRetryable()); - assertFalse(spinnakerHttpException.getRetryable()); + catchThrowableOfType(() -> retrofitService.getFoo(), SpinnakerHttpException.class); + assertThat(spinnakerHttpException.getRetryable()).isNotNull(); + assertThat(spinnakerHttpException.getRetryable()).isFalse(); + assertThat(spinnakerHttpException.getUrl()).isEqualTo(mockWebServer.url("/foo").toString()); } @Test - public void testOtherClientErrorHasNullRetryable() { + void testOtherClientErrorHasNullRetryable() { // Arbitrarily choose GONE as an example of a client (e.g. 4xx) error that // we expect to have null retryable mockWebServer.enqueue(new MockResponse().setResponseCode(HttpStatus.GONE.value())); SpinnakerHttpException spinnakerHttpException = - assertThrows(SpinnakerHttpException.class, () -> retrofitService.getFoo()); - assertNull(spinnakerHttpException.getRetryable()); + catchThrowableOfType(() -> retrofitService.getFoo(), SpinnakerHttpException.class); + assertThat(spinnakerHttpException.getRetryable()).isNull(); + assertThat(spinnakerHttpException.getUrl()).isEqualTo(mockWebServer.url("/foo").toString()); } @Test - public void testResponseHeadersInException() { + void testResponseHeadersInException() { // Check response headers are retrievable from a SpinnakerHttpException mockWebServer.enqueue( new MockResponse() .setResponseCode(HttpStatus.BAD_REQUEST.value()) .setHeader("Test", "true")); SpinnakerHttpException spinnakerHttpException = - assertThrows(SpinnakerHttpException.class, () -> retrofitService.getFoo()); - assertTrue(spinnakerHttpException.getHeaders().containsKey("Test")); - assertTrue(spinnakerHttpException.getHeaders().get("Test").contains("true")); + catchThrowableOfType(() -> retrofitService.getFoo(), SpinnakerHttpException.class); + assertThat(spinnakerHttpException.getHeaders().containsKey("Test")).isTrue(); + assertThat(spinnakerHttpException.getHeaders().get("Test").contains("true")).isTrue(); + assertThat(spinnakerHttpException.getUrl()).isEqualTo(mockWebServer.url("/foo").toString()); } @Test - public void testSimpleSpinnakerNetworkException() { + void testSimpleSpinnakerNetworkException() { String message = "my custom message"; IOException e = new IOException(message); - RetrofitError retrofitError = RetrofitError.networkError("http://localhost", e); + String url = "http://localhost"; + RetrofitError retrofitError = RetrofitError.networkError(url, e); SpinnakerRetrofitErrorHandler handler = SpinnakerRetrofitErrorHandler.getInstance(); Throwable throwable = handler.handleError(retrofitError); - assertEquals(message, throwable.getMessage()); + assertThat(throwable).hasMessage(message); + assertThat(throwable).isInstanceOf(SpinnakerNetworkException.class); + SpinnakerNetworkException spinnakerNetworkException = (SpinnakerNetworkException) throwable; + assertThat(spinnakerNetworkException.getUrl()).isEqualTo(url); } @Test - public void testSpinnakerConversionException() { + void testSpinnakerConversionException() { mockWebServer.enqueue( new MockResponse().setBody("Invalid JSON response").setResponseCode(HttpStatus.OK.value())); SpinnakerConversionException spinnakerConversionException = - assertThrows(SpinnakerConversionException.class, () -> retrofitService.getData()); - assertTrue( - spinnakerConversionException - .getMessage() - .contains("Expected BEGIN_OBJECT but was STRING at line 1 column 1 path $")); + catchThrowableOfType(() -> retrofitService.getData(), SpinnakerConversionException.class); + assertThat(spinnakerConversionException.getRetryable()).isNotNull(); + assertThat(spinnakerConversionException.getRetryable()).isFalse(); + assertThat(spinnakerConversionException) + .hasMessageContaining("Expected BEGIN_OBJECT but was STRING at line 1 column 1 path $"); + assertThat(spinnakerConversionException.getUrl()) + .isEqualTo(mockWebServer.url("/data").toString()); } @Test - public void testChainSpinnakerException_SpinnakerNetworkException() { + void testChainSpinnakerException_SpinnakerNetworkException() { SpinnakerRetrofitErrorHandler handler = SpinnakerRetrofitErrorHandler.getInstance(); String originalMessage = "original message"; String newMessage = "new message"; IOException originalException = new IOException(originalMessage); - RetrofitError retrofitError = RetrofitError.networkError("http://localhost", originalException); + + String url = "http://localhost"; + RetrofitError retrofitError = RetrofitError.networkError(url, originalException); Throwable newException = handler.handleError( retrofitError, (exception) -> String.format("%s: %s", newMessage, exception.getMessage())); - assertTrue(newException instanceof SpinnakerNetworkException); - assertEquals("new message: original message", newException.getMessage()); - assertEquals(originalMessage, newException.getCause().getMessage()); + assertThat(newException).isInstanceOf(SpinnakerNetworkException.class); + assertThat(newException).hasMessage("new message: original message"); + assertThat(newException.getCause()).hasMessage(originalMessage); + SpinnakerNetworkException spinnakerNetworkException = (SpinnakerNetworkException) newException; + assertThat(spinnakerNetworkException.getUrl()).isEqualTo(url); } interface RetrofitService { diff --git a/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofitExceptionHandlersTest.java b/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofitExceptionHandlersTest.java index cb342b20e..93bbc2ad5 100644 --- a/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofitExceptionHandlersTest.java +++ b/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofitExceptionHandlersTest.java @@ -16,7 +16,7 @@ package com.netflix.spinnaker.kork.retrofit.exceptions; -import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -74,7 +74,7 @@ class SpinnakerRetrofitExceptionHandlersTest { private MemoryAppender memoryAppender; @BeforeEach - private void setup(TestInfo testInfo) { + void setup(TestInfo testInfo) { System.out.println("--------------- Test " + testInfo.getDisplayName()); memoryAppender = new MemoryAppender(SpinnakerRetrofitExceptionHandlers.class); } @@ -84,8 +84,8 @@ void testSpinnakerServerException() throws Exception { URI uri = getUri("/spinnakerServerException"); ResponseEntity entity = restTemplate.getForEntity(uri, String.class); - assertEquals(HttpStatus.INTERNAL_SERVER_ERROR, entity.getStatusCode()); - assertEquals(1, memoryAppender.countEventsForLevel(Level.ERROR)); + assertThat(entity.getStatusCode()).isEqualTo(HttpStatus.INTERNAL_SERVER_ERROR); + assertThat(memoryAppender.countEventsForLevel(Level.ERROR)).isEqualTo(1); } @Test @@ -93,11 +93,11 @@ void testChainedSpinnakerServerException() throws Exception { URI uri = getUri("/chainedSpinnakerServerException"); ResponseEntity entity = restTemplate.getForEntity(uri, String.class); - assertEquals(HttpStatus.INTERNAL_SERVER_ERROR, entity.getStatusCode()); - assertEquals(1, memoryAppender.countEventsForLevel(Level.ERROR)); + assertThat(entity.getStatusCode()).isEqualTo(HttpStatus.INTERNAL_SERVER_ERROR); + assertThat(memoryAppender.countEventsForLevel(Level.ERROR)).isEqualTo(1); // Make sure the message is what we expect. - assertEquals(1, memoryAppender.search(CUSTOM_MESSAGE, Level.ERROR).size()); + assertThat(memoryAppender.search(CUSTOM_MESSAGE, Level.ERROR)).hasSize(1); } @ParameterizedTest(name = "testSpinnakerHttpException status = {0}") @@ -106,15 +106,15 @@ void testSpinnakerHttpException(int status) throws Exception { URI uri = getUri("/spinnakerHttpException/" + String.valueOf(status)); ResponseEntity entity = restTemplate.getForEntity(uri, String.class); - assertEquals(status, entity.getStatusCode().value()); + assertThat(entity.getStatusCode().value()).isEqualTo(status); // Only expect error logging for a server error, debug otherwise. No need // to fill up logs with client errors assuming the server is doing the best // it can. - assertEquals( - 1, - memoryAppender.countEventsForLevel( - HttpStatus.resolve(status).is5xxServerError() ? Level.ERROR : Level.DEBUG)); + assertThat( + memoryAppender.countEventsForLevel( + HttpStatus.resolve(status).is5xxServerError() ? Level.ERROR : Level.DEBUG)) + .isEqualTo(1); } @ParameterizedTest(name = "testChainedSpinnakerHttpException status = {0}") @@ -123,24 +123,22 @@ void testChainedSpinnakerHttpException(int status) throws Exception { URI uri = getUri("/chainedSpinnakerHttpException/" + String.valueOf(status)); ResponseEntity entity = restTemplate.getForEntity(uri, String.class); - assertEquals(status, entity.getStatusCode().value()); + assertThat(entity.getStatusCode().value()).isEqualTo(status); // Only expect error logging for a server error, debug otherwise. No need // to fill up logs with client errors assuming the server is doing the best // it can. - assertEquals( - 1, - memoryAppender.countEventsForLevel( - HttpStatus.resolve(status).is5xxServerError() ? Level.ERROR : Level.DEBUG)); + assertThat( + memoryAppender.countEventsForLevel( + HttpStatus.resolve(status).is5xxServerError() ? Level.ERROR : Level.DEBUG)) + .isEqualTo(1); // Make sure the message is what we expect. - assertEquals( - 1, - memoryAppender - .search( + assertThat( + memoryAppender.search( CUSTOM_MESSAGE, - HttpStatus.resolve(status).is5xxServerError() ? Level.ERROR : Level.DEBUG) - .size()); + HttpStatus.resolve(status).is5xxServerError() ? Level.ERROR : Level.DEBUG)) + .hasSize(1); } private URI getUri(String path) { diff --git a/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerExceptionTest.java b/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerExceptionTest.java index 1292eed0a..6c5680ff1 100644 --- a/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerExceptionTest.java +++ b/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerExceptionTest.java @@ -16,13 +16,13 @@ package com.netflix.spinnaker.kork.retrofit.exceptions; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.assertj.core.api.Assertions.assertThat; import com.google.gson.Gson; import com.netflix.spinnaker.kork.exceptions.SpinnakerException; import java.io.IOException; import java.util.List; +import okhttp3.Request; import org.junit.jupiter.api.Test; import retrofit.RetrofitError; import retrofit.client.Response; @@ -30,41 +30,68 @@ import retrofit.converter.GsonConverter; import retrofit.mime.TypedByteArray; -public class SpinnakerServerExceptionTest { +class SpinnakerServerExceptionTest { private static final String CUSTOM_MESSAGE = "custom message"; @Test - public void testSpinnakerNetworkException_NewInstance() { + void testSpinnakerNetworkExceptionWithUrl() { + Throwable cause = new Throwable("arbitrary message"); + String url = "http://some-url/"; + Request request = new Request.Builder().url(url).build(); + SpinnakerNetworkException spinnakerNetworkException = + new SpinnakerNetworkException(cause, request); + assertThat(spinnakerNetworkException.getUrl()).isEqualTo(url); + } + + @Test + void testSpinnakerNetworkException_NewInstance() { IOException initialException = new IOException("message"); + String url = "http://localhost"; try { - RetrofitError error = RetrofitError.networkError("http://localhost", initialException); + RetrofitError error = RetrofitError.networkError(url, initialException); throw new SpinnakerNetworkException(error); } catch (SpinnakerException e) { SpinnakerException newException = e.newInstance(CUSTOM_MESSAGE); - assertTrue(newException instanceof SpinnakerNetworkException); - assertEquals(CUSTOM_MESSAGE, newException.getMessage()); - assertEquals(e, newException.getCause()); + assertThat(newException).isInstanceOf(SpinnakerNetworkException.class); + assertThat(newException).hasMessage(CUSTOM_MESSAGE); + assertThat(newException).hasCause(e); + SpinnakerNetworkException spinnakerNetworkException = + (SpinnakerNetworkException) newException; + assertThat(spinnakerNetworkException.getUrl()).isEqualTo(url); } } @Test - public void testSpinnakerServerException_NewInstance() { + void testSpinnakerServerExceptionWithUrl() { + Throwable cause = new Throwable("arbitrary message"); + String url = "http://some-url/"; + Request request = new Request.Builder().url(url).build(); + SpinnakerServerException spinnakerServerException = + new SpinnakerServerException(cause, request); + assertThat(spinnakerServerException.getUrl()).isEqualTo(url); + } + + @Test + void testSpinnakerServerException_NewInstance() { Throwable cause = new Throwable("message"); + String url = "http://localhost"; try { - RetrofitError error = RetrofitError.unexpectedError("http://localhost", cause); + RetrofitError error = RetrofitError.unexpectedError(url, cause); throw new SpinnakerServerException(error); } catch (SpinnakerException e) { SpinnakerException newException = e.newInstance(CUSTOM_MESSAGE); - assertTrue(newException instanceof SpinnakerServerException); - assertEquals(CUSTOM_MESSAGE, newException.getMessage()); - assertEquals(e, newException.getCause()); + assertThat(newException).isInstanceOf(SpinnakerServerException.class); + assertThat(newException).hasMessage(CUSTOM_MESSAGE); + assertThat(newException).hasCause(e); + SpinnakerServerException spinnakerServerException = (SpinnakerServerException) newException; + assertThat(spinnakerServerException.getUrl()).isEqualTo(url); } } @Test - public void testSpinnakerConversionException_NewInstance() { + void testSpinnakerConversionException_NewInstance() { String url = "http://localhost"; String reason = "reason"; @@ -82,13 +109,18 @@ public void testSpinnakerConversionException_NewInstance() { RetrofitError retrofitError = RetrofitError.conversionError( url, response, new GsonConverter(new Gson()), null, conversionException); - throw new SpinnakerConversionException(retrofitError.getMessage(), retrofitError.getCause()); + throw new SpinnakerConversionException(retrofitError); } catch (SpinnakerException e) { SpinnakerException newException = e.newInstance(CUSTOM_MESSAGE); - assertTrue(newException instanceof SpinnakerConversionException); - assertEquals(CUSTOM_MESSAGE, newException.getMessage()); - assertEquals(e, newException.getCause()); + assertThat(newException).isInstanceOf(SpinnakerConversionException.class); + assertThat(newException).hasMessage(CUSTOM_MESSAGE); + assertThat(newException).hasCause(e); + SpinnakerConversionException spinnakerConversionException = + (SpinnakerConversionException) newException; + assertThat(spinnakerConversionException.getRetryable()).isNotNull(); + assertThat(spinnakerConversionException.getRetryable()).isFalse(); + assertThat(spinnakerConversionException.getUrl()).isEqualTo(url); } } } From 43c06b61c45bdaa7824826cb5fca7e8ef65d8a61 Mon Sep 17 00:00:00 2001 From: Kiran Godishala <53332225+kirangodishala@users.noreply.github.com> Date: Mon, 30 Oct 2023 22:57:24 +0530 Subject: [PATCH 05/32] minor bug fix - changed duplicate logback-classic dependency to logback-access (#1111) --- spinnaker-dependencies/spinnaker-dependencies.gradle | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spinnaker-dependencies/spinnaker-dependencies.gradle b/spinnaker-dependencies/spinnaker-dependencies.gradle index f6d84af94..317a82101 100644 --- a/spinnaker-dependencies/spinnaker-dependencies.gradle +++ b/spinnaker-dependencies/spinnaker-dependencies.gradle @@ -76,7 +76,7 @@ dependencies { constraints { api("cglib:cglib-nodep:3.3.0") //A bug is reported in 1.2.11 and fixed in 1.2.12. - //So pinning the version to 1.2.12 untill spring boot upgrade to 2.5.15 or above. + //So pinning the version to 1.2.12 until spring boot upgrade to 2.5.15 or above. //[https://jira.qos.ch/browse/LOGBACK-1623] api("ch.qos.logback:logback-core"){ version { @@ -88,7 +88,7 @@ dependencies { strictly "${versions.logback}" } } - api("ch.qos.logback:logback-classic"){ + api("ch.qos.logback:logback-access"){ version { strictly "${versions.logback}" } From 5e5812d53d7a7136716b74d287cc4dc140932b9c Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Wed, 1 Nov 2023 17:24:08 -0500 Subject: [PATCH 06/32] config(crypto): Update the default TLS cipher suites (#1112) This adds a common `CipherSuites` class for listing various profiles of cipher suites for use with TLS. The default cipher suites and TLS versions are updated to a slightly more secure version of the intermediate compatibility baseline from the Mozilla recommendations. These settings are used for both Tomcat and OkHttpClient. Of particular note, this adds in the required ciphers in TLS 1.3 to improve compatibility with modern clients and servers. --- .../spinnaker/kork/crypto/CipherSuites.java | 67 +++++++++++++++++++ kork-tomcat/kork-tomcat.gradle | 1 + .../tomcat/TomcatConfigurationProperties.java | 25 ++----- ...OkHttpClientConfigurationProperties.groovy | 20 +----- 4 files changed, 75 insertions(+), 38 deletions(-) create mode 100644 kork-crypto/src/main/java/com/netflix/spinnaker/kork/crypto/CipherSuites.java diff --git a/kork-crypto/src/main/java/com/netflix/spinnaker/kork/crypto/CipherSuites.java b/kork-crypto/src/main/java/com/netflix/spinnaker/kork/crypto/CipherSuites.java new file mode 100644 index 000000000..6a082cbc3 --- /dev/null +++ b/kork-crypto/src/main/java/com/netflix/spinnaker/kork/crypto/CipherSuites.java @@ -0,0 +1,67 @@ +/* + * Copyright 2023 Apple 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.crypto; + +import java.util.ArrayList; +import java.util.List; + +/** + * Provides a common source for lists of TLS cipher suite baselines. + * + * @see Mozilla Server Side TLS + * recommendations + */ +public final class CipherSuites { + private static final List REQUIRED = + List.of("TLS_AES_128_GCM_SHA256", "TLS_AES_256_GCM_SHA384", "TLS_CHACHA20_POLY1305_SHA256"); + private static final List BROWSER_COMPATIBILITY = + List.of( + "TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256", + "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", + "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384", + "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384", + "TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256", + "TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256"); + private static final List RESTRICTED = + List.of( + "TLS_DHE_RSA_WITH_AES_128_GCM_SHA256", + "TLS_DHE_RSA_WITH_AES_256_GCM_SHA384", + "TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256"); + + /** + * Returns the list of baseline ciphers that should be enabled for TLS. These include the required + * ciphers for TLSv1.3. + * + * @see Modern + * compatibility recommendations + */ + public static List getRequiredCiphers() { + return REQUIRED; + } + + public static List getRecommendedCiphers() { + var ciphers = new ArrayList<>(getRequiredCiphers()); + ciphers.addAll(BROWSER_COMPATIBILITY); + return ciphers; + } + + public static List getIntermediateCompatibilityCiphers() { + var ciphers = getRecommendedCiphers(); + ciphers.addAll(RESTRICTED); + return ciphers; + } +} diff --git a/kork-tomcat/kork-tomcat.gradle b/kork-tomcat/kork-tomcat.gradle index 8dedbdb51..30a305248 100644 --- a/kork-tomcat/kork-tomcat.gradle +++ b/kork-tomcat/kork-tomcat.gradle @@ -8,6 +8,7 @@ dependencies { implementation("com.netflix.spectator:spectator-api") implementation("com.google.guava:guava") implementation(project(":kork-core")) + implementation(project(":kork-crypto")) testImplementation "org.springframework.boot:spring-boot-starter-test" } diff --git a/kork-tomcat/src/main/java/com/netflix/spinnaker/kork/tomcat/TomcatConfigurationProperties.java b/kork-tomcat/src/main/java/com/netflix/spinnaker/kork/tomcat/TomcatConfigurationProperties.java index a5eaf94c8..1f890711a 100644 --- a/kork-tomcat/src/main/java/com/netflix/spinnaker/kork/tomcat/TomcatConfigurationProperties.java +++ b/kork-tomcat/src/main/java/com/netflix/spinnaker/kork/tomcat/TomcatConfigurationProperties.java @@ -17,6 +17,7 @@ package com.netflix.spinnaker.kork.tomcat; +import com.netflix.spinnaker.kork.crypto.CipherSuites; import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -31,27 +32,9 @@ public class TomcatConfigurationProperties { private String relaxedQueryCharacters = ""; private String relaxedPathCharacters = ""; - private List tlsVersions = new ArrayList<>(Arrays.asList("TLSv1.2", "TLSv1.1")); - - // Defaults from https://wiki.mozilla.org/Security/Server_Side_TLS#Modern_compatibility - // with some extra ciphers (non SHA384/256) to support TLSv1.1 and some non EC ciphers - private List cipherSuites = - new ArrayList<>( - Arrays.asList( - "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384", - "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384", - "TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256", - "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", - "TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384", - "TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384", - "TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256", - "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256", - "TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA", - "TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA", - "TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA", - "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA", - "TLS_DHE_RSA_WITH_AES_256_CBC_SHA", - "TLS_DHE_RSA_WITH_AES_128_CBC_SHA")); + private List tlsVersions = new ArrayList<>(Arrays.asList("TLSv1.3", "TLSv1.2")); + + private List cipherSuites = CipherSuites.getRecommendedCiphers(); private Boolean rejectIllegalHeader; diff --git a/kork-web/src/main/groovy/com/netflix/spinnaker/okhttp/OkHttpClientConfigurationProperties.groovy b/kork-web/src/main/groovy/com/netflix/spinnaker/okhttp/OkHttpClientConfigurationProperties.groovy index b69e88b13..12a2390dc 100644 --- a/kork-web/src/main/groovy/com/netflix/spinnaker/okhttp/OkHttpClientConfigurationProperties.groovy +++ b/kork-web/src/main/groovy/com/netflix/spinnaker/okhttp/OkHttpClientConfigurationProperties.groovy @@ -16,6 +16,7 @@ package com.netflix.spinnaker.okhttp +import com.netflix.spinnaker.kork.crypto.CipherSuites import groovy.transform.AutoClone import groovy.transform.Canonical import java.time.Duration @@ -43,23 +44,8 @@ class OkHttpClientConfigurationProperties { String secureRandomInstanceType = "NativePRNGNonBlocking" // TLS1.1 isn't supported in newer JVMs... do NOT try to add back - it's also insecure - List tlsVersions = ["TLSv1.2", "TLSv1.3"] - //Defaults from https://wiki.mozilla.org/Security/Server_Side_TLS#Modern_compatibility - // with some extra ciphers (non SHA384/256) to support TLSv1.1 and some non EC ciphers - List cipherSuites = [ - "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384", - "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384", - "TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256", - "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", - "TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384", - "TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384", - "TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256", - "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256", - "TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA", - "TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA", - "TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA", - "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA" - ] + List tlsVersions = ["TLSv1.3", "TLSv1.2"] + List cipherSuites = CipherSuites.recommendedCiphers /** * Provide backwards compatibility for 'okHttpClient.connectTimoutMs' From bbb19fb20c32575e942d28c86fd322a4c5c00d2e Mon Sep 17 00:00:00 2001 From: xibz Date: Thu, 2 Nov 2023 14:17:37 -0500 Subject: [PATCH 07/32] fix(artifacts): references containing '+' break matching (#1113) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(artifacts): references containing '+' break matching (#1496) When expected artifact does matching on a base64 reference, containing special regex characters, it will attempt pattern matching where ‘+’ actually means something in regex and will fail matches because of this. This commit ensures that all base64 characters can be matched against that may not be valid regex expression. Example pipeline: ```json { "application" : "spinnaker-ci", "description" : "This pipeline ensures that artifacts with regex characters like '+' succeed in pipelines.", "expectedArtifacts" : [ { "defaultArtifact" : { "artifactAccount" : "embedded-artifact", "id" : "f435c890-c5b6-4bdb-a733-d788413fd5c1", "type" : "embedded/base64" }, "displayName" : "regex-artifact", "id" : "be22c2c5-6561-4f41-b885-367ef91b8362", "matchArtifact" : { "artifactAccount" : "embedded-artifact", "id" : "cf4891ff-6e56-495b-a41e-46f120783ff8", "name" : "match-artifact", "reference" : "+++", "type" : "embedded/base64" }, "useDefaultArtifact" : false, "usePriorArtifact" : false } ], "id" : "8ed4f44b-a072-37e0-88b5-61a63c5668a9", "keepWaitingPipelines" : false, "limitConcurrent" : true, "name" : "Webhook Trigger Base64 Regex", "stages" : [ { "completeOtherBranchesThenFail" : false, "continuePipeline" : false, "expectedArtifacts" : [ { "defaultArtifact" : { "artifactAccount" : "embedded-artifact", "id" : "f435c890-c5b6-4bdb-a733-d788413fd5c1", "type" : "embedded/base64" }, "displayName" : "regex-artifact", "id" : "be22c2c5-6561-4f41-b885-367ef91b8362", "matchArtifact" : { "artifactAccount" : "embedded-artifact", "id" : "cf4891ff-6e56-495b-a41e-46f120783ff8", "name" : "match-artifact", "reference" : "+++", "type" : "embedded/base64" }, "useDefaultArtifact" : false, "usePriorArtifact" : false } ], "failPipeline" : true, "inputArtifacts" : [ { "account" : "embedded-artifact", "id" : "be22c2c5-6561-4f41-b885-367ef91b8362" } ], "name" : "Bake (Manifest)", "namespace" : "my-namespace", "outputName" : "nginx", "refId" : "1", "requisiteStageRefIds" : [ ], "templateRenderer" : "HELM3", "type" : "bakeManifest" } ], "triggers" : [ { "enabled" : true, "expectedArtifactIds" : [ "be22c2c5-6561-4f41-b885-367ef91b8362" ], "id" : "391f8418-3805-406e-82fb-2bb1ea02ff5d", "payloadConstraints" : { }, "source" : "base64-regex-test", "type" : "webhook" } ] } ``` curl with ```bash curl http://localhost:8084/webhooks/webhook/base64-regex-test -d '{"artifacts":[{"artifactAccount":"embedded-artifact","name":"match-artifact","reference":"+++","type":"embedded/base64"}],"expectedArtifactIds":["be22c2c5-6561-4f41-b885-367ef91b8362"],"expectedArtifacts":[{"defaultArtifact":{"artifactAccount":"embedded-artifact","id":"f435c890-c5b6-4bdb-a733-d788413fd5c1","type":"embedded/base64"},"displayName":"regex-artifact","id":"be22c2c5-6561-4f41-b885-367ef91b8362","matchArtifact":{"artifactAccount":"embedded-artifact","id":"cf4891ff-6e56-495b-a41e-46f120783ff8","name":"match-artifact","reference":"+++","type":"embedded/base64"},"useDefaultArtifact":false,"usePriorArtifact":false}]} ``` echo will log this exception ``` java.util.regex.PatternSyntaxException: Dangling meta character '+' near index 0 ``` This commit addresses this error by instead doing a simple string comparison and if that fails, then we do the regex. Signed-off-by: benjamin-j-powell * fixup! fix(artifacts): references containing '+' break matching (#1496) Signed-off-by: benjamin-j-powell --------- Signed-off-by: benjamin-j-powell Co-authored-by: Benevolent Benjamin Powell Co-authored-by: benjamin-j-powell --- .../artifacts/model/ExpectedArtifact.java | 18 +++++++++- .../artifacts/model/ExpectedArtifactTest.java | 33 +++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/model/ExpectedArtifact.java b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/model/ExpectedArtifact.java index 3debe5bfc..cabbc8a9c 100644 --- a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/model/ExpectedArtifact.java +++ b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/model/ExpectedArtifact.java @@ -98,7 +98,23 @@ public boolean matches(Artifact other) { } private boolean matches(@Nullable String us, @Nullable String other) { - return StringUtils.isEmpty(us) || (other != null && patternMatches(us, other)); + if (StringUtils.isEmpty(us)) { + return true; + } + + if (other == null) { + return false; + } + + // The strict equals is mostly to ensure base64 references can be compared + // against each other, since the '+' is a completely valid base64 character. + // The '+' in regex has the meaning of one or more, and will change the + // semantics of comparing two references. + // + // So rather than having references implement its own matching, users may + // rely on matching artifacts with some regex. To be backwards compatible we + // will do a strict comparison as well as pattern matching. + return us.equals(other) || patternMatches(us, other); } /** diff --git a/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/model/ExpectedArtifactTest.java b/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/model/ExpectedArtifactTest.java index 596e6b906..48899833f 100644 --- a/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/model/ExpectedArtifactTest.java +++ b/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/model/ExpectedArtifactTest.java @@ -189,4 +189,37 @@ private static Stream noMatchConstructors() { s -> Artifact.builder().artifactAccount(s).build()) .map(Arguments::of); } + + @ParameterizedTest + @MethodSource("referenceCases") + void testReferenceMatches( + String matchReference, String otherReference, boolean shouldMatch, String caseName) { + ExpectedArtifact expectedArtifact = + ExpectedArtifact.builder() + .id("test") + .matchArtifact( + Artifact.builder() + .type(ArtifactTypes.EMBEDDED_BASE64.getMimeType()) + .reference(matchReference) + .build()) + .build(); + + assertThat( + expectedArtifact.matches( + Artifact.builder() + .type(ArtifactTypes.EMBEDDED_BASE64.getMimeType()) + .reference(otherReference) + .build())) + .as(caseName) + .isEqualTo(shouldMatch); + } + + private static Stream referenceCases() { + return Stream.of( + Arguments.of("SGVsbG8gV29ybGQK", "SGVsbG8gV29ybGQK", true, "simple"), + Arguments.of("SGVsbG8gV29ybGQK", null, false, "other reference as null"), + Arguments.of("SGVsbG8gV29ybGQK", "", false, "other reference is empty"), + Arguments.of("", "SGVsbG8gV29ybGQK", true, "match reference is empty"), + Arguments.of("+++", "+++", true, "valid base64 but invalid regex")); + } } From 5403c6a35425a8ecef09d2d9640e98e088ec0fd7 Mon Sep 17 00:00:00 2001 From: David Byron <82477955+dbyron-sf@users.noreply.github.com> Date: Fri, 10 Nov 2023 14:16:55 -0500 Subject: [PATCH 08/32] refactor(jedis-test): replace use of io.spinnaker.embedded-redis:embedded-redis with testcontainers (#1116) so we no longer need to maintain https://github.com/spinnaker/embedded-redis and because after upgrading my mac to Sonoma (14.1), I see test failures with: Caused by: java.lang.RuntimeException: Can't start redis server. Check logs for details. at redis.embedded.AbstractRedisInstance.awaitRedisServerReady(AbstractRedisInstance.java:60) at redis.embedded.AbstractRedisInstance.start(AbstractRedisInstance.java:36) at redis.embedded.RedisServer.start(RedisServer.java:9) at com.netflix.spinnaker.kork.jedis.EmbeddedRedis.(EmbeddedRedis.java:29) at com.netflix.spinnaker.kork.jedis.EmbeddedRedis.embed(EmbeddedRedis.java:65) at com.netflix.spinnaker.kork.jedis.EmbeddedRedis$embed.call(Unknown Source) The OS upgrade may not be the cause, but testcontainers works, and is generally a cleaner solution, so let's use it. Change the return value of getPool to the more specific JedisPool (from Pool) so it's useful by e.g. gate's RedisTestConfig class. Note that embedded-redis 0.9 was using version [4.0.10 of redis](https://github.com/spinnaker/embedded-redis/blob/v0.9.0/src/main/java/redis/embedded/RedisExecProvider.java#L30-L34). Use 5-alpine to match [fiat](https://github.com/spinnaker/fiat/blob/b64c9ea249bd262c84a7421faf071cabdd3fdf77/fiat-web/src/test/groovy/com/netflix/spinnaker/config/RedisContainerConfig.groovy#L35). --- kork-jedis-test/kork-jedis-test.gradle | 2 +- .../spinnaker/kork/jedis/EmbeddedRedis.java | 56 +++++++------------ .../spinnaker-dependencies.gradle | 1 - 3 files changed, 21 insertions(+), 38 deletions(-) diff --git a/kork-jedis-test/kork-jedis-test.gradle b/kork-jedis-test/kork-jedis-test.gradle index f47572afd..68aa64cef 100644 --- a/kork-jedis-test/kork-jedis-test.gradle +++ b/kork-jedis-test/kork-jedis-test.gradle @@ -4,5 +4,5 @@ dependencies { api(platform(project(":spinnaker-dependencies"))) api "redis.clients:jedis" - api "io.spinnaker.embedded-redis:embedded-redis" + implementation "org.testcontainers:testcontainers" } diff --git a/kork-jedis-test/src/main/java/com/netflix/spinnaker/kork/jedis/EmbeddedRedis.java b/kork-jedis-test/src/main/java/com/netflix/spinnaker/kork/jedis/EmbeddedRedis.java index 2f6c4fd48..b98400382 100644 --- a/kork-jedis-test/src/main/java/com/netflix/spinnaker/kork/jedis/EmbeddedRedis.java +++ b/kork-jedis-test/src/main/java/com/netflix/spinnaker/kork/jedis/EmbeddedRedis.java @@ -1,32 +1,23 @@ package com.netflix.spinnaker.kork.jedis; -import java.io.IOException; -import java.net.ServerSocket; -import java.net.URI; -import java.net.URISyntaxException; +import org.testcontainers.containers.GenericContainer; +import org.testcontainers.utility.DockerImageName; import redis.clients.jedis.Jedis; import redis.clients.jedis.JedisPool; -import redis.clients.jedis.util.Pool; -import redis.embedded.RedisServer; public class EmbeddedRedis implements AutoCloseable { - private final URI connection; - private final RedisServer redisServer; + private static final int REDIS_PORT = 6379; - private Pool jedis; + private final GenericContainer redisContainer; - private EmbeddedRedis(int port) throws IOException, URISyntaxException { - this.connection = URI.create(String.format("redis://127.0.0.1:%d/0", port)); - this.redisServer = - RedisServer.builder() - .port(port) - .setting("bind 127.0.0.1") - .setting("appendonly no") - .setting("save \"\"") - .setting("databases 1") - .build(); - this.redisServer.start(); + private JedisPool jedis; + + private EmbeddedRedis() { + redisContainer = + new GenericContainer<>(DockerImageName.parse("library/redis:5-alpine")) + .withExposedPorts(REDIS_PORT); + redisContainer.start(); } @Override @@ -35,20 +26,20 @@ public void close() { } public void destroy() { - try { - this.redisServer.stop(); - } catch (Exception e) { - throw new RuntimeException(e); - } + redisContainer.stop(); + } + + public String getHost() { + return redisContainer.getHost(); } public int getPort() { - return redisServer.ports().get(0); + return redisContainer.getMappedPort(REDIS_PORT); } - public Pool getPool() { + public JedisPool getPool() { if (jedis == null) { - jedis = new JedisPool(connection); + jedis = new JedisPool(getHost(), getPort()); } return jedis; } @@ -58,13 +49,6 @@ public Jedis getJedis() { } public static EmbeddedRedis embed() { - try { - ServerSocket serverSocket = new ServerSocket(0); - int port = serverSocket.getLocalPort(); - serverSocket.close(); - return new EmbeddedRedis(port); - } catch (IOException | URISyntaxException e) { - throw new RuntimeException("Failed to create embedded Redis", e); - } + return new EmbeddedRedis(); } } diff --git a/spinnaker-dependencies/spinnaker-dependencies.gradle b/spinnaker-dependencies/spinnaker-dependencies.gradle index 317a82101..253f24018 100644 --- a/spinnaker-dependencies/spinnaker-dependencies.gradle +++ b/spinnaker-dependencies/spinnaker-dependencies.gradle @@ -125,7 +125,6 @@ dependencies { api("com.netflix.spectator:spectator-web-spring:${versions.spectator}") api("com.netflix.spectator:spectator-reg-micrometer:${versions.spectator}") api("com.nimbusds:nimbus-jose-jwt:7.9") - api("io.spinnaker.embedded-redis:embedded-redis:0.9.0") api("com.nhaarman.mockitokotlin2:mockito-kotlin:2.2.0") api("com.nhaarman:mockito-kotlin:1.6.0") api("com.ninja-squad:springmockk:2.0.3") From 7bd9a472d09d5f733af11afe223eeaa68d9daa8c Mon Sep 17 00:00:00 2001 From: Sandesh <30489233+j-sandy@users.noreply.github.com> Date: Mon, 13 Nov 2023 23:18:59 +0530 Subject: [PATCH 09/32] chore(dependency): upgrade spring boot to 2.5.15 and related cleanup (#1115) Removed constraint of ch.qos.logback because spring boot 2.5.15 brings logback:1.2.12. Removed platform dependency com.fasterxml.jackson:jackson-bom because spring boot 2.5.15 brings jackson:2.12.7.20221012 Groovy get transitively upgraded to 3.0.17 https://repo1.maven.org/maven2/org/springframework/boot/spring-boot-dependencies/2.5.15/spring-boot-dependencies-2.5.15.pom --- .../spinnaker-dependencies.gradle | 25 +------------------ 1 file changed, 1 insertion(+), 24 deletions(-) diff --git a/spinnaker-dependencies/spinnaker-dependencies.gradle b/spinnaker-dependencies/spinnaker-dependencies.gradle index 253f24018..f2f55178d 100644 --- a/spinnaker-dependencies/spinnaker-dependencies.gradle +++ b/spinnaker-dependencies/spinnaker-dependencies.gradle @@ -15,10 +15,6 @@ ext { jooq : "3.13.6", jsch : "0.1.54", jschAgentProxy : "0.0.9", - // spring boot 2.5.14 specifies logback 1.2.11, but a rosco test hung with - // 1.2.11 from https://jira.qos.ch/browse/LOGBACK-1623 so pinning it to 1.2.12 - // until spring boot upgrade 2.5.15 or above. - logback : "1.2.12", protobuf : "3.21.12", okhttp : "2.7.5", // CVE-2016-2402 okhttp3 : "4.9.3", @@ -28,7 +24,7 @@ ext { spectator : "1.0.6", spek : "1.1.5", spek2 : "2.0.9", - springBoot : "2.5.14", + springBoot : "2.5.15", springCloud : "2020.0.6", springfoxSwagger : "2.9.2", swagger : "1.5.20", //this should stay in sync with what springfoxSwagger expects @@ -58,7 +54,6 @@ dependencies { //kotlinVersion comes from gradle.properties since we have kotlin code in // this project and need to configure gradle plugins etc. api(platform("org.jetbrains.kotlin:kotlin-bom:$kotlinVersion")) - api(platform("com.fasterxml.jackson:jackson-bom:2.12.7.20221012")) api(platform("io.zipkin.brave:brave-bom:${versions.brave}")) api(platform("org.springframework.boot:spring-boot-dependencies:${versions.springBoot}")) api(platform("com.amazonaws:aws-java-sdk-bom:${versions.aws}")) @@ -75,24 +70,6 @@ dependencies { constraints { api("cglib:cglib-nodep:3.3.0") - //A bug is reported in 1.2.11 and fixed in 1.2.12. - //So pinning the version to 1.2.12 until spring boot upgrade to 2.5.15 or above. - //[https://jira.qos.ch/browse/LOGBACK-1623] - api("ch.qos.logback:logback-core"){ - version { - strictly "${versions.logback}" - } - } - api("ch.qos.logback:logback-classic"){ - version { - strictly "${versions.logback}" - } - } - api("ch.qos.logback:logback-access"){ - version { - strictly "${versions.logback}" - } - } api("com.amazonaws:aws-java-sdk:${versions.aws}") api("com.google.api-client:google-api-client:1.30.10") // TODO: Track update for CVE-2020-7692, reanalysis pending. api("com.google.apis:google-api-services-admin-directory:directory_v1-rev105-1.25.0") From 2fdb42ec4e80a6acecbd814ffd99ef72d7f49382 Mon Sep 17 00:00:00 2001 From: Kiran Godishala <53332225+kirangodishala@users.noreply.github.com> Date: Wed, 15 Nov 2023 01:35:12 +0530 Subject: [PATCH 10/32] chore(dependencies): make snakeyaml version strictly to 1.27 (#1118) --- kork-runtime/kork-runtime.gradle | 2 ++ spinnaker-dependencies/spinnaker-dependencies.gradle | 7 ++++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/kork-runtime/kork-runtime.gradle b/kork-runtime/kork-runtime.gradle index a8fd2d6af..dba679ede 100644 --- a/kork-runtime/kork-runtime.gradle +++ b/kork-runtime/kork-runtime.gradle @@ -1,4 +1,6 @@ dependencies { + runtimeOnly(platform(project(":spinnaker-dependencies"))) + // Add each included runtime project as a runtime dependency gradle.includedRuntimeProjects.each { runtimeOnly project(it) diff --git a/spinnaker-dependencies/spinnaker-dependencies.gradle b/spinnaker-dependencies/spinnaker-dependencies.gradle index f2f55178d..b8c5e29b0 100644 --- a/spinnaker-dependencies/spinnaker-dependencies.gradle +++ b/spinnaker-dependencies/spinnaker-dependencies.gradle @@ -181,7 +181,12 @@ dependencies { // when we upgrade to spring boot 2.6.x. It's safe to upgrade beyond 1.29 // with spring boot >= 2.6.12. See // https://github.com/spring-projects/spring-boot/issues/32228#issue-136185850.0. - api("org.yaml:snakeyaml:1.27") + // making it strict as some of the modules have it resolved to higher versions (ex: kork-secrets-gcp to 1.30) + api("org.yaml:snakeyaml") { + version { + strictly "1.27" + } + } api("org.springdoc:springdoc-openapi-webmvc-core:${versions.openapi}") api("org.springdoc:springdoc-openapi-kotlin:${versions.openapi}") api("org.springframework.boot:spring-boot-configuration-processor:${versions.springBoot}") From 1e904f19c44166ad4fc00288e1d56435b770608a Mon Sep 17 00:00:00 2001 From: Kiran Godishala <53332225+kirangodishala@users.noreply.github.com> Date: Mon, 20 Nov 2023 22:22:28 +0530 Subject: [PATCH 11/32] chore(dependencies): upgrade liquibase to 4.24.0 (#1117) --- .../spinnaker/kork/sql/test/SqlTestUtil.java | 2 +- .../kork/sql/config/SqlMigrationProperties.kt | 7 ++++- .../sql/migration/SpringLiquibaseProxy.kt | 26 +++++++++++-------- .../spinnaker/kork/sql/SpringStartupTests.kt | 2 ++ .../src/test/resources/application-test.yml | 3 +++ .../test/resources/db/changelog-master.yml | 22 +++++++++++++++- .../spinnaker-dependencies.gradle | 2 +- 7 files changed, 49 insertions(+), 15 deletions(-) diff --git a/kork-sql-test/src/main/java/com/netflix/spinnaker/kork/sql/test/SqlTestUtil.java b/kork-sql-test/src/main/java/com/netflix/spinnaker/kork/sql/test/SqlTestUtil.java index 1e55f132f..2ee16db68 100644 --- a/kork-sql-test/src/main/java/com/netflix/spinnaker/kork/sql/test/SqlTestUtil.java +++ b/kork-sql-test/src/main/java/com/netflix/spinnaker/kork/sql/test/SqlTestUtil.java @@ -223,7 +223,7 @@ public static TestDatabase initDatabase( Liquibase migrate; try { - DatabaseChangeLog changeLog = new DatabaseChangeLog(); + DatabaseChangeLog changeLog = new DatabaseChangeLog("db/changelog/"); changeLog.setChangeLogParameters( new ChangeLogParameters( diff --git a/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/config/SqlMigrationProperties.kt b/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/config/SqlMigrationProperties.kt index 7a52c2025..e88a7c578 100644 --- a/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/config/SqlMigrationProperties.kt +++ b/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/config/SqlMigrationProperties.kt @@ -15,6 +15,8 @@ */ package com.netflix.spinnaker.kork.sql.config +import liquibase.GlobalConfiguration + /** * Defines the configuration properties for connecting to a SQL database for schema migration purposes. * @@ -23,11 +25,14 @@ package com.netflix.spinnaker.kork.sql.config * @param password The password to authenticate the [user] * @param driver The JDBC driver name * @param additionalChangeLogs A list of additional change log paths. This is useful for libraries and extensions. + * @param duplicateFileMode flag to handle if multiple files are found in the search path that have duplicate paths. */ data class SqlMigrationProperties( var jdbcUrl: String? = null, var user: String? = null, var password: String? = null, var driver: String? = null, - var additionalChangeLogs: List = mutableListOf() + var additionalChangeLogs: List = mutableListOf(), + var duplicateFileMode: GlobalConfiguration.DuplicateFileMode = GlobalConfiguration.DUPLICATE_FILE_MODE.defaultValue + ) diff --git a/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/migration/SpringLiquibaseProxy.kt b/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/migration/SpringLiquibaseProxy.kt index 3cf1a7935..574ceb708 100644 --- a/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/migration/SpringLiquibaseProxy.kt +++ b/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/migration/SpringLiquibaseProxy.kt @@ -16,6 +16,8 @@ package com.netflix.spinnaker.kork.sql.migration import com.netflix.spinnaker.kork.sql.config.SqlMigrationProperties +import liquibase.GlobalConfiguration +import liquibase.Scope import javax.sql.DataSource import liquibase.integration.spring.SpringLiquibase import org.springframework.jdbc.datasource.SingleConnectionDataSource @@ -52,18 +54,20 @@ class SpringLiquibaseProxy( super.afterPropertiesSet() // Then if anything else has been defined, do that afterwards - (sqlMigrationProperties.additionalChangeLogs + korkAdditionalChangelogs) - .map { - SpringLiquibase().apply { - changeLog = "classpath:$it" - dataSource = createDataSource() - resourceLoader = this@SpringLiquibaseProxy.resourceLoader - shouldRun = !sqlReadOnly + Scope.child(GlobalConfiguration.DUPLICATE_FILE_MODE.key, sqlMigrationProperties.duplicateFileMode) { + (sqlMigrationProperties.additionalChangeLogs + korkAdditionalChangelogs) + .map { + SpringLiquibase().apply { + changeLog = "classpath:$it" + dataSource = createDataSource() + resourceLoader = this@SpringLiquibaseProxy.resourceLoader + shouldRun = !sqlReadOnly + } } - } - .forEach { - it.afterPropertiesSet() - } + .forEach { + it.afterPropertiesSet() + } + } } private fun createDataSource(): DataSource = diff --git a/kork-sql/src/test/kotlin/com/netflix/spinnaker/kork/sql/SpringStartupTests.kt b/kork-sql/src/test/kotlin/com/netflix/spinnaker/kork/sql/SpringStartupTests.kt index 8310f98da..d23df51ee 100644 --- a/kork-sql/src/test/kotlin/com/netflix/spinnaker/kork/sql/SpringStartupTests.kt +++ b/kork-sql/src/test/kotlin/com/netflix/spinnaker/kork/sql/SpringStartupTests.kt @@ -30,6 +30,7 @@ import org.springframework.boot.autoconfigure.SpringBootApplication import org.springframework.boot.test.context.SpringBootTest import org.springframework.context.ApplicationContext import org.springframework.context.annotation.Import +import org.springframework.test.context.TestPropertySource import org.springframework.test.context.junit.jupiter.SpringExtension import strikt.api.expectThat import strikt.assertions.isA @@ -43,6 +44,7 @@ import strikt.assertions.isNotNull "sql.enabled=true", "sql.migration.jdbcUrl=jdbc:h2:mem:test", "sql.migration.dialect=H2", + "sql.migration.duplicateFileMode=WARN", "sql.connectionPool.jdbcUrl=jdbc:h2:mem:test", "sql.connectionPool.dialect=H2" ] diff --git a/kork-sql/src/test/resources/application-test.yml b/kork-sql/src/test/resources/application-test.yml index c3b01d241..662c6420d 100644 --- a/kork-sql/src/test/resources/application-test.yml +++ b/kork-sql/src/test/resources/application-test.yml @@ -26,10 +26,12 @@ sql: jdbcUrl: "jdbc:h2:mem:test" user: password: + duplicateFileMode: WARN secondaryMigration: jdbcUrl: "jdbc:h2:mem:test" user: password: + duplicateFileMode: WARN --- spring: @@ -52,3 +54,4 @@ sql: jdbcUrl: "jdbc:h2:mem:test" user: password: + duplicateFileMode: WARN diff --git a/kork-sql/src/test/resources/db/changelog-master.yml b/kork-sql/src/test/resources/db/changelog-master.yml index daf9f00fc..3d2c63d3b 100644 --- a/kork-sql/src/test/resources/db/changelog-master.yml +++ b/kork-sql/src/test/resources/db/changelog-master.yml @@ -1 +1,21 @@ -databaseChangeLog: [] +databaseChangeLog: + - changeSet: + id: create-sample-table + author: kirangodishala + changes: + - createTable: + tableName: sample + columns: + - column: + name: id + type: boolean + constraints: + primaryKey: true + nullable: false + - modifySql: + dbms: mysql + append: + value: " engine innodb" + rollback: + - dropTable: + tableName: sample diff --git a/spinnaker-dependencies/spinnaker-dependencies.gradle b/spinnaker-dependencies/spinnaker-dependencies.gradle index b8c5e29b0..8f62aeb1d 100644 --- a/spinnaker-dependencies/spinnaker-dependencies.gradle +++ b/spinnaker-dependencies/spinnaker-dependencies.gradle @@ -165,7 +165,7 @@ dependencies { // containing `afterColumn` construct, with a validation error for postgresql. So pin with 3.10.3 api("org.liquibase:liquibase-core"){ version { - strictly "3.10.3" + strictly "4.24.0" } } api("org.objenesis:objenesis:2.5.1") From 65d64c908c1de16499dec4efdbd86fd7e5c875cc Mon Sep 17 00:00:00 2001 From: David Byron <82477955+dbyron-sf@users.noreply.github.com> Date: Mon, 20 Nov 2023 16:05:07 -0500 Subject: [PATCH 12/32] fix(artifacts): always retrieve s3 artifacts even when artifact-store.s3.enabled is false (#1120) * test(artifacts): demonstrate current behavior of artifact-store enable flags * fix(artifacts): clear the ArtifactStore's instance when cleaning up the spring context to prevent objects that aren't managed by spring from using a stale/destroyed object. This also has a side effect of allowing one ArtifactStore bean per spring context as opposed to one per java process. This allows not only tests to configure multiple ArtifactStore beans, but also spring cloud config to update the ArtifactStore bean as part of refreshing the spring context. * fix(artifacts): always retrieve s3 artifacts even when artifact-store.s3.enabled is false so changes to the config value while pipelines are executing, or actions on completed pipelines (e.g. "Baked Manifest YAML") work without disruption. Consider this sequence of events: 1. artifact-store.s3.enabled is true 2. a pipeline starts executing, 3. the pipeline execution context contains an artifact store reference 4. artifact-store.s3.enabled changes to false 5. the pipeline continues to execute and attempts to retrieve the artifact store reference Before this change, step 5 would fail. After this change, it succeeds. * refactor(artifacts): introduce S3ArtifactStore helper class to remove duplicate ENFORCE_PERMS_KEY * feat(artifacts): remove artifact-store.enabled flag so ArtifactStoreConfiguration only uses artifact-store.s3.enabled to determine what beans to create. Given the desire to always support artifact store references once they've been created, it doesn't make sense to disable the artifact store completely. Even in a future with multiple artifact store implementations (e.g. other cloud providers), flags for each implementation (e.g. artifact-store.s3.enabled) seem sufficient. This does make rosco do a little more work, by using EmbeddedArtifactSerializer even when artifact-store.s3.enabled is false, but the overall simplification seems worth it. Note that if no artifact-store.s3 properties are defined, ArtifactStoreConfiguration still creates an S3ArtifactStoreGetter, which uses an S3Client. However, without artifact-store.s3.region defined (and no AWS_REGION env var, nor a region in the other places the aws sdk looks), it's not possible to construct an S3Client. So, treat the S3Client as Optional, and fail attempts to retrieve from s3 if it's not present. * feat(artifacts): remove call to ArtifactStore.setInstance from ArtifactStore bean Without this, spring tests that use ArtifactStoreConfiguration often fail. More specifically, because spring tests cache contexts (see https://docs.spring.io/spring-framework/reference/testing/testcontext-framework/ctx-management/caching.html), the spring context doesn't get destroyed, so ArtifactStore bean doesn't get destroyed, and a second context then fails to start due to the IllegalStateException in ArtifactStore.setInstance. This puts the responsibility of calling ArtifactStore.setInstance on the code that actually needs it. In this case, that's orca because it uses ExpectedArtifact and ExpressionsSupport. * feat(artifacts): change ArtifactStore.setInstance to allow multiple calls but warn when they happen. Those warnings could help reveal bugs if the instance changes in real running code. Warning instead of throwing an exception makes it easier for caching of spring contexts during tests, such that tests don't need to explicitly call ArtifactStore.clearInstance. Since those calls are no longer necessary, this commit also removes ArtifactStore.clearInstance. * refactor(artifacts): construct s3-related artifact store beans when artifact-store.type=s3 and introduce NoopArtifactStoreGetter when there's no artifact-store.type specified. Split s3-related configuration into S3ArtifactStoreConfiguration to simplify ArtifactStoreConfiguration and pave the way for additional artifact store implementations. Simplify S3ArtifactStoreGetter as its S3Client is no longer optional. Yes, this leaves artifact-store.s3.enabled in a bit of a strange place. When it's false, and artifact-store.type=s3, S3ArtifactStoreGetter is still enabled. It could be that renaming "enabled" to "enable-store" is the way forward, but there's been enough juggling in this PR already so I'm leaving that for another time. * fix(artifacts): clarify error message in NoopArtifactStoreGetter --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> --- kork-artifacts/kork-artifacts.gradle | 1 + .../artifactstore/ArtifactStore.java | 44 ++-- .../ArtifactStoreConfiguration.java | 85 ++----- .../ArtifactStoreConfigurationProperties.java | 4 + .../artifactstore/ArtifactStoreGetter.java | 30 +++ .../artifactstore/ArtifactStoreStorer.java | 27 ++ .../NoopArtifactStoreGetter.java | 29 +++ .../NoopArtifactStoreStorer.java | 26 ++ .../artifactstore/s3/S3ArtifactStore.java | 231 +----------------- .../s3/S3ArtifactStoreConfiguration.java | 107 ++++++++ .../s3/S3ArtifactStoreGetter.java | 115 +++++++++ .../s3/S3ArtifactStoreStorer.java | 173 +++++++++++++ .../ArtifactDeserializerTest.java | 35 ++- .../ArtifactStoreConfigurationTest.java | 48 ++++ .../artifactstore/ArtifactStoreTest.java | 60 +++++ .../EmbeddedArtifactSerializerTest.java | 12 +- .../s3/S3ArtifactStoreConfigurationTest.java | 81 ++++++ ...st.java => S3ArtifactStoreStorerTest.java} | 8 +- .../expressions/ExpressionsSupportTest.java | 4 + 19 files changed, 795 insertions(+), 325 deletions(-) create mode 100644 kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreGetter.java create mode 100644 kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreStorer.java create mode 100644 kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/NoopArtifactStoreGetter.java create mode 100644 kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/NoopArtifactStoreStorer.java create mode 100644 kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreConfiguration.java create mode 100644 kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreGetter.java create mode 100644 kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreStorer.java create mode 100644 kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreConfigurationTest.java create mode 100644 kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreTest.java create mode 100644 kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreConfigurationTest.java rename kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/{S3ArtifactStoreTest.java => S3ArtifactStoreStorerTest.java} (92%) diff --git a/kork-artifacts/kork-artifacts.gradle b/kork-artifacts/kork-artifacts.gradle index 6d1b75509..a6a8b0fe9 100644 --- a/kork-artifacts/kork-artifacts.gradle +++ b/kork-artifacts/kork-artifacts.gradle @@ -20,6 +20,7 @@ dependencies { testImplementation "org.junit.jupiter:junit-jupiter-api" testImplementation "org.junit.jupiter:junit-jupiter-params" testImplementation "org.mockito:mockito-core" + testImplementation "org.springframework.boot:spring-boot-starter-test" testImplementation project(":kork-core") testRuntimeOnly "org.junit.jupiter:junit-jupiter-engine" } diff --git a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStore.java b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStore.java index 65b1dc62e..5e5ea591a 100644 --- a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStore.java +++ b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStore.java @@ -16,32 +16,46 @@ package com.netflix.spinnaker.kork.artifacts.artifactstore; import com.netflix.spinnaker.kork.artifacts.model.Artifact; -import java.util.concurrent.atomic.AtomicBoolean; import lombok.Getter; +import lombok.extern.log4j.Log4j2; -/** - * ArtifactStore is an interface that allows for different types of artifact storage to be used - * during runtime - */ -public abstract class ArtifactStore { - /** ensures the singleton has only been set once */ - private static final AtomicBoolean singletonSet = new AtomicBoolean(false); +/** ArtifactStore allows for different types of artifact storage to be used during runtime */ +@Log4j2 +public class ArtifactStore implements ArtifactStoreGetter, ArtifactStoreStorer { + @Getter private static volatile ArtifactStore instance = null; + + private final ArtifactStoreGetter artifactStoreGetter; - @Getter private static ArtifactStore instance = null; + private final ArtifactStoreStorer artifactStoreStorer; + + public ArtifactStore( + ArtifactStoreGetter artifactStoreGetter, ArtifactStoreStorer artifactStoreStorer) { + this.artifactStoreGetter = artifactStoreGetter; + this.artifactStoreStorer = artifactStoreStorer; + } + + /** Store an artifact in the artifact store */ + public Artifact store(Artifact artifact) { + return artifactStoreStorer.store(artifact); + } - public abstract Artifact store(Artifact artifact); /** * get is used to return an artifact with some id, while also decorating that artifact with any * necessary fields needed which should be then be returned by the artifact store. */ - public abstract Artifact get(ArtifactReferenceURI uri, ArtifactDecorator... decorators); + public Artifact get(ArtifactReferenceURI uri, ArtifactDecorator... decorators) { + return artifactStoreGetter.get(uri, decorators); + } public static void setInstance(ArtifactStore storage) { - if (!singletonSet.compareAndSet(false, true)) { - throw new IllegalStateException("Multiple attempts at setting ArtifactStore's singleton"); - } + synchronized (ArtifactStore.class) { + if (instance == null) { + instance = storage; + return; + } - ArtifactStore.instance = storage; + log.warn("Multiple attempts in setting the singleton artifact store"); + } } public boolean isArtifactURI(String value) { diff --git a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreConfiguration.java b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreConfiguration.java index 0b122384a..c2a0b6d60 100644 --- a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreConfiguration.java +++ b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreConfiguration.java @@ -16,29 +16,16 @@ package com.netflix.spinnaker.kork.artifacts.artifactstore; import com.fasterxml.jackson.databind.ObjectMapper; -import com.netflix.spinnaker.kork.artifacts.artifactstore.s3.S3ArtifactStore; -import java.net.URI; -import java.util.Optional; -import lombok.extern.log4j.Log4j2; -import org.springframework.beans.factory.annotation.Qualifier; -import org.springframework.boot.autoconfigure.condition.ConditionalOnExpression; +import com.netflix.spinnaker.kork.artifacts.artifactstore.s3.S3ArtifactStoreConfiguration; +import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; import org.springframework.boot.context.properties.EnableConfigurationProperties; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; -import org.springframework.security.access.PermissionEvaluator; -import software.amazon.awssdk.auth.credentials.AnonymousCredentialsProvider; -import software.amazon.awssdk.auth.credentials.AwsBasicCredentials; -import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider; -import software.amazon.awssdk.auth.credentials.ProfileCredentialsProvider; -import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider; -import software.amazon.awssdk.regions.Region; -import software.amazon.awssdk.services.s3.S3Client; -import software.amazon.awssdk.services.s3.S3ClientBuilder; +import org.springframework.context.annotation.Import; @Configuration -@ConditionalOnExpression("${artifact-store.enabled:false}") @EnableConfigurationProperties(ArtifactStoreConfigurationProperties.class) -@Log4j2 +@Import(S3ArtifactStoreConfiguration.class) public class ArtifactStoreConfiguration { /** * this is strictly used due to Spring and Jackson not behaving nicely together. @@ -55,63 +42,23 @@ public ArtifactStoreURIBuilder artifactStoreURIBuilder() { return new ArtifactStoreURISHA256Builder(); } + @ConditionalOnMissingBean(ArtifactStoreGetter.class) @Bean - @ConditionalOnExpression("${artifact-store.s3.enabled:false}") - public ArtifactStore s3ArtifactStore( - Optional permissionEvaluator, - ArtifactStoreConfigurationProperties properties, - @Qualifier("artifactS3Client") S3Client s3Client, - ArtifactStoreURIBuilder artifactStoreURIBuilder) { - - if (permissionEvaluator.isEmpty()) { - log.warn( - "PermissionEvaluator is not present. This means anyone will be able to access any artifact in the store."); - } - - ArtifactStore storage = - new S3ArtifactStore( - s3Client, - permissionEvaluator.orElse(null), - properties.getS3().getBucket(), - artifactStoreURIBuilder, - properties.getApplicationsRegex()); - - ArtifactStore.setInstance(storage); - return storage; + public ArtifactStoreGetter artifactStoreGetter() { + return new NoopArtifactStoreGetter(); } + @ConditionalOnMissingBean(ArtifactStoreStorer.class) @Bean - @ConditionalOnExpression("${artifact-store.s3.enabled:false}") - public S3Client artifactS3Client(ArtifactStoreConfigurationProperties properties) { - S3ClientBuilder builder = S3Client.builder(); - ArtifactStoreConfigurationProperties.S3ClientConfig config = properties.getS3(); - - // Overwriting the URL is primarily used for S3 compatible object stores - // like seaweedfs - if (config.getUrl() != null) { - builder = - builder - .credentialsProvider(getCredentialsProvider(config)) - .forcePathStyle(config.isForcePathStyle()) - .endpointOverride(URI.create(config.getUrl())); - } else if (config.getProfile() != null) { - builder = builder.credentialsProvider(ProfileCredentialsProvider.create(config.getProfile())); - } - - if (config.getRegion() != null) { - builder = builder.region(Region.of(config.getRegion())); - } - - return builder.build(); + public ArtifactStoreStorer artifactStoreStorer() { + return new NoopArtifactStoreStorer(); } - private AwsCredentialsProvider getCredentialsProvider( - ArtifactStoreConfigurationProperties.S3ClientConfig config) { - if (config.getAccessKey() != null) { - return StaticCredentialsProvider.create( - AwsBasicCredentials.create(config.getAccessKey(), config.getSecretKey())); - } else { - return AnonymousCredentialsProvider.create(); - } + @Bean + public ArtifactStore artifactStore( + ArtifactStoreGetter artifactStoreGetter, ArtifactStoreStorer artifactStoreStorer) { + ArtifactStore artifactStore = new ArtifactStore(artifactStoreGetter, artifactStoreStorer); + ArtifactStore.setInstance(artifactStore); + return artifactStore; } } diff --git a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreConfigurationProperties.java b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreConfigurationProperties.java index e3fbf51be..019da7ce2 100644 --- a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreConfigurationProperties.java +++ b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreConfigurationProperties.java @@ -22,6 +22,10 @@ @ConfigurationProperties("artifact-store") public class ArtifactStoreConfigurationProperties { private String applicationsRegex = null; + + /** The type of artifact store to use (e.g. s3). */ + private String type = null; + /** Configuration for an s3 client which will utilize credentials in the AWS credentials file. */ @Data public static class S3ClientConfig { diff --git a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreGetter.java b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreGetter.java new file mode 100644 index 000000000..3a4fd6cef --- /dev/null +++ b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreGetter.java @@ -0,0 +1,30 @@ +/* + * Copyright 2023 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.kork.artifacts.artifactstore; + +import com.netflix.spinnaker.kork.artifacts.model.Artifact; + +/** + * ArtifactStoreGetter is an interface that allows for different types of artifact storage to be + * used during runtime. + */ +public interface ArtifactStoreGetter { + /** + * get is used to return an artifact with some id, while also decorating that artifact with any + * necessary fields needed which should be then be returned by the artifact store. + */ + public Artifact get(ArtifactReferenceURI uri, ArtifactDecorator... decorators); +} diff --git a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreStorer.java b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreStorer.java new file mode 100644 index 000000000..d817ac351 --- /dev/null +++ b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreStorer.java @@ -0,0 +1,27 @@ +/* + * Copyright 2023 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.kork.artifacts.artifactstore; + +import com.netflix.spinnaker.kork.artifacts.model.Artifact; + +/** + * ArtifactStoreStorer is an interface that allows for different types of artifact storage to be + * used during runtime. + */ +public interface ArtifactStoreStorer { + + Artifact store(Artifact artifact); +} diff --git a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/NoopArtifactStoreGetter.java b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/NoopArtifactStoreGetter.java new file mode 100644 index 000000000..e1b8dbeec --- /dev/null +++ b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/NoopArtifactStoreGetter.java @@ -0,0 +1,29 @@ +/* + * Copyright 2023 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.kork.artifacts.artifactstore; + +import com.netflix.spinnaker.kork.artifacts.model.Artifact; + +/** A no-op ArtifactStoreGetter. In other words, don't actually get the artifact. */ +public class NoopArtifactStoreGetter implements ArtifactStoreGetter { + + public Artifact get(ArtifactReferenceURI uri, ArtifactDecorator... decorators) { + throw new IllegalStateException( + "unable to retrieve artifact " + + uri.toString() + + " since there's no artifact store getter configured"); + } +} diff --git a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/NoopArtifactStoreStorer.java b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/NoopArtifactStoreStorer.java new file mode 100644 index 000000000..b441dfd9d --- /dev/null +++ b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/NoopArtifactStoreStorer.java @@ -0,0 +1,26 @@ +/* + * Copyright 2023 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.kork.artifacts.artifactstore; + +import com.netflix.spinnaker.kork.artifacts.model.Artifact; + +/** A no-op ArtifactStoreStorer. In other words, don't actually store the artifact. */ +public class NoopArtifactStoreStorer implements ArtifactStoreStorer { + + public Artifact store(Artifact artifact) { + return artifact; + } +} diff --git a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStore.java b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStore.java index 14e807d09..a73b2fbe2 100644 --- a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStore.java +++ b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStore.java @@ -15,233 +15,8 @@ */ package com.netflix.spinnaker.kork.artifacts.artifactstore.s3; -import com.netflix.spinnaker.kork.artifacts.ArtifactTypes; -import com.netflix.spinnaker.kork.artifacts.artifactstore.ArtifactDecorator; -import com.netflix.spinnaker.kork.artifacts.artifactstore.ArtifactReferenceURI; -import com.netflix.spinnaker.kork.artifacts.artifactstore.ArtifactStore; -import com.netflix.spinnaker.kork.artifacts.artifactstore.ArtifactStoreURIBuilder; -import com.netflix.spinnaker.kork.artifacts.model.Artifact; -import com.netflix.spinnaker.kork.exceptions.SpinnakerException; -import com.netflix.spinnaker.security.AuthenticatedRequest; -import java.util.Base64; -import java.util.NoSuchElementException; -import java.util.regex.Pattern; -import lombok.extern.log4j.Log4j2; -import org.apache.http.HttpStatus; -import org.springframework.security.access.PermissionEvaluator; -import org.springframework.security.authentication.AuthenticationServiceException; -import org.springframework.security.core.Authentication; -import org.springframework.security.core.context.SecurityContextHolder; -import software.amazon.awssdk.core.ResponseBytes; -import software.amazon.awssdk.core.sync.RequestBody; -import software.amazon.awssdk.services.s3.S3Client; -import software.amazon.awssdk.services.s3.model.GetObjectRequest; -import software.amazon.awssdk.services.s3.model.GetObjectResponse; -import software.amazon.awssdk.services.s3.model.GetObjectTaggingRequest; -import software.amazon.awssdk.services.s3.model.GetObjectTaggingResponse; -import software.amazon.awssdk.services.s3.model.HeadObjectRequest; -import software.amazon.awssdk.services.s3.model.NoSuchKeyException; -import software.amazon.awssdk.services.s3.model.PutObjectRequest; -import software.amazon.awssdk.services.s3.model.S3Exception; -import software.amazon.awssdk.services.s3.model.Tag; -import software.amazon.awssdk.services.s3.model.Tagging; +public final class S3ArtifactStore { + public static final String ENFORCE_PERMS_KEY = "application"; -/** - * S3ArtifactStore will store artifacts in a s3 compatible service - * - *

Note: It is very important that the S3 bucket has object lock on it to prevent multiple writes - * {@see https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-lock-overview.html} - */ -@Log4j2 -public class S3ArtifactStore extends ArtifactStore { - private final S3Client s3Client; - private final PermissionEvaluator permissionEvaluator; - private final String bucket; - private final ArtifactStoreURIBuilder uriBuilder; - private final String applicationsRegex; - private static final String ENFORCE_PERMS_KEY = "application"; - - public S3ArtifactStore( - S3Client s3Client, - PermissionEvaluator permissionEvaluator, - String bucket, - ArtifactStoreURIBuilder uriBuilder, - String applicationsRegex) { - this.s3Client = s3Client; - this.bucket = bucket; - this.permissionEvaluator = permissionEvaluator; - this.uriBuilder = uriBuilder; - this.applicationsRegex = applicationsRegex; - } - - /** - * Will store the artifact using the {@link #s3Client} in some {@link #bucket} - * - *

This method also persists "permissions" by storing the execution id that made the original - * store call. In the event a service wants to retrieve said artifact, they will also need to - * provide the proper execution id - */ - @Override - public Artifact store(Artifact artifact) { - String application = AuthenticatedRequest.getSpinnakerApplication().orElse(null); - if (application == null) { - log.warn("failed to retrieve application from request artifact={}", artifact.getName()); - return artifact; - } - - if (applicationsRegex != null && !Pattern.matches(applicationsRegex, application)) { - return artifact; - } - - ArtifactReferenceURI ref = uriBuilder.buildArtifactURI(application, artifact); - Artifact remoteArtifact = - artifact.toBuilder() - .type(ArtifactTypes.REMOTE_BASE64.getMimeType()) - .reference(ref.uri()) - .build(); - - if (objectExists(ref)) { - return remoteArtifact; - } - - // purpose of tagging is to ensure some sort of identity is persisted to - // enforce permissions when retrieving the artifact - Tag accountTag = Tag.builder().key(ENFORCE_PERMS_KEY).value(application).build(); - - PutObjectRequest request = - PutObjectRequest.builder() - .bucket(bucket) - .key(ref.paths()) - .tagging(Tagging.builder().tagSet(accountTag).build()) - .build(); - - s3Client.putObject(request, RequestBody.fromBytes(getReferenceAsBytes(artifact))); - return remoteArtifact; - } - - private byte[] getReferenceAsBytes(Artifact artifact) { - String reference = artifact.getReference(); - if (reference == null) { - throw new IllegalArgumentException("reference cannot be null"); - } - - String type = artifact.getType(); - if (type != null && type.endsWith("/base64")) { - return Base64.getDecoder().decode(reference); - } - - return reference.getBytes(); - } - - /** - * get will return the Artifact with the provided id, and will lastly run the {@link - * ArtifactDecorator} to further populate the artifact for returning - */ - @Override - public Artifact get(ArtifactReferenceURI uri, ArtifactDecorator... decorators) { - hasAuthorization( - uri, - AuthenticatedRequest.getSpinnakerUser() - .orElseThrow( - () -> new NoSuchElementException("Could not authenticate due to missing user id"))); - - GetObjectRequest request = GetObjectRequest.builder().bucket(bucket).key(uri.paths()).build(); - - ResponseBytes resp = s3Client.getObjectAsBytes(request); - Artifact.ArtifactBuilder builder = - Artifact.builder() - .type(ArtifactTypes.REMOTE_BASE64.getMimeType()) - .reference(Base64.getEncoder().encodeToString(resp.asByteArray())); - - if (decorators == null) { - return builder.build(); - } - - for (ArtifactDecorator decorator : decorators) { - builder = decorator.decorate(builder); - } - - return builder.build(); - } - - /** - * hasAuthorization will ensure that the user has proper permissions for retrieving the stored - * artifact - * - * @throws AuthenticationServiceException when user does not have correct permissions - */ - private void hasAuthorization(ArtifactReferenceURI uri, String userId) { - GetObjectTaggingRequest request = - GetObjectTaggingRequest.builder().bucket(bucket).key(uri.paths()).build(); - - GetObjectTaggingResponse resp = s3Client.getObjectTagging(request); - Tag tag = - resp.tagSet().stream() - .filter(t -> t.key().equals(ENFORCE_PERMS_KEY)) - .findFirst() - .orElse(null); - Authentication auth = SecurityContextHolder.getContext().getAuthentication(); - - if (tag == null - || (permissionEvaluator != null - && !permissionEvaluator.hasPermission(auth, tag.value(), "application", "READ"))) { - log.error( - "Could not authenticate to retrieve artifact user={} applicationOfStoredArtifact={}", - userId, - (tag == null) ? "(none)" : tag.value()); - throw new AuthenticationServiceException( - userId + " does not have permission to access this artifact"); - } - } - - /** - * Helper method to check whether the object exists. This is not thread safe, nor would it help in - * a distributed system due to how S3 works (no conditional statements). If preventing multiple - * writes of the same object is important, another filestore/db needs to be used, possibly - * dynamodb. - */ - private boolean objectExists(ArtifactReferenceURI uri) { - HeadObjectRequest request = HeadObjectRequest.builder().bucket(bucket).key(uri.paths()).build(); - try { - s3Client.headObject(request); - log.debug("Artifact exists. No need to store. reference={}", uri.uri()); - return true; - } catch (NoSuchKeyException e) { - // pretty gross that we need to use exceptions as control flow, but the - // java SDK doesn't have any other way of check if an object exists in s3 - log.info("Artifact does not exist reference={}", uri.uri()); - return false; - } catch (S3Exception e) { - int statusCode = e.statusCode(); - log.error( - "Artifact store failed head object request statusCode={} reference={}", - statusCode, - uri.uri()); - - if (statusCode != 0) { - // due to this being a HEAD request, there is no message giving a clear - // indication of what failed. Rather than seeing a useful message back - // to gate, we instead see just null. To alleviate this, we wrap the - // exception with a more meaningful message - throw new SpinnakerException(buildHeadObjectExceptionMessage(e), e); - } - - throw new SpinnakerException("S3 head object failed", e); - } - } - - /** - * S3's head object can only return 400, 403, and 404, and based on the HTTP status code, we will - * return the appropriate message back - */ - private static String buildHeadObjectExceptionMessage(S3Exception e) { - switch (e.statusCode()) { - case HttpStatus.SC_FORBIDDEN: - return "Failed to query artifact due to IAM permissions either on the bucket or object"; - case HttpStatus.SC_BAD_REQUEST: - return "Failed to query artifact due to invalid request"; - default: - return String.format("Failed to query artifact: %d", e.statusCode()); - } - } + private S3ArtifactStore() {} } diff --git a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreConfiguration.java b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreConfiguration.java new file mode 100644 index 000000000..0b0fdaed1 --- /dev/null +++ b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreConfiguration.java @@ -0,0 +1,107 @@ +/* + * Copyright 2023 Apple 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.artifacts.artifactstore.s3; + +import com.netflix.spinnaker.kork.artifacts.artifactstore.ArtifactStoreConfigurationProperties; +import com.netflix.spinnaker.kork.artifacts.artifactstore.ArtifactStoreGetter; +import com.netflix.spinnaker.kork.artifacts.artifactstore.ArtifactStoreStorer; +import com.netflix.spinnaker.kork.artifacts.artifactstore.ArtifactStoreURIBuilder; +import java.net.URI; +import java.util.Optional; +import lombok.extern.log4j.Log4j2; +import org.springframework.beans.factory.annotation.Qualifier; +import org.springframework.boot.autoconfigure.condition.ConditionalOnExpression; +import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.security.access.PermissionEvaluator; +import software.amazon.awssdk.auth.credentials.AnonymousCredentialsProvider; +import software.amazon.awssdk.auth.credentials.AwsBasicCredentials; +import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider; +import software.amazon.awssdk.auth.credentials.ProfileCredentialsProvider; +import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider; +import software.amazon.awssdk.regions.Region; +import software.amazon.awssdk.services.s3.S3Client; +import software.amazon.awssdk.services.s3.S3ClientBuilder; + +@Configuration +@Log4j2 +@ConditionalOnProperty(name = "artifact-store.type", havingValue = "s3") +public class S3ArtifactStoreConfiguration { + + @Bean + @ConditionalOnExpression("${artifact-store.s3.enabled:false}") + public ArtifactStoreStorer artifactStoreStorer( + ArtifactStoreConfigurationProperties properties, + @Qualifier("artifactS3Client") S3Client s3Client, + ArtifactStoreURIBuilder artifactStoreURIBuilder) { + return new S3ArtifactStoreStorer( + s3Client, + properties.getS3().getBucket(), + artifactStoreURIBuilder, + properties.getApplicationsRegex()); + } + + @Bean + public ArtifactStoreGetter artifactStoreGetter( + Optional permissionEvaluator, + ArtifactStoreConfigurationProperties properties, + @Qualifier("artifactS3Client") S3Client s3Client) { + + if (permissionEvaluator.isEmpty()) { + log.warn( + "PermissionEvaluator is not present. This means anyone will be able to access any artifact in the store."); + } + + String bucket = properties.getS3() != null ? properties.getS3().getBucket() : null; + + return new S3ArtifactStoreGetter(s3Client, permissionEvaluator.orElse(null), bucket); + } + + @Bean + public S3Client artifactS3Client(ArtifactStoreConfigurationProperties properties) { + S3ClientBuilder builder = S3Client.builder(); + ArtifactStoreConfigurationProperties.S3ClientConfig config = properties.getS3(); + + // Overwriting the URL is primarily used for S3 compatible object stores + // like seaweedfs + if (config.getUrl() != null) { + builder = + builder + .credentialsProvider(getCredentialsProvider(config)) + .forcePathStyle(config.isForcePathStyle()) + .endpointOverride(URI.create(config.getUrl())); + } else if (config.getProfile() != null) { + builder = builder.credentialsProvider(ProfileCredentialsProvider.create(config.getProfile())); + } + + if (config.getRegion() != null) { + builder = builder.region(Region.of(config.getRegion())); + } + + return builder.build(); + } + + private AwsCredentialsProvider getCredentialsProvider( + ArtifactStoreConfigurationProperties.S3ClientConfig config) { + if (config.getAccessKey() != null) { + return StaticCredentialsProvider.create( + AwsBasicCredentials.create(config.getAccessKey(), config.getSecretKey())); + } else { + return AnonymousCredentialsProvider.create(); + } + } +} diff --git a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreGetter.java b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreGetter.java new file mode 100644 index 000000000..778c46528 --- /dev/null +++ b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreGetter.java @@ -0,0 +1,115 @@ +/* + * Copyright 2023 Apple 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.artifacts.artifactstore.s3; + +import static com.netflix.spinnaker.kork.artifacts.artifactstore.s3.S3ArtifactStore.ENFORCE_PERMS_KEY; + +import com.netflix.spinnaker.kork.artifacts.ArtifactTypes; +import com.netflix.spinnaker.kork.artifacts.artifactstore.ArtifactDecorator; +import com.netflix.spinnaker.kork.artifacts.artifactstore.ArtifactReferenceURI; +import com.netflix.spinnaker.kork.artifacts.artifactstore.ArtifactStoreGetter; +import com.netflix.spinnaker.kork.artifacts.model.Artifact; +import com.netflix.spinnaker.security.AuthenticatedRequest; +import java.util.Base64; +import java.util.NoSuchElementException; +import lombok.extern.log4j.Log4j2; +import org.springframework.security.access.PermissionEvaluator; +import org.springframework.security.authentication.AuthenticationServiceException; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.context.SecurityContextHolder; +import software.amazon.awssdk.core.ResponseBytes; +import software.amazon.awssdk.services.s3.S3Client; +import software.amazon.awssdk.services.s3.model.GetObjectRequest; +import software.amazon.awssdk.services.s3.model.GetObjectResponse; +import software.amazon.awssdk.services.s3.model.GetObjectTaggingRequest; +import software.amazon.awssdk.services.s3.model.GetObjectTaggingResponse; +import software.amazon.awssdk.services.s3.model.Tag; + +/** Retrieve objects from an s3 compatible service */ +@Log4j2 +public class S3ArtifactStoreGetter implements ArtifactStoreGetter { + private final S3Client s3Client; + private final PermissionEvaluator permissionEvaluator; + private final String bucket; + + public S3ArtifactStoreGetter( + S3Client s3Client, PermissionEvaluator permissionEvaluator, String bucket) { + this.s3Client = s3Client; + this.bucket = bucket; + this.permissionEvaluator = permissionEvaluator; + } + + /** + * get will return the Artifact with the provided id, and will lastly run the {@link + * ArtifactDecorator} to further populate the artifact for returning + */ + @Override + public Artifact get(ArtifactReferenceURI uri, ArtifactDecorator... decorators) { + hasAuthorization( + uri, + AuthenticatedRequest.getSpinnakerUser() + .orElseThrow( + () -> new NoSuchElementException("Could not authenticate due to missing user id"))); + + GetObjectRequest request = GetObjectRequest.builder().bucket(bucket).key(uri.paths()).build(); + + ResponseBytes resp = s3Client.getObjectAsBytes(request); + Artifact.ArtifactBuilder builder = + Artifact.builder() + .type(ArtifactTypes.REMOTE_BASE64.getMimeType()) + .reference(Base64.getEncoder().encodeToString(resp.asByteArray())); + + if (decorators == null) { + return builder.build(); + } + + for (ArtifactDecorator decorator : decorators) { + builder = decorator.decorate(builder); + } + + return builder.build(); + } + + /** + * hasAuthorization will ensure that the user has proper permissions for retrieving the stored + * artifact + * + * @throws AuthenticationServiceException when user does not have correct permissions + */ + private void hasAuthorization(ArtifactReferenceURI uri, String userId) { + GetObjectTaggingRequest request = + GetObjectTaggingRequest.builder().bucket(bucket).key(uri.paths()).build(); + + GetObjectTaggingResponse resp = s3Client.getObjectTagging(request); + Tag tag = + resp.tagSet().stream() + .filter(t -> t.key().equals(ENFORCE_PERMS_KEY)) + .findFirst() + .orElse(null); + Authentication auth = SecurityContextHolder.getContext().getAuthentication(); + + if (tag == null + || (permissionEvaluator != null + && !permissionEvaluator.hasPermission(auth, tag.value(), "application", "READ"))) { + log.error( + "Could not authenticate to retrieve artifact user={} applicationOfStoredArtifact={}", + userId, + (tag == null) ? "(none)" : tag.value()); + throw new AuthenticationServiceException( + userId + " does not have permission to access this artifact"); + } + } +} diff --git a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreStorer.java b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreStorer.java new file mode 100644 index 000000000..bb767e398 --- /dev/null +++ b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreStorer.java @@ -0,0 +1,173 @@ +/* + * Copyright 2023 Apple 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.artifacts.artifactstore.s3; + +import static com.netflix.spinnaker.kork.artifacts.artifactstore.s3.S3ArtifactStore.ENFORCE_PERMS_KEY; + +import com.netflix.spinnaker.kork.artifacts.ArtifactTypes; +import com.netflix.spinnaker.kork.artifacts.artifactstore.ArtifactReferenceURI; +import com.netflix.spinnaker.kork.artifacts.artifactstore.ArtifactStoreStorer; +import com.netflix.spinnaker.kork.artifacts.artifactstore.ArtifactStoreURIBuilder; +import com.netflix.spinnaker.kork.artifacts.model.Artifact; +import com.netflix.spinnaker.kork.exceptions.SpinnakerException; +import com.netflix.spinnaker.security.AuthenticatedRequest; +import java.util.Base64; +import java.util.regex.Pattern; +import lombok.extern.log4j.Log4j2; +import org.apache.http.HttpStatus; +import software.amazon.awssdk.core.sync.RequestBody; +import software.amazon.awssdk.services.s3.S3Client; +import software.amazon.awssdk.services.s3.model.HeadObjectRequest; +import software.amazon.awssdk.services.s3.model.NoSuchKeyException; +import software.amazon.awssdk.services.s3.model.PutObjectRequest; +import software.amazon.awssdk.services.s3.model.S3Exception; +import software.amazon.awssdk.services.s3.model.Tag; +import software.amazon.awssdk.services.s3.model.Tagging; + +/** + * S3ArtifactStoreStorer will store artifacts in a s3 compatible service + * + *

Note: It is very important that the S3 bucket has object lock on it to prevent multiple writes + * {@see https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-lock-overview.html} + */ +@Log4j2 +public class S3ArtifactStoreStorer implements ArtifactStoreStorer { + private final S3Client s3Client; + private final String bucket; + private final ArtifactStoreURIBuilder uriBuilder; + private final String applicationsRegex; + + public S3ArtifactStoreStorer( + S3Client s3Client, + String bucket, + ArtifactStoreURIBuilder uriBuilder, + String applicationsRegex) { + this.s3Client = s3Client; + this.bucket = bucket; + this.uriBuilder = uriBuilder; + this.applicationsRegex = applicationsRegex; + } + + /** + * Will store the artifact using the {@link #s3Client} in some {@link #bucket} + * + *

This method also persists "permissions" by storing the execution id that made the original + * store call. In the event a service wants to retrieve said artifact, they will also need to + * provide the proper execution id + */ + @Override + public Artifact store(Artifact artifact) { + String application = AuthenticatedRequest.getSpinnakerApplication().orElse(null); + if (application == null) { + log.warn("failed to retrieve application from request artifact={}", artifact.getName()); + return artifact; + } + + if (applicationsRegex != null && !Pattern.matches(applicationsRegex, application)) { + return artifact; + } + + ArtifactReferenceURI ref = uriBuilder.buildArtifactURI(application, artifact); + Artifact remoteArtifact = + artifact.toBuilder() + .type(ArtifactTypes.REMOTE_BASE64.getMimeType()) + .reference(ref.uri()) + .build(); + + if (objectExists(ref)) { + return remoteArtifact; + } + + // purpose of tagging is to ensure some sort of identity is persisted to + // enforce permissions when retrieving the artifact + Tag accountTag = Tag.builder().key(ENFORCE_PERMS_KEY).value(application).build(); + + PutObjectRequest request = + PutObjectRequest.builder() + .bucket(bucket) + .key(ref.paths()) + .tagging(Tagging.builder().tagSet(accountTag).build()) + .build(); + + s3Client.putObject(request, RequestBody.fromBytes(getReferenceAsBytes(artifact))); + return remoteArtifact; + } + + private byte[] getReferenceAsBytes(Artifact artifact) { + String reference = artifact.getReference(); + if (reference == null) { + throw new IllegalArgumentException("reference cannot be null"); + } + + String type = artifact.getType(); + if (type != null && type.endsWith("/base64")) { + return Base64.getDecoder().decode(reference); + } + + return reference.getBytes(); + } + + /** + * Helper method to check whether the object exists. This is not thread safe, nor would it help in + * a distributed system due to how S3 works (no conditional statements). If preventing multiple + * writes of the same object is important, another filestore/db needs to be used, possibly + * dynamodb. + */ + private boolean objectExists(ArtifactReferenceURI uri) { + HeadObjectRequest request = HeadObjectRequest.builder().bucket(bucket).key(uri.paths()).build(); + try { + s3Client.headObject(request); + log.debug("Artifact exists. No need to store. reference={}", uri.uri()); + return true; + } catch (NoSuchKeyException e) { + // pretty gross that we need to use exceptions as control flow, but the + // java SDK doesn't have any other way of check if an object exists in s3 + log.info("Artifact does not exist reference={}", uri.uri()); + return false; + } catch (S3Exception e) { + int statusCode = e.statusCode(); + log.error( + "Artifact store failed head object request statusCode={} reference={}", + statusCode, + uri.uri()); + + if (statusCode != 0) { + // due to this being a HEAD request, there is no message giving a clear + // indication of what failed. Rather than seeing a useful message back + // to gate, we instead see just null. To alleviate this, we wrap the + // exception with a more meaningful message + throw new SpinnakerException(buildHeadObjectExceptionMessage(e), e); + } + + throw new SpinnakerException("S3 head object failed", e); + } + } + + /** + * S3's head object can only return 400, 403, and 404, and based on the HTTP status code, we will + * return the appropriate message back + */ + private static String buildHeadObjectExceptionMessage(S3Exception e) { + switch (e.statusCode()) { + case HttpStatus.SC_FORBIDDEN: + return "Failed to query artifact due to IAM permissions either on the bucket or object"; + case HttpStatus.SC_BAD_REQUEST: + return "Failed to query artifact due to invalid request"; + default: + return String.format("Failed to query artifact: %d", e.statusCode()); + } + } +} diff --git a/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactDeserializerTest.java b/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactDeserializerTest.java index 89758db60..25b369f3d 100644 --- a/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactDeserializerTest.java +++ b/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactDeserializerTest.java @@ -28,23 +28,43 @@ import org.junit.jupiter.api.Test; class ArtifactDeserializerTest { - private class InMemoryArtifactStore extends ArtifactStore { - public Map storageMap = new HashMap<>(); + private class InMemoryArtifactStore { + private final Map storageMap = new HashMap<>(); public InMemoryArtifactStore put(String id, Artifact artifact) { storageMap.put(id, artifact); return this; } + public Artifact get(String id) { + return storageMap.get(id); + } + } + + private class InMemoryArtifactStoreStorer implements ArtifactStoreStorer { + public final InMemoryArtifactStore inMemoryArtifactStore; + + public InMemoryArtifactStoreStorer(InMemoryArtifactStore inMemoryArtifactStore) { + this.inMemoryArtifactStore = inMemoryArtifactStore; + } + @Override public Artifact store(Artifact artifact) { - storageMap.put(artifact.getReference(), artifact); + inMemoryArtifactStore.put(artifact.getReference(), artifact); return artifact; } + } + + private class InMemoryArtifactStoreGetter implements ArtifactStoreGetter { + public final InMemoryArtifactStore inMemoryArtifactStore; + + public InMemoryArtifactStoreGetter(InMemoryArtifactStore inMemoryArtifactStore) { + this.inMemoryArtifactStore = inMemoryArtifactStore; + } @Override public Artifact get(ArtifactReferenceURI uri, ArtifactDecorator... decorator) { - return storageMap.get(uri.uri()); + return inMemoryArtifactStore.get(uri.uri()); } } @@ -58,7 +78,12 @@ public void simpleDeserialization() throws IOException { .type(ArtifactTypes.REMOTE_BASE64.getMimeType()) .reference(expectedReference) .build(); - InMemoryArtifactStore storage = new InMemoryArtifactStore().put("ref://link", expectedArtifact); + InMemoryArtifactStore inMemoryArtifactStore = + new InMemoryArtifactStore().put("ref://link", expectedArtifact); + ArtifactStore storage = + new ArtifactStore( + new InMemoryArtifactStoreGetter(inMemoryArtifactStore), + new InMemoryArtifactStoreStorer(inMemoryArtifactStore)); ArtifactDeserializer deserializer = new ArtifactDeserializer(new ObjectMapper(), storage); // We avoid using an object mapper here since the Artifact class has a diff --git a/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreConfigurationTest.java b/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreConfigurationTest.java new file mode 100644 index 000000000..448fef717 --- /dev/null +++ b/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreConfigurationTest.java @@ -0,0 +1,48 @@ +/* + * Copyright 2023 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.kork.artifacts.artifactstore; + +import static org.assertj.core.api.Assertions.assertThat; + +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.test.context.runner.ApplicationContextRunner; + +class ArtifactStoreConfigurationTest { + + private final ApplicationContextRunner runner = + new ApplicationContextRunner() + .withConfiguration(UserConfigurations.of(ArtifactStoreConfiguration.class)); + + @BeforeEach + void init(TestInfo testInfo) { + System.out.println("--------------- Test " + testInfo.getDisplayName()); + } + + @Test + void testArtifactStoreDefaults() { + runner.run( + ctx -> { + assertThat(ctx).hasSingleBean(ArtifactStoreURIBuilder.class); + assertThat(ctx).hasSingleBean(ArtifactStore.class); + assertThat(ctx).hasSingleBean(NoopArtifactStoreGetter.class); + assertThat(ctx).hasSingleBean(NoopArtifactStoreStorer.class); + }); + } +} diff --git a/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreTest.java b/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreTest.java new file mode 100644 index 000000000..51fe2c572 --- /dev/null +++ b/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreTest.java @@ -0,0 +1,60 @@ +/* + * Copyright 2023 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.kork.artifacts.artifactstore; + +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.verifyNoInteractions; +import static org.mockito.Mockito.verifyNoMoreInteractions; +import static org.mockito.Mockito.when; + +import com.netflix.spinnaker.kork.artifacts.model.Artifact; +import org.junit.jupiter.api.Test; + +class ArtifactStoreTest { + + private ArtifactStoreGetter artifactStoreGetter = mock(ArtifactStoreGetter.class); + private ArtifactStoreStorer artifactStoreStorer = mock(ArtifactStoreStorer.class); + private ArtifactStore artifactStore = new ArtifactStore(artifactStoreGetter, artifactStoreStorer); + + @Test + void testArtifactStoreDelegatesToGetter() { + ArtifactReferenceURI uri = mock(ArtifactReferenceURI.class); + ArtifactDecorator artifactDecorator = mock(ArtifactDecorator.class); + + artifactStore.get(uri, artifactDecorator); + + verify(artifactStoreGetter).get(uri, artifactDecorator); + verifyNoMoreInteractions(artifactStoreGetter); + verifyNoInteractions(artifactStoreStorer); + } + + @Test + void testArtifactStoreDelegatesToStorer() { + Artifact inputArtifact = Artifact.builder().build(); + Artifact storedArtifact = Artifact.builder().build(); + when(artifactStore.store(inputArtifact)).thenReturn(storedArtifact); + + Artifact retval = artifactStore.store(inputArtifact); + assertThat(retval).isEqualTo(storedArtifact); + + verify(artifactStoreStorer).store(inputArtifact); + verifyNoMoreInteractions(artifactStoreStorer); + verifyNoInteractions(artifactStoreGetter); + } +} diff --git a/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/EmbeddedArtifactSerializerTest.java b/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/EmbeddedArtifactSerializerTest.java index 007e01324..b76c2417a 100644 --- a/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/EmbeddedArtifactSerializerTest.java +++ b/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/EmbeddedArtifactSerializerTest.java @@ -23,7 +23,8 @@ import com.fasterxml.jackson.databind.module.SimpleModule; import com.netflix.spinnaker.kork.artifacts.ArtifactTypes; import com.netflix.spinnaker.kork.artifacts.artifactstore.exceptions.ArtifactStoreIOException; -import com.netflix.spinnaker.kork.artifacts.artifactstore.s3.S3ArtifactStore; +import com.netflix.spinnaker.kork.artifacts.artifactstore.s3.S3ArtifactStoreGetter; +import com.netflix.spinnaker.kork.artifacts.artifactstore.s3.S3ArtifactStoreStorer; import com.netflix.spinnaker.kork.artifacts.model.Artifact; import com.netflix.spinnaker.kork.common.Header; import com.netflix.spinnaker.security.AuthenticatedRequest; @@ -46,7 +47,7 @@ class EmbeddedArtifactSerializerTest { public void serializeEmbeddedBase64Artifact_test( String name, String expectedJson, Artifact artifact, Artifact mockArtifact) throws IOException { - ArtifactStore storage = Mockito.mock(S3ArtifactStore.class); + ArtifactStore storage = Mockito.mock(ArtifactStore.class); when(storage.store(Mockito.any())).thenReturn(mockArtifact); EmbeddedArtifactSerializer serializer = @@ -66,8 +67,11 @@ public void ensureS3ExceptionHasProperMessages() { when(client.headObject((HeadObjectRequest) Mockito.any())) .thenThrow(S3Exception.builder().statusCode(400).build()); AuthenticatedRequest.set(Header.APPLICATION, "my-application"); - S3ArtifactStore artifactStore = - new S3ArtifactStore(client, null, "my-bucket", new ArtifactStoreURISHA256Builder(), null); + ArtifactStoreGetter s3ArtifactStoreGetter = + new S3ArtifactStoreGetter(client, null, "my-bucket"); + ArtifactStoreStorer artifactStoreStorer = + new S3ArtifactStoreStorer(client, "my-bucket", new ArtifactStoreURISHA256Builder(), null); + ArtifactStore artifactStore = new ArtifactStore(s3ArtifactStoreGetter, artifactStoreStorer); EmbeddedArtifactSerializer serializer = new EmbeddedArtifactSerializer(new ObjectMapper(), artifactStore); diff --git a/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreConfigurationTest.java b/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreConfigurationTest.java new file mode 100644 index 000000000..1397b65b5 --- /dev/null +++ b/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreConfigurationTest.java @@ -0,0 +1,81 @@ +/* + * Copyright 2023 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.kork.artifacts.artifactstore.s3; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.netflix.spinnaker.kork.artifacts.artifactstore.ArtifactStore; +import com.netflix.spinnaker.kork.artifacts.artifactstore.ArtifactStoreConfiguration; +import com.netflix.spinnaker.kork.artifacts.artifactstore.ArtifactStoreURIBuilder; +import com.netflix.spinnaker.kork.artifacts.artifactstore.NoopArtifactStoreGetter; +import com.netflix.spinnaker.kork.artifacts.artifactstore.NoopArtifactStoreStorer; +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.test.context.runner.ApplicationContextRunner; +import software.amazon.awssdk.services.s3.S3Client; + +class S3ArtifactStoreConfigurationTest { + + private final ApplicationContextRunner runner = + new ApplicationContextRunner() + .withConfiguration(UserConfigurations.of(ArtifactStoreConfiguration.class)); + + @BeforeEach + void init(TestInfo testInfo) { + System.out.println("--------------- Test " + testInfo.getDisplayName()); + } + + @Test + void testArtifactStoreS3Disabled() { + runner + .withPropertyValues( + "artifact-store.type=s3", + "artifact-store.s3.enabled=false", + "artifact-store.s3.region=us-west-2") // arbitrary region + .run( + ctx -> { + assertThat(ctx).hasSingleBean(ArtifactStoreURIBuilder.class); + assertThat(ctx).hasSingleBean(ArtifactStore.class); + assertThat(ctx).hasSingleBean(S3ArtifactStoreGetter.class); + assertThat(ctx).hasSingleBean(NoopArtifactStoreStorer.class); + assertThat(ctx).doesNotHaveBean(NoopArtifactStoreGetter.class); + assertThat(ctx).doesNotHaveBean(S3ArtifactStoreStorer.class); + assertThat(ctx).hasSingleBean(S3Client.class); + }); + } + + @Test + void testArtifactStoreS3Enabled() { + runner + .withPropertyValues( + "artifact-store.type=s3", + "artifact-store.s3.enabled=true", + "artifact-store.s3.region=us-west-2") // arbitrary region + .run( + ctx -> { + assertThat(ctx).hasSingleBean(ArtifactStoreURIBuilder.class); + assertThat(ctx).hasSingleBean(ArtifactStore.class); + assertThat(ctx).hasSingleBean(S3ArtifactStoreGetter.class); + assertThat(ctx).hasSingleBean(S3ArtifactStoreStorer.class); + assertThat(ctx).doesNotHaveBean(NoopArtifactStoreGetter.class); + assertThat(ctx).doesNotHaveBean(NoopArtifactStoreStorer.class); + assertThat(ctx).hasSingleBean(S3Client.class); + }); + } +} diff --git a/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreTest.java b/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreStorerTest.java similarity index 92% rename from kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreTest.java rename to kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreStorerTest.java index 8564517fa..e1d8f9790 100644 --- a/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreTest.java +++ b/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreStorerTest.java @@ -33,21 +33,21 @@ import software.amazon.awssdk.services.s3.model.HeadObjectRequest; import software.amazon.awssdk.services.s3.model.S3Exception; -public class S3ArtifactStoreTest { +public class S3ArtifactStoreStorerTest { @Test public void testExceptionPathOfObjectExists() { S3Client client = mock(S3Client.class); when(client.headObject((HeadObjectRequest) Mockito.any())) .thenThrow(S3Exception.builder().statusCode(400).build()); AuthenticatedRequest.set(Header.APPLICATION, "my-application"); - S3ArtifactStore artifactStore = - new S3ArtifactStore(client, null, "my-bucket", new ArtifactStoreURISHA256Builder(), null); + S3ArtifactStoreStorer artifactStoreStorer = + new S3ArtifactStoreStorer(client, "my-bucket", new ArtifactStoreURISHA256Builder(), null); String expectedExceptionMessage = "Failed to query artifact due to invalid request"; SpinnakerException e = Assertions.assertThrows( SpinnakerException.class, () -> { - artifactStore.store( + artifactStoreStorer.store( Artifact.builder() .type(ArtifactTypes.EMBEDDED_BASE64.getMimeType()) .reference("aGVsbG8gd29ybGQK") diff --git a/kork-expressions/src/test/java/com/netflix/spinnaker/kork/expressions/ExpressionsSupportTest.java b/kork-expressions/src/test/java/com/netflix/spinnaker/kork/expressions/ExpressionsSupportTest.java index e4b524588..19024ec83 100644 --- a/kork-expressions/src/test/java/com/netflix/spinnaker/kork/expressions/ExpressionsSupportTest.java +++ b/kork-expressions/src/test/java/com/netflix/spinnaker/kork/expressions/ExpressionsSupportTest.java @@ -155,6 +155,10 @@ public void delegatesTypeConversion() { public class MockArtifactStore extends ArtifactStore { public Map cache = new HashMap<>(); + public MockArtifactStore() { + super(null, null); + } + @Override public Artifact store(Artifact artifact) { return null; From e40af7c4a29d60b38aad22c5bb32d56613ed1591 Mon Sep 17 00:00:00 2001 From: David Byron <82477955+dbyron-sf@users.noreply.github.com> Date: Mon, 20 Nov 2023 21:11:33 -0500 Subject: [PATCH 13/32] chore(deps): use version mysql:mysql-connector-java from spring boot (#1121) to stay up to date and to resolve CVE-2021-2471 and CVE-2022-21363 [Spring boot 2.5.15](https://repo.maven.apache.org/maven2/org/springframework/boot/spring-boot-dependencies/2.5.15/spring-boot-dependencies-2.5.15.pom) uses version 8.0.33. --- spinnaker-dependencies/spinnaker-dependencies.gradle | 1 - 1 file changed, 1 deletion(-) diff --git a/spinnaker-dependencies/spinnaker-dependencies.gradle b/spinnaker-dependencies/spinnaker-dependencies.gradle index 8f62aeb1d..7d0a75835 100644 --- a/spinnaker-dependencies/spinnaker-dependencies.gradle +++ b/spinnaker-dependencies/spinnaker-dependencies.gradle @@ -137,7 +137,6 @@ dependencies { api("io.swagger:swagger-annotations:${versions.swagger}") api("javax.annotation:javax.annotation-api:1.3.2") api("javax.xml.bind:jaxb-api:2.3.1") - api("mysql:mysql-connector-java:8.0.20") api("net.logstash.logback:logstash-logback-encoder:4.11") api("org.apache.commons:commons-exec:1.3") // TODO(jvz): beginning in BC 1.71, this should use the -jdk18on artifacts From 9de47ff4666ade097f0ae41cd1838958fdc72c5d Mon Sep 17 00:00:00 2001 From: Sandesh <30489233+j-sandy@users.noreply.github.com> Date: Wed, 22 Nov 2023 05:03:54 +0530 Subject: [PATCH 14/32] chore(dependency): upgrade kotlin to 1.5.32 (#1119) Upgrade kotlin from 1.4.0 to 1.5.32, in order to sync with spring boot 2.5.15. https://repo1.maven.org/maven2/org/springframework/boot/spring-boot-dependencies/2.5.15/spring-boot-dependencies-2.5.15.pom Upgrade kotlinx-coroutines-bom to 1.5.2, in order to sync with kotlin 1.5.x. https://repo1.maven.org/maven2/org/jetbrains/kotlinx/kotlinx-coroutines-core-jvm/1.5.2/kotlinx-coroutines-core-jvm-1.5.2.pom While upgrading kotlin 1.5.32, encounter below errors in kork-plugins module: ``` Unstarted application context org.springframework.boot.test.context.assertj.AssertableApplicationContext[startupFailure=org.springframework.beans.factory.parsing.BeanDefinitionParsingException] failed to start java.lang.IllegalStateException: Unstarted application context org.springframework.boot.test.context.assertj.AssertableApplicationContext[startupFailure=org.springframework.beans.factory.parsing.BeanDefinitionParsingException] failed to start at org.springframework.boot.test.context.assertj.AssertProviderApplicationContextInvocationHandler.getStartedApplicationContext(AssertProviderApplicationContextInvocationHandler.java:156) at org.springframework.boot.test.context.assertj.AssertProviderApplicationContextInvocationHandler.invokeApplicationContextMethod(AssertProviderApplicationContextInvocationHandler.java:147) at org.springframework.boot.test.context.assertj.AssertProviderApplicationContextInvocationHandler.invoke(AssertProviderApplicationContextInvocationHandler.java:85) at com.sun.proxy.$Proxy68.getBeansOfType(Unknown Source) at com.netflix.spinnaker.kork.plugins.PluginSecretTest$tests$1$2.invoke$lambda-0(PluginSecretTest.kt:46) ... Caused by: org.springframework.beans.factory.parsing.BeanDefinitionParsingException: Configuration problem: @Configuration class 'PluginSecretTest.TestSecretEngineConfiguration' may not be final. Remove the final modifier to continue. Offending resource: com.netflix.spinnaker.kork.plugins.PluginSecretTest$TestSecretEngineConfiguration at app//org.springframework.beans.factory.parsing.FailFastProblemReporter.error(FailFastProblemReporter.java:72) at app//org.springframework.context.annotation.ConfigurationClass.validate(ConfigurationClass.java:217) at app//org.springframework.context.annotation.ConfigurationClassParser.validate(ConfigurationClassParser.java:215) Unstarted application context org.springframework.boot.test.context.assertj.AssertableApplicationContext[startupFailure=org.springframework.beans.factory.parsing.BeanDefinitionParsingException] failed to start java.lang.IllegalStateException: Unstarted application context org.springframework.boot.test.context.assertj.AssertableApplicationContext[startupFailure=org.springframework.beans.factory.parsing.BeanDefinitionParsingException] failed to start at org.springframework.boot.test.context.assertj.AssertProviderApplicationContextInvocationHandler.getStartedApplicationContext(AssertProviderApplicationContextInvocationHandler.java:156) at org.springframework.boot.test.context.assertj.AssertProviderApplicationContextInvocationHandler.invokeApplicationContextMethod(AssertProviderApplicationContextInvocationHandler.java:147) at org.springframework.boot.test.context.assertj.AssertProviderApplicationContextInvocationHandler.invoke(AssertProviderApplicationContextInvocationHandler.java:85) at com.sun.proxy.$Proxy68.getBean(Unknown Source) at com.netflix.spinnaker.kork.plugins.v2.scenarios.ComplexInjectionScenarioTest$tests$1$2.invoke$lambda-0(ComplexInjectionScenarioTest.kt:55) ... Caused by: org.springframework.beans.factory.parsing.BeanDefinitionParsingException: Configuration problem: @Configuration class 'ComplexInjectionScenarioTest.ComplexInjectionTestConfiguration' may not be final. Remove the final modifier to continue. Offending resource: com.netflix.spinnaker.kork.plugins.v2.scenarios.ComplexInjectionScenarioTest$ComplexInjectionTestConfiguration at app//org.springframework.beans.factory.parsing.FailFastProblemReporter.error(FailFastProblemReporter.java:72) at app//org.springframework.context.annotation.ConfigurationClass.validate(ConfigurationClass.java:217) at app//org.springframework.context.annotation.ConfigurationClassParser.validate(ConfigurationClassParser.java:215) Unstarted application context org.springframework.boot.test.context.assertj.AssertableApplicationContext[startupFailure=org.springframework.beans.factory.parsing.BeanDefinitionParsingException] failed to start java.lang.IllegalStateException: Unstarted application context org.springframework.boot.test.context.assertj.AssertableApplicationContext[startupFailure=org.springframework.beans.factory.parsing.BeanDefinitionParsingException] failed to start at org.springframework.boot.test.context.assertj.AssertProviderApplicationContextInvocationHandler.getStartedApplicationContext(AssertProviderApplicationContextInvocationHandler.java:156) at org.springframework.boot.test.context.assertj.AssertProviderApplicationContextInvocationHandler.invokeApplicationContextMethod(AssertProviderApplicationContextInvocationHandler.java:147) at org.springframework.boot.test.context.assertj.AssertProviderApplicationContextInvocationHandler.invoke(AssertProviderApplicationContextInvocationHandler.java:85) at com.sun.proxy.$Proxy68.getBean(Unknown Source) at com.netflix.spinnaker.kork.plugins.v2.scenarios.ServiceDependenciesScenarioTest$tests$1$2.invoke$lambda-0(ServiceDependenciesScenarioTest.kt:49) ... Caused by: org.springframework.beans.factory.parsing.BeanDefinitionParsingException: Configuration problem: @Configuration class 'ServiceDependenciesScenarioTest.TestApplicationConfiguration' may not be final. Remove the final modifier to continue. Offending resource: com.netflix.spinnaker.kork.plugins.v2.scenarios.ServiceDependenciesScenarioTest$TestApplicationConfiguration at app//org.springframework.beans.factory.parsing.FailFastProblemReporter.error(FailFastProblemReporter.java:72) at app//org.springframework.context.annotation.ConfigurationClass.validate(ConfigurationClass.java:217) at app//org.springframework.context.annotation.ConfigurationClassParser.validate(ConfigurationClassParser.java:215) Unstarted application context org.springframework.boot.test.context.assertj.AssertableApplicationContext[startupFailure=org.springframework.beans.factory.parsing.BeanDefinitionParsingException] failed to start java.lang.IllegalStateException: Unstarted application context org.springframework.boot.test.context.assertj.AssertableApplicationContext[startupFailure=org.springframework.beans.factory.parsing.BeanDefinitionParsingException] failed to start at org.springframework.boot.test.context.assertj.AssertProviderApplicationContextInvocationHandler.getStartedApplicationContext(AssertProviderApplicationContextInvocationHandler.java:156) at org.springframework.boot.test.context.assertj.AssertProviderApplicationContextInvocationHandler.invokeApplicationContextMethod(AssertProviderApplicationContextInvocationHandler.java:147) at org.springframework.boot.test.context.assertj.AssertProviderApplicationContextInvocationHandler.invoke(AssertProviderApplicationContextInvocationHandler.java:85) at com.sun.proxy.$Proxy68.getBean(Unknown Source) at com.netflix.spinnaker.kork.plugins.v2.scenarios.ServiceInjectionScenarioTest$tests$1$2.invoke$lambda-0(ServiceInjectionScenarioTest.kt:48) ... Caused by: org.springframework.beans.factory.parsing.BeanDefinitionParsingException: Configuration problem: @Configuration class 'ServiceInjectionScenarioTest.ServiceInjectionTestConfiguration' may not be final. Remove the final modifier to continue. Offending resource: com.netflix.spinnaker.kork.plugins.v2.scenarios.ServiceInjectionScenarioTest$ServiceInjectionTestConfiguration at app//org.springframework.beans.factory.parsing.FailFastProblemReporter.error(FailFastProblemReporter.java:72) at app//org.springframework.context.annotation.ConfigurationClass.validate(ConfigurationClass.java:217) at app//org.springframework.context.annotation.ConfigurationClassParser.validate(ConfigurationClassParser.java:215) ``` To fix these errors make `Configuration` classes as generic scope by removing `private`. --- gradle.properties | 2 +- gradle/kotlin-test.gradle | 2 +- gradle/kotlin.gradle | 4 ++-- .../com/netflix/spinnaker/kork/plugins/PluginSecretTest.kt | 2 +- .../kork/plugins/v2/scenarios/ComplexInjectionScenarioTest.kt | 4 ++-- .../plugins/v2/scenarios/ServiceDependenciesScenarioTest.kt | 2 +- .../kork/plugins/v2/scenarios/ServiceInjectionScenarioTest.kt | 4 ++-- spinnaker-dependencies/spinnaker-dependencies.gradle | 2 +- 8 files changed, 11 insertions(+), 11 deletions(-) diff --git a/gradle.properties b/gradle.properties index 8aab42b43..ed341d5e0 100644 --- a/gradle.properties +++ b/gradle.properties @@ -1,4 +1,4 @@ -kotlinVersion=1.4.32 +kotlinVersion=1.5.32 org.gradle.parallel=true spinnakerGradleVersion=8.31.0 targetJava11=true diff --git a/gradle/kotlin-test.gradle b/gradle/kotlin-test.gradle index b8a2f8229..af4ebaf76 100644 --- a/gradle/kotlin-test.gradle +++ b/gradle/kotlin-test.gradle @@ -35,7 +35,7 @@ dependencies { compileTestKotlin { kotlinOptions { - languageVersion = "1.4" + languageVersion = "1.5" jvmTarget = "11" } } diff --git a/gradle/kotlin.gradle b/gradle/kotlin.gradle index e2884cf25..b8b9c7b2e 100644 --- a/gradle/kotlin.gradle +++ b/gradle/kotlin.gradle @@ -37,14 +37,14 @@ dependencies { compileKotlin { kotlinOptions { - languageVersion = "1.4" + languageVersion = "1.5" jvmTarget = "11" } } compileTestKotlin { kotlinOptions { - languageVersion = "1.4" + languageVersion = "1.5" jvmTarget = "11" } } diff --git a/kork-plugins/src/test/kotlin/com/netflix/spinnaker/kork/plugins/PluginSecretTest.kt b/kork-plugins/src/test/kotlin/com/netflix/spinnaker/kork/plugins/PluginSecretTest.kt index 0d8270201..18690d709 100644 --- a/kork-plugins/src/test/kotlin/com/netflix/spinnaker/kork/plugins/PluginSecretTest.kt +++ b/kork-plugins/src/test/kotlin/com/netflix/spinnaker/kork/plugins/PluginSecretTest.kt @@ -113,7 +113,7 @@ class PluginSecretTest : JUnit5Minutests { } @TestConfiguration - private class TestSecretEngineConfiguration { + class TestSecretEngineConfiguration { @Bean fun testSecretEngine(): SecretEngine = object : SecretEngine { override fun clearCache() = Unit diff --git a/kork-plugins/src/test/kotlin/com/netflix/spinnaker/kork/plugins/v2/scenarios/ComplexInjectionScenarioTest.kt b/kork-plugins/src/test/kotlin/com/netflix/spinnaker/kork/plugins/v2/scenarios/ComplexInjectionScenarioTest.kt index 4cb35894a..62be19eb9 100644 --- a/kork-plugins/src/test/kotlin/com/netflix/spinnaker/kork/plugins/v2/scenarios/ComplexInjectionScenarioTest.kt +++ b/kork-plugins/src/test/kotlin/com/netflix/spinnaker/kork/plugins/v2/scenarios/ComplexInjectionScenarioTest.kt @@ -115,14 +115,14 @@ class ComplexInjectionScenarioTest : JUnit5Minutests { @TestConfiguration @ComponentScan("com.netflix.spinnaker.kork.plugins.v2.scenarios.fixtures") - private class ComplexInjectionTestConfiguration { + class ComplexInjectionTestConfiguration { @Bean fun injectsPluginExtensions(testExtensions: List): InjectsPluginExtensions { return InjectsPluginExtensions(testExtensions) } } - private class InjectsPluginExtensions(val testExtensions: List) { + class InjectsPluginExtensions(val testExtensions: List) { @PostConstruct fun tryOutExtensions() { // Verify that extensions can be used immediately. diff --git a/kork-plugins/src/test/kotlin/com/netflix/spinnaker/kork/plugins/v2/scenarios/ServiceDependenciesScenarioTest.kt b/kork-plugins/src/test/kotlin/com/netflix/spinnaker/kork/plugins/v2/scenarios/ServiceDependenciesScenarioTest.kt index b08a15d10..038c04ae6 100644 --- a/kork-plugins/src/test/kotlin/com/netflix/spinnaker/kork/plugins/v2/scenarios/ServiceDependenciesScenarioTest.kt +++ b/kork-plugins/src/test/kotlin/com/netflix/spinnaker/kork/plugins/v2/scenarios/ServiceDependenciesScenarioTest.kt @@ -78,7 +78,7 @@ class ServiceDependenciesScenarioTest : JUnit5Minutests { @Configuration @ComponentScan("com.netflix.spinnaker.kork.plugins.v2.scenarios.fixtures") - private class TestApplicationConfiguration + class TestApplicationConfiguration companion object { private val GENERATED = testPlugin { diff --git a/kork-plugins/src/test/kotlin/com/netflix/spinnaker/kork/plugins/v2/scenarios/ServiceInjectionScenarioTest.kt b/kork-plugins/src/test/kotlin/com/netflix/spinnaker/kork/plugins/v2/scenarios/ServiceInjectionScenarioTest.kt index 077a5211e..e33739366 100644 --- a/kork-plugins/src/test/kotlin/com/netflix/spinnaker/kork/plugins/v2/scenarios/ServiceInjectionScenarioTest.kt +++ b/kork-plugins/src/test/kotlin/com/netflix/spinnaker/kork/plugins/v2/scenarios/ServiceInjectionScenarioTest.kt @@ -73,14 +73,14 @@ class ServiceInjectionScenarioTest : JUnit5Minutests { } @TestConfiguration - private class ServiceInjectionTestConfiguration { + class ServiceInjectionTestConfiguration { @Bean fun injectsPluginExtensions(testExtensions: List): InjectsPluginExtensions { return InjectsPluginExtensions(testExtensions) } } - private class InjectsPluginExtensions(val testExtensions: List) { + class InjectsPluginExtensions(val testExtensions: List) { @PostConstruct fun tryOutExtensions() { // Verify that extensions can be used immediately. diff --git a/spinnaker-dependencies/spinnaker-dependencies.gradle b/spinnaker-dependencies/spinnaker-dependencies.gradle index 7d0a75835..6ffb88dcb 100644 --- a/spinnaker-dependencies/spinnaker-dependencies.gradle +++ b/spinnaker-dependencies/spinnaker-dependencies.gradle @@ -50,7 +50,7 @@ dependencies { // 2.16.0 is subject to CVE-2021-45105. 2.17.0 is subject to CVE-2021-44832, so use >= 2.17.1. api(platform("org.apache.logging.log4j:log4j-bom:2.20.0")) - api(platform("org.jetbrains.kotlinx:kotlinx-coroutines-bom:1.4.3")) + api(platform("org.jetbrains.kotlinx:kotlinx-coroutines-bom:1.5.2")) //kotlinVersion comes from gradle.properties since we have kotlin code in // this project and need to configure gradle plugins etc. api(platform("org.jetbrains.kotlin:kotlin-bom:$kotlinVersion")) From 78ec4f1bafebeb5038ae9c8025b3f8ad0ee62bf3 Mon Sep 17 00:00:00 2001 From: Kiran Godishala <53332225+kirangodishala@users.noreply.github.com> Date: Tue, 28 Nov 2023 22:15:40 +0530 Subject: [PATCH 15/32] fix(PropertySource): add a delegate accessor to SecretAwarePropertySource to fix clouddriver failure due to inaccessible bootstrap property source. (#1122) --- .../spinnaker/kork/secrets/SecretAwarePropertySource.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/kork-secrets/src/main/java/com/netflix/spinnaker/kork/secrets/SecretAwarePropertySource.java b/kork-secrets/src/main/java/com/netflix/spinnaker/kork/secrets/SecretAwarePropertySource.java index 926058ae0..23c0b4053 100644 --- a/kork-secrets/src/main/java/com/netflix/spinnaker/kork/secrets/SecretAwarePropertySource.java +++ b/kork-secrets/src/main/java/com/netflix/spinnaker/kork/secrets/SecretAwarePropertySource.java @@ -53,4 +53,8 @@ public String[] getPropertyNames() { public boolean containsProperty(String name) { return delegate.containsProperty(name); } + + public EnumerablePropertySource getDelegate() { + return delegate; + } } From e9f1c634eaa8fabd6cf279639fb7faf8e7f3fadf Mon Sep 17 00:00:00 2001 From: spinnakerbot Date: Fri, 1 Dec 2023 04:55:26 -0500 Subject: [PATCH 16/32] chore(dependencies): Autobump spinnakerGradleVersion (#1123) Co-authored-by: root --- gradle.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle.properties b/gradle.properties index ed341d5e0..0c2557f59 100644 --- a/gradle.properties +++ b/gradle.properties @@ -1,5 +1,5 @@ kotlinVersion=1.5.32 org.gradle.parallel=true -spinnakerGradleVersion=8.31.0 +spinnakerGradleVersion=8.32.1 targetJava11=true includeRuntimes=actuator,core,eureka,retrofit,secrets-aws,secrets-gcp,stackdriver,swagger,tomcat,web From aca5d7eee5ed65c8784d8c2d54bac129d5e988f8 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 1 Dec 2023 16:18:10 +0000 Subject: [PATCH 17/32] chore(deps): bump actions/setup-java from 3 to 4 (#1124) Bumps [actions/setup-java](https://github.com/actions/setup-java) from 3 to 4. - [Release notes](https://github.com/actions/setup-java/releases) - [Commits](https://github.com/actions/setup-java/compare/v3...v4) --- updated-dependencies: - dependency-name: actions/setup-java dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/workflows/build.yml | 2 +- .github/workflows/pr.yml | 2 +- .github/workflows/release.yml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 621e2b518..ebf093d3f 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -16,7 +16,7 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - - uses: actions/setup-java@v3 + - uses: actions/setup-java@v4 with: java-version: 11 distribution: 'zulu' diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 54b60a91e..3d4281aa3 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -10,7 +10,7 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - - uses: actions/setup-java@v3 + - uses: actions/setup-java@v4 with: java-version: 11 distribution: 'zulu' diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 0ca6be4b6..a4c970530 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -35,7 +35,7 @@ jobs: fi echo "exactly one branch ($BRANCHES)" echo BRANCH=$BRANCHES >> $GITHUB_ENV - - uses: actions/setup-java@v3 + - uses: actions/setup-java@v4 with: java-version: 11 distribution: 'zulu' From 05b951e004ae30c3ee432e97f12834fab3fed33e Mon Sep 17 00:00:00 2001 From: David Byron <82477955+dbyron-sf@users.noreply.github.com> Date: Mon, 4 Dec 2023 10:45:49 -0500 Subject: [PATCH 18/32] chore(liquibase): remove stale comments (#1125) After https://github.com/spinnaker/kork/pull/1117, these comments are no longer relevant. --- spinnaker-dependencies/spinnaker-dependencies.gradle | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/spinnaker-dependencies/spinnaker-dependencies.gradle b/spinnaker-dependencies/spinnaker-dependencies.gradle index 6ffb88dcb..ec403f01a 100644 --- a/spinnaker-dependencies/spinnaker-dependencies.gradle +++ b/spinnaker-dependencies/spinnaker-dependencies.gradle @@ -151,17 +151,6 @@ dependencies { api("org.jetbrains.spek:spek-subject-extension:${versions.spek}") api("org.jooq:jooq:${versions.jooq}") api("org.jooq:jooq-kotlin:${versions.jooq}") - - // The liquibase version starting from 4.0.0 till 4.12.0 - // has an issue w.r.t parsing the changelog file, - // if found at multiple places within the classpath - // https://github.com/liquibase/liquibase/issues/2818 - // This duplicate changelog issue is fixed in 4.13.0, - // but identified another issue that gets introduced in 4.13.0. - // This issue(https://github.com/liquibase/liquibase/issues/3091) - // hinders the migration of sql scripts available in orca - // (https://github.com/spinnaker/orca/tree/master/orca-sql/src/main/resources/db/changelog), - // containing `afterColumn` construct, with a validation error for postgresql. So pin with 3.10.3 api("org.liquibase:liquibase-core"){ version { strictly "4.24.0" From 2ae4f0b03062151a35b31c3ef8ab686e03ed83a6 Mon Sep 17 00:00:00 2001 From: David Byron <82477955+dbyron-sf@users.noreply.github.com> Date: Tue, 5 Dec 2023 15:53:42 -0500 Subject: [PATCH 19/32] chore(dependencies): add dependencies on spinnaker-dependencies (#1126) When projects have dependencies on artifacts, but depend on spinnaker-dependencies to provide the version, this makes the version available and removes failures from ./gradlew dependencies output. Some examples of failures that this removes: testRuntimeOnlyDependenciesMetadata +--- org.junit.jupiter:junit-jupiter-engine FAILED \--- org.junit.platform:junit-platform-launcher FAILED compileOnly - Compile only dependencies for compilation 'main' (target (jvm)). \--- org.projectlombok:lombok FAILED compileOnlyDependenciesMetadata \--- org.projectlombok:lombok FAILED testCompileOnly - Compile only dependencies for compilation 'test' (target (jvm)). \--- org.projectlombok:lombok FAILED testCompileOnlyDependenciesMetadata \--- org.projectlombok:lombok FAILED --- gradle/kotlin-test.gradle | 2 ++ gradle/kotlin.gradle | 2 ++ gradle/lombok.gradle | 2 ++ kork-actuator/kork-actuator.gradle | 1 + kork-plugins-spring-api/kork-plugins-spring-api.gradle | 2 +- 5 files changed, 8 insertions(+), 1 deletion(-) diff --git a/gradle/kotlin-test.gradle b/gradle/kotlin-test.gradle index af4ebaf76..b6d4c5021 100644 --- a/gradle/kotlin-test.gradle +++ b/gradle/kotlin-test.gradle @@ -20,6 +20,7 @@ apply plugin: "kotlin" dependencies { + testImplementation(platform(project(":spinnaker-dependencies"))) testImplementation "org.jetbrains.kotlin:kotlin-stdlib-jdk8:$kotlinVersion" testImplementation "org.junit.jupiter:junit-jupiter-api" @@ -29,6 +30,7 @@ dependencies { testImplementation "dev.minutest:minutest" testImplementation "io.mockk:mockk" + testRuntimeOnly(platform(project(":spinnaker-dependencies"))) testRuntimeOnly "org.junit.platform:junit-platform-launcher" testRuntimeOnly "org.junit.jupiter:junit-jupiter-engine" } diff --git a/gradle/kotlin.gradle b/gradle/kotlin.gradle index b8b9c7b2e..203c24938 100644 --- a/gradle/kotlin.gradle +++ b/gradle/kotlin.gradle @@ -19,6 +19,7 @@ apply plugin: "kotlin-spring" apply plugin: "io.gitlab.arturbosch.detekt" dependencies { + testImplementation(platform(project(":spinnaker-dependencies"))) testImplementation "org.junit.jupiter:junit-jupiter-api" testImplementation "org.junit.platform:junit-platform-runner" testImplementation "org.spekframework.spek2:spek-dsl-jvm" @@ -28,6 +29,7 @@ dependencies { testImplementation "dev.minutest:minutest" testImplementation "io.mockk:mockk" + testRuntimeOnly(platform(project(":spinnaker-dependencies"))) testRuntimeOnly "org.junit.platform:junit-platform-launcher" testRuntimeOnly "org.junit.jupiter:junit-jupiter-engine" testRuntimeOnly "org.junit.vintage:junit-vintage-engine" diff --git a/gradle/lombok.gradle b/gradle/lombok.gradle index a94257fc1..93f41b25d 100644 --- a/gradle/lombok.gradle +++ b/gradle/lombok.gradle @@ -1,8 +1,10 @@ dependencies { compileOnly "org.projectlombok:lombok" + compileOnly(platform(project(":spinnaker-dependencies"))) annotationProcessor "org.projectlombok:lombok" annotationProcessor(platform(project(":spinnaker-dependencies"))) testCompileOnly "org.projectlombok:lombok" + testCompileOnly(platform(project(":spinnaker-dependencies"))) testAnnotationProcessor(platform(project(":spinnaker-dependencies"))) testAnnotationProcessor "org.projectlombok:lombok" } diff --git a/kork-actuator/kork-actuator.gradle b/kork-actuator/kork-actuator.gradle index 13686dfa7..0f441b752 100644 --- a/kork-actuator/kork-actuator.gradle +++ b/kork-actuator/kork-actuator.gradle @@ -19,6 +19,7 @@ apply plugin: "java-library" apply from: "$rootDir/gradle/kotlin-test.gradle" dependencies { + compileOnly(platform(project(":spinnaker-dependencies"))) implementation(platform(project(":spinnaker-dependencies"))) implementation "org.springframework.security:spring-security-core" implementation "org.springframework.boot:spring-boot-starter-security" diff --git a/kork-plugins-spring-api/kork-plugins-spring-api.gradle b/kork-plugins-spring-api/kork-plugins-spring-api.gradle index 30afa4da4..202d2464c 100644 --- a/kork-plugins-spring-api/kork-plugins-spring-api.gradle +++ b/kork-plugins-spring-api/kork-plugins-spring-api.gradle @@ -18,7 +18,7 @@ apply plugin: "java-library" apply from: "${project.rootDir}/gradle/kotlin-test.gradle" dependencies { - implementation(platform(project(":spinnaker-dependencies"))) + api(platform(project(":spinnaker-dependencies"))) api project(":kork-plugins-api") api "org.springframework.boot:spring-boot-starter-web" From ad676319c7dd96c990bd27aae29800fd6d025593 Mon Sep 17 00:00:00 2001 From: Sandesh <30489233+j-sandy@users.noreply.github.com> Date: Tue, 19 Dec 2023 00:16:25 +0530 Subject: [PATCH 20/32] chore(dependency): refactor correct coordinate of jsch dependency (#1135) Correct coordinate of jsch dependency is com.jcraft:jsch. https://repo1.maven.org/maven2/com/jcraft/jsch/0.1.54/jsch-0.1.54.pom --- spinnaker-dependencies/spinnaker-dependencies.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spinnaker-dependencies/spinnaker-dependencies.gradle b/spinnaker-dependencies/spinnaker-dependencies.gradle index ec403f01a..8f2affc82 100644 --- a/spinnaker-dependencies/spinnaker-dependencies.gradle +++ b/spinnaker-dependencies/spinnaker-dependencies.gradle @@ -83,7 +83,7 @@ dependencies { // JinJava 2.5.3 has a bad bug: https://github.com/HubSpot/jinjava/issues/429 api("com.hubspot.jinjava:jinjava:2.5.2") api("com.jakewharton.retrofit:retrofit1-okhttp3-client:1.1.0") - api("com.jcraft.jsch:${versions.jsch}") + api("com.jcraft:jsch:${versions.jsch}") api("com.jcraft:jsch.agentproxy.connector-factory:${versions.jschAgentProxy}") api("com.jcraft:jsch.agentproxy.jsch:${versions.jschAgentProxy}") api("com.natpryce:hamkrest:1.4.2.2") From ec69211da6962d4b3d0a302304891f3b9784d234 Mon Sep 17 00:00:00 2001 From: David Byron <82477955+dbyron-sf@users.noreply.github.com> Date: Mon, 18 Dec 2023 16:35:56 -0500 Subject: [PATCH 21/32] =?UTF-8?q?fix(artifacts/test):=20clear=20the=20Arti?= =?UTF-8?q?factStore.instance=20in=20S3ArtifactSt=E2=80=A6=20(#1137)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(artifacts/test): clear the ArtifactStore.instance in S3ArtifactStoreConfigurationTest so other tests that assume ArtifactStore.getInstance is null (e.g. ExpectedArtifactTest) don't try to store an artifact and fail trying to communicate with s3 with an error like: software.amazon.awssdk.core.exception.SdkClientException: Unable to marshall request to JSON: Parameter 'Bucket' must not be null at software.amazon.awssdk.core.exception.SdkClientException$BuilderImpl.build(SdkClientException.java:111) at software.amazon.awssdk.services.s3.transform.HeadObjectRequestMarshaller.marshall(HeadObjectRequestMarshaller.java:53) at software.amazon.awssdk.services.s3.transform.HeadObjectRequestMarshaller.marshall(HeadObjectRequestMarshaller.java:31) at software.amazon.awssdk.core.internal.handler.BaseClientHandler.lambda$finalizeSdkHttpFullRequest$0(BaseClientHandler.java:73) at software.amazon.awssdk.core.internal.util.MetricUtils.measureDuration(MetricUtils.java:50) at software.amazon.awssdk.core.internal.handler.BaseClientHandler.finalizeSdkHttpFullRequest(BaseClientHandler.java:72) at software.amazon.awssdk.core.internal.handler.BaseSyncClientHandler.doExecute(BaseSyncClientHandler.java:151) at software.amazon.awssdk.core.internal.handler.BaseSyncClientHandler.lambda$execute$1(BaseSyncClientHandler.java:82) at software.amazon.awssdk.core.internal.handler.BaseSyncClientHandler.measureApiCallSuccess(BaseSyncClientHandler.java:179) at software.amazon.awssdk.core.internal.handler.BaseSyncClientHandler.execute(BaseSyncClientHandler.java:76) at software.amazon.awssdk.core.client.handler.SdkSyncClientHandler.execute(SdkSyncClientHandler.java:45) at software.amazon.awssdk.awscore.client.handler.AwsSyncClientHandler.execute(AwsSyncClientHandler.java:56) at software.amazon.awssdk.services.s3.DefaultS3Client.headObject(DefaultS3Client.java:5438) at com.netflix.spinnaker.kork.artifacts.artifactstore.s3.S3ArtifactStoreStorer.objectExists(S3ArtifactStoreStorer.java:132) at com.netflix.spinnaker.kork.artifacts.artifactstore.s3.S3ArtifactStoreStorer.store(S3ArtifactStoreStorer.java:90) at com.netflix.spinnaker.kork.artifacts.artifactstore.ArtifactStore.store(ArtifactStore.java:39) at com.netflix.spinnaker.kork.artifacts.model.ExpectedArtifact.store(ExpectedArtifact.java:160) at com.netflix.spinnaker.kork.artifacts.model.ExpectedArtifact.(ExpectedArtifact.java:56) at com.netflix.spinnaker.kork.artifacts.model.ExpectedArtifact$ExpectedArtifactBuilder.build(ExpectedArtifact.java:44) at com.netflix.spinnaker.kork.artifacts.model.ExpectedArtifactTest.testReferenceMatches(ExpectedArtifactTest.java:205) Yes, it's possible to configure a bucket name in S3ArtifactStoreConfiguration, but the more fundamental issue is having tests leaving a clean slate when they're done. * refactor(artifacts/test): use @AfterEach to protect against other tests polluting state --- .../s3/S3ArtifactStoreConfigurationTest.java | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreConfigurationTest.java b/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreConfigurationTest.java index 1397b65b5..9374573a1 100644 --- a/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreConfigurationTest.java +++ b/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreConfigurationTest.java @@ -23,11 +23,13 @@ import com.netflix.spinnaker.kork.artifacts.artifactstore.ArtifactStoreURIBuilder; import com.netflix.spinnaker.kork.artifacts.artifactstore.NoopArtifactStoreGetter; import com.netflix.spinnaker.kork.artifacts.artifactstore.NoopArtifactStoreStorer; +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.springframework.boot.context.annotation.UserConfigurations; import org.springframework.boot.test.context.runner.ApplicationContextRunner; +import org.springframework.test.util.ReflectionTestUtils; import software.amazon.awssdk.services.s3.S3Client; class S3ArtifactStoreConfigurationTest { @@ -41,6 +43,16 @@ void init(TestInfo testInfo) { System.out.println("--------------- Test " + testInfo.getDisplayName()); } + @AfterEach + void cleanup() { + // Clear ArtifactStore.instance so we don't leave lingering state for other + // tests that assume it's null. + // + // Note that ArtifactStore.setInstance(null) doesn't set instance to null + // if it's already set. + ReflectionTestUtils.setField(ArtifactStore.class, "instance", null); + } + @Test void testArtifactStoreS3Disabled() { runner From c53be8f05a85aa41f88742040d6aea4278fcfcac Mon Sep 17 00:00:00 2001 From: Matt Gogerly <6519811+mattgogerly@users.noreply.github.com> Date: Wed, 20 Dec 2023 13:49:53 +0000 Subject: [PATCH 22/32] feat(web): make account and cloud provider headers first class citizens (#1136) Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> --- .../netflix/spinnaker/kork/common/Header.java | 2 ++ .../kork/web/context/RequestContext.java | 16 ++++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/kork-core/src/main/java/com/netflix/spinnaker/kork/common/Header.java b/kork-core/src/main/java/com/netflix/spinnaker/kork/common/Header.java index 257d0b333..2ee731034 100644 --- a/kork-core/src/main/java/com/netflix/spinnaker/kork/common/Header.java +++ b/kork-core/src/main/java/com/netflix/spinnaker/kork/common/Header.java @@ -31,6 +31,8 @@ public enum Header { EXECUTION_ID("X-SPINNAKER-EXECUTION-ID", false), EXECUTION_TYPE("X-SPINNAKER-EXECUTION-TYPE", false), APPLICATION("X-SPINNAKER-APPLICATION", false), + ACCOUNT("X-SPINNAKER-ACCOUNT", false), + CLOUD_PROVIDER("X-SPINNAKER-CLOUD-PROVIDER", false), PLUGIN_ID("X-SPINNAKER-PLUGIN-ID", false), PLUGIN_EXTENSION("X-SPINNAKER-PLUGIN-EXTENSION", false); diff --git a/kork-web/src/main/java/com/netflix/spinnaker/kork/web/context/RequestContext.java b/kork-web/src/main/java/com/netflix/spinnaker/kork/web/context/RequestContext.java index dc20ec791..1e5750513 100644 --- a/kork-web/src/main/java/com/netflix/spinnaker/kork/web/context/RequestContext.java +++ b/kork-web/src/main/java/com/netflix/spinnaker/kork/web/context/RequestContext.java @@ -46,6 +46,14 @@ default Optional getApplication() { return get(Header.APPLICATION); } + default Optional getAccount() { + return get(Header.ACCOUNT); + } + + default Optional getCloudProvider() { + return get(Header.CLOUD_PROVIDER); + } + default Optional getExecutionType() { return get(Header.EXECUTION_TYPE); } @@ -83,6 +91,14 @@ default void setApplication(String value) { set(Header.APPLICATION, value); } + default void setAccount(String value) { + set(Header.ACCOUNT, value); + } + + default void setCloudProvider(String value) { + set(Header.CLOUD_PROVIDER, value); + } + default void setExecutionType(String value) { set(Header.EXECUTION_TYPE, value); } From 0c807468d198a35336ace315279711f535047ee9 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Wed, 20 Dec 2023 13:22:33 -0600 Subject: [PATCH 23/32] feat(security): Add more Fiat abstractions (#1138) * feat(security): Add more Fiat abstractions Related to https://github.com/spinnaker/spinnaker/issues/6911 This adds a copy of the Authorization enum from fiat-core, an AbstractPermissionEvaluator base class which fiat-api will be updated to use, some extensions to AccessControlled, and some other security utility functions. * fix(security): Make Authorization parsing robust --- .../security/AbstractPermissionEvaluator.java | 79 +++++++++++++++ .../spinnaker/security/AccessControlled.java | 10 +- .../spinnaker/security/Authorization.java | 45 +++++++++ .../security/AuthorizationMapControlled.java | 32 +++++++ .../security/PermissionMapControlled.java | 52 ++++++++++ .../security/SpinnakerAuthorities.java | 90 +++++++++++++++++ .../spinnaker/security/SpinnakerUsers.java | 45 +++++++++ .../security/AccessControlledSpec.groovy | 67 +++++++++++++ .../security/AuthorizationSpec.groovy | 39 ++++++++ .../security/SpinnakerAuthoritiesSpec.groovy | 63 ++++++++++++ .../security/SpinnakerUsersSpec.groovy | 96 +++++++++++++++++++ 11 files changed, 617 insertions(+), 1 deletion(-) create mode 100644 kork-security/src/main/java/com/netflix/spinnaker/security/AbstractPermissionEvaluator.java create mode 100644 kork-security/src/main/java/com/netflix/spinnaker/security/Authorization.java create mode 100644 kork-security/src/main/java/com/netflix/spinnaker/security/AuthorizationMapControlled.java create mode 100644 kork-security/src/main/java/com/netflix/spinnaker/security/PermissionMapControlled.java create mode 100644 kork-security/src/main/java/com/netflix/spinnaker/security/SpinnakerAuthorities.java create mode 100644 kork-security/src/main/java/com/netflix/spinnaker/security/SpinnakerUsers.java create mode 100644 kork-security/src/test/groovy/com/netflix/spinnaker/security/AccessControlledSpec.groovy create mode 100644 kork-security/src/test/groovy/com/netflix/spinnaker/security/AuthorizationSpec.groovy create mode 100644 kork-security/src/test/groovy/com/netflix/spinnaker/security/SpinnakerAuthoritiesSpec.groovy create mode 100644 kork-security/src/test/groovy/com/netflix/spinnaker/security/SpinnakerUsersSpec.groovy diff --git a/kork-security/src/main/java/com/netflix/spinnaker/security/AbstractPermissionEvaluator.java b/kork-security/src/main/java/com/netflix/spinnaker/security/AbstractPermissionEvaluator.java new file mode 100644 index 000000000..905622ad3 --- /dev/null +++ b/kork-security/src/main/java/com/netflix/spinnaker/security/AbstractPermissionEvaluator.java @@ -0,0 +1,79 @@ +/* + * Copyright 2023 Apple, 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.security; + +import java.io.Serializable; +import org.springframework.security.access.PermissionEvaluator; +import org.springframework.security.core.Authentication; + +/** + * Base implementation for permission evaluators that support {@link AccessControlled} domain + * objects. + */ +public abstract class AbstractPermissionEvaluator implements PermissionEvaluator { + + @Override + public boolean hasPermission( + Authentication authentication, Object targetDomainObject, Object permission) { + if (isDisabled()) { + return true; + } + if (authentication == null || targetDomainObject == null) { + return false; + } + if (SpinnakerAuthorities.isAdmin(authentication)) { + return true; + } + if (targetDomainObject instanceof AccessControlled) { + return ((AccessControlled) targetDomainObject).isAuthorized(authentication, permission); + } + return false; + } + + @Override + public boolean hasPermission( + Authentication authentication, Serializable targetId, String targetType, Object permission) { + if (isDisabled()) { + return true; + } + return hasPermission( + SpinnakerUsers.getUserId(authentication), targetId, targetType, permission); + } + + /** + * Indicates whether permission evaluation is disabled. When this is true, {@code hasPermission} + * calls should return true. This should be overridden to allow for toggling this evaluator at + * runtime. + */ + protected boolean isDisabled() { + return false; + } + + /** + * Alternative method for evaluating a permission where only the identifier of the user and target + * object is available, rather than the authenticated user and target objects themselves. + * + * @param username identifier for user to check permissions for + * @param targetId identifier of the target resource to check permissions + * @param targetType the type of the target resource being checked + * @param permission the permission being validated + * @return true if the permission is granted + */ + public abstract boolean hasPermission( + String username, Serializable targetId, String targetType, Object permission); +} diff --git a/kork-security/src/main/java/com/netflix/spinnaker/security/AccessControlled.java b/kork-security/src/main/java/com/netflix/spinnaker/security/AccessControlled.java index a01d8b5fd..58539de0c 100644 --- a/kork-security/src/main/java/com/netflix/spinnaker/security/AccessControlled.java +++ b/kork-security/src/main/java/com/netflix/spinnaker/security/AccessControlled.java @@ -16,18 +16,26 @@ package com.netflix.spinnaker.security; +import java.util.Collection; import org.springframework.security.core.Authentication; /** * An AccessControlled object is an object that knows its own permissions and can check them against * a given user and authorization. This allows resources to support access control checks via Spring * Security against the resource object directly. + * + * @see AbstractPermissionEvaluator */ public interface AccessControlled { /** * Checks if the authenticated user has a particular authorization on this object. Note that * checking if the user is an admin should be performed by a {@link - * org.springframework.security.access.PermissionEvaluator} rather than in these domain objects. + * org.springframework.security.access.PermissionEvaluator} or by checking {@link + * SpinnakerAuthorities#isAdmin(Authentication)} rather than via this method as the admin role is + * a Spinnaker-specific role. + * + * @see Authorization + * @see SpinnakerAuthorities#hasAnyRole(Authentication, Collection) */ boolean isAuthorized(Authentication authentication, Object authorization); } diff --git a/kork-security/src/main/java/com/netflix/spinnaker/security/Authorization.java b/kork-security/src/main/java/com/netflix/spinnaker/security/Authorization.java new file mode 100644 index 000000000..4f2512068 --- /dev/null +++ b/kork-security/src/main/java/com/netflix/spinnaker/security/Authorization.java @@ -0,0 +1,45 @@ +/* + * Copyright 2022 Apple 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.security; + +import java.util.Locale; +import javax.annotation.Nullable; +import org.springframework.security.core.Authentication; + +/** + * Defines types of authorizations supported by {@link AccessControlled#isAuthorized(Authentication, + * Object)}. + */ +public enum Authorization { + READ, + WRITE, + EXECUTE, + CREATE, + ; + + public static @Nullable Authorization parse(@Nullable Object o) { + if (o == null) { + return null; + } + String name = o.toString().toUpperCase(Locale.ROOT); + try { + return valueOf(name); + } catch (IllegalArgumentException e) { + return null; + } + } +} diff --git a/kork-security/src/main/java/com/netflix/spinnaker/security/AuthorizationMapControlled.java b/kork-security/src/main/java/com/netflix/spinnaker/security/AuthorizationMapControlled.java new file mode 100644 index 000000000..d13ad4f0a --- /dev/null +++ b/kork-security/src/main/java/com/netflix/spinnaker/security/AuthorizationMapControlled.java @@ -0,0 +1,32 @@ +/* + * Copyright 2023 Apple, 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.security; + +import javax.annotation.Nullable; + +/** + * Common interface for access-controlled classes which use a permission map of {@link + * Authorization} enums. + */ +public interface AuthorizationMapControlled extends PermissionMapControlled { + @Nullable + @Override + default Authorization valueOf(@Nullable Object authorization) { + return Authorization.parse(authorization); + } +} diff --git a/kork-security/src/main/java/com/netflix/spinnaker/security/PermissionMapControlled.java b/kork-security/src/main/java/com/netflix/spinnaker/security/PermissionMapControlled.java new file mode 100644 index 000000000..4abe8a391 --- /dev/null +++ b/kork-security/src/main/java/com/netflix/spinnaker/security/PermissionMapControlled.java @@ -0,0 +1,52 @@ +/* + * Copyright 2023 Apple 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.security; + +import com.netflix.spinnaker.kork.annotations.Alpha; +import java.util.Map; +import java.util.Set; +import javax.annotation.Nonnull; +import javax.annotation.Nullable; +import org.springframework.security.core.Authentication; + +/** + * Common interface for access-controlled classes which use a permission map. + * + * @param Authorization enum type + */ +@Alpha +public interface PermissionMapControlled> + extends AccessControlled { + @Nullable + Authorization valueOf(@Nullable Object authorization); + + @Nonnull + default Map> getPermissions() { + return Map.of(); + } + + @Override + default boolean isAuthorized(Authentication authentication, Object authorization) { + Authorization auth = valueOf(authorization); + if (auth == null) { + return false; + } + Set permittedRoles = getPermissions().getOrDefault(auth, Set.of()); + return permittedRoles.isEmpty() + || SpinnakerAuthorities.hasAnyRole(authentication, permittedRoles); + } +} diff --git a/kork-security/src/main/java/com/netflix/spinnaker/security/SpinnakerAuthorities.java b/kork-security/src/main/java/com/netflix/spinnaker/security/SpinnakerAuthorities.java new file mode 100644 index 000000000..75fba07d4 --- /dev/null +++ b/kork-security/src/main/java/com/netflix/spinnaker/security/SpinnakerAuthorities.java @@ -0,0 +1,90 @@ +/* + * Copyright 2022 Apple, 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.security; + +import java.util.Collection; +import java.util.List; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import javax.annotation.Nonnull; +import javax.annotation.Nullable; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.GrantedAuthority; +import org.springframework.security.core.authority.SimpleGrantedAuthority; + +/** + * Constants and utilities for working with Spring Security GrantedAuthority objects specific to + * Spinnaker and Fiat. Spinnaker-specific roles are represented here as granted authorities with the + * {@code SPINNAKER_} prefix. + */ +public class SpinnakerAuthorities { + private static final String ROLE_PREFIX = "ROLE_"; + + public static final String ADMIN = "SPINNAKER_ADMIN"; + /** Granted authority for Spinnaker administrators. */ + public static final GrantedAuthority ADMIN_AUTHORITY = new SimpleGrantedAuthority(ADMIN); + + /** Granted authority for anonymous users. */ + public static final GrantedAuthority ANONYMOUS_AUTHORITY = forRoleName("ANONYMOUS"); + + /** Creates a granted authority corresponding to the provided name of a role. */ + @Nonnull + public static GrantedAuthority forRoleName(@Nonnull String role) { + return new SimpleGrantedAuthority(ROLE_PREFIX + role); + } + + /** Checks if the given user is a Spinnaker admin. */ + public static boolean isAdmin(@Nullable Authentication authentication) { + return authentication != null + && authentication.getAuthorities().contains(SpinnakerAuthorities.ADMIN_AUTHORITY); + } + + /** Checks if the given user has the provided role. */ + public static boolean hasRole(@Nullable Authentication authentication, @Nonnull String role) { + return authentication != null && streamRoles(authentication).anyMatch(role::equals); + } + + /** Checks if the given user has any of the provided roles. */ + public static boolean hasAnyRole( + @Nullable Authentication authentication, @Nonnull Collection roles) { + return authentication != null && streamRoles(authentication).anyMatch(roles::contains); + } + + /** Gets the list of roles assigned to the given user. */ + @Nonnull + public static List getRoles(@Nullable Authentication authentication) { + if (authentication == null) { + return List.of(); + } + return streamRoles(authentication).distinct().collect(Collectors.toList()); + } + + @Nonnull + private static Stream streamRoles(@Nonnull Authentication authentication) { + return authentication.getAuthorities().stream() + .filter(SpinnakerAuthorities::isRole) + .map(SpinnakerAuthorities::getRole); + } + + private static boolean isRole(@Nonnull GrantedAuthority authority) { + return authority.getAuthority().startsWith(ROLE_PREFIX); + } + + private static String getRole(@Nonnull GrantedAuthority authority) { + return authority.getAuthority().substring(ROLE_PREFIX.length()); + } +} diff --git a/kork-security/src/main/java/com/netflix/spinnaker/security/SpinnakerUsers.java b/kork-security/src/main/java/com/netflix/spinnaker/security/SpinnakerUsers.java new file mode 100644 index 000000000..15c799465 --- /dev/null +++ b/kork-security/src/main/java/com/netflix/spinnaker/security/SpinnakerUsers.java @@ -0,0 +1,45 @@ +/* + * Copyright 2023 Apple, 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.security; + +import com.netflix.spinnaker.kork.annotations.NonnullByDefault; +import javax.annotation.Nullable; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.context.SecurityContextHolder; + +/** Constants and utilities related to Spinnaker users (AKA principals). */ +@NonnullByDefault +public class SpinnakerUsers { + /** String constant for the anonymous userid. */ + public static final String ANONYMOUS = "anonymous"; + + /** Gets the userid of the provided authentication token. */ + public static String getUserId(@Nullable Authentication authentication) { + return authentication != null ? authentication.getName() : ANONYMOUS; + } + + /** Gets the current Spinnaker userid. */ + public static String getCurrentUserId() { + Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); + if (authentication != null) { + return getUserId(authentication); + } + // fall back to request header context if relevant (AuthenticatedRequestFilter) + return AuthenticatedRequest.getSpinnakerUser().orElse(ANONYMOUS); + } +} diff --git a/kork-security/src/test/groovy/com/netflix/spinnaker/security/AccessControlledSpec.groovy b/kork-security/src/test/groovy/com/netflix/spinnaker/security/AccessControlledSpec.groovy new file mode 100644 index 000000000..d0845bff9 --- /dev/null +++ b/kork-security/src/test/groovy/com/netflix/spinnaker/security/AccessControlledSpec.groovy @@ -0,0 +1,67 @@ +/* + * Copyright 2023 Apple 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.security + +import org.springframework.security.authentication.TestingAuthenticationToken +import org.springframework.security.core.AuthenticatedPrincipal +import spock.lang.Specification + +import static groovy.lang.Tuple.tuple + +class AccessControlledSpec extends Specification { + static class TestCredentials implements AuthorizationMapControlled { + Map> permissions + } + + static class TestPermissionEvaluator extends AbstractPermissionEvaluator { + Map, Map>> targetIdTypesToUsernamePermissions = [:] + + @Override + boolean hasPermission(String username, Serializable targetId, String targetType, Object permission) { + def targetKey = tuple(targetId, targetType) + def usernamePermissions = targetIdTypesToUsernamePermissions[targetKey] + def permissions = usernamePermissions?[username] + permissions != null && permission in permissions + } + } + + static class TestUser implements AuthenticatedPrincipal { + String name + } + + void "check hasPermission for expected permissions"() { + when: + def username = 'alpha' + def user = new TestUser(name: username) + def role = 'dev' + def authentication = new TestingAuthenticationToken(user, null, [SpinnakerAuthorities.forRoleName(role)]) + def credentials = new TestCredentials(permissions: Map.of(Authorization.READ, Set.of(role), Authorization.WRITE, Set.of('ops'))) + def permissionEvaluator = new TestPermissionEvaluator() + then: + permissionEvaluator.hasPermission(authentication, credentials, Authorization.READ) + !permissionEvaluator.hasPermission(authentication, credentials, Authorization.WRITE) + !permissionEvaluator.hasPermission(null, credentials, Authorization.READ) + !permissionEvaluator.hasPermission(authentication, null, Authorization.READ) + when: + def targetId = 14 + def targetType = 'entry' + permissionEvaluator.targetIdTypesToUsernamePermissions[tuple(targetId, targetType)] = Map.of(username, Set.of(Authorization.READ)) + then: + permissionEvaluator.hasPermission(authentication, targetId, targetType, Authorization.READ) + permissionEvaluator.hasPermission(username, targetId, targetType, Authorization.READ) + } +} diff --git a/kork-security/src/test/groovy/com/netflix/spinnaker/security/AuthorizationSpec.groovy b/kork-security/src/test/groovy/com/netflix/spinnaker/security/AuthorizationSpec.groovy new file mode 100644 index 000000000..7247df46f --- /dev/null +++ b/kork-security/src/test/groovy/com/netflix/spinnaker/security/AuthorizationSpec.groovy @@ -0,0 +1,39 @@ +/* + * Copyright 2023 Apple 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.security + +import spock.lang.Specification + +class AuthorizationSpec extends Specification { + void "parse returns appropriate value"() { + expect: + Authorization.parse(input) == output + where: + input || output + null || null + 'read' || Authorization.READ + 'READ' || Authorization.READ + "Read" || Authorization.READ + Authorization.READ || Authorization.READ + 'write' || Authorization.WRITE + "${Authorization.EXECUTE}" || Authorization.EXECUTE + 'create' || Authorization.CREATE + 'unsupported' || null + ' read ' || null + 'query' || null + } +} diff --git a/kork-security/src/test/groovy/com/netflix/spinnaker/security/SpinnakerAuthoritiesSpec.groovy b/kork-security/src/test/groovy/com/netflix/spinnaker/security/SpinnakerAuthoritiesSpec.groovy new file mode 100644 index 000000000..c3dbe8ca3 --- /dev/null +++ b/kork-security/src/test/groovy/com/netflix/spinnaker/security/SpinnakerAuthoritiesSpec.groovy @@ -0,0 +1,63 @@ +/* + * Copyright 2023 Apple 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.security + +import org.springframework.security.authentication.TestingAuthenticationToken +import org.springframework.security.core.Authentication +import org.springframework.security.core.GrantedAuthority +import spock.lang.Specification + +class SpinnakerAuthoritiesSpec extends Specification { + void "isAdmin returns expected value"() { + expect: + SpinnakerAuthorities.isAdmin(authentication) == isAdmin + where: + authentication || isAdmin + null || false + u(SpinnakerAuthorities.ADMIN_AUTHORITY) || true + u(SpinnakerAuthorities.ANONYMOUS_AUTHORITY) || false + } + + void "hasRole returns expected value"() { + expect: + SpinnakerAuthorities.hasRole(authentication, 'dev') == hasRole + where: + authentication || hasRole + null || false + u('ROLE_dev') || true + u(SpinnakerAuthorities.forRoleName('dev')) || true + u(SpinnakerAuthorities.ANONYMOUS_AUTHORITY) || false + } + + void "getRoles returns expected value"() { + expect: + SpinnakerAuthorities.getRoles(authentication) == expectedRoles + where: + authentication || expectedRoles + null || [] + u('ROLE_a', 'ROLE_b') || ['a', 'b'] + u(SpinnakerAuthorities.forRoleName('c')) || ['c'] + } + + private static Authentication u(String... authorities) { + new TestingAuthenticationToken(null, null, authorities) + } + + private static Authentication u(GrantedAuthority... authorities) { + new TestingAuthenticationToken(null, null, List.of(authorities)) + } +} diff --git a/kork-security/src/test/groovy/com/netflix/spinnaker/security/SpinnakerUsersSpec.groovy b/kork-security/src/test/groovy/com/netflix/spinnaker/security/SpinnakerUsersSpec.groovy new file mode 100644 index 000000000..0b869548e --- /dev/null +++ b/kork-security/src/test/groovy/com/netflix/spinnaker/security/SpinnakerUsersSpec.groovy @@ -0,0 +1,96 @@ +/* + * Copyright 2023 Apple 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.security + +import org.springframework.security.authentication.TestingAuthenticationToken +import org.springframework.security.core.AuthenticatedPrincipal +import org.springframework.security.core.context.SecurityContextHolder +import org.springframework.security.core.userdetails.User +import spock.lang.Specification + +import java.security.Principal + +class SpinnakerUsersSpec extends Specification { + void "user id of null authentication returns anonymous"() { + when: + def authentication = null + then: + SpinnakerUsers.getUserId(authentication) == SpinnakerUsers.ANONYMOUS + } + + void "user id of UserDetails returns username"() { + when: + def username = 'alpha' + def user = User.withUsername(username).password('').authorities('ROLE_USER').build() + def authentication = new TestingAuthenticationToken(user, null) + then: + SpinnakerUsers.getUserId(authentication) == username + } + + void "user id of AuthenticatedPrincipal returns name"() { + when: + def username = 'beta' + def principal = new TestAuthenticatedPrincipal(name: username) + def authentication = new TestingAuthenticationToken(principal, null) + then: + SpinnakerUsers.getUserId(authentication) == username + } + + void "user id of Principal returns name"() { + when: + def username = 'gamma' + def principal = new TestPrincipal(name: username) + def authentication = new TestingAuthenticationToken(principal, null) + then: + SpinnakerUsers.getUserId(authentication) == username + } + + void "current user id is anonymous by default"() { + when: + SecurityContextHolder.context.authentication = null + AuthenticatedRequest.clear() + then: + SpinnakerUsers.currentUserId == SpinnakerUsers.ANONYMOUS + } + + void "current user id uses current security context first"() { + when: + def username = 'delta' + def principal = new TestPrincipal(name: username) + SecurityContextHolder.context.authentication = new TestingAuthenticationToken(principal, null) + AuthenticatedRequest.setUser("not $username") + then: + SpinnakerUsers.currentUserId == username + } + + void "current user id uses authenticated request second"() { + when: + def username = 'epsilon' + SecurityContextHolder.clearContext() + AuthenticatedRequest.setUser(username) + then: + SpinnakerUsers.currentUserId == username + } + + static class TestPrincipal implements Principal { + String name + } + + static class TestAuthenticatedPrincipal implements AuthenticatedPrincipal { + String name + } +} From 3062c129609a6e89a40725170c5bbcbea3a97930 Mon Sep 17 00:00:00 2001 From: David Byron <82477955+dbyron-sf@users.noreply.github.com> Date: Thu, 4 Jan 2024 15:10:13 -0500 Subject: [PATCH 24/32] chore(dependencies): specify org.junit:junit-bom:5.8.2 (#1141) to stay up to date. Previously, org.spockframework:spock-core:2.0-groovy-3.0 and spring boot 2.5.15 brought in org.junit:junit-bom:5.7.2. 5.10.1 is currently the latest, but it causes failures in echo, so let's take a smaller step. Spring boot 2.6.15 brings in 5.8.2 as well. --- spinnaker-dependencies/spinnaker-dependencies.gradle | 1 + 1 file changed, 1 insertion(+) diff --git a/spinnaker-dependencies/spinnaker-dependencies.gradle b/spinnaker-dependencies/spinnaker-dependencies.gradle index 8f2affc82..61a6ed9bf 100644 --- a/spinnaker-dependencies/spinnaker-dependencies.gradle +++ b/spinnaker-dependencies/spinnaker-dependencies.gradle @@ -55,6 +55,7 @@ dependencies { // this project and need to configure gradle plugins etc. api(platform("org.jetbrains.kotlin:kotlin-bom:$kotlinVersion")) api(platform("io.zipkin.brave:brave-bom:${versions.brave}")) + api(platform("org.junit:junit-bom:5.8.2")) // remove with spring boot >= 2.6.2 api(platform("org.springframework.boot:spring-boot-dependencies:${versions.springBoot}")) api(platform("com.amazonaws:aws-java-sdk-bom:${versions.aws}")) api(platform("com.google.protobuf:protobuf-bom:${versions.protobuf}")) From caabf0864e5d73f7af4e40ef61665ff9b1ab02c6 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Fri, 5 Jan 2024 14:43:35 -0600 Subject: [PATCH 25/32] feature(credentials-api): CredentialsLoader extensions (#1139) This adds a CredentialsLoader interface extracted from AbstractCredentialsLoader along with additional SpinnakerExtensionPoint classes updated to allow for plugins to define credentials beans. Related to https://github.com/spinnaker/spinnaker/issues/6914 --- .../kork-credentials-api.gradle | 1 + .../CredentialsLifecycleHandler.java | 5 +++- .../credentials/CredentialsRepository.java | 3 ++- .../definition/AbstractCredentialsLoader.java | 4 ++- .../CredentialsDefinitionSource.java | 4 ++- .../definition/CredentialsLoader.java | 27 +++++++++++++++++++ .../definition/CredentialsParser.java | 4 ++- .../spinnaker/credentials/poller/Poller.java | 4 +-- .../poller/PollerConfiguration.java | 11 ++++---- 9 files changed, 51 insertions(+), 12 deletions(-) create mode 100644 kork-credentials-api/src/main/java/com/netflix/spinnaker/credentials/definition/CredentialsLoader.java diff --git a/kork-credentials-api/kork-credentials-api.gradle b/kork-credentials-api/kork-credentials-api.gradle index 74f70a22c..83478be42 100644 --- a/kork-credentials-api/kork-credentials-api.gradle +++ b/kork-credentials-api/kork-credentials-api.gradle @@ -19,6 +19,7 @@ apply from: "$rootDir/gradle/lombok.gradle" dependencies { implementation(platform(project(":spinnaker-dependencies"))) + api project(":kork-plugins-api") implementation project(":kork-annotations") implementation project(":kork-exceptions") implementation 'javax.annotation:javax.annotation-api' diff --git a/kork-credentials-api/src/main/java/com/netflix/spinnaker/credentials/CredentialsLifecycleHandler.java b/kork-credentials-api/src/main/java/com/netflix/spinnaker/credentials/CredentialsLifecycleHandler.java index 374c9defe..4b8045c0b 100644 --- a/kork-credentials-api/src/main/java/com/netflix/spinnaker/credentials/CredentialsLifecycleHandler.java +++ b/kork-credentials-api/src/main/java/com/netflix/spinnaker/credentials/CredentialsLifecycleHandler.java @@ -16,6 +16,8 @@ package com.netflix.spinnaker.credentials; +import com.netflix.spinnaker.kork.plugins.api.internal.SpinnakerExtensionPoint; + /** * After {@link Credentials} have been parsed, they can be activated, refreshed, or retired - e.g. * adding agents. This happens before credentials are added or updated in the {@link @@ -23,7 +25,8 @@ * * @param */ -public interface CredentialsLifecycleHandler { +public interface CredentialsLifecycleHandler + extends SpinnakerExtensionPoint { /** * Credentials have been added. This is called before credentials are available in {@link diff --git a/kork-credentials-api/src/main/java/com/netflix/spinnaker/credentials/CredentialsRepository.java b/kork-credentials-api/src/main/java/com/netflix/spinnaker/credentials/CredentialsRepository.java index a7f370d19..b085f74fc 100644 --- a/kork-credentials-api/src/main/java/com/netflix/spinnaker/credentials/CredentialsRepository.java +++ b/kork-credentials-api/src/main/java/com/netflix/spinnaker/credentials/CredentialsRepository.java @@ -16,6 +16,7 @@ package com.netflix.spinnaker.credentials; +import com.netflix.spinnaker.kork.plugins.api.internal.SpinnakerExtensionPoint; import java.util.Set; import javax.annotation.Nullable; @@ -24,7 +25,7 @@ * * @param */ -public interface CredentialsRepository { +public interface CredentialsRepository extends SpinnakerExtensionPoint { /** * @param name * @return Credentials with the given name or null diff --git a/kork-credentials-api/src/main/java/com/netflix/spinnaker/credentials/definition/AbstractCredentialsLoader.java b/kork-credentials-api/src/main/java/com/netflix/spinnaker/credentials/definition/AbstractCredentialsLoader.java index 84749f06c..7d3d87b23 100644 --- a/kork-credentials-api/src/main/java/com/netflix/spinnaker/credentials/definition/AbstractCredentialsLoader.java +++ b/kork-credentials-api/src/main/java/com/netflix/spinnaker/credentials/definition/AbstractCredentialsLoader.java @@ -21,7 +21,8 @@ import javax.annotation.PostConstruct; import lombok.Getter; -public abstract class AbstractCredentialsLoader { +public abstract class AbstractCredentialsLoader + implements CredentialsLoader { @Getter protected final CredentialsRepository credentialsRepository; public AbstractCredentialsLoader(CredentialsRepository credentialsRepository) { @@ -34,5 +35,6 @@ public AbstractCredentialsLoader(CredentialsRepository credentialsRepository) * thereafter. */ @PostConstruct + @Override public abstract void load(); } diff --git a/kork-credentials-api/src/main/java/com/netflix/spinnaker/credentials/definition/CredentialsDefinitionSource.java b/kork-credentials-api/src/main/java/com/netflix/spinnaker/credentials/definition/CredentialsDefinitionSource.java index f09eb677d..20b88832f 100644 --- a/kork-credentials-api/src/main/java/com/netflix/spinnaker/credentials/definition/CredentialsDefinitionSource.java +++ b/kork-credentials-api/src/main/java/com/netflix/spinnaker/credentials/definition/CredentialsDefinitionSource.java @@ -16,6 +16,7 @@ package com.netflix.spinnaker.credentials.definition; +import com.netflix.spinnaker.kork.plugins.api.internal.SpinnakerExtensionPoint; import java.util.List; /** @@ -24,6 +25,7 @@ * * @param */ -public interface CredentialsDefinitionSource { +public interface CredentialsDefinitionSource + extends SpinnakerExtensionPoint { List getCredentialsDefinitions(); } diff --git a/kork-credentials-api/src/main/java/com/netflix/spinnaker/credentials/definition/CredentialsLoader.java b/kork-credentials-api/src/main/java/com/netflix/spinnaker/credentials/definition/CredentialsLoader.java new file mode 100644 index 000000000..8b40c1943 --- /dev/null +++ b/kork-credentials-api/src/main/java/com/netflix/spinnaker/credentials/definition/CredentialsLoader.java @@ -0,0 +1,27 @@ +/* + * Copyright 2023 Apple 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.credentials.definition; + +import com.netflix.spinnaker.credentials.Credentials; +import com.netflix.spinnaker.credentials.CredentialsRepository; +import com.netflix.spinnaker.kork.plugins.api.internal.SpinnakerExtensionPoint; + +public interface CredentialsLoader extends SpinnakerExtensionPoint { + CredentialsRepository getCredentialsRepository(); + + void load(); +} diff --git a/kork-credentials-api/src/main/java/com/netflix/spinnaker/credentials/definition/CredentialsParser.java b/kork-credentials-api/src/main/java/com/netflix/spinnaker/credentials/definition/CredentialsParser.java index 489a280d7..dad1b110c 100644 --- a/kork-credentials-api/src/main/java/com/netflix/spinnaker/credentials/definition/CredentialsParser.java +++ b/kork-credentials-api/src/main/java/com/netflix/spinnaker/credentials/definition/CredentialsParser.java @@ -17,6 +17,7 @@ package com.netflix.spinnaker.credentials.definition; import com.netflix.spinnaker.credentials.Credentials; +import com.netflix.spinnaker.kork.plugins.api.internal.SpinnakerExtensionPoint; import javax.annotation.Nullable; /** @@ -25,7 +26,8 @@ * @param * @param */ -public interface CredentialsParser { +public interface CredentialsParser + extends SpinnakerExtensionPoint { /** Parses a definition into credentials. Can return null if the definition is to be ignored. */ @Nullable U parse(T credentials); diff --git a/kork-credentials/src/main/java/com/netflix/spinnaker/credentials/poller/Poller.java b/kork-credentials/src/main/java/com/netflix/spinnaker/credentials/poller/Poller.java index f19a61ce7..a814cca0a 100644 --- a/kork-credentials/src/main/java/com/netflix/spinnaker/credentials/poller/Poller.java +++ b/kork-credentials/src/main/java/com/netflix/spinnaker/credentials/poller/Poller.java @@ -17,7 +17,7 @@ package com.netflix.spinnaker.credentials.poller; import com.netflix.spinnaker.credentials.Credentials; -import com.netflix.spinnaker.credentials.definition.AbstractCredentialsLoader; +import com.netflix.spinnaker.credentials.definition.CredentialsLoader; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; @@ -34,7 +34,7 @@ @Slf4j @RequiredArgsConstructor public class Poller implements Runnable { - private final AbstractCredentialsLoader credentialsLoader; + private final CredentialsLoader credentialsLoader; public void run() { try { diff --git a/kork-credentials/src/main/java/com/netflix/spinnaker/credentials/poller/PollerConfiguration.java b/kork-credentials/src/main/java/com/netflix/spinnaker/credentials/poller/PollerConfiguration.java index 572e78b9f..90c888ebc 100644 --- a/kork-credentials/src/main/java/com/netflix/spinnaker/credentials/poller/PollerConfiguration.java +++ b/kork-credentials/src/main/java/com/netflix/spinnaker/credentials/poller/PollerConfiguration.java @@ -16,11 +16,12 @@ package com.netflix.spinnaker.credentials.poller; -import com.netflix.spinnaker.credentials.definition.AbstractCredentialsLoader; -import java.util.List; +import com.netflix.spinnaker.credentials.Credentials; +import com.netflix.spinnaker.credentials.definition.CredentialsLoader; +import com.netflix.spinnaker.kork.annotations.NonnullByDefault; import java.util.concurrent.TimeUnit; import lombok.RequiredArgsConstructor; -import lombok.extern.slf4j.Slf4j; +import org.springframework.beans.factory.ObjectProvider; import org.springframework.boot.context.properties.EnableConfigurationProperties; import org.springframework.scheduling.annotation.SchedulingConfigurer; import org.springframework.scheduling.config.ScheduledTaskRegistrar; @@ -28,10 +29,10 @@ @EnableConfigurationProperties(PollerConfigurationProperties.class) @RequiredArgsConstructor -@Slf4j +@NonnullByDefault public class PollerConfiguration implements SchedulingConfigurer { private final PollerConfigurationProperties config; - private final List> pollers; + private final ObjectProvider> pollers; @Override public void configureTasks(ScheduledTaskRegistrar taskRegistrar) { From f2fd9ba5b6ff42ed718220c5cb18b0d007b4f2ae Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu, 1 Feb 2024 16:12:23 +0000 Subject: [PATCH 26/32] chore(deps): bump peter-evans/repository-dispatch from 2 to 3 (#1142) Bumps [peter-evans/repository-dispatch](https://github.com/peter-evans/repository-dispatch) from 2 to 3. - [Release notes](https://github.com/peter-evans/repository-dispatch/releases) - [Commits](https://github.com/peter-evans/repository-dispatch/compare/v2...v3) --- updated-dependencies: - dependency-name: peter-evans/repository-dispatch dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/workflows/release.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index a4c970530..bc3d15a34 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -74,7 +74,7 @@ jobs: - name: Pause before dependency bump run: sleep 600 - name: Trigger dependency bump workflow - uses: peter-evans/repository-dispatch@v2 + uses: peter-evans/repository-dispatch@v3 with: token: ${{ secrets.SPINNAKER_GITHUB_TOKEN }} event-type: bump-dependencies From cb9e9d38c78cb4eea5c84d3d776a3206eb863dea Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu, 1 Feb 2024 16:18:22 +0000 Subject: [PATCH 27/32] chore(deps): bump gradle/wrapper-validation-action from 1 to 2 (#1143) Bumps [gradle/wrapper-validation-action](https://github.com/gradle/wrapper-validation-action) from 1 to 2. - [Release notes](https://github.com/gradle/wrapper-validation-action/releases) - [Commits](https://github.com/gradle/wrapper-validation-action/compare/v1...v2) --- updated-dependencies: - dependency-name: gradle/wrapper-validation-action dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/workflows/gradle-wrapper-validation.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/gradle-wrapper-validation.yml b/.github/workflows/gradle-wrapper-validation.yml index a8195158c..f5ccf04c7 100644 --- a/.github/workflows/gradle-wrapper-validation.yml +++ b/.github/workflows/gradle-wrapper-validation.yml @@ -7,4 +7,4 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - - uses: gradle/wrapper-validation-action@v1 + - uses: gradle/wrapper-validation-action@v2 From ea4caf799f519fa1ff1d023f86fbcc15d19d7943 Mon Sep 17 00:00:00 2001 From: Sandesh <30489233+j-sandy@users.noreply.github.com> Date: Fri, 2 Feb 2024 23:12:15 +0530 Subject: [PATCH 28/32] chore(dependency): upgrade springfox documentation from 2.9.2 to 3.0.0 (#1144) Upgrading springfox fix vulnerabilities. Followed the migration steps suggested [here](https://springfox.github.io/springfox/docs/current/#migrating-from-existing-2-x-version) Replaced guava com.google.common.base.Predicate class with java.util.function.Predicate class and modified the logic to accomodate the same. Note: This upgrade has introduced a breaking change where swagger-ui location has moved from http://host/context-path/swagger-ui.html to http://host/context-path/swagger-ui/index.html OR http://host/context-path/swagger-ui/ for short. Dependency insight before the upgrade: ``` $ ./gradlew kork-swagger:dI --dependency swagger > Task :kork-swagger:dependencyInsight io.springfox:springfox-swagger-common:2.9.2 Variant compile: | Attribute Name | Provided | Requested | |--------------------------------|----------|--------------| | org.gradle.status | release | | | org.gradle.category | library | library | | org.gradle.libraryelements | jar | classes | | org.gradle.usage | java-api | java-api | | org.gradle.dependency.bundling | | external | | org.gradle.jvm.environment | | standard-jvm | | org.gradle.jvm.version | | 11 | io.springfox:springfox-swagger-common:2.9.2 \--- io.springfox:springfox-swagger2:2.9.2 +--- compileClasspath (requested io.springfox:springfox-swagger2) \--- project :spinnaker-dependencies \--- compileClasspath io.springfox:springfox-swagger-ui:2.9.2 (by constraint) Variant compile: | Attribute Name | Provided | Requested | |--------------------------------|----------|--------------| | org.gradle.status | release | | | org.gradle.category | library | library | | org.gradle.libraryelements | jar | classes | | org.gradle.usage | java-api | java-api | | org.gradle.dependency.bundling | | external | | org.gradle.jvm.environment | | standard-jvm | | org.gradle.jvm.version | | 11 | io.springfox:springfox-swagger-ui:2.9.2 \--- project :spinnaker-dependencies \--- compileClasspath io.springfox:springfox-swagger-ui -> 2.9.2 \--- compileClasspath io.springfox:springfox-swagger2:2.9.2 (by constraint) Variant compile: | Attribute Name | Provided | Requested | |--------------------------------|----------|--------------| | org.gradle.status | release | | | org.gradle.category | library | library | | org.gradle.libraryelements | jar | classes | | org.gradle.usage | java-api | java-api | | org.gradle.dependency.bundling | | external | | org.gradle.jvm.environment | | standard-jvm | | org.gradle.jvm.version | | 11 | io.springfox:springfox-swagger2:2.9.2 \--- project :spinnaker-dependencies \--- compileClasspath io.springfox:springfox-swagger2 -> 2.9.2 \--- compileClasspath io.swagger:swagger-annotations:1.5.20 (by constraint) Variant compile: | Attribute Name | Provided | Requested | |--------------------------------|----------|--------------| | org.gradle.status | release | | | org.gradle.category | library | library | | org.gradle.libraryelements | jar | classes | | org.gradle.usage | java-api | java-api | | org.gradle.dependency.bundling | | external | | org.gradle.jvm.environment | | standard-jvm | | org.gradle.jvm.version | | 11 | io.swagger:swagger-annotations:1.5.20 +--- project :spinnaker-dependencies | \--- compileClasspath +--- io.springfox:springfox-swagger-common:2.9.2 | \--- io.springfox:springfox-swagger2:2.9.2 | +--- compileClasspath (requested io.springfox:springfox-swagger2) | \--- project :spinnaker-dependencies (*) +--- io.springfox:springfox-swagger2:2.9.2 (*) \--- io.swagger:swagger-models:1.5.20 +--- io.springfox:springfox-swagger2:2.9.2 (*) \--- io.springfox:springfox-swagger-common:2.9.2 (*) io.swagger:swagger-annotations -> 1.5.20 \--- compileClasspath io.swagger:swagger-models:1.5.20 Variant compile: | Attribute Name | Provided | Requested | |--------------------------------|----------|--------------| | org.gradle.status | release | | | org.gradle.category | library | library | | org.gradle.libraryelements | jar | classes | | org.gradle.usage | java-api | java-api | | org.gradle.dependency.bundling | | external | | org.gradle.jvm.environment | | standard-jvm | | org.gradle.jvm.version | | 11 | io.swagger:swagger-models:1.5.20 +--- io.springfox:springfox-swagger-common:2.9.2 | \--- io.springfox:springfox-swagger2:2.9.2 | +--- compileClasspath (requested io.springfox:springfox-swagger2) | \--- project :spinnaker-dependencies | \--- compileClasspath \--- io.springfox:springfox-swagger2:2.9.2 (*) ``` Dependency insight after the upgrade: ``` $ ./gradlew kork-swagger:dI --dependency swagger > Task :kork-swagger:dependencyInsight io.springfox:springfox-swagger-common:3.0.0 Variant compile: | Attribute Name | Provided | Requested | |--------------------------------|----------|--------------| | org.gradle.status | release | | | org.gradle.category | library | library | | org.gradle.libraryelements | jar | classes | | org.gradle.usage | java-api | java-api | | org.gradle.dependency.bundling | | external | | org.gradle.jvm.environment | | standard-jvm | | org.gradle.jvm.version | | 11 | io.springfox:springfox-swagger-common:3.0.0 +--- io.springfox:springfox-oas:3.0.0 | \--- io.springfox:springfox-boot-starter:3.0.0 | +--- compileClasspath (requested io.springfox:springfox-boot-starter) | \--- project :spinnaker-dependencies | \--- compileClasspath \--- io.springfox:springfox-swagger2:3.0.0 +--- project :spinnaker-dependencies (*) \--- io.springfox:springfox-boot-starter:3.0.0 (*) io.springfox:springfox-swagger-ui:3.0.0 Variant compile: | Attribute Name | Provided | Requested | |--------------------------------|----------|--------------| | org.gradle.status | release | | | org.gradle.category | library | library | | org.gradle.libraryelements | jar | classes | | org.gradle.usage | java-api | java-api | | org.gradle.dependency.bundling | | external | | org.gradle.jvm.environment | | standard-jvm | | org.gradle.jvm.version | | 11 | io.springfox:springfox-swagger-ui:3.0.0 \--- io.springfox:springfox-boot-starter:3.0.0 +--- compileClasspath (requested io.springfox:springfox-boot-starter) \--- project :spinnaker-dependencies \--- compileClasspath io.springfox:springfox-swagger2:3.0.0 (by constraint) Variant compile: | Attribute Name | Provided | Requested | |--------------------------------|----------|--------------| | org.gradle.status | release | | | org.gradle.category | library | library | | org.gradle.libraryelements | jar | classes | | org.gradle.usage | java-api | java-api | | org.gradle.dependency.bundling | | external | | org.gradle.jvm.environment | | standard-jvm | | org.gradle.jvm.version | | 11 | io.springfox:springfox-swagger2:3.0.0 +--- project :spinnaker-dependencies | \--- compileClasspath \--- io.springfox:springfox-boot-starter:3.0.0 +--- compileClasspath (requested io.springfox:springfox-boot-starter) \--- project :spinnaker-dependencies (*) io.swagger:swagger-annotations:1.5.20 (by constraint) Variant compile: | Attribute Name | Provided | Requested | |--------------------------------|----------|--------------| | org.gradle.status | release | | | org.gradle.category | library | library | | org.gradle.libraryelements | jar | classes | | org.gradle.usage | java-api | java-api | | org.gradle.dependency.bundling | | external | | org.gradle.jvm.environment | | standard-jvm | | org.gradle.jvm.version | | 11 | io.swagger:swagger-annotations:1.5.20 +--- project :spinnaker-dependencies | \--- compileClasspath +--- io.springfox:springfox-swagger-common:3.0.0 | +--- io.springfox:springfox-swagger2:3.0.0 | | +--- project :spinnaker-dependencies (*) | | \--- io.springfox:springfox-boot-starter:3.0.0 | | +--- compileClasspath (requested io.springfox:springfox-boot-starter) | | \--- project :spinnaker-dependencies (*) | \--- io.springfox:springfox-oas:3.0.0 | \--- io.springfox:springfox-boot-starter:3.0.0 (*) +--- io.springfox:springfox-swagger2:3.0.0 (*) \--- io.swagger:swagger-models:1.5.20 +--- io.springfox:springfox-swagger2:3.0.0 (*) \--- io.springfox:springfox-swagger-common:3.0.0 (*) io.swagger:swagger-annotations -> 1.5.20 \--- compileClasspath io.swagger:swagger-models:1.5.20 Variant compile: | Attribute Name | Provided | Requested | |--------------------------------|----------|--------------| | org.gradle.status | release | | | org.gradle.category | library | library | | org.gradle.libraryelements | jar | classes | | org.gradle.usage | java-api | java-api | | org.gradle.dependency.bundling | | external | | org.gradle.jvm.environment | | standard-jvm | | org.gradle.jvm.version | | 11 | io.swagger:swagger-models:1.5.20 +--- io.springfox:springfox-swagger-common:3.0.0 | +--- io.springfox:springfox-swagger2:3.0.0 | | +--- project :spinnaker-dependencies | | | \--- compileClasspath | | \--- io.springfox:springfox-boot-starter:3.0.0 | | +--- compileClasspath (requested io.springfox:springfox-boot-starter) | | \--- project :spinnaker-dependencies (*) | \--- io.springfox:springfox-oas:3.0.0 | \--- io.springfox:springfox-boot-starter:3.0.0 (*) \--- io.springfox:springfox-swagger2:3.0.0 (*) io.swagger.core.v3:swagger-annotations:2.1.2 Variant compile: | Attribute Name | Provided | Requested | |--------------------------------|----------|--------------| | org.gradle.status | release | | | org.gradle.category | library | library | | org.gradle.libraryelements | jar | classes | | org.gradle.usage | java-api | java-api | | org.gradle.dependency.bundling | | external | | org.gradle.jvm.environment | | standard-jvm | | org.gradle.jvm.version | | 11 | io.swagger.core.v3:swagger-annotations:2.1.2 +--- io.springfox:springfox-oas:3.0.0 | \--- io.springfox:springfox-boot-starter:3.0.0 | +--- compileClasspath (requested io.springfox:springfox-boot-starter) | \--- project :spinnaker-dependencies | \--- compileClasspath \--- io.springfox:springfox-swagger-common:3.0.0 +--- io.springfox:springfox-swagger2:3.0.0 | +--- project :spinnaker-dependencies (*) | \--- io.springfox:springfox-boot-starter:3.0.0 (*) \--- io.springfox:springfox-oas:3.0.0 (*) io.swagger.core.v3:swagger-models:2.1.2 Variant compile: | Attribute Name | Provided | Requested | |--------------------------------|----------|--------------| | org.gradle.status | release | | | org.gradle.category | library | library | | org.gradle.libraryelements | jar | classes | | org.gradle.usage | java-api | java-api | | org.gradle.dependency.bundling | | external | | org.gradle.jvm.environment | | standard-jvm | | org.gradle.jvm.version | | 11 | io.swagger.core.v3:swagger-models:2.1.2 \--- io.springfox:springfox-oas:3.0.0 \--- io.springfox:springfox-boot-starter:3.0.0 +--- compileClasspath (requested io.springfox:springfox-boot-starter) \--- project :spinnaker-dependencies \--- compileClasspath ``` --- kork-swagger/kork-swagger.gradle | 3 +-- .../spinnaker/config/SwaggerConfig.java | 24 +++++-------------- .../spinnaker-dependencies.gradle | 6 ++--- 3 files changed, 10 insertions(+), 23 deletions(-) diff --git a/kork-swagger/kork-swagger.gradle b/kork-swagger/kork-swagger.gradle index 0ba8870b2..5d57e7d5c 100644 --- a/kork-swagger/kork-swagger.gradle +++ b/kork-swagger/kork-swagger.gradle @@ -7,7 +7,6 @@ dependencies { implementation "com.google.guava:guava" implementation "org.springframework.boot:spring-boot-autoconfigure" - implementation "io.springfox:springfox-swagger2" - implementation "io.springfox:springfox-swagger-ui" + implementation "io.springfox:springfox-boot-starter" } diff --git a/kork-swagger/src/main/java/com/netflix/spinnaker/config/SwaggerConfig.java b/kork-swagger/src/main/java/com/netflix/spinnaker/config/SwaggerConfig.java index 54823325c..662e45b2f 100644 --- a/kork-swagger/src/main/java/com/netflix/spinnaker/config/SwaggerConfig.java +++ b/kork-swagger/src/main/java/com/netflix/spinnaker/config/SwaggerConfig.java @@ -16,13 +16,10 @@ package com.netflix.spinnaker.config; -import static com.google.common.base.Predicates.or; - -import com.google.common.base.Predicate; import com.google.common.collect.ImmutableList; import java.util.List; import java.util.Objects; -import java.util.stream.Collectors; +import java.util.function.Predicate; import javax.annotation.Nullable; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.boot.context.properties.ConfigurationProperties; @@ -32,11 +29,9 @@ import springfox.documentation.builders.RequestHandlerSelectors; import springfox.documentation.service.ApiInfo; import springfox.documentation.spi.DocumentationType; -import springfox.documentation.spring.web.paths.AbstractPathProvider; +import springfox.documentation.spring.web.paths.DefaultPathProvider; import springfox.documentation.spring.web.plugins.Docket; -import springfox.documentation.swagger2.annotations.EnableSwagger2; -@EnableSwagger2 @Configuration @ConditionalOnProperty("swagger.enabled") @ConfigurationProperties(prefix = "swagger") @@ -54,7 +49,7 @@ public class SwaggerConfig { @Bean public Docket gateApi() { return new Docket(DocumentationType.SWAGGER_2) - .pathProvider(new BasePathProvider(basePath, documentationPath)) + .pathProvider(new BasePathProvider(documentationPath)) .select() .apis(RequestHandlerSelectors.any()) .paths(paths()) @@ -80,7 +75,7 @@ private static Class getClassIfPresent(String name) { } private Predicate paths() { - return or(patterns.stream().map(PathSelectors::regex).collect(Collectors.toList())); + return patterns.stream().map(PathSelectors::regex).reduce((x, y) -> x.or(y)).get(); } private ApiInfo apiInfo() { @@ -123,20 +118,13 @@ public String getDocumentationPath() { return documentationPath; } - public class BasePathProvider extends AbstractPathProvider { - private String basePath; + public class BasePathProvider extends DefaultPathProvider { private String documentationPath; - private BasePathProvider(String basePath, String documentationPath) { - this.basePath = basePath; + private BasePathProvider(String documentationPath) { this.documentationPath = documentationPath; } - @Override - protected String applicationPath() { - return basePath; - } - @Override protected String getDocumentationPath() { return documentationPath; diff --git a/spinnaker-dependencies/spinnaker-dependencies.gradle b/spinnaker-dependencies/spinnaker-dependencies.gradle index 61a6ed9bf..9cc50add2 100644 --- a/spinnaker-dependencies/spinnaker-dependencies.gradle +++ b/spinnaker-dependencies/spinnaker-dependencies.gradle @@ -26,8 +26,8 @@ ext { spek2 : "2.0.9", springBoot : "2.5.15", springCloud : "2020.0.6", - springfoxSwagger : "2.9.2", - swagger : "1.5.20", //this should stay in sync with what springfoxSwagger expects + springfoxSwagger : "3.0.0", + swagger : "1.5.20", //this should stay in sync with what springfoxSwagger expects. // Spring boot 2.4.13 brings in 9.0.55. Spring boot 2.5.14 brings in // 9.0.63. Use 9.0.69 to resolve CVE-2022-42252 and CVE-2022-45143. Spring @@ -133,7 +133,7 @@ dependencies { api("de.huxhorn.sulky:de.huxhorn.sulky.ulid:8.2.0") api("dev.minutest:minutest:1.13.0") api("io.mockk:mockk:1.10.5") - api("io.springfox:springfox-swagger-ui:${versions.springfoxSwagger}") + api("io.springfox:springfox-boot-starter:${versions.springfoxSwagger}") api("io.springfox:springfox-swagger2:${versions.springfoxSwagger}") api("io.swagger:swagger-annotations:${versions.swagger}") api("javax.annotation:javax.annotation-api:1.3.2") From 41e71002fc19529e1a330a9022417005d2364420 Mon Sep 17 00:00:00 2001 From: xibz Date: Sun, 4 Feb 2024 21:40:54 -0600 Subject: [PATCH 29/32] fix(artifacts): Addresses a couple things regarding artifact store (#1145) * fix(artifacts): Updates Artifact Store README This commit updates the README refect the changes in https://github.com/spinnaker/kork/pull/1120 Also adds a Rosco/Helm section to describe the new `expandOverrides` field Signed-off-by: benjamin-j-powell * feat(artifacts): Move scheme to ArtifactReferenceURI This moves scheme from the artifact URI builders to the URI object instead. Also moves the `isArtifactReference` from artifact stores to the URI class instead. Signed-off-by: benjamin-j-powell * feat(artifacts): Add HelmConfig and ensure properties aren't null This commit adds the new HelmConfig which allows for users to turn on expanding overrides. This will inspect overrides values, and if it is an reference URI, it will expand and use that instead of the raw URI. This also updates the properties, `helmConfig` and `s3` to no longer be `null`. Signed-off-by: benjamin-j-powell --------- Signed-off-by: benjamin-j-powell Co-authored-by: benjamin-j-powell Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> --- .../spinnaker/kork/artifacts/README.md | 19 ++++++++++++--- .../artifactstore/ArtifactReferenceURI.java | 23 ++++++++++++------- .../artifactstore/ArtifactStore.java | 4 ---- .../ArtifactStoreConfigurationProperties.java | 9 +++++++- .../ArtifactStoreURIBuilder.java | 7 ------ .../ArtifactStoreURISHA256Builder.java | 4 ++-- .../s3/S3ArtifactStoreConfiguration.java | 2 +- .../ArtifactUriToReferenceConverter.java | 2 +- 8 files changed, 43 insertions(+), 27 deletions(-) diff --git a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/README.md b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/README.md index 39433b123..0308f4093 100644 --- a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/README.md +++ b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/README.md @@ -81,12 +81,25 @@ To enable artifact storage, simple add this to your `spinnaker-local.yml` file ```yaml artifact-store: - enabled: true + type: s3 s3: enabled: true bucket: some-artifact-store-bucket ``` +### Rosco and Helm + +If any pipelines are passing artifact references to bake stages as a parameter, +enabling this field will allow those URIs to be expanded to the full +references: + +```yaml +artifact-store: + type: s3 + helm: + expandOverrides: true +``` + ## Storage Options ### S3 @@ -97,7 +110,7 @@ against AWS. ```yaml artifact-store: - enabled: true + type: s3 s3: enabled: true profile: dev # if you want to authenticate using a certain profile @@ -119,7 +132,7 @@ Next enable the configuration ```yaml artifact-store: - enabled: true + type: s3 s3: enabled: true url: http://localhost:8333 # this URL will be used to make S3 API requests to diff --git a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactReferenceURI.java b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactReferenceURI.java index 8f1cbb022..3fdfcfa0e 100644 --- a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactReferenceURI.java +++ b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactReferenceURI.java @@ -29,24 +29,31 @@ @Builder @Getter public class ArtifactReferenceURI { - private final String scheme; + /** + * uriScheme is used as an HTTP scheme to let us further distinguish a String that is a URI to an + * artifact. This is helpful in determining what is an artifact since sometimes we are only given + * a string rather than a full artifact. + */ + private static final String uriScheme = "ref://"; + private final List uriPaths; public String uri() { - return String.format("%s://%s", scheme, paths()); + return uriScheme + paths(); } public String paths() { return Strings.join(uriPaths, '/'); } + /** Used to determine whether a String is in the artifact reference URI format. */ + public static boolean is(String reference) { + return reference.startsWith(uriScheme); + } + public static ArtifactReferenceURI parse(String reference) { - String noSchemeURI = - StringUtils.removeStart(reference, ArtifactStoreURIBuilder.uriScheme + "://"); + String noSchemeURI = StringUtils.removeStart(reference, uriScheme); String[] paths = StringUtils.split(noSchemeURI, '/'); - return ArtifactReferenceURI.builder() - .scheme(ArtifactStoreURIBuilder.uriScheme) - .uriPaths(Arrays.asList(paths)) - .build(); + return ArtifactReferenceURI.builder().uriPaths(Arrays.asList(paths)).build(); } } diff --git a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStore.java b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStore.java index 5e5ea591a..7e2d9566e 100644 --- a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStore.java +++ b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStore.java @@ -57,8 +57,4 @@ public static void setInstance(ArtifactStore storage) { log.warn("Multiple attempts in setting the singleton artifact store"); } } - - public boolean isArtifactURI(String value) { - return value.startsWith(ArtifactStoreURIBuilder.uriScheme + "://"); - } } diff --git a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreConfigurationProperties.java b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreConfigurationProperties.java index 019da7ce2..6f06ae786 100644 --- a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreConfigurationProperties.java +++ b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreConfigurationProperties.java @@ -44,5 +44,12 @@ public static class S3ClientConfig { private boolean forcePathStyle = true; } - private S3ClientConfig s3 = null; + @Data + public static class HelmConfig { + /** Enables Rosco to expand any artifact URIs passed as parameters for Helm. */ + private boolean expandOverrides = false; + } + + private S3ClientConfig s3 = new S3ClientConfig(); + private HelmConfig helm = new HelmConfig(); } diff --git a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreURIBuilder.java b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreURIBuilder.java index ba6712008..cef9904e4 100644 --- a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreURIBuilder.java +++ b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreURIBuilder.java @@ -18,13 +18,6 @@ import com.netflix.spinnaker.kork.artifacts.model.Artifact; public abstract class ArtifactStoreURIBuilder { - /** - * uriScheme is used as an HTTP scheme to let us further distinguish a String that is a URI to an - * artifact. This is helpful in determining what is an artifact since sometimes we are only given - * a string rather than a full artifact. - */ - public static final String uriScheme = "ref"; - /** * Returns the remote artifact URI that will be associated with some artifact. * diff --git a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreURISHA256Builder.java b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreURISHA256Builder.java index b7744515b..ee739eaa0 100644 --- a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreURISHA256Builder.java +++ b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/ArtifactStoreURISHA256Builder.java @@ -38,7 +38,7 @@ public ArtifactReferenceURI buildArtifactURI(String context, Artifact artifact) Hashing.sha256() .hashBytes(artifact.getReference().getBytes(StandardCharsets.UTF_8)) .toString()); - return ArtifactReferenceURI.builder().scheme(uriScheme).uriPaths(uriPaths).build(); + return ArtifactReferenceURI.builder().uriPaths(uriPaths).build(); } @Override @@ -47,6 +47,6 @@ public ArtifactReferenceURI buildURIFromPaths(String context, String... paths) { uriPaths.add(context); uriPaths.addAll(List.of(paths)); - return ArtifactReferenceURI.builder().scheme(uriScheme).uriPaths(uriPaths).build(); + return ArtifactReferenceURI.builder().uriPaths(uriPaths).build(); } } diff --git a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreConfiguration.java b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreConfiguration.java index 0b0fdaed1..e792d0c13 100644 --- a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreConfiguration.java +++ b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreConfiguration.java @@ -66,7 +66,7 @@ public ArtifactStoreGetter artifactStoreGetter( "PermissionEvaluator is not present. This means anyone will be able to access any artifact in the store."); } - String bucket = properties.getS3() != null ? properties.getS3().getBucket() : null; + String bucket = properties.getS3().getBucket(); return new S3ArtifactStoreGetter(s3Client, permissionEvaluator.orElse(null), bucket); } diff --git a/kork-expressions/src/main/java/com/netflix/spinnaker/kork/expressions/ArtifactUriToReferenceConverter.java b/kork-expressions/src/main/java/com/netflix/spinnaker/kork/expressions/ArtifactUriToReferenceConverter.java index bfe21ecb7..cacfbc635 100644 --- a/kork-expressions/src/main/java/com/netflix/spinnaker/kork/expressions/ArtifactUriToReferenceConverter.java +++ b/kork-expressions/src/main/java/com/netflix/spinnaker/kork/expressions/ArtifactUriToReferenceConverter.java @@ -55,7 +55,7 @@ public Object convertValue( return defaultTypeConverter.convertValue(value, sourceType, targetType); } - if (artifactStore == null || !artifactStore.isArtifactURI((String) value)) { + if (artifactStore == null || !ArtifactReferenceURI.is((String) value)) { return defaultTypeConverter.convertValue(value, sourceType, targetType); } From a24bcbe15b13213e42a293658fce2cb5b1949835 Mon Sep 17 00:00:00 2001 From: Kiran Godishala <53332225+kirangodishala@users.noreply.github.com> Date: Thu, 8 Feb 2024 00:06:49 +0530 Subject: [PATCH 30/32] chore(dependencies): upgrade bouncycastle from 1.70 to 1.77 (#1146) * chore(dependencies): upgrade bouncycastle from 1.70 to 1.77 * chore(dependencies): pin google-cloud-secretmanager at 2.3.10 to fix bouncycastle CVE * chore(dependencies): upgrade oci-java-sdk-bom to 3.21.0 from 1.5.17 to fix bouncycastle CVE --- kork-crypto/kork-crypto.gradle | 4 ++-- .../spinnaker-dependencies.gradle | 17 +++++++++++------ 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/kork-crypto/kork-crypto.gradle b/kork-crypto/kork-crypto.gradle index fd5a4d9dc..201487335 100644 --- a/kork-crypto/kork-crypto.gradle +++ b/kork-crypto/kork-crypto.gradle @@ -23,11 +23,11 @@ dependencies { api project(":kork-annotations") - implementation "org.bouncycastle:bcpkix-jdk15on" + implementation "org.bouncycastle:bcpkix-jdk18on" implementation "org.springframework:spring-aop" testImplementation "org.springframework.boot:spring-boot-starter-test" - testFixturesApi "org.bouncycastle:bcpkix-jdk15on" + testFixturesApi "org.bouncycastle:bcpkix-jdk18on" testFixturesApi "org.springframework:spring-core" } diff --git a/spinnaker-dependencies/spinnaker-dependencies.gradle b/spinnaker-dependencies/spinnaker-dependencies.gradle index 9cc50add2..9151e920e 100644 --- a/spinnaker-dependencies/spinnaker-dependencies.gradle +++ b/spinnaker-dependencies/spinnaker-dependencies.gradle @@ -9,7 +9,7 @@ ext { arrow : "0.13.2", aws : "1.12.176", awsv2 : "2.19.0", - bouncycastle : "1.70", + bouncycastle : "1.77", brave : "5.12.3", gcp : "25.3.0", jooq : "3.13.6", @@ -61,11 +61,10 @@ dependencies { api(platform("com.google.protobuf:protobuf-bom:${versions.protobuf}")) api(platform("com.google.cloud:libraries-bom:${versions.gcp}")) api(platform("software.amazon.awssdk:bom:${versions.awsv2}")) - api(platform("com.google.cloud:google-cloud-secretmanager:2.1.7")) api(platform("org.springframework.cloud:spring-cloud-dependencies:${versions.springCloud}")) api(platform("io.strikt:strikt-bom:0.31.0")) api(platform("org.spockframework:spock-bom:2.0-groovy-3.0")) - api(platform("com.oracle.oci.sdk:oci-java-sdk-bom:1.5.17")) + api(platform("com.oracle.oci.sdk:oci-java-sdk-bom:3.21.0")) api(platform("org.testcontainers:testcontainers-bom:1.16.2")) api(platform("io.arrow-kt:arrow-stack:${versions.arrow}")) @@ -79,6 +78,7 @@ dependencies { api("com.google.apis:google-api-services-iam:v1-rev267-1.25.0") api("com.google.apis:google-api-services-monitoring:v3-rev477-1.25.0") api("com.google.apis:google-api-services-storage:v1-rev141-1.25.0") + api("com.google.cloud:google-cloud-secretmanager:2.3.10") api("com.google.code.findbugs:jsr305:3.0.2") api("com.google.guava:guava:30.0-jre") // JinJava 2.5.3 has a bad bug: https://github.com/HubSpot/jinjava/issues/429 @@ -140,9 +140,14 @@ dependencies { api("javax.xml.bind:jaxb-api:2.3.1") api("net.logstash.logback:logstash-logback-encoder:4.11") api("org.apache.commons:commons-exec:1.3") - // TODO(jvz): beginning in BC 1.71, this should use the -jdk18on artifacts - api("org.bouncycastle:bcpkix-jdk15on:${versions.bouncycastle}") - api("org.bouncycastle:bcprov-jdk15on:${versions.bouncycastle}") + // from BC 1.71, module names changed from *-jdk15on to *-jdk18on + // due to this change, some of the modules in downstream services like clouddriver, gate would fall back to + // lower versions(<1.70) as transitive dependencies. So to make them use BC >=1.74(CVE free versions), + // the following dependencies are upgraded: oci-java-sdk-bom, google-cloud-secretmanager + // and in some cases *-jdk15on libraries are excluded so as to use the available *-jdk18on or *-jdk15to18 + // some of the modules would still use <1.70 as they can't be upgraded without upgrading spring boot + api("org.bouncycastle:bcpkix-jdk18on:${versions.bouncycastle}") + api("org.bouncycastle:bcprov-jdk18on:${versions.bouncycastle}") api("org.jetbrains:annotations:19.0.0") api("org.spekframework.spek2:spek-dsl-jvm:${versions.spek2}") api("org.spekframework.spek2:spek-runner-junit5:${versions.spek2}") From ef5d3178ed17ca6c53db4357d405dc07af270acd Mon Sep 17 00:00:00 2001 From: Abe Garcia <44239562+abe21412@users.noreply.github.com> Date: Mon, 19 Feb 2024 14:37:41 -0500 Subject: [PATCH 31/32] feat(retrofit): Make HTTP method available in SpinnakerServerException (#1149) * feat(retrofit): added httpMethod variable and getter to SpinnakerServerException, updated tests * feat(retrofit): added assertions to show the http method variable in a spinnaker exception will be null if created from RetrofitError --------- Co-authored-by: abe garcia --- .../exceptions/SpinnakerServerException.java | 6 +++++ .../SpinnakerHttpExceptionTest.java | 3 +++ .../SpinnakerRetrofit2ErrorHandleTest.java | 19 +++++++++++++ .../SpinnakerRetrofitErrorHandlerTest.java | 11 ++++++++ .../SpinnakerServerExceptionTest.java | 27 +++++++++++++++++++ 5 files changed, 66 insertions(+) diff --git a/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerException.java b/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerException.java index 0b2e6ebca..2f6e4e6b2 100644 --- a/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerException.java +++ b/kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerException.java @@ -27,11 +27,13 @@ public class SpinnakerServerException extends SpinnakerException { @Getter private final String url; + @Getter private final String httpMethod; /** Construct a SpinnakerServerException corresponding to a RetrofitError. */ public SpinnakerServerException(RetrofitError e) { super(e.getMessage(), e.getCause()); url = e.getUrl(); + httpMethod = null; } /** @@ -41,6 +43,7 @@ public SpinnakerServerException(RetrofitError e) { public SpinnakerServerException(Request request) { super(); url = request.url().toString(); + httpMethod = request.method(); } /** @@ -50,6 +53,7 @@ public SpinnakerServerException(Request request) { public SpinnakerServerException(Throwable cause, Request request) { super(cause); this.url = request.url().toString(); + this.httpMethod = request.method(); } /** @@ -59,6 +63,7 @@ public SpinnakerServerException(Throwable cause, Request request) { public SpinnakerServerException(String message, Throwable cause, Request request) { super(message, cause); this.url = request.url().toString(); + this.httpMethod = request.method(); } /** @@ -68,6 +73,7 @@ public SpinnakerServerException(String message, Throwable cause, Request request public SpinnakerServerException(String message, SpinnakerServerException cause) { super(message, cause); this.url = cause.getUrl(); + this.httpMethod = cause.getHttpMethod(); } @Override diff --git a/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerHttpExceptionTest.java b/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerHttpExceptionTest.java index bfe9c4b7b..80fea2ceb 100644 --- a/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerHttpExceptionTest.java +++ b/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerHttpExceptionTest.java @@ -28,6 +28,7 @@ import okhttp3.MediaType; import okhttp3.ResponseBody; import org.junit.jupiter.api.Test; +import org.springframework.http.HttpMethod; import org.springframework.http.HttpStatus; import retrofit.RetrofitError; import retrofit.client.Response; @@ -63,6 +64,7 @@ void testSpinnakerHttpExceptionFromRetrofitError() { .isEqualTo("Status: " + statusCode + ", URL: " + url + ", Message: " + message); assertThat(spinnakerHttpException.getUrl()).isEqualTo(url); assertThat(spinnakerHttpException.getReason()).isEqualTo(reason); + assertThat(spinnakerHttpException.getHttpMethod()).isNull(); } @Test @@ -92,6 +94,7 @@ void testSpinnakerHttpExceptionFromRetrofitException() { assertThat(notFoundException.getResponseCode()).isEqualTo(HttpStatus.NOT_FOUND.value()); assertThat(notFoundException) .hasMessageContaining(String.valueOf(HttpStatus.NOT_FOUND.value())); + assertThat(notFoundException.getHttpMethod()).isEqualTo(HttpMethod.GET.toString()); } @Test diff --git a/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofit2ErrorHandleTest.java b/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofit2ErrorHandleTest.java index eb2101e1e..f82efd3a0 100644 --- a/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofit2ErrorHandleTest.java +++ b/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofit2ErrorHandleTest.java @@ -31,6 +31,7 @@ import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; +import org.springframework.http.HttpMethod; import org.springframework.http.HttpStatus; import retrofit2.Call; import retrofit2.Retrofit; @@ -145,6 +146,21 @@ void testResponseHeadersInException() { .isEqualTo(mockWebServer.url("/retrofit2").toString()); } + @Test + void testHttpMethodInException() { + // Check http request method is retrievable from a SpinnakerHttpException + mockWebServer.enqueue( + new MockResponse() + .setResponseCode(HttpStatus.BAD_REQUEST.value()) + .setBody(responseBodyString)); + SpinnakerHttpException spinnakerHttpException = + catchThrowableOfType( + () -> retrofit2Service.deleteRetrofit2().execute(), SpinnakerHttpException.class); + assertThat(spinnakerHttpException.getUrl()) + .isEqualTo(mockWebServer.url("/retrofit2").toString()); + assertThat(spinnakerHttpException.getHttpMethod()).isEqualTo(HttpMethod.DELETE.toString()); + } + @Test void testNotParameterizedException() { IllegalArgumentException illegalArgumentException = @@ -177,6 +193,9 @@ interface Retrofit2Service { @retrofit2.http.GET("/retrofit2/wrongReturnType") DummyWithExecute testWrongReturnType(); + + @retrofit2.http.DELETE("/retrofit2") + Call deleteRetrofit2(); } @Test diff --git a/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofitErrorHandlerTest.java b/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofitErrorHandlerTest.java index 2f5556183..748fa7bc1 100644 --- a/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofitErrorHandlerTest.java +++ b/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofitErrorHandlerTest.java @@ -176,6 +176,17 @@ void testResponseHeadersInException() { assertThat(spinnakerHttpException.getUrl()).isEqualTo(mockWebServer.url("/foo").toString()); } + @Test + void testExceptionFromRetrofitErrorHasNullHttpMethod() { + mockWebServer.enqueue( + new MockResponse() + .setResponseCode(HttpStatus.BAD_REQUEST.value()) + .setHeader("Test", "true")); + SpinnakerHttpException spinnakerHttpException = + catchThrowableOfType(() -> retrofitService.getFoo(), SpinnakerHttpException.class); + assertThat(spinnakerHttpException.getHttpMethod()).isNull(); + } + @Test void testSimpleSpinnakerNetworkException() { String message = "my custom message"; diff --git a/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerExceptionTest.java b/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerExceptionTest.java index 6c5680ff1..2d24ec97c 100644 --- a/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerExceptionTest.java +++ b/kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerExceptionTest.java @@ -24,6 +24,7 @@ import java.util.List; import okhttp3.Request; import org.junit.jupiter.api.Test; +import org.springframework.http.HttpMethod; import retrofit.RetrofitError; import retrofit.client.Response; import retrofit.converter.ConversionException; @@ -41,6 +42,19 @@ void testSpinnakerNetworkExceptionWithUrl() { SpinnakerNetworkException spinnakerNetworkException = new SpinnakerNetworkException(cause, request); assertThat(spinnakerNetworkException.getUrl()).isEqualTo(url); + assertThat(spinnakerNetworkException.getHttpMethod()).isEqualTo(HttpMethod.GET.toString()); + } + + @Test + void testSpinnakerNetworkExceptionWithSpecificMethod() { + Throwable cause = new Throwable("arbitrary message"); + String url = "http://some-url/"; + Request request = + new Request.Builder().url(url).method(HttpMethod.DELETE.toString(), null).build(); + SpinnakerNetworkException spinnakerNetworkException = + new SpinnakerNetworkException(cause, request); + assertThat(spinnakerNetworkException.getUrl()).isEqualTo(url); + assertThat(spinnakerNetworkException.getHttpMethod()).isEqualTo(HttpMethod.DELETE.toString()); } @Test @@ -70,6 +84,19 @@ void testSpinnakerServerExceptionWithUrl() { SpinnakerServerException spinnakerServerException = new SpinnakerServerException(cause, request); assertThat(spinnakerServerException.getUrl()).isEqualTo(url); + assertThat(spinnakerServerException.getHttpMethod()).isEqualTo(HttpMethod.GET.toString()); + } + + @Test + void testSpinnakerServerExceptionWithSpecificMethod() { + Throwable cause = new Throwable("arbitrary message"); + String url = "http://some-url/"; + Request request = + new Request.Builder().url(url).method(HttpMethod.DELETE.toString(), null).build(); + SpinnakerServerException spinnakerServerException = + new SpinnakerServerException(cause, request); + assertThat(spinnakerServerException.getUrl()).isEqualTo(url); + assertThat(spinnakerServerException.getHttpMethod()).isEqualTo(HttpMethod.DELETE.toString()); } @Test From 40890199000277733fe6bf7e2ddc38a70c78cf11 Mon Sep 17 00:00:00 2001 From: David Byron <82477955+dbyron-sf@users.noreply.github.com> Date: Mon, 19 Feb 2024 20:09:53 -0800 Subject: [PATCH 32/32] chore(dependencies): unpin jooq since the spring boot bom specifies it (#1151) version 2.15.15 of the spring boot bom specifies 3.14.16, so that's what we were getting even though we specified 3.13.6. before: org.jooq:jooq:3.13.6 -> 3.14.16 (c) after org.jooq:jooq -> 3.14.16 --- spinnaker-dependencies/spinnaker-dependencies.gradle | 3 --- 1 file changed, 3 deletions(-) diff --git a/spinnaker-dependencies/spinnaker-dependencies.gradle b/spinnaker-dependencies/spinnaker-dependencies.gradle index 9151e920e..35106800b 100644 --- a/spinnaker-dependencies/spinnaker-dependencies.gradle +++ b/spinnaker-dependencies/spinnaker-dependencies.gradle @@ -12,7 +12,6 @@ ext { bouncycastle : "1.77", brave : "5.12.3", gcp : "25.3.0", - jooq : "3.13.6", jsch : "0.1.54", jschAgentProxy : "0.0.9", protobuf : "3.21.12", @@ -155,8 +154,6 @@ dependencies { api("org.jetbrains.spek:spek-junit-platform-engine:${versions.spek}") api("org.jetbrains.spek:spek-junit-platform-runner:${versions.spek}") api("org.jetbrains.spek:spek-subject-extension:${versions.spek}") - api("org.jooq:jooq:${versions.jooq}") - api("org.jooq:jooq-kotlin:${versions.jooq}") api("org.liquibase:liquibase-core"){ version { strictly "4.24.0"