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>

cli/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
 	}

cli/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 {
+	for _, networkIDOrName := range networks {
-		nets = append(nets, swarm.NetworkAttachmentConfig{Target: network})
+		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"

cli/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
+}

cli/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")
 }

docs/reference/commandline/service_update.md

@@ -58,6 +58,8 @@ Options:
       --log-opt list                       Logging driver options (default [])
       --mount-add mount                    Add or update a mount on a service
       --mount-rm list                      Remove a mount by its target path (default [])
+      --network-add list                   Add a network
+      --network-rm list                    Remove a network
       --no-healthcheck                     Disable any container-specified HEALTHCHECK
       --placement-pref-add pref            Add a placement preference
       --placement-pref-rm pref             Remove a placement preference

integration-cli/daemon/daemon_swarm.go

@@ -205,7 +205,30 @@ func (d *Swarm) CheckServiceTasks(service string) func(*check.C) (interface{}, c
 	}
 }
 
-// CheckRunningTaskImages returns the number of different images attached to a running task
+// CheckRunningTaskNetworks returns the number of times each network is referenced from a task.
+func (d *Swarm) CheckRunningTaskNetworks(c *check.C) (interface{}, check.CommentInterface) {
+	var tasks []swarm.Task
+
+	filterArgs := filters.NewArgs()
+	filterArgs.Add("desired-state", "running")
+	filters, err := filters.ToParam(filterArgs)
+	c.Assert(err, checker.IsNil)
+
+	status, out, err := d.SockRequest("GET", "/tasks?filters="+filters, nil)
+	c.Assert(err, checker.IsNil, check.Commentf(string(out)))
+	c.Assert(status, checker.Equals, http.StatusOK, check.Commentf("output: %q", string(out)))
+	c.Assert(json.Unmarshal(out, &tasks), checker.IsNil)
+
+	result := make(map[string]int)
+	for _, task := range tasks {
+		for _, network := range task.Spec.Networks {
+			result[network.Target]++
+		}
+	}
+	return result, nil
+}
+
+// CheckRunningTaskImages returns the times each image is running as a task.
 func (d *Swarm) CheckRunningTaskImages(c *check.C) (interface{}, check.CommentInterface) {
 	var tasks []swarm.Task
 

integration-cli/docker_cli_swarm_test.go

@@ -855,6 +855,45 @@ func (s *DockerSwarmSuite) TestSwarmServiceTTYUpdate(c *check.C) {
 	c.Assert(strings.TrimSpace(out), checker.Equals, "true")
 }
 
+func (s *DockerSwarmSuite) TestSwarmServiceNetworkUpdate(c *check.C) {
+	d := s.AddDaemon(c, true, true)
+
+	result := icmd.RunCmd(d.Command("network", "create", "-d", "overlay", "foo"))
+	result.Assert(c, icmd.Success)
+	fooNetwork := strings.TrimSpace(string(result.Combined()))
+
+	result = icmd.RunCmd(d.Command("network", "create", "-d", "overlay", "bar"))
+	result.Assert(c, icmd.Success)
+	barNetwork := strings.TrimSpace(string(result.Combined()))
+
+	result = icmd.RunCmd(d.Command("network", "create", "-d", "overlay", "baz"))
+	result.Assert(c, icmd.Success)
+	bazNetwork := strings.TrimSpace(string(result.Combined()))
+
+	// Create a service
+	name := "top"
+	result = icmd.RunCmd(d.Command("service", "create", "--network", "foo", "--network", "bar", "--name", name, "busybox", "top"))
+	result.Assert(c, icmd.Success)
+
+	// Make sure task has been deployed.
+	waitAndAssert(c, defaultReconciliationTimeout, d.CheckRunningTaskNetworks, checker.DeepEquals,
+		map[string]int{fooNetwork: 1, barNetwork: 1})
+
+	// Remove a network
+	result = icmd.RunCmd(d.Command("service", "update", "--network-rm", "foo", name))
+	result.Assert(c, icmd.Success)
+
+	waitAndAssert(c, defaultReconciliationTimeout, d.CheckRunningTaskNetworks, checker.DeepEquals,
+		map[string]int{barNetwork: 1})
+
+	// Add a network
+	result = icmd.RunCmd(d.Command("service", "update", "--network-add", "baz", name))
+	result.Assert(c, icmd.Success)
+
+	waitAndAssert(c, defaultReconciliationTimeout, d.CheckRunningTaskNetworks, checker.DeepEquals,
+		map[string]int{barNetwork: 1, bazNetwork: 1})
+}
+
 func (s *DockerSwarmSuite) TestDNSConfig(c *check.C) {
 	d := s.AddDaemon(c, true, true)