From 617f962d458d0ac1fa83610f4d4bdb88046f850b Mon Sep 17 00:00:00 2001 From: Luis Fernando Pollo <1323478+luispollo@users.noreply.github.com> Date: Fri, 28 Feb 2020 10:55:58 -0800 Subject: [PATCH] feat(managed-delivery): Include detailed error info in ImportDeliveryConfig (#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 --- orca-keel/orca-keel.gradle | 1 + .../com/netflix/spinnaker/orca/KeelService.kt | 2 + .../keel/task/ImportDeliveryConfigTask.kt | 29 +++++++++++-- .../keel/ImportDeliveryConfigTaskTests.kt | 42 +++++++++++++++++++ 4 files changed, 70 insertions(+), 4 deletions(-) diff --git a/orca-keel/orca-keel.gradle b/orca-keel/orca-keel.gradle index 091969ac17..3d69866faf 100644 --- 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 index c934b767d8..34be89ee7c 100644 --- 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 index d739d55bfe..0861b4656d 100644 --- 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>(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 index 860adef999..48fb791cc1 100644 --- 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>().isEqualTo(parsingError) + } + } + context("retryable failure to call downstream services") { modifyFixture { with(scmService) {