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

Ytt removes quotes for hex strings, which makes them ambiguous #822

Open
Xander-Polishchuk opened this issue Mar 31, 2023 · 13 comments
Open
Assignees
Labels
bug This issue describes a defect or unexpected behavior can be replicated A bug that has been reproduced carvel accepted This issue should be considered for future work and that the triage process has been completed priority/unprioritized-backlog Higher priority than priority/awaiting-more-evidence but not planned. Contributions are welcome.

Comments

@Xander-Polishchuk
Copy link

Xander-Polishchuk commented Mar 31, 2023

What steps did you take:

echo 'hex-string: "0x5aAeb6053F3E94C9b9A09f33669435E7Ef1BeAed"' > template.yaml
ytt -f template.yaml

What happened:

quotes removed

What did you expect:

quotes preserved, as hex string format might contain specific encoding where string format required (e.g. upper/lower case letters provides checksum)

Environment:

  • ytt version (use ytt --version): 0.45.0
  • OS (e.g. from /etc/os-release): Fedora 36

Vote on this request

This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.

👍 "I would like to see this addressed as soon as possible"
👎 "There are other more important things to focus on right now"

We are also happy to receive and review Pull Requests if you want to help working on this issue.

@Xander-Polishchuk Xander-Polishchuk added bug This issue describes a defect or unexpected behavior carvel triage This issue has not yet been triaged for relevance labels Mar 31, 2023
@github-actions
Copy link

This issue is being marked as stale due to a long period of inactivity and will be closed in 5 days if there is no response.

@github-actions github-actions bot added the stale This issue has had no activity for a while and will be closed soon label May 11, 2023
@Xander-Polishchuk
Copy link
Author

Hey any updates? Still an issue!

@github-actions github-actions bot removed the stale This issue has had no activity for a while and will be closed soon label May 12, 2023
@vmunishwar vmunishwar self-assigned this May 17, 2023
@vmunishwar vmunishwar added can be replicated A bug that has been reproduced and removed carvel triage This issue has not yet been triaged for relevance labels Jun 4, 2023
@vmunishwar
Copy link
Contributor

vmunishwar commented Jun 6, 2023

@Xander-Polishchuk - Thanks for reporting this issue. Yes, we are able to replicate it.

This might be because ytt uses the yaml.v2 library to marshal the YAML.
We had similar issue fixed in ytt v0.37.0 for forcing quotes for strings.

However, this specific length hex string is not getting output as expected. Here are some examples that I ran on the ytt playground - if a string has 18 or less number of characters, the quotes are retained.
image

Also, we are exploring go yaml v3 parser to use in ytt for certain scenarios and that would probably resolve this issue because it preserves the original representation of octals, hexadecimals etc.

Meanwhile, please let us know if you are blocked on this or if any workaround worked for you. Thanks for reaching out.

@vmunishwar vmunishwar added the priority/unprioritized-backlog Higher priority than priority/awaiting-more-evidence but not planned. Contributions are welcome. label Jun 6, 2023
@Xander-Polishchuk
Copy link
Author

Hey @vmunishwar, thanks for a response.

The only workaround is fallback to decimal encoding, but such fallback removes ability to check checksum, therefore not the complete one and opens space for errors.

@prembhaskal
Copy link
Member

So i was checking this issue and this won't be fixed even if we use yaml v3 in ytt. There are open issues for exact problem in yaml project here go-yaml/yaml#435 and go-yaml/yaml#703

@prembhaskal prembhaskal added the wontfix This will not be worked on label Jan 4, 2024
@Xander-Polishchuk
Copy link
Author

That's very regretful decision, I wish you could contribute the fix to the yaml library or drop it altogether in favor of predictable results. Main while we considering dropping ytt in our projects in favor of other templating engines due to lack of WYSIWYG before and after templating.

@prembhaskal
Copy link
Member

prembhaskal commented Jan 7, 2024

@Xander-Polishchuk We will discuss this issue in our next community meeting to get input from experts.

  • I believe dropping yamlv3 for something else is definitely not an option right now (may be in future because yaml/v3 seems to be unmaintained)
  • Another option to overcome this issue could be to fix it in the vendored yaml.v3
  • Another would be to find a suitable workaround for the issue ( I understand you still don't have a workaround for it??)

@prembhaskal prembhaskal added carvel accepted This issue should be considered for future work and that the triage process has been completed and removed wontfix This will not be worked on labels Jan 9, 2024
@oxfn
Copy link

oxfn commented May 14, 2024

@Xander-Polishchuk just do

hex-string: !!str 0x5aAeb6053F3E94C9b9A09f33669435E7Ef1BeAed # even without quotes!

Result should be

hex-string: "0x5aAeb6053F3E94C9b9A09f33669435E7Ef1BeAed"

This is described in YAML spec

@Xander-Polishchuk
Copy link
Author

@oxfn doesn't work, ytt still omit quotes

echo 'hex-string: !!str "0x5aAeb6053F3E94C9b9A09f33669435E7Ef1BeAed"' > template.yaml
ytt -f template.yaml
# hex-string: 0x5aAeb6053F3E94C9b9A09f33669435E7Ef1BeAed

ytt version - develop

Also generally it won't be a good solution, just a workaround

@oxfn
Copy link

oxfn commented May 15, 2024

@Xander-Polishchuk
Well, we're both right. I've tested on small values like 0x1234 and it makes a string. Serialization behavior somehow depends on number value. This is how it works:

!!str 0x1234 -> "0x1234"
!!str 0x12345678abcdef -> "0x12345678abcdef"
!!str 0x12345678abcdef01 -> "0x12345678abcdef01"
!!str 0x12345678abcdef01f -> 0x12345678abcdef01f
!!str 0x012345678abcdef01 -> "0x012345678abcdef01" # WTF?
!!str 0x012345678abcdef012 -> 0x012345678abcdef012

!!str 0x000000000000000011112222 -> "0x000000000000000011112222" # ?!

In fact maximum value for string is 0xFFFFFFFFFFFFFFFF, thus 0x10000000000000000 will be serialized as number

@prembhaskal
Copy link
Member

@oxfn the issue is with the underlying go/yaml library go-yaml/yaml#435 , when a hex string is bigger than max integer representation in golang (for the int type), the parseInt fails and the number is treated as string and hence not quoted.

@Xander-Polishchuk
Copy link
Author

Are you considering go away from go-yaml? As it seems project is dead and doesn't accept any PRs and fixes.

@prembhaskal
Copy link
Member

@Xander-Polishchuk so we had discussed this issue in one of the community meetings and yes, we were considering if moving to https://github.com/kubernetes-sigs/yaml makes more sense now.
Though this particular issue exists there too. so one idea was to provide fix in the k8s-sigs/yaml (if they are open to PRs)
Other idea was to fork and fix issue ourselves (less people favored this one)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue describes a defect or unexpected behavior can be replicated A bug that has been reproduced carvel accepted This issue should be considered for future work and that the triage process has been completed priority/unprioritized-backlog Higher priority than priority/awaiting-more-evidence but not planned. Contributions are welcome.
Projects
Status: No status
Development

No branches or pull requests

4 participants