refactor(mine): use SpinnakerRetrofitErrorHandler with MineService

This PR lays the foundational work for upgrading the retrofit version to 2.x, specifically focusing on refactoring the exception handling for MineService There is no behaviour changes involved. Couple of tests added to verify the same for only RegisterCanaryTask in the previous commits.
spinnaker · Mar 28, 2024 · 64fdd7c · 64fdd7c
1 parent df35151
commit 64fdd7c
Show file tree

Hide file tree

Showing 8 changed files with 35 additions and 15 deletions.
diff --git a/orca-mine/orca-mine.gradle b/orca-mine/orca-mine.gradle
@@ -24,6 +24,7 @@ dependencies {
   implementation("org.springframework:spring-context")
   implementation("org.springframework.boot:spring-boot-autoconfigure")
   implementation("com.netflix.frigga:frigga")
+  implementation("io.spinnaker.kork:kork-retrofit")
 
   testImplementation("org.junit.jupiter:junit-jupiter-params")
   testImplementation("com.github.tomakehurst:wiremock-jre8-standalone")

diff --git a/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/config/MineConfiguration.groovy b/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/config/MineConfiguration.groovy
@@ -17,6 +17,7 @@
 package com.netflix.spinnaker.orca.mine.config
 
 import com.fasterxml.jackson.databind.ObjectMapper
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler
 import com.netflix.spinnaker.orca.mine.MineService
 import com.netflix.spinnaker.orca.retrofit.RetrofitConfiguration
 import com.netflix.spinnaker.orca.retrofit.logging.RetrofitSlf4jLog
@@ -64,6 +65,7 @@ class MineConfiguration {
       .setClient(retrofitClient)
       .setLogLevel(retrofitLogLevel)
       .setLog(new RetrofitSlf4jLog(MineService))
+      .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance())
       .setConverter(new JacksonConverter(objectMapper))
       .build()
       .create(MineService)

diff --git a/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/pipeline/AcaTaskStage.groovy b/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/pipeline/AcaTaskStage.groovy
@@ -16,6 +16,7 @@
 
 package com.netflix.spinnaker.orca.mine.pipeline
 
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException
 import com.netflix.spinnaker.orca.api.pipeline.CancellableStage
 import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution
 import com.netflix.spinnaker.orca.mine.MineService
@@ -27,12 +28,10 @@ import com.netflix.spinnaker.orca.api.pipeline.graph.TaskNode
 import groovy.util.logging.Slf4j
 import org.springframework.beans.factory.annotation.Autowired
 import org.springframework.stereotype.Component
-import retrofit.RetrofitError
 
 import javax.annotation.Nonnull
 
 import static org.springframework.http.HttpStatus.CONFLICT
-import static retrofit.RetrofitError.Kind.HTTP
 
 @Slf4j
 @Component
@@ -79,8 +78,8 @@ class AcaTaskStage implements StageDefinitionBuilder, CancellableStage {
         def cancelCanaryResults = mineService.cancelCanary(stage.context.canary.id as String, reason)
         log.info("Canceled canary in mine (canaryId: ${stage.context.canary.id}, stageId: ${stage.id}, executionId: ${stage.execution.id}): ${reason}")
         return cancelCanaryResults
-      } catch (RetrofitError e) {
-        if (e.kind == HTTP && e.response.status == CONFLICT.value()) {
+      } catch (SpinnakerHttpException e) {
+        if (e.responseCode == CONFLICT.value()) {
           log.info("Canary (canaryId: ${stage.context.canary.id}, stageId: ${stage.id}, executionId: ${stage.execution.id}) has already ended")
           return [:]
         } else {

diff --git a/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/tasks/DisableCanaryTask.groovy b/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/tasks/DisableCanaryTask.groovy
@@ -16,6 +16,7 @@
 
 package com.netflix.spinnaker.orca.mine.tasks
 
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
 import com.netflix.spinnaker.orca.api.pipeline.Task
 import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution
@@ -27,7 +28,6 @@ import com.netflix.spinnaker.orca.mine.MineService
 import groovy.util.logging.Slf4j
 import org.springframework.beans.factory.annotation.Autowired
 import org.springframework.stereotype.Component
-import retrofit.RetrofitError
 
 import javax.annotation.Nonnull
 
@@ -49,7 +49,7 @@ class DisableCanaryTask implements CloudProviderAware, Task {
           unhealthy : true
         ]).build()
       }
-    } catch (RetrofitError e) {
+    } catch (SpinnakerServerException e) {
       log.error("Exception occurred while getting canary status with id {} from mine, continuing with disable",
         stage.context.canary.id, e)
     }

diff --git a/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/tasks/MonitorAcaTaskTask.groovy b/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/tasks/MonitorAcaTaskTask.groovy
@@ -16,6 +16,7 @@
 
 package com.netflix.spinnaker.orca.mine.tasks
 
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException
 import com.netflix.spinnaker.security.AuthenticatedRequest
 import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution
 import java.util.concurrent.TimeUnit
@@ -28,7 +29,6 @@ import com.netflix.spinnaker.orca.mine.MineService
 import groovy.util.logging.Slf4j
 import org.springframework.beans.factory.annotation.Autowired
 import org.springframework.stereotype.Component
-import retrofit.RetrofitError
 
 @Component
 @Slf4j
@@ -51,7 +51,7 @@ class MonitorAcaTaskTask implements CloudProviderAware, OverridableTimeoutRetrya
       outputs << [
           canary: canary
       ]
-    } catch (RetrofitError e) {
+    } catch (SpinnakerServerException e) {
       log.error("Exception occurred while getting canary with id ${context.canary.id} from mine service", e)
       return TaskResult.builder(ExecutionStatus.RUNNING).context(outputs).build()
     }

diff --git a/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/tasks/MonitorCanaryTask.groovy b/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/tasks/MonitorCanaryTask.groovy
@@ -16,6 +16,7 @@
 
 package com.netflix.spinnaker.orca.mine.tasks
 
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException
 import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution
 
 import java.util.concurrent.TimeUnit
@@ -29,7 +30,6 @@ import com.netflix.spinnaker.orca.mine.MineService
 import groovy.util.logging.Slf4j
 import org.springframework.beans.factory.annotation.Autowired
 import org.springframework.stereotype.Component
-import retrofit.RetrofitError
 
 @Component
 @Slf4j
@@ -59,7 +59,7 @@ class MonitorCanaryTask implements CloudProviderAware, OverridableTimeoutRetryab
       outputs << [
         canary : mineService.getCanary(context.canary.id)
       ]
-    } catch (RetrofitError e) {
+    } catch (SpinnakerServerException e) {
       log.error("Exception occurred while getting canary with id ${context.canary.id} from mine service", e)
       return TaskResult.builder(ExecutionStatus.RUNNING).context(outputs).build()
     }

diff --git a/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/tasks/RegisterCanaryTask.groovy b/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/tasks/RegisterCanaryTask.groovy
@@ -16,6 +16,10 @@
 
 package com.netflix.spinnaker.orca.mine.tasks
 
+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.api.pipeline.models.ExecutionStatus
 import com.netflix.spinnaker.orca.api.pipeline.Task
 import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution
@@ -25,7 +29,6 @@ import com.netflix.spinnaker.orca.mine.pipeline.DeployCanaryStage
 import groovy.util.logging.Slf4j
 import org.springframework.beans.factory.annotation.Autowired
 import org.springframework.stereotype.Component
-import retrofit.RetrofitError
 import retrofit.client.Response
 
 import javax.annotation.Nonnull
@@ -59,21 +62,34 @@ class RegisterCanaryTask implements Task {
       if (response.status == 200 && response.body.mimeType().startsWith('text/plain')) {
         canaryId = response.body.in().text
       }
-    } catch (RetrofitError re) {
+    } catch (SpinnakerHttpException re) {
       def response = [:]
       try {
-        def responseBody = re.getBodyAs(Map) as Map
+        def responseBody = re.responseBody
         response = responseBody!=null ? responseBody : response
       } catch (Exception e) {
         response.error = e.message
       }
 
-      response.status = re.response?.status
-      response.errorKind = re.kind
+      response.status = re.responseCode
+      response.errorKind = "HTTP"
 
       throw new IllegalStateException(
         "Unable to register canary (executionId: ${stage.execution.id}, stageId: ${stage.id} canary: ${c}), response: ${response}"
       )
+    } catch(SpinnakerServerException e){
+      def response = [:]
+      response.status = null
+      if (e instanceof SpinnakerNetworkException){
+        response.errorKind = "NETWORK"
+      } else if(e instanceof SpinnakerConversionException) {
+        response.errorKind = "CONVERSION"
+      } else {
+        response.errorKind = "UNEXPECTED"
+      }
+      throw new IllegalStateException(
+          "Unable to register canary (executionId: ${stage.execution.id}, stageId: ${stage.id} canary: ${c}), response: ${response}"
+      )
     }
 
     log.info("Registered Canary (executionId: ${stage.execution.id}, stageId: ${stage.id}, canaryId: ${canaryId})")

diff --git a/orca-mine/src/test/java/com/netflix/spinnaker/orca/mine/tasks/RegisterCanaryTaskTest.java b/orca-mine/src/test/java/com/netflix/spinnaker/orca/mine/tasks/RegisterCanaryTaskTest.java
@@ -27,6 +27,7 @@
 import com.github.tomakehurst.wiremock.http.HttpHeaders;
 import com.github.tomakehurst.wiremock.junit5.WireMockExtension;
 import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler;
 import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution;
 import com.netflix.spinnaker.orca.mine.MineService;
 import com.netflix.spinnaker.orca.mine.pipeline.DeployCanaryStage;
@@ -75,6 +76,7 @@ static void setupOnce(WireMockRuntimeInfo wmRuntimeInfo) {
             .setEndpoint(wmRuntimeInfo.getHttpBaseUrl())
             .setClient(okClient)
             .setLogLevel(retrofitLogLevel)
+            .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance())
             .setLog(new RetrofitSlf4jLog(MineService.class))
             .setConverter(new JacksonConverter(objectMapper))
             .build()