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

libnet/d/bridge: don't parse the MacAddress netlabel #47818

Conversation

akerouanton
Copy link
Member

- What I did

Libnet's method (*Network).createEndpoint() is already parsing this netlabel to set the field ep.iface.mac. Later on, this same method invoke the driver's method CreateEndpoint with an InterfaceInfo arg and an options arg (an opaque map of driver opts).

The InterfaceInfo interface contains a MacAddress() method that returns ep.iface.mac. And the opaque map may contain the key netlabel.MacAddress.

Prior to this change, the bridge driver was calling MacAddress(). If no value was returned, it'd fall back to the option set in the options map, or generate a MAC address based on the IP address.

However, the expected type of the options value is a net.HardwareAddr. This is what's set by the daemon when handing over the endpoint config to libnet controller. If the value is a string, as is the case if the MAC address is provided through EndpointsSettings.DriverOpts, it produces an error.

As such, the opaque option and the MacAddress() are necessarily the same -- either nothing or a net.HardwareAddr. No need to keep both.

Moreover, the struct endpointConfiguration was only used to store that netlabel value. Drop it too.

- How I did it

Careful static code analysis.

- How to verify it

CI and tests.

Or manually:

$ docker run --rm -it --network testnet --mac-address 02:42:ac:13:00:12 nicolaka/netshoot ip -brief link show
$ docker run --rm -it --network=name=testnet,driver-opt=com.docker.network.endpoint.macaddress=60:3e:5f:35:9a:e4 nicolaka/netshoot ip -brief link show

- A picture of a cute animal (not mandatory but encouraged)

@akerouanton akerouanton added area/networking kind/refactor PR's that refactor, or clean-up code labels May 10, 2024
@akerouanton akerouanton self-assigned this May 10, 2024
Libnet's method `(*Network).createEndpoint()` is already parsing this
netlabel to set the field `ep.iface.mac`. Later on, this same method
invoke the driver's method `CreateEndpoint` with an `InterfaceInfo` arg
and an `options` arg (an opaque map of driver otps).

The `InterfaceInfo` interface contains a `MacAddress()` method that
returns `ep.iface.mac`. And the opaque map may contain the key
`netlabel.MacAddress`.

Prior to this change, the bridge driver was calling `MacAddress()`. If
no value was returned, it'd fall back to the option set in the `options`
map, or generate a MAC address based on the IP address.

However, the expected type of the `options` value is a `net.HardwareAddr`.
This is what's set by the daemon when handing over the endpoint config
to libnet controller. If the value is a string, as is the case if the
MAC address is provided through `EndpointsSettings.DriverOpts`, it
produces an error.

As such, the opaque option and the `MacAddress()` are necessarily the
same -- either nothing or a `net.HardwareAddr`. No need to keep both.

Moreover, the struct `endpointConfiguration` was only used to store that
netlabel value. Drop it too.

Signed-off-by: Albin Kerouanton <albinker@gmail.com>
@akerouanton akerouanton force-pushed the libnet-d-bridge-dont-parse-MacAddress-netlabel branch from 858f131 to 6fbae5f Compare May 10, 2024 10:45
@thaJeztah
Copy link
Member

Where is com.docker.network.endpoint.macaddress now parsed/handled after this PR? Or is the interface that's created the mechanism to pass that on?

(It was confusing a bit that com.docker.network.endpoint. reads as passing options for the endpoint but CreateEndpoint no longer accepting options, other than "by proxy" through the interface)

@akerouanton
Copy link
Member Author

Where is com.docker.network.endpoint.macaddress now parsed/handled after this PR?

First, the MacAddress sent through the API is added the generic options by buildCreateEndpointOptions:

moby/daemon/network.go

Lines 843 to 849 in cd08d37

if epConfig.DesiredMacAddress != "" {
mac, err := net.ParseMAC(epConfig.DesiredMacAddress)
if err != nil {
return nil, err
}
genericOptions[netlabel.MacAddress] = mac
}

Then, (*Controller).CreateEndpoint() parses these generic options and sets ep.iface.mac:

moby/libnetwork/network.go

Lines 1170 to 1174 in cd08d37

if opt, ok := ep.generic[netlabel.MacAddress]; ok {
if mac, ok := opt.(net.HardwareAddr); ok {
ep.iface.mac = mac
}
}

Then, driver's CreateEndpoint is called with ep.Iface() (= ep.iface) passed as 3rd arg:

err = d.CreateEndpoint(n.id, ep.id, ep.Iface(), ep.generic)

Finally, the bridge driver initializes endpoint.macAddress from ifInfo.MacAdress() (which returns ep.iface.macAddress):

endpoint.macAddress = ifInfo.MacAddress()

Now, if you're wondering what happens when com.docker.network.endpoint.macaddress is passed as a 'driver opt' (ie. docker run --network=name=testnet,driver-opt=com.docker.network.endpoint.macaddress=...), it was producing the following error and is now silently ignored (ie. the same way as unknown labels):

$ docker run --rm -it --network=name=testnet,driver-opt=com.docker.network.endpoint.macaddress=60:3e:5f:35:9a:e4 nicolaka/netshoot ip -brief link show
docker: Error response from daemon: failed to create endpoint inspiring_galileo on network testnet: trying to create an endpoint with an invalid endpoint configuration.

@akerouanton akerouanton added this to the 27.0.0 milestone May 21, 2024
@akerouanton akerouanton merged commit 145a73a into moby:master May 21, 2024
126 checks passed
@akerouanton akerouanton deleted the libnet-d-bridge-dont-parse-MacAddress-netlabel branch May 21, 2024 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/networking kind/refactor PR's that refactor, or clean-up code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants