Update `FindNetwork` to address network name duplications by yongtang · Pull Request #30897 · moby/moby

yongtang · 2017-02-10T04:36:18Z

- What I did
This fix is part of the effort to address #30242 (comment) where issue arise when creating services because of the fact that multiple networks may share the same name (within or across local/swarm scopes).

- How I did it

The focus of this fix is to allow creation of service when a network in local scope has the same name as the service network.

- How to verify it

An integration test has been added.

- Description for the changelog

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

This fix is related to #30242.

Signed-off-by: Yong Tang yong.tang.github@outlook.com

yongtang · 2017-02-10T04:36:59Z

api/server/router/network/backend.go

Change to networkID to enforcing the usage of ID instead of Name/ID/Prefix

yongtang · 2017-02-10T04:37:56Z

api/server/router/network/network_routes.go

It is a lot easier to enforcing non-ambiguity here.

yongtang · 2017-02-10T04:38:26Z

api/server/router/network/network_routes.go

Non-ambiguity at the api level.

yongtang · 2017-02-10T04:38:54Z

api/server/router/network/network_routes.go

Pass nw.ID() for connect.

yongtang · 2017-02-10T04:39:07Z

api/server/router/network/network_routes.go

Pass nw.ID() for disconnect.

yongtang · 2017-02-10T04:39:44Z

api/server/router/network/network_routes.go

Delete might happen in both local and scoped networks.

yongtang · 2017-02-10T04:40:28Z

api/server/router/network/network_routes.go

This findNetwork is different from daemon.FindNetwork as it will be cross scopes (both local and swarm).

Could you make this comment a docstring (above the function) ? Right now it looks like it's documenting the variables inside the function.

Also what do you think about findUniqueNetwork() as a name?

Thanks @dnephin. The comment has been updated to doc string. The name has also been changed.

yongtang · 2017-02-10T04:41:27Z

daemon/cluster/executor/container/container.go

The networks() is returning the name only, which should not be encouraged (should always use ID if possible).

yongtang · 2017-02-10T04:42:12Z

daemon/container_operations.go

Alway try to use NetworkID whenever NetworkID is available.

This name is a bit odd (from just the name, I would expect it to always return an ID), not a big thing because it's not exported, but perhaps we should find a better name for it

yongtang · 2017-02-10T04:43:04Z

daemon/network.go

In case of swarm networks, we have to allow creation even if a local network with the same name already exist.

yongtang · 2017-02-10T17:06:29Z

The Jenkins failure seems real. Will spend time to investigate it.

yongtang · 2017-02-11T18:05:39Z

All tests passed now.

vdemeester · 2017-02-13T14:14:55Z

/cc @sanimej @mavenugo @aboch @aaronlehmann

dnephin

There seem to be a bunch of changes to DeleteNetwork.

Could you give more of an explanation of what this changes, and why it fixes the problem?

dnephin · 2017-03-09T22:40:24Z

integration-cli/docker_api_network_test.go

Should these be defer, and called immediately after the create for each?

Thanks. The PR has been updated.

yongtang · 2017-03-10T00:29:00Z

Thanks @dnephin for the review. The goal of this PR is to try to address the issue caused by name duplication in network names.

Previously, the network name is unique so some of the implementation uses will look up the network name (instead of network ID) when needed.

However, later on the flag checkDuplicate was introduced and it is possible now to have multiple networks with the same name (the IDs are still unique) if checkDuplicate is false.

Adding swarm network in, the same network name could appears in either local or swarm scope. And even in the same scope (like swarm scope), the same network name could be referring to different network IDs.

However, in the existing implementation, many places uses FindNetwork to lookup a network based on the network name (unfortunately).

That causes issues:

When deleting a network with a name, it may not always delete the same network.
In swarm network, deploy a service may fail (see daemon allows creating two network with same name when checkDuplicate is false, REASONABLE? #30242 (comment)) because the network ID passed at the beginning may be referring to another network ID when service deployment is actually executed on an individual node. As a result, service deployment is always attempting because it incorrectly believe the requested network is not available yet.

This PR tries to address the issue mentioned by always avoid FindNetwork and use network ID for look up if possible (so that the network referred is unique and consistent).

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

cpuguy83 · 2018-01-10T21:09:58Z

daemon/network.go

+	case len(listByFullName) == 1:
+		return listByFullName[0], nil
+	case len(listByFullName) > 1:
+		return nil, fmt.Errorf("network %s is ambiguous (%d matches found based on name)", term, len(listByFullName))


We should not be using fmt.Errorf for these errors.

@cpuguy83 Sorry, that was probably due to the multiple rebase/conflict that was overlooked. Let me take a look and have a fix for it.

No worries. I've got it fixed in my errdefs or.

thaJeztah · 2018-01-11T10:51:21Z

api/server/router/network/backend.go

 // to provide network specific functionality.
 type Backend interface {
-	FindNetwork(idName string) (libnetwork.Network, error)
+	FindUniqueNetwork(idName string) (libnetwork.Network, error)


I wonder if we should've kept the old name here (FindNetwork); the issue being resolved here is a bugfix, so I don't see a real reason to rename the function (in general, I'd expect a function like this to return a unique network, so not sure a change in naming is really needed)

@yongtang @vdemeester wdyt?

GordonTheTurtle added the status/0-triage label Feb 10, 2017