From 11438e9e873243ec2c520840e3c1413482488eea Mon Sep 17 00:00:00 2001 From: Pranav-b-7 Date: Tue, 2 Jan 2024 12:58:14 +0530 Subject: [PATCH 1/2] test(deploymentmonitor): Add Test Case for EvaluateDeploymentTask MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit introduces a test case aimed at demonstrating the existing behaviour of the ‘EvaluateDeploymentTask’ task. The primary objective of this test case is to verify the proper handling of ‘RetrofitError’ thrown by the retrofit client when interacting with DeploymentMonitorService APIs. Details: - Added a dedicated test case to assess the behaviour of ‘EvaluateDeploymentTask’. - The test case specifically focuses on handling ‘RetrofitError’ instances thrown by DeploymentMonitorService APIs. - This step is essential for understanding and validating the current behaviour of the task in response to potential errors during API interactions. This test case serves as a baseline for future modifications and enhancements related to the ‘EvaluateDeploymentTask’. --- .../EvaluateDeploymentHealthTaskSpec.groovy | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/EvaluateDeploymentHealthTaskSpec.groovy b/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/EvaluateDeploymentHealthTaskSpec.groovy index b26591e1d7..a90fcc855c 100644 --- a/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/EvaluateDeploymentHealthTaskSpec.groovy +++ b/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/EvaluateDeploymentHealthTaskSpec.groovy @@ -16,10 +16,12 @@ package com.netflix.spinnaker.orca.clouddriver.tasks.monitoreddeploy +import com.fasterxml.jackson.databind.ObjectMapper import com.netflix.spectator.api.NoopRegistry import com.netflix.spinnaker.config.DeploymentMonitorDefinition import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus import com.netflix.spinnaker.orca.api.pipeline.TaskResult +import com.netflix.spinnaker.orca.clouddriver.MortServiceSpec import com.netflix.spinnaker.orca.deploymentmonitor.DeploymentMonitorService import com.netflix.spinnaker.orca.deploymentmonitor.models.DeploymentMonitorStageConfig import com.netflix.spinnaker.orca.deploymentmonitor.models.DeploymentStep @@ -27,7 +29,10 @@ import com.netflix.spinnaker.orca.deploymentmonitor.models.EvaluateHealthRespons import com.netflix.spinnaker.orca.deploymentmonitor.models.MonitoredDeployInternalStageData import com.netflix.spinnaker.orca.pipeline.model.PipelineExecutionImpl import com.netflix.spinnaker.orca.pipeline.model.StageExecutionImpl +import org.springframework.http.HttpStatus import retrofit.RetrofitError +import retrofit.client.Response +import retrofit.converter.JacksonConverter import spock.lang.Specification import com.netflix.spinnaker.orca.deploymentmonitor.DeploymentMonitorServiceProvider import spock.lang.Unroll @@ -198,6 +203,50 @@ class EvaluateDeploymentHealthTaskSpec extends Specification { false | null || ExecutionStatus.FAILED_CONTINUE } + def "should return status as RUNNING when Retrofit http error is thrown"() { + + def converter = new JacksonConverter(new ObjectMapper()) + + Response response = + new Response( + "/deployment/evaluateHealth", + HttpStatus.BAD_REQUEST.value(), + "bad-request", + Collections.emptyList(), + new MortServiceSpec.MockTypedInput(converter, [ + accountName: "account", + description: "simple description", + name: "sg1", + region: "region", + type: "openstack" + ])) + + given: + def monitorServiceStub = Stub(DeploymentMonitorService) { + evaluateHealth(_) >> { + throw RetrofitError.httpError("https://foo.com/deployment/evaluateHealth", response, converter, null) + } + } + + def serviceProviderStub = getServiceProviderStub(monitorServiceStub) + + def task = new EvaluateDeploymentHealthTask(serviceProviderStub, new NoopRegistry()) + + MonitoredDeployInternalStageData stageData = new MonitoredDeployInternalStageData() + stageData.deploymentMonitor = new DeploymentMonitorStageConfig() + stageData.deploymentMonitor.id = "LogMonitorId" + + def stage = new StageExecutionImpl(pipe, "evaluateDeploymentHealth", stageData.toContextMap() + [application: pipe.application]) + stage.startTime = Instant.now().toEpochMilli() + + when: 'we can still retry' + TaskResult result = task.execute(stage) + + then: 'should retry' + result.status == ExecutionStatus.RUNNING + result.context.deployMonitorHttpRetryCount == 1 + } + private getServiceProviderStub(monitorServiceStub) { return getServiceProviderStub(monitorServiceStub, {}) } From 52e528f784f904cb04210b90628dcfe173ed1832 Mon Sep 17 00:00:00 2001 From: Pranav-b-7 Date: Wed, 22 Nov 2023 13:45:03 +0530 Subject: [PATCH 2/2] refactor(deploymentmonitor): Configure SpinnakerRetrofitErrorHandler for DeploymentMonitorService Update the error handler for the retrofit client, DeploymentMonitorService, to use SpinnakerRetrofitErrorHandler. Additionally, modify catch blocks with RetrofitError to use Spinnaker*Exception. Purpose : This commit is foundational work for upgrading the Retrofit version to retrofit2.x Behavioural change : The modification includes updating catch blocks, leading to a change in the format of logger messages. This aligns with the upcoming modifications discussed in the PR: https://github.com/spinnaker/orca/pull/4608 Additionally, when the error response body is null, an appropriate error message will be printed in the logger. --- .../MonitoredDeployBaseTask.java | 41 +++++----- .../EvaluateDeploymentHealthTaskSpec.groovy | 11 +-- .../MonitoredDeployBaseTaskTest.java | 80 ++++++++++++++----- .../orca-deploymentmonitor.gradle | 1 + .../DeploymentMonitorServiceProvider.java | 2 + 5 files changed, 88 insertions(+), 47 deletions(-) diff --git a/orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTask.java b/orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTask.java index 373012c87a..b007f61604 100644 --- a/orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTask.java +++ b/orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTask.java @@ -16,10 +16,12 @@ package com.netflix.spinnaker.orca.clouddriver.tasks.monitoreddeploy; -import com.google.common.io.CharStreams; +import com.fasterxml.jackson.databind.ObjectMapper; import com.netflix.spectator.api.Registry; import com.netflix.spinnaker.config.DeploymentMonitorDefinition; import com.netflix.spinnaker.kork.annotations.VisibleForTesting; +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException; +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException; import com.netflix.spinnaker.orca.api.pipeline.RetryableTask; import com.netflix.spinnaker.orca.api.pipeline.TaskResult; import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus; @@ -31,19 +33,13 @@ import com.netflix.spinnaker.orca.deploymentmonitor.models.EvaluateHealthResponse; import com.netflix.spinnaker.orca.deploymentmonitor.models.MonitoredDeployInternalStageData; import com.netflix.spinnaker.orca.deploymentmonitor.models.StatusExplanation; -import java.io.InputStreamReader; -import java.nio.charset.StandardCharsets; import java.time.Duration; import java.util.*; import java.util.concurrent.TimeUnit; -import java.util.stream.Collectors; import javax.annotation.Nonnull; import javax.annotation.Nullable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import retrofit.RetrofitError; -import retrofit.client.Header; -import retrofit.client.Response; public class MonitoredDeployBaseTask implements RetryableTask { static final int MAX_RETRY_COUNT = 3; @@ -52,6 +48,7 @@ public class MonitoredDeployBaseTask implements RetryableTask { private DeploymentMonitorServiceProvider deploymentMonitorServiceProvider; private final Map summaryMapping = new HashMap<>(); + ObjectMapper objectMapper = new ObjectMapper(); MonitoredDeployBaseTask( DeploymentMonitorServiceProvider deploymentMonitorServiceProvider, Registry registry) { @@ -138,12 +135,12 @@ public long getDynamicTimeout(StageExecution stage) { try { return executeInternal(stage, monitorDefinition); - } catch (RetrofitError e) { + } catch (SpinnakerServerException e) { log.warn( "HTTP Error encountered while talking to {}->{}, {}}", monitorDefinition, e.getUrl(), - getRetrofitLogMessage(e.getResponse()), + getErrorMessage(e), e); return handleError(stage, e, true, monitorDefinition); @@ -269,28 +266,26 @@ private MonitoredDeployStageData getStageContext(StageExecution stage) { } @VisibleForTesting - String getRetrofitLogMessage(Response response) { - if (response == null) { + String getErrorMessage(SpinnakerServerException spinnakerException) { + if (!(spinnakerException instanceof SpinnakerHttpException)) { return ""; } - String body = ""; - String status = ""; - String headers = ""; + SpinnakerHttpException httpException = (SpinnakerHttpException) spinnakerException; try { - status = String.format("%d (%s)", response.getStatus(), response.getReason()); - body = - CharStreams.toString( - new InputStreamReader(response.getBody().in(), StandardCharsets.UTF_8)); - headers = - response.getHeaders().stream().map(Header::toString).collect(Collectors.joining("\n")); + String body = ""; + if (httpException.getResponseBody() != null) { + body = objectMapper.writeValueAsString(httpException.getResponseBody()); + } else { + body = "Failed to serialize the error response body"; + } + return String.format("headers: %s\nresponse body: %s", httpException.getHeaders(), body); } catch (Exception e) { - log.error( - "Failed to fully parse retrofit error while reading response from deployment monitor", e); + log.error("Failed to fully serialize http error response details", e); } - return String.format("status: %s\nheaders: %s\nresponse body: %s", status, headers, body); + return "headers: \nresponse body: "; } private boolean shouldFailOnError(StageExecution stage, DeploymentMonitorDefinition definition) { diff --git a/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/EvaluateDeploymentHealthTaskSpec.groovy b/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/EvaluateDeploymentHealthTaskSpec.groovy index a90fcc855c..8fbe19fb72 100644 --- a/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/EvaluateDeploymentHealthTaskSpec.groovy +++ b/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/EvaluateDeploymentHealthTaskSpec.groovy @@ -19,6 +19,8 @@ package com.netflix.spinnaker.orca.clouddriver.tasks.monitoreddeploy import com.fasterxml.jackson.databind.ObjectMapper import com.netflix.spectator.api.NoopRegistry import com.netflix.spinnaker.config.DeploymentMonitorDefinition +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus import com.netflix.spinnaker.orca.api.pipeline.TaskResult import com.netflix.spinnaker.orca.clouddriver.MortServiceSpec @@ -46,11 +48,11 @@ class EvaluateDeploymentHealthTaskSpec extends Specification { PipelineExecutionImpl pipe = pipeline { } - def "should retry retrofit errors"() { + def "should retry on SpinnakerServerException"() { given: def monitorServiceStub = Stub(DeploymentMonitorService) { evaluateHealth(_) >> { - throw RetrofitError.networkError("url", new IOException()) + throw new SpinnakerServerException(RetrofitError.networkError("url", new IOException())) } } @@ -203,8 +205,7 @@ class EvaluateDeploymentHealthTaskSpec extends Specification { false | null || ExecutionStatus.FAILED_CONTINUE } - def "should return status as RUNNING when Retrofit http error is thrown"() { - + def "should return status as RUNNING when SpinnakerHttpException is thrown"() { def converter = new JacksonConverter(new ObjectMapper()) Response response = @@ -224,7 +225,7 @@ class EvaluateDeploymentHealthTaskSpec extends Specification { given: def monitorServiceStub = Stub(DeploymentMonitorService) { evaluateHealth(_) >> { - throw RetrofitError.httpError("https://foo.com/deployment/evaluateHealth", response, converter, null) + throw new SpinnakerHttpException(RetrofitError.httpError("https://foo.com/deployment/evaluateHealth", response, converter, null)) } } diff --git a/orca-clouddriver/src/test/java/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTaskTest.java b/orca-clouddriver/src/test/java/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTaskTest.java index 784241c377..f99dffbda1 100644 --- a/orca-clouddriver/src/test/java/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTaskTest.java +++ b/orca-clouddriver/src/test/java/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTaskTest.java @@ -17,10 +17,15 @@ package com.netflix.spinnaker.orca.clouddriver.tasks.monitoreddeploy; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; import com.fasterxml.jackson.databind.ObjectMapper; import com.netflix.spectator.api.NoopRegistry; import com.netflix.spinnaker.config.DeploymentMonitorDefinition; +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.deploymentmonitor.DeploymentMonitorServiceProvider; import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; @@ -50,6 +55,8 @@ public class MonitoredDeployBaseTaskTest { private final ObjectMapper objectMapper = new ObjectMapper(); + private DeploymentMonitorServiceProvider deploymentMonitorServiceProvider; + @BeforeEach void setup() { OkClient okClient = new OkClient(); @@ -63,7 +70,7 @@ void setup() { var deploymentMonitorDefinitions = new ArrayList(); deploymentMonitorDefinitions.add(deploymentMonitorDefinition); - DeploymentMonitorServiceProvider deploymentMonitorServiceProvider = + deploymentMonitorServiceProvider = new DeploymentMonitorServiceProvider( okClient, retrofitLogLevel, requestInterceptor, deploymentMonitorDefinitions); monitoredDeployBaseTask = @@ -93,14 +100,14 @@ public void shouldParseHttpErrorResponseDetailsWhenHttpErrorHasOccurred() { RetrofitError.httpError( "https://foo.com/deployment/evaluateHealth", response, new JacksonConverter(), null); - String logMessageOnHttpError = - monitoredDeployBaseTask.getRetrofitLogMessage(httpError.getResponse()); - String status = HttpStatus.BAD_REQUEST.value() + " (" + HttpStatus.BAD_REQUEST.name() + ")"; + SpinnakerHttpException httpException = new SpinnakerHttpException(httpError); + + String logMessageOnSpinHttpException = monitoredDeployBaseTask.getErrorMessage(httpException); String body = "{\"error\":\"400 - Bad request, application name cannot be empty\"}"; - assertThat(logMessageOnHttpError) + assertThat(logMessageOnSpinHttpException) .isEqualTo( - String.format("status: %s\nheaders: %s\nresponse body: %s", status, header, body)); + String.format("headers: %s\nresponse body: %s", httpException.getHeaders(), body)); } @Test @@ -130,14 +137,13 @@ public void shouldParseHttpErrorResponseDetailsWhenConversionErrorHasOccurred() null, new ConversionException("Failed to parse response")); - String status = HttpStatus.BAD_REQUEST.value() + " (" + HttpStatus.BAD_REQUEST.name() + ")"; - String body = "{\"error\":\"400 - Bad request, application name cannot be empty\"}"; - String logMessageOnConversionError = - monitoredDeployBaseTask.getRetrofitLogMessage(conversionError.getResponse()); + SpinnakerConversionException conversionException = + new SpinnakerConversionException(conversionError); - assertThat(logMessageOnConversionError) - .isEqualTo( - String.format("status: %s\nheaders: %s\nresponse body: %s", status, header, body)); + String logMessageOnSpinConversionException = + monitoredDeployBaseTask.getErrorMessage(conversionException); + + assertThat(logMessageOnSpinConversionException).isEqualTo(""); } @Test @@ -148,10 +154,12 @@ void shouldReturnDefaultLogMsgWhenNetworkErrorHasOccurred() { "https://foo.com/deployment/evaluateHealth", new IOException("Failed to connect to the host : foo.com")); - String logMessageOnNetworkError = - monitoredDeployBaseTask.getRetrofitLogMessage(networkError.getResponse()); + SpinnakerNetworkException networkException = new SpinnakerNetworkException(networkError); + + String logMessageOnSpinNetworkException = + monitoredDeployBaseTask.getErrorMessage(networkException); - assertThat(logMessageOnNetworkError).isEqualTo(""); + assertThat(logMessageOnSpinNetworkException).isEqualTo(""); } @Test @@ -162,10 +170,44 @@ void shouldReturnDefaultLogMsgWhenUnexpectedErrorHasOccurred() { "https://foo.com/deployment/evaluateHealth", new IOException("Failed to connect to the host : foo.com")); - String logMessageOnUnexpectedError = - monitoredDeployBaseTask.getRetrofitLogMessage(unexpectedError.getResponse()); + SpinnakerServerException serverException = new SpinnakerServerException(unexpectedError); + + String logMessageOnSpinServerException = + monitoredDeployBaseTask.getErrorMessage(serverException); + + assertThat(logMessageOnSpinServerException).isEqualTo(""); + } + + @Test + void shouldReturnHeadersAndErrorMessageWhenErrorResponseBodyIsNull() { + + var converter = new JacksonConverter(objectMapper); + var headers = new ArrayList
(); + var header = new Header(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON_VALUE); + monitoredDeployBaseTask.objectMapper = mock(ObjectMapper.class); + + headers.add(header); - assertThat(logMessageOnUnexpectedError).isEqualTo(""); + Response response = + new Response( + "/deployment/evaluateHealth", + HttpStatus.BAD_REQUEST.value(), + HttpStatus.BAD_REQUEST.name(), + headers, + new MockTypedInput(converter, null)); + + RetrofitError httpError = + RetrofitError.httpError( + "https://foo.com/deployment/evaluateHealth", response, converter, null); + + SpinnakerHttpException httpException = new SpinnakerHttpException(httpError); + + String logMessageOnSpinHttpException = monitoredDeployBaseTask.getErrorMessage(httpException); + String body = "Failed to serialize the error response body"; + + assertThat(logMessageOnSpinHttpException) + .isEqualTo( + String.format("headers: %s\nresponse body: %s", httpException.getHeaders(), body)); } static class MockTypedInput implements TypedInput { diff --git a/orca-deploymentmonitor/orca-deploymentmonitor.gradle b/orca-deploymentmonitor/orca-deploymentmonitor.gradle index 14491fc63f..8c940777bb 100644 --- a/orca-deploymentmonitor/orca-deploymentmonitor.gradle +++ b/orca-deploymentmonitor/orca-deploymentmonitor.gradle @@ -19,6 +19,7 @@ apply from: "$rootDir/gradle/spock.gradle" dependencies { implementation(project(":orca-core")) implementation(project(":orca-retrofit")) + implementation("io.spinnaker.kork:kork-retrofit") implementation("org.springframework.boot:spring-boot-autoconfigure") diff --git a/orca-deploymentmonitor/src/main/java/com/netflix/spinnaker/orca/deploymentmonitor/DeploymentMonitorServiceProvider.java b/orca-deploymentmonitor/src/main/java/com/netflix/spinnaker/orca/deploymentmonitor/DeploymentMonitorServiceProvider.java index f3bcd02c22..f4a6a7750e 100644 --- a/orca-deploymentmonitor/src/main/java/com/netflix/spinnaker/orca/deploymentmonitor/DeploymentMonitorServiceProvider.java +++ b/orca-deploymentmonitor/src/main/java/com/netflix/spinnaker/orca/deploymentmonitor/DeploymentMonitorServiceProvider.java @@ -18,6 +18,7 @@ import com.netflix.spinnaker.config.DeploymentMonitorDefinition; import com.netflix.spinnaker.kork.exceptions.UserException; +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler; import com.netflix.spinnaker.orca.retrofit.logging.RetrofitSlf4jLog; import java.util.HashMap; import java.util.List; @@ -81,6 +82,7 @@ private synchronized DeploymentMonitorService getServiceByDefinition( .setLogLevel(retrofitLogLevel) .setLog(new RetrofitSlf4jLog(DeploymentMonitorService.class)) .setConverter(new JacksonConverter()) + .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance()) .build() .create(DeploymentMonitorService.class);