diff --git a/orca-applications/src/main/groovy/com/netflix/spinnaker/orca/applications/tasks/DeleteApplicationTask.groovy b/orca-applications/src/main/groovy/com/netflix/spinnaker/orca/applications/tasks/DeleteApplicationTask.groovy index f88800e15c..ae0d991f39 100644 --- a/orca-applications/src/main/groovy/com/netflix/spinnaker/orca/applications/tasks/DeleteApplicationTask.groovy +++ b/orca-applications/src/main/groovy/com/netflix/spinnaker/orca/applications/tasks/DeleteApplicationTask.groovy @@ -18,6 +18,8 @@ package com.netflix.spinnaker.orca.applications.tasks import com.fasterxml.jackson.databind.ObjectMapper import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException import com.netflix.spinnaker.orca.api.pipeline.TaskResult import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus import com.netflix.spinnaker.orca.front50.Front50Service @@ -93,6 +95,15 @@ class DeleteApplicationTask extends AbstractFront50Task { } log.error("Could not delete application", e) return TaskResult.builder(ExecutionStatus.TERMINAL).outputs(outputs).build() + } catch (SpinnakerHttpException httpException){ + if (httpException.responseCode == 404) { + return TaskResult.SUCCEEDED + } + log.error("Could not delete application", httpException) + return TaskResult.builder(ExecutionStatus.TERMINAL).outputs(outputs).build() + } catch (SpinnakerServerException serverException) { + log.error("Could not delete application", serverException) + return TaskResult.builder(ExecutionStatus.TERMINAL).outputs(outputs).build() } return TaskResult.builder(ExecutionStatus.SUCCEEDED).outputs(outputs).build() } diff --git a/orca-keel/orca-keel.gradle b/orca-keel/orca-keel.gradle index d770738610..e1e361a710 100644 --- a/orca-keel/orca-keel.gradle +++ b/orca-keel/orca-keel.gradle @@ -24,6 +24,7 @@ dependencies { implementation("com.fasterxml.jackson.module:jackson-module-kotlin") implementation("org.springframework:spring-web") implementation("org.springframework.boot:spring-boot-autoconfigure") + implementation("io.spinnaker.kork:kork-retrofit") testImplementation("com.fasterxml.jackson.module:jackson-module-kotlin") testImplementation("dev.minutest:minutest") @@ -33,6 +34,8 @@ dependencies { testImplementation("org.codehaus.groovy:groovy") testImplementation("org.junit.jupiter:junit-jupiter-api") testImplementation("org.junit.jupiter:junit-jupiter-params") + testImplementation("com.github.tomakehurst:wiremock-jre8-standalone") + testImplementation("org.mockito:mockito-junit-jupiter") } test { diff --git a/orca-keel/src/main/kotlin/com/netflix/spinnaker/orca/config/KeelConfiguration.kt b/orca-keel/src/main/kotlin/com/netflix/spinnaker/orca/config/KeelConfiguration.kt index 9bc4a53b72..09924c8298 100644 --- a/orca-keel/src/main/kotlin/com/netflix/spinnaker/orca/config/KeelConfiguration.kt +++ b/orca-keel/src/main/kotlin/com/netflix/spinnaker/orca/config/KeelConfiguration.kt @@ -22,6 +22,7 @@ import com.fasterxml.jackson.module.kotlin.KotlinModule 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.KeelService import com.netflix.spinnaker.orca.jackson.OrcaObjectMapper import org.springframework.beans.factory.annotation.Value @@ -60,6 +61,7 @@ class KeelConfiguration { .setEndpoint(keelEndpoint) .setClient(Ok3Client(clientProvider.getClient(DefaultServiceEndpoint("keel", keelEndpoint.url)))) .setLogLevel(retrofitLogLevel) + .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance()) .setConverter(JacksonConverter(keelObjectMapper)) .build() .create(KeelService::class.java) 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 31cfc13070..dd28ffb86e 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 @@ -19,6 +19,9 @@ 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.retrofit.exceptions.SpinnakerHttpException +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerNetworkException +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException import com.netflix.spinnaker.kork.web.exceptions.InvalidRequestException import com.netflix.spinnaker.orca.KeelService import com.netflix.spinnaker.orca.api.pipeline.RetryableTask @@ -77,6 +80,8 @@ constructor( TaskResult.builder(ExecutionStatus.SUCCEEDED).context(emptyMap()).build() } catch (e: RetrofitError) { handleRetryableFailures(e, context) + } catch (e: SpinnakerServerException) { + handleRetryableFailures(e, context) } catch (e: Exception) { log.error("Unexpected exception while executing {}, aborting.", javaClass.simpleName, e) buildError(e.message ?: "Unknown error (${e.javaClass.simpleName})") @@ -153,6 +158,69 @@ constructor( ?: ""}/${context.manifest}@${context.ref}" } + /* + * Handle (potentially) all Spinnaker*Exception. Smart casts to the respective type on Http error and/or Network error. + * @return default error message on non-http and non-network errors. + * */ + private fun handleRetryableFailures(error: SpinnakerServerException, context: ImportDeliveryConfigContext): TaskResult { + return when { + error is SpinnakerNetworkException -> { + // retry if unable to connect + buildRetry( + context, + "Network error talking to downstream service, attempt ${context.attempt} of ${context.maxRetries}: ${error.networkErrorMessage}" + ) + } + error is SpinnakerHttpException -> { + handleRetryableFailures(error, context) + } else -> { + buildRetry( + context, + "Server error talking to downstream service, attempt ${context.attempt} of ${context.maxRetries}: ${error.serverErrorMessage}" + ) + } + } + } + + /** + * Handle (potentially) retryable failures by looking at the HTTP status code. A few 4xx errors + * are handled as special cases to provide more friendly error messages to the UI. + */ + private fun handleRetryableFailures(httpException: SpinnakerHttpException, context: ImportDeliveryConfigContext): TaskResult{ + return when { + httpException.responseCode in 400..499 -> { + val responseBody = httpException.responseBody + // 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 (httpException.fromIgor && httpException.responseCode == 401) { + UNAUTHORIZED_SCM_ACCESS_MESSAGE + } else if (httpException.fromKeel && responseBody!=null && responseBody.isNotEmpty()) { + // keel's errors should use the standard Spring format + try { + if (responseBody["timestamp"] !=null) { + SpringHttpError(responseBody["error"] as String, responseBody["status"] as Int, responseBody["message"] as? String, Instant.ofEpochMilli(responseBody["timestamp"] as Long), responseBody["details"] as? Map) + } else { + SpringHttpError(error = responseBody["error"] as String, status = responseBody["status"] as Int, message = responseBody["message"] as? String, details = responseBody["details"] as? Map) + } + } catch (_: Exception) { + "Non-retryable HTTP response ${httpException.responseCode} received from downstream service: ${httpException.httpErrorMessage}" + } + } else { + "Non-retryable HTTP response ${httpException.responseCode} received from downstream service: ${httpException.httpErrorMessage}" + } + ) + } + else -> { + // retry on other status codes + buildRetry( + context, + "Retryable HTTP response ${httpException.responseCode} received from downstream service: ${httpException.httpErrorMessage}" + ) + } + } + } + /** * 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. @@ -240,18 +308,45 @@ constructor( "$message: ${cause?.message ?: ""}" } + val SpinnakerHttpException.httpErrorMessage: String + get() { + return "HTTP ${responseCode} ${url}: ${cause?.message ?: message}" + } + + val SpinnakerNetworkException.networkErrorMessage: String + get() { + return "$message: ${cause?.message ?: ""}" + } + + val SpinnakerServerException.serverErrorMessage: String + get() { + return "$message" + } + val RetrofitError.fromIgor: Boolean get() { val parsedUrl = URL(url) return parsedUrl.host.contains("igor") || parsedUrl.port == 8085 } + val SpinnakerServerException.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 } + val SpinnakerServerException.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, @@ -271,7 +366,7 @@ constructor( val error: String, val status: Int, val message: String? = error, - val timestamp: Instant = Instant.now(), + val timestamp: Instant? = Instant.now(), val details: Map? = null // this is keel-specific ) diff --git a/orca-keel/src/test/java/com/netflix/spinnaker/orca/keel/task/ImportDeliveryConfigTaskTest.java b/orca-keel/src/test/java/com/netflix/spinnaker/orca/keel/task/ImportDeliveryConfigTaskTest.java new file mode 100644 index 0000000000..f1b6aa5251 --- /dev/null +++ b/orca-keel/src/test/java/com/netflix/spinnaker/orca/keel/task/ImportDeliveryConfigTaskTest.java @@ -0,0 +1,455 @@ +/* + * Copyright 2024 OpsMx, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.netflix.spinnaker.orca.keel.task; + +import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; +import static com.github.tomakehurst.wiremock.client.WireMock.urlPathEqualTo; +import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig; +import static com.netflix.spinnaker.orca.api.pipeline.models.ExecutionType.PIPELINE; +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Named.named; +import static org.junit.jupiter.params.provider.Arguments.arguments; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.github.tomakehurst.wiremock.client.WireMock; +import com.github.tomakehurst.wiremock.http.Fault; +import com.github.tomakehurst.wiremock.http.HttpHeaders; +import com.github.tomakehurst.wiremock.junit5.WireMockExtension; +import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo; +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler; +import com.netflix.spinnaker.okhttp.SpinnakerRequestInterceptor; +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.StageExecution; +import com.netflix.spinnaker.orca.config.KeelConfiguration; +import com.netflix.spinnaker.orca.igor.ScmService; +import com.netflix.spinnaker.orca.pipeline.model.PipelineExecutionImpl; +import com.netflix.spinnaker.orca.pipeline.model.StageExecutionImpl; +import java.time.Instant; +import java.time.temporal.ChronoUnit; +import java.util.Collections; +import java.util.HashMap; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.stream.Stream; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import org.springframework.http.HttpStatus; +import retrofit.RestAdapter; +import retrofit.RetrofitError; +import retrofit.client.OkClient; +import retrofit.converter.JacksonConverter; + +/* + * @see com.netflix.spinnaker.orca.keel.ImportDeliveryConfigTaskTests.kt already covers up few tests related to @see ImportDeliveryConfigTask. + * This new java class is Introduced to improvise the API testing with the help of wiremock. + * Test using wiremock would help in smooth migration to retrofit2.x along with the addition of {@link SpinnakerRetrofitErrorHandler}. + * */ +public class ImportDeliveryConfigTaskTest { + + private static KeelService keelService; + private static ScmService scmService; + + private static final ObjectMapper objectMapper = new KeelConfiguration().keelObjectMapper(); + + private StageExecution stage; + + private ImportDeliveryConfigTask importDeliveryConfigTask; + + private Map contextMap = new LinkedHashMap<>(); + + private static final int keelPort = 8087; + + @BeforeAll + static void setupOnce(WireMockRuntimeInfo wmRuntimeInfo) { + OkClient okClient = new OkClient(); + RestAdapter.LogLevel retrofitLogLevel = RestAdapter.LogLevel.NONE; + + keelService = + new RestAdapter.Builder() + .setRequestInterceptor(new SpinnakerRequestInterceptor(true)) + .setEndpoint(wmRuntimeInfo.getHttpBaseUrl()) + .setClient(okClient) + .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance()) + .setLogLevel(retrofitLogLevel) + .setConverter(new JacksonConverter(objectMapper)) + .build() + .create(KeelService.class); + } + + @BeforeEach + public void setup() { + scmService = mock(ScmService.class); + importDeliveryConfigTask = new ImportDeliveryConfigTask(keelService, scmService, objectMapper); + + PipelineExecutionImpl pipeline = new PipelineExecutionImpl(PIPELINE, "keeldemo"); + contextMap.put("repoType", "stash"); + contextMap.put("projectKey", "SPKR"); + contextMap.put("repositorySlug", "keeldemo"); + contextMap.put("directory", "."); + contextMap.put("manifest", "spinnaker.yml"); + contextMap.put("ref", "refs/heads/master"); + contextMap.put("attempt", 1); + contextMap.put("maxRetries", 5); + + stage = new StageExecutionImpl(pipeline, ExecutionType.PIPELINE.toString(), contextMap); + } + + @RegisterExtension + static WireMockExtension wireMock = + WireMockExtension.newInstance().options(wireMockConfig().port(keelPort)).build(); + + private static void simulateFault(String url, String body, HttpStatus httpStatus) { + wireMock.givenThat( + WireMock.post(urlPathEqualTo(url)) + .willReturn( + aResponse() + .withHeaders(HttpHeaders.noHeaders()) + .withStatus(httpStatus.value()) + .withBody(body))); + } + + private static void simulateFault(String url, Fault fault) { + wireMock.givenThat(WireMock.post(urlPathEqualTo(url)).willReturn(aResponse().withFault(fault))); + } + + /** + * This test is a positive case which verifies if the task returns {@link + * ImportDeliveryConfigTask.SpringHttpError} on 4xx http error. Here the error body is mocked with + * timestamps in supported Time Units, which will be parsed to exact same timestamp in the + * method @see {@link ImportDeliveryConfigTask#handleRetryableFailures(RetrofitError, + * ImportDeliveryConfigTask.ImportDeliveryConfigContext)} and results in successful assertions of + * all the fields. + * + *

The cases when the timestamp results in accurate value, are when the units in : {@link + * ChronoUnit#MILLIS} {@link ChronoUnit#SECONDS} {@link ChronoUnit#DAYS} {@link ChronoUnit#HOURS} + * {@link ChronoUnit#HALF_DAYS} {@link ChronoUnit#MINUTES} + */ + @ParameterizedTest(name = "{0}") + @MethodSource("parameterizePositiveHttpErrorScenario") + public void verifyPositiveHttpErrorScenarios( + HttpStatus httpStatus, ImportDeliveryConfigTask.SpringHttpError httpError) + throws JsonProcessingException { + + TaskResult expectedTaskResult = + TaskResult.builder(ExecutionStatus.TERMINAL).context(Map.of("error", httpError)).build(); + + // simulate SpringHttpError with http error status code + simulateFault("/delivery-configs/", objectMapper.writeValueAsString(httpError), httpStatus); + + getDeliveryConfigManifest(); + + var result = importDeliveryConfigTask.execute(stage); + + verifyGetDeliveryConfigManifestInvocations(); + + assertThat(expectedTaskResult).isEqualTo(result); + } + + /** + * This test is a negative case which verifies if the task returns {@link + * ImportDeliveryConfigTask.SpringHttpError} on 4xx http error. Here the error body is mocked with + * timestamp in Time Units that are unsupported, which WILL NOT be parsed to exact timestamp in + * the method @see {@link ImportDeliveryConfigTask#handleRetryableFailures(RetrofitError, + * ImportDeliveryConfigTask.ImportDeliveryConfigContext)} and results in will contain incorrect + * timestamp. + * + *

The cases where the timestamp will result in incorrect value, are when the units in : {@link + * ChronoUnit#NANOS} {@link ChronoUnit#MICROS} + */ + @ParameterizedTest(name = "{0}") + @MethodSource("parameterizeNegativeHttpErrorScenario") + public void verifyNegativeHttpErrorScenarios( + HttpStatus httpStatus, ImportDeliveryConfigTask.SpringHttpError httpError) + throws JsonProcessingException { + + // simulate SpringHttpError with http error status code + simulateFault("/delivery-configs/", objectMapper.writeValueAsString(httpError), httpStatus); + + getDeliveryConfigManifest(); + + var result = importDeliveryConfigTask.execute(stage); + ImportDeliveryConfigTask.SpringHttpError actualHttpErrorBody = + (ImportDeliveryConfigTask.SpringHttpError) result.getContext().get("error"); + + verifyGetDeliveryConfigManifestInvocations(); + + // assert all the values in the http error body are true except the timestamp in nanos + assertThat(actualHttpErrorBody.getStatus()).isEqualTo(httpStatus.value()); + assertThat(actualHttpErrorBody.getError()).isEqualTo(httpStatus.getReasonPhrase()); + assertThat(actualHttpErrorBody.getMessage()).isEqualTo(httpStatus.name()); + assertThat(actualHttpErrorBody.getDetails()) + .isEqualTo(Map.of("exception", "Http Error occurred")); + assertThat(actualHttpErrorBody.getTimestamp().getEpochSecond()) + .isEqualTo(httpError.getTimestamp().getEpochSecond()); + assertThat(actualHttpErrorBody.getTimestamp().getNano()) + .isNotEqualTo(httpError.getTimestamp().getNano()); + } + + /** + * Test to verify when the response body doesn't have timestamp. Field will be initialized with + * default value {@link Instant#now} + */ + @Test + public void testSpringHttpErrorWithoutTimestamp() throws JsonProcessingException { + + var httpStatus = HttpStatus.BAD_REQUEST; + + // Map of SpringHttpError is initialized without timestamp + var httpError = mapOfSpringHttpError(httpStatus); + + // simulate SpringHttpError with http error status code + simulateFault("/delivery-configs/", objectMapper.writeValueAsString(httpError), httpStatus); + + getDeliveryConfigManifest(); + + var result = importDeliveryConfigTask.execute(stage); + ImportDeliveryConfigTask.SpringHttpError actualHttpErrorBody = + (ImportDeliveryConfigTask.SpringHttpError) result.getContext().get("error"); + + verifyGetDeliveryConfigManifestInvocations(); + + assertThat(actualHttpErrorBody.getStatus()).isEqualTo(httpStatus.value()); + assertThat(actualHttpErrorBody.getError()).isEqualTo(httpStatus.getReasonPhrase()); + assertThat(actualHttpErrorBody.getMessage()).isEqualTo(httpStatus.name()); + assertThat(actualHttpErrorBody.getDetails()) + .isEqualTo(Map.of("exception", "Http Error occurred")); + + // The timestamp field will have the current time, and hence only the instance type is verified + assertThat(actualHttpErrorBody.getTimestamp()).isInstanceOf(Instant.class); + } + + @Test + public void testTaskResultWhenErrorBodyIsEmpty() { + + String expectedMessage = + String.format( + "Non-retryable HTTP response %s received from downstream service: %s", + HttpStatus.BAD_REQUEST.value(), + "HTTP 400 " + + wireMock.baseUrl() + + "/delivery-configs/: Status: 400, URL: " + + wireMock.baseUrl() + + "/delivery-configs/, Message: Bad Request"); + + var errorMap = new HashMap<>(); + errorMap.put("message", expectedMessage); + + TaskResult terminal = + TaskResult.builder(ExecutionStatus.TERMINAL).context(Map.of("error", errorMap)).build(); + + // Simulate any 4xx http error with empty error response body + String emptyBody = ""; + simulateFault("/delivery-configs/", emptyBody, HttpStatus.BAD_REQUEST); + + getDeliveryConfigManifest(); + + var result = importDeliveryConfigTask.execute(stage); + + verifyGetDeliveryConfigManifestInvocations(); + + assertThat(result).isEqualTo(terminal); + } + + @Test + public void testTaskResultWhenHttp5xxErrorIsThrown() { + + contextMap.put("attempt", (Integer) contextMap.get("attempt") + 1); + contextMap.put( + "errorFromLastAttempt", + "Retryable HTTP response 500 received from downstream service: HTTP 500 " + + wireMock.baseUrl() + + "/delivery-configs/: Status: 500, URL: " + + wireMock.baseUrl() + + "/delivery-configs/, Message: Server Error"); + + TaskResult running = TaskResult.builder(ExecutionStatus.RUNNING).context(contextMap).build(); + + // Simulate any 5xx http error with empty error response body + String emptyBody = ""; + simulateFault("/delivery-configs/", emptyBody, HttpStatus.INTERNAL_SERVER_ERROR); + + getDeliveryConfigManifest(); + + var result = importDeliveryConfigTask.execute(stage); + + verifyGetDeliveryConfigManifestInvocations(); + + assertThat(result).isEqualTo(running); + } + + @Test + public void testTaskResultWhenAPIFailsWithNetworkError() { + + contextMap.put("attempt", (Integer) contextMap.get("attempt") + 1); + contextMap.put( + "errorFromLastAttempt", + String.format( + "Network error talking to downstream service, attempt 1 of %s: Connection reset: Connection reset", + contextMap.get("maxRetries"))); + + TaskResult running = TaskResult.builder(ExecutionStatus.RUNNING).context(contextMap).build(); + + // Simulate network failure + simulateFault("/delivery-configs/", Fault.CONNECTION_RESET_BY_PEER); + + getDeliveryConfigManifest(); + + var result = importDeliveryConfigTask.execute(stage); + + verifyGetDeliveryConfigManifestInvocations(); + + assertThat(result).isEqualTo(running); + } + + private static Stream parameterizePositiveHttpErrorScenario() { + + HttpStatus httpStatus = HttpStatus.BAD_REQUEST; + + // Initialize SpringHttpError with timestamp in milliseconds and HttpStatus 400 bad request. + var httpErrorTimestampInMillis = + makeSpringHttpError(httpStatus, Instant.now().truncatedTo(ChronoUnit.MILLIS)); + + // Initialize SpringHttpError with timestamp in seconds and HttpStatus 400 bad request. + var httpErrorTimestampInSeconds = + makeSpringHttpError(httpStatus, Instant.now().truncatedTo(ChronoUnit.SECONDS)); + + // Initialize SpringHttpError with timestamp in minutes and HttpStatus 400 bad request. + var httpErrorTimestampInMinutes = + makeSpringHttpError(httpStatus, Instant.now().truncatedTo(ChronoUnit.MINUTES)); + + // Initialize SpringHttpError with timestamp in hours and HttpStatus 400 bad request. + var httpErrorTimestampInHours = + makeSpringHttpError(httpStatus, Instant.now().truncatedTo(ChronoUnit.HOURS)); + + // Initialize SpringHttpError with timestamp in days and HttpStatus 400 bad request. + var httpErrorTimestampInDays = + makeSpringHttpError(httpStatus, Instant.now().truncatedTo(ChronoUnit.DAYS)); + + // Initialize SpringHttpError with timestamp in half days and HttpStatus 400 bad request. + var httpErrorTimestampInHalfDays = + makeSpringHttpError(httpStatus, Instant.now().truncatedTo(ChronoUnit.HALF_DAYS)); + + return Stream.of( + arguments( + named("http error with timestamp in " + ChronoUnit.MILLIS.name(), httpStatus), + httpErrorTimestampInMillis), + arguments( + named("http error with timestamp in " + ChronoUnit.SECONDS.name(), httpStatus), + httpErrorTimestampInSeconds), + arguments( + named("http error with timestamp in " + ChronoUnit.MINUTES.name(), httpStatus), + httpErrorTimestampInMinutes), + arguments( + named("http error with timestamp in " + ChronoUnit.HOURS.name(), httpStatus), + httpErrorTimestampInHours), + arguments( + named("http error with timestamp in " + ChronoUnit.DAYS.name(), httpStatus), + httpErrorTimestampInDays), + arguments( + named("http error with timestamp in " + ChronoUnit.HALF_DAYS.name(), httpStatus), + httpErrorTimestampInHalfDays)); + } + + private static Stream parameterizeNegativeHttpErrorScenario() { + + HttpStatus httpStatus = HttpStatus.BAD_REQUEST; + + // Initialize SpringHttpError with timestamp in milliseconds and HttpStatus 400 bad request. + var httpErrorTimestampInNanos = + makeSpringHttpError(httpStatus, Instant.now().truncatedTo(ChronoUnit.NANOS)); + + // Initialize SpringHttpError with timestamp in seconds and HttpStatus 400 bad request. + var httpErrorTimestampInMicros = + makeSpringHttpError(httpStatus, Instant.now().truncatedTo(ChronoUnit.MICROS)); + + return Stream.of( + arguments( + named("http error with timestamp in " + ChronoUnit.NANOS.name(), httpStatus), + httpErrorTimestampInNanos), + arguments( + named("http error with timestamp in " + ChronoUnit.MICROS.name(), httpStatus), + httpErrorTimestampInMicros)); + } + + private void getDeliveryConfigManifest() { + when(scmService.getDeliveryConfigManifest( + (String) contextMap.get("repoType"), + (String) contextMap.get("projectKey"), + (String) contextMap.get("repositorySlug"), + (String) contextMap.get("directory"), + (String) contextMap.get("manifest"), + (String) contextMap.get("ref"))) + .thenReturn( + Map.of( + "name", + "keeldemo-manifest", + "application", + "keeldemo", + "artifacts", + Collections.emptySet(), + "environments", + Collections.emptySet())); + } + + private static ImportDeliveryConfigTask.SpringHttpError makeSpringHttpError( + HttpStatus httpStatus, Instant timestamp) { + + return new ImportDeliveryConfigTask.SpringHttpError( + httpStatus.getReasonPhrase(), + httpStatus.value(), + httpStatus.name(), + timestamp, + Map.of("exception", "Http Error occurred")); + } + + private static Map mapOfSpringHttpError(HttpStatus httpStatus) { + + return Map.of( + "error", + httpStatus.getReasonPhrase(), + "status", + httpStatus.value(), + "message", + httpStatus.name(), + "details", + Map.of("exception", "Http Error occurred")); + } + + private void verifyGetDeliveryConfigManifestInvocations() { + verify(scmService, times(1)) + .getDeliveryConfigManifest( + (String) contextMap.get("repoType"), + (String) contextMap.get("projectKey"), + (String) contextMap.get("repositorySlug"), + (String) contextMap.get("directory"), + (String) contextMap.get("manifest"), + (String) contextMap.get("ref")); + } +} 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 0104af4c42..39ea5b5efa 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 @@ -30,7 +30,6 @@ import com.netflix.spinnaker.orca.igor.ScmService import com.netflix.spinnaker.orca.keel.model.DeliveryConfig 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 @@ -41,21 +40,14 @@ import io.mockk.every import io.mockk.mockk import io.mockk.slot import io.mockk.verify -import org.springframework.http.HttpStatus.BAD_REQUEST -import org.springframework.http.HttpStatus.FORBIDDEN 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.isNotEqualTo import strikt.assertions.isNotNull -import java.time.Instant -import java.time.temporal.ChronoUnit internal class ImportDeliveryConfigTaskTests : JUnit5Minutests { data class ManifestLocation( @@ -111,34 +103,6 @@ internal class ImportDeliveryConfigTaskTests : JUnit5Minutests { context ) ) - - val parsingError = SpringHttpError( - status = BAD_REQUEST.value(), - error = BAD_REQUEST.reasonPhrase, - message = "Parsing error", - details = mapOf( - "message" to "Parsing error", - "path" to listOf( - mapOf( - "type" to "SomeClass", - "field" to "someField" - ) - ), - "pathExpression" to ".someField" - ), - // Jackson writes this as ms-since-epoch, so we need to strip off the nanoseconds, since we'll - // be round-tripping it through Jackson before testing for equality. - timestamp = Instant.now().truncatedTo(ChronoUnit.MILLIS) - ) - - val accessDeniedError = SpringHttpError( - status = FORBIDDEN.value(), - error = FORBIDDEN.reasonPhrase, - message = "Access denied", - // Jackson writes this as ms-since-epoch, so we need to strip off the nanoseconds, since we'll - // be round-tripping it through Jackson before testing for equality. - timestamp = Instant.now().truncatedTo(ChronoUnit.MILLIS) - ) } private fun ManifestLocation.toMap() = @@ -365,66 +329,6 @@ internal class ImportDeliveryConfigTaskTests : JUnit5Minutests { } } - 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().isEqualTo(accessDeniedError) - } - } - - 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) {