From e6ebaf55ddc1dde118c0164dc71a83f977a0adab Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 3 Jan 2018 15:40:55 +0100 Subject: [PATCH] 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 --- cli/command/service/client_test.go | 8 ++++ cli/command/service/opts.go | 26 +++++++----- cli/command/service/opts_test.go | 39 +++++++++++++++++ cli/command/service/update.go | 12 +++--- cli/command/service/update_test.go | 67 ++++++++++++++++++++++++++++++ 5 files changed, 136 insertions(+), 16 deletions(-) diff --git a/cli/command/service/client_test.go b/cli/command/service/client_test.go index 896892b943..b1349844c9 100644 --- a/cli/command/service/client_test.go +++ b/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, diff --git a/cli/command/service/opts.go b/cli/command/service/opts.go index b57c5c8c4e..19915050b9 100644 --- a/cli/command/service/opts.go +++ b/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, nil + return netAttach } 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) - if err != nil { - return service, err + 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 { diff --git a/cli/command/service/opts_test.go b/cli/command/service/opts_test.go index e373c6479d..f68cbb3c5a 100644 --- a/cli/command/service/opts_test.go +++ b/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) +} diff --git a/cli/command/service/update.go b/cli/command/service/update.go index d657871ff2..ca9752a656 100644 --- a/cli/command/service/update.go +++ b/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) - if err != nil { - return err - } + networks := convertNetworks(*values) for _, network := range networks { - if _, exists := existingNetworks[network.Target]; exists { + nwID, err := resolveNetworkID(ctx, apiClient, network.Target) + if err != nil { + return err + } + if _, exists := existingNetworks[nwID]; exists { return errors.Errorf("service is already attached to network %s", network.Target) } + network.Target = nwID newNetworks = append(newNetworks, network) existingNetworks[network.Target] = struct{}{} } diff --git a/cli/command/service/update_test.go b/cli/command/service/update_test.go index ba55269cb0..e7e428192f 100644 --- a/cli/command/service/update_test.go +++ b/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) +}