Merge pull request #50874 from akerouanton/bc-empty-pbs-slices

daemon: backfill empty PBs slices for backward compat

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