From d8e3700d8bbea396ca3af163aaad400ae884ee84 Mon Sep 17 00:00:00 2001 From: kirangodishala Date: Thu, 13 Apr 2023 02:28:08 +0530 Subject: [PATCH] fix(web): invoke pipeline config exception handling * introduce new configuration property services.front50.applicationRefreshInitialDelayMs which provides an initial delay in milliseconds for the thread that refreshes the applications cache in ApplicationService It's primarily to facilitate testing, but it seems reasonable someone might want use it production to keep things quiet at startup. * demonstrate current behavior of PipelineController.invokePipelineConfig as determined by wiremock responses from front50 and orca (InvokePipelineConfigTest) and when PipelineService.trigger throws an exception (PipelineControllerTest) * let exceptions during PipelineController.invokePipelineConfig bubble up so gate's http response code more closely reflects what happened * change PipelineController to use constructor autowiring to prepare for changes to the constructor logic * include information from downstream services in error responses from PipelineController.invokePipelineConfig by handling RetrofitErrors with SpinnakerRetrofitErrorHandler * As part of this PipelineController.invokePipelineConfig no longer logs its own message for RetrofitErrors. There's some loss of information with this, as the initiator of the downstream communication is from no longer clear. A subsequent commit restores this. * chain Spinnaker*Exceptions in PipelineController.invokePipelineConfig so it's clear which operation is failing. This improves both logging and gate's http response. * As part of this, remove the no-op catch and throw for NotFoundException. With no other more general catch block, this code isn't necessary. --- gate-web/gate-web.gradle | 2 +- .../controllers/PipelineController.groovy | 87 ++- .../gate/services/ApplicationService.groovy | 3 +- .../spinnaker/gate/FunctionalSpec.groovy | 8 +- .../controllers/PipelineControllerSpec.groovy | 11 +- .../controllers/InvokePipelineConfigTest.java | 599 ++++++++++++++++++ .../controllers/PipelineControllerTest.java | 124 ++++ 7 files changed, 799 insertions(+), 35 deletions(-) create mode 100644 gate-web/src/test/java/com/netflix/spinnaker/gate/controllers/InvokePipelineConfigTest.java create mode 100644 gate-web/src/test/java/com/netflix/spinnaker/gate/controllers/PipelineControllerTest.java diff --git a/gate-web/gate-web.gradle b/gate-web/gate-web.gradle index d4cec83826..e68f1fe67b 100644 --- a/gate-web/gate-web.gradle +++ b/gate-web/gate-web.gradle @@ -32,6 +32,7 @@ dependencies { implementation "io.spinnaker.kork:kork-core" implementation "io.spinnaker.kork:kork-config" implementation "io.spinnaker.kork:kork-plugins" + implementation "io.spinnaker.kork:kork-retrofit" implementation "io.spinnaker.kork:kork-web" implementation "com.netflix.frigga:frigga" implementation "redis.clients:jedis" @@ -78,7 +79,6 @@ dependencies { testImplementation "io.spinnaker.kork:kork-jedis-test" testImplementation "io.spinnaker.kork:kork-test" testImplementation "org.codehaus.groovy:groovy-json" - testRuntimeOnly "io.spinnaker.kork:kork-retrofit" // Add each included authz provider as a runtime dependency gradle.includedProviderProjects.each { diff --git a/gate-web/src/main/groovy/com/netflix/spinnaker/gate/controllers/PipelineController.groovy b/gate-web/src/main/groovy/com/netflix/spinnaker/gate/controllers/PipelineController.groovy index ea2dd033a9..9e9690e770 100644 --- a/gate-web/src/main/groovy/com/netflix/spinnaker/gate/controllers/PipelineController.groovy +++ b/gate-web/src/main/groovy/com/netflix/spinnaker/gate/controllers/PipelineController.groovy @@ -23,7 +23,9 @@ import com.netflix.spinnaker.gate.services.PipelineService import com.netflix.spinnaker.gate.services.TaskService import com.netflix.spinnaker.gate.services.internal.Front50Service import com.netflix.spinnaker.kork.exceptions.HasAdditionalAttributes -import com.netflix.spinnaker.kork.web.exceptions.InvalidRequestException +import com.netflix.spinnaker.kork.exceptions.SpinnakerException +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler import com.netflix.spinnaker.kork.web.exceptions.NotFoundException import com.netflix.spinnaker.security.AuthenticatedRequest import groovy.transform.CompileDynamic @@ -51,20 +53,44 @@ import static net.logstash.logback.argument.StructuredArguments.value @RestController @RequestMapping("/pipelines") class PipelineController { - @Autowired - PipelineService pipelineService - - @Autowired - TaskService taskService + final PipelineService pipelineService + final TaskService taskService + final Front50Service front50Service + final ObjectMapper objectMapper + final PipelineControllerConfigProperties pipelineControllerConfigProperties + + /** + * Adjusting the front50Service and other retrofit objects for communicating + * with downstream services means changing RetrofitServiceFactory in kork and + * so it affects more than gate. Front50 uses that code to communicate with + * echo. Front50 doesn't currently do any special exception handling when it + * calls echo. Gate does a ton though, and so it would be a big change to + * adjust all the catching of RetrofitError into catching + * SpinnakerHttpException, etc. as appropriate. + * + * Even if RetrofitServiceFactory were configurable by service type, so only + * gate's Front50Service and OrcaService used SpinnakerRetrofitErrorHandler, + * it would still be a big change, affecting gate-iap and gate-oauth2 where + * there's code that uses front50Service but checks for RetrofitError. + * + * To limit the scope of the change to invokePipelineConfig, construct a + * spinnakerRetrofitErrorHandler and use it directly. + */ + final SpinnakerRetrofitErrorHandler spinnakerRetrofitErrorHandler @Autowired - Front50Service front50Service - - @Autowired - ObjectMapper objectMapper - - @Autowired - PipelineControllerConfigProperties pipelineControllerConfigProperties + PipelineController(PipelineService pipelineService, + TaskService taskService, + Front50Service front50Service, + ObjectMapper objectMapper, + PipelineControllerConfigProperties pipelineControllerConfigProperties) { + this.pipelineService = pipelineService + this.taskService = taskService + this.front50Service = front50Service + this.objectMapper = objectMapper + this.pipelineControllerConfigProperties = pipelineControllerConfigProperties + this.spinnakerRetrofitErrorHandler = SpinnakerRetrofitErrorHandler.newInstance() + } @CompileDynamic @ApiOperation(value = "Delete a pipeline definition") @@ -300,15 +326,30 @@ class PipelineController { AuthenticatedRequest.setApplication(application) try { pipelineService.trigger(application, pipelineNameOrId, trigger) - } catch (NotFoundException e) { - throw e - } catch (e) { - log.error("Unable to trigger pipeline (application: {}, pipelineId: {})", - value("application", application), value("pipelineId", pipelineNameOrId), e) - throw new PipelineExecutionException(e.message) + } catch (RetrofitError e) { + // If spinnakerRetrofitErrorHandler were registered as a "real" error handler, the code here would look something like + // + // } catch (SpinnakerHttpException e) { + // throw new SpinnakerHttpException(triggerFailureMessage(application, pipelineNameOrId, e), e) + // } catch (SpinnakerException e) { + // throw new SpinnakerException(triggerFailureMessage(application, pipelineNameOrId, e), e) + // } + Throwable throwable = spinnakerRetrofitErrorHandler.handleError(e) + if (throwable instanceof SpinnakerHttpException) { + throw new SpinnakerHttpException(triggerFailureMessage(application, pipelineNameOrId, throwable), throwable) + } + if (throwable instanceof SpinnakerException) { + throw new SpinnakerException(triggerFailureMessage(application, pipelineNameOrId, throwable), throwable) + } + throw throwable } } + private String triggerFailureMessage(String application, String pipelineNameOrId, Throwable e) { + String.format("Unable to trigger pipeline (application: %s, pipelineId: %s). Error: %s", + value("application", application), value("pipelineId", pipelineNameOrId), e.getMessage()) + } + @ApiOperation(value = "Trigger a pipeline execution", response = Map.class) @PreAuthorize("hasPermission(#application, 'APPLICATION', 'EXECUTE')") @PostMapping("/v2/{application}/{pipelineNameOrId:.+}") @@ -429,12 +470,4 @@ class PipelineController { this.additionalAttributes = additionalAttributes } } - - @ResponseStatus(HttpStatus.UNPROCESSABLE_ENTITY) - @InheritConstructors - class PipelineExecutionException extends InvalidRequestException { - PipelineExecutionException(String message) { - super(message) - } - } } diff --git a/gate-web/src/main/groovy/com/netflix/spinnaker/gate/services/ApplicationService.groovy b/gate-web/src/main/groovy/com/netflix/spinnaker/gate/services/ApplicationService.groovy index 8e97fb6797..9432f11b39 100644 --- a/gate-web/src/main/groovy/com/netflix/spinnaker/gate/services/ApplicationService.groovy +++ b/gate-web/src/main/groovy/com/netflix/spinnaker/gate/services/ApplicationService.groovy @@ -73,7 +73,8 @@ class ApplicationService { this.allApplicationsCache } - @Scheduled(fixedDelayString = '${services.front50.applicationRefreshIntervalMs:5000}') + @Scheduled(fixedDelayString = '${services.front50.applicationRefreshIntervalMs:5000}', + initialDelayString = '${services.front50.applicationRefreshInitialDelayMs:}') void refreshApplicationsCache() { try { allApplicationsCache.set(tick(true)) diff --git a/gate-web/src/test/groovy/com/netflix/spinnaker/gate/FunctionalSpec.groovy b/gate-web/src/test/groovy/com/netflix/spinnaker/gate/FunctionalSpec.groovy index 159e3feb40..9d6bad1beb 100644 --- a/gate-web/src/test/groovy/com/netflix/spinnaker/gate/FunctionalSpec.groovy +++ b/gate-web/src/test/groovy/com/netflix/spinnaker/gate/FunctionalSpec.groovy @@ -259,8 +259,12 @@ class FunctionalSpec extends Specification { } @Bean - PipelineController pipelineController() { - new PipelineController() + PipelineController pipelineController(PipelineService pipelineService, + TaskService taskService, + Front50Service front50Service, + ObjectMapper objectMapper, + PipelineControllerConfigProperties pipelineControllerConfigProperties) { + new PipelineController(pipelineService, taskService, front50Service, objectMapper, pipelineControllerConfigProperties) } @Bean diff --git a/gate-web/src/test/groovy/com/netflix/spinnaker/gate/controllers/PipelineControllerSpec.groovy b/gate-web/src/test/groovy/com/netflix/spinnaker/gate/controllers/PipelineControllerSpec.groovy index e30720592d..0358152798 100644 --- a/gate-web/src/test/groovy/com/netflix/spinnaker/gate/controllers/PipelineControllerSpec.groovy +++ b/gate-web/src/test/groovy/com/netflix/spinnaker/gate/controllers/PipelineControllerSpec.groovy @@ -18,6 +18,7 @@ package com.netflix.spinnaker.gate.controllers import com.fasterxml.jackson.databind.ObjectMapper import com.netflix.spinnaker.gate.config.controllers.PipelineControllerConfigProperties +import com.netflix.spinnaker.gate.services.PipelineService import com.netflix.spinnaker.gate.services.TaskService import com.netflix.spinnaker.gate.services.internal.Front50Service import groovy.json.JsonSlurper @@ -39,12 +40,14 @@ class PipelineControllerSpec extends Specification { def taskSerivce = Mock(TaskService) def front50Service = Mock(Front50Service) + def pipelineService = Mock(PipelineService) def pipelineControllerConfig = new PipelineControllerConfigProperties() def mockMvc = MockMvcBuilders - .standaloneSetup(new PipelineController(objectMapper: new ObjectMapper(), - taskService: taskSerivce, - front50Service: front50Service, - pipelineControllerConfigProperties: pipelineControllerConfig)) + .standaloneSetup(new PipelineController(pipelineService, + taskSerivce, + front50Service, + new ObjectMapper(), + pipelineControllerConfig)) .build() def "should update a pipeline"() { diff --git a/gate-web/src/test/java/com/netflix/spinnaker/gate/controllers/InvokePipelineConfigTest.java b/gate-web/src/test/java/com/netflix/spinnaker/gate/controllers/InvokePipelineConfigTest.java new file mode 100644 index 0000000000..b3891fdafc --- /dev/null +++ b/gate-web/src/test/java/com/netflix/spinnaker/gate/controllers/InvokePipelineConfigTest.java @@ -0,0 +1,599 @@ +/* + * Copyright 2023 Salesforce, 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.gate.controllers; + +import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; +import static com.github.tomakehurst.wiremock.client.WireMock.equalTo; +import static com.github.tomakehurst.wiremock.client.WireMock.exactly; +import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor; +import static com.github.tomakehurst.wiremock.client.WireMock.postRequestedFor; +import static com.github.tomakehurst.wiremock.client.WireMock.urlMatching; +import static com.github.tomakehurst.wiremock.client.WireMock.urlPathEqualTo; +import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig; +import static com.netflix.spinnaker.kork.common.Header.ACCOUNTS; +import static com.netflix.spinnaker.kork.common.Header.REQUEST_ID; +import static com.netflix.spinnaker.kork.common.Header.USER; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.Mockito.when; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; +import static org.springframework.test.web.servlet.result.MockMvcResultHandlers.print; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.header; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; +import static org.springframework.test.web.servlet.setup.MockMvcBuilders.webAppContextSetup; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.github.tomakehurst.wiremock.client.ResponseDefinitionBuilder; +import com.github.tomakehurst.wiremock.client.WireMock; +import com.github.tomakehurst.wiremock.http.Fault; +import com.github.tomakehurst.wiremock.junit5.WireMockExtension; +import com.github.tomakehurst.wiremock.matching.AnythingPattern; +import com.netflix.spinnaker.gate.Main; +import com.netflix.spinnaker.gate.health.DownstreamServicesHealthIndicator; +import com.netflix.spinnaker.gate.services.internal.ClouddriverService; +import com.netflix.spinnaker.gate.services.internal.ClouddriverServiceSelector; +import java.nio.charset.StandardCharsets; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInfo; +import org.junit.jupiter.api.extension.RegisterExtension; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Qualifier; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.test.mock.mockito.MockBean; +import org.springframework.boot.web.servlet.FilterRegistrationBean; +import org.springframework.http.HttpStatus; +import org.springframework.http.MediaType; +import org.springframework.test.context.DynamicPropertyRegistry; +import org.springframework.test.context.DynamicPropertySource; +import org.springframework.test.context.TestPropertySource; +import org.springframework.test.web.servlet.MockMvc; +import org.springframework.test.web.servlet.RequestBuilder; +import org.springframework.web.context.WebApplicationContext; + +/** + * This is a bit of an end-to-end test. It demonstrates the behavior of + * PipelineController.invokePipelineConfig and PipelineService.trigger based on particular responses + * (or lack thereof) from front50, clouddriver and orca. This facilitates seeing the consequences of + * changing things like exception handlers in http clients in the code without changing the tests. + * + *

See https://github.com/spring-projects/spring-boot/issues/5574#issuecomment-506282892 for a + * discussion of why it's difficult to assert on response bodies generated by error handlers. It's + * not necessary in these tests, as the information we need is available elsewhere (e.g. the http + * status line). + */ +// Without an initial delay, the ApplicationService queries for information +// before our @BeforeEach method runs and sets up the appropriate mocks. This +// results in NullPointerExceptions. +@SpringBootTest(classes = Main.class) +@TestPropertySource( + properties = { + "spring.config.location=classpath:gate-test.yml", + "services.front50.applicationRefreshInitialDelayMs=3600000" + }) +class InvokePipelineConfigTest { + + private MockMvc webAppMockMvc; + + /** See https://wiremock.org/docs/junit-jupiter/#advanced-usage---programmatic */ + @RegisterExtension + static WireMockExtension wmFront50 = + WireMockExtension.newInstance().options(wireMockConfig().dynamicPort()).build(); + + @RegisterExtension + static WireMockExtension wmOrca = + WireMockExtension.newInstance().options(wireMockConfig().dynamicPort()).build(); + + @Autowired private WebApplicationContext webApplicationContext; + + @Autowired ObjectMapper objectMapper; + + /** + * This takes X-SPINNAKER-* headers from requests to gate and puts them in the MDC. This is + * enabled when gate runs normally (by GateConfig), but needs explicit mention to function in + * these tests. + */ + @Autowired + @Qualifier("authenticatedRequestFilter") + private FilterRegistrationBean filterRegistrationBean; + + @MockBean ClouddriverServiceSelector clouddriverServiceSelector; + + @MockBean ClouddriverService clouddriverService; + + /** To prevent periodic calls to service's /health endpoints */ + @MockBean DownstreamServicesHealthIndicator downstreamServicesHealthIndicator; + + private static final String APPLICATION = "my-application"; + private static final String PIPELINE_ID = "my-pipeline-id"; + private static final String USERNAME = "some user"; + private static final String ACCOUNT = "my-account"; + private static final String SUBMITTED_REQUEST_ID = "submitted-request-id"; + + /** + * Using an empty map as the trigger is mostly arbitrary. Without a user in the trigger, expect + * the user query parameter in the request to orca to be the user we provide via the header. + */ + private final Map TRIGGER = Collections.emptyMap(); + + private final AnythingPattern anythingPattern = new AnythingPattern(); + + @DynamicPropertySource + static void registerUrls(DynamicPropertyRegistry registry) { + // Configure wiremock's random ports into gate + System.out.println("wiremock front50 url: " + wmFront50.baseUrl()); + System.out.println("wiremock orca url: " + wmOrca.baseUrl()); + registry.add("services.front50.base-url", wmFront50::baseUrl); + registry.add("services.orca.base-url", wmOrca::baseUrl); + } + + @BeforeEach + void init(TestInfo testInfo) throws JsonProcessingException { + System.out.println("--------------- Test " + testInfo.getDisplayName()); + + webAppMockMvc = + webAppContextSetup(webApplicationContext) + .addFilters(filterRegistrationBean.getFilter()) + .build(); + + // Keep the background thread that refreshes the applications cache in + // ApplicationService happy. + when(clouddriverServiceSelector.select()).thenReturn(clouddriverService); + when(clouddriverService.getAllApplicationsUnrestricted(anyBoolean())) + .thenReturn(Collections.emptyList()); + + // That background thread also calls a front50 endpoint, separate from the ones used in + // invokePipelineConfig + List> allApplicationsFromFront50 = Collections.emptyList(); + String allApplicationsFromFront50Json = + objectMapper.writeValueAsString(allApplicationsFromFront50); + wmFront50.stubFor( + WireMock.put(urlMatching("/v2/applications")) + .willReturn( + aResponse() + .withStatus(HttpStatus.OK.value()) + .withBody(allApplicationsFromFront50Json))); + } + + @Test + void invokePipelineConfigSuccess() throws Exception { + // As a baseline, demonstrate the behavior when both front50 and orca respond with success. + simulateFront50Success(); + simulateOrcaSuccess(); + + // The response body from gate is the response from orca, which we simulate. + webAppMockMvc + .perform(invokePipelineConfigRequest()) + .andDo(print()) + .andExpect(status().isAccepted()) + .andExpect(header().string(REQUEST_ID.getHeader(), SUBMITTED_REQUEST_ID)) + .andExpect(content().string(orcaSuccessResponse())); + + verifyFront50PipelinesRequest(); + verifyOrcaOrchestrateRequest(); + } + + @Test + void invokePipelineConfigNoPipelineConfigInFront50() throws Exception { + + // If front50 doesn't return a configuration for the test pipeline id, don't + // expect a call to orca, so there's no mock to set up for orca. + simulateFront50SuccessWithoutPipelineConfig(); + + webAppMockMvc + .perform(invokePipelineConfigRequest()) + .andDo(print()) + .andExpect(status().isNotFound()) + .andExpect(status().reason("Pipeline configuration not found (id: " + PIPELINE_ID + ")")) + .andExpect(header().string(REQUEST_ID.getHeader(), SUBMITTED_REQUEST_ID)); + + verifyFront50PipelinesRequest(); + verifyNoOrcaOrchestrateRequest(); + } + + @Test + void invokePipelineConfig404ResponseFromFront50() throws Exception { + + // Different than a 200 from front50 without the requested info, this test + // is about front50 responding with a 404. Currently, front50's GET + // /pipelines/{app} endpoint doesn't generate a 404, but a request to an + // endpoint that doesn't exist does. For example: + // + // $ curl -s http://localhost:8080/endpointDoesNotExist | python -mjson.tool + // { + // "error": "Not Found", + // "message": "No message available", + // "status": 404, + // "timestamp": "2023-04-07T17:51:22.560+00:00" + // } + // + // and + // + // $ curl -s -i http://localhost:8080/endpointDoesNotExist | head -1 + // HTTP/1.1 404 + Map front50Response = + Map.of( + "error", + "Not Found", + "message", + "message from front50", + "status", + 404, + "timestamp", + "2023-04-07T17:51:22.560+00:00"); + String front50ResponseJson = objectMapper.writeValueAsString(front50Response); + simulateFront50HttpResponse(HttpStatus.NOT_FOUND, front50ResponseJson); + + // gate's 404 response in this case makes it difficult to distinguish + // between a 404 because the endpoint itself doesn't exist and a "real" 404 + // -- one where all the requests are correct, but front50 doesn't know about + // the pipeline. This seems fairly minor, and callers who want to + // distinguish can look at more of the resposne than the status code. + webAppMockMvc + .perform(invokePipelineConfigRequest()) + .andDo(print()) + .andExpect(status().isNotFound()) + .andExpect( + status() + .reason( + "Unable to trigger pipeline (application: my-application, pipelineId: my-pipeline-id). Error: Status: 404, URL: " + + wmFront50.baseUrl() + + "/pipelines/my-application?refresh=true, Message: message from front50")) + .andExpect(header().string(REQUEST_ID.getHeader(), SUBMITTED_REQUEST_ID)); + + verifyFront50PipelinesRequest(); + verifyNoOrcaOrchestrateRequest(); + } + + @Test + void invokePipelineConfigFront50BadRequest() throws Exception { + + // There are two different code paths for generating 400 responses from + // front50. One is via spring itself if the request doesn't match what + // spring expects. Another is if application logic determines a request is + // bad and throws an exception such that the exception handling code + // generates a 400. + // + // Here, spring expects true/false for refresh so it generates an empty response. + // + // $ curl -s -i http://localhost:8080/pipelines/foo?refresh=bar | head -1 + // HTTP/1.1 400 + // + // Here's one generated by front50: + // + // $ curl -s http://localhost:8080/pluginBinaries/foo/bar | python -mjson.tool + // { + // "error": "Bad Request", + // "exception": "java.lang.IllegalArgumentException", + // "message": "Plugin binary storage service is yet not available for your persistence + // backend", + // "status": 400, + // "timestamp": "2023-04-07T18:26:46.757+00:00" + // } + // + // $ curl -s -i http://localhost:8080/pluginBinaries/foo/bar | head -1 + // HTTP/1.1 400 + Map front50Response = + Map.of( + "error", + "Bad XX Request", + "message", + "message from front50", + "status", + 400, + "timestamp", + "2023-04-07T18:26:46.757+00:00"); + String front50ResponseJson = objectMapper.writeValueAsString(front50Response); + simulateFront50HttpResponse(HttpStatus.BAD_REQUEST, front50ResponseJson); + + webAppMockMvc + .perform(invokePipelineConfigRequest()) + .andDo(print()) + .andExpect(status().isBadRequest()) + .andExpect( + status() + .reason( + "Unable to trigger pipeline (application: my-application, pipelineId: my-pipeline-id). Error: Status: 400, URL: " + + wmFront50.baseUrl() + + "/pipelines/my-application?refresh=true, Message: message from front50")) + .andExpect(header().string(REQUEST_ID.getHeader(), SUBMITTED_REQUEST_ID)); + + verifyFront50PipelinesRequest(); + verifyNoOrcaOrchestrateRequest(); + } + + @Test + void invokePipelineConfigFront50Error() throws Exception { + // I haven't figured out how to make front50's get pipelines endpoint + // respond with 500 (this is good). I did find a way to make another + // front50 endpoint respond with 500. For example: + // + // $ curl -s http://localhost:8080/pipelines/foo/history?limit=-1 | python -mjson.tool + // { + // "error": "Internal Server Error", + // "exception": "org.springframework.jdbc.BadSqlGrammarException", + // "message": "jOOQ; bad SQL grammar [select body from pipelines_history where id = ? order + // by recorded_at desc limit ?]; nested exception is java.sql.SQLSyntaxErrorException: You have + // an error in your SQL syntax; check the manual that corresponds to your MySQL server version + // for the right syntax to use near '-1' at line 1", + // "status": 500, + // "timestamp": "2023-04-07T18:06:16.935+00:00" + // } + // + // and + // + // $ curl -s -i http://localhost:8080/pipelines/foo/history?limit=-1 | head -1 + // HTTP/1.1 500 + Map front50Response = + Map.of( + "error", + "Internal Server Error", + "exception", + "org.springframework.jdbc.BadSqlGrammarException", + "message", + "jOOQ; message from front50", + "status", + 500, + "timestamp", + "2023-04-07T18:06:16.935+00:00"); + String front50ResponseJson = objectMapper.writeValueAsString(front50Response); + simulateFront50HttpResponse(HttpStatus.INTERNAL_SERVER_ERROR, front50ResponseJson); + + webAppMockMvc + .perform(invokePipelineConfigRequest()) + .andDo(print()) + .andExpect(status().isInternalServerError()) + .andExpect( + status() + .reason( + "Unable to trigger pipeline (application: my-application, pipelineId: my-pipeline-id). Error: Status: 500, URL: " + + wmFront50.baseUrl() + + "/pipelines/my-application?refresh=true, Message: jOOQ; message from front50")) + .andExpect(header().string(REQUEST_ID.getHeader(), SUBMITTED_REQUEST_ID)); + + verifyFront50PipelinesRequest(); + verifyNoOrcaOrchestrateRequest(); + } + + @Test + void invokePipelineConfigFront50NetworkError() throws Exception { + simulateFront50Response(aResponse().withFault(Fault.CONNECTION_RESET_BY_PEER)); + + webAppMockMvc + .perform(invokePipelineConfigRequest()) + .andDo(print()) + .andExpect(status().isInternalServerError()) + .andExpect(status().reason("Connection reset")) + .andExpect(header().string(REQUEST_ID.getHeader(), SUBMITTED_REQUEST_ID)); + + verifyFront50PipelinesRequest(); + verifyNoOrcaOrchestrateRequest(); + } + + @Test + void invokePipelineConfigFront50NonJsonRespone() throws Exception { + simulateFront50HttpResponse(HttpStatus.OK, "this is not json"); + + webAppMockMvc + .perform(invokePipelineConfigRequest()) + .andDo(print()) + .andExpect(status().isInternalServerError()) + .andExpect( + status() + .reason( + "com.fasterxml.jackson.core.JsonParseException: Unrecognized token 'this': was expecting (JSON String, Number, Array, Object or token 'null', 'true' or 'false')\n at [Source: (retrofit.ExceptionCatchingTypedInput$ExceptionCatchingInputStream); line: 1, column: 6]")) + .andExpect(header().string(REQUEST_ID.getHeader(), SUBMITTED_REQUEST_ID)); + + verifyFront50PipelinesRequest(); + verifyNoOrcaOrchestrateRequest(); + } + + @Test + void invokePipelineConfigOrcaError() throws Exception { + // Unless front50 succeeds, gate doesn't get far enough to call orca + simulateFront50Success(); + + // Simulate a 500 response from orca, based on this real one. + // + // $ curl -s -H 'Content-Type: application/json' -d "{}" http://localhost:8080/orchestrate | + // python -mjson.tool + // { + // "error": "Internal Server Error", + // "exception": "java.lang.NullPointerException", + // "message": "null", + // "status": 500, + // "timestamp": 1680866655500 + // } + // + // And for completeness, here are the response headers: + // + // $ curl -s -i -H 'Content-Type: application/json' -d "{}" http://localhost:8080/orchestrate | + // head -1 + // HTTP/1.1 500 + Map orcaResponse = + Map.of( + "error", + "Internal XXX Server YYY Error", + "exception", + "java.lang.NullPointerException", + "message", + "message from orca", + "status", + 500, + "timestamp", + 1680866655500L); + String orcaResponseJson = objectMapper.writeValueAsString(orcaResponse); + simulateOrcaResponse(HttpStatus.INTERNAL_SERVER_ERROR, orcaResponseJson); + + webAppMockMvc + .perform(invokePipelineConfigRequest()) + .andDo(print()) + .andExpect(status().isInternalServerError()) + .andExpect( + status() + .reason( + "Unable to trigger pipeline (application: my-application, pipelineId: my-pipeline-id). Error: Status: 500, URL: " + + wmOrca.baseUrl() + + "/orchestrate?user=some+user, Message: message from orca")) + .andExpect(header().string(REQUEST_ID.getHeader(), SUBMITTED_REQUEST_ID)); + + verifyFront50PipelinesRequest(); + verifyOrcaOrchestrateRequest(); + } + + @Test + void invokePipelineConfigOrcaBadRequest() throws Exception { + // Unless front50 succeeds, gate doesn't get far enough to call orca + simulateFront50Success(); + + // Simulate a 400 response from orca, similar to the front50 response above + Map orcaResponse = + Map.of( + "error", + "Bad XX Request", + "message", + "message from orca", + "status", + 400, + "timestamp", + 1680866655500L); + String orcaResponseJson = objectMapper.writeValueAsString(orcaResponse); + simulateOrcaResponse(HttpStatus.BAD_REQUEST, orcaResponseJson); + + webAppMockMvc + .perform(invokePipelineConfigRequest()) + .andDo(print()) + .andExpect(status().isBadRequest()) + .andExpect( + status() + .reason( + "Unable to trigger pipeline (application: my-application, pipelineId: my-pipeline-id). Error: Status: 400, URL: " + + wmOrca.baseUrl() + + "/orchestrate?user=some+user, Message: message from orca")) + .andExpect(header().string(REQUEST_ID.getHeader(), SUBMITTED_REQUEST_ID)); + + verifyFront50PipelinesRequest(); + verifyOrcaOrchestrateRequest(); + } + + /** + * Simulate a successful response from front50. The actual response needs to be sufficient for + * PipelineService.trigger to get far enough to call orca, which means we need a configuration for + * the pipeline we're triggering + */ + private void simulateFront50Success() throws JsonProcessingException { + List> pipelineConfigs = List.of(Map.of("id", PIPELINE_ID)); + String pipelineConfigsJson = objectMapper.writeValueAsString(pipelineConfigs); + simulateFront50HttpResponse(HttpStatus.OK, pipelineConfigsJson); + } + + /** + * Simulate a successful response from front50, but one that doesn't contain a pipeline + * configuration for the test id + */ + private void simulateFront50SuccessWithoutPipelineConfig() throws JsonProcessingException { + List> pipelineConfigs = Collections.emptyList(); + String pipelineConfigsJson = objectMapper.writeValueAsString(pipelineConfigs); + simulateFront50HttpResponse(HttpStatus.OK, pipelineConfigsJson); + } + + /** + * Simulate an http response from front50 + * + * @param httpStatus the response code + * @param body the response body + */ + private void simulateFront50HttpResponse(HttpStatus httpStatus, String body) { + simulateFront50Response(aResponse().withStatus(httpStatus.value()).withBody(body)); + } + + /** Simulate a response from front50 */ + private void simulateFront50Response(ResponseDefinitionBuilder responseDefinitionBuilder) { + wmFront50.stubFor( + WireMock.get(urlPathEqualTo("/pipelines/" + APPLICATION)) + .withQueryParam("refresh", anythingPattern) + .willReturn(responseDefinitionBuilder)); + } + + /** An arbitrary successful response from orca */ + private String orcaSuccessResponse() throws JsonProcessingException { + Map orcaResponse = Collections.emptyMap(); + return objectMapper.writeValueAsString(orcaResponse); + } + + /** Simulate a successful response from orca */ + private void simulateOrcaSuccess() throws JsonProcessingException { + simulateOrcaResponse(HttpStatus.OK, orcaSuccessResponse()); + } + + /** + * Simulate a response from orca + * + * @param httpStatus the response code + * @param body the response body + */ + private void simulateOrcaResponse(HttpStatus httpStatus, String body) { + wmOrca.stubFor( + WireMock.post(urlPathEqualTo("/orchestrate")) + .withQueryParam("user", equalTo(USERNAME)) + .withHeader(USER.getHeader(), equalTo(USERNAME)) + .willReturn(aResponse().withStatus(httpStatus.value()).withBody(body))); + } + + /** Generate a request to the endpoint that PipelineController.invokePipelineConfig serves */ + private RequestBuilder invokePipelineConfigRequest() throws JsonProcessingException { + return post("/pipelines/" + APPLICATION + "/" + PIPELINE_ID) + .contentType(MediaType.APPLICATION_JSON_VALUE) + .characterEncoding(StandardCharsets.UTF_8.toString()) + .header(USER.getHeader(), USERNAME) + .header(REQUEST_ID.getHeader(), SUBMITTED_REQUEST_ID) + .header( + ACCOUNTS.getHeader(), + ACCOUNT) // to silence warning when X-SPINNAKER-ACCOUNTS is missing + .content(objectMapper.writeValueAsString(TRIGGER)); + } + + /** + * Verify that a request to front50 for pipelines happened and contained the X-SPINNAKER-* header + * passed to gate. + */ + private void verifyFront50PipelinesRequest() { + wmFront50.verify( + getRequestedFor(urlPathEqualTo("/pipelines/" + APPLICATION)) + .withQueryParam("refresh", anythingPattern) + .withHeader(USER.getHeader(), equalTo(USERNAME))); + } + + /** + * Verify that a request to orca to orchestrate a pipeline happened and contained the + * X-SPINNAKER-* header passed to gate. + */ + private void verifyOrcaOrchestrateRequest() { + wmOrca.verify( + postRequestedFor(urlPathEqualTo("/orchestrate")) + .withQueryParam("user", equalTo(USERNAME)) + .withHeader(USER.getHeader(), equalTo(USERNAME))); + } + + /** Verify that there was no request to orca to orchestrate a pipeline */ + private void verifyNoOrcaOrchestrateRequest() { + wmOrca.verify(exactly(0), postRequestedFor(urlPathEqualTo("/orchestrate"))); + } +} diff --git a/gate-web/src/test/java/com/netflix/spinnaker/gate/controllers/PipelineControllerTest.java b/gate-web/src/test/java/com/netflix/spinnaker/gate/controllers/PipelineControllerTest.java new file mode 100644 index 0000000000..c92deba682 --- /dev/null +++ b/gate-web/src/test/java/com/netflix/spinnaker/gate/controllers/PipelineControllerTest.java @@ -0,0 +1,124 @@ +/* + * Copyright 2023 Salesforce, 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.gate.controllers; + +import static com.netflix.spinnaker.kork.common.Header.REQUEST_ID; +import static org.mockito.ArgumentMatchers.anyMap; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.when; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; +import static org.springframework.test.web.servlet.result.MockMvcResultHandlers.print; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.header; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; +import static org.springframework.test.web.servlet.setup.MockMvcBuilders.webAppContextSetup; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.netflix.spinnaker.gate.Main; +import com.netflix.spinnaker.gate.health.DownstreamServicesHealthIndicator; +import com.netflix.spinnaker.gate.services.ApplicationService; +import com.netflix.spinnaker.gate.services.DefaultProviderLookupService; +import com.netflix.spinnaker.gate.services.PipelineService; +import java.nio.charset.StandardCharsets; +import java.util.Collections; +import java.util.Map; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInfo; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Qualifier; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.test.mock.mockito.MockBean; +import org.springframework.boot.web.servlet.FilterRegistrationBean; +import org.springframework.http.MediaType; +import org.springframework.test.context.TestPropertySource; +import org.springframework.test.web.servlet.MockMvc; +import org.springframework.test.web.servlet.RequestBuilder; +import org.springframework.web.context.WebApplicationContext; + +/** + * See https://github.com/spring-projects/spring-boot/issues/5574#issuecomment-506282892 for a + * discussion of why it's difficult to assert on response bodies generated by error handlers. It's + * not necessary in these tests, as the information we need is available elsewhere (e.g. the http + * status line). + */ +@SpringBootTest(classes = Main.class) +@TestPropertySource(properties = "spring.config.location=classpath:gate-test.yml") +class PipelineControllerTest { + + private MockMvc webAppMockMvc; + + @Autowired private WebApplicationContext webApplicationContext; + + @Autowired ObjectMapper objectMapper; + + @MockBean PipelineService pipelineService; + + /** + * This takes X-SPINNAKER-* headers from requests to gate and puts them in the MDC. This is + * enabled when gate runs normally (by GateConfig), but needs explicit mention to function in + * these tests. + */ + @Autowired + @Qualifier("authenticatedRequestFilter") + private FilterRegistrationBean filterRegistrationBean; + + /** Mock the application service to disable the background thread that caches applications */ + @MockBean ApplicationService applicationService; + + /** To prevent periodic calls to service's /health endpoints */ + @MockBean DownstreamServicesHealthIndicator downstreamServicesHealthIndicator; + + /** To prevent periodic calls to clouddriver to query for accounts */ + @MockBean DefaultProviderLookupService defaultProviderLookupService; + + private static final String APPLICATION = "my-application"; + private static final String PIPELINE_ID = "my-pipeline-id"; + private static final String SUBMITTED_REQUEST_ID = "submitted-request-id"; + private final Map TRIGGER = Collections.emptyMap(); // arbitrary + + @BeforeEach + void init(TestInfo testInfo) throws JsonProcessingException { + System.out.println("--------------- Test " + testInfo.getDisplayName()); + + webAppMockMvc = + webAppContextSetup(webApplicationContext) + .addFilters(filterRegistrationBean.getFilter()) + .build(); + } + + @Test + void invokePipelineConfigRuntimeException() throws Exception { + RuntimeException exception = new RuntimeException("arbitrary message"); + when(pipelineService.trigger(anyString(), anyString(), anyMap())).thenThrow(exception); + + webAppMockMvc + .perform(invokePipelineConfigRequest()) + .andDo(print()) + .andExpect(status().isInternalServerError()) + .andExpect(status().reason(exception.getMessage())) + .andExpect(header().string(REQUEST_ID.getHeader(), SUBMITTED_REQUEST_ID)); + } + + /** Generate a request to the endpoint that PipelineController.invokePipelineConfig serves */ + private RequestBuilder invokePipelineConfigRequest() throws JsonProcessingException { + return post("/pipelines/" + APPLICATION + "/" + PIPELINE_ID) + .contentType(MediaType.APPLICATION_JSON_VALUE) + .characterEncoding(StandardCharsets.UTF_8.toString()) + .header(REQUEST_ID.getHeader(), SUBMITTED_REQUEST_ID) + .content(objectMapper.writeValueAsString(TRIGGER)); + } +}