refactor(front50): use SpinnakerRetrofitErrorHandler with Front50Serv…

…ice (#4623)

* 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.

* refactor(applications): Cleanup RetrofitError in DeleteApplicationTask

There are 3 APIs invoked inside the try block:
1. Keel Service API - keelService.deleteDeliveryConfig(): With the addition of the commit - c417922, the keelService uses SpinnakerRetrofitErrorHandler.
2. Front50 Service APIs - front50Service.get() and front50Service.delete(): With the addition of the commit - a44b5bb, the front50 service uses SpinnakerRetrofitErrorHandler.

and hence with the above mentioned commits, the catch block with RetrofitError becomes irrelevant.

* refactor(igor): Cleanup RetrofitError in GetCommitsTask

There are total of 5 API calls happening inside the try block:
1. Igor Service API - scmService.compareCommits(): With the addition of the commit - d430b75, the igorService uses SpinnakerRetrofitErrorHandler.
2. Clouddriver APIs (OortService) - cloudDriverService.getByAmiId() - This is invoked twice in the block, and oortService.getServerGroupFromCluster(): With the addition of the commit - 84a7106, the oortService uses SpinnakerRetrofitErrorHandler.
3. Front50 API - front50Service.get(): With the addition of the commit - a44b5bb, the front50Service uses SpinnakerRetrofitErrorHandler.

and hence with the above mentioned commits, the catch block with RetrofitError becomes irrelevant.

* refactor(test/clouddriver): Cleanup impractical tests in TrafficGuardSpec

The API call 'front50Service.get(application)' mocked/stubbed to return null value is not realistic, because if the application is not available , then this API would throw a 404 HTTP error and thats already handled in the same class: https://github.com/spinnaker/orca/blob/7e923cc05af0f66b952245722aa69c667f685c5c/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/utils/TrafficGuardSpec.groovy#L464.

and hence this test is cleaned up.

* refactor(test/applications): Cleanup impractical tests in UpsertApplicationTaskSpec

The API call 'front50Service.get(application)' mocked/stubbed to return null value is not realistic, because if the application is not present, then this API would throw a 404 HTTP error.

and hence this test is cleaned up.

* refactor(test): adjust UpsertApplicationTaskSpec tests to use realistic behavior of Front50Service.get

when an application isn't found.  It doesn't return null in this case, it throws an exception.

Restore previously removed "should create an application in global registries" test since
it was providing useful coverage.

---------

Co-authored-by: Pranav-b-7 <[email protected]>
Co-authored-by: David Byron <[email protected]>

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

@@ -29,7 +29,6 @@ import com.netflix.spinnaker.orca.KeelService
 import groovy.util.logging.Slf4j
 import org.springframework.lang.Nullable
 import org.springframework.stereotype.Component
-import retrofit.RetrofitError
 
 @Slf4j
 @Component
@@ -76,25 +75,22 @@ 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) {
           log.debug("Deleting Managed Delivery data for {}", application.name)
           keelService.deleteDeliveryConfig(application.name)
         }
       }
-    } 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 httpException){
       if (httpException.responseCode == 404) {
         return TaskResult.SUCCEEDED

orca-applications/src/test/groovy/com/netflix/spinnaker/orca/applications/tasks/UpsertApplicationTaskSpec.groovy

@@ -19,9 +19,12 @@ 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.orca.api.pipeline.models.ExecutionStatus
 import com.netflix.spinnaker.orca.front50.Front50Service
 import com.netflix.spinnaker.orca.front50.model.Application
+import retrofit.RetrofitError
+import retrofit.client.Response
 import spock.lang.Specification
 import spock.lang.Subject
 import spock.lang.Unroll
@@ -57,7 +60,7 @@ class UpsertApplicationTaskSpec extends Specification {
     given:
     def app = new Application(config.application + [user: config.user])
     task.front50Service = Mock(Front50Service) {
-      1 * get(config.application.name) >> null
+      1 * get(config.application.name) >> { throw notFoundError() }
       1 * create(app)
       1 * updatePermission(*_)
       0 * _._
@@ -99,7 +102,11 @@ class UpsertApplicationTaskSpec extends Specification {
     application.user = config.user
 
     task.front50Service = Mock(Front50Service) {
-      1 * get(config.application.name) >> initialState
+      if (initialState == null) {
+        1 * get(config.application.name) >> { throw notFoundError() }
+      } else {
+        1 * get(config.application.name) >> initialState
+      }
       1 * "${operation}"(*_)
       _ * updatePermission(*_)
       0 * _._
@@ -150,4 +157,13 @@ class UpsertApplicationTaskSpec extends Specification {
     [:]                                                         || 1
     [READ: ["[email protected]"], WRITE: ["[email protected]"]] || 1
   }
+
+  private static SpinnakerHttpException notFoundError() {
+    return new SpinnakerHttpException(RetrofitError.httpError(
+      "http://front50",
+      new Response("http://front50", 404, "Not Found", [], null),
+      null,
+      null
+    ))
+  }
 }

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

@@ -34,7 +34,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
@@ -198,11 +197,7 @@ public class WaitOnJobCompletion implements CloudProviderAware, OverridableTimeo
     if (appName == null || front50Service == null) {
       return false
     }
-    try {
-      return front50Service.get(appName) != null
-    } catch (RetrofitError e) {
-      throw e
-    }
+    return front50Service.get(appName) != null
   }
 
   /**

orca-clouddriver/src/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);
     }
   }
 

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

@@ -30,7 +30,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 {
@@ -93,10 +92,6 @@ private Boolean applicationExists(String appName) {
     if (appName == null || front50Service == null) {
       return false;
     }
-    try {
-      return front50Service.get(appName) != null;
-    } catch (RetrofitError e) {
-      throw e;
-    }
+    return front50Service.get(appName) != null;
   }
 }

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
@@ -445,15 +446,6 @@ class TrafficGuardSpec extends Specification {
     "app"   | "test"  | null       | "zz"        | "test"       | "us-east-1"   || false // different detail
   }
 
-  void "hasDisableLock returns false on missing applications"() {
-    when:
-    boolean result = trafficGuard.hasDisableLock(new Moniker(app: "app", cluster: "app"), "test", location)
-
-    then:
-    result == false
-    1 * front50Service.get("app") >> null
-  }
-
   void "hasDisableLock returns false on applications with no guards configured"() {
     when:
     boolean result = trafficGuard.hasDisableLock(new Moniker(app: "app", cluster: "app"), "test", location)
@@ -462,7 +454,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,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
     }
   }

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;