Only restore a configured MAC addr on restart.

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>

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
+	endpointConfig.IPAMOperational = false
-	operMAC := true
 	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{
+	container.NetworkSettings.Networks[nwName] = endpointConfig
-		EndpointSettings: endpointConfig,
-		IPAMOperational:  operIPAM,
-		MACOperational:   operMAC,
-	}
 
 	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

@@ -70,6 +70,7 @@ func (daemon *Daemon) ContainerInspectCurrent(ctx context.Context, name string,
 		if epConf.EndpointSettings != nil {
 			// We must make a copy of this pointer object otherwise it can race with other operations
 			apiNetworks[nwName] = epConf.EndpointSettings.Copy()
+			apiNetworks[nwName].DesiredMacAddress = ""
 		}
 	}
 

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 != "" {
+		if epConfig.DesiredMacAddress != "" {
-			mac, err := net.ParseMAC(epConfig.MacAddress)
+			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.
+	// DesiredMacAddress is the configured value, it's copied from MacAddress (the
-	MACOperational bool
+	// 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

@@ -6,6 +6,7 @@ import (
 	containertypes "github.com/docker/docker/api/types/container"
 	"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/daemon"
 	"gotest.tools/v3/assert"
 	is "gotest.tools/v3/assert/cmp"
@@ -35,7 +36,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 +78,60 @@ 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()
+}