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.

Also, because the daemon's EndpointSettings.MACOperational field was not
visible when generating inspect output, it could not be used to avoid adding
the generated address to Config.MacAddress.

So, this change removes that code, and adds DesiredMacAddress to the API's
version of EndpointSettings. Its value is copied from MacAddress (the API
field) when a container is created, it is stored as part of the container's
config, and used to inform the network driver of the configured value. But,
it is removed from 'inspect' output.

Signed-off-by: Rob Murray <rob.murray@docker.com>

api/types/network/endpoint.go

@@ -14,7 +14,16 @@ type EndpointSettings struct {
 	IPAMConfig *EndpointIPAMConfig
 	Links      []string
 	Aliases    []string // Aliases holds the list of extra, user-specified DNS names for this endpoint.
+	// MacAddress may contain a configured value from the API. Its value is copied
+	// into DesiredMacAddress. when the container is created, after which MacAddress
+	// becomes operational data (it may contain a generated address).
 	MacAddress string
+	// DesiredMacAddress is the configured value, it's copied from MacAddress (the
+	// API param field) when the container is created. It's cleared in 'inspect'
+	// output, so that only MacAddress is returned to clients - but its value is
+	// JSON encoded for persistence over a daemon restart. DesiredMacAddress is
+	// used to set the value for netlabel.MacAddress passed to the driver.
+	DesiredMacAddress string `json:",omitempty"`
 	// Operational data
 	NetworkID           string
 	EndpointID          string

daemon/container_operations.go

@@ -634,12 +634,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 {
@@ -711,7 +709,6 @@ func (daemon *Daemon) connectToNetwork(cfg *config.Config, container *container.
 	}
 
 	var operIPAM bool
-	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) {
@@ -721,11 +718,6 @@ func (daemon *Daemon) connectToNetwork(cfg *config.Config, container *container.
 			// 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
 		}
 	}
 
@@ -755,7 +747,6 @@ func (daemon *Daemon) connectToNetwork(cfg *config.Config, container *container.
 	container.NetworkSettings.Networks[nwName] = &network.EndpointSettings{
 		EndpointSettings: endpointConfig,
 		IPAMOperational:  operIPAM,
-		MACOperational:   operMAC,
 	}
 
 	delete(container.NetworkSettings.Networks, n.ID())

daemon/create.go

@@ -105,6 +105,18 @@ func (daemon *Daemon) containerCreate(ctx context.Context, daemonCfg *configStor
 		return containertypes.CreateResponse{Warnings: warnings}, errdefs.InvalidParameter(err)
 	}
 
+	// If a MAC address has been configured, it's in the MacAddress field of the
+	// network's EndpointConfig. But, MacAddress is also used to store a generated
+	// MAC address, and we must not restore a generated MAC address when a container
+	// is restarted. So, copy the configured value to DesiredMacAddress here. It will
+	// be stored as part of the container's config, and used to pass the configured
+	// value to the driver, but it is removed from 'inspect' output.
+	if opts.params.NetworkingConfig != nil {
+		for _, ep := range opts.params.NetworkingConfig.EndpointsConfig {
+			ep.DesiredMacAddress = ep.MacAddress
+		}
+	}
+
 	if opts.params.HostConfig == nil {
 		opts.params.HostConfig = &containertypes.HostConfig{}
 	}

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

@@ -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,6 @@ type Settings struct {
 type EndpointSettings struct {
 	*networktypes.EndpointSettings
 	IPAMOperational bool
-	// MACOperational is false if EndpointSettings.MacAddress is a user-configured value.
-	MACOperational bool
 }
 
 // AttachmentStore stores the load balancer IP address for a network id.

integration/networking/mac_addr_test.go

@@ -77,3 +77,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("com.docker.network.bridge.name", 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()
+}