Remove --port and update --publish for services to support syntaxes

Add support for simple and complex syntax to `--publish` through the
use of `PortOpt`.

Signed-off-by: Vincent Demeester <[email protected]>

CHANGELOG.md

@@ -72,7 +72,7 @@ To manually remove all plugins and resolve this problem, take the following step
 ### Networking
 
 + Add `--attachable` network support to enable `docker run` to work in swarm-mode overlay network [#25962](https://github.com/docker/docker/pull/25962)
-+ Add support for host port PublishMode in services using the `--port` option in `docker service create` [#27917](https://github.com/docker/docker/pull/27917)
++ Add support for host port PublishMode in services using the `--publish` option in `docker service create` [#27917](https://github.com/docker/docker/pull/27917) and [#28943](https://github.com/docker/docker/pull/28943)
 + Add support for Windows server 2016 overlay network driver (requires upcoming ws2016 update) [#28182](https://github.com/docker/docker/pull/28182)
 * Change the default `FORWARD` policy to `DROP` [#28257](https://github.com/docker/docker/pull/28257)
 + Add support for specifying static IP addresses for predefined network on windows [#22208](https://github.com/docker/docker/pull/22208)

cli/command/service/create.go

@@ -40,13 +40,11 @@ func newCreateCommand(dockerCli *command.DockerCli) *cobra.Command {
 	flags.Var(&opts.networks, flagNetwork, "Network attachments")
 	flags.Var(&opts.secrets, flagSecret, "Specify secrets to expose to the service")
 	flags.VarP(&opts.endpoint.publishPorts, flagPublish, "p", "Publish a port as a node port")
-	flags.MarkHidden(flagPublish)
 	flags.Var(&opts.groups, flagGroup, "Set one or more supplementary user groups for the container")
 	flags.Var(&opts.dns, flagDNS, "Set custom DNS servers")
 	flags.Var(&opts.dnsOption, flagDNSOption, "Set DNS options")
 	flags.Var(&opts.dnsSearch, flagDNSSearch, "Set custom DNS search domains")
 	flags.Var(&opts.hosts, flagHost, "Set one or more custom host-to-IP mappings (host:ip)")
-	flags.Var(&opts.endpoint.expandedPorts, flagPort, "Publish a port")
 
 	flags.SetInterspersed(false)
 	return cmd

cli/command/service/opts.go

@@ -287,45 +287,17 @@ func convertNetworks(networks []string) []swarm.NetworkAttachmentConfig {
 }
 
 type endpointOptions struct {
-	mode          string
-	publishPorts  opts.ListOpts
-	expandedPorts opts.PortOpt
+	mode         string
+	publishPorts opts.PortOpt
 }
 
 func (e *endpointOptions) ToEndpointSpec() *swarm.EndpointSpec {
-	portConfigs := []swarm.PortConfig{}
-	// We can ignore errors because the format was already validated by ValidatePort
-	ports, portBindings, _ := nat.ParsePortSpecs(e.publishPorts.GetAll())
-
-	for port := range ports {
-		portConfigs = append(portConfigs, ConvertPortToPortConfig(port, portBindings)...)
-	}
-
 	return &swarm.EndpointSpec{
 		Mode:  swarm.ResolutionMode(strings.ToLower(e.mode)),
-		Ports: append(portConfigs, e.expandedPorts.Value()...),
+		Ports: e.publishPorts.Value(),
 	}
 }
 
-// ConvertPortToPortConfig converts ports to the swarm type
-func ConvertPortToPortConfig(
-	port nat.Port,
-	portBindings map[nat.Port][]nat.PortBinding,
-) []swarm.PortConfig {
-	ports := []swarm.PortConfig{}
-
-	for _, binding := range portBindings[port] {
-		hostPort, _ := strconv.ParseUint(binding.HostPort, 10, 16)
-		ports = append(ports, swarm.PortConfig{
-			//TODO Name: ?
-			Protocol:      swarm.PortConfigProtocol(strings.ToLower(port.Proto())),
-			TargetPort:    uint32(port.Int()),
-			PublishedPort: uint32(hostPort),
-		})
-	}
-	return ports
-}
-
 type logDriverOptions struct {
 	name string
 	opts opts.ListOpts
@@ -459,16 +431,13 @@ func newServiceOptions() *serviceOptions {
 		containerLabels: opts.NewListOpts(runconfigopts.ValidateEnv),
 		env:             opts.NewListOpts(runconfigopts.ValidateEnv),
 		envFile:         opts.NewListOpts(nil),
-		endpoint: endpointOptions{
-			publishPorts: opts.NewListOpts(ValidatePort),
-		},
-		groups:    opts.NewListOpts(nil),
-		logDriver: newLogDriverOptions(),
-		dns:       opts.NewListOpts(opts.ValidateIPAddress),
-		dnsOption: opts.NewListOpts(nil),
-		dnsSearch: opts.NewListOpts(opts.ValidateDNSSearch),
-		hosts:     opts.NewListOpts(runconfigopts.ValidateExtraHost),
-		networks:  opts.NewListOpts(nil),
+		groups:          opts.NewListOpts(nil),
+		logDriver:       newLogDriverOptions(),
+		dns:             opts.NewListOpts(opts.ValidateIPAddress),
+		dnsOption:       opts.NewListOpts(nil),
+		dnsSearch:       opts.NewListOpts(opts.ValidateDNSSearch),
+		hosts:           opts.NewListOpts(runconfigopts.ValidateExtraHost),
+		networks:        opts.NewListOpts(nil),
 	}
 }
 
@@ -649,9 +618,6 @@ const (
 	flagPublish               = "publish"
 	flagPublishRemove         = "publish-rm"
 	flagPublishAdd            = "publish-add"
-	flagPort                  = "port"
-	flagPortAdd               = "port-add"
-	flagPortRemove            = "port-rm"
 	flagReplicas              = "replicas"
 	flagReserveCPU            = "reserve-cpu"
 	flagReserveMemory         = "reserve-memory"

cli/command/service/update.go

@@ -24,7 +24,7 @@ import (
 )
 
 func newUpdateCommand(dockerCli *command.DockerCli) *cobra.Command {
-	opts := newServiceOptions()
+	serviceOpts := newServiceOptions()
 
 	cmd := &cobra.Command{
 		Use:   "update [OPTIONS] SERVICE",
@@ -40,36 +40,33 @@ func newUpdateCommand(dockerCli *command.DockerCli) *cobra.Command {
 	flags.String("args", "", "Service command args")
 	flags.Bool("rollback", false, "Rollback to previous specification")
 	flags.Bool("force", false, "Force update even if no changes require it")
-	addServiceFlags(cmd, opts)
+	addServiceFlags(cmd, serviceOpts)
 
 	flags.Var(newListOptsVar(), flagEnvRemove, "Remove an environment variable")
 	flags.Var(newListOptsVar(), flagGroupRemove, "Remove a previously added supplementary user group from the container")
 	flags.Var(newListOptsVar(), flagLabelRemove, "Remove a label by its key")
 	flags.Var(newListOptsVar(), flagContainerLabelRemove, "Remove a container label by its key")
 	flags.Var(newListOptsVar(), flagMountRemove, "Remove a mount by its target path")
-	flags.Var(newListOptsVar().WithValidator(validatePublishRemove), flagPublishRemove, "Remove a published port by its target port")
-	flags.MarkHidden(flagPublishRemove)
-	flags.Var(newListOptsVar(), flagPortRemove, "Remove a port(target-port mandatory)")
+	// flags.Var(newListOptsVar().WithValidator(validatePublishRemove), flagPublishRemove, "Remove a published port by its target port")
+	flags.Var(&opts.PortOpt{}, flagPublishRemove, "Remove a published port by its target port")
 	flags.Var(newListOptsVar(), flagConstraintRemove, "Remove a constraint")
 	flags.Var(newListOptsVar(), flagDNSRemove, "Remove a custom DNS server")
 	flags.Var(newListOptsVar(), flagDNSOptionRemove, "Remove a DNS option")
 	flags.Var(newListOptsVar(), flagDNSSearchRemove, "Remove a DNS search domain")
 	flags.Var(newListOptsVar(), flagHostRemove, "Remove a custom host-to-IP mapping (host:ip)")
-	flags.Var(&opts.labels, flagLabelAdd, "Add or update a service label")
-	flags.Var(&opts.containerLabels, flagContainerLabelAdd, "Add or update a container label")
-	flags.Var(&opts.env, flagEnvAdd, "Add or update an environment variable")
+	flags.Var(&serviceOpts.labels, flagLabelAdd, "Add or update a service label")
+	flags.Var(&serviceOpts.containerLabels, flagContainerLabelAdd, "Add or update a container label")
+	flags.Var(&serviceOpts.env, flagEnvAdd, "Add or update an environment variable")
 	flags.Var(newListOptsVar(), flagSecretRemove, "Remove a secret")
-	flags.Var(&opts.secrets, flagSecretAdd, "Add or update a secret on a service")
-	flags.Var(&opts.mounts, flagMountAdd, "Add or update a mount on a service")
-	flags.Var(&opts.constraints, flagConstraintAdd, "Add or update a placement constraint")
-	flags.Var(&opts.endpoint.publishPorts, flagPublishAdd, "Add or update a published port")
-	flags.MarkHidden(flagPublishAdd)
-	flags.Var(&opts.endpoint.expandedPorts, flagPortAdd, "Add or update a port")
-	flags.Var(&opts.groups, flagGroupAdd, "Add an additional supplementary user group to the container")
-	flags.Var(&opts.dns, flagDNSAdd, "Add or update a custom DNS server")
-	flags.Var(&opts.dnsOption, flagDNSOptionAdd, "Add or update a DNS option")
-	flags.Var(&opts.dnsSearch, flagDNSSearchAdd, "Add or update a custom DNS search domain")
-	flags.Var(&opts.hosts, flagHostAdd, "Add or update a custom host-to-IP mapping (host:ip)")
+	flags.Var(&serviceOpts.secrets, flagSecretAdd, "Add or update a secret on a service")
+	flags.Var(&serviceOpts.mounts, flagMountAdd, "Add or update a mount on a service")
+	flags.Var(&serviceOpts.constraints, flagConstraintAdd, "Add or update a placement constraint")
+	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.Var(&serviceOpts.dns, flagDNSAdd, "Add or update a custom DNS server")
+	flags.Var(&serviceOpts.dnsOption, flagDNSOptionAdd, "Add or update a DNS option")
+	flags.Var(&serviceOpts.dnsSearch, flagDNSSearchAdd, "Add or update a custom DNS search domain")
+	flags.Var(&serviceOpts.hosts, flagHostAdd, "Add or update a custom host-to-IP mapping (host:ip)")
 
 	return cmd
 }
@@ -276,7 +273,7 @@ func updateService(flags *pflag.FlagSet, spec *swarm.ServiceSpec) error {
 		}
 	}
 
-	if anyChanged(flags, flagPublishAdd, flagPublishRemove, flagPortAdd, flagPortRemove) {
+	if anyChanged(flags, flagPublishAdd, flagPublishRemove) {
 		if spec.EndpointSpec == nil {
 			spec.EndpointSpec = &swarm.EndpointSpec{}
 		}
@@ -645,6 +642,7 @@ func portConfigToString(portConfig *swarm.PortConfig) string {
 	return fmt.Sprintf("%v:%v/%s/%s", portConfig.PublishedPort, portConfig.TargetPort, protocol, mode)
 }
 
+// FIXME(vdemeester) port to opts.PortOpt
 // This validation is only used for `--publish-rm`.
 // The `--publish-rm` takes:
 // <TargetPort>[/<Protocol>] (e.g., 80, 80/tcp, 53/udp)
@@ -667,26 +665,13 @@ func updatePorts(flags *pflag.FlagSet, portConfig *[]swarm.PortConfig) error {
 	portSet := map[string]swarm.PortConfig{}
 	// Check to see if there are any conflict in flags.
 	if flags.Changed(flagPublishAdd) {
-		values := flags.Lookup(flagPublishAdd).Value.(*opts.ListOpts).GetAll()
-		ports, portBindings, _ := nat.ParsePortSpecs(values)
-
-		for port := range ports {
-			newConfigs := ConvertPortToPortConfig(port, portBindings)
-			for _, entry := range newConfigs {
-				if v, ok := portSet[portConfigToString(&entry)]; ok && v != entry {
-					return fmt.Errorf("conflicting port mapping between %v:%v/%s and %v:%v/%s", entry.PublishedPort, entry.TargetPort, entry.Protocol, v.PublishedPort, v.TargetPort, v.Protocol)
-				}
-				portSet[portConfigToString(&entry)] = entry
-			}
-		}
-	}
+		ports := flags.Lookup(flagPublishAdd).Value.(*opts.PortOpt).Value()
 
-	if flags.Changed(flagPortAdd) {
-		for _, entry := range flags.Lookup(flagPortAdd).Value.(*opts.PortOpt).Value() {
-			if v, ok := portSet[portConfigToString(&entry)]; ok && v != entry {
-				return fmt.Errorf("conflicting port mapping between %v:%v/%s and %v:%v/%s", entry.PublishedPort, entry.TargetPort, entry.Protocol, v.PublishedPort, v.TargetPort, v.Protocol)
+		for _, port := range ports {
+			if v, ok := portSet[portConfigToString(&port)]; ok && v != port {
+				return fmt.Errorf("conflicting port mapping between %v:%v/%s and %v:%v/%s", port.PublishedPort, port.TargetPort, port.Protocol, v.PublishedPort, v.TargetPort, v.Protocol)
 			}
-			portSet[portConfigToString(&entry)] = entry
+			portSet[portConfigToString(&port)] = port
 		}
 	}
 
@@ -697,26 +682,12 @@ func updatePorts(flags *pflag.FlagSet, portConfig *[]swarm.PortConfig) error {
 		}
 	}
 
-	toRemove := flags.Lookup(flagPublishRemove).Value.(*opts.ListOpts).GetAll()
-	removePortCSV := flags.Lookup(flagPortRemove).Value.(*opts.ListOpts).GetAll()
-	removePortOpts := &opts.PortOpt{}
-	for _, portCSV := range removePortCSV {
-		if err := removePortOpts.Set(portCSV); err != nil {
-			return err
-		}
-	}
+	toRemove := flags.Lookup(flagPublishRemove).Value.(*opts.PortOpt).Value()
 
 	newPorts := []swarm.PortConfig{}
 portLoop:
 	for _, port := range portSet {
-		for _, rawTargetPort := range toRemove {
-			targetPort := nat.Port(rawTargetPort)
-			if equalPort(targetPort, port) {
-				continue portLoop
-			}
-		}
-
-		for _, pConfig := range removePortOpts.Value() {
+		for _, pConfig := range toRemove {
 			if equalProtocol(port.Protocol, pConfig.Protocol) &&
 				port.TargetPort == pConfig.TargetPort &&
 				equalPublishMode(port.PublishMode, pConfig.PublishMode) {

cli/command/service/update_test.go

@@ -364,6 +364,7 @@ func TestUpdatePortsRmWithProtocol(t *testing.T) {
 	assert.Equal(t, portConfigs[0].TargetPort, uint32(82))
 }
 
+// FIXME(vdemeester) port to opts.PortOpt
 func TestValidatePort(t *testing.T) {
 	validPorts := []string{"80/tcp", "80", "80/udp"}
 	invalidPorts := map[string]string{

cli/command/stack/deploy.go

@@ -21,7 +21,6 @@ import (
 	"github.com/docker/docker/api/types/swarm"
 	"github.com/docker/docker/cli"
 	"github.com/docker/docker/cli/command"
-	servicecmd "github.com/docker/docker/cli/command/service"
 	dockerclient "github.com/docker/docker/client"
 	"github.com/docker/docker/opts"
 	runconfigopts "github.com/docker/docker/runconfig/opts"
@@ -745,7 +744,7 @@ func convertEndpointSpec(source []string) (*swarm.EndpointSpec, error) {
 	for port := range ports {
 		portConfigs = append(
 			portConfigs,
-			servicecmd.ConvertPortToPortConfig(port, portBindings)...)
+			opts.ConvertPortToPortConfig(port, portBindings)...)
 	}
 
 	return &swarm.EndpointSpec{Ports: portConfigs}, nil

contrib/completion/bash/docker

@@ -2752,7 +2752,7 @@ _docker_service_update() {
 			--host
 			--mode
 			--name
-			--port
+			--publish
 			--secret
 		"
 
@@ -2799,8 +2799,8 @@ _docker_service_update() {
 			--host-add
 			--host-rm
 			--image
-			--port-add
-			--port-rm
+			--publish-add
+			--publish-rm
 			--secret-add
 			--secret-rm
 		"

contrib/completion/zsh/_docker

@@ -1795,7 +1795,7 @@ __docker_service_subcommand() {
                 "($help)*--env-file=[Read environment variables from a file]:environment file:_files" \
                 "($help)--mode=[Service Mode]:mode:(global replicated)" \
                 "($help)--name=[Service name]:name: " \
-                "($help)*--port=[Publish a port]:port: " \
+                "($help)*--publish=[Publish a port]:port: " \
                 "($help -): :__docker_complete_images" \
                 "($help -):command: _command_names -e" \
                 "($help -)*::arguments: _normal" && ret=0
@@ -1868,8 +1868,8 @@ __docker_service_subcommand() {
                 "($help)*--group-add=[Add additional supplementary user groups to the container]:group:_groups" \
                 "($help)*--group-rm=[Remove previously added supplementary user groups from the container]:group:_groups" \
                 "($help)--image=[Service image tag]:image:__docker_complete_repositories" \
-                "($help)*--port-add=[Add or update a port]:port: " \
-                "($help)*--port-rm=[Remove a port(target-port mandatory)]:port: " \
+                "($help)*--publish-add=[Add or update a port]:port: " \
+                "($help)*--publish-rm=[Remove a port(target-port mandatory)]:port: " \
                 "($help)--rollback[Rollback to previous specification]" \
                 "($help -)1:service:__docker_complete_services" && ret=0
             ;;

integration-cli/docker_cli_swarm_test.go

@@ -235,23 +235,51 @@ func (s *DockerSwarmSuite) TestSwarmNodeTaskListFilter(c *check.C) {
 func (s *DockerSwarmSuite) TestSwarmPublishAdd(c *check.C) {
 	d := s.AddDaemon(c, true, true)
 
-	name := "top"
-	out, err := d.Cmd("service", "create", "--name", name, "--label", "x=y", "busybox", "top")
-	c.Assert(err, checker.IsNil)
-	c.Assert(strings.TrimSpace(out), checker.Not(checker.Equals), "")
+	testCases := []struct {
+		name       string
+		publishAdd []string
+		ports      string
+	}{
+		{
+			name: "simple-syntax",
+			publishAdd: []string{
+				"80:80",
+				"80:80",
+				"80:80",
+				"80:20",
+			},
+			ports: "[{ tcp 80 80 ingress}]",
+		},
+		{
+			name: "complex-syntax",
+			publishAdd: []string{
+				"target=90,published=90,protocol=tcp,mode=ingress",
+				"target=90,published=90,protocol=tcp,mode=ingress",
+				"target=90,published=90,protocol=tcp,mode=ingress",
+				"target=30,published=90,protocol=tcp,mode=ingress",
+			},
+			ports: "[{ tcp 90 90 ingress}]",
+		},
+	}
 
-	out, err = d.Cmd("service", "update", "--publish-add", "80:80", name)
-	c.Assert(err, checker.IsNil)
+	for _, tc := range testCases {
+		out, err := d.Cmd("service", "create", "--name", tc.name, "--label", "x=y", "busybox", "top")
+		c.Assert(err, checker.IsNil, check.Commentf(out))
+		c.Assert(strings.TrimSpace(out), checker.Not(checker.Equals), "")
 
-	out, err = d.CmdRetryOutOfSequence("service", "update", "--publish-add", "80:80", name)
-	c.Assert(err, checker.IsNil)
+		out, err = d.CmdRetryOutOfSequence("service", "update", "--publish-add", tc.publishAdd[0], tc.name)
+		c.Assert(err, checker.IsNil, check.Commentf(out))
 
-	out, err = d.CmdRetryOutOfSequence("service", "update", "--publish-add", "80:80", "--publish-add", "80:20", name)
-	c.Assert(err, checker.NotNil)
+		out, err = d.CmdRetryOutOfSequence("service", "update", "--publish-add", tc.publishAdd[1], tc.name)
+		c.Assert(err, checker.IsNil, check.Commentf(out))
 
-	out, err = d.Cmd("service", "inspect", "--format", "{{ .Spec.EndpointSpec.Ports }}", name)
-	c.Assert(err, checker.IsNil)
-	c.Assert(strings.TrimSpace(out), checker.Equals, "[{ tcp 80 80 ingress}]")
+		out, err = d.CmdRetryOutOfSequence("service", "update", "--publish-add", tc.publishAdd[2], "--publish-add", tc.publishAdd[3], tc.name)
+		c.Assert(err, checker.NotNil, check.Commentf(out))
+
+		out, err = d.Cmd("service", "inspect", "--format", "{{ .Spec.EndpointSpec.Ports }}", tc.name)
+		c.Assert(err, checker.IsNil)
+		c.Assert(strings.TrimSpace(out), checker.Equals, tc.ports)
+	}
 }
 
 func (s *DockerSwarmSuite) TestSwarmServiceWithGroup(c *check.C) {