From a580544d829d9e18871f1090e52d3f71e22bb66f Mon Sep 17 00:00:00 2001 From: Rob Murray Date: Wed, 28 Feb 2024 20:06:29 +0000 Subject: [PATCH] Don't create endpoint config for MAC addr config migration 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 --- .../router/container/container_routes.go | 73 +++++--- .../router/container/container_routes_test.go | 160 ++++++++++++++++++ integration/networking/mac_addr_test.go | 49 ++++++ 3 files changed, 261 insertions(+), 21 deletions(-) create mode 100644 api/server/router/container/container_routes_test.go diff --git a/api/server/router/container/container_routes.go b/api/server/router/container/container_routes.go index 5c173aade4..5d7a0b8519 100644 --- 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 new file mode 100644 index 0000000000..2a96ad75d5 --- /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 92c24d4db4..8dfcac0c6c 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)) +}