Refactor remote pull to provide progress

podman and podman-remote do not exactly match as the lower layer code
checks if the output is destined for a  TTY before creating the progress
bars.  A future PR for containers/images could change this behavior.

Fixes #7543

Tested with:

$ (echo '# start'; podman-remote pull nginx ) 2>&1 | ts '[%Y-%m-%d %H:%M:%.S]'
$ (echo '# start'; podman pull nginx ) 2>&1 | ts '[%Y-%m-%d %H:%M:%.S]'

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

5 files changed, 302 insertions, 171 deletions

diff --git a/pkg/api/handlers/libpod/images.go b/pkg/api/handlers/libpod/images.go
index 1da41ad88..bc1bdc287 100644
--- a/pkg/api/handlers/libpod/images.go
+++ b/pkg/api/handlers/libpod/images.go
@@ -11,8 +11,6 @@ import (
 	"strings"
 
 	"github.com/containers/buildah"
-	"github.com/containers/image/v5/docker"
-	"github.com/containers/image/v5/docker/reference"
 	"github.com/containers/image/v5/manifest"
 	"github.com/containers/image/v5/types"
 	"github.com/containers/podman/v2/libpod"
@@ -25,7 +23,6 @@ import (
 	"github.com/containers/podman/v2/pkg/domain/entities"
 	"github.com/containers/podman/v2/pkg/domain/infra/abi"
 	"github.com/containers/podman/v2/pkg/errorhandling"
-	"github.com/containers/podman/v2/pkg/util"
 	utils2 "github.com/containers/podman/v2/utils"
 	"github.com/gorilla/schema"
 	"github.com/pkg/errors"
@@ -400,123 +397,6 @@ func ImagesImport(w http.ResponseWriter, r *http.Request) {
 	utils.WriteResponse(w, http.StatusOK, entities.ImageImportReport{Id: importedImage})
 }
 
-// ImagesPull is the v2 libpod endpoint for pulling images.  Note that the
-// mandatory `reference` must be a reference to a registry (i.e., of docker
-// transport or be normalized to one).  Other transports are rejected as they
-// do not make sense in a remote context.
-func ImagesPull(w http.ResponseWriter, r *http.Request) {
-	runtime := r.Context().Value("runtime").(*libpod.Runtime)
-	decoder := r.Context().Value("decoder").(*schema.Decoder)
-	query := struct {
-		Reference       string `schema:"reference"`
-		OverrideOS      string `schema:"overrideOS"`
-		OverrideArch    string `schema:"overrideArch"`
-		OverrideVariant string `schema:"overrideVariant"`
-		TLSVerify       bool   `schema:"tlsVerify"`
-		AllTags         bool   `schema:"allTags"`
-	}{
-		TLSVerify: true,
-	}
-
-	if err := decoder.Decode(&query, r.URL.Query()); err != nil {
-		utils.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest,
-			errors.Wrapf(err, "failed to parse parameters for %s", r.URL.String()))
-		return
-	}
-
-	if len(query.Reference) == 0 {
-		utils.InternalServerError(w, errors.New("reference parameter cannot be empty"))
-		return
-	}
-
-	imageRef, err := utils.ParseDockerReference(query.Reference)
-	if err != nil {
-		utils.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest,
-			errors.Wrapf(err, "image destination %q is not a docker-transport reference", query.Reference))
-		return
-	}
-
-	// Trim the docker-transport prefix.
-	rawImage := strings.TrimPrefix(query.Reference, fmt.Sprintf("%s://", docker.Transport.Name()))
-
-	// all-tags doesn't work with a tagged reference, so let's check early
-	namedRef, err := reference.Parse(rawImage)
-	if err != nil {
-		utils.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest,
-			errors.Wrapf(err, "error parsing reference %q", rawImage))
-		return
-	}
-	if _, isTagged := namedRef.(reference.Tagged); isTagged && query.AllTags {
-		utils.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest,
-			errors.Errorf("reference %q must not have a tag for all-tags", rawImage))
-		return
-	}
-
-	authConf, authfile, err := auth.GetCredentials(r)
-	if err != nil {
-		utils.Error(w, "Something went wrong.", http.StatusBadRequest, errors.Wrapf(err, "Failed to parse %q header for %s", auth.XRegistryAuthHeader, r.URL.String()))
-		return
-	}
-	defer auth.RemoveAuthfile(authfile)
-
-	// Setup the registry options
-	dockerRegistryOptions := image.DockerRegistryOptions{
-		DockerRegistryCreds: authConf,
-		OSChoice:            query.OverrideOS,
-		ArchitectureChoice:  query.OverrideArch,
-		VariantChoice:       query.OverrideVariant,
-	}
-	if _, found := r.URL.Query()["tlsVerify"]; found {
-		dockerRegistryOptions.DockerInsecureSkipTLSVerify = types.NewOptionalBool(!query.TLSVerify)
-	}
-
-	sys := runtime.SystemContext()
-	if sys == nil {
-		sys = image.GetSystemContext("", authfile, false)
-	}
-	dockerRegistryOptions.DockerCertPath = sys.DockerCertPath
-	sys.DockerAuthConfig = authConf
-
-	// Prepare the images we want to pull
-	imagesToPull := []string{}
-	res := []handlers.LibpodImagesPullReport{}
-	imageName := namedRef.String()
-
-	if !query.AllTags {
-		imagesToPull = append(imagesToPull, imageName)
-	} else {
-		tags, err := docker.GetRepositoryTags(context.Background(), sys, imageRef)
-		if err != nil {
-			utils.InternalServerError(w, errors.Wrap(err, "error getting repository tags"))
-			return
-		}
-		for _, tag := range tags {
-			imagesToPull = append(imagesToPull, fmt.Sprintf("%s:%s", imageName, tag))
-		}
-	}
-
-	// Finally pull the images
-	for _, img := range imagesToPull {
-		newImage, err := runtime.ImageRuntime().New(
-			context.Background(),
-			img,
-			"",
-			authfile,
-			os.Stderr,
-			&dockerRegistryOptions,
-			image.SigningOptions{},
-			nil,
-			util.PullImageAlways)
-		if err != nil {
-			utils.InternalServerError(w, err)
-			return
-		}
-		res = append(res, handlers.LibpodImagesPullReport{ID: newImage.ID()})
-	}
-
-	utils.WriteResponse(w, http.StatusOK, res)
-}
-
 // PushImage is the handler for the compat http endpoint for pushing images.
 func PushImage(w http.ResponseWriter, r *http.Request) {
 	decoder := r.Context().Value("decoder").(*schema.Decoder)
diff --git a/pkg/api/handlers/libpod/images_pull.go b/pkg/api/handlers/libpod/images_pull.go
new file mode 100644
index 000000000..8a2f4f4cf
--- /dev/null
+++ b/pkg/api/handlers/libpod/images_pull.go
@@ -0,0 +1,193 @@
+package libpod
+
+import (
+	"context"
+	"encoding/json"
+	"fmt"
+	"net/http"
+	"strings"
+
+	"github.com/containers/image/v5/docker"
+	"github.com/containers/image/v5/docker/reference"
+	"github.com/containers/image/v5/types"
+	"github.com/containers/podman/v2/libpod"
+	"github.com/containers/podman/v2/libpod/image"
+	"github.com/containers/podman/v2/pkg/api/handlers/utils"
+	"github.com/containers/podman/v2/pkg/auth"
+	"github.com/containers/podman/v2/pkg/channel"
+	"github.com/containers/podman/v2/pkg/domain/entities"
+	"github.com/containers/podman/v2/pkg/util"
+	"github.com/gorilla/schema"
+	"github.com/pkg/errors"
+	"github.com/sirupsen/logrus"
+)
+
+// ImagesPull is the v2 libpod endpoint for pulling images.  Note that the
+// mandatory `reference` must be a reference to a registry (i.e., of docker
+// transport or be normalized to one).  Other transports are rejected as they
+// do not make sense in a remote context.
+func ImagesPull(w http.ResponseWriter, r *http.Request) {
+	runtime := r.Context().Value("runtime").(*libpod.Runtime)
+	decoder := r.Context().Value("decoder").(*schema.Decoder)
+	query := struct {
+		Reference       string `schema:"reference"`
+		OverrideOS      string `schema:"overrideOS"`
+		OverrideArch    string `schema:"overrideArch"`
+		OverrideVariant string `schema:"overrideVariant"`
+		TLSVerify       bool   `schema:"tlsVerify"`
+		AllTags         bool   `schema:"allTags"`
+	}{
+		TLSVerify: true,
+	}
+
+	if err := decoder.Decode(&query, r.URL.Query()); err != nil {
+		utils.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest,
+			errors.Wrapf(err, "failed to parse parameters for %s", r.URL.String()))
+		return
+	}
+
+	if len(query.Reference) == 0 {
+		utils.InternalServerError(w, errors.New("reference parameter cannot be empty"))
+		return
+	}
+
+	imageRef, err := utils.ParseDockerReference(query.Reference)
+	if err != nil {
+		utils.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest,
+			errors.Wrapf(err, "image destination %q is not a docker-transport reference", query.Reference))
+		return
+	}
+
+	// Trim the docker-transport prefix.
+	rawImage := strings.TrimPrefix(query.Reference, fmt.Sprintf("%s://", docker.Transport.Name()))
+
+	// all-tags doesn't work with a tagged reference, so let's check early
+	namedRef, err := reference.Parse(rawImage)
+	if err != nil {
+		utils.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest,
+			errors.Wrapf(err, "error parsing reference %q", rawImage))
+		return
+	}
+	if _, isTagged := namedRef.(reference.Tagged); isTagged && query.AllTags {
+		utils.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest,
+			errors.Errorf("reference %q must not have a tag for all-tags", rawImage))
+		return
+	}
+
+	authConf, authfile, err := auth.GetCredentials(r)
+	if err != nil {
+		utils.Error(w, "Something went wrong.", http.StatusBadRequest, errors.Wrapf(err, "Failed to parse %q header for %s", auth.XRegistryAuthHeader, r.URL.String()))
+		return
+	}
+	defer auth.RemoveAuthfile(authfile)
+
+	// Setup the registry options
+	dockerRegistryOptions := image.DockerRegistryOptions{
+		DockerRegistryCreds: authConf,
+		OSChoice:            query.OverrideOS,
+		ArchitectureChoice:  query.OverrideArch,
+		VariantChoice:       query.OverrideVariant,
+	}
+	if _, found := r.URL.Query()["tlsVerify"]; found {
+		dockerRegistryOptions.DockerInsecureSkipTLSVerify = types.NewOptionalBool(!query.TLSVerify)
+	}
+
+	sys := runtime.SystemContext()
+	if sys == nil {
+		sys = image.GetSystemContext("", authfile, false)
+	}
+	dockerRegistryOptions.DockerCertPath = sys.DockerCertPath
+	sys.DockerAuthConfig = authConf
+
+	// Prepare the images we want to pull
+	imagesToPull := []string{}
+	imageName := namedRef.String()
+
+	if !query.AllTags {
+		imagesToPull = append(imagesToPull, imageName)
+	} else {
+		tags, err := docker.GetRepositoryTags(context.Background(), sys, imageRef)
+		if err != nil {
+			utils.InternalServerError(w, errors.Wrap(err, "error getting repository tags"))
+			return
+		}
+		for _, tag := range tags {
+			imagesToPull = append(imagesToPull, fmt.Sprintf("%s:%s", imageName, tag))
+		}
+	}
+
+	writer := channel.NewWriter(make(chan []byte, 1))
+	defer writer.Close()
+
+	stderr := channel.NewWriter(make(chan []byte, 1))
+	defer stderr.Close()
+
+	images := make([]string, 0, len(imagesToPull))
+	runCtx, cancel := context.WithCancel(context.Background())
+	go func(imgs []string) {
+		defer cancel()
+		// Finally pull the images
+		for _, img := range imgs {
+			newImage, err := runtime.ImageRuntime().New(
+				runCtx,
+				img,
+				"",
+				authfile,
+				writer,
+				&dockerRegistryOptions,
+				image.SigningOptions{},
+				nil,
+				util.PullImageAlways)
+			if err != nil {
+				stderr.Write([]byte(err.Error() + "\n"))
+			} else {
+				images = append(images, newImage.ID())
+			}
+		}
+	}(imagesToPull)
+
+	flush := func() {
+		if flusher, ok := w.(http.Flusher); ok {
+			flusher.Flush()
+		}
+	}
+
+	w.WriteHeader(http.StatusOK)
+	w.Header().Add("Content-Type", "application/json")
+	flush()
+
+	enc := json.NewEncoder(w)
+	enc.SetEscapeHTML(true)
+	var failed bool
+loop: // break out of for/select infinite loop
+	for {
+		var report entities.ImagePullReport
+		select {
+		case e := <-writer.Chan():
+			report.Stream = string(e)
+			if err := enc.Encode(report); err != nil {
+				stderr.Write([]byte(err.Error()))
+			}
+			flush()
+		case e := <-stderr.Chan():
+			failed = true
+			report.Error = string(e)
+			if err := enc.Encode(report); err != nil {
+				logrus.Warnf("Failed to json encode error %q", err.Error())
+			}
+			flush()
+		case <-runCtx.Done():
+			if !failed {
+				report.Images = images
+				if err := enc.Encode(report); err != nil {
+					logrus.Warnf("Failed to json encode error %q", err.Error())
+				}
+				flush()
+			}
+			break loop // break out of for/select infinite loop
+		case <-r.Context().Done():
+			// Client has closed connection
+			break loop // break out of for/select infinite loop
+		}
+	}
+}
diff --git a/pkg/bindings/images/images.go b/pkg/bindings/images/images.go
index 05ab25d5b..596491044 100644
--- a/pkg/bindings/images/images.go
+++ b/pkg/bindings/images/images.go
@@ -270,51 +270,6 @@ func Import(ctx context.Context, changes []string, message, reference, u *string
 	return &report, response.Process(&report)
 }
 
-// Pull is the binding for libpod's v2 endpoints for pulling images.  Note that
-// `rawImage` must be a reference to a registry (i.e., of docker transport or be
-// normalized to one).  Other transports are rejected as they do not make sense
-// in a remote context.
-func Pull(ctx context.Context, rawImage string, options entities.ImagePullOptions) ([]string, error) {
-	conn, err := bindings.GetClient(ctx)
-	if err != nil {
-		return nil, err
-	}
-	params := url.Values{}
-	params.Set("reference", rawImage)
-	params.Set("overrideArch", options.OverrideArch)
-	params.Set("overrideOS", options.OverrideOS)
-	params.Set("overrideVariant", options.OverrideVariant)
-	if options.SkipTLSVerify != types.OptionalBoolUndefined {
-		// Note: we have to verify if skipped is false.
-		verifyTLS := bool(options.SkipTLSVerify == types.OptionalBoolFalse)
-		params.Set("tlsVerify", strconv.FormatBool(verifyTLS))
-	}
-	params.Set("allTags", strconv.FormatBool(options.AllTags))
-
-	// TODO: have a global system context we can pass around (1st argument)
-	header, err := auth.Header(nil, options.Authfile, options.Username, options.Password)
-	if err != nil {
-		return nil, err
-	}
-
-	response, err := conn.DoRequest(nil, http.MethodPost, "/images/pull", params, header)
-	if err != nil {
-		return nil, err
-	}
-
-	reports := []handlers.LibpodImagesPullReport{}
-	if err := response.Process(&reports); err != nil {
-		return nil, err
-	}
-
-	pulledImages := []string{}
-	for _, r := range reports {
-		pulledImages = append(pulledImages, r.ID)
-	}
-
-	return pulledImages, nil
-}
-
 // Push is the binding for libpod's v2 endpoints for push images.  Note that
 // `source` must be a referring to an image in the remote's container storage.
 // The destination must be a reference to a registry (i.e., of docker transport
diff --git a/pkg/bindings/images/pull.go b/pkg/bindings/images/pull.go
new file mode 100644
index 000000000..261a481a2
--- /dev/null
+++ b/pkg/bindings/images/pull.go
@@ -0,0 +1,98 @@
+package images
+
+import (
+	"context"
+	"encoding/json"
+	"errors"
+	"fmt"
+	"io"
+	"io/ioutil"
+	"net/http"
+	"net/url"
+	"os"
+	"strconv"
+
+	"github.com/containers/image/v5/types"
+	"github.com/containers/podman/v2/pkg/auth"
+	"github.com/containers/podman/v2/pkg/bindings"
+	"github.com/containers/podman/v2/pkg/domain/entities"
+	"github.com/hashicorp/go-multierror"
+)
+
+// Pull is the binding for libpod's v2 endpoints for pulling images.  Note that
+// `rawImage` must be a reference to a registry (i.e., of docker transport or be
+// normalized to one).  Other transports are rejected as they do not make sense
+// in a remote context. Progress reported on stderr
+func Pull(ctx context.Context, rawImage string, options entities.ImagePullOptions) ([]string, error) {
+	conn, err := bindings.GetClient(ctx)
+	if err != nil {
+		return nil, err
+	}
+	params := url.Values{}
+	params.Set("reference", rawImage)
+	params.Set("overrideArch", options.OverrideArch)
+	params.Set("overrideOS", options.OverrideOS)
+	params.Set("overrideVariant", options.OverrideVariant)
+
+	if options.SkipTLSVerify != types.OptionalBoolUndefined {
+		// Note: we have to verify if skipped is false.
+		verifyTLS := bool(options.SkipTLSVerify == types.OptionalBoolFalse)
+		params.Set("tlsVerify", strconv.FormatBool(verifyTLS))
+	}
+	params.Set("allTags", strconv.FormatBool(options.AllTags))
+
+	// TODO: have a global system context we can pass around (1st argument)
+	header, err := auth.Header(nil, options.Authfile, options.Username, options.Password)
+	if err != nil {
+		return nil, err
+	}
+
+	response, err := conn.DoRequest(nil, http.MethodPost, "/images/pull", params, header)
+	if err != nil {
+		return nil, err
+	}
+	defer response.Body.Close()
+
+	if !response.IsSuccess() {
+		return nil, response.Process(err)
+	}
+
+	// Historically pull writes status to stderr
+	stderr := io.Writer(os.Stderr)
+	if options.Quiet {
+		stderr = ioutil.Discard
+	}
+
+	dec := json.NewDecoder(response.Body)
+	var images []string
+	var mErr error
+	for {
+		var report entities.ImagePullReport
+		if err := dec.Decode(&report); err != nil {
+			if errors.Is(err, io.EOF) {
+				break
+			}
+			report.Error = err.Error() + "\n"
+		}
+
+		select {
+		case <-response.Request.Context().Done():
+			return images, mErr
+		default:
+			// non-blocking select
+		}
+
+		switch {
+		case report.Stream != "":
+			fmt.Fprint(stderr, report.Stream)
+		case report.Error != "":
+			mErr = multierror.Append(mErr, errors.New(report.Error))
+		case len(report.Images) > 0:
+			images = report.Images
+		default:
+			return images, errors.New("failed to parse pull results stream, unexpected input")
+		}
+
+	}
+	return images, mErr
+}
diff --git a/pkg/domain/entities/images.go b/pkg/domain/entities/images.go
index 3a2e762d6..d0b738934 100644
--- a/pkg/domain/entities/images.go
+++ b/pkg/domain/entities/images.go
@@ -45,7 +45,7 @@ type Image struct {
 	HealthCheck   *manifest.Schema2HealthConfig `json:",omitempty"`
 }
 
-func (i *Image) Id() string { //nolint
+func (i *Image) Id() string { // nolint
 	return i.ID
 }
 
@@ -70,7 +70,7 @@ type ImageSummary struct {
 	History      []string `json:",omitempty"`
 }
 
-func (i *ImageSummary) Id() string { //nolint
+func (i *ImageSummary) Id() string { // nolint
 	return i.ID
 }
 
@@ -150,7 +150,12 @@ type ImagePullOptions struct {
 
 // ImagePullReport is the response from pulling one or more images.
 type ImagePullReport struct {
-	Images []string
+	// Stream used to provide output from c/image
+	Stream string `json:"stream,omitempty"`
+	// Error contains text of errors from c/image
+	Error string `json:"error,omitempty"`
+	// Images contains the ID's of the images pulled
+	Images []string `json:"images,omitempty"`
 }
 
 // ImagePushOptions are the arguments for pushing images.
@@ -269,7 +274,7 @@ type ImageImportOptions struct {
 }
 
 type ImageImportReport struct {
-	Id string //nolint
+	Id string // nolint
 }
 
 // ImageSaveOptions provide options for saving images.
@@ -349,7 +354,7 @@ type ImageUnmountOptions struct {
 // ImageMountReport describes the response from image mount
 type ImageMountReport struct {
 	Err          error
-	Id           string //nolint
+	Id           string // nolint
 	Name         string
 	Repositories []string
 	Path         string
@@ -358,5 +363,5 @@ type ImageMountReport struct {
 // ImageUnmountReport describes the response from umounting an image
 type ImageUnmountReport struct {
 	Err error
-	Id  string //nolint
+	Id  string // nolint
 }