diff --git a/gradle.properties b/gradle.properties index 1fa230ddb2..74edffd2a6 100644 --- a/gradle.properties +++ b/gradle.properties @@ -1,5 +1,5 @@ -fiatVersion=1.43.0 -korkVersion=7.218.0 +fiatVersion=1.44.0 +korkVersion=7.221.0 kotlinVersion=1.5.32 org.gradle.parallel=true org.gradle.jvmargs=-Xmx6g 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(); 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() 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() 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) } 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();