diff --git a/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/config/IgorConfiguration.groovy b/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/config/IgorConfiguration.groovy index 8176a2c0e5e..2fc638d15a2 100644 --- a/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/config/IgorConfiguration.groovy +++ b/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/config/IgorConfiguration.groovy @@ -20,6 +20,7 @@ import com.fasterxml.jackson.databind.ObjectMapper 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.igor.IgorService import com.netflix.spinnaker.orca.retrofit.RetrofitConfiguration import com.netflix.spinnaker.orca.retrofit.logging.RetrofitSlf4jLog @@ -34,7 +35,6 @@ import org.springframework.context.annotation.Import import retrofit.Endpoint import retrofit.RequestInterceptor import retrofit.RestAdapter -import retrofit.client.Client import retrofit.converter.JacksonConverter import static retrofit.Endpoints.newFixedEndpoint @@ -61,6 +61,7 @@ class IgorConfiguration { .setEndpoint(igorEndpoint) .setClient(new Ok3Client(clientProvider.getClient(new DefaultServiceEndpoint("igor", igorEndpoint.url)))) .setLogLevel(retrofitLogLevel) + .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance()) .setRequestInterceptor(spinnakerRequestInterceptor) .setLog(new RetrofitSlf4jLog(IgorService)) .setConverter(new JacksonConverter(mapper)) diff --git a/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorJenkinsJobTask.groovy b/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorJenkinsJobTask.groovy index 6aecaaecfae..10b5225c2f0 100644 --- a/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorJenkinsJobTask.groovy +++ b/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorJenkinsJobTask.groovy @@ -18,6 +18,7 @@ package com.netflix.spinnaker.orca.igor.tasks import com.google.common.base.Enums import com.netflix.spinnaker.kork.core.RetrySupport +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus import com.netflix.spinnaker.orca.api.pipeline.OverridableTimeoutRetryableTask import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution @@ -26,7 +27,6 @@ import com.netflix.spinnaker.orca.igor.BuildService import groovy.util.logging.Slf4j import org.springframework.beans.factory.annotation.Autowired import org.springframework.stereotype.Component -import retrofit.RetrofitError import java.util.concurrent.TimeUnit @@ -81,9 +81,9 @@ class MonitorJenkinsJobTask implements OverridableTimeoutRetryableTask { } return TaskResult.builder(taskStatus).context(outputs).outputs(outputs).build() - } catch (RetrofitError e) { - if ([503, 500, 404].contains(e.response?.status)) { - log.warn("Http ${e.response.status} received from `igor`, retrying...") + } catch (SpinnakerHttpException e) { + if ([503, 500, 404].contains(e.responseCode)) { + log.warn("Http ${e.responseCode} received from `igor`, retrying...") return TaskResult.ofStatus(ExecutionStatus.RUNNING) } diff --git a/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorQueuedJenkinsJobTask.groovy b/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorQueuedJenkinsJobTask.groovy index 3a479d8f5bc..7db9cd9c58d 100644 --- a/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorQueuedJenkinsJobTask.groovy +++ b/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorQueuedJenkinsJobTask.groovy @@ -16,6 +16,7 @@ package com.netflix.spinnaker.orca.igor.tasks +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution import com.netflix.spinnaker.orca.igor.IgorService import org.springframework.beans.factory.annotation.Value @@ -28,7 +29,6 @@ import com.netflix.spinnaker.orca.igor.BuildService import groovy.util.logging.Slf4j import org.springframework.beans.factory.annotation.Autowired import org.springframework.stereotype.Component -import retrofit.RetrofitError @Slf4j @Component @@ -56,9 +56,9 @@ class MonitorQueuedJenkinsJobTask implements OverridableTimeoutRetryableTask { createBacklink(stage, jenkinsController, jobName, build.number as Integer) return TaskResult.builder(ExecutionStatus.SUCCEEDED).context([buildNumber: build.number]).build() } - } catch (RetrofitError e) { - if ([503, 500, 404].contains(e.response?.status)) { - log.warn("Http ${e.response.status} received from `igor`, retrying...") + } catch (SpinnakerHttpException e) { + if ([503, 500, 404].contains(e.responseCode)) { + log.warn("Http ${e.responseCode} received from `igor`, retrying...") return TaskResult.ofStatus(ExecutionStatus.RUNNING) } throw e diff --git a/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorWerckerJobStartedTask.groovy b/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorWerckerJobStartedTask.groovy index 9fe96b056c8..5fac717cf14 100644 --- a/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorWerckerJobStartedTask.groovy +++ b/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorWerckerJobStartedTask.groovy @@ -8,6 +8,7 @@ */ package com.netflix.spinnaker.orca.igor.tasks +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus import com.netflix.spinnaker.orca.api.pipeline.OverridableTimeoutRetryableTask import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution @@ -16,7 +17,6 @@ import com.netflix.spinnaker.orca.igor.BuildService import groovy.util.logging.Slf4j import org.springframework.beans.factory.annotation.Autowired import org.springframework.stereotype.Component -import retrofit.RetrofitError import javax.annotation.Nonnull import java.util.concurrent.TimeUnit @@ -38,7 +38,6 @@ class MonitorWerckerJobStartedTask implements OverridableTimeoutRetryableTask { try { Map build = buildService.getBuild(buildNumber, master, job) - Map outputs = [:] if ("not_built".equals(build?.result) || build?.number == null) { //The build has not yet started, so the job started monitoring task needs to be re-run return TaskResult.ofStatus(ExecutionStatus.RUNNING) @@ -47,9 +46,9 @@ class MonitorWerckerJobStartedTask implements OverridableTimeoutRetryableTask { return TaskResult.builder(ExecutionStatus.SUCCEEDED).context([buildNumber: build.number]).build() } - } catch (RetrofitError e) { - if ([503, 500, 404].contains(e.response?.status)) { - log.warn("Http ${e.response.status} received from `igor`, retrying...") + } catch (SpinnakerHttpException e) { + if ([503, 500, 404].contains(e.responseCode)) { + log.warn("Http ${e.responseCode} received from `igor`, retrying...") return TaskResult.ofStatus(ExecutionStatus.RUNNING) } diff --git a/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/StartJenkinsJobTask.groovy b/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/StartJenkinsJobTask.groovy index 5138cfea449..24b39584137 100644 --- a/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/StartJenkinsJobTask.groovy +++ b/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/StartJenkinsJobTask.groovy @@ -18,20 +18,19 @@ package com.netflix.spinnaker.orca.igor.tasks import com.fasterxml.jackson.databind.ObjectMapper import com.netflix.spinnaker.kork.exceptions.SystemException +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException import com.netflix.spinnaker.orca.api.pipeline.RetryableTask import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus -import com.netflix.spinnaker.orca.api.pipeline.Task import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution import com.netflix.spinnaker.orca.api.pipeline.TaskResult import com.netflix.spinnaker.orca.exceptions.ExceptionHandler import com.netflix.spinnaker.orca.igor.BuildService -import com.netflix.spinnaker.orca.retrofit.exceptions.RetrofitExceptionHandler +import com.netflix.spinnaker.orca.retrofit.exceptions.SpinnakerServerExceptionHandler import org.slf4j.Logger import org.slf4j.LoggerFactory import org.springframework.beans.factory.annotation.Autowired import org.springframework.http.HttpStatus import org.springframework.stereotype.Component -import retrofit.RetrofitError import retrofit.client.Response import javax.annotation.Nonnull @@ -48,7 +47,7 @@ class StartJenkinsJobTask implements RetryableTask { ObjectMapper objectMapper @Autowired - RetrofitExceptionHandler retrofitExceptionHandler + SpinnakerServerExceptionHandler spinnakerServerExceptionHandler @Nonnull @Override @@ -72,9 +71,9 @@ class StartJenkinsJobTask implements RetryableTask { .build() } } - catch (RetrofitError e) { + catch (SpinnakerServerException e){ // This igor call is idempotent so we can retry despite it being PUT/POST - ExceptionHandler.Response exceptionResponse = retrofitExceptionHandler.handle("StartJenkinsJob", e) + ExceptionHandler.Response exceptionResponse = spinnakerServerExceptionHandler.handle("StartJenkinsJob", e) if (exceptionResponse.shouldRetry) { log.warn("Failure communicating with igor to start a jenkins job, will retry", e) diff --git a/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/StartScriptTask.groovy b/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/StartScriptTask.groovy index c41bf144f71..5811b072d25 100644 --- a/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/StartScriptTask.groovy +++ b/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/StartScriptTask.groovy @@ -18,13 +18,14 @@ package com.netflix.spinnaker.orca.igor.tasks import com.fasterxml.jackson.databind.ObjectMapper import com.netflix.spinnaker.kork.exceptions.SystemException +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException import com.netflix.spinnaker.orca.api.pipeline.RetryableTask import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution import com.netflix.spinnaker.orca.api.pipeline.TaskResult import com.netflix.spinnaker.orca.exceptions.ExceptionHandler import com.netflix.spinnaker.orca.igor.BuildService -import com.netflix.spinnaker.orca.retrofit.exceptions.RetrofitExceptionHandler +import com.netflix.spinnaker.orca.retrofit.exceptions.SpinnakerServerExceptionHandler import org.slf4j.Logger import org.slf4j.LoggerFactory import org.springframework.beans.factory.annotation.Autowired @@ -32,7 +33,6 @@ import org.springframework.beans.factory.annotation.Value import org.springframework.core.env.Environment import org.springframework.http.HttpStatus import org.springframework.stereotype.Component -import retrofit.RetrofitError import retrofit.client.Response import javax.annotation.Nonnull @@ -49,7 +49,7 @@ class StartScriptTask implements RetryableTask { ObjectMapper objectMapper @Autowired - RetrofitExceptionHandler retrofitExceptionHandler + SpinnakerServerExceptionHandler spinnakerServerExceptionHandler @Autowired Environment environment @@ -113,9 +113,9 @@ class StartScriptTask implements RetryableTask { .build() } } - catch (RetrofitError e) { + catch (SpinnakerServerException e) { // This igor call is idempotent so we can retry despite it being PUT/POST - ExceptionHandler.Response exceptionResponse = retrofitExceptionHandler.handle("StartJenkinsJob", e) + ExceptionHandler.Response exceptionResponse = spinnakerServerExceptionHandler.handle("StartJenkinsJob", e) if (exceptionResponse.shouldRetry) { log.warn("Failure communicating with igor to start a jenkins job, will retry", e) diff --git a/orca-igor/src/main/java/com/netflix/spinnaker/orca/igor/tasks/RetryableIgorTask.java b/orca-igor/src/main/java/com/netflix/spinnaker/orca/igor/tasks/RetryableIgorTask.java index 97e004b1989..c7abee72ec5 100644 --- a/orca-igor/src/main/java/com/netflix/spinnaker/orca/igor/tasks/RetryableIgorTask.java +++ b/orca-igor/src/main/java/com/netflix/spinnaker/orca/igor/tasks/RetryableIgorTask.java @@ -17,6 +17,9 @@ package com.netflix.spinnaker.orca.igor.tasks; import com.google.common.collect.ImmutableMap; +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.orca.api.pipeline.RetryableTask; import com.netflix.spinnaker.orca.api.pipeline.TaskResult; import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus; @@ -28,7 +31,6 @@ import javax.annotation.Nonnull; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; -import retrofit.RetrofitError; @RequiredArgsConstructor @Slf4j @@ -53,7 +55,7 @@ protected int getMaxConsecutiveErrors() { try { TaskResult result = tryExecute(stageDefinition); return resetErrorCount(result); - } catch (RetrofitError e) { + } catch (SpinnakerServerException e) { if (stageDefinition.getConsecutiveErrors() < getMaxConsecutiveErrors() && isRetryable(e)) { return TaskResult.builder(ExecutionStatus.RUNNING) .context(errorContext(errors + 1)) @@ -83,14 +85,13 @@ private Map errorContext(int errors) { return Collections.singletonMap("consecutiveErrors", errors); } - private boolean isRetryable(RetrofitError retrofitError) { - if (retrofitError.getKind() == RetrofitError.Kind.NETWORK) { + private boolean isRetryable(SpinnakerServerException exception) { + if (exception instanceof SpinnakerNetworkException) { log.warn("Failed to communicate with igor, retrying..."); return true; - } - - if (retrofitError.getResponse() != null) { - int status = retrofitError.getResponse().getStatus(); + } else if (exception instanceof SpinnakerHttpException) { + var httpException = (SpinnakerHttpException) exception; + int status = httpException.getResponseCode(); if (status == 500 || status == 503) { log.warn(String.format("Received HTTP %s response from igor, retrying...", status)); return true; diff --git a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetAwsCodeBuildArtifactsTaskSpec.groovy b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetAwsCodeBuildArtifactsTaskSpec.groovy index fe160d668fd..6166474ebb3 100644 --- a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetAwsCodeBuildArtifactsTaskSpec.groovy +++ b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetAwsCodeBuildArtifactsTaskSpec.groovy @@ -17,6 +17,7 @@ package com.netflix.spinnaker.orca.igor.tasks import com.netflix.spinnaker.kork.artifacts.model.Artifact +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerNetworkException import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus import com.netflix.spinnaker.orca.api.pipeline.TaskResult import com.netflix.spinnaker.orca.igor.IgorService @@ -73,14 +74,14 @@ class GetAwsCodeBuildArtifactsTaskSpec extends Specification { TaskResult result = task.execute(stage) then: - 1 * igorService.getAwsCodeBuildArtifacts(ACCOUNT, BUILD_ID) >> { throw stubRetrofitError() } + 1 * igorService.getAwsCodeBuildArtifacts(ACCOUNT, BUILD_ID) >> { throw stubNetworkError() } 0 * igorService._ result.getStatus() == ExecutionStatus.RUNNING } - def stubRetrofitError() { - return Stub(RetrofitError) { + def stubNetworkError() { + return new SpinnakerNetworkException(Stub(RetrofitError) { getKind() >> RetrofitError.Kind.NETWORK - } + }) } } diff --git a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetBuildPropertiesTaskSpec.groovy b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetBuildPropertiesTaskSpec.groovy index 43a57d7eaac..c5ca50c3e20 100644 --- a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetBuildPropertiesTaskSpec.groovy +++ b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetBuildPropertiesTaskSpec.groovy @@ -19,6 +19,7 @@ package com.netflix.spinnaker.orca.igor.tasks import com.fasterxml.jackson.databind.ObjectMapper import com.netflix.spinnaker.kork.artifacts.model.Artifact import com.netflix.spinnaker.kork.exceptions.ConfigurationException +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus import com.netflix.spinnaker.orca.api.pipeline.TaskResult import com.netflix.spinnaker.orca.igor.BuildService @@ -120,9 +121,11 @@ class GetBuildPropertiesTaskSpec extends Specification { getResponse() >> new Response("", 500, "", Collections.emptyList(), null) } + def httpException = new SpinnakerHttpException(igorError) + and: 1 * buildService.getPropertyFile(BUILD_NUMBER, PROPERTY_FILE, MASTER, JOB) >> - { throw igorError } + { throw httpException } when: TaskResult result = task.execute(stage) diff --git a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetCommitsTaskSpec.groovy b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetCommitsTaskSpec.groovy index ff13049e648..91256a84d49 100644 --- a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetCommitsTaskSpec.groovy +++ b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetCommitsTaskSpec.groovy @@ -260,8 +260,7 @@ class GetCommitsTaskSpec extends Specification { then: 1 * scmService.compareCommits("stash", "projectKey", "repositorySlug", ['to': '186605b', 'from': 'a86305d', 'limit': 100]) >> { - throw new RetrofitError(null, null, - new Response("http://stash.com", 500, "test reason", [], null), null, null, null, null) + throw new SpinnakerHttpException(RetrofitError.httpError("http://stash.com", new Response("http://stash.com", 500, "test reason", [], null), null, null)) } result.status == taskStatus @@ -450,7 +449,7 @@ class GetCommitsTaskSpec extends Specification { and: task.scmService = Stub(ScmService) { compareCommits("stash", "projectKey", "repositorySlug", ['to': '186605b', 'from': 'a86305d', 'limit': 100]) >> { - throw new RetrofitError(null, null, new Response("http://stash.com", 404, "test reason", [], null), null, null, null, null) + throw new SpinnakerHttpException(RetrofitError.httpError("http://stash.com", new Response("http://stash.com", 404, "test reason", [], null), null, null)) } } diff --git a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetGoogleCloudBuildArtifactsTaskSpec.groovy b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetGoogleCloudBuildArtifactsTaskSpec.groovy index a72aca200da..abc8e6fb7be 100644 --- a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetGoogleCloudBuildArtifactsTaskSpec.groovy +++ b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetGoogleCloudBuildArtifactsTaskSpec.groovy @@ -17,6 +17,7 @@ package com.netflix.spinnaker.orca.igor.tasks import com.netflix.spinnaker.kork.artifacts.model.Artifact +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerNetworkException import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus import com.netflix.spinnaker.orca.api.pipeline.TaskResult import com.netflix.spinnaker.orca.igor.IgorService @@ -72,14 +73,14 @@ class GetGoogleCloudBuildArtifactsTaskSpec extends Specification { TaskResult result = task.execute(stage) then: - 1 * igorService.getGoogleCloudBuildArtifacts(ACCOUNT, BUILD_ID) >> { throw stubRetrofitError() } + 1 * igorService.getGoogleCloudBuildArtifacts(ACCOUNT, BUILD_ID) >> { throw stubNetworkError() } 0 * igorService._ result.getStatus() == ExecutionStatus.RUNNING } - def stubRetrofitError() { - return Stub(RetrofitError) { + def stubNetworkError() { + return new SpinnakerNetworkException(Stub(RetrofitError) { getKind() >> RetrofitError.Kind.NETWORK - } + }) } } diff --git a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorAwsCodeBuildTaskSpec.groovy b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorAwsCodeBuildTaskSpec.groovy index f9fcf3191af..fdd7e36efda 100644 --- a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorAwsCodeBuildTaskSpec.groovy +++ b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorAwsCodeBuildTaskSpec.groovy @@ -16,6 +16,7 @@ package com.netflix.spinnaker.orca.igor.tasks +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerNetworkException import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus import com.netflix.spinnaker.orca.api.pipeline.TaskResult import com.netflix.spinnaker.orca.igor.IgorService @@ -82,14 +83,14 @@ class MonitorAwsCodeBuildTaskSpec extends Specification { TaskResult result = task.execute(stage) then: - 1 * igorService.getAwsCodeBuildExecution(ACCOUNT, BUILD_ID) >> { throw stubRetrofitError() } + 1 * igorService.getAwsCodeBuildExecution(ACCOUNT, BUILD_ID) >> { throw stubNetworkError() } 0 * igorService._ result.getStatus() == ExecutionStatus.RUNNING } - def stubRetrofitError() { - return Stub(RetrofitError) { + def stubNetworkError() { + return new SpinnakerNetworkException(Stub(RetrofitError) { getKind() >> RetrofitError.Kind.NETWORK - } + }) } } diff --git a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorGoogleCloudBuildTaskSpec.groovy b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorGoogleCloudBuildTaskSpec.groovy index 68c7917d9aa..74057951f63 100644 --- a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorGoogleCloudBuildTaskSpec.groovy +++ b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorGoogleCloudBuildTaskSpec.groovy @@ -16,6 +16,7 @@ package com.netflix.spinnaker.orca.igor.tasks +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerNetworkException import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus import com.netflix.spinnaker.orca.api.pipeline.TaskResult import com.netflix.spinnaker.orca.igor.IgorService @@ -85,14 +86,14 @@ class MonitorGoogleCloudBuildTaskSpec extends Specification { TaskResult result = task.execute(stage) then: - 1 * igorService.getGoogleCloudBuild(ACCOUNT, BUILD_ID) >> { throw stubRetrofitError() } + 1 * igorService.getGoogleCloudBuild(ACCOUNT, BUILD_ID) >> { throw stubNetworkError() } 0 * igorService._ result.getStatus() == ExecutionStatus.RUNNING } - def stubRetrofitError() { - return Stub(RetrofitError) { + def stubNetworkError() { + return new SpinnakerNetworkException(Stub(RetrofitError) { getKind() >> RetrofitError.Kind.NETWORK - } + }) } } diff --git a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorJenkinsJobTaskSpec.groovy b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorJenkinsJobTaskSpec.groovy index 3929879b3cd..45323115f73 100644 --- a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorJenkinsJobTaskSpec.groovy +++ b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorJenkinsJobTaskSpec.groovy @@ -16,6 +16,8 @@ package com.netflix.spinnaker.orca.igor.tasks +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus import com.netflix.spinnaker.orca.clouddriver.pipeline.job.model.JobStatus import com.netflix.spinnaker.orca.igor.BuildService @@ -111,8 +113,10 @@ class MonitorJenkinsJobTaskSpec extends Specification { getResponse() >> new Response('', httpStatus, '', [], null) } + def httpException = new SpinnakerHttpException(exception) + task.buildService = Stub(BuildService) { - getBuild(stage.context.buildNumber, stage.context.master, stage.context.job) >> { throw exception } + getBuild(stage.context.buildNumber, stage.context.master, stage.context.job) >> { throw httpException } } when: @@ -120,12 +124,12 @@ class MonitorJenkinsJobTaskSpec extends Specification { def thrownException = null try { result = task.execute(stage) - } catch (RetrofitError e) { + } catch (SpinnakerHttpException e) { thrownException = e } then: - thrownException ? thrownException == exception : result.status == expectedExecutionStatus + thrownException ? thrownException == httpException : result.status == expectedExecutionStatus where: httpStatus || expectedExecutionStatus diff --git a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/RetryableIgorTaskSpec.groovy b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/RetryableIgorTaskSpec.groovy index 409def020ba..a40e2431f63 100644 --- a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/RetryableIgorTaskSpec.groovy +++ b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/RetryableIgorTaskSpec.groovy @@ -16,6 +16,8 @@ package com.netflix.spinnaker.orca.igor.tasks +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerNetworkException import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus import com.netflix.spinnaker.orca.api.pipeline.TaskResult import com.netflix.spinnaker.orca.igor.model.RetryableStageDefinition @@ -51,7 +53,7 @@ class RetryableIgorTaskSpec extends Specification { def result = task.execute(stage) then: - 1 * task.tryExecute(jobRequest) >> { throw stubRetrofitError(500) } + 1 * task.tryExecute(jobRequest) >> { throw stubHttpError(500) } jobRequest.getConsecutiveErrors() >> 0 result.status == ExecutionStatus.RUNNING result.context.get("consecutiveErrors") == 1 @@ -62,7 +64,7 @@ class RetryableIgorTaskSpec extends Specification { def result = task.execute(stage) then: - 1 * task.tryExecute(jobRequest) >> { throw stubRetrofitNetworkError() } + 1 * task.tryExecute(jobRequest) >> { throw stubNetworkError() } jobRequest.getConsecutiveErrors() >> 0 result.status == ExecutionStatus.RUNNING result.context.get("consecutiveErrors") == 1 @@ -73,9 +75,9 @@ class RetryableIgorTaskSpec extends Specification { def result = task.execute(stage) then: - 1 * task.tryExecute(jobRequest) >> { throw stubRetrofitError(404) } + 1 * task.tryExecute(jobRequest) >> { throw stubHttpError(404) } jobRequest.getConsecutiveErrors() >> 0 - thrown RetrofitError + thrown SpinnakerHttpException } def "should propagate the error we have reached the retry limit"() { @@ -83,9 +85,9 @@ class RetryableIgorTaskSpec extends Specification { def result = task.execute(stage) then: - 1 * task.tryExecute(jobRequest) >> { throw stubRetrofitError(500) } + 1 * task.tryExecute(jobRequest) >> { throw stubHttpError(500) } jobRequest.getConsecutiveErrors() >> 5 - thrown RetrofitError + thrown SpinnakerHttpException } def "should propagate a non-successful task status"() { @@ -109,16 +111,16 @@ class RetryableIgorTaskSpec extends Specification { result.context.get("consecutiveErrors") == 0 } - def stubRetrofitError(int statusCode) { - return Stub(RetrofitError) { + def stubHttpError(int statusCode) { + return new SpinnakerHttpException(Stub(RetrofitError) { getKind() >> RetrofitError.Kind.HTTP getResponse() >> new Response("", statusCode, "", Collections.emptyList(), null) - } + }) } - def stubRetrofitNetworkError() { - return Stub(RetrofitError) { + def stubNetworkError() { + return new SpinnakerNetworkException(Stub(RetrofitError) { getKind() >> RetrofitError.Kind.NETWORK - } + }) } } diff --git a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/StartJenkinsJobTaskSpec.groovy b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/StartJenkinsJobTaskSpec.groovy index 6aa397e5d96..a49f2f7f482 100644 --- a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/StartJenkinsJobTaskSpec.groovy +++ b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/StartJenkinsJobTaskSpec.groovy @@ -17,11 +17,12 @@ package com.netflix.spinnaker.orca.igor.tasks import com.fasterxml.jackson.databind.ObjectMapper +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus import com.netflix.spinnaker.orca.igor.BuildService import com.netflix.spinnaker.orca.pipeline.model.PipelineExecutionImpl import com.netflix.spinnaker.orca.pipeline.model.StageExecutionImpl -import com.netflix.spinnaker.orca.retrofit.exceptions.RetrofitExceptionHandler +import com.netflix.spinnaker.orca.retrofit.exceptions.SpinnakerServerExceptionHandler import retrofit.RetrofitError import retrofit.client.Response import retrofit.mime.TypedString @@ -39,7 +40,7 @@ class StartJenkinsJobTaskSpec extends Specification { convertValue(_,_) >> [:] } - task.retrofitExceptionHandler = new RetrofitExceptionHandler() + task.spinnakerServerExceptionHandler = new SpinnakerServerExceptionHandler() } @Shared @@ -85,14 +86,14 @@ class StartJenkinsJobTaskSpec extends Specification { and: task.buildService = Stub(BuildService) { - build(stage.context.master, stage.context.job, stage.context.parameters, stage.startTime.toString()) >> {throw RetrofitError.unexpectedError("http://test", new RuntimeException())} + build(stage.context.master, stage.context.job, stage.context.parameters, stage.startTime.toString()) >> {throw new SpinnakerServerException(RetrofitError.unexpectedError("http://test", new RuntimeException()))} } when: task.execute(stage) then: - thrown(RetrofitError) + thrown(SpinnakerServerException) } def "handle 202 response from igor"() { 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 dd28ffb86e8..a8137889daf 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 @@ -40,7 +40,6 @@ 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, @@ -78,8 +77,6 @@ constructor( keelService.publishDeliveryConfig(deliveryConfig) TaskResult.builder(ExecutionStatus.SUCCEEDED).context(emptyMap()).build() - } catch (e: RetrofitError) { - handleRetryableFailures(e, context) } catch (e: SpinnakerServerException) { handleRetryableFailures(e, context) } catch (e: Exception) { @@ -221,48 +218,6 @@ constructor( } } - /** - * 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 { - error.kind == RetrofitError.Kind.NETWORK -> { - // retry if unable to connect - buildRetry( - context, - "Network error talking to downstream service, attempt ${context.attempt} of ${context.maxRetries}: ${error.friendlyMessage}" - ) - } - 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( - if (error.fromIgor && response.status == 401) { - UNAUTHORIZED_SCM_ACCESS_MESSAGE - } 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(response.body.`in`()) - } catch (_: Exception) { - "Non-retryable HTTP response ${error.response?.status} received from downstream service: ${error.friendlyMessage}" - } - } else { - "Non-retryable HTTP response ${error.response?.status} received from downstream service: ${error.friendlyMessage}" - } - ) - } - else -> { - // retry on other status codes - buildRetry( - context, - "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. */ @@ -301,12 +256,6 @@ constructor( override fun getTimeout() = TimeUnit.SECONDS.toMillis(180) - val RetrofitError.friendlyMessage: String - get() = if (kind == RetrofitError.Kind.HTTP) { - "HTTP ${response.status} ${response.url}: ${cause?.message ?: message}" - } else { - "$message: ${cause?.message ?: ""}" - } val SpinnakerHttpException.httpErrorMessage: String get() { @@ -323,11 +272,6 @@ constructor( return "$message" } - val RetrofitError.fromIgor: Boolean - get() { - val parsedUrl = URL(url) - return parsedUrl.host.contains("igor") || parsedUrl.port == 8085 - } val SpinnakerServerException.fromIgor: Boolean get() { @@ -335,12 +279,6 @@ constructor( 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) 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 39ea5b5efab..4caba7797b3 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 @@ -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.SpinnakerHttpException import com.netflix.spinnaker.kork.web.exceptions.InvalidRequestException import com.netflix.spinnaker.orca.KeelService import com.netflix.spinnaker.orca.api.pipeline.TaskResult @@ -288,11 +289,11 @@ internal class ImportDeliveryConfigTaskTests : JUnit5Minutests { manifestLocation.manifest, manifestLocation.ref ) - } throws RetrofitError.httpError( + } throws SpinnakerHttpException(RetrofitError.httpError( "http://igor", Response("http://igor", 404, "", emptyList(), null), null, null - ) + )) } } @@ -314,11 +315,11 @@ internal class ImportDeliveryConfigTaskTests : JUnit5Minutests { manifestLocation.manifest, manifestLocation.ref ) - } throws RetrofitError.httpError( + } throws SpinnakerHttpException(RetrofitError.httpError( "http://igor", Response("http://igor", 401, "", emptyList(), null), null, null - ) + )) } } @@ -341,11 +342,11 @@ internal class ImportDeliveryConfigTaskTests : JUnit5Minutests { manifestLocation.manifest, manifestLocation.ref ) - } throws RetrofitError.httpError( + } throws SpinnakerHttpException(RetrofitError.httpError( "http://igor", Response("http://igor", 503, "", emptyList(), null), null, null - ) + )) } } diff --git a/orca-web/orca-web.gradle b/orca-web/orca-web.gradle index 82ad37be6cd..c9e6a4c1c52 100644 --- a/orca-web/orca-web.gradle +++ b/orca-web/orca-web.gradle @@ -63,6 +63,7 @@ dependencies { implementation("net.logstash.logback:logstash-logback-encoder") implementation("io.spinnaker.fiat:fiat-api:$fiatVersion") implementation("io.spinnaker.fiat:fiat-core:$fiatVersion") + implementation("io.spinnaker.kork:kork-retrofit") runtimeOnly("io.spinnaker.kork:kork-runtime") diff --git a/orca-web/src/main/groovy/com/netflix/spinnaker/orca/controllers/OperationsController.groovy b/orca-web/src/main/groovy/com/netflix/spinnaker/orca/controllers/OperationsController.groovy index c0a6cf9a646..1352c43faef 100644 --- a/orca-web/src/main/groovy/com/netflix/spinnaker/orca/controllers/OperationsController.groovy +++ b/orca-web/src/main/groovy/com/netflix/spinnaker/orca/controllers/OperationsController.groovy @@ -23,6 +23,8 @@ import com.netflix.spinnaker.fiat.shared.FiatService import com.netflix.spinnaker.fiat.shared.FiatStatus import com.netflix.spinnaker.kork.exceptions.ConfigurationException import com.netflix.spinnaker.kork.exceptions.SpinnakerException +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException import com.netflix.spinnaker.orca.api.pipeline.models.PipelineExecution import com.netflix.spinnaker.orca.api.pipeline.models.Trigger import com.netflix.spinnaker.orca.clouddriver.service.JobService @@ -327,12 +329,14 @@ class OperationsController { def buildInfo try { buildInfo = buildService.getBuild(trigger.buildNumber, trigger.master, trigger.job) - } catch (RetrofitError e) { - if (e.response?.status == 404) { + } catch (SpinnakerHttpException e) { + if (e.responseCode == 404) { throw new ConfigurationException("Build ${trigger.buildNumber} of ${trigger.master}/${trigger.job} not found") } else { throw new OperationFailedException("Failed to get build ${trigger.buildNumber} of ${trigger.master}/${trigger.job}", e) } + } catch (SpinnakerServerException e){ + throw new OperationFailedException("Failed to get build ${trigger.buildNumber} of ${trigger.master}/${trigger.job}", e) } if (buildInfo?.artifacts) { if (trigger.type == "manual") { @@ -348,12 +352,14 @@ class OperationsController { trigger.master as String, trigger.job as String ) - } catch (RetrofitError e) { - if (e.response?.status == 404) { + } catch (SpinnakerHttpException e) { + if (e.responseCode == 404) { throw new ConfigurationException("Expected properties file " + trigger.propertyFile + " (configured on trigger), but it was missing") } else { throw new OperationFailedException("Failed to get properties file ${trigger.propertyFile}", e) } + } catch (SpinnakerServerException e){ + throw new OperationFailedException("Failed to get properties file ${trigger.propertyFile}", e) } } }