diff --git a/docker/docker_client.go b/docker/docker_client.go index c7ef38b9d..307fed67f 100644 --- a/docker/docker_client.go +++ b/docker/docker_client.go @@ -313,8 +313,14 @@ func CheckAuth(ctx context.Context, sys *types.SystemContext, username, password return err } defer resp.Body.Close() - - return httpResponseToError(resp, "") + if resp.StatusCode != http.StatusOK { + err := registryHTTPResponseToError(resp) + if resp.StatusCode == http.StatusUnauthorized { + err = ErrUnauthorizedForCredentials{Err: err} + } + return err + } + return nil } // SearchResult holds the information of each matching image @@ -411,7 +417,7 @@ func SearchRegistry(ctx context.Context, sys *types.SystemContext, registry, ima } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - err := httpResponseToError(resp, "") + err := registryHTTPResponseToError(resp) logrus.Errorf("error getting search results from v2 endpoint %q: %v", registry, err) return nil, fmt.Errorf("couldn't search registry %q: %w", registry, err) } @@ -816,7 +822,7 @@ func (c *dockerClient) detectPropertiesHelper(ctx context.Context) error { defer resp.Body.Close() logrus.Debugf("Ping %s status %d", url.Redacted(), resp.StatusCode) if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusUnauthorized { - return httpResponseToError(resp, "") + return registryHTTPResponseToError(resp) } c.challenges = parseAuthHeader(resp.Header) c.scheme = scheme @@ -956,9 +962,10 @@ func (c *dockerClient) getBlob(ctx context.Context, ref dockerReference, info ty if err != nil { return nil, 0, err } - if err := httpResponseToError(res, "Error fetching blob"); err != nil { + if res.StatusCode != http.StatusOK { + err := registryHTTPResponseToError(res) res.Body.Close() - return nil, 0, err + return nil, 0, fmt.Errorf("fetching blob: %w", err) } cache.RecordKnownLocation(ref.Transport(), bicTransportScope(ref), info.Digest, newBICLocationReference(ref)) return res.Body, getBlobSize(res), nil @@ -982,13 +989,8 @@ func (c *dockerClient) getOCIDescriptorContents(ctx context.Context, ref dockerR // isManifestUnknownError returns true iff err from fetchManifest is a “manifest unknown” error. func isManifestUnknownError(err error) bool { - var errs errcode.Errors - if !errors.As(err, &errs) || len(errs) == 0 { - return false - } - err = errs[0] - ec, ok := err.(errcode.ErrorCoder) - if !ok { + var ec errcode.ErrorCoder + if !errors.As(err, &ec) { return false } return ec.ErrorCode() == v2.ErrorCodeManifestUnknown @@ -1037,9 +1039,8 @@ func (c *dockerClient) getExtensionsSignatures(ctx context.Context, ref dockerRe return nil, err } defer res.Body.Close() - if res.StatusCode != http.StatusOK { - return nil, fmt.Errorf("downloading signatures for %s in %s: %w", manifestDigest, ref.ref.Name(), handleErrorResponse(res)) + return nil, fmt.Errorf("downloading signatures for %s in %s: %w", manifestDigest, ref.ref.Name(), registryHTTPResponseToError(res)) } body, err := iolimits.ReadAtMost(res.Body, iolimits.MaxSignatureListBodySize) diff --git a/docker/docker_client_test.go b/docker/docker_client_test.go index a7c085215..a8d1078cb 100644 --- a/docker/docker_client_test.go +++ b/docker/docker_client_test.go @@ -1,6 +1,8 @@ package docker import ( + "bufio" + "bytes" "context" "errors" "fmt" @@ -328,3 +330,26 @@ func TestNeedsNoRetry(t *testing.T) { t.Fatal("Got the need to retry, but none should be required") } } + +func TestIsManifestUnknownError(t *testing.T) { + // Mostly a smoke test; we can add more registries here if they need special handling. + + // docker.io when a tag in an _existing repo_ is not found + response := "HTTP/1.1 404 Not Found\r\n" + + "Connection: close\r\n" + + "Content-Length: 109\r\n" + + "Content-Type: application/json\r\n" + + "Date: Thu, 12 Aug 2021 20:51:32 GMT\r\n" + + "Docker-Distribution-Api-Version: registry/2.0\r\n" + + "Ratelimit-Limit: 100;w=21600\r\n" + + "Ratelimit-Remaining: 100;w=21600\r\n" + + "Strict-Transport-Security: max-age=31536000\r\n" + + "\r\n" + + "{\"errors\":[{\"code\":\"MANIFEST_UNKNOWN\",\"message\":\"manifest unknown\",\"detail\":{\"Tag\":\"this-does-not-exist\"}}]}\n" + resp, err := http.ReadResponse(bufio.NewReader(bytes.NewReader([]byte(response))), nil) + require.NoError(t, err) + err = fmt.Errorf("wrapped: %w", registryHTTPResponseToError(resp)) + + res := isManifestUnknownError(err) + assert.True(t, res, "%#v", err) +} diff --git a/docker/docker_image.go b/docker/docker_image.go index 3e8dbbee1..6a4331e33 100644 --- a/docker/docker_image.go +++ b/docker/docker_image.go @@ -77,8 +77,8 @@ func GetRepositoryTags(ctx context.Context, sys *types.SystemContext, ref types. return nil, err } defer res.Body.Close() - if err := httpResponseToError(res, "fetching tags list"); err != nil { - return nil, err + if res.StatusCode != http.StatusOK { + return nil, fmt.Errorf("fetching tags list: %w", registryHTTPResponseToError(res)) } var tagsHolder struct { diff --git a/docker/docker_image_dest.go b/docker/docker_image_dest.go index 44b45c472..80a8efbb0 100644 --- a/docker/docker_image_dest.go +++ b/docker/docker_image_dest.go @@ -244,7 +244,7 @@ func (d *dockerImageDestination) blobExists(ctx context.Context, repo reference. logrus.Debugf("... not present") return false, -1, nil default: - return false, -1, fmt.Errorf("failed to read from destination repository %s: %d (%s)", reference.Path(d.ref.ref), res.StatusCode, http.StatusText(res.StatusCode)) + return false, -1, fmt.Errorf("checking whether a blob %s exists in %s: %w", digest, repo.Name(), registryHTTPResponseToError(res)) } } @@ -487,15 +487,10 @@ func successStatus(status int) bool { return status >= 200 && status <= 399 } -// isManifestInvalidError returns true iff err from client.HandleErrorResponse is a “manifest invalid” error. +// isManifestInvalidError returns true iff err from registryHTTPResponseToError is a “manifest invalid” error. func isManifestInvalidError(err error) bool { - errors, ok := err.(errcode.Errors) - if !ok || len(errors) == 0 { - return false - } - err = errors[0] - ec, ok := err.(errcode.ErrorCoder) - if !ok { + var ec errcode.ErrorCoder + if ok := errors.As(err, &ec); !ok { return false } diff --git a/docker/docker_image_src.go b/docker/docker_image_src.go index a7aa8b088..d726ecd05 100644 --- a/docker/docker_image_src.go +++ b/docker/docker_image_src.go @@ -376,12 +376,9 @@ func (s *dockerImageSource) GetBlobAt(ctx context.Context, info types.BlobInfo, res.Body.Close() return nil, nil, private.BadPartialRequestError{Status: res.Status} default: - err := httpResponseToError(res, "Error fetching partial blob") - if err == nil { - err = fmt.Errorf("invalid status code returned when fetching blob %d (%s)", res.StatusCode, http.StatusText(res.StatusCode)) - } + err := registryHTTPResponseToError(res) res.Body.Close() - return nil, nil, err + return nil, nil, fmt.Errorf("fetching partial blob: %w", err) } } @@ -622,16 +619,16 @@ func deleteImage(ctx context.Context, sys *types.SystemContext, ref dockerRefere return err } defer get.Body.Close() - manifestBody, err := iolimits.ReadAtMost(get.Body, iolimits.MaxManifestBodySize) - if err != nil { - return err - } switch get.StatusCode { case http.StatusOK: case http.StatusNotFound: return fmt.Errorf("Unable to delete %v. Image may not exist or is not stored with a v2 Schema in a v2 registry", ref.ref) default: - return fmt.Errorf("Failed to delete %v: %s (%v)", ref.ref, manifestBody, get.Status) + return fmt.Errorf("deleting %v: %w", ref.ref, registryHTTPResponseToError(get)) + } + manifestBody, err := iolimits.ReadAtMost(get.Body, iolimits.MaxManifestBodySize) + if err != nil { + return err } manifestDigest, err := manifest.Digest(manifestBody) @@ -647,13 +644,8 @@ func deleteImage(ctx context.Context, sys *types.SystemContext, ref dockerRefere return err } defer delete.Body.Close() - - body, err := iolimits.ReadAtMost(delete.Body, iolimits.MaxErrorBodySize) - if err != nil { - return err - } if delete.StatusCode != http.StatusAccepted { - return fmt.Errorf("Failed to delete %v: %s (%v)", deletePath, string(body), delete.Status) + return fmt.Errorf("deleting %v: %w", ref.ref, registryHTTPResponseToError(delete)) } for i := 0; ; i++ { diff --git a/docker/errors.go b/docker/errors.go index 9c02e27e3..74fe17648 100644 --- a/docker/errors.go +++ b/docker/errors.go @@ -4,6 +4,9 @@ import ( "errors" "fmt" "net/http" + + "github.com/docker/distribution/registry/api/errcode" + "github.com/sirupsen/logrus" ) var ( @@ -33,7 +36,7 @@ func httpResponseToError(res *http.Response, context string) error { case http.StatusTooManyRequests: return ErrTooManyRequests case http.StatusUnauthorized: - err := handleErrorResponse(res) + err := registryHTTPResponseToError(res) return ErrUnauthorizedForCredentials{Err: err} default: if context != "" { @@ -47,12 +50,47 @@ func httpResponseToError(res *http.Response, context string) error { // registry func registryHTTPResponseToError(res *http.Response) error { err := handleErrorResponse(res) - if e, ok := err.(*unexpectedHTTPResponseError); ok { + // len(errs) == 0 should never be returned by handleErrorResponse; if it does, we don't modify it and let the caller report it as is. + if errs, ok := err.(errcode.Errors); ok && len(errs) > 0 { + // The docker/distribution registry implementation almost never returns + // more than one error in the HTTP body; it seems there is only one + // possible instance, where the second error reports a cleanup failure + // we don't really care about. + // + // The only _common_ case where a multi-element error is returned is + // created by the handleErrorResponse parser when OAuth authorization fails: + // the first element contains errors from a WWW-Authenticate header, the second + // element contains errors from the response body. + // + // In that case the first one is currently _slightly_ more informative (ErrorCodeUnauthorized + // for invalid tokens, ErrorCodeDenied for permission denied with a valid token + // for the first error, vs. ErrorCodeUnauthorized for both cases for the second error.) + // + // Also, docker/docker similarly only logs the other errors and returns the + // first one. + if len(errs) > 1 { + logrus.Debugf("Discarding non-primary errors:") + for _, err := range errs[1:] { + logrus.Debugf(" %s", err.Error()) + } + } + err = errs[0] + } + switch e := err.(type) { + case *unexpectedHTTPResponseError: response := string(e.Response) if len(response) > 50 { response = response[:50] + "..." } - err = fmt.Errorf("StatusCode: %d, %s", e.StatusCode, response) + // %.0w makes e visible to error.Unwrap() without including any text + err = fmt.Errorf("StatusCode: %d, %s%.0w", e.StatusCode, response, e) + case errcode.Error: + // e.Error() is fmt.Sprintf("%s: %s", e.Code.Error(), e.Message, which is usually + // rather redundant. So reword it without using e.Code.Error() if e.Message is the default. + if e.Message == e.Code.Message() { + // %.0w makes e visible to error.Unwrap() without including any text + err = fmt.Errorf("%s%.0w", e.Message, e) + } } return err } diff --git a/docker/errors_test.go b/docker/errors_test.go index 72f7b9c39..b7539d494 100644 --- a/docker/errors_test.go +++ b/docker/errors_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/docker/distribution/registry/api/errcode" + v2 "github.com/docker/distribution/registry/api/v2" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -16,12 +17,16 @@ import ( // NOR the error texts are an API commitment subject to API stability expectations; // they can change at any time for any reason. func TestRegistryHTTPResponseToError(t *testing.T) { + var unwrappedUnexpectedHTTPResponseError *unexpectedHTTPResponseError + var unwrappedErrcodeError errcode.Error for _, c := range []struct { name string response string errorString string - errorType interface{} // A value of the same type as the expected error, or nil - unwrappedErrorPtr interface{} // A pointer to a value expected to be reachable using errors.As, or nil + errorType interface{} // A value of the same type as the expected error, or nil + unwrappedErrorPtr interface{} // A pointer to a value expected to be reachable using errors.As, or nil + errorCode *errcode.ErrorCode // A matching ErrorCode, or nil + fn func(t *testing.T, err error) // A more specialized test, or nil }{ { name: "HTTP status out of registry error range", @@ -40,7 +45,7 @@ func TestRegistryHTTPResponseToError(t *testing.T) { "JSON? What JSON?\r\n", errorString: "StatusCode: 400, JSON? What JSON?\r\n", errorType: nil, - unwrappedErrorPtr: nil, + unwrappedErrorPtr: &unwrappedUnexpectedHTTPResponseError, }, { name: "401 body not in expected format", @@ -48,9 +53,10 @@ func TestRegistryHTTPResponseToError(t *testing.T) { "Header1: Value1\r\n" + "\r\n" + "JSON? What JSON?\r\n", - errorString: "unauthorized: authentication required", - errorType: errcode.Error{}, - unwrappedErrorPtr: nil, + errorString: "authentication required", + errorType: nil, + unwrappedErrorPtr: &unwrappedErrcodeError, + errorCode: &errcode.ErrorCodeUnauthorized, }, { // docker.io when an image is not found name: "GET https://registry-1.docker.io/v2/library/this-does-not-exist/manifests/latest", @@ -64,9 +70,10 @@ func TestRegistryHTTPResponseToError(t *testing.T) { "Www-Authenticate: Bearer realm=\"https://auth.docker.io/token\",service=\"registry.docker.io\",scope=\"repository:library/this-does-not-exist:pull\",error=\"insufficient_scope\"\r\n" + "\r\n" + "{\"errors\":[{\"code\":\"UNAUTHORIZED\",\"message\":\"authentication required\",\"detail\":[{\"Type\":\"repository\",\"Class\":\"\",\"Name\":\"library/this-does-not-exist\",\"Action\":\"pull\"}]}]}\n", - errorString: "errors:\ndenied: requested access to the resource is denied\nunauthorized: authentication required\n", - errorType: errcode.Errors{}, - unwrappedErrorPtr: nil, + errorString: "requested access to the resource is denied", + errorType: nil, + unwrappedErrorPtr: &unwrappedErrcodeError, + errorCode: &errcode.ErrorCodeDenied, }, { // docker.io when a tag is not found name: "GET https://registry-1.docker.io/v2/library/busybox/manifests/this-does-not-exist", @@ -81,24 +88,36 @@ func TestRegistryHTTPResponseToError(t *testing.T) { "Strict-Transport-Security: max-age=31536000\r\n" + "\r\n" + "{\"errors\":[{\"code\":\"MANIFEST_UNKNOWN\",\"message\":\"manifest unknown\",\"detail\":{\"Tag\":\"this-does-not-exist\"}}]}\n", - errorString: "manifest unknown: manifest unknown", - errorType: errcode.Errors{}, - unwrappedErrorPtr: nil, + errorString: "manifest unknown", + errorType: nil, + unwrappedErrorPtr: &unwrappedErrcodeError, + errorCode: &v2.ErrorCodeManifestUnknown, }, { // public.ecr.aws does not implement tag list name: "GET https://public.ecr.aws/v2/nginx/nginx/tags/list", response: "HTTP/1.1 404 Not Found\r\n" + "Connection: close\r\n" + - "Content-Length: 19\r\n" + - "Content-Type: text/plain; charset=utf-8\r\n" + - "Date: Thu, 12 Aug 2021 19:54:58 GMT\r\n" + + "Content-Length: 65\r\n" + + "Content-Type: application/json; charset=utf-8\r\n" + + "Date: Tue, 06 Sep 2022 21:19:02 GMT\r\n" + "Docker-Distribution-Api-Version: registry/2.0\r\n" + - "X-Content-Type-Options: nosniff\r\n" + "\r\n" + - "404 page not found\n", - errorString: "StatusCode: 404, 404 page not found\n", + "{\"errors\":[{\"code\":\"NOT_FOUND\",\"message\":\"404 page not found\"}]}\r\n", + errorString: "unknown: 404 page not found", errorType: nil, - unwrappedErrorPtr: nil, + unwrappedErrorPtr: &unwrappedErrcodeError, + errorCode: &errcode.ErrorCodeUnknown, + fn: func(t *testing.T, err error) { + var e errcode.Error + ok := errors.As(err, &e) + require.True(t, ok) + // Note: (skopeo inspect) is checking for this errcode.Error value + assert.Equal(t, errcode.Error{ + Code: errcode.ErrorCodeUnknown, // The NOT_FOUND value is not defined, and turns into Unknown + Message: "404 page not found", + Detail: nil, + }, e) + }, }, } { res, err := http.ReadResponse(bufio.NewReader(bytes.NewReader([]byte(c.response))), nil) @@ -113,5 +132,14 @@ func TestRegistryHTTPResponseToError(t *testing.T) { found := errors.As(err, c.unwrappedErrorPtr) assert.True(t, found, c.name) } + if c.errorCode != nil { + var ec errcode.ErrorCoder + ok := errors.As(err, &ec) + require.True(t, ok, c.name) + assert.Equal(t, *c.errorCode, ec.ErrorCode(), c.name) + } + if c.fn != nil { + c.fn(t, err) + } } }