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.

Sign up for GitHub

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

Jump to bottom

Update `FindNetwork` to address network name duplications#30897

Merged

yongtang merged 2 commits into moby:master from yongtang:30242-network-duplicate-names

Jan 10, 2018

Member

yongtang commented Feb 10, 2017

- 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

GordonTheTurtle added the status/0-triage label

yongtang commented

View reviewed changes

api/server/router/network/backend.go

    
            @@ -15,6 +15,6 @@ type Backend interface {
          
              	CreateNetwork(nc types.NetworkCreateRequest) (*types.NetworkCreateResponse, error)

              	ConnectContainerToNetwork(containerName, networkName string, endpointConfig *network.EndpointSettings) error

              	DisconnectContainerFromNetwork(containerName string, networkName string, force bool) error

              	DeleteNetwork(name string) error

              	DeleteNetwork(networkID string) error

Member Author

yongtang Feb 10, 2017

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

yongtang commented

View reviewed changes

api/server/router/network/network_routes.go

    
            @@ -208,7 +139,12 @@ func (n *networkRouter) postNetworkConnect(ctx context.Context, w http.ResponseW
          
              		return err

              	}

              	return n.backend.ConnectContainerToNetwork(connect.Container, vars["id"], connect.EndpointConfig)

              	// Always make sure there is no ambiguity with respect to the network ID/name

Member Author

yongtang Feb 10, 2017

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

yongtang commented

View reviewed changes

api/server/router/network/network_routes.go Outdated

    
            @@ -225,22 +161,31 @@ func (n *networkRouter) postNetworkDisconnect(ctx context.Context, w http.Respon
          
              		return err

              	}

              	return n.backend.DisconnectContainerFromNetwork(disconnect.Container, vars["id"], disconnect.Force)

              	// Always make sure there is no ambiguity with respect to the network ID/name

              	nw, err := n.backend.FindNetwork(vars["id"])

Member Author

yongtang Feb 10, 2017

Non-ambiguity at the api level.

yongtang commented

View reviewed changes

api/server/router/network/network_routes.go Outdated

    
              	if err != nil {

              		return err

              	}

              	return n.backend.ConnectContainerToNetwork(connect.Container, nw.ID(), connect.EndpointConfig)

Member Author

yongtang Feb 10, 2017

Pass nw.ID() for connect.

yongtang commented

View reviewed changes

api/server/router/network/network_routes.go Outdated

    
              	if err != nil {

              		return err

              	}

              	return n.backend.DisconnectContainerFromNetwork(disconnect.Container, nw.ID(), disconnect.Force)

Member Author

yongtang Feb 10, 2017

Pass nw.ID() for disconnect.

yongtang commented

View reviewed changes

api/server/router/network/network_routes.go

    
              			return err

              		}

              	} else {

              		if err := n.backend.DeleteNetwork(nw.ID); err != nil {

Member Author

yongtang Feb 10, 2017

Delete might happen in both local and scoped networks.

yongtang commented

View reviewed changes

api/server/router/network/network_routes.go Outdated

    
            @@ -412,3 +357,82 @@ func (n *networkRouter) postNetworksPrune(ctx context.Context, w http.ResponseWr
          
              	}

              	return httputils.WriteJSON(w, http.StatusOK, pruneReport)

              }

              func (n *networkRouter) findNetwork(term string) (types.NetworkResource, error) {

              	// NOTE: This findNetwork is differnt from FindNetwork from the daemon.

Member Author

yongtang Feb 10, 2017

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

Member

dnephin Mar 13, 2017

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?

Member Author

yongtang Apr 4, 2017 •
edited

Loading

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

yongtang commented

View reviewed changes

daemon/cluster/executor/container/container.go

    
              // networks returns a list of network names attached to the container. The

              // returned name can be used to lookup the corresponding network create

              // options.

              func (c *containerConfig) networks() []string {

Member Author

yongtang Feb 10, 2017

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

yongtang commented

View reviewed changes

daemon/container_operations.go

    
            @@ -1053,3 +1050,10 @@ func (daemon *Daemon) DeactivateContainerServiceBinding(containerName string) er
          
              	}

              	return sb.DisableService()

              }

              func getNetworkID(name string, endpointSettings *networktypes.EndpointSettings) string {

Member Author

yongtang Feb 10, 2017

Alway try to use NetworkID whenever NetworkID is available.

Member

thaJeztah Jan 11, 2018

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 commented

View reviewed changes

daemon/network.go Outdated

    
            @@ -249,7 +267,9 @@ func (daemon *Daemon) createNetwork(create types.NetworkCreateRequest, id string
          
              	}

              	if nw != nil {

              		if create.CheckDuplicate {

              			return nil, libnetwork.NetworkNameError(create.Name)

              			if !agent || nw.Info().Dynamic() {

Member Author

yongtang Feb 10, 2017

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

yongtang mentioned this pull request

daemon allows creating two network with same name when checkDuplicate is false, REASONABLE? #30242

Closed

yongtang force-pushed the 30242-network-duplicate-names branch 2 times, most recently from 9c44301 to 7c4538f Compare

February 10, 2017 05:17

Member Author

yongtang commented Feb 10, 2017

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

yongtang changed the title ~~Update FindNetwork to address network name duplications~~ [WIP] Update FindNetwork to address network name duplications

yongtang force-pushed the 30242-network-duplicate-names branch 2 times, most recently from 5666f16 to a2bfaab Compare

February 11, 2017 03:24

yongtang changed the title ~~[WIP] Update FindNetwork to address network name duplications~~ Update FindNetwork to address network name duplications

Member Author

yongtang commented Feb 11, 2017

All tests passed now.

vdemeester added status/2-code-review and removed status/0-triage labels

vdemeester requested review from mavenugo, sanimej and aboch

February 13, 2017 14:14

Member

vdemeester commented Feb 13, 2017

/cc @sanimej @mavenugo @aboch @aaronlehmann

GordonTheTurtle assigned dnephin

dnephin reviewed

View reviewed changes

Member

dnephin left a comment

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?

integration-cli/docker_api_network_test.go Outdated

    
              	id2 := createNetwork(c, configNotCheck, true)

              	deleteNetwork(c, id1, true)

              	deleteNetwork(c, id2, true)

Member

dnephin Mar 9, 2017 •
edited

Loading

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

Member Author

yongtang Mar 10, 2017

Thanks. The PR has been updated.

yongtang force-pushed the 30242-network-duplicate-names branch from a2bfaab to 7295c58 Compare

March 10, 2017 00:15

Member Author

yongtang commented Mar 10, 2017 •
edited

Loading

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).

GordonTheTurtle removed the rebuild/powerpc label

yongtang added the rebuild/janky label

GordonTheTurtle removed the rebuild/janky label

yongtang added the rebuild/janky label

GordonTheTurtle removed the rebuild/janky label

yongtang added the rebuild/janky label

GordonTheTurtle removed the rebuild/janky label

yongtang added the rebuild/z label

GordonTheTurtle removed the rebuild/z label

yongtang added the rebuild/z label

GordonTheTurtle removed the rebuild/z label

yongtang added the rebuild/z label

GordonTheTurtle removed the rebuild/z label

yongtang added the rebuild/z label

GordonTheTurtle removed the rebuild/z label

yongtang added the rebuild/z label

GordonTheTurtle removed the rebuild/z label

yongtang merged commit 484612f into moby:master

yongtang deleted the 30242-network-duplicate-names branch

January 10, 2018 13:31

cpuguy83 reviewed

View reviewed changes

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))

Member

cpuguy83 Jan 10, 2018 •
edited

Loading

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

Member Author

yongtang Jan 11, 2018

@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.

Member

cpuguy83 Jan 11, 2018

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

thaJeztah reviewed

View reviewed changes

api/server/router/network/backend.go

@@ @@ -12,11 +12,11 @@ import ( @@
               // Backend is all the methods that need to be implemented
               // to provide network specific functionality.
               type Backend interface {
-              	FindNetwork(idName string) (libnetwork.Network, error)
+              	FindUniqueNetwork(idName string) (libnetwork.Network, error)

Member

thaJeztah Jan 11, 2018

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?

yongtang mentioned this pull request

Rename FindUniqueNetwork to FindNetwork #36021

Merged

thaJeztah added the impact/changelog label

selansen mentioned this pull request

Fix to address regression caused by PR 30897 #36316

Merged

thaJeztah mentioned this pull request

[18.02.0] Swarm service cannot attach to host network - "only one instance of "host" network is allowed" #36247

Closed

thaJeztah mentioned this pull request

Ambiguous network names #37479

Open

This was referenced Jul 25, 2018

Win 10 1607 LTSB - Cant start container with transparent network after dockerd restarts docker/for-win#2260

Closed

Win 10 1607 LTSB - Container cant find transparent network after dockerd restarts #37527

Closed

silvin-lubecki pushed a commit to silvin-lubecki/engine-extract that referenced this pull request


          Add scope filter in /networks/<id>

23e7f90

This fix tries to add a `scope` in the query of `/networks/<id>`
(`NetworkInspect`) so that in case of duplicate network names,
it is possible to locate the network ID based on the network
scope (`local`, 'swarm', or `global`).

Multiple networks might exist in different scopes, which is a legitimate case.
For example, a network name `foo` might exists locally and in swarm network.

However, before this PR it was not possible to query a network name `foo`
in a specific scope like swarm.

This fix fixes the issue by allowing a `scope` query in `/networks/<id>`.

Additional test cases have been added to unit tests and integration tests.

This fix is related to docker/cli#167, moby/moby#30897, moby/moby#33561, moby/moby#30242

This fix fixes docker/cli#167

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Upstream-commit: 158b2a1
Component: engine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

cpuguy83 cpuguy83 left review comments

thaJeztah thaJeztah left review comments

vdemeester vdemeester approved these changes

dnephin dnephin approved these changes

mavenugo Awaiting requested review from mavenugo

sanimej Awaiting requested review from sanimej

aboch Awaiting requested review from aboch

Labels

impact/changelog status/2-code-review