Merge pull request #4052 from thaJeztah/client_refactor_errhandling

registry/client/auth/session.go

@@ -360,8 +360,7 @@ func (th *tokenHandler) fetchTokenWithOAuth(ctx context.Context, realm *url.URL,
 	}
 	defer resp.Body.Close()
 
-	if !client.SuccessStatus(resp.StatusCode) {
+	if err := client.HandleHTTPResponseError(resp); err != nil {
-		err := client.HandleErrorResponse(resp)
 		return "", time.Time{}, err
 	}
 
@@ -443,8 +442,7 @@ func (th *tokenHandler) fetchTokenWithBasicAuth(ctx context.Context, realm *url.
 	}
 	defer resp.Body.Close()
 
-	if !client.SuccessStatus(resp.StatusCode) {
+	if err := client.HandleHTTPResponseError(resp); err != nil {
-		err := client.HandleErrorResponse(resp)
 		return "", time.Time{}, err
 	}
 

registry/client/blob_writer.go

@@ -33,7 +33,7 @@ func (hbu *httpBlobUpload) handleErrorResponse(resp *http.Response) error {
 	if resp.StatusCode == http.StatusNotFound {
 		return distribution.ErrBlobUploadUnknown
 	}
-	return HandleErrorResponse(resp)
+	return HandleHTTPResponseError(resp)
 }
 
 func (hbu *httpBlobUpload) ReadFrom(r io.Reader) (n int64, err error) {
@@ -51,8 +51,8 @@ func (hbu *httpBlobUpload) ReadFrom(r io.Reader) (n int64, err error) {
 	}
 	defer resp.Body.Close()
 
-	if !SuccessStatus(resp.StatusCode) {
+	if err := hbu.handleErrorResponse(resp); err != nil {
-		return 0, hbu.handleErrorResponse(resp)
+		return 0, err
 	}
 
 	hbu.uuid = resp.Header.Get("Docker-Upload-UUID")
@@ -87,8 +87,8 @@ func (hbu *httpBlobUpload) Write(p []byte) (n int, err error) {
 	}
 	defer resp.Body.Close()
 
-	if !SuccessStatus(resp.StatusCode) {
+	if err := hbu.handleErrorResponse(resp); err != nil {
-		return 0, hbu.handleErrorResponse(resp)
+		return 0, err
 	}
 
 	hbu.uuid = resp.Header.Get("Docker-Upload-UUID")
@@ -137,8 +137,8 @@ func (hbu *httpBlobUpload) Commit(ctx context.Context, desc distribution.Descrip
 	}
 	defer resp.Body.Close()
 
-	if !SuccessStatus(resp.StatusCode) {
+	if err := hbu.handleErrorResponse(resp); err != nil {
-		return distribution.Descriptor{}, hbu.handleErrorResponse(resp)
+		return distribution.Descriptor{}, err
 	}
 
 	return hbu.statter.Stat(ctx, desc.Digest)
@@ -155,7 +155,7 @@ func (hbu *httpBlobUpload) Cancel(ctx context.Context) error {
 	}
 	defer resp.Body.Close()
 
-	if resp.StatusCode == http.StatusNotFound || SuccessStatus(resp.StatusCode) {
+	if resp.StatusCode == http.StatusNotFound {
 		return nil
 	}
 	return hbu.handleErrorResponse(resp)

registry/client/errors.go

@@ -116,11 +116,16 @@ func mergeErrors(err1, err2 error) error {
 	return errcode.Errors(append(makeErrorList(err1), makeErrorList(err2)...))
 }
 
-// HandleErrorResponse returns error parsed from HTTP response for an
+// HandleHTTPResponseError returns error parsed from HTTP response, if any.
-// unsuccessful HTTP response code (in the range 400 - 499 inclusive). An
+// It returns nil if no error occurred (HTTP status 200-399), or an error
-// UnexpectedHTTPStatusError returned for response code outside of expected
+// for unsuccessful HTTP response codes (in the range 400 - 499 inclusive).
-// range.
+// If possible, it returns a typed error, but an UnexpectedHTTPStatusError
-func HandleErrorResponse(resp *http.Response) error {
+// is returned for response code outside the expected range (HTTP status < 200
+// and > 500).
+func HandleHTTPResponseError(resp *http.Response) error {
+	if resp.StatusCode >= 200 && resp.StatusCode <= 399 {
+		return nil
+	}
 	if resp.StatusCode >= 400 && resp.StatusCode < 500 {
 		// Check for OAuth errors within the `WWW-Authenticate` header first
 		// See https://tools.ietf.org/html/rfc6750#section-3
@@ -153,8 +158,23 @@ func HandleErrorResponse(resp *http.Response) error {
 	return &UnexpectedHTTPStatusError{Status: resp.Status}
 }
 
+// HandleErrorResponse returns error parsed from HTTP response for an
+// unsuccessful HTTP response code (in the range 400 - 499 inclusive). An
+// UnexpectedHTTPStatusError returned for response code outside of expected
+// range.
+//
+// Deprecated: use [HandleHTTPResponseError] and check the error.
+func HandleErrorResponse(resp *http.Response) error {
+	if resp.StatusCode >= 200 && resp.StatusCode <= 399 {
+		return &UnexpectedHTTPStatusError{Status: resp.Status}
+	}
+	return HandleHTTPResponseError(resp)
+}
+
 // SuccessStatus returns true if the argument is a successful HTTP response
 // code (in the range 200 - 399 inclusive).
+//
+// Deprecated: use [HandleHTTPResponseError] and check the error.
 func SuccessStatus(status int) bool {
 	return status >= 200 && status <= 399
 }

registry/client/errors_test.go

@@ -14,7 +14,18 @@ type nopCloser struct {
 
 func (nopCloser) Close() error { return nil }
 
-func TestHandleErrorResponse401ValidBody(t *testing.T) {
+func TestHandleHTTPResponseError200ValidBody(t *testing.T) {
+	response := &http.Response{
+		Status:     "200 OK",
+		StatusCode: 200,
+	}
+	err := HandleHTTPResponseError(response)
+	if err != nil {
+		t.Errorf("Expected no error, got: %v", err)
+	}
+}
+
+func TestHandleHTTPResponseError401ValidBody(t *testing.T) {
 	json := `{"errors":[{"code":"UNAUTHORIZED","message":"action requires authentication"}]}`
 	response := &http.Response{
 		Status:     "401 Unauthorized",
@@ -22,7 +33,7 @@ func TestHandleErrorResponse401ValidBody(t *testing.T) {
 		Body:       nopCloser{bytes.NewBufferString(json)},
 		Header:     http.Header{"Content-Type": []string{"application/json; charset=utf-8"}},
 	}
-	err := HandleErrorResponse(response)
+	err := HandleHTTPResponseError(response)
 
 	expectedMsg := "unauthorized: action requires authentication"
 	if !strings.Contains(err.Error(), expectedMsg) {
@@ -30,7 +41,7 @@ func TestHandleErrorResponse401ValidBody(t *testing.T) {
 	}
 }
 
-func TestHandleErrorResponse401WithInvalidBody(t *testing.T) {
+func TestHandleHTTPResponseError401WithInvalidBody(t *testing.T) {
 	json := "{invalid json}"
 	response := &http.Response{
 		Status:     "401 Unauthorized",
@@ -38,7 +49,7 @@ func TestHandleErrorResponse401WithInvalidBody(t *testing.T) {
 		Body:       nopCloser{bytes.NewBufferString(json)},
 		Header:     http.Header{"Content-Type": []string{"application/json; charset=utf-8"}},
 	}
-	err := HandleErrorResponse(response)
+	err := HandleHTTPResponseError(response)
 
 	expectedMsg := "unauthorized: authentication required"
 	if !strings.Contains(err.Error(), expectedMsg) {
@@ -46,7 +57,7 @@ func TestHandleErrorResponse401WithInvalidBody(t *testing.T) {
 	}
 }
 
-func TestHandleErrorResponseExpectedStatusCode400ValidBody(t *testing.T) {
+func TestHandleHTTPResponseErrorExpectedStatusCode400ValidBody(t *testing.T) {
 	json := `{"errors":[{"code":"DIGEST_INVALID","message":"provided digest does not match"}]}`
 	response := &http.Response{
 		Status:     "400 Bad Request",
@@ -54,7 +65,7 @@ func TestHandleErrorResponseExpectedStatusCode400ValidBody(t *testing.T) {
 		Body:       nopCloser{bytes.NewBufferString(json)},
 		Header:     http.Header{"Content-Type": []string{"application/json"}},
 	}
-	err := HandleErrorResponse(response)
+	err := HandleHTTPResponseError(response)
 
 	expectedMsg := "digest invalid: provided digest does not match"
 	if !strings.Contains(err.Error(), expectedMsg) {
@@ -62,7 +73,7 @@ func TestHandleErrorResponseExpectedStatusCode400ValidBody(t *testing.T) {
 	}
 }
 
-func TestHandleErrorResponseExpectedStatusCode404EmptyErrorSlice(t *testing.T) {
+func TestHandleHTTPResponseErrorExpectedStatusCode404EmptyErrorSlice(t *testing.T) {
 	json := `{"randomkey": "randomvalue"}`
 	response := &http.Response{
 		Status:     "404 Not Found",
@@ -70,7 +81,7 @@ func TestHandleErrorResponseExpectedStatusCode404EmptyErrorSlice(t *testing.T) {
 		Body:       nopCloser{bytes.NewBufferString(json)},
 		Header:     http.Header{"Content-Type": []string{"application/json; charset=utf-8"}},
 	}
-	err := HandleErrorResponse(response)
+	err := HandleHTTPResponseError(response)
 
 	expectedMsg := `error parsing HTTP 404 response body: no error details found in HTTP response body: "{\"randomkey\": \"randomvalue\"}"`
 	if !strings.Contains(err.Error(), expectedMsg) {
@@ -78,7 +89,7 @@ func TestHandleErrorResponseExpectedStatusCode404EmptyErrorSlice(t *testing.T) {
 	}
 }
 
-func TestHandleErrorResponseExpectedStatusCode404InvalidBody(t *testing.T) {
+func TestHandleHTTPResponseErrorExpectedStatusCode404InvalidBody(t *testing.T) {
 	json := "{invalid json}"
 	response := &http.Response{
 		Status:     "404 Not Found",
@@ -86,7 +97,7 @@ func TestHandleErrorResponseExpectedStatusCode404InvalidBody(t *testing.T) {
 		Body:       nopCloser{bytes.NewBufferString(json)},
 		Header:     http.Header{"Content-Type": []string{"application/json"}},
 	}
-	err := HandleErrorResponse(response)
+	err := HandleHTTPResponseError(response)
 
 	expectedMsg := "error parsing HTTP 404 response body: invalid character 'i' looking for beginning of object key string: \"{invalid json}\""
 	if !strings.Contains(err.Error(), expectedMsg) {
@@ -94,14 +105,14 @@ func TestHandleErrorResponseExpectedStatusCode404InvalidBody(t *testing.T) {
 	}
 }
 
-func TestHandleErrorResponseUnexpectedStatusCode501(t *testing.T) {
+func TestHandleHTTPResponseErrorUnexpectedStatusCode501(t *testing.T) {
 	response := &http.Response{
 		Status:     "501 Not Implemented",
 		StatusCode: 501,
 		Body:       nopCloser{bytes.NewBufferString("{\"Error Encountered\" : \"Function not implemented.\"}")},
 		Header:     http.Header{"Content-Type": []string{"application/json"}},
 	}
-	err := HandleErrorResponse(response)
+	err := HandleHTTPResponseError(response)
 
 	expectedMsg := "received unexpected HTTP status: 501 Not Implemented"
 	if !strings.Contains(err.Error(), expectedMsg) {
@@ -109,7 +120,7 @@ func TestHandleErrorResponseUnexpectedStatusCode501(t *testing.T) {
 	}
 }
 
-func TestHandleErrorResponseInsufficientPrivileges403(t *testing.T) {
+func TestHandleHTTPResponseErrorInsufficientPrivileges403(t *testing.T) {
 	json := `{"details":"requesting higher privileges than access token allows"}`
 	response := &http.Response{
 		Status:     "403 Forbidden",
@@ -117,7 +128,7 @@ func TestHandleErrorResponseInsufficientPrivileges403(t *testing.T) {
 		Body:       nopCloser{bytes.NewBufferString(json)},
 		Header:     http.Header{"Content-Type": []string{"application/json"}},
 	}
-	err := HandleErrorResponse(response)
+	err := HandleHTTPResponseError(response)
 
 	expectedMsg := "denied: requesting higher privileges than access token allows"
 	if !strings.Contains(err.Error(), expectedMsg) {
@@ -125,14 +136,14 @@ func TestHandleErrorResponseInsufficientPrivileges403(t *testing.T) {
 	}
 }
 
-func TestHandleErrorResponseNonJson(t *testing.T) {
+func TestHandleHTTPResponseErrorNonJson(t *testing.T) {
 	msg := `{"details":"requesting higher privileges than access token allows"}`
 	response := &http.Response{
 		Status:     "403 Forbidden",
 		StatusCode: 403,
 		Body:       nopCloser{bytes.NewBufferString(msg)},
 	}
-	err := HandleErrorResponse(response)
+	err := HandleHTTPResponseError(response)
 
 	if !strings.Contains(err.Error(), msg) {
 		t.Errorf("Expected %q, got: %q", msg, err.Error())

registry/client/repository.go

@@ -89,8 +89,6 @@ type registry struct {
 // of the slice, starting at the value provided in 'last'.  The number of entries will be returned along with io.EOF if there
 // are no more entries
 func (r *registry) Repositories(ctx context.Context, entries []string, last string) (int, error) {
-	var numFilled int
-	var returnErr error
 
 	values := buildCatalogValues(len(entries), last)
 	u, err := r.ub.BuildCatalogURL(values)
@@ -108,7 +106,10 @@ func (r *registry) Repositories(ctx context.Context, entries []string, last stri
 	}
 	defer resp.Body.Close()
 
-	if SuccessStatus(resp.StatusCode) {
+	if err := HandleHTTPResponseError(resp); err != nil {
+		return 0, err
+	}
+
 	var ctlg struct {
 		Repositories []string `json:"repositories"`
 	}
@@ -119,17 +120,13 @@ func (r *registry) Repositories(ctx context.Context, entries []string, last stri
 	}
 
 	copy(entries, ctlg.Repositories)
-		numFilled = len(ctlg.Repositories)
+	numFilled := len(ctlg.Repositories)
 
-		link := resp.Header.Get("Link")
+	if resp.Header.Get("Link") == "" {
-		if link == "" {
+		return numFilled, io.EOF
-			returnErr = io.EOF
-		}
-	} else {
-		return 0, HandleErrorResponse(resp)
 	}
 
-	return numFilled, returnErr
+	return numFilled, nil
 }
 
 // NewRepository creates a new Repository for the given repository name and base URL.
@@ -200,18 +197,17 @@ type tags struct {
 
 // All returns all tags
 func (t *tags) All(ctx context.Context) ([]string, error) {
-	var tags []string
-
 	listURLStr, err := t.ub.BuildTagsURL(t.name)
 	if err != nil {
-		return tags, err
+		return nil, err
 	}
 
 	listURL, err := url.Parse(listURLStr)
 	if err != nil {
-		return tags, err
+		return nil, err
 	}
 
+	var allTags []string
 	for {
 		req, err := http.NewRequestWithContext(ctx, http.MethodGet, listURL.String(), nil)
 		if err != nil {
@@ -219,36 +215,36 @@ func (t *tags) All(ctx context.Context) ([]string, error) {
 		}
 		resp, err := t.client.Do(req)
 		if err != nil {
-			return tags, err
+			return allTags, err
 		}
 		defer resp.Body.Close()
 
-		if SuccessStatus(resp.StatusCode) {
+		if err := HandleHTTPResponseError(resp); err != nil {
+			return allTags, err
+		}
+
 		b, err := io.ReadAll(resp.Body)
 		if err != nil {
-				return tags, err
+			return allTags, err
 		}
 
 		tagsResponse := struct {
 			Tags []string `json:"tags"`
 		}{}
 		if err := json.Unmarshal(b, &tagsResponse); err != nil {
-				return tags, err
+			return allTags, err
 		}
-			tags = append(tags, tagsResponse.Tags...)
+		allTags = append(allTags, tagsResponse.Tags...)
 		if link := resp.Header.Get("Link"); link != "" {
 			firsLink, _, _ := strings.Cut(link, ";")
 			linkURL, err := url.Parse(strings.Trim(firsLink, "<>"))
 			if err != nil {
-					return tags, err
+				return allTags, err
 			}
 
 			listURL = listURL.ResolveReference(linkURL)
 		} else {
-				return tags, nil
+			return allTags, nil
-			}
-		} else {
-			return tags, HandleErrorResponse(resp)
 		}
 	}
 }
@@ -345,7 +341,7 @@ func (t *tags) Get(ctx context.Context, tag string) (distribution.Descriptor, er
 		if resp.StatusCode >= 200 && resp.StatusCode < 400 {
 			return descriptorFromResponse(resp)
 		}
-		return distribution.Descriptor{}, HandleErrorResponse(resp)
+		return distribution.Descriptor{}, HandleHTTPResponseError(resp)
 	}
 }
 
@@ -378,10 +374,7 @@ func (t *tags) Untag(ctx context.Context, tag string) error {
 	}
 	defer resp.Body.Close()
 
-	if SuccessStatus(resp.StatusCode) {
+	return HandleHTTPResponseError(resp)
-		return nil
-	}
-	return HandleErrorResponse(resp)
 }
 
 type manifests struct {
@@ -411,12 +404,14 @@ func (ms *manifests) Exists(ctx context.Context, dgst digest.Digest) (bool, erro
 	}
 	defer resp.Body.Close()
 
-	if SuccessStatus(resp.StatusCode) {
+	if resp.StatusCode == http.StatusNotFound {
-		return true, nil
-	} else if resp.StatusCode == http.StatusNotFound {
 		return false, nil
 	}
-	return false, HandleErrorResponse(resp)
+
+	if err := HandleHTTPResponseError(resp); err != nil {
+		return false, err
+	}
+	return true, nil
 }
 
 // AddEtagToTag allows a client to supply an eTag to Get which will be
@@ -517,7 +512,11 @@ func (ms *manifests) Get(ctx context.Context, dgst digest.Digest, options ...dis
 	defer resp.Body.Close()
 	if resp.StatusCode == http.StatusNotModified {
 		return nil, distribution.ErrManifestNotModified
-	} else if SuccessStatus(resp.StatusCode) {
+	}
+	if err := HandleHTTPResponseError(resp); err != nil {
+		return nil, err
+	}
+
 	if contentDgst != nil {
 		dgst, err := digest.Parse(resp.Header.Get("Docker-Content-Digest"))
 		if err == nil {
@@ -535,8 +534,6 @@ func (ms *manifests) Get(ctx context.Context, dgst digest.Digest, options ...dis
 	}
 	return m, nil
 }
-	return nil, HandleErrorResponse(resp)
-}
 
 // Put puts a manifest.  A tag can be specified using an options parameter which uses some shared state to hold the
 // tag name in order to build the correct upload URL.
@@ -594,9 +591,11 @@ func (ms *manifests) Put(ctx context.Context, m distribution.Manifest, options .
 	}
 	defer resp.Body.Close()
 
-	if SuccessStatus(resp.StatusCode) {
+	if err := HandleHTTPResponseError(resp); err != nil {
-		dgstHeader := resp.Header.Get("Docker-Content-Digest")
+		return "", err
-		dgst, err := digest.Parse(dgstHeader)
+	}
+
+	dgst, err := digest.Parse(resp.Header.Get("Docker-Content-Digest"))
 	if err != nil {
 		return "", err
 	}
@@ -604,9 +603,6 @@ func (ms *manifests) Put(ctx context.Context, m distribution.Manifest, options .
 	return dgst, nil
 }
 
-	return "", HandleErrorResponse(resp)
-}
-
 func (ms *manifests) Delete(ctx context.Context, dgst digest.Digest) error {
 	ref, err := reference.WithDigest(ms.name, dgst)
 	if err != nil {
@@ -627,10 +623,7 @@ func (ms *manifests) Delete(ctx context.Context, dgst digest.Digest) error {
 	}
 	defer resp.Body.Close()
 
-	if SuccessStatus(resp.StatusCode) {
+	return HandleHTTPResponseError(resp)
-		return nil
-	}
-	return HandleErrorResponse(resp)
 }
 
 // todo(richardscothern): Restore interface and implementation with merge of #1050
@@ -689,7 +682,7 @@ func (bs *blobs) Open(ctx context.Context, dgst digest.Digest) (io.ReadSeekClose
 		if resp.StatusCode == http.StatusNotFound {
 			return distribution.ErrBlobUnknown
 		}
-		return HandleErrorResponse(resp)
+		return HandleHTTPResponseError(resp)
 	}), nil
 }
 
@@ -732,13 +725,11 @@ func (bs *blobs) Put(ctx context.Context, mediaType string, p []byte) (distribut
 		return distribution.Descriptor{}, fmt.Errorf("short copy: wrote %d of %d", n, len(p))
 	}
 
-	desc := distribution.Descriptor{
+	return writer.Commit(ctx, distribution.Descriptor{
 		MediaType: mediaType,
 		Size:      int64(len(p)),
 		Digest:    dgstr.Digest(),
-	}
+	})
-
-	return writer.Commit(ctx, desc)
 }
 
 type optionFunc func(interface{}) error
@@ -827,7 +818,7 @@ func (bs *blobs) Create(ctx context.Context, options ...distribution.BlobCreateO
 			location:  location,
 		}, nil
 	default:
-		return nil, HandleErrorResponse(resp)
+		return nil, HandleHTTPResponseError(resp)
 	}
 }
 
@@ -877,7 +868,14 @@ func (bs *blobStatter) Stat(ctx context.Context, dgst digest.Digest) (distributi
 	}
 	defer resp.Body.Close()
 
-	if SuccessStatus(resp.StatusCode) {
+	if resp.StatusCode == http.StatusNotFound {
+		return distribution.Descriptor{}, distribution.ErrBlobUnknown
+	}
+
+	if err := HandleHTTPResponseError(resp); err != nil {
+		return distribution.Descriptor{}, err
+	}
+
 	lengthHeader := resp.Header.Get("Content-Length")
 	if lengthHeader == "" {
 		return distribution.Descriptor{}, fmt.Errorf("missing content-length header for request: %s", u)
@@ -893,10 +891,6 @@ func (bs *blobStatter) Stat(ctx context.Context, dgst digest.Digest) (distributi
 		Size:      length,
 		Digest:    dgst,
 	}, nil
-	} else if resp.StatusCode == http.StatusNotFound {
-		return distribution.Descriptor{}, distribution.ErrBlobUnknown
-	}
-	return distribution.Descriptor{}, HandleErrorResponse(resp)
 }
 
 func buildCatalogValues(maxEntries int, last string) url.Values {
@@ -934,10 +928,7 @@ func (bs *blobStatter) Clear(ctx context.Context, dgst digest.Digest) error {
 	}
 	defer resp.Body.Close()
 
-	if SuccessStatus(resp.StatusCode) {
+	return HandleHTTPResponseError(resp)
-		return nil
-	}
-	return HandleErrorResponse(resp)
 }
 
 func (bs *blobStatter) SetDescriptor(ctx context.Context, dgst digest.Digest, desc distribution.Descriptor) error {