From 81ac487d712c6501492393dc52f7d78e0d0c0d9c Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 22 Dec 2021 14:07:15 +0100 Subject: [PATCH 1/4] api: postImagesCreate(): rename ambiguous err variable This error is meant to be used in the output stream, and some comments were added to prevent accidentally using local variables. Renaming the variable instead to make it less ambiguous. Signed-off-by: Sebastiaan van Stijn --- api/server/router/image/image_routes.go | 27 +++++++++++-------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/api/server/router/image/image_routes.go b/api/server/router/image/image_routes.go index 9a4648bb49..2d4cc6c3c0 100644 --- a/api/server/router/image/image_routes.go +++ b/api/server/router/image/image_routes.go @@ -29,13 +29,13 @@ func (s *imageRouter) postImagesCreate(ctx context.Context, w http.ResponseWrite } var ( - image = r.Form.Get("fromImage") - repo = r.Form.Get("repo") - tag = r.Form.Get("tag") - message = r.Form.Get("message") - err error - output = ioutils.NewWriteFlusher(w) - platform *specs.Platform + image = r.Form.Get("fromImage") + repo = r.Form.Get("repo") + tag = r.Form.Get("tag") + message = r.Form.Get("message") + progressErr error + output = ioutils.NewWriteFlusher(w) + platform *specs.Platform ) defer output.Close() @@ -71,23 +71,20 @@ func (s *imageRouter) postImagesCreate(ctx context.Context, w http.ResponseWrite authConfig = &types.AuthConfig{} } } - err = s.backend.PullImage(ctx, image, tag, platform, metaHeaders, authConfig, output) + progressErr = s.backend.PullImage(ctx, image, tag, platform, metaHeaders, authConfig, output) } else { // import src := r.Form.Get("fromSrc") - // 'err' MUST NOT be defined within this block, we need any error - // generated from the download to be available to the output - // stream processing below os := "" if platform != nil { os = platform.OS } - err = s.backend.ImportImage(src, repo, os, tag, message, r.Body, output, r.Form["changes"]) + progressErr = s.backend.ImportImage(src, repo, os, tag, message, r.Body, output, r.Form["changes"]) } - if err != nil { + if progressErr != nil { if !output.Flushed() { - return err + return progressErr } - _, _ = output.Write(streamformatter.FormatError(err)) + _, _ = output.Write(streamformatter.FormatError(progressErr)) } return nil From 9839ddd80047c62ce5e28f9436a1709eb2a5555d Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 22 Dec 2021 14:12:00 +0100 Subject: [PATCH 2/4] api: postImagesCreate(): use local variable for platform Signed-off-by: Sebastiaan van Stijn --- api/server/router/image/image_routes.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/api/server/router/image/image_routes.go b/api/server/router/image/image_routes.go index 2d4cc6c3c0..59722fe861 100644 --- a/api/server/router/image/image_routes.go +++ b/api/server/router/image/image_routes.go @@ -43,9 +43,8 @@ func (s *imageRouter) postImagesCreate(ctx context.Context, w http.ResponseWrite version := httputils.VersionFromContext(ctx) if versions.GreaterThanOrEqualTo(version, "1.32") { - apiPlatform := r.FormValue("platform") - if apiPlatform != "" { - sp, err := platforms.Parse(apiPlatform) + if p := r.FormValue("platform"); p != "" { + sp, err := platforms.Parse(p) if err != nil { return err } From 6b69de61f9ef499ced56394cb79c72d53dc15b9f Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 22 Dec 2021 15:15:44 +0100 Subject: [PATCH 3/4] integration-cli: TestImportBadURL: use proper rfc6761 "invalid domain" Just a minor nit: make sure we use a designated "bad" domain https://datatracker.ietf.org/doc/html/rfc6761#section-6.4 Signed-off-by: Sebastiaan van Stijn --- integration-cli/docker_cli_import_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration-cli/docker_cli_import_test.go b/integration-cli/docker_cli_import_test.go index 03ce0b9fac..d8fbeb0fc3 100644 --- a/integration-cli/docker_cli_import_test.go +++ b/integration-cli/docker_cli_import_test.go @@ -33,7 +33,7 @@ func (s *DockerSuite) TestImportDisplay(c *testing.T) { } func (s *DockerSuite) TestImportBadURL(c *testing.T) { - out, _, err := dockerCmdWithError("import", "http://nourl/bad") + out, _, err := dockerCmdWithError("import", "https://nosuchdomain.invalid/bad") assert.Assert(c, err != nil, "import was supposed to fail but didn't") // Depending on your system you can get either of these errors if !strings.Contains(out, "dial tcp") && From 01ae9525dd7d3e1fac7bd1b05f1d3286d853ff3a Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 22 Dec 2021 17:21:27 +0100 Subject: [PATCH 4/4] Add support for platform (os and architecture) on image import Commit 0380fbff37922cadf294851b1546f4c212c7f364 added the ability to pass a --platform flag on `docker import` when importing an archive. The intent of that commit was to allow importing a Linux rootfs on a Windows daemon (as part of the experimental LCOW feature). A later commit (337ba71fc1124603302e28d94e2f08674e31a756) changed some of this code to take both OS and Architecture into account (for `docker build` and `docker pull`), but did not yet update the `docker image import`. This patch updates the import endpoitn to allow passing both OS and Architecture. Note that currently only matching OSes are accepted, and an error will be produced when (e.g.) specifying `linux` on Windows and vice-versa. Signed-off-by: Sebastiaan van Stijn --- api/server/router/image/backend.go | 2 +- api/server/router/image/image_routes.go | 6 +- api/swagger.yaml | 17 ++++- daemon/images/image_import.go | 23 +++--- docs/api/version-history.md | 6 ++ integration/image/import_test.go | 98 +++++++++++++++++++++++++ 6 files changed, 135 insertions(+), 17 deletions(-) diff --git a/api/server/router/image/backend.go b/api/server/router/image/backend.go index 624b6c7363..d54d2c0e6c 100644 --- a/api/server/router/image/backend.go +++ b/api/server/router/image/backend.go @@ -30,7 +30,7 @@ type imageBackend interface { type importExportBackend interface { LoadImage(inTar io.ReadCloser, outStream io.Writer, quiet bool) error - ImportImage(src string, repository, platform string, tag string, msg string, inConfig io.ReadCloser, outStream io.Writer, changes []string) error + ImportImage(src string, repository string, platform *specs.Platform, tag string, msg string, inConfig io.ReadCloser, outStream io.Writer, changes []string) error ExportImage(names []string, outStream io.Writer) error } diff --git a/api/server/router/image/image_routes.go b/api/server/router/image/image_routes.go index 59722fe861..71b0c14f64 100644 --- a/api/server/router/image/image_routes.go +++ b/api/server/router/image/image_routes.go @@ -73,11 +73,7 @@ func (s *imageRouter) postImagesCreate(ctx context.Context, w http.ResponseWrite progressErr = s.backend.PullImage(ctx, image, tag, platform, metaHeaders, authConfig, output) } else { // import src := r.Form.Get("fromSrc") - os := "" - if platform != nil { - os = platform.OS - } - progressErr = s.backend.ImportImage(src, repo, os, tag, message, r.Body, output, r.Form["changes"]) + progressErr = s.backend.ImportImage(src, repo, platform, tag, message, r.Body, output, r.Form["changes"]) } if progressErr != nil { if !output.Flushed() { diff --git a/api/swagger.yaml b/api/swagger.yaml index 3b1ab5e404..46505d06c0 100644 --- a/api/swagger.yaml +++ b/api/swagger.yaml @@ -7678,7 +7678,22 @@ paths: type: "string" - name: "platform" in: "query" - description: "Platform in the format os[/arch[/variant]]" + description: | + Platform in the format os[/arch[/variant]]. + + When used in combination with the `fromImage` option, the daemon checks + if the given image is present in the local image cache with the given + OS and Architecture, and otherwise attempts to pull the image. If the + option is not set, the host's native OS and Architecture are used. + If the given image does not exist in the local image cache, the daemon + attempts to pull the image with the host's native OS and Architecture. + If the given image does exists in the local image cache, but its OS or + architecture does not match, a warning is produced. + + When used with the `fromSrc` option to import an image from an archive, + this option sets the platform information for the imported image. If + the option is not set, the host's native OS and Architecture are used + for the imported image. type: "string" default: "" tags: ["Image"] diff --git a/daemon/images/image_import.go b/daemon/images/image_import.go index bd0f60c674..bec67f68a4 100644 --- a/daemon/images/image_import.go +++ b/daemon/images/image_import.go @@ -5,10 +5,10 @@ import ( "io" "net/http" "net/url" - "runtime" "strings" "time" + "github.com/containerd/containerd/platforms" "github.com/docker/distribution/reference" "github.com/docker/docker/api/types/container" "github.com/docker/docker/builder/dockerfile" @@ -20,6 +20,7 @@ import ( "github.com/docker/docker/pkg/archive" "github.com/docker/docker/pkg/progress" "github.com/docker/docker/pkg/streamformatter" + specs "github.com/opencontainers/image-spec/specs-go/v1" "github.com/pkg/errors" ) @@ -27,18 +28,13 @@ import ( // inConfig (if src is "-"), or from a URI specified in src. Progress output is // written to outStream. Repository and tag names can optionally be given in // the repo and tag arguments, respectively. -func (i *ImageService) ImportImage(src string, repository, os string, tag string, msg string, inConfig io.ReadCloser, outStream io.Writer, changes []string) error { +func (i *ImageService) ImportImage(src string, repository string, platform *specs.Platform, tag string, msg string, inConfig io.ReadCloser, outStream io.Writer, changes []string) error { var ( rc io.ReadCloser resp *http.Response newRef reference.Named ) - // Default the operating system if not supplied. - if os == "" { - os = runtime.GOOS - } - if repository != "" { var err error newRef, err = reference.ParseNormalizedNamed(repository) @@ -57,7 +53,13 @@ func (i *ImageService) ImportImage(src string, repository, os string, tag string } } - config, err := dockerfile.BuildFromConfig(&container.Config{}, changes, os) + // Normalize platform - default to the operating system and architecture if not supplied. + if platform == nil { + p := platforms.DefaultSpec() + platform = &p + } + + config, err := dockerfile.BuildFromConfig(&container.Config{}, changes, platform.OS) if err != nil { return err } @@ -102,8 +104,9 @@ func (i *ImageService) ImportImage(src string, repository, os string, tag string V1Image: image.V1Image{ DockerVersion: dockerversion.Version, Config: config, - Architecture: runtime.GOARCH, - OS: os, + Architecture: platform.Architecture, + Variant: platform.Variant, + OS: platform.OS, Created: created, Comment: msg, }, diff --git a/docs/api/version-history.md b/docs/api/version-history.md index e782fa6782..d6e44c4bbc 100644 --- a/docs/api/version-history.md +++ b/docs/api/version-history.md @@ -34,6 +34,12 @@ keywords: "API, Docker, rcli, REST, documentation" (`a disk usage operation is already running`) would be returned in this situation. This change is not versioned, and affects all API versions if the daemon has this patch. +* The `POST /images/create` now supports both the operating system and architecture + that is passed through the `platform` query parameter when using the `fromSrc` + option to import an image from an archive. Previously, only the operating system + was used and the architecture was ignored. If no `platform` option is set, the + host's operating system and architecture as used as default. This change is not + versioned, and affects all API versions if the daemon has this patch. ## v1.41 API changes diff --git a/integration/image/import_test.go b/integration/image/import_test.go index 2880df8d32..110ab87a5f 100644 --- a/integration/image/import_test.go +++ b/integration/image/import_test.go @@ -6,10 +6,12 @@ import ( "context" "io" "runtime" + "strconv" "strings" "testing" "github.com/docker/docker/api/types" + "github.com/docker/docker/image" "github.com/docker/docker/testutil" "github.com/docker/docker/testutil/daemon" "gotest.tools/v3/assert" @@ -47,3 +49,99 @@ func TestImportExtremelyLargeImageWorks(t *testing.T) { types.ImageImportOptions{}) assert.NilError(t, err) } + +func TestImportWithCustomPlatform(t *testing.T) { + skip.If(t, testEnv.OSType == "windows", "TODO enable on windows") + + defer setupTest(t)() + client := testEnv.APIClient() + ctx := context.Background() + + // Construct an empty tar archive. + var tarBuffer bytes.Buffer + + tw := tar.NewWriter(&tarBuffer) + err := tw.Close() + assert.NilError(t, err) + imageRdr := io.MultiReader(&tarBuffer, io.LimitReader(testutil.DevZero, 0)) + + tests := []struct { + name string + platform string + expected image.V1Image + expectedErr string + }{ + { + platform: "", + expected: image.V1Image{ + OS: runtime.GOOS, + Architecture: runtime.GOARCH, // this may fail on armhf due to normalization? + }, + }, + { + platform: " ", + expectedErr: "is an invalid component", + }, + { + platform: "/", + expectedErr: "is an invalid component", + }, + { + platform: runtime.GOOS, + expected: image.V1Image{ + OS: runtime.GOOS, + Architecture: runtime.GOARCH, // this may fail on armhf due to normalization? + }, + }, + { + platform: strings.ToUpper(runtime.GOOS), + expected: image.V1Image{ + OS: runtime.GOOS, + Architecture: runtime.GOARCH, // this may fail on armhf due to normalization? + }, + }, + { + platform: runtime.GOOS + "/sparc64", + expected: image.V1Image{ + OS: runtime.GOOS, + Architecture: "sparc64", + }, + }, + { + platform: "macos", + expectedErr: "operating system is not supported", + }, + { + platform: "macos/arm64", + expectedErr: "operating system is not supported", + }, + { + // TODO: platforms.Normalize() only validates os or arch if a single component is passed, + // but ignores unknown os/arch in other cases. See: + // https://github.com/containerd/containerd/blob/7d4891783aac5adf6cd83f657852574a71875631/platforms/platforms.go#L183-L209 + platform: "nintendo64", + expectedErr: "unknown operating system or architecture", + }, + } + + for i, tc := range tests { + tc := tc + t.Run(tc.platform, func(t *testing.T) { + reference := "import-with-platform:tc-" + strconv.Itoa(i) + _, err = client.ImageImport(context.Background(), + types.ImageImportSource{Source: imageRdr, SourceName: "-"}, + reference, + types.ImageImportOptions{Platform: tc.platform}) + if tc.expectedErr != "" { + assert.ErrorContains(t, err, tc.expectedErr) + } else { + assert.NilError(t, err) + + inspect, _, err := client.ImageInspectWithRaw(ctx, reference) + assert.NilError(t, err) + assert.Equal(t, inspect.Os, tc.expected.OS) + assert.Equal(t, inspect.Architecture, tc.expected.Architecture) + } + }) + } +}