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.38k stars 3.79k forks source link

(core & cloudformation-include): Tags are autosorted by default, causing issues when adopting an older stack #21474

Open Thomas-X opened 1 year ago

Thomas-X commented 1 year ago

Describe the feature

Tags are automatically sorted which causes an undeployable setup if using https://github.com/aws/aws-cdk/tree/main/packages/@aws-cdk/cloudformation-include for a big sized CloudFormation stack.

The place of evil: https://github.com/aws/aws-cdk/blob/main/packages/@aws-cdk/core/lib/tag-manager.ts#L413

Use Case

Currently when you want to use the cloudformation-include package to adopt a legacy big stack CDK just craps out because CDK is trying to sort the tags of the stack, which it shouldn't do for this case.

Proposed Solution

There should be a feature flag when using CfnInclude from the cloudformation-include package to NOT try to sort the tags.

Other Information

No response

Acknowledgements

CDK version used

1.167.0

Environment details (OS name and version, etc.)

Windows 10

rix0rrr commented 1 year ago

Can you go into why sorting tags is a problem?

github-actions[bot] commented 1 year ago

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

Thomas-X commented 1 year ago

In my case I had the cloudformation deployment failing because after using cloudformation-include to import a stack, it sorted the tags. This also included cloudfront distributions, a lot of them. So naturally the cloudformation deployment fails as cloudfront will timeout rather quickly if it receives too many requests at once (GeneralServiceError).

I couldn't successfully deploy the stack because CDK was trying to sort the tags of the existing template stack which it really shouldn't do without an escape hatch being possible.

I managed to do it in the end by manually sorting the tags of 100+ cloudfront distros.

ranman commented 1 year ago

There should be a method to disable the sorting of cfninclude items lexicographically - this generates large stack diffs that generate no material changes.

These large diffs make it difficult to manually verify that no unintended stack changes are being introduced and reduces confidence when migrating from cloudformation to CDK.

rix0rrr commented 1 year ago

If I'm understanding correctly, the problem is that if a CloudFront distribution goes from:

  Tags: 
    - { Key: 'a', Value: 'a' }
    - { Key: 'b', Value: 'b' }

# ----> 

  Tags: 
    - { Key: 'b', Value: 'b' }
    - { Key: 'a', Value: 'a' }

That leads to TagResource and UntagResource calls? And too many of these leads to a GeneralServiceError even?

I understand what I'm about to say is of little comfort if you're affected by this issue, but that seems like a problem with CloudFront and/or its CloudFormation resource handler, and should be reported there.

ranman commented 1 year ago

While the resource issue is indeed a cloudfront issue it actively prevents adoption of CDK.

The problem is preserving immutable deployments and minimizing generated change sets.

Having the option to disable the sorting of keys on imported templates would allow for immutable deploys of imported templates and gradual migration to CDK.

Because CDK currently sorts all the keys it's very difficult to review generated changesets. In large enterprises attempting to adopt CDK they have legacy CFN change set review processes that make the task of reviewing large changesets time intensive.

The recommendation we've been making so far is to first deploy the tagging only changeset. Then subsequently deploy all other changes. This works for many resources but you also run into issues like the one highlighted above.

Key order of the original imported template should, optionally, be preserved.

ranman commented 1 year ago

We've seen this come up more and more as people adopt CDK.

A specific scenario: It's especially annoying for people working with MAP as they're trying to confirm their tags are unchanged when incrementally migrating to CDK from raw CFN. Broken MAP tags can cause significant financial loss for customers.

beckleyc commented 2 months ago

+1, this is causing a lot of headaches