From 5b13a381448be25a7150bf5843d4ca153be7aff2 Mon Sep 17 00:00:00 2001 From: Rob Murray Date: Fri, 26 Jan 2024 18:00:32 +0000 Subject: [PATCH 1/2] Only restore a configured MAC addr on restart. The API's EndpointConfig struct has a MacAddress field that's used for both the configured address, and the current address (which may be generated). A configured address must be restored when a container is restarted, but a generated address must not. The previous attempt to differentiate between the two, without adding a field to the API's EndpointConfig that would show up in 'inspect' output, was a field in the daemon's version of EndpointSettings, MACOperational. It did not work, MACOperational was set to true when a configured address was used. So, while it ensured addresses were regenerated, it failed to preserve a configured address. So, this change removes that code, and adds DesiredMacAddress to the wrapped version of EndpointSettings, where it is persisted but does not appear in 'inspect' results. Its value is copied from MacAddress (the API field) when a container is created. Signed-off-by: Rob Murray (cherry picked from commit dae33031e0ee81fac77d54116d2ef71146be9e12) Signed-off-by: Albin Kerouanton --- api/types/network/endpoint.go | 3 ++ daemon/container_operations.go | 44 +++++++++--------- daemon/inspect.go | 1 + daemon/network.go | 6 +-- daemon/network/settings.go | 5 ++- integration/networking/mac_addr_test.go | 60 ++++++++++++++++++++++++- 6 files changed, 90 insertions(+), 29 deletions(-) diff --git a/api/types/network/endpoint.go b/api/types/network/endpoint.go index 4b3c06a52b583..9edd1c38d9196 100644 --- a/api/types/network/endpoint.go +++ b/api/types/network/endpoint.go @@ -14,6 +14,9 @@ type EndpointSettings struct { IPAMConfig *EndpointIPAMConfig Links []string Aliases []string // Aliases holds the list of extra, user-specified DNS names for this endpoint. + // MacAddress may be used to specify a MAC address when the container is created. + // Once the container is running, it becomes operational data (it may contain a + // generated address). MacAddress string // Operational data NetworkID string diff --git a/daemon/container_operations.go b/daemon/container_operations.go index 5df9640bb9a22..ccf361f8470ac 100644 --- a/daemon/container_operations.go +++ b/daemon/container_operations.go @@ -442,6 +442,11 @@ func (daemon *Daemon) updateContainerNetworkSettings(container *container.Contai for name, epConfig := range endpointsConfig { container.NetworkSettings.Networks[name] = &network.EndpointSettings{ EndpointSettings: epConfig, + // At this point, during container creation, epConfig.MacAddress is the + // configured value from the API. If there is no configured value, the + // same field will later be used to store a generated MAC address. So, + // remember the requested address now. + DesiredMacAddress: epConfig.MacAddress, } } } @@ -508,7 +513,7 @@ func (daemon *Daemon) allocateNetwork(cfg *config.Config, container *container.C defaultNetName := runconfig.DefaultDaemonNetworkMode().NetworkName() if nConf, ok := container.NetworkSettings.Networks[defaultNetName]; ok { cleanOperationalData(nConf) - if err := daemon.connectToNetwork(cfg, container, defaultNetName, nConf.EndpointSettings, updateSettings); err != nil { + if err := daemon.connectToNetwork(cfg, container, defaultNetName, nConf, updateSettings); err != nil { return err } } @@ -525,7 +530,7 @@ func (daemon *Daemon) allocateNetwork(cfg *config.Config, container *container.C for netName, epConf := range networks { cleanOperationalData(epConf) - if err := daemon.connectToNetwork(cfg, container, netName, epConf.EndpointSettings, updateSettings); err != nil { + if err := daemon.connectToNetwork(cfg, container, netName, epConf, updateSettings); err != nil { return err } } @@ -634,12 +639,10 @@ func cleanOperationalData(es *network.EndpointSettings) { es.IPv6Gateway = "" es.GlobalIPv6Address = "" es.GlobalIPv6PrefixLen = 0 + es.MacAddress = "" if es.IPAMOperational { es.IPAMConfig = nil } - if es.MACOperational { - es.MacAddress = "" - } } func (daemon *Daemon) updateNetworkConfig(container *container.Container, n *libnetwork.Network, endpointConfig *networktypes.EndpointSettings, updateSettings bool) error { @@ -682,7 +685,7 @@ func buildEndpointDNSNames(ctr *container.Container, aliases []string) []string return sliceutil.Dedup(dnsNames) } -func (daemon *Daemon) connectToNetwork(cfg *config.Config, container *container.Container, idOrName string, endpointConfig *networktypes.EndpointSettings, updateSettings bool) (retErr error) { +func (daemon *Daemon) connectToNetwork(cfg *config.Config, container *container.Container, idOrName string, endpointConfig *network.EndpointSettings, updateSettings bool) (retErr error) { start := time.Now() if container.HostConfig.NetworkMode.IsContainer() { return runconfig.ErrConflictSharedNetwork @@ -692,10 +695,12 @@ func (daemon *Daemon) connectToNetwork(cfg *config.Config, container *container. return nil } if endpointConfig == nil { - endpointConfig = &networktypes.EndpointSettings{} + endpointConfig = &network.EndpointSettings{ + EndpointSettings: &networktypes.EndpointSettings{}, + } } - n, nwCfg, err := daemon.findAndAttachNetwork(container, idOrName, endpointConfig) + n, nwCfg, err := daemon.findAndAttachNetwork(container, idOrName, endpointConfig.EndpointSettings) if err != nil { return err } @@ -710,26 +715,20 @@ func (daemon *Daemon) connectToNetwork(cfg *config.Config, container *container. } } - var operIPAM bool - operMAC := true + endpointConfig.IPAMOperational = false if nwCfg != nil { if epConfig, ok := nwCfg.EndpointsConfig[nwName]; ok { if endpointConfig.IPAMConfig == nil || (endpointConfig.IPAMConfig.IPv4Address == "" && endpointConfig.IPAMConfig.IPv6Address == "" && len(endpointConfig.IPAMConfig.LinkLocalIPs) == 0) { - operIPAM = true + endpointConfig.IPAMOperational = true } // copy IPAMConfig and NetworkID from epConfig via AttachNetwork endpointConfig.IPAMConfig = epConfig.IPAMConfig endpointConfig.NetworkID = epConfig.NetworkID - - // Work out whether the MAC address is user-configured. - operMAC = endpointConfig.MacAddress == "" - // Copy the configured MAC address (which may be empty). - endpointConfig.MacAddress = epConfig.MacAddress } } - if err := daemon.updateNetworkConfig(container, n, endpointConfig, updateSettings); err != nil { + if err := daemon.updateNetworkConfig(container, n, endpointConfig.EndpointSettings, updateSettings); err != nil { return err } @@ -752,11 +751,7 @@ func (daemon *Daemon) connectToNetwork(cfg *config.Config, container *container. } } }() - container.NetworkSettings.Networks[nwName] = &network.EndpointSettings{ - EndpointSettings: endpointConfig, - IPAMOperational: operIPAM, - MACOperational: operMAC, - } + container.NetworkSettings.Networks[nwName] = endpointConfig delete(container.NetworkSettings.Networks, n.ID()) @@ -1060,7 +1055,10 @@ func (daemon *Daemon) ConnectToNetwork(container *container.Container, idOrName } } } else { - if err := daemon.connectToNetwork(&daemon.config().Config, container, idOrName, endpointConfig, true); err != nil { + epc := &network.EndpointSettings{ + EndpointSettings: endpointConfig, + } + if err := daemon.connectToNetwork(&daemon.config().Config, container, idOrName, epc, true); err != nil { return err } } diff --git a/daemon/inspect.go b/daemon/inspect.go index 915b9d810b183..6d75fdd11ba55 100644 --- a/daemon/inspect.go +++ b/daemon/inspect.go @@ -70,6 +70,7 @@ func (daemon *Daemon) ContainerInspectCurrent(ctx context.Context, name string, if epConf.EndpointSettings != nil { // We must make a copy of this pointer object otherwise it can race with other operations apiNetworks[nwName] = epConf.EndpointSettings.Copy() + apiNetworks[nwName].DesiredMacAddress = "" } } diff --git a/daemon/network.go b/daemon/network.go index c4b0c93c1dcda..fa58df1aed4fc 100644 --- a/daemon/network.go +++ b/daemon/network.go @@ -788,7 +788,7 @@ func (daemon *Daemon) clearAttachableNetworks() { } // buildCreateEndpointOptions builds endpoint options from a given network. -func buildCreateEndpointOptions(c *container.Container, n *libnetwork.Network, epConfig *network.EndpointSettings, sb *libnetwork.Sandbox, daemonDNS []string) ([]libnetwork.EndpointOption, error) { +func buildCreateEndpointOptions(c *container.Container, n *libnetwork.Network, epConfig *internalnetwork.EndpointSettings, sb *libnetwork.Sandbox, daemonDNS []string) ([]libnetwork.EndpointOption, error) { var createOptions []libnetwork.EndpointOption var genericOptions = make(options.Generic) @@ -824,8 +824,8 @@ func buildCreateEndpointOptions(c *container.Container, n *libnetwork.Network, e createOptions = append(createOptions, libnetwork.EndpointOptionGeneric(options.Generic{k: v})) } - if epConfig.MacAddress != "" { - mac, err := net.ParseMAC(epConfig.MacAddress) + if epConfig.DesiredMacAddress != "" { + mac, err := net.ParseMAC(epConfig.DesiredMacAddress) if err != nil { return nil, err } diff --git a/daemon/network/settings.go b/daemon/network/settings.go index ec44ba0e31370..8236d16ac0b82 100644 --- a/daemon/network/settings.go +++ b/daemon/network/settings.go @@ -33,8 +33,9 @@ type Settings struct { type EndpointSettings struct { *networktypes.EndpointSettings IPAMOperational bool - // MACOperational is false if EndpointSettings.MacAddress is a user-configured value. - MACOperational bool + // DesiredMacAddress is the configured value, it's copied from MacAddress (the + // API param field) when the container is created. + DesiredMacAddress string } // AttachmentStore stores the load balancer IP address for a network id. diff --git a/integration/networking/mac_addr_test.go b/integration/networking/mac_addr_test.go index 497b4f96dc7f3..f79c20b118c1e 100644 --- a/integration/networking/mac_addr_test.go +++ b/integration/networking/mac_addr_test.go @@ -6,6 +6,7 @@ import ( containertypes "github.com/docker/docker/api/types/container" "github.com/docker/docker/integration/internal/container" "github.com/docker/docker/integration/internal/network" + "github.com/docker/docker/libnetwork/drivers/bridge" "github.com/docker/docker/testutil/daemon" "gotest.tools/v3/assert" is "gotest.tools/v3/assert/cmp" @@ -35,7 +36,7 @@ func TestMACAddrOnRestart(t *testing.T) { const netName = "testmacaddrs" network.CreateNoError(ctx, t, c, netName, network.WithDriver("bridge"), - network.WithOption("com.docker.network.bridge.name", netName)) + network.WithOption(bridge.BridgeName, netName)) defer network.RemoveNoError(ctx, t, c, netName) const ctr1Name = "ctr1" @@ -77,3 +78,60 @@ func TestMACAddrOnRestart(t *testing.T) { assert.Check(t, ctr1MAC != ctr2MAC, "expected containers to have different MAC addresses; got %q for both", ctr1MAC) } + +// Check that a configured MAC address is restored after a container restart, +// and after a daemon restart. +func TestCfgdMACAddrOnRestart(t *testing.T) { + skip.If(t, testEnv.DaemonInfo.OSType == "windows") + + ctx := setupTest(t) + + d := daemon.New(t) + d.StartWithBusybox(ctx, t) + defer d.Stop(t) + + c := d.NewClientT(t) + defer c.Close() + + const netName = "testcfgmacaddr" + network.CreateNoError(ctx, t, c, netName, + network.WithDriver("bridge"), + network.WithOption(bridge.BridgeName, netName)) + defer network.RemoveNoError(ctx, t, c, netName) + + const wantMAC = "02:42:ac:11:00:42" + const ctr1Name = "ctr1" + id1 := container.Run(ctx, t, c, + container.WithName(ctr1Name), + container.WithImage("busybox:latest"), + container.WithCmd("top"), + container.WithNetworkMode(netName), + container.WithMacAddress(netName, wantMAC)) + defer c.ContainerRemove(ctx, id1, containertypes.RemoveOptions{ + Force: true, + }) + + inspect := container.Inspect(ctx, t, c, ctr1Name) + gotMAC := inspect.NetworkSettings.Networks[netName].MacAddress + assert.Check(t, is.Equal(wantMAC, gotMAC)) + + startAndCheck := func() { + t.Helper() + err := c.ContainerStart(ctx, ctr1Name, containertypes.StartOptions{}) + assert.Assert(t, is.Nil(err)) + inspect = container.Inspect(ctx, t, c, ctr1Name) + gotMAC = inspect.NetworkSettings.Networks[netName].MacAddress + assert.Check(t, is.Equal(wantMAC, gotMAC)) + } + + // Restart the container, check that the MAC address is restored. + err := c.ContainerStop(ctx, ctr1Name, containertypes.StopOptions{}) + assert.Assert(t, is.Nil(err)) + startAndCheck() + + // Restart the daemon, check that the MAC address is restored. + err = c.ContainerStop(ctx, ctr1Name, containertypes.StopOptions{}) + assert.Assert(t, is.Nil(err)) + d.Restart(t) + startAndCheck() +} From bbe6f09afc855df1b8d6a4bb3346433c96b07760 Mon Sep 17 00:00:00 2001 From: Rob Murray Date: Mon, 29 Jan 2024 18:38:05 +0000 Subject: [PATCH 2/2] No inspect 'Config.MacAddress' unless configured. Do not set 'Config.MacAddress' in inspect output unless the MAC address is configured. Also, make sure it is filled in for a configured address on the default network before the container is started (by translating the network name from 'default' to 'config' so that the address lookup works). Signed-off-by: Rob Murray (cherry picked from commit 8c64b85fb90c9e518aee6da3e8adfd0bfeabf255) Signed-off-by: Albin Kerouanton --- daemon/inspect.go | 9 ++- integration/networking/mac_addr_test.go | 96 +++++++++++++++++++++++++ 2 files changed, 102 insertions(+), 3 deletions(-) diff --git a/daemon/inspect.go b/daemon/inspect.go index 6d75fdd11ba55..9ec907a5ba902 100644 --- a/daemon/inspect.go +++ b/daemon/inspect.go @@ -70,7 +70,6 @@ func (daemon *Daemon) ContainerInspectCurrent(ctx context.Context, name string, if epConf.EndpointSettings != nil { // We must make a copy of this pointer object otherwise it can race with other operations apiNetworks[nwName] = epConf.EndpointSettings.Copy() - apiNetworks[nwName].DesiredMacAddress = "" } } @@ -163,8 +162,12 @@ func (daemon *Daemon) getInspectData(daemonCfg *config.Config, container *contai // unversioned API endpoints. if container.Config != nil && container.Config.MacAddress == "" { //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44. if nwm := hostConfig.NetworkMode; nwm.IsDefault() || nwm.IsBridge() || nwm.IsUserDefined() { - if epConf, ok := container.NetworkSettings.Networks[nwm.NetworkName()]; ok { - container.Config.MacAddress = epConf.MacAddress //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44. + name := nwm.NetworkName() + if nwm.IsDefault() { + name = daemon.netController.Config().DefaultNetwork + } + if epConf, ok := container.NetworkSettings.Networks[name]; ok { + container.Config.MacAddress = epConf.DesiredMacAddress //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44. } } } diff --git a/integration/networking/mac_addr_test.go b/integration/networking/mac_addr_test.go index f79c20b118c1e..92c24d4db48c0 100644 --- a/integration/networking/mac_addr_test.go +++ b/integration/networking/mac_addr_test.go @@ -4,9 +4,11 @@ import ( "testing" containertypes "github.com/docker/docker/api/types/container" + "github.com/docker/docker/client" "github.com/docker/docker/integration/internal/container" "github.com/docker/docker/integration/internal/network" "github.com/docker/docker/libnetwork/drivers/bridge" + "github.com/docker/docker/testutil" "github.com/docker/docker/testutil/daemon" "gotest.tools/v3/assert" is "gotest.tools/v3/assert/cmp" @@ -135,3 +137,97 @@ func TestCfgdMACAddrOnRestart(t *testing.T) { d.Restart(t) startAndCheck() } + +// Regression test for https://github.com/moby/moby/issues/47228 - check that a +// generated MAC address is not included in the Config section of 'inspect' +// output, but a configured address is. +func TestInspectCfgdMAC(t *testing.T) { + skip.If(t, testEnv.DaemonInfo.OSType == "windows") + + ctx := setupTest(t) + + d := daemon.New(t) + d.StartWithBusybox(ctx, t) + defer d.Stop(t) + + testcases := []struct { + name string + desiredMAC string + netName string + ctrWide bool + }{ + { + name: "generated address default bridge", + netName: "bridge", + }, + { + name: "configured address default bridge", + desiredMAC: "02:42:ac:11:00:42", + netName: "bridge", + }, + { + name: "generated address custom bridge", + netName: "testnet", + }, + { + name: "configured address custom bridge", + desiredMAC: "02:42:ac:11:00:42", + netName: "testnet", + }, + { + name: "ctr-wide address default bridge", + desiredMAC: "02:42:ac:11:00:42", + netName: "bridge", + ctrWide: true, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + ctx := testutil.StartSpan(ctx, t) + + var copts []client.Opt + if tc.ctrWide { + copts = append(copts, client.WithVersion("1.43")) + } + c := d.NewClientT(t, copts...) + defer c.Close() + + if tc.netName != "bridge" { + const netName = "inspectcfgmac" + network.CreateNoError(ctx, t, c, netName, + network.WithDriver("bridge"), + network.WithOption(bridge.BridgeName, netName)) + defer network.RemoveNoError(ctx, t, c, netName) + } + + const ctrName = "ctr" + opts := []func(*container.TestContainerConfig){ + container.WithName(ctrName), + container.WithCmd("top"), + container.WithImage("busybox:latest"), + } + // Don't specify the network name for the bridge network, because that + // exercises a different code path (the network name isn't set until the + // container starts, until then it's "default"). + if tc.netName != "bridge" { + opts = append(opts, container.WithNetworkMode(tc.netName)) + } + if tc.desiredMAC != "" { + if tc.ctrWide { + opts = append(opts, container.WithContainerWideMacAddress(tc.desiredMAC)) + } else { + opts = append(opts, container.WithMacAddress(tc.netName, tc.desiredMAC)) + } + } + id := container.Create(ctx, t, c, opts...) + defer c.ContainerRemove(ctx, id, containertypes.RemoveOptions{ + Force: true, + }) + + inspect := container.Inspect(ctx, t, c, ctrName) + configMAC := inspect.Config.MacAddress //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44. + assert.Check(t, is.DeepEqual(configMAC, tc.desiredMAC)) + }) + } +}