fix: deploy-time stack tags cause synthesis to fail #32041
Conversation
In #31457, we introduced a change that made synthesis fail if one of the stack tags was a deploy-time value. Since stack tags are assigned outside a CloudFormation context, deploy-time values cannot be evaluated, so the stack ends up with a tag like `{ Key: "my-tag", Value: "${Token[1234]}" }`, which is probably not what is intended. Worse, those tags are automatically propagated to all resources in the stack by CloudFormation, and some may validate the tag value and find that `$` or any of the other characters are not valid tag values. The intent was that customers would be alerted to these kinds of mistakes and apply their tags to resources, or skip stacks when applying tags to large scopes: ```ts Tags.of(this).add('my-tag', Fn.importValue('SomeExport'), { excludeResourceTypes: ['aws:cdk:stack'], }); ``` The previous change was a bit drastic in its attempts. In this one we ignore the unresolved tags and add a warning instead. That way, synthesis still succeeds.
There was a problem hiding this comment.
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.
rix0rrr
added
the
pr-linter/exempt-integ-test
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
aws-cdk-automation
added
the
pr/needs-maintainer-review
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
To add slightly more context, a secondary behavior of tagging a Stack, as mentioned in the PR description, is to propagate the tag to all resources within the Stack. The previous change resulted in a breaking change where a user could be applying a deploy-time tag to a Stack for the purpose of propagating it to the rest of the Stack resources, without needing the tag to apply to the Stack itself. This is not the ideal way to do this, since the stack type can be excluded as shown above, while still propagating the tag to all resources, and not triggering the thrown Error in the previous implementation. The goal of this change should be (from my perspective) to still skip the tagging of the Stack, but rather than throw an error, display a warning, and importantly still propagate the tag to resources within the Stack to avoid breaking prior implementations. It should be on the user to decide whether to implement a different tagging strategy at the suggestion of the warning, while keeping the same functionality as before if the warning is not implemented. All that being said, @rix0rrr can you confirm that this change keeps the propagating behavior of the tag to Stack resources even when the warning is triggered and the Stack tag is skipped? Assuming we don't want to introduce any breaking changes, the behavior should remain the same (don't tag Stack, still tag all resources) when a tag has a Token in it. i.e. the warning suggestion should be optional |
aws-cdk-automation
removed
the
pr/needs-maintainer-review
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Hey @mcintoac-aws, Yes I can confirm that. The reason because it works is because applying a Tag to a scope tags all the resources inside that scope. So the reason that when you tag a stack, all resources inside the stack are tagged as well is not because the tag is applied to the stack, but because the tag is applied to the stack and all resources inside it. The behavior in this PR only fails to apply the tag to the stack itself, but the propagating behavior of the tag is unchanged -- i.e., the tag is applied to the stack (dropped) and to all the resources (still applies). |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Comments on closed issues and PRs are hard for our team to see. |
In #31457, we introduced a change that made synthesis fail if one of the stack tags was a deploy-time value. Since stack tags are assigned outside a CloudFormation context, deploy-time values cannot be evaluated, so the stack ends up with a tag like
{ Key: "my-tag", Value: "${Token[1234]}" }, which is probably not what is intended.Worse, those tags are automatically propagated to all resources in the stack by CloudFormation, and some may validate the tag value and find that
$or any of the other characters are not valid tag values.The intent was that customers would be alerted to these kinds of mistakes and apply their tags to resources, or skip stacks when applying tags to large scopes:
The previous change was a bit drastic in its attempts. In this one we ignore the unresolved tags and add a warning instead. That way, synthesis still succeeds.
Closes #32040.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license