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

[RFC] Make client return "rich" errors #38467

Open
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@thaJeztah
Copy link
Member

thaJeztah commented Dec 31, 2018

The API client currently discards HTTP-status codes that are returned from the daemon. As a result, users of the API client that want to implement logic based on the type of error returned, will have to resort to string-matching (which is brittle).

This PR is a first attempt to make the client return more useful errors;

  • Errors that are returned by the API are converted back into errdef errors, so that it's easy to check the type of error (errdef.IsNotFound(err), errdef.IsConflict(err))
  • The original HTTP status code is returned, so that for errors that have no 1-1 mapping to an errdef will preserve the status-code for further handling

With this patch, something like the below will be possible;

package main

import (
	"context"
	"fmt"

	"github.com/docker/docker/api/server/httputils"
	"github.com/docker/docker/api/types"
	"github.com/docker/docker/api/types/container"
	"github.com/docker/docker/client"
	"github.com/docker/docker/errdefs"
)

func main() {
	ctx := context.Background()
	cli, err := client.NewClientWithOpts(client.FromEnv)
	if err != nil {
		panic(err)
	}
	cli.NegotiateAPIVersion(ctx)

	_, err = cli.ImagePull(ctx, "docker.io/library/alpine", types.ImagePullOptions{})
	if err != nil {
		panic(err)
	}

	_, err = cli.ContainerCreate(ctx, &container.Config{
		Image: "nosuchimage",
		Cmd:   []string{"echo", "hello world"},
	}, nil, nil, "")
	if err != nil {
		fmt.Println(err.Error())
		if errdefs.IsNotFound(err) {
			fmt.Println("it's a 'not found' error")
		}
		fmt.Println("status code: ", httputils.GetHTTPErrorStatusCode(err))
	}

	_, err = cli.ContainerCreate(ctx, &container.Config{
		Image: "invalid@@@name",
		Cmd:   []string{"echo", "hello world"},
	}, nil, nil, "")
	if err != nil {
		fmt.Println(err.Error())
		if errdefs.IsInvalidParameter(err) {
			fmt.Println("it's an 'invalid parameter' error")
		}
		fmt.Println("status code: ", httputils.GetHTTPErrorStatusCode(err))
	}
}

Feedback needed

  • I was a bit in doubt where we want the helpers/types for the new errors defined;
    • the api/server/httputils package? (could make sense, because it deals with the conversion from errdefs errors to HTTP errors)
    • the errdefs package? (I think we want to keep that package separated from any HTTP-specific logic
    • in the client? (there's already some error-handling stuff in client/errors.go); could make sense, because there are also client-side errors that could be defined there
  • Is returning the HTTP status too low-level, and should we limit to only converting back to errdef Errors?
  • As a follow-up; do we want a conversion from error-types to exit-codes for the cli itself?
@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Dec 31, 2018

@thaJeztah thaJeztah added area/api and removed status/0-triage labels Dec 31, 2018


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

This comment has been minimized.

@thaJeztah

thaJeztah Dec 31, 2018

Member

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
}

This comment has been minimized.

@thaJeztah

thaJeztah Dec 31, 2018

Member

Related to the above: #38462

This comment has been minimized.

@thaJeztah

thaJeztah Jan 10, 2019

Member

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()

@thaJeztah

This comment was marked as resolved.

Copy link
Member

thaJeztah commented Jan 1, 2019

hmf

api/server/httputils/errors_test.go:97:22:warning: undeclared name: errTest (gosimple)
18:18:20 Build step 'Execute shell' marked build as failure

@thaJeztah thaJeztah force-pushed the thaJeztah:add_errdefs_utils branch 3 times, most recently from fc418e6 to 3531246 Jan 1, 2019

@thaJeztah thaJeztah added the rebuild/* label Jan 2, 2019

@olljanat

This comment has been minimized.

Copy link
Contributor

olljanat commented Jan 9, 2019

(reserved for my derek commands)

thaJeztah added some commits Dec 31, 2018

Add httputils.FromStatusCode()
This utility allows a client to convert an API response
back to a typed error; allowing the client to perform
different actions based on the type of error, without
having to resort to string-matching the error.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Implement httputils.ErrWithStatusCode
This error-type enables getting the HTTP status code
that was returned by the API for further handling.

This can be useful for errors that do not have a corresponding
error defined in the errdefs package, or errors that are mapped
to the same errdef error (alowing the raw status-code to be obtained).

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
client/request: don't discard server response info on error
The client is currently discarding the server information
if the request resulted in an error.

This patch keeps the server response, even in an error-situation,
so that additional information from the server (such as the
status-code) can be used.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Make client/errors helpers work with errdefs errors
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Make client.notfound error match errdefs.notfound
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
client: return rich / errdefs errors
this patch makes the client return errors matching
the errdefs interface.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

@thaJeztah thaJeztah force-pushed the thaJeztah:add_errdefs_utils branch from 3531246 to a5f0d9e Jan 9, 2019

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 9, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@cb50188). Click here to learn what that means.
The diff coverage is 62.65%.

@@            Coverage Diff            @@
##             master   #38467   +/-   ##
=========================================
  Coverage          ?   36.65%           
=========================================
  Files             ?      608           
  Lines             ?    45238           
  Branches          ?        0           
=========================================
  Hits              ?    16583           
  Misses            ?    26363           
  Partials          ?     2292

thaJeztah added some commits Dec 31, 2018

Update tests to check returned errors
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
client: remove special error handling for "no such image"
looks like we don't need this handling

Before this patch:

    Error: No such image: nosuchimage

After this patch:

    Error response from daemon: No such image: nosuchimage:latest
"

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

@thaJeztah thaJeztah force-pushed the thaJeztah:add_errdefs_utils branch from a5f0d9e to 911a6d5 Jan 9, 2019

err = errdefs.Unavailable(err)
case http.StatusForbidden:
err = errdefs.Forbidden(err)
case http.StatusNotModified:

This comment has been minimized.

@thaJeztah

thaJeztah Jan 10, 2019

Member

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}
}

This comment has been minimized.

@thaJeztah

thaJeztah Jan 10, 2019

Member

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 🤷‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment