API return network-list if no network-name or id is provided by thaJeztah · Pull Request #24634 · moby/moby

thaJeztah · 2016-07-14T13:18:07Z

When calling the /networks/ endpoint (with a trailing slash), the default network was returned.

This changes the endpoint to return the list of networks instead.

Also updated the description for GetNetworkByName to
explain that the "default" network is returned if
no name or id is provided.

closes #24595

thaJeztah · 2016-07-14T13:22:05Z

Note that I also tried a different approach, and changing the route; https://github.com/docker/docker/blob/v1.12.0-rc4/api/server/router/network/network.go#L33-L34

diff --git a/api/server/router/network/network.go b/api/server/router/network/network.go
index 8688c3e..45548b5 100644
--- a/api/server/router/network/network.go
+++ b/api/server/router/network/network.go
@@ -31,7 +31,7 @@ func (r *networkRouter) initRoutes() {
        r.routes = []router.Route{
                // GET
                router.NewGetRoute("/networks", r.getNetworksList),
-               router.NewGetRoute("/networks/{id:.*}", r.getNetwork),
+               router.NewGetRoute("/networks/{id:.+}", r.getNetwork),
                // POST
                router.NewPostRoute("/networks/create", r.postNetworkCreate),
                router.NewPostRoute("/networks/{id:.*}/connect", r.postNetworkConnect),

This gives a different result (a 404 error with a "page not found"); perhaps doing that would break more clients that look for the return message, so I didn't use that;

/ # curl -sv --unix-socket /var/run/docker.sock http:/networks/
*   Trying /var/run/docker.sock...
* Connected to http (/var/run/docker.sock) port 80 (#0)
> GET /networks/ HTTP/1.1
> Host: http
> User-Agent: curl/7.49.1
> Accept: */*
>
< HTTP/1.1 404 Not Found
< Content-Type: application/json
< Date: Thu, 14 Jul 2016 12:38:29 GMT
< Content-Length: 29
<
{"message":"page not found"}
* Connection #0 to host http left intact

So I'm open to suggestions what the better approach is 😄

ping @mgoelzer @aboch

thaJeztah · 2016-07-14T13:23:57Z

api/server/router/network/network_routes.go

we could use

libnetwork.ErrNoSuchNetwork("(empty string)")

to be more explicit

I feel like this should be an impossibility.

See the repro case at the top; probably not possible through the CLI, but it's possible to get there when directly using the API

Changing to {id} instead of {id:.*} seems to do the trick.

@cpuguy83 yes, see above (#24634 (comment)), but that would change the response from the current error (if an invalid name is provided); I wasn't sure if we wanted to risk that. Also see #24634 (comment)

We can just add an explicit route for the trailing slash:

router.NewGetRoute("/networks/", r.getNetworksList),

Displaying the bridge inspect for /networks/ is a bug.

justincormack · 2016-07-14T14:48:16Z

cc @bfirsh who obsesses details of API behaviour...

bfirsh · 2016-07-14T16:44:53Z

👍

Changing the route to .+ seems neater but for consistency with other routes your approach seems best.

Are POST /networks/id/connect, POST /networks/id/disconnect, DELETE /networks/id affected by this too?

thaJeztah · 2016-07-14T16:52:14Z

Are POST /networks/id/connect, /networks/id/disconnect, DELETE /networks/id affected by this too?

@bfirsh I'll check, but looking at those I think we can change those to ../{id:.+}/disconnect, as I don't think supporting /network//disconnect would make a lot of sense (having a standard 404 for those should be okay). I'll check those

LK4D4 · 2016-08-05T16:04:18Z

@thaJeztah what's up here?

thaJeztah · 2016-08-05T16:07:06Z

I think we can merge, but perhaps @cpuguy83 wants the other approach

vdemeester · 2016-08-25T16:19:10Z

I do agree with @cpuguy83 on that one

We can just add an explicit route for the trailing slash:
router.NewGetRoute("/networks/", r.getNetworksList),
Displaying the bridge inspect for /networks/ is a bug.

thaJeztah · 2016-09-12T14:25:21Z

Updated the PR to make /networks and /networks/ return the same results

thaJeztah · 2016-09-16T23:02:50Z

ping @cpuguy83 @vdemeester PTAL

vdemeester

LGTM 🐸

thaJeztah · 2016-09-26T16:42:15Z

rebased. ping @cpuguy83 PTAL

cpuguy83 · 2016-09-26T16:44:40Z

LGTM

When calling the /networks/ endpoint with a trailing slash, the default network was returned. This changes the endpoint to return the list of networks instead (same response as `/networks` without trailing slash). Also updated the description for GetNetworkByName to explain that the "default" network is returned if no name or id is provided. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2016-09-26T19:45:00Z

rebased (again) 😅

thaJeztah · 2016-09-26T21:07:47Z

boo, flaky test?

20:08:46 ----------------------------------------------------------------------
20:08:46 FAIL: docker_cli_attach_unix_test.go:18: DockerSuite.TestAttachClosedOnContainerStop
20:08:46 
20:08:46 docker_cli_attach_unix_test.go:51:
20:08:46     c.Assert(err, check.IsNil)
20:08:46 ... value *exec.ExitError = &exec.ExitError{ProcessState:(*os.ProcessState)(0xc4204f20a0), Stderr:[]uint8(nil)} ("exit status 1")
20:08:46 
20:08:47

GordonTheTurtle added the status/0-triage label Jul 14, 2016

thaJeztah added status/1-design-review and removed status/0-triage labels Jul 14, 2016

thaJeztah reviewed Jul 14, 2016
View reviewed changes

justincormack added status/2-code-review and removed status/1-design-review labels Jul 15, 2016

thaJeztah force-pushed the make-network-name-required branch from 4cf2d66 to 6081c71 Compare September 12, 2016 14:23

thaJeztah changed the title ~~API return error if no network-name or id is provided~~ API return network-list if no network-name or id is provided Sep 12, 2016

thaJeztah force-pushed the make-network-name-required branch from 6081c71 to f90f749 Compare September 16, 2016 22:54

vdemeester approved these changes Sep 17, 2016

View reviewed changes

thaJeztah force-pushed the make-network-name-required branch from f90f749 to cff56e4 Compare September 26, 2016 16:41

cpuguy83 added status/4-merge and removed status/2-code-review labels Sep 26, 2016

thaJeztah added impact/api impact/changelog labels Sep 26, 2016

thaJeztah added this to the 1.13.0 milestone Sep 26, 2016

thaJeztah force-pushed the make-network-name-required branch from cff56e4 to 6ad4bf0 Compare September 26, 2016 19:44

vdemeester merged commit b826beb into moby:master Sep 27, 2016

thaJeztah deleted the make-network-name-required branch September 27, 2016 08:39

thaJeztah mentioned this pull request Oct 26, 2016

change network route url #27725

Closed

thaJeztah mentioned this pull request Nov 8, 2016

Secret Management #27794

Merged

4 tasks

thaJeztah mentioned this pull request Feb 6, 2018

Produce errors when empty ids are passed into inspect calls. #36144

Merged

This was referenced Nov 30, 2020

Add feature: remove spare network url #41728

Closed

Networking: add regression test for networks endpoint with trailing slash #41729

Merged

Conversation

thaJeztah commented Jul 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Jul 14, 2016

Uh oh!

thaJeztah Jul 14, 2016

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Jul 19, 2016

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jul 19, 2016

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Jul 19, 2016

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jul 19, 2016

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Jul 19, 2016

Choose a reason for hiding this comment

Uh oh!

justincormack commented Jul 14, 2016

Uh oh!

bfirsh commented Jul 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Jul 14, 2016

Uh oh!

LK4D4 commented Aug 5, 2016

Uh oh!

thaJeztah commented Aug 5, 2016

Uh oh!

vdemeester commented Aug 25, 2016

Uh oh!

thaJeztah commented Sep 12, 2016

Uh oh!

thaJeztah commented Sep 16, 2016

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Sep 26, 2016

Uh oh!

cpuguy83 commented Sep 26, 2016

Uh oh!

thaJeztah commented Sep 26, 2016

Uh oh!

thaJeztah commented Sep 26, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

thaJeztah commented Jul 14, 2016 •

edited

Loading

bfirsh commented Jul 14, 2016 •

edited

Loading