From 4ace85d57806201d492984bf29c7217859da8d0d Mon Sep 17 00:00:00 2001 From: Rob Murray Date: Mon, 29 Jan 2024 18:38:05 +0000 Subject: [PATCH] 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 --- api/types/network/endpoint.go | 6 +- daemon/container_operations.go | 7 +- daemon/inspect.go | 9 ++- integration/networking/mac_addr_test.go | 101 +++++++++++++++++++++++- 4 files changed, 111 insertions(+), 12 deletions(-) diff --git a/api/types/network/endpoint.go b/api/types/network/endpoint.go index 8bc686f1cd675..9edd1c38d9196 100644 --- a/api/types/network/endpoint.go +++ b/api/types/network/endpoint.go @@ -14,9 +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 contain a configured value from the API. Its value is copied - // into DesiredMacAddress when the container is created, after which MacAddress - // becomes operational data (it may contain a generated address). + // 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 90b5dbb7128cd..ccf361f8470ac 100644 --- a/daemon/container_operations.go +++ b/daemon/container_operations.go @@ -695,10 +695,9 @@ func (daemon *Daemon) connectToNetwork(cfg *config.Config, container *container. return nil } if endpointConfig == nil { - endpointConfig = &network.EndpointSettings{} - } - if endpointConfig.EndpointSettings == nil { - endpointConfig.EndpointSettings = &networktypes.EndpointSettings{} + endpointConfig = &network.EndpointSettings{ + EndpointSettings: &networktypes.EndpointSettings{}, + } } n, nwCfg, err := daemon.findAndAttachNetwork(container, idOrName, endpointConfig.EndpointSettings) 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 1c515aeb16b58..92c24d4db48c0 100644 --- a/integration/networking/mac_addr_test.go +++ b/integration/networking/mac_addr_test.go @@ -4,8 +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" @@ -35,7 +38,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" @@ -95,7 +98,7 @@ func TestCfgdMACAddrOnRestart(t *testing.T) { const netName = "testcfgmacaddr" 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 wantMAC = "02:42:ac:11:00:42" @@ -134,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)) + }) + } +}