Strip off node name network prefixes in endpoint configs in networking configs #2941

wsong · 2019-04-30T16:35:33Z

Previously, if you specified something like:

docker run --rm \
  --net=mynode/testnet
  --network-alias=foobar \
  alpine

Swarm classic would fail because it would try to pass an endpoint config to the
daemon with mynode/testnet as the network name

GordonTheTurtle · 2019-04-30T16:35:36Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "fix_endpoints_config2" [email protected]:wsong/swarm.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

dperny · 2019-04-30T16:45:20Z

cluster/config_test.go

+	config = BuildContainerConfig(container.Config{}, container.HostConfig{}, networkingConfig)
+	assert.Contains(t, config.NetworkingConfig.EndpointsConfig, "testnet1")
+	assert.Contains(t, config.NetworkingConfig.EndpointsConfig, "testnet2")
+	assert.Contains(t, config.NetworkingConfig.EndpointsConfig, "test/net3")


does assert.Contains not mean that the string "nodename/testnet2" would pass the assertion?

config.NetworkingConfig.EndpointsConfig is a map, so we're checking for whether or not this exact key is in the map.

dperny · 2019-05-03T16:29:05Z

OK yes that makes sense.

I feel like the CI should not be failing on every single version, though.

wsong · 2019-05-03T16:34:20Z

@dperny and I looked at the failures and it looks like most of the failures are actually from DinD failing to start. Not sure what's going on there but it seems clearly unrelated to this PR.

dperny · 2019-05-03T16:36:54Z

ok, that's good enough for me. i don't have write permissions, though.

thaJeztah · 2019-05-03T17:07:42Z

cluster/config.go

+	for networkName, v := range n.EndpointsConfig {
+		parts := strings.SplitN(networkName, "/", 2)
+		if len(parts) > 1 {
+			networkName = parts[1]


I think this will be a backward breaking change, as I can create custom networks having a /;

docker network create foo/bar/baz docker network ls NETWORK ID NAME DRIVER SCOPE 5ce5654a460e bridge bridge local 81d2f269d9ff docker_gwbridge bridge local 7296e31838b7 foo/bar/baz bridge local 26d3887604d2 host host local 59e5e41d3a7b none null local

Hmm, I guess we'll have to iterate through and see if parts[0] is the name of a node.

wsong · 2019-05-03T20:18:12Z

Okay, I refactored this so that we check the list of node names and make sure that the part we're stripping off is the name of a node. PTAL

thaJeztah

overall approach SGTM. left some thoughts/suggestions

thaJeztah · 2019-05-08T16:42:54Z

cluster/config.go

+// user specified a Swarm classic-style network name in the endpoints config
+// (i.e. <node name>/<network name>), strips off the node name so that the
+// daemon will recognize the network.
+func CleanupNetworkingConfig(n network.NetworkingConfig, nodeNames []string) network.NetworkingConfig {


Perhaps we should rename this to what it actually does, e.g. StripNodeNames (but also see my other comment w.r.t possibly making it a local helper function

Good idea, done.

thaJeztah · 2019-05-08T16:43:37Z

cluster/config_test.go

+func TestCleanupNetworkingConfig(t *testing.T) {
+	networkingConfig := network.NetworkingConfig{
+		EndpointsConfig: map[string]*network.EndpointSettings{
+			"testnet1":           &network.EndpointSettings{},


the &network.EndpointSettings is redundant here;

EndpointsConfig: map[string]*network.EndpointSettings{ "testnet1": {}, "nodename/testnet2": {}, "nodename/test/net3": {}, "foo/test/net4": {}, },

thaJeztah · 2019-05-08T16:46:28Z

cluster/config_test.go

+		EndpointsConfig: map[string]*network.EndpointSettings{
+			"testnet1":           &network.EndpointSettings{},
+			"nodename/testnet2":  &network.EndpointSettings{},
+			"nodename/test/net3": &network.EndpointSettings{},


Quite unlikely to be a problem, but might want to add a case where nodename is actually part of the network name, e.g. "nodename/nodename/net4" (which should produce nodename/net4)

thaJeztah · 2019-05-08T16:53:23Z

cluster/swarm/cluster.go


+// EngineNames returns the names of all the engines in the cluster.
+func (c *Cluster) EngineNames() []string {
+	ret := []string{}


perhaps

Suggested change

ret := []string{}

ret := make([]string, len(c.engines))

thaJeztah · 2019-05-08T17:04:47Z

api/handlers.go

 		httpError(w, err.Error(), http.StatusInternalServerError)
 		return
 	}
+	containerConfig.NetworkingConfig = cluster.CleanupNetworkingConfig(containerConfig.NetworkingConfig, c.cluster.EngineNames())


I was a bit on the fence if we should move this into cluster.CreateContainer(); that way CreateContainer would responsible for dealing with this (and we wouldn't have to modify the Interface to add EngineNames()).

I went looking for uses of CreateContainer() and the other location it's used is in rescheduleContainers, which copies the configuration of existing containers, so in that case the nodename would be stripped twice

We could consider making it a local utility and move it here, given that it's fixing something specific to this endpoint, but happy to get your thought on that

I think it makes sense to just strip the node names when the container configs first come into the API; as you point out, we should definitely not be stripping node names in rescheduleContainers. I'll move CleanupNetworkingConfig (aka StripNodeNames) into this file.

…g configs Previously, if you specified something like: ``` docker run --rm \ --net=mynode/testnet --network-alias=foobar \ alpine ``` Swarm classic would fail because it would try to pass an endpoint config to the daemon with `mynode/testnet` as the network name Signed-off-by: Wayne Song <[email protected]>

thaJeztah

LGTM, thanks!

thaJeztah · 2019-05-08T19:56:26Z

17.06 is green now; let's merge

GordonTheTurtle added the dco/no label Apr 30, 2019

wsong force-pushed the fix_endpoints_config2 branch from 2e5d2af to 05ad851 Compare April 30, 2019 16:35

GordonTheTurtle removed the dco/no label Apr 30, 2019

wsong force-pushed the fix_endpoints_config2 branch from 05ad851 to 8b1e4e7 Compare April 30, 2019 16:37

dperny reviewed Apr 30, 2019

View reviewed changes

thaJeztah reviewed May 3, 2019

View reviewed changes

wsong force-pushed the fix_endpoints_config2 branch from 8b1e4e7 to 379adde Compare May 3, 2019 20:17

wsong force-pushed the fix_endpoints_config2 branch 2 times, most recently from 718a49f to 7f090b7 Compare May 8, 2019 16:39

thaJeztah suggested changes May 8, 2019

View reviewed changes

wsong force-pushed the fix_endpoints_config2 branch 2 times, most recently from 27e59d3 to 84ac348 Compare May 8, 2019 17:26

wsong force-pushed the fix_endpoints_config2 branch from 84ac348 to 7c91dc7 Compare May 8, 2019 17:27

thaJeztah approved these changes May 8, 2019

View reviewed changes

thaJeztah merged commit 9392e47 into docker-archive:master May 8, 2019

thaJeztah mentioned this pull request Jun 25, 2019

Fix advanced options for backward compatibility docker/cli#1963

Merged

Strip off node name network prefixes in endpoint configs in networking configs #2941

Strip off node name network prefixes in endpoint configs in networking configs #2941

Uh oh!

Conversation

wsong commented Apr 30, 2019

Uh oh!

GordonTheTurtle commented Apr 30, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dperny commented May 3, 2019

Uh oh!

wsong commented May 3, 2019

Uh oh!

dperny commented May 3, 2019

Uh oh!

thaJeztah May 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wsong commented May 3, 2019

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented May 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thaJeztah May 3, 2019 •

edited

Loading