Skip to content

Commit 7c7e453

Browse files
akerouantonakerouanton
authored
Merge pull request moby#47474 from robmry/47441_mac_addr_config_migration
Don't create endpoint config for MAC addr config migration
2 parents 4046928 + a580544 commit 7c7e453

File tree

3 files changed

+261
-21
lines changed

3 files changed

+261
-21
lines changed

‎api/server/router/container/container_routes.go‎

Lines changed: 52 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -634,42 +634,73 @@ func (s *containerRouter) postContainersCreate(ctx context.Context, w http.Respo
634634
// networkingConfig to set the endpoint-specific MACAddress field introduced in API v1.44. It returns a warning message
635635
// or an error if the container-wide field was specified for API >= v1.44.
636636
func handleMACAddressBC(config *container.Config, hostConfig *container.HostConfig, networkingConfig *network.NetworkingConfig, version string) (string, error) {
637-
if config.MacAddress == "" { //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44.
638-
return "", nil
639-
}
640-
641637
deprecatedMacAddress := config.MacAddress //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44.
642638

639+
// For older versions of the API, migrate the container-wide MAC address to EndpointsConfig.
643640
if versions.LessThan(version, "1.44") {
644-
// The container-wide MacAddress parameter is deprecated and should now be specified in EndpointsConfig.
645-
if hostConfig.NetworkMode.IsDefault() || hostConfig.NetworkMode.IsBridge() || hostConfig.NetworkMode.IsUserDefined() {
646-
nwName := hostConfig.NetworkMode.NetworkName()
647-
if _, ok := networkingConfig.EndpointsConfig[nwName]; !ok {
648-
networkingConfig.EndpointsConfig[nwName] = &network.EndpointSettings{}
641+
if deprecatedMacAddress == "" {
642+
// If a MAC address is supplied in EndpointsConfig, discard it because the old API
643+
// would have ignored it.
644+
for _, ep := range networkingConfig.EndpointsConfig {
645+
ep.MacAddress = ""
649646
}
650-
// Overwrite the config: either the endpoint's MacAddress was set by the user on API < v1.44, which
651-
// must be ignored, or migrate the top-level MacAddress to the endpoint's config.
652-
networkingConfig.EndpointsConfig[nwName].MacAddress = deprecatedMacAddress
647+
return "", nil
653648
}
654649
if !hostConfig.NetworkMode.IsDefault() && !hostConfig.NetworkMode.IsBridge() && !hostConfig.NetworkMode.IsUserDefined() {
655650
return "", runconfig.ErrConflictContainerNetworkAndMac
656651
}
657652

653+
// There cannot be more than one entry in EndpointsConfig with API < 1.44.
654+
655+
// If there's no EndpointsConfig, create a place to store the configured address. It is
656+
// safe to use NetworkMode as the network name, whether it's a name or id/short-id, as
657+
// it will be normalised later and there is no other EndpointSettings object that might
658+
// refer to this network/endpoint.
659+
if len(networkingConfig.EndpointsConfig) == 0 {
660+
nwName := hostConfig.NetworkMode.NetworkName()
661+
networkingConfig.EndpointsConfig[nwName] = &network.EndpointSettings{}
662+
}
663+
// There's exactly one network in EndpointsConfig, either from the API or just-created.
664+
// Migrate the container-wide setting to it.
665+
// No need to check for a match between NetworkMode and the names/ids in EndpointsConfig,
666+
// the old version of the API would have applied the address to this network anyway.
667+
for _, ep := range networkingConfig.EndpointsConfig {
668+
ep.MacAddress = deprecatedMacAddress
669+
}
658670
return "", nil
659671
}
660672

673+
// The container-wide MacAddress parameter is deprecated and should now be specified in EndpointsConfig.
674+
if deprecatedMacAddress == "" {
675+
return "", nil
676+
}
661677
var warning string
662678
if hostConfig.NetworkMode.IsDefault() || hostConfig.NetworkMode.IsBridge() || hostConfig.NetworkMode.IsUserDefined() {
663679
nwName := hostConfig.NetworkMode.NetworkName()
664-
if _, ok := networkingConfig.EndpointsConfig[nwName]; !ok {
665-
networkingConfig.EndpointsConfig[nwName] = &network.EndpointSettings{}
666-
}
667-
668-
ep := networkingConfig.EndpointsConfig[nwName]
669-
if ep.MacAddress == "" {
670-
ep.MacAddress = deprecatedMacAddress
671-
} else if ep.MacAddress != deprecatedMacAddress {
672-
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"))
680+
// If there's no endpoint config, create a place to store the configured address.
681+
if len(networkingConfig.EndpointsConfig) == 0 {
682+
networkingConfig.EndpointsConfig[nwName] = &network.EndpointSettings{
683+
MacAddress: deprecatedMacAddress,
684+
}
685+
} else {
686+
// There is existing endpoint config - if it's not indexed by NetworkMode.Name(), we
687+
// can't tell which network the container-wide settings was intended for. NetworkMode,
688+
// the keys in EndpointsConfig and the NetworkID in EndpointsConfig may mix network
689+
// name/id/short-id. It's not safe to create EndpointsConfig under the NetworkMode
690+
// name to store the container-wide MAC address, because that may result in two sets
691+
// of EndpointsConfig for the same network and one set will be discarded later. So,
692+
// reject the request ...
693+
ep, ok := networkingConfig.EndpointsConfig[nwName]
694+
if !ok {
695+
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"))
696+
}
697+
// ep is the endpoint that needs the container-wide MAC address; migrate the address
698+
// to it, or bail out if there's a mismatch.
699+
if ep.MacAddress == "" {
700+
ep.MacAddress = deprecatedMacAddress
701+
} else if ep.MacAddress != deprecatedMacAddress {
702+
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"))
703+
}
673704
}
674705
}
675706
warning = "The container-wide MacAddress field is now deprecated. It should be specified in EndpointsConfig instead."

‎api/server/router/container/container_routes_test.go‎

Lines changed: 160 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,160 @@
1+
package container
2+
3+
import (
4+
"testing"
5+
6+
"github.com/docker/docker/api/types/container"
7+
"github.com/docker/docker/api/types/network"
8+
"gotest.tools/v3/assert"
9+
is "gotest.tools/v3/assert/cmp"
10+
)
11+
12+
func TestHandleMACAddressBC(t *testing.T) {
13+
testcases := []struct {
14+
name string
15+
apiVersion string
16+
ctrWideMAC string
17+
networkMode container.NetworkMode
18+
epConfig map[string]*network.EndpointSettings
19+
expEpWithCtrWideMAC string
20+
expEpWithNoMAC string
21+
expCtrWideMAC string
22+
expWarning string
23+
expError string
24+
}{
25+
{
26+
name: "old api ctr-wide mac mix id and name",
27+
apiVersion: "1.43",
28+
ctrWideMAC: "11:22:33:44:55:66",
29+
networkMode: "aNetId",
30+
epConfig: map[string]*network.EndpointSettings{"aNetName": {}},
31+
expEpWithCtrWideMAC: "aNetName",
32+
expCtrWideMAC: "11:22:33:44:55:66",
33+
},
34+
{
35+
name: "old api clear ep mac",
36+
apiVersion: "1.43",
37+
networkMode: "aNetId",
38+
epConfig: map[string]*network.EndpointSettings{"aNetName": {MacAddress: "11:22:33:44:55:66"}},
39+
expEpWithNoMAC: "aNetName",
40+
},
41+
{
42+
name: "old api no-network ctr-wide mac",
43+
apiVersion: "1.43",
44+
networkMode: "none",
45+
ctrWideMAC: "11:22:33:44:55:66",
46+
expError: "conflicting options: mac-address and the network mode",
47+
expCtrWideMAC: "11:22:33:44:55:66",
48+
},
49+
{
50+
name: "old api create ep",
51+
apiVersion: "1.43",
52+
networkMode: "aNetId",
53+
ctrWideMAC: "11:22:33:44:55:66",
54+
epConfig: map[string]*network.EndpointSettings{},
55+
expEpWithCtrWideMAC: "aNetId",
56+
expCtrWideMAC: "11:22:33:44:55:66",
57+
},
58+
{
59+
name: "old api migrate ctr-wide mac",
60+
apiVersion: "1.43",
61+
ctrWideMAC: "11:22:33:44:55:66",
62+
networkMode: "aNetName",
63+
epConfig: map[string]*network.EndpointSettings{"aNetName": {}},
64+
expEpWithCtrWideMAC: "aNetName",
65+
expCtrWideMAC: "11:22:33:44:55:66",
66+
},
67+
{
68+
name: "new api no macs",
69+
apiVersion: "1.44",
70+
networkMode: "aNetId",
71+
epConfig: map[string]*network.EndpointSettings{"aNetName": {}},
72+
},
73+
{
74+
name: "new api ep specific mac",
75+
apiVersion: "1.44",
76+
networkMode: "aNetName",
77+
epConfig: map[string]*network.EndpointSettings{"aNetName": {MacAddress: "11:22:33:44:55:66"}},
78+
},
79+
{
80+
name: "new api migrate ctr-wide mac to new ep",
81+
apiVersion: "1.44",
82+
ctrWideMAC: "11:22:33:44:55:66",
83+
networkMode: "aNetName",
84+
epConfig: map[string]*network.EndpointSettings{},
85+
expEpWithCtrWideMAC: "aNetName",
86+
expWarning: "The container-wide MacAddress field is now deprecated",
87+
expCtrWideMAC: "",
88+
},
89+
{
90+
name: "new api migrate ctr-wide mac to existing ep",
91+
apiVersion: "1.44",
92+
ctrWideMAC: "11:22:33:44:55:66",
93+
networkMode: "aNetName",
94+
epConfig: map[string]*network.EndpointSettings{"aNetName": {}},
95+
expEpWithCtrWideMAC: "aNetName",
96+
expWarning: "The container-wide MacAddress field is now deprecated",
97+
expCtrWideMAC: "",
98+
},
99+
{
100+
name: "new api mode vs name mismatch",
101+
apiVersion: "1.44",
102+
ctrWideMAC: "11:22:33:44:55:66",
103+
networkMode: "aNetId",
104+
epConfig: map[string]*network.EndpointSettings{"aNetName": {}},
105+
expError: "if a container-wide MAC address is supplied, HostConfig.NetworkMode must match the identity of a network in NetworkSettings.Networks",
106+
expCtrWideMAC: "11:22:33:44:55:66",
107+
},
108+
{
109+
name: "new api mac mismatch",
110+
apiVersion: "1.44",
111+
ctrWideMAC: "11:22:33:44:55:66",
112+
networkMode: "aNetName",
113+
epConfig: map[string]*network.EndpointSettings{"aNetName": {MacAddress: "00:11:22:33:44:55"}},
114+
expError: "the container-wide MAC address must match the endpoint-specific MAC address",
115+
expCtrWideMAC: "11:22:33:44:55:66",
116+
},
117+
}
118+
119+
for _, tc := range testcases {
120+
t.Run(tc.name, func(t *testing.T) {
121+
cfg := &container.Config{
122+
MacAddress: tc.ctrWideMAC, //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44.
123+
}
124+
hostCfg := &container.HostConfig{
125+
NetworkMode: tc.networkMode,
126+
}
127+
epConfig := make(map[string]*network.EndpointSettings, len(tc.epConfig))
128+
for k, v := range tc.epConfig {
129+
v := v
130+
epConfig[k] = v
131+
}
132+
netCfg := &network.NetworkingConfig{
133+
EndpointsConfig: epConfig,
134+
}
135+
136+
warning, err := handleMACAddressBC(cfg, hostCfg, netCfg, tc.apiVersion)
137+
138+
if tc.expError == "" {
139+
assert.Check(t, err)
140+
} else {
141+
assert.Check(t, is.ErrorContains(err, tc.expError))
142+
}
143+
if tc.expWarning == "" {
144+
assert.Check(t, is.Equal(warning, ""))
145+
} else {
146+
assert.Check(t, is.Contains(warning, tc.expWarning))
147+
}
148+
if tc.expEpWithCtrWideMAC != "" {
149+
got := netCfg.EndpointsConfig[tc.expEpWithCtrWideMAC].MacAddress
150+
assert.Check(t, is.Equal(got, tc.ctrWideMAC))
151+
}
152+
if tc.expEpWithNoMAC != "" {
153+
got := netCfg.EndpointsConfig[tc.expEpWithNoMAC].MacAddress
154+
assert.Check(t, is.Equal(got, ""))
155+
}
156+
gotCtrWideMAC := cfg.MacAddress //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44.
157+
assert.Check(t, is.Equal(gotCtrWideMAC, tc.expCtrWideMAC))
158+
})
159+
}
160+
}

‎integration/networking/mac_addr_test.go‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -231,3 +231,52 @@ func TestInspectCfgdMAC(t *testing.T) {
231231
})
232232
}
233233
}
234+
235+
// Regression test for https://github.com/moby/moby/issues/47441
236+
// Migration of a container-wide MAC address to the new per-endpoint setting,
237+
// where NetworkMode uses network id, and the key in endpoint settings is the
238+
// network name.
239+
func TestWatchtowerCreate(t *testing.T) {
240+
skip.If(t, testEnv.DaemonInfo.OSType == "windows", "no macvlan")
241+
242+
ctx := setupTest(t)
243+
244+
d := daemon.New(t)
245+
d.StartWithBusybox(ctx, t)
246+
defer d.Stop(t)
247+
248+
c := d.NewClientT(t, client.WithVersion("1.25"))
249+
defer c.Close()
250+
251+
// Create a "/29" network, with a single address in iprange for IPAM to
252+
// allocate, but no gateway address. So, the gateway will get the single
253+
// free address. It'll only be possible to start a container by explicitly
254+
// assigning an address.
255+
const netName = "wtmvl"
256+
netId := network.CreateNoError(ctx, t, c, netName,
257+
network.WithIPAMRange("172.30.0.0/29", "172.30.0.1/32", ""),
258+
network.WithDriver("macvlan"),
259+
)
260+
defer network.RemoveNoError(ctx, t, c, netName)
261+
262+
// Start a container, using the network's id in NetworkMode but its name
263+
// in EndpointsConfig. (The container-wide MAC address must be merged with
264+
// the endpoint config containing the preferred IP address, but the names
265+
// don't match.)
266+
const ctrName = "ctr1"
267+
const ctrIP = "172.30.0.2"
268+
const ctrMAC = "02:42:ac:11:00:42"
269+
id := container.Run(ctx, t, c,
270+
container.WithName(ctrName),
271+
container.WithNetworkMode(netId),
272+
container.WithContainerWideMacAddress(ctrMAC),
273+
container.WithIPv4(netName, ctrIP),
274+
)
275+
defer c.ContainerRemove(ctx, id, containertypes.RemoveOptions{Force: true})
276+
277+
// Check that the container got the expected addresses.
278+
inspect := container.Inspect(ctx, t, c, ctrName)
279+
netSettings := inspect.NetworkSettings.Networks[netName]
280+
assert.Check(t, is.Equal(netSettings.IPAddress, ctrIP))
281+
assert.Check(t, is.Equal(netSettings.MacAddress, ctrMAC))
282+
}

0 commit comments

Comments
 (0)