diff --git a/pkg/api/types.go b/pkg/api/types.go index 884eda145a5..1fa2a5c1448 100644 --- a/pkg/api/types.go +++ b/pkg/api/types.go @@ -83,7 +83,10 @@ type VolumeMount struct { // Optional: Defaults to false (read-write). ReadOnly bool `yaml:"readOnly,omitempty" json:"readOnly,omitempty"` // Required. + // 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"` // One of: "LOCAL" (local volume) or "HOST" (external mount from the host). Default: LOCAL. MountType string `yaml:"mountType,omitempty" json:"mountType,omitempty"` } diff --git a/pkg/api/validation.go b/pkg/api/validation.go index 33f8270221f..8d1fdef085e 100644 --- a/pkg/api/validation.go +++ b/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 @@ -35,6 +32,7 @@ const ( ErrTypeInvalid ValidationErrorEnum = "invalid value" ErrTypeNotSupported ValidationErrorEnum = "unsupported value" ErrTypeDuplicate ValidationErrorEnum = "duplicate value" + ErrTypeNotFound ValidationErrorEnum = "not found" ) // ValidationError is an implementation of the 'error' interface, which represents an error of validation. @@ -61,6 +59,10 @@ func makeDuplicateError(field string, value interface{}) ValidationError { return ValidationError{ErrTypeDuplicate, field, value} } +func makeNotFoundError(field string, value interface{}) ValidationError { + return ValidationError{ErrTypeNotFound, field, value} +} + func validateVolumes(volumes []Volume) (util.StringSet, error) { allNames := util.StringSet{} for i := range volumes { @@ -76,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 @@ -95,6 +129,50 @@ func validateEnv(vars []EnvVar) error { return nil } +func validateVolumeMounts(mounts []VolumeMount, volumes util.StringSet) error { + for i := range mounts { + mnt := &mounts[i] // so we can set default values + if len(mnt.Name) == 0 { + return makeInvalidError("VolumeMount.Name", mnt.Name) + } + if !volumes.Has(mnt.Name) { + return makeNotFoundError("VolumeMount.Name", mnt.Name) + } + if len(mnt.MountPath) == 0 { + // Backwards compat. + if len(mnt.Path) == 0 { + return makeInvalidError("VolumeMount.MountPath", mnt.MountPath) + } + glog.Warning("DEPRECATED: VolumeMount.Path has been replaced by VolumeMount.MountPath") + mnt.MountPath = mnt.Path + mnt.Path = "" + } + } + 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 { @@ -109,15 +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 } - - // TODO(thockin): finish validation. + if err := validateVolumeMounts(ctr.VolumeMounts, volumes); err != nil { + return err + } } - 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 @@ -127,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 !util.IsDNSSubdomain(manifest.ID) { diff --git a/pkg/api/validation_test.go b/pkg/api/validation_test.go index 52b2c24c6a2..26d2eea7477 100644 --- a/pkg/api/validation_test.go +++ b/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"}, @@ -82,6 +128,42 @@ func TestValidateEnv(t *testing.T) { } } +func TestValidateVolumeMounts(t *testing.T) { + volumes := util.NewStringSet("abc", "123", "abc-123") + + successCase := []VolumeMount{ + {Name: "abc", MountPath: "/foo"}, + {Name: "123", MountPath: "/foo"}, + {Name: "abc-123", MountPath: "/bar"}, + } + if err := validateVolumeMounts(successCase, volumes); err != nil { + t.Errorf("expected success: %v", err) + } + + nonCanonicalCase := []VolumeMount{ + {Name: "abc", Path: "/foo"}, + } + err := validateVolumeMounts(nonCanonicalCase, volumes) + if err != nil { + t.Errorf("expected success: %v", err) + } + if nonCanonicalCase[0].MountPath != "/foo" { + t.Errorf("expected canonicalized values: %+v", nonCanonicalCase[0]) + } + + errorCases := map[string][]VolumeMount{ + "empty name": {{Name: "", MountPath: "/foo"}}, + "name not found": {{Name: "", MountPath: "/foo"}}, + "empty mountpath": {{Name: "abc", MountPath: ""}}, + } + for k, v := range errorCases { + err := validateVolumeMounts(v, volumes) + if err == nil { + t.Errorf("expected failure for %s", k) + } + } +} + func TestValidateContainers(t *testing.T) { volumes := util.StringSet{} @@ -103,9 +185,16 @@ 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"}}}, }, + "unknown volume name": { + {Name: "abc", Image: "image", VolumeMounts: []VolumeMount{{Name: "anything", MountPath: "/foo"}}}, + }, } for k, v := range errorCases { if err := validateContainers(v, volumes); err == nil { @@ -117,8 +206,8 @@ func TestValidateContainers(t *testing.T) { func TestValidateManifest(t *testing.T) { successCases := []ContainerManifest{ {Version: "v1beta1", ID: "abc"}, - {Version: "v1beta1", ID: "123"}, - {Version: "v1beta1", ID: "abc.123.do-re-mi"}, + {Version: "v1beta2", ID: "123"}, + {Version: "V1BETA1", ID: "abc.123.do-re-mi"}, { Version: "v1beta1", ID: "abc", @@ -131,11 +220,20 @@ 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"}, {Key: "EV3", Value: "val3"}, }, + VolumeMounts: []VolumeMount{ + {Name: "vol1", MountPath: "/foo"}, + {Name: "vol1", Path: "/bar"}, + }, }, }, }, diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go index 377dec334bd..296734315ef 100644 --- a/pkg/kubelet/kubelet.go +++ b/pkg/kubelet/kubelet.go @@ -318,15 +318,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 { - case "udp": + switch strings.ToUpper(port.Protocol) { + 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) @@ -770,6 +768,20 @@ func (kl *Kubelet) SyncManifests(config []api.ContainerManifest) error { return err } +// Check that all Port.HostPort values are unique across all manifests. +func checkHostPortConflicts(allManifests []api.ContainerManifest, newManifest *api.ContainerManifest) error { + allPorts := map[int]bool{} + extract := func(p *api.Port) int { return p.HostPort } + for i := range allManifests { + manifest := &allManifests[i] + err := api.AccumulateUniquePorts(manifest.Containers, allPorts, extract) + if err != nil { + return err + } + } + return api.AccumulateUniquePorts(newManifest.Containers, allPorts, extract) +} + // syncLoop is the main loop for processing changes. It watches for changes from // four channels (file, etcd, server, and http) and creates a union of them. For // any new change seen, will run a sync against desired state and running state. If @@ -787,14 +799,26 @@ func (kl *Kubelet) syncLoop(updateChannel <-chan manifestUpdate, handler SyncHan } allManifests := []api.ContainerManifest{} + allIds := util.StringSet{} for src, srcManifests := range last { for i := range srcManifests { m := &srcManifests[i] + if allIds.Has(m.ID) { + glog.Warningf("Manifest from %s has duplicate ID, ignoring: %v", src, m.ID) + continue + } + allIds.Insert(m.ID) if err := api.ValidateManifest(m); err != nil { glog.Warningf("Manifest from %s failed validation, ignoring: %v", src, err) continue } + // Check for host-wide HostPort conflicts. + if err := checkHostPortConflicts(allManifests, m); err != nil { + glog.Warningf("Manifest from %s failed validation, ignoring: %v", src, err) + continue + } } + // TODO(thockin): There's no reason to collect manifests by value. Don't pessimize. allManifests = append(allManifests, srcManifests...) } diff --git a/pkg/kubelet/kubelet_test.go b/pkg/kubelet/kubelet_test.go index f415ff933b7..5484e2265e1 100644 --- a/pkg/kubelet/kubelet_test.go +++ b/pkg/kubelet/kubelet_test.go @@ -572,7 +572,32 @@ func TestMakePortsAndBindings(t *testing.T) { } } } +} +func TestCheckHostPortConflicts(t *testing.T) { + successCaseAll := []api.ContainerManifest{ + {Containers: []api.Container{{Ports: []api.Port{{HostPort: 80}}}}}, + {Containers: []api.Container{{Ports: []api.Port{{HostPort: 81}}}}}, + {Containers: []api.Container{{Ports: []api.Port{{HostPort: 82}}}}}, + } + successCaseNew := api.ContainerManifest{ + Containers: []api.Container{{Ports: []api.Port{{HostPort: 83}}}}, + } + if err := checkHostPortConflicts(successCaseAll, &successCaseNew); err != nil { + t.Errorf("Expected success: %v", err) + } + + failureCaseAll := []api.ContainerManifest{ + {Containers: []api.Container{{Ports: []api.Port{{HostPort: 80}}}}}, + {Containers: []api.Container{{Ports: []api.Port{{HostPort: 81}}}}}, + {Containers: []api.Container{{Ports: []api.Port{{HostPort: 82}}}}}, + } + failureCaseNew := api.ContainerManifest{ + Containers: []api.Container{{Ports: []api.Port{{HostPort: 81}}}}, + } + if err := checkHostPortConflicts(failureCaseAll, &failureCaseNew); err == nil { + t.Errorf("Expected failure") + } } func TestExtractFromNonExistentFile(t *testing.T) { @@ -597,7 +622,6 @@ func TestExtractFromBadDataFile(t *testing.T) { if err == nil { t.Error("Unexpected non-error.") } - } func TestExtractFromValidDataFile(t *testing.T) {