From 3beb2e4422766b42dadc75be3139f51f55570b68 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Wed, 17 Feb 2021 20:41:12 +0000 Subject: [PATCH] Move cpu variant checks into platform matcher Wrap platforms.Only and fallback to our ignore mismatches due to empty CPU variants. This just cleans things up and makes the logic re-usable in other places. Signed-off-by: Brian Goff (cherry picked from commit 50f39e724707ef121a988e422eb52045d3754802) Signed-off-by: Brian Goff --- daemon/create.go | 3 +- daemon/images/image.go | 72 ++++++++++++++++++++++++------------ daemon/images/images_test.go | 33 +++++++++++++++++ 3 files changed, 84 insertions(+), 24 deletions(-) create mode 100644 daemon/images/images_test.go diff --git a/daemon/create.go b/daemon/create.go index e0d6eeea8b..57f1eff665 100644 --- a/daemon/create.go +++ b/daemon/create.go @@ -12,6 +12,7 @@ import ( containertypes "github.com/docker/docker/api/types/container" networktypes "github.com/docker/docker/api/types/network" "github.com/docker/docker/container" + "github.com/docker/docker/daemon/images" "github.com/docker/docker/errdefs" "github.com/docker/docker/image" "github.com/docker/docker/pkg/idtools" @@ -89,7 +90,7 @@ func (daemon *Daemon) containerCreate(opts createOpts) (containertypes.Container Variant: img.Variant, } - if !platforms.Only(p).Match(imgPlat) { + if !images.OnlyPlatformWithFallback(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))) } } diff --git a/daemon/images/image.go b/daemon/images/image.go index ac02dde1c7..cffa5414ae 100644 --- a/daemon/images/image.go +++ b/daemon/images/image.go @@ -55,6 +55,8 @@ func (i *ImageService) manifestMatchesPlatform(img *image.Image, platform specs. return false } + // Note we are comparing against manifest lists here, which we expect to always have a CPU variant set (where applicable). + // So there is no need for the fallback matcher here. comparer := platforms.Only(platform) var ( @@ -161,31 +163,24 @@ func (i *ImageService) GetImage(refOrID string, platform *specs.Platform) (retIm 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) { - // Sometimes image variant is not populated due to legacy builders - // We still should support falling back here. - if imgPlat.OS == platform.OS && imgPlat.Architecture == platform.Architecture && imgPlat.Variant == "" { - logrus.WithField("image", refOrID).WithField("platform", platforms.Format(p)).Debug("Image platform cpu variant is not populated, but otherwise it matches what was requested") - return - } - - // In some cases the image config can actually be wrong (e.g. classic `docker build` may not handle `--platform` correctly) - // So we'll look up the manifest list that coresponds to this imaage to check if at least the manifest list says it is the correct image. - if i.manifestMatchesPlatform(retImg, p) { - 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. - 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))) + if OnlyPlatformWithFallback(p).Match(imgPlat) { return } + // In some cases the image config can actually be wrong (e.g. classic `docker build` may not handle `--platform` correctly) + // So we'll look up the manifest list that coresponds to this imaage to check if at least the manifest list says it is the correct image. + if i.manifestMatchesPlatform(retImg, p) { + 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. + 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))) }() ref, err := reference.ParseAnyReference(refOrID) if err != nil { @@ -223,3 +218,34 @@ func (i *ImageService) GetImage(refOrID string, platform *specs.Platform) (retIm return nil, ErrImageDoesNotExist{ref} } + +// OnlyPlatformWithFallback uses `platforms.Only` with a fallback to handle the case where the platform +// being matched does not have a CPU variant. +// +// The reason for this is that CPU variant is not even if the official image config spec as of this writing. +// See: https://github.com/opencontainers/image-spec/pull/809 +// Since Docker tends to compare platforms from the image config, we need to handle this case. +func OnlyPlatformWithFallback(p specs.Platform) platforms.Matcher { + return &onlyFallbackMatcher{only: platforms.Only(p), p: platforms.Normalize(p)} +} + +type onlyFallbackMatcher struct { + only platforms.Matcher + p specs.Platform +} + +func (m *onlyFallbackMatcher) Match(other specs.Platform) bool { + if m.only.Match(other) { + // It matches, no reason to fallback + return true + } + if other.Variant != "" { + // If there is a variant then this fallback does not apply, and there is no match + return false + } + otherN := platforms.Normalize(other) + otherN.Variant = "" // normalization adds a default variant... which is the whole problem with `platforms.Only` + + return m.p.OS == otherN.OS && + m.p.Architecture == otherN.Architecture +} diff --git a/daemon/images/images_test.go b/daemon/images/images_test.go new file mode 100644 index 0000000000..2608c0b4ed --- /dev/null +++ b/daemon/images/images_test.go @@ -0,0 +1,33 @@ +package images + +import ( + "testing" + + specs "github.com/opencontainers/image-spec/specs-go/v1" + "gotest.tools/v3/assert" +) + +func TestOnlyPlatformWithFallback(t *testing.T) { + p := specs.Platform{ + OS: "linux", + Architecture: "arm", + Variant: "v8", + } + + // Check no variant + assert.Assert(t, OnlyPlatformWithFallback(p).Match(specs.Platform{ + OS: p.OS, + Architecture: p.Architecture, + })) + // check with variant + assert.Assert(t, OnlyPlatformWithFallback(p).Match(specs.Platform{ + OS: p.OS, + Architecture: p.Architecture, + Variant: p.Variant, + })) + // Make sure non-matches are false. + assert.Assert(t, !OnlyPlatformWithFallback(p).Match(specs.Platform{ + OS: p.OS, + Architecture: "amd64", + })) +}