From e82b7b2fa00d1f30f3eabb58eaf7d74806581369 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 21 Mar 2022 11:27:39 +0100 Subject: [PATCH] errdefs: move GetHTTPErrorStatusCode to api/server/httpstatus This reverts the changes made in 2a9c987e5a72549775ffa4dc31595ceff4f06a78, which moved the GetHTTPErrorStatusCode() utility to the errdefs package. While it seemed to make sense at the time to have the errdefs package provide conversion both from HTTP status codes errdefs and the reverse, a side-effect of the move was that the errdefs package now had a dependency on various external modules, to handle conversio of errors coming from those sub-systems, such as; - github.com/containerd/containerd - github.com/docker/distribution - google.golang.org/grpc This patch moves the conversion from (errdef-) errors to HTTP status-codes to a api/server/httpstatus package, which is only used by the API server, and should not be needed by client-code using the errdefs package. The MakeErrorHandler() utility was moved to the API server itself, as that's the only place it's used. While the same applies to the GetHTTPErrorStatusCode func, I opted for keeping that in its own package for a slightly cleaner interface. Why not move it into the api/server/httputils package? The api/server/httputils package is also imported in the client package, which uses the httputils.ParseForm() and httputils.HijackConnection() functions as part of the TestTLSCloseWriter() test. While this is only used in tests, I wanted to avoid introducing the indirect depdencencies outside of the api/server code. Signed-off-by: Sebastiaan van Stijn --- api/server/errorhandler.go | 34 ++++ api/server/httpstatus/status.go | 150 ++++++++++++++++++ api/server/httputils/errors_deprecated.go | 9 -- api/server/httputils/httputils.go | 26 --- .../router/container/container_routes.go | 3 +- api/server/server.go | 8 +- errdefs/http_helpers.go | 138 ---------------- 7 files changed, 190 insertions(+), 178 deletions(-) create mode 100644 api/server/errorhandler.go create mode 100644 api/server/httpstatus/status.go delete mode 100644 api/server/httputils/errors_deprecated.go diff --git a/api/server/errorhandler.go b/api/server/errorhandler.go new file mode 100644 index 0000000000..71a0c13c84 --- /dev/null +++ b/api/server/errorhandler.go @@ -0,0 +1,34 @@ +package server + +import ( + "net/http" + + "github.com/docker/docker/api/server/httpstatus" + "github.com/docker/docker/api/server/httputils" + "github.com/docker/docker/api/types" + "github.com/docker/docker/api/types/versions" + "github.com/gorilla/mux" + "google.golang.org/grpc/status" +) + +// makeErrorHandler makes an HTTP handler that decodes a Docker error and +// returns it in the response. +func makeErrorHandler(err error) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + statusCode := httpstatus.FromError(err) + vars := mux.Vars(r) + if apiVersionSupportsJSONErrors(vars["version"]) { + response := &types.ErrorResponse{ + Message: err.Error(), + } + _ = httputils.WriteJSON(w, statusCode, response) + } else { + http.Error(w, status.Convert(err).Message(), statusCode) + } + } +} + +func apiVersionSupportsJSONErrors(version string) bool { + const firstAPIVersionWithJSONErrors = "1.23" + return version == "" || versions.GreaterThan(version, firstAPIVersionWithJSONErrors) +} diff --git a/api/server/httpstatus/status.go b/api/server/httpstatus/status.go new file mode 100644 index 0000000000..f6a8816032 --- /dev/null +++ b/api/server/httpstatus/status.go @@ -0,0 +1,150 @@ +package httpstatus // import "github.com/docker/docker/api/server/httpstatus" + +import ( + "fmt" + "net/http" + + containerderrors "github.com/containerd/containerd/errdefs" + "github.com/docker/distribution/registry/api/errcode" + "github.com/docker/docker/errdefs" + "github.com/sirupsen/logrus" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +type causer interface { + Cause() error +} + +// FromError retrieves status code from error message. +func FromError(err error) int { + if err == nil { + logrus.WithFields(logrus.Fields{"error": err}).Error("unexpected HTTP error handling") + return http.StatusInternalServerError + } + + var statusCode int + + // Stop right there + // Are you sure you should be adding a new error class here? Do one of the existing ones work? + + // Note that the below functions are already checking the error causal chain for matches. + switch { + case errdefs.IsNotFound(err): + statusCode = http.StatusNotFound + case errdefs.IsInvalidParameter(err): + statusCode = http.StatusBadRequest + case errdefs.IsConflict(err): + statusCode = http.StatusConflict + case errdefs.IsUnauthorized(err): + statusCode = http.StatusUnauthorized + case errdefs.IsUnavailable(err): + statusCode = http.StatusServiceUnavailable + case errdefs.IsForbidden(err): + statusCode = http.StatusForbidden + case errdefs.IsNotModified(err): + statusCode = http.StatusNotModified + case errdefs.IsNotImplemented(err): + statusCode = http.StatusNotImplemented + case errdefs.IsSystem(err) || errdefs.IsUnknown(err) || errdefs.IsDataLoss(err) || errdefs.IsDeadline(err) || errdefs.IsCancelled(err): + statusCode = http.StatusInternalServerError + default: + statusCode = statusCodeFromGRPCError(err) + if statusCode != http.StatusInternalServerError { + return statusCode + } + statusCode = statusCodeFromContainerdError(err) + if statusCode != http.StatusInternalServerError { + return statusCode + } + statusCode = statusCodeFromDistributionError(err) + if statusCode != http.StatusInternalServerError { + return statusCode + } + if e, ok := err.(causer); ok { + return FromError(e.Cause()) + } + + logrus.WithFields(logrus.Fields{ + "module": "api", + "error_type": fmt.Sprintf("%T", err), + }).Debugf("FIXME: Got an API for which error does not match any expected type!!!: %+v", err) + } + + if statusCode == 0 { + statusCode = http.StatusInternalServerError + } + + return statusCode +} + +// statusCodeFromGRPCError returns status code according to gRPC error +func statusCodeFromGRPCError(err error) int { + switch status.Code(err) { + case codes.InvalidArgument: // code 3 + return http.StatusBadRequest + case codes.NotFound: // code 5 + return http.StatusNotFound + case codes.AlreadyExists: // code 6 + return http.StatusConflict + case codes.PermissionDenied: // code 7 + return http.StatusForbidden + case codes.FailedPrecondition: // code 9 + return http.StatusBadRequest + case codes.Unauthenticated: // code 16 + return http.StatusUnauthorized + case codes.OutOfRange: // code 11 + return http.StatusBadRequest + case codes.Unimplemented: // code 12 + return http.StatusNotImplemented + case codes.Unavailable: // code 14 + return http.StatusServiceUnavailable + default: + // codes.Canceled(1) + // codes.Unknown(2) + // codes.DeadlineExceeded(4) + // codes.ResourceExhausted(8) + // codes.Aborted(10) + // codes.Internal(13) + // codes.DataLoss(15) + return http.StatusInternalServerError + } +} + +// statusCodeFromDistributionError returns status code according to registry errcode +// code is loosely based on errcode.ServeJSON() in docker/distribution +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 { + return statusCodeFromDistributionError(errs[0]) + } + case errcode.ErrorCoder: + return errs.ErrorCode().Descriptor().HTTPStatusCode + } + return http.StatusInternalServerError +} + +// statusCodeFromContainerdError returns status code for containerd errors when +// consumed directly (not through gRPC) +func statusCodeFromContainerdError(err error) int { + switch { + case containerderrors.IsInvalidArgument(err): + return http.StatusBadRequest + case containerderrors.IsNotFound(err): + return http.StatusNotFound + case containerderrors.IsAlreadyExists(err): + return http.StatusConflict + case containerderrors.IsFailedPrecondition(err): + return http.StatusPreconditionFailed + case containerderrors.IsUnavailable(err): + return http.StatusServiceUnavailable + case containerderrors.IsNotImplemented(err): + return http.StatusNotImplemented + default: + return http.StatusInternalServerError + } +} diff --git a/api/server/httputils/errors_deprecated.go b/api/server/httputils/errors_deprecated.go deleted file mode 100644 index 1263f02f9c..0000000000 --- a/api/server/httputils/errors_deprecated.go +++ /dev/null @@ -1,9 +0,0 @@ -package httputils // import "github.com/docker/docker/api/server/httputils" -import "github.com/docker/docker/errdefs" - -// GetHTTPErrorStatusCode retrieves status code from error message. -// -// Deprecated: use errdefs.GetHTTPErrorStatusCode -func GetHTTPErrorStatusCode(err error) int { - return errdefs.GetHTTPErrorStatusCode(err) -} diff --git a/api/server/httputils/httputils.go b/api/server/httputils/httputils.go index accbb06eb3..da407a80ef 100644 --- a/api/server/httputils/httputils.go +++ b/api/server/httputils/httputils.go @@ -7,13 +7,9 @@ import ( "net/http" "strings" - "github.com/docker/docker/api/types" - "github.com/docker/docker/api/types/versions" "github.com/docker/docker/errdefs" - "github.com/gorilla/mux" "github.com/pkg/errors" "github.com/sirupsen/logrus" - "google.golang.org/grpc/status" ) // APIVersionKey is the client's requested API version. @@ -92,28 +88,6 @@ func VersionFromContext(ctx context.Context) string { return "" } -// MakeErrorHandler makes an HTTP handler that decodes a Docker error and -// returns it in the response. -func MakeErrorHandler(err error) http.HandlerFunc { - return func(w http.ResponseWriter, r *http.Request) { - statusCode := errdefs.GetHTTPErrorStatusCode(err) - vars := mux.Vars(r) - if apiVersionSupportsJSONErrors(vars["version"]) { - response := &types.ErrorResponse{ - Message: err.Error(), - } - _ = WriteJSON(w, statusCode, response) - } else { - http.Error(w, status.Convert(err).Message(), statusCode) - } - } -} - -func apiVersionSupportsJSONErrors(version string) bool { - const firstAPIVersionWithJSONErrors = "1.23" - return version == "" || versions.GreaterThan(version, firstAPIVersionWithJSONErrors) -} - // matchesContentType validates the content type against the expected one func matchesContentType(contentType, expectedType string) bool { mimetype, _, err := mime.ParseMediaType(contentType) diff --git a/api/server/router/container/container_routes.go b/api/server/router/container/container_routes.go index 524d9f197f..de0a4474a7 100644 --- a/api/server/router/container/container_routes.go +++ b/api/server/router/container/container_routes.go @@ -10,6 +10,7 @@ import ( "syscall" "github.com/containerd/containerd/platforms" + "github.com/docker/docker/api/server/httpstatus" "github.com/docker/docker/api/server/httputils" "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/backend" @@ -641,7 +642,7 @@ func (s *containerRouter) postContainersAttach(ctx context.Context, w http.Respo // Remember to close stream if error happens conn, _, errHijack := hijacker.Hijack() if errHijack == nil { - statusCode := errdefs.GetHTTPErrorStatusCode(err) + statusCode := httpstatus.FromError(err) statusText := http.StatusText(statusCode) fmt.Fprintf(conn, "HTTP/1.1 %d %s\r\nContent-Type: application/vnd.docker.raw-stream\r\n\r\n%s\r\n", statusCode, statusText, err.Error()) httputils.CloseStreams(conn) diff --git a/api/server/server.go b/api/server/server.go index a8ce5cb163..71dcb664c7 100644 --- a/api/server/server.go +++ b/api/server/server.go @@ -7,12 +7,12 @@ import ( "net/http" "strings" + "github.com/docker/docker/api/server/httpstatus" "github.com/docker/docker/api/server/httputils" "github.com/docker/docker/api/server/middleware" "github.com/docker/docker/api/server/router" "github.com/docker/docker/api/server/router/debug" "github.com/docker/docker/dockerversion" - "github.com/docker/docker/errdefs" "github.com/gorilla/mux" "github.com/sirupsen/logrus" ) @@ -139,11 +139,11 @@ func (s *Server) makeHTTPHandler(handler httputils.APIFunc) http.HandlerFunc { } if err := handlerFunc(ctx, w, r, vars); err != nil { - statusCode := errdefs.GetHTTPErrorStatusCode(err) + statusCode := httpstatus.FromError(err) if statusCode >= 500 { logrus.Errorf("Handler for %s %s returned error: %v", r.Method, r.URL.Path, err) } - httputils.MakeErrorHandler(err)(w, r) + makeErrorHandler(err)(w, r) } } } @@ -184,7 +184,7 @@ func (s *Server) createMux() *mux.Router { m.Path("/debug" + r.Path()).Handler(f) } - notFoundHandler := httputils.MakeErrorHandler(pageNotFoundError{}) + notFoundHandler := makeErrorHandler(pageNotFoundError{}) m.HandleFunc(versionMatcher+"/{path:.*}", notFoundHandler) m.NotFoundHandler = notFoundHandler m.MethodNotAllowedHandler = notFoundHandler diff --git a/errdefs/http_helpers.go b/errdefs/http_helpers.go index 73576f1c6e..5afe486779 100644 --- a/errdefs/http_helpers.go +++ b/errdefs/http_helpers.go @@ -1,78 +1,11 @@ package errdefs // import "github.com/docker/docker/errdefs" import ( - "fmt" "net/http" - containerderrors "github.com/containerd/containerd/errdefs" - "github.com/docker/distribution/registry/api/errcode" "github.com/sirupsen/logrus" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" ) -// GetHTTPErrorStatusCode retrieves status code from error message. -func GetHTTPErrorStatusCode(err error) int { - if err == nil { - logrus.WithFields(logrus.Fields{"error": err}).Error("unexpected HTTP error handling") - return http.StatusInternalServerError - } - - var statusCode int - - // Stop right there - // Are you sure you should be adding a new error class here? Do one of the existing ones work? - - // Note that the below functions are already checking the error causal chain for matches. - switch { - case IsNotFound(err): - statusCode = http.StatusNotFound - case IsInvalidParameter(err): - statusCode = http.StatusBadRequest - case IsConflict(err): - statusCode = http.StatusConflict - case IsUnauthorized(err): - statusCode = http.StatusUnauthorized - case IsUnavailable(err): - statusCode = http.StatusServiceUnavailable - case IsForbidden(err): - statusCode = http.StatusForbidden - case IsNotModified(err): - statusCode = http.StatusNotModified - case IsNotImplemented(err): - statusCode = http.StatusNotImplemented - case IsSystem(err) || IsUnknown(err) || IsDataLoss(err) || IsDeadline(err) || IsCancelled(err): - statusCode = http.StatusInternalServerError - default: - statusCode = statusCodeFromGRPCError(err) - if statusCode != http.StatusInternalServerError { - return statusCode - } - statusCode = statusCodeFromContainerdError(err) - if statusCode != http.StatusInternalServerError { - return statusCode - } - statusCode = statusCodeFromDistributionError(err) - if statusCode != http.StatusInternalServerError { - return statusCode - } - if e, ok := err.(causer); ok { - return GetHTTPErrorStatusCode(e.Cause()) - } - - logrus.WithFields(logrus.Fields{ - "module": "api", - "error_type": fmt.Sprintf("%T", err), - }).Debugf("FIXME: Got an API for which error does not match any expected type!!!: %+v", err) - } - - if statusCode == 0 { - statusCode = http.StatusInternalServerError - } - - return statusCode -} - // FromStatusCode creates an errdef error, based on the provided HTTP status-code func FromStatusCode(err error, statusCode int) error { if err == nil { @@ -118,74 +51,3 @@ func FromStatusCode(err error, statusCode int) error { } return err } - -// statusCodeFromGRPCError returns status code according to gRPC error -func statusCodeFromGRPCError(err error) int { - switch status.Code(err) { - case codes.InvalidArgument: // code 3 - return http.StatusBadRequest - case codes.NotFound: // code 5 - return http.StatusNotFound - case codes.AlreadyExists: // code 6 - return http.StatusConflict - case codes.PermissionDenied: // code 7 - return http.StatusForbidden - case codes.FailedPrecondition: // code 9 - return http.StatusBadRequest - case codes.Unauthenticated: // code 16 - return http.StatusUnauthorized - case codes.OutOfRange: // code 11 - return http.StatusBadRequest - case codes.Unimplemented: // code 12 - return http.StatusNotImplemented - case codes.Unavailable: // code 14 - return http.StatusServiceUnavailable - default: - // codes.Canceled(1) - // codes.Unknown(2) - // codes.DeadlineExceeded(4) - // codes.ResourceExhausted(8) - // codes.Aborted(10) - // codes.Internal(13) - // codes.DataLoss(15) - return http.StatusInternalServerError - } -} - -// statusCodeFromDistributionError returns status code according to registry errcode -// code is loosely based on errcode.ServeJSON() in docker/distribution -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 { - return statusCodeFromDistributionError(errs[0]) - } - case errcode.ErrorCoder: - return errs.ErrorCode().Descriptor().HTTPStatusCode - } - return http.StatusInternalServerError -} - -// statusCodeFromContainerdError returns status code for containerd errors when -// consumed directly (not through gRPC) -func statusCodeFromContainerdError(err error) int { - switch { - case containerderrors.IsInvalidArgument(err): - return http.StatusBadRequest - case containerderrors.IsNotFound(err): - return http.StatusNotFound - case containerderrors.IsAlreadyExists(err): - return http.StatusConflict - case containerderrors.IsFailedPrecondition(err): - return http.StatusPreconditionFailed - case containerderrors.IsUnavailable(err): - return http.StatusServiceUnavailable - case containerderrors.IsNotImplemented(err): - return http.StatusNotImplemented - default: - return http.StatusInternalServerError - } -}