fix(web): invoke pipeline config exception handling (#1831)

* feat(web): 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.

* test(web): demonstrate current behavior of PipelineController.invokePipelineConfig

as determined by wiremock responses from front50 and orca (InvokePipelineConfigTest)

and when PipelineService.trigger throws an exception (PipelineControllerTest)

* fix(web): let exceptions during PipelineController.invokePipelineConfig bubble up

so gate's http response code more closely reflects what happened

* refactor(web): change PipelineController to use constructor autowiring
to prepare for changes to the constructor logic

* fix(web): 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.

* 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.

* refactor(web): updating custom exception message creation

Uses new kork methods to clean up some error handling and custom exception message creation.

---------

Co-authored-by: David Byron <dbyron@salesforce.com>
Co-authored-by: Richard Timpson <richard.timpson@salesforce.com>

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.apache.groovy:groovy-json"
-  testRuntimeOnly "io.spinnaker.kork:kork-retrofit"
 
   // Add each included authz provider as a runtime dependency
   gradle.includedProviderProjects.each {

gate-web/src/main/groovy/com/netflix/spinnaker/gate/controllers/PipelineController.groovy

@@ -23,7 +23,7 @@ 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.retrofit.exceptions.SpinnakerRetrofitErrorHandler
 import com.netflix.spinnaker.kork.web.exceptions.NotFoundException
 import com.netflix.spinnaker.security.AuthenticatedRequest
 import groovy.transform.CompileDynamic
@@ -51,20 +51,44 @@ import static net.logstash.logback.argument.StructuredArguments.value
 @RestController
 @RequestMapping("/pipelines")
 class PipelineController {
-  @Autowired
-  PipelineService pipelineService
-
-  @Autowired
-  TaskService taskService
-
-  @Autowired
-  Front50Service front50Service
+  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
-  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 +324,22 @@ 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 (SpinnakerException e) {
+      //   throw new e.newInstance(triggerFailureMessage(application, pipelineNameOrId, e));
+      // }
+      throw spinnakerRetrofitErrorHandler.handleError(e, {
+        exception -> triggerFailureMessage(application, pipelineNameOrId, exception) });
     }
   }
 
+  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 +460,4 @@ class PipelineController {
       this.additionalAttributes = additionalAttributes
     }
   }
-
-  @ResponseStatus(HttpStatus.UNPROCESSABLE_ENTITY)
-  @InheritConstructors
-  class PipelineExecutionException extends InvalidRequestException {
-    PipelineExecutionException(String message) {
-      super(message)
-    }
-  }
 }

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))

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

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"() {