Refactor manifest list operations

* Update method/function signatures use the manifest list name and
  images associated with the operation explicitly, in general

  func f(ctx context.Context, manifestListName string,
         ImageNames []string, options *fOptions)

* Leverage gorilla/mux Subrouters to support API v3.x and v4.x for
  manifests
* Make manifest API endpoints more RESTful
* Add PUT /manifest/{id} to update existing manifests
* Add manifests.Annotate to go bindings, uncommented unit test
* Add DELETE /manifest/{Id} to remove existing manifest list, use
  PUT /manifest/{id} to remove images from a list
* Deprecated POST /manifest/{id}/add and /manifest/{id}/remove, use
  PUT /manifest/{id} instead
* Corrected swagger godoc and updated to cover API changes
* Update podman manifest commands to use registry.Context()
* Expose utils.GetVar() to obtain query parameters by name
* Unexpose server.registerSwaggerHandlers, not sure why this was ever
  exposed.
* Refactored code to use http.Header instead of map[string]string when
  operating on HTTP headers.
* Add API-Version header support in bindings to allow calling explicate
  versions of the API. Header is _NOT_ forwarded to the API service.

Signed-off-by: Jhon Honce <jhonce@redhat.com>

6 files changed, 125 insertions, 105 deletions

diff --git a/pkg/domain/entities/engine_image.go b/pkg/domain/entities/engine_image.go
index bec505163..bf9fcfd27 100644
--- a/pkg/domain/entities/engine_image.go
+++ b/pkg/domain/entities/engine_image.go
@@ -31,12 +31,12 @@ type ImageEngine interface {
 	Tree(ctx context.Context, nameOrID string, options ImageTreeOptions) (*ImageTreeReport, error)
 	Unmount(ctx context.Context, images []string, options ImageUnmountOptions) ([]*ImageUnmountReport, error)
 	Untag(ctx context.Context, nameOrID string, tags []string, options ImageUntagOptions) error
-	ManifestCreate(ctx context.Context, names, images []string, opts ManifestCreateOptions) (string, error)
+	ManifestCreate(ctx context.Context, name string, images []string, opts ManifestCreateOptions) (string, error)
 	ManifestExists(ctx context.Context, name string) (*BoolReport, error)
 	ManifestInspect(ctx context.Context, name string) ([]byte, error)
-	ManifestAdd(ctx context.Context, opts ManifestAddOptions) (string, error)
-	ManifestAnnotate(ctx context.Context, names []string, opts ManifestAnnotateOptions) (string, error)
-	ManifestRemove(ctx context.Context, names []string) (string, error)
+	ManifestAdd(ctx context.Context, listName string, imageNames []string, opts ManifestAddOptions) (string, error)
+	ManifestAnnotate(ctx context.Context, names, image string, opts ManifestAnnotateOptions) (string, error)
+	ManifestRemoveDigest(ctx context.Context, names, image string) (string, error)
 	ManifestRm(ctx context.Context, names []string) (*ImageRemoveReport, []error)
 	ManifestPush(ctx context.Context, name, destination string, imagePushOpts ImagePushOptions) (string, error)
 	Sign(ctx context.Context, names []string, options SignOptions) (*SignReport, error)
diff --git a/pkg/domain/entities/images.go b/pkg/domain/entities/images.go
index 62e7f67c8..bc41d7844 100644
--- a/pkg/domain/entities/images.go
+++ b/pkg/domain/entities/images.go
@@ -94,7 +94,7 @@ type ImageRemoveOptions struct {
 	LookupManifest bool
 }
 
-// ImageRemoveResponse is the response for removing one or more image(s) from storage
+// ImageRemoveReport is the response for removing one or more image(s) from storage
 // and images what was untagged vs actually removed.
 type ImageRemoveReport struct {
 	// Deleted images.
diff --git a/pkg/domain/entities/manifest.go b/pkg/domain/entities/manifest.go
index 3f89e4d30..81f3e837b 100644
--- a/pkg/domain/entities/manifest.go
+++ b/pkg/domain/entities/manifest.go
@@ -2,35 +2,79 @@ package entities
 
 import "github.com/containers/image/v5/types"
 
-// TODO: add comments to *all* types and fields.
-
+// ManifestCreateOptions provides model for creating manifest
 type ManifestCreateOptions struct {
 	All bool `schema:"all"`
 }
 
-// swagger:model ManifestAddOpts
+// ManifestAddOptions provides model for adding digests to manifest list
+//
+// swagger:model
 type ManifestAddOptions struct {
-	All           bool               `json:"all" schema:"all"`
-	Annotation    []string           `json:"annotation" schema:"annotation"`
-	Arch          string             `json:"arch" schema:"arch"`
-	Authfile      string             `json:"-" schema:"-"`
-	CertDir       string             `json:"-" schema:"-"`
-	Features      []string           `json:"features" schema:"features"`
-	Images        []string           `json:"images" schema:"images"`
-	OS            string             `json:"os" schema:"os"`
-	OSVersion     string             `json:"os_version" schema:"os_version"`
-	Password      string             `json:"-" schema:"-"`
+	ManifestAnnotateOptions
+	// True when operating on a list to include all images
+	All bool `json:"all" schema:"all"`
+	// authfile to use when pushing manifest list
+	Authfile string `json:"-" schema:"-"`
+	// Home directory for certificates when pushing a manifest list
+	CertDir string `json:"-" schema:"-"`
+	// Password to authenticate to registry when pushing manifest list
+	Password string `json:"-" schema:"-"`
+	// Should TLS registry certificate be verified?
 	SkipTLSVerify types.OptionalBool `json:"-" schema:"-"`
-	Username      string             `json:"-" schema:"-"`
-	Variant       string             `json:"variant" schema:"variant"`
+	// Username to authenticate to registry when pushing manifest list
+	Username string `json:"-" schema:"-"`
+	// Images is an optional list of images to add to manifest list
+	Images []string `json:"images" schema:"images"`
 }
 
+// ManifestAnnotateOptions provides model for annotating manifest list
 type ManifestAnnotateOptions struct {
-	Annotation []string `json:"annotation"`
-	Arch       string   `json:"arch" schema:"arch"`
-	Features   []string `json:"features" schema:"features"`
-	OS         string   `json:"os" schema:"os"`
+	// Annotation to add to manifest list
+	Annotation []string `json:"annotation" schema:"annotation"`
+	// Arch overrides the architecture for the image
+	Arch string `json:"arch" schema:"arch"`
+	// Feature list for the image
+	Features []string `json:"features" schema:"features"`
+	// OS overrides the operating system for the image
+	OS string `json:"os" schema:"os"`
+	// OS features for the image
 	OSFeatures []string `json:"os_features" schema:"os_features"`
-	OSVersion  string   `json:"os_version" schema:"os_version"`
-	Variant    string   `json:"variant" schema:"variant"`
+	// OSVersion overrides the operating system for the image
+	OSVersion string `json:"os_version" schema:"os_version"`
+	// Variant for the image
+	Variant string `json:"variant" schema:"variant"`
+}
+
+// ManifestModifyOptions provides the model for mutating a manifest
+//
+// swagger 2.0 does not support oneOf for schema validation.
+//
+// Operation "update" uses all fields.
+// Operation "remove" uses fields: Operation and Images
+// Operation "annotate" uses fields: Operation and Annotations
+//
+// swagger:model
+type ManifestModifyOptions struct {
+	Operation string `json:"operation" schema:"operation"` // Valid values: update, remove, annotate
+	ManifestAddOptions
+	ManifestRemoveOptions
+}
+
+// ManifestRemoveOptions provides the model for removing digests from a manifest
+//
+// swagger:model
+type ManifestRemoveOptions struct {
+}
+
+// ManifestModifyReport provides the model for removed digests and changed manifest
+//
+// swagger:model
+type ManifestModifyReport struct {
+	// Manifest List ID
+	ID string `json:"Id"`
+	// Images to removed from manifest list, otherwise not provided.
+	Images []string `json:"images,omitempty" schema:"images"`
+	// Errors associated with operation
+	Errors []error `json:"errors,omitempty"`
 }
diff --git a/pkg/domain/infra/abi/manifest.go b/pkg/domain/infra/abi/manifest.go
index d1bd5e2e4..d8733130b 100644
--- a/pkg/domain/infra/abi/manifest.go
+++ b/pkg/domain/infra/abi/manifest.go
@@ -22,13 +22,10 @@ import (
 )
 
 // ManifestCreate implements logic for creating manifest lists via ImageEngine
-func (ir *ImageEngine) ManifestCreate(ctx context.Context, names []string, images []string, opts entities.ManifestCreateOptions) (string, error) {
-	// FIXME: change the interface of manifest create `names []string` ->
-	// `name string`.
-	if len(names) == 0 {
+func (ir *ImageEngine) ManifestCreate(ctx context.Context, name string, images []string, opts entities.ManifestCreateOptions) (string, error) {
+	if len(name) == 0 {
 		return "", errors.New("no name specified for creating a manifest list")
 	}
-	name := names[0]
 
 	manifestList, err := ir.Libpod.LibimageRuntime().CreateManifestList(name)
 	if err != nil {
@@ -175,18 +172,12 @@ func (ir *ImageEngine) remoteManifestInspect(ctx context.Context, name string) (
 }
 
 // ManifestAdd adds images to the manifest list
-func (ir *ImageEngine) ManifestAdd(ctx context.Context, opts entities.ManifestAddOptions) (string, error) {
-	// FIXME: the name options below are *mandatory* arguments and should
-	// be reflected as such in the signature.
-
-	if len(opts.Images) < 2 {
-		return "", errors.New("manifest add requires two images")
+func (ir *ImageEngine) ManifestAdd(ctx context.Context, name string, images []string, opts entities.ManifestAddOptions) (string, error) {
+	if len(images) < 1 {
+		return "", errors.New("manifest add requires at least one image")
 	}
 
-	imageName := opts.Images[0]
-	listName := opts.Images[1]
-
-	manifestList, err := ir.Libpod.LibimageRuntime().LookupManifestList(listName)
+	manifestList, err := ir.Libpod.LibimageRuntime().LookupManifestList(name)
 	if err != nil {
 		return "", err
 	}
@@ -200,53 +191,46 @@ func (ir *ImageEngine) ManifestAdd(ctx context.Context, opts entities.ManifestAd
 		Password:              opts.Password,
 	}
 
-	instanceDigest, err := manifestList.Add(ctx, imageName, addOptions)
-	if err != nil {
-		return "", err
-	}
+	for _, image := range images {
+		instanceDigest, err := manifestList.Add(ctx, image, addOptions)
+		if err != nil {
+			return "", err
+		}
 
-	annotateOptions := &libimage.ManifestListAnnotateOptions{
-		Architecture: opts.Arch,
-		Features:     opts.Features,
-		OS:           opts.OS,
-		OSVersion:    opts.OSVersion,
-		Variant:      opts.Variant,
-	}
-	if len(opts.Annotation) != 0 {
-		annotations := make(map[string]string)
-		for _, annotationSpec := range opts.Annotation {
-			spec := strings.SplitN(annotationSpec, "=", 2)
-			if len(spec) != 2 {
-				return "", errors.Errorf("no value given for annotation %q", spec[0])
+		annotateOptions := &libimage.ManifestListAnnotateOptions{
+			Architecture: opts.Arch,
+			Features:     opts.Features,
+			OS:           opts.OS,
+			OSVersion:    opts.OSVersion,
+			Variant:      opts.Variant,
+		}
+		if len(opts.Annotation) != 0 {
+			annotations := make(map[string]string)
+			for _, annotationSpec := range opts.Annotation {
+				spec := strings.SplitN(annotationSpec, "=", 2)
+				if len(spec) != 2 {
+					return "", errors.Errorf("no value given for annotation %q", spec[0])
+				}
+				annotations[spec[0]] = spec[1]
 			}
-			annotations[spec[0]] = spec[1]
+			annotateOptions.Annotations = annotations
 		}
-		annotateOptions.Annotations = annotations
-	}
 
-	if err := manifestList.AnnotateInstance(instanceDigest, annotateOptions); err != nil {
-		return "", err
+		if err := manifestList.AnnotateInstance(instanceDigest, annotateOptions); err != nil {
+			return "", err
+		}
 	}
-
 	return manifestList.ID(), nil
 }
 
 // ManifestAnnotate updates an entry of the manifest list
-func (ir *ImageEngine) ManifestAnnotate(ctx context.Context, names []string, opts entities.ManifestAnnotateOptions) (string, error) {
-	// FIXME: the `names` are *mandatory* arguments and should be
-	// reflected as such in the signature.
-
-	if len(names) < 2 {
-		return "", errors.New("manifest annotate requires two names")
-	}
-
-	listName := names[0]
-	instanceDigest, err := digest.Parse(names[1])
+func (ir *ImageEngine) ManifestAnnotate(ctx context.Context, name, image string, opts entities.ManifestAnnotateOptions) (string, error) {
+	instanceDigest, err := digest.Parse(image)
 	if err != nil {
-		return "", errors.Errorf(`invalid image digest "%s": %v`, names[1], err)
+		return "", errors.Errorf(`invalid image digest "%s": %v`, image, err)
 	}
 
-	manifestList, err := ir.Libpod.LibimageRuntime().LookupManifestList(listName)
+	manifestList, err := ir.Libpod.LibimageRuntime().LookupManifestList(name)
 	if err != nil {
 		return "", err
 	}
@@ -277,22 +261,14 @@ func (ir *ImageEngine) ManifestAnnotate(ctx context.Context, names []string, opt
 	return manifestList.ID(), nil
 }
 
-// ManifestRemove removes specified digest from the specified manifest list
-func (ir *ImageEngine) ManifestRemove(ctx context.Context, names []string) (string, error) {
-	// FIXME: the `names` are *mandatory* arguments and should be
-	// reflected as such in the signature.
-
-	if len(names) < 2 {
-		return "", errors.New("manifest remove requires two names")
-	}
-
-	listName := names[0]
-	instanceDigest, err := digest.Parse(names[1])
+// ManifestRemoveDigest removes specified digest from the specified manifest list
+func (ir *ImageEngine) ManifestRemoveDigest(ctx context.Context, name, image string) (string, error) {
+	instanceDigest, err := digest.Parse(image)
 	if err != nil {
-		return "", errors.Errorf(`invalid image digest "%s": %v`, names[1], err)
+		return "", errors.Errorf(`invalid image digest "%s": %v`, image, err)
 	}
 
-	manifestList, err := ir.Libpod.LibimageRuntime().LookupManifestList(listName)
+	manifestList, err := ir.Libpod.LibimageRuntime().LookupManifestList(name)
 	if err != nil {
 		return "", err
 	}
diff --git a/pkg/domain/infra/runtime_abi.go b/pkg/domain/infra/runtime_abi.go
index 177e9cff4..ec7de2743 100644
--- a/pkg/domain/infra/runtime_abi.go
+++ b/pkg/domain/infra/runtime_abi.go
@@ -26,7 +26,7 @@ func NewContainerEngine(facts *entities.PodmanConfig) (entities.ContainerEngine,
 	return nil, fmt.Errorf("runtime mode '%v' is not supported", facts.EngineMode)
 }
 
-// NewContainerEngine factory provides a libpod runtime for image-related operations
+// NewImageEngine factory provides a libpod runtime for image-related operations
 func NewImageEngine(facts *entities.PodmanConfig) (entities.ImageEngine, error) {
 	switch facts.EngineMode {
 	case entities.ABIMode:
diff --git a/pkg/domain/infra/tunnel/manifest.go b/pkg/domain/infra/tunnel/manifest.go
index 62634f561..f89c59bc7 100644
--- a/pkg/domain/infra/tunnel/manifest.go
+++ b/pkg/domain/infra/tunnel/manifest.go
@@ -7,16 +7,16 @@ import (
 	"strings"
 
 	"github.com/containers/image/v5/types"
-	images "github.com/containers/podman/v3/pkg/bindings/images"
+	"github.com/containers/podman/v3/pkg/bindings/images"
 	"github.com/containers/podman/v3/pkg/bindings/manifests"
 	"github.com/containers/podman/v3/pkg/domain/entities"
 	"github.com/pkg/errors"
 )
 
 // ManifestCreate implements manifest create via ImageEngine
-func (ir *ImageEngine) ManifestCreate(ctx context.Context, names, images []string, opts entities.ManifestCreateOptions) (string, error) {
+func (ir *ImageEngine) ManifestCreate(ctx context.Context, name string, images []string, opts entities.ManifestCreateOptions) (string, error) {
 	options := new(manifests.CreateOptions).WithAll(opts.All)
-	imageID, err := manifests.Create(ir.ClientCtx, names, images, options)
+	imageID, err := manifests.Create(ir.ClientCtx, name, images, options)
 	if err != nil {
 		return imageID, errors.Wrapf(err, "error creating manifest")
 	}
@@ -33,7 +33,7 @@ func (ir *ImageEngine) ManifestExists(ctx context.Context, name string) (*entiti
 }
 
 // ManifestInspect returns contents of manifest list with given name
-func (ir *ImageEngine) ManifestInspect(ctx context.Context, name string) ([]byte, error) {
+func (ir *ImageEngine) ManifestInspect(_ context.Context, name string) ([]byte, error) {
 	list, err := manifests.Inspect(ir.ClientCtx, name, nil)
 	if err != nil {
 		return nil, errors.Wrapf(err, "error getting content of manifest list or image %s", name)
@@ -47,9 +47,9 @@ func (ir *ImageEngine) ManifestInspect(ctx context.Context, name string) ([]byte
 }
 
 // ManifestAdd adds images to the manifest list
-func (ir *ImageEngine) ManifestAdd(ctx context.Context, opts entities.ManifestAddOptions) (string, error) {
+func (ir *ImageEngine) ManifestAdd(_ context.Context, name string, imageNames []string, opts entities.ManifestAddOptions) (string, error) {
 	options := new(manifests.AddOptions).WithAll(opts.All).WithArch(opts.Arch).WithVariant(opts.Variant)
-	options.WithFeatures(opts.Features).WithImages(opts.Images).WithOS(opts.OS).WithOSVersion(opts.OSVersion)
+	options.WithFeatures(opts.Features).WithImages(imageNames).WithOS(opts.OS).WithOSVersion(opts.OSVersion)
 	if len(opts.Annotation) != 0 {
 		annotations := make(map[string]string)
 		for _, annotationSpec := range opts.Annotation {
@@ -62,25 +62,25 @@ func (ir *ImageEngine) ManifestAdd(ctx context.Context, opts entities.ManifestAd
 		options.WithAnnotation(annotations)
 	}
 
-	listID, err := manifests.Add(ir.ClientCtx, opts.Images[1], options)
+	id, err := manifests.Add(ir.ClientCtx, name, options)
 	if err != nil {
-		return listID, errors.Wrapf(err, "error adding to manifest list %s", opts.Images[1])
+		return id, errors.Wrapf(err, "error adding to manifest list %s", name)
 	}
-	return listID, nil
+	return id, nil
 }
 
 // ManifestAnnotate updates an entry of the manifest list
-func (ir *ImageEngine) ManifestAnnotate(ctx context.Context, names []string, opts entities.ManifestAnnotateOptions) (string, error) {
+func (ir *ImageEngine) ManifestAnnotate(ctx context.Context, name, images string, opts entities.ManifestAnnotateOptions) (string, error) {
 	return "", errors.New("not implemented")
 }
 
-// ManifestRemove removes the digest from manifest list
-func (ir *ImageEngine) ManifestRemove(ctx context.Context, names []string) (string, error) {
-	updatedListID, err := manifests.Remove(ir.ClientCtx, names[0], names[1], 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)
 	if err != nil {
-		return updatedListID, errors.Wrapf(err, "error removing from manifest %s", names[0])
+		return updatedListID, errors.Wrapf(err, "error removing from manifest %s", name)
 	}
-	return fmt.Sprintf("%s :%s\n", updatedListID, names[1]), nil
+	return fmt.Sprintf("%s :%s\n", updatedListID, image), nil
 }
 
 // ManifestRm removes the specified manifest list from storage