Convert field errors into a selector for the object

Satisfies the "Field" condition for a Cause.  Also addresses
parts of #914.

pkg/api/errors/errors.go

@@ -106,7 +106,7 @@ func NewNotFound(field string, value interface{}) ValidationError {
 // the ToError() method, which will return nil for an empty ErrorList.
 type ErrorList []error
 
-// This helper implements the error interface for ErrorList, but must prevents
+// This helper implements the error interface for ErrorList, but prevents
 // accidental conversion of ErrorList to error.
 type errorListInternal ErrorList
 
@@ -129,3 +129,27 @@ func (list ErrorList) ToError() error {
 	}
 	return errorListInternal(list)
 }
+
+// Prefix adds a prefix to the Field of every ValidationError in the list. Returns
+// the list for convenience.
+func (list ErrorList) Prefix(prefix string) ErrorList {
+	for i := range list {
+		if err, ok := list[i].(ValidationError); ok {
+			if strings.HasPrefix(err.Field, "[") {
+				err.Field = prefix + err.Field
+			} else if len(err.Field) != 0 {
+				err.Field = prefix + "." + err.Field
+			} else {
+				err.Field = prefix
+			}
+			list[i] = err
+		}
+	}
+	return list
+}
+
+// PrefixIndex adds an index to the Field of every ValidationError in the list. Returns
+// the list for convenience.
+func (list ErrorList) PrefixIndex(index int) ErrorList {
+	return list.Prefix(fmt.Sprintf("[%d]", index))
+}

pkg/api/validation.go

@@ -34,19 +34,19 @@ func validateVolumes(volumes []Volume) (util.StringSet, errs.ErrorList) {
 		el := errs.ErrorList{}
 		// TODO(thockin) enforce that a source is set once we deprecate the implied form.
 		if vol.Source != nil {
-			el = validateSource(vol.Source)
+			el = validateSource(vol.Source).Prefix("source")
 		}
 		if len(vol.Name) == 0 {
-			el = append(el, errs.NewRequired("Volume.Name", vol.Name))
+			el = append(el, errs.NewRequired("name", vol.Name))
 		} else if !util.IsDNSLabel(vol.Name) {
-			el = append(el, errs.NewInvalid("Volume.Name", vol.Name))
+			el = append(el, errs.NewInvalid("name", vol.Name))
 		} else if allNames.Has(vol.Name) {
-			el = append(el, errs.NewDuplicate("Volume.Name", vol.Name))
+			el = append(el, errs.NewDuplicate("name", vol.Name))
 		}
 		if len(el) == 0 {
 			allNames.Insert(vol.Name)
 		} else {
-			allErrs = append(allErrs, el...)
+			allErrs = append(allErrs, el.PrefixIndex(i)...)
 		}
 	}
 	return allNames, allErrs
@@ -57,14 +57,14 @@ func validateSource(source *VolumeSource) errs.ErrorList {
 	allErrs := errs.ErrorList{}
 	if source.HostDirectory != nil {
 		numVolumes++
-		allErrs = append(allErrs, validateHostDir(source.HostDirectory)...)
+		allErrs = append(allErrs, validateHostDir(source.HostDirectory).Prefix("hostDirectory")...)
 	}
 	if source.EmptyDirectory != nil {
 		numVolumes++
 		//EmptyDirs have nothing to validate
 	}
 	if numVolumes != 1 {
-		allErrs = append(allErrs, errs.NewInvalid("Volume.Source", source))
+		allErrs = append(allErrs, errs.NewInvalid("", source))
 	}
 	return allErrs
 }
@@ -72,7 +72,7 @@ func validateSource(source *VolumeSource) errs.ErrorList {
 func validateHostDir(hostDir *HostDirectory) errs.ErrorList {
 	allErrs := errs.ErrorList{}
 	if hostDir.Path == "" {
-		allErrs = append(allErrs, errs.NewNotFound("HostDir.Path", hostDir.Path))
+		allErrs = append(allErrs, errs.NewNotFound("path", hostDir.Path))
 	}
 	return allErrs
 }
@@ -84,29 +84,31 @@ func validatePorts(ports []Port) errs.ErrorList {
 
 	allNames := util.StringSet{}
 	for i := range ports {
+		pErrs := errs.ErrorList{}
 		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, errs.NewInvalid("Port.Name", port.Name))
+				pErrs = append(pErrs, errs.NewInvalid("name", port.Name))
 			} else if allNames.Has(port.Name) {
-				allErrs = append(allErrs, errs.NewDuplicate("Port.name", port.Name))
+				pErrs = append(pErrs, errs.NewDuplicate("name", port.Name))
 			} else {
 				allNames.Insert(port.Name)
 			}
 		}
 		if port.ContainerPort == 0 {
-			allErrs = append(allErrs, errs.NewRequired("Port.ContainerPort", port.ContainerPort))
+			pErrs = append(pErrs, errs.NewRequired("containerPort", port.ContainerPort))
 		} else if !util.IsValidPortNum(port.ContainerPort) {
-			allErrs = append(allErrs, errs.NewInvalid("Port.ContainerPort", port.ContainerPort))
+			pErrs = append(pErrs, errs.NewInvalid("containerPort", port.ContainerPort))
 		}
 		if port.HostPort != 0 && !util.IsValidPortNum(port.HostPort) {
-			allErrs = append(allErrs, errs.NewInvalid("Port.HostPort", port.HostPort))
+			pErrs = append(pErrs, errs.NewInvalid("hostPort", port.HostPort))
 		}
 		if len(port.Protocol) == 0 {
 			port.Protocol = "TCP"
 		} else if !supportedPortProtocols.Has(strings.ToUpper(port.Protocol)) {
-			allErrs = append(allErrs, errs.NewNotSupported("Port.Protocol", port.Protocol))
+			pErrs = append(pErrs, errs.NewNotSupported("protocol", port.Protocol))
 		}
+		allErrs = append(allErrs, pErrs.PrefixIndex(i)...)
 	}
 	return allErrs
 }
@@ -115,13 +117,15 @@ func validateEnv(vars []EnvVar) errs.ErrorList {
 	allErrs := errs.ErrorList{}
 
 	for i := range vars {
+		vErrs := errs.ErrorList{}
 		ev := &vars[i] // so we can set default values
 		if len(ev.Name) == 0 {
-			allErrs = append(allErrs, errs.NewRequired("EnvVar.Name", ev.Name))
+			vErrs = append(vErrs, errs.NewRequired("name", ev.Name))
 		}
 		if !util.IsCIdentifier(ev.Name) {
-			allErrs = append(allErrs, errs.NewInvalid("EnvVar.Name", ev.Name))
+			vErrs = append(vErrs, errs.NewInvalid("name", ev.Name))
 		}
+		allErrs = append(allErrs, vErrs.PrefixIndex(i)...)
 	}
 	return allErrs
 }
@@ -130,16 +134,17 @@ func validateVolumeMounts(mounts []VolumeMount, volumes util.StringSet) errs.Err
 	allErrs := errs.ErrorList{}
 
 	for i := range mounts {
+		mErrs := errs.ErrorList{}
 		mnt := &mounts[i] // so we can set default values
 		if len(mnt.Name) == 0 {
-			allErrs = append(allErrs, errs.NewRequired("VolumeMount.Name", mnt.Name))
+			mErrs = append(mErrs, errs.NewRequired("name", mnt.Name))
 		} else if !volumes.Has(mnt.Name) {
-			allErrs = append(allErrs, errs.NewNotFound("VolumeMount.Name", mnt.Name))
+			mErrs = append(mErrs, errs.NewNotFound("name", mnt.Name))
 		}
 		if len(mnt.MountPath) == 0 {
 			// Backwards compat.
 			if len(mnt.Path) == 0 {
-				allErrs = append(allErrs, errs.NewRequired("VolumeMount.MountPath", mnt.MountPath))
+				mErrs = append(mErrs, errs.NewRequired("mountPath", mnt.MountPath))
 			} else {
 				glog.Warning("DEPRECATED: VolumeMount.Path has been replaced by VolumeMount.MountPath")
 				mnt.MountPath = mnt.Path
@@ -149,6 +154,7 @@ func validateVolumeMounts(mounts []VolumeMount, volumes util.StringSet) errs.Err
 		if len(mnt.MountType) != 0 {
 			glog.Warning("DEPRECATED: VolumeMount.MountType will be removed. The Volume struct will handle types")
 		}
+		allErrs = append(allErrs, mErrs.PrefixIndex(i)...)
 	}
 	return allErrs
 }
@@ -159,6 +165,7 @@ func AccumulateUniquePorts(containers []Container, accumulator map[int]bool, ext
 	allErrs := errs.ErrorList{}
 
 	for ci := range containers {
+		cErrs := errs.ErrorList{}
 		ctr := &containers[ci]
 		for pi := range ctr.Ports {
 			port := extract(&ctr.Ports[pi])
@@ -166,11 +173,12 @@ func AccumulateUniquePorts(containers []Container, accumulator map[int]bool, ext
 				continue
 			}
 			if accumulator[port] {
-				allErrs = append(allErrs, errs.NewDuplicate("Port", port))
+				cErrs = append(cErrs, errs.NewDuplicate("Port", port))
 			} else {
 				accumulator[port] = true
 			}
 		}
+		allErrs = append(allErrs, cErrs.PrefixIndex(ci)...)
 	}
 	return allErrs
 }
@@ -186,22 +194,24 @@ func validateContainers(containers []Container, volumes util.StringSet) errs.Err
 
 	allNames := util.StringSet{}
 	for i := range containers {
+		cErrs := errs.ErrorList{}
 		ctr := &containers[i] // so we can set default values
 		if len(ctr.Name) == 0 {
-			allErrs = append(allErrs, errs.NewRequired("Container.Name", ctr.Name))
+			cErrs = append(cErrs, errs.NewRequired("name", ctr.Name))
 		} else if !util.IsDNSLabel(ctr.Name) {
-			allErrs = append(allErrs, errs.NewInvalid("Container.Name", ctr.Name))
+			cErrs = append(cErrs, errs.NewInvalid("name", ctr.Name))
 		} else if allNames.Has(ctr.Name) {
-			allErrs = append(allErrs, errs.NewDuplicate("Container.Name", ctr.Name))
+			cErrs = append(cErrs, errs.NewDuplicate("name", ctr.Name))
 		} else {
 			allNames.Insert(ctr.Name)
 		}
 		if len(ctr.Image) == 0 {
-			allErrs = append(allErrs, errs.NewInvalid("Container.Image", ctr.Name))
+			cErrs = append(cErrs, errs.NewInvalid("image", ctr.Name))
 		}
-		allErrs = append(allErrs, validatePorts(ctr.Ports)...)
+		cErrs = append(cErrs, validatePorts(ctr.Ports).Prefix("ports")...)
-		allErrs = append(allErrs, validateEnv(ctr.Env)...)
+		cErrs = append(cErrs, validateEnv(ctr.Env).Prefix("env")...)
-		allErrs = append(allErrs, validateVolumeMounts(ctr.VolumeMounts, volumes)...)
+		cErrs = append(cErrs, validateVolumeMounts(ctr.VolumeMounts, volumes).Prefix("volumeMounts")...)
+		allErrs = append(allErrs, cErrs.PrefixIndex(i)...)
 	}
 	// Check for colliding ports across all containers.
 	// TODO(thockin): This really is dependent on the network config of the host (IP per pod?)
@@ -223,26 +233,24 @@ func ValidateManifest(manifest *ContainerManifest) errs.ErrorList {
 	allErrs := errs.ErrorList{}
 
 	if len(manifest.Version) == 0 {
-		allErrs = append(allErrs, errs.NewRequired("ContainerManifest.Version", manifest.Version))
+		allErrs = append(allErrs, errs.NewRequired("version", manifest.Version))
 	} else if !supportedManifestVersions.Has(strings.ToLower(manifest.Version)) {
-		allErrs = append(allErrs, errs.NewNotSupported("ContainerManifest.Version", manifest.Version))
+		allErrs = append(allErrs, errs.NewNotSupported("version", manifest.Version))
 	}
 	allVolumes, errs := validateVolumes(manifest.Volumes)
-	if len(errs) != 0 {
+	allErrs = append(allErrs, errs.Prefix("volumes")...)
-		allErrs = append(allErrs, errs...)
+	allErrs = append(allErrs, validateContainers(manifest.Containers, allVolumes).Prefix("containers")...)
-	}
-	allErrs = append(allErrs, validateContainers(manifest.Containers, allVolumes)...)
 	return allErrs
 }
 
 func ValidatePodState(podState *PodState) errs.ErrorList {
-	allErrs := errs.ErrorList(ValidateManifest(&podState.Manifest))
+	allErrs := errs.ErrorList(ValidateManifest(&podState.Manifest)).Prefix("manifest")
 	if podState.RestartPolicy.Type == "" {
 		podState.RestartPolicy.Type = RestartAlways
 	} else if podState.RestartPolicy.Type != RestartAlways &&
 		podState.RestartPolicy.Type != RestartOnFailure &&
 		podState.RestartPolicy.Type != RestartNever {
-		allErrs = append(allErrs, errs.NewNotSupported("PodState.RestartPolicy.Type", podState.RestartPolicy.Type))
+		allErrs = append(allErrs, errs.NewNotSupported("restartPolicy.type", podState.RestartPolicy.Type))
 	}
 
 	return allErrs
@@ -252,9 +260,9 @@ func ValidatePodState(podState *PodState) errs.ErrorList {
 func ValidatePod(pod *Pod) errs.ErrorList {
 	allErrs := errs.ErrorList{}
 	if len(pod.ID) == 0 {
-		allErrs = append(allErrs, errs.NewRequired("Pod.ID", pod.ID))
+		allErrs = append(allErrs, errs.NewRequired("id", pod.ID))
 	}
-	allErrs = append(allErrs, ValidatePodState(&pod.DesiredState)...)
+	allErrs = append(allErrs, ValidatePodState(&pod.DesiredState).Prefix("desiredState")...)
 	return allErrs
 }
 
@@ -262,15 +270,15 @@ func ValidatePod(pod *Pod) errs.ErrorList {
 func ValidateService(service *Service) errs.ErrorList {
 	allErrs := errs.ErrorList{}
 	if len(service.ID) == 0 {
-		allErrs = append(allErrs, errs.NewRequired("Service.ID", service.ID))
+		allErrs = append(allErrs, errs.NewRequired("id", service.ID))
 	} else if !util.IsDNS952Label(service.ID) {
-		allErrs = append(allErrs, errs.NewInvalid("Service.ID", service.ID))
+		allErrs = append(allErrs, errs.NewInvalid("id", service.ID))
 	}
 	if !util.IsValidPortNum(service.Port) {
 		allErrs = append(allErrs, errs.NewInvalid("Service.Port", service.Port))
 	}
 	if labels.Set(service.Selector).AsSelector().Empty() {
-		allErrs = append(allErrs, errs.NewRequired("Service.Selector", service.Selector))
+		allErrs = append(allErrs, errs.NewRequired("selector", service.Selector))
 	}
 	return allErrs
 }
@@ -279,19 +287,19 @@ func ValidateService(service *Service) errs.ErrorList {
 func ValidateReplicationController(controller *ReplicationController) errs.ErrorList {
 	allErrs := errs.ErrorList{}
 	if len(controller.ID) == 0 {
-		allErrs = append(allErrs, errs.NewRequired("ReplicationController.ID", controller.ID))
+		allErrs = append(allErrs, errs.NewRequired("id", controller.ID))
 	}
 	if labels.Set(controller.DesiredState.ReplicaSelector).AsSelector().Empty() {
-		allErrs = append(allErrs, errs.NewRequired("ReplicationController.ReplicaSelector", controller.DesiredState.ReplicaSelector))
+		allErrs = append(allErrs, errs.NewRequired("desiredState.replicaSelector", controller.DesiredState.ReplicaSelector))
 	}
 	selector := labels.Set(controller.DesiredState.ReplicaSelector).AsSelector()
 	labels := labels.Set(controller.DesiredState.PodTemplate.Labels)
 	if !selector.Matches(labels) {
-		allErrs = append(allErrs, errs.NewInvalid("ReplicaController.DesiredState.PodTemplate.Labels", controller.DesiredState.PodTemplate))
+		allErrs = append(allErrs, errs.NewInvalid("desiredState.podTemplate.labels", controller.DesiredState.PodTemplate))
 	}
 	if controller.DesiredState.Replicas < 0 {
-		allErrs = append(allErrs, errs.NewInvalid("ReplicationController.Replicas", controller.DesiredState.Replicas))
+		allErrs = append(allErrs, errs.NewInvalid("desiredState.replicas", controller.DesiredState.Replicas))
 	}
-	allErrs = append(allErrs, ValidateManifest(&controller.DesiredState.PodTemplate.DesiredState.Manifest)...)
+	allErrs = append(allErrs, ValidateManifest(&controller.DesiredState.PodTemplate.DesiredState.Manifest).Prefix("desiredState.podTemplate.desiredState.manifest")...)
 	return allErrs
 }

pkg/api/validation_test.go

@@ -21,10 +21,19 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/v1beta1"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
 )
 
+func expectPrefix(t *testing.T, prefix string, errs errors.ErrorList) {
+	for i := range errs {
+		if !strings.HasPrefix(errs[i].(errors.ValidationError).Field, prefix) {
+			t.Errorf("expected prefix '%s' for %v", errs[i])
+		}
+	}
+}
+
 func TestValidateVolumes(t *testing.T) {
 	successCase := []Volume{
 		{Name: "abc"},
@@ -40,15 +49,29 @@ func TestValidateVolumes(t *testing.T) {
 		t.Errorf("wrong names result: %v", names)
 	}
 
-	errorCases := map[string][]Volume{
+	errorCases := map[string]struct {
-		"zero-length name":     {{Name: ""}},
+		V []Volume
-		"name > 63 characters": {{Name: strings.Repeat("a", 64)}},
+		T errors.ValidationErrorType
-		"name not a DNS label": {{Name: "a.b.c"}},
+		F string
-		"name not unique":      {{Name: "abc"}, {Name: "abc"}},
+	}{
+		"zero-length name":     {[]Volume{{Name: ""}}, errors.ValidationErrorTypeRequired, "[0].name"},
+		"name > 63 characters": {[]Volume{{Name: strings.Repeat("a", 64)}}, errors.ValidationErrorTypeInvalid, "[0].name"},
+		"name not a DNS label": {[]Volume{{Name: "a.b.c"}}, errors.ValidationErrorTypeInvalid, "[0].name"},
+		"name not unique":      {[]Volume{{Name: "abc"}, {Name: "abc"}}, errors.ValidationErrorTypeDuplicate, "[1].name"},
 	}
 	for k, v := range errorCases {
-		if _, errs := validateVolumes(v); len(errs) == 0 {
+		_, errs := validateVolumes(v.V)
+		if len(errs) == 0 {
 			t.Errorf("expected failure for %s", k)
+			continue
+		}
+		for i := range errs {
+			if errs[i].(errors.ValidationError).Type != v.T {
+				t.Errorf("%s: expected errors to have type %s: %v", k, v.T, errs[i])
+			}
+			if errs[i].(errors.ValidationError).Field != v.F {
+				t.Errorf("%s: expected errors to have field %s: %v", k, v.F, errs[i])
+			}
 		}
 	}
 }
@@ -77,22 +100,35 @@ func TestValidatePorts(t *testing.T) {
 		t.Errorf("expected default values: %+v", nonCanonicalCase[0])
 	}
 
-	errorCases := map[string][]Port{
+	errorCases := map[string]struct {
-		"name > 63 characters": {{Name: strings.Repeat("a", 64), ContainerPort: 80}},
+		P []Port
-		"name not a DNS label": {{Name: "a.b.c", ContainerPort: 80}},
+		T errors.ValidationErrorType
-		"name not unique": {
+		F string
+	}{
+		"name > 63 characters": {[]Port{{Name: strings.Repeat("a", 64), ContainerPort: 80}}, errors.ValidationErrorTypeInvalid, "[0].name"},
+		"name not a DNS label": {[]Port{{Name: "a.b.c", ContainerPort: 80}}, errors.ValidationErrorTypeInvalid, "[0].name"},
+		"name not unique": {[]Port{
 			{Name: "abc", ContainerPort: 80},
 			{Name: "abc", ContainerPort: 81},
-		},
+		}, errors.ValidationErrorTypeDuplicate, "[1].name"},
-		"zero container port":    {{ContainerPort: 0}},
+		"zero container port":    {[]Port{{ContainerPort: 0}}, errors.ValidationErrorTypeRequired, "[0].containerPort"},
-		"invalid container port": {{ContainerPort: 65536}},
+		"invalid container port": {[]Port{{ContainerPort: 65536}}, errors.ValidationErrorTypeInvalid, "[0].containerPort"},
-		"invalid host port":      {{ContainerPort: 80, HostPort: 65536}},
+		"invalid host port":      {[]Port{{ContainerPort: 80, HostPort: 65536}}, errors.ValidationErrorTypeInvalid, "[0].hostPort"},
-		"invalid protocol":       {{ContainerPort: 80, Protocol: "ICMP"}},
+		"invalid protocol":       {[]Port{{ContainerPort: 80, Protocol: "ICMP"}}, errors.ValidationErrorTypeNotSupported, "[0].protocol"},
 	}
 	for k, v := range errorCases {
-		if errs := validatePorts(v); len(errs) == 0 {
+		errs := validatePorts(v.P)
+		if len(errs) == 0 {
 			t.Errorf("expected failure for %s", k)
 		}
+		for i := range errs {
+			if errs[i].(errors.ValidationError).Type != v.T {
+				t.Errorf("%s: expected errors to have type %s: %v", k, v.T, errs[i])
+			}
+			if errs[i].(errors.ValidationError).Field != v.F {
+				t.Errorf("%s: expected errors to have field %s: %v", k, v.F, errs[i])
+			}
+		}
 	}
 }
 
@@ -449,8 +485,18 @@ func TestValidateReplicationController(t *testing.T) {
 		},
 	}
 	for k, v := range errorCases {
-		if errs := ValidateReplicationController(&v); len(errs) == 0 {
+		errs := ValidateReplicationController(&v)
+		if len(errs) == 0 {
 			t.Errorf("expected failure for %s", k)
 		}
+		for i := range errs {
+			field := errs[i].(errors.ValidationError).Field
+			if !strings.HasPrefix(field, "desiredState.podTemplate.") &&
+				field != "id" &&
+				field != "desiredState.replicaSelector" &&
+				field != "desiredState.replicas" {
+				t.Errorf("%s: missing prefix for: %v", k, errs[i])
+			}
+		}
 	}
 }