Skip to content

(core): (Aspects are not processed on portions of the construct tree, when other aspects dynamically modify the tree (e.g. cdk pipelines)) #21341

Closed
@DaniloTommasinaTR

Description

@DaniloTommasinaTR

Describe the bug

Aspects can modify the constructs' tree dynamically. The tree is modified when the Aspect is being visited.
If the modification happens in a higher node in the tree, then the aspects in this new branch are not processed, since the Aspects' processing recursion has already passed that level.

This is the case with CDK Pipelines, the "Source" stage is added in a higher location in the tree, causing tags and other aspects to not be added/processed for some elements of the dynamically generated tree.

In theory this can happen with other constructs that use a similar technique to dynamically extend the constructs' tree.

Expected Behavior

// Constructs' tree
Stack (with Tag aspect)
 +- Bucket
 +- Construct (with dynamic extension Aspect, adds Bucket to Stack)

After synthesis the tree should look like

// Constructs' tree
Stack (tagged)
 +- Bucket  (tagged)
 +- Construct  (tagged)
 +- Bucket (tagged)

Current Behavior

// Constructs' tree
Stack (with Tag aspect)
 +- Bucket
 +- Construct (with dynamic extension Aspect, adds Bucket to Stack)

After synthesis the tree looks like

// Constructs' tree
Stack (tagged)
 +- Bucket  (tagged)
 +- Construct  (tagged)
 +- Bucket (NOT TAGGED) <<<====

Reproduction Steps

Init sample app with:

mkdir /tmp/cdk-issue
cd /tmp/cdk-issue
cdk init sample-app --language typescript

Replace content of /tmp/cdk-issue/lib/cdk-issue-stack.ts with the following:

import { Aspects, Tags, IAspect, Stack, StackProps } from 'aws-cdk-lib';
import { Bucket } from 'aws-cdk-lib/aws-s3';
import { Construct, IConstruct } from 'constructs';

export class CdkIssueStack extends Stack {
  constructor(scope: Construct, id: string, props?: StackProps) {
    super(scope, id, props);
    
    Tags.of(this).add('test-tag', 'test-value');
    new MyConstruct(this, 'myConstruct');
  }
}

class MyConstruct extends Construct {

  constructor(scope: IConstruct, id: string) {
    super(scope, id);

    const stack = Stack.of(scope);
    const s3Bucket = new Bucket(stack, 'bucket-with-tags');
    
    Aspects.of(this).add(new MyDynAspect());
  }
}

class MyDynAspect implements IAspect {
  private processed = false;
  
  public visit(node: IConstruct): void {
    if (!this.processed) {
      //IMPORTANT: The bucket is added to the parent of the construct where the aspect it was initialized
      const stack = Stack.of(node);
      const s3Bucket = new Bucket(stack, 'bucket-without-tags-that-should-have');
      
      this.processed = true;
    }
  }
}
npm run build
cdk synth

===> Tags should be present on both buckets, but tags are set only on fist bucket.

Possible Solution

In aws-cdk-lib/core/lib/private/synthesis.ts

Enhance function invokeAspects as follows:

invokeAspects = function (root: IConstruct): void {

    const invokedByPath: { [nodePath: string]: IAspect[] } = {};

    let nestedAspectWarning = false;
    
    // CHANGE: repeat full recursion until no more invocations happen
    let noOfInvocationsInLastFullRecursion;
    do {
        noOfInvocationsInLastFullRecursion = recurse(root, []);
    } while(noOfInvocationsInLastFullRecursion > 0);

    function recurse(construct: IConstruct, inheritedAspects: IAspect[]): number {
        let noOfInvocations = 0;
        const node = construct.node;
        const aspects = Aspects.of(construct);
        const allAspectsHere = [...inheritedAspects ?? [], ...aspects.all];
        const nodeAspectsCount = aspects.all.length;
        for (const aspect of allAspectsHere) {
            let invoked = invokedByPath[node.path];
            if (!invoked) {
                invoked = invokedByPath[node.path] = [];
            }

            if (invoked.includes(aspect)) { continue; }

            aspect.visit(construct);
            noOfInvocations++;

            // if an aspect was added to the node while invoking another aspect it will not be invoked, emit a warning
            // the `nestedAspectWarning` flag is used to prevent the warning from being emitted for every child
            if (!nestedAspectWarning && nodeAspectsCount !== aspects.all.length) {
                Annotations.of(construct).addWarning('We detected an Aspect was added via another Aspect, and will not be applied');
                nestedAspectWarning = true;
            }

            // mark as invoked for this node
            invoked.push(aspect);
        }

        for (const child of construct.node.children) {
            if (!Stage.isStage(child)) {
                noOfInvocations += recurse(child, allAspectsHere);
            }
        }

        return noOfInvocations;
    }
}

The above addition will repeat the recursion on the full tree as long as any new invocation happens.

Additional Information/Context

This affects CDK Pipelines, tagging (and other Aspects) are not propagated to all stages, which is a key blocker within our company.

CDK CLI Version

2.31.2

Framework Version

No response

Node.js Version

v14.19.1

OS

All

Language

Typescript, Python, .NET, Java, Go

Language Version

all

Other information

No response

Metadata

Metadata

Assignees

Labels

@aws-cdk/coreRelated to core CDK functionalitybugThis issue is a bug.effort/largeLarge work item – several weeks of effortp1

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions