From dae33031e0ee81fac77d54116d2ef71146be9e12 Mon Sep 17 00:00:00 2001 From: Rob Murray Date: Fri, 26 Jan 2024 18:00:32 +0000 Subject: [PATCH] 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 --- 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 4b3c06a52b..9edd1c38d9 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 5df9640bb9..ccf361f847 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 915b9d810b..6d75fdd11b 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 c4b0c93c1d..fa58df1aed 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 ec44ba0e31..8236d16ac0 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 497b4f96dc..f79c20b118 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() +}