Skip to content

mmds: Smoother migration from v1 to v2 #5290

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

Merged
merged 10 commits into from
Jul 16, 2025

Conversation

zulinx86
Copy link
Contributor

@zulinx86 zulinx86 commented Jul 4, 2025

Reason / Changes

To allow easier migration from MMDS v1 to v2, made the following changes:

  • Extends MMDS to support EC2 IMDS-compatible token headers (i.e. X-aws-ec2-metadata-token and X-aws-ec2-metadata-token-ttl-seconds).
  • Adds an integration test that checks AWS SDK for Python (boto3) is able to get AWS credentials via MMDS out of the box.
  • Makes MMDS versions 1 support token generation for the session-oriented method as in MMDS version 2. Note that MMDS version 1 continues to accept GET requests with invalid tokens or no token.
  • Adds mmds.rx_invalid_token and mmds.rx_no_token metrics to allow users to keep track of the number of GET requests that were rejected in MMDS version 2. Even with MMDS version 1 configured, they also count GET requests that would be rejected under MMDS version 1.

Fixed some minor things

  • Validates X-metadata-token-ttl-seconds header only if it is a PUT request to /latest/api/token.
  • Rejectes X-Forwarded-For header in a case-insensitive way.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.

PR Checklist

  • I have read and understand CONTRIBUTING.md.
  • I have run tools/devtool checkstyle to verify that the PR passes the
    automated style checks.
  • I have described what is done in these changes, why they are needed, and
    how they are solving the problem in a clear and encompassing way.
  • I have updated any relevant documentation (both in code and in the docs)
    in the PR.
  • I have mentioned all user-facing changes in CHANGELOG.md.
  • [ ] If a specific issue led to this PR, this PR closes the issue.
  • [ ] When making API changes, I have followed the
    Runbook for Firecracker API changes.
  • I have tested all new and changed functionalities in unit tests and/or
    integration tests.
  • [ ] I have linked an issue to every new TODO.

  • This functionality cannot be added in rust-vmm.

Copy link

codecov bot commented Jul 4, 2025

Codecov Report

Attention: Patch coverage is 97.24771% with 3 lines in your changes missing coverage. Please review.

Project coverage is 83.02%. Comparing base (ea87024) to head (6f4b604).
Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
src/vmm/src/rpc_interface.rs 84.61% 2 Missing ⚠️
src/vmm/src/device_manager/persist.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5290      +/-   ##
==========================================
+ Coverage   82.95%   83.02%   +0.06%     
==========================================
  Files         250      250              
  Lines       26879    26828      -51     
==========================================
- Hits        22298    22273      -25     
+ Misses       4581     4555      -26     
Flag Coverage Δ
5.10-c5n.metal 83.47% <97.24%> (+0.01%) ⬆️
5.10-m5n.metal 83.45% <97.24%> (+<0.01%) ⬆️
5.10-m6a.metal 82.68% <97.24%> (+0.01%) ⬆️
5.10-m6g.metal 79.13% <97.24%> (+<0.01%) ⬆️
5.10-m6i.metal 83.45% <97.24%> (+0.01%) ⬆️
5.10-m7a.metal-48xl 82.66% <97.24%> (?)
5.10-m7g.metal 79.13% <97.24%> (+<0.01%) ⬆️
5.10-m7i.metal-24xl 83.42% <97.24%> (?)
5.10-m7i.metal-48xl 83.42% <97.24%> (?)
5.10-m8g.metal-24xl 79.12% <97.24%> (?)
5.10-m8g.metal-48xl 79.12% <97.24%> (?)
6.1-c5n.metal 83.51% <97.24%> (+0.01%) ⬆️
6.1-m5n.metal 83.51% <97.24%> (+0.01%) ⬆️
6.1-m6a.metal 82.72% <97.24%> (+0.01%) ⬆️
6.1-m6g.metal 79.13% <97.24%> (+0.01%) ⬆️
6.1-m6i.metal 83.51% <97.24%> (+0.01%) ⬆️
6.1-m7a.metal-48xl 82.72% <97.24%> (?)
6.1-m7g.metal 79.13% <97.24%> (+<0.01%) ⬆️
6.1-m7i.metal-24xl 83.52% <97.24%> (?)
6.1-m7i.metal-48xl 83.52% <97.24%> (?)
6.1-m8g.metal-24xl 79.12% <97.24%> (?)
6.1-m8g.metal-48xl 79.12% <97.24%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@zulinx86 zulinx86 force-pushed the mmds_compat branch 17 times, most recently from d1f34a0 to fc485e6 Compare July 7, 2025 08:45
@zulinx86 zulinx86 force-pushed the mmds_compat branch 2 times, most recently from 067665a to 189c487 Compare July 11, 2025 15:36
@zulinx86 zulinx86 changed the title [DRAFT] mmds: Smoother migration from v1 to v2 mmds: Smoother migration from v1 to v2 Jul 11, 2025
@zulinx86 zulinx86 marked this pull request as ready for review July 11, 2025 15:36
@zulinx86 zulinx86 force-pushed the mmds_compat branch 2 times, most recently from 8eedccb to a9ff226 Compare July 15, 2025 16:28
@zulinx86 zulinx86 enabled auto-merge (rebase) July 15, 2025 16:28
@zulinx86 zulinx86 requested review from Manciukic and roypat July 15, 2025 16:31
Manciukic
Manciukic previously approved these changes Jul 15, 2025
Copy link
Contributor

@Manciukic Manciukic left a comment

Choose a reason for hiding this comment

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

LGTM!

         .
         _\____
        |_===__`.        ==/
        \/  '---"\ _ _ _ _/
  ______/_______/_|_|_|_|_|
_|--------------------==."
 \____________________.'  LGB
------------------------------------------------

@Manciukic
Copy link
Contributor

Clippy is angry at you 🫨

error: this expression creates a reference which is immediately dereferenced by the compiler
   --> src/vmm/src/mmds/mod.rs:185:31
    |
185 |     match mmds.is_valid_token(&token) {
    |                               ^^^^^^ help: change this to: `token`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
    = note: `-D clippy::needless-borrow` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::needless_borrow)]`

@zulinx86
Copy link
Contributor Author

Clippy is angry at you 🫨

error: this expression creates a reference which is immediately dereferenced by the compiler
   --> src/vmm/src/mmds/mod.rs:185:31
    |
185 |     match mmds.is_valid_token(&token) {
    |                               ^^^^^^ help: change this to: `token`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
    = note: `-D clippy::needless-borrow` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::needless_borrow)]`

Fixed! Sorry about that :(

zulinx86 added 10 commits July 16, 2025 06:22
* Insert a line break with "\" after "\r\n" if another line follows
* Make multi-line strings start at the same column.
* Eliminate unneeded variable assignments (`request_bytes`).
* Use test utility function if possible
* Remove duplicate test cases

Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
We checked that "X-metadata-token-ttl-seconds" has a valid positive
number regardless of the request type. On the oher hand, EC2 IMDS only
validates it only if it is a PUT request to "/latest/api/token". Such
a behavioral difference from EC2 IMDS could result in unexpected issues.

Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
The custom headers supported by MMDS (X-metadata-token and
X-metadata-token-ttl-seconds) are different from what EC2 IMDS supports
(X-aws-ec2-metadata-token and X-aws-ec2-metadata-token-ttl-seconds).
Supporting EC2 IMDS-compatible custom headers in MMDS, users become able
to make their workloads work with MMDS out of the box.

Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
Add an integration test that checks AWS SDK for Python (boto3) is able
to get credentials via MMDS without modification.

CI artifacts (guest rootfs) update was needed to install AWS SDK for
Python.

Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
The constructor for TokenAuthority can fail and returns Result.

Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
To enable smoother migration from v1 to v2, make MMDS v1 support PUT
request to /latest/api/token for token generation.

We do NOT make MMDS v1 deny GET requests even with invalid tokens not
to break existing workloads. It does not validate a given toekn at this
point, but it will validate to increment a metric that will be added to
count such requests having invalid tokens.

Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
In the previous commit, MMDS v1 was made to support token generation but
a given token to GET request was not validated. Validates the token and
increments a new metric `rx_invalid_token` if it is not valid.

Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
The metric is useful for users to track whether v1-style requests are
issued or not inside a guest.

Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
* Move out version-independent parts to dedicated tests
* Test happy path first, and then test error cases

No test cases are dropped.

Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
As EC2 IMDS does, MMDS denies PUT requests if X-Forwarded-For header is
included, but it was validated only in a case-sensitive way. However, it
is defined that  HTTP headers are case-insensitive. We should deny it
regardless of its case.

Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
@zulinx86 zulinx86 requested a review from Manciukic July 16, 2025 09:28
@zulinx86 zulinx86 merged commit abc6947 into firecracker-microvm:main Jul 16, 2025
7 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.

3 participants