From c4d773cdfe94a8ba4862a0f8df237b0fc31d2337 Mon Sep 17 00:00:00 2001
From: Yong Tang <yong.tang.github@outlook.com>
Date: Thu, 18 Aug 2016 18:09:07 -0700
Subject: [PATCH] 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 | 19 +++++++++++++-
cli/command/service/update_test.go | 40 ++++++++++++++++++++++++++++++
opts/opts.go | 6 +++++
3 files changed, 64 insertions(+), 1 deletion(-)
diff --git a/cli/command/service/update.go b/cli/command/service/update.go
index 92329d1439..200f58c3a6 100644
--- a/cli/command/service/update.go
+++ b/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{}
diff --git a/cli/command/service/update_test.go b/cli/command/service/update_test.go
index 998d06d3bd..bb2e9c1073 100644
--- a/cli/command/service/update_test.go
+++ b/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)
+ }
+}
diff --git a/opts/opts.go b/opts/opts.go
index ae851537ec..9f66c039e7 100644
--- a/opts/opts.go
+++ b/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 {