From 133f1c5921d20f75f636349f5c8751fb53d83bd8 Mon Sep 17 00:00:00 2001 From: Pranav-b-7 Date: Wed, 22 Nov 2023 13:45:03 +0530 Subject: [PATCH] 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, a new specific exception handler is introduced to manage JsonProcessing during the serialization of the HTTP error body. The impact of these changes should be compared with another related PR: https://github.com/spinnaker/orca/pull/4617 --- .../MonitoredDeployBaseTask.java | 44 +++++---- .../EvaluateDeploymentHealthTaskSpec.groovy | 11 +-- .../MonitoredDeployBaseTaskTest.java | 89 +++++++++++++++---- .../orca-deploymentmonitor.gradle | 1 + .../DeploymentMonitorServiceProvider.java | 2 + 5 files changed, 100 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 373012c87ae..d6ee6a319eb 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,13 @@ package com.netflix.spinnaker.orca.clouddriver.tasks.monitoreddeploy; -import com.google.common.io.CharStreams; +import com.fasterxml.jackson.core.JsonProcessingException; +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 +34,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 +49,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 +136,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 +267,28 @@ 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()); + } + return String.format("headers: %s\nresponse body: %s", httpException.getHeaders(), body); + } catch (JsonProcessingException e) { + log.error("Failed to serialize the error response body : ", e); + return String.format( + "headers: %s\nresponse body: %s", httpException.getHeaders(), e.getMessage()); } 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 a90fcc855ca..8fbe19fb728 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 784241c377d..87ec975a2a3 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,17 @@ package com.netflix.spinnaker.orca.clouddriver.tasks.monitoreddeploy; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +import com.fasterxml.jackson.core.JsonProcessingException; 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 +57,8 @@ public class MonitoredDeployBaseTaskTest { private final ObjectMapper objectMapper = new ObjectMapper(); + private DeploymentMonitorServiceProvider deploymentMonitorServiceProvider; + @BeforeEach void setup() { OkClient okClient = new OkClient(); @@ -63,7 +72,7 @@ void setup() { var deploymentMonitorDefinitions = new ArrayList(); deploymentMonitorDefinitions.add(deploymentMonitorDefinition); - DeploymentMonitorServiceProvider deploymentMonitorServiceProvider = + deploymentMonitorServiceProvider = new DeploymentMonitorServiceProvider( okClient, retrofitLogLevel, requestInterceptor, deploymentMonitorDefinitions); monitoredDeployBaseTask = @@ -93,14 +102,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 +139,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 +156,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); - assertThat(logMessageOnNetworkError).isEqualTo(""); + String logMessageOnSpinNetworkException = + monitoredDeployBaseTask.getErrorMessage(networkException); + + assertThat(logMessageOnSpinNetworkException).isEqualTo(""); } @Test @@ -162,10 +172,51 @@ 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 shouldReturnHeadersAndErrorMessageWhenSerializingFailsWhileReadingBody() + throws JsonProcessingException { + + var converter = new JacksonConverter(objectMapper); + var responseBody = new HashMap(); + var headers = new ArrayList
(); + var header = new Header(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON_VALUE); + monitoredDeployBaseTask.objectMapper = mock(ObjectMapper.class); + + headers.add(header); + responseBody.put("error", "400 - Bad request, application name cannot be empty"); - assertThat(logMessageOnUnexpectedError).isEqualTo(""); + Response response = + new Response( + "/deployment/evaluateHealth", + HttpStatus.BAD_REQUEST.value(), + HttpStatus.BAD_REQUEST.name(), + headers, + new MockTypedInput(converter, responseBody)); + + RetrofitError httpError = + RetrofitError.httpError( + "https://foo.com/deployment/evaluateHealth", response, converter, null); + + SpinnakerHttpException httpException = new SpinnakerHttpException(httpError); + JsonProcessingException jpe = new JsonProcessingException("Failed to parse the error body") {}; + + when(monitoredDeployBaseTask.objectMapper.writeValueAsString(httpException.getResponseBody())) + .thenThrow(jpe); + + String logMessageOnSpinHttpException = monitoredDeployBaseTask.getErrorMessage(httpException); + String body = jpe.getMessage(); + + 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 14491fc63f8..8c940777bb7 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 f3bcd02c225..f4a6a7750eb 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);