remote push: show copy progress

`podman-remote push` has shown absolutely no progress at all. Fix that
by doing essentially the same as the remote-pull code does.

The get-free-out-of-jail-card for backwards compatibility is to let the
`quiet` parameter default to true.  Since the --quioet flag wasn't
working before either, older Podman clients do not set it.

Also add regression tests to make sure we won't regress again.

Fixes: #11554
Fixes: #14971
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

11 files changed, 297 insertions, 123 deletions

diff --git a/pkg/api/handlers/libpod/images.go b/pkg/api/handlers/libpod/images.go
index ed1c65f8e..67943ecf1 100644
--- a/pkg/api/handlers/libpod/images.go
+++ b/pkg/api/handlers/libpod/images.go
@@ -1,7 +1,6 @@
 package libpod
 
 import (
-	"context"
 	"errors"
 	"fmt"
 	"io"
@@ -14,13 +13,11 @@ import (
 	"github.com/containers/buildah"
 	"github.com/containers/common/libimage"
 	"github.com/containers/image/v5/manifest"
-	"github.com/containers/image/v5/types"
 	"github.com/containers/podman/v4/libpod"
 	"github.com/containers/podman/v4/libpod/define"
 	"github.com/containers/podman/v4/pkg/api/handlers"
 	"github.com/containers/podman/v4/pkg/api/handlers/utils"
 	api "github.com/containers/podman/v4/pkg/api/types"
-	"github.com/containers/podman/v4/pkg/auth"
 	"github.com/containers/podman/v4/pkg/domain/entities"
 	"github.com/containers/podman/v4/pkg/domain/entities/reports"
 	"github.com/containers/podman/v4/pkg/domain/infra/abi"
@@ -416,74 +413,6 @@ func ImagesImport(w http.ResponseWriter, r *http.Request) {
 	utils.WriteResponse(w, http.StatusOK, report)
 }
 
-// PushImage is the handler for the compat http endpoint for pushing images.
-func PushImage(w http.ResponseWriter, r *http.Request) {
-	decoder := r.Context().Value(api.DecoderKey).(*schema.Decoder)
-	runtime := r.Context().Value(api.RuntimeKey).(*libpod.Runtime)
-
-	query := struct {
-		All              bool   `schema:"all"`
-		Destination      string `schema:"destination"`
-		Format           string `schema:"format"`
-		RemoveSignatures bool   `schema:"removeSignatures"`
-		TLSVerify        bool   `schema:"tlsVerify"`
-	}{
-		// This is where you can override the golang default value for one of fields
-	}
-	if err := decoder.Decode(&query, r.URL.Query()); err != nil {
-		utils.Error(w, http.StatusBadRequest, fmt.Errorf("failed to parse parameters for %s: %w", r.URL.String(), err))
-		return
-	}
-
-	source := strings.TrimSuffix(utils.GetName(r), "/push") // GetName returns the entire path
-	if _, err := utils.ParseStorageReference(source); err != nil {
-		utils.Error(w, http.StatusBadRequest, err)
-		return
-	}
-
-	destination := query.Destination
-	if destination == "" {
-		destination = source
-	}
-
-	if err := utils.IsRegistryReference(destination); err != nil {
-		utils.Error(w, http.StatusBadRequest, err)
-		return
-	}
-
-	authconf, authfile, err := auth.GetCredentials(r)
-	if err != nil {
-		utils.Error(w, http.StatusBadRequest, err)
-		return
-	}
-	defer auth.RemoveAuthfile(authfile)
-	var username, password string
-	if authconf != nil {
-		username = authconf.Username
-		password = authconf.Password
-	}
-	options := entities.ImagePushOptions{
-		All:              query.All,
-		Authfile:         authfile,
-		Format:           query.Format,
-		Password:         password,
-		Quiet:            true,
-		RemoveSignatures: query.RemoveSignatures,
-		Username:         username,
-	}
-	if _, found := r.URL.Query()["tlsVerify"]; found {
-		options.SkipTLSVerify = types.NewOptionalBool(!query.TLSVerify)
-	}
-
-	imageEngine := abi.ImageEngine{Libpod: runtime}
-	if err := imageEngine.Push(context.Background(), source, destination, options); err != nil {
-		utils.Error(w, http.StatusBadRequest, fmt.Errorf("error pushing image %q: %w", destination, err))
-		return
-	}
-
-	utils.WriteResponse(w, http.StatusOK, "")
-}
-
 func CommitContainer(w http.ResponseWriter, r *http.Request) {
 	var (
 		destImage string
diff --git a/pkg/api/handlers/libpod/images_push.go b/pkg/api/handlers/libpod/images_push.go
new file mode 100644
index 000000000..f427dc01b
--- /dev/null
+++ b/pkg/api/handlers/libpod/images_push.go
@@ -0,0 +1,144 @@
+package libpod
+
+import (
+	"context"
+	"encoding/json"
+	"fmt"
+	"net/http"
+	"strings"
+
+	"github.com/containers/image/v5/types"
+	"github.com/containers/podman/v4/libpod"
+	"github.com/containers/podman/v4/pkg/api/handlers/utils"
+	api "github.com/containers/podman/v4/pkg/api/types"
+	"github.com/containers/podman/v4/pkg/auth"
+	"github.com/containers/podman/v4/pkg/channel"
+	"github.com/containers/podman/v4/pkg/domain/entities"
+	"github.com/containers/podman/v4/pkg/domain/infra/abi"
+	"github.com/gorilla/schema"
+	"github.com/sirupsen/logrus"
+)
+
+// PushImage is the handler for the compat http endpoint for pushing images.
+func PushImage(w http.ResponseWriter, r *http.Request) {
+	decoder := r.Context().Value(api.DecoderKey).(*schema.Decoder)
+	runtime := r.Context().Value(api.RuntimeKey).(*libpod.Runtime)
+
+	query := struct {
+		All              bool   `schema:"all"`
+		Destination      string `schema:"destination"`
+		Format           string `schema:"format"`
+		RemoveSignatures bool   `schema:"removeSignatures"`
+		TLSVerify        bool   `schema:"tlsVerify"`
+		Quiet            bool   `schema:"quiet"`
+	}{
+		// #14971: older versions did not sent *any* data, so we need
+		//         to be quiet by default to remain backwards compatible
+		Quiet: true,
+	}
+	if err := decoder.Decode(&query, r.URL.Query()); err != nil {
+		utils.Error(w, http.StatusBadRequest, fmt.Errorf("failed to parse parameters for %s: %w", r.URL.String(), err))
+		return
+	}
+
+	source := strings.TrimSuffix(utils.GetName(r), "/push") // GetName returns the entire path
+	if _, err := utils.ParseStorageReference(source); err != nil {
+		utils.Error(w, http.StatusBadRequest, err)
+		return
+	}
+
+	destination := query.Destination
+	if destination == "" {
+		destination = source
+	}
+
+	if err := utils.IsRegistryReference(destination); err != nil {
+		utils.Error(w, http.StatusBadRequest, err)
+		return
+	}
+
+	authconf, authfile, err := auth.GetCredentials(r)
+	if err != nil {
+		utils.Error(w, http.StatusBadRequest, err)
+		return
+	}
+	defer auth.RemoveAuthfile(authfile)
+
+	var username, password string
+	if authconf != nil {
+		username = authconf.Username
+		password = authconf.Password
+	}
+	options := entities.ImagePushOptions{
+		All:              query.All,
+		Authfile:         authfile,
+		Format:           query.Format,
+		Password:         password,
+		Quiet:            true,
+		RemoveSignatures: query.RemoveSignatures,
+		Username:         username,
+	}
+
+	if _, found := r.URL.Query()["tlsVerify"]; found {
+		options.SkipTLSVerify = types.NewOptionalBool(!query.TLSVerify)
+	}
+
+	imageEngine := abi.ImageEngine{Libpod: runtime}
+
+	// Let's keep thing simple when running in quiet mode and push directly.
+	if query.Quiet {
+		if err := imageEngine.Push(context.Background(), source, destination, options); err != nil {
+			utils.Error(w, http.StatusBadRequest, fmt.Errorf("error pushing image %q: %w", destination, err))
+			return
+		}
+		utils.WriteResponse(w, http.StatusOK, "")
+		return
+	}
+
+	writer := channel.NewWriter(make(chan []byte))
+	defer writer.Close()
+	options.Writer = writer
+
+	pushCtx, pushCancel := context.WithCancel(r.Context())
+	var pushError error
+	go func() {
+		defer pushCancel()
+		pushError = imageEngine.Push(pushCtx, source, destination, options)
+	}()
+
+	flush := func() {
+		if flusher, ok := w.(http.Flusher); ok {
+			flusher.Flush()
+		}
+	}
+
+	w.WriteHeader(http.StatusOK)
+	w.Header().Set("Content-Type", "application/json")
+	flush()
+
+	enc := json.NewEncoder(w)
+	enc.SetEscapeHTML(true)
+	for {
+		var report entities.ImagePushReport
+		select {
+		case s := <-writer.Chan():
+			report.Stream = string(s)
+			if err := enc.Encode(report); err != nil {
+				logrus.Warnf("Failed to encode json: %v", err)
+			}
+			flush()
+		case <-pushCtx.Done():
+			if pushError != nil {
+				report.Error = pushError.Error()
+				if err := enc.Encode(report); err != nil {
+					logrus.Warnf("Failed to encode json: %v", err)
+				}
+			}
+			flush()
+			return
+		case <-r.Context().Done():
+			// Client has closed connection
+			return
+		}
+	}
+}
diff --git a/pkg/api/server/register_images.go b/pkg/api/server/register_images.go
index a2f46cb35..11ab8cae0 100644
--- a/pkg/api/server/register_images.go
+++ b/pkg/api/server/register_images.go
@@ -730,6 +730,11 @@ func (s *APIServer) registerImagesHandlers(r *mux.Router) error {
 	//    description: Require TLS verification.
 	//    type: boolean
 	//    default: true
+	//  - in: query
+	//    name: quiet
+	//    description: "silences extra stream data on push"
+	//    type: boolean
+	//    default: true
 	//  - in: header
 	//    name: X-Registry-Auth
 	//    type: string
diff --git a/pkg/bindings/images/images.go b/pkg/bindings/images/images.go
index cd5147629..bb7867c4e 100644
--- a/pkg/bindings/images/images.go
+++ b/pkg/bindings/images/images.go
@@ -267,46 +267,6 @@ func Import(ctx context.Context, r io.Reader, options *ImportOptions) (*entities
 	return &report, response.Process(&report)
 }
 
-// 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
-// or be normalized to one).  Other transports are rejected as they do not make
-// sense in a remote context.
-func Push(ctx context.Context, source string, destination string, options *PushOptions) error {
-	if options == nil {
-		options = new(PushOptions)
-	}
-	conn, err := bindings.GetClient(ctx)
-	if err != nil {
-		return err
-	}
-	header, err := auth.MakeXRegistryAuthHeader(&imageTypes.SystemContext{AuthFilePath: options.GetAuthfile()}, options.GetUsername(), options.GetPassword())
-	if err != nil {
-		return err
-	}
-
-	params, err := options.ToParams()
-	if err != nil {
-		return err
-	}
-	// SkipTLSVerify is special.  We need to delete the param added by
-	// toparams and change the key and flip the bool
-	if options.SkipTLSVerify != nil {
-		params.Del("SkipTLSVerify")
-		params.Set("tlsVerify", strconv.FormatBool(!options.GetSkipTLSVerify()))
-	}
-	params.Set("destination", destination)
-
-	path := fmt.Sprintf("/images/%s/push", source)
-	response, err := conn.DoRequest(ctx, nil, http.MethodPost, path, params, header)
-	if err != nil {
-		return err
-	}
-	defer response.Body.Close()
-
-	return response.Process(err)
-}
-
 // Search is the binding for libpod's v2 endpoints for Search images.
 func Search(ctx context.Context, term string, options *SearchOptions) ([]entities.ImageSearchReport, error) {
 	if options == nil {
diff --git a/pkg/bindings/images/push.go b/pkg/bindings/images/push.go
new file mode 100644
index 000000000..8db3726e6
--- /dev/null
+++ b/pkg/bindings/images/push.go
@@ -0,0 +1,96 @@
+package images
+
+import (
+	"context"
+	"encoding/json"
+	"errors"
+	"fmt"
+	"io"
+	"io/ioutil"
+	"net/http"
+	"os"
+	"strconv"
+
+	imageTypes "github.com/containers/image/v5/types"
+	"github.com/containers/podman/v4/pkg/auth"
+	"github.com/containers/podman/v4/pkg/bindings"
+	"github.com/containers/podman/v4/pkg/domain/entities"
+)
+
+// Push is the binding for libpod's 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
+// or be normalized to one).  Other transports are rejected as they do not make
+// sense in a remote context.
+func Push(ctx context.Context, source string, destination string, options *PushOptions) error {
+	if options == nil {
+		options = new(PushOptions)
+	}
+	conn, err := bindings.GetClient(ctx)
+	if err != nil {
+		return err
+	}
+	header, err := auth.MakeXRegistryAuthHeader(&imageTypes.SystemContext{AuthFilePath: options.GetAuthfile()}, options.GetUsername(), options.GetPassword())
+	if err != nil {
+		return err
+	}
+
+	params, err := options.ToParams()
+	if err != nil {
+		return err
+	}
+	// SkipTLSVerify is special.  We need to delete the param added by
+	// toparams and change the key and flip the bool
+	if options.SkipTLSVerify != nil {
+		params.Del("SkipTLSVerify")
+		params.Set("tlsVerify", strconv.FormatBool(!options.GetSkipTLSVerify()))
+	}
+	params.Set("destination", destination)
+
+	path := fmt.Sprintf("/images/%s/push", source)
+	response, err := conn.DoRequest(ctx, nil, http.MethodPost, path, params, header)
+	if err != nil {
+		return err
+	}
+	defer response.Body.Close()
+
+	if !response.IsSuccess() {
+		return response.Process(err)
+	}
+
+	// Historically push writes status to stderr
+	writer := io.Writer(os.Stderr)
+	if options.GetQuiet() {
+		writer = ioutil.Discard
+	}
+
+	dec := json.NewDecoder(response.Body)
+	for {
+		var report entities.ImagePushReport
+		if err := dec.Decode(&report); err != nil {
+			if errors.Is(err, io.EOF) {
+				break
+			}
+			return err
+		}
+
+		select {
+		case <-response.Request.Context().Done():
+			break
+		default:
+			// non-blocking select
+		}
+
+		switch {
+		case report.Stream != "":
+			fmt.Fprint(writer, report.Stream)
+		case report.Error != "":
+			// There can only be one error.
+			return errors.New(report.Error)
+		default:
+			return fmt.Errorf("failed to parse push results stream, unexpected input: %v", report)
+		}
+	}
+
+	return nil
+}
diff --git a/pkg/bindings/images/types.go b/pkg/bindings/images/types.go
index 3728ae5c0..0e672cdea 100644
--- a/pkg/bindings/images/types.go
+++ b/pkg/bindings/images/types.go
@@ -133,6 +133,8 @@ type PushOptions struct {
 	RemoveSignatures *bool
 	// Username for authenticating against the registry.
 	Username *string
+	// Quiet can be specified to suppress progress when pushing.
+	Quiet *bool
 }
 
 //go:generate go run ../generator/generator.go SearchOptions
diff --git a/pkg/bindings/images/types_push_options.go b/pkg/bindings/images/types_push_options.go
index 25f6c5546..63a19fb81 100644
--- a/pkg/bindings/images/types_push_options.go
+++ b/pkg/bindings/images/types_push_options.go
@@ -136,3 +136,18 @@ func (o *PushOptions) GetUsername() string {
 	}
 	return *o.Username
 }
+
+// WithQuiet set field Quiet to given value
+func (o *PushOptions) WithQuiet(value bool) *PushOptions {
+	o.Quiet = &value
+	return o
+}
+
+// GetQuiet returns value of field Quiet
+func (o *PushOptions) GetQuiet() bool {
+	if o.Quiet == nil {
+		var z bool
+		return z
+	}
+	return *o.Quiet
+}
diff --git a/pkg/domain/entities/images.go b/pkg/domain/entities/images.go
index da317cfad..b8b346005 100644
--- a/pkg/domain/entities/images.go
+++ b/pkg/domain/entities/images.go
@@ -1,6 +1,7 @@
 package entities
 
 import (
+	"io"
 	"net/url"
 	"time"
 
@@ -192,8 +193,7 @@ type ImagePushOptions struct {
 	// image. Default is manifest type of source, with fallbacks.
 	// Ignored for remote calls.
 	Format string
-	// Quiet can be specified to suppress pull progress when pulling.  Ignored
-	// for remote calls.
+	// Quiet can be specified to suppress push progress when pushing.
 	Quiet bool
 	// Rm indicates whether to remove the manifest list if push succeeds
 	Rm bool
@@ -211,6 +211,17 @@ type ImagePushOptions struct {
 	Progress chan types.ProgressProperties
 	// CompressionFormat is the format to use for the compression of the blobs
 	CompressionFormat string
+	// Writer is used to display copy information including progress bars.
+	Writer io.Writer
+}
+
+// ImagePushReport is the response from pushing an image.
+// Currently only used in the remote API.
+type ImagePushReport struct {
+	// Stream used to provide push progress
+	Stream string `json:"stream,omitempty"`
+	// Error contains text of errors from pushing
+	Error string `json:"error,omitempty"`
 }
 
 // ImageSearchOptions are the arguments for searching images.
diff --git a/pkg/domain/infra/abi/images.go b/pkg/domain/infra/abi/images.go
index 38008c7b9..ff42b0367 100644
--- a/pkg/domain/infra/abi/images.go
+++ b/pkg/domain/infra/abi/images.go
@@ -305,6 +305,7 @@ func (ir *ImageEngine) Push(ctx context.Context, source string, destination stri
 	pushOptions.RemoveSignatures = options.RemoveSignatures
 	pushOptions.SignBy = options.SignBy
 	pushOptions.InsecureSkipTLSVerify = options.SkipTLSVerify
+	pushOptions.Writer = options.Writer
 
 	compressionFormat := options.CompressionFormat
 	if compressionFormat == "" {
@@ -322,7 +323,7 @@ func (ir *ImageEngine) Push(ctx context.Context, source string, destination stri
 		pushOptions.CompressionFormat = &algo
 	}
 
-	if !options.Quiet {
+	if !options.Quiet && pushOptions.Writer == nil {
 		pushOptions.Writer = os.Stderr
 	}
 
diff --git a/pkg/domain/infra/tunnel/images.go b/pkg/domain/infra/tunnel/images.go
index 18f750dcc..9ad408850 100644
--- a/pkg/domain/infra/tunnel/images.go
+++ b/pkg/domain/infra/tunnel/images.go
@@ -240,7 +240,7 @@ func (ir *ImageEngine) Import(ctx context.Context, opts entities.ImageImportOpti
 
 func (ir *ImageEngine) Push(ctx context.Context, source string, destination string, opts entities.ImagePushOptions) error {
 	options := new(images.PushOptions)
-	options.WithAll(opts.All).WithCompress(opts.Compress).WithUsername(opts.Username).WithPassword(opts.Password).WithAuthfile(opts.Authfile).WithFormat(opts.Format).WithRemoveSignatures(opts.RemoveSignatures)
+	options.WithAll(opts.All).WithCompress(opts.Compress).WithUsername(opts.Username).WithPassword(opts.Password).WithAuthfile(opts.Authfile).WithFormat(opts.Format).WithRemoveSignatures(opts.RemoveSignatures).WithQuiet(opts.Quiet)
 
 	if s := opts.SkipTLSVerify; s != types.OptionalBoolUndefined {
 		if s == types.OptionalBoolTrue {
diff --git a/test/e2e/push_test.go b/test/e2e/push_test.go
index 97567e40d..f2a103f6b 100644
--- a/test/e2e/push_test.go
+++ b/test/e2e/push_test.go
@@ -116,15 +116,26 @@ var _ = Describe("Podman push", func() {
 		push := podmanTest.Podman([]string{"push", "-q", "--tls-verify=false", "--remove-signatures", ALPINE, "localhost:5000/my-alpine"})
 		push.WaitWithDefaultTimeout()
 		Expect(push).Should(Exit(0))
+		Expect(len(push.ErrorToString())).To(Equal(0))
 
-		SkipIfRemote("Remote does not support --digestfile")
-		// Test --digestfile option
-		push2 := podmanTest.Podman([]string{"push", "--tls-verify=false", "--digestfile=/tmp/digestfile.txt", "--remove-signatures", ALPINE, "localhost:5000/my-alpine"})
-		push2.WaitWithDefaultTimeout()
-		fi, err := os.Lstat("/tmp/digestfile.txt")
-		Expect(err).To(BeNil())
-		Expect(fi.Name()).To(Equal("digestfile.txt"))
-		Expect(push2).Should(Exit(0))
+		push = podmanTest.Podman([]string{"push", "--tls-verify=false", "--remove-signatures", ALPINE, "localhost:5000/my-alpine"})
+		push.WaitWithDefaultTimeout()
+		Expect(push).Should(Exit(0))
+		output := push.ErrorToString()
+		Expect(output).To(ContainSubstring("Copying blob "))
+		Expect(output).To(ContainSubstring("Copying config "))
+		Expect(output).To(ContainSubstring("Writing manifest to image destination"))
+		Expect(output).To(ContainSubstring("Storing signatures"))
+
+		if !IsRemote() { // Remote does not support --digestfile
+			// Test --digestfile option
+			push2 := podmanTest.Podman([]string{"push", "--tls-verify=false", "--digestfile=/tmp/digestfile.txt", "--remove-signatures", ALPINE, "localhost:5000/my-alpine"})
+			push2.WaitWithDefaultTimeout()
+			fi, err := os.Lstat("/tmp/digestfile.txt")
+			Expect(err).To(BeNil())
+			Expect(fi.Name()).To(Equal("digestfile.txt"))
+			Expect(push2).Should(Exit(0))
+		}
 	})
 
 	It("podman push to local registry with authorization", func() {