feat(managed-delivery): Include detailed error info in ImportDelivery…

…Config (#3463) * feat(managed-delivery): Include detailed error info in ImportDeliveryConfig * fix(pr): Fix parsing of returned error from keel * fix(pr): Fix test * fix(pr): Avoid null checks for readability * fix(pr): Fix detection of igor and keel responses
spinnaker · Feb 28, 2020 · 617f962 · 617f962
1 parent 49ff3f3
commit 617f962
Show file tree

Hide file tree

Showing 4 changed files with 70 additions and 4 deletions.
diff --git a/orca-keel/orca-keel.gradle b/orca-keel/orca-keel.gradle
@@ -20,6 +20,7 @@ dependencies {
   implementation(project(":orca-core"))
   implementation(project(":orca-retrofit"))
   implementation(project(":orca-igor"))
+  implementation("com.fasterxml.jackson.core:jackson-databind")
   implementation("com.fasterxml.jackson.module:jackson-module-kotlin")
   implementation("org.springframework:spring-web")
   implementation("org.springframework.boot:spring-boot-autoconfigure")

diff --git a/orca-keel/src/main/kotlin/com/netflix/spinnaker/orca/KeelService.kt b/orca-keel/src/main/kotlin/com/netflix/spinnaker/orca/KeelService.kt
@@ -20,9 +20,11 @@ import com.netflix.spinnaker.orca.keel.model.DeliveryConfig
 import retrofit.client.Response
 import retrofit.http.Body
 import retrofit.http.Header
+import retrofit.http.Headers
 import retrofit.http.POST
 
 interface KeelService {
   @POST("/delivery-configs/")
+  @Headers("Accept: application/json")
   fun publishDeliveryConfig(@Body deliveryConfig: DeliveryConfig, @Header(value = "X-SPINNAKER-USER") user: String): Response
 }
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
@@ -18,6 +18,7 @@ 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.orca.ExecutionStatus
 import com.netflix.spinnaker.orca.KeelService
 import com.netflix.spinnaker.orca.RetryableTask
@@ -110,11 +111,14 @@ constructor(
           "Network error talking to downstream service, attempt ${context.attempt} of ${context.maxRetries}: ${e.friendlyMessage}")
       }
       e.response?.status in 400..499 -> {
+        val response = e.response!!
         // just give up on 4xx errors, which are unlikely to resolve with retries
         buildError(
           // ...but give users a hint about 401 errors from igor/scm
-          if (e.response?.status == 401 && URL(e.url).host.contains("igor")) {
+          if (response.status == 401 && e.fromIgor) {
             UNAUTHORIZED_SCM_ACCESS_MESSAGE
+          } else if (response.status == 400 && e.fromKeel && response.body.length() > 0) {
+            objectMapper.readValue<Map<String, Any?>>(response.body.`in`())
           } else {
             "Non-retryable HTTP response ${e.response?.status} received from downstream service: ${e.friendlyMessage}"
           }
@@ -145,9 +149,14 @@ constructor(
     }
   }
 
-  private fun buildError(errorMessage: String): TaskResult {
-    log.error(errorMessage)
-    return TaskResult.builder(ExecutionStatus.TERMINAL).context(mapOf("error" to errorMessage)).build()
+  private fun buildError(error: Any): TaskResult {
+    val normalizedError = if (error is Map<*, *>) {
+      error["error"] ?: error
+    } else {
+      error.toString()
+    }
+    log.error(normalizedError.toString())
+    return TaskResult.builder(ExecutionStatus.TERMINAL).context(mapOf("error" to normalizedError)).build()
   }
 
   override fun getBackoffPeriod() = TimeUnit.SECONDS.toMillis(30)
@@ -161,6 +170,18 @@ constructor(
       "$message: ${cause?.message ?: ""}"
     }
 
+  val RetrofitError.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
+    }
+
   data class ImportDeliveryConfigContext(
     var repoType: String? = null,
     var projectKey: String? = null,

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
@@ -36,9 +36,12 @@ import io.mockk.mockk
 import io.mockk.verify
 import retrofit.RetrofitError
 import retrofit.client.Response
+import retrofit.converter.JacksonConverter
+import retrofit.mime.TypedInput
 import strikt.api.expectThat
 import strikt.api.expectThrows
 import strikt.assertions.contains
+import strikt.assertions.isA
 import strikt.assertions.isEqualTo
 import strikt.assertions.isNotNull
 import java.lang.IllegalArgumentException
@@ -98,6 +101,20 @@ internal class ImportDeliveryConfigTaskTests : JUnit5Minutests {
           context
         )
       )
+
+    val parsingError = mapOf(
+      "message" to "Parsing error",
+      "details" to mapOf(
+        "message" to "Parsing error",
+        "path" to listOf(
+          mapOf(
+            "type" to "SomeClass",
+            "field" to "someField"
+          )
+        ),
+        "pathExpression" to ".someField"
+      )
+    )
   }
 
   private fun ManifestLocation.toMap() =
@@ -292,6 +309,31 @@ internal class ImportDeliveryConfigTaskTests : JUnit5Minutests {
         }
       }
 
+      context("delivery config parsing error") {
+        modifyFixture {
+          with(scmService) {
+            every {
+              getDeliveryConfigManifest(
+                manifestLocation.repoType,
+                manifestLocation.projectKey,
+                manifestLocation.repositorySlug,
+                manifestLocation.directory,
+                manifestLocation.manifest,
+                manifestLocation.ref
+              )
+            } throws RetrofitError.httpError("http://keel",
+              Response("http://keel", 400, "", emptyList(), JacksonConverter(ObjectMapper()).toBody(parsingError) as TypedInput),
+              null, 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<Map<String, Any>>().isEqualTo(parsingError)
+        }
+      }
+
       context("retryable failure to call downstream services") {
         modifyFixture {
           with(scmService) {