From ca3db121c841c22f97e6a41012a2bbf69ac0aa13 Mon Sep 17 00:00:00 2001 From: Pranav Bhaskaran <67958542+Pranav-b-7@users.noreply.github.com> Date: Tue, 27 Feb 2024 23:40:37 +0530 Subject: [PATCH] refactor(keel): use SpinnakerRetrofitErrorHandler with KeelService (#4636) * test(keel): Test to verify the timestamps in SpringHttpError across different time units when any 4xx http error has occured. These tests ensures the upcoming changes on KeelService with SpinnakerRetrofitErrorHandler, will not modify/break the existing functionality. Tests covers positive and negative cases with different units of timestamps. In addition, another test added to verify when the error response body doesn't have timestamp field. * test(keel): demonstrate behavior of ImportDeliveryConfigTask on http errors and network errors Test cases added to verify the upcoming changes when KeelService is configured with SpinnakerRetrofitErrorHandler. These tests verifies the behaviour of the method handleRetryableFailures() when 4xx with empty error body and 5xx http erros are thrown, and also during network errors. NOTE : These tests only verifies the scenario when the error response body is empty/null, and network errors. * test(keel): remove duplicate tests from ImportDeliveryConfigTaskTests The deleted tests are the cases/scenarios which are already covered as part of the commit: 5d711ab18a2c26c3420c55ffcd89af0c1ed67d4f. These tests covers up the http 4xx error cases of the API : keelService.publishDeliveryConfig. Tests deleted are : 1. 'keel access denied error', 2. 'delivery config parsing error'. * refactor(keel): use SpinnakerRetrofitErrorHandler with KeelService This PR lays the foundational work for upgrading the retrofit version to 2.x, specifically focusing on refactoring the exception handling for KeelService The tests modified as part of this PR will verify the new changes with the scenarios:- Reading the Http error response body and building the TaskResult by instantiating SpringHttpError. Note, there's a behaviour change on the Task Results error message format when KeelService API throws any 4xx/5xx http errors with empty error body. - On any 4xx http errors with empty error body: before: 11:56:19.324 [Test worker] ERROR com.netflix.spinnaker.orca.keel.task.ImportDeliveryConfigTask - {message=Non-retryable HTTP response 400 received from downstream service: HTTP 400 http://localhost:62130/delivery-configs/: 400 Bad Request} after: 12:00:02.018 [Test worker] ERROR com.netflix.spinnaker.orca.keel.task.ImportDeliveryConfigTask - {message=Non-retryable HTTP response 400 received from downstream service: HTTP 400 http://localhost:62275/delivery-configs/: Status: 400, URL: http://localhost:62275/delivery-configs/, Message: Bad Request} - On any 5xx http errors with empty error body: before: TaskResult(status=RUNNING, context={repoType=stash, projectKey=SPKR, repositorySlug=keeldemo, directory=., manifest=spinnaker.yml, ref=refs/heads/master, attempt=2, maxRetries=5, errorFromLastAttempt=Retryable HTTP response 500 received from downstream service: HTTP 500 http://localhost:65311/delivery-configs/: 500 Server Error}, outputs={}) after: TaskResult(status=RUNNING, context={repoType=stash, projectKey=SPKR, repositorySlug=keeldemo, directory=., manifest=spinnaker.yml, ref=refs/heads/master, attempt=1, maxRetries=5, errorFromLastAttempt=Retryable HTTP response 500 received from downstream service: HTTP 500 http://localhost:49862/delivery-configs/: Status: 500, URL: http://localhost:49862/delivery-configs/, Message: Server Error}, outputs={}) --- .../tasks/DeleteApplicationTask.groovy | 11 + orca-keel/orca-keel.gradle | 3 + .../orca/config/KeelConfiguration.kt | 2 + .../keel/task/ImportDeliveryConfigTask.kt | 97 +++- .../task/ImportDeliveryConfigTaskTest.java | 455 ++++++++++++++++++ .../keel/ImportDeliveryConfigTaskTests.kt | 96 ---- 6 files changed, 567 insertions(+), 97 deletions(-) create mode 100644 orca-keel/src/test/java/com/netflix/spinnaker/orca/keel/task/ImportDeliveryConfigTaskTest.java 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) {