New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix returned error code for network creation from 500 to 409 #35030

Merged
merged 1 commit into from Oct 25, 2017

Conversation

@tossmilestone
Contributor

tossmilestone commented Sep 29, 2017

Signed-off-by: He Xiaoxi tossmilestone@gmail.com

- What I did

Fixes #34459

- How I did it

Add a conflictError error type, and return the error when create a network with a conflict network name.

- How to verify it

In python console, run:

>>> import docker
>>> c = docker.from_env()
>>> c.networks.create('net1')
<Network: 4c7d2d08e3>
>>> c.networks.create('net1', check_duplicate=True)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python2.7/site-packages/docker/models/networks.py", line 144, in create
    resp = self.client.api.create_network(name, *args, **kwargs)
  File "/usr/lib/python2.7/site-packages/docker/utils/decorators.py", line 35, in wrapper
    return f(self, *args, **kwargs)
  File "/usr/lib/python2.7/site-packages/docker/api/network.py", line 134, in create_network
    return self._result(res, json=True)
  File "/usr/lib/python2.7/site-packages/docker/api/client.py", line 220, in _result
    self._raise_for_status(response)
  File "/usr/lib/python2.7/site-packages/docker/api/client.py", line 216, in _raise_for_status
    raise create_api_error_from_http_exception(e)
  File "/usr/lib/python2.7/site-packages/docker/errors.py", line 30, in create_api_error_from_http_exception
    raise cls(e, response=response, explanation=explanation)
docker.errors.APIError: 409 Client Error: Conflict for url: http+docker://localunixsocket/v1.24/networks/create ("network with name net1 already exists")
>>> 

- Description for the changelog

Return 409 instead of 500 if create a network with an existed name.

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah

code LGTM, and looks to work as expected;

$ docker network create foo
91e999cc2de4a8be51b55afe23389d791af95561626e24f0315d2838c4ebb63a

$  curl --unix-socket /var/run/docker.sock -v -XPOST -H"Content-Type: application/json" -d'{"Attachable":false,"CheckDuplicate":true,"ConfigFrom":null,"ConfigOnly":false,"Driver":"bridge","EnableIPv6":false,"IPAM":{"Config":[],"Driver":"default","Options":{}},"Ingress":false,"Internal":false,"Labels":{},"Name":"foo","Options":{},"Scope":""}' http://localhost/networks/create
Note: Unnecessary use of -X or --request, POST is already inferred.
*   Trying /var/run/docker.sock...
* Connected to localhost (/var/run/docker.sock) port 80 (#0)
> POST /networks/create HTTP/1.1
> Host: localhost
> User-Agent: curl/7.52.1
> Accept: */*
> Content-Type: application/json
> Content-Length: 251
>
* upload completely sent off: 251 out of 251 bytes
< HTTP/1.1 409 Conflict
< Api-Version: 1.33
< Content-Type: application/json
< Docker-Experimental: false
< Ostype: linux
< Server: Docker/17.06.0-dev (linux)
< Date: Fri, 29 Sep 2017 15:19:39 GMT
< Content-Length: 51
<
{"message":"network with name foo already exists"}
* Curl_http_done: called premature == 0
* Connection #0 to host localhost left intact
@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Sep 29, 2017

Contributor

Maybe provide a helper function like:

func nameConflict(name string) error {
    conflictError{libnetwork.NetworkNameError(name)}
}
Contributor

cpuguy83 commented Sep 29, 2017

Maybe provide a helper function like:

func nameConflict(name string) error {
    conflictError{libnetwork.NetworkNameError(name)}
}
@tossmilestone

This comment has been minimized.

Show comment
Hide comment
@tossmilestone

tossmilestone Sep 30, 2017

Contributor

@cpuguy83 updated, PTAL.

Contributor

tossmilestone commented Sep 30, 2017

@cpuguy83 updated, PTAL.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Sep 30, 2017

Contributor

Can you add an API test checking the error code?

Contributor

cpuguy83 commented Sep 30, 2017

Can you add an API test checking the error code?

@tossmilestone tossmilestone requested review from dnephin and vdemeester as code owners Oct 8, 2017

@tossmilestone

This comment has been minimized.

Show comment
Hide comment
@tossmilestone

tossmilestone Oct 8, 2017

Contributor

@cpuguy83 Added the api test for the error code, PTAL.

Contributor

tossmilestone commented Oct 8, 2017

@cpuguy83 Added the api test for the error code, PTAL.

Fix returned error code for network creation from 500 to 409
Signed-off-by: He Xiaoxi <tossmilestone@gmail.com>
@boaz1337

Maybe it's a good time to move those tests to integration/network/? @dnephin @vdemeester WDYT?

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Oct 10, 2017

Member

I would be, but it's not necessary either

Member

dnephin commented Oct 10, 2017

I would be, but it's not necessary either

@ehazlett

This comment has been minimized.

Show comment
Hide comment
@ehazlett

ehazlett Oct 25, 2017

Contributor

LGTM

ping @cpuguy83

Contributor

ehazlett commented Oct 25, 2017

LGTM

ping @cpuguy83

@thaJeztah

still LGTM

@thaJeztah thaJeztah merged commit e309f98 into moby:master Oct 25, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 37275 has succeeded
Details
janky Jenkins build Docker-PRs 45955 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 6342 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 17520 has succeeded
Details
z Jenkins build Docker-PRs-s390x 6141 has succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment