Fix --network-add adding duplicate networks

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>

cli/command/service/client_test.go

@@ -16,6 +16,7 @@ type fakeClient struct {
 	serviceListFunc           func(context.Context, types.ServiceListOptions) ([]swarm.Service, error)
 	taskListFunc              func(context.Context, types.TaskListOptions) ([]swarm.Task, error)
 	infoFunc                  func(ctx context.Context) (types.Info, error)
+	networkInspectFunc        func(ctx context.Context, networkID string, options types.NetworkInspectOptions) (types.NetworkResource, error)
 }
 
 func (f *fakeClient) NodeList(ctx context.Context, options types.NodeListOptions) ([]swarm.Node, error) {
@@ -60,6 +61,13 @@ func (f *fakeClient) Info(ctx context.Context) (types.Info, error) {
 	return f.infoFunc(ctx)
 }
 
+func (f *fakeClient) NetworkInspect(ctx context.Context, networkID string, options types.NetworkInspectOptions) (types.NetworkResource, error) {
+	if f.networkInspectFunc != nil {
+		return f.networkInspectFunc(ctx, networkID, options)
+	}
+	return types.NetworkResource{}, nil
+}
+
 func newService(id string, name string) swarm.Service {
 	return swarm.Service{
 		ID:   id,

cli/command/service/opts.go

@@ -353,22 +353,21 @@ func (c *credentialSpecOpt) Value() *swarm.CredentialSpec {
 	return c.value
 }
 
-func convertNetworks(ctx context.Context, apiClient client.NetworkAPIClient, networks opts.NetworkOpt) ([]swarm.NetworkAttachmentConfig, error) {
+func resolveNetworkID(ctx context.Context, apiClient client.NetworkAPIClient, networkIDOrName string) (string, error) {
+	nw, err := apiClient.NetworkInspect(ctx, networkIDOrName, types.NetworkInspectOptions{Scope: "swarm"})
+	return nw.ID, err
+}
+
+func convertNetworks(networks opts.NetworkOpt) []swarm.NetworkAttachmentConfig {
 	var netAttach []swarm.NetworkAttachmentConfig
 	for _, net := range networks.Value() {
-		networkIDOrName := net.Target
-		_, err := apiClient.NetworkInspect(ctx, networkIDOrName, types.NetworkInspectOptions{Scope: "swarm"})
-		if err != nil {
-			return nil, err
-		}
 		netAttach = append(netAttach, swarm.NetworkAttachmentConfig{
 			Target:     net.Target,
 			Aliases:    net.Aliases,
 			DriverOpts: net.DriverOpts,
 		})
 	}
-	sort.Sort(byNetworkTarget(netAttach))
+	return netAttach
-	return netAttach, nil
 }
 
 type endpointOptions struct {
@@ -590,10 +589,15 @@ func (options *serviceOptions) ToService(ctx context.Context, apiClient client.N
 		return service, err
 	}
 
-	networks, err := convertNetworks(ctx, apiClient, options.networks)
+	networks := convertNetworks(options.networks)
+	for i, net := range networks {
+		nwID, err := resolveNetworkID(ctx, apiClient, net.Target)
 		if err != nil {
 			return service, err
 		}
+		networks[i].Target = nwID
+	}
+	sort.Sort(byNetworkTarget(networks))
 
 	resources, err := options.resources.ToResourceRequirements()
 	if err != nil {

cli/command/service/opts_test.go

@@ -1,12 +1,17 @@
 package service
 
 import (
+	"context"
+	"fmt"
 	"testing"
 	"time"
 
 	"github.com/docker/cli/opts"
+	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/api/types/container"
+	"github.com/docker/docker/api/types/swarm"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestMemBytesString(t *testing.T) {
@@ -123,3 +128,37 @@ func TestResourceOptionsToResourceRequirements(t *testing.T) {
 	}
 
 }
+
+func TestToServiceNetwork(t *testing.T) {
+	nws := []types.NetworkResource{
+		{Name: "aaa-network", ID: "id555"},
+		{Name: "mmm-network", ID: "id999"},
+		{Name: "zzz-network", ID: "id111"},
+	}
+
+	client := &fakeClient{
+		networkInspectFunc: func(ctx context.Context, networkID string, options types.NetworkInspectOptions) (types.NetworkResource, error) {
+			for _, network := range nws {
+				if network.ID == networkID || network.Name == networkID {
+					return network, nil
+				}
+			}
+			return types.NetworkResource{}, fmt.Errorf("network not found: %s", networkID)
+		},
+	}
+
+	nwo := opts.NetworkOpt{}
+	nwo.Set("zzz-network")
+	nwo.Set("mmm-network")
+	nwo.Set("aaa-network")
+
+	o := newServiceOptions()
+	o.mode = "replicated"
+	o.networks = nwo
+
+	ctx := context.Background()
+	flags := newCreateCommand(nil).Flags()
+	service, err := o.ToService(ctx, client, flags)
+	require.NoError(t, err)
+	assert.Equal(t, []swarm.NetworkAttachmentConfig{{Target: "id111"}, {Target: "id555"}, {Target: "id999"}}, service.TaskTemplate.Networks)
+}

cli/command/service/update.go

@@ -1119,14 +1119,16 @@ func updateNetworks(ctx context.Context, apiClient client.NetworkAPIClient, flag
 
 	if flags.Changed(flagNetworkAdd) {
 		values := flags.Lookup(flagNetworkAdd).Value.(*opts.NetworkOpt)
-		networks, err := convertNetworks(ctx, apiClient, *values)
+		networks := convertNetworks(*values)
+		for _, network := range networks {
+			nwID, err := resolveNetworkID(ctx, apiClient, network.Target)
 			if err != nil {
 				return err
 			}
-		for _, network := range networks {
+			if _, exists := existingNetworks[nwID]; exists {
-			if _, exists := existingNetworks[network.Target]; exists {
 				return errors.Errorf("service is already attached to network %s", network.Target)
 			}
+			network.Target = nwID
 			newNetworks = append(newNetworks, network)
 			existingNetworks[network.Target] = struct{}{}
 		}

cli/command/service/update_test.go

@@ -1,6 +1,7 @@
 package service
 
 import (
+	"fmt"
 	"reflect"
 	"sort"
 	"testing"
@@ -586,3 +587,69 @@ func TestRemoveGenericResources(t *testing.T) {
 	assert.NoError(t, removeGenericResources(flags, task))
 	assert.Len(t, task.Resources.Reservations.GenericResources, 1)
 }
+
+func TestUpdateNetworks(t *testing.T) {
+	ctx := context.Background()
+	nws := []types.NetworkResource{
+		{Name: "aaa-network", ID: "id555"},
+		{Name: "mmm-network", ID: "id999"},
+		{Name: "zzz-network", ID: "id111"},
+	}
+
+	client := &fakeClient{
+		networkInspectFunc: func(ctx context.Context, networkID string, options types.NetworkInspectOptions) (types.NetworkResource, error) {
+			for _, network := range nws {
+				if network.ID == networkID || network.Name == networkID {
+					return network, nil
+				}
+			}
+			return types.NetworkResource{}, fmt.Errorf("network not found: %s", networkID)
+		},
+	}
+
+	svc := swarm.ServiceSpec{
+		TaskTemplate: swarm.TaskSpec{
+			ContainerSpec: &swarm.ContainerSpec{},
+			Networks: []swarm.NetworkAttachmentConfig{
+				{Target: "id999"},
+			},
+		},
+	}
+
+	flags := newUpdateCommand(nil).Flags()
+	err := flags.Set(flagNetworkAdd, "aaa-network")
+	require.NoError(t, err)
+	err = updateService(ctx, client, flags, &svc)
+	require.NoError(t, err)
+	assert.Equal(t, []swarm.NetworkAttachmentConfig{{Target: "id555"}, {Target: "id999"}}, svc.TaskTemplate.Networks)
+
+	flags = newUpdateCommand(nil).Flags()
+	err = flags.Set(flagNetworkAdd, "aaa-network")
+	require.NoError(t, err)
+	err = updateService(ctx, client, flags, &svc)
+	assert.EqualError(t, err, "service is already attached to network aaa-network")
+	assert.Equal(t, []swarm.NetworkAttachmentConfig{{Target: "id555"}, {Target: "id999"}}, svc.TaskTemplate.Networks)
+
+	flags = newUpdateCommand(nil).Flags()
+	err = flags.Set(flagNetworkAdd, "id555")
+	require.NoError(t, err)
+	err = updateService(ctx, client, flags, &svc)
+	assert.EqualError(t, err, "service is already attached to network id555")
+	assert.Equal(t, []swarm.NetworkAttachmentConfig{{Target: "id555"}, {Target: "id999"}}, svc.TaskTemplate.Networks)
+
+	flags = newUpdateCommand(nil).Flags()
+	err = flags.Set(flagNetworkRemove, "id999")
+	require.NoError(t, err)
+	err = updateService(ctx, client, flags, &svc)
+	assert.NoError(t, err)
+	assert.Equal(t, []swarm.NetworkAttachmentConfig{{Target: "id555"}}, svc.TaskTemplate.Networks)
+
+	flags = newUpdateCommand(nil).Flags()
+	err = flags.Set(flagNetworkAdd, "mmm-network")
+	require.NoError(t, err)
+	err = flags.Set(flagNetworkRemove, "aaa-network")
+	require.NoError(t, err)
+	err = updateService(ctx, client, flags, &svc)
+	assert.NoError(t, err)
+	assert.Equal(t, []swarm.NetworkAttachmentConfig{{Target: "id999"}}, svc.TaskTemplate.Networks)
+}