-
Notifications
You must be signed in to change notification settings - Fork 380
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
docker: handle http 429 status codes #703
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
package docker | ||
|
||
import ( | ||
"errors" | ||
"net/http" | ||
|
||
perrors "github.com/pkg/errors" | ||
) | ||
|
||
var ( | ||
// ErrV1NotSupported is returned when we're trying to talk to a | ||
// docker V1 registry. | ||
ErrV1NotSupported = errors.New("can't talk to a V1 docker registry") | ||
// 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") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds like a nice improvement! |
||
) | ||
|
||
// httpResponseToError translates the https.Response into an error. It returns | ||
// nil if the response is not considered an error. | ||
func httpResponseToError(res *http.Response) error { | ||
switch res.StatusCode { | ||
case http.StatusOK: | ||
return nil | ||
case http.StatusTooManyRequests: | ||
return ErrTooManyRequests | ||
case http.StatusUnauthorized: | ||
return ErrUnauthorizedForCredentials | ||
default: | ||
return perrors.Errorf("invalid status code from registry %d (%s)", res.StatusCode, http.StatusText(res.StatusCode)) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropping
client.HandleErrorResponse(res)
seems rather undesirable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case, I'd switch all checks to that to have ~consistent error messaging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vendor/github.com/docker/distribution/registry/api/v2/errors.go
just map tohttp.StatusBadRequest
, andisManifestInvalidError
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That won't work for all responses:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quay.io is known to be non-compliant, what else is new?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's news for me at least. I'll try with nginx pointed to another registry...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.