Check if response has unformatted error details by cpuguy83 · Pull Request #4674 · containerd/containerd

cpuguy83 · 2020-10-29T17:02:54Z

In some cases a registry may not return a formaatted message, in this
case try to get error details from the response body as a simple string.

Closes #4672

cpuguy83 · 2020-10-29T17:03:09Z

Ping @thaJeztah @tonistiigi WDYT?

theopenlab-ci · 2020-10-29T17:11:42Z

Build succeeded.

containerd-build-arm64 : FAILURE in 5m 44s (non-voting)

estesp · 2020-10-29T17:13:19Z

Looks like there is some string matching in TestDockerFetcherOpen that is broken by this change

cpuguy83 · 2020-10-29T17:29:25Z

It like the error handling is just incorrect anyway.
I've actually noticed containerd is always missing error details.

The specific case of 429 returns an object rather than an array:

{
  "details": "Too Many Requests. Please see https://docs.docker.com/docker-hub/download-rate-limit/",
  "error": {
    "details": "Too Many Requests. Please see https://docs.docker.com/docker-hub/download-rate-limit/"
  }
}

cpuguy83 · 2020-10-29T17:30:54Z

But handling the case of this possibly being just an unencoded message is probably OK as well, e.g. maybe the registry is in a degraded state.

estesp · 2020-10-29T17:37:15Z

What I'm confused about is that it was "fixed" in #3109 (I'm pretty sure @bainsy88 confirmed at the time that the full error message was getting passed through after that PR), and shouldn't have been broken by #3173, at least on its face, since it is still trying to decode into the same value, but with the more canonical approach of letting the decoder read the stream rather than the "ReadAll(..)".

thaJeztah · 2020-10-29T17:39:16Z

Is it because the registry in this case returns application/json, but a format that doesn't match?

estesp · 2020-10-29T17:41:29Z

Oh, I see Brian's point--the response is not an array, so it's not going to decode into Errors; could that have changed at some point with DockerHub? Or maybe the original PR from the IBM registry team still works properly with ICCR/icr.io responses, which may properly be encoded as a JSON array

cpuguy83 · 2020-10-29T17:42:20Z

We could update UnmarshalJSON for Errors to check for this case.

`Errors` here should used lowercase `errors` in the json. This should fix issues where error details are not present when the registry returns a non-OK response. Signed-off-by: Brian Goff <[email protected]>

1. See if only a single error is returned if there were no errors in the errors list. 2. Fallback to legacy message type from https://github.com/docker/distribution/blob/749f6afb4572201e3c37325d0ffedb6f32be8950/registry/client/errors.go#L48-L52 Signed-off-by: Brian Goff <[email protected]>

Makes suere we do the same thing in either case and do our best to get an error. Also adds an additional fallback for plain text error responses. Signed-off-by: Brian Goff <[email protected]>

cpuguy83 · 2020-10-29T23:15:11Z

Ok, updated this with some commits split up.

The error unmarshalling was incorrect to begin with. It was looking for Errors in json instead of errors, which explains why I've seen missing error details when using containerd libs.
Add fallback to handle:
i. If error is a single error instead of a { Errors: []Error }
ii. Additional fallback for legacy behavior like https://github.com/docker/distribution/blob/749f6afb4572201e3c37325d0ffedb6f32be8950/registry/client/errors.go#L48-L52
Coalesce error handling for resolve and fetch, with an additional fallback if the error is plain text.

cpuguy83 · 2020-10-29T23:16:51Z

$ bin/ctr image pull docker.io/ratelimitalways/test:latest
docker.io/ratelimitalways/test:latest:                                            resolved       |++++++++++++++++++++++++++++++++++++++| 
manifest-sha256:c2d41d2ba6d8b7b4a3ffec621578eb4d9a0909df29dfa2f6fd8a2e5fd0836aed: downloading    |--------------------------------------|    0.0 B/527.0 B 
elapsed: 2.6 s                                                                    total:   0.0 B (0.0 B/s)                                         
ctr: failed to copy: httpReaderSeeker: failed open: unexpected status code https://registry-1.docker.io/v2/ratelimitalways/test/manifests/sha256:c2d41d2ba6d8b7b4a3ffec621578eb4d9a0909df29dfa2f6fd8a2e5fd0836aed: 429 Too Many Requests - Server message: unknown: Too Many Requests. Please see https://docs.docker.com/docker-hub/download-rate-limit/

theopenlab-ci · 2020-10-29T23:17:25Z

Build succeeded.

containerd-build-arm64 : FAILURE in 5m 44s (non-voting)

thaJeztah · 2020-10-29T23:26:19Z

curious; where is the unknown coming from in Server message: unknown ?

cpuguy83 · 2020-10-29T23:31:04Z

It's from the errcode being 0, since the server is not giving us one in the json.

estesp

LGTM

crosbymichael · 2020-10-30T14:58:49Z

LGTM but I'll let @dmcgowan do the final check since he knows this better than I do.

dmcgowan · 2020-10-30T15:51:57Z

remotes/docker/resolver.go

 					continue
 				}
-				return "", ocispec.Descriptor{}, errors.Errorf("unexpected status code %v: %v", u, resp.Status)
+				return "", ocispec.Descriptor{}, getResponseErr(ctx, req, resp)


There is a resp.Body.Close() a few lines above this

thaJeztah · 2020-10-30T15:56:26Z

Should we decide wether or not we must support the invalid JSON response?

thaJeztah · 2020-10-30T15:58:59Z

i.e.; I agree that plain text responses should be returned; mostly wondering where the "backward compat" part in moby / distribution came from, and if it's still relevant (not taking the current Docker Hub response into account) #4672 (comment)

dmcgowan · 2020-10-30T16:02:31Z

remotes/docker/fetcher.go

+		if err2 != nil {
+			log.G(ctx).WithError(err2).Debug("Error copying response body details")
+		}
+		return errors.Errorf("unexpected status code %v: %v, details: %s", req.String(), resp.Status, sb)


I would rather us type this error, shoveling the raw http body into an error string still doesn't seem right. The cases this occurs is if the registry is not implementing the same error handling format or the response is from middleware. The output is more likely to be mangled html than a valid error message.

We could wrap the registry error type in a response error which holds the request, status code, registry error type. In this case, we can use the unknown registry error type and let the caller decide if it wants to present details.

The output is more likely to be mangled html than a valid error message.

Distribution spec is a bit sketchy on that ("If the response body is in JSON format"); i.e. it mentions the body to be JSON format, but doesn't mention wether or not that requires the content-type to be application/json (i.e., should a text/plain response be attempted to parse as JSON?)

We could wrap the registry error type in a response error which holds the request, status code, registry error type. In this case, we can use the unknown registry error type and let the caller decide if it wants to present details.

You mean; preserve the body, but convert it to a "valid" format? I think that would work; at least the information would not be lost, and clients should be able to handle the standard format (how to handle other formats is somewhat "unspecified": "A 4XX response code from the registry MAY return a body in any format" )

@tonistiigi additional thoughts on the above? ^^

@dmcgowan That's a fair point. It also kinda sucks to expect all callers to be aware of this to make that determination.
I'd be fine dropping that, or possibly checking explicitly text/plain as the content type.

Can you just dump the body into the log and mention the status code is unexpected and the error is not formatted.

thaJeztah · 2020-10-30T16:03:43Z

quick search in the docker/distribution repository; distribution/distribution@ec636bb distribution/distribution#1379

distribution/distribution#1249 changed token fetching to parse HTTP error response bodies as serialized errcodes. However, Docker Hub's authentication endpoint does not return error bodies in this format. To work around this, convert its format into ErrCodeUnauthorized or ErrCodeUnknown.

fixes / related to distribution/distribution#1373
related to moby/moby#19476

estesp · 2022-01-18T18:59:33Z

this PR now has conflicts with main; given the last few comments, is there a path forward with @dmcgowan's suggestion of a typed error and/or logging the body content?

dmcgowan · 2022-04-22T17:04:57Z

There is still a tracking issue, going to close this for now. We do need to address better remote error response handling still.

cpuguy83 requested review from crosbymichael and dmcgowan October 29, 2020 17:03

cpuguy83 force-pushed the docker_errors_details branch from e71df13 to 3880fa9 Compare October 29, 2020 17:04

cpuguy83 mentioned this pull request Oct 29, 2020

DockerFetcher swallows non-JSON errors returned by registries #4672

Closed

cpuguy83 added 3 commits October 29, 2020 23:03

docker remote: Fix error unmarshaling

1ace75e

`Errors` here should used lowercase `errors` in the json. This should fix issues where error details are not present when the registry returns a non-OK response. Signed-off-by: Brian Goff <[email protected]>

docker remote: Coalesce errs for resolve/pull

a8dd01d

Makes suere we do the same thing in either case and do our best to get an error. Also adds an additional fallback for plain text error responses. Signed-off-by: Brian Goff <[email protected]>

cpuguy83 force-pushed the docker_errors_details branch from 3880fa9 to a8dd01d Compare October 29, 2020 23:10

estesp approved these changes Oct 30, 2020

View reviewed changes

crosbymichael approved these changes Oct 30, 2020

View reviewed changes

dmcgowan reviewed Oct 30, 2020

View reviewed changes

dmcgowan added the status/needs-update Awaiting contributor update label May 27, 2021

dmcgowan closed this Apr 22, 2022

thaJeztah mentioned this pull request Jun 29, 2022

buildkit not propagating unauthorized error message moby/buildkit#2938

Open

kzys mentioned this pull request Oct 20, 2022

DockerFetcher should return the original payload even if it is non-JSON #7564

Closed

Conversation

cpuguy83 commented Oct 29, 2020

Uh oh!

cpuguy83 commented Oct 29, 2020

Uh oh!

theopenlab-ci bot commented Oct 29, 2020

Uh oh!

estesp commented Oct 29, 2020

Uh oh!

cpuguy83 commented Oct 29, 2020

Uh oh!

cpuguy83 commented Oct 29, 2020

Uh oh!

estesp commented Oct 29, 2020

Uh oh!

thaJeztah commented Oct 29, 2020

Uh oh!

estesp commented Oct 29, 2020

Uh oh!

cpuguy83 commented Oct 29, 2020

Uh oh!

cpuguy83 commented Oct 29, 2020

Uh oh!

cpuguy83 commented Oct 29, 2020

Uh oh!

theopenlab-ci bot commented Oct 29, 2020

Uh oh!

thaJeztah commented Oct 29, 2020

Uh oh!

cpuguy83 commented Oct 29, 2020

Uh oh!

estesp left a comment

Choose a reason for hiding this comment

Uh oh!

crosbymichael commented Oct 30, 2020

Uh oh!

dmcgowan Oct 30, 2020

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Oct 30, 2020

Uh oh!

thaJeztah commented Oct 30, 2020

Uh oh!

dmcgowan Oct 30, 2020

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 30, 2020

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 30, 2020

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Oct 30, 2020

Choose a reason for hiding this comment

Uh oh!

dmcgowan May 19, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Oct 30, 2020

Uh oh!

estesp commented Jan 18, 2022

Uh oh!

dmcgowan commented Apr 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants