Skip to content

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Feb 7, 2019

closes #38467
Alternative to #38467, but without preserving the original status code (only transforming status codes back to either a known errdef or to a "generic" system/invalidrequest error

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?

@codecov
Copy link

codecov bot commented Feb 7, 2019

Codecov Report

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

@@            Coverage Diff            @@
##             master   #38689   +/-   ##
=========================================
  Coverage          ?   36.48%           
=========================================
  Files             ?      614           
  Lines             ?    45876           
  Branches          ?        0           
=========================================
  Hits              ?    16737           
  Misses            ?    26857           
  Partials          ?     2282

@thaJeztah
Copy link
Member Author

discussing in the maintainers meeting; we can move the http utilities to the errdefs package

@thaJeztah
Copy link
Member Author

ping @cpuguy83 is this what you meant (at least for now?); I can work on a follow-up to preserve the statuscode (without the interface)

Copy link
Member

Choose a reason for hiding this comment

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

Did we mention we'd move this to errdefs?

Copy link
Member Author

Choose a reason for hiding this comment

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

ach, yes, slipped my mind; let me work in that one

Copy link
Member

Choose a reason for hiding this comment

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

Not sure I understand these checks here. We have a mock client and are testing that the mock does what we just told it to do?
Seems strange... of course I know you didn't write this.

@cpuguy83
Copy link
Member

cpuguy83 commented Feb 9, 2019

Looks good.

@thaJeztah
Copy link
Member Author

ping @cpuguy83 I moved the httputils (except for MakeErrorHandler, which I think didn't really fit the errdefs package); PTAL

@thaJeztah thaJeztah force-pushed the add_errdefs_utils_take2 branch from 173faf3 to be0be67 Compare February 9, 2019 15:07
client/errors.go Outdated
Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, thinking of reverting this change; I already updated the IsNotFound() utility to match both this error, and the errdefs.NotFound() interface, so perhaps we should keep this one (for now) to make both the old and new ones work.

@cpuguy83 wdyt?

@cpuguy83
Copy link
Member

0:24:21 --- FAIL: TestCopyToContainerNotStatusOKError (0.00s)
10:24:21     container_copy_test.go:130: expected an unexpected status code error, got <nil>
10:24:21 --- FAIL: TestCopyFromContainerNotStatusOKError (0.00s)
10:24:21     container_copy_test.go:209: expected an unexpected status code error, got unable to get resource stat from response: unable to decode container path stat header: EOF
10:24:21 2019/02/09 18:24:21 RoundTripper returned a response & error; ignoring response
10:24:21 2019/02/09 18:24:21 RoundTripper returned a response & error; ignoring response

@thaJeztah
Copy link
Member Author

Argh. That test looks weird; it's expecting an error to be produced for a non-error (204) status 🤔

@thaJeztah thaJeztah force-pushed the add_errdefs_utils_take2 branch from 76dc0d8 to cbc1d4f Compare February 11, 2019 14:45
@thaJeztah
Copy link
Member Author

For now, reverted that part in the last commit, but added TODO's instead (as it looks incorrect)

// TODO this code converts non-error status-codes (e.g., "204 No Content") into an error; verify if this is the desired behavior
if response.statusCode != http.StatusOK {
	return nil, types.ContainerPathStat{}, fmt.Errorf("unexpected status code from daemon: %d", response.statusCode)
}

@thaJeztah
Copy link
Member Author

Interesting; failures on docker-py tests;

17:59:25 ==================================== ERRORS ====================================
17:59:25 ____ ERROR at teardown of TestNetworks.test_create_network_with_ipam_config ____
17:59:25 /docker-py/tests/integration/api_network_test.py:11: in tearDown
17:59:25     self.client.leave_swarm(force=True)
17:59:25 /docker-py/docker/utils/decorators.py:34: in wrapper
17:59:25     return f(self, *args, **kwargs)
17:59:25 /docker-py/docker/api/swarm.py:231: in leave_swarm
17:59:25     self._raise_for_status(response)
17:59:25 /docker-py/docker/api/client.py:258: in _raise_for_status
17:59:25     raise create_api_error_from_http_exception(e)
17:59:25 /docker-py/docker/errors.py:31: in create_api_error_from_http_exception
17:59:25     raise cls(e, response=response, explanation=explanation)
17:59:25 E   APIError: 500 Server Error: Internal Server Error ("This node is not part of a swarm")

@thaJeztah
Copy link
Member Author

Interesting; failures on docker-py tests;

Ah! Found what happened 😂 My IDE tried to be helpful and decided to add an import for github.com/containerd/containerd/errdefs when I moved the httputils file 🤦‍♂️

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>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
this patch makes the client return errors matching
the errdefs interface.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
the containerd errdefs functions have the same name as the
docker errdefs, but their types use a different signature;
use an alias to prevent them from being mistaken for the
docker errdefs equivalents.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the add_errdefs_utils_take2 branch from 042e4f7 to 818d0dc Compare March 15, 2019 23:44
@thaJeztah
Copy link
Member Author

Updated the comment; PTAL

@thaJeztah
Copy link
Member Author

so rich

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants