From d26bdfe226b35fc754c3de3e395fb68bd324a723 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 21 Jan 2024 20:08:44 +0100 Subject: [PATCH 01/19] runconfig: remove fixtures for api < v1.19 Signed-off-by: Sebastiaan van Stijn --- runconfig/config_test.go | 3 +- .../fixtures/unix/container_config_1_14.json | 30 ----------- .../fixtures/unix/container_config_1_17.json | 50 ------------------- 3 files changed, 1 insertion(+), 82 deletions(-) delete mode 100644 runconfig/fixtures/unix/container_config_1_14.json delete mode 100644 runconfig/fixtures/unix/container_config_1_17.json diff --git a/runconfig/config_test.go b/runconfig/config_test.go index c001fe932e..ff4a412e16 100644 --- a/runconfig/config_test.go +++ b/runconfig/config_test.go @@ -26,11 +26,10 @@ func TestDecodeContainerConfig(t *testing.T) { imgName string ) + // FIXME (thaJeztah): update fixtures for more current versions. if runtime.GOOS != "windows" { imgName = "ubuntu" fixtures = []f{ - {"fixtures/unix/container_config_1_14.json", strslice.StrSlice{}}, - {"fixtures/unix/container_config_1_17.json", strslice.StrSlice{"bash"}}, {"fixtures/unix/container_config_1_19.json", strslice.StrSlice{"bash"}}, } } else { diff --git a/runconfig/fixtures/unix/container_config_1_14.json b/runconfig/fixtures/unix/container_config_1_14.json deleted file mode 100644 index b08334c095..0000000000 --- a/runconfig/fixtures/unix/container_config_1_14.json +++ /dev/null @@ -1,30 +0,0 @@ -{ - "Hostname":"", - "Domainname": "", - "User":"", - "Memory": 1000, - "MemorySwap":0, - "CpuShares": 512, - "Cpuset": "0,1", - "AttachStdin":false, - "AttachStdout":true, - "AttachStderr":true, - "PortSpecs":null, - "Tty":false, - "OpenStdin":false, - "StdinOnce":false, - "Env":null, - "Cmd":[ - "bash" - ], - "Image":"ubuntu", - "Volumes":{ - "/tmp": {} - }, - "WorkingDir":"", - "NetworkDisabled": false, - "ExposedPorts":{ - "22/tcp": {} - }, - "RestartPolicy": { "Name": "always" } -} diff --git a/runconfig/fixtures/unix/container_config_1_17.json b/runconfig/fixtures/unix/container_config_1_17.json deleted file mode 100644 index 0d780877b4..0000000000 --- a/runconfig/fixtures/unix/container_config_1_17.json +++ /dev/null @@ -1,50 +0,0 @@ -{ - "Hostname": "", - "Domainname": "", - "User": "", - "Memory": 1000, - "MemorySwap": 0, - "CpuShares": 512, - "Cpuset": "0,1", - "AttachStdin": false, - "AttachStdout": true, - "AttachStderr": true, - "Tty": false, - "OpenStdin": false, - "StdinOnce": false, - "Env": null, - "Cmd": [ - "date" - ], - "Entrypoint": "bash", - "Image": "ubuntu", - "Volumes": { - "/tmp": {} - }, - "WorkingDir": "", - "NetworkDisabled": false, - "MacAddress": "12:34:56:78:9a:bc", - "ExposedPorts": { - "22/tcp": {} - }, - "SecurityOpt": [""], - "HostConfig": { - "Binds": ["/tmp:/tmp"], - "Links": ["redis3:redis"], - "LxcConf": {"lxc.utsname":"docker"}, - "PortBindings": { "22/tcp": [{ "HostPort": "11022" }] }, - "PublishAllPorts": false, - "Privileged": false, - "ReadonlyRootfs": false, - "Dns": ["8.8.8.8"], - "DnsSearch": [""], - "DnsOptions": [""], - "ExtraHosts": null, - "VolumesFrom": ["parent", "other:ro"], - "CapAdd": ["NET_ADMIN"], - "CapDrop": ["MKNOD"], - "RestartPolicy": { "Name": "", "MaximumRetryCount": 0 }, - "NetworkMode": "bridge", - "Devices": [] - } -} From 1b1147e46b732caeaed4ae365cd56ccbfdf40233 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 21 Jan 2024 23:49:10 +0100 Subject: [PATCH 02/19] api: POST /commit: remove version-gate for "pause" (api < v1.16) The "pause" flag was added in API v1.13 (Docker Engine v1.1.0), and is enabled by default (see 17d870bed5ef997c30da1e8b9843f4e84202f8d4). API v1.23 and older are deprecated, so we can remove the version-gate. Signed-off-by: Sebastiaan van Stijn --- api/server/router/container/container_routes.go | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/api/server/router/container/container_routes.go b/api/server/router/container/container_routes.go index dbb2b9beda..8b400e61cb 100644 --- a/api/server/router/container/container_routes.go +++ b/api/server/router/container/container_routes.go @@ -39,13 +39,6 @@ func (s *containerRouter) postCommit(ctx context.Context, w http.ResponseWriter, return err } - // TODO: remove pause arg, and always pause in backend - pause := httputils.BoolValue(r, "pause") - version := httputils.VersionFromContext(ctx) - if r.FormValue("pause") == "" && versions.GreaterThanOrEqualTo(version, "1.13") { - pause = true - } - config, _, _, err := s.decoder.DecodeConfig(r.Body) if err != nil && !errors.Is(err, io.EOF) { // Do not fail if body is empty. return err @@ -57,7 +50,7 @@ func (s *containerRouter) postCommit(ctx context.Context, w http.ResponseWriter, } imgID, err := s.backend.CreateImageFromContainer(ctx, r.Form.Get("container"), &backend.CreateImageConfig{ - Pause: pause, + Pause: httputils.BoolValueOrDefault(r, "pause", true), // TODO(dnephin): remove pause arg, and always pause in backend Tag: ref, Author: r.Form.Get("author"), Comment: r.Form.Get("comment"), From 7fa116830b9187d9aa39420982a7e0c88caf011a Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 21 Jan 2024 21:32:28 +0100 Subject: [PATCH 03/19] api: POST /build: remove version-gate for "rm", "force-rm" (api < v1.16) The "rm" option was made the default in API v1.12 (Docker Engine v1.0.0) in commit b60d6471721bc914dca179a4372303d41913cc4c, and "force-rm" was added in 667e2bd4ea5fbc8698c34565f955cb92cff92890. API v1.23 and older are deprecated, so we can remove these gates. Signed-off-by: Sebastiaan van Stijn --- api/server/router/build/build_routes.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/api/server/router/build/build_routes.go b/api/server/router/build/build_routes.go index 0da19e61c9..11a7108a51 100644 --- a/api/server/router/build/build_routes.go +++ b/api/server/router/build/build_routes.go @@ -66,14 +66,14 @@ func newImageBuildOptions(ctx context.Context, r *http.Request) (*types.ImageBui return nil, invalidParam{errors.New("security options are not supported on " + runtime.GOOS)} } - version := httputils.VersionFromContext(ctx) - if httputils.BoolValue(r, "forcerm") && versions.GreaterThanOrEqualTo(version, "1.12") { + if httputils.BoolValue(r, "forcerm") { options.Remove = true - } else if r.FormValue("rm") == "" && versions.GreaterThanOrEqualTo(version, "1.12") { + } else if r.FormValue("rm") == "" { options.Remove = true } else { options.Remove = httputils.BoolValue(r, "rm") } + version := httputils.VersionFromContext(ctx) if httputils.BoolValue(r, "pull") && versions.GreaterThanOrEqualTo(version, "1.16") { options.PullParent = true } From ef25f0aa52fdcd6c5b1ec4c66024f5e7bafb5c29 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 21 Jan 2024 21:21:59 +0100 Subject: [PATCH 04/19] api: POST /build: remove version-gate for "pull" (api < v1.16) The "pull" option was added in API v1.16 (Docker Engine v1.4.0) in commit 054e57a622e6a065c343806e7334920d17a03c5b, which gated the option by API version. API v1.23 and older are deprecated, so we can remove the gate. Signed-off-by: Sebastiaan van Stijn --- api/server/router/build/build_routes.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/api/server/router/build/build_routes.go b/api/server/router/build/build_routes.go index 11a7108a51..186e8e483b 100644 --- a/api/server/router/build/build_routes.go +++ b/api/server/router/build/build_routes.go @@ -42,6 +42,7 @@ func newImageBuildOptions(ctx context.Context, r *http.Request) (*types.ImageBui SuppressOutput: httputils.BoolValue(r, "q"), NoCache: httputils.BoolValue(r, "nocache"), ForceRemove: httputils.BoolValue(r, "forcerm"), + PullParent: httputils.BoolValue(r, "pull"), MemorySwap: httputils.Int64ValueOrZero(r, "memswap"), Memory: httputils.Int64ValueOrZero(r, "memory"), CPUShares: httputils.Int64ValueOrZero(r, "cpushares"), @@ -74,9 +75,6 @@ func newImageBuildOptions(ctx context.Context, r *http.Request) (*types.ImageBui options.Remove = httputils.BoolValue(r, "rm") } version := httputils.VersionFromContext(ctx) - if httputils.BoolValue(r, "pull") && versions.GreaterThanOrEqualTo(version, "1.16") { - options.PullParent = true - } if versions.GreaterThanOrEqualTo(version, "1.32") { options.Platform = r.FormValue("platform") } From 2970b320aa12455ca16f1e0bcf34007a9f198439 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 21 Jan 2024 16:54:06 +0100 Subject: [PATCH 05/19] api: remove code for adjusting CPU shares (api < v1.19) API versions before 1.19 allowed CpuShares that were greater than the maximum or less than the minimum supported by the kernel, and relied on the kernel to do the right thing. Commit ed39fbeb2ad3959f37cf6c16aaf30aacb3292817 introduced code to adjust the CPU shares to be within the accepted range when using API version 1.18 or lower. API v1.23 and older are deprecated, so we can remove support for this functionality. Currently, there's no validation for CPU shares to be within an acceptable range; a TODO was added to add validation for this option, and to use the `linuxMinCPUShares` and `linuxMaxCPUShares` consts for this. Signed-off-by: Sebastiaan van Stijn --- .../router/container/container_routes.go | 2 - api/types/backend/backend.go | 1 - daemon/create.go | 2 +- daemon/daemon_unix.go | 23 +++--- daemon/daemon_unix_test.go | 81 ------------------- daemon/daemon_windows.go | 2 +- daemon/start.go | 2 +- integration-cli/docker_api_containers_test.go | 14 ---- 8 files changed, 13 insertions(+), 114 deletions(-) diff --git a/api/server/router/container/container_routes.go b/api/server/router/container/container_routes.go index 8b400e61cb..69dfbf9076 100644 --- a/api/server/router/container/container_routes.go +++ b/api/server/router/container/container_routes.go @@ -505,7 +505,6 @@ func (s *containerRouter) postContainersCreate(ctx context.Context, w http.Respo } version := httputils.VersionFromContext(ctx) - adjustCPUShares := versions.LessThan(version, "1.19") // When using API 1.24 and under, the client is responsible for removing the container if versions.LessThan(version, "1.25") { @@ -649,7 +648,6 @@ func (s *containerRouter) postContainersCreate(ctx context.Context, w http.Respo Config: config, HostConfig: hostConfig, NetworkingConfig: networkingConfig, - AdjustCPUShares: adjustCPUShares, Platform: platform, }) if err != nil { diff --git a/api/types/backend/backend.go b/api/types/backend/backend.go index a4fb9d638a..c9fc978884 100644 --- a/api/types/backend/backend.go +++ b/api/types/backend/backend.go @@ -18,7 +18,6 @@ type ContainerCreateConfig struct { HostConfig *container.HostConfig NetworkingConfig *network.NetworkingConfig Platform *ocispec.Platform - AdjustCPUShares bool } // ContainerRmConfig holds arguments for the container remove diff --git a/daemon/create.go b/daemon/create.go index aa063b60aa..4f8da469b7 100644 --- a/daemon/create.go +++ b/daemon/create.go @@ -108,7 +108,7 @@ func (daemon *Daemon) containerCreate(ctx context.Context, daemonCfg *configStor if opts.params.HostConfig == nil { opts.params.HostConfig = &containertypes.HostConfig{} } - err = daemon.adaptContainerSettings(&daemonCfg.Config, opts.params.HostConfig, opts.params.AdjustCPUShares) + err = daemon.adaptContainerSettings(&daemonCfg.Config, opts.params.HostConfig) if err != nil { return containertypes.CreateResponse{Warnings: warnings}, errdefs.InvalidParameter(err) } diff --git a/daemon/daemon_unix.go b/daemon/daemon_unix.go index 1143dda063..120f514ef7 100644 --- a/daemon/daemon_unix.go +++ b/daemon/daemon_unix.go @@ -54,9 +54,16 @@ import ( const ( isWindows = false + // These values were used to adjust the CPU-shares for older API versions, + // but were not used for validation. + // + // TODO(thaJeztah): validate min/max values for CPU-shares, similar to Windows: https://github.com/moby/moby/issues/47340 + // https://github.com/moby/moby/blob/27e85c7b6885c2d21ae90791136d9aba78b83d01/daemon/daemon_windows.go#L97-L99 + // // See https://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/tree/kernel/sched/sched.h?id=8cd9234c64c584432f6992fe944ca9e46ca8ea76#n269 - linuxMinCPUShares = 2 - linuxMaxCPUShares = 262144 + // linuxMinCPUShares = 2 + // linuxMaxCPUShares = 262144 + // It's not kernel limit, we want this 6M limit to account for overhead during startup, and to supply a reasonable functional container linuxMinMemory = 6291456 // constants for remapped root settings @@ -306,17 +313,7 @@ func adjustParallelLimit(n int, limit int) int { // adaptContainerSettings is called during container creation to modify any // settings necessary in the HostConfig structure. -func (daemon *Daemon) adaptContainerSettings(daemonCfg *config.Config, hostConfig *containertypes.HostConfig, adjustCPUShares bool) error { - if adjustCPUShares && hostConfig.CPUShares > 0 { - // Handle unsupported CPUShares - if hostConfig.CPUShares < linuxMinCPUShares { - log.G(context.TODO()).Warnf("Changing requested CPUShares of %d to minimum allowed of %d", hostConfig.CPUShares, linuxMinCPUShares) - hostConfig.CPUShares = linuxMinCPUShares - } else if hostConfig.CPUShares > linuxMaxCPUShares { - log.G(context.TODO()).Warnf("Changing requested CPUShares of %d to maximum allowed of %d", hostConfig.CPUShares, linuxMaxCPUShares) - hostConfig.CPUShares = linuxMaxCPUShares - } - } +func (daemon *Daemon) adaptContainerSettings(daemonCfg *config.Config, hostConfig *containertypes.HostConfig) error { if hostConfig.Memory > 0 && hostConfig.MemorySwap == 0 { // By default, MemorySwap is set to twice the size of Memory. hostConfig.MemorySwap = hostConfig.Memory * 2 diff --git a/daemon/daemon_unix_test.go b/daemon/daemon_unix_test.go index 37dab2dd9b..e681eac892 100644 --- a/daemon/daemon_unix_test.go +++ b/daemon/daemon_unix_test.go @@ -57,87 +57,6 @@ func TestAdjustSharedNamespaceContainerName(t *testing.T) { } } -// Unix test as uses settings which are not available on Windows -func TestAdjustCPUShares(t *testing.T) { - tmp, err := os.MkdirTemp("", "docker-daemon-unix-test-") - if err != nil { - t.Fatal(err) - } - defer os.RemoveAll(tmp) - daemon := &Daemon{ - repository: tmp, - root: tmp, - } - cfg := &config.Config{} - muteLogs(t) - - hostConfig := &containertypes.HostConfig{ - Resources: containertypes.Resources{CPUShares: linuxMinCPUShares - 1}, - } - daemon.adaptContainerSettings(cfg, hostConfig, true) - if hostConfig.CPUShares != linuxMinCPUShares { - t.Errorf("Expected CPUShares to be %d", linuxMinCPUShares) - } - - hostConfig.CPUShares = linuxMaxCPUShares + 1 - daemon.adaptContainerSettings(cfg, hostConfig, true) - if hostConfig.CPUShares != linuxMaxCPUShares { - t.Errorf("Expected CPUShares to be %d", linuxMaxCPUShares) - } - - hostConfig.CPUShares = 0 - daemon.adaptContainerSettings(cfg, hostConfig, true) - if hostConfig.CPUShares != 0 { - t.Error("Expected CPUShares to be unchanged") - } - - hostConfig.CPUShares = 1024 - daemon.adaptContainerSettings(cfg, hostConfig, true) - if hostConfig.CPUShares != 1024 { - t.Error("Expected CPUShares to be unchanged") - } -} - -// Unix test as uses settings which are not available on Windows -func TestAdjustCPUSharesNoAdjustment(t *testing.T) { - tmp, err := os.MkdirTemp("", "docker-daemon-unix-test-") - if err != nil { - t.Fatal(err) - } - defer os.RemoveAll(tmp) - daemon := &Daemon{ - repository: tmp, - root: tmp, - } - cfg := &config.Config{} - - hostConfig := &containertypes.HostConfig{ - Resources: containertypes.Resources{CPUShares: linuxMinCPUShares - 1}, - } - daemon.adaptContainerSettings(cfg, hostConfig, false) - if hostConfig.CPUShares != linuxMinCPUShares-1 { - t.Errorf("Expected CPUShares to be %d", linuxMinCPUShares-1) - } - - hostConfig.CPUShares = linuxMaxCPUShares + 1 - daemon.adaptContainerSettings(cfg, hostConfig, false) - if hostConfig.CPUShares != linuxMaxCPUShares+1 { - t.Errorf("Expected CPUShares to be %d", linuxMaxCPUShares+1) - } - - hostConfig.CPUShares = 0 - daemon.adaptContainerSettings(cfg, hostConfig, false) - if hostConfig.CPUShares != 0 { - t.Error("Expected CPUShares to be unchanged") - } - - hostConfig.CPUShares = 1024 - daemon.adaptContainerSettings(cfg, hostConfig, false) - if hostConfig.CPUShares != 1024 { - t.Error("Expected CPUShares to be unchanged") - } -} - // Unix test as uses settings which are not available on Windows func TestParseSecurityOptWithDeprecatedColon(t *testing.T) { opts := &container.SecurityOptions{} diff --git a/daemon/daemon_windows.go b/daemon/daemon_windows.go index 1b03e4f5a7..54bc6d4941 100644 --- a/daemon/daemon_windows.go +++ b/daemon/daemon_windows.go @@ -66,7 +66,7 @@ func setupInitLayer(idMapping idtools.IdentityMapping) func(string) error { // adaptContainerSettings is called during container creation to modify any // settings necessary in the HostConfig structure. -func (daemon *Daemon) adaptContainerSettings(daemonCfg *config.Config, hostConfig *containertypes.HostConfig, adjustCPUShares bool) error { +func (daemon *Daemon) adaptContainerSettings(daemonCfg *config.Config, hostConfig *containertypes.HostConfig) error { return nil } diff --git a/daemon/start.go b/daemon/start.go index 516c069958..d315ccff56 100644 --- a/daemon/start.go +++ b/daemon/start.go @@ -96,7 +96,7 @@ func (daemon *Daemon) ContainerStart(ctx context.Context, name string, hostConfi // Adapt for old containers in case we have updates in this function and // old containers never have chance to call the new function in create stage. if hostConfig != nil { - if err := daemon.adaptContainerSettings(&daemonCfg.Config, ctr.HostConfig, false); err != nil { + if err := daemon.adaptContainerSettings(&daemonCfg.Config, ctr.HostConfig); err != nil { return errdefs.InvalidParameter(err) } } diff --git a/integration-cli/docker_api_containers_test.go b/integration-cli/docker_api_containers_test.go index c3839aad66..fdab67e0fc 100644 --- a/integration-cli/docker_api_containers_test.go +++ b/integration-cli/docker_api_containers_test.go @@ -1251,20 +1251,6 @@ func (s *DockerAPISuite) TestPostContainersCreateWithStringOrSliceCapAddDrop(c * assert.NilError(c, err) } -// #14915 -func (s *DockerAPISuite) TestContainerAPICreateNoHostConfig118(c *testing.T) { - testRequires(c, DaemonIsLinux) // Windows only support 1.25 or later - config := container.Config{ - Image: "busybox", - } - - apiClient, err := client.NewClientWithOpts(client.FromEnv, client.WithVersion("v1.18")) - assert.NilError(c, err) - - _, err = apiClient.ContainerCreate(testutil.GetContext(c), &config, &container.HostConfig{}, &network.NetworkingConfig{}, nil, "") - assert.NilError(c, err) -} - // Ensure an error occurs when you have a container read-only rootfs but you // extract an archive to a symlink in a writable volume which points to a // directory outside of the volume. From dfdf2adf0cc97103b8d2a81f28bd2885e1f324ad Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 21 Jan 2024 19:46:53 +0100 Subject: [PATCH 06/19] api: POST /containers/{id}/kill: remove handling for api < 1.20 API v1.20 and up produces an error when signalling / killing a non-running container (see c92377e300d2e9863ffa8eda9c3166a039b60e09). Older API versions allowed this, and an exception was added in 621e3d8587bbee86b4e36d0b7822662bfbedd76c. API v1.23 and older are deprecated, so we can remove this handling. Signed-off-by: Sebastiaan van Stijn --- api/server/router/container/container_routes.go | 15 ++------------- integration/container/kill_test.go | 10 ---------- 2 files changed, 2 insertions(+), 23 deletions(-) diff --git a/api/server/router/container/container_routes.go b/api/server/router/container/container_routes.go index 69dfbf9076..2b35c00b54 100644 --- a/api/server/router/container/container_routes.go +++ b/api/server/router/container/container_routes.go @@ -248,25 +248,14 @@ func (s *containerRouter) postContainersStop(ctx context.Context, w http.Respons return nil } -func (s *containerRouter) postContainersKill(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error { +func (s *containerRouter) postContainersKill(_ context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error { if err := httputils.ParseForm(r); err != nil { return err } name := vars["name"] if err := s.backend.ContainerKill(name, r.Form.Get("signal")); err != nil { - var isStopped bool - if errdefs.IsConflict(err) { - isStopped = true - } - - // Return error that's not caused because the container is stopped. - // Return error if the container is not running and the api is >= 1.20 - // to keep backwards compatibility. - version := httputils.VersionFromContext(ctx) - if versions.GreaterThanOrEqualTo(version, "1.20") || !isStopped { - return errors.Wrapf(err, "Cannot kill container: %s", name) - } + return errors.Wrapf(err, "cannot kill container: %s", name) } w.WriteHeader(http.StatusNoContent) diff --git a/integration/container/kill_test.go b/integration/container/kill_test.go index 8992dd23dc..4d44f02a6b 100644 --- a/integration/container/kill_test.go +++ b/integration/container/kill_test.go @@ -6,7 +6,6 @@ import ( "time" containertypes "github.com/docker/docker/api/types/container" - "github.com/docker/docker/client" "github.com/docker/docker/integration/internal/container" "github.com/docker/docker/testutil" "github.com/docker/docker/testutil/request" @@ -133,15 +132,6 @@ func TestKillStoppedContainer(t *testing.T) { assert.Assert(t, is.Contains(err.Error(), "is not running")) } -func TestKillStoppedContainerAPIPre120(t *testing.T) { - skip.If(t, testEnv.DaemonInfo.OSType == "windows", "Windows only supports 1.25 or later") - ctx := setupTest(t) - apiClient := request.NewAPIClient(t, client.WithVersion("1.19")) - id := container.Create(ctx, t, apiClient) - err := apiClient.ContainerKill(ctx, id, "SIGKILL") - assert.NilError(t, err) -} - func TestKillDifferentUserContainer(t *testing.T) { // TODO Windows: Windows does not yet support -u (Feb 2016). skip.If(t, testEnv.DaemonInfo.OSType == "windows", "User containers (container.Config.User) are not yet supported on %q platform", testEnv.DaemonInfo.OSType) From f0dd554e3c44225fdb03f3418cc4626d6a302929 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 21 Jan 2024 18:27:54 +0100 Subject: [PATCH 07/19] api: remove code for ContainerInspect on api < v1.20 API v1.23 and older are deprecated, so we can remove the code to adjust responses for API v1.19 and lower. Signed-off-by: Sebastiaan van Stijn --- api/types/versions/v1p19/types.go | 35 ----------------- daemon/inspect.go | 2 - daemon/inspect_linux.go | 44 ---------------------- daemon/inspect_windows.go | 7 ---- integration-cli/docker_api_inspect_test.go | 5 +-- integration/container/inspect_test.go | 36 ------------------ 6 files changed, 2 insertions(+), 127 deletions(-) delete mode 100644 api/types/versions/v1p19/types.go diff --git a/api/types/versions/v1p19/types.go b/api/types/versions/v1p19/types.go deleted file mode 100644 index 58afe32da0..0000000000 --- a/api/types/versions/v1p19/types.go +++ /dev/null @@ -1,35 +0,0 @@ -// Package v1p19 provides specific API types for the API version 1, patch 19. -package v1p19 // import "github.com/docker/docker/api/types/versions/v1p19" - -import ( - "github.com/docker/docker/api/types" - "github.com/docker/docker/api/types/container" - "github.com/docker/docker/api/types/versions/v1p20" - "github.com/docker/go-connections/nat" -) - -// ContainerJSON is a backcompatibility struct for APIs prior to 1.20. -// Note this is not used by the Windows daemon. -type ContainerJSON struct { - *types.ContainerJSONBase - Volumes map[string]string - VolumesRW map[string]bool - Config *ContainerConfig - NetworkSettings *v1p20.NetworkSettings -} - -// ContainerConfig is a backcompatibility struct for APIs prior to 1.20. -type ContainerConfig struct { - *container.Config - - MacAddress string - NetworkDisabled bool - ExposedPorts map[nat.Port]struct{} - - // backward compatibility, they now live in HostConfig - VolumeDriver string - Memory int64 - MemorySwap int64 - CPUShares int64 `json:"CpuShares"` - CPUSet string `json:"Cpuset"` -} diff --git a/daemon/inspect.go b/daemon/inspect.go index db2d0f7d65..306ac7cbbd 100644 --- a/daemon/inspect.go +++ b/daemon/inspect.go @@ -29,8 +29,6 @@ import ( // there is an error getting the data. func (daemon *Daemon) ContainerInspect(ctx context.Context, name string, size bool, version string) (interface{}, error) { switch { - case versions.LessThan(version, "1.20"): - return daemon.containerInspectPre120(ctx, name) case versions.Equal(version, "1.20"): return daemon.containerInspect120(name) case versions.LessThan(version, "1.45"): diff --git a/daemon/inspect_linux.go b/daemon/inspect_linux.go index bcfaf105ed..5f8606b38a 100644 --- a/daemon/inspect_linux.go +++ b/daemon/inspect_linux.go @@ -1,11 +1,8 @@ package daemon // import "github.com/docker/docker/daemon" import ( - "context" - "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/backend" - "github.com/docker/docker/api/types/versions/v1p19" "github.com/docker/docker/container" ) @@ -19,47 +16,6 @@ func setPlatformSpecificContainerFields(container *container.Container, contJSON return contJSONBase } -// containerInspectPre120 gets containers for pre 1.20 APIs. -func (daemon *Daemon) containerInspectPre120(ctx context.Context, name string) (*v1p19.ContainerJSON, error) { - ctr, err := daemon.GetContainer(name) - if err != nil { - return nil, err - } - - ctr.Lock() - defer ctr.Unlock() - - base, err := daemon.getInspectData(&daemon.config().Config, ctr) - if err != nil { - return nil, err - } - - volumes := make(map[string]string) - volumesRW := make(map[string]bool) - for _, m := range ctr.MountPoints { - volumes[m.Destination] = m.Path() - volumesRW[m.Destination] = m.RW - } - - return &v1p19.ContainerJSON{ - ContainerJSONBase: base, - Volumes: volumes, - VolumesRW: volumesRW, - Config: &v1p19.ContainerConfig{ - Config: ctr.Config, - 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, - Memory: ctr.HostConfig.Memory, - MemorySwap: ctr.HostConfig.MemorySwap, - CPUShares: ctr.HostConfig.CPUShares, - CPUSet: ctr.HostConfig.CpusetCpus, - }, - NetworkSettings: daemon.getBackwardsCompatibleNetworkSettings(ctr.NetworkSettings), - }, nil -} - func inspectExecProcessConfig(e *container.ExecConfig) *backend.ExecProcessConfig { return &backend.ExecProcessConfig{ Tty: e.Tty, diff --git a/daemon/inspect_windows.go b/daemon/inspect_windows.go index c43112f853..95aa772c65 100644 --- a/daemon/inspect_windows.go +++ b/daemon/inspect_windows.go @@ -1,8 +1,6 @@ package daemon // import "github.com/docker/docker/daemon" import ( - "context" - "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/backend" "github.com/docker/docker/container" @@ -13,11 +11,6 @@ func setPlatformSpecificContainerFields(container *container.Container, contJSON return contJSONBase } -// containerInspectPre120 get containers for pre 1.20 APIs. -func (daemon *Daemon) containerInspectPre120(ctx context.Context, name string) (*types.ContainerJSON, error) { - return daemon.ContainerInspectCurrent(ctx, name, false) -} - func inspectExecProcessConfig(e *container.ExecConfig) *backend.ExecProcessConfig { return &backend.ExecProcessConfig{ Tty: e.Tty, diff --git a/integration-cli/docker_api_inspect_test.go b/integration-cli/docker_api_inspect_test.go index e058e7f64a..b89986acdd 100644 --- a/integration-cli/docker_api_inspect_test.go +++ b/integration-cli/docker_api_inspect_test.go @@ -37,7 +37,6 @@ func (s *DockerAPISuite) TestInspectAPIContainerResponse(c *testing.T) { } else { cases = []acase{ {"v1.20", append(keysBase, "Mounts")}, - {"v1.19", append(keysBase, "Volumes", "VolumesRW")}, } } @@ -65,7 +64,7 @@ func (s *DockerAPISuite) TestInspectAPIContainerVolumeDriverLegacy(c *testing.T) out := cli.DockerCmd(c, "run", "-d", "busybox", "true").Stdout() cleanedContainerID := strings.TrimSpace(out) - cases := []string{"v1.19", "v1.20"} + cases := []string{"v1.20"} for _, version := range cases { body := getInspectBody(c, version, cleanedContainerID) @@ -125,7 +124,7 @@ func (s *DockerAPISuite) TestInspectAPIEmptyFieldsInConfigPre121(c *testing.T) { out := cli.DockerCmd(c, "run", "-d", "busybox", "true").Stdout() cleanedContainerID := strings.TrimSpace(out) - cases := []string{"v1.19", "v1.20"} + cases := []string{"v1.20"} for _, version := range cases { body := getInspectBody(c, version, cleanedContainerID) diff --git a/integration/container/inspect_test.go b/integration/container/inspect_test.go index 39f7eeab78..3f8e10628f 100644 --- a/integration/container/inspect_test.go +++ b/integration/container/inspect_test.go @@ -1,53 +1,17 @@ package container // import "github.com/docker/docker/integration/container" import ( - "encoding/json" "runtime" "strings" "testing" - "time" containertypes "github.com/docker/docker/api/types/container" - "github.com/docker/docker/client" "github.com/docker/docker/integration/internal/container" "github.com/docker/docker/testutil/request" "gotest.tools/v3/assert" is "gotest.tools/v3/assert/cmp" - "gotest.tools/v3/poll" - "gotest.tools/v3/skip" ) -func TestInspectCpusetInConfigPre120(t *testing.T) { - skip.If(t, testEnv.DaemonInfo.OSType == "windows" || !testEnv.DaemonInfo.CPUSet) - - ctx := setupTest(t) - apiClient := request.NewAPIClient(t, client.WithVersion("1.19")) - - name := strings.ToLower(t.Name()) - // Create container with up to-date-API - container.Run(ctx, t, request.NewAPIClient(t), container.WithName(name), - container.WithCmd("true"), - func(c *container.TestContainerConfig) { - c.HostConfig.Resources.CpusetCpus = "0" - }, - ) - poll.WaitOn(t, container.IsInState(ctx, apiClient, name, "exited"), poll.WithDelay(100*time.Millisecond)) - - _, body, err := apiClient.ContainerInspectWithRaw(ctx, name, false) - assert.NilError(t, err) - - var inspectJSON map[string]interface{} - err = json.Unmarshal(body, &inspectJSON) - assert.NilError(t, err, "unable to unmarshal body for version 1.19: %s", err) - - config, ok := inspectJSON["Config"] - assert.Check(t, is.Equal(true, ok), "Unable to find 'Config'") - - cfg := config.(map[string]interface{}) - _, ok = cfg["Cpuset"] - assert.Check(t, is.Equal(true, ok), "API version 1.19 expected to include Cpuset in 'Config'") -} - func TestInspectAnnotations(t *testing.T) { ctx := setupTest(t) apiClient := request.NewAPIClient(t) From 570d5a964537bf1e90e784353b4e7be775a88c2f Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 21 Jan 2024 18:42:07 +0100 Subject: [PATCH 08/19] api: remove code for ContainerInspect on api v1.20 API v1.23 and older are deprecated, so we can remove the code to adjust responses for API v1.20. Signed-off-by: Sebastiaan van Stijn --- daemon/inspect.go | 51 -------------- integration-cli/docker_api_inspect_test.go | 67 +------------------ .../docker_cli_network_unix_test.go | 13 +--- 3 files changed, 4 insertions(+), 127 deletions(-) diff --git a/daemon/inspect.go b/daemon/inspect.go index 306ac7cbbd..62c7896ac3 100644 --- a/daemon/inspect.go +++ b/daemon/inspect.go @@ -14,7 +14,6 @@ import ( containertypes "github.com/docker/docker/api/types/container" networktypes "github.com/docker/docker/api/types/network" "github.com/docker/docker/api/types/versions" - "github.com/docker/docker/api/types/versions/v1p20" "github.com/docker/docker/container" "github.com/docker/docker/daemon/config" "github.com/docker/docker/daemon/network" @@ -29,8 +28,6 @@ import ( // there is an error getting the data. func (daemon *Daemon) ContainerInspect(ctx context.Context, name string, size bool, version string) (interface{}, error) { switch { - case versions.Equal(version, "1.20"): - return daemon.containerInspect120(name) case versions.LessThan(version, "1.45"): ctr, err := daemon.ContainerInspectCurrent(ctx, name, size) if err != nil { @@ -115,35 +112,6 @@ func (daemon *Daemon) ContainerInspectCurrent(ctx context.Context, name string, }, nil } -// containerInspect120 serializes the master version of a container into a json type. -func (daemon *Daemon) containerInspect120(name string) (*v1p20.ContainerJSON, error) { - ctr, err := daemon.GetContainer(name) - if err != nil { - return nil, err - } - - ctr.Lock() - defer ctr.Unlock() - - base, err := daemon.getInspectData(&daemon.config().Config, ctr) - if err != nil { - return nil, err - } - - return &v1p20.ContainerJSON{ - ContainerJSONBase: base, - Mounts: ctr.GetMountPoints(), - Config: &v1p20.ContainerConfig{ - Config: ctr.Config, - 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, - }, - NetworkSettings: daemon.getBackwardsCompatibleNetworkSettings(ctr.NetworkSettings), - }, nil -} - func (daemon *Daemon) getInspectData(daemonCfg *config.Config, container *container.Container) (*types.ContainerJSONBase, error) { // make a copy to play with hostConfig := *container.HostConfig @@ -281,25 +249,6 @@ func (daemon *Daemon) ContainerExecInspect(id string) (*backend.ExecInspect, err }, nil } -func (daemon *Daemon) getBackwardsCompatibleNetworkSettings(settings *network.Settings) *v1p20.NetworkSettings { - result := &v1p20.NetworkSettings{ - NetworkSettingsBase: types.NetworkSettingsBase{ - Bridge: settings.Bridge, - SandboxID: settings.SandboxID, - SandboxKey: settings.SandboxKey, - HairpinMode: settings.HairpinMode, - LinkLocalIPv6Address: settings.LinkLocalIPv6Address, - LinkLocalIPv6PrefixLen: settings.LinkLocalIPv6PrefixLen, - Ports: settings.Ports, - SecondaryIPAddresses: settings.SecondaryIPAddresses, - SecondaryIPv6Addresses: settings.SecondaryIPv6Addresses, - }, - DefaultNetworkSettings: daemon.getDefaultNetworkSettings(settings.Networks), - } - - return result -} - // getDefaultNetworkSettings creates the deprecated structure that holds the information // about the bridge network for a container. func (daemon *Daemon) getDefaultNetworkSettings(networks map[string]*network.EndpointSettings) types.DefaultNetworkSettings { diff --git a/integration-cli/docker_api_inspect_test.go b/integration-cli/docker_api_inspect_test.go index b89986acdd..dde0c244e2 100644 --- a/integration-cli/docker_api_inspect_test.go +++ b/integration-cli/docker_api_inspect_test.go @@ -6,7 +6,6 @@ import ( "testing" "github.com/docker/docker/api/types" - "github.com/docker/docker/api/types/versions/v1p20" "github.com/docker/docker/client" "github.com/docker/docker/integration-cli/cli" "github.com/docker/docker/testutil" @@ -31,12 +30,13 @@ func (s *DockerAPISuite) TestInspectAPIContainerResponse(c *testing.T) { var cases []acase if testEnv.DaemonInfo.OSType == "windows" { + // Windows only supports 1.25 or later cases = []acase{ {"v1.25", append(keysBase, "Mounts")}, } } else { cases = []acase{ - {"v1.20", append(keysBase, "Mounts")}, + {"v1.24", append(keysBase, "Mounts")}, } } @@ -58,28 +58,6 @@ func (s *DockerAPISuite) TestInspectAPIContainerResponse(c *testing.T) { } } -func (s *DockerAPISuite) TestInspectAPIContainerVolumeDriverLegacy(c *testing.T) { - // No legacy implications for Windows - testRequires(c, DaemonIsLinux) - out := cli.DockerCmd(c, "run", "-d", "busybox", "true").Stdout() - cleanedContainerID := strings.TrimSpace(out) - - cases := []string{"v1.20"} - for _, version := range cases { - body := getInspectBody(c, version, cleanedContainerID) - - var inspectJSON map[string]interface{} - err := json.Unmarshal(body, &inspectJSON) - assert.NilError(c, err, "Unable to unmarshal body for version %s", version) - - config, ok := inspectJSON["Config"] - assert.Assert(c, ok, "Unable to find 'Config'") - cfg := config.(map[string]interface{}) - _, ok = cfg["VolumeDriver"] - assert.Assert(c, ok, "API version %s expected to include VolumeDriver in 'Config'", version) - } -} - func (s *DockerAPISuite) TestInspectAPIContainerVolumeDriver(c *testing.T) { out := cli.DockerCmd(c, "run", "-d", "--volume-driver", "local", "busybox", "true").Stdout() cleanedContainerID := strings.TrimSpace(out) @@ -117,47 +95,6 @@ func (s *DockerAPISuite) TestInspectAPIImageResponse(c *testing.T) { assert.Check(c, is.Contains(imageJSON.RepoTags, "busybox:mytag")) } -// #17131, #17139, #17173 -func (s *DockerAPISuite) TestInspectAPIEmptyFieldsInConfigPre121(c *testing.T) { - // Not relevant on Windows - testRequires(c, DaemonIsLinux) - out := cli.DockerCmd(c, "run", "-d", "busybox", "true").Stdout() - cleanedContainerID := strings.TrimSpace(out) - - cases := []string{"v1.20"} - for _, version := range cases { - body := getInspectBody(c, version, cleanedContainerID) - - var inspectJSON map[string]interface{} - err := json.Unmarshal(body, &inspectJSON) - assert.NilError(c, err, "Unable to unmarshal body for version %s", version) - config, ok := inspectJSON["Config"] - assert.Assert(c, ok, "Unable to find 'Config'") - cfg := config.(map[string]interface{}) - for _, f := range []string{"MacAddress", "NetworkDisabled", "ExposedPorts"} { - _, ok := cfg[f] - assert.Check(c, ok, "API version %s expected to include %s in 'Config'", version, f) - } - } -} - -func (s *DockerAPISuite) TestInspectAPIBridgeNetworkSettings120(c *testing.T) { - // Not relevant on Windows, and besides it doesn't have any bridge network settings - testRequires(c, DaemonIsLinux) - out := cli.DockerCmd(c, "run", "-d", "busybox", "top").Stdout() - containerID := strings.TrimSpace(out) - cli.WaitRun(c, containerID) - - body := getInspectBody(c, "v1.20", containerID) - - var inspectJSON v1p20.ContainerJSON - err := json.Unmarshal(body, &inspectJSON) - assert.NilError(c, err) - - settings := inspectJSON.NetworkSettings - assert.Assert(c, len(settings.IPAddress) != 0) -} - // Inspect for API v1.21 and up; see // // - https://github.com/moby/moby/issues/17131 diff --git a/integration-cli/docker_cli_network_unix_test.go b/integration-cli/docker_cli_network_unix_test.go index e4d3d3fe0a..24a241ec3a 100644 --- a/integration-cli/docker_cli_network_unix_test.go +++ b/integration-cli/docker_cli_network_unix_test.go @@ -15,7 +15,6 @@ import ( "time" "github.com/docker/docker/api/types" - "github.com/docker/docker/api/types/versions/v1p20" "github.com/docker/docker/integration-cli/cli" "github.com/docker/docker/integration-cli/daemon" "github.com/docker/docker/libnetwork/driverapi" @@ -1015,22 +1014,14 @@ func (s *DockerCLINetworkSuite) TestInspectAPIMultipleNetworks(c *testing.T) { cli.DockerCmd(c, "network", "connect", "mybridge1", id) cli.DockerCmd(c, "network", "connect", "mybridge2", id) - body := getInspectBody(c, "v1.20", id) - var inspect120 v1p20.ContainerJSON - err := json.Unmarshal(body, &inspect120) - assert.NilError(c, err) - - versionedIP := inspect120.NetworkSettings.IPAddress - // Current API version (API v1.21 and up) - body = getInspectBody(c, "", id) + body := getInspectBody(c, "", id) var inspectCurrent types.ContainerJSON - err = json.Unmarshal(body, &inspectCurrent) + err := json.Unmarshal(body, &inspectCurrent) assert.NilError(c, err) assert.Equal(c, len(inspectCurrent.NetworkSettings.Networks), 3) bridge := inspectCurrent.NetworkSettings.Networks["bridge"] - assert.Equal(c, bridge.IPAddress, versionedIP) assert.Equal(c, bridge.IPAddress, inspectCurrent.NetworkSettings.IPAddress) } From ed93110e1173f2e3d7708f708e1299ceb0c3f7da Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 22 Jan 2024 01:03:07 +0100 Subject: [PATCH 09/19] api: update test to reflect reality on Windows The TestInspectAPIContainerResponse mentioned that Windows does not support API versions before v1.25. While technically, no stable release existed for Windows with API versions before that (see f811d5b1288583b4bb4b978e58ca0466236a9a30), API version v1.24 was enabled in e4af39aeb311a08fd2d15e01eabd6bae9db36382, to have a consistend fallback version for API version negotiation. This patch updates the test to reflect that change. Signed-off-by: Sebastiaan van Stijn --- integration-cli/docker_api_inspect_test.go | 19 ++++--------------- integration/system/info_linux_test.go | 1 - 2 files changed, 4 insertions(+), 16 deletions(-) diff --git a/integration-cli/docker_api_inspect_test.go b/integration-cli/docker_api_inspect_test.go index dde0c244e2..5a3c160f7d 100644 --- a/integration-cli/docker_api_inspect_test.go +++ b/integration-cli/docker_api_inspect_test.go @@ -20,26 +20,15 @@ func (s *DockerAPISuite) TestInspectAPIContainerResponse(c *testing.T) { keysBase := []string{ "Id", "State", "Created", "Path", "Args", "Config", "Image", "NetworkSettings", "ResolvConfPath", "HostnamePath", "HostsPath", "LogPath", "Name", "Driver", "MountLabel", "ProcessLabel", "GraphDriver", + "Mounts", } - type acase struct { + cases := []struct { version string keys []string + }{ + {version: "v1.24", keys: keysBase}, } - - var cases []acase - - if testEnv.DaemonInfo.OSType == "windows" { - // Windows only supports 1.25 or later - cases = []acase{ - {"v1.25", append(keysBase, "Mounts")}, - } - } else { - cases = []acase{ - {"v1.24", append(keysBase, "Mounts")}, - } - } - for _, cs := range cases { body := getInspectBody(c, cs.version, cleanedContainerID) diff --git a/integration/system/info_linux_test.go b/integration/system/info_linux_test.go index 98b3367990..a6d8360914 100644 --- a/integration/system/info_linux_test.go +++ b/integration/system/info_linux_test.go @@ -31,7 +31,6 @@ func TestInfoBinaryCommits(t *testing.T) { func TestInfoAPIVersioned(t *testing.T) { ctx := testutil.StartSpan(baseContext, t) - // Windows only supports 1.25 or later res, body, err := req.Get(ctx, "/v1.24/info") assert.NilError(t, err) From d1974aa49236de73945a8d50d28c21af4e22e62c Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 21 Jan 2024 18:57:00 +0100 Subject: [PATCH 10/19] api: remove code for container stats on api < v1.21 API v1.23 and older are deprecated, so we can remove the code to adjust responses for API v1.20 and lower. Signed-off-by: Sebastiaan van Stijn --- .../router/container/container_routes.go | 7 +-- api/types/backend/backend.go | 1 - api/types/versions/README.md | 14 ------ api/types/versions/v1p20/types.go | 40 --------------- daemon/stats.go | 50 +------------------ integration-cli/docker_api_stats_test.go | 44 +++------------- 6 files changed, 9 insertions(+), 147 deletions(-) delete mode 100644 api/types/versions/README.md delete mode 100644 api/types/versions/v1p20/types.go diff --git a/api/server/router/container/container_routes.go b/api/server/router/container/container_routes.go index 2b35c00b54..80ce7e3cb4 100644 --- a/api/server/router/container/container_routes.go +++ b/api/server/router/container/container_routes.go @@ -111,14 +111,11 @@ func (s *containerRouter) getContainersStats(ctx context.Context, w http.Respons oneShot = httputils.BoolValueOrDefault(r, "one-shot", false) } - config := &backend.ContainerStatsConfig{ + return s.backend.ContainerStats(ctx, vars["name"], &backend.ContainerStatsConfig{ Stream: stream, OneShot: oneShot, OutStream: w, - Version: httputils.VersionFromContext(ctx), - } - - return s.backend.ContainerStats(ctx, vars["name"], config) + }) } func (s *containerRouter) getContainersLogs(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error { diff --git a/api/types/backend/backend.go b/api/types/backend/backend.go index c9fc978884..cb965c25be 100644 --- a/api/types/backend/backend.go +++ b/api/types/backend/backend.go @@ -89,7 +89,6 @@ type ContainerStatsConfig struct { Stream bool OneShot bool OutStream io.Writer - Version string } // ExecInspect holds information about a running process started diff --git a/api/types/versions/README.md b/api/types/versions/README.md deleted file mode 100644 index 1ef911edb0..0000000000 --- a/api/types/versions/README.md +++ /dev/null @@ -1,14 +0,0 @@ -# Legacy API type versions - -This package includes types for legacy API versions. The stable version of the API types live in `api/types/*.go`. - -Consider moving a type here when you need to keep backwards compatibility in the API. This legacy types are organized by the latest API version they appear in. For instance, types in the `v1p19` package are valid for API versions below or equal `1.19`. Types in the `v1p20` package are valid for the API version `1.20`, since the versions below that will use the legacy types in `v1p19`. - -## Package name conventions - -The package name convention is to use `v` as a prefix for the version number and `p`(patch) as a separator. We use this nomenclature due to a few restrictions in the Go package name convention: - -1. We cannot use `.` because it's interpreted by the language, think of `v1.20.CallFunction`. -2. We cannot use `_` because golint complains about it. The code is actually valid, but it looks probably more weird: `v1_20.CallFunction`. - -For instance, if you want to modify a type that was available in the version `1.21` of the API but it will have different fields in the version `1.22`, you want to create a new package under `api/types/versions/v1p21`. diff --git a/api/types/versions/v1p20/types.go b/api/types/versions/v1p20/types.go deleted file mode 100644 index cc7277b1b4..0000000000 --- a/api/types/versions/v1p20/types.go +++ /dev/null @@ -1,40 +0,0 @@ -// Package v1p20 provides specific API types for the API version 1, patch 20. -package v1p20 // import "github.com/docker/docker/api/types/versions/v1p20" - -import ( - "github.com/docker/docker/api/types" - "github.com/docker/docker/api/types/container" - "github.com/docker/go-connections/nat" -) - -// ContainerJSON is a backcompatibility struct for the API 1.20 -type ContainerJSON struct { - *types.ContainerJSONBase - Mounts []types.MountPoint - Config *ContainerConfig - NetworkSettings *NetworkSettings -} - -// ContainerConfig is a backcompatibility struct used in ContainerJSON for the API 1.20 -type ContainerConfig struct { - *container.Config - - MacAddress string - NetworkDisabled bool - ExposedPorts map[nat.Port]struct{} - - // backward compatibility, they now live in HostConfig - VolumeDriver string -} - -// StatsJSON is a backcompatibility struct used in Stats for APIs prior to 1.21 -type StatsJSON struct { - types.Stats - Network types.NetworkStats `json:"network,omitempty"` -} - -// NetworkSettings is a backward compatible struct for APIs prior to 1.21 -type NetworkSettings struct { - types.NetworkSettingsBase - types.DefaultNetworkSettings -} diff --git a/daemon/stats.go b/daemon/stats.go index 342d7a9c66..5dcd6121d4 100644 --- a/daemon/stats.go +++ b/daemon/stats.go @@ -10,8 +10,6 @@ import ( "github.com/containerd/log" "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/backend" - "github.com/docker/docker/api/types/versions" - "github.com/docker/docker/api/types/versions/v1p20" "github.com/docker/docker/container" "github.com/docker/docker/errdefs" "github.com/docker/docker/pkg/ioutils" @@ -20,13 +18,6 @@ import ( // ContainerStats writes information about the container to the stream // given in the config object. func (daemon *Daemon) ContainerStats(ctx context.Context, prefixOrName string, config *backend.ContainerStatsConfig) error { - // Engine API version (used for backwards compatibility) - apiVersion := config.Version - - if isWindows && versions.LessThan(apiVersion, "1.21") { - return errors.New("API versions pre v1.21 do not support stats on Windows") - } - ctr, err := daemon.GetContainer(prefixOrName) if err != nil { return err @@ -87,46 +78,7 @@ func (daemon *Daemon) ContainerStats(ctx context.Context, prefixOrName string, c return nil } - var statsJSON interface{} - statsJSONPost120 := getStatJSON(v) - if versions.LessThan(apiVersion, "1.21") { - var ( - rxBytes uint64 - rxPackets uint64 - rxErrors uint64 - rxDropped uint64 - txBytes uint64 - txPackets uint64 - txErrors uint64 - txDropped uint64 - ) - for _, v := range statsJSONPost120.Networks { - rxBytes += v.RxBytes - rxPackets += v.RxPackets - rxErrors += v.RxErrors - rxDropped += v.RxDropped - txBytes += v.TxBytes - txPackets += v.TxPackets - txErrors += v.TxErrors - txDropped += v.TxDropped - } - statsJSON = &v1p20.StatsJSON{ - Stats: statsJSONPost120.Stats, - Network: types.NetworkStats{ - RxBytes: rxBytes, - RxPackets: rxPackets, - RxErrors: rxErrors, - RxDropped: rxDropped, - TxBytes: txBytes, - TxPackets: txPackets, - TxErrors: txErrors, - TxDropped: txDropped, - }, - } - } else { - statsJSON = statsJSONPost120 - } - + statsJSON := getStatJSON(v) if !config.Stream && noStreamFirstFrame { // prime the cpu stats so they aren't 0 in the final output noStreamFirstFrame = false diff --git a/integration-cli/docker_api_stats_test.go b/integration-cli/docker_api_stats_test.go index 4d31aee079..6b50bcf8f0 100644 --- a/integration-cli/docker_api_stats_test.go +++ b/integration-cli/docker_api_stats_test.go @@ -8,13 +8,11 @@ import ( "runtime" "strconv" "strings" - "sync" "testing" "time" "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/system" - "github.com/docker/docker/api/types/versions" "github.com/docker/docker/client" "github.com/docker/docker/integration-cli/cli" "github.com/docker/docker/testutil" @@ -166,27 +164,13 @@ func (s *DockerAPISuite) TestAPIStatsNetworkStats(c *testing.T) { } func (s *DockerAPISuite) TestAPIStatsNetworkStatsVersioning(c *testing.T) { - // Windows doesn't support API versions less than 1.25, so no point testing 1.17 .. 1.21 testRequires(c, testEnv.IsLocalDaemon, DaemonIsLinux) id := runSleepingContainer(c) cli.WaitRun(c, id) - wg := sync.WaitGroup{} - for i := 17; i <= 21; i++ { - wg.Add(1) - go func(i int) { - defer wg.Done() - apiVersion := fmt.Sprintf("v1.%d", i) - statsJSONBlob := getVersionedStats(c, id, apiVersion) - if versions.LessThan(apiVersion, "v1.21") { - assert.Assert(c, jsonBlobHasLTv121NetworkStats(statsJSONBlob), "Stats JSON blob from API %s %#v does not look like a =v1.21 API stats structure", apiVersion, statsJSONBlob) - } - }(i) - } - wg.Wait() + statsJSONBlob := getStats(c, id) + assert.Assert(c, jsonBlobHasGTE121NetworkStats(statsJSONBlob), "Stats JSON blob from API does not look like a >=v1.21 API stats structure", statsJSONBlob) } func getNetworkStats(c *testing.T, id string) map[string]types.NetworkStats { @@ -202,14 +186,15 @@ func getNetworkStats(c *testing.T, id string) map[string]types.NetworkStats { return st.Networks } -// getVersionedStats returns stats result for the +// getStats returns stats result for the // container with id using an API call with version apiVersion. Since the // stats result type differs between API versions, we simply return // map[string]interface{}. -func getVersionedStats(c *testing.T, id string, apiVersion string) map[string]interface{} { +func getStats(c *testing.T, id string) map[string]interface{} { + c.Helper() stats := make(map[string]interface{}) - _, body, err := request.Get(testutil.GetContext(c), "/"+apiVersion+"/containers/"+id+"/stats?stream=false") + _, body, err := request.Get(testutil.GetContext(c), "/containers/"+id+"/stats?stream=false") assert.NilError(c, err) defer body.Close() @@ -219,23 +204,6 @@ func getVersionedStats(c *testing.T, id string, apiVersion string) map[string]in return stats } -func jsonBlobHasLTv121NetworkStats(blob map[string]interface{}) bool { - networkStatsIntfc, ok := blob["network"] - if !ok { - return false - } - networkStats, ok := networkStatsIntfc.(map[string]interface{}) - if !ok { - return false - } - for _, expectedKey := range expectedNetworkInterfaceStats { - if _, ok := networkStats[expectedKey]; !ok { - return false - } - } - return true -} - func jsonBlobHasGTE121NetworkStats(blob map[string]interface{}) bool { networksStatsIntfc, ok := blob["networks"] if !ok { From 83f790ccccb8214a22b6efd5a461eafcc6d533ef Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 21 Jan 2024 20:04:21 +0100 Subject: [PATCH 11/19] api: POST /exec/{id}/start: remove support for API < v1.21 API v1.21 (Docker Engine v1.9.0) enforces the request to have a JSON content-type on exec start (see 45dc57f22931e6cb8ad522602da8aa985bddcfab). An exception was added in 0b5e628e14a673cd1940876b2f3d091c2fe236d9 to make this check conditional (supporting API < 1.21). API v1.23 and older are deprecated, and this patch removes the feature. Signed-off-by: Sebastiaan van Stijn --- api/server/router/container/exec.go | 11 ++--------- integration-cli/docker_api_exec_test.go | 14 -------------- 2 files changed, 2 insertions(+), 23 deletions(-) diff --git a/api/server/router/container/exec.go b/api/server/router/container/exec.go index 619355a50a..b62342af85 100644 --- a/api/server/router/container/exec.go +++ b/api/server/router/container/exec.go @@ -71,15 +71,6 @@ func (s *containerRouter) postContainerExecStart(ctx context.Context, w http.Res return err } - version := httputils.VersionFromContext(ctx) - if versions.LessThan(version, "1.22") { - // API versions before 1.22 did not enforce application/json content-type. - // Allow older clients to work by patching the content-type. - if r.Header.Get("Content-Type") != "application/json" { - r.Header.Set("Content-Type", "application/json") - } - } - var ( execName = vars["name"] stdin, inStream io.ReadCloser @@ -96,6 +87,8 @@ func (s *containerRouter) postContainerExecStart(ctx context.Context, w http.Res } if execStartCheck.ConsoleSize != nil { + version := httputils.VersionFromContext(ctx) + // Not supported before 1.42 if versions.LessThan(version, "1.42") { execStartCheck.ConsoleSize = nil diff --git a/integration-cli/docker_api_exec_test.go b/integration-cli/docker_api_exec_test.go index f9612fc667..612ef292b1 100644 --- a/integration-cli/docker_api_exec_test.go +++ b/integration-cli/docker_api_exec_test.go @@ -109,20 +109,6 @@ func (s *DockerAPISuite) TestExecAPIStartEnsureHeaders(c *testing.T) { assert.Assert(c, resp.Header.Get("Server") != "") } -func (s *DockerAPISuite) TestExecAPIStartBackwardsCompatible(c *testing.T) { - testRequires(c, DaemonIsLinux) // Windows only supports 1.25 or later - runSleepingContainer(c, "-d", "--name", "test") - id := createExec(c, "test") - - resp, body, err := request.Post(testutil.GetContext(c), fmt.Sprintf("/v1.20/exec/%s/start", id), request.RawString(`{"Detach": true}`), request.ContentType("text/plain")) - assert.NilError(c, err) - - b, err := request.ReadBody(body) - comment := fmt.Sprintf("response body: %s", b) - assert.NilError(c, err, comment) - assert.Equal(c, resp.StatusCode, http.StatusOK, comment) -} - // #19362 func (s *DockerAPISuite) TestExecAPIStartMultipleTimesError(c *testing.T) { runSleepingContainer(c, "-d", "--name", "test") From b3a0ff994450e60890ac8a81e80bbd49004ac8b6 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 21 Jan 2024 15:02:49 +0100 Subject: [PATCH 12/19] api: remove POST /containers/{id}/copy endpoint (api < v1.23) This endpoint was deprecated in API v1.20 (Docker Engine v1.8.0) in commit db9cc91a9ef7dea4c8d85f64578889cec3dd99b2, in favor of the `PUT /containers/{id}/archive` and `HEAD /containers/{id}/archive` endpoints, and disabled in API v1.24 (Docker Engine v1.12.0) through commit 428328908dc529b1678fb3d8b033fb0591a294e3. This patch removes the endpoint, and the associated `daemon.ContainerCopy` method in the backend. Signed-off-by: Sebastiaan van Stijn --- api/server/router/container/backend.go | 1 - api/server/router/container/container.go | 1 - api/server/router/container/copy.go | 41 +--------- daemon/archive.go | 19 ----- daemon/archive_unix.go | 49 ------------ integration-cli/docker_api_containers_test.go | 75 ------------------- integration/container/container_test.go | 1 - 7 files changed, 1 insertion(+), 186 deletions(-) diff --git a/api/server/router/container/backend.go b/api/server/router/container/backend.go index fa1c0dcc54..f35ab6a023 100644 --- a/api/server/router/container/backend.go +++ b/api/server/router/container/backend.go @@ -24,7 +24,6 @@ type execBackend interface { // copyBackend includes functions to implement to provide container copy functionality. type copyBackend interface { ContainerArchivePath(name string, path string) (content io.ReadCloser, stat *types.ContainerPathStat, err error) - ContainerCopy(name string, res string) (io.ReadCloser, error) ContainerExport(ctx context.Context, name string, out io.Writer) error ContainerExtractToDir(name, path string, copyUIDGID, noOverwriteDirNonDir bool, content io.Reader) error ContainerStatPath(name string, path string) (stat *types.ContainerPathStat, err error) diff --git a/api/server/router/container/container.go b/api/server/router/container/container.go index 7ac9dc7aac..ebe5f082c1 100644 --- a/api/server/router/container/container.go +++ b/api/server/router/container/container.go @@ -56,7 +56,6 @@ func (r *containerRouter) initRoutes() { router.NewPostRoute("/containers/{name:.*}/wait", r.postContainersWait), router.NewPostRoute("/containers/{name:.*}/resize", r.postContainersResize), router.NewPostRoute("/containers/{name:.*}/attach", r.postContainersAttach), - router.NewPostRoute("/containers/{name:.*}/copy", r.postContainersCopy), // Deprecated since 1.8 (API v1.20), errors out since 1.12 (API v1.24) router.NewPostRoute("/containers/{name:.*}/exec", r.postContainerExecCreate), router.NewPostRoute("/exec/{name:.*}/start", r.postContainerExecStart), router.NewPostRoute("/exec/{name:.*}/resize", r.postContainerExecResize), diff --git a/api/server/router/container/copy.go b/api/server/router/container/copy.go index 28920b6d7b..7596818e56 100644 --- a/api/server/router/container/copy.go +++ b/api/server/router/container/copy.go @@ -11,49 +11,10 @@ import ( "github.com/docker/docker/api/server/httputils" "github.com/docker/docker/api/types" - "github.com/docker/docker/api/types/versions" gddohttputil "github.com/golang/gddo/httputil" ) -type pathError struct{} - -func (pathError) Error() string { - return "Path cannot be empty" -} - -func (pathError) InvalidParameter() {} - -// postContainersCopy is deprecated in favor of getContainersArchive. -// -// Deprecated since 1.8 (API v1.20), errors out since 1.12 (API v1.24) -func (s *containerRouter) postContainersCopy(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error { - version := httputils.VersionFromContext(ctx) - if versions.GreaterThanOrEqualTo(version, "1.24") { - w.WriteHeader(http.StatusNotFound) - return nil - } - - cfg := types.CopyConfig{} - if err := httputils.ReadJSON(r, &cfg); err != nil { - return err - } - - if cfg.Resource == "" { - return pathError{} - } - - data, err := s.backend.ContainerCopy(vars["name"], cfg.Resource) - if err != nil { - return err - } - defer data.Close() - - w.Header().Set("Content-Type", "application/x-tar") - _, err = io.Copy(w, data) - return err -} - -// // Encode the stat to JSON, base64 encode, and place in a header. +// setContainerPathStatHeader encodes the stat to JSON, base64 encode, and place in a header. func setContainerPathStatHeader(stat *types.ContainerPathStat, header http.Header) error { statJSON, err := json.Marshal(stat) if err != nil { diff --git a/daemon/archive.go b/daemon/archive.go index 4be02b25c6..65a67ffcc2 100644 --- a/daemon/archive.go +++ b/daemon/archive.go @@ -8,25 +8,6 @@ import ( "github.com/docker/docker/errdefs" ) -// ContainerCopy performs a deprecated operation of archiving the resource at -// the specified path in the container identified by the given name. -func (daemon *Daemon) ContainerCopy(name string, res string) (io.ReadCloser, error) { - ctr, err := daemon.GetContainer(name) - if err != nil { - return nil, err - } - - data, err := daemon.containerCopy(ctr, res) - if err == nil { - return data, nil - } - - if os.IsNotExist(err) { - return nil, containerFileNotFound{res, name} - } - return nil, errdefs.System(err) -} - // ContainerStatPath stats the filesystem resource at the specified path in the // container identified by the given name. func (daemon *Daemon) ContainerStatPath(name string, path string) (stat *types.ContainerPathStat, err error) { diff --git a/daemon/archive_unix.go b/daemon/archive_unix.go index 837e671c2c..cbbf9c5f8a 100644 --- a/daemon/archive_unix.go +++ b/daemon/archive_unix.go @@ -161,55 +161,6 @@ func (daemon *Daemon) containerExtractToDir(container *container.Container, path return nil } -func (daemon *Daemon) containerCopy(container *container.Container, resource string) (rc io.ReadCloser, err error) { - container.Lock() - - defer func() { - if err != nil { - // Wait to unlock the container until the archive is fully read - // (see the ReadCloseWrapper func below) or if there is an error - // before that occurs. - container.Unlock() - } - }() - - cfs, err := daemon.openContainerFS(container) - if err != nil { - return nil, err - } - defer func() { - if err != nil { - cfs.Close() - } - }() - - err = cfs.RunInFS(context.TODO(), func() error { - _, err := os.Stat(resource) - return err - }) - if err != nil { - return nil, err - } - - tb, err := archive.NewTarballer(resource, &archive.TarOptions{ - Compression: archive.Uncompressed, - }) - if err != nil { - return nil, err - } - - cfs.GoInFS(context.TODO(), tb.Do) - archv := tb.Reader() - reader := ioutils.NewReadCloserWrapper(archv, func() error { - err := archv.Close() - _ = cfs.Close() - container.Unlock() - return err - }) - daemon.LogContainerEvent(container, events.ActionCopy) - return reader, nil -} - // checkIfPathIsInAVolume checks if the path is in a volume. If it is, it // cannot be in a read-only volume. If it is not in a volume, the container // cannot be configured with a read-only rootfs. diff --git a/integration-cli/docker_api_containers_test.go b/integration-cli/docker_api_containers_test.go index fdab67e0fc..52fb611d1f 100644 --- a/integration-cli/docker_api_containers_test.go +++ b/integration-cli/docker_api_containers_test.go @@ -958,81 +958,6 @@ func (s *DockerAPISuite) TestContainerAPICopyNotExistsAnyMore(c *testing.T) { assert.Equal(c, res.StatusCode, http.StatusNotFound) } -func (s *DockerAPISuite) TestContainerAPICopyPre124(c *testing.T) { - testRequires(c, DaemonIsLinux) // Windows only supports 1.25 or later - const name = "test-container-api-copy" - cli.DockerCmd(c, "run", "--name", name, "busybox", "touch", "/test.txt") - - postData := types.CopyConfig{ - Resource: "/test.txt", - } - - res, body, err := request.Post(testutil.GetContext(c), "/v1.23/containers/"+name+"/copy", request.JSONBody(postData)) - assert.NilError(c, err) - assert.Equal(c, res.StatusCode, http.StatusOK) - - found := false - for tarReader := tar.NewReader(body); ; { - h, err := tarReader.Next() - if err != nil { - if err == io.EOF { - break - } - c.Fatal(err) - } - if h.Name == "test.txt" { - found = true - break - } - } - assert.Assert(c, found) -} - -func (s *DockerAPISuite) TestContainerAPICopyResourcePathEmptyPre124(c *testing.T) { - testRequires(c, DaemonIsLinux) // Windows only supports 1.25 or later - const name = "test-container-api-copy-resource-empty" - cli.DockerCmd(c, "run", "--name", name, "busybox", "touch", "/test.txt") - - postData := types.CopyConfig{ - Resource: "", - } - - res, body, err := request.Post(testutil.GetContext(c), "/v1.23/containers/"+name+"/copy", request.JSONBody(postData)) - assert.NilError(c, err) - assert.Equal(c, res.StatusCode, http.StatusBadRequest) - b, err := request.ReadBody(body) - assert.NilError(c, err) - assert.Assert(c, is.Regexp("^Path cannot be empty\n$", string(b))) -} - -func (s *DockerAPISuite) TestContainerAPICopyResourcePathNotFoundPre124(c *testing.T) { - testRequires(c, DaemonIsLinux) // Windows only supports 1.25 or later - const name = "test-container-api-copy-resource-not-found" - cli.DockerCmd(c, "run", "--name", name, "busybox") - - postData := types.CopyConfig{ - Resource: "/notexist", - } - - res, body, err := request.Post(testutil.GetContext(c), "/v1.23/containers/"+name+"/copy", request.JSONBody(postData)) - assert.NilError(c, err) - assert.Equal(c, res.StatusCode, http.StatusNotFound) - b, err := request.ReadBody(body) - assert.NilError(c, err) - assert.Assert(c, is.Regexp("^Could not find the file /notexist in container "+name+"\n$", string(b))) -} - -func (s *DockerAPISuite) TestContainerAPICopyContainerNotFoundPr124(c *testing.T) { - testRequires(c, DaemonIsLinux) // Windows only supports 1.25 or later - postData := types.CopyConfig{ - Resource: "/something", - } - - res, _, err := request.Post(testutil.GetContext(c), "/v1.23/containers/notexists/copy", request.JSONBody(postData)) - assert.NilError(c, err) - assert.Equal(c, res.StatusCode, http.StatusNotFound) -} - func (s *DockerAPISuite) TestContainerAPIDelete(c *testing.T) { id := runSleepingContainer(c) cli.WaitRun(c, id) diff --git a/integration/container/container_test.go b/integration/container/container_test.go index aa62cb89e7..65394ec75b 100644 --- a/integration/container/container_test.go +++ b/integration/container/container_test.go @@ -29,7 +29,6 @@ func TestContainerInvalidJSON(t *testing.T) { if runtime.GOOS != "windows" { endpoints = append( endpoints, - "/v1.23/containers/foobar/copy", // deprecated since 1.8 (API v1.20), errors out since 1.12 (API v1.24) "/v1.23/containers/foobar/start", // accepts a body on API < v1.24 ) } From ffd877f94866975bd0c23c425d363a75610524d6 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 21 Jan 2024 16:08:40 +0100 Subject: [PATCH 13/19] api: remove plain-text error-responses (api < v1.24) Commit 322e2a7d059a81617b593cf6ece2cfd9f6d4ea03 changed the format of errors returned by the API to be in JSON format for API v1.24. Older versions of the API returned errors in plain-text format. API v1.23 and older are deprecated, so we can remove support for plain-text error responses. Signed-off-by: Sebastiaan van Stijn --- api/server/errorhandler.go | 34 ------------------------------ api/server/server.go | 20 +++++++++--------- integration-cli/docker_api_test.go | 28 ++---------------------- 3 files changed, 12 insertions(+), 70 deletions(-) delete mode 100644 api/server/errorhandler.go diff --git a/api/server/errorhandler.go b/api/server/errorhandler.go deleted file mode 100644 index 71a0c13c84..0000000000 --- a/api/server/errorhandler.go +++ /dev/null @@ -1,34 +0,0 @@ -package server - -import ( - "net/http" - - "github.com/docker/docker/api/server/httpstatus" - "github.com/docker/docker/api/server/httputils" - "github.com/docker/docker/api/types" - "github.com/docker/docker/api/types/versions" - "github.com/gorilla/mux" - "google.golang.org/grpc/status" -) - -// makeErrorHandler makes an HTTP handler that decodes a Docker error and -// returns it in the response. -func makeErrorHandler(err error) http.HandlerFunc { - return func(w http.ResponseWriter, r *http.Request) { - statusCode := httpstatus.FromError(err) - vars := mux.Vars(r) - if apiVersionSupportsJSONErrors(vars["version"]) { - response := &types.ErrorResponse{ - Message: err.Error(), - } - _ = httputils.WriteJSON(w, statusCode, response) - } else { - http.Error(w, status.Convert(err).Message(), statusCode) - } - } -} - -func apiVersionSupportsJSONErrors(version string) bool { - const firstAPIVersionWithJSONErrors = "1.23" - return version == "" || versions.GreaterThan(version, firstAPIVersionWithJSONErrors) -} diff --git a/api/server/server.go b/api/server/server.go index 3855197fa2..5e90d305a9 100644 --- a/api/server/server.go +++ b/api/server/server.go @@ -10,6 +10,7 @@ import ( "github.com/docker/docker/api/server/middleware" "github.com/docker/docker/api/server/router" "github.com/docker/docker/api/server/router/debug" + "github.com/docker/docker/api/types" "github.com/docker/docker/dockerversion" "github.com/gorilla/mux" "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp" @@ -57,19 +58,13 @@ func (s *Server) makeHTTPHandler(handler httputils.APIFunc, operation string) ht if statusCode >= 500 { log.G(ctx).Errorf("Handler for %s %s returned error: %v", r.Method, r.URL.Path, err) } - makeErrorHandler(err)(w, r) + _ = httputils.WriteJSON(w, statusCode, &types.ErrorResponse{ + Message: err.Error(), + }) } }), operation).ServeHTTP } -type pageNotFoundError struct{} - -func (pageNotFoundError) Error() string { - return "page not found" -} - -func (pageNotFoundError) NotFound() {} - // CreateMux returns a new mux with all the routers registered. func (s *Server) CreateMux(routers ...router.Router) *mux.Router { m := mux.NewRouter() @@ -91,7 +86,12 @@ func (s *Server) CreateMux(routers ...router.Router) *mux.Router { m.Path("/debug" + r.Path()).Handler(f) } - notFoundHandler := makeErrorHandler(pageNotFoundError{}) + notFoundHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _ = httputils.WriteJSON(w, http.StatusNotFound, &types.ErrorResponse{ + Message: "page not found", + }) + }) + m.HandleFunc(versionMatcher+"/{path:.*}", notFoundHandler) m.NotFoundHandler = notFoundHandler m.MethodNotAllowedHandler = notFoundHandler diff --git a/integration-cli/docker_api_test.go b/integration-cli/docker_api_test.go index 085afcbedd..ca1235a9ff 100644 --- a/integration-cli/docker_api_test.go +++ b/integration-cli/docker_api_test.go @@ -3,7 +3,6 @@ package main import ( "context" "fmt" - "io" "net/http" "runtime" "strconv" @@ -62,9 +61,9 @@ func (s *DockerAPISuite) TestAPIClientVersionOldNotSupported(c *testing.T) { defer body.Close() assert.Equal(c, resp.StatusCode, http.StatusBadRequest) expected := fmt.Sprintf("client version %s is too old. Minimum supported API version is %s, please upgrade your client to a newer version", version, testEnv.DaemonVersion.MinAPIVersion) - content, err := io.ReadAll(body) + b, err := request.ReadBody(body) assert.NilError(c, err) - assert.Equal(c, strings.TrimSpace(string(content)), expected) + assert.Equal(c, getErrorMessage(c, b), expected) } func (s *DockerAPISuite) TestAPIErrorJSON(c *testing.T) { @@ -77,19 +76,6 @@ func (s *DockerAPISuite) TestAPIErrorJSON(c *testing.T) { assert.Equal(c, getErrorMessage(c, b), runconfig.ErrEmptyConfig.Error()) } -func (s *DockerAPISuite) TestAPIErrorPlainText(c *testing.T) { - // Windows requires API 1.25 or later. This test is validating a behaviour which was present - // in v1.23, but changed in 1.24, hence not applicable on Windows. See apiVersionSupportsJSONErrors - testRequires(c, DaemonIsLinux) - httpResp, body, err := request.Post(testutil.GetContext(c), "/v1.23/containers/create", request.JSONBody(struct{}{})) - assert.NilError(c, err) - assert.Equal(c, httpResp.StatusCode, http.StatusBadRequest) - assert.Assert(c, strings.Contains(httpResp.Header.Get("Content-Type"), "text/plain")) - b, err := request.ReadBody(body) - assert.NilError(c, err) - assert.Equal(c, strings.TrimSpace(string(b)), runconfig.ErrEmptyConfig.Error()) -} - func (s *DockerAPISuite) TestAPIErrorNotFoundJSON(c *testing.T) { // 404 is a different code path to normal errors, so test separately httpResp, body, err := request.Get(testutil.GetContext(c), "/notfound", request.JSON) @@ -100,13 +86,3 @@ func (s *DockerAPISuite) TestAPIErrorNotFoundJSON(c *testing.T) { assert.NilError(c, err) assert.Equal(c, getErrorMessage(c, b), "page not found") } - -func (s *DockerAPISuite) TestAPIErrorNotFoundPlainText(c *testing.T) { - httpResp, body, err := request.Get(testutil.GetContext(c), "/v1.23/notfound", request.JSON) - assert.NilError(c, err) - assert.Equal(c, httpResp.StatusCode, http.StatusNotFound) - assert.Assert(c, strings.Contains(httpResp.Header.Get("Content-Type"), "text/plain")) - b, err := request.ReadBody(body) - assert.NilError(c, err) - assert.Equal(c, strings.TrimSpace(string(b)), "page not found") -} From 8758d08bb4762cad7f375d0c7fa64e4356d51459 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 21 Jan 2024 17:52:05 +0100 Subject: [PATCH 14/19] api: remove handling of HostConfig on POST /containers/{id}/start (api < v1.24) API v1.20 (Docker Engine v1.11.0) and older allowed a HostConfig to be passed when starting a container. This feature was deprecated in API v1.21 (Docker Engine v1.10.0) in 3e7405aea8589f4b6b0713640596f7dee9cf7193, and removed in API v1.23 (Docker Engine v1.12.0) in commit 0a8386c8be3fa87b7523bef7fd31c81a7f84709c. API v1.23 and older are deprecated, and this patch removes the feature. Signed-off-by: Sebastiaan van Stijn --- api/server/httputils/decoder.go | 1 - api/server/router/container/backend.go | 2 +- .../router/container/container_routes.go | 30 +-- builder/builder.go | 2 +- builder/dockerfile/containerbackend.go | 2 +- builder/dockerfile/mockbackend_test.go | 2 +- daemon/cluster/executor/backend.go | 2 +- daemon/cluster/executor/container/adapter.go | 2 +- daemon/start.go | 47 +--- .../docker_deprecated_api_v124_test.go | 239 ------------------ .../docker_deprecated_api_v124_unix_test.go | 33 --- integration/container/container_test.go | 9 - runconfig/config.go | 5 - .../unix/container_hostconfig_1_14.json | 18 -- .../unix/container_hostconfig_1_19.json | 30 --- runconfig/hostconfig.go | 11 - runconfig/hostconfig_test.go | 39 --- 17 files changed, 12 insertions(+), 462 deletions(-) delete mode 100644 integration-cli/docker_deprecated_api_v124_test.go delete mode 100644 integration-cli/docker_deprecated_api_v124_unix_test.go delete mode 100644 runconfig/fixtures/unix/container_hostconfig_1_14.json delete mode 100644 runconfig/fixtures/unix/container_hostconfig_1_19.json diff --git a/api/server/httputils/decoder.go b/api/server/httputils/decoder.go index 8293503c48..c254e8e312 100644 --- a/api/server/httputils/decoder.go +++ b/api/server/httputils/decoder.go @@ -12,5 +12,4 @@ import ( // container configuration. type ContainerDecoder interface { DecodeConfig(src io.Reader) (*container.Config, *container.HostConfig, *network.NetworkingConfig, error) - DecodeHostConfig(src io.Reader) (*container.HostConfig, error) } diff --git a/api/server/router/container/backend.go b/api/server/router/container/backend.go index f35ab6a023..cc7e8f6767 100644 --- a/api/server/router/container/backend.go +++ b/api/server/router/container/backend.go @@ -38,7 +38,7 @@ type stateBackend interface { ContainerResize(name string, height, width int) error ContainerRestart(ctx context.Context, name string, options container.StopOptions) error ContainerRm(name string, config *backend.ContainerRmConfig) error - ContainerStart(ctx context.Context, name string, hostConfig *container.HostConfig, checkpoint string, checkpointDir string) error + ContainerStart(ctx context.Context, name string, checkpoint string, checkpointDir string) error ContainerStop(ctx context.Context, name string, options container.StopOptions) error ContainerUnpause(name string) error ContainerUpdate(name string, hostConfig *container.HostConfig) (container.ContainerUpdateOKBody, error) diff --git a/api/server/router/container/container_routes.go b/api/server/router/container/container_routes.go index 80ce7e3cb4..5c173aade4 100644 --- a/api/server/router/container/container_routes.go +++ b/api/server/router/container/container_routes.go @@ -168,14 +168,6 @@ func (s *containerRouter) getContainersExport(ctx context.Context, w http.Respon return s.backend.ContainerExport(ctx, vars["name"], w) } -type bodyOnStartError struct{} - -func (bodyOnStartError) Error() string { - return "starting container with non-empty request body was deprecated since API v1.22 and removed in v1.24" -} - -func (bodyOnStartError) InvalidParameter() {} - func (s *containerRouter) postContainersStart(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error { // If contentLength is -1, we can assumed chunked encoding // or more technically that the length is unknown @@ -183,33 +175,17 @@ func (s *containerRouter) postContainersStart(ctx context.Context, w http.Respon // net/http otherwise seems to swallow any headers related to chunked encoding // including r.TransferEncoding // allow a nil body for backwards compatibility - - version := httputils.VersionFromContext(ctx) - var hostConfig *container.HostConfig + // // A non-nil json object is at least 7 characters. if r.ContentLength > 7 || r.ContentLength == -1 { - if versions.GreaterThanOrEqualTo(version, "1.24") { - return bodyOnStartError{} - } - - if err := httputils.CheckForJSON(r); err != nil { - return err - } - - c, err := s.decoder.DecodeHostConfig(r.Body) - if err != nil { - return err - } - hostConfig = c + return errdefs.InvalidParameter(errors.New("starting container with non-empty request body was deprecated since API v1.22 and removed in v1.24")) } if err := httputils.ParseForm(r); err != nil { return err } - checkpoint := r.Form.Get("checkpoint") - checkpointDir := r.Form.Get("checkpoint-dir") - if err := s.backend.ContainerStart(ctx, vars["name"], hostConfig, checkpoint, checkpointDir); err != nil { + if err := s.backend.ContainerStart(ctx, vars["name"], r.Form.Get("checkpoint"), r.Form.Get("checkpoint-dir")); err != nil { return err } diff --git a/builder/builder.go b/builder/builder.go index fc855f133d..dff93cfac7 100644 --- a/builder/builder.go +++ b/builder/builder.go @@ -64,7 +64,7 @@ type ExecBackend interface { // ContainerRm removes a container specified by `id`. ContainerRm(name string, config *backend.ContainerRmConfig) error // ContainerStart starts a new container - ContainerStart(ctx context.Context, containerID string, hostConfig *container.HostConfig, checkpoint string, checkpointDir string) error + ContainerStart(ctx context.Context, containerID string, checkpoint string, checkpointDir string) error // ContainerWait stops processing until the given container is stopped. ContainerWait(ctx context.Context, name string, condition containerpkg.WaitCondition) (<-chan containerpkg.StateStatus, error) } diff --git a/builder/dockerfile/containerbackend.go b/builder/dockerfile/containerbackend.go index 8986c1277a..c81923cbc6 100644 --- a/builder/dockerfile/containerbackend.go +++ b/builder/dockerfile/containerbackend.go @@ -72,7 +72,7 @@ func (c *containerManager) Run(ctx context.Context, cID string, stdout, stderr i } }() - if err := c.backend.ContainerStart(ctx, cID, nil, "", ""); err != nil { + if err := c.backend.ContainerStart(ctx, cID, "", ""); err != nil { close(finished) logCancellationError(cancelErrCh, "error from ContainerStart: "+err.Error()) return err diff --git a/builder/dockerfile/mockbackend_test.go b/builder/dockerfile/mockbackend_test.go index e21e5b9ba5..a5f6d3e73f 100644 --- a/builder/dockerfile/mockbackend_test.go +++ b/builder/dockerfile/mockbackend_test.go @@ -46,7 +46,7 @@ func (m *MockBackend) CommitBuildStep(ctx context.Context, c backend.CommitConfi return "", nil } -func (m *MockBackend) ContainerStart(ctx context.Context, containerID string, hostConfig *container.HostConfig, checkpoint string, checkpointDir string) error { +func (m *MockBackend) ContainerStart(ctx context.Context, containerID string, checkpoint string, checkpointDir string) error { return nil } diff --git a/daemon/cluster/executor/backend.go b/daemon/cluster/executor/backend.go index 855dc14e48..6362ca31e1 100644 --- a/daemon/cluster/executor/backend.go +++ b/daemon/cluster/executor/backend.go @@ -38,7 +38,7 @@ type Backend interface { SetupIngress(clustertypes.NetworkCreateRequest, string) (<-chan struct{}, error) ReleaseIngress() (<-chan struct{}, error) CreateManagedContainer(ctx context.Context, config backend.ContainerCreateConfig) (container.CreateResponse, error) - ContainerStart(ctx context.Context, name string, hostConfig *container.HostConfig, checkpoint string, checkpointDir string) error + ContainerStart(ctx context.Context, name string, checkpoint string, checkpointDir string) error ContainerStop(ctx context.Context, name string, config container.StopOptions) error ContainerLogs(ctx context.Context, name string, config *container.LogsOptions) (msgs <-chan *backend.LogMessage, tty bool, err error) ConnectContainerToNetwork(containerName, networkName string, endpointConfig *network.EndpointSettings) error diff --git a/daemon/cluster/executor/container/adapter.go b/daemon/cluster/executor/container/adapter.go index a6f04fae7a..7360031ffa 100644 --- a/daemon/cluster/executor/container/adapter.go +++ b/daemon/cluster/executor/container/adapter.go @@ -347,7 +347,7 @@ func (c *containerAdapter) start(ctx context.Context) error { return err } - return c.backend.ContainerStart(ctx, c.container.name(), nil, "", "") + return c.backend.ContainerStart(ctx, c.container.name(), "", "") } func (c *containerAdapter) inspect(ctx context.Context) (types.ContainerJSON, error) { diff --git a/daemon/start.go b/daemon/start.go index d315ccff56..b967947af2 100644 --- a/daemon/start.go +++ b/daemon/start.go @@ -2,12 +2,10 @@ package daemon // import "github.com/docker/docker/daemon" import ( "context" - "runtime" "time" "github.com/containerd/log" "github.com/docker/docker/api/types/backend" - containertypes "github.com/docker/docker/api/types/container" "github.com/docker/docker/api/types/events" "github.com/docker/docker/container" "github.com/docker/docker/errdefs" @@ -41,7 +39,7 @@ func validateState(ctr *container.Container) error { } // ContainerStart starts a container. -func (daemon *Daemon) ContainerStart(ctx context.Context, name string, hostConfig *containertypes.HostConfig, checkpoint string, checkpointDir string) error { +func (daemon *Daemon) ContainerStart(ctx context.Context, name string, checkpoint string, checkpointDir string) error { daemonCfg := daemon.config() if checkpoint != "" && !daemonCfg.Experimental { return errdefs.InvalidParameter(errors.New("checkpoint is only supported in experimental mode")) @@ -55,51 +53,12 @@ func (daemon *Daemon) ContainerStart(ctx context.Context, name string, hostConfi return err } - // Windows does not have the backwards compatibility issue here. - if runtime.GOOS != "windows" { - // This is kept for backward compatibility - hostconfig should be passed when - // creating a container, not during start. - if hostConfig != nil { - log.G(ctx).Warn("DEPRECATED: Setting host configuration options when the container starts is deprecated and has been removed in Docker 1.12") - oldNetworkMode := ctr.HostConfig.NetworkMode - if err := daemon.setSecurityOptions(&daemonCfg.Config, ctr, hostConfig); err != nil { - return errdefs.InvalidParameter(err) - } - if err := daemon.mergeAndVerifyLogConfig(&hostConfig.LogConfig); err != nil { - return errdefs.InvalidParameter(err) - } - if err := daemon.setHostConfig(ctr, hostConfig); err != nil { - return errdefs.InvalidParameter(err) - } - newNetworkMode := ctr.HostConfig.NetworkMode - if string(oldNetworkMode) != string(newNetworkMode) { - // if user has change the network mode on starting, clean up the - // old networks. It is a deprecated feature and has been removed in Docker 1.12 - ctr.NetworkSettings.Networks = nil - } - if err := ctr.CheckpointTo(daemon.containersReplica); err != nil { - return errdefs.System(err) - } - ctr.InitDNSHostConfig() - } - } else { - if hostConfig != nil { - return errdefs.InvalidParameter(errors.New("Supplying a hostconfig on start is not supported. It should be supplied on create")) - } - } - // check if hostConfig is in line with the current system settings. - // It may happen cgroups are umounted or the like. + // It may happen cgroups are unmounted or the like. if _, err = daemon.verifyContainerSettings(daemonCfg, ctr.HostConfig, nil, false); err != nil { return errdefs.InvalidParameter(err) } - // Adapt for old containers in case we have updates in this function and - // old containers never have chance to call the new function in create stage. - if hostConfig != nil { - if err := daemon.adaptContainerSettings(&daemonCfg.Config, ctr.HostConfig); err != nil { - return errdefs.InvalidParameter(err) - } - } + return daemon.containerStart(ctx, daemonCfg, ctr, checkpoint, checkpointDir, true) } diff --git a/integration-cli/docker_deprecated_api_v124_test.go b/integration-cli/docker_deprecated_api_v124_test.go deleted file mode 100644 index 68ff53f42e..0000000000 --- a/integration-cli/docker_deprecated_api_v124_test.go +++ /dev/null @@ -1,239 +0,0 @@ -// This file will be removed when we completely drop support for -// passing HostConfig to container start API. - -package main - -import ( - "net/http" - "strings" - "testing" - - "github.com/docker/docker/integration-cli/cli" - "github.com/docker/docker/testutil" - "github.com/docker/docker/testutil/request" - "gotest.tools/v3/assert" - is "gotest.tools/v3/assert/cmp" -) - -func formatV123StartAPIURL(url string) string { - return "/v1.23" + url -} - -func (s *DockerAPISuite) TestDeprecatedContainerAPIStartHostConfig(c *testing.T) { - name := "test-deprecated-api-124" - cli.DockerCmd(c, "create", "--name", name, "busybox") - config := map[string]interface{}{ - "Binds": []string{"/aa:/bb"}, - } - res, body, err := request.Post(testutil.GetContext(c), "/containers/"+name+"/start", request.JSONBody(config)) - assert.NilError(c, err) - assert.Equal(c, res.StatusCode, http.StatusBadRequest) - buf, err := request.ReadBody(body) - assert.NilError(c, err) - - assert.Equal(c, res.StatusCode, http.StatusBadRequest) - assert.Assert(c, strings.Contains(string(buf), "was deprecated since API v1.22")) -} - -func (s *DockerAPISuite) TestDeprecatedContainerAPIStartVolumeBinds(c *testing.T) { - // TODO Windows CI: Investigate further why this fails on Windows to Windows CI. - testRequires(c, DaemonIsLinux) - path := "/foo" - if testEnv.DaemonInfo.OSType == "windows" { - path = `c:\foo` - } - name := "testing" - config := map[string]interface{}{ - "Image": "busybox", - "Volumes": map[string]struct{}{path: {}}, - } - - res, _, err := request.Post(testutil.GetContext(c), formatV123StartAPIURL("/containers/create?name="+name), request.JSONBody(config)) - assert.NilError(c, err) - assert.Equal(c, res.StatusCode, http.StatusCreated) - - bindPath := RandomTmpDirPath("test", testEnv.DaemonInfo.OSType) - config = map[string]interface{}{ - "Binds": []string{bindPath + ":" + path}, - } - res, _, err = request.Post(testutil.GetContext(c), formatV123StartAPIURL("/containers/"+name+"/start"), request.JSONBody(config)) - assert.NilError(c, err) - assert.Equal(c, res.StatusCode, http.StatusNoContent) - - pth, err := inspectMountSourceField(name, path) - assert.NilError(c, err) - assert.Equal(c, pth, bindPath, "expected volume host path to be %s, got %s", bindPath, pth) -} - -// Test for GH#10618 -func (s *DockerAPISuite) TestDeprecatedContainerAPIStartDupVolumeBinds(c *testing.T) { - // TODO Windows to Windows CI - Port this - testRequires(c, DaemonIsLinux) - name := "testdups" - config := map[string]interface{}{ - "Image": "busybox", - "Volumes": map[string]struct{}{"/tmp": {}}, - } - - res, _, err := request.Post(testutil.GetContext(c), formatV123StartAPIURL("/containers/create?name="+name), request.JSONBody(config)) - assert.NilError(c, err) - assert.Equal(c, res.StatusCode, http.StatusCreated) - - bindPath1 := RandomTmpDirPath("test1", testEnv.DaemonInfo.OSType) - bindPath2 := RandomTmpDirPath("test2", testEnv.DaemonInfo.OSType) - - config = map[string]interface{}{ - "Binds": []string{bindPath1 + ":/tmp", bindPath2 + ":/tmp"}, - } - res, body, err := request.Post(testutil.GetContext(c), formatV123StartAPIURL("/containers/"+name+"/start"), request.JSONBody(config)) - assert.NilError(c, err) - - buf, err := request.ReadBody(body) - assert.NilError(c, err) - assert.Equal(c, res.StatusCode, http.StatusBadRequest) - assert.Assert(c, strings.Contains(string(buf), "Duplicate mount point"), "Expected failure due to duplicate bind mounts to same path, instead got: %q with error: %v", string(buf), err) -} - -func (s *DockerAPISuite) TestDeprecatedContainerAPIStartVolumesFrom(c *testing.T) { - // TODO Windows to Windows CI - Port this - testRequires(c, DaemonIsLinux) - volName := "voltst" - volPath := "/tmp" - - cli.DockerCmd(c, "run", "--name", volName, "-v", volPath, "busybox") - - name := "TestContainerAPIStartVolumesFrom" - config := map[string]interface{}{ - "Image": "busybox", - "Volumes": map[string]struct{}{volPath: {}}, - } - - res, _, err := request.Post(testutil.GetContext(c), formatV123StartAPIURL("/containers/create?name="+name), request.JSONBody(config)) - assert.NilError(c, err) - assert.Equal(c, res.StatusCode, http.StatusCreated) - - config = map[string]interface{}{ - "VolumesFrom": []string{volName}, - } - res, _, err = request.Post(testutil.GetContext(c), formatV123StartAPIURL("/containers/"+name+"/start"), request.JSONBody(config)) - assert.NilError(c, err) - assert.Equal(c, res.StatusCode, http.StatusNoContent) - - pth, err := inspectMountSourceField(name, volPath) - assert.NilError(c, err) - pth2, err := inspectMountSourceField(volName, volPath) - assert.NilError(c, err) - assert.Equal(c, pth, pth2, "expected volume host path to be %s, got %s", pth, pth2) -} - -// #9981 - Allow a docker created volume (ie, one in /var/lib/docker/volumes) to be used to overwrite (via passing in Binds on api start) an existing volume -func (s *DockerAPISuite) TestDeprecatedPostContainerBindNormalVolume(c *testing.T) { - // TODO Windows to Windows CI - Port this - testRequires(c, DaemonIsLinux) - cli.DockerCmd(c, "create", "-v", "/foo", "--name=one", "busybox") - - fooDir, err := inspectMountSourceField("one", "/foo") - assert.NilError(c, err) - - cli.DockerCmd(c, "create", "-v", "/foo", "--name=two", "busybox") - - bindSpec := map[string][]string{"Binds": {fooDir + ":/foo"}} - res, _, err := request.Post(testutil.GetContext(c), formatV123StartAPIURL("/containers/two/start"), request.JSONBody(bindSpec)) - assert.NilError(c, err) - assert.Equal(c, res.StatusCode, http.StatusNoContent) - - fooDir2, err := inspectMountSourceField("two", "/foo") - assert.NilError(c, err) - assert.Equal(c, fooDir2, fooDir, "expected volume path to be %s, got: %s", fooDir, fooDir2) -} - -func (s *DockerAPISuite) TestDeprecatedStartWithTooLowMemoryLimit(c *testing.T) { - // TODO Windows: Port once memory is supported - testRequires(c, DaemonIsLinux) - containerID := cli.DockerCmd(c, "create", "busybox").Stdout() - containerID = strings.TrimSpace(containerID) - - const config = `{ - "CpuShares": 100, - "Memory": 524287 - }` - - res, body, err := request.Post(testutil.GetContext(c), formatV123StartAPIURL("/containers/"+containerID+"/start"), request.RawString(config), request.JSON) - assert.NilError(c, err) - b, err := request.ReadBody(body) - assert.NilError(c, err) - assert.Equal(c, res.StatusCode, http.StatusBadRequest) - assert.Assert(c, is.Contains(string(b), "Minimum memory limit allowed is 6MB")) -} - -// #14640 -func (s *DockerAPISuite) TestDeprecatedPostContainersStartWithoutLinksInHostConfig(c *testing.T) { - // TODO Windows: Windows doesn't support supplying a hostconfig on start. - // An alternate test could be written to validate the negative testing aspect of this - testRequires(c, DaemonIsLinux) - name := "test-host-config-links" - cli.DockerCmd(c, append([]string{"create", "--name", name, "busybox"}, sleepCommandForDaemonPlatform()...)...) - - hc := inspectFieldJSON(c, name, "HostConfig") - config := `{"HostConfig":` + hc + `}` - - res, b, err := request.Post(testutil.GetContext(c), formatV123StartAPIURL("/containers/"+name+"/start"), request.RawString(config), request.JSON) - assert.NilError(c, err) - assert.Equal(c, res.StatusCode, http.StatusNoContent) - b.Close() -} - -// #14640 -func (s *DockerAPISuite) TestDeprecatedPostContainersStartWithLinksInHostConfig(c *testing.T) { - // TODO Windows: Windows doesn't support supplying a hostconfig on start. - // An alternate test could be written to validate the negative testing aspect of this - testRequires(c, DaemonIsLinux) - name := "test-host-config-links" - cli.DockerCmd(c, "run", "--name", "foo", "-d", "busybox", "top") - cli.DockerCmd(c, "create", "--name", name, "--link", "foo:bar", "busybox", "top") - - hc := inspectFieldJSON(c, name, "HostConfig") - config := `{"HostConfig":` + hc + `}` - - res, b, err := request.Post(testutil.GetContext(c), formatV123StartAPIURL("/containers/"+name+"/start"), request.RawString(config), request.JSON) - assert.NilError(c, err) - assert.Equal(c, res.StatusCode, http.StatusNoContent) - b.Close() -} - -// #14640 -func (s *DockerAPISuite) TestDeprecatedPostContainersStartWithLinksInHostConfigIdLinked(c *testing.T) { - // Windows does not support links - testRequires(c, DaemonIsLinux) - const name = "test-host-config-links" - containerID := cli.DockerCmd(c, "run", "--name", "link0", "-d", "busybox", "top").Combined() - containerID = strings.TrimSpace(containerID) - defer cli.DockerCmd(c, "stop", "link0") - cli.DockerCmd(c, "create", "--name", name, "--link", containerID, "busybox", "top") - defer cli.DockerCmd(c, "stop", name) - - hc := inspectFieldJSON(c, name, "HostConfig") - config := `{"HostConfig":` + hc + `}` - - res, b, err := request.Post(testutil.GetContext(c), formatV123StartAPIURL("/containers/"+name+"/start"), request.RawString(config), request.JSON) - assert.NilError(c, err) - assert.Equal(c, res.StatusCode, http.StatusNoContent) - b.Close() -} - -func (s *DockerAPISuite) TestDeprecatedStartWithNilDNS(c *testing.T) { - // TODO Windows: Add once DNS is supported - testRequires(c, DaemonIsLinux) - containerID := cli.DockerCmd(c, "create", "busybox").Stdout() - containerID = strings.TrimSpace(containerID) - - const config = `{"HostConfig": {"Dns": null}}` - - res, b, err := request.Post(testutil.GetContext(c), formatV123StartAPIURL("/containers/"+containerID+"/start"), request.RawString(config), request.JSON) - assert.NilError(c, err) - assert.Equal(c, res.StatusCode, http.StatusNoContent) - b.Close() - - dns := inspectFieldJSON(c, containerID, "HostConfig.Dns") - assert.Equal(c, dns, "[]") -} diff --git a/integration-cli/docker_deprecated_api_v124_unix_test.go b/integration-cli/docker_deprecated_api_v124_unix_test.go deleted file mode 100644 index 574f14f22b..0000000000 --- a/integration-cli/docker_deprecated_api_v124_unix_test.go +++ /dev/null @@ -1,33 +0,0 @@ -//go:build !windows - -package main - -import ( - "strings" - "testing" - - "github.com/docker/docker/integration-cli/cli" - "github.com/docker/docker/testutil" - "github.com/docker/docker/testutil/request" - "gotest.tools/v3/assert" -) - -// #19100 This is a deprecated feature test, it should be removed in Docker 1.12 -func (s *DockerNetworkSuite) TestDeprecatedDockerNetworkStartAPIWithHostconfig(c *testing.T) { - const netName = "test" - const conName = "foo" - cli.DockerCmd(c, "network", "create", netName) - cli.DockerCmd(c, "create", "--name", conName, "busybox", "top") - - config := map[string]interface{}{ - "HostConfig": map[string]interface{}{ - "NetworkMode": netName, - }, - } - _, _, err := request.Post(testutil.GetContext(c), formatV123StartAPIURL("/containers/"+conName+"/start"), request.JSONBody(config)) - assert.NilError(c, err) - cli.WaitRun(c, conName) - networks := inspectField(c, conName, "NetworkSettings.Networks") - assert.Assert(c, strings.Contains(networks, netName), "Should contain '%s' network", netName) - assert.Assert(c, !strings.Contains(networks, "bridge"), "Should not contain 'bridge' network") -} diff --git a/integration/container/container_test.go b/integration/container/container_test.go index 65394ec75b..31876aaed1 100644 --- a/integration/container/container_test.go +++ b/integration/container/container_test.go @@ -2,7 +2,6 @@ package container // import "github.com/docker/docker/integration/container" import ( "net/http" - "runtime" "testing" "github.com/docker/docker/testutil" @@ -25,14 +24,6 @@ func TestContainerInvalidJSON(t *testing.T) { "/exec/foobar/start", } - // windows doesnt support API < v1.24 - if runtime.GOOS != "windows" { - endpoints = append( - endpoints, - "/v1.23/containers/foobar/start", // accepts a body on API < v1.24 - ) - } - for _, ep := range endpoints { ep := ep t.Run(ep[1:], func(t *testing.T) { diff --git a/runconfig/config.go b/runconfig/config.go index 3ba1609e91..81047ea6d1 100644 --- a/runconfig/config.go +++ b/runconfig/config.go @@ -27,11 +27,6 @@ func (r ContainerDecoder) DecodeConfig(src io.Reader) (*container.Config, *conta return decodeContainerConfig(src, si) } -// DecodeHostConfig makes ContainerDecoder to implement httputils.ContainerDecoder -func (r ContainerDecoder) DecodeHostConfig(src io.Reader) (*container.HostConfig, error) { - return decodeHostConfig(src) -} - // decodeContainerConfig decodes a json encoded config into a ContainerConfigWrapper // struct and returns both a Config and a HostConfig struct, and performs some // validation. Certain parameters need daemon-side validation that cannot be done diff --git a/runconfig/fixtures/unix/container_hostconfig_1_14.json b/runconfig/fixtures/unix/container_hostconfig_1_14.json deleted file mode 100644 index c72ac91cab..0000000000 --- a/runconfig/fixtures/unix/container_hostconfig_1_14.json +++ /dev/null @@ -1,18 +0,0 @@ -{ - "Binds": ["/tmp:/tmp"], - "ContainerIDFile": "", - "LxcConf": [], - "Privileged": false, - "PortBindings": { - "80/tcp": [ - { - "HostIp": "0.0.0.0", - "HostPort": "49153" - } - ] - }, - "Links": ["/name:alias"], - "PublishAllPorts": false, - "CapAdd": ["NET_ADMIN"], - "CapDrop": ["MKNOD"] -} diff --git a/runconfig/fixtures/unix/container_hostconfig_1_19.json b/runconfig/fixtures/unix/container_hostconfig_1_19.json deleted file mode 100644 index 5ca8aa7e19..0000000000 --- a/runconfig/fixtures/unix/container_hostconfig_1_19.json +++ /dev/null @@ -1,30 +0,0 @@ -{ - "Binds": ["/tmp:/tmp"], - "Links": ["redis3:redis"], - "LxcConf": {"lxc.utsname":"docker"}, - "Memory": 0, - "MemorySwap": 0, - "CpuShares": 512, - "CpuPeriod": 100000, - "CpusetCpus": "0,1", - "CpusetMems": "0,1", - "BlkioWeight": 300, - "OomKillDisable": false, - "PortBindings": { "22/tcp": [{ "HostPort": "11022" }] }, - "PublishAllPorts": false, - "Privileged": false, - "ReadonlyRootfs": false, - "Dns": ["8.8.8.8"], - "DnsSearch": [""], - "ExtraHosts": null, - "VolumesFrom": ["parent", "other:ro"], - "CapAdd": ["NET_ADMIN"], - "CapDrop": ["MKNOD"], - "RestartPolicy": { "Name": "", "MaximumRetryCount": 0 }, - "NetworkMode": "bridge", - "Devices": [], - "Ulimits": [{}], - "LogConfig": { "Type": "json-file", "Config": {} }, - "SecurityOpt": [""], - "CgroupParent": "" -} diff --git a/runconfig/hostconfig.go b/runconfig/hostconfig.go index 8a9e65f1a2..84a4ae0b6f 100644 --- a/runconfig/hostconfig.go +++ b/runconfig/hostconfig.go @@ -1,23 +1,12 @@ package runconfig // import "github.com/docker/docker/runconfig" import ( - "io" "strings" "github.com/docker/docker/api/types/container" "github.com/docker/docker/api/types/network" ) -// DecodeHostConfig creates a HostConfig based on the specified Reader. -// It assumes the content of the reader will be JSON, and decodes it. -func decodeHostConfig(src io.Reader) (*container.HostConfig, error) { - var w ContainerConfigWrapper - if err := loadJSON(src, &w); err != nil { - return nil, err - } - return w.getHostConfig(), nil -} - // SetDefaultNetModeIfBlank changes the NetworkMode in a HostConfig structure // to default if it is not populated. This ensures backwards compatibility after // the validation of the network mode was moved from the docker CLI to the diff --git a/runconfig/hostconfig_test.go b/runconfig/hostconfig_test.go index 77f1de0ecb..a3ac0f9cf0 100644 --- a/runconfig/hostconfig_test.go +++ b/runconfig/hostconfig_test.go @@ -3,51 +3,12 @@ package runconfig // import "github.com/docker/docker/runconfig" import ( - "bytes" - "fmt" - "os" "testing" "github.com/docker/docker/api/types/container" "github.com/docker/docker/pkg/sysinfo" - "gotest.tools/v3/assert" ) -func TestDecodeHostConfig(t *testing.T) { - fixtures := []struct { - file string - }{ - {"fixtures/unix/container_hostconfig_1_14.json"}, - {"fixtures/unix/container_hostconfig_1_19.json"}, - } - - for _, f := range fixtures { - b, err := os.ReadFile(f.file) - if err != nil { - t.Fatal(err) - } - - c, err := decodeHostConfig(bytes.NewReader(b)) - if err != nil { - t.Fatal(fmt.Errorf("Error parsing %s: %v", f, err)) - } - - assert.Check(t, !c.Privileged) - - if l := len(c.Binds); l != 1 { - t.Fatalf("Expected 1 bind, found %d\n", l) - } - - if len(c.CapAdd) != 1 && c.CapAdd[0] != "NET_ADMIN" { - t.Fatalf("Expected CapAdd NET_ADMIN, got %v", c.CapAdd) - } - - if len(c.CapDrop) != 1 && c.CapDrop[0] != "NET_ADMIN" { - t.Fatalf("Expected CapDrop NET_ADMIN, got %v", c.CapDrop) - } - } -} - func TestValidateResources(t *testing.T) { type resourceTest struct { ConfigCPURealtimePeriod int64 From 19a04efa2f1cc7bc6a6d1567c63202a68e643c98 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 22 Jan 2024 00:36:13 +0100 Subject: [PATCH 15/19] api: remove API < v1.24 Commit 08e4e88482d640b9543d4ba6332109e6b6f2495d (Docker Engine v25.0.0) deprecated API version v1.23 and lower, but older API versions could be enabled through the DOCKER_MIN_API_VERSION environment variable. This patch removes all support for API versions < v1.24. Signed-off-by: Sebastiaan van Stijn --- daemon/config/config.go | 8 ++++---- daemon/config/config_linux.go | 3 --- daemon/config/config_windows.go | 7 ------- hack/make/.integration-daemon-start | 3 --- 4 files changed, 4 insertions(+), 17 deletions(-) diff --git a/daemon/config/config.go b/daemon/config/config.go index 6e01495b08..8b867b953a 100644 --- a/daemon/config/config.go +++ b/daemon/config/config.go @@ -56,8 +56,8 @@ const ( DefaultPluginNamespace = "plugins.moby" // defaultMinAPIVersion is the minimum API version supported by the API. // This version can be overridden through the "DOCKER_MIN_API_VERSION" - // environment variable. The minimum allowed version is determined - // by [minAPIVersion]. + // environment variable. It currently defaults to the minimum API version + // supported by the API server. defaultMinAPIVersion = "1.24" // SeccompProfileDefault is the built-in default seccomp profile. SeccompProfileDefault = "builtin" @@ -610,8 +610,8 @@ func ValidateMinAPIVersion(ver string) error { if strings.EqualFold(ver[0:1], "v") { return errors.New(`API version must be provided without "v" prefix`) } - if versions.LessThan(ver, minAPIVersion) { - return errors.Errorf(`minimum supported API version is %s: %s`, minAPIVersion, ver) + if versions.LessThan(ver, defaultMinAPIVersion) { + return errors.Errorf(`minimum supported API version is %s: %s`, defaultMinAPIVersion, ver) } if versions.GreaterThan(ver, api.DefaultVersion) { return errors.Errorf(`maximum supported API version is %s: %s`, api.DefaultVersion, ver) diff --git a/daemon/config/config_linux.go b/daemon/config/config_linux.go index 2c2aabafd6..986677a0ac 100644 --- a/daemon/config/config_linux.go +++ b/daemon/config/config_linux.go @@ -33,9 +33,6 @@ const ( // OCI runtime being shipped with the docker daemon package. StockRuntimeName = "runc" - // minAPIVersion represents Minimum REST API version supported - minAPIVersion = "1.12" - // userlandProxyBinary is the name of the userland-proxy binary. // In rootless-mode, [rootless.RootlessKitDockerProxyBinary] is used instead. userlandProxyBinary = "docker-proxy" diff --git a/daemon/config/config_windows.go b/daemon/config/config_windows.go index 825b150f85..05e185da88 100644 --- a/daemon/config/config_windows.go +++ b/daemon/config/config_windows.go @@ -13,13 +13,6 @@ const ( // default value. On Windows keep this empty so the value is auto-detected // based on other options. StockRuntimeName = "" - - // minAPIVersion represents Minimum REST API version supported - // Technically the first daemon API version released on Windows is v1.25 in - // engine version 1.13. However, some clients are explicitly using downlevel - // APIs (e.g. docker-compose v2.1 file format) and that is just too restrictive. - // Hence also allowing 1.24 on Windows. - minAPIVersion string = "1.24" ) // BridgeConfig is meant to store all the parameters for both the bridge driver and the default bridge network. On diff --git a/hack/make/.integration-daemon-start b/hack/make/.integration-daemon-start index 503425ae2e..cf8002afba 100644 --- a/hack/make/.integration-daemon-start +++ b/hack/make/.integration-daemon-start @@ -46,9 +46,6 @@ export DOCKER_ALLOW_SCHEMA1_PUSH_DONOTUSE=1 export DOCKER_GRAPHDRIVER=${DOCKER_GRAPHDRIVER:-vfs} export DOCKER_USERLANDPROXY=${DOCKER_USERLANDPROXY:-true} -# Allow testing old API versions -export DOCKER_MIN_API_VERSION=1.12 - # example usage: DOCKER_STORAGE_OPTS="dm.basesize=20G,dm.loopdatasize=200G" storage_params="" if [ -n "$DOCKER_STORAGE_OPTS" ]; then From 6b01719ffbfabce28d0e06c670c6ce3d106dfe3f Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 22 Jan 2024 16:49:26 +0100 Subject: [PATCH 16/19] api: add MinSupportedAPIVersion const This const contains the minimum API version that can be supported by the API server. The daemon is currently configured to use the same version, but we may increment the _configured_ minimum version when deprecating old API versions in future. Signed-off-by: Sebastiaan van Stijn --- api/common.go | 11 ++++++++++- daemon/config/config.go | 2 +- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/api/common.go b/api/common.go index 866ad8f676..b11c2fe02b 100644 --- a/api/common.go +++ b/api/common.go @@ -2,9 +2,18 @@ package api // import "github.com/docker/docker/api" // Common constants for daemon and client. const ( - // DefaultVersion of Current REST API + // DefaultVersion of the current REST API. DefaultVersion = "1.45" + // MinSupportedAPIVersion is the minimum API version that can be supported + // by the API server, specified as "major.minor". Note that the daemon + // may be configured with a different minimum API version, as returned + // in [github.com/docker/docker/api/types.Version.MinAPIVersion]. + // + // API requests for API versions lower than the configured version produce + // an error. + MinSupportedAPIVersion = "1.24" + // NoBaseImageSpecifier is the symbol used by the FROM // command to specify that no base image is to be used. NoBaseImageSpecifier = "scratch" diff --git a/daemon/config/config.go b/daemon/config/config.go index 8b867b953a..a72fba044d 100644 --- a/daemon/config/config.go +++ b/daemon/config/config.go @@ -58,7 +58,7 @@ const ( // This version can be overridden through the "DOCKER_MIN_API_VERSION" // environment variable. It currently defaults to the minimum API version // supported by the API server. - defaultMinAPIVersion = "1.24" + defaultMinAPIVersion = api.MinSupportedAPIVersion // SeccompProfileDefault is the built-in default seccomp profile. SeccompProfileDefault = "builtin" // SeccompProfileUnconfined is a special profile name for seccomp to use an From 0fef6e1c99a834b34e219aa4a3cbaba8a61ec838 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 22 Jan 2024 16:53:38 +0100 Subject: [PATCH 17/19] api/server/middleware: VersionMiddleware: improve docs Improve documentation and rename fields and variables to be more descriptive. Signed-off-by: Sebastiaan van Stijn --- api/server/middleware/version.go | 42 ++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/api/server/middleware/version.go b/api/server/middleware/version.go index 1a4b3bedef..18689fdece 100644 --- a/api/server/middleware/version.go +++ b/api/server/middleware/version.go @@ -13,18 +13,30 @@ import ( // VersionMiddleware is a middleware that // validates the client and server versions. type VersionMiddleware struct { - serverVersion string - defaultVersion string - minVersion string + serverVersion string + + // defaultAPIVersion is the default API version provided by the API server, + // specified as "major.minor". It is usually configured to the latest API + // version [github.com/docker/docker/api.DefaultVersion]. + // + // API requests for API versions greater than this version are rejected by + // the server and produce a [versionUnsupportedError]. + defaultAPIVersion string + + // minAPIVersion is the minimum API version provided by the API server, + // specified as "major.minor". + // + // API requests for API versions lower than this version are rejected by + // the server and produce a [versionUnsupportedError]. + minAPIVersion string } -// NewVersionMiddleware creates a new VersionMiddleware -// with the default versions. -func NewVersionMiddleware(s, d, m string) VersionMiddleware { +// NewVersionMiddleware creates a VersionMiddleware with the given versions. +func NewVersionMiddleware(serverVersion, defaultAPIVersion, minAPIVersion string) VersionMiddleware { return VersionMiddleware{ - serverVersion: s, - defaultVersion: d, - minVersion: m, + serverVersion: serverVersion, + defaultAPIVersion: defaultAPIVersion, + minAPIVersion: minAPIVersion, } } @@ -45,18 +57,18 @@ func (e versionUnsupportedError) InvalidParameter() {} func (v VersionMiddleware) WrapHandler(handler func(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error) func(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error { return func(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error { w.Header().Set("Server", fmt.Sprintf("Docker/%s (%s)", v.serverVersion, runtime.GOOS)) - w.Header().Set("API-Version", v.defaultVersion) + w.Header().Set("API-Version", v.defaultAPIVersion) w.Header().Set("OSType", runtime.GOOS) apiVersion := vars["version"] if apiVersion == "" { - apiVersion = v.defaultVersion + apiVersion = v.defaultAPIVersion } - if versions.LessThan(apiVersion, v.minVersion) { - return versionUnsupportedError{version: apiVersion, minVersion: v.minVersion} + if versions.LessThan(apiVersion, v.minAPIVersion) { + return versionUnsupportedError{version: apiVersion, minVersion: v.minAPIVersion} } - if versions.GreaterThan(apiVersion, v.defaultVersion) { - return versionUnsupportedError{version: apiVersion, maxVersion: v.defaultVersion} + if versions.GreaterThan(apiVersion, v.defaultAPIVersion) { + return versionUnsupportedError{version: apiVersion, maxVersion: v.defaultAPIVersion} } ctx = context.WithValue(ctx, httputils.APIVersionKey{}, apiVersion) return handler(ctx, w, r, vars) From e1897cbde4c3c0ee1475857ff93e95750cb218da Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 22 Jan 2024 17:14:31 +0100 Subject: [PATCH 18/19] api/server/middleware:use API-consts in tests Use the API consts to have more realistic values in tests. Signed-off-by: Sebastiaan van Stijn --- api/server/middleware/version_test.go | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/api/server/middleware/version_test.go b/api/server/middleware/version_test.go index a102f8e6c4..74c358b359 100644 --- a/api/server/middleware/version_test.go +++ b/api/server/middleware/version_test.go @@ -2,27 +2,27 @@ package middleware // import "github.com/docker/docker/api/server/middleware" import ( "context" + "fmt" "net/http" "net/http/httptest" "runtime" "testing" + "github.com/docker/docker/api" "github.com/docker/docker/api/server/httputils" "gotest.tools/v3/assert" is "gotest.tools/v3/assert/cmp" ) func TestVersionMiddlewareVersion(t *testing.T) { - defaultVersion := "1.10.0" - minVersion := "1.2.0" - expectedVersion := defaultVersion + expectedVersion := "" handler := func(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error { v := httputils.VersionFromContext(ctx) assert.Check(t, is.Equal(expectedVersion, v)) return nil } - m := NewVersionMiddleware(defaultVersion, defaultVersion, minVersion) + m := NewVersionMiddleware("1.2.3", api.DefaultVersion, api.MinSupportedAPIVersion) h := m.WrapHandler(handler) req, _ := http.NewRequest(http.MethodGet, "/containers/json", nil) @@ -35,19 +35,19 @@ func TestVersionMiddlewareVersion(t *testing.T) { errString string }{ { - expectedVersion: "1.10.0", + expectedVersion: api.DefaultVersion, }, { - reqVersion: "1.9.0", - expectedVersion: "1.9.0", + reqVersion: api.MinSupportedAPIVersion, + expectedVersion: api.MinSupportedAPIVersion, }, { reqVersion: "0.1", - errString: "client version 0.1 is too old. Minimum supported API version is 1.2.0, please upgrade your client to a newer version", + errString: fmt.Sprintf("client version 0.1 is too old. Minimum supported API version is %s, please upgrade your client to a newer version", api.MinSupportedAPIVersion), }, { reqVersion: "9999.9999", - errString: "client version 9999.9999 is too new. Maximum supported API version is 1.10.0", + errString: fmt.Sprintf("client version 9999.9999 is too new. Maximum supported API version is %s", api.DefaultVersion), }, } @@ -71,9 +71,7 @@ func TestVersionMiddlewareWithErrorsReturnsHeaders(t *testing.T) { return nil } - defaultVersion := "1.10.0" - minVersion := "1.2.0" - m := NewVersionMiddleware(defaultVersion, defaultVersion, minVersion) + m := NewVersionMiddleware("1.2.3", api.DefaultVersion, api.MinSupportedAPIVersion) h := m.WrapHandler(handler) req, _ := http.NewRequest(http.MethodGet, "/containers/json", nil) @@ -85,8 +83,8 @@ func TestVersionMiddlewareWithErrorsReturnsHeaders(t *testing.T) { assert.Check(t, is.ErrorContains(err, "")) hdr := resp.Result().Header - assert.Check(t, is.Contains(hdr.Get("Server"), "Docker/"+defaultVersion)) + assert.Check(t, is.Contains(hdr.Get("Server"), "Docker/1.2.3")) assert.Check(t, is.Contains(hdr.Get("Server"), runtime.GOOS)) - assert.Check(t, is.Equal(hdr.Get("API-Version"), defaultVersion)) + assert.Check(t, is.Equal(hdr.Get("API-Version"), api.DefaultVersion)) assert.Check(t, is.Equal(hdr.Get("OSType"), runtime.GOOS)) } From 14503ccebda590c6315462bb2f391b2dde7bcc2b Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 22 Jan 2024 18:38:59 +0100 Subject: [PATCH 19/19] api/server/middleware: NewVersionMiddleware: add validation Make sure the middleware cannot be initialized with out of range versions. Signed-off-by: Sebastiaan van Stijn --- api/server/middleware/version.go | 16 +++++-- api/server/middleware/version_test.go | 62 +++++++++++++++++++++++++-- api/server/server_test.go | 7 ++- cmd/dockerd/daemon.go | 18 +++++--- 4 files changed, 88 insertions(+), 15 deletions(-) diff --git a/api/server/middleware/version.go b/api/server/middleware/version.go index 18689fdece..6bd181ffeb 100644 --- a/api/server/middleware/version.go +++ b/api/server/middleware/version.go @@ -6,6 +6,7 @@ import ( "net/http" "runtime" + "github.com/docker/docker/api" "github.com/docker/docker/api/server/httputils" "github.com/docker/docker/api/types/versions" ) @@ -32,12 +33,21 @@ type VersionMiddleware struct { } // NewVersionMiddleware creates a VersionMiddleware with the given versions. -func NewVersionMiddleware(serverVersion, defaultAPIVersion, minAPIVersion string) VersionMiddleware { - return VersionMiddleware{ +func NewVersionMiddleware(serverVersion, defaultAPIVersion, minAPIVersion string) (*VersionMiddleware, error) { + if versions.LessThan(defaultAPIVersion, api.MinSupportedAPIVersion) || versions.GreaterThan(defaultAPIVersion, api.DefaultVersion) { + return nil, fmt.Errorf("invalid default API version (%s): must be between %s and %s", defaultAPIVersion, api.MinSupportedAPIVersion, api.DefaultVersion) + } + if versions.LessThan(minAPIVersion, api.MinSupportedAPIVersion) || versions.GreaterThan(minAPIVersion, api.DefaultVersion) { + return nil, fmt.Errorf("invalid minimum API version (%s): must be between %s and %s", minAPIVersion, api.MinSupportedAPIVersion, api.DefaultVersion) + } + if versions.GreaterThan(minAPIVersion, defaultAPIVersion) { + return nil, fmt.Errorf("invalid API version: the minimum API version (%s) is higher than the default version (%s)", minAPIVersion, defaultAPIVersion) + } + return &VersionMiddleware{ serverVersion: serverVersion, defaultAPIVersion: defaultAPIVersion, minAPIVersion: minAPIVersion, - } + }, nil } type versionUnsupportedError struct { diff --git a/api/server/middleware/version_test.go b/api/server/middleware/version_test.go index 74c358b359..1c7888bd95 100644 --- a/api/server/middleware/version_test.go +++ b/api/server/middleware/version_test.go @@ -14,6 +14,60 @@ import ( is "gotest.tools/v3/assert/cmp" ) +func TestNewVersionMiddlewareValidation(t *testing.T) { + tests := []struct { + doc, defaultVersion, minVersion, expectedErr string + }{ + { + doc: "defaults", + defaultVersion: api.DefaultVersion, + minVersion: api.MinSupportedAPIVersion, + }, + { + doc: "invalid default lower than min", + defaultVersion: api.MinSupportedAPIVersion, + minVersion: api.DefaultVersion, + expectedErr: fmt.Sprintf("invalid API version: the minimum API version (%s) is higher than the default version (%s)", api.DefaultVersion, api.MinSupportedAPIVersion), + }, + { + doc: "invalid default too low", + defaultVersion: "0.1", + minVersion: api.MinSupportedAPIVersion, + expectedErr: fmt.Sprintf("invalid default API version (0.1): must be between %s and %s", api.MinSupportedAPIVersion, api.DefaultVersion), + }, + { + doc: "invalid default too high", + defaultVersion: "9999.9999", + minVersion: api.DefaultVersion, + expectedErr: fmt.Sprintf("invalid default API version (9999.9999): must be between %s and %s", api.MinSupportedAPIVersion, api.DefaultVersion), + }, + { + doc: "invalid minimum too low", + defaultVersion: api.MinSupportedAPIVersion, + minVersion: "0.1", + expectedErr: fmt.Sprintf("invalid minimum API version (0.1): must be between %s and %s", api.MinSupportedAPIVersion, api.DefaultVersion), + }, + { + doc: "invalid minimum too high", + defaultVersion: api.DefaultVersion, + minVersion: "9999.9999", + expectedErr: fmt.Sprintf("invalid minimum API version (9999.9999): must be between %s and %s", api.MinSupportedAPIVersion, api.DefaultVersion), + }, + } + + for _, tc := range tests { + tc := tc + t.Run(tc.doc, func(t *testing.T) { + _, err := NewVersionMiddleware("1.2.3", tc.defaultVersion, tc.minVersion) + if tc.expectedErr == "" { + assert.Check(t, err) + } else { + assert.Check(t, is.Error(err, tc.expectedErr)) + } + }) + } +} + func TestVersionMiddlewareVersion(t *testing.T) { expectedVersion := "" handler := func(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error { @@ -22,7 +76,8 @@ func TestVersionMiddlewareVersion(t *testing.T) { return nil } - m := NewVersionMiddleware("1.2.3", api.DefaultVersion, api.MinSupportedAPIVersion) + m, err := NewVersionMiddleware("1.2.3", api.DefaultVersion, api.MinSupportedAPIVersion) + assert.NilError(t, err) h := m.WrapHandler(handler) req, _ := http.NewRequest(http.MethodGet, "/containers/json", nil) @@ -71,7 +126,8 @@ func TestVersionMiddlewareWithErrorsReturnsHeaders(t *testing.T) { return nil } - m := NewVersionMiddleware("1.2.3", api.DefaultVersion, api.MinSupportedAPIVersion) + m, err := NewVersionMiddleware("1.2.3", api.DefaultVersion, api.MinSupportedAPIVersion) + assert.NilError(t, err) h := m.WrapHandler(handler) req, _ := http.NewRequest(http.MethodGet, "/containers/json", nil) @@ -79,7 +135,7 @@ func TestVersionMiddlewareWithErrorsReturnsHeaders(t *testing.T) { ctx := context.Background() vars := map[string]string{"version": "0.1"} - err := h(ctx, resp, req, vars) + err = h(ctx, resp, req, vars) assert.Check(t, is.ErrorContains(err, "")) hdr := resp.Result().Header diff --git a/api/server/server_test.go b/api/server/server_test.go index 65dda95c9c..cf8b0568aa 100644 --- a/api/server/server_test.go +++ b/api/server/server_test.go @@ -15,8 +15,11 @@ import ( func TestMiddlewares(t *testing.T) { srv := &Server{} - const apiMinVersion = "1.12" - srv.UseMiddleware(middleware.NewVersionMiddleware("0.1omega2", api.DefaultVersion, apiMinVersion)) + m, err := middleware.NewVersionMiddleware("0.1omega2", api.DefaultVersion, api.MinSupportedAPIVersion) + if err != nil { + t.Fatal(err) + } + srv.UseMiddleware(*m) req, _ := http.NewRequest(http.MethodGet, "/containers/json", nil) resp := httptest.NewRecorder() diff --git a/cmd/dockerd/daemon.go b/cmd/dockerd/daemon.go index 9a8ffe0b0c..99c5aabbf8 100644 --- a/cmd/dockerd/daemon.go +++ b/cmd/dockerd/daemon.go @@ -256,7 +256,10 @@ func (cli *DaemonCli) start(opts *daemonOptions) (err error) { pluginStore := plugin.NewStore() var apiServer apiserver.Server - cli.authzMiddleware = initMiddlewares(&apiServer, cli.Config, pluginStore) + cli.authzMiddleware, err = initMiddlewares(&apiServer, cli.Config, pluginStore) + if err != nil { + return errors.Wrap(err, "failed to start API server") + } d, err := daemon.NewDaemon(ctx, cli.Config, pluginStore, cli.authzMiddleware) if err != nil { @@ -708,14 +711,15 @@ func (opts routerOptions) Build() []router.Router { return routers } -func initMiddlewares(s *apiserver.Server, cfg *config.Config, pluginStore plugingetter.PluginGetter) *authorization.Middleware { - v := dockerversion.Version - +func initMiddlewares(s *apiserver.Server, cfg *config.Config, pluginStore plugingetter.PluginGetter) (*authorization.Middleware, error) { exp := middleware.NewExperimentalMiddleware(cfg.Experimental) s.UseMiddleware(exp) - vm := middleware.NewVersionMiddleware(v, api.DefaultVersion, cfg.MinAPIVersion) - s.UseMiddleware(vm) + vm, err := middleware.NewVersionMiddleware(dockerversion.Version, api.DefaultVersion, cfg.MinAPIVersion) + if err != nil { + return nil, err + } + s.UseMiddleware(*vm) if cfg.CorsHeaders != "" { c := middleware.NewCORSMiddleware(cfg.CorsHeaders) @@ -724,7 +728,7 @@ func initMiddlewares(s *apiserver.Server, cfg *config.Config, pluginStore plugin authzMiddleware := authorization.NewMiddleware(cfg.AuthorizationPlugins, pluginStore) s.UseMiddleware(authzMiddleware) - return authzMiddleware + return authzMiddleware, nil } func (cli *DaemonCli) getContainerdDaemonOpts() ([]supervisor.DaemonOpt, error) {