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={})

- On conversion error:

  before:

  11:38:32.041 [Test worker] ERROR com.netflix.spinnaker.orca.keel.task.ImportDeliveryConfigTask - SpringHttpError(error=Bad Request, status=400, message=Parsing error, timestamp=2024-02-02T06:08:32.032Z, details={message=Parsing error, path=[{type=SomeC  lass, field=someField}], pathExpression=.someField})

  after:

  13:32:47.520 [Test worker] ERROR com.netflix.spinnaker.orca.keel.task.ImportDeliveryConfigTask - Handling retryable failure 1 of 5: Server error talking to downstream service, attempt 1 of 5: Failed to parse the http error body

orca-applications/src/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()
   }

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")

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)

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
@@ -38,6 +41,7 @@ import java.util.concurrent.TimeUnit
 import org.slf4j.LoggerFactory
 import org.springframework.stereotype.Component
 import retrofit.RetrofitError
+import java.util.Collections
 
 /**
  * Task that retrieves a Managed Delivery config manifest from source control via igor, then publishes it to keel,
@@ -77,6 +81,12 @@ 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 (serverException: SpinnakerServerException){
+      handleRetryableFailures(serverException, context)
     } catch (e: Exception) {
       log.error("Unexpected exception while executing {}, aborting.", javaClass.simpleName, e)
       buildError(e.message ?: "Unknown error (${e.javaClass.simpleName})")
@@ -153,6 +163,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 +306,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,

orca-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();
 

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.SpinnakerConversionException
 import com.netflix.spinnaker.kork.web.exceptions.InvalidRequestException
 import com.netflix.spinnaker.orca.KeelService
 import com.netflix.spinnaker.orca.api.pipeline.TaskResult
@@ -426,7 +427,7 @@ internal class ImportDeliveryConfigTaskTests : JUnit5Minutests {
         }
       }
 
-      context("test Retrofit conversion error") {
+      context("test taskResults on SpinnakerConversionException") {
         modifyFixture {
           with(scmService) {
             every {
@@ -443,21 +444,21 @@ internal class ImportDeliveryConfigTaskTests : JUnit5Minutests {
           with(keelService){
             every {
               publishDeliveryConfig(manifest)
-            } throws RetrofitError.conversionError(
+            } throws SpinnakerConversionException(RetrofitError.conversionError(
               "http://keel",
             Response(
               "http://keel", 400, "", emptyList(),
               JacksonConverter(objectMapper).toBody(parsingError) as TypedInput
             ),
             JacksonConverter(objectMapper), null, ConversionException("Failed to parse the http error body")
-            )
+            ))
           }
         }
 
-        test("task throws retrofit conversion error and includes the error details returned by keel") {
+        test("task throws SpinnakerConversionException 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.status).isEqualTo(ExecutionStatus.RUNNING)
+          expectThat(result.context["errorFromLastAttempt"]).isEqualTo("Server error talking to downstream service, attempt 1 of ${result.context["maxRetries"]}: Failed to parse the http error body")
         }
       }