Don't create endpoint config for MAC addr config migration

robmry · robmry · commit a580544d829d · 2024-02-29T17:02:19.000Z
In a container-create API request, HostConfig.NetworkMode (the identity
of the "main" network) may be a name, id or short-id.

The configuration for that network, including preferred IP address etc,
may be keyed on network name or id - it need not match the NetworkMode.

So, when migrating the old container-wide MAC address to the new
per-endpoint field - it is not safe to create a new EndpointSettings
entry unless there is no possibility that it will duplicate settings
intended for the same network (because one of the duplicates will be
discarded later, dropping the settings it contains).

This change introduces a new API restriction, if the deprecated container
wide field is used in the new API, and EndpointsConfig is provided for
any network, the NetworkMode and key under which the EndpointsConfig is
store must be the same - no mixing of ids and names.

Signed-off-by: Rob Murray &lt;rob.murray@docker.com&gt;
diff --git a/api/server/router/container/container_routes.go b/api/server/router/container/container_routes.go
@@ -623,42 +623,73 @@ func (s *containerRouter) postContainersCreate(ctx context.Context, w http.Respo
 // networkingConfig to set the endpoint-specific MACAddress field introduced in API v1.44. It returns a warning message
 // or an error if the container-wide field was specified for API >= v1.44.
 func handleMACAddressBC(config *container.Config, hostConfig *container.HostConfig, networkingConfig *network.NetworkingConfig, version string) (string, error) {
-	if config.MacAddress == "" { //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44.
-		return "", nil
-	}
-
 	deprecatedMacAddress := config.MacAddress //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44.
 
+	// For older versions of the API, migrate the container-wide MAC address to EndpointsConfig.
 	if versions.LessThan(version, "1.44") {
-		// The container-wide MacAddress parameter is deprecated and should now be specified in EndpointsConfig.
-		if hostConfig.NetworkMode.IsDefault() || hostConfig.NetworkMode.IsBridge() || hostConfig.NetworkMode.IsUserDefined() {
-			nwName := hostConfig.NetworkMode.NetworkName()
-			if _, ok := networkingConfig.EndpointsConfig[nwName]; !ok {
-				networkingConfig.EndpointsConfig[nwName] = &network.EndpointSettings{}
+		if deprecatedMacAddress == "" {
+			// If a MAC address is supplied in EndpointsConfig, discard it because the old API
+			// would have ignored it.
+			for _, ep := range networkingConfig.EndpointsConfig {
+				ep.MacAddress = ""
 			}
-			// Overwrite the config: either the endpoint's MacAddress was set by the user on API < v1.44, which
-			// must be ignored, or migrate the top-level MacAddress to the endpoint's config.
-			networkingConfig.EndpointsConfig[nwName].MacAddress = deprecatedMacAddress
+			return "", nil
 		}
 		if !hostConfig.NetworkMode.IsDefault() && !hostConfig.NetworkMode.IsBridge() && !hostConfig.NetworkMode.IsUserDefined() {
 			return "", runconfig.ErrConflictContainerNetworkAndMac
 		}
 
+		// There cannot be more than one entry in EndpointsConfig with API < 1.44.
+
+		// If there's no EndpointsConfig, create a place to store the configured address. It is
+		// safe to use NetworkMode as the network name, whether it's a name or id/short-id, as
+		// it will be normalised later and there is no other EndpointSettings object that might
+		// refer to this network/endpoint.
+		if len(networkingConfig.EndpointsConfig) == 0 {
+			nwName := hostConfig.NetworkMode.NetworkName()
+			networkingConfig.EndpointsConfig[nwName] = &network.EndpointSettings{}
+		}
+		// There's exactly one network in EndpointsConfig, either from the API or just-created.
+		// Migrate the container-wide setting to it.
+		// No need to check for a match between NetworkMode and the names/ids in EndpointsConfig,
+		// the old version of the API would have applied the address to this network anyway.
+		for _, ep := range networkingConfig.EndpointsConfig {
+			ep.MacAddress = deprecatedMacAddress
+		}
 		return "", nil
 	}
 
+	// The container-wide MacAddress parameter is deprecated and should now be specified in EndpointsConfig.
+	if deprecatedMacAddress == "" {
+		return "", nil
+	}
 	var warning string
 	if hostConfig.NetworkMode.IsDefault() || hostConfig.NetworkMode.IsBridge() || hostConfig.NetworkMode.IsUserDefined() {
 		nwName := hostConfig.NetworkMode.NetworkName()
-		if _, ok := networkingConfig.EndpointsConfig[nwName]; !ok {
-			networkingConfig.EndpointsConfig[nwName] = &network.EndpointSettings{}
-		}
-
-		ep := networkingConfig.EndpointsConfig[nwName]
-		if ep.MacAddress == "" {
-			ep.MacAddress = deprecatedMacAddress
-		} else if ep.MacAddress != deprecatedMacAddress {
-			return "", errdefs.InvalidParameter(errors.New("the container-wide MAC address should match the endpoint-specific MAC address for the main network or should be left empty"))
+		// If there's no endpoint config, create a place to store the configured address.
+		if len(networkingConfig.EndpointsConfig) == 0 {
+			networkingConfig.EndpointsConfig[nwName] = &network.EndpointSettings{
+				MacAddress: deprecatedMacAddress,
+			}
+		} else {
+			// There is existing endpoint config - if it's not indexed by NetworkMode.Name(), we
+			// can't tell which network the container-wide settings was intended for. NetworkMode,
+			// the keys in EndpointsConfig and the NetworkID in EndpointsConfig may mix network
+			// name/id/short-id. It's not safe to create EndpointsConfig under the NetworkMode
+			// name to store the container-wide MAC address, because that may result in two sets
+			// of EndpointsConfig for the same network and one set will be discarded later. So,
+			// reject the request ...
+			ep, ok := networkingConfig.EndpointsConfig[nwName]
+			if !ok {
+				return "", errdefs.InvalidParameter(errors.New("if a container-wide MAC address is supplied, HostConfig.NetworkMode must match the identity of a network in NetworkSettings.Networks"))
+			}
+			// ep is the endpoint that needs the container-wide MAC address; migrate the address
+			// to it, or bail out if there's a mismatch.
+			if ep.MacAddress == "" {
+				ep.MacAddress = deprecatedMacAddress
+			} else if ep.MacAddress != deprecatedMacAddress {
+				return "", errdefs.InvalidParameter(errors.New("the container-wide MAC address must match the endpoint-specific MAC address for the main network, or be left empty"))
+			}
 		}
 	}
 	warning = "The container-wide MacAddress field is now deprecated. It should be specified in EndpointsConfig instead."
diff --git a/api/server/router/container/container_routes_test.go b/api/server/router/container/container_routes_test.go
@@ -0,0 +1,160 @@
+package container
+
+import (
+	"testing"
+
+	"github.com/docker/docker/api/types/container"
+	"github.com/docker/docker/api/types/network"
+	"gotest.tools/v3/assert"
+	is "gotest.tools/v3/assert/cmp"
+)
+
+func TestHandleMACAddressBC(t *testing.T) {
+	testcases := []struct {
+		name                string
+		apiVersion          string
+		ctrWideMAC          string
+		networkMode         container.NetworkMode
+		epConfig            map[string]*network.EndpointSettings
+		expEpWithCtrWideMAC string
+		expEpWithNoMAC      string
+		expCtrWideMAC       string
+		expWarning          string
+		expError            string
+	}{
+		{
+			name:                "old api ctr-wide mac mix id and name",
+			apiVersion:          "1.43",
+			ctrWideMAC:          "11:22:33:44:55:66",
+			networkMode:         "aNetId",
+			epConfig:            map[string]*network.EndpointSettings{"aNetName": {}},
+			expEpWithCtrWideMAC: "aNetName",
+			expCtrWideMAC:       "11:22:33:44:55:66",
+		},
+		{
+			name:           "old api clear ep mac",
+			apiVersion:     "1.43",
+			networkMode:    "aNetId",
+			epConfig:       map[string]*network.EndpointSettings{"aNetName": {MacAddress: "11:22:33:44:55:66"}},
+			expEpWithNoMAC: "aNetName",
+		},
+		{
+			name:          "old api no-network ctr-wide mac",
+			apiVersion:    "1.43",
+			networkMode:   "none",
+			ctrWideMAC:    "11:22:33:44:55:66",
+			expError:      "conflicting options: mac-address and the network mode",
+			expCtrWideMAC: "11:22:33:44:55:66",
+		},
+		{
+			name:                "old api create ep",
+			apiVersion:          "1.43",
+			networkMode:         "aNetId",
+			ctrWideMAC:          "11:22:33:44:55:66",
+			epConfig:            map[string]*network.EndpointSettings{},
+			expEpWithCtrWideMAC: "aNetId",
+			expCtrWideMAC:       "11:22:33:44:55:66",
+		},
+		{
+			name:                "old api migrate ctr-wide mac",
+			apiVersion:          "1.43",
+			ctrWideMAC:          "11:22:33:44:55:66",
+			networkMode:         "aNetName",
+			epConfig:            map[string]*network.EndpointSettings{"aNetName": {}},
+			expEpWithCtrWideMAC: "aNetName",
+			expCtrWideMAC:       "11:22:33:44:55:66",
+		},
+		{
+			name:        "new api no macs",
+			apiVersion:  "1.44",
+			networkMode: "aNetId",
+			epConfig:    map[string]*network.EndpointSettings{"aNetName": {}},
+		},
+		{
+			name:        "new api ep specific mac",
+			apiVersion:  "1.44",
+			networkMode: "aNetName",
+			epConfig:    map[string]*network.EndpointSettings{"aNetName": {MacAddress: "11:22:33:44:55:66"}},
+		},
+		{
+			name:                "new api migrate ctr-wide mac to new ep",
+			apiVersion:          "1.44",
+			ctrWideMAC:          "11:22:33:44:55:66",
+			networkMode:         "aNetName",
+			epConfig:            map[string]*network.EndpointSettings{},
+			expEpWithCtrWideMAC: "aNetName",
+			expWarning:          "The container-wide MacAddress field is now deprecated",
+			expCtrWideMAC:       "",
+		},
+		{
+			name:                "new api migrate ctr-wide mac to existing ep",
+			apiVersion:          "1.44",
+			ctrWideMAC:          "11:22:33:44:55:66",
+			networkMode:         "aNetName",
+			epConfig:            map[string]*network.EndpointSettings{"aNetName": {}},
+			expEpWithCtrWideMAC: "aNetName",
+			expWarning:          "The container-wide MacAddress field is now deprecated",
+			expCtrWideMAC:       "",
+		},
+		{
+			name:          "new api mode vs name mismatch",
+			apiVersion:    "1.44",
+			ctrWideMAC:    "11:22:33:44:55:66",
+			networkMode:   "aNetId",
+			epConfig:      map[string]*network.EndpointSettings{"aNetName": {}},
+			expError:      "if a container-wide MAC address is supplied, HostConfig.NetworkMode must match the identity of a network in NetworkSettings.Networks",
+			expCtrWideMAC: "11:22:33:44:55:66",
+		},
+		{
+			name:          "new api mac mismatch",
+			apiVersion:    "1.44",
+			ctrWideMAC:    "11:22:33:44:55:66",
+			networkMode:   "aNetName",
+			epConfig:      map[string]*network.EndpointSettings{"aNetName": {MacAddress: "00:11:22:33:44:55"}},
+			expError:      "the container-wide MAC address must match the endpoint-specific MAC address",
+			expCtrWideMAC: "11:22:33:44:55:66",
+		},
+	}
+
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			cfg := &container.Config{
+				MacAddress: tc.ctrWideMAC, //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44.
+			}
+			hostCfg := &container.HostConfig{
+				NetworkMode: tc.networkMode,
+			}
+			epConfig := make(map[string]*network.EndpointSettings, len(tc.epConfig))
+			for k, v := range tc.epConfig {
+				v := v
+				epConfig[k] = v
+			}
+			netCfg := &network.NetworkingConfig{
+				EndpointsConfig: epConfig,
+			}
+
+			warning, err := handleMACAddressBC(cfg, hostCfg, netCfg, tc.apiVersion)
+
+			if tc.expError == "" {
+				assert.Check(t, err)
+			} else {
+				assert.Check(t, is.ErrorContains(err, tc.expError))
+			}
+			if tc.expWarning == "" {
+				assert.Check(t, is.Equal(warning, ""))
+			} else {
+				assert.Check(t, is.Contains(warning, tc.expWarning))
+			}
+			if tc.expEpWithCtrWideMAC != "" {
+				got := netCfg.EndpointsConfig[tc.expEpWithCtrWideMAC].MacAddress
+				assert.Check(t, is.Equal(got, tc.ctrWideMAC))
+			}
+			if tc.expEpWithNoMAC != "" {
+				got := netCfg.EndpointsConfig[tc.expEpWithNoMAC].MacAddress
+				assert.Check(t, is.Equal(got, ""))
+			}
+			gotCtrWideMAC := cfg.MacAddress //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44.
+			assert.Check(t, is.Equal(gotCtrWideMAC, tc.expCtrWideMAC))
+		})
+	}
+}
diff --git a/integration/networking/mac_addr_test.go b/integration/networking/mac_addr_test.go
@@ -231,3 +231,52 @@ func TestInspectCfgdMAC(t *testing.T) {
 		})
 	}
 }
+
+// Regression test for https://github.com/moby/moby/issues/47441
+// Migration of a container-wide MAC address to the new per-endpoint setting,
+// where NetworkMode uses network id, and the key in endpoint settings is the
+// network name.
+func TestWatchtowerCreate(t *testing.T) {
+	skip.If(t, testEnv.DaemonInfo.OSType == "windows", "no macvlan")
+
+	ctx := setupTest(t)
+
+	d := daemon.New(t)
+	d.StartWithBusybox(ctx, t)
+	defer d.Stop(t)
+
+	c := d.NewClientT(t, client.WithVersion("1.25"))
+	defer c.Close()
+
+	// Create a "/29" network, with a single address in iprange for IPAM to
+	// allocate, but no gateway address. So, the gateway will get the single
+	// free address. It'll only be possible to start a container by explicitly
+	// assigning an address.
+	const netName = "wtmvl"
+	netId := network.CreateNoError(ctx, t, c, netName,
+		network.WithIPAMRange("172.30.0.0/29", "172.30.0.1/32", ""),
+		network.WithDriver("macvlan"),
+	)
+	defer network.RemoveNoError(ctx, t, c, netName)
+
+	// Start a container, using the network's id in NetworkMode but its name
+	// in EndpointsConfig. (The container-wide MAC address must be merged with
+	// the endpoint config containing the preferred IP address, but the names
+	// don't match.)
+	const ctrName = "ctr1"
+	const ctrIP = "172.30.0.2"
+	const ctrMAC = "02:42:ac:11:00:42"
+	id := container.Run(ctx, t, c,
+		container.WithName(ctrName),
+		container.WithNetworkMode(netId),
+		container.WithContainerWideMacAddress(ctrMAC),
+		container.WithIPv4(netName, ctrIP),
+	)
+	defer c.ContainerRemove(ctx, id, containertypes.RemoveOptions{Force: true})
+
+	// Check that the container got the expected addresses.
+	inspect := container.Inspect(ctx, t, c, ctrName)
+	netSettings := inspect.NetworkSettings.Networks[netName]
+	assert.Check(t, is.Equal(netSettings.IPAddress, ctrIP))
+	assert.Check(t, is.Equal(netSettings.MacAddress, ctrMAC))
+}
