fix(stack): stack tags are separate from other tags (under feature flag)

[rix0rrr]

Stacks are considered taggable, and so Tags.of(this).add('key', 'value') used to add tags to Stacks in scope. Usually this happens if this is an instance of Stack, which it commonly is in user code.

Since Tags.of(...) walks the construct tree, it will add tags to the stack and to all the resources in the stack. Then, come deploy time, CloudFormation will also try and apply all the stack tags to the resources again. This is silly and unnecessary.

In #28017, someone tries to use a CloudFormation intrisinc in a tag applied using Tags.of(this); that will work for resources as the tags are rendered into the template, but it will not work for the stack tags as those are passed via an API call, and intrinsics don't work there.

In this change

The correct solution to tagging all resources with an intrinsic would be to tag each of them individually, as tagging a Stack with an intrinsic is not possible. That's a poor user experience.

Resolve both the silly duplicate work as well as the "tagging with an intrinsic" use case as follows:

• Stacks no longer participate in the hierarchical Tags.of(...) tagging.
• Instead, only tags explicitly applied at the stack level (using the tags constructor property) are rendered as stack tags.

This requires a user to make a conscious decision between resource-level and stack-level tagging: either apply tags to the stack, which will apply it to all resources; or apply tags to (groups of) resources inside the template.

Another benefit is that for tags applied at the stack level, this will resolve the following issue: https://github.com/aws/aws-cdk/issues/15947, as resources "becoming" taggable all of a sudden will not affect the template anymore.

This change is under a feature flag, @aws-cdk/core:explicitStackTags.

Interested in thoughts

The duality of Stack tags vs resource tags, and the fact that we effectively try and tag all resources twice, has been a thorn in my side for a while. I took this issue as a chance to address that long-standing design flaw.

I can be convinced that it's too messy to explain, and not worth fixing (we haven't had complaints about this explicitly broken behavior in over 5 years), and we should just error out on the unresolvable tag and that's it.

Closes #28017.

---

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

[mrgrain]

The duality of Stack tags vs resource tags, and the fact that we effectively try and tag all resources twice, has been a thorn in my side for a while. I took this issue as a chance to address that long-standing design flaw.

I can be convinced that it's too messy to explain, and not worth fixing (we haven't had complaints about this explicitly broken behavior in over 5 years), and we should just error out on the unresolvable tag and that's it.

I think this makes a lot of sense and it's the right think to address the fundamental flaw. My concern is the implementation is using a feature flag and otherwise sticks to the very cool but also super vague Tags.of(this).add('key', 'value') naming. There will be many docs, articles and tutorials out there that reference this Aspect with the current behavior.

We could instead take this as an opportunity to deprecate the current aspect (and maybe add a warning/error to catch the unsupported use case) and implement the new feature as TagAllResources.of(this).add('key', 'value') or similar. Yes this will cause some additional work for our customers (if they care about deprecated features) but also forces them to re-think what they actually want.

[rix0rrr]

There will be many docs, articles and tutorials out there that reference this Aspect with the current behavior.

I considered this and was a bit worried about it.

Under the new behavior, Tags.of(this).add(key, value) will still tag all resources (presumably what people are most interested in), but not tag the Stack anymore. I didn't think that was that bad of a behavior. It's mostly what you want.

I suppose we could deprecate Tags and replace it with ResourceTags... but that will cause deprecation warnings in literally everyone's code and invalidate all tutorials... and is that worth it?

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: a91bdeed953d8643582c401681004f0057dd8f7b
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[comcalvi]

There's two other options:

1. (under feature flag) Make Tags.of(this).add(key, value) silently drop unresolved tokens, if it detects any; otherwise, it behaves as it does today.
2. Implement a post-deploy token resolver. This is probably too much effort for too little payoff, but we could put special markers in unresolved stack tags that we look up in AWS (presumably, a resource in the stack itself) and fill in after the deployment succeeds. We'd to store a token map in the cloud assembly.

I didn't think that was that bad of a behavior. It's mostly what you want.

I agree; it seems the only time you don't want it is when that tag contains a Token, so dropping the unresolved tokens would be nice; is that too much behavior hidden from users though?

I don't think the Tags.of() naming is too vague, CDK users have become familiar with it and it behaves as you would expect, bar this unresolved tag issue.

[rix0rrr]

(under feature flag) Make Tags.of(this).add(key, value) silently drop unresolved tokens, if it detects any; otherwise, it behaves as it does today.

I think silently ignoring what the user asks for is not great behavior.

Implement a post-deploy token resolver. This is probably too much effort for too little payoff,

Exactly 😉

[rix0rrr]

As I thought, this would be harder to close.

So I split off the most important bit to another PR: https://github.com/aws/aws-cdk/pull/31457