Merge pull request #41955 from cpuguy83/fallback_manifest_on_bad_plat

Fallback to  manifest list when no platform match

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)))
 		}
 	}

daemon/images/image.go

@@ -1,15 +1,24 @@
 package images // import "github.com/docker/docker/daemon/images"
 
 import (
+	"context"
+	"encoding/json"
 	"fmt"
+	"io"
+	"io/ioutil"
 
+	"github.com/containerd/containerd/content"
+	c8derrdefs "github.com/containerd/containerd/errdefs"
+	"github.com/containerd/containerd/images"
+	"github.com/containerd/containerd/leases"
 	"github.com/containerd/containerd/platforms"
-	"github.com/pkg/errors"
-
 	"github.com/docker/distribution/reference"
 	"github.com/docker/docker/errdefs"
 	"github.com/docker/docker/image"
+	digest "github.com/opencontainers/go-digest"
 	specs "github.com/opencontainers/image-spec/specs-go/v1"
+	"github.com/pkg/errors"
+	"github.com/sirupsen/logrus"
 )
 
 // ErrImageDoesNotExist is error returned when no image can be found for a reference.
@@ -28,6 +37,117 @@ func (e ErrImageDoesNotExist) Error() string {
 // NotFound implements the NotFound interface
 func (e ErrImageDoesNotExist) NotFound() {}
 
+type manifestList struct {
+	Manifests []specs.Descriptor `json:"manifests"`
+}
+
+type manifest struct {
+	Config specs.Descriptor `json:"config"`
+}
+
+func (i *ImageService) manifestMatchesPlatform(img *image.Image, platform specs.Platform) bool {
+	ctx := context.TODO()
+	logger := logrus.WithField("image", img.ID).WithField("desiredPlatform", platforms.Format(platform))
+
+	ls, leaseErr := i.leases.ListResources(context.TODO(), leases.Lease{ID: imageKey(img.ID().Digest())})
+	if leaseErr != nil {
+		logger.WithError(leaseErr).Error("Error looking up image leases")
+		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 (
+		ml manifestList
+		m  manifest
+	)
+
+	makeRdr := func(ra content.ReaderAt) io.Reader {
+		return io.LimitReader(io.NewSectionReader(ra, 0, ra.Size()), 1e6)
+	}
+
+	for _, r := range ls {
+		logger := logger.WithField("resourceID", r.ID).WithField("resourceType", r.Type)
+		logger.Debug("Checking lease resource for platform match")
+		if r.Type != "content" {
+			continue
+		}
+
+		ra, err := i.content.ReaderAt(ctx, specs.Descriptor{Digest: digest.Digest(r.ID)})
+		if err != nil {
+			if c8derrdefs.IsNotFound(err) {
+				continue
+			}
+			logger.WithError(err).Error("Error looking up referenced manifest list for image")
+			continue
+		}
+
+		data, err := ioutil.ReadAll(makeRdr(ra))
+		ra.Close()
+
+		if err != nil {
+			logger.WithError(err).Error("Error reading manifest list for image")
+			continue
+		}
+
+		ml.Manifests = nil
+
+		if err := json.Unmarshal(data, &ml); err != nil {
+			logger.WithError(err).Error("Error unmarshalling content")
+			continue
+		}
+
+		for _, md := range ml.Manifests {
+			switch md.MediaType {
+			case specs.MediaTypeImageManifest, images.MediaTypeDockerSchema2Manifest:
+			default:
+				continue
+			}
+
+			p := specs.Platform{
+				Architecture: md.Platform.Architecture,
+				OS:           md.Platform.OS,
+				Variant:      md.Platform.Variant,
+			}
+			if !comparer.Match(p) {
+				logger.WithField("otherPlatform", platforms.Format(p)).Debug("Manifest is not a match")
+				continue
+			}
+
+			// Here we have a platform match for the referenced manifest, let's make sure the manifest is actually for the image config we are using.
+
+			ra, err := i.content.ReaderAt(ctx, specs.Descriptor{Digest: md.Digest})
+			if err != nil {
+				logger.WithField("otherDigest", md.Digest).WithError(err).Error("Could not get reader for manifest")
+				continue
+			}
+
+			data, err := ioutil.ReadAll(makeRdr(ra))
+			ra.Close()
+			if err != nil {
+				logger.WithError(err).Error("Error reading manifest for image")
+				continue
+			}
+
+			if err := json.Unmarshal(data, &m); err != nil {
+				logger.WithError(err).Error("Error desserializing manifest")
+				continue
+			}
+
+			if m.Config.Digest == img.ID().Digest() {
+				logger.WithField("manifestDigest", md.Digest).Debug("Found matching manifest for image")
+				return true
+			}
+
+			logger.WithField("otherDigest", md.Digest).Debug("Skipping non-matching manifest")
+		}
+	}
+
+	return false
+}
+
 // GetImage returns an image corresponding to the image referred to by refOrID.
 func (i *ImageService) GetImage(refOrID string, platform *specs.Platform) (retImg *image.Image, retErr error) {
 	defer func() {
@@ -43,18 +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) {
-			// 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 {
@@ -92,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
+}

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",
+	}))
+}