Repository: google/go-containerregistry
Pull request #750 (2020-08-03 03:52:38)

Retry on TOOMANYREQUESTS error from ECR
What it says on the tin

I made a test go program [here](https://gist.github.com/jchorl/24aa7797524fbb0b1b59d1d3877cb751) to force 429s. The output was:
```
...
I0803 03:21:58.695399    1477 main.go:44] body: {"errors":[{"code":"TOOMANYREQUESTS","message":"Rate exceeded"}]}
I0803 03:21:58.695412    1477 main.go:45] temporary: false
...
```

Quite obviously, go-containerregistry is not treating `TOOMANYREQUESTS` as temporary even though it is, in fact, a temporary error. This is preventing retries [here](https://github.com/google/go-containerregistry/blob/38ad4ec12b1d707307f901c6675d99e738f4d7e3/pkg/v1/remote/write.go#L377) because the error _does_ implement the `temporary` interface ([here](https://github.com/google/go-containerregistry/blob/38ad4ec12b1d707307f901c6675d99e738f4d7e3/pkg/internal/retry/retry.go#L33-L35)) but this error is deemed non-temporary.

The error manifested itself when kaniko was trying to push a bunch of images in parallel:
```
error pushing image: failed to push to destination <redacted>.dkr.ecr.<redacted>.amazonaws.com/<redacted>: POST https://<redacted>.dkr.ecr.<redacted>.amazonaws.com/v2/<redacted>/blobs/uploads/: TOOMANYREQUESTS: Rate exceeded
```

The error is explained in the registry spec here: https://github.com/docker/distribution/blob/master/docs/spec/api.md#errors-2

Anyway, this change causes go-containerregistry to treat a 429 as a retryable error.



codecov-commenter: # [Codecov](https://codecov.io/gh/google/go-containerregistry/pull/750?src=pr&el=h1) Report
> Merging [#750](https://codecov.io/gh/google/go-containerregistry/pull/750?src=pr&el=desc) into [master](https://codecov.io/gh/google/go-containerregistry/commit/38ad4ec12b1d707307f901c6675d99e738f4d7e3&el=desc) will **increase** coverage by `0.01%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/google/go-containerregistry/pull/750/graphs/tree.svg?width=650&height=150&src=pr&token=wG7iiDCdZ4)](https://codecov.io/gh/google/go-containerregistry/pull/750?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #750      +/-   ##
==========================================
+ Coverage   79.06%   79.08%   +0.01%     
==========================================
  Files         102      102              
  Lines        4725     4729       +4     
==========================================
+ Hits         3736     3740       +4     
  Misses        548      548              
  Partials      441      441              
```


| [Impacted Files](https://codecov.io/gh/google/go-containerregistry/pull/750?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/v1/remote/transport/error.go](https://codecov.io/gh/google/go-containerregistry/pull/750/diff?src=pr&el=tree#diff-cGtnL3YxL3JlbW90ZS90cmFuc3BvcnQvZXJyb3IuZ28=) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/google/go-containerregistry/pull/750?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/google/go-containerregistry/pull/750?src=pr&el=footer). Last update [38ad4ec...4281d34](https://codecov.io/gh/google/go-containerregistry/pull/750?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

jchorl: Agreed, backoff behaviour would be nice. The [current retry behaviour](https://github.com/google/go-containerregistry/blob/38ad4ec12b1d707307f901c6675d99e738f4d7e3/pkg/v1/remote/write.go#L377) waits 1/3/9(?) seconds, which might deal with overloading a registry. I'd also be in favour of bumping those timeouts. Having different exponential backoff behaviour depending on the error would... be ambitious ;)
jonjohnsonjr: Let's just stick with this and revisit something more sophisticated if you're still seeing issues.

Patch:
diff --git a/pkg/v1/remote/transport/error.go b/pkg/v1/remote/transport/error.go
index 0f0e92af..62f95363 100644
--- a/pkg/v1/remote/transport/error.go
+++ b/pkg/v1/remote/transport/error.go
@@ -89,8 +89,7 @@ func (e *Error) Temporary() bool {
 		return false
 	}
 	for _, d := range e.Errors {
-		// TODO: Include other error types.
-		if d.Code != BlobUploadInvalidErrorCode {
+		if _, ok := temporaryErrorCodes[d.Code]; !ok {
 			return false
 		}
 	}
@@ -149,8 +148,15 @@ const (
 	UnauthorizedErrorCode        ErrorCode = "UNAUTHORIZED"
 	DeniedErrorCode              ErrorCode = "DENIED"
 	UnsupportedErrorCode         ErrorCode = "UNSUPPORTED"
+	TooManyRequestsErrorCode     ErrorCode = "TOOMANYREQUESTS"
 )
 
+// TODO: Include other error types.
+var temporaryErrorCodes = map[ErrorCode]struct{}{
+	BlobUploadInvalidErrorCode: struct{}{},
+	TooManyRequestsErrorCode:   struct{}{},
+}
+
 // CheckError returns a structured error if the response status is not in codes.
 func CheckError(resp *http.Response, codes ...int) error {
 	for _, code := range codes {


Test patch:
diff --git a/pkg/v1/remote/transport/error_test.go b/pkg/v1/remote/transport/error_test.go
index ededc509..a6036972 100644
--- a/pkg/v1/remote/transport/error_test.go
+++ b/pkg/v1/remote/transport/error_test.go
@@ -50,6 +50,13 @@ func TestTemporary(t *testing.T) {
 			}},
 		},
 		retry: false,
+	}, {
+		error: &Error{
+			Errors: []Diagnostic{{
+				Code: TooManyRequestsErrorCode,
+			}},
+		},
+		retry: true,
 	}}
 
 	for _, test := range tests {