containerd: DockerFetcher swallows non-JSON errors returned by registries

Description

Docker Hub has introduced rate-limits, and now returns a 429 status code if those rate limits are reached, with details about the rate limits in the response body (as plain text).

However, it looks like the containerd “docker” fetcher code does not return the response body if it’s not formatted as JSON.

Steps to reproduce the issue:

When using buildkit (which uses the containerd code to pull images), not details are returned;

DOCKER_BUILDKIT=1 docker build -<<EOF
FROM ratelimitalways/test
EOF
...
failed to load cache key: failed to copy: httpReaderSeeker: failed open: unexpected status code https://registry-1.docker.io/v2/ratelimitalways/test/manifests/sha256:c2d41d2ba6d8b7b4a3ffec621578eb4d9a0909df29dfa2f6fd8a2e5fd0836aed: 429 Too Many Requests

Whereas using the “classic” builder, which uses the docker/distribution code returns the error-message returned by the registry;

DOCKER_BUILDKIT=0 docker build -<<EOF
FROM ratelimitalways/test
EOF

Sending build context to Docker daemon  2.048kB
Step 1/1 : FROM ratelimitalways/test
toomanyrequests: Too Many Requests. Please see https://docs.docker.com/docker-hub/download-rate-limit/

Same when doing a straight docker pull;

docker pull  ratelimitalways/test
Using default tag: latest
Error response from daemon: toomanyrequests: Too Many Requests. Please see https://docs.docker.com/docker-hub/download-rate-limit/

Describe the results you expected:

I expect the pull code to either;

• return details from the JSON error message (if provided), or
• return the plain-text BODY of the response

While a JSON formatted error can be returned, a plain-text body is allowed to be returned as well; looking at the distribution spec; https://github.com/opencontainers/distribution-spec/blob/v1.0.0-rc1/spec.md#error-codes

A 4XX response code from the registry MAY return a body in any format. If the response body is in JSON format, it MUST have the following format:

Digging a bit into the error handling, I see that https://github.com/containerd/containerd/commit/de1da8be32a73a2d92572c2d8e800971215dcb3c (https://github.com/containerd/containerd/pull/3173) may be the culprit. That commit was opened as a follow-up to https://github.com/containerd/containerd/pull/3109, which addressed a similar issue as reported here (https://github.com/containerd/containerd/issues/3076).

I couldn’t find details about https://github.com/containerd/containerd/pull/3173. I see that commit removed the ioutil.ReadAll(resp.Body), which may have been the reason (to prevent a DOS where the body would lead to memory exhaustion). The PR review did have this comment; https://github.com/containerd/containerd/pull/3173#discussion_r272291497

The original PR was submitted to expose servers (registries) providing specific details to help users understand why a specific action failed (e.g. billing, quota, etc.)

So on the bright side, I guess we at least have a way to test server responses.

Any other relevant information:

More of a side-note (probably worth a separate “enhancement” ticket; there may be other omissions in handling of server-responses. I see that “old” docker/distribution code that docker uses is (too) “complicated”, but has more specific handling for errors returned; https://github.com/docker/distribution/blob/749f6afb4572201e3c37325d0ffedb6f32be8950/registry/client/errors.go#L41-L84

And the docker code further improves handling by looking if certain request should be retried; https://github.com/moby/moby/blob/ad1b781e44fa1e44b9e654e5078929aec56aed66/distribution/errors.go#L148-L180