From 72ad04c32cec08cacd08cd281d9eef4e5d9dfdc9 Mon Sep 17 00:00:00 2001 From: Pranav Bhaskaran <67958542+Pranav-b-7@users.noreply.github.com> Date: Tue, 12 Mar 2024 21:15:55 +0530 Subject: [PATCH 1/9] refactor(flex): use SpinnakerRetrofitErrorHandler with FlexService (#4671) This code change is to maintain the uniformity across all the retrofit client configurations in ORCA, as part of upgrading the retrofit version to 2.x. There are neither any changes to the exception handler logics nor any behaviour changes involved. --- orca-flex/orca-flex.gradle | 1 + .../netflix/spinnaker/orca/flex/config/FlexConfiguration.groovy | 2 ++ 2 files changed, 3 insertions(+) diff --git a/orca-flex/orca-flex.gradle b/orca-flex/orca-flex.gradle index 1fa5592e3b..a7343f829b 100644 --- a/orca-flex/orca-flex.gradle +++ b/orca-flex/orca-flex.gradle @@ -24,4 +24,5 @@ dependencies { implementation("com.netflix.frigga:frigga") implementation("io.spinnaker.kork:kork-core") implementation("io.spinnaker.kork:kork-moniker") + implementation("io.spinnaker.kork:kork-retrofit") } diff --git a/orca-flex/src/main/groovy/com/netflix/spinnaker/orca/flex/config/FlexConfiguration.groovy b/orca-flex/src/main/groovy/com/netflix/spinnaker/orca/flex/config/FlexConfiguration.groovy index 4a58530ad0..b0131e6998 100644 --- a/orca-flex/src/main/groovy/com/netflix/spinnaker/orca/flex/config/FlexConfiguration.groovy +++ b/orca-flex/src/main/groovy/com/netflix/spinnaker/orca/flex/config/FlexConfiguration.groovy @@ -17,6 +17,7 @@ package com.netflix.spinnaker.orca.flex.config import com.fasterxml.jackson.databind.ObjectMapper +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler import com.netflix.spinnaker.orca.flex.FlexService import com.netflix.spinnaker.orca.retrofit.RetrofitConfiguration import com.netflix.spinnaker.orca.retrofit.logging.RetrofitSlf4jLog @@ -59,6 +60,7 @@ class FlexConfiguration { .setEndpoint(flexEndpoint) .setClient(retrofitClient) .setLogLevel(retrofitLogLevel) + .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance()) .setLog(new RetrofitSlf4jLog(FlexService)) .setConverter(new JacksonConverter(mapper)) .build() From a0960044713a2bd143af5ec1c9e8865be8f74f9b Mon Sep 17 00:00:00 2001 From: spinnakerbot Date: Tue, 12 Mar 2024 12:27:37 -0400 Subject: [PATCH 2/9] chore(dependencies): Autobump korkVersion (#4672) Co-authored-by: root --- gradle.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle.properties b/gradle.properties index 1fa230ddb2..b1687f1553 100644 --- a/gradle.properties +++ b/gradle.properties @@ -1,5 +1,5 @@ fiatVersion=1.43.0 -korkVersion=7.218.0 +korkVersion=7.219.0 kotlinVersion=1.5.32 org.gradle.parallel=true org.gradle.jvmargs=-Xmx6g From b2f959212e5f0dc3d8ed3c05a37731cb88a03441 Mon Sep 17 00:00:00 2001 From: spinnakerbot Date: Tue, 12 Mar 2024 13:41:16 -0400 Subject: [PATCH 3/9] chore(dependencies): Autobump korkVersion (#4673) Co-authored-by: root --- gradle.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle.properties b/gradle.properties index b1687f1553..bf39d1015f 100644 --- a/gradle.properties +++ b/gradle.properties @@ -1,5 +1,5 @@ fiatVersion=1.43.0 -korkVersion=7.219.0 +korkVersion=7.220.0 kotlinVersion=1.5.32 org.gradle.parallel=true org.gradle.jvmargs=-Xmx6g From 74ba9caa785f817e7f475d1994add5b9c5c5c4e4 Mon Sep 17 00:00:00 2001 From: spinnakerbot Date: Tue, 12 Mar 2024 16:12:53 -0400 Subject: [PATCH 4/9] chore(dependencies): Autobump fiatVersion (#4674) Co-authored-by: root --- gradle.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle.properties b/gradle.properties index bf39d1015f..442788aa5c 100644 --- a/gradle.properties +++ b/gradle.properties @@ -1,4 +1,4 @@ -fiatVersion=1.43.0 +fiatVersion=1.44.0 korkVersion=7.220.0 kotlinVersion=1.5.32 org.gradle.parallel=true From ad53c1762f8a63dea8fea7b80517b5063d4109d7 Mon Sep 17 00:00:00 2001 From: Pranav Bhaskaran <67958542+Pranav-b-7@users.noreply.github.com> Date: Thu, 14 Mar 2024 21:56:10 +0530 Subject: [PATCH 5/9] refactor(integrations-gremlin): use SpinnakerRetrofitErrorHandler with GremlinService (#4676) As part of the upgradation process of retrofit version to 2.x, this PR aims at ensuring the uniformity across all the retrofit client configurations. Since the use of RetrofitError are not detected, no changes in the exception handler and no behaviour changes as well. --- orca-integrations-gremlin/orca-integrations-gremlin.gradle | 1 + .../com/netflix/spinnaker/orca/config/GremlinConfiguration.kt | 2 ++ 2 files changed, 3 insertions(+) diff --git a/orca-integrations-gremlin/orca-integrations-gremlin.gradle b/orca-integrations-gremlin/orca-integrations-gremlin.gradle index eebff9edb0..a1d746dc08 100644 --- a/orca-integrations-gremlin/orca-integrations-gremlin.gradle +++ b/orca-integrations-gremlin/orca-integrations-gremlin.gradle @@ -6,6 +6,7 @@ dependencies { implementation(project(":orca-retrofit")) implementation("org.springframework.boot:spring-boot-autoconfigure") implementation("org.slf4j:slf4j-api") + implementation("io.spinnaker.kork:kork-retrofit") testImplementation(project(":orca-core-tck")) } diff --git a/orca-integrations-gremlin/src/main/java/com/netflix/spinnaker/orca/config/GremlinConfiguration.kt b/orca-integrations-gremlin/src/main/java/com/netflix/spinnaker/orca/config/GremlinConfiguration.kt index febce12931..b9adb02ac5 100644 --- a/orca-integrations-gremlin/src/main/java/com/netflix/spinnaker/orca/config/GremlinConfiguration.kt +++ b/orca-integrations-gremlin/src/main/java/com/netflix/spinnaker/orca/config/GremlinConfiguration.kt @@ -2,6 +2,7 @@ package com.netflix.spinnaker.orca.config import com.fasterxml.jackson.databind.PropertyNamingStrategy import com.fasterxml.jackson.databind.SerializationFeature +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler import com.netflix.spinnaker.orca.gremlin.GremlinConverter import com.netflix.spinnaker.orca.gremlin.GremlinService import com.netflix.spinnaker.orca.jackson.OrcaObjectMapper @@ -47,6 +48,7 @@ class GremlinConfiguration { .setEndpoint(gremlinEndpoint) .setClient(retrofitClient) .setLogLevel(RestAdapter.LogLevel.BASIC) + .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance()) .setLog(RetrofitSlf4jLog(GremlinService::class.java)) .setConverter(GremlinConverter(mapper)) .build() From ad1b83c17f262e0f2c94566cf1af32caf0b9bf19 Mon Sep 17 00:00:00 2001 From: Pranav Bhaskaran <67958542+Pranav-b-7@users.noreply.github.com> Date: Fri, 15 Mar 2024 21:28:13 +0530 Subject: [PATCH 6/9] refactor(kayenta): use SpinnakerRetrofitErrorHandler with KayentaService (#4678) As part of the upgradation process of retrofit version to 2.x, this PR aims at ensuring the uniformity across all the retrofit client configurations. Since the use of RetrofitError are not detected, no changes in the exception handler and no behaviour changes as well. --- orca-kayenta/orca-kayenta.gradle | 1 + .../spinnaker/orca/kayenta/config/KayentaConfiguration.kt | 2 ++ 2 files changed, 3 insertions(+) diff --git a/orca-kayenta/orca-kayenta.gradle b/orca-kayenta/orca-kayenta.gradle index a7a073c840..2514a1ea17 100644 --- a/orca-kayenta/orca-kayenta.gradle +++ b/orca-kayenta/orca-kayenta.gradle @@ -27,6 +27,7 @@ dependencies { implementation("com.netflix.frigga:frigga") implementation("io.spinnaker.kork:kork-moniker") implementation("com.fasterxml.jackson.module:jackson-module-kotlin") + implementation("io.spinnaker.kork:kork-retrofit") testImplementation(project(":orca-test-kotlin")) testImplementation("com.github.tomakehurst:wiremock-jre8-standalone") diff --git a/orca-kayenta/src/main/kotlin/com/netflix/spinnaker/orca/kayenta/config/KayentaConfiguration.kt b/orca-kayenta/src/main/kotlin/com/netflix/spinnaker/orca/kayenta/config/KayentaConfiguration.kt index 45b516d07b..588b90695f 100644 --- a/orca-kayenta/src/main/kotlin/com/netflix/spinnaker/orca/kayenta/config/KayentaConfiguration.kt +++ b/orca-kayenta/src/main/kotlin/com/netflix/spinnaker/orca/kayenta/config/KayentaConfiguration.kt @@ -18,6 +18,7 @@ package com.netflix.spinnaker.orca.kayenta.config import com.fasterxml.jackson.databind.SerializationFeature.WRITE_DATES_AS_TIMESTAMPS import com.netflix.spinnaker.kork.api.expressions.ExpressionFunctionProvider +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler import com.netflix.spinnaker.orca.jackson.OrcaObjectMapper import com.netflix.spinnaker.orca.kayenta.KayentaService import com.netflix.spinnaker.orca.retrofit.RetrofitConfiguration @@ -77,6 +78,7 @@ class KayentaConfiguration { .setLogLevel(retrofitLogLevel) .setLog(RetrofitSlf4jLog(KayentaService::class.java)) .setConverter(JacksonConverter(mapper)) + .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance()) .build() .create(KayentaService::class.java) } From da1f0bbb93f8980b87a986cbfc3c1548fed3aeb5 Mon Sep 17 00:00:00 2001 From: Abe Garcia <44239562+abe21412@users.noreply.github.com> Date: Fri, 15 Mar 2024 15:52:58 -0400 Subject: [PATCH 7/9] fix(retrofit): use SpinnakerServerException.getHttpMethod() instead of reflection in SpinnakerServerExceptionHandler (#4675) * tests(retrofit): add retrofit2 exceptions to SpinnakerServerExceptionHandlerTest * fix(retrofit): update shouldRetry method in SpinnakerServerExceptionHandler to default to using SpinnakerServerException.getHttpMethod() since findHttpMethodAnnotation returns null for retrofit2 exceptions --------- Co-authored-by: abe garcia --- orca-retrofit/orca-retrofit.gradle | 2 + .../BaseRetrofitExceptionHandler.groovy | 14 +- .../SpinnakerServerExceptionHandler.java | 9 +- .../SpinnakerServerExceptionHandlerTest.java | 122 +++++++++++++++++- 4 files changed, 140 insertions(+), 7 deletions(-) diff --git a/orca-retrofit/orca-retrofit.gradle b/orca-retrofit/orca-retrofit.gradle index 4629990cce..1a25549c05 100644 --- a/orca-retrofit/orca-retrofit.gradle +++ b/orca-retrofit/orca-retrofit.gradle @@ -37,6 +37,8 @@ dependencies { testImplementation("org.junit.jupiter:junit-jupiter-params") testImplementation("org.mockito:mockito-core") testImplementation("org.springframework.boot:spring-boot-test") + testImplementation("com.squareup.okhttp3:mockwebserver") + testImplementation("com.squareup.retrofit2:converter-jackson") testRuntimeOnly("org.junit.jupiter:junit-jupiter-engine") testRuntimeOnly("org.springframework:spring-test") diff --git a/orca-retrofit/src/main/groovy/com/netflix/spinnaker/orca/retrofit/exceptions/BaseRetrofitExceptionHandler.groovy b/orca-retrofit/src/main/groovy/com/netflix/spinnaker/orca/retrofit/exceptions/BaseRetrofitExceptionHandler.groovy index 7c184bf8ac..25bbb52a03 100644 --- a/orca-retrofit/src/main/groovy/com/netflix/spinnaker/orca/retrofit/exceptions/BaseRetrofitExceptionHandler.groovy +++ b/orca-retrofit/src/main/groovy/com/netflix/spinnaker/orca/retrofit/exceptions/BaseRetrofitExceptionHandler.groovy @@ -24,6 +24,10 @@ import static java.net.HttpURLConnection.* abstract class BaseRetrofitExceptionHandler implements ExceptionHandler { boolean shouldRetry(Exception e, String kind, Integer responseCode) { + return shouldRetry(e, kind, null, responseCode) + } + + boolean shouldRetry(Exception e, String kind, String httpMethod, Integer responseCode) { if (isMalformedRequest(kind, e.getMessage())) { return false } @@ -33,7 +37,11 @@ abstract class BaseRetrofitExceptionHandler implements ExceptionHandler { return true } - return isIdempotentRequest(e) && (isNetworkError(kind) || isGatewayErrorCode(kind, responseCode) || isThrottle(kind, responseCode)) + if(httpMethod == null) { + httpMethod = findHttpMethodAnnotation(e) + } + + return isIdempotentRequest(httpMethod) && (isNetworkError(kind) || isGatewayErrorCode(kind, responseCode) || isThrottle(kind, responseCode)) } private boolean isGatewayErrorCode(String kind, Integer responseCode) { @@ -55,8 +63,8 @@ abstract class BaseRetrofitExceptionHandler implements ExceptionHandler { return "UNEXPECTED".equals(kind) && exceptionMessage?.contains("Path parameter") } - private static boolean isIdempotentRequest(Exception e) { - findHttpMethodAnnotation(e) in ["GET", "HEAD", "DELETE", "PUT"] + private static boolean isIdempotentRequest(String httpMethod) { + httpMethod in ["GET", "HEAD", "DELETE", "PUT"] } private static String findHttpMethodAnnotation(Exception exception) { diff --git a/orca-retrofit/src/main/java/com/netflix/spinnaker/orca/retrofit/exceptions/SpinnakerServerExceptionHandler.java b/orca-retrofit/src/main/java/com/netflix/spinnaker/orca/retrofit/exceptions/SpinnakerServerExceptionHandler.java index a4501a2ed0..8e5b74a598 100644 --- a/orca-retrofit/src/main/java/com/netflix/spinnaker/orca/retrofit/exceptions/SpinnakerServerExceptionHandler.java +++ b/orca-retrofit/src/main/java/com/netflix/spinnaker/orca/retrofit/exceptions/SpinnakerServerExceptionHandler.java @@ -43,6 +43,7 @@ public Response handle(String taskName, Exception exception) { String kind; Integer responseCode = null; + String httpMethod = ex.getHttpMethod(); if (ex instanceof SpinnakerNetworkException) { kind = "NETWORK"; @@ -108,6 +109,12 @@ public Response handle(String taskName, Exception exception) { responseDetails.put("kind", kind); + // http method may be null if exception is created from RetrofitError + // so only include in responseDetails when value is valid + if (httpMethod != null) { + responseDetails.put("method", httpMethod); + } + // Although Spinnaker*Exception has a retryable property that other parts of // spinnaker use, ignore it here for compatibility with // RetrofitExceptionHandler, specifically because that doesn't retry (most) @@ -116,6 +123,6 @@ public Response handle(String taskName, Exception exception) { ex.getClass().getSimpleName(), taskName, responseDetails, - shouldRetry(ex, kind, responseCode)); + shouldRetry(ex, kind, httpMethod, responseCode)); } } diff --git a/orca-retrofit/src/test/java/com/netflix/spinnaker/orca/retrofit/exceptions/SpinnakerServerExceptionHandlerTest.java b/orca-retrofit/src/test/java/com/netflix/spinnaker/orca/retrofit/exceptions/SpinnakerServerExceptionHandlerTest.java index 3d4af3c1df..978bf3c342 100644 --- a/orca-retrofit/src/test/java/com/netflix/spinnaker/orca/retrofit/exceptions/SpinnakerServerExceptionHandlerTest.java +++ b/orca-retrofit/src/test/java/com/netflix/spinnaker/orca/retrofit/exceptions/SpinnakerServerExceptionHandlerTest.java @@ -17,6 +17,7 @@ package com.netflix.spinnaker.orca.retrofit.exceptions; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.catchThrowableOfType; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -24,6 +25,7 @@ import com.google.common.base.Throwables; import com.google.common.collect.ImmutableMap; import com.google.gson.Gson; +import com.netflix.spinnaker.kork.retrofit.ErrorHandlingExecutorCallAdapterFactory; import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException; import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerNetworkException; import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler; @@ -33,10 +35,18 @@ import java.util.List; import java.util.Map; import java.util.concurrent.Callable; +import java.util.concurrent.TimeUnit; import java.util.stream.Stream; +import okhttp3.OkHttpClient; +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.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; +import org.springframework.http.HttpStatus; import retrofit.RestAdapter; import retrofit.RetrofitError; import retrofit.client.Client; @@ -45,9 +55,16 @@ import retrofit.converter.GsonConverter; import retrofit.converter.JacksonConverter; import retrofit.mime.TypedString; +import retrofit2.Call; +import retrofit2.Retrofit; +import retrofit2.converter.jackson.JacksonConverterFactory; class SpinnakerServerExceptionHandlerTest { - private static final String URL = "https://some-url"; + private static Retrofit2Service retrofit2Service; + + private static final MockWebServer mockWebServer = new MockWebServer(); + + private static final String URL = mockWebServer.url("https://some-url").toString(); private static final String TASK_NAME = "task name"; @@ -135,6 +152,35 @@ class SpinnakerServerExceptionHandlerTest { SpinnakerServerExceptionHandler spinnakerServerExceptionHandler = new SpinnakerServerExceptionHandler(); + @BeforeAll + static void setupOnce() { + retrofit2Service = + new Retrofit.Builder() + .baseUrl(mockWebServer.url("/")) + .client( + new OkHttpClient.Builder() + .callTimeout(1, TimeUnit.SECONDS) + .connectTimeout(1, TimeUnit.SECONDS) + .build()) + .addCallAdapterFactory(ErrorHandlingExecutorCallAdapterFactory.getInstance()) + .addConverterFactory(JacksonConverterFactory.create()) + .build() + .create(Retrofit2Service.class); + } + + @AfterAll + static void shutdownOnce() throws Exception { + mockWebServer.shutdown(); + } + + interface Retrofit2Service { + @retrofit2.http.GET("/retrofit2") + Call getRetrofit2(); + + @retrofit2.http.POST("/retrofit2") + Call postRetrofit2(); + } + @ParameterizedTest(name = "{index} => onlyHandlesSpinnakerServerExceptionAndSubclasses {0}") @MethodSource("exceptionsForHandlesTest") void onlyHandlesSpinnakerServerExceptionAndSubclasses(Exception ex, boolean supported) { @@ -153,12 +199,71 @@ private static Stream exceptionsForHandlesTest() { } @ParameterizedTest(name = "{index} => testRetryable {0}") - @MethodSource("exceptionsForRetryTest") + @MethodSource({"exceptionsForRetryTest", "retrofit2Exceptions"}) void testRetryable(SpinnakerServerException ex, boolean expectedRetryable) { ExceptionHandler.Response response = spinnakerServerExceptionHandler.handle(TASK_NAME, ex); assertThat(response.isShouldRetry()).isEqualTo(expectedRetryable); } + private static Stream retrofit2Exceptions() { + String responseBody = "{\"test\":\"test\"}"; + + // HTTP 503 is always retryable + mockWebServer.enqueue( + new MockResponse() + .setResponseCode(HttpStatus.SERVICE_UNAVAILABLE.value()) + .setBody(responseBody)); + SpinnakerServerException retryable = + catchThrowableOfType( + () -> retrofit2Service.getRetrofit2().execute(), SpinnakerServerException.class); + + // not retryable because POST is not idempotent + mockWebServer.enqueue( + new MockResponse().setResponseCode(HttpStatus.BAD_REQUEST.value()).setBody(responseBody)); + SpinnakerServerException notRetryable = + catchThrowableOfType( + () -> retrofit2Service.postRetrofit2().execute(), SpinnakerServerException.class); + + // not retryable despite idempotent method because response code not within 429, 502, 503, 504 + mockWebServer.enqueue( + new MockResponse().setResponseCode(HttpStatus.UNAUTHORIZED.value()).setBody(responseBody)); + SpinnakerServerException notRetryableIdempotent = + catchThrowableOfType( + () -> retrofit2Service.getRetrofit2().execute(), SpinnakerServerException.class); + + // idempotent network errors are retryable + mockWebServer.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.NO_RESPONSE)); + SpinnakerServerException idempotentNetworkException = + catchThrowableOfType( + () -> retrofit2Service.getRetrofit2().execute(), SpinnakerNetworkException.class); + + // throttling is retryable if idempotent + mockWebServer.enqueue( + new MockResponse() + .setResponseCode(HttpStatus.TOO_MANY_REQUESTS.value()) + .setBody(responseBody)); + SpinnakerServerException throttledIdempotent = + catchThrowableOfType( + () -> retrofit2Service.getRetrofit2().execute(), SpinnakerServerException.class); + + // throttling is not retryable if not idempotent + mockWebServer.enqueue( + new MockResponse() + .setResponseCode(HttpStatus.TOO_MANY_REQUESTS.value()) + .setBody(responseBody)); + SpinnakerServerException throttledNonIdempotent = + catchThrowableOfType( + () -> retrofit2Service.postRetrofit2().execute(), SpinnakerServerException.class); + + return Stream.of( + Arguments.of(retryable, true), + Arguments.of(notRetryable, false), + Arguments.of(notRetryableIdempotent, false), + Arguments.of(idempotentNetworkException, true), + Arguments.of(throttledIdempotent, true), + Arguments.of(throttledNonIdempotent, false)); + } + private static Stream exceptionsForRetryTest() throws Exception { // This isn't retryable because it's not considered an idempotent request // since there's no retrofit http method annotation in the exception's stack @@ -220,7 +325,7 @@ private static Stream exceptionsForRetryTest() throws Exception { } @ParameterizedTest(name = "{index} => verifyResponseDetails {0}") - @MethodSource("exceptionsForResponseDetailsTest") + @MethodSource({"exceptionsForResponseDetailsTest", "nonRetryableRetrofit2Exceptions"}) void verifyResponseDetails(SpinnakerHttpException spinnakerHttpException) { ExceptionHandler.Response response = spinnakerServerExceptionHandler.handle(TASK_NAME, spinnakerHttpException); @@ -229,6 +334,7 @@ void verifyResponseDetails(SpinnakerHttpException spinnakerHttpException) { // expected. This duplicates some logic in SpinnakerServerExceptionHandler, // but at least it helps detect future changes. Map responseBody = spinnakerHttpException.getResponseBody(); + String httpMethod = spinnakerHttpException.getHttpMethod(); String error = null; List errors = null; @@ -238,6 +344,10 @@ void verifyResponseDetails(SpinnakerHttpException spinnakerHttpException) { .put("url", spinnakerHttpException.getUrl()) .put("status", spinnakerHttpException.getResponseCode()); + if (httpMethod != null) { + builder.put("method", httpMethod); + } + if (responseBody == null) { error = spinnakerHttpException.getMessage(); errors = List.of(); @@ -285,6 +395,12 @@ private static Stream exceptionsForResponseDetailsTest() .map(SpinnakerServerExceptionHandlerTest::makeSpinnakerHttpException); } + private static Stream nonRetryableRetrofit2Exceptions() { + return retrofit2Exceptions() + .filter(args -> args.get()[1].equals(false)) + .map(args -> ((SpinnakerHttpException) args.get()[0])); + } + private void compareResponse( ExceptionHandler.Response expectedResponse, ExceptionHandler.Response actualResponse) { assertThat(actualResponse).isNotNull(); From e08819af1f2f163812560c99bd055aa869ddde84 Mon Sep 17 00:00:00 2001 From: spinnakerbot Date: Thu, 21 Mar 2024 12:50:28 -0400 Subject: [PATCH 8/9] chore(dependencies): Autobump korkVersion (#4680) Co-authored-by: root --- gradle.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle.properties b/gradle.properties index 442788aa5c..74edffd2a6 100644 --- a/gradle.properties +++ b/gradle.properties @@ -1,5 +1,5 @@ fiatVersion=1.44.0 -korkVersion=7.220.0 +korkVersion=7.221.0 kotlinVersion=1.5.32 org.gradle.parallel=true org.gradle.jvmargs=-Xmx6g From 613427f41bb16461a7ee8bb0f31212b438d75458 Mon Sep 17 00:00:00 2001 From: Yash Saxena <41922677+yashsaxena1503@users.noreply.github.com> Date: Fri, 22 Mar 2024 23:07:27 +0530 Subject: [PATCH 9/9] fix(check-pre-condition): CheckPrecondition doesn't evaluate expression correctly after upstream stages get restarted (#4682) Co-authored-by: ysaxena --- .../pipeline/CompoundExecutionOperator.java | 15 +-- .../CompoundExecutionOperatorTest.java | 92 +++++++++++++++++++ 2 files changed, 100 insertions(+), 7 deletions(-) diff --git a/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/CompoundExecutionOperator.java b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/CompoundExecutionOperator.java index dce150404d..2e0b45a543 100644 --- a/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/CompoundExecutionOperator.java +++ b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/CompoundExecutionOperator.java @@ -141,15 +141,15 @@ public PipelineExecution restartStage( private PipelineExecution updatePreconditionStageExpression( Map restartDetails, PipelineExecution execution) { - List preconditionList = getPreconditionsFromStage(restartDetails); - if (preconditionList.isEmpty()) { + Map> preconditionMap = getPreconditionsFromStage(restartDetails); + if (preconditionMap.isEmpty()) { return execution; } for (StageExecution stage : execution.getStages()) { if (stage.getType() != null && stage.getType().equalsIgnoreCase("checkPreconditions")) { if (stage.getContext().get("preconditions") != null) { - stage.getContext().replace("preconditions", preconditionList); + stage.getContext().replace("preconditions", preconditionMap.get(stage.getRefId())); repository.storeStage(stage); log.info("Updated preconditions for CheckPreconditions stage"); } @@ -158,8 +158,8 @@ private PipelineExecution updatePreconditionStageExpression( return execution; } - private List getPreconditionsFromStage(Map restartDetails) { - List preconditionList = new ArrayList(); + private Map> getPreconditionsFromStage(Map restartDetails) { + Map> preconditionMap = new HashMap<>(); Map pipelineConfigMap = new HashMap(restartDetails); List keysToRetain = new ArrayList(); @@ -173,12 +173,13 @@ private List getPreconditionsFromStage(Map restartDetails) { List pipelineStageList = pipelineStageMap.get(keysToRetain.get(0)); for (Map stageMap : pipelineStageList) { if (stageMap.get("type").toString().equalsIgnoreCase("checkPreconditions")) { - preconditionList = (List) stageMap.get("preconditions"); + preconditionMap.put( + (String) stageMap.get("refId"), (List) stageMap.get("preconditions")); log.info("Retrieved preconditions for CheckPreconditions stage"); } } } - return preconditionList; + return preconditionMap; } private PipelineExecution doInternal( diff --git a/orca-core/src/test/java/com/netflix/spinnaker/orca/pipeline/CompoundExecutionOperatorTest.java b/orca-core/src/test/java/com/netflix/spinnaker/orca/pipeline/CompoundExecutionOperatorTest.java index a3220b5053..a8864ae091 100644 --- a/orca-core/src/test/java/com/netflix/spinnaker/orca/pipeline/CompoundExecutionOperatorTest.java +++ b/orca-core/src/test/java/com/netflix/spinnaker/orca/pipeline/CompoundExecutionOperatorTest.java @@ -41,6 +41,9 @@ final class CompoundExecutionOperatorTest { private static final String PIPELINE = "mypipeline"; private static final String EXECUTION_ID = "EXECUTION_ID"; private static final String STAGE_ID = "STAGE_ID"; + private static final String UPSTREAM_STAGE = "UPSTREAM_STAGE"; + private static final String UPSTREAM_STAGE_ID = "UPSTREAM_STAGE_ID"; + private static final int PRECONDITION_STAGE_LIST_SIZE = 2; private final ExecutionRepository repository = mock(ExecutionRepository.class); private final ExecutionRunner runner = mock(ExecutionRunner.class); private final RetrySupport retrySupport = mock(RetrySupport.class); @@ -76,6 +79,46 @@ void restartStageWithValidExpression() { assertEquals(APPLICATION, updatedExecution.getApplication()); } + @Test + void restartUpstreamStageWithMultiplePreconditionStages() { + // ARRANGE + StageExecution upstreamStage = buildUpstreamStage(); + List preconditionStages = new ArrayList<>(PRECONDITION_STAGE_LIST_SIZE); + List executionStages = execution.getStages(); + executionStages.add(upstreamStage); + + for (int i = 0; i < PRECONDITION_STAGE_LIST_SIZE; i++) { + StageExecution preconditionStage = + buildPreconditionStage("expression_" + i, "precondition_" + i, "precondition_stage_" + i); + preconditionStages.add(preconditionStage); + executionStages.add(preconditionStage); + } + + Map restartDetails = + Map.of( + "application", APPLICATION, + "name", PIPELINE, + "executionId", EXECUTION_ID, + "stages", buildExpectedRestartStageList(preconditionStages, upstreamStage)); + + when(repository.retrieve(any(), anyString())).thenReturn(execution); + when(repository.handlesPartition(execution.getPartition())).thenReturn(true); + + // ACT + PipelineExecution updatedExecution = + executionOperator.restartStage(EXECUTION_ID, STAGE_ID, restartDetails); + + // ASSERT + assertEquals(APPLICATION, updatedExecution.getApplication()); + for (int i = 0, size = preconditionStages.size(); i < size; i++) { + StageExecution originalPreconditionStage = preconditionStages.get(i); + StageExecution updatedPreconditionStage = updatedExecution.getStages().get(i + 1); + assertEquals(originalPreconditionStage.getContext(), updatedPreconditionStage.getContext()); + assertEquals(originalPreconditionStage.getType(), updatedPreconditionStage.getType()); + assertEquals(originalPreconditionStage.getName(), updatedPreconditionStage.getName()); + } + } + @Test void restartStageWithNoPreconditions() { @@ -135,6 +178,55 @@ private PipelineExecution buildExpectedExecution( return execution; } + private List> buildExpectedRestartStageList( + List preconditionStages, StageExecution upstreamStage) { + List> restartStageList = new ArrayList<>(preconditionStages.size() + 1); + + Map upstreamStageMap = + Map.of( + "name", upstreamStage.getName(), + "type", upstreamStage.getType()); + restartStageList.add(upstreamStageMap); + + for (StageExecution stage : preconditionStages) { + Map stageMap = + Map.of( + "name", stage.getName(), + "type", stage.getType(), + "preconditions", stage.getContext().get("preconditions")); + restartStageList.add(stageMap); + } + return restartStageList; + } + + private StageExecution buildPreconditionStage( + String stageStatus, String stageId, String preConditionStageName) { + StageExecution preconditionStage = new StageExecutionImpl(); + preconditionStage.setId(stageId); + preconditionStage.setType("checkPreconditions"); + preconditionStage.setName(preConditionStageName); + Map contextMap = new HashMap<>(); + contextMap.put("stageName", UPSTREAM_STAGE); + contextMap.put("stageStatus", stageStatus); + List> preconditionList = new ArrayList<>(); + Map preconditionMap = new HashMap<>(); + preconditionMap.put("context", contextMap); + preconditionMap.put("failPipeline", true); + preconditionMap.put("type", "stageStatus"); + preconditionList.add(preconditionMap); + contextMap.put("preconditions", preconditionList); + preconditionStage.setContext(contextMap); + return preconditionStage; + } + + private StageExecution buildUpstreamStage() { + StageExecution upstreamStage = new StageExecutionImpl(); + upstreamStage.setId(UPSTREAM_STAGE_ID); + upstreamStage.setType("manualJudgment"); + upstreamStage.setName(UPSTREAM_STAGE); + return upstreamStage; + } + private Map buildExpectedRestartDetailsMap(String expression) { Map restartDetails = new HashMap<>(); List pipelineStageList = new ArrayList();