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

[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

[otaviomacedo]

After looking into this, I don't think this is a good idea.

If we do multiple passes in the construct tree, nested aspects that were added in the first pass will be executed in the second pass. This is a behavior that we are explicitly ruling out. To understand the consequences of dropping this constraint, remember that every node in the tree inherits the aspects from all its ancestors. This means that nested aspects get replicated as you go down the tree. As an illustration, consider this scenario (taken from a unit test):

const app = new App();
const root = new MyConstruct(app, 'MyConstruct');
const child = new MyConstruct(root, 'ChildConstruct');
Aspects.of(root).add({
  visit(construct: IConstruct) {
    Aspects.of(construct).add({
      visit(inner: IConstruct) {
        inner.node.addMetadata('test', 'would-be-ignored');
      },
    });
  },
});

In the fist pass, the outer aspect adds the inner aspect to the root and to the child. Note that, to Node.js, these are two different objects. In the second pass, when the root is visited, the nested aspect will be executed once. When the child is visited, however, it will be executed twice (once for its own version of the nested aspect, and once for the nested aspect inherited from the root). If we add a grandchild, the aspect would be executed three times there, and so on. And this problem is two-dimensional. If you have an aspect that adds another aspect, that adds another aspect, you multiply the number of nested aspects by the level of the node in tree.

@DaniloTommasinaTR If you have a suggestion to avoid this problem while allowing aspects to operate on new nodes, please let us know.

[DaniloTommasinaTR]

Hi @otaviomacedo

Thanks for looking into this.

This PR is not about removing the constraint, the following check stays as it was before:

            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;
            }

The check to avoid the same aspect being visited twice on the same node stays also as it was:

            let invoked = invokedByPath[node.path];
            if (!invoked) {
                invoked = invokedByPath[node.path] = [];
            }

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

            // ...

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

What my code does, is just ensuring that any new nodes (and only those) added during one pass of the tree are being visited in the following pass. This is repeated as long as all nodes have been processed exactly once, respectively until no new nodes are processed in a pass.

If the dynamically created nodes add nested aspects, then the warning will be created as it was before.
The issue is about aspects adding nodes, not aspects adding new aspects. The dynamically added nodes without the fix do not follow the aspects inheritance rules, with the fix they do.

I tested with your nested aspects and I do not see a difference in behavior with my fix, the warning is shown and nested aspects are not processed, I just see the aspects inherited from the parent nodes being processed as expected.

[otaviomacedo]

@DaniloTommasinaTR the problem is not the warning. It's the fact that the warning will no longer be true. The nested aspects will be applied. Right now, nested aspects don't get applied because each node is visited only once. So even if an aspect creates another aspect, the nested one will simply be left behind and never executed. With your proposed change, these nested aspects will be picked up in the subsequent pass.

Suppose you have an aspect A that creates another aspect B. And initially you have the following tree, in which only the root has an aspect, A, attached to it:

Root (A) <-- Child ()

After the first pass, as a result of executing A, you have:

Root (A, B1) <-- Child (B2)

Notice that B1 and B2 are different aspects, even though they are functionally equivalent. Both were created by A. So the child effectively has two aspects: B2, that is directly attached to it, and B1, which it inherited from its parent.

In the second pass, A will no longer be executed, but the Bs will. So after this second pass, not only will the nested aspects have been executed (contradicting our warning), but also executed three times (once for the root and twice for the child). In general, for each node at a distance $d$ from the root, the aspect will be executed $d + 1$ times.

[DaniloTommasinaTR]

@otaviomacedo ok I see now what you mean, thanks for the details.
This is why I had the processed check in my example MyDynAspect, it avoids an endless recursion because of the inheritance of an Aspect to its children. Sorry it is a while back where I filed this issue.

So my question here is, if Aspects adding other Aspects are not permitted, respectively do not produce any result other than a warning, why not throwing an error then instead of a warning? Do you expect any regression error with existing stacks?
As it is today CDK behaves inconsistently, there are e.g. CDK pipelines using Aspects to dynamically extend the tree but then the dynamically added resources to not get tagged as expected, and so on.

Maybe it could be also an option to allow disabling inheritance for Aspects as a general feature e.g. in the Aspects.of(...).add() could take an extra argument inherit = true, but this is another topic.

[postsa]

I ran into this issue as well and I agree with @DaniloTommasinaTR that I would almost expect an error from CDK in this case. Short of that, it might be worth an update to the documentation, unless I missed something that already exists.

I was looking at using aspects to add a third party construct to nodes in a stack, but lost tags as a result. In this case because the third party used the Tags aspect to add tagging associated with their construct, which no op'd with the aforementioned warning.

Is it safe to conclude that it is inadvisable to use aspects to interact with third party constructs? I cannot control whether they use aspects themselves (either now or in the future), and if they do, I won't receive any functionality from them that was added via aspects.

[juadavard]

Came to a similar error where I wanted to tag certain resources by using a simbol so I am using aspects to check for such symbol, but then I would like to tag the node if is flagged

public visit(node: Construct): void { if (Resource.isX(node)) { Tags.of(node).add('tag', 'value') } }

But get the same problem where it would not tag the resource, did you find another way @postsa ?

[swachter]

The Taggging documentation includes the following example code that tries to tag constructs by an aspect:

class PathTagger implements cdk.IAspect {
  visit(node: IConstruct) {
    new cdk.Tag("aws-cdk-path", node.node.path).visit(node);
  }
}
 
stack = new MyStack(app);
cdk.Aspects.of(stack).add(new PathTagger())

It seems that this code does not work as of CDK 2.144.0.

However, I finally found a solution that directly uses the TagManager. For example this aspect tags log groups with their name:

export class LogGroupTagger implements cdk.IAspect {
    public visit(construct: IConstruct): void {
        if (construct instanceof cdk.aws_logs.LogGroup) {
            const cfnLogGroup = construct.node.defaultChild as CfnLogGroup;
            if (cfnLogGroup.logGroupName) {
                if (TagManager.isTaggable(cfnLogGroup)) {
                    cfnLogGroup.tags.setTag('Name', cfnLogGroup.logGroupName)
                } else {
                    Annotations.of(construct).addError('Could not tag log group; default child is not taggable')
                }
            } else {
                Annotations.of(construct).addError('Could not tag log group; missing log group name')
            }
        }
    }
}

[sumupitchayan]

@DaniloTommasinaTR have you validated that the enhanced invokedAspects function you wrote works as expected? I am trying to reproduce this fix but still don't see the second tag being created

[DaniloTommasinaTR]

Hi @sumupitchayan

It's a while back, I think my proposed fix was working on version 2.31.2, but we have now 2.162.1 and things might have changed...
However looking at the invokeAspects() code, I do not see any changes in the last 4 years, so in theory it should still be working.

I re-checked the code and to me it seems it should work as expected, but I would need some time to setup the environment to test it again and I am pretty tied up at the moment. :(

[sumupitchayan]

Can confirm that we are able to reproduce this issue. Looking into it

[sumupitchayan]

@DaniloTommasinaTR we are writing an RFC to redesign how Aspects are applied in CDK. Would love your feedback/thoughts since you are the one who brought this issue to our attention!

aws/aws-cdk-rfcs#651

[DaniloTommasinaTR]

Hi @sumupitchayan
Thanks for pushing this forward, I'll gladly have a look and provide feedback.