From ee9f0ed895a15131ee9e3e85670d25f8a5abe2e1 Mon Sep 17 00:00:00 2001 From: Albin Kerouanton Date: Tue, 4 Jul 2023 12:40:35 +0200 Subject: [PATCH] api: Deprecate ContainerConfig.MacAddress Having a sandbox/container-wide MacAddress field makes little sense since a container can be connected to multiple networks at the same time. This field is an artefact of old times where a container could be connected to a single network only. As we now have a way to specify per-endpoint mac address, this field is now deprecated. Signed-off-by: Albin Kerouanton Signed-off-by: Sebastiaan van Stijn --- .../router/container/container_routes.go | 57 ++++++++++++++++++- api/swagger.yaml | 5 +- api/types/container/config.go | 15 +++-- client/container_create.go | 7 ++- daemon/inspect.go | 17 +++++- daemon/inspect_linux.go | 2 +- daemon/network.go | 16 ------ docs/api/version-history.md | 1 + integration-cli/docker_cli_netmode_test.go | 2 - integration-cli/docker_cli_run_test.go | 5 -- integration/internal/container/ops.go | 2 +- runconfig/hostconfig.go | 4 -- 12 files changed, 93 insertions(+), 40 deletions(-) diff --git a/api/server/router/container/container_routes.go b/api/server/router/container/container_routes.go index 2251fb1400..0278859751 100644 --- a/api/server/router/container/container_routes.go +++ b/api/server/router/container/container_routes.go @@ -628,6 +628,13 @@ func (s *containerRouter) postContainersCreate(ctx context.Context, w http.Respo } } + var warnings []string + if warn, err := handleMACAddressBC(config, hostConfig, networkingConfig, version); err != nil { + return err + } else if warn != "" { + warnings = append(warnings, warn) + } + if hostConfig.PidsLimit != nil && *hostConfig.PidsLimit <= 0 { // Don't set a limit if either no limit was specified, or "unlimited" was // explicitly set. @@ -647,10 +654,58 @@ func (s *containerRouter) postContainersCreate(ctx context.Context, w http.Respo if err != nil { return err } - + ccr.Warnings = append(ccr.Warnings, warnings...) return httputils.WriteJSON(w, http.StatusCreated, ccr) } +// handleMACAddressBC takes care of backward-compatibility for the container-wide MAC address by mutating the +// networkingConfig to set the endpoint-specific MACAddress field introduced in API v1.44. It returns a warning message +// or an error if the container-wide field was specified for API >= v1.44. +func handleMACAddressBC(config *container.Config, hostConfig *container.HostConfig, networkingConfig *network.NetworkingConfig, version string) (string, error) { + if config.MacAddress == "" { //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44. + return "", nil + } + + deprecatedMacAddress := config.MacAddress //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44. + + if versions.LessThan(version, "1.44") { + // The container-wide MacAddress parameter is deprecated and should now be specified in EndpointsConfig. + if hostConfig.NetworkMode.IsDefault() || hostConfig.NetworkMode.IsBridge() || hostConfig.NetworkMode.IsUserDefined() { + nwName := hostConfig.NetworkMode.NetworkName() + if _, ok := networkingConfig.EndpointsConfig[nwName]; !ok { + networkingConfig.EndpointsConfig[nwName] = &network.EndpointSettings{} + } + // Overwrite the config: either the endpoint's MacAddress was set by the user on API < v1.44, which + // must be ignored, or migrate the top-level MacAddress to the endpoint's config. + networkingConfig.EndpointsConfig[nwName].MacAddress = deprecatedMacAddress + } + if !hostConfig.NetworkMode.IsDefault() && !hostConfig.NetworkMode.IsBridge() && !hostConfig.NetworkMode.IsUserDefined() { + return "", runconfig.ErrConflictContainerNetworkAndMac + } + + return "", nil + } + + var warning string + if hostConfig.NetworkMode.IsDefault() || hostConfig.NetworkMode.IsBridge() || hostConfig.NetworkMode.IsUserDefined() { + nwName := hostConfig.NetworkMode.NetworkName() + if _, ok := networkingConfig.EndpointsConfig[nwName]; !ok { + networkingConfig.EndpointsConfig[nwName] = &network.EndpointSettings{} + } + + ep := networkingConfig.EndpointsConfig[nwName] + if ep.MacAddress == "" { + ep.MacAddress = deprecatedMacAddress + } else if ep.MacAddress != deprecatedMacAddress { + 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")) + } + } + warning = "The container-wide MacAddress field is now deprecated. It should be specified in EndpointsConfig instead." + config.MacAddress = "" //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44. + + return warning, 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/swagger.yaml b/api/swagger.yaml index c5725d73c4..3ee7e50b81 100644 --- a/api/swagger.yaml +++ b/api/swagger.yaml @@ -1313,7 +1313,10 @@ definitions: type: "boolean" x-nullable: true MacAddress: - description: "MAC address of the container." + description: | + MAC address of the container. + + Deprecated: this field is deprecated in API v1.44 and up. Use EndpointSettings.MacAddress instead. type: "string" x-nullable: true OnBuild: diff --git a/api/types/container/config.go b/api/types/container/config.go index d8cfe443f8..be41d6315e 100644 --- a/api/types/container/config.go +++ b/api/types/container/config.go @@ -70,10 +70,13 @@ type Config struct { WorkingDir string // Current directory (PWD) in the command will be launched Entrypoint strslice.StrSlice // Entrypoint to run when starting the container NetworkDisabled bool `json:",omitempty"` // Is network disabled - MacAddress string `json:",omitempty"` // Mac Address of the container - OnBuild []string // ONBUILD metadata that were defined on the image Dockerfile - Labels map[string]string // List of labels set to this container - StopSignal string `json:",omitempty"` // Signal to stop a container - StopTimeout *int `json:",omitempty"` // Timeout (in seconds) to stop a container - Shell strslice.StrSlice `json:",omitempty"` // Shell for shell-form of RUN, CMD, ENTRYPOINT + // Mac Address of the container. + // + // Deprecated: this field is deprecated since API v1.44. Use EndpointSettings.MacAddress instead. + MacAddress string `json:",omitempty"` + OnBuild []string // ONBUILD metadata that were defined on the image Dockerfile + Labels map[string]string // List of labels set to this container + StopSignal string `json:",omitempty"` // Signal to stop a container + StopTimeout *int `json:",omitempty"` // Timeout (in seconds) to stop a container + Shell strslice.StrSlice `json:",omitempty"` // Shell for shell-form of RUN, CMD, ENTRYPOINT } diff --git a/client/container_create.go b/client/container_create.go index ce9a7a404d..409f5b492a 100644 --- a/client/container_create.go +++ b/client/container_create.go @@ -39,7 +39,7 @@ func (cli *Client) ContainerCreate(ctx context.Context, config *container.Config if err := cli.NewVersionError(ctx, "1.44", "specify health-check start interval"); config != nil && config.Healthcheck != nil && config.Healthcheck.StartInterval != 0 && err != nil { return response, err } - if err := cli.NewVersionError("1.44", "specify mac-address per network"); hasEndpointSpecificMacAddress(networkingConfig) && err != nil { + if err := cli.NewVersionError(ctx, "1.44", "specify mac-address per network"); hasEndpointSpecificMacAddress(networkingConfig) && err != nil { return response, err } @@ -58,6 +58,11 @@ func (cli *Client) ContainerCreate(ctx context.Context, config *container.Config } } + // Since API 1.44, the container-wide MacAddress is deprecated and will trigger a WARNING if it's specified. + if versions.GreaterThanOrEqualTo(cli.ClientVersion(), "1.44") { + config.MacAddress = "" //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44. + } + query := url.Values{} if p := formatPlatform(platform); p != "" { query.Set("platform", p) diff --git a/daemon/inspect.go b/daemon/inspect.go index a60b61d67b..a6fc35b10a 100644 --- a/daemon/inspect.go +++ b/daemon/inspect.go @@ -27,8 +27,9 @@ func (daemon *Daemon) ContainerInspect(ctx context.Context, name string, size bo return daemon.containerInspectPre120(ctx, name) case versions.Equal(version, "1.20"): return daemon.containerInspect120(name) + default: + return daemon.ContainerInspectCurrent(ctx, name, size) } - return daemon.ContainerInspectCurrent(ctx, name, size) } // ContainerInspectCurrent returns low-level information about a @@ -116,7 +117,7 @@ func (daemon *Daemon) containerInspect120(name string) (*v1p20.ContainerJSON, er Mounts: ctr.GetMountPoints(), Config: &v1p20.ContainerConfig{ Config: ctr.Config, - MacAddress: ctr.Config.MacAddress, + MacAddress: ctr.Config.MacAddress, //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44. NetworkDisabled: ctr.Config.NetworkDisabled, ExposedPorts: ctr.Config.ExposedPorts, VolumeDriver: ctr.HostConfig.VolumeDriver, @@ -138,6 +139,18 @@ func (daemon *Daemon) getInspectData(daemonCfg *config.Config, container *contai // We merge the Ulimits from hostConfig with daemon default daemon.mergeUlimits(&hostConfig, daemonCfg) + // Migrate the container's default network's MacAddress to the top-level + // Config.MacAddress field for older API versions (< 1.44). We set it here + // unconditionally, to keep backward compatibility with clients that use + // 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. + } + } + } + var containerHealth *types.Health if container.State.Health != nil { containerHealth = &types.Health{ diff --git a/daemon/inspect_linux.go b/daemon/inspect_linux.go index 77cb008677..bcfaf105ed 100644 --- a/daemon/inspect_linux.go +++ b/daemon/inspect_linux.go @@ -47,7 +47,7 @@ func (daemon *Daemon) containerInspectPre120(ctx context.Context, name string) ( VolumesRW: volumesRW, Config: &v1p19.ContainerConfig{ Config: ctr.Config, - MacAddress: ctr.Config.MacAddress, + MacAddress: ctr.Config.MacAddress, //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44. NetworkDisabled: ctr.Config.NetworkDisabled, ExposedPorts: ctr.Config.ExposedPorts, VolumeDriver: ctr.HostConfig.VolumeDriver, diff --git a/daemon/network.go b/daemon/network.go index a49ded0f8b..35624e5acb 100644 --- a/daemon/network.go +++ b/daemon/network.go @@ -861,22 +861,6 @@ func buildCreateEndpointOptions(c *container.Container, n *libnetwork.Network, e createOptions = append(createOptions, libnetwork.CreateOptionDisableResolution()) } - // configs that are applicable only for the endpoint in the network - // to which container was connected to on docker run. - // Ideally all these network-specific endpoint configurations must be moved under - // container.NetworkSettings.Networks[n.Name()] - netMode := c.HostConfig.NetworkMode - if nwName == netMode.NetworkName() || n.ID() == netMode.NetworkName() || (nwName == defaultNetName && netMode.IsDefault()) { - if c.Config.MacAddress != "" { - mac, err := net.ParseMAC(c.Config.MacAddress) - if err != nil { - return nil, err - } - - genericOptions[netlabel.MacAddress] = mac - } - } - // Port-mapping rules belong to the container & applicable only to non-internal networks. // // TODO(thaJeztah): Look if we can provide a more minimal function for getPortMapInfo, as it does a lot, and we only need the "length". diff --git a/docs/api/version-history.md b/docs/api/version-history.md index 6d60ba46d3..ddef65e155 100644 --- a/docs/api/version-history.md +++ b/docs/api/version-history.md @@ -57,6 +57,7 @@ keywords: "API, Docker, rcli, REST, documentation" some configuration of Seccomp and AppArmor in Swarm services. * A new endpoint-specific `MacAddress` field has been added to `NetworkSettings.EndpointSettings` on `POST /containers/create`, and to `EndpointConfig` on `POST /networks/{id}/connect`. + The container-wide `MacAddress` field in `Config`, on `POST /containers/create`, is now deprecated. ## v1.43 API changes diff --git a/integration-cli/docker_cli_netmode_test.go b/integration-cli/docker_cli_netmode_test.go index 80ba1374f1..5756a06360 100644 --- a/integration-cli/docker_cli_netmode_test.go +++ b/integration-cli/docker_cli_netmode_test.go @@ -88,8 +88,6 @@ func (s *DockerCLINetmodeSuite) TestConflictNetworkModeAndOptions(c *testing.T) assert.Assert(c, strings.Contains(out, runconfig.ErrConflictNetworkAndDNS.Error())) out = dockerCmdWithFail(c, "run", "--net=container:other", "--add-host=name:8.8.8.8", "busybox", "ps") assert.Assert(c, strings.Contains(out, runconfig.ErrConflictNetworkHosts.Error())) - out = dockerCmdWithFail(c, "run", "--net=container:other", "--mac-address=92:d0:c6:0a:29:33", "busybox", "ps") - assert.Assert(c, strings.Contains(out, runconfig.ErrConflictContainerNetworkAndMac.Error())) out = dockerCmdWithFail(c, "run", "--net=container:other", "-P", "busybox", "ps") assert.Assert(c, strings.Contains(out, runconfig.ErrConflictNetworkPublishPorts.Error())) out = dockerCmdWithFail(c, "run", "--net=container:other", "-p", "8080", "busybox", "ps") diff --git a/integration-cli/docker_cli_run_test.go b/integration-cli/docker_cli_run_test.go index c96a0f6a9b..585a3239bb 100644 --- a/integration-cli/docker_cli_run_test.go +++ b/integration-cli/docker_cli_run_test.go @@ -3311,11 +3311,6 @@ func (s *DockerCLIRunSuite) TestRunContainerNetModeWithDNSMacHosts(c *testing.T) c.Fatalf("run --net=container with --dns should error out") } - out, _, err = dockerCmdWithError("run", "--mac-address", "92:d0:c6:0a:29:33", "--net=container:parent", "busybox") - if err == nil || !strings.Contains(out, runconfig.ErrConflictContainerNetworkAndMac.Error()) { - c.Fatalf("run --net=container with --mac-address should error out") - } - out, _, err = dockerCmdWithError("run", "--add-host", "test:192.168.2.109", "--net=container:parent", "busybox") if err == nil || !strings.Contains(out, runconfig.ErrConflictNetworkHosts.Error()) { c.Fatalf("run --net=container with --add-host should error out") diff --git a/integration/internal/container/ops.go b/integration/internal/container/ops.go index 2e29d4562a..43247c8c17 100644 --- a/integration/internal/container/ops.go +++ b/integration/internal/container/ops.go @@ -312,6 +312,6 @@ func WithStopSignal(stopSignal string) func(c *TestContainerConfig) { func WithContainerWideMacAddress(address string) func(c *TestContainerConfig) { return func(c *TestContainerConfig) { - c.Config.MacAddress = address + c.Config.MacAddress = address //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44. } } diff --git a/runconfig/hostconfig.go b/runconfig/hostconfig.go index 9603a94eca..29d3ac812e 100644 --- a/runconfig/hostconfig.go +++ b/runconfig/hostconfig.go @@ -53,10 +53,6 @@ func validateNetContainerMode(c *container.Config, hc *container.HostConfig) err return ErrConflictNetworkHosts } - if (hc.NetworkMode.IsContainer() || hc.NetworkMode.IsHost()) && c.MacAddress != "" { - return ErrConflictContainerNetworkAndMac - } - if hc.NetworkMode.IsContainer() && (len(hc.PortBindings) > 0 || hc.PublishAllPorts) { return ErrConflictNetworkPublishPorts }