ociregistry: breaking change: improve errors with respect to HTTP status

Currently the standard wire representation of an error is duplicated
across both client and server, and there is no way for:
- a client to access the actual HTTP status of a response
- an `Interface` implementation to cause the `ociserver` to return
  a specific HTTP error status for an error code that it doesn't know
  about.

This change addresses that by moving the wire representation into the
top level `ociregistry` package, splitting `HTTPError` into its own
type, and making both `ociclient` and `ociserver` aware of them.

One deliberate decision made here guards against some scenarios when
nesting an `ociclient` implementation inside `ociserver`. There is a
risk that, due to the fact we're using the same `HTTPError` in
`httpclient` and `httpserve, if `ociclient` talks to a misbehaving
server that returns inappropriate HTTP response codes, those codes could
propagate back through `ociserver`, causing that to return inappropriate
codes too.

So in `ociserver` we only use the `HTTPError` code if there is no known
HTTP status for the error code. This seems like a reasonable
half-way-house compromise.

We also change the `ociregistry.Error` interface so that `Detail` returns
`json.RawMessage` instead of `any`. This means that the error detail
is consistent no matter if the error has come from `ociclient` or from
a Go-implemented `ociregistry.Interface` implementation, and
means we can use `ociregistry.WireError` as the base error implementation
used by all the exported specific error values. This in turn means
that the error message changes because `WireError` includes the error
code (lower-cased) in the message, but that consistency seems like a
good thing.

Fixes #26.

Signed-off-by: Roger Peppe <[email protected]>
Change-Id: I5a79c1c6fec9f22c1f565830d73486b406dd181d
Reviewed-on: https://review.gerrithub.io/c/cue-labs/oci/+/1186153
Reviewed-by: Tianon Gravi <[email protected]>
TryBot-Result: CUE porcuepine <[email protected]>
Reviewed-by: Paul Jolly <[email protected]>

Author: rogpeppe

ociregistry/error.go

@@ -14,12 +14,100 @@
 
 package ociregistry
 
+import (
+	"bytes"
+	"encoding/json"
+	"errors"
+	"net/http"
+	"strconv"
+	"strings"
+	"unicode"
+)
+
+// WireErrors is the JSON format used for error responses in
+// the OCI HTTP API. It should always contain at least one
+// error.
+type WireErrors struct {
+	Errors []WireError `json:"errors"`
+}
+
+// Unwrap allows [errors.Is] and [errors.As] to
+// see the errors inside e.
+func (e *WireErrors) Unwrap() []error {
+	// TODO we could do this only once.
+	errs := make([]error, len(e.Errors))
+	for i := range e.Errors {
+		errs[i] = &e.Errors[i]
+	}
+	return errs
+}
+
+func (e *WireErrors) Error() string {
+	var buf strings.Builder
+	buf.WriteString(e.Errors[0].Error())
+	for i := range e.Errors[1:] {
+		buf.WriteString("; ")
+		buf.WriteString(e.Errors[i+1].Error())
+	}
+	return buf.String()
+}
+
+// WireError holds a single error in an OCI HTTP response.
+type WireError struct {
+	Code_   string `json:"code"`
+	Message string `json:"message,omitempty"`
+	// Detail_ holds the JSON detail for the message.
+	// It's assumed to be valid JSON if non-empty.
+	Detail_ json.RawMessage `json:"detail,omitempty"`
+}
+
+// Is makes it possible for users to write `if errors.Is(err, ociregistry.ErrBlobUnknown)`
+// even when the error hasn't exactly wrapped that error.
+func (e *WireError) Is(err error) bool {
+	var rerr Error
+	return errors.As(err, &rerr) && rerr.Code() == e.Code()
+}
+
+// Error implements the [error] interface.
+func (e *WireError) Error() string {
+	var buf strings.Builder
+	for _, r := range e.Code_ {
+		if r == '_' {
+			buf.WriteByte(' ')
+		} else {
+			buf.WriteRune(unicode.ToLower(r))
+		}
+	}
+	if buf.Len() == 0 {
+		buf.WriteString("(no code)")
+	}
+	if e.Message != "" {
+		buf.WriteString(": ")
+		buf.WriteString(e.Message)
+	}
+	if len(e.Detail_) != 0 && !bytes.Equal(e.Detail_, []byte("null")) {
+		buf.WriteString("; detail: ")
+		buf.Write(e.Detail_)
+	}
+	return buf.String()
+}
+
+// Code implements [Error.Code].
+func (e *WireError) Code() string {
+	return e.Code_
+}
+
+// Detail implements [Error.Detail].
+func (e *WireError) Detail() json.RawMessage {
+	return e.Detail_
+}
+
 // NewError returns a new error with the given code, message and detail.
-func NewError(msg string, code string, detail any) Error {
-	return &registryError{
-		code:    code,
-		message: msg,
-		detail:  detail,
+func NewError(msg string, code string, detail json.RawMessage) Error {
+	return &WireError{
+		Code_:   code,
+		Message: msg,
+		Detail_: detail,
 	}
 }
 
@@ -31,12 +119,110 @@ type Error interface {
 	// error.Error provides the error message.
 	error
 
-	// Code returns the error code. See
+	// Code returns the error code.
 	Code() string
 
-	// Detail returns any detail to be associated with the error; it should
-	// be JSON-marshable.
-	Detail() any
+	// Detail returns any detail  associated with the error,
+	// or nil if there is none.
+	// The caller should not mutate the returned slice.
+	Detail() json.RawMessage
+}
+
+// HTTPError is optionally implemented by an error when
+// the error has originated from an HTTP request
+// or might be returned from one.
+type HTTPError interface {
+	error
+
+	// StatusCode returns the HTTP status code of the response.
+	StatusCode() int
+
+	// Response holds the HTTP response that caused the HTTPError to
+	// be created. It will return nil if the error was not created
+	// as a result of an HTTP response.
+	//
+	// The caller should not read the response body or otherwise
+	// change the response (mutation of errors is a Bad Thing).
+	//
+	// Use the ResponseBody method to obtain the body of the
+	// response if needed.
+	Response() *http.Response
+
+	// ResponseBody returns the contents of the response body. It
+	// will return nil if the error was not created as a result of
+	// an HTTP response.
+	//
+	// The caller should not change or append to the returned data.
+	ResponseBody() []byte
+}
+
+// NewHTTPError returns an error that wraps err to make an [HTTPError]
+// that represents the given status code, response and response body.
+// Both response and body may be nil.
+//
+// A shallow copy is made of the response.
+func NewHTTPError(err error, statusCode int, response *http.Response, body []byte) HTTPError {
+	herr := &httpError{
+		underlying: err,
+		statusCode: statusCode,
+	}
+	if response != nil {
+		herr.response = ref(*response)
+		herr.response.Body = nil
+		herr.body = body
+	}
+	return herr
+}
+
+type httpError struct {
+	underlying error
+	statusCode int
+	response   *http.Response
+	body       []byte
+}
+
+// Unwrap implements the [errors] Unwrap interface.
+func (e *httpError) Unwrap() error {
+	return e.underlying
+}
+
+// Is makes it possible for users to write `if errors.Is(err, ociregistry.ErrRangeInvalid)`
+// even when the error hasn't exactly wrapped that error.
+func (e *httpError) Is(err error) bool {
+	switch e.statusCode {
+	case http.StatusRequestedRangeNotSatisfiable:
+		return err == ErrRangeInvalid
+	}
+	return false
+}
+
+// Error implements [error.Error].
+func (e *httpError) Error() string {
+	var buf strings.Builder
+	buf.WriteString(strconv.Itoa(e.statusCode))
+	buf.WriteString(" ")
+	buf.WriteString(http.StatusText(e.statusCode))
+	if e.underlying != nil {
+		buf.WriteString(": ")
+		buf.WriteString(e.underlying.Error())
+	}
+	// TODO if underlying is nil, include some portion of e.body in the message?
+	return buf.String()
+}
+
+// StatusCode implements [HTTPError.StatusCode].
+func (e *httpError) StatusCode() int {
+	return e.statusCode
+}
+
+// Response implements [HTTPError.Response].
+func (e *httpError) Response() *http.Response {
+	return e.response
+}
+
+// ResponseBody implements [HTTPError.ResponseBody].
+func (e *httpError) ResponseBody() []byte {
+	return e.body
 }
 
 // The following values represent the known error codes.
@@ -66,20 +252,6 @@ var (
 	ErrRangeInvalid = NewError("invalid content range", "RANGE_INVALID", nil)
 )
 
-type registryError struct {
-	code    string
-	message string
-	detail  any
-}
-
-func (e *registryError) Code() string {
-	return e.code
-}
-
-func (e *registryError) Error() string {
-	return e.message
-}
-
-func (e *registryError) Detail() any {
-	return e.detail
+func ref[T any](x T) *T {
+	return &x
 }

ociregistry/internal/ocirequest/request_test.go

@@ -62,7 +62,7 @@ var parseRequestTests = []struct {
 	testName:  "getBlobInvalidRepo",
 	method:    "GET",
 	url:       "/v2/foo/bAr/blobs/sha256:2cf24dba5fb0a30e26e83b2ac5b9e29e1b161e5c1fa7425e73043362938b9824",
-	wantError: `invalid repository name`,
+	wantError: `name invalid: invalid repository name`,
 }, {
 	testName: "startUpload",
 	method:   "POST",

ociregistry/ociauth/auth.go

@@ -12,6 +12,8 @@ import (
 	"strings"
 	"sync"
 	"time"
+
+	"cuelabs.dev/go/oci/ociregistry"
 )
 
 // TODO decide on a good value for this.
@@ -298,8 +300,8 @@ func (r *registry) acquireAccessToken(ctx context.Context, requiredScope, wantSc
 	scope := requiredScope.Union(wantScope)
 	tok, err := r.acquireToken(ctx, scope)
 	if err != nil {
-		var rerr *responseError
-		if !errors.As(err, &rerr) || rerr.statusCode != http.StatusUnauthorized {
+		var herr ociregistry.HTTPError
+		if !errors.As(err, &herr) || herr.StatusCode() != http.StatusUnauthorized {
 			return "", err
 		}
 		// The documentation says this:
@@ -372,8 +374,8 @@ func (r *registry) acquireToken(ctx context.Context, scope Scope) (*wireToken, e
 		if err == nil {
 			return tok, nil
 		}
-		var rerr *responseError
-		if !errors.As(err, &rerr) || rerr.statusCode != http.StatusNotFound {
+		var herr ociregistry.HTTPError
+		if !errors.As(err, &herr) || herr.StatusCode() != http.StatusNotFound {
 			return tok, err
 		}
 		// The request to the endpoint returned 404 from the POST request,
@@ -448,12 +450,12 @@ func (r *registry) doTokenRequest(req *http.Request) (*wireToken, error) {
 		return nil, err
 	}
 	defer resp.Body.Close()
+	data, bodyErr := io.ReadAll(resp.Body)
 	if resp.StatusCode != http.StatusOK {
-		return nil, errorFromResponse(resp)
+		return nil, ociregistry.NewHTTPError(nil, resp.StatusCode, resp, data)
 	}
-	data, err := io.ReadAll(resp.Body)
-	if err != nil {
-		return nil, fmt.Errorf("cannot read response body: %v", err)
+	if bodyErr != nil {
+		return nil, fmt.Errorf("error reading response body: %v", err)
 	}
 	var tok wireToken
 	if err := json.Unmarshal(data, &tok); err != nil {
@@ -462,22 +464,6 @@ func (r *registry) doTokenRequest(req *http.Request) (*wireToken, error) {
 	return &tok, nil
 }
 
-type responseError struct {
-	statusCode int
-	msg        string
-}
-
-func errorFromResponse(resp *http.Response) error {
-	// TODO include body of response in error message.
-	return &responseError{
-		statusCode: resp.StatusCode,
-	}
-}
-
-func (e *responseError) Error() string {
-	return fmt.Sprintf("unexpected HTTP response %d", e.statusCode)
-}
-
 // deleteExpiredTokens removes all tokens from r that expire after the given
 // time.
 // TODO ask the store to remove expired tokens?

ociregistry/ociclient/badname_test.go

@@ -17,7 +17,7 @@ func TestBadRepoName(t *testing.T) {
 	})
 	qt.Assert(t, qt.IsNil(err))
 	_, err = r.GetBlob(ctx, "Invalid--Repo", "sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855")
-	qt.Check(t, qt.ErrorMatches(err, "invalid OCI request: invalid repository name"))
+	qt.Check(t, qt.ErrorMatches(err, "invalid OCI request: name invalid: invalid repository name"))
 	_, err = r.GetBlob(ctx, "okrepo", "bad-digest")
 	qt.Check(t, qt.ErrorMatches(err, "invalid OCI request: badly formed digest"))
 	_, err = r.ResolveTag(ctx, "okrepo", "bad-Tag!")