diff --git a/api/server/router/container/container_routes.go b/api/server/router/container/container_routes.go index 254250d659eac..154802fa0f180 100644 --- a/api/server/router/container/container_routes.go +++ b/api/server/router/container/container_routes.go @@ -673,42 +673,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 new file mode 100644 index 0000000000000..2a96ad75d5c6b --- /dev/null +++ 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 index 92c24d4db48c0..8dfcac0c6c7b8 100644 --- 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)) +}