daemon: backfill empty PBs slices for backward compat

So far, on ContainerStart, the daemon was silently backfilling empty
PortBindings slices with a PortBinding with unspecified HostIP and
HostPort. This was done by github.com/docker/go-connections/nat.SortPortMap.

This backfilling doesn't make much sense, and we're trying to remove
that package. So, move the backfilling to the API server, keep it for
older API versions, deprecate it for API 1.52, and drop it for API 1.53
and above.

Signed-off-by: Albin Kerouanton <albinker@gmail.com>

api/docs/CHANGELOG.md

@@ -22,6 +22,10 @@ keywords: "API, Docker, rcli, REST, documentation"
   saved.
 * `POST /images/load` now accepts multiple `platform` query-arguments
   to allow selecting which platform(s) of a multi-platform image to load.
+* Deprecated: the Engine was automatically backfilling empty `PortBindings` lists with
+  a PortBinding with an empty HostIP and HostPort when calling `POST /containers/{id}/start`.
+  This behavior is now deprecated, and a warning is returned by `POST /containers/create`.
+  The next API version will drop empty `PortBindings` list altogether.
 
 ## v1.51 API changes
 

daemon/container/container.go

@@ -260,6 +260,7 @@ func (container *Container) readHostConfig() error {
 	}
 
 	container.InitDNSHostConfig()
+	container.BackfillEmptyPBs()
 
 	return nil
 }
@@ -625,6 +626,32 @@ func (container *Container) InitDNSHostConfig() {
 	}
 }
 
+// BackfillEmptyPBs backfills empty PortBindings slices with a port binding
+// with an unspecified HostIP and HostPort. This is required to ensure backward
+// compatibility for containers created with older Engine versions.
+//
+// Prior to v29.0, the daemon was doing this backfilling automatically through
+// github.com/docker/go-connections/nat on ContainerStart. Starting with that
+// version, it is done by the API for older API versions (i.e. < 1.53). Newer
+// API versions are just dropping entries with empty PortBindings slices.
+//
+// See https://github.com/moby/moby/pull/50710#discussion_r2315840899 for more
+// context.
+func (container *Container) BackfillEmptyPBs() {
+	if container.HostConfig == nil {
+		return
+	}
+
+	for portProto, pb := range container.HostConfig.PortBindings {
+		if len(pb) > 0 || pb == nil {
+			continue
+		}
+		container.HostConfig.PortBindings[portProto] = []containertypes.PortBinding{
+			{}, // Backfill an empty PortBinding
+		}
+	}
+}
+
 // UpdateMonitor updates monitor configure for running container
 func (container *Container) UpdateMonitor(restartPolicy containertypes.RestartPolicy) {
 	container.RestartManager().SetPolicy(restartPolicy)

daemon/server/router/container/container_routes.go

@@ -12,6 +12,7 @@ import (
 
 	"github.com/containerd/log"
 	"github.com/containerd/platforms"
+	"github.com/docker/go-connections/nat"
 	"github.com/moby/moby/api/types"
 	"github.com/moby/moby/api/types/container"
 	"github.com/moby/moby/api/types/filters"
@@ -684,6 +685,10 @@ func (c *containerRouter) postContainersCreate(ctx context.Context, w http.Respo
 		warnings = append(warnings, warn)
 	}
 
+	if warn := handlePortBindingsBC(hostConfig, version); 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.
@@ -874,6 +879,45 @@ func handleSysctlBC(
 	return warning, nil
 }
 
+// handlePortBindingsBC handles backward-compatibility for empty port bindings.
+//
+// Before Engine v29.0, an empty list of port bindings for a container port was
+// treated as if a PortBinding with an unspecified IP address and HostPort was
+// provided. The daemon was doing this backfilling on ContainerStart.
+//
+// Preserve this behavior for older API versions but emit a warning for API
+// v1.52 and drop that behavior for newer API versions.
+//
+// See https://github.com/moby/moby/pull/50710#discussion_r2315840899 for more
+// context.
+func handlePortBindingsBC(hostConfig *container.HostConfig, version string) string {
+	var warning string
+
+	for portProto, bindings := range hostConfig.PortBindings {
+		if len(bindings) > 0 {
+			continue
+		}
+		if versions.GreaterThan(version, "1.52") && len(bindings) == 0 {
+			// Starting with API 1.53, no backfilling is done. An empty slice
+			// of port bindings is treated as "no port bindings" by the daemon,
+			// but it still needs to backfill empty slices when loading the
+			// on-disk state for containers created by older versions of the
+			// Engine. Drop the PortBindings entry to ensure that no backfilling
+			// will happen when restarting the daemon.
+			delete(hostConfig.PortBindings, portProto)
+			continue
+		}
+
+		if versions.Equal(version, "1.52") {
+			warning = fmt.Sprintf("Container port %s has an empty list of port-bindings. Starting with API 1.53, this will be discarded.", portProto)
+		}
+
+		hostConfig.PortBindings[portProto] = []nat.PortBinding{{}}
+	}
+
+	return warning
+}
+
 // epConfigForNetMode finds, or creates, an entry in netConfig.EndpointsConfig
 // corresponding to nwMode.
 //

integration/network/bridge/bridge_linux_test.go

@@ -9,10 +9,12 @@ import (
 	"testing"
 	"time"
 
+	"github.com/docker/go-connections/nat"
 	containertypes "github.com/moby/moby/api/types/container"
 	networktypes "github.com/moby/moby/api/types/network"
 	"github.com/moby/moby/api/types/versions"
 	"github.com/moby/moby/client"
+	"github.com/moby/moby/v2/daemon/container"
 	"github.com/moby/moby/v2/daemon/libnetwork/drivers/bridge"
 	"github.com/moby/moby/v2/daemon/libnetwork/netlabel"
 	"github.com/moby/moby/v2/daemon/libnetwork/nlwrap"
@@ -941,3 +943,124 @@ func TestFirewallBackendSwitch(t *testing.T) {
 	assert.Check(t, len(dockerChains) > 0, "Expected iptables to have at least one docker chain")
 	assert.Check(t, !nftablesTablesExist(), "nftables tables exist after running with iptables")
 }
+
+func TestEmptyPortBindingsBC(t *testing.T) {
+	ctx := setupTest(t)
+	d := daemon.New(t)
+	d.StartWithBusybox(ctx, t)
+	defer d.Stop(t)
+
+	createInspect := func(t *testing.T, version string, pbs []nat.PortBinding) (containertypes.PortMap, []string) {
+		apiClient := d.NewClientT(t, client.WithVersion(version))
+		defer apiClient.Close()
+
+		// Skip this subtest if the daemon doesn't support the client version.
+		// TODO(aker): drop this once the Engine supports API version >= 1.53
+		_, err := apiClient.ServerVersion(context.TODO())
+		if err != nil && strings.Contains(err.Error(), fmt.Sprintf("client version %s is too new", version)) {
+			t.Skipf("requires API %s", version)
+		}
+		assert.NilError(t, err)
+
+		// Create a container with an empty list of port bindings for container port 80/tcp.
+		config := ctr.NewTestConfig(ctr.WithCmd("top"),
+			ctr.WithExposedPorts("80/tcp"),
+			ctr.WithPortMap(containertypes.PortMap{"80/tcp": pbs}))
+		c, err := apiClient.ContainerCreate(ctx, config.Config, config.HostConfig, config.NetworkingConfig, config.Platform, config.Name)
+		assert.NilError(t, err)
+		defer apiClient.ContainerRemove(ctx, c.ID, containertypes.RemoveOptions{Force: true})
+
+		// Inspect the container and return its port bindings, along with
+		// warnings returns on container create.
+		inspect, err := apiClient.ContainerInspect(ctx, c.ID)
+		assert.NilError(t, err)
+		return inspect.HostConfig.PortBindings, c.Warnings
+	}
+
+	t.Run("backfilling on old client version", func(t *testing.T) {
+		expMappings := containertypes.PortMap{"80/tcp": {
+			{}, // An empty PortBinding is backfilled
+		}}
+		expWarnings := make([]string, 0)
+
+		mappings, warnings := createInspect(t, "1.51", []nat.PortBinding{})
+		assert.DeepEqual(t, expMappings, mappings)
+		assert.DeepEqual(t, expWarnings, warnings)
+	})
+
+	t.Run("backfilling on API 1.52, with a warning", func(t *testing.T) {
+		expMappings := containertypes.PortMap{"80/tcp": {
+			{}, // An empty PortBinding is backfilled
+		}}
+		expWarnings := []string{
+			"Container port 80/tcp has an empty list of port-bindings. Starting with API 1.53, this will be discarded.",
+		}
+
+		mappings, warnings := createInspect(t, "1.52", []nat.PortBinding{})
+		assert.DeepEqual(t, expMappings, mappings)
+		assert.DeepEqual(t, expWarnings, warnings)
+	})
+
+	t.Run("no backfilling on API 1.53", func(t *testing.T) {
+		expMappings := containertypes.PortMap{}
+		expWarnings := make([]string, 0)
+
+		mappings, warnings := createInspect(t, "1.53", []nat.PortBinding{})
+		assert.DeepEqual(t, expMappings, mappings)
+		assert.DeepEqual(t, expWarnings, warnings)
+	})
+
+	for _, apiVersion := range []string{"1.51", "1.52", "1.53"} {
+		t.Run("no backfilling on API "+apiVersion+" with non-empty bindings", func(t *testing.T) {
+			expMappings := containertypes.PortMap{"80/tcp": {
+				{HostPort: "8080"},
+			}}
+			expWarnings := make([]string, 0)
+
+			mappings, warnings := createInspect(t, apiVersion, []nat.PortBinding{{HostPort: "8080"}})
+			assert.DeepEqual(t, expMappings, mappings)
+			assert.DeepEqual(t, expWarnings, warnings)
+		})
+	}
+}
+
+// TestPortBindingBackfillingForOlderContainers verify that the daemon
+// correctly backfills empty port bindings for containers created with prior
+// versions of the Engine.
+func TestPortBindingBackfillingForOlderContainers(t *testing.T) {
+	ctx := setupTest(t)
+	d := daemon.New(t)
+	d.StartWithBusybox(ctx, t)
+	defer d.Stop(t)
+
+	// We don't really care which version of the API is used here as we're
+	// going to tamper with the on-disk state of the container. Even if the
+	// daemon backfills the empty port bindings on ContainerCreate (e.g.,
+	// API < 1.53), the tampering will reinitialize the PortBindings slice to
+	// an empty list.
+	c := d.NewClientT(t)
+
+	cid := ctr.Create(ctx, t, c,
+		ctr.WithExposedPorts("80/tcp"),
+		ctr.WithPortMap(containertypes.PortMap{"80/tcp": {}}))
+	defer c.ContainerRemove(ctx, cid, containertypes.RemoveOptions{Force: true})
+
+	// Stop the daemon to safely tamper with the on-disk state.
+	d.Stop(t)
+
+	d.TamperWithContainerConfig(t, cid, func(container *container.Container) {
+		// Simulate a container created with an older version of the Engine
+		// by setting an empty list of port bindings.
+		container.HostConfig.PortBindings = containertypes.PortMap{"80/tcp": {}}
+	})
+
+	// Restart the daemon — it should backfill the empty port binding slice.
+	d.StartWithBusybox(ctx, t)
+
+	inspect := ctr.Inspect(ctx, t, c, cid)
+
+	expMappings := containertypes.PortMap{"80/tcp": {
+		{}, // An empty PortBinding is backfilled
+	}}
+	assert.DeepEqual(t, expMappings, inspect.HostConfig.PortBindings)
+}

testutil/daemon/daemon.go

@@ -1017,13 +1017,24 @@ func (d *Daemon) TamperWithContainerConfig(t testing.TB, containerID string, tam
 	configBytes, err := os.ReadFile(configPath)
 	assert.NilError(t, err)
 
+	hostConfigPath := filepath.Join(d.Root, "containers", containerID, "hostconfig.json")
+	hostConfigBytes, err := os.ReadFile(hostConfigPath)
+	assert.NilError(t, err)
+
 	var c container.Container
 	assert.NilError(t, json.Unmarshal(configBytes, &c))
+	assert.NilError(t, json.Unmarshal(hostConfigBytes, &c.HostConfig))
+
 	c.State = container.NewState()
 	tamper(&c)
+
 	configBytes, err = json.Marshal(&c)
 	assert.NilError(t, err)
 	assert.NilError(t, os.WriteFile(configPath, configBytes, 0o600))
+
+	hostConfigBytes, err = json.Marshal(&c.HostConfig)
+	assert.NilError(t, err)
+	assert.NilError(t, os.WriteFile(hostConfigPath, hostConfigBytes, 0o600))
 }
 
 // cleanupRaftDir removes swarmkit wal files if present