From 808ca15347de5d9274fc4add94cdbcb03252cd1b Mon Sep 17 00:00:00 2001 From: Aaron Lehmann Date: Thu, 23 Mar 2017 17:51:57 -0700 Subject: [PATCH 1/3] 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") } From f804f893b67b021159e578ef05362c88c3b701a2 Mon Sep 17 00:00:00 2001 From: Aaron Lehmann Date: Fri, 24 Mar 2017 16:58:42 -0700 Subject: [PATCH 2/3] cli: Deploying a compose file must use TaskTemplate.Networks This is the non-deprecated field, and the one that can be changed in a service update. Since old daemon versions don't allow migrating from one field to the other, make this conditional on the API version. Signed-off-by: Aaron Lehmann --- compose/convert/service.go | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/compose/convert/service.go b/compose/convert/service.go index 7af24b2ec7..66f7868767 100644 --- a/compose/convert/service.go +++ b/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 } From 008f6d1b3f535ccb561374b71b76565b0f494ebc Mon Sep 17 00:00:00 2001 From: Aaron Lehmann Date: Fri, 7 Apr 2017 16:37:03 -0700 Subject: [PATCH 3/3] Show network names in "docker service inspect --pretty" Signed-off-by: Aaron Lehmann --- command/formatter/service.go | 29 +++++++++++++++++++++++++---- command/service/inspect.go | 10 +++++++++- command/service/inspect_test.go | 30 +++++++++++++++++++++--------- 3 files changed, 55 insertions(+), 14 deletions(-) diff --git a/command/formatter/service.go b/command/formatter/service.go index f5eb1aeaf7..8f57af22d9 100644 --- a/command/formatter/service.go +++ b/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 { - out = append(out, n.Target) + for _, n := range ctx.Service.Spec.TaskTemplate.Networks { + if name, ok := ctx.networkNames[n.Target]; ok { + out = append(out, name) + } else { + out = append(out, n.Target) + } } return out } diff --git a/command/service/inspect.go b/command/service/inspect.go index 8247d45afa..8a8b51cd0e 100644 --- a/command/service/inspect.go +++ b/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 diff --git a/command/service/inspect_test.go b/command/service/inspect_test.go index 94c96cc164..44d9df9176 100644 --- a/command/service/inspect_test.go +++ b/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) { - return s, nil, nil - }) + err := formatter.ServiceInspectWrite(ctx, []string{"de179gar9d0o7ltdybungplod"}, + 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) {