From 886e2b1e19f0e5adfc01c53406bd14a7ffff9af9 Mon Sep 17 00:00:00 2001 From: Pranav Bhaskaran <67958542+Pranav-b-7@users.noreply.github.com> Date: Thu, 28 Mar 2024 21:57:46 +0530 Subject: [PATCH] refactor(mine): use SpinnakerRetrofitErrorHandler with MineService (#4679) * test(mine): verify http error case in RegisterCanaryTask These tests ensures the upcoming changes on MineService with SpinnakerRetrofitErrorHandler, will not modify/break the existing functionality. Test covers the existing behaviour of RegisterCanaryTask when some HTTP error has occurred. * fix(mine): handle exceptions when error response body is null in RegisterCanaryTask Before this change the RetrofitError catch block would throw a NullPointerException when error response body is null at line no: https://github.com/spinnaker/orca/blob/613427f41bb16461a7ee8bb0f31212b438d75458/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/tasks/RegisterCanaryTask.groovy#L70 Here is the exact stack trace : java.lang.NullPointerException: Cannot set property 'status' on null object at org.codehaus.groovy.runtime.NullObject.setProperty(NullObject.java:80) at org.codehaus.groovy.runtime.InvokerHelper.setProperty(InvokerHelper.java:213) at org.codehaus.groovy.runtime.ScriptBytecodeAdapter.setProperty(ScriptBytecodeAdapter.java:496) at com.netflix.spinnaker.orca.mine.tasks.RegisterCanaryTask.execute(RegisterCanaryTask.groovy:70) * test(mine): verify network error case in RegisterCanaryTask These tests ensures the upcoming changes on MineService with SpinnakerRetrofitErrorHandler, will not modify/break the fix made in the commit: 9d554a986a8b66be77489cbcebbacc8cddd34165. * refactor(mine): use SpinnakerRetrofitErrorHandler with MineService This PR lays the foundational work for upgrading the retrofit version to 2.x, specifically focusing on refactoring the exception handling for MineService There is no behaviour changes involved. Couple of tests added to verify the same for only RegisterCanaryTask in the previous commits. --- orca-mine/orca-mine.gradle | 4 + .../orca/mine/config/MineConfiguration.groovy | 2 + .../orca/mine/pipeline/AcaTaskStage.groovy | 7 +- .../orca/mine/tasks/DisableCanaryTask.groovy | 4 +- .../orca/mine/tasks/MonitorAcaTaskTask.groovy | 4 +- .../orca/mine/tasks/MonitorCanaryTask.groovy | 4 +- .../orca/mine/tasks/RegisterCanaryTask.groovy | 27 +- .../mine/tasks/RegisterCanaryTaskTest.java | 247 ++++++++++++++++++ 8 files changed, 284 insertions(+), 15 deletions(-) create mode 100644 orca-mine/src/test/java/com/netflix/spinnaker/orca/mine/tasks/RegisterCanaryTaskTest.java diff --git a/orca-mine/orca-mine.gradle b/orca-mine/orca-mine.gradle index f384de8f14..b0b03e4010 100644 --- a/orca-mine/orca-mine.gradle +++ b/orca-mine/orca-mine.gradle @@ -24,6 +24,10 @@ dependencies { implementation("org.springframework:spring-context") implementation("org.springframework.boot:spring-boot-autoconfigure") implementation("com.netflix.frigga:frigga") + implementation("io.spinnaker.kork:kork-retrofit") + + testImplementation("org.junit.jupiter:junit-jupiter-params") + testImplementation("com.github.tomakehurst:wiremock-jre8-standalone") } sourceSets { diff --git a/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/config/MineConfiguration.groovy b/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/config/MineConfiguration.groovy index 94e03f5fbc..07b1500b03 100644 --- a/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/config/MineConfiguration.groovy +++ b/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/config/MineConfiguration.groovy @@ -17,6 +17,7 @@ package com.netflix.spinnaker.orca.mine.config import com.fasterxml.jackson.databind.ObjectMapper +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler import com.netflix.spinnaker.orca.mine.MineService import com.netflix.spinnaker.orca.retrofit.RetrofitConfiguration import com.netflix.spinnaker.orca.retrofit.logging.RetrofitSlf4jLog @@ -64,6 +65,7 @@ class MineConfiguration { .setClient(retrofitClient) .setLogLevel(retrofitLogLevel) .setLog(new RetrofitSlf4jLog(MineService)) + .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance()) .setConverter(new JacksonConverter(objectMapper)) .build() .create(MineService) diff --git a/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/pipeline/AcaTaskStage.groovy b/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/pipeline/AcaTaskStage.groovy index dca0c89356..73827cd471 100644 --- a/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/pipeline/AcaTaskStage.groovy +++ b/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/pipeline/AcaTaskStage.groovy @@ -16,6 +16,7 @@ package com.netflix.spinnaker.orca.mine.pipeline +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException import com.netflix.spinnaker.orca.api.pipeline.CancellableStage import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution import com.netflix.spinnaker.orca.mine.MineService @@ -27,12 +28,10 @@ import com.netflix.spinnaker.orca.api.pipeline.graph.TaskNode import groovy.util.logging.Slf4j import org.springframework.beans.factory.annotation.Autowired import org.springframework.stereotype.Component -import retrofit.RetrofitError import javax.annotation.Nonnull import static org.springframework.http.HttpStatus.CONFLICT -import static retrofit.RetrofitError.Kind.HTTP @Slf4j @Component @@ -79,8 +78,8 @@ class AcaTaskStage implements StageDefinitionBuilder, CancellableStage { def cancelCanaryResults = mineService.cancelCanary(stage.context.canary.id as String, reason) log.info("Canceled canary in mine (canaryId: ${stage.context.canary.id}, stageId: ${stage.id}, executionId: ${stage.execution.id}): ${reason}") return cancelCanaryResults - } catch (RetrofitError e) { - if (e.kind == HTTP && e.response.status == CONFLICT.value()) { + } catch (SpinnakerHttpException e) { + if (e.responseCode == CONFLICT.value()) { log.info("Canary (canaryId: ${stage.context.canary.id}, stageId: ${stage.id}, executionId: ${stage.execution.id}) has already ended") return [:] } else { diff --git a/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/tasks/DisableCanaryTask.groovy b/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/tasks/DisableCanaryTask.groovy index a7ec625f9e..03912761dc 100644 --- a/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/tasks/DisableCanaryTask.groovy +++ b/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/tasks/DisableCanaryTask.groovy @@ -16,6 +16,7 @@ package com.netflix.spinnaker.orca.mine.tasks +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus import com.netflix.spinnaker.orca.api.pipeline.Task import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution @@ -27,7 +28,6 @@ import com.netflix.spinnaker.orca.mine.MineService import groovy.util.logging.Slf4j import org.springframework.beans.factory.annotation.Autowired import org.springframework.stereotype.Component -import retrofit.RetrofitError import javax.annotation.Nonnull @@ -49,7 +49,7 @@ class DisableCanaryTask implements CloudProviderAware, Task { unhealthy : true ]).build() } - } catch (RetrofitError e) { + } catch (SpinnakerServerException e) { log.error("Exception occurred while getting canary status with id {} from mine, continuing with disable", stage.context.canary.id, e) } diff --git a/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/tasks/MonitorAcaTaskTask.groovy b/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/tasks/MonitorAcaTaskTask.groovy index 3cd507d373..2b9f92a601 100644 --- a/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/tasks/MonitorAcaTaskTask.groovy +++ b/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/tasks/MonitorAcaTaskTask.groovy @@ -16,6 +16,7 @@ package com.netflix.spinnaker.orca.mine.tasks +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException import com.netflix.spinnaker.security.AuthenticatedRequest import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution import java.util.concurrent.TimeUnit @@ -28,7 +29,6 @@ import com.netflix.spinnaker.orca.mine.MineService import groovy.util.logging.Slf4j import org.springframework.beans.factory.annotation.Autowired import org.springframework.stereotype.Component -import retrofit.RetrofitError @Component @Slf4j @@ -51,7 +51,7 @@ class MonitorAcaTaskTask implements CloudProviderAware, OverridableTimeoutRetrya outputs << [ canary: canary ] - } catch (RetrofitError e) { + } catch (SpinnakerServerException e) { log.error("Exception occurred while getting canary with id ${context.canary.id} from mine service", e) return TaskResult.builder(ExecutionStatus.RUNNING).context(outputs).build() } diff --git a/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/tasks/MonitorCanaryTask.groovy b/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/tasks/MonitorCanaryTask.groovy index 18b4961022..d6d45ccc2e 100644 --- a/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/tasks/MonitorCanaryTask.groovy +++ b/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/tasks/MonitorCanaryTask.groovy @@ -16,6 +16,7 @@ package com.netflix.spinnaker.orca.mine.tasks +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution import java.util.concurrent.TimeUnit @@ -29,7 +30,6 @@ import com.netflix.spinnaker.orca.mine.MineService import groovy.util.logging.Slf4j import org.springframework.beans.factory.annotation.Autowired import org.springframework.stereotype.Component -import retrofit.RetrofitError @Component @Slf4j @@ -59,7 +59,7 @@ class MonitorCanaryTask implements CloudProviderAware, OverridableTimeoutRetryab outputs << [ canary : mineService.getCanary(context.canary.id) ] - } catch (RetrofitError e) { + } catch (SpinnakerServerException e) { log.error("Exception occurred while getting canary with id ${context.canary.id} from mine service", e) return TaskResult.builder(ExecutionStatus.RUNNING).context(outputs).build() } diff --git a/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/tasks/RegisterCanaryTask.groovy b/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/tasks/RegisterCanaryTask.groovy index ac56935e2e..9228b8357b 100644 --- a/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/tasks/RegisterCanaryTask.groovy +++ b/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/tasks/RegisterCanaryTask.groovy @@ -16,6 +16,10 @@ package com.netflix.spinnaker.orca.mine.tasks +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerConversionException +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerNetworkException +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus import com.netflix.spinnaker.orca.api.pipeline.Task import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution @@ -25,7 +29,6 @@ import com.netflix.spinnaker.orca.mine.pipeline.DeployCanaryStage import groovy.util.logging.Slf4j import org.springframework.beans.factory.annotation.Autowired import org.springframework.stereotype.Component -import retrofit.RetrofitError import retrofit.client.Response import javax.annotation.Nonnull @@ -59,20 +62,34 @@ class RegisterCanaryTask implements Task { if (response.status == 200 && response.body.mimeType().startsWith('text/plain')) { canaryId = response.body.in().text } - } catch (RetrofitError re) { + } catch (SpinnakerHttpException re) { def response = [:] try { - response = re.getBodyAs(Map) as Map + def responseBody = re.responseBody + response = responseBody!=null ? responseBody : response } catch (Exception e) { response.error = e.message } - response.status = re.response?.status - response.errorKind = re.kind + response.status = re.responseCode + response.errorKind = "HTTP" throw new IllegalStateException( "Unable to register canary (executionId: ${stage.execution.id}, stageId: ${stage.id} canary: ${c}), response: ${response}" ) + } catch(SpinnakerServerException e){ + def response = [:] + response.status = null + if (e instanceof SpinnakerNetworkException){ + response.errorKind = "NETWORK" + } else if(e instanceof SpinnakerConversionException) { + response.errorKind = "CONVERSION" + } else { + response.errorKind = "UNEXPECTED" + } + throw new IllegalStateException( + "Unable to register canary (executionId: ${stage.execution.id}, stageId: ${stage.id} canary: ${c}), response: ${response}" + ) } log.info("Registered Canary (executionId: ${stage.execution.id}, stageId: ${stage.id}, canaryId: ${canaryId})") diff --git a/orca-mine/src/test/java/com/netflix/spinnaker/orca/mine/tasks/RegisterCanaryTaskTest.java b/orca-mine/src/test/java/com/netflix/spinnaker/orca/mine/tasks/RegisterCanaryTaskTest.java new file mode 100644 index 0000000000..1beb5f57f1 --- /dev/null +++ b/orca-mine/src/test/java/com/netflix/spinnaker/orca/mine/tasks/RegisterCanaryTaskTest.java @@ -0,0 +1,247 @@ +/* + * Copyright 2024 OpsMx, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.netflix.spinnaker.orca.mine.tasks; + +import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; +import static com.github.tomakehurst.wiremock.client.WireMock.urlPathEqualTo; +import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import com.fasterxml.jackson.databind.ObjectMapper; +import com.github.tomakehurst.wiremock.client.WireMock; +import com.github.tomakehurst.wiremock.http.Fault; +import com.github.tomakehurst.wiremock.http.HttpHeaders; +import com.github.tomakehurst.wiremock.junit5.WireMockExtension; +import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo; +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler; +import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution; +import com.netflix.spinnaker.orca.mine.MineService; +import com.netflix.spinnaker.orca.mine.pipeline.DeployCanaryStage; +import com.netflix.spinnaker.orca.pipeline.model.PipelineExecutionImpl; +import com.netflix.spinnaker.orca.pipeline.model.StageExecutionImpl; +import com.netflix.spinnaker.orca.retrofit.logging.RetrofitSlf4jLog; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.UUID; +import org.jetbrains.annotations.NotNull; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; +import org.springframework.http.HttpStatus; +import retrofit.RestAdapter; +import retrofit.client.OkClient; +import retrofit.client.Response; +import retrofit.converter.JacksonConverter; +import retrofit.mime.TypedString; + +public class RegisterCanaryTaskTest { + + private static MineService mineService; + + private static RegisterCanaryTask registerCanaryTask; + + private static StageExecution deployCanaryStage; + + private static ObjectMapper objectMapper = new ObjectMapper(); + + @RegisterExtension + static WireMockExtension wireMock = + WireMockExtension.newInstance().options(wireMockConfig().dynamicPort()).build(); + + @BeforeAll + static void setupOnce(WireMockRuntimeInfo wmRuntimeInfo) { + OkClient okClient = new OkClient(); + RestAdapter.LogLevel retrofitLogLevel = RestAdapter.LogLevel.NONE; + + mineService = + new RestAdapter.Builder() + .setEndpoint(wmRuntimeInfo.getHttpBaseUrl()) + .setClient(okClient) + .setLogLevel(retrofitLogLevel) + .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance()) + .setLog(new RetrofitSlf4jLog(MineService.class)) + .setConverter(new JacksonConverter(objectMapper)) + .build() + .create(MineService.class); + + registerCanaryTask = new RegisterCanaryTask(); + registerCanaryTask.setMineService(mineService); + } + + @BeforeEach + public void setup() { + var pipeline = PipelineExecutionImpl.newPipeline("foo"); + + var canaryStageId = UUID.randomUUID().toString(); + var parentStageId = UUID.randomUUID().toString(); + + deployCanaryStage = getDeployCanaryStage(pipeline, canaryStageId); + deployCanaryStage.setParentStageId(parentStageId); + var monitorCanaryStage = + new StageExecutionImpl(pipeline, "monitorCanary", Collections.emptyMap()); + + pipeline.getStages().addAll(List.of(deployCanaryStage, monitorCanaryStage)); + } + + private static void simulateFault(String url, String body, HttpStatus httpStatus) { + wireMock.givenThat( + WireMock.post(urlPathEqualTo(url)) + .willReturn( + aResponse() + .withHeaders(HttpHeaders.noHeaders()) + .withStatus(httpStatus.value()) + .withBody(body))); + } + + private static void simulateFault(String url, Fault fault) { + wireMock.givenThat(WireMock.post(urlPathEqualTo(url)).willReturn(aResponse().withFault(fault))); + } + + @Test + public void verifyRegisterCanaryThrowsHttpError() throws Exception { + + var url = "https://mine.service.com/registerCanary"; + + Response response = + new Response( + url, + HttpStatus.NOT_ACCEPTABLE.value(), + HttpStatus.NOT_ACCEPTABLE.getReasonPhrase(), + List.of(), + new TypedString("canaryId")); + + String errorResponseBody = objectMapper.writeValueAsString(response); + + // simulate error HTTP 406 + simulateFault("/registerCanary", errorResponseBody, HttpStatus.NOT_ACCEPTABLE); + + var canaryObject = (LinkedHashMap) deployCanaryStage.getContext().get("canary"); + canaryObject.put("application", "foo"); + + var canaryConfig = (LinkedHashMap) canaryObject.get("canaryConfig"); + canaryConfig.put("name", deployCanaryStage.getExecution().getId()); + canaryConfig.put("application", "foo"); + + // Format the canary data as per error log message + var canary = + Objects.toString(deployCanaryStage.getContext().get("canary")) + .replace("{", "[") + .replace("}", "]") + .replace("=", ":"); + + assertThatThrownBy(() -> registerCanaryTask.execute(deployCanaryStage)) + .hasMessageStartingWith( + String.format( + "Unable to register canary (executionId: %s, stageId: %s canary: %s)", + deployCanaryStage.getExecution().getId(), deployCanaryStage.getId(), canary)) + .hasMessageContaining("response: [") + .hasMessageContaining("reason:" + HttpStatus.NOT_ACCEPTABLE.getReasonPhrase()) + .hasMessageContaining("body:[bytes:Y2FuYXJ5SWQ=]") + .hasMessageContaining("errorKind:HTTP") + .hasMessageContaining("url:" + url) + .hasMessageContaining("status:" + HttpStatus.NOT_ACCEPTABLE.value()); + } + + @Test + public void verifyRegisterCanaryThrowsNetworkError() { + + // simulate network error + simulateFault("/registerCanary", Fault.CONNECTION_RESET_BY_PEER); + + var canaryObject = (LinkedHashMap) deployCanaryStage.getContext().get("canary"); + canaryObject.put("application", "foo"); + + var canaryConfig = (LinkedHashMap) canaryObject.get("canaryConfig"); + canaryConfig.put("name", deployCanaryStage.getExecution().getId()); + canaryConfig.put("application", "foo"); + + // Format the canary data as per error log message + var canary = + Objects.toString(deployCanaryStage.getContext().get("canary")) + .replace("{", "[") + .replace("}", "]") + .replace("=", ":"); + + String errorResponseBody = "[status:null, errorKind:NETWORK]"; + + assertThatThrownBy(() -> registerCanaryTask.execute(deployCanaryStage)) + .hasMessage( + String.format( + "Unable to register canary (executionId: %s, stageId: %s canary: %s), response: %s", + deployCanaryStage.getExecution().getId(), + deployCanaryStage.getId(), + canary, + errorResponseBody)); + } + + /* + * Populate Register canary stage execution test data, + * {@link LinkedHashMap} is used to maintain the insertion order , so the assertions will be much simpler + * */ + @NotNull + private static StageExecutionImpl getDeployCanaryStage( + PipelineExecutionImpl pipeline, String canaryStageId) { + return new StageExecutionImpl( + pipeline, + DeployCanaryStage.PIPELINE_CONFIG_TYPE, + new LinkedHashMap<>( + Map.of( + "canaryStageId", + canaryStageId, + "account", + "test", + "canary", + new LinkedHashMap<>( + Map.of( + "owner", + new LinkedHashMap<>( + Map.of("name", "cfieber", "email", "cfieber@netflix.com")), + "watchers", + Collections.emptyList(), + "canaryConfig", + new LinkedHashMap<>( + Map.of( + "lifetimeHours", + 1, + "combinedCanaryResultStrategy", + "LOWEST", + "canarySuccessCriteria", + new LinkedHashMap<>(Map.of("canaryResultScore", 95)), + "canaryHealthCheckHandler", + new LinkedHashMap<>( + Map.of( + "minimumCanaryResultScore", + 75, + "@class", + "com.netflix.spinnaker.mine.CanaryResultHealthCheckHandler")), + "canaryAnalysisConfig", + new LinkedHashMap<>( + Map.of( + "name", + "beans", + "beginCanaryAnalysisAfterMins", + 5, + "notificationHours", + List.of(1, 2), + "canaryAnalysisIntervalMins", + 15))))))))); + } +}