Merge pull request #47304 from akerouanton/25.0-duplicate_mac_addrs2

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

api/types/network/endpoint.go

@@ -14,6 +14,9 @@ type EndpointSettings struct {
 	IPAMConfig *EndpointIPAMConfig
 	Links      []string
 	Aliases    []string // Aliases holds the list of extra, user-specified DNS names for this endpoint.
+	// MacAddress may be used to specify a MAC address when the container is created.
+	// Once the container is running, it becomes operational data (it may contain a
+	// generated address).
 	MacAddress string
 	// Operational data
 	NetworkID           string

daemon/container_operations.go

@@ -442,6 +442,11 @@ func (daemon *Daemon) updateContainerNetworkSettings(container *container.Contai
 		for name, epConfig := range endpointsConfig {
 			container.NetworkSettings.Networks[name] = &network.EndpointSettings{
 				EndpointSettings: epConfig,
+				// At this point, during container creation, epConfig.MacAddress is the
+				// configured value from the API. If there is no configured value, the
+				// same field will later be used to store a generated MAC address. So,
+				// remember the requested address now.
+				DesiredMacAddress: epConfig.MacAddress,
 			}
 		}
 	}
@@ -508,7 +513,7 @@ func (daemon *Daemon) allocateNetwork(cfg *config.Config, container *container.C
 	defaultNetName := runconfig.DefaultDaemonNetworkMode().NetworkName()
 	if nConf, ok := container.NetworkSettings.Networks[defaultNetName]; ok {
 		cleanOperationalData(nConf)
-		if err := daemon.connectToNetwork(cfg, container, defaultNetName, nConf.EndpointSettings, updateSettings); err != nil {
+		if err := daemon.connectToNetwork(cfg, container, defaultNetName, nConf, updateSettings); err != nil {
 			return err
 		}
 	}
@@ -525,7 +530,7 @@ func (daemon *Daemon) allocateNetwork(cfg *config.Config, container *container.C
 
 	for netName, epConf := range networks {
 		cleanOperationalData(epConf)
-		if err := daemon.connectToNetwork(cfg, container, netName, epConf.EndpointSettings, updateSettings); err != nil {
+		if err := daemon.connectToNetwork(cfg, container, netName, epConf, updateSettings); err != nil {
 			return err
 		}
 	}
@@ -634,12 +639,10 @@ func cleanOperationalData(es *network.EndpointSettings) {
 	es.IPv6Gateway = ""
 	es.GlobalIPv6Address = ""
 	es.GlobalIPv6PrefixLen = 0
+	es.MacAddress = ""
 	if es.IPAMOperational {
 		es.IPAMConfig = nil
 	}
-	if es.MACOperational {
-		es.MacAddress = ""
-	}
 }
 
 func (daemon *Daemon) updateNetworkConfig(container *container.Container, n *libnetwork.Network, endpointConfig *networktypes.EndpointSettings, updateSettings bool) error {
@@ -682,7 +685,7 @@ func buildEndpointDNSNames(ctr *container.Container, aliases []string) []string
 	return sliceutil.Dedup(dnsNames)
 }
 
-func (daemon *Daemon) connectToNetwork(cfg *config.Config, container *container.Container, idOrName string, endpointConfig *networktypes.EndpointSettings, updateSettings bool) (retErr error) {
+func (daemon *Daemon) connectToNetwork(cfg *config.Config, container *container.Container, idOrName string, endpointConfig *network.EndpointSettings, updateSettings bool) (retErr error) {
 	start := time.Now()
 	if container.HostConfig.NetworkMode.IsContainer() {
 		return runconfig.ErrConflictSharedNetwork
@@ -692,10 +695,12 @@ func (daemon *Daemon) connectToNetwork(cfg *config.Config, container *container.
 		return nil
 	}
 	if endpointConfig == nil {
-		endpointConfig = &networktypes.EndpointSettings{}
+		endpointConfig = &network.EndpointSettings{
+			EndpointSettings: &networktypes.EndpointSettings{},
+		}
 	}
 
-	n, nwCfg, err := daemon.findAndAttachNetwork(container, idOrName, endpointConfig)
+	n, nwCfg, err := daemon.findAndAttachNetwork(container, idOrName, endpointConfig.EndpointSettings)
 	if err != nil {
 		return err
 	}
@@ -710,26 +715,20 @@ func (daemon *Daemon) connectToNetwork(cfg *config.Config, container *container.
 		}
 	}
 
-	var operIPAM bool
-	operMAC := true
+	endpointConfig.IPAMOperational = false
 	if nwCfg != nil {
 		if epConfig, ok := nwCfg.EndpointsConfig[nwName]; ok {
 			if endpointConfig.IPAMConfig == nil || (endpointConfig.IPAMConfig.IPv4Address == "" && endpointConfig.IPAMConfig.IPv6Address == "" && len(endpointConfig.IPAMConfig.LinkLocalIPs) == 0) {
-				operIPAM = true
+				endpointConfig.IPAMOperational = true
 			}
 
 			// copy IPAMConfig and NetworkID from epConfig via AttachNetwork
 			endpointConfig.IPAMConfig = epConfig.IPAMConfig
 			endpointConfig.NetworkID = epConfig.NetworkID
-
-			// Work out whether the MAC address is user-configured.
-			operMAC = endpointConfig.MacAddress == ""
-			// Copy the configured MAC address (which may be empty).
-			endpointConfig.MacAddress = epConfig.MacAddress
 		}
 	}
 
-	if err := daemon.updateNetworkConfig(container, n, endpointConfig, updateSettings); err != nil {
+	if err := daemon.updateNetworkConfig(container, n, endpointConfig.EndpointSettings, updateSettings); err != nil {
 		return err
 	}
 
@@ -752,11 +751,7 @@ func (daemon *Daemon) connectToNetwork(cfg *config.Config, container *container.
 			}
 		}
 	}()
-	container.NetworkSettings.Networks[nwName] = &network.EndpointSettings{
-		EndpointSettings: endpointConfig,
-		IPAMOperational:  operIPAM,
-		MACOperational:   operMAC,
-	}
+	container.NetworkSettings.Networks[nwName] = endpointConfig
 
 	delete(container.NetworkSettings.Networks, n.ID())
 
@@ -1060,7 +1055,10 @@ func (daemon *Daemon) ConnectToNetwork(container *container.Container, idOrName
 			}
 		}
 	} else {
-		if err := daemon.connectToNetwork(&daemon.config().Config, container, idOrName, endpointConfig, true); err != nil {
+		epc := &network.EndpointSettings{
+			EndpointSettings: endpointConfig,
+		}
+		if err := daemon.connectToNetwork(&daemon.config().Config, container, idOrName, epc, true); err != nil {
 			return err
 		}
 	}

daemon/inspect.go

@@ -162,8 +162,12 @@ func (daemon *Daemon) getInspectData(daemonCfg *config.Config, container *contai
 	// unversioned API endpoints.
 	if container.Config != nil && container.Config.MacAddress == "" { //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44.
 		if nwm := hostConfig.NetworkMode; nwm.IsDefault() || nwm.IsBridge() || nwm.IsUserDefined() {
-			if epConf, ok := container.NetworkSettings.Networks[nwm.NetworkName()]; ok {
-				container.Config.MacAddress = epConf.MacAddress //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44.
+			name := nwm.NetworkName()
+			if nwm.IsDefault() {
+				name = daemon.netController.Config().DefaultNetwork
+			}
+			if epConf, ok := container.NetworkSettings.Networks[name]; ok {
+				container.Config.MacAddress = epConf.DesiredMacAddress //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44.
 			}
 		}
 	}

daemon/network.go

@@ -788,7 +788,7 @@ func (daemon *Daemon) clearAttachableNetworks() {
 }
 
 // buildCreateEndpointOptions builds endpoint options from a given network.
-func buildCreateEndpointOptions(c *container.Container, n *libnetwork.Network, epConfig *network.EndpointSettings, sb *libnetwork.Sandbox, daemonDNS []string) ([]libnetwork.EndpointOption, error) {
+func buildCreateEndpointOptions(c *container.Container, n *libnetwork.Network, epConfig *internalnetwork.EndpointSettings, sb *libnetwork.Sandbox, daemonDNS []string) ([]libnetwork.EndpointOption, error) {
 	var createOptions []libnetwork.EndpointOption
 	var genericOptions = make(options.Generic)
 
@@ -824,8 +824,8 @@ func buildCreateEndpointOptions(c *container.Container, n *libnetwork.Network, e
 			createOptions = append(createOptions, libnetwork.EndpointOptionGeneric(options.Generic{k: v}))
 		}
 
-		if epConfig.MacAddress != "" {
-			mac, err := net.ParseMAC(epConfig.MacAddress)
+		if epConfig.DesiredMacAddress != "" {
+			mac, err := net.ParseMAC(epConfig.DesiredMacAddress)
 			if err != nil {
 				return nil, err
 			}

daemon/network/settings.go

@@ -33,8 +33,9 @@ type Settings struct {
 type EndpointSettings struct {
 	*networktypes.EndpointSettings
 	IPAMOperational bool
-	// MACOperational is false if EndpointSettings.MacAddress is a user-configured value.
-	MACOperational bool
+	// DesiredMacAddress is the configured value, it's copied from MacAddress (the
+	// API param field) when the container is created.
+	DesiredMacAddress string
 }
 
 // AttachmentStore stores the load balancer IP address for a network id.

integration/networking/mac_addr_test.go

@@ -4,8 +4,11 @@ import (
 	"testing"
 
 	containertypes "github.com/docker/docker/api/types/container"
+	"github.com/docker/docker/client"
 	"github.com/docker/docker/integration/internal/container"
 	"github.com/docker/docker/integration/internal/network"
+	"github.com/docker/docker/libnetwork/drivers/bridge"
+	"github.com/docker/docker/testutil"
 	"github.com/docker/docker/testutil/daemon"
 	"gotest.tools/v3/assert"
 	is "gotest.tools/v3/assert/cmp"
@@ -35,7 +38,7 @@ func TestMACAddrOnRestart(t *testing.T) {
 	const netName = "testmacaddrs"
 	network.CreateNoError(ctx, t, c, netName,
 		network.WithDriver("bridge"),
-		network.WithOption("com.docker.network.bridge.name", netName))
+		network.WithOption(bridge.BridgeName, netName))
 	defer network.RemoveNoError(ctx, t, c, netName)
 
 	const ctr1Name = "ctr1"
@@ -77,3 +80,154 @@ func TestMACAddrOnRestart(t *testing.T) {
 	assert.Check(t, ctr1MAC != ctr2MAC,
 		"expected containers to have different MAC addresses; got %q for both", ctr1MAC)
 }
+
+// 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")
+
+	ctx := setupTest(t)
+
+	d := daemon.New(t)
+	d.StartWithBusybox(ctx, t)
+	defer d.Stop(t)
+
+	c := d.NewClientT(t)
+	defer c.Close()
+
+	const netName = "testcfgmacaddr"
+	network.CreateNoError(ctx, t, c, netName,
+		network.WithDriver("bridge"),
+		network.WithOption(bridge.BridgeName, netName))
+	defer network.RemoveNoError(ctx, t, c, netName)
+
+	const wantMAC = "02:42:ac:11:00:42"
+	const ctr1Name = "ctr1"
+	id1 := container.Run(ctx, t, c,
+		container.WithName(ctr1Name),
+		container.WithImage("busybox:latest"),
+		container.WithCmd("top"),
+		container.WithNetworkMode(netName),
+		container.WithMacAddress(netName, wantMAC))
+	defer c.ContainerRemove(ctx, id1, containertypes.RemoveOptions{
+		Force: true,
+	})
+
+	inspect := container.Inspect(ctx, t, c, ctr1Name)
+	gotMAC := inspect.NetworkSettings.Networks[netName].MacAddress
+	assert.Check(t, is.Equal(wantMAC, gotMAC))
+
+	startAndCheck := func() {
+		t.Helper()
+		err := c.ContainerStart(ctx, ctr1Name, containertypes.StartOptions{})
+		assert.Assert(t, is.Nil(err))
+		inspect = container.Inspect(ctx, t, c, ctr1Name)
+		gotMAC = inspect.NetworkSettings.Networks[netName].MacAddress
+		assert.Check(t, is.Equal(wantMAC, gotMAC))
+	}
+
+	// Restart the container, check that the MAC address is restored.
+	err := c.ContainerStop(ctx, ctr1Name, containertypes.StopOptions{})
+	assert.Assert(t, is.Nil(err))
+	startAndCheck()
+
+	// Restart the daemon, check that the MAC address is restored.
+	err = c.ContainerStop(ctx, ctr1Name, containertypes.StopOptions{})
+	assert.Assert(t, is.Nil(err))
+	d.Restart(t)
+	startAndCheck()
+}
+
+// Regression test for https://github.com/moby/moby/issues/47228 - check that a
+// generated MAC address is not included in the Config section of 'inspect'
+// output, but a configured address is.
+func TestInspectCfgdMAC(t *testing.T) {
+	skip.If(t, testEnv.DaemonInfo.OSType == "windows")
+
+	ctx := setupTest(t)
+
+	d := daemon.New(t)
+	d.StartWithBusybox(ctx, t)
+	defer d.Stop(t)
+
+	testcases := []struct {
+		name       string
+		desiredMAC string
+		netName    string
+		ctrWide    bool
+	}{
+		{
+			name:    "generated address default bridge",
+			netName: "bridge",
+		},
+		{
+			name:       "configured address default bridge",
+			desiredMAC: "02:42:ac:11:00:42",
+			netName:    "bridge",
+		},
+		{
+			name:    "generated address custom bridge",
+			netName: "testnet",
+		},
+		{
+			name:       "configured address custom bridge",
+			desiredMAC: "02:42:ac:11:00:42",
+			netName:    "testnet",
+		},
+		{
+			name:       "ctr-wide address default bridge",
+			desiredMAC: "02:42:ac:11:00:42",
+			netName:    "bridge",
+			ctrWide:    true,
+		},
+	}
+
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			ctx := testutil.StartSpan(ctx, t)
+
+			var copts []client.Opt
+			if tc.ctrWide {
+				copts = append(copts, client.WithVersion("1.43"))
+			}
+			c := d.NewClientT(t, copts...)
+			defer c.Close()
+
+			if tc.netName != "bridge" {
+				const netName = "inspectcfgmac"
+				network.CreateNoError(ctx, t, c, netName,
+					network.WithDriver("bridge"),
+					network.WithOption(bridge.BridgeName, netName))
+				defer network.RemoveNoError(ctx, t, c, netName)
+			}
+
+			const ctrName = "ctr"
+			opts := []func(*container.TestContainerConfig){
+				container.WithName(ctrName),
+				container.WithCmd("top"),
+				container.WithImage("busybox:latest"),
+			}
+			// Don't specify the network name for the bridge network, because that
+			// exercises a different code path (the network name isn't set until the
+			// container starts, until then it's "default").
+			if tc.netName != "bridge" {
+				opts = append(opts, container.WithNetworkMode(tc.netName))
+			}
+			if tc.desiredMAC != "" {
+				if tc.ctrWide {
+					opts = append(opts, container.WithContainerWideMacAddress(tc.desiredMAC))
+				} else {
+					opts = append(opts, container.WithMacAddress(tc.netName, tc.desiredMAC))
+				}
+			}
+			id := container.Create(ctx, t, c, opts...)
+			defer c.ContainerRemove(ctx, id, containertypes.RemoveOptions{
+				Force: true,
+			})
+
+			inspect := container.Inspect(ctx, t, c, ctrName)
+			configMAC := inspect.Config.MacAddress //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44.
+			assert.Check(t, is.DeepEqual(configMAC, tc.desiredMAC))
+		})
+	}
+}