diff --git a/image/copy/multiple.go b/image/copy/multiple.go index 85bba72885..9ab82f9bb0 100644 --- a/image/copy/multiple.go +++ b/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 { diff --git a/image/internal/manifest/docker_schema2_list.go b/image/internal/manifest/docker_schema2_list.go index 47a5699fb1..af4cda5bdb 100644 --- a/image/internal/manifest/docker_schema2_list.go +++ b/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) { diff --git a/image/internal/manifest/docker_schema2_list_test.go b/image/internal/manifest/docker_schema2_list_test.go index 9bcd9682ee..e347456658 100644 --- a/image/internal/manifest/docker_schema2_list_test.go +++ b/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 diff --git a/image/internal/manifest/list.go b/image/internal/manifest/list.go index 100d1c86b9..a0d535891e 100644 --- a/image/internal/manifest/list.go +++ b/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. diff --git a/image/internal/manifest/oci_index.go b/image/internal/manifest/oci_index.go index 922c8754c9..4e4060255a 100644 --- a/image/internal/manifest/oci_index.go +++ b/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. diff --git a/image/internal/manifest/oci_index_test.go b/image/internal/manifest/oci_index_test.go index 06e55cc08a..9045f1ed4c 100644 --- a/image/internal/manifest/oci_index_test.go +++ b/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) {