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: #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: #4617
spinnaker · Jan 2, 2024 · b681cef · b681cef
1 parent d481b84
commit b681cef
Show file tree

Hide file tree

Showing 5 changed files with 100 additions and 47 deletions.
diff --git a/...com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTask.java b/...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<EvaluateHealthResponse.NextStepDirective, String> 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 "<NO RESPONSE>";
     }
 
-    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/.../spinnaker/orca/clouddriver/tasks/monitoreddeploy/EvaluateDeploymentHealthTaskSpec.groovy b/.../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/...netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTaskTest.java b/...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<DeploymentMonitorDefinition>();
     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("<NO RESPONSE>");
   }
 
   @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("<NO RESPONSE>");
+    String logMessageOnSpinNetworkException =
+        monitoredDeployBaseTask.getErrorMessage(networkException);
+
+    assertThat(logMessageOnSpinNetworkException).isEqualTo("<NO RESPONSE>");
   }
 
   @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("<NO RESPONSE>");
+  }
+
+  @Test
+  void shouldReturnHeadersAndErrorMessageWhenSerializingFailsWhileReadingBody()
+      throws JsonProcessingException {
+
+    var converter = new JacksonConverter(objectMapper);
+    var responseBody = new HashMap<String, String>();
+    var headers = new ArrayList<Header>();
+    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("<NO RESPONSE>");
+    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
@@ -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/...n/java/com/netflix/spinnaker/orca/deploymentmonitor/DeploymentMonitorServiceProvider.java b/...n/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);