Add remote.Reuse for Pusher/Puller (#1672)

* code organization: split fetcher from descriptor

* Refactor everything to go through Puller

* crane: use puller/pusher in copy to drop legacy

This package was only used to copy schema 1 images, which we don't need
anymore.

* Add remote.Reuse for Pusher/Puller

This is a big change, but it helps callers out a lot.

The Pusher/Puller interfaces allow us to deduplicate a bunch of work
(largely, ping an auth), but they only work if you actually use them.

It's a huge pain to migrate callers from remote.{Image,Index,...}
interfaces to start using Pusher/Puller because the remote functions can
be called from anywhere, which means plumbing pushers and pullers around
the entire callgraph, which is super painful.

Happily, though, most callers already plumb remote.Options around the
callgraph so that they can set their options in one place and have them
be consistent throughout their application.

This change takes advantage of that fact by introducing remote.Reuse,
which takes either a Puller or a Pusher, and calls equivalent methods on
said pusher/puller whenever you pass remote.Reuse into a remote
function.

The end result is that we can get almost all of the performance wins of
using Puller/Pusher directly with a 3 line change to most applications
rather than refactoring everything.

* Address review feedback

Author: jonjohnsonjr

internal/legacy/copy.go

@@ -17,11 +17,9 @@ package crane
 import (
 	"fmt"
 
-	"github.com/google/go-containerregistry/internal/legacy"
 	"github.com/google/go-containerregistry/pkg/logs"
 	"github.com/google/go-containerregistry/pkg/name"
 	"github.com/google/go-containerregistry/pkg/v1/remote"
-	"github.com/google/go-containerregistry/pkg/v1/types"
 )
 
 // Copy copies a remote image or index from src to dst.
@@ -37,52 +35,30 @@ func Copy(src, dst string, opt ...Option) error {
 		return fmt.Errorf("parsing reference for %q: %w", dst, err)
 	}
 
-	logs.Progress.Printf("Copying from %v to %v", srcRef, dstRef)
-	desc, err := remote.Get(srcRef, o.Remote...)
+	pusher, err := remote.NewPusher(o.Remote...)
 	if err != nil {
-		return fmt.Errorf("fetching %q: %w", src, err)
+		return err
 	}
 
-	switch desc.MediaType {
-	case types.OCIImageIndex, types.DockerManifestList:
-		// Handle indexes separately.
-		if o.Platform != nil {
-			// If platform is explicitly set, don't copy the whole index, just the appropriate image.
-			if err := copyImage(desc, dstRef, o); err != nil {
-				return fmt.Errorf("failed to copy image: %w", err)
-			}
-		} else {
-			if err := copyIndex(desc, dstRef, o); err != nil {
-				return fmt.Errorf("failed to copy index: %w", err)
-			}
-		}
-	case types.DockerManifestSchema1, types.DockerManifestSchema1Signed:
-		// Handle schema 1 images separately.
-		if err := legacy.CopySchema1(desc, srcRef, dstRef, o.Remote...); err != nil {
-			return fmt.Errorf("failed to copy schema 1 image: %w", err)
-		}
-	default:
-		// Assume anything else is an image, since some registries don't set mediaTypes properly.
-		if err := copyImage(desc, dstRef, o); err != nil {
-			return fmt.Errorf("failed to copy image: %w", err)
-		}
+	puller, err := remote.NewPuller(o.Remote...)
+	if err != nil {
+		return err
 	}
 
-	return nil
-}
-
-func copyImage(desc *remote.Descriptor, dstRef name.Reference, o Options) error {
-	img, err := desc.Image()
+	logs.Progress.Printf("Copying from %v to %v", srcRef, dstRef)
+	desc, err := puller.Get(o.ctx, srcRef)
 	if err != nil {
-		return err
+		return fmt.Errorf("fetching %q: %w", src, err)
 	}
-	return remote.Write(dstRef, img, o.Remote...)
-}
 
-func copyIndex(desc *remote.Descriptor, dstRef name.Reference, o Options) error {
-	idx, err := desc.ImageIndex()
+	if o.Platform == nil {
+		return pusher.Push(o.ctx, dstRef, desc)
+	}
+
+	// If platform is explicitly set, don't copy the whole index, just the appropriate image.
+	img, err := desc.Image()
 	if err != nil {
 		return err
 	}
-	return remote.WriteIndex(dstRef, idx, o.Remote...)
+	return pusher.Push(o.ctx, dstRef, img)
 }

internal/legacy/copy_test.go

@@ -34,6 +34,7 @@ type Options struct {
 
 	transport http.RoundTripper
 	insecure  bool
+	ctx       context.Context
 }
 
 // GetOptions exposes the underlying []remote.Option, []name.Option, and
@@ -50,6 +51,7 @@ func makeOptions(opts ...Option) Options {
 			remote.WithAuthFromKeychain(authn.DefaultKeychain),
 		},
 		Keychain: authn.DefaultKeychain,
+		ctx:      context.Background(),
 	}
 
 	for _, o := range opts {
@@ -144,6 +146,7 @@ func WithNondistributable() Option {
 // WithContext is a functional option for setting the context.
 func WithContext(ctx context.Context) Option {
 	return func(o *Options) {
+		o.ctx = ctx
 		o.Remote = append(o.Remote, remote.WithContext(ctx))
 	}
 }

pkg/crane/copy.go

@@ -36,7 +36,8 @@ func CatalogPage(target name.Registry, last string, n int, options ...Option) ([
 	if err != nil {
 		return nil, err
 	}
-	f, err := makeFetcher(o.context, target, o)
+
+	f, err := newPuller(o).fetcher(o.context, target)
 	if err != nil {
 		return nil, err
 	}
@@ -82,18 +83,18 @@ func Catalog(ctx context.Context, target name.Registry, options ...Option) ([]st
 		ctx = o.context
 	}
 
-	return newPuller(o).Catalog(ctx, target)
+	return newPuller(o).catalog(ctx, target, o.pageSize)
 }
 
-func (f *fetcher) catalogPage(ctx context.Context, reg name.Registry, next string) (*Catalogs, error) {
+func (f *fetcher) catalogPage(ctx context.Context, reg name.Registry, next string, pageSize int) (*Catalogs, error) {
 	if next == "" {
 		uri := &url.URL{
 			Scheme: reg.Scheme(),
 			Host:   reg.RegistryStr(),
 			Path:   "/v2/_catalog",
 		}
-		if f.pageSize > 0 {
-			uri.RawQuery = fmt.Sprintf("n=%d", f.pageSize)
+		if pageSize > 0 {
+			uri.RawQuery = fmt.Sprintf("n=%d", pageSize)
 		}
 		next = uri.String()
 	}
@@ -134,8 +135,9 @@ func (f *fetcher) catalogPage(ctx context.Context, reg name.Registry, next strin
 }
 
 type Catalogger struct {
-	f   *fetcher
-	reg name.Registry
+	f        *fetcher
+	reg      name.Registry
+	pageSize int
 
 	page *Catalogs
 	err  error
@@ -145,7 +147,7 @@ type Catalogger struct {
 
 func (l *Catalogger) Next(ctx context.Context) (*Catalogs, error) {
 	if l.needMore {
-		l.page, l.err = l.f.catalogPage(ctx, l.reg, l.page.Next)
+		l.page, l.err = l.f.catalogPage(ctx, l.reg, l.page.Next, l.pageSize)
 	} else {
 		l.needMore = true
 	}

pkg/crane/options.go

@@ -15,12 +15,7 @@
 package remote
 
 import (
-	"fmt"
-	"net/http"
-	"net/url"
-
 	"github.com/google/go-containerregistry/pkg/name"
-	"github.com/google/go-containerregistry/pkg/v1/remote/transport"
 )
 
 // Delete removes the specified image reference from the remote registry.
@@ -29,32 +24,5 @@ func Delete(ref name.Reference, options ...Option) error {
 	if err != nil {
 		return err
 	}
-	w, err := makeWriter(o.context, ref.Context(), nil, o)
-	if err != nil {
-		return err
-	}
-	c := w.client
-
-	u := url.URL{
-		Scheme: ref.Context().Registry.Scheme(),
-		Host:   ref.Context().RegistryStr(),
-		Path:   fmt.Sprintf("/v2/%s/manifests/%s", ref.Context().RepositoryStr(), ref.Identifier()),
-	}
-
-	req, err := http.NewRequest(http.MethodDelete, u.String(), nil)
-	if err != nil {
-		return err
-	}
-
-	resp, err := c.Do(req.WithContext(o.context))
-	if err != nil {
-		return err
-	}
-	defer resp.Body.Close()
-
-	return transport.CheckError(resp, http.StatusOK, http.StatusAccepted)
-
-	// TODO(jason): If the manifest had a `subject`, and if the registry
-	// doesn't support Referrers, update the index pointed to by the
-	// subject's fallback tag to remove the descriptor for this manifest.
+	return newPusher(o).Delete(o.context, ref)
 }