Merge pull request #1268 from mtrmac/manifest-list-nits

Manifest list support nits

copy/copy.go

@@ -343,7 +343,7 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef,
 		case CopySpecificImages:
 			logrus.Debugf("Source is a manifest list; copying some instances")
 		}
-		if copiedManifest, _, err = c.copyMultipleImages(ctx, policyContext, options, unparsedToplevel); err != nil {
+		if copiedManifest, err = c.copyMultipleImages(ctx, policyContext, options, unparsedToplevel); err != nil {
 			return nil, err
 		}
 	}
@@ -412,15 +412,15 @@ func compareImageDestinationManifestEqual(ctx context.Context, options *Options,
 
 // copyMultipleImages copies some or all of an image list's instances, using
 // policyContext to validate source image admissibility.
-func (c *copier) copyMultipleImages(ctx context.Context, policyContext *signature.PolicyContext, options *Options, unparsedToplevel *image.UnparsedImage) (copiedManifest []byte, copiedManifestType string, retErr error) {
+func (c *copier) copyMultipleImages(ctx context.Context, policyContext *signature.PolicyContext, options *Options, unparsedToplevel *image.UnparsedImage) (copiedManifest []byte, retErr error) {
 	// Parse the list and get a copy of the original value after it's re-encoded.
 	manifestList, manifestType, err := unparsedToplevel.Manifest(ctx)
 	if err != nil {
-		return nil, "", errors.Wrapf(err, "Error reading manifest list")
+		return nil, errors.Wrapf(err, "Error reading manifest list")
 	}
 	originalList, err := manifest.ListFromBlob(manifestList, manifestType)
 	if err != nil {
-		return nil, "", errors.Wrapf(err, "Error parsing manifest list %q", string(manifestList))
+		return nil, errors.Wrapf(err, "Error parsing manifest list %q", string(manifestList))
 	}
 	updatedList := originalList.Clone()
 
@@ -432,14 +432,14 @@ func (c *copier) copyMultipleImages(ctx context.Context, policyContext *signatur
 		c.Printf("Getting image list signatures\n")
 		s, err := c.rawSource.GetSignatures(ctx, nil)
 		if err != nil {
-			return nil, "", errors.Wrap(err, "Error reading signatures")
+			return nil, errors.Wrap(err, "Error reading signatures")
 		}
 		sigs = s
 	}
 	if len(sigs) != 0 {
 		c.Printf("Checking if image list destination supports signatures\n")
 		if err := c.dest.SupportsSignatures(ctx); err != nil {
-			return nil, "", errors.Wrapf(err, "Can not copy signatures to %s", transports.ImageName(c.dest.Reference()))
+			return nil, errors.Wrapf(err, "Can not copy signatures to %s", transports.ImageName(c.dest.Reference()))
 		}
 	}
 	canModifyManifestList := (len(sigs) == 0)
@@ -454,11 +454,11 @@ func (c *copier) copyMultipleImages(ctx context.Context, policyContext *signatur
 	}
 	selectedListType, otherManifestMIMETypeCandidates, err := c.determineListConversion(manifestType, c.dest.SupportedManifestMIMETypes(), forceListMIMEType)
 	if err != nil {
-		return nil, "", errors.Wrapf(err, "Error determining manifest list type to write to destination")
+		return nil, errors.Wrapf(err, "Error determining manifest list type to write to destination")
 	}
 	if selectedListType != originalList.MIMEType() {
 		if !canModifyManifestList {
-			return nil, "", errors.Errorf("Error: manifest list must be converted to type %q to be written to destination, but that would invalidate signatures", selectedListType)
+			return nil, errors.Errorf("Error: manifest list must be converted to type %q to be written to destination, but that would invalidate signatures", selectedListType)
 		}
 	}
 
@@ -483,7 +483,7 @@ func (c *copier) copyMultipleImages(ctx context.Context, policyContext *signatur
 			if skip {
 				update, err := updatedList.Instance(instanceDigest)
 				if err != nil {
-					return nil, "", err
+					return nil, err
 				}
 				logrus.Debugf("Skipping instance %s (%d/%d)", instanceDigest, i+1, len(instanceDigests))
 				// Record the digest/size/type of the manifest that we didn't copy.
@@ -496,7 +496,7 @@ func (c *copier) copyMultipleImages(ctx context.Context, policyContext *signatur
 		unparsedInstance := image.UnparsedInstance(c.rawSource, &instanceDigest)
 		updatedManifest, updatedManifestType, updatedManifestDigest, err := c.copyOneImage(ctx, policyContext, options, unparsedToplevel, unparsedInstance, &instanceDigest)
 		if err != nil {
-			return nil, "", err
+			return nil, err
 		}
 		instancesCopied++
 		// Record the result of a possible conversion here.
@@ -510,7 +510,7 @@ func (c *copier) copyMultipleImages(ctx context.Context, policyContext *signatur
 
 	// Now reset the digest/size/types of the manifests in the list to account for any conversions that we made.
 	if err = updatedList.UpdateInstances(updates); err != nil {
-		return nil, "", errors.Wrapf(err, "Error updating manifest list")
+		return nil, errors.Wrapf(err, "Error updating manifest list")
 	}
 
 	// Iterate through supported list types, preferred format first.
@@ -525,25 +525,25 @@ func (c *copier) copyMultipleImages(ctx context.Context, policyContext *signatur
 		if thisListType != updatedList.MIMEType() {
 			attemptedList, err = updatedList.ConvertToMIMEType(thisListType)
 			if err != nil {
-				return nil, "", errors.Wrapf(err, "Error converting manifest list to list with MIME type %q", thisListType)
+				return nil, errors.Wrapf(err, "Error converting manifest list to list with MIME type %q", thisListType)
 			}
 		}
 
 		// Check if the updates or a type conversion meaningfully changed the list of images
 		// by serializing them both so that we can compare them.
 		attemptedManifestList, err := attemptedList.Serialize()
 		if err != nil {
-			return nil, "", errors.Wrapf(err, "Error encoding updated manifest list (%q: %#v)", updatedList.MIMEType(), updatedList.Instances())
+			return nil, errors.Wrapf(err, "Error encoding updated manifest list (%q: %#v)", updatedList.MIMEType(), updatedList.Instances())
 		}
 		originalManifestList, err := originalList.Serialize()
 		if err != nil {
-			return nil, "", errors.Wrapf(err, "Error encoding original manifest list for comparison (%q: %#v)", originalList.MIMEType(), originalList.Instances())
+			return nil, errors.Wrapf(err, "Error encoding original manifest list for comparison (%q: %#v)", originalList.MIMEType(), originalList.Instances())
 		}
 
 		// If we can't just use the original value, but we have to change it, flag an error.
 		if !bytes.Equal(attemptedManifestList, originalManifestList) {
 			if !canModifyManifestList {
-				return nil, "", errors.Errorf("Error: manifest list must be converted to type %q to be written to destination, but that would invalidate signatures", thisListType)
+				return nil, errors.Errorf("Error: manifest list must be converted to type %q to be written to destination, but that would invalidate signatures", thisListType)
 			}
 			logrus.Debugf("Manifest list has been updated")
 		} else {
@@ -563,24 +563,24 @@ func (c *copier) copyMultipleImages(ctx context.Context, policyContext *signatur
 		break
 	}
 	if errs != nil {
-		return nil, "", fmt.Errorf("Uploading manifest list failed, attempted the following formats: %s", strings.Join(errs, ", "))
+		return nil, fmt.Errorf("Uploading manifest list failed, attempted the following formats: %s", strings.Join(errs, ", "))
 	}
 
 	// Sign the manifest list.
 	if options.SignBy != "" {
 		newSig, err := c.createSignature(manifestList, options.SignBy)
 		if err != nil {
-			return nil, "", err
+			return nil, err
 		}
 		sigs = append(sigs, newSig)
 	}
 
 	c.Printf("Storing list signatures\n")
 	if err := c.dest.PutSignatures(ctx, sigs, nil); err != nil {
-		return nil, "", errors.Wrap(err, "Error writing signatures")
+		return nil, errors.Wrap(err, "Error writing signatures")
 	}
 
-	return manifestList, selectedListType, nil
+	return manifestList, nil
 }
 
 // copyOneImage copies a single (non-manifest-list) image unparsedImage, using policyContext to validate

copy/manifest.go

@@ -137,30 +137,29 @@ func (c *copier) determineListConversion(currentListMIMEType string, destSupport
 	if forcedListMIMEType != "" {
 		destSupportedMIMETypes = []string{forcedListMIMEType}
 	}
-	var selectedType string
-	var otherSupportedTypes []string
-	for i := range destSupportedMIMETypes {
-		// The second priority is the first member of the list of acceptable types that is a list,
-		// but keep going in case current type occurs later in the list.
-		if selectedType == "" && manifest.MIMETypeIsMultiImage(destSupportedMIMETypes[i]) {
-			selectedType = destSupportedMIMETypes[i]
-		}
-		// The first priority is the current type, if it's in the list, since that lets us avoid a
-		// conversion that isn't strictly necessary.
-		if destSupportedMIMETypes[i] == currentListMIMEType {
-			selectedType = destSupportedMIMETypes[i]
+
+	prioritizedTypes := newOrderedSet()
+	// The first priority is the current type, if it's in the list, since that lets us avoid a
+	// conversion that isn't strictly necessary.
+	for _, t := range destSupportedMIMETypes {
+		if t == currentListMIMEType {
+			prioritizedTypes.append(currentListMIMEType)
+			break
 		}
 	}
 	// Pick out the other list types that we support.
-	for i := range destSupportedMIMETypes {
-		if selectedType != destSupportedMIMETypes[i] && manifest.MIMETypeIsMultiImage(destSupportedMIMETypes[i]) {
-			otherSupportedTypes = append(otherSupportedTypes, destSupportedMIMETypes[i])
+	for _, t := range destSupportedMIMETypes {
+		if manifest.MIMETypeIsMultiImage(t) {
+			prioritizedTypes.append(t)
 		}
 	}
+
 	logrus.Debugf("Manifest list has MIME type %s, ordered candidate list [%s]", currentListMIMEType, strings.Join(destSupportedMIMETypes, ", "))
-	if selectedType == "" {
+	if len(prioritizedTypes.list) == 0 {
 		return "", nil, errors.Errorf("destination does not support any supported manifest list types (%v)", manifest.SupportedListMIMETypes)
 	}
+	selectedType := prioritizedTypes.list[0]
+	otherSupportedTypes := prioritizedTypes.list[1:]
 	if selectedType != currentListMIMEType {
 		logrus.Debugf("... will convert to %s first, and then try %v", selectedType, otherSupportedTypes)
 	} else {

copy/manifest_test.go

@@ -9,7 +9,7 @@ import (
 	"github.com/containers/image/v5/docker/reference"
 	"github.com/containers/image/v5/manifest"
 	"github.com/containers/image/v5/types"
-	"github.com/opencontainers/image-spec/specs-go/v1"
+	v1 "github.com/opencontainers/image-spec/specs-go/v1"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -220,3 +220,74 @@ func TestIsMultiImage(t *testing.T) {
 	_, err := isMultiImage(context.Background(), src)
 	assert.Error(t, err)
 }
+
+func TestDetermineManifestListConversion(t *testing.T) {
+	supportS1S2OCI := []string{
+		v1.MediaTypeImageIndex,
+		v1.MediaTypeImageManifest,
+		manifest.DockerV2ListMediaType,
+		manifest.DockerV2Schema2MediaType,
+		manifest.DockerV2Schema1SignedMediaType,
+		manifest.DockerV2Schema1MediaType,
+	}
+	supportS1S2 := []string{
+		manifest.DockerV2ListMediaType,
+		manifest.DockerV2Schema2MediaType,
+		manifest.DockerV2Schema1SignedMediaType,
+		manifest.DockerV2Schema1MediaType,
+	}
+	supportOnlyOCI := []string{
+		v1.MediaTypeImageIndex,
+		v1.MediaTypeImageManifest,
+	}
+	supportOnlyS1 := []string{
+		manifest.DockerV2Schema1SignedMediaType,
+		manifest.DockerV2Schema1MediaType,
+	}
+
+	cases := []struct {
+		description             string
+		sourceType              string
+		destTypes               []string
+		expectedUpdate          string
+		expectedOtherCandidates []string
+	}{
+		// Destination accepts anything — try all variants
+		{"s2→anything", manifest.DockerV2ListMediaType, nil, "", []string{v1.MediaTypeImageIndex}},
+		{"OCI→anything", v1.MediaTypeImageIndex, nil, "", []string{manifest.DockerV2ListMediaType}},
+		// Destination accepts the unmodified original
+		{"s2→s1s2OCI", manifest.DockerV2ListMediaType, supportS1S2OCI, "", []string{v1.MediaTypeImageIndex}},
+		{"OCI→s1s2OCI", v1.MediaTypeImageIndex, supportS1S2OCI, "", []string{manifest.DockerV2ListMediaType}},
+		{"s2→s1s2", manifest.DockerV2ListMediaType, supportS1S2, "", []string{}},
+		{"OCI→OCI", v1.MediaTypeImageIndex, supportOnlyOCI, "", []string{}},
+		// Conversion necessary, try the preferred formats in order.
+		{"special→OCI", "unrecognized", supportS1S2OCI, v1.MediaTypeImageIndex, []string{manifest.DockerV2ListMediaType}},
+		{"special→s2", "unrecognized", supportS1S2, manifest.DockerV2ListMediaType, []string{}},
+	}
+
+	for _, c := range cases {
+		copier := &copier{}
+		preferredMIMEType, otherCandidates, err := copier.determineListConversion(c.sourceType, c.destTypes, "")
+		require.NoError(t, err, c.description)
+		if c.expectedUpdate == "" {
+			assert.Equal(t, manifest.NormalizedMIMEType(c.sourceType), preferredMIMEType, c.description)
+		} else {
+			assert.Equal(t, c.expectedUpdate, preferredMIMEType, c.description)
+		}
+		assert.Equal(t, c.expectedOtherCandidates, otherCandidates, c.description)
+	}
+
+	// With forceManifestMIMEType, the output is always the forced manifest type (in this case OCI index)
+	for _, c := range cases {
+		copier := &copier{}
+		preferredMIMEType, otherCandidates, err := copier.determineListConversion(c.sourceType, c.destTypes, v1.MediaTypeImageIndex)
+		require.NoError(t, err, c.description)
+		assert.Equal(t, v1.MediaTypeImageIndex, preferredMIMEType, c.description)
+		assert.Equal(t, []string{}, otherCandidates, c.description)
+	}
+
+	// The destination doesn’t support list formats at all
+	copier := &copier{}
+	_, _, err := copier.determineListConversion(v1.MediaTypeImageIndex, supportOnlyS1, "")
+	assert.Error(t, err)
+}

directory/directory_dest.go

@@ -239,6 +239,9 @@ func (d *dirImageDestination) PutSignatures(ctx context.Context, signatures [][]
 }
 
 // Commit marks the process of storing the image as successful and asks for the image to be persisted.
+// unparsedToplevel contains data about the top-level manifest of the source (which may be a single-arch image or a manifest list
+// if PutManifest was only called for the single-arch image with instanceDigest == nil), primarily to allow lookups by the
+// original manifest list digest, if desired.
 // WARNING: This does not have any transactional semantics:
 // - Uploaded data MAY be visible to others before Commit() is called
 // - Uploaded data MAY be removed or MAY remain around if Close() is called without Commit() (i.e. rollback is allowed but not guaranteed)

directory/directory_test.go

@@ -42,7 +42,7 @@ func TestGetPutManifest(t *testing.T) {
 	assert.NoError(t, err)
 	err = dest.PutManifest(context.Background(), list, nil)
 	assert.NoError(t, err)
-	err = dest.Commit(context.Background(), nil)
+	err = dest.Commit(context.Background(), nil) // nil unparsedToplevel is invalid, we don’t currently use the value
 	assert.NoError(t, err)
 
 	src, err := ref.NewImageSource(context.Background(), nil)
@@ -71,7 +71,7 @@ func TestGetPutBlob(t *testing.T) {
 	assert.Equal(t, types.PreserveOriginal, dest.DesiredLayerCompression())
 	info, err := dest.PutBlob(context.Background(), bytes.NewReader(blob), types.BlobInfo{Digest: digest.Digest("sha256:digest-test"), Size: int64(9)}, cache, false)
 	assert.NoError(t, err)
-	err = dest.Commit(context.Background(), nil)
+	err = dest.Commit(context.Background(), nil) // nil unparsedToplevel is invalid, we don’t currently use the value
 	assert.NoError(t, err)
 	assert.Equal(t, int64(9), info.Size)
 	assert.Equal(t, digest.FromBytes(blob), info.Digest)
@@ -130,7 +130,7 @@ func TestPutBlobDigestFailure(t *testing.T) {
 	_, err = dest.PutBlob(context.Background(), reader, types.BlobInfo{Digest: blobDigest, Size: -1}, cache, false)
 	assert.Error(t, err)
 	assert.Contains(t, digestErrorString, err.Error())
-	err = dest.Commit(context.Background(), nil)
+	err = dest.Commit(context.Background(), nil) // nil unparsedToplevel is invalid, we don’t currently use the value
 	assert.NoError(t, err)
 
 	_, err = os.Lstat(blobPath)
@@ -142,40 +142,35 @@ func TestGetPutSignatures(t *testing.T) {
 	ref, tmpDir := refToTempDir(t)
 	defer os.RemoveAll(tmpDir)
 
-	dest, err := ref.NewImageDestination(context.Background(), nil)
-	require.NoError(t, err)
-	defer dest.Close()
 	man := []byte("test-manifest")
+	list := []byte("test-manifest-list")
+	md, err := manifest.Digest(man)
+	require.NoError(t, err)
 	signatures := [][]byte{
 		[]byte("sig1"),
 		[]byte("sig2"),
 	}
-	err = dest.SupportsSignatures(context.Background())
-	assert.NoError(t, err)
-	err = dest.PutManifest(context.Background(), man, nil)
-	require.NoError(t, err)
-
-	err = dest.PutSignatures(context.Background(), signatures, nil)
-	require.NoError(t, err)
 	listSignatures := [][]byte{
 		[]byte("sig3"),
 		[]byte("sig4"),
 	}
-	md, err := manifest.Digest(man)
-	require.NoError(t, err)
 
+	dest, err := ref.NewImageDestination(context.Background(), nil)
+	require.NoError(t, err)
+	defer dest.Close()
 	err = dest.SupportsSignatures(context.Background())
 	assert.NoError(t, err)
-	err = dest.PutManifest(context.Background(), man, nil)
-	require.NoError(t, err)
+
 	err = dest.PutManifest(context.Background(), man, &md)
 	require.NoError(t, err)
+	err = dest.PutManifest(context.Background(), list, nil)
+	require.NoError(t, err)
 
-	err = dest.PutSignatures(context.Background(), listSignatures, nil)
-	assert.NoError(t, err)
 	err = dest.PutSignatures(context.Background(), signatures, &md)
 	assert.NoError(t, err)
-	err = dest.Commit(context.Background(), nil)
+	err = dest.PutSignatures(context.Background(), listSignatures, nil)
+	assert.NoError(t, err)
+	err = dest.Commit(context.Background(), nil) // nil unparsedToplevel is invalid, we don’t currently use the value
 	assert.NoError(t, err)
 
 	src, err := ref.NewImageSource(context.Background(), nil)

directory/directory_transport_test.go

@@ -160,7 +160,7 @@ func TestReferenceNewImage(t *testing.T) {
 	require.NoError(t, err)
 	err = dest.PutManifest(context.Background(), mFixture, nil)
 	assert.NoError(t, err)
-	err = dest.Commit(context.Background(), nil)
+	err = dest.Commit(context.Background(), nil) // nil unparsedToplevel is invalid, we don’t currently use the value
 	assert.NoError(t, err)
 
 	img, err := ref.NewImage(context.Background(), nil)
@@ -177,7 +177,7 @@ func TestReferenceNewImageNoValidManifest(t *testing.T) {
 	defer dest.Close()
 	err = dest.PutManifest(context.Background(), []byte(`{"schemaVersion":1}`), nil)
 	assert.NoError(t, err)
-	err = dest.Commit(context.Background(), nil)
+	err = dest.Commit(context.Background(), nil) // nil unparsedToplevel is invalid, we don’t currently use the value
 	assert.NoError(t, err)
 
 	_, err = ref.NewImage(context.Background(), nil)

docker/archive/dest.go

@@ -67,6 +67,9 @@ func (d *archiveImageDestination) Close() error {
 }
 
 // Commit marks the process of storing the image as successful and asks for the image to be persisted.
+// unparsedToplevel contains data about the top-level manifest of the source (which may be a single-arch image or a manifest list
+// if PutManifest was only called for the single-arch image with instanceDigest == nil), primarily to allow lookups by the
+// original manifest list digest, if desired.
 // WARNING: This does not have any transactional semantics:
 // - Uploaded data MAY be visible to others before Commit() is called
 // - Uploaded data MAY be removed or MAY remain around if Close() is called without Commit() (i.e. rollback is allowed but not guaranteed)