Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix panic in manifest annotate --index #24776

Merged
merged 2 commits into from
Dec 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/podman/manifest/annotate.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
10 changes: 8 additions & 2 deletions pkg/api/handlers/libpod/manifests.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()))
Expand Down
21 changes: 11 additions & 10 deletions pkg/bindings/manifests/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Arch *string // Arch overrides the architecture for the image
Features []string // Feature list for the image
OS *string // OS overrides the operating system for the image
OSFeatures []string `json:"os_features" schema:"os_features"` // OSFeatures overrides the OS features 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
Annotations map[string]string // Annotations to add to the entries for Images in the manifest list
IndexAnnotations map[string]string `json:"index_annotations" schema:"index_annotations"` // Annotations to add to the manifest list as a whole
Arch *string // Arch overrides the architecture for the image
Features []string // Feature list for the image
OS *string // OS overrides the operating system for the image
OSFeatures []string `json:"os_features" schema:"os_features"` // OSFeatures overrides the OS features 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
Expand Down
15 changes: 15 additions & 0 deletions pkg/bindings/manifests/types_modify_options.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions pkg/domain/entities/manifest.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
}
Expand Down
29 changes: 14 additions & 15 deletions pkg/domain/infra/abi/manifest.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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)
for _, annotationSpec := range opts.Annotation {
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)

if annotateOptions.Annotations, err = mergeAnnotations(opts.Annotations, opts.Annotation); err != nil {
return "", err
}

if annotateOptions.IndexAnnotations, err = mergeAnnotations(opts.IndexAnnotations, opts.IndexAnnotation); err != nil {
return "", err
}
annotateOptions.Annotations = opts.Annotations

if err := manifestList.AnnotateInstance(instanceDigest, annotateOptions); err != nil {
return "", err
Expand Down Expand Up @@ -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
}

Expand Down
39 changes: 28 additions & 11 deletions pkg/domain/infra/tunnel/manifest.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 := make(map[string]string)
for _, annotationSpec := range opts.Annotation {
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)
annotations, err := mergeAnnotations(opts.Annotations, opts.Annotation)
if err != nil {
return "", err
}
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 {
Expand All @@ -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)
Expand Down
50 changes: 46 additions & 4 deletions test/e2e/manifest_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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})
Expand All @@ -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"`))
// Check added annotation
Expect(session.OutputToString()).To(ContainSubstring(`"hello": "world,withcomma"`))
// Extract the digest from the name of the image that we just added to the list
ref, err := alltransports.ParseImageName(imageListInstance)
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() {
Expand Down
Loading