fix(managed-delivery): Better error messages for ImportDeliveryConfig (…

…spinnaker#3453)

* fix(managed-delivery): Better error messages for ImportDeliveryConfig

* fix(pr): Fix unauthorized message per review feedback

orca-keel/src/main/kotlin/com/netflix/spinnaker/orca/keel/task/ImportDeliveryConfigTask.kt

@@ -27,9 +27,9 @@ import com.netflix.spinnaker.orca.pipeline.model.SourceCodeTrigger
 import com.netflix.spinnaker.orca.pipeline.model.Stage
 import com.netflix.spinnaker.orca.pipeline.model.Trigger
 import org.slf4j.LoggerFactory
-import org.springframework.http.HttpStatus
 import org.springframework.stereotype.Component
 import retrofit.RetrofitError
+import java.net.URL
 import java.util.concurrent.TimeUnit
 
 /**
@@ -64,7 +64,7 @@ constructor(
       handleRetryableFailures(e, context)
     } catch (e: Exception) {
       log.error("Unexpected exception while executing {}, aborting.", javaClass.simpleName, e)
-      buildError(e.message)
+      buildError(e.message ?: "Unknown error (${e.javaClass.simpleName})")
     }
   }
 
@@ -106,36 +106,49 @@ constructor(
     return when {
       e.kind == RetrofitError.Kind.NETWORK -> {
         // retry if unable to connect
-        log.error("network error talking to downstream service, attempt ${context.attempt} of ${context.maxRetries}: ${e.friendlyMessage}")
-        buildRetry(context)
+        buildRetry(context,
+          "Network error talking to downstream service, attempt ${context.attempt} of ${context.maxRetries}: ${e.friendlyMessage}")
       }
-      e.response?.status == HttpStatus.NOT_FOUND.value() -> {
-        // just give up on 404
-        val errorDetails = "404 response from downstream service, giving up: ${e.friendlyMessage}"
-        log.error(errorDetails)
-        buildError(errorDetails)
+      e.response?.status in 400..499 -> {
+        // 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")) {
+            UNAUTHORIZED_SCM_ACCESS_MESSAGE
+          } else {
+            "Non-retryable HTTP response ${e.response?.status} received from downstream service: ${e.friendlyMessage}"
+          }
+        )
       }
       else -> {
         // retry on other status codes
-        log.error("HTTP error talking to downstream service, attempt ${context.attempt} of ${context.maxRetries}: ${e.friendlyMessage}")
-        buildRetry(context)
+        buildRetry(context,
+          "Retryable HTTP response ${e.response?.status} received from downstream service: ${e.friendlyMessage}")
       }
     }
   }
 
-  private fun buildRetry(context: ImportDeliveryConfigContext): TaskResult {
+  private fun buildRetry(context: ImportDeliveryConfigContext, errorMessage: String): TaskResult {
+    log.error("Handling retry failure ${context.attempt} of ${context.maxRetries}: $errorMessage")
+    context.errorFromLastAttempt = errorMessage
     context.incrementAttempt()
+
     return if (context.attempt > context.maxRetries!!) {
-      val error = "Maximum number of retries exceeded (${context.maxRetries})"
+      val error = "Maximum number of retries exceeded (${context.maxRetries}). " +
+        "The error from the last attempt was: $errorMessage"
       log.error("$error. Aborting.")
-      TaskResult.builder(ExecutionStatus.TERMINAL).context(mapOf("error" to error)).build()
+      TaskResult.builder(ExecutionStatus.TERMINAL).context(
+        mapOf("error" to error, "errorFromLastAttempt" to errorMessage)
+      ).build()
     } else {
       TaskResult.builder(ExecutionStatus.RUNNING).context(context.toMap()).build()
     }
   }
 
-  private fun buildError(errorDetails: String?) =
-    TaskResult.builder(ExecutionStatus.TERMINAL).context(mapOf("error" to errorDetails)).build()
+  private fun buildError(errorMessage: String): TaskResult {
+    log.error(errorMessage)
+    return TaskResult.builder(ExecutionStatus.TERMINAL).context(mapOf("error" to errorMessage)).build()
+  }
 
   override fun getBackoffPeriod() = TimeUnit.SECONDS.toMillis(30)
 
@@ -156,13 +169,17 @@ constructor(
     var manifest: String? = "spinnaker.yml",
     var ref: String? = null,
     var attempt: Int = 1,
-    val maxRetries: Int? = MAX_RETRIES
+    val maxRetries: Int? = MAX_RETRIES,
+    var errorFromLastAttempt: String? = null
   )
 
   fun ImportDeliveryConfigContext.incrementAttempt() = this.also { attempt += 1 }
   fun ImportDeliveryConfigContext.toMap() = objectMapper.convertValue<Map<String, Any?>>(this)
 
   companion object {
     const val MAX_RETRIES = 5
+    const val UNAUTHORIZED_SCM_ACCESS_MESSAGE =
+      "HTTP 401 response received while trying to read your delivery config file. " +
+      "Spinnaker may be missing permissions in your source code repository to read the file."
   }
 }

orca-keel/src/test/kotlin/com/netflix/spinnaker/orca/keel/ImportDeliveryConfigTaskTests.kt

@@ -23,6 +23,7 @@ import com.netflix.spinnaker.orca.KeelService
 import com.netflix.spinnaker.orca.TaskResult
 import com.netflix.spinnaker.orca.igor.ScmService
 import com.netflix.spinnaker.orca.keel.task.ImportDeliveryConfigTask
+import com.netflix.spinnaker.orca.keel.task.ImportDeliveryConfigTask.Companion.UNAUTHORIZED_SCM_ACCESS_MESSAGE
 import com.netflix.spinnaker.orca.pipeline.model.DefaultTrigger
 import com.netflix.spinnaker.orca.pipeline.model.Execution
 import com.netflix.spinnaker.orca.pipeline.model.GitTrigger
@@ -37,7 +38,9 @@ import retrofit.RetrofitError
 import retrofit.client.Response
 import strikt.api.expectThat
 import strikt.api.expectThrows
+import strikt.assertions.contains
 import strikt.assertions.isEqualTo
+import strikt.assertions.isNotNull
 import java.lang.IllegalArgumentException
 
 internal class ImportDeliveryConfigTaskTests : JUnit5Minutests {
@@ -264,7 +267,32 @@ internal class ImportDeliveryConfigTaskTests : JUnit5Minutests {
         }
       }
 
-      context("failure to call downstream services") {
+      context("unauthorized access to manifest") {
+        modifyFixture {
+          with(scmService) {
+            every {
+              getDeliveryConfigManifest(
+                manifestLocation.repoType,
+                manifestLocation.projectKey,
+                manifestLocation.repositorySlug,
+                manifestLocation.directory,
+                manifestLocation.manifest,
+                manifestLocation.ref
+              )
+            } throws RetrofitError.httpError("http://igor",
+              Response("http://igor", 401, "", emptyList(), null),
+              null, null)
+          }
+        }
+
+        test("task fails with a helpful error message") {
+          val result = execute(manifestLocation.toMap())
+          expectThat(result.status).isEqualTo(ExecutionStatus.TERMINAL)
+          expectThat(result.context["error"]).isEqualTo(UNAUTHORIZED_SCM_ACCESS_MESSAGE)
+        }
+      }
+
+      context("retryable failure to call downstream services") {
         modifyFixture {
           with(scmService) {
             every {
@@ -295,6 +323,15 @@ internal class ImportDeliveryConfigTaskTests : JUnit5Minutests {
           val result = execute(manifestLocation.toMap().also { it["attempt"] = ImportDeliveryConfigTask.MAX_RETRIES })
           expectThat(result.status).isEqualTo(ExecutionStatus.TERMINAL)
         }
+
+        test("task result context includes the error from the last attempt") {
+          var result: TaskResult? = null
+          for (attempt in 1..ImportDeliveryConfigTask.MAX_RETRIES) {
+            result = execute(manifestLocation.toMap().also { it["attempt"] = attempt })
+          }
+          expectThat(result!!.context["errorFromLastAttempt"]).isNotNull()
+          expectThat(result!!.context["error"] as String).contains(result!!.context["errorFromLastAttempt"] as String)
+        }
       }
     }
   }