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