refactor(front50): Enhance Exception Handling in Front50Service Retro…

…fit Client

These changes lay the groundwork for a smooth and successful upgrade to retrofit2.x. The refactoring ensures that the retrofit client in 'Front50Service' is well-prepared for the upcoming version, and the accompanying test coverage guarantees the stability of the exception handling logic.

Key Changes:

1. Error Handler Addition:
    - Integrated the ‘SpinnakerRetrofitErrorHandler’ as the error handler in the front50 retrofit configuration. This sets the stage for a seamless transition to retrofit2.x, ensuring a standardised approach to error handling.

2. Catch Block Replacement:
    - Replaced the catch block that previously caught ‘RetrofitError’ with catching ‘Spinnaker*Exception’. This aligns the exception handling logic with the new retrofit version, enhancing compatibility and maintainability.

3. Test Cases Coverage:
    - Covered test cases for each of the changes made, ensuring comprehensive testing of the refactored exception handling logic. This step is crucial to validate the correctness and reliability of the updated error handling.

orca-applications/src/main/groovy/com/netflix/spinnaker/orca/applications/tasks/DeleteApplicationTask.groovy

@@ -18,6 +18,8 @@ package com.netflix.spinnaker.orca.applications.tasks
 
 import com.fasterxml.jackson.databind.ObjectMapper
 import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException
 import com.netflix.spinnaker.orca.api.pipeline.TaskResult
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
 import com.netflix.spinnaker.orca.front50.Front50Service
@@ -74,26 +76,43 @@ class DeleteApplicationTask extends AbstractFront50Task {
         front50Service.delete(application.name)
         try {
           front50Service.deletePermission(application.name)
-        } catch (RetrofitError re) {
-          if (re.response?.status == 404) {
+        } catch (SpinnakerHttpException re) {
+          if (re.responseCode == 404) {
             return TaskResult.SUCCEEDED
           }
           log.error("Could not delete application permission", re)
           return TaskResult.builder(ExecutionStatus.TERMINAL).outputs(outputs).build()
+        } catch (SpinnakerServerException serverException){
+          log.error("Could not delete application permission", serverException)
+          return TaskResult.builder(ExecutionStatus.TERMINAL).outputs(outputs).build()
         }
         // delete Managed Delivery data
         if (keelService != null) {
           log.debug("Deleting Managed Delivery data for {}", application.name)
           keelService.deleteDeliveryConfig(application.name)
         }
       }
-    } catch (RetrofitError e) {
+    }/**
+     * @see KeelService#deleteDeliveryConfig(java.lang.String) is invoked in the above try block.
+     * Since it throws RetrofitError, the below catch block with RetrofitError is still relevant.
+     * */
+    catch (RetrofitError e) {
       if (e.response?.status == 404) {
         return TaskResult.SUCCEEDED
       }
       log.error("Could not delete application", e)
       return TaskResult.builder(ExecutionStatus.TERMINAL).outputs(outputs).build()
     }
+    catch (SpinnakerHttpException e){
+      if (e.responseCode == 404) {
+        return TaskResult.SUCCEEDED
+      }
+      log.error("Could not delete application", e)
+      return TaskResult.builder(ExecutionStatus.TERMINAL).outputs(outputs).build()
+    } catch (SpinnakerServerException e){
+      log.error("Could not delete application", e)
+      return TaskResult.builder(ExecutionStatus.TERMINAL).outputs(outputs).build()
+    }
     return TaskResult.builder(ExecutionStatus.SUCCEEDED).outputs(outputs).build()
   }
 

orca-bakery/src/main/groovy/com/netflix/spinnaker/orca/bakery/tasks/CreateBakeTask.groovy

@@ -20,6 +20,7 @@ import com.fasterxml.jackson.databind.ObjectMapper
 import com.netflix.spinnaker.kork.artifacts.model.Artifact
 import com.netflix.spinnaker.kork.core.RetrySupport
 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.RetryableTask
 import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution
@@ -41,7 +42,6 @@ import org.slf4j.Logger
 import org.slf4j.LoggerFactory
 import org.springframework.beans.factory.annotation.Autowired
 import org.springframework.stereotype.Component
-import retrofit.RetrofitError
 
 
 import static com.netflix.spinnaker.kork.web.selector.v2.SelectableService.*
@@ -86,7 +86,7 @@ class CreateBakeTask implements RetryableTask {
           stage.context.user = user
         }
       }
-    } catch (RetrofitError e) {
+    } catch (SpinnakerServerException e) {
       // ignore exception, we will just use the owner passed to us
       if (!e.message.contains("404")) {
         log.warn("Error retrieving application {} from front50, ignoring.", stage.execution.application, e)

orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/job/WaitOnJobCompletion.groovy

@@ -21,6 +21,7 @@ import com.fasterxml.jackson.databind.ObjectMapper
 import com.netflix.frigga.Names
 import com.netflix.spinnaker.kork.core.RetrySupport
 import com.netflix.spinnaker.kork.exceptions.ConfigurationException
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
 import com.netflix.spinnaker.orca.api.pipeline.OverridableTimeoutRetryableTask
 import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution
@@ -34,7 +35,6 @@ import org.slf4j.Logger
 import org.slf4j.LoggerFactory
 
 import org.springframework.stereotype.Component
-import retrofit.RetrofitError
 
 import javax.annotation.Nonnull
 import javax.annotation.Nullable
@@ -200,7 +200,7 @@ public class WaitOnJobCompletion implements CloudProviderAware, OverridableTimeo
     }
     try {
       return front50Service.get(appName) != null
-    } catch (RetrofitError e) {
+    } catch (SpinnakerServerException e) {
       throw e
     }
   }

orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/pollers/EphemeralServerGroupsPoller.java

@@ -27,6 +27,8 @@
 import com.netflix.spinnaker.kork.annotations.VisibleForTesting;
 import com.netflix.spinnaker.kork.core.RetrySupport;
 import com.netflix.spinnaker.kork.exceptions.SystemException;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException;
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionType;
 import com.netflix.spinnaker.orca.clouddriver.CloudDriverService;
 import com.netflix.spinnaker.orca.clouddriver.config.PollerConfigurationProperties;
@@ -50,7 +52,6 @@
 import org.springframework.boot.autoconfigure.condition.ConditionalOnExpression;
 import org.springframework.http.HttpStatus;
 import org.springframework.stereotype.Component;
-import retrofit.RetrofitError;
 
 @Slf4j
 @Component
@@ -279,11 +280,13 @@ private Map<String, Object> buildDestroyServerGroupOperation(
   Optional<Application> getApplication(String applicationName) {
     try {
       return Optional.of(front50Service.get(applicationName));
-    } catch (RetrofitError e) {
-      if (e.getResponse().getStatus() == HttpStatus.NOT_FOUND.value()) {
+    } catch (SpinnakerHttpException e) {
+      if (e.getResponseCode() == HttpStatus.NOT_FOUND.value()) {
         return Optional.empty();
       }
       throw new SystemException(format("Failed to retrieve application '%s'", applicationName), e);
+    } catch (SpinnakerServerException e) {
+      throw new SystemException(format("Failed to retrieve application '%s'", applicationName), e);
     }
   }
 

orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/tasks/job/JobUtils.java

@@ -19,6 +19,7 @@
 
 import com.netflix.frigga.Names;
 import com.netflix.spinnaker.kork.core.RetrySupport;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException;
 import com.netflix.spinnaker.moniker.Moniker;
 import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution;
 import com.netflix.spinnaker.orca.clouddriver.KatoRestService;
@@ -30,7 +31,6 @@
 import java.util.Optional;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.stereotype.Component;
-import retrofit.RetrofitError;
 
 @Component
 public class JobUtils implements CloudProviderAware {
@@ -95,7 +95,7 @@ private Boolean applicationExists(String appName) {
     }
     try {
       return front50Service.get(appName) != null;
-    } catch (RetrofitError e) {
+    } catch (SpinnakerServerException e) {
       throw e;
     }
   }

orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/utils/TrafficGuard.java

@@ -23,6 +23,7 @@
 import com.netflix.spectator.api.Registry;
 import com.netflix.spectator.impl.Preconditions;
 import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException;
 import com.netflix.spinnaker.moniker.Moniker;
 import com.netflix.spinnaker.orca.clouddriver.CloudDriverService;
 import com.netflix.spinnaker.orca.clouddriver.model.Cluster;
@@ -39,7 +40,6 @@
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.stereotype.Component;
-import retrofit.RetrofitError;
 
 @Component
 public class TrafficGuard {
@@ -466,10 +466,9 @@ public boolean hasDisableLock(Moniker clusterMoniker, String account, Location l
     Application application;
     try {
       application = front50Service.get(clusterMoniker.getApp());
-    } catch (RetrofitError e) {
+    } catch (SpinnakerHttpException e) {
       // ignore an unknown (404) or unauthorized (403) application
-      if (e.getResponse() != null
-          && Arrays.asList(404, 403).contains(e.getResponse().getStatus())) {
+      if (Arrays.asList(404, 403).contains(e.getResponseCode())) {
         application = null;
       } else {
         throw e;

orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/utils/TrafficGuardSpec.groovy

@@ -20,6 +20,7 @@ import com.fasterxml.jackson.databind.ObjectMapper
 import com.netflix.spectator.api.NoopRegistry
 import com.netflix.spectator.api.Registry
 import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException
 import com.netflix.spinnaker.moniker.Moniker
 import com.netflix.spinnaker.orca.clouddriver.CloudDriverService
 import com.netflix.spinnaker.orca.clouddriver.ModelUtils
@@ -462,7 +463,7 @@ class TrafficGuardSpec extends Specification {
     !applicationDetails.containsKey("trafficGuards")
     result == false
     1 * front50Service.get("app") >> {
-      throw new RetrofitError(null, null, new Response("http://stash.com", 404, "test reason", [], null), null, null, null, null)
+      throw new SpinnakerHttpException(new RetrofitError(null, null, new Response("http://stash.com", 404, "test reason", [], null), null, null, null, null))
     }
   }
 

orca-front50/orca-front50.gradle

@@ -30,6 +30,7 @@ dependencies {
   implementation("org.springframework.security:spring-security-config")
   implementation("org.springframework.security:spring-security-core")
   implementation("org.springframework.security:spring-security-web")
+  implementation("io.spinnaker.kork:kork-retrofit")
 
   compileOnly("org.projectlombok:lombok")
   annotationProcessor("org.projectlombok:lombok")

orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/config/Front50Configuration.groovy

@@ -20,6 +20,7 @@ import com.fasterxml.jackson.databind.ObjectMapper
 import com.jakewharton.retrofit.Ok3Client
 import com.netflix.spinnaker.config.DefaultServiceEndpoint
 import com.netflix.spinnaker.config.okhttp3.OkHttpClientProvider
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler
 import com.netflix.spinnaker.orca.events.ExecutionEvent
 import com.netflix.spinnaker.orca.events.ExecutionListenerAdapter
 import com.netflix.spinnaker.orca.front50.Front50Service
@@ -78,6 +79,7 @@ class Front50Configuration {
       .setLogLevel(retrofitLogLevel)
       .setLog(new RetrofitSlf4jLog(Front50Service))
       .setConverter(new JacksonConverter(mapper))
+      .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance())
       .build()
       .create(Front50Service)
   }

orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/tasks/AbstractFront50Task.groovy

@@ -19,13 +19,13 @@ package com.netflix.spinnaker.orca.front50.tasks
 
 import com.fasterxml.jackson.databind.ObjectMapper
 import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException
 import com.netflix.spinnaker.orca.api.pipeline.RetryableTask
 import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution
 import com.netflix.spinnaker.orca.api.pipeline.TaskResult
 import com.netflix.spinnaker.orca.front50.Front50Service
 import com.netflix.spinnaker.orca.front50.model.Application
 import org.springframework.lang.Nullable
-import retrofit.RetrofitError
 
 import javax.annotation.Nonnull
 import java.util.concurrent.TimeUnit
@@ -84,8 +84,8 @@ abstract class AbstractFront50Task implements RetryableTask {
   Application fetchApplication(String applicationName) {
     try {
       return front50Service.get(applicationName)
-    } catch (RetrofitError e) {
-      if (e.response?.status == 404) {
+    } catch (SpinnakerHttpException e) {
+      if (e.responseCode == 404) {
         return null
       }
 

orca-front50/src/main/java/com/netflix/spinnaker/orca/front50/pipeline/EnabledPipelineValidator.java

@@ -18,6 +18,7 @@
 
 import static java.lang.String.format;
 
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException;
 import com.netflix.spinnaker.orca.api.pipeline.models.PipelineExecution;
 import com.netflix.spinnaker.orca.api.pipeline.models.Trigger;
 import com.netflix.spinnaker.orca.front50.Front50Service;
@@ -30,7 +31,6 @@
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.stereotype.Component;
-import retrofit.RetrofitError;
 
 @Component
 public class EnabledPipelineValidator implements PipelineValidator {
@@ -73,7 +73,7 @@ public void checkRunnable(PipelineExecution pipeline) {
         }
 
         return;
-      } catch (RetrofitError ignored) {
+      } catch (SpinnakerServerException ignored) {
         // treat a failure to fetch pipeline config history as non-fatal and fallback to the
         // previous behavior
         // (handles the fast property case where the supplied pipeline config id does _not_

orca-front50/src/main/java/com/netflix/spinnaker/orca/front50/tasks/DeleteDeliveryConfigTask.java

@@ -2,6 +2,7 @@
 
 import com.fasterxml.jackson.core.JsonProcessingException;
 import com.fasterxml.jackson.databind.ObjectMapper;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException;
 import com.netflix.spinnaker.orca.api.pipeline.Task;
 import com.netflix.spinnaker.orca.api.pipeline.TaskResult;
 import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution;
@@ -14,7 +15,6 @@
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.stereotype.Component;
-import retrofit.RetrofitError;
 
 @Component
 public class DeleteDeliveryConfigTask implements Task {
@@ -62,10 +62,9 @@ public Optional<DeliveryConfig> getDeliveryConfig(String id) {
     try {
       DeliveryConfig deliveryConfig = front50Service.getDeliveryConfig(id);
       return Optional.of(deliveryConfig);
-    } catch (RetrofitError e) {
+    } catch (SpinnakerHttpException e) {
       // ignore an unknown (404) or unauthorized (403, 401)
-      if (e.getResponse() != null
-          && Arrays.asList(404, 403, 401).contains(e.getResponse().getStatus())) {
+      if (Arrays.asList(404, 403, 401).contains(e.getResponseCode())) {
         return Optional.empty();
       } else {
         throw e;

orca-front50/src/main/java/com/netflix/spinnaker/orca/front50/tasks/MonitorFront50Task.java

@@ -20,6 +20,7 @@
 
 import com.fasterxml.jackson.annotation.JsonProperty;
 import com.fasterxml.jackson.databind.ObjectMapper;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException;
 import com.netflix.spinnaker.orca.api.pipeline.RetryableTask;
 import com.netflix.spinnaker.orca.api.pipeline.TaskResult;
 import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution;
@@ -36,7 +37,6 @@
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.beans.factory.annotation.Value;
 import org.springframework.stereotype.Component;
-import retrofit.RetrofitError;
 
 @Component
 public class MonitorFront50Task implements RetryableTask {
@@ -162,8 +162,8 @@ private TaskResult monitor(
   private Optional<Map<String, Object>> getPipeline(String id) {
     try {
       return Optional.of(front50Service.getPipeline(id));
-    } catch (RetrofitError e) {
-      if (e.getResponse() != null && e.getResponse().getStatus() == HTTP_NOT_FOUND) {
+    } catch (SpinnakerHttpException e) {
+      if (e.getResponseCode() == HTTP_NOT_FOUND) {
         return Optional.empty();
       }
       throw e;
@@ -175,10 +175,9 @@ private Optional<Map<String, Object>> getDeliveryConfig(String id) {
     try {
       DeliveryConfig deliveryConfig = front50Service.getDeliveryConfig(id);
       return Optional.of(objectMapper.convertValue(deliveryConfig, Map.class));
-    } catch (RetrofitError e) {
+    } catch (SpinnakerHttpException e) {
       // ignore an unknown (404) or unauthorized (403, 401)
-      if (e.getResponse() != null
-          && Arrays.asList(404, 403, 401).contains(e.getResponse().getStatus())) {
+      if (Arrays.asList(404, 403, 401).contains(e.getResponseCode())) {
         return Optional.empty();
       } else {
         throw e;

orca-front50/src/main/java/com/netflix/spinnaker/orca/front50/tasks/UpsertDeliveryConfigTask.java

@@ -2,6 +2,7 @@
 
 import com.fasterxml.jackson.core.type.TypeReference;
 import com.fasterxml.jackson.databind.ObjectMapper;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException;
 import com.netflix.spinnaker.orca.api.pipeline.Task;
 import com.netflix.spinnaker.orca.api.pipeline.TaskResult;
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus;
@@ -16,7 +17,6 @@
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.stereotype.Component;
-import retrofit.RetrofitError;
 
 @Component
 public class UpsertDeliveryConfigTask implements Task {
@@ -65,9 +65,8 @@ private boolean configExists(String id) {
     try {
       front50Service.getDeliveryConfig(id);
       return true;
-    } catch (RetrofitError e) {
-      if (e.getResponse() != null
-          && Arrays.asList(404, 403, 401).contains(e.getResponse().getStatus())) {
+    } catch (SpinnakerHttpException e) {
+      if (Arrays.asList(404, 403, 401).contains(e.getResponseCode())) {
         return false;
       } else {
         throw e;