refactor(front50): use SpinnakerRetrofitErrorHandler with Front50Service

This PR lays the foundational work for upgrading the retrofit version to 2.x, specifically focusing on refactoring the exception handling for Front50Service. There are no behaviour changes detected with these code changes and hence no additional tests added to demonstrate the functionalities. Few existing tests are modified to align it with the new changes.
spinnaker · Apr 5, 2024 · ae24d3f · ae24d3f
1 parent 2a2110d
commit ae24d3f
Show file tree

Hide file tree

Showing 21 changed files with 61 additions and 50 deletions.
diff --git a/...rc/main/groovy/com/netflix/spinnaker/orca/applications/tasks/DeleteApplicationTask.groovy b/...rc/main/groovy/com/netflix/spinnaker/orca/applications/tasks/DeleteApplicationTask.groovy
@@ -76,12 +76,15 @@ 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 e){
+          log.error("Could not delete application permission", e)
+          return TaskResult.builder(ExecutionStatus.TERMINAL).outputs(outputs).build()
         }
         // delete Managed Delivery data
         if (keelService != null) {

diff --git a/orca-bakery/src/main/groovy/com/netflix/spinnaker/orca/bakery/tasks/CreateBakeTask.groovy b/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)

diff --git a/...c/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/job/WaitOnJobCompletion.groovy b/...c/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
     }
   }

diff --git a/...main/java/com/netflix/spinnaker/orca/clouddriver/pollers/EphemeralServerGroupsPoller.java b/...main/java/com/netflix/spinnaker/orca/clouddriver/pollers/EphemeralServerGroupsPoller.java
@@ -26,6 +26,8 @@
 import com.netflix.spectator.api.Registry;
 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;
@@ -49,7 +51,6 @@
 import org.springframework.boot.autoconfigure.condition.ConditionalOnExpression;
 import org.springframework.http.HttpStatus;
 import org.springframework.stereotype.Component;
-import retrofit.RetrofitError;
 
 @Slf4j
 @Component
@@ -277,11 +278,13 @@ private Map<String, Object> buildDestroyServerGroupOperation(
   private 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);
     }
   }
 

diff --git a/...-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/tasks/job/JobUtils.java b/...-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;
     }
   }

diff --git a/...-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/utils/TrafficGuard.java b/...-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;

diff --git a/...iver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/utils/TrafficGuardSpec.groovy b/...iver/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))
     }
   }
 

diff --git a/orca-front50/orca-front50.gradle b/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")

diff --git a/...t50/src/main/groovy/com/netflix/spinnaker/orca/front50/config/Front50Configuration.groovy b/...t50/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)
   }

diff --git a/...ont50/src/main/groovy/com/netflix/spinnaker/orca/front50/tasks/AbstractFront50Task.groovy b/...ont50/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,11 +84,10 @@ 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
       }
-
       throw e
     }
   }

diff --git a/...0/src/main/java/com/netflix/spinnaker/orca/front50/pipeline/EnabledPipelineValidator.java b/...0/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_

diff --git a/...nt50/src/main/java/com/netflix/spinnaker/orca/front50/tasks/DeleteDeliveryConfigTask.java b/...nt50/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;

diff --git a/orca-front50/src/main/java/com/netflix/spinnaker/orca/front50/tasks/MonitorFront50Task.java b/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;

diff --git a/...nt50/src/main/java/com/netflix/spinnaker/orca/front50/tasks/UpsertDeliveryConfigTask.java b/...nt50/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;

diff --git a/...st/groovy/com/netflix/spinnaker/orca/front50/pipeline/EnabledPipelineValidatorSpec.groovy b/...st/groovy/com/netflix/spinnaker/orca/front50/pipeline/EnabledPipelineValidatorSpec.groovy
@@ -16,6 +16,7 @@
 
 package com.netflix.spinnaker.orca.front50.pipeline
 
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException
 import com.netflix.spinnaker.orca.front50.Front50Service
 import com.netflix.spinnaker.orca.pipeline.PipelineValidator.PipelineValidationFailed
 import com.netflix.spinnaker.orca.pipeline.model.DefaultTrigger
@@ -181,12 +182,12 @@ class EnabledPipelineValidatorSpec extends Specification {
   }
 
   def notFoundError() {
-    RetrofitError.httpError(
+    new SpinnakerHttpException(RetrofitError.httpError(
       "http://localhost",
       new Response("http://localhost", HTTP_NOT_FOUND, "Not Found", [], null),
       null,
       null
-    )
+    ))
   }
 
 }
diff --git a/orca-pipelinetemplate/orca-pipelinetemplate.gradle b/orca-pipelinetemplate/orca-pipelinetemplate.gradle
@@ -30,4 +30,5 @@ dependencies {
   testImplementation("org.spockframework:spock-unitils")
   testImplementation("org.codehaus.groovy:groovy-json")
   testImplementation("io.spinnaker.fiat:fiat-core:$fiatVersion")
+  testImplementation("io.spinnaker.kork:kork-retrofit")
 }
diff --git a/.../groovy/com/netflix/spinnaker/orca/pipelinetemplate/loader/Front50SchemeLoaderSpec.groovy b/.../groovy/com/netflix/spinnaker/orca/pipelinetemplate/loader/Front50SchemeLoaderSpec.groovy
@@ -16,6 +16,7 @@
 package com.netflix.spinnaker.orca.pipelinetemplate.loader
 
 import com.fasterxml.jackson.databind.ObjectMapper
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerNetworkException
 import com.netflix.spinnaker.orca.front50.Front50Service
 import com.netflix.spinnaker.orca.pipelinetemplate.exceptions.TemplateLoaderException
 import retrofit.RetrofitError
@@ -47,10 +48,10 @@ class Front50SchemeLoaderSpec extends Specification {
 
     then:
     front50Service.getPipelineTemplate("myTemplateId") >> {
-      throw RetrofitError.networkError("http://front50/no-exist", new IOException("resource not found"))
+      throw new SpinnakerNetworkException(RetrofitError.networkError("http://front50/no-exist", new IOException("resource not found")))
     }
     def e = thrown(TemplateLoaderException)
-    e.cause instanceof RetrofitError
+    e.cause instanceof SpinnakerNetworkException
   }
 
   void 'should load simple pipeline template'() {