Skip to content

Commit

Permalink
fix(errors): Improve handling of keel errors in ImportDeliveryConfigT…
Browse files Browse the repository at this point in the history 
…ask (spinnaker#3604)
  • Loading branch information
@luispollo
luispollo authored Apr 15, 2020
1 parent dceaf96 commit 0ad1592
Show file tree
Hide file tree
Showing 2 changed files with 99 additions and 29 deletions.
69 changes: 50 additions & 19 deletions orca-keel/src/main/kotlin/com/netflix/spinnaker/orca/keel/task/ImportDeliveryConfigTask.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,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.kork.web.exceptions.InvalidRequestException
import com.netflix.spinnaker.orca.KeelService
import com.netflix.spinnaker.orca.api.pipeline.RetryableTask
import com.netflix.spinnaker.orca.api.pipeline.TaskResult
Expand All @@ -28,14 +29,15 @@ import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution
import com.netflix.spinnaker.orca.api.pipeline.models.Trigger
import com.netflix.spinnaker.orca.igor.ScmService
import java.net.URL
import java.time.Instant
import java.util.concurrent.TimeUnit
import org.slf4j.LoggerFactory
import org.springframework.stereotype.Component
import retrofit.RetrofitError

/**
* Task that retrieves a Managed Delivery config manifest from source control via igor, then publishes it to keel,
* to support GitOps flows.
* to support git-based workflows.
*/
@Component
class ImportDeliveryConfigTask
Expand Down Expand Up @@ -69,6 +71,11 @@ constructor(
}
}

/**
* Process the trigger and context data to make sure we can find the delivery config file.
*
* @throws InvalidRequestException if there's not enough information to locate the file.
*/
private fun processDeliveryConfigLocation(trigger: Trigger, context: ImportDeliveryConfigContext): String {
if (trigger is SourceCodeTrigger) {
// if the pipeline has a source code trigger (git, etc.), infer what context we can from the trigger
Expand All @@ -94,7 +101,7 @@ constructor(
context.ref = "refs/heads/master"
}
if (context.repoType == null || context.projectKey == null || context.repositorySlug == null) {
throw IllegalArgumentException("repoType, projectKey and repositorySlug are required fields in the stage if there's no git trigger.")
throw InvalidRequestException("repoType, projectKey and repositorySlug are required fields in the stage if there's no git trigger.")
}
}

Expand All @@ -103,37 +110,49 @@ constructor(
?: ""}/${context.manifest}@${context.ref}"
}

private fun handleRetryableFailures(e: RetrofitError, context: ImportDeliveryConfigContext): TaskResult {
/**
* 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.
*/
private fun handleRetryableFailures(error: RetrofitError, context: ImportDeliveryConfigContext): TaskResult {
return when {
e.kind == RetrofitError.Kind.NETWORK -> {
error.kind == RetrofitError.Kind.NETWORK -> {
// retry if unable to connect
buildRetry(context,
"Network error talking to downstream service, attempt ${context.attempt} of ${context.maxRetries}: ${e.friendlyMessage}")
"Network error talking to downstream service, attempt ${context.attempt} of ${context.maxRetries}: ${error.friendlyMessage}")
}
e.response?.status in 400..499 -> {
val response = e.response!!
// just give up on 4xx errors, which are unlikely to resolve with retries
error.response?.status in 400..499 -> {
val response = error.response!!
// 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(
// ...but give users a hint about 401 errors from igor/scm
if (response.status == 401 && e.fromIgor) {
if (error.fromIgor && response.status == 401) {
UNAUTHORIZED_SCM_ACCESS_MESSAGE
} else if (response.status == 400 && e.fromKeel && response.body.length() > 0) {
objectMapper.readValue<Map<String, Any?>>(response.body.`in`())
} else if (error.fromKeel && response.body.length() > 0) {
// keel's errors should use the standard Spring format, so we try to parse them
try {
objectMapper.readValue<SpringHttpError>(response.body.`in`())
} catch (_: Exception) {
"Non-retryable HTTP response ${error.response?.status} received from downstream service: ${error.friendlyMessage}"
}
} else {
"Non-retryable HTTP response ${e.response?.status} received from downstream service: ${e.friendlyMessage}"
"Non-retryable HTTP response ${error.response?.status} received from downstream service: ${error.friendlyMessage}"
}
)
}
else -> {
// retry on other status codes
buildRetry(context,
"Retryable HTTP response ${e.response?.status} received from downstream service: ${e.friendlyMessage}")
"Retryable HTTP response ${error.response?.status} received from downstream service: ${error.friendlyMessage}")
}
}
}

/**
* Builds a [TaskResult] that indicates the task is still running, so that we will try again in the next execution loop.
*/
private fun buildRetry(context: ImportDeliveryConfigContext, errorMessage: String): TaskResult {
log.error("Handling retry failure ${context.attempt} of ${context.maxRetries}: $errorMessage")
log.error("Handling retryable failure ${context.attempt} of ${context.maxRetries}: $errorMessage")
context.errorFromLastAttempt = errorMessage
context.incrementAttempt()

Expand All @@ -149,11 +168,15 @@ constructor(
}
}

/**
* Builds a [TaskResult] that indicates the task has failed. If the error has the shape of a [SpringHttpError],
* uses that format so the UI has better error information to display.
*/
private fun buildError(error: Any): TaskResult {
val normalizedError = if (error is Map<*, *>) {
error["error"] ?: error
val normalizedError = if (error is SpringHttpError) {
error
} else {
error.toString()
mapOf("message" to error.toString())
}
log.error(normalizedError.toString())
return TaskResult.builder(ExecutionStatus.TERMINAL).context(mapOf("error" to normalizedError)).build()
Expand Down Expand Up @@ -197,10 +220,18 @@ constructor(
fun ImportDeliveryConfigContext.incrementAttempt() = this.also { attempt += 1 }
fun ImportDeliveryConfigContext.toMap() = objectMapper.convertValue<Map<String, Any?>>(this)

data class SpringHttpError(
val error: String,
val status: Int,
val message: String? = error,
val timestamp: Instant = Instant.now(),
val details: Map<String, Any?>? = null // this is keel-specific
)

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."
"Spinnaker may be missing permissions in your source code repository to read the file."
}
}
59 changes: 49 additions & 10 deletions orca-keel/src/test/kotlin/com/netflix/spinnaker/orca/keel/ImportDeliveryConfigTaskTests.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,17 @@ package com.netflix.spinnaker.orca.keel

import com.fasterxml.jackson.databind.ObjectMapper
import com.fasterxml.jackson.module.kotlin.convertValue
import com.netflix.spinnaker.kork.web.exceptions.InvalidRequestException
import com.netflix.spinnaker.orca.KeelService
import com.netflix.spinnaker.orca.api.pipeline.TaskResult
import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionType
import com.netflix.spinnaker.orca.api.pipeline.models.Trigger
import com.netflix.spinnaker.orca.config.KeelConfiguration
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.keel.task.ImportDeliveryConfigTask.SpringHttpError
import com.netflix.spinnaker.orca.pipeline.model.DefaultTrigger
import com.netflix.spinnaker.orca.pipeline.model.GitTrigger
import com.netflix.spinnaker.orca.pipeline.model.PipelineExecutionImpl
Expand All @@ -35,7 +38,8 @@ import dev.minutest.rootContext
import io.mockk.every
import io.mockk.mockk
import io.mockk.verify
import java.lang.IllegalArgumentException
import org.springframework.http.HttpStatus.BAD_REQUEST
import org.springframework.http.HttpStatus.FORBIDDEN
import retrofit.RetrofitError
import retrofit.client.Response
import retrofit.converter.JacksonConverter
Expand All @@ -61,7 +65,7 @@ internal class ImportDeliveryConfigTaskTests : JUnit5Minutests {
val trigger: Trigger
) {
companion object {
val objectMapper = ObjectMapper()
val objectMapper: ObjectMapper = KeelConfiguration().keelObjectMapper()
}

val manifestLocation = ManifestLocation(
Expand Down Expand Up @@ -103,9 +107,11 @@ internal class ImportDeliveryConfigTaskTests : JUnit5Minutests {
)
)

val parsingError = mapOf(
"message" to "Parsing error",
"details" to mapOf(
val parsingError = SpringHttpError(
status = BAD_REQUEST.value(),
error = BAD_REQUEST.reasonPhrase,
message = "Parsing error",
details = mapOf(
"message" to "Parsing error",
"path" to listOf(
mapOf(
Expand All @@ -116,11 +122,19 @@ internal class ImportDeliveryConfigTaskTests : JUnit5Minutests {
"pathExpression" to ".someField"
)
)

val accessDeniedError = SpringHttpError(
status = FORBIDDEN.value(),
error = FORBIDDEN.reasonPhrase,
message = "Access denied"
)
}

private fun ManifestLocation.toMap() =
Fixture.objectMapper.convertValue<Map<String, Any?>>(this).toMutableMap()

private val objectMapper = Fixture.objectMapper

fun tests() = rootContext<Fixture> {
context("basic behavior") {
fixture {
Expand Down Expand Up @@ -156,7 +170,7 @@ internal class ImportDeliveryConfigTaskTests : JUnit5Minutests {

context("with required stage context missing") {
test("throws an exception") {
expectThrows<IllegalArgumentException> {
expectThrows<InvalidRequestException> {
execute(manifestLocation.toMap().also { it.remove("repoType") })
}
}
Expand Down Expand Up @@ -306,7 +320,32 @@ internal class ImportDeliveryConfigTaskTests : JUnit5Minutests {
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)
expectThat(result.context["error"]).isEqualTo(mapOf("message" to UNAUTHORIZED_SCM_ACCESS_MESSAGE))
}
}

context("keel access denied 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", 403, "", emptyList(),
JacksonConverter(objectMapper).toBody(accessDeniedError) 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<SpringHttpError>().isEqualTo(accessDeniedError)
}
}

Expand All @@ -323,15 +362,15 @@ internal class ImportDeliveryConfigTaskTests : JUnit5Minutests {
manifestLocation.ref
)
} throws RetrofitError.httpError("http://keel",
Response("http://keel", 400, "", emptyList(), JacksonConverter(ObjectMapper()).toBody(parsingError) as TypedInput),
null, null)
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)
expectThat(result.context["error"]).isA<SpringHttpError>().isEqualTo(parsingError)
}
}

Expand Down

0 comments on commit 0ad1592

Please sign in to comment.