From f257f77c6c13ee851d3b899f8d51628a5dec92db Mon Sep 17 00:00:00 2001 From: allencloud Date: Thu, 23 Mar 2017 23:28:45 +0800 Subject: [PATCH] choose rpc code to determine status code Signed-off-by: allencloud --- Dockerfile | 2 +- Dockerfile.aarch64 | 2 +- Dockerfile.armhf | 2 +- Dockerfile.ppc64le | 2 +- Dockerfile.s390x | 2 +- api/server/httputils/errors.go | 40 +++++++++++++++++++ docs/api/version-history.md | 4 ++ .../docker_api_swarm_config_test.go | 2 +- .../docker_api_swarm_secret_test.go | 20 ++++++++-- integration-cli/docker_api_swarm_test.go | 2 +- 10 files changed, 68 insertions(+), 10 deletions(-) diff --git a/Dockerfile b/Dockerfile index ddcd3e9daa..a51b2f7a54 100644 --- a/Dockerfile +++ b/Dockerfile @@ -186,7 +186,7 @@ RUN set -x \ && rm -rf "$GOPATH" # Get the "docker-py" source so we can run their integration tests -ENV DOCKER_PY_COMMIT 4a08d04aef0595322e1b5ac7c52f28a931da85a5 +ENV DOCKER_PY_COMMIT dc2b24dcdd4ed7c8f6874ee5dd95716761ab6c93 # To run integration tests docker-pycreds is required. # Before running the integration tests conftest.py is # loaded which results in loads auth.py that diff --git a/Dockerfile.aarch64 b/Dockerfile.aarch64 index b96d473d5b..0560ae60c2 100644 --- a/Dockerfile.aarch64 +++ b/Dockerfile.aarch64 @@ -142,7 +142,7 @@ RUN set -x \ && rm -rf "$GOPATH" # Get the "docker-py" source so we can run their integration tests -ENV DOCKER_PY_COMMIT 4a08d04aef0595322e1b5ac7c52f28a931da85a5 +ENV DOCKER_PY_COMMIT dc2b24dcdd4ed7c8f6874ee5dd95716761ab6c93 # Before running the integration tests conftest.py is # loaded which results in loads auth.py that # imports the docker-pycreds module. diff --git a/Dockerfile.armhf b/Dockerfile.armhf index fe190a3fd0..59f673197d 100644 --- a/Dockerfile.armhf +++ b/Dockerfile.armhf @@ -137,7 +137,7 @@ RUN set -x \ && rm -rf "$GOPATH" # Get the "docker-py" source so we can run their integration tests -ENV DOCKER_PY_COMMIT e2655f658408f9ad1f62abdef3eb6ed43c0cf324 +ENV DOCKER_PY_COMMIT dc2b24dcdd4ed7c8f6874ee5dd95716761ab6c93 RUN git clone https://github.com/docker/docker-py.git /docker-py \ && cd /docker-py \ && git checkout -q $DOCKER_PY_COMMIT \ diff --git a/Dockerfile.ppc64le b/Dockerfile.ppc64le index d61205573a..9bbdacc003 100644 --- a/Dockerfile.ppc64le +++ b/Dockerfile.ppc64le @@ -143,7 +143,7 @@ RUN set -x \ && rm -rf "$GOPATH" # Get the "docker-py" source so we can run their integration tests -ENV DOCKER_PY_COMMIT e2655f658408f9ad1f62abdef3eb6ed43c0cf324 +ENV DOCKER_PY_COMMIT dc2b24dcdd4ed7c8f6874ee5dd95716761ab6c93 RUN git clone https://github.com/docker/docker-py.git /docker-py \ && cd /docker-py \ && git checkout -q $DOCKER_PY_COMMIT \ diff --git a/Dockerfile.s390x b/Dockerfile.s390x index c568480ecd..2c3385aff9 100644 --- a/Dockerfile.s390x +++ b/Dockerfile.s390x @@ -136,7 +136,7 @@ RUN set -x \ && rm -rf "$GOPATH" # Get the "docker-py" source so we can run their integration tests -ENV DOCKER_PY_COMMIT e2655f658408f9ad1f62abdef3eb6ed43c0cf324 +ENV DOCKER_PY_COMMIT dc2b24dcdd4ed7c8f6874ee5dd95716761ab6c93 RUN git clone https://github.com/docker/docker-py.git /docker-py \ && cd /docker-py \ && git checkout -q $DOCKER_PY_COMMIT \ diff --git a/api/server/httputils/errors.go b/api/server/httputils/errors.go index 40ff351495..6b1c50411e 100644 --- a/api/server/httputils/errors.go +++ b/api/server/httputils/errors.go @@ -9,6 +9,7 @@ import ( "github.com/docker/docker/api/types/versions" "github.com/gorilla/mux" "google.golang.org/grpc" + "google.golang.org/grpc/codes" ) // httpStatusError is an interface @@ -44,11 +45,17 @@ func GetHTTPErrorStatusCode(err error) int { case inputValidationError: statusCode = http.StatusBadRequest default: + statusCode = statusCodeFromGRPCError(err) + if statusCode != http.StatusInternalServerError { + return statusCode + } + // FIXME: this is brittle and should not be necessary, but we still need to identify if // there are errors falling back into this logic. // If we need to differentiate between different possible error types, // we should create appropriate error types that implement the httpStatusError interface. errStr := strings.ToLower(errMsg) + for _, status := range []struct { keyword string code int @@ -102,3 +109,36 @@ func MakeErrorHandler(err error) http.HandlerFunc { } } } + +// statusCodeFromGRPCError returns status code according to gRPC error +func statusCodeFromGRPCError(err error) int { + switch grpc.Code(err) { + case codes.InvalidArgument: // code 3 + return http.StatusBadRequest + case codes.NotFound: // code 5 + return http.StatusNotFound + case codes.AlreadyExists: // code 6 + return http.StatusConflict + case codes.PermissionDenied: // code 7 + return http.StatusForbidden + case codes.FailedPrecondition: // code 9 + return http.StatusBadRequest + case codes.Unauthenticated: // code 16 + return http.StatusUnauthorized + case codes.OutOfRange: // code 11 + return http.StatusBadRequest + case codes.Unimplemented: // code 12 + return http.StatusNotImplemented + case codes.Unavailable: // code 14 + return http.StatusServiceUnavailable + default: + // codes.Canceled(1) + // codes.Unknown(2) + // codes.DeadlineExceeded(4) + // codes.ResourceExhausted(8) + // codes.Aborted(10) + // codes.Internal(13) + // codes.DataLoss(15) + return http.StatusInternalServerError + } +} diff --git a/docs/api/version-history.md b/docs/api/version-history.md index 3891d35c04..d909a92c18 100644 --- a/docs/api/version-history.md +++ b/docs/api/version-history.md @@ -29,6 +29,10 @@ keywords: "API, Docker, rcli, REST, documentation" generate and rotate to a new CA certificate/key pair. * `POST /service/create` and `POST /services/(id or name)/update` now take the field `Platforms` as part of the service `Placement`, allowing to specify platforms supported by the service. * `POST /containers/(name)/wait` now accepts a `condition` query parameter to indicate which state change condition to wait for. Also, response headers are now returned immediately to acknowledge that the server has registered a wait callback for the client. +* `DELETE /secrets/(name)` now returns status code 404 instead of 500 when the secret does not exist. +* `POST /secrets/create` now returns status code 409 instead of 500 when creating an already existing secret. +* `POST /secrets/(name)/update` now returns status code 400 instead of 500 when updating a secret's content which is not the labels. +* `POST /nodes/(name)/update` now returns status code 400 instead of 500 when demoting last node fails. ## v1.29 API changes diff --git a/integration-cli/docker_api_swarm_config_test.go b/integration-cli/docker_api_swarm_config_test.go index 5d80ad0661..fab65ccbd0 100644 --- a/integration-cli/docker_api_swarm_config_test.go +++ b/integration-cli/docker_api_swarm_config_test.go @@ -114,5 +114,5 @@ func (s *DockerSwarmSuite) TestAPISwarmConfigsUpdate(c *check.C) { status, out, err := d.SockRequest("POST", url, config.Spec) c.Assert(err, checker.IsNil, check.Commentf(string(out))) - c.Assert(status, checker.Equals, http.StatusInternalServerError, check.Commentf("output: %q", string(out))) + c.Assert(status, checker.Equals, http.StatusBadRequest, check.Commentf("output: %q", string(out))) } diff --git a/integration-cli/docker_api_swarm_secret_test.go b/integration-cli/docker_api_swarm_secret_test.go index 64a1fc8d87..cb82af8e2e 100644 --- a/integration-cli/docker_api_swarm_secret_test.go +++ b/integration-cli/docker_api_swarm_secret_test.go @@ -23,18 +23,25 @@ func (s *DockerSwarmSuite) TestAPISwarmSecretsCreate(c *check.C) { d := s.AddDaemon(c, true, true) testName := "test_secret" - id := d.CreateSecret(c, swarm.SecretSpec{ + secretSpec := swarm.SecretSpec{ Annotations: swarm.Annotations{ Name: testName, }, Data: []byte("TESTINGDATA"), - }) + } + + id := d.CreateSecret(c, secretSpec) c.Assert(id, checker.Not(checker.Equals), "", check.Commentf("secrets: %s", id)) secrets := d.ListSecrets(c) c.Assert(len(secrets), checker.Equals, 1, check.Commentf("secrets: %#v", secrets)) name := secrets[0].Spec.Annotations.Name c.Assert(name, checker.Equals, testName, check.Commentf("secret: %s", name)) + + // create an already existing secret, daemon should return a status code of 409 + status, out, err := d.SockRequest("POST", "/secrets/create", secretSpec) + c.Assert(err, checker.IsNil) + c.Assert(status, checker.Equals, http.StatusConflict, check.Commentf("secret create: %s", string(out))) } func (s *DockerSwarmSuite) TestAPISwarmSecretsDelete(c *check.C) { @@ -55,6 +62,13 @@ func (s *DockerSwarmSuite) TestAPISwarmSecretsDelete(c *check.C) { status, out, err := d.SockRequest("GET", "/secrets/"+id, nil) c.Assert(err, checker.IsNil) c.Assert(status, checker.Equals, http.StatusNotFound, check.Commentf("secret delete: %s", string(out))) + + // delete non-existing secret, daemon should return a status code of 404 + id = "non-existing" + status, out, err = d.SockRequest("DELETE", "/secrets/"+id, nil) + c.Assert(err, checker.IsNil) + c.Assert(status, checker.Equals, http.StatusNotFound, check.Commentf("secret delete: %s", string(out))) + } func (s *DockerSwarmSuite) TestAPISwarmSecretsUpdate(c *check.C) { @@ -114,5 +128,5 @@ func (s *DockerSwarmSuite) TestAPISwarmSecretsUpdate(c *check.C) { status, out, err := d.SockRequest("POST", url, secret.Spec) c.Assert(err, checker.IsNil, check.Commentf(string(out))) - c.Assert(status, checker.Equals, http.StatusInternalServerError, check.Commentf("output: %q", string(out))) + c.Assert(status, checker.Equals, http.StatusBadRequest, check.Commentf("output: %q", string(out))) } diff --git a/integration-cli/docker_api_swarm_test.go b/integration-cli/docker_api_swarm_test.go index 9f3f3e53ca..0e0f0e6f7a 100644 --- a/integration-cli/docker_api_swarm_test.go +++ b/integration-cli/docker_api_swarm_test.go @@ -229,7 +229,7 @@ func (s *DockerSwarmSuite) TestAPISwarmPromoteDemote(c *check.C) { url := fmt.Sprintf("/nodes/%s/update?version=%d", node.ID, node.Version.Index) status, out, err := d1.SockRequest("POST", url, node.Spec) c.Assert(err, checker.IsNil) - c.Assert(status, checker.Equals, http.StatusInternalServerError, check.Commentf("output: %q", string(out))) + c.Assert(status, checker.Equals, http.StatusBadRequest, check.Commentf("output: %q", string(out))) // The warning specific to demoting the last manager is best-effort and // won't appear until the Role field of the demoted manager has been // updated.