Only restore a configured MAC addr on restart. #47233

robmry · 2024-01-26T18:33:34Z

- What I did

The API's EndpointConfig struct has a MacAddress field that's used for both the configured address, and the current address (which may be generated).

A configured address must be restored when a container is restarted, but a generated address must not.

The previous attempt to differentiate between the two, without adding a field to the API's EndpointConfig that would show up in 'inspect' output, was a field in the daemon's version of EndpointSettings, MACOperational. It did not work, MACOperational was set to 'true' when a configured address was used. So, it ensured addresses were regenerated, but failed to preserve a configured address.

Also, only fill in Config.MacAddress in 'inspect' output when the MAC address is configured.

Fixes #47228
Fixes #47146

- How I did it

So, this change removes code from #47168, and instead adds DesiredMacAddress to the wrapped version of EndpointSettings, which are persisted but do not form part of the 'inspect' output.

Its value is copied from MacAddress (the API field) when a container is created.

The second git commit fills in Config.MacAddress in inspect output using DesiredMacAddress, which is empty when the address is generated. It also ensures the field is filled in for a configured address on the default bridge network, before the container is started, by translating the network name default into bridge.

- How to verify it

New tests added.

On Windows - run a container like this ...

docker run -ti --mac-address 00-11-22-33-44-55 mcr.microsoft.com/windows/nanoserver:ltsc2022 cmd.exe

... and use ipconfig /all to check that the configured MAC address has been used. Also check inspect shows the correct address.

Stop the container, restart the daemon, and restart the container. Then, exec into the container, check that the configured MAC address is still in-use - and that the inspect output is still correct.

This should be automated - #47283

- Description for the changelog

Ensure that a generated MAC address is not restored when a container is restarted, but a configured MAC address is preserved.

Containers created using 25.0.0 may have duplicate MAC addresses, they must be re-created.

Containers created using 25.0.0 or 25.0.1 with user-defined MAC addresses will get generated MAC addresses when they are started using 25.0.2. They must also be re-created.

akerouanton · 2024-01-29T09:27:54Z

api/types/network/endpoint.go

+	// API param field) when the container is created, and cleared in the inspect
+	// output so that only MacAddress is returned to clients. DesiredMacAddress is
+	// used to set the value for netlabel.MacAddress passed to the driver.
+	DesiredMacAddress string `json:",omitempty"`


I'd even not expose it through the API, otherwise we'll have to support it until the API version v1.44 is dropped:

Suggested change

DesiredMacAddress string `json:",omitempty"`

DesiredMacAddress string `json:"-"`

That'd stop it being encoded for persistent storage... so its value wouldn't be preserved over a daemon restart.

So, the idea is to deal with that by including it in the JSON when it has a value - but always clear it in the copy used for 'inspect' responses.

Huh, right. I totally forgot we're using the same json tag for both HTTP and storage concerns.

Why couldn't this field be added to EndpointSettings?

// EndpointSettings is a package local wrapper for // networktypes.EndpointSettings which stores Endpoint state that // needs to be persisted to disk but not exposed in the api.

I'd forgotten / not-noticed that the comment says those extra fields are persisted - but I don't think they are?

I could be wrong but, I think, the wrapper is always reconstructed here when the container's started ...

moby/daemon/container_operations.go

Lines 758 to 762 in 794f712

container.NetworkSettings.Networks[nwName] = &network.EndpointSettings{

EndpointSettings: endpointConfig,

IPAMOperational: operIPAM,

MACOperational: operMAC,

}

Also, the local-wrapper fields aren't available to the daemon's 'inspect' code - so it couldn't differentiate between a generated address and a configured one when it needed to fill in the container-wide Config.MacAddress.

(In the first PR, I got the logic wrong for deciding whether the address was configured - it wasn't as similar to the operIPAM case as we thought, so operMAC wasn't set up properly. And because the field isn't persisted, I don't think it was possible to tell the difference between a generated MAC and a configured one during the container-run, it has to be spotted and remembered during the create.)

If that seems right, I'll fix the comment.

I'd forgotten / not-noticed that the comment says those extra fields are persisted - but I don't think they are?

They're persisted in the container config: (container.Container).NetworkSettings[].Networks[].

I could be wrong but, I think, the wrapper is always reconstructed here when the container's started ...

So? That could be changed.

Also, the local-wrapper fields aren't available to the daemon's 'inspect' code - so it couldn't differentiate between a generated address and a configured one when it needed to fill in the container-wide Config.MacAddress.

The local-wrapper fields are accessible from the *Container. I find it hard to believe that they are not available to the code which maps an ID to a *Container and then to an API response.

Ok, thank you - that helped me to untangle it a bit.

akerouanton · 2024-01-29T09:30:00Z

api/types/network/endpoint.go

@@ -24,6 +28,7 @@ type EndpointSettings struct {
 	IPv6Gateway         string
 	GlobalIPv6Address   string
 	GlobalIPv6PrefixLen int
+	MacAddress          string


No need to move that field, it's still what should be used by API clients to configure a container's MAC address.

My thinking was that after the value from the API has been copied into DesiredMacAddress, the MacAddress field does become operational data - it's reset in cleanOperationalData(), along with the rest.

But yes, if this struct might be used by someone trying to figure out what to put in the API request, I can move it back. (It's bound to be a bit confusing until we properly separate configured state from running state.)

daemon/inspect.go

daemon/container_operations.go

corhere

Everything not commented on LGTM

api/types/network/endpoint.go

daemon/container_operations.go

daemon/network/settings.go

corhere · 2024-01-31T15:57:04Z

integration/networking/mac_addr_test.go

+// Check that a configured MAC address is restored after a container restart,
+// and after a daemon restart.
+func TestCfgdMACAddrOnRestart(t *testing.T) {
+	skip.If(t, testEnv.DaemonInfo.OSType == "windows")


Are the changes applicable to Windows containers? How feasible would it be to add Windows integration tests?

The issue with a MAC address generated by the bridge n/w driver getting reused after a container restart, then becoming a duplicate, doesn't apply on native-Windows - there's no bridge driver. (And these tests are all bridge-network based.)

Do we have any networking integration tests that start containers on Windows? There's probably a gap we should try to fill.

But, I think it has to be out of scope for this bug fix?

The tests use the bridge driver, but are the daemon code paths being touched specific to bridge, or Linux? Because it looks suspiciously like the code is cross-platform.

moby/libnetwork/drivers/windows/windows.go

Lines 539 to 545 in cff4f20

if opt, ok := epOptions[netlabel.MacAddress]; ok {

if mac, ok := opt.(net.HardwareAddr); ok {

ec.MacAddress = mac

} else {

return nil, fmt.Errorf("Invalid endpoint configuration")

}

}

There's no way to repro the Linux duplicate MAC address problem, because Windows MAC address generation doesn't work like that (addresses seem to be random, not based on IPAM-assigned IP addresses).

But I have checked that, on Windows without this change, a configured MAC address was not preserved over a daemon restart. With the change, it is.

It should be possible to write an automated regression test for that, and probably not too difficult - but I'd rather deal with it separately ... it's going to take me some time to figure out how to run an integration test on local Windows, and/or a number of pushes to GitHub to get it right.

So, perhaps we can resolve this if I raise an issue saying the regression test is needed, and assign it to myself to have a go at?

So, perhaps we can resolve this if I raise an issue saying the regression test is needed, and assign it to myself to have a go at?

I'd be satisfied with editing the "how to verify" section of the PR description to mention that you manually tested the change on Windows. Filing an issue to add a Windows regression test would be a huge bonus.

Good stuff... that's done.

integration/networking/mac_addr_test.go

The API's EndpointConfig struct has a MacAddress field that's used for both the configured address, and the current address (which may be generated). A configured address must be restored when a container is restarted, but a generated address must not. The previous attempt to differentiate between the two, without adding a field to the API's EndpointConfig that would show up in 'inspect' output, was a field in the daemon's version of EndpointSettings, MACOperational. It did not work, MACOperational was set to true when a configured address was used. So, while it ensured addresses were regenerated, it failed to preserve a configured address. So, this change removes that code, and adds DesiredMacAddress to the wrapped version of EndpointSettings, where it is persisted but does not appear in 'inspect' results. Its value is copied from MacAddress (the API field) when a container is created. Signed-off-by: Rob Murray <rob.murray@docker.com>

Do not set 'Config.MacAddress' in inspect output unless the MAC address is configured. Also, make sure it is filled in for a configured address on the default network before the container is started (by translating the network name from 'default' to 'config' so that the address lookup works). Signed-off-by: Rob Murray <rob.murray@docker.com>

akerouanton

LGTM

robmry force-pushed the 47146-duplicate_mac_addrs2 branch from d20f9f3 to ac4388b Compare January 29, 2024 09:15

robmry marked this pull request as ready for review January 29, 2024 09:16

robmry requested a review from akerouanton January 29, 2024 09:16

akerouanton reviewed Jan 29, 2024

View reviewed changes

robmry force-pushed the 47146-duplicate_mac_addrs2 branch 2 times, most recently from 51ce31b to 9e387e5 Compare January 29, 2024 14:43

akerouanton requested a review from thaJeztah January 29, 2024 15:32

robmry requested review from corhere and removed request for thaJeztah January 29, 2024 18:50

robmry force-pushed the 47146-duplicate_mac_addrs2 branch from 54c8faa to aa20cae Compare January 29, 2024 21:28

robmry commented Jan 29, 2024

View reviewed changes

daemon/container_operations.go Outdated Show resolved Hide resolved

robmry force-pushed the 47146-duplicate_mac_addrs2 branch 2 times, most recently from c113c51 to 98a0d0d Compare January 30, 2024 16:24

corhere reviewed Jan 31, 2024

View reviewed changes

robmry force-pushed the 47146-duplicate_mac_addrs2 branch from 98a0d0d to 4ace85d Compare January 31, 2024 20:17

robmry mentioned this pull request Jan 31, 2024

Add a Windows regression test for --mac-address #47283

Open

corhere approved these changes Jan 31, 2024

View reviewed changes

robmry added 2 commits February 1, 2024 09:55

robmry force-pushed the 47146-duplicate_mac_addrs2 branch from 4ace85d to 8c64b85 Compare February 1, 2024 09:58

corhere added area/networking kind/bugfix PR's that fix bugs process/cherry-pick/25.0 labels Feb 1, 2024

corhere added this to the 26.0.0 milestone Feb 1, 2024

thaJeztah requested a review from akerouanton February 1, 2024 20:12

akerouanton approved these changes Feb 2, 2024

View reviewed changes

akerouanton assigned robmry Feb 2, 2024

akerouanton merged commit ca683c1 into moby:master Feb 2, 2024
126 checks passed

akerouanton mentioned this pull request Feb 2, 2024

[25.0 backport] Only restore a configured MAC addr on restart. #47304

Merged

akerouanton added the process/cherry-picked label Feb 2, 2024

akerouanton removed the process/cherry-pick/25.0 label Feb 2, 2024

robmry mentioned this pull request Mar 4, 2024

Don't create endpoint config for MAC addr config migration #47474

Merged

wojiushixiaobai mentioned this pull request Mar 22, 2024

feat: 更新 docker 26.0.0 jumpserver/installer#401

Merged

robmry deleted the 47146-duplicate_mac_addrs2 branch March 27, 2024 16:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only restore a configured MAC addr on restart. #47233

Only restore a configured MAC addr on restart. #47233

robmry commented Jan 26, 2024 •

edited

akerouanton Jan 29, 2024

robmry Jan 29, 2024

akerouanton Jan 29, 2024

corhere Jan 29, 2024

robmry Jan 29, 2024

corhere Jan 29, 2024

robmry Jan 29, 2024

akerouanton Jan 29, 2024

robmry Jan 29, 2024

corhere left a comment

corhere Jan 31, 2024

robmry Jan 31, 2024

corhere Jan 31, 2024

robmry Jan 31, 2024

corhere Jan 31, 2024 •

edited

robmry Jan 31, 2024

akerouanton left a comment

	DesiredMacAddress string `json:",omitempty"`
	DesiredMacAddress string `json:"-"`

	container.NetworkSettings.Networks[nwName] = &network.EndpointSettings{
	EndpointSettings: endpointConfig,
	IPAMOperational: operIPAM,
	MACOperational: operMAC,
	}

	if opt, ok := epOptions[netlabel.MacAddress]; ok {
	if mac, ok := opt.(net.HardwareAddr); ok {
	ec.MacAddress = mac
	} else {
	return nil, fmt.Errorf("Invalid endpoint configuration")
	}
	}

Only restore a configured MAC addr on restart. #47233

Only restore a configured MAC addr on restart. #47233

Conversation

robmry commented Jan 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corhere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corhere Jan 31, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akerouanton left a comment

Choose a reason for hiding this comment

robmry commented Jan 26, 2024 •

edited

corhere Jan 31, 2024 •

edited