Merge pull request #1299 from mtrmac/collapse-errors

Improve registry client error handling

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)

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)
+}

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 {

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
 	}
 

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++ {

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
 }

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,17 +45,18 @@ func TestRegistryHTTPResponseToError(t *testing.T) {
 				"<html><body>JSON? What JSON?</body></html>\r\n",
 			errorString:       "StatusCode: 400, <html><body>JSON? What JSON?</body></html>\r\n",
 			errorType:         nil,
-			unwrappedErrorPtr: nil,
+			unwrappedErrorPtr: &unwrappedUnexpectedHTTPResponseError,
 		},
 		{
 			name: "401 body not in expected format",
 			response: "HTTP/1.1 401 I don't like this request\r\n" +
 				"Header1: Value1\r\n" +
 				"\r\n" +
 				"<html><body>JSON? What JSON?</body></html>\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)
+		}
 	}
 }