fix(tags): excludeResourceTypes does not work for tags applied at stack level (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 both unnecessary, as well as leads to loss of control: excludeResourceTypes appears to not work, since it will lead to resources not being tagged in the template (good) but then the resources will still be tagged by CloudFormation because the stack itself is tagged (bad).

Also, if the tags applied this way contain intrinsics, they will contain nonsense because they are applied in a context where CloudFormation expressions don't work.

In this change

There is way to prevent Stacks from being tagged, by including aws:cdk:stack in the list of excludeResourceTypes (this is a fake resource type that Stack tags respect).

Under a feature flag, @aws-cdk/core:explicitStackTags, this is now the default behavior. That resource type will be excluded by default, unless it is listed in the includeResourceTypes list. However, doing includeResourceTypes is still not desirable: stack tags should be applied directly on the Stack object if desired.

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 but remove the ability to do excludeResourceTypes; 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: #15947, as resources "becoming" taggable all of a sudden will not affect the template anymore.

Closes #28017. Closes #33945. Closes #30055.

---

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

[aws-cdk-automation]

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

[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?

[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: #31457

[moelasmar]

@rix0rrr, then should we close this PR ?

[github-actions]

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

[otaviomacedo]

If props.includeResourceTypes?.includes('aws:cdk:stack') is true, is there still a point in having the aspect traverse the tree? Can't we call addStackTag() to the stack and stop?

[rix0rrr]

Fair point, but if the user wanted exactly that behavior they would just set the stack tags directly.

I'm including this as an easy way of restoring the old behavior locally, for those people who might want it (for whatever ill-conceived reason). I don't seriously think it will see big adoption because it shouldn't, but I'm hesitant to take it away alltogether as well.

[aws-cdk-automation]

AWS CodeBuild CI Report

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

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