From aa4ecd153b88644a53b23b7c46332d9b50007930 Mon Sep 17 00:00:00 2001 From: Vincent Demeester Date: Mon, 6 Feb 2017 14:16:03 +0100 Subject: [PATCH] Make sure we validate simple syntax on service commands We ignored errors for simple syntax in `PortOpt` (missed that in the previous migration of this code). This make sure we don't ignore `nat.Parse` errors. Test has been migrate too (errors are not exactly the same as before though -_-) Signed-off-by: Vincent Demeester --- cli/command/service/opts.go | 12 ---------- cli/command/service/update.go | 18 --------------- cli/command/service/update_test.go | 23 ------------------- opts/port.go | 15 ++++++++++--- opts/port_test.go | 36 ++++++++++++++++++++++++++++++ 5 files changed, 48 insertions(+), 56 deletions(-) diff --git a/cli/command/service/opts.go b/cli/command/service/opts.go index f2470673a6..9a0ae64ca9 100644 --- a/cli/command/service/opts.go +++ b/cli/command/service/opts.go @@ -10,7 +10,6 @@ import ( "github.com/docker/docker/api/types/swarm" "github.com/docker/docker/opts" runconfigopts "github.com/docker/docker/runconfig/opts" - "github.com/docker/go-connections/nat" "github.com/spf13/cobra" ) @@ -244,17 +243,6 @@ func (opts *healthCheckOptions) toHealthConfig() (*container.HealthConfig, error return healthConfig, nil } -// ValidatePort validates a string is in the expected format for a port definition -func ValidatePort(value string) (string, error) { - portMappings, err := nat.ParsePortSpec(value) - for _, portMapping := range portMappings { - if portMapping.Binding.HostIP != "" { - return "", fmt.Errorf("HostIP is not supported by a service.") - } - } - return value, err -} - // convertExtraHostsToSwarmHosts converts an array of extra hosts in cli // : // into a swarmkit host format: diff --git a/cli/command/service/update.go b/cli/command/service/update.go index 57a4577f88..7f461c90a9 100644 --- a/cli/command/service/update.go +++ b/cli/command/service/update.go @@ -663,24 +663,6 @@ 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: -// [/] (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 [/] (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 992ae9ef3b..f2887e229d 100644 --- a/cli/command/service/update_test.go +++ b/cli/command/service/update_test.go @@ -362,29 +362,6 @@ func TestUpdatePortsRmWithProtocol(t *testing.T) { assert.Equal(t, portConfigs[1].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{ - "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) - } -} - type secretAPIClientMock struct { listResult []swarm.Secret } diff --git a/opts/port.go b/opts/port.go index 531908fb24..152683c981 100644 --- a/opts/port.go +++ b/opts/port.go @@ -94,11 +94,20 @@ func (p *PortOpt) Set(value string) error { } else { // short syntax portConfigs := []swarm.PortConfig{} - // We can ignore errors because the format was already validated by ValidatePort - ports, portBindings, _ := nat.ParsePortSpecs([]string{value}) + ports, portBindingMap, err := nat.ParsePortSpecs([]string{value}) + if err != nil { + return err + } + for _, portBindings := range portBindingMap { + for _, portBinding := range portBindings { + if portBinding.HostIP != "" { + return fmt.Errorf("HostIP is not supported.") + } + } + } for port := range ports { - portConfig, err := ConvertPortToPortConfig(port, portBindings) + portConfig, err := ConvertPortToPortConfig(port, portBindingMap) if err != nil { return err } diff --git a/opts/port_test.go b/opts/port_test.go index 67bcf8f1d9..a483d269aa 100644 --- a/opts/port_test.go +++ b/opts/port_test.go @@ -245,6 +245,42 @@ func TestPortOptInvalidComplexSyntax(t *testing.T) { } } +func TestPortOptInvalidSimpleSyntax(t *testing.T) { + testCases := []struct { + value string + expectedError string + }{ + { + value: "9999999", + expectedError: "Invalid containerPort: 9999999", + }, + { + value: "80/xyz", + expectedError: "Invalid proto: xyz", + }, + { + value: "tcp", + expectedError: "Invalid containerPort: tcp", + }, + { + value: "udp", + expectedError: "Invalid containerPort: udp", + }, + { + value: "", + expectedError: "No port specified", + }, + { + value: "1.1.1.1:80:80", + expectedError: "HostIP is not supported", + }, + } + for _, tc := range testCases { + var port PortOpt + assert.Error(t, port.Set(tc.value), tc.expectedError) + } +} + func assertContains(t *testing.T, portConfigs []swarm.PortConfig, expected swarm.PortConfig) { var contains = false for _, portConfig := range portConfigs {