Merge pull request #47474 from robmry/47441_mac_addr_config_migration

Don't create endpoint config for MAC addr config migration

api/server/router/container/container_routes.go

@@ -634,42 +634,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."

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))
+		})
+	}
+}

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))
+}