Merge pull request #29819 from vdemeester/service-validate-publish-simple-syntax

Make sure we validate simple syntax on service commands

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
 //     <host>:<ip>
 // into a swarmkit host format:

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:
-// <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

@@ -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
 }

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, portBindingMap, err := nat.ParsePortSpecs([]string{value})
-		ports, portBindings, _ := 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
 			}

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 {