Repository: google/go-containerregistry
Pull request #1122 (2021-09-09 00:31:04)

Make retrying transport and http errors configurable 
Related to https://github.com/google/go-containerregistry/issues/1114

Adds the following 4 options:
- WithRetryHTTPBackoff
- WithRetryHTTPPredicate
- WithRetryTransportBackoff
- WithRetryTransportPredicate

Went with a finer level API around configuring these (in my head) 2 groups of retry logic (low-level transport errors and higher level HTTP errors).

I was also aiming to keep the 'default' retry logic values the same (if these options are not provided).

jonjohnsonjr: So I don't love how many options this adds, but I get the need for this to be more configurable.

For the transport pieces, I think I'd like to revisit this: https://github.com/google/go-containerregistry/issues/740

Ideally, you could supply your own transport via `remote.WithTransport` and use `transport.NewRetry` to configure it. The only issue is around double-wrapping because of how we wrap transports by default. One option here is to just not wrap the supplied transport if you pass in a custom transport, but then you'd lose out on some "niceness". If we exposed [this stuff](https://github.com/google/go-containerregistry/blob/8388fde8bcadbc8ddb820b5ac0046e10e6c572e7/pkg/v1/remote/options.go#L81-L94) in a nice way, you could claw that back by opting into it, e.g.:

```go
// Wraps with retries and useragent and debug logging
remote.Image(..., remote.WithTransport(remote.Transport{t}))

// No wrapping of t
remote.Image(..., remote.WithTransport(t))

// Configurable backoff stuff.
remote.Image(..., remote.WithTransport(transport.NewRetry(t, ...)))
```

My only issue with that approach is that it's probably a breaking change :/

Another approach might be exposing more on the errors we return from `transport`, e.g. the request: https://github.com/google/go-containerregistry/blob/8388fde8bcadbc8ddb820b5ac0046e10e6c572e7/pkg/v1/remote/transport/error.go#L52

That would allow you to write a predicate that checked the request method, URL, and status code yourself to determine if you should retry it. 
codecov-commenter: # [Codecov](https://codecov.io/gh/google/go-containerregistry/pull/1122?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google) Report
> Merging [#1122](https://codecov.io/gh/google/go-containerregistry/pull/1122?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google) (822e1a1) into [main](https://codecov.io/gh/google/go-containerregistry/commit/8388fde8bcadbc8ddb820b5ac0046e10e6c572e7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google) (8388fde) will **decrease** coverage by `0.14%`.
> The diff coverage is `64.91%`.

> :exclamation: Current head 822e1a1 differs from pull request most recent head 4cdd7d1. Consider uploading reports for the commit 4cdd7d1 to get more accurate results
[![Impacted file tree graph](https://codecov.io/gh/google/go-containerregistry/pull/1122/graphs/tree.svg?width=650&height=150&src=pr&token=wG7iiDCdZ4&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google)](https://codecov.io/gh/google/go-containerregistry/pull/1122?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google)

```diff
@@            Coverage Diff             @@
##             main    #1122      +/-   ##
==========================================
- Coverage   75.16%   75.02%   -0.15%     
==========================================
  Files         108      108              
  Lines        7724     7759      +35     
==========================================
+ Hits         5806     5821      +15     
- Misses       1363     1383      +20     
  Partials      555      555              
```


| [Impacted Files](https://codecov.io/gh/google/go-containerregistry/pull/1122?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google) | Coverage Δ | |
|---|---|---|
| [pkg/v1/remote/options.go](https://codecov.io/gh/google/go-containerregistry/pull/1122/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google#diff-cGtnL3YxL3JlbW90ZS9vcHRpb25zLmdv) | `54.86% <41.17%> (-6.39%)` | :arrow_down: |
| [pkg/v1/remote/multi\_write.go](https://codecov.io/gh/google/go-containerregistry/pull/1122/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google#diff-cGtnL3YxL3JlbW90ZS9tdWx0aV93cml0ZS5nbw==) | `59.60% <100.00%> (+0.40%)` | :arrow_up: |
| [pkg/v1/remote/write.go](https://codecov.io/gh/google/go-containerregistry/pull/1122/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google#diff-cGtnL3YxL3JlbW90ZS93cml0ZS5nbw==) | `62.86% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/google/go-containerregistry/pull/1122?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/google/go-containerregistry/pull/1122?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google). Last update [8388fde...4cdd7d1](https://codecov.io/gh/google/go-containerregistry/pull/1122?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google).

DennisDenuto: @jonjohnsonjr 

Yeah I think iterating over the API change to make it as minimal as needed makes lots of sense. Both from maintainability and from a usability perspective! With the changes above sounds like a trade-off between a simpler API vs breaking changes to existing APIs.

I played around with the idea of using `WithTransport` as a way to allow a consumer to configure retrying.

> Another approach might be exposing more on the errors we return from transport

I did this by creating a new `RetryError` that wraps the transport error (if it exists), and exposes the status code and request.
I also made it implement the `Temporary` function to make it a compatible with the ['default' transport predicate](https://github.com/google/go-containerregistry/blob/main/pkg/v1/remote/transport/retry.go#L67)

I pushed the changes. I'd love some early feedback to make sure i'm properly understanding your idea to begin with!

This PR comes with breaking changes:
- Using `WithTransport(t)` with a non `remote.Transport` no longer wraps retry, logging etc...
- Retrying with the transport.Retry will return a `RetryError` (even if the `RoundTripper` did not return an error). It always returns a `RetryError` since it contains http data (status code) to allow specifying a `Predicate` to determine whether to retry or not.

Note:

Some tests are failing due to these breaking changes. But I wanted to get feedback before proceeding down this path.
jonjohnsonjr: I don't think we need `RetryError` if we just expose the existing request field on the `transport.Error` struct. Callers can check if an error is `transport.Error` and use that information. If it's some other kind of error, they can just do that in an "else" rather than using `Inner`.

> Using WithTransport(t) with a non remote.Transport no longer wraps retry, logging etc...

This feels kinda backwards. I think if someone gives us a `transport.Transport`, we should avoid wrapping it at all. We could use that as a signal that the caller knows what they're doing and we should get out of their way.

It would still make sense to me to expose Backoff/Predicate options in `remote` for higher level things that cannot be retried at a request level, e.g. layer uploads. I'm not sure how I would want to structure those hooks, but there are a handful of places that might make sense to inject some kind of retry check.

I'd expect the default predicate to be somewhat conservative, but having the option would let callers do some pretty flexible things like only retry for certain methods or for certain paths. 
DennisDenuto: > if we just expose the existing request field on the transport.Error

hmmm. I don't see how `transport.Error` ever gets returned by a RoundTripper. I *can* however see how it is returned at the http req/resp layer via `CheckError(...)`  

or put another way...

I don't see how a consumer providing their own Retry Transport would ever get an error of type `transport.Error`

Although I see how exposing the `Request` on the `transport.Error` can be useful in conjunction with exposing Backoff/Predicate options in remote for higher level http retries.

> I think if someone gives us a transport.Transport, we should avoid wrapping it at all.

hmm ok, I mis-understood your code snippet above then

```
// Wraps with retries and useragent and debug logging
remote.Image(..., remote.WithTransport(remote.Transport{t}))
```

but thinking about it more, it does make sense that it should *not* wrap, and i think will result in this not being a breaking change. i'll def update the PR with this.

 > It would still make sense to me to expose Backoff/Predicate options in `remote`

I think this helps clarify things a lot for me.

I was trying to have `transport.Retry` provide a way to retry both low level transport errors and http level errors. I updated the PR to put back Backoff/Predicate options in remote :-) 
jonjohnsonjr: > hmm ok, I mis-understood your code snippet above then

Yeah I _didn't_ love my code snippet, but what you've got here is actually a better solution, I think.

> I think this helps clarify things a lot for me.
> 
> I was trying to have transport.Retry provide a way to retry both low level transport errors and http level errors. I updated the PR to put back Backoff/Predicate options in remote :-)

Ah yeah, sorry this wasn't more clear. There were two categories of retry stuff going on, but we kind of lumped them together. Current PR is pretty close to optimal, I think.
DennisDenuto: @jonjohnsonjr Just checking in to see if theres anything else needed to have this merged in?
jonjohnsonjr: Sorry I missed your follow-up commits!

One final thing I forgot to mention. If we're dealing with a `transport.Wrapper`, we probably also want to skip all of this:
https://github.com/google/go-containerregistry/blob/8388fde8bcadbc8ddb820b5ac0046e10e6c572e7/pkg/v1/remote/transport/transport.go#L53-L68

We can do a little introspection to determine if the the given transport can be reused by looking at `reg` to see if it's the same as what `t` already pinged. Then we can skip some pinging. For the bearer case, we probably want to merge the existing scopes with the given `scopes` like with: https://github.com/google/go-containerregistry/blob/8388fde8bcadbc8ddb820b5ac0046e10e6c572e7/pkg/v1/remote/transport/bearer.go#L99-L105

DennisDenuto: Thanks I appreciate the pointers @jonjohnsonjr 

> We can do a little introspection to determine if the the given transport can be reused by looking at reg to see if it's the same as what t already pinged

I did this by saving state ('pingedRegistries') in `transport.Wrapper`

> For the bearer case, we probably want to merge the existing scopes with the given scopes like with:

I figured that the scopes should be tied to the registry that the transport pinged against. Hence scopes being a map on the wrapped transport.
jonjohnsonjr: Okay so this might make you hate me, but I'm not thrilled with how this turned out after my suggestion (I know... I'm sorry).

Do you mind backing this out to the previous revision? We can just submit because it's useful on its own, then I can revisit the ping optimizations when I have more time to look at it.

Again, sorry, I appreciate your patience.
DennisDenuto: @jonjohnsonjr  lol no worries. i've reverted that commit

fwiw I have enjoyed working on this PR and we get a ton of value from using this library. so more than happy to be patient and get things done right! 😄 


Patch:
diff --git a/pkg/v1/google/list.go b/pkg/v1/google/list.go
index d8574086..847387bb 100644
--- a/pkg/v1/google/list.go
+++ b/pkg/v1/google/list.go
@@ -57,19 +57,23 @@ func newLister(repo name.Repository, options ...Option) (*lister, error) {
 		}
 	}
 
-	// Wrap the transport in something that logs requests and responses.
-	// It's expensive to generate the dumps, so skip it if we're writing
-	// to nothing.
-	if logs.Enabled(logs.Debug) {
-		l.transport = transport.NewLogger(l.transport)
-	}
+	// transport.Wrapper is a signal that consumers are opt-ing into providing their own transport without any additional wrapping.
+	// This is to allow consumers full control over the transports logic, such as providing retry logic.
+	if _, ok := l.transport.(*transport.Wrapper); !ok {
+		// Wrap the transport in something that logs requests and responses.
+		// It's expensive to generate the dumps, so skip it if we're writing
+		// to nothing.
+		if logs.Enabled(logs.Debug) {
+			l.transport = transport.NewLogger(l.transport)
+		}
 
-	// Wrap the transport in something that can retry network flakes.
-	l.transport = transport.NewRetry(l.transport)
+		// Wrap the transport in something that can retry network flakes.
+		l.transport = transport.NewRetry(l.transport)
 
-	// Wrap this last to prevent transport.New from double-wrapping.
-	if l.userAgent != "" {
-		l.transport = transport.NewUserAgent(l.transport, l.userAgent)
+		// Wrap this last to prevent transport.New from double-wrapping.
+		if l.userAgent != "" {
+			l.transport = transport.NewUserAgent(l.transport, l.userAgent)
+		}
 	}
 
 	scopes := []string{repo.Scope(transport.PullScope)}
diff --git a/pkg/v1/remote/multi_write.go b/pkg/v1/remote/multi_write.go
index fcac1cdc..3d9a7e7e 100644
--- a/pkg/v1/remote/multi_write.go
+++ b/pkg/v1/remote/multi_write.go
@@ -92,6 +92,8 @@ func MultiWrite(m map[name.Reference]Taggable, options ...Option) (rerr error) {
 		context:    o.context,
 		updates:    o.updates,
 		lastUpdate: &v1.Update{},
+		backoff:    o.retryBackoff,
+		predicate:  o.retryPredicate,
 	}
 
 	// Collect the total size of blobs and manifests we're about to write.
diff --git a/pkg/v1/remote/options.go b/pkg/v1/remote/options.go
index 6be4b666..6ccbb89e 100644
--- a/pkg/v1/remote/options.go
+++ b/pkg/v1/remote/options.go
@@ -17,8 +17,12 @@ package remote
 import (
 	"context"
 	"errors"
+	"io"
 	"net/http"
+	"syscall"
+	"time"
 
+	"github.com/google/go-containerregistry/internal/retry"
 	"github.com/google/go-containerregistry/pkg/authn"
 	"github.com/google/go-containerregistry/pkg/logs"
 	v1 "github.com/google/go-containerregistry/pkg/v1"
@@ -39,6 +43,8 @@ type options struct {
 	allowNondistributableArtifacts bool
 	updates                        chan<- v1.Update
 	pageSize                       int
+	retryBackoff                   Backoff
+	retryPredicate                 retry.Predicate
 }
 
 var defaultPlatform = v1.Platform{
@@ -46,6 +52,27 @@ var defaultPlatform = v1.Platform{
 	OS:           "linux",
 }
 
+// Backoff is an alias of retry.Backoff to expose this configuration option to consumers of this lib
+type Backoff = retry.Backoff
+
+var defaultRetryPredicate retry.Predicate = func(err error) bool {
+	// Various failure modes here, as we're often reading from and writing to
+	// the network.
+	if retry.IsTemporary(err) || errors.Is(err, io.ErrUnexpectedEOF) || errors.Is(err, syscall.EPIPE) {
+		logs.Warn.Printf("retrying %v", err)
+		return true
+	}
+	return false
+}
+
+// Try this three times, waiting 1s after first failure, 3s after second.
+var defaultRetryBackoff = Backoff{
+	Duration: 1.0 * time.Second,
+	Factor:   3.0,
+	Jitter:   0.1,
+	Steps:    3,
+}
+
 const (
 	defaultJobs = 4
 
@@ -56,12 +83,14 @@ const (
 
 func makeOptions(target authn.Resource, opts ...Option) (*options, error) {
 	o := &options{
-		auth:      authn.Anonymous,
-		transport: http.DefaultTransport,
-		platform:  defaultPlatform,
-		context:   context.Background(),
-		jobs:      defaultJobs,
-		pageSize:  defaultPageSize,
+		auth:           authn.Anonymous,
+		transport:      http.DefaultTransport,
+		platform:       defaultPlatform,
+		context:        context.Background(),
+		jobs:           defaultJobs,
+		pageSize:       defaultPageSize,
+		retryPredicate: defaultRetryPredicate,
+		retryBackoff:   defaultRetryBackoff,
 	}
 
 	for _, option := range opts {
@@ -78,19 +107,23 @@ func makeOptions(target authn.Resource, opts ...Option) (*options, error) {
 		o.auth = auth
 	}
 
-	// Wrap the transport in something that logs requests and responses.
-	// It's expensive to generate the dumps, so skip it if we're writing
-	// to nothing.
-	if logs.Enabled(logs.Debug) {
-		o.transport = transport.NewLogger(o.transport)
-	}
+	// transport.Wrapper is a signal that consumers are opt-ing into providing their own transport without any additional wrapping.
+	// This is to allow consumers full control over the transports logic, such as providing retry logic.
+	if _, ok := o.transport.(*transport.Wrapper); !ok {
+		// Wrap the transport in something that logs requests and responses.
+		// It's expensive to generate the dumps, so skip it if we're writing
+		// to nothing.
+		if logs.Enabled(logs.Debug) {
+			o.transport = transport.NewLogger(o.transport)
+		}
 
-	// Wrap the transport in something that can retry network flakes.
-	o.transport = transport.NewRetry(o.transport)
+		// Wrap the transport in something that can retry network flakes.
+		o.transport = transport.NewRetry(o.transport)
 
-	// Wrap this last to prevent transport.New from double-wrapping.
-	if o.userAgent != "" {
-		o.transport = transport.NewUserAgent(o.transport, o.userAgent)
+		// Wrap this last to prevent transport.New from double-wrapping.
+		if o.userAgent != "" {
+			o.transport = transport.NewUserAgent(o.transport, o.userAgent)
+		}
 	}
 
 	return o, nil
@@ -98,6 +131,8 @@ func makeOptions(target authn.Resource, opts ...Option) (*options, error) {
 
 // WithTransport is a functional option for overriding the default transport
 // for remote operations.
+// If transport.Wrapper is provided, this signals that the consumer does *not* want any further wrapping to occur.
+// i.e. logging, retry and useragent
 //
 // The default transport its http.DefaultTransport.
 func WithTransport(t http.RoundTripper) Option {
@@ -212,3 +247,19 @@ func WithPageSize(size int) Option {
 		return nil
 	}
 }
+
+// WithRetryBackoff sets the httpBackoff for retry HTTP operations.
+func WithRetryBackoff(backoff Backoff) Option {
+	return func(o *options) error {
+		o.retryBackoff = backoff
+		return nil
+	}
+}
+
+// WithRetryPredicate sets the predicate for retry HTTP operations.
+func WithRetryPredicate(predicate retry.Predicate) Option {
+	return func(o *options) error {
+		o.retryPredicate = predicate
+		return nil
+	}
+}
diff --git a/pkg/v1/remote/transport/error.go b/pkg/v1/remote/transport/error.go
index 154b3036..2c57f1a3 100644
--- a/pkg/v1/remote/transport/error.go
+++ b/pkg/v1/remote/transport/error.go
@@ -46,10 +46,10 @@ type Error struct {
 	Errors []Diagnostic `json:"errors,omitempty"`
 	// The http status code returned.
 	StatusCode int
+	// The request that failed.
+	Request *http.Request
 	// The raw body if we couldn't understand it.
 	rawBody string
-	// The request that failed.
-	request *http.Request
 }
 
 // Check that Error implements error
@@ -58,8 +58,8 @@ var _ error = (*Error)(nil)
 // Error implements error
 func (e *Error) Error() string {
 	prefix := ""
-	if e.request != nil {
-		prefix = fmt.Sprintf("%s %s: ", e.request.Method, redactURL(e.request.URL))
+	if e.Request != nil {
+		prefix = fmt.Sprintf("%s %s: ", e.Request.Method, redactURL(e.Request.URL))
 	}
 	return prefix + e.responseErr()
 }
@@ -68,7 +68,7 @@ func (e *Error) responseErr() string {
 	switch len(e.Errors) {
 	case 0:
 		if len(e.rawBody) == 0 {
-			if e.request != nil && e.request.Method == http.MethodHead {
+			if e.Request != nil && e.Request.Method == http.MethodHead {
 				return fmt.Sprintf("unexpected status code %d %s (HEAD responses have no body, use GET for details)", e.StatusCode, http.StatusText(e.StatusCode))
 			}
 			return fmt.Sprintf("unexpected status code %d %s", e.StatusCode, http.StatusText(e.StatusCode))
@@ -194,7 +194,7 @@ func CheckError(resp *http.Response, codes ...int) error {
 
 	structuredError.rawBody = string(b)
 	structuredError.StatusCode = resp.StatusCode
-	structuredError.request = resp.Request
+	structuredError.Request = resp.Request
 
 	return structuredError
 }
diff --git a/pkg/v1/remote/transport/retry.go b/pkg/v1/remote/transport/retry.go
index 7f7d1e45..0a45dc75 100644
--- a/pkg/v1/remote/transport/retry.go
+++ b/pkg/v1/remote/transport/retry.go
@@ -46,8 +46,11 @@ type options struct {
 	predicate retry.Predicate
 }
 
+// Backoff is an alias of retry.Backoff to expose this configuration option to consumers of this lib
+type Backoff = retry.Backoff
+
 // WithRetryBackoff sets the backoff for retry operations.
-func WithRetryBackoff(backoff retry.Backoff) Option {
+func WithRetryBackoff(backoff Backoff) Option {
 	return func(o *options) {
 		o.backoff = backoff
 	}
diff --git a/pkg/v1/remote/transport/transport.go b/pkg/v1/remote/transport/transport.go
index 5c35fc7c..ea40e3a4 100644
--- a/pkg/v1/remote/transport/transport.go
+++ b/pkg/v1/remote/transport/transport.go
@@ -69,9 +69,9 @@ func NewWithContext(ctx context.Context, reg name.Registry, auth authn.Authentic
 
 	switch pr.challenge.Canonical() {
 	case anonymous:
-		return t, nil
+		return &Wrapper{t}, nil
 	case basic:
-		return &basicTransport{inner: t, auth: auth, target: reg.RegistryStr()}, nil
+		return &Wrapper{&basicTransport{inner: t, auth: auth, target: reg.RegistryStr()}}, nil
 	case bearer:
 		// We require the realm, which tells us where to send our Basic auth to turn it into Bearer auth.
 		realm, ok := pr.parameters["realm"]
@@ -96,8 +96,19 @@ func NewWithContext(ctx context.Context, reg name.Registry, auth authn.Authentic
 		if err := bt.refresh(ctx); err != nil {
 			return nil, err
 		}
-		return bt, nil
+		return &Wrapper{bt}, nil
 	default:
 		return nil, fmt.Errorf("unrecognized challenge: %s", pr.challenge)
 	}
 }
+
+// Wrapper results in *not* wrapping supplied transport with additional logic such as retries, useragent and debug logging
+// Consumers are opt-ing into providing their own transport without any additional wrapping.
+type Wrapper struct {
+	inner http.RoundTripper
+}
+
+// RoundTrip delegates to the inner RoundTripper
+func (w *Wrapper) RoundTrip(in *http.Request) (*http.Response, error) {
+	return w.inner.RoundTrip(in)
+}
diff --git a/pkg/v1/remote/write.go b/pkg/v1/remote/write.go
index a894d8e9..663bb024 100644
--- a/pkg/v1/remote/write.go
+++ b/pkg/v1/remote/write.go
@@ -24,8 +24,6 @@ import (
 	"net/url"
 	"strings"
 	"sync/atomic"
-	"syscall"
-	"time"
 
 	"github.com/google/go-containerregistry/internal/redact"
 	"github.com/google/go-containerregistry/internal/retry"
@@ -80,6 +78,8 @@ func writeImage(ctx context.Context, ref name.Reference, img v1.Image, o *option
 		context:    ctx,
 		updates:    o.updates,
 		lastUpdate: lastUpdate,
+		backoff:    o.retryBackoff,
+		predicate:  o.retryPredicate,
 	}
 
 	// Upload individual blobs and collect any errors.
@@ -176,6 +176,8 @@ type writer struct {
 
 	updates    chan<- v1.Update
 	lastUpdate *v1.Update
+	backoff    Backoff
+	predicate  retry.Predicate
 }
 
 func sendError(ch chan<- v1.Update, err error) error {
@@ -406,24 +408,6 @@ func (w *writer) incrProgress(written int64) {
 	}
 }
 
-var shouldRetry retry.Predicate = func(err error) bool {
-	// Various failure modes here, as we're often reading from and writing to
-	// the network.
-	if retry.IsTemporary(err) || errors.Is(err, io.ErrUnexpectedEOF) || errors.Is(err, syscall.EPIPE) {
-		logs.Warn.Printf("retrying %v", err)
-		return true
-	}
-	return false
-}
-
-// Try this three times, waiting 1s after first failure, 3s after second.
-var backoff = retry.Backoff{
-	Duration: 1.0 * time.Second,
-	Factor:   3.0,
-	Jitter:   0.1,
-	Steps:    3,
-}
-
 // uploadOne performs a complete upload of a single layer.
 func (w *writer) uploadOne(ctx context.Context, l v1.Layer) error {
 	var from, mount string
@@ -503,7 +487,7 @@ func (w *writer) uploadOne(ctx context.Context, l v1.Layer) error {
 		return nil
 	}
 
-	return retry.Retry(tryUpload, shouldRetry, backoff)
+	return retry.Retry(tryUpload, w.predicate, w.backoff)
 }
 
 type withLayer interface {
@@ -644,7 +628,7 @@ func (w *writer) commitManifest(ctx context.Context, t Taggable, ref name.Refere
 		return nil
 	}
 
-	return retry.Retry(tryUpload, shouldRetry, backoff)
+	return retry.Retry(tryUpload, w.predicate, w.backoff)
 }
 
 func scopesForUploadingImage(repo name.Repository, layers []v1.Layer) []string {
@@ -687,10 +671,12 @@ func WriteIndex(ref name.Reference, ii v1.ImageIndex, options ...Option) (rerr e
 		return err
 	}
 	w := writer{
-		repo:    ref.Context(),
-		client:  &http.Client{Transport: tr},
-		context: o.context,
-		updates: o.updates,
+		repo:      ref.Context(),
+		client:    &http.Client{Transport: tr},
+		context:   o.context,
+		updates:   o.updates,
+		backoff:   o.retryBackoff,
+		predicate: o.retryPredicate,
 	}
 
 	if o.updates != nil {
@@ -826,10 +812,12 @@ func WriteLayer(repo name.Repository, layer v1.Layer, options ...Option) (rerr e
 		return err
 	}
 	w := writer{
-		repo:    repo,
-		client:  &http.Client{Transport: tr},
-		context: o.context,
-		updates: o.updates,
+		repo:      repo,
+		client:    &http.Client{Transport: tr},
+		context:   o.context,
+		updates:   o.updates,
+		backoff:   o.retryBackoff,
+		predicate: o.retryPredicate,
 	}
 
 	if o.updates != nil {
@@ -893,9 +881,11 @@ func Put(ref name.Reference, t Taggable, options ...Option) error {
 		return err
 	}
 	w := writer{
-		repo:    ref.Context(),
-		client:  &http.Client{Transport: tr},
-		context: o.context,
+		repo:      ref.Context(),
+		client:    &http.Client{Transport: tr},
+		context:   o.context,
+		backoff:   o.retryBackoff,
+		predicate: o.retryPredicate,
 	}
 
 	return w.commitManifest(o.context, t, ref)


Test patch:
diff --git a/pkg/v1/remote/multi_write_test.go b/pkg/v1/remote/multi_write_test.go
index 7784a503..c16a9e08 100644
--- a/pkg/v1/remote/multi_write_test.go
+++ b/pkg/v1/remote/multi_write_test.go
@@ -15,6 +15,8 @@
 package remote
 
 import (
+	"context"
+	"io"
 	"io/ioutil"
 	"log"
 	"net/http"
@@ -28,6 +30,7 @@ import (
 	"github.com/google/go-containerregistry/pkg/v1/empty"
 	"github.com/google/go-containerregistry/pkg/v1/mutate"
 	"github.com/google/go-containerregistry/pkg/v1/random"
+	"github.com/google/go-containerregistry/pkg/v1/remote/transport"
 	"github.com/google/go-containerregistry/pkg/v1/types"
 	"github.com/google/go-containerregistry/pkg/v1/validate"
 )
@@ -220,6 +223,78 @@ func TestMultiWrite_Retry(t *testing.T) {
 		}
 
 	})
+
+	t.Run("do not retry transport errors if transport.Wrapper is used", func(t *testing.T) {
+		// reference a http server that is not listening (used to pick a port that isn't listening)
+		onlyHandlesPing := http.HandlerFunc(func(responseWriter http.ResponseWriter, request *http.Request) {
+			if strings.HasSuffix(request.URL.Path, "/v2/") {
+				responseWriter.WriteHeader(200)
+				return
+			}
+		})
+		s := httptest.NewServer(onlyHandlesPing)
+		defer s.Close()
+
+		u, err := url.Parse(s.URL)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		tag1 := mustNewTag(t, u.Host+"/repo:tag1")
+
+		// using a transport.Wrapper, meaning retry logic should not be wrapped
+		doesNotRetryTransport := &countTransport{inner: http.DefaultTransport}
+		transportWrapper, err := transport.NewWithContext(context.Background(), tag1.Repository.Registry, nil, doesNotRetryTransport, nil)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		if err := MultiWrite(map[name.Reference]Taggable{
+			tag1: img1,
+		}, WithTransport(transportWrapper), WithJobs(1)); err == nil {
+			t.Errorf("Expected an error, got nil")
+		}
+
+		// expect count == 1 since jobs is set to 1 and we should not retry on transport eof error
+		if doesNotRetryTransport.count != 1 {
+			t.Errorf("Incorrect count, got %d, want %d", doesNotRetryTransport.count, 1)
+		}
+	})
+
+	t.Run("do not add UserAgent if transport.Wrapper is used", func(t *testing.T) {
+		expectedNotUsedUserAgent := "TEST_USER_AGENT"
+
+		handler := registry.New()
+
+		registryThatAssertsUserAgentIsCorrect := http.HandlerFunc(func(responseWriter http.ResponseWriter, request *http.Request) {
+			if strings.Contains(request.Header.Get("User-Agent"), expectedNotUsedUserAgent) {
+				t.Fatalf("Should not contain User-Agent: %s, Got: %s", expectedNotUsedUserAgent, request.Header.Get("User-Agent"))
+			}
+
+			handler.ServeHTTP(responseWriter, request)
+		})
+
+		s := httptest.NewServer(registryThatAssertsUserAgentIsCorrect)
+
+		defer s.Close()
+		u, err := url.Parse(s.URL)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		tag1 := mustNewTag(t, u.Host+"/repo:tag1")
+		// using a transport.Wrapper, meaning retry logic should not be wrapped
+		transportWrapper, err := transport.NewWithContext(context.Background(), tag1.Repository.Registry, nil, http.DefaultTransport, nil)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		if err := MultiWrite(map[name.Reference]Taggable{
+			tag1: img1,
+		}, WithTransport(transportWrapper), WithUserAgent(expectedNotUsedUserAgent)); err != nil {
+			t.Fatal(err)
+		}
+	})
 }
 
 // TestMultiWrite_Deep tests that a deeply nested tree of manifest lists gets
@@ -259,3 +334,17 @@ func TestMultiWrite_Deep(t *testing.T) {
 		t.Error("Validate() =", err)
 	}
 }
+
+type countTransport struct {
+	count int
+	inner http.RoundTripper
+}
+
+func (t *countTransport) RoundTrip(req *http.Request) (*http.Response, error) {
+	if strings.HasSuffix(req.URL.Path, "/v2/") {
+		return t.inner.RoundTrip(req)
+	}
+
+	t.count++
+	return nil, io.ErrUnexpectedEOF
+}
diff --git a/pkg/v1/remote/transport/transport_test.go b/pkg/v1/remote/transport/transport_test.go
index a1046d81..831d8959 100644
--- a/pkg/v1/remote/transport/transport_test.go
+++ b/pkg/v1/remote/transport/transport_test.go
@@ -85,7 +85,9 @@ func TestTransportSelectionBasic(t *testing.T) {
 	if err != nil {
 		t.Errorf("New() = %v", err)
 	}
-	if _, ok := tp.(*basicTransport); !ok {
+	if tpw, ok := tp.(*Wrapper); !ok {
+		t.Errorf("New(); got %T, want *Wrapper", tp)
+	} else if _, ok := tpw.inner.(*basicTransport); !ok {
 		t.Errorf("New(); got %T, want *basicTransport", tp)
 	}
 }
@@ -153,7 +155,9 @@ func TestTransportSelectionBearer(t *testing.T) {
 	if err != nil {
 		t.Errorf("New() = %v", err)
 	}
-	if _, ok := tp.(*bearerTransport); !ok {
+	if tpw, ok := tp.(*Wrapper); !ok {
+		t.Errorf("New(); got %T, want *Wrapper", tp)
+	} else if _, ok := tpw.inner.(*bearerTransport); !ok {
 		t.Errorf("New(); got %T, want *bearerTransport", tp)
 	}
 }
diff --git a/pkg/v1/remote/write_test.go b/pkg/v1/remote/write_test.go
index 81e7f5dd..16bd8979 100644
--- a/pkg/v1/remote/write_test.go
+++ b/pkg/v1/remote/write_test.go
@@ -166,9 +166,11 @@ func setupWriterWithServer(server *httptest.Server, repo string) (*writer, close
 	}
 
 	return &writer{
-		repo:    tag.Context(),
-		client:  http.DefaultClient,
-		context: context.Background(),
+		repo:      tag.Context(),
+		client:    http.DefaultClient,
+		context:   context.Background(),
+		predicate: defaultRetryPredicate,
+		backoff:   defaultRetryBackoff,
 	}, server, nil
 }
 
@@ -917,7 +919,7 @@ func TestWriteWithErrors(t *testing.T) {
 		t.Error("Write() = nil; wanted error")
 	} else if se, ok := err.(*transport.Error); !ok {
 		t.Errorf("Write() = %T; wanted *remote.Error", se)
-	} else if diff := cmp.Diff(expectedError, se, cmpopts.IgnoreUnexported(transport.Error{})); diff != "" {
+	} else if diff := cmp.Diff(expectedError, se, cmpopts.IgnoreFields(transport.Error{}, "Request", "rawBody")); diff != "" {
 		t.Errorf("Write(); (-want +got) = %s", diff)
 	}