Merge pull request #24776 from nalind/manifest-annotate-index

Fix panic in `manifest annotate --index`

cmd/podman/manifest/annotate.go

@@ -119,7 +119,7 @@ func annotate(cmd *cobra.Command, args []string) error {
 	} else {
 		opts.Annotations = annotations
 	}
-	id, err := registry.ImageEngine().ManifestAnnotate(registry.Context(), args[0], args[1], opts)
+	id, err := registry.ImageEngine().ManifestAnnotate(registry.Context(), listImageSpec, instanceSpec, opts)
 	if err != nil {
 		return err
 	}

pkg/api/handlers/libpod/manifests.go

@@ -706,14 +706,20 @@ func ManifestModify(w http.ResponseWriter, r *http.Request) {
 		}
 	case strings.EqualFold("annotate", body.Operation):
 		options := body.ManifestAnnotateOptions
-		for _, image := range body.Images {
+		images := []string{""}
+		if len(body.Images) > 0 {
+			images = body.Images
+		}
+		for _, image := range images {
 			id, err := imageEngine.ManifestAnnotate(r.Context(), name, image, options)
 			if err != nil {
 				report.Errors = append(report.Errors, err)
 				continue
 			}
 			report.ID = id
-			report.Images = append(report.Images, image)
+			if image != "" {
+				report.Images = append(report.Images, image)
+			}
 		}
 	default:
 		utils.Error(w, http.StatusBadRequest, fmt.Errorf("illegal operation %q for %q", body.Operation, r.URL.String()))

pkg/bindings/manifests/types.go

@@ -19,7 +19,7 @@ type InspectOptions struct {
 type CreateOptions struct {
 	All        *bool
 	Amend      *bool
-	Annotation map[string]string
+	Annotation map[string]string `json:"annotations" schema:"annotations"`
 }
 
 // ExistsOptions are optional options for checking
@@ -35,7 +35,7 @@ type ExistsOptions struct {
 type AddOptions struct {
 	All *bool
 
-	Annotation map[string]string
+	Annotation map[string]string `json:"annotations" schema:"annotations"`
 	Arch       *string
 	Features   []string
 	OS         *string
@@ -54,7 +54,7 @@ type AddOptions struct {
 //
 //go:generate go run ../generator/generator.go AddArtifactOptions
 type AddArtifactOptions struct {
-	Annotation map[string]string
+	Annotation map[string]string `json:"annotations" schema:"annotations"`
 	Arch       *string
 	Features   []string
 	OS         *string
@@ -87,13 +87,14 @@ type ModifyOptions struct {
 	Operation *string
 	All       *bool // All when true, operate on all images in a manifest list that may be included in Images
 
-	Annotations map[string]string // Annotations to add to the entries for Images in the manifest list
+	Annotations      map[string]string // Annotations to add to the entries for Images in the manifest list
-	Arch        *string           // Arch overrides the architecture for the image
+	IndexAnnotations map[string]string `json:"index_annotations" schema:"index_annotations"` // Annotations to add to the manifest list as a whole
-	Features    []string          // Feature list for the image
+	Arch             *string           // Arch overrides the architecture for the image
-	OS          *string           // OS overrides the operating system for the image
+	Features         []string          // Feature list for the image
-	OSFeatures  []string          `json:"os_features" schema:"os_features"` // OSFeatures overrides the OS features for the image
+	OS               *string           // OS overrides the operating system for the image
-	OSVersion   *string           `json:"os_version" schema:"os_version"`   // OSVersion overrides the operating system version for the image
+	OSFeatures       []string          `json:"os_features" schema:"os_features"` // OSFeatures overrides the OS features for the image
-	Variant     *string           // Variant overrides the architecture variant for the image
+	OSVersion        *string           `json:"os_version" schema:"os_version"`   // OSVersion overrides the operating system version for the image
+	Variant          *string           // Variant overrides the architecture variant for the image
 
 	Images        []string // Images is an optional list of images to add/remove to/from manifest list depending on operation
 	Authfile      *string

pkg/bindings/manifests/types_modify_options.go

@@ -63,6 +63,21 @@ func (o *ModifyOptions) GetAnnotations() map[string]string {
 	return o.Annotations
 }
 
+// WithIndexAnnotations set annotations to add to the manifest list as a whole
+func (o *ModifyOptions) WithIndexAnnotations(value map[string]string) *ModifyOptions {
+	o.IndexAnnotations = value
+	return o
+}
+
+// GetIndexAnnotations returns value of annotations to add to the manifest list as a whole
+func (o *ModifyOptions) GetIndexAnnotations() map[string]string {
+	if o.IndexAnnotations == nil {
+		var z map[string]string
+		return z
+	}
+	return o.IndexAnnotations
+}
+
 // WithArch set arch overrides the architecture for the image
 func (o *ModifyOptions) WithArch(value string) *ModifyOptions {
 	o.Arch = &value

pkg/domain/entities/manifest.go

@@ -82,9 +82,9 @@ type ManifestAnnotateOptions struct {
 	// Variant for the item in the manifest list
 	Variant string `json:"variant" schema:"variant"`
 	// IndexAnnotation is a slice of key=value annotations to add to the manifest list itself
-	IndexAnnotation []string `json:"index_annotation" schema:"annotation"`
+	IndexAnnotation []string `json:"index_annotation" schema:"index_annotation"`
 	// IndexAnnotations is a map of key:value annotations to add to the manifest list itself, by a map which is preferred over IndexAnnotation
-	IndexAnnotations map[string]string `json:"index_annotations" schema:"annotations"`
+	IndexAnnotations map[string]string `json:"index_annotations" schema:"index_annotations"`
 	// IndexSubject is a subject value to set in the manifest list itself
 	IndexSubject string `json:"subject" schema:"subject"`
 }

pkg/domain/infra/abi/manifest.go

@@ -49,9 +49,10 @@ func (ir *ImageEngine) ManifestCreate(ctx context.Context, name string, images [
 		}
 	}
 
-	annotateOptions := &libimage.ManifestListAnnotateOptions{}
 	if len(opts.Annotations) != 0 {
-		annotateOptions.IndexAnnotations = opts.Annotations
+		annotateOptions := &libimage.ManifestListAnnotateOptions{
+			IndexAnnotations: opts.Annotations,
+		}
 		if err := manifestList.AnnotateInstance("", annotateOptions); err != nil {
 			return "", err
 		}
@@ -230,18 +231,14 @@ func (ir *ImageEngine) ManifestAdd(ctx context.Context, name string, images []st
 			Variant:      opts.Variant,
 			Subject:      opts.IndexSubject,
 		}
-		if len(opts.Annotation) != 0 {
+
-			annotations := make(map[string]string)
+		if annotateOptions.Annotations, err = mergeAnnotations(opts.Annotations, opts.Annotation); err != nil {
-			for _, annotationSpec := range opts.Annotation {
+			return "", err
-				key, val, hasVal := strings.Cut(annotationSpec, "=")
+		}
-				if !hasVal {
+
-					return "", fmt.Errorf("no value given for annotation %q", key)
+		if annotateOptions.IndexAnnotations, err = mergeAnnotations(opts.IndexAnnotations, opts.IndexAnnotation); err != nil {
-				}
+			return "", err
-				annotations[key] = val
-			}
-			opts.Annotations = envLib.Join(opts.Annotations, annotations)
 		}
-		annotateOptions.Annotations = opts.Annotations
 
 		if err := manifestList.AnnotateInstance(instanceDigest, annotateOptions); err != nil {
 			return "", err
@@ -380,10 +377,12 @@ func (ir *ImageEngine) ManifestAddArtifact(ctx context.Context, name string, fil
 		Variant:      opts.Variant,
 		Subject:      opts.IndexSubject,
 	}
-	if annotateOptions.Annotations, err = mergeAnnotations(opts.Annotations, opts.Annotation); err != nil {
+
+	if annotateOptions.Annotations, err = mergeAnnotations(opts.ManifestAnnotateOptions.Annotations, opts.ManifestAnnotateOptions.Annotation); err != nil {
 		return "", err
 	}
-	if annotateOptions.IndexAnnotations, err = mergeAnnotations(opts.IndexAnnotations, opts.IndexAnnotation); err != nil {
+
+	if annotateOptions.IndexAnnotations, err = mergeAnnotations(opts.ManifestAnnotateOptions.IndexAnnotations, opts.ManifestAnnotateOptions.IndexAnnotation); err != nil {
 		return "", err
 	}
 

pkg/domain/infra/tunnel/manifest.go

@@ -131,18 +131,17 @@ func (ir *ImageEngine) ManifestAnnotate(ctx context.Context, name, images string
 	options := new(manifests.ModifyOptions).WithArch(opts.Arch).WithVariant(opts.Variant)
 	options.WithFeatures(opts.Features).WithOS(opts.OS).WithOSVersion(opts.OSVersion)
 
-	if len(opts.Annotation) != 0 {
+	annotations, err := mergeAnnotations(opts.Annotations, opts.Annotation)
-		annotations := make(map[string]string)
+	if err != nil {
-		for _, annotationSpec := range opts.Annotation {
+		return "", err
-			key, val, hasVal := strings.Cut(annotationSpec, "=")
-			if !hasVal {
-				return "", fmt.Errorf("no value given for annotation %q", key)
-			}
-			annotations[key] = val
-		}
-		opts.Annotations = envLib.Join(opts.Annotations, annotations)
 	}
-	options.WithAnnotations(opts.Annotations)
+	options.WithAnnotations(annotations)
+
+	indexAnnotations, err := mergeAnnotations(opts.IndexAnnotations, opts.IndexAnnotation)
+	if err != nil {
+		return "", err
+	}
+	options.WithIndexAnnotations(indexAnnotations)
 
 	id, err := manifests.Annotate(ir.ClientCtx, name, []string{images}, options)
 	if err != nil {
@@ -151,6 +150,24 @@ func (ir *ImageEngine) ManifestAnnotate(ctx context.Context, name, images string
 	return id, nil
 }
 
+func mergeAnnotations(preferred map[string]string, aux []string) (map[string]string, error) {
+	if len(aux) != 0 {
+		auxAnnotations := make(map[string]string)
+		for _, annotationSpec := range aux {
+			key, val, hasVal := strings.Cut(annotationSpec, "=")
+			if !hasVal {
+				return nil, fmt.Errorf("no value given for annotation %q", key)
+			}
+			auxAnnotations[key] = val
+		}
+		if preferred == nil {
+			preferred = make(map[string]string)
+		}
+		preferred = envLib.Join(auxAnnotations, preferred)
+	}
+	return preferred, nil
+}
+
 // ManifestRemoveDigest removes the digest from manifest list
 func (ir *ImageEngine) ManifestRemoveDigest(ctx context.Context, name string, image string) (string, error) {
 	updatedListID, err := manifests.Remove(ir.ClientCtx, name, image, nil)

test/e2e/manifest_test.go

@@ -10,12 +10,16 @@ import (
 	"strings"
 
 	"github.com/containers/common/libimage/define"
+	"github.com/containers/image/v5/docker/reference"
+	manifest "github.com/containers/image/v5/manifest"
+	"github.com/containers/image/v5/transports/alltransports"
 	podmanRegistry "github.com/containers/podman/v5/hack/podman-registry-go"
 	. "github.com/containers/podman/v5/test/utils"
 	"github.com/containers/storage/pkg/archive"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	. "github.com/onsi/gomega/gexec"
+	imgspec "github.com/opencontainers/image-spec/specs-go"
 	imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1"
 )
 
@@ -368,7 +372,7 @@ add_compression = ["zstd"]`), 0o644)
 	})
 
 	It("annotate", func() {
-		session := podmanTest.Podman([]string{"manifest", "create", "foo"})
+		session := podmanTest.Podman([]string{"manifest", "create", "--annotation", "up=down", "foo"})
 		session.WaitWithDefaultTimeout()
 		Expect(session).Should(ExitCleanly())
 		session = podmanTest.Podman([]string{"manifest", "add", "foo", imageListInstance})
@@ -377,12 +381,50 @@ add_compression = ["zstd"]`), 0o644)
 		session = podmanTest.Podman([]string{"manifest", "annotate", "--annotation", "hello=world,withcomma", "--arch", "bar", "foo", imageListARM64InstanceDigest})
 		session.WaitWithDefaultTimeout()
 		Expect(session).Should(ExitCleanly())
+		session = podmanTest.Podman([]string{"manifest", "annotate", "--index", "--annotation", "top=bottom", "foo"})
+		session.WaitWithDefaultTimeout()
+		Expect(session).Should(ExitCleanly())
 		session = podmanTest.Podman([]string{"manifest", "inspect", "foo"})
 		session.WaitWithDefaultTimeout()
 		Expect(session).Should(ExitCleanly())
-		Expect(session.OutputToString()).To(ContainSubstring(`"architecture": "bar"`))
+		// Extract the digest from the name of the image that we just added to the list
-		// Check added annotation
+		ref, err := alltransports.ParseImageName(imageListInstance)
-		Expect(session.OutputToString()).To(ContainSubstring(`"hello": "world,withcomma"`))
+		Expect(err).ToNot(HaveOccurred())
+		dockerReference := ref.DockerReference()
+		referenceWithDigest, ok := dockerReference.(reference.Canonical)
+		Expect(ok).To(BeTrueBecause("we started with a canonical reference"))
+		// Check that the index has all of the information we've just added to it
+		encoded, err := json.Marshal(&imgspecv1.Index{
+			Versioned: imgspec.Versioned{
+				SchemaVersion: 2,
+			},
+			// media type forced because we need to be able to represent annotations
+			MediaType: imgspecv1.MediaTypeImageIndex,
+			Manifests: []imgspecv1.Descriptor{
+				{
+					// from imageListInstance
+					MediaType: manifest.DockerV2Schema2MediaType,
+					Digest:    referenceWithDigest.Digest(),
+					Size:      527,
+					// OS from imageListInstance(?), Architecture/Variant as above
+					Platform: &imgspecv1.Platform{
+						Architecture: "bar",
+						OS:           "linux",
+					},
+					// added above
+					Annotations: map[string]string{
+						"hello": "world,withcomma",
+					},
+				},
+			},
+			// added above
+			Annotations: map[string]string{
+				"top": "bottom",
+				"up":  "down",
+			},
+		})
+		Expect(err).ToNot(HaveOccurred())
+		Expect(session.OutputToString()).To(MatchJSON(encoded))
 	})
 
 	It("remove digest", func() {