aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.65k stars 3.91k forks source link

aws_core: NestedStack tags cannot override parent stack tags in constructor #30996

Open sumpfork opened 3 months ago

sumpfork commented 3 months ago

Describe the bug

In this python code snippet:

app = aws_cdk.App()

stack1 = aws_cdk.Stack(app, "stack1")
tags = aws_cdk.Tags.of(stack1)
tags.add("foo", "bar")

stack2 = aws_cdk.NestedStack(stack1, "stack2")
tags = aws_cdk.Tags.of(stack2)
tags.add("foo", "zap", priority=2000)

The nested stack has tag "foo": "bar".

Expected Behavior

I expected stack2 to have tag "foo":"zap" but it has tag "foo":"bar" inherited from the parent stack. Similarly, removing a tag that was added in the parent stack does not work, but adding a new ones does.

Current Behavior

The inherited parent stack's tags cannot be modified by the nested stack.

Reproduction Steps

import aws_cdk

app = aws_cdk.App()

stack1 = aws_cdk.Stack(app, "stack1")
tags = aws_cdk.Tags.of(stack1)
tags.add("foo", "bar")

stack2 = aws_cdk.NestedStack(stack1, "stack2")
tags = aws_cdk.Tags.of(stack2)
tags.add("foo", "zap", priority=2000)

app.synth()

Observe that stack2 in the resulting cloudformation template has tag "foo":"bar".

Possible Solution

If this is expected behaviour I feel it should at least be documented (unless I missed that documentation somewhere).

Additional Information/Context

No response

CDK CLI Version

2.150.0 (build 3f93027)

Framework Version

2.149.0

Node.js Version

v20.12.1

OS

Mac os X 14.5

Language

Python

Language Version

Python 3.10.11

Other information

No response

sumpfork commented 3 months ago

I simplified the example (I was wrong before, this also doesn't work outside of constructors).

sumpfork commented 3 months ago

As another note, this does work if you access the L1 construct to add the second tag directly:

cfn_nested_stack = stack2.node.default_child
cfn_nested_stack.tags.set_tag(
    "foo", "baz", priority=300
)
khushail commented 3 months ago

Hi @sumpfork , thanks for reaching out.

This is my analysis -

The tags are implemented using Aspects and the way nested stack would work is , one has to mention the Parent stack in the scope value(thats how CDK knows its a nested stack) of the Nested stack construct. If you add tags to a scope that includes a NestedStack construct, the CDK propagates the tags down to the nested stack's child resources at synth-time.

https://github.com/aws/aws-cdk/blob/9295a85a8fb893d7f5eae06108b68df864096c4c/packages/aws-cdk-lib/core/lib/tag-aspect.ts#L147

you can add as many as tags using

cdk.Tags.of(nestedStack).add('tag2', 'nestedStack03');

but I am skeptical about changing the tag obtained from Parent stack. I have not tried it though, will try it out.

khushail commented 3 months ago

I also see that existing documentation covers these -

  1. Tagging single construct
  2. Tags - Manages AWS tags for all resources within a construct scope.
  3. Tag - The Tag Aspect will handle adding a tag to this node and cascading tags to children.

Having clear documentation around tagging would definitely be very helpful.

pahud commented 3 months ago

Tags.of(scope).add('foo', 'bar'); is essentially to set tags for "resources in that scope".

Consider this example:

export class DummyStack extends Stack {
  constructor(scope: Construct, id: string, props?: StackProps) {
    super(scope, id, props);

    // create a dummy sqs.Queue
    new sqs.Queue(this, 'Queue1');

    // set tags for everything within current stack, including Queue1
    Tags.of(this).add('foo', 'bar');

    const nested = new NestedStack(this, 'NestedStack');
    // place Queue2 in the nested stack
    new sqs.Queue(nested, 'Queue2');

    // set everything in the nested stack, including the Queue2
    Tags.of(nested).add('foo', 'zap', { priority: 1000 });

  }
} 

Now if you check the synthesized templates in cdk.out.

Queue1 is having foo:bar, which makes sense.

  "Type": "AWS::SQS::Queue",
   "Properties": {
    "Tags": [
     {
      "Key": "foo",
      "Value": "bar"
     }

And the template of nested stack would be in another template file under cdk.out

And if you check that

Queue2 is having foo:zap

  "Queue26CB7866F": {
   "Type": "AWS::SQS::Queue",
   "Properties": {
    "Tags": [
     {
      "Key": "foo",
      "Value": "zap"
     }
    ]
   },
khushail commented 3 months ago

@sumpfork , I tried to change the nested stack tags using the node.defaultchild().

const cfnstack = nestedStack.node.defaultChild as CfnStack;
cfnstack.tags.setTag('tag1', 'parentStackTagValueReplaced');

It does not work and the reason is Aspects. Since the tagging of nested stack is done during synth time, the Aspects take over and having the highest priority, they override the set value to previous value propagated by parent stack . So the tags from the Parent stack might not be changeable.

(credits: @pahud )

However I am marking this issue for core team's input here. A clear documentation would really help in this regard!

comcalvi commented 2 months ago

We can't change Tags.of() to allow child constructs to override the parent's tags. We should clarify our docs around this.