Merge pull request #495 from mtrmac/zstd-annotation

Fix copying of non-Buildah Zstd images with --preserve-digests

Author: mtrmac

image/copy/multiple.go

@@ -216,6 +216,7 @@ func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte,
 	case imgspecv1.MediaTypeImageManifest:
 		forceListMIMEType = imgspecv1.MediaTypeImageIndex
 	}
+	// FIXME: This does not take into account cannotModifyManifestListReason.
 	selectedListType, otherManifestMIMETypeCandidates, err := c.determineListConversion(manifestType, c.dest.SupportedManifestMIMETypes(), forceListMIMEType)
 	if err != nil {
 		return nil, fmt.Errorf("determining manifest list type to write to destination: %w", err)
@@ -284,7 +285,7 @@ func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte,
 	}
 
 	// Now reset the digest/size/types of the manifests in the list to account for any conversions that we made.
-	if err = updatedList.EditInstances(instanceEdits); err != nil {
+	if err = updatedList.EditInstances(instanceEdits, cannotModifyManifestListReason != ""); err != nil {
 		return nil, fmt.Errorf("updating manifest list: %w", err)
 	}
 
@@ -318,7 +319,7 @@ func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte,
 		// If we can't just use the original value, but we have to change it, flag an error.
 		if !bytes.Equal(attemptedManifestList, originalManifestList) {
 			if cannotModifyManifestListReason != "" {
-				return nil, fmt.Errorf("Manifest list must be converted to type %q to be written to destination, but we cannot modify it: %q", thisListType, cannotModifyManifestListReason)
+				return nil, fmt.Errorf("Manifest list was edited, but we cannot modify it: %q", cannotModifyManifestListReason)
 			}
 			logrus.Debugf("Manifest list has been updated")
 		} else {

image/internal/manifest/docker_schema2_list.go

@@ -85,10 +85,13 @@ func (list *Schema2ListPublic) UpdateInstances(updates []ListUpdate) error {
 			ListOperation:   ListOpUpdate,
 		})
 	}
-	return list.editInstances(editInstances)
+	return list.editInstances(editInstances, false)
 }
 
-func (list *Schema2ListPublic) editInstances(editInstances []ListEdit) error {
+// editInstances edits information about the list's instances, based on instructions in editInstances.
+// If cannotModifyManifest, avoidable updates should be skipped.
+func (list *Schema2ListPublic) editInstances(editInstances []ListEdit, cannotModifyManifest bool) error {
+	_ = cannotModifyManifest // None of the edits we make are avoidable.
 	addedEntries := []Schema2ManifestDescriptor{}
 	for i, editInstance := range editInstances {
 		switch editInstance.ListOperation {
@@ -141,8 +144,10 @@ func (list *Schema2ListPublic) editInstances(editInstances []ListEdit) error {
 	return nil
 }
 
-func (list *Schema2List) EditInstances(editInstances []ListEdit) error {
-	return list.editInstances(editInstances)
+// EditInstances edits information about the list's instances, based on instructions in editInstances.
+// If cannotModifyManifest, avoidable updates should be skipped.
+func (list *Schema2List) EditInstances(editInstances []ListEdit, cannotModifyManifest bool) error {
+	return list.editInstances(editInstances, cannotModifyManifest)
 }
 
 func (list *Schema2ListPublic) ChooseInstanceByCompression(ctx *types.SystemContext, preferGzip types.OptionalBool) (digest.Digest, error) {

image/internal/manifest/docker_schema2_list_test.go

@@ -47,7 +47,7 @@ func TestSchema2ListEditInstances(t *testing.T) {
 		UpdateMediaType: "something",
 		ListOperation:   ListOpUpdate,
 	})
-	err = list.EditInstances(editInstances)
+	err = list.EditInstances(editInstances, false)
 	require.NoError(t, err)
 
 	expectedDigests[0] = editInstances[0].UpdateDigest
@@ -82,7 +82,7 @@ func TestSchema2ListEditInstances(t *testing.T) {
 		AddPlatform:   &imgspecv1.Platform{Architecture: "amd64", OS: "linux", OSFeatures: []string{"sse4"}},
 		ListOperation: ListOpAdd,
 	})
-	err = list.EditInstances(editInstances)
+	err = list.EditInstances(editInstances, false)
 	require.NoError(t, err)
 
 	// Verify new elements are added to the end of old list

image/internal/manifest/list.go

@@ -56,10 +56,9 @@ type List interface {
 	// SystemContext ( or for the current platform if the SystemContext doesn't specify any detail ) and preferGzip for compression which
 	// when configured to OptionalBoolTrue and chooses best available compression when it is OptionalBoolFalse or left OptionalBoolUndefined.
 	ChooseInstanceByCompression(ctx *types.SystemContext, preferGzip types.OptionalBool) (digest.Digest, error)
-	// Edit information about the list's instances. Contains Slice of ListEdit where each element
-	// is responsible for either Modifying or Adding a new instance to the Manifest. Operation is
-	// selected on the basis of configured ListOperation field.
-	EditInstances([]ListEdit) error
+	// EditInstances edits information about the list's instances, based on instructions in editInstances.
+	// If cannotModifyManifest, avoidable updates should be skipped.
+	EditInstances(editInstances []ListEdit, cannotModifyManifest bool) error
 }
 
 // ListUpdate includes the fields which a List's UpdateInstances() method will modify.

image/internal/manifest/oci_index.go

@@ -82,7 +82,7 @@ func (index *OCI1IndexPublic) UpdateInstances(updates []ListUpdate) error {
 			ListOperation:   ListOpUpdate,
 		})
 	}
-	return index.editInstances(editInstances)
+	return index.editInstances(editInstances, false)
 }
 
 func annotationsToCompressionAlgorithmNames(annotations map[string]string) []string {
@@ -97,7 +97,12 @@ func annotationsToCompressionAlgorithmNames(annotations map[string]string) []str
 	return result
 }
 
-func addCompressionAnnotations(compressionAlgorithms []compression.Algorithm, annotationsMap *map[string]string) {
+// addCompressionAnnotations updates annotationsMap for a manifest, based on compressionAlgorithms known to be used in that instance.
+// If cannotModifyManifest, avoidable updates should be skipped (i.e. this does nothing, because the algorithm annotation is “only” used for prioritization).
+func addCompressionAnnotations(compressionAlgorithms []compression.Algorithm, annotationsMap *map[string]string, cannotModifyManifest bool) {
+	if cannotModifyManifest {
+		return
+	}
 	// TODO: This should also delete the algorithm if map already contains an algorithm and compressionAlgorithm
 	// list has a different algorithm. To do that, we would need to modify the callers to always provide a reliable
 	// and full compressionAlghorithms list.
@@ -114,7 +119,9 @@ func addCompressionAnnotations(compressionAlgorithms []compression.Algorithm, an
 	}
 }
 
-func (index *OCI1IndexPublic) editInstances(editInstances []ListEdit) error {
+// editInstances edits information about the list's instances, based on instructions in editInstances.
+// If cannotModifyManifest, avoidable updates should be skipped.
+func (index *OCI1IndexPublic) editInstances(editInstances []ListEdit, cannotModifyManifest bool) error {
 	addedEntries := []imgspecv1.Descriptor{}
 	updatedAnnotations := false
 	for i, editInstance := range editInstances {
@@ -152,13 +159,15 @@ func (index *OCI1IndexPublic) editInstances(editInstances []ListEdit) error {
 					maps.Copy(index.Manifests[targetIndex].Annotations, editInstance.UpdateAnnotations)
 				}
 			}
-			addCompressionAnnotations(editInstance.UpdateCompressionAlgorithms, &index.Manifests[targetIndex].Annotations)
+			// FIXME: This does not set updatedAnnotations, so we don’t re-sort images with Zstd instances if some other
+			// tool created them without the OCI1InstanceAnnotationCompressionZSTD annotation.
+			addCompressionAnnotations(editInstance.UpdateCompressionAlgorithms, &index.Manifests[targetIndex].Annotations, cannotModifyManifest)
 		case ListOpAdd:
 			annotations := map[string]string{}
 			if editInstance.AddAnnotations != nil {
 				annotations = maps.Clone(editInstance.AddAnnotations)
 			}
-			addCompressionAnnotations(editInstance.AddCompressionAlgorithms, &annotations)
+			addCompressionAnnotations(editInstance.AddCompressionAlgorithms, &annotations, false) // We are adding a full new entry, so skipping the annotation would still not allow preserving the original manifest.
 			addedEntries = append(addedEntries, imgspecv1.Descriptor{
 				MediaType:    editInstance.AddMediaType,
 				ArtifactType: editInstance.AddArtifactType,
@@ -195,8 +204,10 @@ func (index *OCI1IndexPublic) editInstances(editInstances []ListEdit) error {
 	return nil
 }
 
-func (index *OCI1Index) EditInstances(editInstances []ListEdit) error {
-	return index.editInstances(editInstances)
+// EditInstances edits information about the list's instances, based on instructions in editInstances.
+// If cannotModifyManifest, avoidable updates should be skipped.
+func (index *OCI1Index) EditInstances(editInstances []ListEdit, cannotModifyManifest bool) error {
+	return index.editInstances(editInstances, cannotModifyManifest)
 }
 
 // instanceIsZstd returns true if instance is a zstd instance otherwise false.

image/internal/manifest/oci_index_test.go

@@ -68,7 +68,7 @@ func TestOCI1EditInstances(t *testing.T) {
 		UpdateMediaType: "something",
 		ListOperation:   ListOpUpdate,
 	})
-	err = list.EditInstances(editInstances)
+	err = list.EditInstances(editInstances, false)
 	require.NoError(t, err)
 
 	expectedDigests[0] = editInstances[0].UpdateDigest
@@ -135,7 +135,7 @@ func TestOCI1EditInstances(t *testing.T) {
 		AddPlatform:   &imgspecv1.Platform{Architecture: "amd64", OS: "linux", OSFeatures: []string{"sse4"}},
 		ListOperation: ListOpAdd,
 	})
-	err = list.EditInstances(editInstances)
+	err = list.EditInstances(editInstances, false)
 	require.NoError(t, err)
 
 	// Zstd should be kept on lowest priority as compared to the default gzip ones and order of prior elements must be preserved.
@@ -162,7 +162,7 @@ func TestOCI1EditInstances(t *testing.T) {
 		UpdateAnnotations:       map[string]string{},
 		ListOperation:           ListOpUpdate,
 	})
-	err = list.EditInstances(editInstances)
+	err = list.EditInstances(editInstances, false)
 	require.NoError(t, err)
 	// Digest `ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff` should be re-ordered on update.
 	assert.Equal(t, list.Instances(), []digest.Digest{digest.Digest("sha256:e692418e4cbaf90ca69d05a66403747baa33ee08806650b51fab815ad7fc331f"), digest.Digest("sha256:5b0bcabd1ed22e9fb1310cf6c2dec7cdef19f0ad69efa1f392e94a4333501270"), digest.Digest("sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"), digest.Digest("sha256:cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc"), digest.Digest("sha256:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"), digest.Digest("sha256:eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee"), digest.Digest("sha256:hhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhh")})
@@ -187,12 +187,47 @@ func TestOCI1EditInstances(t *testing.T) {
 		UpdateAffectAnnotations: true,
 		UpdateAnnotations:       map[string]string{},
 	}}
-	err = list.EditInstances(editInstances)
+	err = list.EditInstances(editInstances, false)
 	require.NoError(t, err)
 	// Verify that the artifactType wasn't lost.
 	instance, err = list.Instance(digest.Digest("sha256:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"))
 	require.NoError(t, err)
 	assert.Equal(t, "application/x-tar", instance.ReadOnly.ArtifactType)
+
+	// Updating the zstd annotation for an existing instance works
+	for _, cannotModifyManifest := range []bool{false, true} {
+		list, err = ListFromBlob(validManifest, GuessMIMEType(validManifest))
+		require.NoError(t, err)
+		originalInstance, err := list.Instance(list.Instances()[0])
+		require.NoError(t, err)
+		editInstances = []ListEdit{{
+			ListOperation:               ListOpUpdate,
+			UpdateOldDigest:             originalInstance.Digest,
+			UpdateDigest:                originalInstance.Digest,
+			UpdateSize:                  originalInstance.Size,
+			UpdateMediaType:             originalInstance.MediaType,
+			UpdateAffectAnnotations:     false,
+			UpdateAnnotations:           nil,
+			UpdateCompressionAlgorithms: []compression.Algorithm{compression.Zstd},
+		}}
+		err = list.EditInstances(editInstances, cannotModifyManifest)
+		require.NoError(t, err)
+		instance, err = list.Instance(list.Instances()[0])
+		require.NoError(t, err)
+
+		if cannotModifyManifest {
+			assert.Equal(t, originalInstance, instance) // No changes
+		} else {
+			assert.Equal(t, "true", instance.ReadOnly.Annotations["io.github.containers.compression.zstd"])
+			assert.Equal(t, []string{compressionTypes.ZstdAlgorithmName}, instance.ReadOnly.CompressionAlgorithmNames)
+			// These are the only changes:
+			delete(instance.ReadOnly.Annotations, "io.github.containers.compression.zstd")
+			require.Empty(t, instance.ReadOnly.Annotations)
+			instance.ReadOnly.Annotations = nil
+			instance.ReadOnly.CompressionAlgorithmNames = []string{compressionTypes.GzipAlgorithmName}
+			assert.Equal(t, originalInstance, instance)
+		}
+	}
 }
 
 func TestOCI1IndexChooseInstanceByCompression(t *testing.T) {