Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(cdk-core): CDK diff does not work with umlauts #25309

Closed
Xarno opened this issue Apr 26, 2023 · 5 comments · Fixed by #25912
Closed

(cdk-core): CDK diff does not work with umlauts #25309

Xarno opened this issue Apr 26, 2023 · 5 comments · Fixed by #25912
Labels
blocked Work is blocked on this issue for this codebase. Other labels or comments may indicate why. bug This issue is a bug. effort/small Small work item – less than a day of effort needs-cfn This issue is waiting on changes to CloudFormation before it can be addressed. p2 package/tools Related to AWS CDK Tools or CLI

Comments

@Xarno
Copy link

Xarno commented Apr 26, 2023

Describe the bug

When doing a CDK diff it always detect a change in the tags because the „ü“ in this case results in a „?“.
Although the Tags on the resource are correct including the umlaut as specified.

Expected Behavior

Diff command should be capable to handle all allowed chars for tags.

Current Behavior

[] AWS::IAM::Role VirtualGuideiaaInstance/Cert/Certificate/CertificateRequestorFunction/ServiceRole VirtualGuideiaaInstanceCertCertificateCertificateRequestorFunctionServiceRole3B312417
└─ [
] Tags
└─ @@ -5,7 +5,7 @@
[ ] },
[ ] {
[ ] "Key": "customer",
[-] "Value": "Messe M?nchen"
[+] "Value": "Messe München"
[ ] },
[ ] {
[ ] "Key": "environment",

Reproduction Steps

const app = new App();
// ...
Tags.of(app).add("Ära", "München")

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.76.0 (build 78c411b)

Framework Version

2.76.0

Node.js Version

v16.17.0

OS

Mac OS 13.3

Language

Typescript

Language Version

3.9.10

Other information

No response

@Xarno Xarno added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Apr 26, 2023
@github-actions github-actions bot added the package/tools Related to AWS CDK Tools or CLI label Apr 26, 2023
@pahud pahud added p2 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Apr 26, 2023
@pahud
Copy link
Contributor

pahud commented Apr 26, 2023

Yes I can reproduce this. Thank you for the report.

@rix0rrr rix0rrr added needs-cfn This issue is waiting on changes to CloudFormation before it can be addressed. blocked Work is blocked on this issue for this codebase. Other labels or comments may indicate why. labels Apr 28, 2023
@rix0rrr
Copy link
Contributor

rix0rrr commented Apr 28, 2023

We can reproduce it but not fix it. This is a limitation of the GetStackTemplate API offered by CloudFormation, which does not support unicode.

@peterwoodworth peterwoodworth added effort/small Small work item – less than a day of effort and removed effort/medium Medium work item – several days of effort labels May 18, 2023
@peterwoodworth
Copy link
Contributor

We've had multiple reports of this, making this the main thread

@DanielLaberge
Copy link

Could someone from the CloudFormation API Team confirm whether or not there is any chance of getting this addressed?

The fact that UTF-8 Characters are supported by many AWS APIs, but are mangled by the CloudFormation GetStackTemplate API is not only inconsistent and confusing, but potentially dangerous.

I wonder how CloudFormation's team would feel about the proposed solution to this issue which is currently being implemented in this PR. On one hand, I'm grateful there might be a solution coming to "fix" CDK diff, but on the other hand, the solution is more akin to a band-aid since it can't be fixed on the CDK side.

@peterwoodworth: It looks like inter-team communication might be required for a proper fix, and it would be appreciated if someone that can make it happen chimed in.

@mergify mergify bot closed this as completed in #25912 Jun 15, 2023
mergify bot pushed a commit that referenced this issue Jun 15, 2023
I am reopening this from #25525

and following up on my comments here:
#24557 (comment)
#24557 (comment)
#25008 (comment)
#25008 (comment)
#25008 (comment)
#25008 (comment)
#25008 (comment)
#25008 (comment)
#25525 (comment)
#25525 (comment)
🫠 #25525 (comment) 🫠

---

Fixes #25309
Fixes #22203
Fixes #20212
Fixes #13634
Fixes #10523
Fixes #10219
See also: aws-cloudformation/cloudformation-coverage-roadmap#1220
See also: aws-cloudformation/cloudformation-coverage-roadmap#814

---

👻 I have retitled this PR as a `chore` instead of a `fix` because @aws-cdk-automation keeps closing my PRs as abandoned even though they are clearly not abandoned.

> This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

---

@otaviomacedo @rix0rrr @TheRealAmazonKendra - I'm happy to adjust the approach, add more tests, or do what else needs to be done. I'm not getting any feedback from the team so I'm not sure how to proceed. The diff noise with non-ASCII information in cdk diff makes it difficult to find meaningful changes to our stacks.

🗿🗞️📬 **Crucially, this change only affects the CLI output and therefore an integration test isn't possible.**

---

CloudFormation's `GetStackTemplate` irrecoverably mangles any character not in the 7-bit ASCII range. This causes noisy output from `cdk diff` when a template contains non-English languages or emoji. We can detect this case and consider these strings equal.

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

Many AWS services accept non-ASCII input, eg many "description" fields. CloudFormation will correctly dispatch these templates but when invoking `GetStackTemplate` the result is mangled. This causes annoying noise in the output of `cdk diff`:

```
Resources
[~] AWS::Lambda::Function Lambda/Resource
 └─ [~] Description
     ├─ [-] ?????
     └─ [+] 🤦🏻‍♂️
```

This change modifies the diff algorithm to consider the string equal if the lvalue is a mangled version of the rvalue.

Of course this runs the risk of hiding changesets which modify only a single non-ASCII character to another non-ASCII character, but these fields already tend to be informative in nature.
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Work is blocked on this issue for this codebase. Other labels or comments may indicate why. bug This issue is a bug. effort/small Small work item – less than a day of effort needs-cfn This issue is waiting on changes to CloudFormation before it can be addressed. p2 package/tools Related to AWS CDK Tools or CLI
Projects
None yet
6 participants