From 24855d9c4ef2acd15c208f852a31cf20195ffb02 Mon Sep 17 00:00:00 2001 From: David Byron Date: Fri, 7 Apr 2023 19:24:56 -0700 Subject: [PATCH] fix(web): 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. --- .../controllers/PipelineController.groovy | 29 ++++++++++++++----- .../controllers/InvokePipelineConfigTest.java | 18 +++++++----- 2 files changed, 32 insertions(+), 15 deletions(-) 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 1e6b2d8bd1..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,6 +23,8 @@ 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.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 @@ -324,17 +326,30 @@ class PipelineController { AuthenticatedRequest.setApplication(application) try { pipelineService.trigger(application, pipelineNameOrId, trigger) - } catch (NotFoundException e) { - throw e } catch (RetrofitError e) { - throw spinnakerRetrofitErrorHandler.handleError(e) - } catch (e) { - log.error("Unable to trigger pipeline (application: {}, pipelineId: {})", - value("application", application), value("pipelineId", pipelineNameOrId), e) - throw 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:.+}") 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 index e841a05843..b3891fdafc 100644 --- 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 @@ -241,9 +241,11 @@ void invokePipelineConfig404ResponseFromFront50() throws Exception { 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. + // 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()) @@ -251,7 +253,7 @@ void invokePipelineConfig404ResponseFromFront50() throws Exception { .andExpect( status() .reason( - "Status: 404, URL: " + "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)); @@ -308,7 +310,7 @@ void invokePipelineConfigFront50BadRequest() throws Exception { .andExpect( status() .reason( - "Status: 400, URL: " + "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)); @@ -361,7 +363,7 @@ void invokePipelineConfigFront50Error() throws Exception { .andExpect( status() .reason( - "Status: 500, URL: " + "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)); @@ -447,7 +449,7 @@ void invokePipelineConfigOrcaError() throws Exception { .andExpect( status() .reason( - "Status: 500, URL: " + "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)); @@ -482,7 +484,7 @@ void invokePipelineConfigOrcaBadRequest() throws Exception { .andExpect( status() .reason( - "Status: 400, URL: " + "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));