-
Notifications
You must be signed in to change notification settings - Fork 2.1k
network: add --mac-address support to docker network connect
#6728
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
base: master
Are you sure you want to change the base?
network: add --mac-address support to docker network connect
#6728
Conversation
This adds support for specifying a MAC address when connecting a container to a network, particularly useful for macvlan networks. The flag follows the same pattern as existing network options like --ip and --ip6, parsing and validating the MAC address before sending it to the daemon via the EndpointSettings.MacAddress field. Note: This requires a companion change in moby/moby to have the daemon actually apply the MAC address during network conne Signed-off-by: ahmedharabi <[email protected]>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
thaJeztah
left a comment
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.
Thanks for contributing! While reviewing, and testing my code suggestions, I think the "network connect" endpoint currently doesn't support setting a MAC-address 🤔 at least, I didn't get the expected result, so I think changes may still be needed on the daemon side for this
docker rm -fv foo
docker network create net1
docker run -d --name foo nginx:alpine
docker network connect --mac-address=92:d0:c6:0a:29:33 net1 foo
docker container inspect foo | jq '.[0].NetworkSettings.Networks."net1".MacAddress'
"62:ad:7e:be:84:dc"
cli/command/network/connect.go
Outdated
| if err != nil { | ||
| return fmt.Errorf("invalid MAC address: %q", macStr) | ||
| } | ||
| options.macAddress = mac |
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.
Lacking a special mac-address flag-type (I'm looking into creating some generics for these), we can probably add macAddress as string for now, and handle the parsing in runConnect
cli/command/network/connect.go
Outdated
| // Validate MAC address if provided | ||
|
|
||
| if macStr != "" { | ||
| mac, err := net.ParseMAC(macStr) |
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.
Looking at the type; we could probably use the network.HardwareAddr's UnmarshalText method for this; that way we wouldn't have to cast it to the right type 🤔
|
I pushed a temporary commit with suggestions, but let me move this one to draft, as it looks like it requires changes on the daemon-side for this cc @robmry |
Signed-off-by: Sebastiaan van Stijn <[email protected]>
935dabf to
e605695
Compare
|
Thanks for checking and for the detailed review. I reached the same conclusion after multiple attempts: the MAC address does not appear to be applied by the daemon during network connect. I was unable to observe any change on the container side either. My intention with this change was to surface the flag at the CLI level first, so the behavior and expectations are explicit and testable, and so any required follow-up on the daemon side is clearer and easier to reason about. If this needs to be handled entirely in the daemon first, I’m happy to adjust or pause this PR and follow your guidance on the preferred approach. |
Yes, looks like we need to look what's needed on the daemon-side. I saw your original ticket, and thought "oh, this probably needs to be implemented", but then tried this PR and it didn't produce an error, so initially thought "perhaps not, and perhaps it was just not wired up in the CLI!" (hoping that if that was the case, we could still include this in the upcoming 29.2 release 😄) The fact that the daemon doesn't produce an error was slightly surprising, but some of this may be due to the same Go types / structs being reused for multiple places (both "create", "connect" and "inspect"). Those were probably poor choices made in the past, and we've made some attempts to be more specific (e.g. moby/moby#51204), but some of those are very deeply ingrained in the code-base, making it hard to change, so may be something we just need to live with now. But yeah, it looks like more work is needed to make this work for |
|
Yeah, agreed changing the shared struct types would likely require a lot more work than makes sense right now. Given that, returning a clear error to the user instead of silently ignoring the option feels like the best approach for now. I’m happy to update the PR in that direction, or we can leave this here and revisit once daemon support becomes feasible. |
mac-addressoption available asdocker run --networkoption, but missing as argument indocker network connectmoby/moby#51796Add
--mac-addresssupport todocker network connectCloses moby/moby#51796
This change adds support for specifying a MAC address when connecting an existing container to a network using
docker network connect, bringing feature parity withdocker run --network.The flag follows the same behavior as existing network-related options such as
--ipand--ip6. The MAC address is parsed and validated client-side, then forwarded to the daemon viaEndpointSettings.MacAddress.This is particularly useful for macvlan networks, where explicit MAC address assignment is a common requirement.
Note: This change requires a companion update in
moby/mobyfor the daemon to actually apply the MAC address during network connection.What I did
--mac-addressflag todocker network connectEndpointSettings.MacAddressto the daemonHow I did it
net.ParseMACnetwork.HardwareAddrbefore sending it to the APIHow to verify it