cli: Allow service's networks to be updated

Resolve networks IDs on the client side.

Avoid filling in deprecated Spec.Networks field.

Sort networks in the TaskSpec for update stability.

Add an integration test for changing service networks.

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

command/service/create.go

@@ -63,7 +63,9 @@ func runCreate(dockerCli *command.DockerCli, flags *pflag.FlagSet, opts *service
 	apiClient := dockerCli.Client()
 	createOpts := types.ServiceCreateOptions{}
 
-	service, err := opts.ToService()
+	ctx := context.Background()
+
+	service, err := opts.ToService(ctx, apiClient)
 	if err != nil {
 		return err
 	}
@@ -79,8 +81,6 @@ func runCreate(dockerCli *command.DockerCli, flags *pflag.FlagSet, opts *service
 
 	}
 
-	ctx := context.Background()
-
 	if err := resolveServiceImageDigest(dockerCli, &service); err != nil {
 		return err
 	}

command/service/opts.go

@@ -2,17 +2,20 @@ package service
 
 import (
 	"fmt"
+	"sort"
 	"strconv"
 	"strings"
 	"time"
 
 	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/api/types/swarm"
+	"github.com/docker/docker/client"
 	"github.com/docker/docker/opts"
 	runconfigopts "github.com/docker/docker/runconfig/opts"
 	shlex "github.com/flynn-archive/go-shlex"
 	"github.com/pkg/errors"
 	"github.com/spf13/pflag"
+	"golang.org/x/net/context"
 )
 
 type int64Value interface {
@@ -270,12 +273,17 @@ func (c *credentialSpecOpt) Value() *swarm.CredentialSpec {
 	return c.value
 }
 
-func convertNetworks(networks []string) []swarm.NetworkAttachmentConfig {
+func convertNetworks(ctx context.Context, apiClient client.NetworkAPIClient, networks []string) ([]swarm.NetworkAttachmentConfig, error) {
 	nets := []swarm.NetworkAttachmentConfig{}
-	for _, network := range networks {
-		nets = append(nets, swarm.NetworkAttachmentConfig{Target: network})
+	for _, networkIDOrName := range networks {
+		network, err := apiClient.NetworkInspect(ctx, networkIDOrName, false)
+		if err != nil {
+			return nil, err
+		}
+		nets = append(nets, swarm.NetworkAttachmentConfig{Target: network.ID})
 	}
-	return nets
+	sort.Sort(byNetworkTarget(nets))
+	return nets, nil
 }
 
 type endpointOptions struct {
@@ -455,7 +463,7 @@ func (opts *serviceOptions) ToServiceMode() (swarm.ServiceMode, error) {
 	return serviceMode, nil
 }
 
-func (opts *serviceOptions) ToService() (swarm.ServiceSpec, error) {
+func (opts *serviceOptions) ToService(ctx context.Context, apiClient client.APIClient) (swarm.ServiceSpec, error) {
 	var service swarm.ServiceSpec
 
 	envVariables, err := runconfigopts.ReadKVStrings(opts.envFile.GetAll(), opts.env.GetAll())
@@ -487,6 +495,11 @@ func (opts *serviceOptions) ToService() (swarm.ServiceSpec, error) {
 		return service, err
 	}
 
+	networks, err := convertNetworks(ctx, apiClient, opts.networks.GetAll())
+	if err != nil {
+		return service, err
+	}
+
 	service = swarm.ServiceSpec{
 		Annotations: swarm.Annotations{
 			Name:   opts.name,
@@ -517,7 +530,7 @@ func (opts *serviceOptions) ToService() (swarm.ServiceSpec, error) {
 				Secrets:         nil,
 				Healthcheck:     healthConfig,
 			},
-			Networks:      convertNetworks(opts.networks.GetAll()),
+			Networks:      networks,
 			Resources:     opts.resources.ToResourceRequirements(),
 			RestartPolicy: opts.restartPolicy.ToRestartPolicy(),
 			Placement: &swarm.Placement{
@@ -526,7 +539,6 @@ func (opts *serviceOptions) ToService() (swarm.ServiceSpec, error) {
 			},
 			LogDriver: opts.logDriver.toLogDriver(),
 		},
-		Networks:       convertNetworks(opts.networks.GetAll()),
 		Mode:           serviceMode,
 		UpdateConfig:   opts.update.config(),
 		RollbackConfig: opts.rollback.config(),
@@ -666,6 +678,8 @@ const (
 	flagMountAdd                = "mount-add"
 	flagName                    = "name"
 	flagNetwork                 = "network"
+	flagNetworkAdd              = "network-add"
+	flagNetworkRemove           = "network-rm"
 	flagPublish                 = "publish"
 	flagPublishRemove           = "publish-rm"
 	flagPublishAdd              = "publish-add"

command/service/update.go

@@ -74,6 +74,10 @@ func newUpdateCommand(dockerCli *command.DockerCli) *cobra.Command {
 	flags.SetAnnotation(flagPlacementPrefAdd, "version", []string{"1.28"})
 	flags.Var(&placementPrefOpts{}, flagPlacementPrefRemove, "Remove a placement preference")
 	flags.SetAnnotation(flagPlacementPrefRemove, "version", []string{"1.28"})
+	flags.Var(&serviceOpts.networks, flagNetworkAdd, "Add a network")
+	flags.SetAnnotation(flagNetworkAdd, "version", []string{"1.29"})
+	flags.Var(newListOptsVar(), flagNetworkRemove, "Remove a network")
+	flags.SetAnnotation(flagNetworkRemove, "version", []string{"1.29"})
 	flags.Var(&serviceOpts.endpoint.publishPorts, flagPublishAdd, "Add or update a published port")
 	flags.Var(&serviceOpts.groups, flagGroupAdd, "Add an additional supplementary user group to the container")
 	flags.SetAnnotation(flagGroupAdd, "version", []string{"1.25"})
@@ -147,7 +151,7 @@ func runUpdate(dockerCli *command.DockerCli, flags *pflag.FlagSet, opts *service
 		updateOpts.Rollback = "previous"
 	}
 
-	err = updateService(flags, spec)
+	err = updateService(ctx, apiClient, flags, spec)
 	if err != nil {
 		return err
 	}
@@ -207,7 +211,7 @@ func runUpdate(dockerCli *command.DockerCli, flags *pflag.FlagSet, opts *service
 	return waitOnService(ctx, dockerCli, serviceID, opts)
 }
 
-func updateService(flags *pflag.FlagSet, spec *swarm.ServiceSpec) error {
+func updateService(ctx context.Context, apiClient client.APIClient, flags *pflag.FlagSet, spec *swarm.ServiceSpec) error {
 	updateString := func(flag string, field *string) {
 		if flags.Changed(flag) {
 			*field, _ = flags.GetString(flag)
@@ -316,6 +320,12 @@ func updateService(flags *pflag.FlagSet, spec *swarm.ServiceSpec) error {
 		updatePlacementPreferences(flags, task.Placement)
 	}
 
+	if anyChanged(flags, flagNetworkAdd, flagNetworkRemove) {
+		if err := updateNetworks(ctx, apiClient, flags, spec); err != nil {
+			return err
+		}
+	}
+
 	if err := updateReplicas(flags, &spec.Mode); err != nil {
 		return err
 	}
@@ -623,7 +633,6 @@ func (m byMountSource) Less(i, j int) bool {
 }
 
 func updateMounts(flags *pflag.FlagSet, mounts *[]mounttypes.Mount) error {
-
 	mountsByTarget := map[string]mounttypes.Mount{}
 
 	if flags.Changed(flagMountAdd) {
@@ -947,3 +956,63 @@ func updateHealthcheck(flags *pflag.FlagSet, containerSpec *swarm.ContainerSpec)
 	}
 	return nil
 }
+
+type byNetworkTarget []swarm.NetworkAttachmentConfig
+
+func (m byNetworkTarget) Len() int      { return len(m) }
+func (m byNetworkTarget) Swap(i, j int) { m[i], m[j] = m[j], m[i] }
+func (m byNetworkTarget) Less(i, j int) bool {
+	return m[i].Target < m[j].Target
+}
+
+func updateNetworks(ctx context.Context, apiClient client.NetworkAPIClient, flags *pflag.FlagSet, spec *swarm.ServiceSpec) error {
+	// spec.TaskTemplate.Networks takes precedence over the deprecated
+	// spec.Networks field. If spec.Network is in use, we'll migrate those
+	// values to spec.TaskTemplate.Networks.
+	specNetworks := spec.TaskTemplate.Networks
+	if len(specNetworks) == 0 {
+		specNetworks = spec.Networks
+	}
+	spec.Networks = nil
+
+	toRemove := buildToRemoveSet(flags, flagNetworkRemove)
+	idsToRemove := make(map[string]struct{})
+	for networkIDOrName := range toRemove {
+		network, err := apiClient.NetworkInspect(ctx, networkIDOrName, false)
+		if err != nil {
+			return err
+		}
+		idsToRemove[network.ID] = struct{}{}
+	}
+
+	existingNetworks := make(map[string]struct{})
+	var newNetworks []swarm.NetworkAttachmentConfig
+	for _, network := range specNetworks {
+		if _, exists := idsToRemove[network.Target]; exists {
+			continue
+		}
+
+		newNetworks = append(newNetworks, network)
+		existingNetworks[network.Target] = struct{}{}
+	}
+
+	if flags.Changed(flagNetworkAdd) {
+		values := flags.Lookup(flagNetworkAdd).Value.(*opts.ListOpts).GetAll()
+		networks, err := convertNetworks(ctx, apiClient, values)
+		if err != nil {
+			return err
+		}
+		for _, network := range networks {
+			if _, exists := existingNetworks[network.Target]; exists {
+				return errors.Errorf("service is already attached to network %s", network.Target)
+			}
+			newNetworks = append(newNetworks, network)
+			existingNetworks[network.Target] = struct{}{}
+		}
+	}
+
+	sort.Sort(byNetworkTarget(newNetworks))
+
+	spec.TaskTemplate.Networks = newNetworks
+	return nil
+}

command/service/update_test.go

@@ -22,7 +22,7 @@ func TestUpdateServiceArgs(t *testing.T) {
 	cspec := &spec.TaskTemplate.ContainerSpec
 	cspec.Args = []string{"old", "args"}
 
-	updateService(flags, spec)
+	updateService(nil, nil, flags, spec)
 	assert.EqualStringSlice(t, cspec.Args, []string{"the", "new args"})
 }
 
@@ -458,18 +458,18 @@ func TestUpdateReadOnly(t *testing.T) {
 	// Update with --read-only=true, changed to true
 	flags := newUpdateCommand(nil).Flags()
 	flags.Set("read-only", "true")
-	updateService(flags, spec)
+	updateService(nil, nil, flags, spec)
 	assert.Equal(t, cspec.ReadOnly, true)
 
 	// Update without --read-only, no change
 	flags = newUpdateCommand(nil).Flags()
-	updateService(flags, spec)
+	updateService(nil, nil, flags, spec)
 	assert.Equal(t, cspec.ReadOnly, true)
 
 	// Update with --read-only=false, changed to false
 	flags = newUpdateCommand(nil).Flags()
 	flags.Set("read-only", "false")
-	updateService(flags, spec)
+	updateService(nil, nil, flags, spec)
 	assert.Equal(t, cspec.ReadOnly, false)
 }
 
@@ -480,17 +480,17 @@ func TestUpdateStopSignal(t *testing.T) {
 	// Update with --stop-signal=SIGUSR1
 	flags := newUpdateCommand(nil).Flags()
 	flags.Set("stop-signal", "SIGUSR1")
-	updateService(flags, spec)
+	updateService(nil, nil, flags, spec)
 	assert.Equal(t, cspec.StopSignal, "SIGUSR1")
 
 	// Update without --stop-signal, no change
 	flags = newUpdateCommand(nil).Flags()
-	updateService(flags, spec)
+	updateService(nil, nil, flags, spec)
 	assert.Equal(t, cspec.StopSignal, "SIGUSR1")
 
 	// Update with --stop-signal=SIGWINCH
 	flags = newUpdateCommand(nil).Flags()
 	flags.Set("stop-signal", "SIGWINCH")
-	updateService(flags, spec)
+	updateService(nil, nil, flags, spec)
 	assert.Equal(t, cspec.StopSignal, "SIGWINCH")
 }