Merge pull request #32062 from aaronlehmann/change-network-attachments

Support service network attachment changes

command/formatter/service.go

@@ -6,6 +6,7 @@ import (
 	"time"
 
 	"github.com/docker/distribution/reference"
+	"github.com/docker/docker/api/types"
 	mounttypes "github.com/docker/docker/api/types/mount"
 	"github.com/docker/docker/api/types/swarm"
 	"github.com/docker/docker/cli/command/inspect"
@@ -137,8 +138,20 @@ func NewServiceFormat(source string) Format {
 	}
 }
 
+func resolveNetworks(service swarm.Service, getNetwork inspect.GetRefFunc) map[string]string {
+	networkNames := make(map[string]string)
+	for _, network := range service.Spec.TaskTemplate.Networks {
+		if resolved, _, err := getNetwork(network.Target); err == nil {
+			if resolvedNetwork, ok := resolved.(types.NetworkResource); ok {
+				networkNames[resolvedNetwork.ID] = resolvedNetwork.Name
+			}
+		}
+	}
+	return networkNames
+}
+
 // ServiceInspectWrite renders the context for a list of services
-func ServiceInspectWrite(ctx Context, refs []string, getRef inspect.GetRefFunc) error {
+func ServiceInspectWrite(ctx Context, refs []string, getRef, getNetwork inspect.GetRefFunc) error {
 	if ctx.Format != serviceInspectPrettyTemplate {
 		return inspect.Inspect(ctx.Output, refs, string(ctx.Format), getRef)
 	}
@@ -152,7 +165,7 @@ func ServiceInspectWrite(ctx Context, refs []string, getRef inspect.GetRefFunc)
 			if !ok {
 				return errors.Errorf("got wrong object to inspect")
 			}
-			if err := format(&serviceInspectContext{Service: service}); err != nil {
+			if err := format(&serviceInspectContext{Service: service, networkNames: resolveNetworks(service, getNetwork)}); err != nil {
 				return err
 			}
 		}
@@ -164,6 +177,10 @@ func ServiceInspectWrite(ctx Context, refs []string, getRef inspect.GetRefFunc)
 type serviceInspectContext struct {
 	swarm.Service
 	subContext
+
+	// networkNames is a map from network IDs (as found in
+	// Networks[x].Target) to network names.
+	networkNames map[string]string
 }
 
 func (ctx *serviceInspectContext) MarshalJSON() ([]byte, error) {
@@ -383,8 +400,12 @@ func (ctx *serviceInspectContext) ResourceLimitMemory() string {
 
 func (ctx *serviceInspectContext) Networks() []string {
 	var out []string
-	for _, n := range ctx.Service.Spec.Networks {
+	for _, n := range ctx.Service.Spec.TaskTemplate.Networks {
-		out = append(out, n.Target)
+		if name, ok := ctx.networkNames[n.Target]; ok {
+			out = append(out, name)
+		} else {
+			out = append(out, n.Target)
+		}
 	}
 	return out
 }

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/inspect.go

@@ -58,6 +58,14 @@ func runInspect(dockerCli *command.DockerCli, opts inspectOptions) error {
 		return nil, nil, errors.Errorf("Error: no such service: %s", ref)
 	}
 
+	getNetwork := func(ref string) (interface{}, []byte, error) {
+		network, _, err := client.NetworkInspectWithRaw(ctx, ref, false)
+		if err == nil || !apiclient.IsErrNetworkNotFound(err) {
+			return network, nil, err
+		}
+		return nil, nil, errors.Errorf("Error: no such network: %s", ref)
+	}
+
 	f := opts.format
 	if len(f) == 0 {
 		f = "raw"
@@ -77,7 +85,7 @@ func runInspect(dockerCli *command.DockerCli, opts inspectOptions) error {
 		Format: formatter.NewServiceFormat(f),
 	}
 
-	if err := formatter.ServiceInspectWrite(serviceCtx, opts.refs, getRef); err != nil {
+	if err := formatter.ServiceInspectWrite(serviceCtx, opts.refs, getRef, getNetwork); err != nil {
 		return cli.StatusError{StatusCode: 1, Status: err.Error()}
 	}
 	return nil

command/service/inspect_test.go

@@ -7,6 +7,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/api/types/swarm"
 	"github.com/docker/docker/cli/command/formatter"
 	"github.com/docker/docker/pkg/testutil/assert"
@@ -43,18 +44,18 @@ func formatServiceInspect(t *testing.T, format formatter.Format, now time.Time)
 				ContainerSpec: swarm.ContainerSpec{
 					Image: "foo/bar@sha256:this_is_a_test",
 				},
+				Networks: []swarm.NetworkAttachmentConfig{
+					{
+						Target:  "5vpyomhb6ievnk0i0o60gcnei",
+						Aliases: []string{"web"},
+					},
+				},
 			},
 			Mode: swarm.ServiceMode{
 				Replicated: &swarm.ReplicatedService{
 					Replicas: &two,
 				},
 			},
-			Networks: []swarm.NetworkAttachmentConfig{
-				{
-					Target:  "5vpyomhb6ievnk0i0o60gcnei",
-					Aliases: []string{"web"},
-				},
-			},
 			EndpointSpec: endpointSpec,
 		},
 		Endpoint: swarm.Endpoint{
@@ -84,9 +85,17 @@ func formatServiceInspect(t *testing.T, format formatter.Format, now time.Time)
 		Format: format,
 	}
 
-	err := formatter.ServiceInspectWrite(ctx, []string{"de179gar9d0o7ltdybungplod"}, func(ref string) (interface{}, []byte, error) {
+	err := formatter.ServiceInspectWrite(ctx, []string{"de179gar9d0o7ltdybungplod"},
-		return s, nil, nil
+		func(ref string) (interface{}, []byte, error) {
-	})
+			return s, nil, nil
+		},
+		func(ref string) (interface{}, []byte, error) {
+			return types.NetworkResource{
+				ID:   "5vpyomhb6ievnk0i0o60gcnei",
+				Name: "mynetwork",
+			}, nil, nil
+		},
+	)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -98,6 +107,9 @@ func TestPrettyPrintWithNoUpdateConfig(t *testing.T) {
 	if strings.Contains(s, "UpdateStatus") {
 		t.Fatal("Pretty print failed before parsing UpdateStatus")
 	}
+	if !strings.Contains(s, "mynetwork") {
+		t.Fatal("network name not found in inspect output")
+	}
 }
 
 func TestJSONFormatWithNoUpdateConfig(t *testing.T) {

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"

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

compose/convert/service.go

@@ -9,6 +9,7 @@ import (
 
 	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/api/types/swarm"
+	"github.com/docker/docker/api/types/versions"
 	servicecli "github.com/docker/docker/cli/command/service"
 	composetypes "github.com/docker/docker/cli/compose/types"
 	"github.com/docker/docker/client"
@@ -20,11 +21,10 @@ import (
 const defaultNetwork = "default"
 
 // Services from compose-file types to engine API types
-// TODO: fix secrets API so that SecretAPIClient is not required here
 func Services(
 	namespace Namespace,
 	config *composetypes.Config,
-	client client.SecretAPIClient,
+	client client.CommonAPIClient,
 ) (map[string]swarm.ServiceSpec, error) {
 	result := make(map[string]swarm.ServiceSpec)
 
@@ -33,12 +33,11 @@ func Services(
 	networks := config.Networks
 
 	for _, service := range services {
-
 		secrets, err := convertServiceSecrets(client, namespace, service.Secrets, config.Secrets)
 		if err != nil {
 			return nil, errors.Wrapf(err, "service %s", service.Name)
 		}
-		serviceSpec, err := convertService(namespace, service, networks, volumes, secrets)
+		serviceSpec, err := convertService(client.ClientVersion(), namespace, service, networks, volumes, secrets)
 		if err != nil {
 			return nil, errors.Wrapf(err, "service %s", service.Name)
 		}
@@ -49,6 +48,7 @@ func Services(
 }
 
 func convertService(
+	apiVersion string,
 	namespace Namespace,
 	service composetypes.ServiceConfig,
 	networkConfigs map[string]composetypes.NetworkConfig,
@@ -133,10 +133,21 @@ func convertService(
 		},
 		EndpointSpec: endpoint,
 		Mode:         mode,
-		Networks:     networks,
 		UpdateConfig: convertUpdateConfig(service.Deploy.UpdateConfig),
 	}
 
+	// ServiceSpec.Networks is deprecated and should not have been used by
+	// this package. It is possible to update TaskTemplate.Networks, but it
+	// is not possible to update ServiceSpec.Networks. Unfortunately, we
+	// can't unconditionally start using TaskTemplate.Networks, because that
+	// will break with older daemons that don't support migrating from
+	// ServiceSpec.Networks to TaskTemplate.Networks. So which field to use
+	// is conditional on daemon version.
+	if versions.LessThan(apiVersion, "1.29") {
+		serviceSpec.Networks = networks
+	} else {
+		serviceSpec.TaskTemplate.Networks = networks
+	}
 	return serviceSpec, nil
 }