Skip to content

Commit

Permalink
perf(web): Query for individual pipelines (#1836)
Browse files Browse the repository at this point in the history 
* refactor(web): adjust error message in PipelineController.invokePipelineConfig

and use pipeline name instead of id in InvokePipelineConfigTest

to reduce diffs when ApplicationService.getPipelineConfigForApplication changes the queries it makes to front50

* perf(web): change ApplicationService.getPipelineConfigForApplication to query front50 for individual pipelines

instead of querying for all pipelines in an application and then filtering locally.

* refactor(web): teach ApplicationService.getPipelineConfigForApplication to handle errors with SpinnakerRetrofitErrorHandler

to centralize exception handling there / remove the need for callers to check for null

---------

Co-authored-by: David Byron <dbyron@salesforce.com>
  • Loading branch information
@kirangodishala @dbyron-sf
kirangodishala and dbyron-sf authored Sep 30, 2024
1 parent 2db0484 commit ea57f38
Show file tree
Hide file tree
Showing 6 changed files with 230 additions and 96 deletions.
7 changes: 7 additions & 0 deletions gate-core/src/main/java/com/netflix/spinnaker/gate/services/internal/Front50Service.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
10 changes: 4 additions & 6 deletions gate-web/src/main/groovy/com/netflix/spinnaker/gate/controllers/PipelineController.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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())
}

Expand Down
84 changes: 82 additions & 2 deletions gate-web/src/main/groovy/com/netflix/spinnaker/gate/services/ApplicationService.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand All @@ -66,6 +89,7 @@ class ApplicationService {
this.applicationConfigurationProperties = applicationConfigurationProperties
this.executorService = Executors.newCachedThreadPool()
this.allApplicationsCache = new AtomicReference<>([])
this.spinnakerRetrofitErrorHandler = SpinnakerRetrofitErrorHandler.newInstance()
}

// used in tests
Expand Down Expand Up @@ -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) {
Expand Down
4 changes: 0 additions & 4 deletions gate-web/src/main/groovy/com/netflix/spinnaker/gate/services/PipelineService.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down
91 changes: 80 additions & 11 deletions gate-web/src/test/groovy/com/netflix/spinnaker/gate/ApplicationServiceSpec.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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]]]
Expand Down Expand Up @@ -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"() {
Expand All @@ -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)
}
}
Loading

0 comments on commit ea57f38

Please sign in to comment.