From c8ba5d70818a61f90bf27ac00dce76d15a8e25a3 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 8 Sep 2023 14:33:46 +0200 Subject: [PATCH] registry/client: combine SuccessStatus and HandleErrorResponse The SuccessStatus acted on the response's status code, and was used to return early, before checking the same status code with HandleErrorResponse. This patch combines both functions into a HandleHTTPResponseError, which returns an error for "non-success" status-codes, which simplifies handling of responses, and makes some logic slightly more idiomatic. Signed-off-by: Sebastiaan van Stijn --- registry/client/auth/session.go | 6 +- registry/client/blob_writer.go | 16 +-- registry/client/errors.go | 30 ++++- registry/client/errors_test.go | 43 +++--- registry/client/repository.go | 225 +++++++++++++++----------------- 5 files changed, 170 insertions(+), 150 deletions(-) diff --git a/registry/client/auth/session.go b/registry/client/auth/session.go index fe212831..b0879fb4 100644 --- a/registry/client/auth/session.go +++ b/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) { - err := client.HandleErrorResponse(resp) + if err := client.HandleHTTPResponseError(resp); err != nil { 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) { - err := client.HandleErrorResponse(resp) + if err := client.HandleHTTPResponseError(resp); err != nil { return "", time.Time{}, err } diff --git a/registry/client/blob_writer.go b/registry/client/blob_writer.go index bff0e08b..f85c84f7 100644 --- a/registry/client/blob_writer.go +++ b/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) { - return 0, hbu.handleErrorResponse(resp) + if err := hbu.handleErrorResponse(resp); err != nil { + 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) { - return 0, hbu.handleErrorResponse(resp) + if err := hbu.handleErrorResponse(resp); err != nil { + 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) { - return distribution.Descriptor{}, hbu.handleErrorResponse(resp) + if err := hbu.handleErrorResponse(resp); err != nil { + 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) diff --git a/registry/client/errors.go b/registry/client/errors.go index 30c76926..9d20b8b7 100644 --- a/registry/client/errors.go +++ b/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 -// unsuccessful HTTP response code (in the range 400 - 499 inclusive). An -// UnexpectedHTTPStatusError returned for response code outside of expected -// range. -func HandleErrorResponse(resp *http.Response) error { +// HandleHTTPResponseError returns error parsed from HTTP response, if any. +// It returns nil if no error occurred (HTTP status 200-399), or an error +// for unsuccessful HTTP response codes (in the range 400 - 499 inclusive). +// If possible, it returns a typed error, but an UnexpectedHTTPStatusError +// 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 } diff --git a/registry/client/errors_test.go b/registry/client/errors_test.go index 135608c7..432e2bcc 100644 --- a/registry/client/errors_test.go +++ b/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()) diff --git a/registry/client/repository.go b/registry/client/repository.go index 58e763a4..d39af256 100644 --- a/registry/client/repository.go +++ b/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,28 +106,27 @@ func (r *registry) Repositories(ctx context.Context, entries []string, last stri } defer resp.Body.Close() - if SuccessStatus(resp.StatusCode) { - var ctlg struct { - Repositories []string `json:"repositories"` - } - decoder := json.NewDecoder(resp.Body) - - if err := decoder.Decode(&ctlg); err != nil { - return 0, err - } - - copy(entries, ctlg.Repositories) - numFilled = len(ctlg.Repositories) - - link := resp.Header.Get("Link") - if link == "" { - returnErr = io.EOF - } - } else { - return 0, HandleErrorResponse(resp) + if err := HandleHTTPResponseError(resp); err != nil { + return 0, err } - return numFilled, returnErr + var ctlg struct { + Repositories []string `json:"repositories"` + } + decoder := json.NewDecoder(resp.Body) + + if err := decoder.Decode(&ctlg); err != nil { + return 0, err + } + + copy(entries, ctlg.Repositories) + numFilled := len(ctlg.Repositories) + + if resp.Header.Get("Link") == "" { + return numFilled, io.EOF + } + + 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) { - b, err := io.ReadAll(resp.Body) + if err := HandleHTTPResponseError(resp); err != nil { + return allTags, err + } + + b, err := io.ReadAll(resp.Body) + if err != nil { + return allTags, err + } + + tagsResponse := struct { + Tags []string `json:"tags"` + }{} + if err := json.Unmarshal(b, &tagsResponse); err != nil { + return allTags, err + } + 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 } - tagsResponse := struct { - Tags []string `json:"tags"` - }{} - if err := json.Unmarshal(b, &tagsResponse); err != nil { - return tags, err - } - tags = append(tags, 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 - } - - listURL = listURL.ResolveReference(linkURL) - } else { - return tags, nil - } + listURL = listURL.ResolveReference(linkURL) } else { - return tags, HandleErrorResponse(resp) + return allTags, nil } } } @@ -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 nil - } - return HandleErrorResponse(resp) + return HandleHTTPResponseError(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) { - return true, nil - } else if resp.StatusCode == http.StatusNotFound { + 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,25 +512,27 @@ 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 contentDgst != nil { - dgst, err := digest.Parse(resp.Header.Get("Docker-Content-Digest")) - if err == nil { - *contentDgst = dgst - } - } - mt := resp.Header.Get("Content-Type") - body, err := io.ReadAll(resp.Body) - if err != nil { - return nil, err - } - m, _, err := distribution.UnmarshalManifest(mt, body) - if err != nil { - return nil, err - } - return m, nil } - return nil, HandleErrorResponse(resp) + 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 { + *contentDgst = dgst + } + } + mt := resp.Header.Get("Content-Type") + body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, err + } + m, _, err := distribution.UnmarshalManifest(mt, body) + if err != nil { + return nil, err + } + return m, nil } // Put puts a manifest. A tag can be specified using an options parameter which uses some shared state to hold the @@ -594,17 +591,16 @@ func (ms *manifests) Put(ctx context.Context, m distribution.Manifest, options . } defer resp.Body.Close() - if SuccessStatus(resp.StatusCode) { - dgstHeader := resp.Header.Get("Docker-Content-Digest") - dgst, err := digest.Parse(dgstHeader) - if err != nil { - return "", err - } - - return dgst, nil + if err := HandleHTTPResponseError(resp); err != nil { + return "", err } - return "", HandleErrorResponse(resp) + dgst, err := digest.Parse(resp.Header.Get("Docker-Content-Digest")) + if err != nil { + return "", err + } + + return dgst, nil } func (ms *manifests) Delete(ctx context.Context, dgst digest.Digest) error { @@ -627,10 +623,7 @@ func (ms *manifests) Delete(ctx context.Context, dgst digest.Digest) error { } defer resp.Body.Close() - if SuccessStatus(resp.StatusCode) { - return nil - } - return HandleErrorResponse(resp) + return HandleHTTPResponseError(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,26 +868,29 @@ func (bs *blobStatter) Stat(ctx context.Context, dgst digest.Digest) (distributi } defer resp.Body.Close() - if SuccessStatus(resp.StatusCode) { - lengthHeader := resp.Header.Get("Content-Length") - if lengthHeader == "" { - return distribution.Descriptor{}, fmt.Errorf("missing content-length header for request: %s", u) - } - - length, err := strconv.ParseInt(lengthHeader, 10, 64) - if err != nil { - return distribution.Descriptor{}, fmt.Errorf("error parsing content-length: %v", err) - } - - return distribution.Descriptor{ - MediaType: resp.Header.Get("Content-Type"), - Size: length, - Digest: dgst, - }, nil - } else if resp.StatusCode == http.StatusNotFound { + if resp.StatusCode == http.StatusNotFound { return distribution.Descriptor{}, distribution.ErrBlobUnknown } - return distribution.Descriptor{}, HandleErrorResponse(resp) + + 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) + } + + length, err := strconv.ParseInt(lengthHeader, 10, 64) + if err != nil { + return distribution.Descriptor{}, fmt.Errorf("error parsing content-length: %v", err) + } + + return distribution.Descriptor{ + MediaType: resp.Header.Get("Content-Type"), + Size: length, + Digest: dgst, + }, nil } 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 nil - } - return HandleErrorResponse(resp) + return HandleHTTPResponseError(resp) } func (bs *blobStatter) SetDescriptor(ctx context.Context, dgst digest.Digest, desc distribution.Descriptor) error {