refactor(retrofit): use SpinnakerRetrofitErrorHandler as a step towards simplifying exception handling

orca-bakery/orca-bakery.gradle

@@ -26,6 +26,7 @@ dependencies {
   implementation("com.fasterxml.jackson.datatype:jackson-datatype-guava")
   implementation("io.spinnaker.fiat:fiat-core:$fiatVersion")
   implementation("org.codehaus.groovy:groovy-datetime")
+  implementation("io.spinnaker.kork:kork-retrofit")
 
   api("io.spinnaker.kork:kork-web")
 

orca-bakery/src/main/groovy/com/netflix/spinnaker/orca/bakery/config/BakeryConfiguration.groovy

@@ -16,6 +16,7 @@
 
 package com.netflix.spinnaker.orca.bakery.config
 
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler
 import com.netflix.spinnaker.orca.bakery.BakerySelector
 import org.springframework.boot.autoconfigure.condition.ConditionalOnExpression
 import org.springframework.boot.context.properties.EnableConfigurationProperties
@@ -80,6 +81,7 @@ class BakeryConfiguration {
       .setClient(retrofitClient)
       .setLogLevel(retrofitLogLevel)
       .setLog(new RetrofitSlf4jLog(BakeryService))
+      .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance())
       .build()
       .create(BakeryService)
   }

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

@@ -19,6 +19,7 @@ package com.netflix.spinnaker.orca.bakery.tasks
 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.orca.api.pipeline.models.ExecutionStatus
 import com.netflix.spinnaker.orca.api.pipeline.RetryableTask
 import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution
@@ -35,12 +36,14 @@ import com.netflix.spinnaker.orca.pipeline.util.PackageInfo
 import com.netflix.spinnaker.orca.pipeline.util.PackageType
 import groovy.transform.CompileDynamic
 import groovy.transform.CompileStatic
+import java.time.Duration
 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.*
 
 @Component
@@ -61,7 +64,7 @@ class CreateBakeTask implements RetryableTask {
   @Autowired(required = false)
   Front50Service front50Service
 
-  RetrySupport retrySupport = new RetrySupport()
+  private final RetrySupport retrySupport = new RetrySupport()
 
   private final Logger log = LoggerFactory.getLogger(getClass())
 
@@ -76,7 +79,8 @@ class CreateBakeTask implements RetryableTask {
     try {
       if (front50Service != null) {
         String appName = stage.execution.application
-        Application application = retrySupport.retry({return front50Service.get(appName)}, 5, 2000, false)
+        Application application =
+            retrySupport.retry({ return front50Service.get(appName) }, 5, Duration.ofMillis(2000), false)
         String user = application.email
         if (user != null && user != "") {
           stage.context.user = user
@@ -100,12 +104,16 @@ class CreateBakeTask implements RetryableTask {
         bake.amiSuffix = stage.context.amiSuffix
       }
 
-      def bakeStatus = bakery.service.createBake(stage.context.region as String, bake, rebake)
+      def bakeStatus = retrySupport.retry(
+          { return bakery.service.createBake(stage.context.region as String, bake, rebake) },
+          5,
+          Duration.ofMillis(2000),
+          false)
 
       def stageOutputs = [
-        status         : bakeStatus,
-        bakePackageName: bake.packageName ?: "",
-        previouslyBaked: bakeStatus.state == BakeStatus.State.COMPLETED
+          status         : bakeStatus,
+          bakePackageName: bake.packageName ?: "",
+          previouslyBaked: bakeStatus.state == BakeStatus.State.COMPLETED
       ] as Map<String, ? extends Object>
 
       if (bake.buildInfoUrl) {
@@ -114,9 +122,9 @@ class CreateBakeTask implements RetryableTask {
 
       if (bake.buildHost) {
         stageOutputs << [
-          buildHost  : bake.buildHost,
-          job        : bake.job,
-          buildNumber: bake.buildNumber
+            buildHost  : bake.buildHost,
+            job        : bake.job,
+            buildNumber: bake.buildNumber
         ]
 
         if (bake.commitHash) {
@@ -125,19 +133,13 @@ class CreateBakeTask implements RetryableTask {
       }
 
       TaskResult.builder(ExecutionStatus.SUCCEEDED).context(stageOutputs).build()
-    } catch (RetrofitError e) {
-      if (e.response?.status && e.response.status == 404) {
-        try {
-          def exceptionResult = mapper.readValue(e.response.body.in().text, Map)
-          def exceptionMessages = (exceptionResult.messages ?: []) as List<String>
-          if (exceptionMessages) {
-            throw new IllegalStateException(exceptionMessages[0])
-          }
-        } catch (IOException ignored) {
-          // do nothing
+    } catch (SpinnakerHttpException e) {
+      if (e.responseCode == 404) {
+        def exceptionMessages =
+            (e.responseBody?.messages ?: e.responseBody?.message ? [e.responseBody.message] : []) as List<String>
+        if (exceptionMessages) {
+          throw new IllegalStateException(exceptionMessages[0])
         }
-
-        return TaskResult.ofStatus(ExecutionStatus.RUNNING)
       }
       throw e
     }

orca-bakery/src/main/java/com/netflix/spinnaker/orca/bakery/tasks/manifests/CreateBakeManifestTask.java

@@ -19,6 +19,7 @@
 
 import com.netflix.spinnaker.kork.artifacts.model.Artifact;
 import com.netflix.spinnaker.kork.artifacts.model.ExpectedArtifact;
+import com.netflix.spinnaker.kork.core.RetrySupport;
 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;
@@ -30,6 +31,7 @@
 import com.netflix.spinnaker.orca.bakery.api.manifests.kustomize.KustomizeBakeManifestRequest;
 import com.netflix.spinnaker.orca.pipeline.util.ArtifactUtils;
 import com.netflix.spinnaker.orca.pipeline.util.ContextParameterProcessor;
+import java.time.Duration;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
@@ -62,6 +64,8 @@ public long getTimeout() {
 
   private final ContextParameterProcessor contextParameterProcessor;
 
+  private final RetrySupport retrySupport = new RetrySupport();
+
   @Autowired
   public CreateBakeManifestTask(
       ArtifactUtils artifactUtils,
@@ -146,7 +150,12 @@ public TaskResult execute(@Nonnull StageExecution stage) {
             "Invalid template renderer " + context.getTemplateRenderer());
     }
 
-    Artifact result = bakery.bakeManifest(request.getTemplateRenderer(), request);
+    Artifact result =
+        retrySupport.retry(
+            () -> bakery.bakeManifest(request.getTemplateRenderer(), request),
+            5,
+            Duration.ofMillis(2000),
+            false);
 
     Map<String, Object> outputs = new HashMap<>();
     outputs.put("artifacts", Collections.singleton(result));

orca-bakery/src/test/groovy/com/netflix/spinnaker/orca/bakery/api/BakeryServiceSpec.groovy

@@ -17,6 +17,7 @@
 package com.netflix.spinnaker.orca.bakery.api
 
 import com.github.tomakehurst.wiremock.WireMockServer
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException
 import com.netflix.spinnaker.orca.bakery.config.BakeryConfiguration
 import com.netflix.spinnaker.orca.jackson.OrcaObjectMapper
 import retrofit.RequestInterceptor
@@ -105,14 +106,15 @@ class BakeryServiceSpec extends Specification {
         .willReturn(
         aResponse()
           .withStatus(HTTP_NOT_FOUND)
+          .withBody("{\"message\": \"error\"}")
       )
     )
 
     when:
     bakery.lookupStatus(region, statusId)
 
     then:
-    def ex = thrown(RetrofitError)
+    def ex = thrown(SpinnakerHttpException)
     ex.response.status == HTTP_NOT_FOUND
   }
 

orca-bakery/src/test/groovy/com/netflix/spinnaker/orca/bakery/tasks/CreateBakeTaskSpec.groovy

@@ -17,6 +17,7 @@
 package com.netflix.spinnaker.orca.bakery.tasks
 
 import com.netflix.spinnaker.kork.artifacts.model.Artifact
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException
 import com.netflix.spinnaker.kork.web.selector.v2.SelectableService
 import com.netflix.spinnaker.orca.api.pipeline.models.Trigger
 import com.netflix.spinnaker.orca.bakery.BakerySelector
@@ -30,6 +31,7 @@ import com.netflix.spinnaker.orca.pipeline.util.ArtifactUtils
 import com.netflix.spinnaker.orca.pipeline.util.PackageType
 import retrofit.RetrofitError
 import retrofit.client.Response
+import retrofit.converter.JacksonConverter
 import retrofit.mime.TypedString
 import spock.lang.Shared
 import spock.lang.Specification
@@ -245,9 +247,10 @@ class CreateBakeTaskSpec extends Specification {
   def error404 = RetrofitError.httpError(
     null,
     new Response("", HTTP_NOT_FOUND, "Not Found", [], new TypedString("{ \"messages\": [\"Error Message\"]}")),
-    null,
+    new JacksonConverter(),
     null
   )
+  def httpError404 = new SpinnakerHttpException(error404)
 
   def setup() {
     task.mapper = mapper
@@ -332,8 +335,8 @@ class CreateBakeTaskSpec extends Specification {
     task.execute(bakeStage)
 
     then:
-    1 * bakery.createBake(bakeConfig.region, _ as BakeRequest, null) >> {
-      throw error404
+    5 * bakery.createBake(bakeConfig.region, _ as BakeRequest, null) >> {
+      throw httpError404
     }
     IllegalStateException e = thrown()
     e.message == "Error Message"