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.
spinnaker · Oct 3, 2019 · 420256f · 420256f
1 parent 74fb2a5
commit 420256f
Show file tree

Hide file tree

Showing 6 changed files with 31 additions and 23 deletions.
diff --git a/...ore/src/main/groovy/com/netflix/spinnaker/clouddriver/deploy/DeployAtomicOperation.groovy b/...ore/src/main/groovy/com/netflix/spinnaker/clouddriver/deploy/DeployAtomicOperation.groovy
@@ -23,14 +23,15 @@ 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<DeploymentResult>, SagaContextAware {
   private static final String TASK_PHASE = "DEPLOY"
 
   @Autowired
   DeployHandlerRegistry deploymentHandlerRegistry
 
   private final DeployDescription description
-  SagaContext sagaContext
 
   DeployAtomicOperation(DeployDescription description) {
     this.description = description
@@ -54,10 +55,6 @@ class DeployAtomicOperation implements AtomicOperation<DeploymentResult>, 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<DeploymentResult>, 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/...in/groovy/com/netflix/spinnaker/clouddriver/orchestration/AtomicOperationConverter.groovy b/...in/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/...re/src/main/groovy/com/netflix/spinnaker/clouddriver/orchestration/OperationsService.java b/...re/src/main/groovy/com/netflix/spinnaker/clouddriver/orchestration/OperationsService.java
@@ -174,15 +174,16 @@ private List<AtomicOperationBindingResult> 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/.../spinnaker/clouddriver/titus/deploy/converters/TitusDeployAtomicOperationConverter.groovy b/.../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/...vy/com/netflix/spinnaker/clouddriver/titus/deploy/description/TitusDeployDescription.java b/...vy/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<String> 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<String> getApplications() {
     return Arrays.asList(application);
   }
 
+  @Override
+  public void setSagaContext(SagaContext sagaContext) {
+    this.sagaContext = sagaContext;
+  }
+
   /** For Jackson deserialization. */
   public void setApplications(List<String> applications) {
     if (!applications.isEmpty()) {

diff --git a/...in/groovy/com/netflix/spinnaker/clouddriver/titus/deploy/handlers/TitusDeployHandler.java b/...in/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<TitusDeployDescription>, SagaContextAware {
+public class TitusDeployHandler implements DeployHandler<TitusDeployDescription> {
   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;
-  }
 }