refactor(mine): use SpinnakerRetrofitErrorHandler with MineService (#…

…4679) * test(mine): verify http error case in RegisterCanaryTask These tests ensures the upcoming changes on MineService with SpinnakerRetrofitErrorHandler, will not modify/break the existing functionality. Test covers the existing behaviour of RegisterCanaryTask when some HTTP error has occurred. * fix(mine): handle exceptions when error response body is null in RegisterCanaryTask Before this change the RetrofitError catch block would throw a NullPointerException when error response body is null at line no: https://github.com/spinnaker/orca/blob/613427f41bb16461a7ee8bb0f31212b438d75458/orca-mine/src/main/groovy/com/netflix/spinnaker/orca/mine/tasks/RegisterCanaryTask.groovy#L70 Here is the exact stack trace : java.lang.NullPointerException: Cannot set property 'status' on null object at org.codehaus.groovy.runtime.NullObject.setProperty(NullObject.java:80) at org.codehaus.groovy.runtime.InvokerHelper.setProperty(InvokerHelper.java:213) at org.codehaus.groovy.runtime.ScriptBytecodeAdapter.setProperty(ScriptBytecodeAdapter.java:496) at com.netflix.spinnaker.orca.mine.tasks.RegisterCanaryTask.execute(RegisterCanaryTask.groovy:70) * test(mine): verify network error case in RegisterCanaryTask These tests ensures the upcoming changes on MineService with SpinnakerRetrofitErrorHandler, will not modify/break the fix made in the commit: 9d554a9. * 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 · 886e2b1 · 886e2b1
1 parent 613427f
commit 886e2b1
Show file tree

Hide file tree

Showing 8 changed files with 284 additions and 15 deletions.
diff --git a/orca-mine/orca-mine.gradle b/orca-mine/orca-mine.gradle
@@ -24,6 +24,10 @@ 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")
 }
 
 sourceSets {

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,20 +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 {
-        response = 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})")