From 7b2d5556d57a9e140c6068a7b7d85e44d28e2d31 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 15 Jul 2019 18:15:45 +0200 Subject: [PATCH 1/2] 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. This patch converts containerd errors to the correct HTTP status code. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 4a516215e26542b723fe2d74c6c196a366164473) Signed-off-by: Sebastiaan van Stijn --- errdefs/http_helpers.go | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/errdefs/http_helpers.go b/errdefs/http_helpers.go index ac9bf6d33e673..c67a65b6ae26b 100644 --- a/errdefs/http_helpers.go +++ b/errdefs/http_helpers.go @@ -4,6 +4,7 @@ import ( "fmt" "net/http" + containerderrors "github.com/containerd/containerd/errdefs" "github.com/docker/distribution/registry/api/errcode" "github.com/sirupsen/logrus" "google.golang.org/grpc/codes" @@ -47,6 +48,10 @@ func GetHTTPErrorStatusCode(err error) int { if statusCode != http.StatusInternalServerError { return statusCode } + statusCode = statusCodeFromContainerdError(err) + if statusCode != http.StatusInternalServerError { + return statusCode + } statusCode = statusCodeFromDistributionError(err) if statusCode != http.StatusInternalServerError { return statusCode @@ -170,3 +175,24 @@ func statusCodeFromDistributionError(err error) int { } return http.StatusInternalServerError } + +// statusCodeFromContainerdError returns status code for containerd errors when +// consumed directory (not through gRPC) +func statusCodeFromContainerdError(err error) int { + switch { + case containerderrors.IsInvalidArgument(err): + return http.StatusBadRequest + case containerderrors.IsNotFound(err): + return http.StatusNotFound + case containerderrors.IsAlreadyExists(err): + return http.StatusConflict + case containerderrors.IsFailedPrecondition(err): + return http.StatusPreconditionFailed + case containerderrors.IsUnavailable(err): + return http.StatusServiceUnavailable + case containerderrors.IsNotImplemented(err): + return http.StatusNotImplemented + default: + return http.StatusInternalServerError + } +} From 8ab5e2a00433944b1e33b25dc25f4112ab401572 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 15 Jul 2019 18:19:31 +0200 Subject: [PATCH 2/2] Add regression tests for invalid platform status codes 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 (cherry picked from commit 9d1b4f5fc3fd647ac4f258374ee5369ffab85c0d) Signed-off-by: Sebastiaan van Stijn --- errdefs/http_helpers.go | 2 +- integration/build/build_test.go | 30 ++++++++++++++++++++++++++++++ integration/image/pull_test.go | 24 ++++++++++++++++++++++++ 3 files changed, 55 insertions(+), 1 deletion(-) create mode 100644 integration/image/pull_test.go diff --git a/errdefs/http_helpers.go b/errdefs/http_helpers.go index c67a65b6ae26b..1debd2ae004b7 100644 --- a/errdefs/http_helpers.go +++ b/errdefs/http_helpers.go @@ -177,7 +177,7 @@ func statusCodeFromDistributionError(err error) int { } // statusCodeFromContainerdError returns status code for containerd errors when -// consumed directory (not through gRPC) +// consumed directly (not through gRPC) func statusCodeFromContainerdError(err error) int { switch { case containerderrors.IsInvalidArgument(err): diff --git a/integration/build/build_test.go b/integration/build/build_test.go index b48921d1a4931..e5f17fe4ad8fd 100644 --- a/integration/build/build_test.go +++ b/integration/build/build_test.go @@ -13,6 +13,7 @@ import ( "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/filters" "github.com/docker/docker/api/types/versions" + "github.com/docker/docker/errdefs" "github.com/docker/docker/internal/test/fakecontext" "github.com/docker/docker/pkg/jsonmessage" "gotest.tools/assert" @@ -614,6 +615,35 @@ func TestBuildPreserveOwnership(t *testing.T) { } } +func TestBuildPlatformInvalid(t *testing.T) { + skip.If(t, versions.LessThan(testEnv.DaemonAPIVersion(), "1.40"), "experimental in older versions") + + ctx := context.Background() + defer setupTest(t)() + + dockerfile := `FROM busybox +` + + buf := bytes.NewBuffer(nil) + w := tar.NewWriter(buf) + writeTarRecord(t, w, "Dockerfile", dockerfile) + err := w.Close() + assert.NilError(t, err) + + apiclient := testEnv.APIClient() + _, err = apiclient.ImageBuild(ctx, + buf, + types.ImageBuildOptions{ + Remove: true, + ForceRemove: true, + Platform: "foobar", + }) + + assert.Assert(t, err != nil) + assert.ErrorContains(t, err, "unknown operating system or architecture") + assert.Assert(t, errdefs.IsInvalidParameter(err)) +} + func writeTarRecord(t *testing.T, w *tar.Writer, fn, contents string) { err := w.WriteHeader(&tar.Header{ Name: fn, diff --git a/integration/image/pull_test.go b/integration/image/pull_test.go new file mode 100644 index 0000000000000..f05dc64672ec5 --- /dev/null +++ b/integration/image/pull_test.go @@ -0,0 +1,24 @@ +package image + +import ( + "context" + "testing" + + "github.com/docker/docker/api/types" + "github.com/docker/docker/api/types/versions" + "github.com/docker/docker/errdefs" + "gotest.tools/assert" + "gotest.tools/skip" +) + +func TestImagePullPlatformInvalid(t *testing.T) { + skip.If(t, versions.LessThan(testEnv.DaemonAPIVersion(), "1.40"), "experimental in older versions") + defer setupTest(t)() + client := testEnv.APIClient() + ctx := context.Background() + + _, err := client.ImagePull(ctx, "docker.io/library/hello-world:latest", types.ImagePullOptions{Platform: "foobar"}) + assert.Assert(t, err != nil) + assert.ErrorContains(t, err, "unknown operating system or architecture") + assert.Assert(t, errdefs.IsInvalidParameter(err)) +}