Skip to content

API: Add test for status code on conflicting service names#38142

Merged
cpuguy83 merged 1 commit intomoby:masterfrom
thaJeztah:fix_api_return_code
Dec 10, 2018
Merged

API: Add test for status code on conflicting service names#38142
cpuguy83 merged 1 commit intomoby:masterfrom
thaJeztah:fix_api_return_code

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Nov 5, 2018

fixes #38140

Before this change, attempting to create a service with a name that already exists produces a 500 error;

curl -v \
  --unix-socket /var/run/docker.sock \
  -X POST \
  "http://localhost/v1.30/services/create" \
  -H "Content-Type: application/json" \
  -d '{"EndpointSpec":{"Mode":"vip"},"Labels":{},"Mode":{"Replicated":{}},"Name":"testing","TaskTemplate":{"ContainerSpec":{"DNSConfig":{},"Image":"nginx:alpine@sha256:ae5da813f8ad7fa785d7668f0b018ecc8c3a87331527a61d83b3b5e816a0f03c","Init":false},"ForceUpdate":0,"Placement":{"Platforms":[{"Architecture":"amd64","OS":"linux"},{"OS":"linux"},{"Architecture":"arm64","OS":"linux"},{"Architecture":"386","OS":"linux"},{"Architecture":"ppc64le","OS":"linux"},{"Architecture":"s390x","OS":"linux"}]},"Resources":{"Limits":{},"Reservations":{}}}}'
> POST /v1.30/services/create HTTP/1.1
> Host: localhost
> User-Agent: curl/7.61.1
> Accept: */*
> Content-Type: application/json
> Content-Length: 536
> 
* upload completely sent off: 536 out of 536 bytes
< HTTP/1.1 500 Internal Server Error
< Api-Version: 1.38
< Content-Type: application/json
< Docker-Experimental: false
< Ostype: linux
< Server: Docker/18.06.1-ce (linux)
< Date: Mon, 05 Nov 2018 10:31:29 GMT
< Content-Length: 86
< 
{"message":"rpc error: code = Unknown desc = name conflicts with an existing object"}

After this change, a 409 (conflict) is returned:

curl -v \
  --unix-socket /var/run/docker.sock \
  -X POST \
  "http://localhost/v1.30/services/create" \
  -H "Content-Type: application/json" \
  -d '{"EndpointSpec":{"Mode":"vip"},"Labels":{},"Mode":{"Replicated":{}},"Name":"testing","TaskTemplate":{"ContainerSpec":{"DNSConfig":{},"Image":"nginx:alpine@sha256:ae5da813f8ad7fa785d7668f0b018ecc8c3a87331527a61d83b3b5e816a0f03c","Init":false},"ForceUpdate":0,"Placement":{"Platforms":[{"Architecture":"amd64","OS":"linux"},{"OS":"linux"},{"Architecture":"arm64","OS":"linux"},{"Architecture":"386","OS":"linux"},{"Architecture":"ppc64le","OS":"linux"},{"Architecture":"s390x","OS":"linux"}]},"Resources":{"Limits":{},"Reservations":{}}}}'
> POST /v1.30/services/create HTTP/1.1
> Host: localhost
> User-Agent: curl/7.52.1
> Accept: */*
> Content-Type: application/json
> Content-Length: 536
> 
* upload completely sent off: 536 out of 536 bytes
< HTTP/1.1 409 Conflict
< Api-Version: 1.40
< Content-Type: application/json
< Docker-Experimental: false
< Ostype: linux
< Server: Docker/dev (linux)
< Date: Mon, 05 Nov 2018 11:33:20 GMT
< Content-Length: 84
< 
{"message":"rpc error: code = AlreadyExists desc = service testing already exists"}

@thaJeztah thaJeztah requested a review from vdemeester as a code owner November 5, 2018 15:01
@thaJeztah thaJeztah force-pushed the fix_api_return_code branch 2 times, most recently from c127395 to 2f52293 Compare November 5, 2018 15:59
@codecov
Copy link

codecov bot commented Nov 5, 2018 •
edited
Loading

Codecov Report

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

@@            Coverage Diff            @@
##             master   #38142   +/-   ##
=========================================
  Coverage          ?   36.09%           
=========================================
  Files             ?      610           
  Lines             ?    45249           
  Branches          ?        0           
=========================================
  Hits              ?    16331           
  Misses            ?    26677           
  Partials          ?     2241

@thaJeztah thaJeztah force-pushed the fix_api_return_code branch from 2f52293 to f9e7cb4 Compare November 5, 2018 20:53
@thaJeztah thaJeztah mentioned this pull request Nov 5, 2018
Merged
@thaJeztah thaJeztah force-pushed the fix_api_return_code branch 2 times, most recently from 011cbfb to 5093078 Compare November 6, 2018 09:39
@thaJeztah
Copy link
Member Author

thaJeztah commented Nov 21, 2018

depends on moby/swarmkit#2779 to be vendored

@thaJeztah thaJeztah changed the title [WIP] API: fix service endpoint to return a 4xx status on conflicting service name API: Add test for status code on conflicting service names Nov 24, 2018
@thaJeztah
Copy link
Member Author

thaJeztah commented Nov 24, 2018

ok; swarmkit changes were merged and vendored, so this is no longer WIP

@vdemeester @cpuguy83 PTAL

vdemeester
vdemeester approved these changes Nov 26, 2018
View reviewed changes
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐯

cpuguy83
cpuguy83 reviewed Nov 26, 2018
View reviewed changes
@thaJeztah thaJeztah mentioned this pull request Dec 10, 2018
Merged
cpuguy83
cpuguy83 approved these changes Dec 10, 2018
View reviewed changes
Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

@cpuguy83 cpuguy83 merged commit 3e44f58 into moby:master Dec 10, 2018
@thaJeztah thaJeztah deleted the fix_api_return_code branch December 10, 2018 23:43
thaJeztah added a commit to thaJeztah/docker that referenced this pull request Feb 1, 2019
@thaJeztah
Bump swarmkit to 1a0ebd4
0e60e48 
Full diff: moby/swarmkit@8af8c42...1a0ebd4

relevant changes:

- swarmkit#2771 Allow using Configs as CredentialSpecs
- swarmkit#2804 Make Service.UpdateStatus non-ambiguous
- swarmkit#2805 Refactor condition in restart supervisor
- swarmkit#2780 api: add BindOptions.NonRecursive
  - related to moby#38003
- swarmkit#2790 Fix possible panic if NetworkConfig is nil
- swarmkit#2797 Include old error-message for backward compatibility
  - related to swarmkit#2779 / moby#38140 / moby#38142

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah mentioned this pull request Feb 1, 2019
Merged
adi-dhulipala pushed a commit to adi-dhulipala/docker that referenced this pull request Apr 11, 2019
@thaJeztah @adi-dhulipala
Bump swarmkit to 1a0ebd4
45f04e5 
Full diff: moby/swarmkit@8af8c42...1a0ebd4

relevant changes:

- swarmkit#2771 Allow using Configs as CredentialSpecs
- swarmkit#2804 Make Service.UpdateStatus non-ambiguous
- swarmkit#2805 Refactor condition in restart supervisor
- swarmkit#2780 api: add BindOptions.NonRecursive
  - related to moby#38003
- swarmkit#2790 Fix possible panic if NetworkConfig is nil
- swarmkit#2797 Include old error-message for backward compatibility
  - related to swarmkit#2779 / moby#38140 / moby#38142

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cpuguy83 cpuguy83 cpuguy83 approved these changes

+1 more reviewer

@vdemeester vdemeester vdemeester approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@vdemeester vdemeester

Labels

area/testing status/2-code-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Service create API returns 5xx status instead of 4xx

4 participants

@thaJeztah @vdemeester @cpuguy83 @GordonTheTurtle