From 808ca15347de5d9274fc4add94cdbcb03252cd1b Mon Sep 17 00:00:00 2001 From: Aaron Lehmann Date: Thu, 23 Mar 2017 17:51:57 -0700 Subject: [PATCH] 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 --- command/service/create.go | 6 +-- command/service/opts.go | 28 +++++++++---- command/service/update.go | 75 ++++++++++++++++++++++++++++++++-- command/service/update_test.go | 14 +++---- 4 files changed, 103 insertions(+), 20 deletions(-) diff --git a/command/service/create.go b/command/service/create.go index 76b61f6c2e..0e77f73d32 100644 --- a/command/service/create.go +++ b/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 } diff --git a/command/service/opts.go b/command/service/opts.go index 47d417fb25..066436838c 100644 --- a/command/service/opts.go +++ b/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" diff --git a/command/service/update.go b/command/service/update.go index bf428b4ae6..b59f163829 100644 --- a/command/service/update.go +++ b/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 +} diff --git a/command/service/update_test.go b/command/service/update_test.go index 7a588d7fef..090372fb78 100644 --- a/command/service/update_test.go +++ b/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") }