From 88c02716053d3cb6f88b17ea9d4a3f21f4d8bcba Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Thu, 15 Oct 2020 23:01:17 +0000 Subject: [PATCH] Don't set default platform on container create This fixes a regression based on expectations of the runtime: ``` docker pull arm32v7/alpine docker run arm32v7/alpine ``` Without this change, the `docker run` will fail due to platform matching on non-arm32v7 systems, even though the image could run (assuming the system is setup correctly). This also emits a warning to make sure that the user is aware that a platform that does not match the default platform of the system is being run, for the cases like: ``` docker pull --platform armhf busybox docker run busybox ``` Not typically an issue if the requests are done together like that, but if the image was already there and someone did `docker run` without an explicit `--platform`, they may very well be expecting to run a native version of the image instead of the armhf one. This warning does add some extra noise in the case of platform specific images being run, such as `arm32v7/alpine`, but this can be supressed by explicitly setting the platform. Signed-off-by: Brian Goff --- Dockerfile | 3 +- .../router/container/container_routes.go | 11 ----- daemon/create.go | 19 ++++++++- daemon/images/image.go | 41 ++++++++----------- integration/container/create_test.go | 20 +++++++++ 5 files changed, 58 insertions(+), 36 deletions(-) diff --git a/Dockerfile b/Dockerfile index 822f97ea1b..c9ae07d13e 100644 --- a/Dockerfile +++ b/Dockerfile @@ -96,7 +96,8 @@ RUN /download-frozen-image-v2.sh /build \ busybox:latest@sha256:95cf004f559831017cdf4628aaf1bb30133677be8702a8c5f2994629f637a209 \ busybox:glibc@sha256:1f81263701cddf6402afe9f33fca0266d9fff379e59b1748f33d3072da71ee85 \ debian:buster@sha256:46d659005ca1151087efa997f1039ae45a7bf7a2cbbe2d17d3dcbda632a3ee9a \ - hello-world:latest@sha256:d58e752213a51785838f9eed2b7a498ffa1cb3aa7f946dda11af39286c3db9a9 + hello-world:latest@sha256:d58e752213a51785838f9eed2b7a498ffa1cb3aa7f946dda11af39286c3db9a9 \ + arm32v7/hello-world:latest@sha256:50b8560ad574c779908da71f7ce370c0a2471c098d44d1c8f6b513c5a55eeeb1 # See also ensureFrozenImagesLinux() in "integration-cli/fixtures_linux_daemon_test.go" (which needs to be updated when adding images to this list) FROM base AS cross-false diff --git a/api/server/router/container/container_routes.go b/api/server/router/container/container_routes.go index 641537dfd7..7f3fb34f4b 100644 --- a/api/server/router/container/container_routes.go +++ b/api/server/router/container/container_routes.go @@ -510,17 +510,6 @@ func (s *containerRouter) postContainersCreate(ctx context.Context, w http.Respo } platform = &p } - defaultPlatform := platforms.DefaultSpec() - if platform == nil { - platform = &defaultPlatform - } - if platform.OS == "" { - platform.OS = defaultPlatform.OS - } - if platform.Architecture == "" { - platform.Architecture = defaultPlatform.Architecture - platform.Variant = defaultPlatform.Variant - } } if hostConfig != nil && hostConfig.PidsLimit != nil && *hostConfig.PidsLimit <= 0 { diff --git a/daemon/create.go b/daemon/create.go index 2bf4418419..d4aeb77c7b 100644 --- a/daemon/create.go +++ b/daemon/create.go @@ -7,6 +7,7 @@ import ( "strings" "time" + "github.com/containerd/containerd/platforms" "github.com/docker/docker/api/types" containertypes "github.com/docker/docker/api/types/container" networktypes "github.com/docker/docker/api/types/network" @@ -16,6 +17,7 @@ import ( "github.com/docker/docker/pkg/idtools" "github.com/docker/docker/pkg/system" "github.com/docker/docker/runconfig" + v1 "github.com/opencontainers/image-spec/specs-go/v1" "github.com/opencontainers/selinux/go-selinux" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -59,8 +61,10 @@ func (daemon *Daemon) containerCreate(opts createOpts) (containertypes.Container } os := runtime.GOOS + var img *image.Image if opts.params.Config.Image != "" { - img, err := daemon.imageService.GetImage(opts.params.Config.Image, opts.params.Platform) + var err error + img, err = daemon.imageService.GetImage(opts.params.Config.Image, opts.params.Platform) if err == nil { os = img.OS } @@ -77,6 +81,19 @@ func (daemon *Daemon) containerCreate(opts createOpts) (containertypes.Container return containertypes.ContainerCreateCreatedBody{Warnings: warnings}, errdefs.InvalidParameter(err) } + if img != nil && opts.params.Platform == nil { + p := platforms.DefaultSpec() + imgPlat := v1.Platform{ + OS: img.OS, + Architecture: img.Architecture, + Variant: img.Variant, + } + + if !platforms.Only(p).Match(imgPlat) { + warnings = append(warnings, fmt.Sprintf("The requested image's platform (%s) does not match the detected host platform (%s) and no specific platform was requested", platforms.Format(imgPlat), platforms.Format(p))) + } + } + err = verifyNetworkingConfig(opts.params.NetworkingConfig) if err != nil { return containertypes.ContainerCreateCreatedBody{Warnings: warnings}, errdefs.InvalidParameter(err) diff --git a/daemon/images/image.go b/daemon/images/image.go index f060700f67..361a2d37d5 100644 --- a/daemon/images/image.go +++ b/daemon/images/image.go @@ -3,6 +3,7 @@ package images // import "github.com/docker/docker/daemon/images" import ( "fmt" + "github.com/containerd/containerd/platforms" "github.com/pkg/errors" "github.com/docker/distribution/reference" @@ -34,30 +35,24 @@ func (i *ImageService) GetImage(refOrID string, platform *specs.Platform) (retIm return } - // This allows us to tell clients that we don't have the image they asked for - // Where this gets hairy is the image store does not currently support multi-arch images, e.g.: - // An image `foo` may have a multi-arch manifest, but the image store only fetches the image for a specific platform - // The image store does not store the manifest list and image tags are assigned to architecture specific images. - // So we can have a `foo` image that is amd64 but the user requested armv7. If the user looks at the list of images. - // This may be confusing. - // The alternative to this is to return a errdefs.Conflict error with a helpful message, but clients will not be - // able to automatically tell what causes the conflict. - if retImg.OS != platform.OS { - retErr = errdefs.NotFound(errors.Errorf("image with reference %s was found but does not match the specified OS platform: wanted: %s, actual: %s", refOrID, platform.OS, retImg.OS)) - retImg = nil - return + imgPlat := specs.Platform{ + OS: retImg.OS, + Architecture: retImg.Architecture, + Variant: retImg.Variant, } - if retImg.Architecture != platform.Architecture { - retErr = errdefs.NotFound(errors.Errorf("image with reference %s was found but does not match the specified platform cpu architecture: wanted: %s, actual: %s", refOrID, platform.Architecture, retImg.Architecture)) - retImg = nil - return - } - - // Only validate variant if retImg has a variant set. - // The image variant may not be set since it's a newer field. - if platform.Variant != "" && retImg.Variant != "" && retImg.Variant != platform.Variant { - retErr = errdefs.NotFound(errors.Errorf("image with reference %s was found but does not match the specified platform cpu architecture variant: wanted: %s, actual: %s", refOrID, platform.Variant, retImg.Variant)) - retImg = nil + p := *platform + // Note that `platforms.Only` will fuzzy match this for us + // For example: an armv6 image will run just fine an an armv7 CPU, without emulation or anything. + if !platforms.Only(p).Match(imgPlat) { + // This allows us to tell clients that we don't have the image they asked for + // Where this gets hairy is the image store does not currently support multi-arch images, e.g.: + // An image `foo` may have a multi-arch manifest, but the image store only fetches the image for a specific platform + // The image store does not store the manifest list and image tags are assigned to architecture specific images. + // So we can have a `foo` image that is amd64 but the user requested armv7. If the user looks at the list of images. + // This may be confusing. + // The alternative to this is to return a errdefs.Conflict error with a helpful message, but clients will not be + // able to automatically tell what causes the conflict. + retErr = errdefs.NotFound(errors.Errorf("image with reference %s was found but does not match the specified platform: wanted %s, actual: %s", refOrID, platforms.Format(p), platforms.Format(imgPlat))) return } }() diff --git a/integration/container/create_test.go b/integration/container/create_test.go index 801f902772..b47d4c7dbb 100644 --- a/integration/container/create_test.go +++ b/integration/container/create_test.go @@ -508,3 +508,23 @@ func TestCreateVolumesFromNonExistingContainer(t *testing.T) { ) assert.Check(t, errdefs.IsInvalidParameter(err)) } + +// Test that we can create a container from an image that is for a different platform even if a platform was not specified +// This is for the regression detailed here: https://github.com/moby/moby/issues/41552 +func TestCreatePlatformSpecificImageNoPlatform(t *testing.T) { + defer setupTest(t)() + + skip.If(t, testEnv.DaemonInfo.Architecture == "arm", "test only makes sense to run on non-arm systems") + skip.If(t, testEnv.OSType != "linux", "test image is only available on linux") + cli := testEnv.APIClient() + + _, err := cli.ContainerCreate( + context.Background(), + &container.Config{Image: "arm32v7/hello-world"}, + &container.HostConfig{}, + nil, + nil, + "", + ) + assert.NilError(t, err) +}