Merge pull request #54125 from thaJeztah/fix_httpstatus

httpstatus: handle missing errdefs HTTP status codes

daemon/server/httpstatus/status.go

@@ -24,6 +24,8 @@ func FromError(err error) int {
 
 	// Note that the below functions are already checking the error causal chain for matches.
 	// Only check errors from the errdefs package, no new error type checking may be added
+	//
+	// TODO(thaJeztah): rewrite using github.com/containerd/errdefs/pkg/errhttp.ToHTTP after providing a utility to detect if a match was found.
 	switch {
 	case cerrdefs.IsNotFound(rerr):
 		return http.StatusNotFound
@@ -33,6 +35,10 @@ func FromError(err error) int {
 		return http.StatusConflict
 	case cerrdefs.IsUnauthorized(rerr):
 		return http.StatusUnauthorized
+	case cerrdefs.IsFailedPrecondition(rerr):
+		return http.StatusPreconditionFailed
+	case cerrdefs.IsResourceExhausted(rerr):
+		return http.StatusTooManyRequests
 	case cerrdefs.IsUnavailable(rerr):
 		return http.StatusServiceUnavailable
 	case cerrdefs.IsPermissionDenied(rerr):
@@ -111,14 +117,13 @@ func statusCodeFromGRPCError(err error) int {
 func statusCodeFromDistributionError(err error) int {
 	switch errs := err.(type) {
 	case errcode.Errors:
-		if len(errs) < 1 {
-			return http.StatusInternalServerError
-		}
-		if _, ok := errs[0].(errcode.ErrorCoder); ok {
+		if len(errs) > 0 {
 			return statusCodeFromDistributionError(errs[0])
 		}
+		return http.StatusInternalServerError
 	case errcode.ErrorCoder:
 		return errs.ErrorCode().Descriptor().HTTPStatusCode
+	default:
+		return http.StatusInternalServerError
 	}
-	return http.StatusInternalServerError
 }

daemon/server/httpstatus/status_test.go

@@ -0,0 +1,173 @@
+package httpstatus_test
+
+import (
+	"context"
+	"errors"
+	"fmt"
+	"net/http"
+	"testing"
+
+	cerrdefs "github.com/containerd/errdefs"
+	"github.com/docker/distribution/registry/api/errcode"
+	"github.com/moby/moby/v2/daemon/server/httpstatus"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
+	"gotest.tools/v3/assert"
+	is "gotest.tools/v3/assert/cmp"
+)
+
+func TestFromError(t *testing.T) {
+	tests := []struct {
+		name string
+		err  error
+		want int
+	}{
+		{
+			name: "nil",
+			want: http.StatusInternalServerError,
+		},
+		{
+			name: "not found",
+			err:  cerrdefs.ErrNotFound,
+			want: http.StatusNotFound,
+		},
+		{
+			name: "invalid argument",
+			err:  cerrdefs.ErrInvalidArgument,
+			want: http.StatusBadRequest,
+		},
+		{
+			name: "conflict",
+			err:  cerrdefs.ErrConflict,
+			want: http.StatusConflict,
+		},
+		{
+			name: "unauthorized",
+			err:  cerrdefs.ErrUnauthenticated,
+			want: http.StatusUnauthorized,
+		},
+		{
+			name: "unavailable",
+			err:  cerrdefs.ErrUnavailable,
+			want: http.StatusServiceUnavailable,
+		},
+		{
+			name: "permission denied",
+			err:  cerrdefs.ErrPermissionDenied,
+			want: http.StatusForbidden,
+		},
+		{
+			name: "not modified",
+			err:  cerrdefs.ErrNotModified,
+			want: http.StatusNotModified,
+		},
+		{
+			name: "not implemented",
+			err:  cerrdefs.ErrNotImplemented,
+			want: http.StatusNotImplemented,
+		},
+		{
+			name: "failed precondition",
+			err:  cerrdefs.ErrFailedPrecondition,
+			want: http.StatusPreconditionFailed,
+		},
+		{
+			name: "resource exhausted",
+			err:  cerrdefs.ErrResourceExhausted,
+			want: http.StatusTooManyRequests,
+		},
+		{
+			name: "internal",
+			err:  cerrdefs.ErrInternal,
+			want: http.StatusInternalServerError,
+		},
+		{
+			name: "data loss",
+			err:  cerrdefs.ErrDataLoss,
+			want: http.StatusInternalServerError,
+		},
+		{
+			name: "unknown",
+			err:  cerrdefs.ErrUnknown,
+			want: http.StatusInternalServerError,
+		},
+		{
+			name: "unclassified",
+			err:  errors.New("unclassified"),
+			want: http.StatusInternalServerError,
+		},
+		{
+			name: "canceled",
+			err:  context.Canceled,
+			want: http.StatusInternalServerError,
+		},
+		{
+			name: "deadline exceeded",
+			err:  context.DeadlineExceeded,
+			want: http.StatusInternalServerError,
+		},
+		{
+			name: "wrapped",
+			err:  fmt.Errorf("wrapped: %w", cerrdefs.ErrNotFound),
+			want: http.StatusNotFound,
+		},
+		{
+			name: "outermost classification",
+			err:  fmt.Errorf("wrapped: %w: %w", cerrdefs.ErrInvalidArgument, fmt.Errorf("wrapped: %w", cerrdefs.ErrNotFound)),
+			want: http.StatusBadRequest,
+		},
+		{
+			name: "wrapped unclassified error",
+			err:  fmt.Errorf("wrapped: %w", errors.New("unclassified")),
+			want: http.StatusInternalServerError,
+		},
+		{
+			name: "joined errors",
+			err:  errors.Join(errors.New("unclassified"), cerrdefs.ErrNotFound),
+			want: http.StatusNotFound,
+		},
+		{
+			name: "joined unclassified errors",
+			err:  errors.Join(errors.New("first"), errors.New("second")),
+			want: http.StatusInternalServerError,
+		},
+		// gRPC errors
+		{
+			name: "gRPC not found",
+			err:  status.Error(codes.NotFound, "not found"),
+			want: http.StatusNotFound,
+		},
+		{
+			name: "gRPC failed precondition",
+			err:  status.Error(codes.FailedPrecondition, "failed"),
+			want: http.StatusBadRequest,
+		},
+		// distribution errors
+		{
+			name: "distribution unauthorized",
+			err:  errcode.ErrorCodeUnauthorized,
+			want: http.StatusUnauthorized,
+		},
+		{
+			name: "distribution errors",
+			err:  errcode.Errors{errcode.ErrorCodeDenied},
+			want: http.StatusForbidden,
+		},
+		{
+			name: "empty distribution errors",
+			err:  errcode.Errors{},
+			want: http.StatusInternalServerError,
+		},
+		{
+			name: "wrapped distribution error",
+			err:  fmt.Errorf("wrapped: %w", errcode.ErrorCodeDenied),
+			want: http.StatusForbidden,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			assert.Check(t, is.Equal(tc.want, httpstatus.FromError(tc.err)))
+		})
+	}
+}