Fix error handling of incorrect --platform values #39527

thaJeztah · 2019-07-15T16:26:48Z

Relates to docker/docker-py#2382 (comment)

errdefs: convert containerd errors to the correct status code

In situations where the containerd error is consumed directly
and not received over gRPC, errors were not translated.

Add regression tests for invalid platform status codes

Before we handled containerd errors, using an invalid platform produced a 500 status:

curl -v \
-X POST \
--unix-socket /var/run/docker.sock \
"http://localhost:2375/v1.40/images/create?fromImage=hello-world&platform=foobar&tag=latest" \
-H "Content-Type: application/json"

* Connected to localhost (docker.sock) port 80 (#0)
> POST /v1.40/images/create?fromImage=hello-world&platform=foobar&tag=latest HTTP/1.1
> Host: localhost:2375
> User-Agent: curl/7.54.0
> Accept: */*
> Content-Type: application/json
>
< HTTP/1.1 500 Internal Server Error
< Api-Version: 1.40
< Content-Length: 85
< Content-Type: application/json
< Date: Mon, 15 Jul 2019 15:25:44 GMT
< Docker-Experimental: true
< Ostype: linux
< Server: Docker/19.03.0-rc2 (linux)
<
{"message":"\"foobar\": unknown operating system or architecture: invalid argument"}

That problem is now fixed, and the API correctly returns a 4xx status:

curl -v \
-X POST \
--unix-socket /var/run/docker.sock \
"http://localhost:2375/v1.40/images/create?fromImage=hello-world&platform=foobar&tag=latest" \
-H "Content-Type: application/json"

* Connected to localhost (/var/run/docker.sock) port 80 (#0)
> POST /v1.40/images/create?fromImage=hello-world&platform=foobar&tag=latest HTTP/1.1
> Host: localhost:2375
> User-Agent: curl/7.52.1
> Accept: */*
> Content-Type: application/json
>
< HTTP/1.1 400 Bad Request
< Api-Version: 1.41
< Content-Type: application/json
< Docker-Experimental: true
< Ostype: linux
< Server: Docker/dev (linux)
< Date: Mon, 15 Jul 2019 15:13:42 GMT
< Content-Length: 85
<
{"message":"\"foobar\": unknown operating system or architecture: invalid argument"}
* Curl_http_done: called premature == 0

This patch adds tests to validate the behaviour

- How to verify it

make \
  DOCKER_GRAPHDRIVER=vfs \
  TESTDIRS='github.com/docker/docker/integration/image' \
  TESTFLAGS='-test.run ^TestImagePullPlatformInvalid$' \
  binary test-integration


make \
  DOCKER_GRAPHDRIVER=vfs \
  TESTDIRS='github.com/docker/docker/integration/build' \
  TESTFLAGS='-test.run ^TestBuildPlatformInvalid$' \
  binary test-integration

- Description for the changelog

* Fix `POST /images/create` returning a 500 status code when providing an incorrect platform option
* Fix `POST /build` returning a 500 status code when providing an incorrect platform option

thaJeztah · 2019-07-15T16:27:16Z

ping @cpuguy83 @dmcgowan PTAL

cpuguy83

LGTM

errdefs/http_helpers.go

codecov · 2019-07-15T18:36:52Z

Codecov Report

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

@@            Coverage Diff            @@
##             master   #39527   +/-   ##
=========================================
  Coverage          ?   37.29%           
=========================================
  Files             ?      609           
  Lines             ?    45253           
  Branches          ?        0           
=========================================
  Hits              ?    16879           
  Misses            ?    26083           
  Partials          ?     2291

In situations where the containerd error is consumed directly and not received over gRPC, errors were not translated. This patch converts containerd errors to the correct HTTP status code. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Before we handled containerd errors, using an invalid platform produced a 500 status: ```bash curl -v \ -X POST \ --unix-socket /var/run/docker.sock \ "http://localhost:2375/v1.40/images/create?fromImage=hello-world&platform=foobar&tag=latest" \ -H "Content-Type: application/json" ``` ``` * Connected to localhost (docker.sock) port 80 (#0) > POST /v1.40/images/create?fromImage=hello-world&platform=foobar&tag=latest HTTP/1.1 > Host: localhost:2375 > User-Agent: curl/7.54.0 > Accept: */* > Content-Type: application/json > < HTTP/1.1 500 Internal Server Error < Api-Version: 1.40 < Content-Length: 85 < Content-Type: application/json < Date: Mon, 15 Jul 2019 15:25:44 GMT < Docker-Experimental: true < Ostype: linux < Server: Docker/19.03.0-rc2 (linux) < {"message":"\"foobar\": unknown operating system or architecture: invalid argument"} ``` That problem is now fixed, and the API correctly returns a 4xx status: ```bash curl -v \ -X POST \ --unix-socket /var/run/docker.sock \ "http://localhost:2375/v1.40/images/create?fromImage=hello-world&platform=foobar&tag=latest" \ -H "Content-Type: application/json" ``` ``` * Connected to localhost (/var/run/docker.sock) port 80 (#0) > POST /v1.40/images/create?fromImage=hello-world&platform=foobar&tag=latest HTTP/1.1 > Host: localhost:2375 > User-Agent: curl/7.52.1 > Accept: */* > Content-Type: application/json > < HTTP/1.1 400 Bad Request < Api-Version: 1.41 < Content-Type: application/json < Docker-Experimental: true < Ostype: linux < Server: Docker/dev (linux) < Date: Mon, 15 Jul 2019 15:13:42 GMT < Content-Length: 85 < {"message":"\"foobar\": unknown operating system or architecture: invalid argument"} * Curl_http_done: called premature == 0 ``` This patch adds tests to validate the behaviour Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

dmcgowan

LGTM

dmcgowan · 2019-07-15T21:01:05Z

errdefs/http_helpers.go

+
+// statusCodeFromContainerdError returns status code for containerd errors when
+// consumed directly (not through gRPC)
+func statusCodeFromContainerdError(err error) int {


With containerd errors they only need to be unwrapped once with errors.Cause then a normal switch could be used. This is fine though for now, hopefully we can make the techniques more consistent so they can all be checked together in the future (for example maybe docker's version of the Is* functions can just also check containerd errors there too).

Yes, we should look at integrating them (if they all can be mapped). As to unwrapping; it's possible (although not the case currently) that errors are wrapped multiple times, so I guess recursively unwrapping is still ok to do

thaJeztah added area/api API area/runtime Runtime status/2-code-review process/cherry-pick labels Jul 15, 2019

GordonTheTurtle added the status/0-triage label Jul 15, 2019

thaJeztah force-pushed the pull_platform_regression branch from 2afc2bf to 0b7db1f Compare July 15, 2019 16:28

thaJeztah removed the status/0-triage label Jul 15, 2019

thaJeztah requested a review from vdemeester July 15, 2019 16:30

thaJeztah force-pushed the pull_platform_regression branch from 0b7db1f to 7f5bd0d Compare July 15, 2019 16:33

cpuguy83 approved these changes Jul 15, 2019

View reviewed changes

thaJeztah commented Jul 15, 2019

View reviewed changes

errdefs/http_helpers.go Outdated Show resolved Hide resolved

thaJeztah force-pushed the pull_platform_regression branch from 7f5bd0d to 7e54862 Compare July 15, 2019 18:36

thaJeztah added 2 commits July 15, 2019 20:36

thaJeztah force-pushed the pull_platform_regression branch from 7e54862 to 9d1b4f5 Compare July 15, 2019 18:37

dmcgowan approved these changes Jul 15, 2019

View reviewed changes

thaJeztah added status/4-merge and removed status/2-code-review labels Jul 15, 2019

thaJeztah merged commit 81dbed4 into moby:master Jul 16, 2019

thaJeztah deleted the pull_platform_regression branch July 16, 2019 01:29

thaJeztah mentioned this pull request Sep 16, 2019

[19.03 backport] Fix error handling of incorrect --platform values docker-archive/engine#365

Merged

thaJeztah added process/cherry-picked and removed process/cherry-pick labels Sep 16, 2019

thaJeztah added this to the 20.03.0 milestone Apr 2, 2020

thaJeztah mentioned this pull request Jul 13, 2021

errdefs: remove dependency on grpc, containerd, and distribution #42624

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix error handling of incorrect --platform values #39527

Fix error handling of incorrect --platform values #39527

Uh oh!

thaJeztah commented Jul 15, 2019

Uh oh!

thaJeztah commented Jul 15, 2019

Uh oh!

cpuguy83 left a comment

Uh oh!

Uh oh!

codecov bot commented Jul 15, 2019 •

edited

Loading

Uh oh!

dmcgowan left a comment

Uh oh!

dmcgowan Jul 15, 2019

Uh oh!

thaJeztah Jul 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Fix error handling of incorrect --platform values #39527

Fix error handling of incorrect --platform values #39527

Uh oh!

Conversation

thaJeztah commented Jul 15, 2019

errdefs: convert containerd errors to the correct status code

Add regression tests for invalid platform status codes

Uh oh!

thaJeztah commented Jul 15, 2019

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

dmcgowan left a comment

Choose a reason for hiding this comment

Uh oh!

dmcgowan Jul 15, 2019

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jul 15, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Jul 15, 2019 •

edited

Loading