api: allow NW name that is the prefix of a swarm NW ID #27938

AkihiroSuda · 2016-11-01T08:34:59Z

- What I did

Allows creating a network of which name is the prefix of the ID of a swarm network

Previously, it doesn't allow creating such a network:

e.g.

$ docker network inspect -f '{{.Id}}' ingress
84xh9knigj6zyt00u31e26nj3
$ docker network create 84
Error response from daemon: network with name 84 already exists

Fix #27866 (Flaky test: DockerSwarmSuite.TestPruneNetwork)

- How I did it
Please look at api/server/router/network/network_routes.go

- How to verify it

$ docker network inspect -f '{{.Id}}' ingress
84xh9knigj6zyt00u31e26nj3
$ docker network create 84
40a945ea84e643bd5234751846e72aadd08a3afab91fbcd4ab3b6e44d3fda8f3

- Description for the changelog

api: allow NW name that is the prefix of a swarm NW ID

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

Signed-off-by: Akihiro Suda suda.akihiro@lab.ntt.co.jp

cpuguy83 · 2016-11-01T14:32:27Z

ping @mrjana

aaronlehmann · 2016-11-01T16:53:42Z

api/server/router/network/network_routes.go

@@ -79,8 +80,16 @@ func (n *networkRouter) postNetworkCreate(ctx context.Context, w http.ResponseWr
 		return err
 	}

-	if _, err := n.clusterProvider.GetNetwork(create.Name); err == nil {
-		return libnetwork.NetworkNameError(create.Name)
+	if nw, err := n.clusterProvider.GetNetwork(create.Name); err == nil {


This check doesn't seem very useful. First, it's racy. Also, it does a prefix match, which is not what we want here.

n.clusterProvider.CreateNetwork will fail if the name conflicts. Could we just remove this check?

Makes sense, however, we need to check the conflict before calling backend.CreateNetwork(), so as to detect a conflict like this:

$ docker service create -d overlay foo $ docker service create -d bridge foo Error response from daemon: network with name foo already exists

A solution I can come up with is to call clusterProvider.CreateNetwork() before calling backend.CreateNetwork() for swarm-scoped networks, but I'm not sure how we can detect the scope here.

WDYT?
cc @mrjana @mavenugo @aboch

In my opinion we should not even try to fix this. We should just fix the test to use longer names.

I can understand your opinion. But even for production, people may try to create a network named "n1", "n2"..
It would be great if we can find a solution.

Agree. But I am afraid this cannot be guaranteed unless we serialize the network creation at API level. Which seems too much of a penalty.

I agree with @AkihiroSuda. It's ridiculous not to allow creating a network if its name is a prefix of another network's ID.

And using longer names in the test won't solve the problem, it will just make the failures less likely - so when they do happen, they will be very hard to reproduce and cause a lot of confusion.

This entire check needs to be coordinated by the swarm manager. The prefix/name/id game can be handled but doing so correctly is subtle. I've written out the approach in moby/swarmkit#1279 (comment) and a few other times.

isn't it the same as partial ID matches for containers; ID > full name match > partial ID match

isn't it the same as partial ID matches for containers; ID > full name match > partial ID match

I am not sure. Typically, there should be a resolution component and the getter should only take ID. In this case, I think we hit the docker API, so ambiguity of id/name may be a problem.

AkihiroSuda · 2016-11-30T06:50:53Z

Updated PR.
PTAL @aaronlehmann @aboch @stevvooe

This fix should be enough for the original issue #27866 .

However, this doesn't fix an issue about removing networks.

e.g.

$ docker network ls
NETWORK ID          NAME                DRIVER              SCOPE
0db1e31d181f        bridge              bridge              local
4f8a8c0144ad        docker_gwbridge     bridge              local
4aa7a6b845ef        host                host                local
kveaqnegpm66        ingress             overlay             swarm
82ba9477298d        kvea                bridge              swarm
7190e8d532c5        none                null                local
$ docker network rm kvea                                                                  
Error response from daemon: rpc error: code = 7 desc = ingress (kveaqnegpm66y75q9wj6urfwq) is a pre-defined network and cannot be removed

(docker network rm kvea should remove the network of which name iskvea, of which ID starts with 82ba9477298d)

For removal, we would need to implement the "set" approach which @stevvooe repeatedly mentioned (e.g. moby/swarmkit#1279 (comment), moby/swarmkit#1194 (comment)) in another PR.

thaJeztah · 2016-11-30T11:16:51Z

api/server/router/network/network_routes.go

@@ -80,7 +80,7 @@ func (n *networkRouter) postNetworkCreate(ctx context.Context, w http.ResponseWr
 		return err
 	}

-	if _, err := n.clusterProvider.GetNetwork(create.Name); err == nil {
+	if _, err := n.clusterProvider.GetNetworkByName(create.Name); err == nil {
 		return libnetwork.NetworkNameError(create.Name)


Not directly related to this PR, but wondering; should this check only be performed if create.CheckDuplicate is true? (see daemon/network.go#L256-L258)

Since allowing duplicate names is by design (#18864 (comment)), I wonder if it should error out.

(t.b.h. I would rather stop allowing duplicate names, because I think the name is used in many cases as the only reference)

I see this was added in #24158, so ping @mavenugo ^^

hmm, probably in another PR?

Yes, that makes sense, then we can confirm if it should be fixed / changed on that PR

mavenugo · 2016-11-30T16:47:47Z

@AkihiroSuda @thaJeztah I can't think of a good use-case for this to be a useful fix. Yes. this is an inconvenience for some cases, but I don't see a strong need to get this PR. IMO living with this restriction is better than trying to fix it and cause more head-aches.

aaronlehmann · 2016-11-30T17:45:21Z

@mavenugo: What downside do you see? The old code was querying for a duplicate network in a way that would do prefix matching on IDs, which is clearly wrong. This was causing random integration test failures (sometimes "n1" would be a prefix of a network with ID "n1..."). I know there are still some remaining issues, but would like to understand what headaches it may cause to correct this check.

mavenugo · 2016-11-30T18:10:49Z

@aaronlehmann the existing UX accepts name, id and partial-id in many places. And this will cause confusion on those areas. and that is the reason @AkihiroSuda mentioned this : However, this doesn't fix an issue about removing networks. and I expect the same problem in other areas such as inspect.
am trying to understand the use-case for such a requirement. Especially because names are user-provided and can be anything. Why would someone need a overlapping name with an existing id ?

aaronlehmann · 2016-11-30T18:19:04Z

One of the use cases is what I mentioned above. If you try to create a network named n1, and there is an existing network whose ID is the base36 string starting with n1..., it fails with the current code. This caused problems in an integration test. With the current code, it would be impossible to choose a name that is always okay, because any name can potentially overlap with an ID prefix.

I don't see this as adding a feature, but instead correcting incorrect validation. It's completely legitimate to have a name overlap with an ID prefix. We allow it for other kinds of objects. Networks are handled inconsistently and this is a bug.

There should be no issue with inspect. We always prefer exact name matches to ID prefix matches. This is what is done for all other objects I know of, such as images, containers, services, and so on. We need to make networks consistent with these other objects.

mavenugo · 2016-11-30T18:34:19Z

@aaronlehmann okay. if this is something unique to network then i agree we should fix it. Do you prefer fixing the IT issue alone in this PR and address the rest later in other PRs ?

Also, the simpler fix for the IT issue would be to change the TestPruneNetwork to use a longer randomized network name. And we do rely on the uniqueness of IDs (since the randomization really works and doesn't cause flaky tests for other tests :) )

aaronlehmann · 2016-11-30T18:39:06Z

okay. if this is something unique to network then i agree we should fix it. Do you prefer fixing the IT issue alone in this PR and address the rest later in other PRs ?

Yes, I think we should start by allowing names to overlap with ID prefixes, and in separate PRs we should fix the other issues.

Also, the simpler fix for the IT issue would be to change the TestPruneNetwork to use a longer randomized network name. And we do rely on the uniqueness of IDs (since the randomization really works and doesn't cause flaky tests for other tests :)

I agree that this would technically work and that we rely on ID uniqueness. My preference is to allow names to overlap with ID prefixes for consistency with other objects, as done in the latest revision of this PR.

tiborvass · 2016-12-01T19:51:45Z

daemon/cluster/cluster.go

 	if err != nil {
 		return apitypes.NetworkResource{}, err
 	}
 	return convert.BasicNetworkFromGRPC(*network), nil
 }

+// GetNetwork returns a cluster network by an ID.
+func (c *Cluster) GetNetwork(input string) (apitypes.NetworkResource, error) {


rename input to id to make it clearer

tiborvass · 2016-12-01T19:52:10Z

daemon/cluster/cluster.go

+}
+
+// GetNetworkByName returns a cluster network by name.
+func (c *Cluster) GetNetworkByName(input string) (apitypes.NetworkResource, error) {


rename input to name

AkihiroSuda · 2016-12-05T05:17:13Z

updated PR, about @tiborvass 's comments

AkihiroSuda · 2016-12-08T02:28:54Z

rebased

AkihiroSuda · 2016-12-09T07:33:07Z

@thaJeztah thank you a lot for testing!
I'll look into that..

update: updated PR and added two tests: TestSwarmNetworkCreateDup, TestSwarmNetworkCreateDup2
hope CI will be green..

aaronlehmann · 2016-12-10T01:29:16Z

daemon/cluster/cluster.go:1392:1: comment on exported method Cluster.GetNetworksByName should be of the form "GetNetworksByName ..."

AkihiroSuda · 2016-12-11T14:21:44Z

Now CI green

thaJeztah · 2016-12-13T00:12:05Z

integration-cli/docker_cli_swarm_test.go

+	c.Assert(err, checker.IsNil, check.Commentf("out: %v", out))
+}
+
+// Reversed case for TestSwarmNeetworkCreateDup


typo: TestSwarmNeetworkCreateDup

done (and rebased)

thaJeztah

LGTM

we should probably address #27938 (comment) though (in a separate PR)

thaJeztah · 2016-12-13T00:13:27Z

oh, and needs a rebase 😢

vdemeester

Two small comments (that could be seen as nits) but otherwise looks good

vdemeester · 2016-12-13T09:15:11Z

integration-cli/docker_cli_swarm_test.go

+	c.Assert(err, checker.IsNil, check.Commentf("out: %v", out))
+}
+
+// Test case for https://github.com/docker/docker/pull/27938#issuecomment-265768303


Would be cool if there could also be a description on what it does 👼 (linking the issue is 👍, but it would be even better with a small explanation so the reader doesn't have to necessary open his browser to know why it's there 👼)

Same above for TestSwarmNetworkCreateIssue27866

vdemeester · 2016-12-13T09:15:33Z

integration-cli/docker_cli_swarm_test.go

+}
+
+// Reversed case for TestSwarmNetworkCreateDup
+func (s *DockerSwarmSuite) TestSwarmNetworkCreateDup2(c *check.C) {


Pretty sure these 2 tests could be on the same one 👼

thaJeztah · 2016-12-15T12:35:27Z

ping @AkihiroSuda this needs a rebase, also, could you address @vdemeester's comments, then we should be ready to merge for 1.13.0-rc4

…f a swarm network Previously, it doesn't allow creating such a network: e.g. $ docker network inspect -f '{{.Id}}' ingress 84xh9knigj6zyt00u31e26nj3 $ docker network create 84 Error response from daemon: network with name 84 already exists Fix moby#27866 Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

AkihiroSuda · 2016-12-15T15:21:08Z

@thaJeztah @vdemeester updated PR, sorry for being late 😅

vdemeester

LGTM 🐸

vdemeester · 2016-12-15T15:43:02Z

integration-cli/docker_cli_swarm_test.go

+	c.Assert(err, checker.IsNil, check.Commentf("out: %v", out))
+}
+
+// Test case for https://github.com/docker/docker/pull/27938#issuecomment-265768303


thaJeztah · 2016-12-16T09:40:42Z

all green 💚

GordonTheTurtle added the status/0-triage label Nov 1, 2016

AkihiroSuda mentioned this pull request Nov 1, 2016

Flaky test: DockerSwarmSuite.TestPruneNetwork #27866

Closed

aaronlehmann reviewed Nov 1, 2016

View reviewed changes

thaJeztah added status/2-code-review and removed status/0-triage labels Nov 2, 2016

thaJeztah added this to the 1.13.0 milestone Nov 2, 2016

AkihiroSuda added status/1-design-review and removed status/2-code-review labels Nov 7, 2016

AkihiroSuda mentioned this pull request Nov 11, 2016

Add reference filter and deprecated filter param… #27872

Merged

3 tasks

AkihiroSuda mentioned this pull request Nov 25, 2016

cmd: fix name prefix matching moby/swarmkit#1279

Closed

AkihiroSuda force-pushed the ovlnw-27866 branch from 1e5398f to f771e06 Compare November 30, 2016 06:36

AkihiroSuda added the rebuild/windowsRS1 label Nov 30, 2016

GordonTheTurtle removed the rebuild/windowsRS1 label Nov 30, 2016

thaJeztah reviewed Nov 30, 2016

View reviewed changes

tiborvass reviewed Dec 1, 2016

View reviewed changes

AkihiroSuda force-pushed the ovlnw-27866 branch from f771e06 to 81d6c93 Compare December 5, 2016 05:16

AkihiroSuda force-pushed the ovlnw-27866 branch from 81d6c93 to 4cc6ff2 Compare December 8, 2016 02:28

AkihiroSuda force-pushed the ovlnw-27866 branch from 4cc6ff2 to 4ca15ef Compare December 8, 2016 11:20

thaJeztah added the status/2-code-review label Dec 8, 2016

AkihiroSuda changed the title ~~api: allow NW name that is the prefix of a swarm NW ID~~ [DNM] api: allow NW name that is the prefix of a swarm NW ID Dec 9, 2016

AkihiroSuda force-pushed the ovlnw-27866 branch 3 times, most recently from 38d0542 to 9a36f17 Compare December 9, 2016 08:38

AkihiroSuda changed the title ~~[DNM] api: allow NW name that is the prefix of a swarm NW ID~~ api: allow NW name that is the prefix of a swarm NW ID Dec 9, 2016

aaronlehmann added the status/failing-ci Indicates that the PR in its current state fails the test suite label Dec 10, 2016

AkihiroSuda force-pushed the ovlnw-27866 branch from 9a36f17 to 9c5ed6a Compare December 11, 2016 12:17

AkihiroSuda removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Dec 11, 2016

thaJeztah reviewed Dec 13, 2016

View reviewed changes

thaJeztah approved these changes Dec 13, 2016

View reviewed changes

AkihiroSuda force-pushed the ovlnw-27866 branch from 9c5ed6a to 3beb858 Compare December 13, 2016 03:48

vdemeester requested changes Dec 13, 2016

View reviewed changes

AkihiroSuda force-pushed the ovlnw-27866 branch from 3beb858 to 401804d Compare December 15, 2016 14:57

AkihiroSuda force-pushed the ovlnw-27866 branch from 401804d to edfbc3b Compare December 15, 2016 15:09

AkihiroSuda added the process/cherry-pick label Dec 15, 2016

vdemeester approved these changes Dec 15, 2016

View reviewed changes

thaJeztah added status/4-merge and removed status/2-code-review labels Dec 15, 2016

thaJeztah merged commit 92e6397 into moby:master Dec 16, 2016

vieux added process/cherry-picked and removed process/cherry-pick labels Dec 16, 2016

vieux mentioned this pull request Dec 16, 2016

1.13.0-rc4 cherry-picks: part2 #29465

Merged

api: allow NW name that is the prefix of a swarm NW ID #27938

api: allow NW name that is the prefix of a swarm NW ID #27938

Conversation

AkihiroSuda commented Nov 1, 2016

cpuguy83 commented Nov 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda commented Nov 30, 2016

thaJeztah Nov 30, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavenugo commented Nov 30, 2016

aaronlehmann commented Nov 30, 2016

mavenugo commented Nov 30, 2016

aaronlehmann commented Nov 30, 2016

mavenugo commented Nov 30, 2016

aaronlehmann commented Nov 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda commented Dec 5, 2016

AkihiroSuda commented Dec 8, 2016

AkihiroSuda commented Dec 9, 2016 • edited

aaronlehmann commented Dec 10, 2016

AkihiroSuda commented Dec 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah commented Dec 13, 2016

vdemeester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Dec 15, 2016

AkihiroSuda commented Dec 15, 2016

vdemeester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Dec 16, 2016

thaJeztah Nov 30, 2016 •

edited

AkihiroSuda commented Dec 9, 2016 •

edited