Make client return "rich" errors by thaJeztah · Pull Request #38467 · moby/moby

thaJeztah · 2018-12-31T18:06:30Z

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 · 2018-12-31T18:06:57Z

ping @cpuguy83 @vdemeester @wsong @mavenugo PTAL

thaJeztah · 2018-12-31T18:12:18Z

api/server/httputils/errors.go

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

}

Related to the above: #38462

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

olljanat · 2019-01-09T11:59:18Z

(reserved for my derek commands)

codecov · 2019-01-09T12:36:48Z

Codecov Report

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

@@            Coverage Diff            @@
##             master   #38467   +/-   ##
=========================================
  Coverage          ?   36.65%           
=========================================
  Files             ?      610           
  Lines             ?    45298           
  Branches          ?        0           
=========================================
  Hits              ?    16606           
  Misses            ?    26409           
  Partials          ?     2283

thaJeztah · 2019-01-10T20:10:31Z

api/server/httputils/errors.go

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}

}

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

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 <[email protected]>

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 <[email protected]>

Signed-off-by: Sebastiaan van Stijn <[email protected]>

this patch makes the client return errors matching the errdefs interface. Signed-off-by: Sebastiaan van Stijn <[email protected]>

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 <[email protected]>

Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah · 2019-01-31T23:24:12Z

Hm... some Windows failure on RS1
https://jenkins.dockerproject.org/job/Docker-PRs-WoW-RS1/23921/console

23:00:06 FAIL: check_test.go:107: DockerSuite.TearDownTest
23:00:06 
23:00:06 assertion failed: error is not nil: Error response from daemon: container 0556b5ad134bba95bb4495a48c59ed93aaf31f6a9b0a4a889acb01fdb253e72a:
driver "windowsfilter" failed to remove root filesystem: hcsshim::GetComputeSystems:
The requested compute system operation is not valid in the current state.:
failed to remove 0556b5ad134bba95bb4495a48c59ed93aaf31f6a9b0a4a889acb01fdb253e72a
23:00:06 
23:00:06 ----------------------------------------------------------------------
23:00:06 PANIC: docker_cli_start_test.go:189: DockerSuite.TestStartReturnCorrectExitCode
23:00:06 
23:00:06 docker_cli_start_test.go:199:
23:00:06     c.Assert(exitCode, checker.Equals, 12, check.Commentf("out: %s", out))
23:00:06 ... obtained int = 125
23:00:06 ... expected int = 12
23:00:06 ... out: time="2019-01-31T23:00:06Z" level=error msg="error waiting for container: EOF" 
23:00:06 
23:00:06 
23:00:06 ... Panic: Fixture has panicked (see related PANIC)

cpuguy83 · 2019-02-02T05:16:47Z

I think maybe instead of creating an interface for this, we should just have a package that you can get the status code from for any error...

func HTTPStatusCode(err error) (int, bool)

For example

thaJeztah · 2019-02-02T17:39:43Z

I think maybe instead of creating an interface for this, we should just have a package that you can get the status code from for any error...

@cpuguy83 trying to grasp your suggestion; do you mean we'd still add the WithStatusCode() utility to wrap the existing errors (and add a StatusCode fields), or only do the FromStatusCode conversion (converting http.StatusNotFound to errdefs.NotFound(err))?

The point is that if the latter, that there's http statuses that don't map to an errdef, so in that case it would become an errdefs.Unknown or (if the status is 4xx or 5xx) a generic errdefs.InvalidParameter or errdefs.System). In that case, the original status code would get lost.

cpuguy83 · 2019-02-05T23:06:00Z

I mean, adding an interface with StatusCode is not needed.

If there is a status that we can't convert, we should look at adding the right type for it.

thaJeztah · 2019-02-07T15:20:00Z

@cpuguy83 I created #38689, which is the same as this one, but without commit 2c633a2

thaJeztah · 2019-02-07T15:22:23Z

If there is a status that we can't convert, we should look at adding the right type for it.

My concern there was a bit that (especially if communication with a registry is involved) we may not know all status-codes upfront, and (I guess) we don't want to be implementing a IamATeaPot() errdef, or other statuses, such as a HTTP 451

cpuguy83 · 2019-02-07T16:34:35Z

We shouldn't need to think about things in terms of http status codes, but rather failure modes and how to communicate that failure mode.

In general I did model these interfaces after grpc status codes just because they are pretty well designed and meant exactly for communicating a failure type over the wire.

A registry can return whatever it wants, we can only support a subset of that unless the distribution spec defines what a registry should send.

cpuguy83 · 2019-02-07T16:36:03Z

And taking that a step further, it's not even so much failure modes hit recovery modes.

thaJeztah · 2019-02-07T16:43:22Z

Yeah, perhaps it's sufficient to only handle what we defined; my train of thought was to preserve the information we get, and allow the consumer to still access it through the status code interface.

But I'm ok leaving that out, and go for #38689 instead

thaJeztah added the status/1-design-review label Dec 31, 2018

GordonTheTurtle added the status/0-triage label Dec 31, 2018

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

thaJeztah commented Dec 31, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

thaJeztah force-pushed the add_errdefs_utils branch 3 times, most recently from fc418e6 to 3531246 Compare January 1, 2019 15:39

thaJeztah added the rebuild/* label Jan 2, 2019

GordonTheTurtle removed the rebuild/* label Jan 2, 2019

derek bot added the rebuild/z label Jan 9, 2019

GordonTheTurtle removed the rebuild/z label Jan 9, 2019

derek bot added the rebuild/windowsRS1 label Jan 9, 2019

GordonTheTurtle removed the rebuild/windowsRS1 label Jan 9, 2019

thaJeztah force-pushed the add_errdefs_utils branch from 3531246 to a5f0d9e Compare January 9, 2019 12:36

thaJeztah force-pushed the add_errdefs_utils branch from a5f0d9e to 911a6d5 Compare January 9, 2019 12:45

thaJeztah commented Jan 10, 2019

View reviewed changes

GordonTheTurtle assigned dnephin Jan 15, 2019

chris-crone mentioned this pull request Jan 16, 2019

Pushing a non existent tag doesn't raise an exception docker/docker-py#2226

Open

thaJeztah mentioned this pull request Jan 24, 2019

Include old error-message for backward compatibility moby/swarmkit#2797

Merged

thaJeztah added 6 commits January 31, 2019 23:16

Make client/errors helpers work with errdefs errors

ff3ec5b

Signed-off-by: Sebastiaan van Stijn <[email protected]>

Make client.notfound error match errdefs.notfound

cc8ff72

Signed-off-by: Sebastiaan van Stijn <[email protected]>

client: return rich / errdefs errors

e12ceca

this patch makes the client return errors matching the errdefs interface. Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah added 2 commits January 31, 2019 23:17

Update tests to check returned errors

de6f271

Signed-off-by: Sebastiaan van Stijn <[email protected]>

Update more tests to use new errors

e7dacbd

Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah force-pushed the add_errdefs_utils branch from 911a6d5 to e7dacbd Compare January 31, 2019 22:18

thaJeztah changed the title ~~[RFC] Make client return "rich" errors~~ Make client return "rich" errors Jan 31, 2019

thaJeztah added the rebuild/windowsRS1 label Jan 31, 2019

GordonTheTurtle removed the rebuild/windowsRS1 label Jan 31, 2019

thaJeztah mentioned this pull request Feb 7, 2019

Make client return "rich" errors (take 2) #38689

Merged

thaJeztah mentioned this pull request Mar 6, 2019

[WIP] Add support for server-side stacks #38832

Closed

thaJeztah closed this in #38689 Mar 16, 2019

thaJeztah deleted the add_errdefs_utils branch April 28, 2025 16:18

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

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

	if container.Running {
	return containerNotModifiedError{running: true}
	}

Conversation

thaJeztah commented Dec 31, 2018

Feedback needed

Uh oh!

thaJeztah commented Dec 31, 2018

Uh oh!

thaJeztah Dec 31, 2018

Choose a reason for hiding this comment

Uh oh!

thaJeztah Dec 31, 2018

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jan 10, 2019

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

olljanat commented Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thaJeztah Jan 10, 2019

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jan 10, 2019

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jan 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpuguy83 commented Feb 2, 2019

Uh oh!

thaJeztah commented Feb 2, 2019

Uh oh!

cpuguy83 commented Feb 5, 2019

Uh oh!

thaJeztah commented Feb 7, 2019

Uh oh!

thaJeztah commented Feb 7, 2019

Uh oh!

cpuguy83 commented Feb 7, 2019

Uh oh!

cpuguy83 commented Feb 7, 2019

Uh oh!

thaJeztah commented Feb 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

olljanat commented Jan 9, 2019 •

edited

Loading

codecov bot commented Jan 9, 2019 •

edited

Loading

thaJeztah commented Jan 31, 2019 •

edited

Loading