From 0ca7ac325850a47c40024c9c9df1f39d885bff8c Mon Sep 17 00:00:00 2001 From: Albin Kerouanton Date: Tue, 2 Sep 2025 13:29:56 +0200 Subject: [PATCH] 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 --- api/docs/CHANGELOG.md | 4 + daemon/container/container.go | 27 ++++ .../router/container/container_routes.go | 44 +++++++ .../network/bridge/bridge_linux_test.go | 123 ++++++++++++++++++ testutil/daemon/daemon.go | 11 ++ 5 files changed, 209 insertions(+) diff --git a/api/docs/CHANGELOG.md b/api/docs/CHANGELOG.md index ac39c6e1d3..ab48fb5b47 100644 --- a/api/docs/CHANGELOG.md +++ b/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 diff --git a/daemon/container/container.go b/daemon/container/container.go index 0698a0b1c1..2fa4aaa441 100644 --- a/daemon/container/container.go +++ b/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) diff --git a/daemon/server/router/container/container_routes.go b/daemon/server/router/container/container_routes.go index 54a0f1032d..ad92a73d20 100644 --- a/daemon/server/router/container/container_routes.go +++ b/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. // diff --git a/integration/network/bridge/bridge_linux_test.go b/integration/network/bridge/bridge_linux_test.go index 92ec82a4c3..4139808d0b 100644 --- a/integration/network/bridge/bridge_linux_test.go +++ b/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) +} diff --git a/testutil/daemon/daemon.go b/testutil/daemon/daemon.go index 5f4afbdc14..17a4e8cf49 100644 --- a/testutil/daemon/daemon.go +++ b/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