From 54790080b325b68d84bfcf41970249948450c21b Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Mon, 7 Jul 2014 23:20:30 -0700 Subject: [PATCH 1/2] Accumulate validation errors Rather than report the first error, accumulate all errors and report them all at once. --- pkg/api/validation.go | 149 ++++++++++++++++++++---------------- pkg/api/validation_test.go | 55 ++++++------- pkg/kubelet/kubelet.go | 36 +++++---- pkg/kubelet/kubelet_test.go | 6 +- 4 files changed, 133 insertions(+), 113 deletions(-) diff --git a/pkg/api/validation.go b/pkg/api/validation.go index 8d1fdef085e..1488128408d 100644 --- a/pkg/api/validation.go +++ b/pkg/api/validation.go @@ -63,138 +63,152 @@ func makeNotFoundError(field string, value interface{}) ValidationError { return ValidationError{ErrTypeNotFound, field, value} } -func validateVolumes(volumes []Volume) (util.StringSet, error) { +func validateVolumes(volumes []Volume) (util.StringSet, []error) { + allErrs := []error{} + allNames := util.StringSet{} for i := range volumes { vol := &volumes[i] // so we can set default values if !util.IsDNSLabel(vol.Name) { - return util.StringSet{}, makeInvalidError("Volume.Name", vol.Name) + allErrs = append(allErrs, makeInvalidError("Volume.Name", vol.Name)) + } else if allNames.Has(vol.Name) { + allErrs = append(allErrs, makeDuplicateError("Volume.Name", vol.Name)) + } else { + allNames.Insert(vol.Name) } - if allNames.Has(vol.Name) { - return util.StringSet{}, makeDuplicateError("Volume.Name", vol.Name) - } - allNames.Insert(vol.Name) } - return allNames, nil + return allNames, allErrs } var supportedPortProtocols util.StringSet = util.NewStringSet("TCP", "UDP") -func validatePorts(ports []Port) error { +func validatePorts(ports []Port) []error { + allErrs := []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) + allErrs = append(allErrs, makeInvalidError("Port.Name", port.Name)) + } else if allNames.Has(port.Name) { + allErrs = append(allErrs, makeDuplicateError("Port.name", port.Name)) + } else { + allNames.Insert(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) + allErrs = append(allErrs, 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) + allErrs = append(allErrs, 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) + allErrs = append(allErrs, makeNotSupportedError("Port.Protocol", port.Protocol)) } } - return nil + return allErrs } -func validateEnv(vars []EnvVar) error { +func validateEnv(vars []EnvVar) []error { + allErrs := []error{} + for i := range vars { ev := &vars[i] // so we can set default values if len(ev.Name) == 0 { // Backwards compat. if len(ev.Key) == 0 { - return makeInvalidError("EnvVar.Name", ev.Name) + allErrs = append(allErrs, makeInvalidError("EnvVar.Name", ev.Name)) + } else { + glog.Warning("DEPRECATED: EnvVar.Key has been replaced by EnvVar.Name") + ev.Name = ev.Key + ev.Key = "" } - glog.Warning("DEPRECATED: EnvVar.Key has been replaced by EnvVar.Name") - ev.Name = ev.Key - ev.Key = "" } if !util.IsCIdentifier(ev.Name) { - return makeInvalidError("EnvVar.Name", ev.Name) + allErrs = append(allErrs, makeInvalidError("EnvVar.Name", ev.Name)) } } - return nil + return allErrs } -func validateVolumeMounts(mounts []VolumeMount, volumes util.StringSet) error { +func validateVolumeMounts(mounts []VolumeMount, volumes util.StringSet) []error { + allErrs := []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) + allErrs = append(allErrs, makeInvalidError("VolumeMount.Name", mnt.Name)) + } else if !volumes.Has(mnt.Name) { + allErrs = append(allErrs, makeNotFoundError("VolumeMount.Name", mnt.Name)) } if len(mnt.MountPath) == 0 { // Backwards compat. if len(mnt.Path) == 0 { - return makeInvalidError("VolumeMount.MountPath", mnt.MountPath) + allErrs = append(allErrs, makeInvalidError("VolumeMount.MountPath", mnt.MountPath)) + } else { + glog.Warning("DEPRECATED: VolumeMount.Path has been replaced by VolumeMount.MountPath") + mnt.MountPath = mnt.Path + mnt.Path = "" } - glog.Warning("DEPRECATED: VolumeMount.Path has been replaced by VolumeMount.MountPath") - mnt.MountPath = mnt.Path - mnt.Path = "" } } - return nil + return allErrs } // 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 { +func AccumulateUniquePorts(containers []Container, accumulator map[int]bool, extract func(*Port) int) []error { + allErrs := []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) + allErrs = append(allErrs, makeDuplicateError("Port", port)) + } else { + accumulator[port] = true } - accumulator[port] = true } } - return nil + return allErrs } // Checks for colliding Port.HostPort values across a slice of containers. -func checkHostPortConflicts(containers []Container) error { +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 { +func validateContainers(containers []Container, volumes util.StringSet) []error { + allErrs := []error{} + allNames := util.StringSet{} for i := range containers { ctr := &containers[i] // so we can set default values if !util.IsDNSLabel(ctr.Name) { - return makeInvalidError("Container.Name", ctr.Name) + allErrs = append(allErrs, makeInvalidError("Container.Name", ctr.Name)) + } else if allNames.Has(ctr.Name) { + allErrs = append(allErrs, makeDuplicateError("Container.Name", ctr.Name)) + } else { + allNames.Insert(ctr.Name) } - if allNames.Has(ctr.Name) { - return makeDuplicateError("Container.Name", ctr.Name) - } - allNames.Insert(ctr.Name) if len(ctr.Image) == 0 { - return makeInvalidError("Container.Image", ctr.Name) + allErrs = append(allErrs, makeInvalidError("Container.Image", ctr.Name)) } - if err := validatePorts(ctr.Ports); err != nil { - return err + if errs := validatePorts(ctr.Ports); len(errs) != 0 { + allErrs = append(allErrs, errs...) } - if err := validateEnv(ctr.Env); err != nil { - return err + if errs := validateEnv(ctr.Env); len(errs) != 0 { + allErrs = append(allErrs, errs...) } - if err := validateVolumeMounts(ctr.VolumeMounts, volumes); err != nil { - return err + if errs := validateVolumeMounts(ctr.VolumeMounts, volumes); len(errs) != 0 { + allErrs = append(allErrs, errs...) } } // Check for colliding ports across all containers. @@ -202,7 +216,10 @@ func validateContainers(containers []Container, volumes util.StringSet) error { // 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) + if errs := checkHostPortConflicts(containers); len(errs) != 0 { + allErrs = append(allErrs, errs...) + } + return allErrs } var supportedManifestVersions util.StringSet = util.NewStringSet("v1beta1", "v1beta2") @@ -211,23 +228,23 @@ var supportedManifestVersions util.StringSet = util.NewStringSet("v1beta1", "v1b // This includes checking formatting and uniqueness. It also canonicalizes the // structure by setting default values and implementing any backwards-compatibility // tricks. -// TODO(thockin): We should probably collect all errors rather than aborting the validation. -func ValidateManifest(manifest *ContainerManifest) error { +func ValidateManifest(manifest *ContainerManifest) []error { + allErrs := []error{} + if len(manifest.Version) == 0 { - return makeInvalidError("ContainerManifest.Version", manifest.Version) - } - if !supportedManifestVersions.Has(strings.ToLower(manifest.Version)) { - return makeNotSupportedError("ContainerManifest.Version", manifest.Version) + allErrs = append(allErrs, makeInvalidError("ContainerManifest.Version", manifest.Version)) + } else if !supportedManifestVersions.Has(strings.ToLower(manifest.Version)) { + allErrs = append(allErrs, makeNotSupportedError("ContainerManifest.Version", manifest.Version)) } if !util.IsDNSSubdomain(manifest.ID) { - return makeInvalidError("ContainerManifest.ID", manifest.ID) + allErrs = append(allErrs, makeInvalidError("ContainerManifest.ID", manifest.ID)) } - allVolumes, err := validateVolumes(manifest.Volumes) - if err != nil { - return err + allVolumes, errs := validateVolumes(manifest.Volumes) + if len(errs) != 0 { + allErrs = append(allErrs, errs...) } - if err := validateContainers(manifest.Containers, allVolumes); err != nil { - return err + if errs := validateContainers(manifest.Containers, allVolumes); len(errs) != 0 { + allErrs = append(allErrs, errs...) } - return nil + return allErrs } diff --git a/pkg/api/validation_test.go b/pkg/api/validation_test.go index 26d2eea7477..34bf26bc1ee 100644 --- a/pkg/api/validation_test.go +++ b/pkg/api/validation_test.go @@ -29,9 +29,9 @@ func TestValidateVolumes(t *testing.T) { {Name: "123"}, {Name: "abc-123"}, } - names, err := validateVolumes(successCase) - if err != nil { - t.Errorf("expected success: %v", err) + names, errs := validateVolumes(successCase) + if len(errs) != 0 { + t.Errorf("expected success: %v", errs) } if len(names) != 3 || !names.Has("abc") || !names.Has("123") || !names.Has("abc-123") { t.Errorf("wrong names result: %v", names) @@ -44,7 +44,7 @@ func TestValidateVolumes(t *testing.T) { "name not unique": {{Name: "abc"}, {Name: "abc"}}, } for k, v := range errorCases { - if _, err := validateVolumes(v); err == nil { + if _, errs := validateVolumes(v); len(errs) == 0 { t.Errorf("expected failure for %s", k) } } @@ -60,17 +60,15 @@ func TestValidatePorts(t *testing.T) { {Name: "baby-you-and-me", ContainerPort: 82, Protocol: "tcp"}, {ContainerPort: 85}, } - err := validatePorts(successCase) - if err != nil { - t.Errorf("expected success: %v", err) + if errs := validatePorts(successCase); len(errs) != 0 { + t.Errorf("expected success: %v", errs) } nonCanonicalCase := []Port{ {ContainerPort: 80}, } - err = validatePorts(nonCanonicalCase) - if err != nil { - t.Errorf("expected success: %v", err) + if errs := validatePorts(nonCanonicalCase); len(errs) != 0 { + t.Errorf("expected success: %v", errs) } if nonCanonicalCase[0].HostPort != 80 || nonCanonicalCase[0].Protocol != "TCP" { t.Errorf("expected default values: %+v", nonCanonicalCase[0]) @@ -89,8 +87,7 @@ func TestValidatePorts(t *testing.T) { "invalid protocol": {{ContainerPort: 80, Protocol: "ICMP"}}, } for k, v := range errorCases { - err := validatePorts(v) - if err == nil { + if errs := validatePorts(v); len(errs) == 0 { t.Errorf("expected failure for %s", k) } } @@ -103,15 +100,15 @@ func TestValidateEnv(t *testing.T) { {Name: "AbC_123", Value: "value"}, {Name: "abc", Value: ""}, } - if err := validateEnv(successCase); err != nil { - t.Errorf("expected success: %v", err) + if errs := validateEnv(successCase); len(errs) != 0 { + t.Errorf("expected success: %v", errs) } nonCanonicalCase := []EnvVar{ {Key: "EV"}, } - if err := validateEnv(nonCanonicalCase); err != nil { - t.Errorf("expected success: %v", err) + if errs := validateEnv(nonCanonicalCase); len(errs) != 0 { + t.Errorf("expected success: %v", errs) } if nonCanonicalCase[0].Name != "EV" || nonCanonicalCase[0].Value != "" { t.Errorf("expected default values: %+v", nonCanonicalCase[0]) @@ -122,7 +119,7 @@ func TestValidateEnv(t *testing.T) { "name not a C identifier": {{Name: "a.b.c"}}, } for k, v := range errorCases { - if err := validateEnv(v); err == nil { + if errs := validateEnv(v); len(errs) == 0 { t.Errorf("expected failure for %s", k) } } @@ -136,16 +133,15 @@ func TestValidateVolumeMounts(t *testing.T) { {Name: "123", MountPath: "/foo"}, {Name: "abc-123", MountPath: "/bar"}, } - if err := validateVolumeMounts(successCase, volumes); err != nil { - t.Errorf("expected success: %v", err) + if errs := validateVolumeMounts(successCase, volumes); len(errs) != 0 { + t.Errorf("expected success: %v", errs) } nonCanonicalCase := []VolumeMount{ {Name: "abc", Path: "/foo"}, } - err := validateVolumeMounts(nonCanonicalCase, volumes) - if err != nil { - t.Errorf("expected success: %v", err) + if errs := validateVolumeMounts(nonCanonicalCase, volumes); len(errs) != 0 { + t.Errorf("expected success: %v", errs) } if nonCanonicalCase[0].MountPath != "/foo" { t.Errorf("expected canonicalized values: %+v", nonCanonicalCase[0]) @@ -157,8 +153,7 @@ func TestValidateVolumeMounts(t *testing.T) { "empty mountpath": {{Name: "abc", MountPath: ""}}, } for k, v := range errorCases { - err := validateVolumeMounts(v, volumes) - if err == nil { + if errs := validateVolumeMounts(v, volumes); len(errs) == 0 { t.Errorf("expected failure for %s", k) } } @@ -172,8 +167,8 @@ func TestValidateContainers(t *testing.T) { {Name: "123", Image: "image"}, {Name: "abc-123", Image: "image"}, } - if err := validateContainers(successCase, volumes); err != nil { - t.Errorf("expected success: %v", err) + if errs := validateContainers(successCase, volumes); len(errs) != 0 { + t.Errorf("expected success: %v", errs) } errorCases := map[string][]Container{ @@ -197,7 +192,7 @@ func TestValidateContainers(t *testing.T) { }, } for k, v := range errorCases { - if err := validateContainers(v, volumes); err == nil { + if errs := validateContainers(v, volumes); len(errs) == 0 { t.Errorf("expected failure for %s", k) } } @@ -239,8 +234,8 @@ func TestValidateManifest(t *testing.T) { }, } for _, manifest := range successCases { - if err := ValidateManifest(&manifest); err != nil { - t.Errorf("expected success: %v", err) + if errs := ValidateManifest(&manifest); len(errs) != 0 { + t.Errorf("expected success: %v", errs) } } @@ -262,7 +257,7 @@ func TestValidateManifest(t *testing.T) { }, } for k, v := range errorCases { - if err := ValidateManifest(&v); err == nil { + if errs := ValidateManifest(&v); len(errs) == 0 { t.Errorf("expected failure for %s", k) } } diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go index 296734315ef..ca50c5016c0 100644 --- a/pkg/kubelet/kubelet.go +++ b/pkg/kubelet/kubelet.go @@ -769,17 +769,22 @@ func (kl *Kubelet) SyncManifests(config []api.ContainerManifest) error { } // Check that all Port.HostPort values are unique across all manifests. -func checkHostPortConflicts(allManifests []api.ContainerManifest, newManifest *api.ContainerManifest) error { +func checkHostPortConflicts(allManifests []api.ContainerManifest, newManifest *api.ContainerManifest) []error { + allErrs := []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 + errs := api.AccumulateUniquePorts(manifest.Containers, allPorts, extract) + if len(errs) != 0 { + allErrs = append(allErrs, errs...) } } - return api.AccumulateUniquePorts(newManifest.Containers, allPorts, extract) + if errs := api.AccumulateUniquePorts(newManifest.Containers, allPorts, extract); len(errs) != 0 { + allErrs = append(allErrs, errs...) + } + return allErrs } // syncLoop is the main loop for processing changes. It watches for changes from @@ -802,20 +807,23 @@ func (kl *Kubelet) syncLoop(updateChannel <-chan manifestUpdate, handler SyncHan allIds := util.StringSet{} for src, srcManifests := range last { for i := range srcManifests { + allErrs := []error{} + m := &srcManifests[i] if allIds.Has(m.ID) { - glog.Warningf("Manifest from %s has duplicate ID, ignoring: %v", src, m.ID) - continue + allErrs = append(allErrs, api.ValidationError{api.ErrTypeDuplicate, "ContainerManifest.ID", m.ID}) + } else { + allIds.Insert(m.ID) } - allIds.Insert(m.ID) - if err := api.ValidateManifest(m); err != nil { - glog.Warningf("Manifest from %s failed validation, ignoring: %v", src, err) - continue + if errs := api.ValidateManifest(m); len(errs) != 0 { + allErrs = append(allErrs, errs...) } // 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 + if errs := checkHostPortConflicts(allManifests, m); len(errs) != 0 { + allErrs = append(allErrs, errs...) + } + if len(allErrs) > 0 { + glog.Warningf("Manifest from %s failed validation, ignoring: %v", src, allErrs) } } // TODO(thockin): There's no reason to collect manifests by value. Don't pessimize. diff --git a/pkg/kubelet/kubelet_test.go b/pkg/kubelet/kubelet_test.go index 5484e2265e1..3a1288a20f5 100644 --- a/pkg/kubelet/kubelet_test.go +++ b/pkg/kubelet/kubelet_test.go @@ -583,8 +583,8 @@ func TestCheckHostPortConflicts(t *testing.T) { successCaseNew := api.ContainerManifest{ Containers: []api.Container{{Ports: []api.Port{{HostPort: 83}}}}, } - if err := checkHostPortConflicts(successCaseAll, &successCaseNew); err != nil { - t.Errorf("Expected success: %v", err) + if errs := checkHostPortConflicts(successCaseAll, &successCaseNew); len(errs) != 0 { + t.Errorf("Expected success: %v", errs) } failureCaseAll := []api.ContainerManifest{ @@ -595,7 +595,7 @@ func TestCheckHostPortConflicts(t *testing.T) { failureCaseNew := api.ContainerManifest{ Containers: []api.Container{{Ports: []api.Port{{HostPort: 81}}}}, } - if err := checkHostPortConflicts(failureCaseAll, &failureCaseNew); err == nil { + if errs := checkHostPortConflicts(failureCaseAll, &failureCaseNew); len(errs) == 0 { t.Errorf("Expected failure") } } From 4ecefd1ba3b24e293147bc5ee040a2ad31215811 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Tue, 8 Jul 2014 17:33:15 -0700 Subject: [PATCH 2/2] Define an errorList type and use it to accumulate --- pkg/api/validation.go | 100 +++++++++++++++++++++--------------------- 1 file changed, 49 insertions(+), 51 deletions(-) diff --git a/pkg/api/validation.go b/pkg/api/validation.go index 1488128408d..aa090551900 100644 --- a/pkg/api/validation.go +++ b/pkg/api/validation.go @@ -63,16 +63,23 @@ func makeNotFoundError(field string, value interface{}) ValidationError { return ValidationError{ErrTypeNotFound, field, value} } -func validateVolumes(volumes []Volume) (util.StringSet, []error) { - allErrs := []error{} +// A helper for accumulating errors. This could be moved to util if anyone else needs it. +type errorList []error; + +func (list *errorList) Append(errs ...error) { + *list = append(*list, errs...) +} + +func validateVolumes(volumes []Volume) (util.StringSet, errorList) { + allErrs := errorList{} allNames := util.StringSet{} for i := range volumes { vol := &volumes[i] // so we can set default values if !util.IsDNSLabel(vol.Name) { - allErrs = append(allErrs, makeInvalidError("Volume.Name", vol.Name)) + allErrs.Append(makeInvalidError("Volume.Name", vol.Name)) } else if allNames.Has(vol.Name) { - allErrs = append(allErrs, makeDuplicateError("Volume.Name", vol.Name)) + allErrs.Append(makeDuplicateError("Volume.Name", vol.Name)) } else { allNames.Insert(vol.Name) } @@ -82,47 +89,47 @@ func validateVolumes(volumes []Volume) (util.StringSet, []error) { var supportedPortProtocols util.StringSet = util.NewStringSet("TCP", "UDP") -func validatePorts(ports []Port) []error { - allErrs := []error{} +func validatePorts(ports []Port) errorList { + allErrs := errorList{} 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) { - allErrs = append(allErrs, makeInvalidError("Port.Name", port.Name)) + allErrs.Append(makeInvalidError("Port.Name", port.Name)) } else if allNames.Has(port.Name) { - allErrs = append(allErrs, makeDuplicateError("Port.name", port.Name)) + allErrs.Append(makeDuplicateError("Port.name", port.Name)) } else { allNames.Insert(port.Name) } } if !util.IsValidPortNum(port.ContainerPort) { - allErrs = append(allErrs, makeInvalidError("Port.ContainerPort", port.ContainerPort)) + allErrs.Append(makeInvalidError("Port.ContainerPort", port.ContainerPort)) } if port.HostPort == 0 { port.HostPort = port.ContainerPort } else if !util.IsValidPortNum(port.HostPort) { - allErrs = append(allErrs, makeInvalidError("Port.HostPort", port.HostPort)) + allErrs.Append(makeInvalidError("Port.HostPort", port.HostPort)) } if len(port.Protocol) == 0 { port.Protocol = "TCP" } else if !supportedPortProtocols.Has(strings.ToUpper(port.Protocol)) { - allErrs = append(allErrs, makeNotSupportedError("Port.Protocol", port.Protocol)) + allErrs.Append(makeNotSupportedError("Port.Protocol", port.Protocol)) } } return allErrs } -func validateEnv(vars []EnvVar) []error { - allErrs := []error{} +func validateEnv(vars []EnvVar) errorList { + allErrs := errorList{} for i := range vars { ev := &vars[i] // so we can set default values if len(ev.Name) == 0 { // Backwards compat. if len(ev.Key) == 0 { - allErrs = append(allErrs, makeInvalidError("EnvVar.Name", ev.Name)) + allErrs.Append(makeInvalidError("EnvVar.Name", ev.Name)) } else { glog.Warning("DEPRECATED: EnvVar.Key has been replaced by EnvVar.Name") ev.Name = ev.Key @@ -130,26 +137,26 @@ func validateEnv(vars []EnvVar) []error { } } if !util.IsCIdentifier(ev.Name) { - allErrs = append(allErrs, makeInvalidError("EnvVar.Name", ev.Name)) + allErrs.Append(makeInvalidError("EnvVar.Name", ev.Name)) } } return allErrs } -func validateVolumeMounts(mounts []VolumeMount, volumes util.StringSet) []error { - allErrs := []error{} +func validateVolumeMounts(mounts []VolumeMount, volumes util.StringSet) errorList { + allErrs := errorList{} for i := range mounts { mnt := &mounts[i] // so we can set default values if len(mnt.Name) == 0 { - allErrs = append(allErrs, makeInvalidError("VolumeMount.Name", mnt.Name)) + allErrs.Append(makeInvalidError("VolumeMount.Name", mnt.Name)) } else if !volumes.Has(mnt.Name) { - allErrs = append(allErrs, makeNotFoundError("VolumeMount.Name", mnt.Name)) + allErrs.Append(makeNotFoundError("VolumeMount.Name", mnt.Name)) } if len(mnt.MountPath) == 0 { // Backwards compat. if len(mnt.Path) == 0 { - allErrs = append(allErrs, makeInvalidError("VolumeMount.MountPath", mnt.MountPath)) + allErrs.Append(makeInvalidError("VolumeMount.MountPath", mnt.MountPath)) } else { glog.Warning("DEPRECATED: VolumeMount.Path has been replaced by VolumeMount.MountPath") mnt.MountPath = mnt.Path @@ -162,15 +169,15 @@ func validateVolumeMounts(mounts []VolumeMount, volumes util.StringSet) []error // 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 { - allErrs := []error{} +func AccumulateUniquePorts(containers []Container, accumulator map[int]bool, extract func(*Port) int) errorList { + allErrs := errorList{} for ci := range containers { ctr := &containers[ci] for pi := range ctr.Ports { port := extract(&ctr.Ports[pi]) if accumulator[port] { - allErrs = append(allErrs, makeDuplicateError("Port", port)) + allErrs.Append(makeDuplicateError("Port", port)) } else { accumulator[port] = true } @@ -180,46 +187,39 @@ func AccumulateUniquePorts(containers []Container, accumulator map[int]bool, ext } // Checks for colliding Port.HostPort values across a slice of containers. -func checkHostPortConflicts(containers []Container) []error { +func checkHostPortConflicts(containers []Container) errorList { allPorts := map[int]bool{} return AccumulateUniquePorts(containers, allPorts, func(p *Port) int { return p.HostPort }) } -func validateContainers(containers []Container, volumes util.StringSet) []error { - allErrs := []error{} +func validateContainers(containers []Container, volumes util.StringSet) errorList { + allErrs := errorList{} allNames := util.StringSet{} for i := range containers { ctr := &containers[i] // so we can set default values if !util.IsDNSLabel(ctr.Name) { - allErrs = append(allErrs, makeInvalidError("Container.Name", ctr.Name)) + allErrs.Append(makeInvalidError("Container.Name", ctr.Name)) } else if allNames.Has(ctr.Name) { - allErrs = append(allErrs, makeDuplicateError("Container.Name", ctr.Name)) + allErrs.Append(makeDuplicateError("Container.Name", ctr.Name)) } else { allNames.Insert(ctr.Name) } if len(ctr.Image) == 0 { - allErrs = append(allErrs, makeInvalidError("Container.Image", ctr.Name)) - } - if errs := validatePorts(ctr.Ports); len(errs) != 0 { - allErrs = append(allErrs, errs...) - } - if errs := validateEnv(ctr.Env); len(errs) != 0 { - allErrs = append(allErrs, errs...) - } - if errs := validateVolumeMounts(ctr.VolumeMounts, volumes); len(errs) != 0 { - allErrs = append(allErrs, errs...) + allErrs.Append(makeInvalidError("Container.Image", ctr.Name)) } + allErrs.Append(validatePorts(ctr.Ports)...) + allErrs.Append(validateEnv(ctr.Env)...) + allErrs.Append(validateVolumeMounts(ctr.VolumeMounts, volumes)...) } // 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. - if errs := checkHostPortConflicts(containers); len(errs) != 0 { - allErrs = append(allErrs, errs...) - } - return allErrs + allErrs.Append(checkHostPortConflicts(containers)...) + + return allErrs; } var supportedManifestVersions util.StringSet = util.NewStringSet("v1beta1", "v1beta2") @@ -229,22 +229,20 @@ var supportedManifestVersions util.StringSet = util.NewStringSet("v1beta1", "v1b // structure by setting default values and implementing any backwards-compatibility // tricks. func ValidateManifest(manifest *ContainerManifest) []error { - allErrs := []error{} + allErrs := errorList{} if len(manifest.Version) == 0 { - allErrs = append(allErrs, makeInvalidError("ContainerManifest.Version", manifest.Version)) + allErrs.Append(makeInvalidError("ContainerManifest.Version", manifest.Version)) } else if !supportedManifestVersions.Has(strings.ToLower(manifest.Version)) { - allErrs = append(allErrs, makeNotSupportedError("ContainerManifest.Version", manifest.Version)) + allErrs.Append(makeNotSupportedError("ContainerManifest.Version", manifest.Version)) } if !util.IsDNSSubdomain(manifest.ID) { - allErrs = append(allErrs, makeInvalidError("ContainerManifest.ID", manifest.ID)) + allErrs.Append(makeInvalidError("ContainerManifest.ID", manifest.ID)) } allVolumes, errs := validateVolumes(manifest.Volumes) if len(errs) != 0 { - allErrs = append(allErrs, errs...) + allErrs.Append(errs...) } - if errs := validateContainers(manifest.Containers, allVolumes); len(errs) != 0 { - allErrs = append(allErrs, errs...) - } - return allErrs + allErrs.Append(validateContainers(manifest.Containers, allVolumes)...) + return []error(allErrs) }