From 8768d60425b2fab09ea950327887eb262e1815c2 Mon Sep 17 00:00:00 2001 From: Rob Murray Date: Tue, 2 Apr 2024 15:10:33 +0100 Subject: [PATCH] Factor out selection of endpoint for config migration Signed-off-by: Rob Murray --- .../router/container/container_routes.go | 109 +++++++++++------- .../router/container/container_routes_test.go | 76 +++++++++++- 2 files changed, 140 insertions(+), 45 deletions(-) diff --git a/api/server/router/container/container_routes.go b/api/server/router/container/container_routes.go index 3872028652..f5ecba6253 100644 --- a/api/server/router/container/container_routes.go +++ b/api/server/router/container/container_routes.go @@ -662,23 +662,11 @@ func handleMACAddressBC(config *container.Config, hostConfig *container.HostConf 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 + epConfig, err := epConfigForNetMode(version, hostConfig.NetworkMode, networkingConfig) + if err != nil { + return "", err } + epConfig.MacAddress = deprecatedMacAddress return "", nil } @@ -687,32 +675,17 @@ func handleMACAddressBC(config *container.Config, hostConfig *container.HostConf return "", nil } var warning string - if hostConfig.NetworkMode.IsBridge() || hostConfig.NetworkMode.IsUserDefined() { - nwName := hostConfig.NetworkMode.NetworkName() - // 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")) - } + if hostConfig.NetworkMode.IsDefault() || hostConfig.NetworkMode.IsBridge() || hostConfig.NetworkMode.IsUserDefined() { + ep, err := epConfigForNetMode(version, hostConfig.NetworkMode, networkingConfig) + if err != nil { + return "", errors.Wrap(err, "unable to migrate container-wide MAC address to a specific network") + } + // 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." @@ -721,6 +694,58 @@ func handleMACAddressBC(config *container.Config, hostConfig *container.HostConf return warning, nil } +// epConfigForNetMode finds, or creates, an entry in netConfig.EndpointsConfig +// corresponding to nwMode. +// +// nwMode.NetworkName() may be the network's name, its id, or its short-id. +// +// The corresponding endpoint in netConfig.EndpointsConfig may be keyed on a +// different one of name/id/short-id. If there's any ambiguity (there are +// endpoints but the names don't match), return an error and do not create a new +// endpoint, because it might be a duplicate. +func epConfigForNetMode( + version string, + nwMode container.NetworkMode, + netConfig *network.NetworkingConfig, +) (*network.EndpointSettings, error) { + nwName := nwMode.NetworkName() + + // It's always safe to create an EndpointsConfig entry under nwName if there are + // no entries already (because there can't be an entry for this network nwName + // refers to under any other name/short-id/id). + if len(netConfig.EndpointsConfig) == 0 { + es := &network.EndpointSettings{} + netConfig.EndpointsConfig = map[string]*network.EndpointSettings{ + nwName: es, + } + return es, nil + } + + // There cannot be more than one entry in EndpointsConfig with API < 1.44. + if versions.LessThan(version, "1.44") { + // No need to check for a match between NetworkMode and the names/ids in EndpointsConfig, + // the old version of the API would pick this network anyway. + for _, ep := range netConfig.EndpointsConfig { + return ep, nil + } + } + + // 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 := netConfig.EndpointsConfig[nwName] + if !ok { + return nil, errdefs.InvalidParameter( + errors.New("HostConfig.NetworkMode must match the identity of a network in NetworkSettings.Networks")) + } + + return ep, nil +} + func (s *containerRouter) deleteContainers(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error { if err := httputils.ParseForm(r); err != nil { return err diff --git a/api/server/router/container/container_routes_test.go b/api/server/router/container/container_routes_test.go index 2a96ad75d5..7c70e1c01f 100644 --- a/api/server/router/container/container_routes_test.go +++ b/api/server/router/container/container_routes_test.go @@ -102,7 +102,7 @@ func TestHandleMACAddressBC(t *testing.T) { 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", + expError: "unable to migrate container-wide MAC address to a specific network: HostConfig.NetworkMode must match the identity of a network in NetworkSettings.Networks", expCtrWideMAC: "11:22:33:44:55:66", }, { @@ -126,8 +126,8 @@ func TestHandleMACAddressBC(t *testing.T) { } epConfig := make(map[string]*network.EndpointSettings, len(tc.epConfig)) for k, v := range tc.epConfig { - v := v - epConfig[k] = v + v := *v + epConfig[k] = &v } netCfg := &network.NetworkingConfig{ EndpointsConfig: epConfig, @@ -158,3 +158,73 @@ func TestHandleMACAddressBC(t *testing.T) { }) } } + +func TestEpConfigForNetMode(t *testing.T) { + testcases := []struct { + name string + apiVersion string + networkMode string + epConfig map[string]*network.EndpointSettings + expEpId string + expNumEps int + expError bool + }{ + { + name: "old api no eps", + apiVersion: "1.43", + networkMode: "mynet", + expNumEps: 1, + }, + { + name: "new api no eps", + apiVersion: "1.44", + networkMode: "mynet", + expNumEps: 1, + }, + { + name: "old api with ep", + apiVersion: "1.43", + networkMode: "mynet", + epConfig: map[string]*network.EndpointSettings{ + "anything": {EndpointID: "epone"}, + }, + expEpId: "epone", + expNumEps: 1, + }, + { + name: "new api with matching ep", + apiVersion: "1.44", + networkMode: "mynet", + epConfig: map[string]*network.EndpointSettings{ + "mynet": {EndpointID: "epone"}, + }, + expEpId: "epone", + expNumEps: 1, + }, + { + name: "new api with mismatched ep", + apiVersion: "1.44", + networkMode: "mynet", + epConfig: map[string]*network.EndpointSettings{ + "shortid": {EndpointID: "epone"}, + }, + expError: true, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + netConfig := &network.NetworkingConfig{ + EndpointsConfig: tc.epConfig, + } + ep, err := epConfigForNetMode(tc.apiVersion, container.NetworkMode(tc.networkMode), netConfig) + if tc.expError { + assert.Check(t, is.ErrorContains(err, "HostConfig.NetworkMode must match the identity of a network in NetworkSettings.Networks")) + } else { + assert.Assert(t, err) + assert.Check(t, is.Equal(ep.EndpointID, tc.expEpId)) + assert.Check(t, is.Len(netConfig.EndpointsConfig, tc.expNumEps)) + } + }) + } +}