Simplify digest references to manifest lists in c/storage

- If the digest references a single-platform manifest,
  all matching images will have the same manifest, same config,
  and same behavior of the dummy index created by
  imageMatchesSystemContext ; so don't build that dummy index
  and rename the function to multiArchImageMatchesSystemContext .
- If the digest references a multi-platform list, just check
  whether ChooseInstance would chose the current image;
  don't check _both_ ChooseInstance based on the multi-platform
  list and config values through the dummy index.
  The original pull did both (ChooseInstance to choose
  a per-platform image, and
  copy.checkImageDestinationForCurrentRuntime(), so they
  are expected to match (and we don't do a config check
  vs. the current SystemContext to refuse handling
  single-platform images in c/storage, in general).
- Even more verbosely document the situation,
  explaining how we can end up with multiple images that
  match the same digest, and what we want to achieve.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

storage/storage_reference.go

@@ -11,7 +11,6 @@ import (
 	"github.com/containers/image/v5/types"
 	"github.com/containers/storage"
 	digest "github.com/opencontainers/go-digest"
-	imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 )
@@ -62,18 +61,17 @@ func imageMatchesRepo(image *storage.Image, ref reference.Named) bool {
 	return false
 }
 
-// imageMatchesSystemContext checks if the passed-in image both contains a
+// multiArchImageMatchesSystemContext returns true if if the passed-in image both contains a
-// manifest that matches the passed-in digest, and identifies itself as being
+// multi-arch manifest that matches the passed-in digest, and the image is the per-platform
-// appropriate for running on the system that matches sys.
+// image instance that matches sys.
-// If we somehow ended up sharing the same storage among multiple types of
+//
-// systems, and managed to download multiple images from the same manifest
+// See the comment in storageReference.ResolveImage explaining why
-// list, their image records will all contain copies of the manifest list, and
+// this check is necessary.
-// this check will help us decide which of them we want to return when we've
+func multiArchImageMatchesSystemContext(store storage.Store, img *storage.Image, manifestDigest digest.Digest, sys *types.SystemContext) bool {
-// been asked to resolve an image reference that uses the list's digest to a
+	// Load the manifest that matches the specified digest.
-// specific image ID.
+	// We don't need to care about storage.ImageDigestBigDataKey because
-func imageMatchesSystemContext(store storage.Store, img *storage.Image, manifestDigest digest.Digest, sys *types.SystemContext) bool {
+	// manifests lists are only stored into storage by c/image versions
-	// First, check if the image record has a manifest that matches the
+	// that know about manifestBigDataKey, and only using that key.
-	// specified digest.
 	key := manifestBigDataKey(manifestDigest)
 	manifestBytes, err := store.ImageBigData(img.ID, key)
 	if err != nil {
@@ -83,56 +81,22 @@ func imageMatchesSystemContext(store storage.Store, img *storage.Image, manifest
 	// the digest of the instance that matches the current system, and try
 	// to load that manifest from the image record, and use it.
 	manifestType := manifest.GuessMIMEType(manifestBytes)
-	if manifest.MIMETypeIsMultiImage(manifestType) {
+	if !manifest.MIMETypeIsMultiImage(manifestType) {
+		// manifestDigest directly specifies a per-platform image, so we aren't
+		// choosing among different variants.
+		return false
+	}
 	list, err := manifest.ListFromBlob(manifestBytes, manifestType)
 	if err != nil {
 		return false
 	}
-		manifestDigest, err = list.ChooseInstance(sys)
+	chosenInstance, err := list.ChooseInstance(sys)
 	if err != nil {
 		return false
 	}
-		key = manifestBigDataKey(manifestDigest)
+	key = manifestBigDataKey(chosenInstance)
-		manifestBytes, err = store.ImageBigData(img.ID, key)
-		if err != nil {
-			return false
-		}
-		manifestType = manifest.GuessMIMEType(manifestBytes)
-	}
-	// Load the image's configuration blob.
-	m, err := manifest.FromBlob(manifestBytes, manifestType)
-	if err != nil {
-		return false
-	}
-	getConfig := func(blobInfo types.BlobInfo) ([]byte, error) {
-		return store.ImageBigData(img.ID, blobInfo.Digest.String())
-	}
-	ii, err := m.Inspect(getConfig)
-	if err != nil {
-		return false
-	}
-	// Build a dummy index containing one instance and information about
-	// the image's target system from the image's configuration.
-	index := manifest.OCI1IndexFromComponents([]imgspecv1.Descriptor{{
-		MediaType: imgspecv1.MediaTypeImageManifest,
-		Digest:    manifestDigest,
-		Size:      int64(len(manifestBytes)),
-		Platform: &imgspecv1.Platform{
-			OS:           ii.Os,
-			Architecture: ii.Architecture,
-		},
-	}}, nil)
-	// Check that ChooseInstance() would select this image for this system,
-	// from a list of images.
-	instanceDigest, err := index.ChooseInstance(sys)
-	if err != nil {
-		return false
-	}
-	// Double-check that we can read the runnable image's manifest from the
-	// image record.
-	key = manifestBigDataKey(instanceDigest)
 	_, err = store.ImageBigData(img.ID, key)
-	return err == nil
+	return err == nil // true if img.ID is based on chosenInstance.
 }
 
 // Resolve the reference's name to an image ID in the store, if there's already
@@ -152,11 +116,24 @@ func (s *storageReference) resolveImage(sys *types.SystemContext) (*storage.Imag
 			// Look for an image with the specified digest that has the same name,
 			// though possibly with a different tag or digest, as a Name value, so
 			// that the canonical reference can be implicitly resolved to the image.
+			//
+			// Typically there should be at most one such image, because the same
+			// manifest digest implies the same config, and we choose the storage ID
+			// based on the config (deduplicating images), except:
+			// - the user can explicitly specify an ID when creating the image.
+			//   In this case we don't have a preference among the alternatives.
+			// - when pulling an image from a multi-platform manifest list, we also
+			//   store the manifest list in the image; this allows referencing a
+			//   per-platform image using the manifest list digest, but that also
+			//   means that we can have multiple genuinely different images in the
+			//   storage matching the same manifest list digest (if pulled using different
+			//   SystemContext.{OS,Architecture,Variant}Choice to the same storage).
+			//   In this case we prefer the image matching the current SystemContext.
 			images, err := s.transport.store.ImagesByDigest(digested.Digest())
 			if err == nil && len(images) > 0 {
 				for _, image := range images {
 					if imageMatchesRepo(image, s.named) {
-						if loadedImage == nil || imageMatchesSystemContext(s.transport.store, image, digested.Digest(), sys) {
+						if loadedImage == nil || multiArchImageMatchesSystemContext(s.transport.store, image, digested.Digest(), sys) {
 							loadedImage = image
 							s.id = image.ID
 						}