From 420256fa0ad03de97984abf7a20692b6c0392d36 Mon Sep 17 00:00:00 2001 From: Rob Zienert Date: Thu, 3 Oct 2019 15:28:18 -0700 Subject: [PATCH] fix(titus): Fix SagaContext wiring in DeployHandler cases (#4070) It turns out that TitusDeployHandler (and other DeployHandler classes) are singleton components that get injected into prototype AtomicOperation components, which I didn't realize until now. The issue with this is that I was setting request-scoped context into a singleton bean, which could lead to some neat concurrency bugs. This change modifies things for DeployAtomicOperation specifically to inject the SagaContext into the description (which is actually request-scoped) rather than passing through to the TitusDeployHandler object. --- .../deploy/DeployAtomicOperation.groovy | 18 +++++++++++++----- .../AtomicOperationConverter.groovy | 2 -- .../orchestration/OperationsService.java | 9 +++++---- .../TitusDeployAtomicOperationConverter.groovy | 2 ++ .../description/TitusDeployDescription.java | 9 ++++++++- .../deploy/handlers/TitusDeployHandler.java | 14 +++----------- 6 files changed, 31 insertions(+), 23 deletions(-) diff --git a/clouddriver-core/src/main/groovy/com/netflix/spinnaker/clouddriver/deploy/DeployAtomicOperation.groovy b/clouddriver-core/src/main/groovy/com/netflix/spinnaker/clouddriver/deploy/DeployAtomicOperation.groovy index 1006fe94c10..eb707d90be0 100644 --- a/clouddriver-core/src/main/groovy/com/netflix/spinnaker/clouddriver/deploy/DeployAtomicOperation.groovy +++ b/clouddriver-core/src/main/groovy/com/netflix/spinnaker/clouddriver/deploy/DeployAtomicOperation.groovy @@ -23,6 +23,8 @@ import com.netflix.spinnaker.clouddriver.orchestration.SagaContextAware import com.netflix.spinnaker.clouddriver.orchestration.events.OperationEvent import org.springframework.beans.factory.annotation.Autowired +import javax.annotation.Nonnull + class DeployAtomicOperation implements AtomicOperation, SagaContextAware { private static final String TASK_PHASE = "DEPLOY" @@ -30,7 +32,6 @@ class DeployAtomicOperation implements AtomicOperation, SagaCo DeployHandlerRegistry deploymentHandlerRegistry private final DeployDescription description - SagaContext sagaContext DeployAtomicOperation(DeployDescription description) { this.description = description @@ -54,10 +55,6 @@ class DeployAtomicOperation implements AtomicOperation, SagaCo throw new DeployHandlerNotFoundException("Could not find handler for ${description.getClass().simpleName}!") } - if (deployHandler instanceof SagaContextAware) { - deployHandler.sagaContext = sagaContext - } - task.updateStatus TASK_PHASE, "Found handler: ${deployHandler.getClass().simpleName}" task.updateStatus TASK_PHASE, "Invoking Handler." @@ -67,4 +64,15 @@ class DeployAtomicOperation implements AtomicOperation, SagaCo return deploymentResult } + + @Override + void setSagaContext(@Nonnull SagaContext sagaContext) { + // DeployHandlers are singleton objects autowired differently than their one-off AtomicOperations, so we can't + // set a SagaContext onto them. Instead, we need to set it onto the description. To pile on, AtomicOperationConverters + // throw away the initial converted AtomicOperationDescription, so we can't apply the SagaContext to the description + // on behalf of cloud provider integrators... so we have to wire that up for them manually in any AtomicOperation. + if (description instanceof SagaContextAware) { + ((SagaContextAware) description).sagaContext = sagaContext + } + } } diff --git a/clouddriver-core/src/main/groovy/com/netflix/spinnaker/clouddriver/orchestration/AtomicOperationConverter.groovy b/clouddriver-core/src/main/groovy/com/netflix/spinnaker/clouddriver/orchestration/AtomicOperationConverter.groovy index a1a6e34dd31..b020fecc8e6 100644 --- a/clouddriver-core/src/main/groovy/com/netflix/spinnaker/clouddriver/orchestration/AtomicOperationConverter.groovy +++ b/clouddriver-core/src/main/groovy/com/netflix/spinnaker/clouddriver/orchestration/AtomicOperationConverter.groovy @@ -16,9 +16,7 @@ package com.netflix.spinnaker.clouddriver.orchestration - import javax.annotation.Nullable - /** * Implementations of this trait will provide an object capable of converting a Map of input parameters to an * operation's description object and an {@link AtomicOperation} instance. diff --git a/clouddriver-core/src/main/groovy/com/netflix/spinnaker/clouddriver/orchestration/OperationsService.java b/clouddriver-core/src/main/groovy/com/netflix/spinnaker/clouddriver/orchestration/OperationsService.java index 4668df33bff..cb49a0d2eda 100644 --- a/clouddriver-core/src/main/groovy/com/netflix/spinnaker/clouddriver/orchestration/OperationsService.java +++ b/clouddriver-core/src/main/groovy/com/netflix/spinnaker/clouddriver/orchestration/OperationsService.java @@ -174,15 +174,16 @@ private List convert( } allowedAccountValidators.forEach( - it -> { - it.validate(username, allowedAccounts, description, errors); - }); + it -> it.validate(username, allowedAccounts, description, errors)); // TODO(rz): Assert `description` is T descriptionAuthorizer.authorize(description, errors); + // TODO(rz): This is so bad. We convert the description input twice (once + // above) and then once inside of this convertOperation procedure. This + // means that we do a bunch of serde work twice without needing to. AtomicOperation atomicOperation = - converter.convertOperation(descriptionInput); + converter.convertOperation(processedInput); if (atomicOperation == null) { throw new AtomicOperationNotFoundException(descriptionName); } diff --git a/clouddriver-titus/src/main/groovy/com/netflix/spinnaker/clouddriver/titus/deploy/converters/TitusDeployAtomicOperationConverter.groovy b/clouddriver-titus/src/main/groovy/com/netflix/spinnaker/clouddriver/titus/deploy/converters/TitusDeployAtomicOperationConverter.groovy index 040c4cf835d..99738fde858 100644 --- a/clouddriver-titus/src/main/groovy/com/netflix/spinnaker/clouddriver/titus/deploy/converters/TitusDeployAtomicOperationConverter.groovy +++ b/clouddriver-titus/src/main/groovy/com/netflix/spinnaker/clouddriver/titus/deploy/converters/TitusDeployAtomicOperationConverter.groovy @@ -28,10 +28,12 @@ import org.springframework.stereotype.Component @Component class TitusDeployAtomicOperationConverter extends AbstractAtomicOperationsCredentialsSupport { + @Override AtomicOperation convertOperation(Map input) { new DeployAtomicOperation(convertDescription(input)) } + @Override TitusDeployDescription convertDescription(Map input) { // Backwards-compatibility for when the Titus provider blindly accepted any container // attribute value, when in reality this can only be string values. Now that the diff --git a/clouddriver-titus/src/main/groovy/com/netflix/spinnaker/clouddriver/titus/deploy/description/TitusDeployDescription.java b/clouddriver-titus/src/main/groovy/com/netflix/spinnaker/clouddriver/titus/deploy/description/TitusDeployDescription.java index 87ea4049d36..438b0d48941 100644 --- a/clouddriver-titus/src/main/groovy/com/netflix/spinnaker/clouddriver/titus/deploy/description/TitusDeployDescription.java +++ b/clouddriver-titus/src/main/groovy/com/netflix/spinnaker/clouddriver/titus/deploy/description/TitusDeployDescription.java @@ -1,6 +1,7 @@ package com.netflix.spinnaker.clouddriver.titus.deploy.description; import com.netflix.spinnaker.clouddriver.deploy.DeployDescription; +import com.netflix.spinnaker.clouddriver.orchestration.SagaContextAware; import com.netflix.spinnaker.clouddriver.orchestration.events.OperationEvent; import com.netflix.spinnaker.clouddriver.security.resources.ApplicationNameable; import com.netflix.spinnaker.clouddriver.titus.client.model.DisruptionBudget; @@ -24,7 +25,7 @@ @Data @EqualsAndHashCode(callSuper = true) public class TitusDeployDescription extends AbstractTitusCredentialsDescription - implements DeployDescription, ApplicationNameable { + implements DeployDescription, ApplicationNameable, SagaContextAware { private String region; private String subnet; private List zones = new ArrayList<>(); @@ -56,6 +57,7 @@ public class TitusDeployDescription extends AbstractTitusCredentialsDescription private DisruptionBudget disruptionBudget; private SubmitJobRequest.Constraints constraints = new SubmitJobRequest.Constraints(); private ServiceJobProcesses serviceJobProcesses; + private SagaContext sagaContext; /** * Will be overridden by any the label {@code PrepareTitusDeploy.USE_APPLICATION_DEFAULT_SG_LABEL} @@ -79,6 +81,11 @@ public Collection getApplications() { return Arrays.asList(application); } + @Override + public void setSagaContext(SagaContext sagaContext) { + this.sagaContext = sagaContext; + } + /** For Jackson deserialization. */ public void setApplications(List applications) { if (!applications.isEmpty()) { diff --git a/clouddriver-titus/src/main/groovy/com/netflix/spinnaker/clouddriver/titus/deploy/handlers/TitusDeployHandler.java b/clouddriver-titus/src/main/groovy/com/netflix/spinnaker/clouddriver/titus/deploy/handlers/TitusDeployHandler.java index 86cfb943b81..1a7aee7c11d 100644 --- a/clouddriver-titus/src/main/groovy/com/netflix/spinnaker/clouddriver/titus/deploy/handlers/TitusDeployHandler.java +++ b/clouddriver-titus/src/main/groovy/com/netflix/spinnaker/clouddriver/titus/deploy/handlers/TitusDeployHandler.java @@ -4,7 +4,6 @@ import com.netflix.spinnaker.clouddriver.data.task.TaskRepository; import com.netflix.spinnaker.clouddriver.deploy.DeployDescription; import com.netflix.spinnaker.clouddriver.deploy.DeployHandler; -import com.netflix.spinnaker.clouddriver.orchestration.SagaContextAware; import com.netflix.spinnaker.clouddriver.orchestration.events.CreateServerGroupEvent; import com.netflix.spinnaker.clouddriver.orchestration.sagas.LoadFront50App; import com.netflix.spinnaker.clouddriver.orchestration.sagas.LoadFront50App.LoadFront50AppCommand; @@ -24,15 +23,13 @@ import groovy.util.logging.Slf4j; import java.util.List; import java.util.Objects; -import javax.annotation.Nonnull; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Component; @Slf4j @Component -public class TitusDeployHandler implements DeployHandler, SagaContextAware { +public class TitusDeployHandler implements DeployHandler { private final SagaService sagaService; - private SagaContext sagaContext; @Autowired public TitusDeployHandler(SagaService sagaService) { @@ -46,7 +43,7 @@ private static Task getTask() { @Override public TitusDeploymentResult handle( final TitusDeployDescription inputDescription, List priorOutputs) { - Objects.requireNonNull(sagaContext); + Objects.requireNonNull(inputDescription.getSagaContext(), "A saga context must be provided"); SagaFlow flow = new SagaFlow() @@ -69,7 +66,7 @@ public TitusDeploymentResult handle( .sagaName(TitusDeployHandler.class.getSimpleName()) .inputDescription(inputDescription) .priorOutputs(priorOutputs) - .sagaContext(sagaContext) + .sagaContext(inputDescription.getSagaContext()) .task(getTask()) .sagaFlow(flow) .initialCommand( @@ -109,9 +106,4 @@ public TitusDeploymentResult handle( public boolean handles(DeployDescription description) { return description instanceof TitusDeployDescription; } - - @Override - public void setSagaContext(@Nonnull SagaContext sagaContext) { - this.sagaContext = sagaContext; - } }