Return error for incorrect argument of `service update --publish-rm <TargetPort>`

Currently `--publish-rm` only accepts `<TargetPort>` or `<TargetPort>[/Protocol]`
though there are some confusions.

Since `--publish-add` accepts `<PublishedPort>:<TargetPort>[/Protocol]`, some user
may provide `--publish-rm 80:80`. However, there is no error checking so the incorrect
provided argument is ignored silently.

This fix adds the check to make sure `--publish-rm` only accepts `<TargetPort>[/Protocol]`
and returns error if the format is invalid.

The `--publish-rm` itself may needs to be revisited to have a better UI/UX experience,
see discussions on:
https://github.com/docker/swarmkit/issues/1396
https://github.com/docker/docker/issues/25200#issuecomment-236213242
https://github.com/docker/docker/issues/25338#issuecomment-240787002

This fix is short term measure so that end users are not misled by the silently ignored error
of `--publish-rm`.

This fix is related to (but is not a complete fix):
https://github.com/docker/swarmkit/issues/1396

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

cli/command/service/update.go

@@ -47,7 +47,7 @@ func newUpdateCommand(dockerCli *command.DockerCli) *cobra.Command {
 	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(), flagPublishRemove, "Remove a published port by its target port")
+	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(), flagConstraintRemove, "Remove a constraint")
@@ -645,6 +645,23 @@ func portConfigToString(portConfig *swarm.PortConfig) string {
 	return fmt.Sprintf("%v:%v/%s/%s", portConfig.PublishedPort, portConfig.TargetPort, protocol, mode)
 }
 
+// This validation is only used for `--publish-rm`.
+// The `--publish-rm` takes:
+// <TargetPort>[/<Protocol>] (e.g., 80, 80/tcp, 53/udp)
+func validatePublishRemove(val string) (string, error) {
+	proto, port := nat.SplitProtoPort(val)
+	if proto != "tcp" && proto != "udp" {
+		return "", fmt.Errorf("invalid protocol '%s' for %s", proto, val)
+	}
+	if strings.Contains(port, ":") {
+		return "", fmt.Errorf("invalid port format: '%s', should be <TargetPort>[/<Protocol>] (e.g., 80, 80/tcp, 53/udp)", port)
+	}
+	if _, err := nat.ParsePort(port); err != nil {
+		return "", err
+	}
+	return val, nil
+}
+
 func updatePorts(flags *pflag.FlagSet, portConfig *[]swarm.PortConfig) error {
 	// The key of the map is `port/protocol`, e.g., `80/tcp`
 	portSet := map[string]swarm.PortConfig{}

cli/command/service/update_test.go

@@ -345,3 +345,43 @@ func TestUpdateHosts(t *testing.T) {
 	assert.Equal(t, hosts[1], "2001:db8:abc8::1 ipv6.net")
 	assert.Equal(t, hosts[2], "4.3.2.1 example.org")
 }
+
+func TestUpdatePortsRmWithProtocol(t *testing.T) {
+	flags := newUpdateCommand(nil).Flags()
+	flags.Set("publish-add", "8081:81")
+	flags.Set("publish-add", "8082:82")
+	flags.Set("publish-rm", "80")
+	flags.Set("publish-rm", "81/tcp")
+	flags.Set("publish-rm", "82/udp")
+
+	portConfigs := []swarm.PortConfig{
+		{TargetPort: 80, PublishedPort: 8080, Protocol: swarm.PortConfigProtocolTCP},
+	}
+
+	err := updatePorts(flags, &portConfigs)
+	assert.Equal(t, err, nil)
+	assert.Equal(t, len(portConfigs), 1)
+	assert.Equal(t, portConfigs[0].TargetPort, uint32(82))
+}
+
+func TestValidatePort(t *testing.T) {
+	validPorts := []string{"80/tcp", "80", "80/udp"}
+	invalidPorts := map[string]string{
+		"9999999":   "out of range",
+		"80:80/tcp": "invalid port format",
+		"53:53/udp": "invalid port format",
+		"80:80":     "invalid port format",
+		"80/xyz":    "invalid protocol",
+		"tcp":       "invalid syntax",
+		"udp":       "invalid syntax",
+		"":          "invalid protocol",
+	}
+	for _, port := range validPorts {
+		_, err := validatePublishRemove(port)
+		assert.Equal(t, err, nil)
+	}
+	for port, e := range invalidPorts {
+		_, err := validatePublishRemove(port)
+		assert.Error(t, err, e)
+	}
+}

opts/opts.go

@@ -108,6 +108,12 @@ func (opts *ListOpts) Type() string {
 	return "list"
 }
 
+// WithValidator returns the ListOpts with validator set.
+func (opts *ListOpts) WithValidator(validator ValidatorFctType) *ListOpts {
+	opts.validator = validator
+	return opts
+}
+
 // NamedOption is an interface that list and map options
 // with names implement.
 type NamedOption interface {