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 <[email protected]> Signed-off-by: Sebastiaan van Stijn <[email protected]>
trebonian · Oct 25, 2023 · ee9f0ed · ee9f0ed
1 parent 052562f
commit ee9f0ed
Show file tree

Hide file tree

Showing 12 changed files with 93 additions and 40 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
 	}