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

fix(ec2): Fix broken uuid match with other-endianness #5236

Merged
merged 2 commits into from
May 1, 2024

Conversation

holmanb
Copy link
Member

@holmanb holmanb commented Apr 29, 2024

fix(ec2): Fix broken uuid match with other-endianness

EC2 documents that the system-uuid may be reported in different endianness[1].

A user has reported a case where cloud-init is broken due to inability to
detect the system platform. Fix it.

Behavior change:

Cloud-init was previously making the assumption that uuid and serial would match
on ec2. This assumption was:

1) not documented as a valid way to identify ec2[1]

2) proven invalid on ec2 by the DMI_PRODUCT_SERIAL and DMI_PRODUCT_UUID reported in #5105

3) used in the logic which warns about not running on the "real" ec2

Preserving this warning logic exactly as it was presents several challenges:

a) Risk of regression outside of our control: Since this logic relied upon undocumented
   behavior, AWS could change this at any point, which would break all cloud-init
   instances.

b) Risk of incorrect implementation: What format is the uuid and product serial actually
   in? We don't know. It's easy and safe to just swap the byteorder of the first segment
   of the uuid because this is documented, but matching the whole uuid is problematic
   because UUID formats may be presented as mixed encoding (partially little endian and
   partially big endian). To implement this behavior while fixing this bug we would have
   to make even more assumptions than before. I propose we stop assuming and if a cloud
   happens to implement the same as EC2 (minus the serial/product match), then we just
   don't emit that warning. It's simpler, it's safer, and I really don't think that it is
   a huge change. This is a "change in behavior", but the change is that the code more
   correctly identifies EC2 and would no longer emit a warning on valid ec2 instances, so
   I don't think that this would require omitting this change from SRU.

c) Implementing whatever assumptions we make in b) would require implementing a
   byteswapping algorithm in POSIX shell, which is possible but best to avoid this if
   possible.

[1] https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/identify_ec2_instances.html

Fixes GH-5105

Additional Context

Cloud-init cannot identify that it is running on ec2 when the uuid is reported in swapped endianness, which is a valid documented way for ec2 to report.

#5105

Test Steps

See new unit tests.

Checklist

Merge type

  • Squash merge using "Proposed Commit Message"
  • Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

EC2 documents that the system-uuid may be reported in different
endianness.

A user has reported a case where cloud-init is broken due to inability to
detect the system platform. Fix it.

Fixes canonicalGH-5105
Copy link
Contributor

@aciba90 aciba90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for finding and fixing this issue. I left some non-0blocking questions, comments, suggestions.

cloudinit/sources/DataSourceEc2.py Outdated Show resolved Hide resolved
cloudinit/sources/DataSourceEc2.py Show resolved Hide resolved
cloudinit/sources/DataSourceEc2.py Show resolved Hide resolved
tests/unittests/sources/test_ec2.py Show resolved Hide resolved
@holmanb
Copy link
Member Author

holmanb commented Apr 30, 2024

LGTM, thanks for finding and fixing this issue. I left some non-0blocking questions, comments, suggestions.

Thanks for the review @aciba90. I've updated the commit message to better justify and explain the change. I also pushed another commit with the suggested changes.

@holmanb holmanb requested a review from aciba90 April 30, 2024 16:44
Copy link
Contributor

@aciba90 aciba90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the detailed explanations and fixing this!

@aciba90 aciba90 merged commit c1a19d7 into canonical:main May 1, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants