diff --git a/packages/@aws-cdk/pipelines/lib/helpers-internal/graph.ts b/packages/@aws-cdk/pipelines/lib/helpers-internal/graph.ts index 798cc207f0aeb..7ff5208a410ae 100644 --- a/packages/@aws-cdk/pipelines/lib/helpers-internal/graph.ts +++ b/packages/@aws-cdk/pipelines/lib/helpers-internal/graph.ts @@ -73,6 +73,14 @@ export class GraphNode { return x; } + public get rootGraph(): Graph { + 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 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 { - private readonly targets: GraphNode[] = []; - private readonly sources: GraphNode[] = []; + private readonly _producers: GraphNode[] = []; + private readonly _consumers: GraphNode[] = []; + /** + * Add a producer: make all nodes added by 'dependBy' depend on these + */ public dependOn(...targets: GraphNode[]) { 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[]) { 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> { + return this._consumers; + } + + public consumersAsString() { + return this.consumers.map(c => `${c}`).join(','); + } } -export class DependencyBuilders { +/** + * A set of dependency builders identified by a given key. + */ +export class DependencyBuilders { private readonly builders = new Map>(); - public get(key: K) { + public for(key: K) { const b = this.builders.get(key); if (b) { return b; } const ret = new DependencyBuilder(); 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]>(); + + for (const [k, builder] of this.builders.entries()) { + if (builder.hasUnsatisfiedConsumers) { + ret.push([k, builder]); + } + } + + return ret; + } } export interface GraphProps extends GraphNodeProps { @@ -304,12 +365,32 @@ export class GraphNodeCollection { this.nodes = Array.from(nodes); } + /** + * Add one or more dependencies to all nodes in the collection + */ public dependOn(...dependencies: Array | 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 { return paths[0][0]; } + + public toString() { + return this.nodes.map(n => `${n}`).join(', '); + } } /** diff --git a/packages/@aws-cdk/pipelines/lib/helpers-internal/pipeline-graph.ts b/packages/@aws-cdk/pipelines/lib/helpers-internal/pipeline-graph.ts index a411a2dfc547a..cc245cd58e4d4 100644 --- a/packages/@aws-cdk/pipelines/lib/helpers-internal/pipeline-graph.ts +++ b/packages/@aws-cdk/pipelines/lib/helpers-internal/pipeline-graph.ts @@ -54,7 +54,9 @@ export class PipelineGraph { private readonly assetNodesByType = new Map(); private readonly synthNode?: AGraphNode; private readonly selfMutateNode?: AGraphNode; - private readonly stackOutputDependencies = new DependencyBuilders(); + private readonly stackOutputDependencies = new DependencyBuilders(); + /** Mapping steps to depbuilders, satisfied by the step itself */ + private readonly nodeDependencies = new DependencyBuilders(); private readonly publishTemplate: boolean; private readonly prepareStep: boolean; private readonly singlePublisher: boolean; @@ -102,8 +104,7 @@ 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) { @@ -111,7 +112,7 @@ export class PipelineGraph { } 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()); 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'); diff --git a/packages/@aws-cdk/pipelines/test/blueprint/helpers-internal/pipeline-graph.test.ts b/packages/@aws-cdk/pipelines/test/blueprint/helpers-internal/pipeline-graph.test.ts index 747dcfcb91d77..f473d4e0cf001 100644 --- a/packages/@aws-cdk/pipelines/test/blueprint/helpers-internal/pipeline-graph.test.ts +++ b/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/); }); diff --git a/packages/@aws-cdk/pipelines/test/codepipeline/codepipeline.test.ts b/packages/@aws-cdk/pipelines/test/codepipeline/codepipeline.test.ts index b0ecae2286d1a..f7f1f4843dacf 100644 --- a/packages/@aws-cdk/pipelines/test/codepipeline/codepipeline.test.ts +++ b/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; } diff --git a/packages/@aws-cdk/pipelines/test/testhelpers/test-app.ts b/packages/@aws-cdk/pipelines/test/testhelpers/test-app.ts index fca2cd26f04d4..da35f1b979f4e 100644 --- a/packages/@aws-cdk/pipelines/test/testhelpers/test-app.ts +++ b/packages/@aws-cdk/pipelines/test/testhelpers/test-app.ts @@ -75,14 +75,28 @@ export class AppWithOutput extends Stage { } } +export interface TwoStackAppProps extends StageProps { + /** + * Create a dependency between the two stacks + * + * @default true + */ + readonly withDependency?: boolean; +} + export class TwoStackApp extends Stage { - constructor(scope: Construct, id: string, props?: StageProps) { + public readonly stack1: Stack; + public readonly stack2: Stack; + + constructor(scope: Construct, id: string, props?: TwoStackAppProps) { super(scope, id, props); - const stack2 = new BucketStack(this, 'Stack2'); - const stack1 = new BucketStack(this, 'Stack1'); + this.stack2 = new BucketStack(this, 'Stack2'); + this.stack1 = new BucketStack(this, 'Stack1'); - stack2.addDependency(stack1); + if (props?.withDependency ?? true) { + this.stack2.addDependency(this.stack1); + } } }