docker: handle http 429 status codes #703

vrothberg · 2019-09-17T13:47:56Z

Consolidate checking the http-status codes to allow for a more uniform
error handling. Also treat code 429 (too many requests) as a known
error instead of an invalid status code.

When hitting 429, perform an exponential back off starting a 2 seconds
for at most 5 iterations.

Signed-off-by: Valentin Rothberg [email protected]

vrothberg · 2019-09-17T13:49:26Z

Fixes: #618

docker/docker_client.go

vrothberg

Thanks for the reviews, @MartinBasti and @twaugh.

As we're already on it: we're not (yet) looking for the Retry-After but can easily add support for it. The header can be both, an http date or a numerical value for the delay in seconds. Both can be exploited by a malicious server, so we had to set a maximum. Hence the questions:

What should the maximum delay in seconds be?
Should we attempt exponential back offs for the values from the header as well?

docker/docker_client.go

twaugh · 2019-09-17T15:03:21Z

* What should the maximum delay in seconds be?

Not sure. 120? 60?

* Should we attempt exponential back offs for the values from the header as well?

I don't mind. I don't think it's mandatory, but it's possible it will give better behaviour.

rhatdan

LGTM

mtrmac · 2019-09-18T08:53:44Z

docker/docker_client.go

+	// iterations.
+	delay = 2
+	for i := 0; i < 5; i++ {
+		res, err = c.makeRequestToResolvedURLHelper(ctx, method, url, headers, stream, streamLen, auth, extraScope)


stream can be only consumed once; this fundamentally can’t be done, at least not at this level of abstraction.

Good point. Looking at the call sites, we should not perform any retry when stream != nil which avoids the issue.

mtrmac · 2019-09-18T08:53:51Z

docker/docker_client.go

@@ -627,9 +636,11 @@ func (c *dockerClient) getExtensionsSignatures(ctx context.Context, ref dockerRe
 		return nil, err
 	}
 	defer res.Body.Close()
-	if res.StatusCode != http.StatusOK {
-		return nil, errors.Wrapf(client.HandleErrorResponse(res), "Error downloading signatures for %s in %s", manifestDigest, ref.ref.Name())


Dropping client.HandleErrorResponse(res) seems rather undesirable.

Could you elaborate why? I was found the output of HandleErrorResponse not very user friendly and hard to brain parse. Is there some behavior or output you'd like to keep?

In case, I'd switch all checks to that to have ~consistent error messaging.

It’s the only way to get actual server-produced error messages; dumping the raw HTTP response body would be much worse. Sure, they are often useless, but at least they can be improved.

The failures are machine-identifiable and converted to specific Go types. See how many different failures in See vendor/github.com/docker/distribution/registry/api/v2/errors.go just map to http.StatusBadRequest, and isManifestInvalidError.

In case, I'd switch all checks to that to have ~consistent error messaging.

Sure, as long as the API documents that this format of error reporting is used. That’s not the case for look-aside signatures, at the very least; I haven’t checked the other cases.

In case, I'd switch all checks to that to have ~consistent error messaging.

That won't work for all responses:

error parsing HTTP 429 response body: invalid character '<' looking for beginning of value: "<html>\r\n<head><title>429 Too Many Requests</title></head>\r\n<body>\r\n<center><h1>429 Too Many Requests</h1></center>\r\n<hr><center>nginx/1.17.3</center>\r\n</body>\r\n</html>\r\n"

Quay.io is known to be non-compliant, what else is new?

That's news for me at least. I'll try with nginx pointed to another registry...

I’m sorry, this was too flippant and not helpful. If we are centralizing error handling, adding a centralized workaround for Quay (e.g. “body starts with <html>”) would of course be an option. (Sure, technically it was an option before as well, but sprinkling that workaround all over the package would be too ugly.)

I left this unchanged. Improving and further centralizing error handling sounds like a good idea but is not the intention of this PR. The new function was added to avoid too much boilerplate.

mtrmac · 2019-09-18T09:00:43Z

docker/docker_client.go

+	return res, err
+}
+
+func (c *dockerClient) makeRequestToResolvedURLHelper(ctx context.Context, method, url string, headers map[string][]string, stream io.Reader, streamLen int64, auth sendAuth, extraScope *authScope) (*http.Response, error) {


Helper says nothing, especially without any documentation about its purpose. …WithoutRetries, maybe?

Added a comment and renamed it to makeRequestToResolvedURLOnce.

vrothberg · 2019-09-18T13:45:46Z

Updated. @mtrmac, mind having another look?

I'll tackle the Retry-After handling now.

vrothberg · 2019-09-19T08:59:56Z

We're now handling the Retry-After header for both, numerical values and dates.
@twaugh @MartinBasti @mtrmac PTAL.

twaugh · 2019-09-19T11:10:08Z

LGTM. Thanks!

vrothberg · 2019-09-30T09:27:34Z

Rebased on latest master.

vrothberg · 2019-10-17T10:06:05Z

CI hiccups from Skopeo have been fixed, and we're green again.

@nalind @rhatdan @mrunalp @baude @giuseppe PTAL

rhatdan · 2019-10-17T17:30:05Z

LGTM

nalind · 2019-10-17T17:51:32Z

docker/errors.go

+	// ErrUnauthorizedForCredentials is returned when the status code returned is 401
+	ErrUnauthorizedForCredentials = errors.New("unable to retrieve auth token: invalid username/password")
+	// ErrTooManyRequests is returned when the status code returned is 429
+	ErrTooManyRequests = errors.New("too many request to registry")


github.com/pkg/errors.New() records a stack trace that leads back here. I'm guessing that the standard library's errors.New() would work better, perhaps using github.com/pkg/errors.WithStack() to wrap them before returning them in httpResponseToError().

Sounds like a nice improvement!

docker/docker_client.go

Consolidate checking the http-status codes to allow for a more uniform error handling. Also treat code 429 (too many requests) as a known error instead of an invalid status code. When hitting 429, perform an exponential back off starting a 2 seconds for at most 5 iterations. If the http.Response set the `Retry-Header` then use the provided value or date to compute the delay until the next attempt. Note that the maximum delay is 60 seconds. Signed-off-by: Valentin Rothberg <[email protected]>

vrothberg · 2019-10-18T12:34:46Z

Updated

nalind · 2019-10-18T17:37:02Z

LGTM

vrothberg mentioned this pull request Sep 17, 2019

gracefully handle HTTP 429 return codes (Too Many Requests) #618

Closed

MartinBasti reviewed Sep 17, 2019

View reviewed changes

docker/docker_client.go Outdated Show resolved Hide resolved

MartinBasti reviewed Sep 17, 2019

View reviewed changes

docker/docker_client.go Outdated Show resolved Hide resolved

twaugh reviewed Sep 17, 2019

View reviewed changes

docker/docker_client.go Outdated Show resolved Hide resolved

vrothberg force-pushed the status-checks branch from b40bf88 to 2967616 Compare September 17, 2019 14:36

vrothberg commented Sep 17, 2019

View reviewed changes

twaugh reviewed Sep 17, 2019

View reviewed changes

docker/docker_client.go Outdated Show resolved Hide resolved

vrothberg force-pushed the status-checks branch 2 times, most recently from 994ca9c to abb6f83 Compare September 17, 2019 15:19

rhatdan reviewed Sep 17, 2019

View reviewed changes

mtrmac requested changes Sep 18, 2019

View reviewed changes

mtrmac reviewed Sep 18, 2019

View reviewed changes

vrothberg force-pushed the status-checks branch from abb6f83 to 17c818f Compare September 18, 2019 13:45

vrothberg force-pushed the status-checks branch 2 times, most recently from 26d1a7d to c9d6299 Compare September 19, 2019 08:51

vrothberg force-pushed the status-checks branch from c9d6299 to 9dd1193 Compare September 19, 2019 09:17

mtrmac mentioned this pull request Sep 25, 2019

add http response to log #709

Merged

vrothberg force-pushed the status-checks branch from 9dd1193 to d0b93a2 Compare September 30, 2019 09:19

vrothberg force-pushed the status-checks branch from d0b93a2 to 7b5fe14 Compare October 7, 2019 14:49

vrothberg force-pushed the status-checks branch 2 times, most recently from 90c6f20 to c58f11c Compare October 17, 2019 09:38

nalind reviewed Oct 17, 2019

View reviewed changes

docker/docker_client.go Outdated Show resolved Hide resolved

vrothberg force-pushed the status-checks branch from c58f11c to d51a7ca Compare October 18, 2019 09:50

rhatdan merged commit 98f3f0e into containers:master Oct 18, 2019

mtrmac mentioned this pull request Dec 11, 2019

Retry logic cleanups #774

Merged

mtrmac mentioned this pull request Jan 31, 2020

Podman Pull Images always error when docker not containers/podman#4251

Closed

dymurray mentioned this pull request Jun 9, 2020

Add retry logic for image copying migtools/openshift-migration-plugin#63

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docker: handle http 429 status codes #703

docker: handle http 429 status codes #703

vrothberg commented Sep 17, 2019 •

edited

Loading

vrothberg commented Sep 17, 2019

vrothberg left a comment

twaugh commented Sep 17, 2019

rhatdan left a comment

mtrmac Sep 18, 2019

vrothberg Sep 18, 2019

mtrmac Sep 18, 2019

vrothberg Sep 18, 2019

vrothberg Sep 18, 2019

mtrmac Sep 18, 2019

mtrmac Sep 18, 2019

vrothberg Sep 18, 2019

mtrmac Sep 18, 2019

vrothberg Sep 19, 2019

mtrmac Sep 25, 2019

vrothberg Sep 30, 2019

mtrmac Sep 18, 2019

vrothberg Sep 18, 2019

vrothberg commented Sep 18, 2019

vrothberg commented Sep 19, 2019

twaugh commented Sep 19, 2019

vrothberg commented Sep 30, 2019

vrothberg commented Oct 17, 2019

rhatdan commented Oct 17, 2019

nalind Oct 17, 2019

vrothberg Oct 18, 2019

vrothberg commented Oct 18, 2019

nalind commented Oct 18, 2019

docker: handle http 429 status codes #703

docker: handle http 429 status codes #703

Conversation

vrothberg commented Sep 17, 2019 • edited Loading

vrothberg commented Sep 17, 2019

vrothberg left a comment

Choose a reason for hiding this comment

twaugh commented Sep 17, 2019

rhatdan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrothberg commented Sep 18, 2019

vrothberg commented Sep 19, 2019

twaugh commented Sep 19, 2019

vrothberg commented Sep 30, 2019

vrothberg commented Oct 17, 2019

rhatdan commented Oct 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrothberg commented Oct 18, 2019

nalind commented Oct 18, 2019

vrothberg commented Sep 17, 2019 •

edited

Loading