Add validation of Ports

Also do caseless compares for "enum" strings.

pkg/api/types.go

@@ -87,7 +87,7 @@ type VolumeMount struct {
 	// Exactly one of the following must be set.  If both are set, prefer MountPath.
 	// DEPRECATED: Path will be removed in a future version of the API.
 	MountPath string `yaml:"mountPath,omitempty" json:"mountPath,omitempty"`
-	Path string `yaml:"Path,omitempty" json:"Path,omitempty"`
+	Path string `yaml:"path,omitempty" json:"path,omitempty"`
 	// One of: "LOCAL" (local volume) or "HOST" (external mount from the host). Default: LOCAL.
 	MountType string `yaml:"mountType,omitempty" json:"mountType,omitempty"`
 }

pkg/api/validation.go

@@ -18,15 +18,12 @@ package api
 
 import (
 	"fmt"
+	"strings"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
 	"github.com/golang/glog"
 )
 
-var (
-	supportedManifestVersions = util.NewStringSet("v1beta1", "v1beta2")
-)
-
 // ValidationErrorEnum is a type of validation error.
 type ValidationErrorEnum string
 
@@ -81,6 +78,38 @@ func validateVolumes(volumes []Volume) (util.StringSet, error) {
 	return allNames, nil
 }
 
+var supportedPortProtocols util.StringSet = util.NewStringSet("TCP", "UDP")
+
+func validatePorts(ports []Port) error {
+	allNames := util.StringSet{}
+	for i := range ports {
+		port := &ports[i] // so we can set default values
+		if len(port.Name) > 0 {
+			if len(port.Name) > 63 || !util.IsDNSLabel(port.Name) {
+				return makeInvalidError("Port.Name", port.Name)
+			}
+			if allNames.Has(port.Name) {
+				return makeDuplicateError("Port.name", port.Name)
+			}
+			allNames.Insert(port.Name)
+		}
+		if !util.IsValidPortNum(port.ContainerPort) {
+			return makeInvalidError("Port.ContainerPort", port.ContainerPort)
+		}
+		if port.HostPort == 0 {
+			port.HostPort = port.ContainerPort
+		} else if !util.IsValidPortNum(port.HostPort) {
+			return makeInvalidError("Port.HostPort", port.HostPort)
+		}
+		if len(port.Protocol) == 0 {
+			port.Protocol = "TCP"
+		} else if !supportedPortProtocols.Has(strings.ToUpper(port.Protocol)) {
+			return makeNotSupportedError("Port.Protocol", port.Protocol)
+		}
+	}
+	return nil
+}
+
 func validateEnv(vars []EnvVar) error {
 	for i := range vars {
 		ev := &vars[i] // so we can set default values
@@ -122,6 +151,28 @@ func validateVolumeMounts(mounts []VolumeMount, volumes util.StringSet) error {
 	return nil
 }
 
+// AccumulateUniquePorts runs an extraction function on each Port of each Container,
+// accumulating the results and returning an error if any ports conflict.
+func AccumulateUniquePorts(containers []Container, accumulator map[int]bool, extract func(*Port) int) error {
+	for ci := range containers {
+		ctr := &containers[ci]
+		for pi := range ctr.Ports {
+			port := extract(&ctr.Ports[pi])
+			if accumulator[port] {
+				return makeDuplicateError("Port", port)
+			}
+			accumulator[port] = true
+		}
+	}
+	return nil
+}
+
+// Checks for colliding Port.HostPort values across a slice of containers.
+func checkHostPortConflicts(containers []Container) error {
+	allPorts := map[int]bool{}
+	return AccumulateUniquePorts(containers, allPorts, func(p *Port) int { return p.HostPort })
+}
+
 func validateContainers(containers []Container, volumes util.StringSet) error {
 	allNames := util.StringSet{}
 	for i := range containers {
@@ -136,17 +187,26 @@ func validateContainers(containers []Container, volumes util.StringSet) error {
 		if len(ctr.Image) == 0 {
 			return makeInvalidError("Container.Image", ctr.Name)
 		}
+		if err := validatePorts(ctr.Ports); err != nil {
+			return err
+		}
 		if err := validateEnv(ctr.Env); err != nil {
 			return err
 		}
 		if err := validateVolumeMounts(ctr.VolumeMounts, volumes); err != nil {
 			return err
 		}
-		// TODO(thockin): finish validation.
 	}
-	return nil
+	// Check for colliding ports across all containers.
+	// TODO(thockin): This really is dependent on the network config of the host (IP per pod?)
+	// and the config of the new manifest.  But we have not specced that out yet, so we'll just
+	// make some assumptions for now.  As of now, pods share a network namespace, which means that
+	// every Port.HostPort across the whole pod must be unique.
+	return checkHostPortConflicts(containers)
 }
 
+var supportedManifestVersions util.StringSet = util.NewStringSet("v1beta1", "v1beta2")
+
 // ValidateManifest tests that the specified ContainerManifest has valid data.
 // This includes checking formatting and uniqueness.  It also canonicalizes the
 // structure by setting default values and implementing any backwards-compatibility
@@ -156,7 +216,7 @@ func ValidateManifest(manifest *ContainerManifest) error {
 	if len(manifest.Version) == 0 {
 		return makeInvalidError("ContainerManifest.Version", manifest.Version)
 	}
-	if !supportedManifestVersions.Has(manifest.Version) {
+	if !supportedManifestVersions.Has(strings.ToLower(manifest.Version)) {
 		return makeNotSupportedError("ContainerManifest.Version", manifest.Version)
 	}
 	if len(manifest.ID) > 255 || !util.IsDNSSubdomain(manifest.ID) {

pkg/api/validation_test.go

@@ -50,6 +50,52 @@ func TestValidateVolumes(t *testing.T) {
 	}
 }
 
+func TestValidatePorts(t *testing.T) {
+	successCase := []Port{
+		{Name: "abc", ContainerPort: 80, HostPort: 80, Protocol: "TCP"},
+		{Name: "123", ContainerPort: 81, HostPort: 81},
+		{Name: "easy", ContainerPort: 82, Protocol: "TCP"},
+		{Name: "as", ContainerPort: 83, Protocol: "UDP"},
+		{Name: "do-re-me", ContainerPort: 84},
+		{Name: "baby-you-and-me", ContainerPort: 82, Protocol: "tcp"},
+		{ContainerPort: 85},
+	}
+	err := validatePorts(successCase)
+	if err != nil {
+		t.Errorf("expected success: %v", err)
+	}
+
+	nonCanonicalCase := []Port{
+		{ContainerPort: 80},
+	}
+	err = validatePorts(nonCanonicalCase)
+	if err != nil {
+		t.Errorf("expected success: %v", err)
+	}
+	if nonCanonicalCase[0].HostPort != 80 || nonCanonicalCase[0].Protocol != "TCP" {
+		t.Errorf("expected default values: %+v", nonCanonicalCase[0])
+	}
+
+	errorCases := map[string][]Port{
+		"name > 63 characters": {{Name: strings.Repeat("a", 64), ContainerPort: 80}},
+		"name not a DNS label": {{Name: "a.b.c", ContainerPort: 80}},
+		"name not unique": {
+			{Name: "abc", ContainerPort: 80},
+			{Name: "abc", ContainerPort: 81},
+		},
+		"zero container port":    {{ContainerPort: 0}},
+		"invalid container port": {{ContainerPort: 65536}},
+		"invalid host port":      {{ContainerPort: 80, HostPort: 65536}},
+		"invalid protocol":       {{ContainerPort: 80, Protocol: "ICMP"}},
+	}
+	for k, v := range errorCases {
+		err := validatePorts(v)
+		if err == nil {
+			t.Errorf("expected failure for %s", k)
+		}
+	}
+}
+
 func TestValidateEnv(t *testing.T) {
 	successCase := []EnvVar{
 		{Name: "abc", Value: "value"},
@@ -139,6 +185,10 @@ func TestValidateContainers(t *testing.T) {
 			{Name: "abc", Image: "image"},
 		},
 		"zero-length image": {{Name: "abc", Image: ""}},
+		"host port not unique": {
+			{Name: "abc", Image: "image", Ports: []Port{{ContainerPort: 80, HostPort: 80}}},
+			{Name: "def", Image: "image", Ports: []Port{{ContainerPort: 81, HostPort: 80}}},
+		},
 		"invalid env var name": {
 			{Name: "abc", Image: "image", Env: []EnvVar{{Name: "ev.1"}}},
 		},
@@ -156,8 +206,8 @@ func TestValidateContainers(t *testing.T) {
 func TestValidateManifest(t *testing.T) {
 	successCases := []ContainerManifest{
 		{Version: "v1beta1", ID: "abc"},
-		{Version: "v1beta1", ID: "123"},
+		{Version: "v1beta2", ID: "123"},
-		{Version: "v1beta1", ID: "abc.123.do-re-mi"},
+		{Version: "V1BETA1", ID: "abc.123.do-re-mi"},
 		{
 			Version: "v1beta1",
 			ID:      "abc",
@@ -170,6 +220,11 @@ func TestValidateManifest(t *testing.T) {
 					WorkingDir: "/tmp",
 					Memory:     1,
 					CPU:        1,
+					Ports: []Port{
+						{Name: "p1", ContainerPort: 80, HostPort: 8080},
+						{Name: "p2", ContainerPort: 81},
+						{ContainerPort: 82},
+					},
 					Env: []EnvVar{
 						{Name: "ev1", Value: "val1"},
 						{Name: "ev2", Value: "val2"},

pkg/kubelet/kubelet.go

@@ -310,15 +310,13 @@ func makePortsAndBindings(container *api.Container) (map[docker.Port]struct{}, m
 		// Some of this port stuff is under-documented voodoo.
 		// See http://stackoverflow.com/questions/20428302/binding-a-port-to-a-host-interface-using-the-rest-api
 		var protocol string
-		switch port.Protocol {
+		switch strings.ToUpper(port.Protocol) {
-		case "udp":
+		case "UDP":
 			protocol = "/udp"
-		case "tcp":
+		case "TCP":
 			protocol = "/tcp"
 		default:
-			if len(port.Protocol) != 0 {
+			glog.Infof("Unknown protocol '%s': defaulting to TCP", port.Protocol)
-				glog.Infof("Unknown protocol: %s, defaulting to tcp.", port.Protocol)
-			}
 			protocol = "/tcp"
 		}
 		dockerPort := docker.Port(strconv.Itoa(interiorPort) + protocol)