Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make client return "rich" errors #38467

Closed
wants to merge 8 commits into from
101 changes: 101 additions & 0 deletions api/server/httputils/errors.go
Expand Up @@ -23,6 +23,9 @@ func GetHTTPErrorStatusCode(err error) int {
logrus.WithFields(logrus.Fields{"error": err}).Error("unexpected HTTP error handling")
return http.StatusInternalServerError
}
if e, ok := getImplementer(err).(ErrWithStatusCode); ok {
return e.StatusCode()
}

var statusCode int

Expand Down Expand Up @@ -72,6 +75,104 @@ func GetHTTPErrorStatusCode(err error) int {
return statusCode
}

// FromStatusCode creates an errdef error, based on the provided status-code
func FromStatusCode(err error, statusCode int) error {
if err == nil {
return err
}
switch statusCode {
case http.StatusNotFound:
err = errdefs.NotFound(err)
case http.StatusBadRequest:
err = errdefs.InvalidParameter(err)
case http.StatusConflict:
err = errdefs.Conflict(err)
case http.StatusUnauthorized:
err = errdefs.Unauthorized(err)
case http.StatusServiceUnavailable:
err = errdefs.Unavailable(err)
case http.StatusForbidden:
err = errdefs.Forbidden(err)
case http.StatusNotModified:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may actually have to be discussed as well; this maps to a 304 Not Modified, so not an "error" (similar to #38462);

moby/errdefs/helpers.go

Lines 117 to 131 in cb50188

type errNotModified struct{ error }
func (errNotModified) NotModified() {}
func (e errNotModified) Cause() error {
return e.error
}
// NotModified is a helper to create an error of the class with the same name from any error type
func NotModified(err error) error {
if err == nil || IsNotModified(err) {
return err
}
return errNotModified{err}
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NotModified() is in active use (see

moby/daemon/start.go

Lines 36 to 38 in d4a6e1c

if container.Running {
return containerNotModifiedError{running: true}
}
). I guess the confusing bit there is that it's treated as an error but not really an error. Maybe just semantics 🤷‍♂️

err = errdefs.NotModified(err)
case http.StatusNotImplemented:
err = errdefs.NotImplemented(err)
case http.StatusInternalServerError:
if !errdefs.IsSystem(err) && !errdefs.IsUnknown(err) && !errdefs.IsDataLoss(err) && !errdefs.IsDeadline(err) && !errdefs.IsCancelled(err) {
err = errdefs.System(err)
}
default:
logrus.WithFields(logrus.Fields{
"module": "api",
"status_code": fmt.Sprintf("%d", statusCode),
}).Debugf("FIXME: Got an status-code for which error does not match any expected type!!!: %d", statusCode)

switch {
case statusCode >= 200 && statusCode < 400:
// it's a client error
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally had this at the top to return nil, but not sure what's best. Note that the client actually discards these errors;

moby/client/request.go

Lines 194 to 196 in 3e5b9cb

if serverResp.statusCode >= 200 && serverResp.statusCode < 400 {
return nil
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to the above: #38462

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar implementation 😄 https://github.com/cpuguy83/strongerrors/blob/67f9d3e5dd563226c4b7956e0c1d8f20ffb026d7/status/http.go#L39-L71

Biggest difference (at a glance);

My implementation does not return Unknown for errors if the the status-code is in a known range (i.e. any unknown 4xx error returns an InvalidParameter and any 5xx status return a System error.

Downside is that this may make errors Ambiguous; i.e. a 504 Gateway Timeout (which currently doesn't have a definition / mapping) is transformed into a generic System error. But the original status code is preserved, and can be obtained with err.StatusCode()

case statusCode >= 400 && statusCode < 500:
err = errdefs.InvalidParameter(err)
case statusCode >= 500 && statusCode < 600:
err = errdefs.System(err)
default:
err = errdefs.Unknown(err)
}
}

return WithStatusCode(err, statusCode)
}

// ErrWithStatusCode is an error that provides the HTTP status-code
type ErrWithStatusCode interface {
error
StatusCode() int
}

type errWithStatusCode struct {
error error
statusCode int
}

func (e errWithStatusCode) StatusCode() int {
return e.statusCode
}

func (e errWithStatusCode) Cause() error {
return e.error
}

func (e errWithStatusCode) Error() string {
return e.error.Error()
}

// WithStatusCode is a helper to create an error with a HTTP-statuscode
func WithStatusCode(err error, statusCode int) error {
if err == nil {
return nil
}
if IsWithStatusCode(err) {
return err
}
return errWithStatusCode{error: err, statusCode: statusCode}
}

// IsWithStatusCode returns if the passed in error is an ErrWithStatusCode error
func IsWithStatusCode(err error) bool {
_, ok := getImplementer(err).(ErrWithStatusCode)
return ok
}

func getImplementer(err error) error {
switch e := err.(type) {
case ErrWithStatusCode:
return err
case causer:
return getImplementer(e.Cause())
default:
return err
}
}

func apiVersionSupportsJSONErrors(version string) bool {
const firstAPIVersionWithJSONErrors = "1.23"
return version == "" || versions.GreaterThan(version, firstAPIVersionWithJSONErrors)
Expand Down
115 changes: 115 additions & 0 deletions api/server/httputils/errors_test.go
@@ -0,0 +1,115 @@
package httputils

import (
"fmt"
"net/http"
"testing"

"github.com/docker/docker/errdefs"
"gotest.tools/assert"
)

func TestFromStatusCode(t *testing.T) {
testErr := fmt.Errorf("some error occurred")

testCases := []struct {
err error
status int
check func(error) bool
}{
{
err: testErr,
status: http.StatusNotFound,
check: errdefs.IsNotFound,
},
{
err: testErr,
status: http.StatusBadRequest,
check: errdefs.IsInvalidParameter,
},
{
err: testErr,
status: http.StatusConflict,
check: errdefs.IsConflict,
},
{
err: testErr,
status: http.StatusUnauthorized,
check: errdefs.IsUnauthorized,
},
{
err: testErr,
status: http.StatusServiceUnavailable,
check: errdefs.IsUnavailable,
},
{
err: testErr,
status: http.StatusForbidden,
check: errdefs.IsForbidden,
},
{
err: testErr,
status: http.StatusNotModified,
check: errdefs.IsNotModified,
},
{
err: testErr,
status: http.StatusNotImplemented,
check: errdefs.IsNotImplemented,
},
{
err: testErr,
status: http.StatusInternalServerError,
check: errdefs.IsSystem,
},
{
err: errdefs.Unknown(testErr),
status: http.StatusInternalServerError,
check: errdefs.IsUnknown,
},
{
err: errdefs.DataLoss(testErr),
status: http.StatusInternalServerError,
check: errdefs.IsDataLoss,
},
{
err: errdefs.Deadline(testErr),
status: http.StatusInternalServerError,
check: errdefs.IsDeadline,
},
{
err: errdefs.Cancelled(testErr),
status: http.StatusInternalServerError,
check: errdefs.IsCancelled,
},
}

for _, tc := range testCases {
t.Run(http.StatusText(tc.status), func(t *testing.T) {
err := FromStatusCode(tc.err, tc.status)
assert.Check(t, tc.check(err), "unexpected error-type %T", err)
})
}
}

func TestWithStatusCode(t *testing.T) {
testErr := fmt.Errorf("some error occurred")

type causal interface {
Cause() error
}

if IsWithStatusCode(testErr) {
t.Fatalf("did not expect error with status code, got %T", testErr)
}
e := WithStatusCode(testErr, 499)
if !IsWithStatusCode(e) {
t.Fatalf("expected error with status code, got %T", e)
}
if cause := e.(causal).Cause(); cause != testErr {
t.Fatalf("causual should be errTest, got: %v", cause)
}
if status := e.(ErrWithStatusCode).StatusCode(); status != 499 {
t.Fatalf("status should be 499, got: %d", status)
}
}
4 changes: 4 additions & 0 deletions client/checkpoint_create_test.go
Expand Up @@ -11,6 +11,7 @@ import (
"testing"

"github.com/docker/docker/api/types"
"github.com/docker/docker/errdefs"
)

func TestCheckpointCreateError(t *testing.T) {
Expand All @@ -25,6 +26,9 @@ func TestCheckpointCreateError(t *testing.T) {
if err == nil || err.Error() != "Error response from daemon: Server error" {
t.Fatalf("expected a Server Error, got %v", err)
}
if !errdefs.IsSystem(err) {
t.Fatalf("expected a Server Error, got %T", err)
}
}

func TestCheckpointCreate(t *testing.T) {
Expand Down
4 changes: 4 additions & 0 deletions client/checkpoint_delete_test.go
Expand Up @@ -10,6 +10,7 @@ import (
"testing"

"github.com/docker/docker/api/types"
"github.com/docker/docker/errdefs"
)

func TestCheckpointDeleteError(t *testing.T) {
Expand All @@ -24,6 +25,9 @@ func TestCheckpointDeleteError(t *testing.T) {
if err == nil || err.Error() != "Error response from daemon: Server error" {
t.Fatalf("expected a Server Error, got %v", err)
}
if !errdefs.IsSystem(err) {
t.Fatalf("expected a Server Error, got %T", err)
}
}

func TestCheckpointDelete(t *testing.T) {
Expand Down
4 changes: 4 additions & 0 deletions client/checkpoint_list_test.go
Expand Up @@ -11,6 +11,7 @@ import (
"testing"

"github.com/docker/docker/api/types"
"github.com/docker/docker/errdefs"
)

func TestCheckpointListError(t *testing.T) {
Expand All @@ -22,6 +23,9 @@ func TestCheckpointListError(t *testing.T) {
if err == nil || err.Error() != "Error response from daemon: Server error" {
t.Fatalf("expected a Server Error, got %v", err)
}
if !errdefs.IsSystem(err) {
t.Fatalf("expected a Server Error, got %T", err)
}
}

func TestCheckpointList(t *testing.T) {
Expand Down
4 changes: 4 additions & 0 deletions client/config_create_test.go
Expand Up @@ -12,6 +12,7 @@ import (

"github.com/docker/docker/api/types"
"github.com/docker/docker/api/types/swarm"
"github.com/docker/docker/errdefs"
"gotest.tools/assert"
is "gotest.tools/assert/cmp"
)
Expand All @@ -34,6 +35,9 @@ func TestConfigCreateError(t *testing.T) {
if err == nil || err.Error() != "Error response from daemon: Server error" {
t.Fatalf("expected a Server Error, got %v", err)
}
if !errdefs.IsSystem(err) {
t.Fatalf("expected a Server Error, got %T", err)
}
}

func TestConfigCreate(t *testing.T) {
Expand Down
4 changes: 4 additions & 0 deletions client/config_inspect_test.go
Expand Up @@ -11,6 +11,7 @@ import (
"testing"

"github.com/docker/docker/api/types/swarm"
"github.com/docker/docker/errdefs"
"github.com/pkg/errors"
"gotest.tools/assert"
is "gotest.tools/assert/cmp"
Expand Down Expand Up @@ -58,6 +59,9 @@ func TestConfigInspectError(t *testing.T) {
if err == nil || err.Error() != "Error response from daemon: Server error" {
t.Fatalf("expected a Server Error, got %v", err)
}
if !errdefs.IsSystem(err) {
t.Fatalf("expected a Server Error, got %T", err)
}
}

func TestConfigInspectConfigNotFound(t *testing.T) {
Expand Down
4 changes: 4 additions & 0 deletions client/config_list_test.go
Expand Up @@ -13,6 +13,7 @@ import (
"github.com/docker/docker/api/types"
"github.com/docker/docker/api/types/filters"
"github.com/docker/docker/api/types/swarm"
"github.com/docker/docker/errdefs"
"gotest.tools/assert"
is "gotest.tools/assert/cmp"
)
Expand All @@ -36,6 +37,9 @@ func TestConfigListError(t *testing.T) {
if err == nil || err.Error() != "Error response from daemon: Server error" {
t.Fatalf("expected a Server Error, got %v", err)
}
if !errdefs.IsSystem(err) {
t.Fatalf("expected a Server Error, got %T", err)
}
}

func TestConfigList(t *testing.T) {
Expand Down
4 changes: 4 additions & 0 deletions client/config_remove_test.go
Expand Up @@ -9,6 +9,7 @@ import (
"strings"
"testing"

"github.com/docker/docker/errdefs"
"gotest.tools/assert"
is "gotest.tools/assert/cmp"
)
Expand All @@ -32,6 +33,9 @@ func TestConfigRemoveError(t *testing.T) {
if err == nil || err.Error() != "Error response from daemon: Server error" {
t.Fatalf("expected a Server Error, got %v", err)
}
if !errdefs.IsSystem(err) {
t.Fatalf("expected a Server Error, got %T", err)
}
}

func TestConfigRemove(t *testing.T) {
Expand Down
4 changes: 4 additions & 0 deletions client/config_update_test.go
Expand Up @@ -10,6 +10,7 @@ import (
"testing"

"github.com/docker/docker/api/types/swarm"
"github.com/docker/docker/errdefs"
"gotest.tools/assert"
is "gotest.tools/assert/cmp"
)
Expand All @@ -33,6 +34,9 @@ func TestConfigUpdateError(t *testing.T) {
if err == nil || err.Error() != "Error response from daemon: Server error" {
t.Fatalf("expected a Server Error, got %v", err)
}
if !errdefs.IsSystem(err) {
t.Fatalf("expected a Server Error, got %T", err)
}
}

func TestConfigUpdate(t *testing.T) {
Expand Down
4 changes: 4 additions & 0 deletions client/container_commit_test.go
Expand Up @@ -11,6 +11,7 @@ import (
"testing"

"github.com/docker/docker/api/types"
"github.com/docker/docker/errdefs"
)

func TestContainerCommitError(t *testing.T) {
Expand All @@ -21,6 +22,9 @@ func TestContainerCommitError(t *testing.T) {
if err == nil || err.Error() != "Error response from daemon: Server error" {
t.Fatalf("expected a Server Error, got %v", err)
}
if !errdefs.IsSystem(err) {
t.Fatalf("expected a Server Error, got %T", err)
}
}

func TestContainerCommit(t *testing.T) {
Expand Down