Merge branch 'master' into 6766

gate-core/src/main/java/com/netflix/spinnaker/gate/services/internal/Front50Service.java

@@ -44,6 +44,13 @@ List<Map> getApplicationHistory(
   List<Map> getPipelineConfigsForApplication(
       @Path("app") String app, @Query("refresh") boolean refresh);
 
+  @GET("/pipelines/{app}/name/{name}")
+  Map getPipelineConfigByApplicationAndName(
+      @Path("app") String app, @Path("name") String name, @Query("refresh") boolean refresh);
+
+  @GET("/pipelines/{id}/get")
+  Map getPipelineConfigById(@Path("id") String id);
+
   @DELETE("/pipelines/{app}/{name}")
   Response deletePipelineConfig(@Path("app") String app, @Path("name") String name);
 

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

@@ -23,6 +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.exceptions.SpinnakerException
 import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler
 import com.netflix.spinnaker.kork.web.exceptions.NotFoundException
 import com.netflix.spinnaker.security.AuthenticatedRequest
@@ -324,19 +325,16 @@ class PipelineController {
     AuthenticatedRequest.setApplication(application)
     try {
       pipelineService.trigger(application, pipelineNameOrId, trigger)
+    } catch (SpinnakerException e) {
+      throw e.newInstance(triggerFailureMessage(application, pipelineNameOrId, e));
     } 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",
+    String.format("Unable to trigger pipeline (application: %s, pipelineNameOrId: %s). Error: %s",
         value("application", application), value("pipelineId", pipelineNameOrId), e.getMessage())
   }
 

gate-web/src/main/groovy/com/netflix/spinnaker/gate/services/ApplicationService.groovy

@@ -22,11 +22,15 @@ import com.netflix.spinnaker.gate.config.ServiceConfiguration
 import com.netflix.spinnaker.gate.services.internal.ClouddriverService
 import com.netflix.spinnaker.gate.services.internal.ClouddriverServiceSelector
 import com.netflix.spinnaker.gate.services.internal.Front50Service
+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
 import groovy.transform.CompileStatic
 import groovy.util.logging.Slf4j
 import org.springframework.beans.factory.annotation.Autowired
+import org.springframework.http.HttpStatus
 import org.springframework.scheduling.annotation.Scheduled
 import org.springframework.security.core.context.SecurityContextHolder
 import org.springframework.stereotype.Component
@@ -53,6 +57,25 @@ class ApplicationService {
   private AtomicReference<List<Map>> allApplicationsCache
   private ApplicationConfigurationProperties applicationConfigurationProperties
 
+  /**
+   * 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 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 getPipelineConfigForApplication, construct a
+   * spinnakerRetrofitErrorHandler and use it directly.
+   */
+  final SpinnakerRetrofitErrorHandler spinnakerRetrofitErrorHandler
+
   @Autowired
   ApplicationService(
     ServiceConfiguration serviceConfiguration,
@@ -66,6 +89,7 @@ class ApplicationService {
     this.applicationConfigurationProperties = applicationConfigurationProperties
     this.executorService = Executors.newCachedThreadPool()
     this.allApplicationsCache = new AtomicReference<>([])
+    this.spinnakerRetrofitErrorHandler = SpinnakerRetrofitErrorHandler.newInstance()
   }
 
   // used in tests
@@ -151,9 +175,65 @@ class ApplicationService {
     return front50Service.getPipelineConfigsForApplication(app, true)
   }
 
+  /**
+   * Return the pipeline configuration from front50
+   *
+   * @param app the application of the pipeline
+   * @param pipelineNameOrId the name or id of the pipeline
+   * @return the pipeline configuration, or throws an exception if not found
+   */
   Map getPipelineConfigForApplication(String app, String pipelineNameOrId) {
-    return front50Service.getPipelineConfigsForApplication(app, true)
-      .find { it.name == pipelineNameOrId || it.id == pipelineNameOrId }
+    // Since the argument can be a pipeline name or id, handle both cases.
+    // Query by name first since that's more likely.
+    try {
+      Map pipelineConfig = front50Service.getPipelineConfigByApplicationAndName(app, pipelineNameOrId, true)
+      if (pipelineConfig.name == pipelineNameOrId) {
+        log.debug("front50 returned a pipeline with name ${pipelineNameOrId} in application ${app}")
+        return pipelineConfig
+      }
+      log.error("front50 query for a pipeline with name ${pipelineNameOrId} in application ${app} returned a pipeline named ${pipelineConfig.name}")
+      // Tempting to return null here, but querying by id might work, so give it a shot.
+    } catch (RetrofitError e) {
+      // If spinnakerRetrofitErrorHandler were registered as a "real" error handler, the code here would look something like
+      //
+      // } catch (SpinnakeHttpException e) {
+      //   if (e.getResponseCode() == HttpStatus.NOT_FOUND.value()) {
+      //     ...
+      //   }
+      // }
+      Throwable throwable = spinnakerRetrofitErrorHandler.handleError(e);
+      if (throwable instanceof SpinnakerHttpException && throwable.getResponseCode() == HttpStatus.NOT_FOUND.value()) {
+        log.info("front50 returned no pipeline with name ${pipelineNameOrId} in application ${app}")
+        // fall through to try querying by id
+      } else {
+        // It's a pretty arbitrary choice whether to throw e or throwable here
+        // since PipelineController.invokePipelineConfig handles both.  May as
+        // well throw throwable to avoid converting the RetrofitError to a
+        // Spinnaker*Exception again.
+        throw throwable
+      }
+    }
+
+    // query by id
+    try {
+      Map pipelineConfig = front50Service.getPipelineConfigById(pipelineNameOrId)
+      if (pipelineConfig.id == pipelineNameOrId) {
+        log.debug("front50 returned a pipeline with id ${pipelineNameOrId}")
+        return pipelineConfig
+      }
+      log.error("front50 query for a pipeline with id ${pipelineNameOrId} returned a pipeline with id ${pipelineConfig.id}")
+    } catch (RetrofitError e) {
+      Throwable throwable = spinnakerRetrofitErrorHandler.handleError(e);
+      if (throwable instanceof SpinnakerHttpException && throwable.getResponseCode() == HttpStatus.NOT_FOUND.value()) {
+        log.info("front50 returned no pipeline with id ${pipelineNameOrId}")
+        throw throwable.newInstance("Pipeline configuration not found (id: ${pipelineNameOrId}): " + throwable.getMessage())
+      }
+      throw throwable
+    }
+
+    // If we get here, the query by id returned a pipeline whose id didn't match
+    // what we asked for.
+    throw new NotFoundException("Pipeline configuration not found (id: ${pipelineNameOrId})")
   }
 
   List<Map> getStrategyConfigsForApplication(String app) {

gate-web/src/main/groovy/com/netflix/spinnaker/gate/services/PipelineService.groovy

@@ -23,7 +23,6 @@ import com.netflix.spinnaker.gate.services.internal.EchoService
 import com.netflix.spinnaker.gate.services.internal.Front50Service
 import com.netflix.spinnaker.gate.services.internal.OrcaServiceSelector
 import com.netflix.spinnaker.kork.core.RetrySupport
-import com.netflix.spinnaker.kork.web.exceptions.NotFoundException
 import com.netflix.spinnaker.security.AuthenticatedRequest
 import de.huxhorn.sulky.ulid.ULID
 import groovy.util.logging.Slf4j
@@ -93,9 +92,6 @@ class PipelineService {
 
   Map trigger(String application, String pipelineNameOrId, Map trigger) {
     def pipelineConfig = applicationService.getPipelineConfigForApplication(application, pipelineNameOrId)
-    if (!pipelineConfig) {
-      throw new NotFoundException("Pipeline configuration not found (id: ${pipelineNameOrId})")
-    }
     pipelineConfig.trigger = trigger
     if (trigger.notifications) {
       if (pipelineConfig.notifications) {

gate-web/src/test/groovy/com/netflix/spinnaker/gate/ApplicationServiceSpec.groovy

@@ -16,15 +16,19 @@
 
 package com.netflix.spinnaker.gate
 
+import com.google.gson.Gson
 import com.netflix.spinnaker.gate.config.ApplicationConfigurationProperties
 import com.netflix.spinnaker.gate.config.Service
 import com.netflix.spinnaker.gate.config.ServiceConfiguration
 import com.netflix.spinnaker.gate.services.ApplicationService
 import com.netflix.spinnaker.gate.services.internal.ClouddriverServiceSelector
 import com.netflix.spinnaker.gate.services.internal.Front50Service
 import com.netflix.spinnaker.gate.services.internal.ClouddriverService
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException
+import com.netflix.spinnaker.kork.web.exceptions.NotFoundException
 import retrofit.RetrofitError
 import retrofit.client.Response
+import retrofit.converter.GsonConverter
 import retrofit.mime.TypedByteArray
 import spock.lang.Specification
 import spock.lang.Unroll
@@ -51,6 +55,10 @@ class ApplicationServiceSpec extends Specification {
     return service
   }
 
+  void setup() {
+    println "--------------- Test " + specificationContext.currentIteration.name
+  }
+
   void "should properly aggregate application data from Front50 and Clouddriver when useFront50AsSourceOfTruth: #useFront50AsSourceOfTruth"() {
     given:
     def clouddriverApp = [name: name, attributes: [clouddriverName: name, name: "bad"], clusters: [(account): [cluster]]]
@@ -458,23 +466,84 @@ class ApplicationServiceSpec extends Specification {
     null        | null          || ""
   }
 
-  @Unroll
-  void "should return pipeline config based on name or id"() {
+  void "getPipelineConfigForApplication returns pipeline config based on name"() {
+    given:
+    def app = "theApp"
+    def nameOrId = "by-name"
+
+    when:
+    def service = applicationService()
+    def result = service.getPipelineConfigForApplication(app, nameOrId)
+
+    then:
+    result != null
+    1 * front50.getPipelineConfigByApplicationAndName(app, nameOrId, true) >> [ id: "by-id", name: "by-name" ]
+    0 * front50.getPipelineConfigById(_)
+  }
+
+  void "getPipelineConfigForApplication returns pipeline config based on id"() {
+    given:
+    def app = "theApp"
+    def nameOrId = "by-id"
+
     when:
     def service = applicationService()
     def result = service.getPipelineConfigForApplication(app, nameOrId) != null
 
     then:
-    result == expected
-    1 * front50.getPipelineConfigsForApplication(app, true) >> [ [ id: "by-id", name: "by-name" ] ]
+    result != null
+    1 * front50.getPipelineConfigByApplicationAndName(app, nameOrId, true) >> { throw retrofit404() }
+    1 * front50.getPipelineConfigById(nameOrId) >> [ id: "by-id", name: "by-name" ]
+  }
 
-    where:
-    app = "theApp"
+  void "getPipelineConfigForApplication throws an exception when neither name nor id match"() {
+    given:
+    def app = "theApp"
+    def nameOrId = "not-id"
+
+    when:
+    def service = applicationService()
+    service.getPipelineConfigForApplication(app, nameOrId)
 
-    nameOrId  || expected
-    "by-id"   || true
-    "by-name" || true
-    "not-id"  || false
+    then:
+    def e = thrown SpinnakerHttpException
+    e.responseCode == 404
+    1 * front50.getPipelineConfigByApplicationAndName(app, nameOrId, true) >> { throw retrofit404() }
+    1 * front50.getPipelineConfigById(nameOrId) >> { throw retrofit404() }
+  }
+
+  void "getPipelineConfigForApplication queries by id when response to query by name doesn't match"() {
+    given:
+    def app = "theApp"
+    def nameOrId = "by-name"
+
+    when:
+    def service = applicationService()
+    def result = service.getPipelineConfigForApplication(app, nameOrId)
+
+    then:
+    result != null
+    1 * front50.getPipelineConfigByApplicationAndName(app, nameOrId, true) >> [ id: "arbitrary-id", name: "some-other-name" ]
+
+    // The key part of this test is that gate queries front50 by id.  The choice
+    // of id here needs to match the expectation for result (i.e. throw an exception or not),
+    // but is otherwise arbitrary.
+    1 * front50.getPipelineConfigById(nameOrId) >> [ id: nameOrId, name: "arbitrary-name" ]
+  }
+
+  void "getPipelineConfigForApplication throws an exception when response to query by id doesn't match"() {
+    given:
+    def app = "theApp"
+    def nameOrId = "by-id"
+
+    when:
+    def service = applicationService()
+    service.getPipelineConfigForApplication(app, nameOrId)
+
+    then:
+    thrown NotFoundException
+    1 * front50.getPipelineConfigByApplicationAndName(app, nameOrId, true) >> { throw retrofit404() }
+    1 * front50.getPipelineConfigById(nameOrId) >> [ id: "some-other-id", name: "arbitrary-name" ]
   }
 
   void "should skip clouddriver call if expand set to false"() {
@@ -501,6 +570,6 @@ class ApplicationServiceSpec extends Specification {
   }
 
   def retrofit404(){
-    RetrofitError.httpError("http://localhost", new Response("http://localhost", 404, "Not Found", [], new TypedByteArray("application/json", new byte[0])), null, Map)
+    RetrofitError.httpError("http://localhost", new Response("http://localhost", 404, "Not Found", [], new TypedByteArray("application/json", new byte[0])), new GsonConverter(new Gson()), Map)
   }
 }