ociregistry: implement MarshalError and use it in ociserver

This provides a standard way for code to convert
from a regular Go error to the form appropriate for OCI
error response bodies.

It seems to make sense to put this inside the top level
ociregistry package because the JSON-oriented
`WireError` types are already implemented there, and
the logic for stripping error code prefixes is directly related
to the logic in the same package that adds them.

Also in passing fix an unintended use of the deprecated
`io/ioutil` package to use `io` instead.

Also add a test for error stuttering to `ociclient` and fix
the code to avoid stuttering the HTTP status code
as exposed by that test.

Fixes #31.

Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
Change-Id: I22408dd3320b73bc52f37181987f79003dbaa115
Dispatch-Trailer: {"type":"trybot","CL":1191146,"patchset":5,"ref":"refs/changes/46/1191146/5","targetBranch":"main"}

ociregistry/error.go

@@ -15,15 +15,33 @@
 package ociregistry
 
 import (
-	"bytes"
 	"encoding/json"
 	"errors"
+	"fmt"
 	"net/http"
 	"strconv"
 	"strings"
 	"unicode"
 )
 
+var errorStatuses = map[string]int{
+	ErrBlobUnknown.Code():         http.StatusNotFound,
+	ErrBlobUploadInvalid.Code():   http.StatusRequestedRangeNotSatisfiable,
+	ErrBlobUploadUnknown.Code():   http.StatusNotFound,
+	ErrDigestInvalid.Code():       http.StatusBadRequest,
+	ErrManifestBlobUnknown.Code(): http.StatusNotFound,
+	ErrManifestInvalid.Code():     http.StatusBadRequest,
+	ErrManifestUnknown.Code():     http.StatusNotFound,
+	ErrNameInvalid.Code():         http.StatusBadRequest,
+	ErrNameUnknown.Code():         http.StatusNotFound,
+	ErrSizeInvalid.Code():         http.StatusBadRequest,
+	ErrUnauthorized.Code():        http.StatusUnauthorized,
+	ErrDenied.Code():              http.StatusForbidden,
+	ErrUnsupported.Code():         http.StatusBadRequest,
+	ErrTooManyRequests.Code():     http.StatusTooManyRequests,
+	ErrRangeInvalid.Code():        http.StatusRequestedRangeNotSatisfiable,
+}
+
 // WireErrors is the JSON format used for error responses in
 // the OCI HTTP API. It should always contain at least one
 // error.
@@ -70,26 +88,18 @@ func (e *WireError) Is(err error) bool {
 
 // 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)")
-	}
+	buf := make([]byte, 0, 128)
+	buf = appendErrorCodePrefix(buf, e.Code_)
+
 	if e.Message != "" {
-		buf.WriteString(": ")
-		buf.WriteString(e.Message)
+		buf = append(buf, ": "...)
+		buf = append(buf, e.Message...)
 	}
-	if len(e.Detail_) != 0 && !bytes.Equal(e.Detail_, []byte("null")) {
-		buf.WriteString("; detail: ")
-		buf.Write(e.Detail_)
-	}
-	return buf.String()
+	// TODO: it would be nice to have some way to surface the detail
+	// in a message, but it's awkward to do so here because we don't
+	// really want the detail to be duplicated in the "message"
+	// and "detail" fields.
+	return string(buf)
 }
 
 // Code implements [Error.Code].
@@ -198,16 +208,14 @@ func (e *httpError) Is(err error) bool {
 
 // 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))
+	buf := make([]byte, 0, 128)
+	buf = appendHTTPStatusPrefix(buf, e.statusCode)
 	if e.underlying != nil {
-		buf.WriteString(": ")
-		buf.WriteString(e.underlying.Error())
+		buf = append(buf, ": "...)
+		buf = append(buf, e.underlying.Error()...)
 	}
 	// TODO if underlying is nil, include some portion of e.body in the message?
-	return buf.String()
+	return string(buf)
 }
 
 // StatusCode implements [HTTPError.StatusCode].
@@ -225,6 +233,79 @@ func (e *httpError) ResponseBody() []byte {
 	return e.body
 }
 
+// MarshalError marshals the given error as JSON according
+// to the OCI distribution specification. It also returns
+// the associated HTTP status code, or [http.StatusInternalServerError]
+// if no specific code can be found.
+//
+// If err is or wraps [Error], that code will be used for the "code"
+// field in the marshaled error.
+//
+// If err wraps [HTTPError] and no HTTP status code is known
+// for the error code, [HTTPError.StatusCode] will be used.
+func MarshalError(err error) (errorBody []byte, httpStatus int) {
+	var e WireError
+	// TODO perhaps we should iterate through all the
+	// errors instead of just choosing one.
+	// See https://github.com/golang/go/issues/66455
+	var ociErr Error
+	if errors.As(err, &ociErr) {
+		e.Code_ = ociErr.Code()
+		e.Detail_ = ociErr.Detail()
+	}
+	if e.Code_ == "" {
+		// This is contrary to spec, but it's what the Docker registry
+		// does, so it can't be too bad.
+		e.Code_ = "UNKNOWN"
+	}
+	// Use the HTTP status code from the error only when there isn't
+	// one implied from the error code. This means that the HTTP status
+	// is always consistent with the error code, but still allows a registry
+	// to choose custom HTTP status codes for other codes.
+	httpStatus = http.StatusInternalServerError
+	if status, ok := errorStatuses[e.Code_]; ok {
+		httpStatus = status
+	} else {
+		var httpErr HTTPError
+		if errors.As(err, &httpErr) {
+			httpStatus = httpErr.StatusCode()
+		}
+	}
+	// Prevent the message from containing a redundant
+	// error code prefix by stripping it before sending over the
+	// wire. This won't always work, but is enough to prevent
+	// adjacent stuttering of code prefixes when a client
+	// creates a WireError from an error response.
+	e.Message = trimErrorCodePrefix(err, httpStatus, e.Code_)
+	data, err := json.Marshal(WireErrors{
+		Errors: []WireError{e},
+	})
+	if err != nil {
+		panic(fmt.Errorf("cannot marshal error: %v", err))
+	}
+	return data, httpStatus
+}
+
+// trimErrorCodePrefix returns err's string
+// with any prefix codes added by [HTTPError]
+// or [WireError] removed.
+func trimErrorCodePrefix(err error, httpStatus int, errorCode string) string {
+	msg := err.Error()
+	buf := make([]byte, 0, 128)
+	if httpStatus != 0 {
+		buf = appendHTTPStatusPrefix(buf, httpStatus)
+		buf = append(buf, ": "...)
+		msg = strings.TrimPrefix(msg, string(buf))
+	}
+	if errorCode != "" {
+		buf = buf[:0]
+		buf = appendErrorCodePrefix(buf, errorCode)
+		buf = append(buf, ": "...)
+		msg = strings.TrimPrefix(msg, string(buf))
+	}
+	return msg
+}
+
 // The following values represent the known error codes.
 var (
 	ErrBlobUnknown         = NewError("blob unknown to registry", "BLOB_UNKNOWN", nil)
@@ -252,6 +333,27 @@ var (
 	ErrRangeInvalid = NewError("invalid content range", "RANGE_INVALID", nil)
 )
 
+func appendHTTPStatusPrefix(buf []byte, statusCode int) []byte {
+	buf = strconv.AppendInt(buf, int64(statusCode), 10)
+	buf = append(buf, ' ')
+	buf = append(buf, http.StatusText(statusCode)...)
+	return buf
+}
+
+func appendErrorCodePrefix(buf []byte, code string) []byte {
+	if code == "" {
+		return append(buf, "(no code)"...)
+	}
+	for _, r := range code {
+		if r == '_' {
+			buf = append(buf, ' ')
+		} else {
+			buf = append(buf, string(unicode.ToLower(r))...)
+		}
+	}
+	return buf
+}
+
 func ref[T any](x T) *T {
 	return &x
 }

ociregistry/error_test.go

@@ -0,0 +1,94 @@
+package ociregistry
+
+import (
+	"encoding/json"
+	"errors"
+	"fmt"
+	"net/http"
+	"testing"
+
+	"github.com/go-quicktest/qt"
+)
+
+var errorTests = []struct {
+	testName              string
+	err                   error
+	wantMsg               string
+	wantMarshalData       rawJSONMessage
+	wantMarshalHTTPStatus int
+}{{
+	testName:              "RegularGoError",
+	err:                   fmt.Errorf("unknown error"),
+	wantMsg:               "unknown error",
+	wantMarshalData:       `{"errors":[{"code":"UNKNOWN","message":"unknown error"}]}`,
+	wantMarshalHTTPStatus: http.StatusInternalServerError,
+}, {
+	testName:              "RegistryError",
+	err:                   ErrBlobUnknown,
+	wantMsg:               "blob unknown: blob unknown to registry",
+	wantMarshalData:       `{"errors":[{"code":"BLOB_UNKNOWN","message":"blob unknown to registry"}]}`,
+	wantMarshalHTTPStatus: http.StatusNotFound,
+}, {
+	testName:              "WrappedRegistryErrorWithContextAtStart",
+	err:                   fmt.Errorf("some context: %w", ErrBlobUnknown),
+	wantMsg:               "some context: blob unknown: blob unknown to registry",
+	wantMarshalData:       `{"errors":[{"code":"BLOB_UNKNOWN","message":"some context: blob unknown: blob unknown to registry"}]}`,
+	wantMarshalHTTPStatus: http.StatusNotFound,
+}, {
+	testName:              "WrappedRegistryErrorWithContextAtEnd",
+	err:                   fmt.Errorf("%w: some context", ErrBlobUnknown),
+	wantMsg:               "blob unknown: blob unknown to registry: some context",
+	wantMarshalData:       `{"errors":[{"code":"BLOB_UNKNOWN","message":"blob unknown to registry: some context"}]}`,
+	wantMarshalHTTPStatus: http.StatusNotFound,
+}, {
+	testName: "HTTPStatusIgnoredWithKnownCode",
+	err:      NewHTTPError(fmt.Errorf("%w: some context", ErrBlobUnknown), http.StatusUnauthorized, nil, nil),
+	wantMsg:  "401 Unauthorized: blob unknown: blob unknown to registry: some context",
+	// Note: the "401 Unauthorized" remains intact because it's not redundant with respect
+	// to the 404 HTTP response code.
+	wantMarshalData:       `{"errors":[{"code":"BLOB_UNKNOWN","message":"401 Unauthorized: blob unknown: blob unknown to registry: some context"}]}`,
+	wantMarshalHTTPStatus: http.StatusNotFound,
+}, {
+	testName:              "HTTPStatusUsedWithUnknownCode",
+	err:                   NewHTTPError(NewError("a message with a code", "SOME_CODE", nil), http.StatusUnauthorized, nil, nil),
+	wantMsg:               "401 Unauthorized: some code: a message with a code",
+	wantMarshalData:       `{"errors":[{"code":"SOME_CODE","message":"a message with a code"}]}`,
+	wantMarshalHTTPStatus: http.StatusUnauthorized,
+}, {
+	testName:              "ErrorWithDetail",
+	err:                   NewError("a message with some detail", "SOME_CODE", json.RawMessage(`{"foo": true}`)),
+	wantMsg:               `some code: a message with some detail`,
+	wantMarshalData:       `{"errors":[{"code":"SOME_CODE","message":"a message with some detail","detail":{"foo":true}}]}`,
+	wantMarshalHTTPStatus: http.StatusInternalServerError,
+}}
+
+func TestError(t *testing.T) {
+	for _, test := range errorTests {
+		t.Run(test.testName, func(t *testing.T) {
+			qt.Check(t, qt.ErrorMatches(test.err, test.wantMsg))
+			data, httpStatus := MarshalError(test.err)
+			qt.Check(t, qt.Equals(httpStatus, test.wantMarshalHTTPStatus))
+			qt.Check(t, qt.JSONEquals(data, test.wantMarshalData), qt.Commentf("marshal data: %s", data))
+
+			// Check that the marshaled error unmarshals into WireError OK and
+			// that the code matches appropriately.
+			var errs *WireErrors
+			err := json.Unmarshal(data, &errs)
+			qt.Assert(t, qt.IsNil(err))
+			if ociErr := Error(nil); errors.As(test.err, &ociErr) {
+				qt.Assert(t, qt.IsTrue(errors.Is(errs, NewError("something", ociErr.Code(), nil))))
+			}
+		})
+	}
+}
+
+type rawJSONMessage string
+
+func (m rawJSONMessage) MarshalJSON() ([]byte, error) {
+	return []byte(m), nil
+}
+
+func (m *rawJSONMessage) UnmarshalJSON(data []byte) error {
+	*m = rawJSONMessage(data)
+	return nil
+}

ociregistry/ociclient/error.go

@@ -55,6 +55,9 @@ func makeError1(resp *http.Response, bodyData []byte) error {
 		// When we've made a HEAD request, we can't see any of
 		// the actual error, so we'll have to make up something
 		// from the HTTP status.
+		// TODO would we do better if we interpreted the HTTP status
+		// relative to the actual method that was called in order
+		// to come up with a more plausible error?
 		var err error
 		switch resp.StatusCode {
 		case http.StatusNotFound:
@@ -71,7 +74,7 @@ func makeError1(resp *http.Response, bodyData []byte) error {
 			// Our caller will turn this into a non-nil error.
 			return nil
 		}
-		return fmt.Errorf("error response: %v: %w", resp.Status, err)
+		return err
 	}
 	if ctype := resp.Header.Get("Content-Type"); !isJSONMediaType(ctype) {
 		return fmt.Errorf("non-JSON error response %q; body %q", ctype, bodyData)

ociregistry/ociclient/error_test.go

@@ -10,10 +10,39 @@ import (
 	"testing"
 
 	"cuelabs.dev/go/oci/ociregistry"
+	"cuelabs.dev/go/oci/ociregistry/ociserver"
 	"github.com/go-quicktest/qt"
 	"github.com/opencontainers/go-digest"
 )
 
+func TestErrorStuttering(t *testing.T) {
+	// This checks that the stuttering observed in issue #31
+	// isn't an issue when ociserver wraps ociclient.
+	srv := httptest.NewServer(ociserver.New(&ociregistry.Funcs{
+		NewError: func(ctx context.Context, methodName, repo string) error {
+			return ociregistry.ErrManifestUnknown
+		},
+	}, nil))
+	defer srv.Close()
+
+	srvURL, _ := url.Parse(srv.URL)
+	r, err := New(srvURL.Host, &Options{
+		Insecure: true,
+	})
+	qt.Assert(t, qt.IsNil(err))
+	_, err = r.GetTag(context.Background(), "foo", "sometag")
+	qt.Check(t, qt.ErrorIs(err, ociregistry.ErrManifestUnknown))
+	qt.Check(t, qt.ErrorMatches(err, `404 Not Found: manifest unknown: manifest unknown to registry`))
+
+	// ResolveTag uses HEAD rather than GET, so here we're testing
+	// the path where a response with no body gets turned back into
+	// something vaguely resembling the original error, which is why
+	// the code and message have changed.
+	_, err = r.ResolveTag(context.Background(), "foo", "sometag")
+	qt.Check(t, qt.ErrorIs(err, ociregistry.ErrNameUnknown))
+	qt.Check(t, qt.ErrorMatches(err, `404 Not Found: name unknown: repository name not known to registry`))
+}
+
 func TestNonJSONErrorResponse(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
 		w.WriteHeader(http.StatusTeapot)