-
Notifications
You must be signed in to change notification settings - Fork 2k
feat(mmds): Add flag for making MMDS operate like EC2 IMDS #5310
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5310 +/- ##
==========================================
+ Coverage 82.96% 83.03% +0.06%
==========================================
Files 250 250
Lines 26828 26828
==========================================
+ Hits 22259 22276 +17
+ Misses 4569 4552 -17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ce0ed24
to
c2c67c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Mostly lgtm, just a couple of nits.
nit: commit message cccfe86 s/oeprate/operate/
7833b2b
to
27ab7ab
Compare
The snapshot versioning follows the semantic versioning (i.e. snapshot restore is allowed only if the restore major version is same as the snapshot major version and the restore minor version is greater than or equal to the snapshot minor version). We already bumped the major version up multiple times since we added `mmds_version` state. That means snapshots taken when `mmds_version` didn't exist are not able to be restored in the current version. The code to restore such snapshots is no longer needed. Signed-off-by: Takahiro Itazuri <[email protected]>
Currently the only MMDS-related state saved in the snapshot is the version info. To enable persisting more MMDS-related state, generalize the struct for MMDS state. Bumped up the snapshot major version since it is a breaking change. Signed-off-by: Takahiro Itazuri <[email protected]>
The method sets not only the version but also the AAD. Signed-off-by: Takahiro Itazuri <[email protected]>
While EC2 IMDS only supports text/plain and ignores Accept header, Firecracker MMDS supports not only text/plain but also application/json. If users don't have the control of libraries that set "Accept: application/json" but expect MMDS to behave like EC2 IMDS, the above difference becomes a problem. Not to break existing workloads, add a new flag `imds_compat` (default to false) to PUT /mmds/config API. Signed-off-by: Takahiro Itazuri <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ship it 🚢
/|\
/__| )
/____| ))
/______| )))
/________| )))
_|____))
\======| o o /
~~~~~~~~~~~~~~~~~~~
`_validate_mmds_snapshot()` is used only once. Signed-off-by: Takahiro Itazuri <[email protected]>
@Manciukic Sorry, I needed to fix a style error (unused import)... |
Changes
Added an optional
imds_compat
field (default to false if not provided) to PUT requests to/mmds/config
to enforce MMDS to always respond plain text contents in the IMDS format regardless of theAccept
header in requests.Reason
While EC2 IMDS only supports
text/plain
content type and ignoresAccept
header, Firecracker MMDS supports not onlytext/plain
but alsoapplication/json
. If users don't have the control of libraries that set "Accept: application/json" but expect MMDS to behave like EC2 IMDS, the above difference becomes a problem.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
tools/devtool checkbuild --all
to verify that the PR passesbuild checks on all supported architectures.
tools/devtool checkstyle
to verify that the PR passes theautomated style checks.
how they are solving the problem in a clear and encompassing way.
in the PR.
CHANGELOG.md
.[ ] If a specific issue led to this PR, this PR closes the issue.Runbook for Firecracker API changes.
integration tests.
[ ] I have linked an issue to every newTODO
.rust-vmm
.