Fix --network-add adding duplicate networks #780

thaJeztah · 2018-01-03T19:36:03Z

When adding a network using docker service update --network-add,
the new network was added by name.

Existing entries in a service spec are listed by network ID, which
resulted in the CLI not detecting duplicate entries for the same
network.

This patch changes the behavior to always use the network-ID,
so that duplicate entries are correctly caught.

Before this change;

$ docker network create -d overlay foo
$ docker service create --name=test --network=foo nginx:alpine
$ docker service update --network-add foo test
$ docker service inspect --format '{{ json .Spec.TaskTemplate.Networks}}' test
[
  {
    "Target": "9ot0ieagg5xv1gxd85m7y33eq"
  },
  {
    "Target": "9ot0ieagg5xv1gxd85m7y33eq"
  }
]

After this change:

$ docker network create -d overlay foo
$ docker service create --name=test --network=foo nginx:alpine
$ docker service update --network-add foo test
service is already attached to network 9ot0ieagg5xv1gxd85m7y33eq

- Description for the changelog

* Fix duplicate networks being added with `docker service update --network-add` [docker/cli#780](https://github.com/docker/cli/pull/780)

thaJeztah · 2018-01-03T19:37:23Z

Didn't have a look yet at creating a unit-test for this (if we want one)

codecov-io · 2018-01-03T19:41:42Z

Codecov Report

Merging #780 into master will increase coverage by 1.31%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##           master     #780      +/-   ##
==========================================
+ Coverage    50.9%   52.22%   +1.31%     
==========================================
  Files         237      237              
  Lines       15338    15342       +4     
==========================================
+ Hits         7808     8012     +204     
+ Misses       7028     6819     -209     
- Partials      502      511       +9

dnephin · 2018-01-03T20:11:13Z

hmm, not sure how this works. Isn't the problem in cli/command/service/update.go updateNetworks(), where it compares network.Target to the id?

thaJeztah · 2018-01-03T20:30:02Z

Yeah, basically updateNetworks takes;

existing networks
networks set on --network-rm
networks set on --network-add

Each network set by --network-rm / --network-add is looked up, and network name is translated to network ID

However, that translation was not done for --network-add (also not on docker service create, but that's not problematic; the translation is done server-side in that case).

So on network create;

{
  "EndpointSpec": {
    "Mode": "vip"
  },
  "Labels": {},
  "Mode": {
    "Replicated": {}
  },
  "Name": "bla",
  "TaskTemplate": {
    "ContainerSpec": {
      "DNSConfig": {},
      "Image": "nginx:alpine@sha256:40e95eef5082a5b26c5fd9441bd7ee6bcda1bb5f9fbf7a4a1ef9b2b0f88d5c43"
    },
    "ForceUpdate": 0,
    "Networks": [
      {
        "Target": "foo"
      }
    ],
    "Placement": {
      "Platforms": [
        {
          "Architecture": "amd64",
          "OS": "linux"
        }
      ]
    },
    "Resources": {
      "Limits": {},
      "Reservations": {}
    }
  }
}

Then, on update, the spec is fetched from the daemon (which returns network ID's), but the added network is added to the spec by name, so this is sent on update:

{
  "EndpointSpec": {
    "Mode": "vip"
  },
  "Labels": {},
  "Mode": {
    "Replicated": {
      "Replicas": 1
    }
  },
  "Name": "bla",
  "TaskTemplate": {
    "ContainerSpec": {
      "DNSConfig": {},
      "Image": "nginx:alpine@sha256:40e95eef5082a5b26c5fd9441bd7ee6bcda1bb5f9fbf7a4a1ef9b2b0f88d5c43"
    },
    "ForceUpdate": 0,
    "Networks": [
      {
        "Target": "foo"
      },
      {
        "Target": "x8bnlf8np4o8gk8nnh0ilg6oo"
      }
    ],
    "Placement": {
      "Platforms": [
        {
          "Architecture": "amd64",
          "OS": "linux"
        }
      ]
    },
    "Resources": {
      "Limits": {},
      "Reservations": {}
    },
    "Runtime": "container"
  }
}

Where foo is the name for the network with ID x8bnlf8np4o8gk8nnh0ilg6oo

dnephin · 2018-01-03T20:57:40Z

Ok, but the code that is changed here is for converting opts to the swarn spec, not specifically for update, right? Isn't the update logic in cli/command/service/update.go updateNetworks() ?

So I'm wondering if this fix is maybe in the wrong place.

thaJeztah · 2018-01-03T21:16:56Z

Well, updateNetworks() calls this function (but only for the --network-add networks);

For --network-rm this is called:

cli/cli/command/service/update.go

Lines 1101 to 1107 in 97b148b

    
           for networkIDOrName := range toRemove { 
        
           	network, err := apiClient.NetworkInspect(ctx, networkIDOrName, types.NetworkInspectOptions{Scope: "swarm"}) 
        
           	if err != nil { 
        
           		return err 
        
           	} 
        
           	idsToRemove[network.ID] = struct{}{} 
        
           }

Existing networks (in the spec) already have an ID (no name);

cli/cli/command/service/update.go

Lines 1111 to 1118 in 97b148b

    
           for _, network := range specNetworks { 
        
           	if _, exists := idsToRemove[network.Target]; exists { 
        
           		continue 
        
           	} 
        
           	newNetworks = append(newNetworks, network) 
        
           	existingNetworks[network.Target] = struct{}{} 
        
           }

For --network-add, all networks are looked up first, and converted, then added to the list;

cli/cli/command/service/update.go

Lines 1122 to 1126 in 97b148b

    
           networks, err := convertNetworks(ctx, apiClient, *values) 
        
           if err != nil { 
        
           	return err 
        
           } 
        
           for _, network := range networks {

So this change will address both docker service create and docker service update to consistently use a network-ID

Alternatively, we move the apiClient.NetworkInspect() call out of convertNetworks() because it's not really related to the conversion itself, and currently only used to validate that a network exists

dnephin · 2018-01-03T21:49:22Z

It would be great to remove NetworkInspect from convertNetworks() although I guess it's still needed by the other caller.

Thanks, I understand the change now. I missed that this was being called from updateNetworks.

dnephin

LGTM

A unit test for updateNetworks() would be great, but unfortunately (due to all the "lookup by id nonsense") that is a lot more difficult than it should be.

It's should still be possible with a fakeClient that implements NetworkInspect()

thaJeztah · 2018-01-03T22:26:52Z

It would be great to remove NetworkInspect from convertNetworks() although I guess it's still needed by the other caller.

I agree; looking at this again, I think moving the NetworkInspect outside of that function may be the better solution (I want to look at the sorting though; not even sure if that should be in there, because it will sort on name without my change, but after processing, it's sorted again but this time on ID.

Given that this didn't make it for 18.01-rc1; let me change this to WIP, then;

I'll move the NetworkInspect
I'll write some unit tests (probably with a simple fakeClient)

I confirmed this bug already being in 17.09, so it's not new, so not super-urgent (and containers get only a single network assigned, so no duplicates there)

thaJeztah · 2018-01-09T14:17:11Z

@dnephin updated with the changes discussed, PTAL

thaJeztah · 2018-01-09T14:18:09Z

cli/command/service/opts.go

 		netAttach = append(netAttach, swarm.NetworkAttachmentConfig{
 			Target:     net.Target,
 			Aliases:    net.Aliases,
 			DriverOpts: net.DriverOpts,
 		})
 	}
-	sort.Sort(byNetworkTarget(netAttach))


Moved the sorting out of here as well, otherwise service create would sort by network name, and service update sorted by network ID

dnephin

LGTM!

dnephin · 2018-01-09T19:29:54Z

cli/command/service/opts.go

+	if err != nil {
+		return "", err
+	}
+	return nw.ID, nil


nit: the if is unnecessary since NetworkIsnpect returns a struct not a pointer. It can be replaced by

return nw.ID, err

Oh, nice one! I'll add that change

When adding a network using `docker service update --network-add`, the new network was added by _name_. Existing entries in a service spec are listed by network ID, which resulted in the CLI not detecting duplicate entries for the same network. This patch changes the behavior to always use the network-ID, so that duplicate entries are correctly caught. Before this change; $ docker network create -d overlay foo $ docker service create --name=test --network=foo nginx:alpine $ docker service update --network-add foo test $ docker service inspect --format '{{ json .Spec.TaskTemplate.Networks}}' test [ { "Target": "9ot0ieagg5xv1gxd85m7y33eq" }, { "Target": "9ot0ieagg5xv1gxd85m7y33eq" } ] After this change: $ docker network create -d overlay foo $ docker service create --name=test --network=foo nginx:alpine $ docker service update --network-add foo test service is already attached to network foo Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2018-01-21T00:04:01Z

ping @vdemeester PTAL

vdemeester

LGTM 🐯

GordonTheTurtle added the status/0-triage label Jan 3, 2018

thaJeztah added status/2-code-review and removed status/0-triage labels Jan 3, 2018

dnephin approved these changes Jan 3, 2018

View reviewed changes

thaJeztah changed the title ~~Fix --network-add adding duplicate networks~~ [WIP] Fix --network-add adding duplicate networks Jan 3, 2018

thaJeztah force-pushed the fix-network-add branch from 9b19a8c to 56d3fd9 Compare January 9, 2018 14:09

thaJeztah commented Jan 9, 2018

View reviewed changes

thaJeztah changed the title ~~[WIP] Fix --network-add adding duplicate networks~~ Fix --network-add adding duplicate networks Jan 9, 2018

dnephin approved these changes Jan 9, 2018

View reviewed changes

thaJeztah force-pushed the fix-network-add branch from 56d3fd9 to e6ebaf5 Compare January 9, 2018 19:42

vdemeester approved these changes Jan 22, 2018

View reviewed changes

vdemeester merged commit fbb9de2 into docker:master Jan 22, 2018

GordonTheTurtle added this to the 18.02.0 milestone Jan 22, 2018

thaJeztah deleted the fix-network-add branch November 21, 2018 10:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix --network-add adding duplicate networks #780

Fix --network-add adding duplicate networks #780

thaJeztah commented Jan 3, 2018 •

edited

Loading

thaJeztah commented Jan 3, 2018

codecov-io commented Jan 3, 2018 •

edited

Loading

dnephin commented Jan 3, 2018

thaJeztah commented Jan 3, 2018

dnephin commented Jan 3, 2018 •

edited

Loading

thaJeztah commented Jan 3, 2018

dnephin commented Jan 3, 2018

dnephin left a comment

thaJeztah commented Jan 3, 2018

thaJeztah commented Jan 9, 2018

thaJeztah Jan 9, 2018

dnephin left a comment

dnephin Jan 9, 2018

thaJeztah Jan 9, 2018

thaJeztah Jan 9, 2018

thaJeztah commented Jan 21, 2018

vdemeester left a comment

Fix --network-add adding duplicate networks #780

Fix --network-add adding duplicate networks #780

Conversation

thaJeztah commented Jan 3, 2018 • edited Loading

thaJeztah commented Jan 3, 2018

codecov-io commented Jan 3, 2018 • edited Loading

Codecov Report

dnephin commented Jan 3, 2018

thaJeztah commented Jan 3, 2018

dnephin commented Jan 3, 2018 • edited Loading

thaJeztah commented Jan 3, 2018

dnephin commented Jan 3, 2018

dnephin left a comment

Choose a reason for hiding this comment

thaJeztah commented Jan 3, 2018

thaJeztah commented Jan 9, 2018

thaJeztah Jan 9, 2018

Choose a reason for hiding this comment

dnephin left a comment

Choose a reason for hiding this comment

dnephin Jan 9, 2018

Choose a reason for hiding this comment

thaJeztah Jan 9, 2018

Choose a reason for hiding this comment

thaJeztah Jan 9, 2018

Choose a reason for hiding this comment

thaJeztah commented Jan 21, 2018

vdemeester left a comment

Choose a reason for hiding this comment

thaJeztah commented Jan 3, 2018 •

edited

Loading

codecov-io commented Jan 3, 2018 •

edited

Loading

dnephin commented Jan 3, 2018 •

edited

Loading