fix(pipelines): cross-stack step dependencies have wrong name (#23594)

We used to immediately recurse into the dependencies of a Step, and add a new Node for them to the current Graph if they didn't exist yet.

This was originally written like this to support `inputs` for the `ShellSteps`, which would automatically add in any necessary Nodes into the same Graph as necessary without explicitly adding them.

However, it would also have the effect that Steps which **would have** been added to the Graph would be parented incorrectly (they would be eagerly added to the wrong parent).

Do this in two stages now: first try to resolve all step dependencies between things that are added to the graph, then create new Nodes for missing Step dependencies later.

Fixes #21843.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Author: rix0rrr

packages/@aws-cdk/pipelines/lib/helpers-internal/graph.ts

@@ -73,6 +73,14 @@ export class GraphNode<A> {
     return x;
   }
 
+  public get rootGraph(): Graph<A> {
+    const root = this.root;
+    if (!(root instanceof Graph)) {
+      throw new Error(`Expecting a graph as root, got: ${root}`);
+    }
+    return root;
+  }
+
   public get parentGraph() {
     return this._parentGraph;
   }
@@ -93,50 +101,103 @@ export class GraphNode<A> {
 }
 
 /**
- * A dependency set that can be constructed partially and later finished
+ * A dependency set that is constructed over time
  *
  * It doesn't matter in what order sources and targets for the dependency
  * relationship(s) get added. This class can serve as a synchronization
  * point if the order in which graph nodes get added to the graph is not
  * well-defined.
  *
- * Useful utility during graph building.
+ * You can think of a DependencyBuilder as a vertex that doesn't actually exist in the tree:
+ *
+ *     ┌────┐               ┌────┐
+ *     │ P1 │◀─┐         ┌──│ S1 │
+ *     └────┘  │   .─.   │  └────┘
+ *             ├──( B )◀─┤
+ *     ┌────┐  │   `─'   │  ┌────┐
+ *     │ P2 │◀─┘         └──│ S2 │
+ *     └────┘               └────┘
+ *
+ * Ultimately leads to: { S1 -> P1, S1 -> P2, S2 -> P1, S2 -> P2 }.
  */
 export class DependencyBuilder<A> {
-  private readonly targets: GraphNode<A>[] = [];
-  private readonly sources: GraphNode<A>[] = [];
+  private readonly _producers: GraphNode<A>[] = [];
+  private readonly _consumers: GraphNode<A>[] = [];
 
+  /**
+   * Add a producer: make all nodes added by 'dependBy' depend on these
+   */
   public dependOn(...targets: GraphNode<A>[]) {
     for (const target of targets) {
-      for (const source of this.sources) {
+      for (const source of this._consumers) {
         source.dependOn(target);
       }
-      this.targets.push(target);
+      this._producers.push(target);
     }
     return this;
   }
 
+  /**
+   * Add a consumer: make these nodes depend on all nodes added by 'dependOn'.
+   */
   public dependBy(...sources: GraphNode<A>[]) {
     for (const source of sources) {
-      for (const target of this.targets) {
+      for (const target of this._producers) {
         source.dependOn(target);
       }
-      this.sources.push(source);
+      this._consumers.push(source);
     }
     return this;
   }
+
+  /**
+   * Whether there are any consumers (nodes added by 'dependBy') but no producers (nodes added by 'dependOn')
+   */
+  public get hasUnsatisfiedConsumers() {
+    return this._consumers.length > 0 && this._producers.length === 0;
+  }
+
+  public get consumers(): ReadonlyArray<GraphNode<A>> {
+    return this._consumers;
+  }
+
+  public consumersAsString() {
+    return this.consumers.map(c => `${c}`).join(',');
+  }
 }
 
-export class DependencyBuilders<K, A> {
+/**
+ * A set of dependency builders identified by a given key.
+ */
+export class DependencyBuilders<K, A=any> {
   private readonly builders = new Map<K, DependencyBuilder<A>>();
 
-  public get(key: K) {
+  public for(key: K) {
     const b = this.builders.get(key);
     if (b) { return b; }
     const ret = new DependencyBuilder<A>();
     this.builders.set(key, ret);
     return ret;
   }
+
+  /**
+   * @deprecated Use 'for'
+   */
+  public get(key: K) {
+    return this.for(key);
+  }
+
+  public unsatisfiedBuilders() {
+    const ret = new Array<[K, DependencyBuilder<A>]>();
+
+    for (const [k, builder] of this.builders.entries()) {
+      if (builder.hasUnsatisfiedConsumers) {
+        ret.push([k, builder]);
+      }
+    }
+
+    return ret;
+  }
 }
 
 export interface GraphProps<A> extends GraphNodeProps<A> {
@@ -304,12 +365,32 @@ export class GraphNodeCollection<A> {
     this.nodes = Array.from(nodes);
   }
 
+  /**
+   * Add one or more dependencies to all nodes in the collection
+   */
   public dependOn(...dependencies: Array<GraphNode<A> | undefined>) {
     for (const node of this.nodes) {
       node.dependOn(...dependencies.filter(isDefined));
     }
   }
 
+  /**
+   * Return the topographically first node in the collection
+   */
+  public first() {
+    const nodes = new Set(this.nodes);
+    const sorted = this.nodes[0].rootGraph.sortedLeaves();
+    for (const tranche of sorted) {
+      for (const node of tranche) {
+        if (nodes.has(node)) {
+          return node;
+        }
+      }
+    }
+
+    throw new Error(`Could not calculate first node between ${this}`);
+  }
+
   /**
   * Returns the graph node that's shared between these nodes
   */
@@ -351,6 +432,10 @@ export class GraphNodeCollection<A> {
 
     return paths[0][0];
   }
+
+  public toString() {
+    return this.nodes.map(n => `${n}`).join(', ');
+  }
 }
 
 /**

packages/@aws-cdk/pipelines/lib/helpers-internal/pipeline-graph.ts

@@ -54,7 +54,9 @@ export class PipelineGraph {
   private readonly assetNodesByType = new Map<AssetType, AGraphNode>();
   private readonly synthNode?: AGraphNode;
   private readonly selfMutateNode?: AGraphNode;
-  private readonly stackOutputDependencies = new DependencyBuilders<StackDeployment, any>();
+  private readonly stackOutputDependencies = new DependencyBuilders<StackDeployment>();
+  /** Mapping steps to depbuilders, satisfied by the step itself  */
+  private readonly nodeDependencies = new DependencyBuilders<Step>();
   private readonly publishTemplate: boolean;
   private readonly prepareStep: boolean;
   private readonly singlePublisher: boolean;
@@ -102,16 +104,15 @@ export class PipelineGraph {
       waves[i].dependOn(waves[i - 1]);
     }
 
-    // Add additional dependencies between steps that depend on stack outputs and the stacks
-    // that produce them.
+    this.addMissingDependencyNodes();
   }
 
   public isSynthNode(node: AGraphNode) {
     return this.synthNode === node;
   }
 
   private addBuildStep(step: Step) {
-    return this.addAndRecurse(step, this.topLevelGraph('Build'));
+    return this.addStepNode(step, this.topLevelGraph('Build'));
   }
 
   private addWave(wave: Wave): AGraph {
@@ -174,7 +175,7 @@ export class PipelineGraph {
 
       const cloudAssembly = this.cloudAssemblyFileSet;
 
-      firstDeployNode.dependOn(this.addAndRecurse(cloudAssembly.producer, retGraph));
+      firstDeployNode.dependOn(this.addStepNode(cloudAssembly.producer, retGraph));
 
       // add the template asset
       if (this.publishTemplate) {
@@ -195,7 +196,7 @@ export class PipelineGraph {
 
       // Add stack output synchronization point
       if (this.queries.stackOutputsReferenced(stack).length > 0) {
-        this.stackOutputDependencies.get(stack).dependOn(deployNode);
+        this.stackOutputDependencies.for(stack).dependOn(deployNode);
       }
     }
 
@@ -220,7 +221,7 @@ export class PipelineGraph {
 
   private addChangeSetNode(changeSet: Step[], prepareNode: AGraphNode, deployNode: AGraphNode, graph: AGraph) {
     for (const c of changeSet) {
-      const changeSetNode = this.addAndRecurse(c, graph);
+      const changeSetNode = this.addStepNode(c, graph);
       changeSetNode?.dependOn(prepareNode);
       deployNode.dependOn(changeSetNode);
     }
@@ -230,12 +231,12 @@ export class PipelineGraph {
     const currentNodes = new GraphNodeCollection(parent.nodes);
     const preNodes = new GraphNodeCollection(new Array<AGraphNode>());
     for (const p of pre) {
-      const preNode = this.addAndRecurse(p, parent);
+      const preNode = this.addStepNode(p, parent);
       currentNodes.dependOn(preNode);
       preNodes.nodes.push(preNode!);
     }
     for (const p of post) {
-      const postNode = this.addAndRecurse(p, parent);
+      const postNode = this.addStepNode(p, parent);
       postNode?.dependOn(...currentNodes.nodes);
     }
     return preNodes;
@@ -250,7 +251,12 @@ export class PipelineGraph {
     return ret as AGraph;
   }
 
-  private addAndRecurse(step: Step, parent: AGraph) {
+  /**
+   * Add a Node to a Graph for a given Step
+   *
+   * Adds all dependencies for that Node to the same Step as well.
+   */
+  private addStepNode(step: Step, parent: AGraph) {
     if (step === PipelineGraph.NO_STEP) { return undefined; }
 
     const previous = this.added.get(step);
@@ -267,21 +273,56 @@ export class PipelineGraph {
     parent.add(node);
     this.added.set(step, node);
 
+    // This used to recurse -- that's not safe, because it might create nodes in the
+    // wrong graph (it would create a dependency node, that might need to be created in
+    // a different graph, in the current one). Instead, use DependencyBuilders.
     for (const dep of step.dependencies) {
-      const producerNode = this.addAndRecurse(dep, parent);
-      node.dependOn(producerNode);
+      this.nodeDependencies.for(dep).dependBy(node);
     }
+    this.nodeDependencies.for(step).dependOn(node);
 
     // Add stack dependencies (by use of the dependency builder this also works
     // if we encounter the Step before the Stack has been properly added yet)
     for (const output of step.consumedStackOutputs) {
       const stack = this.queries.producingStack(output);
-      this.stackOutputDependencies.get(stack).dependBy(node);
+      this.stackOutputDependencies.for(stack).dependBy(node);
     }
 
     return node;
   }
 
+  /**
+   * Add dependencies that aren't in the pipeline yet
+   *
+   * Build steps reference as many sources (or other builds) as they want, which will be added
+   * automatically. Do that here. We couldn't do it earlier, because if there were dependencies
+   * between steps we didn't want to reparent those unnecessarily.
+   */
+  private addMissingDependencyNodes() {
+    // May need to do this more than once to recursively add all missing producers
+    let attempts = 20;
+    while (attempts-- > 0) {
+      const unsatisfied = this.nodeDependencies.unsatisfiedBuilders().filter(([s]) => s !== PipelineGraph.NO_STEP);
+      if (unsatisfied.length === 0) { return; }
+
+      for (const [step, builder] of unsatisfied) {
+        // Add a new node for this step to the parent of the "leftmost" consumer.
+        const leftMostConsumer = new GraphNodeCollection(builder.consumers).first();
+        const parent = leftMostConsumer.parentGraph;
+        if (!parent) {
+          throw new Error(`Consumer doesn't have a parent graph: ${leftMostConsumer}`);
+        }
+        this.addStepNode(step, parent);
+      }
+    }
+
+    const unsatisfied = this.nodeDependencies.unsatisfiedBuilders();
+    throw new Error([
+      'Recursion depth too large while adding dependency nodes:',
+      unsatisfied.map(([step, builder]) => `${builder.consumersAsString()} awaiting ${step}.`),
+    ].join(' '));
+  }
+
   private publishAsset(stackAsset: StackAsset): AGraphNode {
     const assetsGraph = this.topLevelGraph('Assets');
 

packages/@aws-cdk/pipelines/test/blueprint/helpers-internal/pipeline-graph.test.ts

@@ -340,8 +340,8 @@ describe('with app with output', () => {
     });
 
     // WHEN
-    const graph = new PipelineGraph(blueprint).graph;
     expect(() => {
+      const graph = new PipelineGraph(blueprint).graph;
       assertGraph(nodeAt(graph, 'Alpha')).sortedLeaves();
     }).toThrow(/Dependency cycle/);
   });

packages/@aws-cdk/pipelines/test/codepipeline/codepipeline.test.ts

@@ -8,7 +8,7 @@ import { Stack } from '@aws-cdk/core';
 import { Construct } from 'constructs';
 import * as cdkp from '../../lib';
 import { CodePipeline } from '../../lib';
-import { PIPELINE_ENV, TestApp, ModernTestGitHubNpmPipeline, FileAssetApp } from '../testhelpers';
+import { PIPELINE_ENV, TestApp, ModernTestGitHubNpmPipeline, FileAssetApp, TwoStackApp } from '../testhelpers';
 
 let app: TestApp;
 
@@ -298,6 +298,41 @@ describe('deployment of stack', () => {
   });
 });
 
+test('action name is calculated properly if it has cross-stack dependencies', () => {
+  // GIVEN
+  const pipelineStack = new cdk.Stack(app, 'PipelineStack', { env: PIPELINE_ENV });
+  const pipeline = new ModernTestGitHubNpmPipeline(pipelineStack, 'Cdk', {
+    crossAccountKeys: true,
+  });
+
+  // WHEN
+  const s1step = new cdkp.ManualApprovalStep('S1');
+  const s2step = new cdkp.ManualApprovalStep('S2');
+  s1step.addStepDependency(s2step);
+
+  // The issue we were diagnosing only manifests if the stacks don't have
+  // a dependency on each other
+  const stage = new TwoStackApp(app, 'TheApp', { withDependency: false });
+  pipeline.addStage(stage, {
+    stackSteps: [
+      { stack: stage.stack1, post: [s1step] },
+      { stack: stage.stack2, post: [s2step] },
+    ],
+  });
+
+  // THEN
+  const template = Template.fromStack(pipelineStack);
+  template.hasResourceProperties('AWS::CodePipeline::Pipeline', {
+    Stages: Match.arrayWith([{
+      Name: 'TheApp',
+      Actions: Match.arrayWith([
+        Match.objectLike({ Name: 'Stack2.S2', RunOrder: 3 }),
+        Match.objectLike({ Name: 'Stack1.S1', RunOrder: 4 }),
+      ]),
+    }]),
+  });
+});
+
 interface ReuseCodePipelineStackProps extends cdk.StackProps {
   reuseCrossRegionSupportStacks?: boolean;
 }