Merge pull request #371 from thockin/valid3

Accumulate errors during validation

pkg/api/validation.go

@@ -63,146 +63,163 @@ func makeNotFoundError(field string, value interface{}) ValidationError {
 	return ValidationError{ErrTypeNotFound, field, value}
 }
 
-func validateVolumes(volumes []Volume) (util.StringSet, 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) {
-			return util.StringSet{}, makeInvalidError("Volume.Name", vol.Name)
+			allErrs.Append(makeInvalidError("Volume.Name", vol.Name))
+		} else if allNames.Has(vol.Name) {
+			allErrs.Append(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) 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) {
-				return makeInvalidError("Port.Name", port.Name)
+				allErrs.Append(makeInvalidError("Port.Name", port.Name))
+			} else if allNames.Has(port.Name) {
+				allErrs.Append(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(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(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(makeNotSupportedError("Port.Protocol", port.Protocol))
 		}
 	}
-	return nil
+	return allErrs
 }
 
-func validateEnv(vars []EnvVar) 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 {
-				return 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
+				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(makeInvalidError("EnvVar.Name", ev.Name))
 		}
 	}
-	return nil
+	return allErrs
 }
 
-func validateVolumeMounts(mounts []VolumeMount, volumes util.StringSet) 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 {
-			return makeInvalidError("VolumeMount.Name", mnt.Name)
+			allErrs.Append(makeInvalidError("VolumeMount.Name", mnt.Name))
-		}
+		} else if !volumes.Has(mnt.Name) {
-		if !volumes.Has(mnt.Name) {
+			allErrs.Append(makeNotFoundError("VolumeMount.Name", 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)
+				allErrs.Append(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) errorList {
+	allErrs := errorList{}
+
 	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(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) errorList {
 	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) errorList {
+	allErrs := errorList{}
+
 	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(makeInvalidError("Container.Name", ctr.Name))
+		} else if allNames.Has(ctr.Name) {
+			allErrs.Append(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(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
 		}
+		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.
-	return checkHostPortConflicts(containers)
+	allErrs.Append(checkHostPortConflicts(containers)...)
+
+	return allErrs;
 }
 
 var supportedManifestVersions util.StringSet = util.NewStringSet("v1beta1", "v1beta2")
@@ -211,23 +228,21 @@ 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 := errorList{}
+
 	if len(manifest.Version) == 0 {
-		return makeInvalidError("ContainerManifest.Version", manifest.Version)
+		allErrs.Append(makeInvalidError("ContainerManifest.Version", manifest.Version))
-	}
+	} else if !supportedManifestVersions.Has(strings.ToLower(manifest.Version)) {
-	if !supportedManifestVersions.Has(strings.ToLower(manifest.Version)) {
+		allErrs.Append(makeNotSupportedError("ContainerManifest.Version", manifest.Version))
-		return makeNotSupportedError("ContainerManifest.Version", manifest.Version)
 	}
 	if !util.IsDNSSubdomain(manifest.ID) {
-		return makeInvalidError("ContainerManifest.ID", manifest.ID)
+		allErrs.Append(makeInvalidError("ContainerManifest.ID", manifest.ID))
 	}
-	allVolumes, err := validateVolumes(manifest.Volumes)
+	allVolumes, errs := validateVolumes(manifest.Volumes)
-	if err != nil {
+	if len(errs) != 0 {
-		return err
+		allErrs.Append(errs...)
 	}
-	if err := validateContainers(manifest.Containers, allVolumes); err != nil {
+	allErrs.Append(validateContainers(manifest.Containers, allVolumes)...)
-		return err
+	return []error(allErrs)
-	}
-	return nil
 }

pkg/api/validation_test.go

@@ -29,9 +29,9 @@ func TestValidateVolumes(t *testing.T) {
 		{Name: "123"},
 		{Name: "abc-123"},
 	}
-	names, err := validateVolumes(successCase)
+	names, errs := validateVolumes(successCase)
-	if err != nil {
+	if len(errs) != 0 {
-		t.Errorf("expected success: %v", err)
+		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 errs := validatePorts(successCase); len(errs) != 0 {
-	if err != nil {
+		t.Errorf("expected success: %v", errs)
-		t.Errorf("expected success: %v", err)
 	}
 
 	nonCanonicalCase := []Port{
 		{ContainerPort: 80},
 	}
-	err = validatePorts(nonCanonicalCase)
+	if errs := validatePorts(nonCanonicalCase); len(errs) != 0 {
-	if err != nil {
+		t.Errorf("expected success: %v", errs)
-		t.Errorf("expected success: %v", err)
 	}
 	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 errs := validatePorts(v); len(errs) == 0 {
-		if err == nil {
 			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 {
+	if errs := validateEnv(successCase); len(errs) != 0 {
-		t.Errorf("expected success: %v", err)
+		t.Errorf("expected success: %v", errs)
 	}
 
 	nonCanonicalCase := []EnvVar{
 		{Key: "EV"},
 	}
-	if err := validateEnv(nonCanonicalCase); err != nil {
+	if errs := validateEnv(nonCanonicalCase); len(errs) != 0 {
-		t.Errorf("expected success: %v", err)
+		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 {
+	if errs := validateVolumeMounts(successCase, volumes); len(errs) != 0 {
-		t.Errorf("expected success: %v", err)
+		t.Errorf("expected success: %v", errs)
 	}
 
 	nonCanonicalCase := []VolumeMount{
 		{Name: "abc", Path: "/foo"},
 	}
-	err := validateVolumeMounts(nonCanonicalCase, volumes)
+	if errs := validateVolumeMounts(nonCanonicalCase, volumes); len(errs) != 0 {
-	if err != nil {
+		t.Errorf("expected success: %v", errs)
-		t.Errorf("expected success: %v", err)
 	}
 	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 errs := validateVolumeMounts(v, volumes); len(errs) == 0 {
-		if err == nil {
 			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 {
+	if errs := validateContainers(successCase, volumes); len(errs) != 0 {
-		t.Errorf("expected success: %v", err)
+		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 {
+		if errs := ValidateManifest(&manifest); len(errs) != 0 {
-			t.Errorf("expected success: %v", err)
+			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)
 		}
 	}

pkg/kubelet/kubelet.go

@@ -770,17 +770,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)
+		errs := api.AccumulateUniquePorts(manifest.Containers, allPorts, extract)
-		if err != nil {
+		if len(errs) != 0 {
-			return err
+			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
@@ -803,20 +808,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)
+					allErrs = append(allErrs, api.ValidationError{api.ErrTypeDuplicate, "ContainerManifest.ID", m.ID})
-					continue
+				} else {
+					allIds.Insert(m.ID)
 				}
-				allIds.Insert(m.ID)
+				if errs := api.ValidateManifest(m); len(errs) != 0 {
-				if err := api.ValidateManifest(m); err != nil {
+					allErrs = append(allErrs, errs...)
-					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 {
+				if errs := checkHostPortConflicts(allManifests, m); len(errs) != 0 {
-					glog.Warningf("Manifest from %s failed validation, ignoring: %v", src, err)
+					allErrs = append(allErrs, errs...)
-					continue
+				}
+				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.

pkg/kubelet/kubelet_test.go

@@ -596,8 +596,8 @@ func TestCheckHostPortConflicts(t *testing.T) {
 	successCaseNew := api.ContainerManifest{
 		Containers: []api.Container{{Ports: []api.Port{{HostPort: 83}}}},
 	}
-	if err := checkHostPortConflicts(successCaseAll, &successCaseNew); err != nil {
+	if errs := checkHostPortConflicts(successCaseAll, &successCaseNew); len(errs) != 0 {
-		t.Errorf("Expected success: %v", err)
+		t.Errorf("Expected success: %v", errs)
 	}
 
 	failureCaseAll := []api.ContainerManifest{
@@ -608,7 +608,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")
 	}
 }