Repository navigation
fix(app): reject a guest_mac reserved for the metadata interface - #1286
Open
Saadanjum0 wants to merge 3 commits into
Open
Saadanjum0 wants to merge 3 commits into
Saadanjum0 wants to merge 3 commits into
Conversation
When the provider supports the metadata service and the spec has no eth0, CreateMicroVM prepends a metadata interface with the fixed MAC AA:FF:00:00:00:01. If another interface in the spec already uses that MAC, the VMM refuses to start because two NICs share an address, yet the spec is saved and the MicroVM is reported as created. Check for the collision when the metadata interface is added and return an error naming the interface instead. MACs are compared after parsing, so case and notation differences are caught too. Fixes liquidmetal-dev#1283 Signed-off-by: Saadanjum0 <160885369+Saadanjum0@users.noreply.github.com>
✅ Deploy Preview for flintlock-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
richardcase
reviewed
Oct 9, 2026
richardcase
left a comment
richardcase
left a comment
Member
There was a problem hiding this comment.
Thanks for this @Saadanjum0 . Just the 1 one comment.
| // The VMM refuses to start with two interfaces sharing a MAC address, so | ||
| // a spec that reuses the metadata interface's MAC can never run. | ||
| metadataMAC, _ := net.ParseMAC(metadataInterfaceMAC) | ||
| for _, netInt := range mvm.Spec.NetworkInterfaces { |
Member
There was a problem hiding this comment.
I wonder if instead of doing the check here we do this as part of the struct validation early in the command, so as part of this:
https://github.com/liquidmetal-dev/flintlock/blob/main/core/application/commands.go#L34-L38
It would require:
- Custom validator in here https://github.com/liquidmetal-dev/flintlock/blob/main/pkg/validation/validate.go
- Add the validator name to the field https://github.com/liquidmetal-dev/flintlock/blob/main/core/models/microvm.go#L39
Member
|
@Saadanjum0 - one more comment, it would be good to update the docs to state that the MAC address is reserved and shouldn't be used. Hopefully that would stop people using it in the first place. |
Move the check out of addMetadataInterface into a notReservedMAC validator on NetworkInterface.GuestMAC, so it runs with the rest of the spec validation. The metadata interface itself (eth0) may still carry the MAC. Document the reserved address in the proto, swagger and user docs.
Author
|
Thanks @richardcase, both done in a89e91f:
Tests: a validation case for a non-metadata interface using the MAC (fails validation), one confirming |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
When the provider has the metadata service capability and the spec has no
eth0,CreateMicroVMprepends a metadata interface with the fixed MACAA:FF:00:00:00:01. If another interface in the spec already uses that MAC, the spec is saved and reported as created, but Firecracker then exits withThe MAC address is already in use.This PR checks for that collision in
addMetadataInterfaceand returns an error that names the offending interface, soCreateMicroVMfails up front and nothing is saved. MACs are compared afternet.ParseMAC, so a different case (aa:ff:00:00:00:01) or notation (aa-ff-00-00-00-01) is caught too. A spec that defines its owneth0is unchanged, because no metadata interface is added in that case.The fixed MAC is now a constant shared by the interface definition and the check.
Note: like the existing validation errors from
CreateMicroVM, this error is returned through the gRPC handler as-is, so it does not map toINVALID_ARGUMENT. Changing how create errors map to gRPC codes felt like a separate change, so I left it out.Which issue(s) this PR fixes:
Fixes #1283
Special notes for your reviewer:
Added a
TestApp_CreateMicroVMcase where an interface other thaneth0usesaa:ff:00:00:00:01. Without the fix the case fails (the spec reachesRepo.Save, which the mock does not expect); with the fix it passes.go test -race ./core/... ./pkg/... ./infrastructure/grpc/...andgolangci-lint run ./core/application/...pass in a Linux container.The second option from the issue (deriving the metadata MAC per MicroVM) is not part of this PR.
Checklist: