Merge pull request #2515 from thockin/why

Add details on Invalid validation errors

pkg/api/errors/errors.go

@@ -30,6 +30,8 @@ type StatusError struct {
 	ErrStatus api.Status
 }
 
+var _ error = &StatusError{}
+
 // Error implements the Error interface.
 func (e *StatusError) Error() string {
 	return e.ErrStatus.Message
@@ -107,7 +109,7 @@ func NewConflict(kind, name string, err error) error {
 func NewInvalid(kind, name string, errs ValidationErrorList) error {
 	causes := make([]api.StatusCause, 0, len(errs))
 	for i := range errs {
-		if err, ok := errs[i].(ValidationError); ok {
+		if err, ok := errs[i].(*ValidationError); ok {
 			causes = append(causes, api.StatusCause{
 				Type:    api.CauseType(err.Type),
 				Message: err.Error(),
@@ -130,18 +132,16 @@ func NewInvalid(kind, name string, errs ValidationErrorList) error {
 
 // NewBadRequest creates an error that indicates that the request is invalid and can not be processed.
 func NewBadRequest(reason string) error {
-	return &StatusError{
+	return &StatusError{api.Status{
-		api.Status{
+		Status: api.StatusFailure,
-			Status: api.StatusFailure,
+		Code:   http.StatusBadRequest,
-			Code:   http.StatusBadRequest,
+		Reason: api.StatusReasonBadRequest,
-			Reason: api.StatusReasonBadRequest,
+		Details: &api.StatusDetails{
-			Details: &api.StatusDetails{
+			Causes: []api.StatusCause{
-				Causes: []api.StatusCause{
+				{Message: reason},
-					{Message: reason},
-				},
 			},
 		},
-	}
+	}}
 }
 
 // NewInternalError returns an error indicating the item is invalid and cannot be processed.

pkg/api/errors/errors_test.go

@@ -54,7 +54,7 @@ func TestErrorNew(t *testing.T) {
 
 func TestNewInvalid(t *testing.T) {
 	testCases := []struct {
-		Err     ValidationError
+		Err     *ValidationError
 		Details *api.StatusDetails
 	}{
 		{
@@ -69,7 +69,7 @@ func TestNewInvalid(t *testing.T) {
 			},
 		},
 		{
-			NewFieldInvalid("field[0].name", "bar"),
+			NewFieldInvalid("field[0].name", "bar", "detail"),
 			&api.StatusDetails{
 				Kind: "kind",
 				ID:   "name",

pkg/api/errors/validation.go

@@ -78,40 +78,47 @@ type ValidationError struct {
 	Type     ValidationErrorType
 	Field    string
 	BadValue interface{}
+	Detail   string
 }
 
-func (v ValidationError) Error() string {
+var _ error = &ValidationError{}
-	return fmt.Sprintf("%s: %v '%v'", v.Field, ValueOf(v.Type), v.BadValue)
+
+func (v *ValidationError) Error() string {
+	s := fmt.Sprintf("%s: %s '%v'", v.Field, ValueOf(v.Type), v.BadValue)
+	if v.Detail != "" {
+		s += fmt.Sprintf(": %s", v.Detail)
+	}
+	return s
 }
 
-// NewFieldRequired returns a ValidationError indicating "value required"
+// NewFieldRequired returns a *ValidationError indicating "value required"
-func NewFieldRequired(field string, value interface{}) ValidationError {
+func NewFieldRequired(field string, value interface{}) *ValidationError {
-	return ValidationError{ValidationErrorTypeRequired, field, value}
+	return &ValidationError{ValidationErrorTypeRequired, field, value, ""}
 }
 
-// NewFieldInvalid returns a ValidationError indicating "invalid value"
+// NewFieldInvalid returns a *ValidationError indicating "invalid value"
-func NewFieldInvalid(field string, value interface{}) ValidationError {
+func NewFieldInvalid(field string, value interface{}, detail string) *ValidationError {
-	return ValidationError{ValidationErrorTypeInvalid, field, value}
+	return &ValidationError{ValidationErrorTypeInvalid, field, value, detail}
 }
 
-// NewFieldNotSupported returns a ValidationError indicating "unsupported value"
+// NewFieldNotSupported returns a *ValidationError indicating "unsupported value"
-func NewFieldNotSupported(field string, value interface{}) ValidationError {
+func NewFieldNotSupported(field string, value interface{}) *ValidationError {
-	return ValidationError{ValidationErrorTypeNotSupported, field, value}
+	return &ValidationError{ValidationErrorTypeNotSupported, field, value, ""}
 }
 
-// NewFieldForbidden returns a ValidationError indicating "forbidden"
+// NewFieldForbidden returns a *ValidationError indicating "forbidden"
-func NewFieldForbidden(field string, value interface{}) ValidationError {
+func NewFieldForbidden(field string, value interface{}) *ValidationError {
-	return ValidationError{ValidationErrorTypeForbidden, field, value}
+	return &ValidationError{ValidationErrorTypeForbidden, field, value, ""}
 }
 
-// NewFieldDuplicate returns a ValidationError indicating "duplicate value"
+// NewFieldDuplicate returns a *ValidationError indicating "duplicate value"
-func NewFieldDuplicate(field string, value interface{}) ValidationError {
+func NewFieldDuplicate(field string, value interface{}) *ValidationError {
-	return ValidationError{ValidationErrorTypeDuplicate, field, value}
+	return &ValidationError{ValidationErrorTypeDuplicate, field, value, ""}
 }
 
-// NewFieldNotFound returns a ValidationError indicating "value not found"
+// NewFieldNotFound returns a *ValidationError indicating "value not found"
-func NewFieldNotFound(field string, value interface{}) ValidationError {
+func NewFieldNotFound(field string, value interface{}) *ValidationError {
-	return ValidationError{ValidationErrorTypeNotFound, field, value}
+	return &ValidationError{ValidationErrorTypeNotFound, field, value, ""}
 }
 
 // ValidationErrorList is a collection of ValidationErrors.  This does not
@@ -131,7 +138,7 @@ func (list ValidationErrorList) ToError() error {
 // Returns the list for convenience.
 func (list ValidationErrorList) Prefix(prefix string) ValidationErrorList {
 	for i := range list {
-		if err, ok := list[i].(ValidationError); ok {
+		if err, ok := list[i].(*ValidationError); ok {
 			if strings.HasPrefix(err.Field, "[") {
 				err.Field = prefix + err.Field
 			} else if len(err.Field) != 0 {

pkg/api/errors/validation_test.go

@@ -23,27 +23,27 @@ import (
 
 func TestMakeFuncs(t *testing.T) {
 	testCases := []struct {
-		fn       func() ValidationError
+		fn       func() *ValidationError
 		expected ValidationErrorType
 	}{
 		{
-			func() ValidationError { return NewFieldInvalid("f", "v") },
+			func() *ValidationError { return NewFieldInvalid("f", "v", "d") },
 			ValidationErrorTypeInvalid,
 		},
 		{
-			func() ValidationError { return NewFieldNotSupported("f", "v") },
+			func() *ValidationError { return NewFieldNotSupported("f", "v") },
 			ValidationErrorTypeNotSupported,
 		},
 		{
-			func() ValidationError { return NewFieldDuplicate("f", "v") },
+			func() *ValidationError { return NewFieldDuplicate("f", "v") },
 			ValidationErrorTypeDuplicate,
 		},
 		{
-			func() ValidationError { return NewFieldNotFound("f", "v") },
+			func() *ValidationError { return NewFieldNotFound("f", "v") },
 			ValidationErrorTypeNotFound,
 		},
 		{
-			func() ValidationError { return NewFieldRequired("f", "v") },
+			func() *ValidationError { return NewFieldRequired("f", "v") },
 			ValidationErrorTypeRequired,
 		},
 	}
@@ -57,15 +57,15 @@ func TestMakeFuncs(t *testing.T) {
 }
 
 func TestValidationError(t *testing.T) {
-	s := NewFieldInvalid("foo", "bar").Error()
+	s := NewFieldInvalid("foo", "bar", "deet").Error()
-	if !strings.Contains(s, "foo") || !strings.Contains(s, "bar") || !strings.Contains(s, ValueOf(ValidationErrorTypeInvalid)) {
+	if !strings.Contains(s, "foo") || !strings.Contains(s, "bar") || !strings.Contains(s, "deet") || !strings.Contains(s, ValueOf(ValidationErrorTypeInvalid)) {
 		t.Errorf("error message did not contain expected values, got %s", s)
 	}
 }
 
 func TestErrListPrefix(t *testing.T) {
 	testCases := []struct {
-		Err      ValidationError
+		Err      *ValidationError
 		Expected string
 	}{
 		{
@@ -73,7 +73,7 @@ func TestErrListPrefix(t *testing.T) {
 			"foo[0].bar",
 		},
 		{
-			NewFieldInvalid("field", "value"),
+			NewFieldInvalid("field", "value", ""),
 			"foo.field",
 		},
 		{
@@ -87,7 +87,7 @@ func TestErrListPrefix(t *testing.T) {
 		if prefix == nil || len(prefix) != len(errList) {
 			t.Errorf("Prefix should return self")
 		}
-		if e, a := testCase.Expected, errList[0].(ValidationError).Field; e != a {
+		if e, a := testCase.Expected, errList[0].(*ValidationError).Field; e != a {
 			t.Errorf("expected %s, got %s", e, a)
 		}
 	}
@@ -95,7 +95,7 @@ func TestErrListPrefix(t *testing.T) {
 
 func TestErrListPrefixIndex(t *testing.T) {
 	testCases := []struct {
-		Err      ValidationError
+		Err      *ValidationError
 		Expected string
 	}{
 		{
@@ -103,7 +103,7 @@ func TestErrListPrefixIndex(t *testing.T) {
 			"[1][0].bar",
 		},
 		{
-			NewFieldInvalid("field", "value"),
+			NewFieldInvalid("field", "value", ""),
 			"[1].field",
 		},
 		{
@@ -117,7 +117,7 @@ func TestErrListPrefixIndex(t *testing.T) {
 		if prefix == nil || len(prefix) != len(errList) {
 			t.Errorf("PrefixIndex should return self")
 		}
-		if e, a := testCase.Expected, errList[0].(ValidationError).Field; e != a {
+		if e, a := testCase.Expected, errList[0].(*ValidationError).Field; e != a {
 			t.Errorf("expected %s, got %s", e, a)
 		}
 	}

pkg/api/validation/events.go

@@ -26,10 +26,10 @@ import (
 func ValidateEvent(event *api.Event) errs.ValidationErrorList {
 	allErrs := errs.ValidationErrorList{}
 	if event.Namespace != event.InvolvedObject.Namespace {
-		allErrs = append(allErrs, errs.NewFieldInvalid("involvedObject.namespace", event.InvolvedObject.Namespace))
+		allErrs = append(allErrs, errs.NewFieldInvalid("involvedObject.namespace", event.InvolvedObject.Namespace, "namespace does not match involvedObject"))
 	}
 	if !util.IsDNSSubdomain(event.Namespace) {
-		allErrs = append(allErrs, errs.NewFieldInvalid("namespace", event.Namespace))
+		allErrs = append(allErrs, errs.NewFieldInvalid("namespace", event.Namespace, ""))
 	}
 	return allErrs
 }

pkg/api/validation/validation.go

@@ -50,7 +50,7 @@ func validateVolumes(volumes []api.Volume) (util.StringSet, errs.ValidationError
 		if len(vol.Name) == 0 {
 			el = append(el, errs.NewFieldRequired("name", vol.Name))
 		} else if !util.IsDNSLabel(vol.Name) {
-			el = append(el, errs.NewFieldInvalid("name", vol.Name))
+			el = append(el, errs.NewFieldInvalid("name", vol.Name, ""))
 		} else if allNames.Has(vol.Name) {
 			el = append(el, errs.NewFieldDuplicate("name", vol.Name))
 		}
@@ -83,7 +83,7 @@ func validateSource(source *api.VolumeSource) errs.ValidationErrorList {
 		allErrs = append(allErrs, validateGCEPersistentDisk(source.GCEPersistentDisk)...)
 	}
 	if numVolumes != 1 {
-		allErrs = append(allErrs, errs.NewFieldInvalid("", source))
+		allErrs = append(allErrs, errs.NewFieldInvalid("", source, "exactly 1 volume type is required"))
 	}
 	return allErrs
 }
@@ -113,7 +113,7 @@ func validateGCEPersistentDisk(PD *api.GCEPersistentDisk) errs.ValidationErrorLi
 		allErrs = append(allErrs, errs.NewFieldRequired("PD.FSType", PD.FSType))
 	}
 	if PD.Partition < 0 || PD.Partition > 255 {
-		allErrs = append(allErrs, errs.NewFieldInvalid("PD.Partition", PD.Partition))
+		allErrs = append(allErrs, errs.NewFieldInvalid("PD.Partition", PD.Partition, ""))
 	}
 	return allErrs
 }
@@ -129,7 +129,7 @@ func validatePorts(ports []api.Port) errs.ValidationErrorList {
 		port := &ports[i] // so we can set default values
 		if len(port.Name) > 0 {
 			if len(port.Name) > 63 || !util.IsDNSLabel(port.Name) {
-				pErrs = append(pErrs, errs.NewFieldInvalid("name", port.Name))
+				pErrs = append(pErrs, errs.NewFieldInvalid("name", port.Name, ""))
 			} else if allNames.Has(port.Name) {
 				pErrs = append(pErrs, errs.NewFieldDuplicate("name", port.Name))
 			} else {
@@ -139,10 +139,10 @@ func validatePorts(ports []api.Port) errs.ValidationErrorList {
 		if port.ContainerPort == 0 {
 			pErrs = append(pErrs, errs.NewFieldRequired("containerPort", port.ContainerPort))
 		} else if !util.IsValidPortNum(port.ContainerPort) {
-			pErrs = append(pErrs, errs.NewFieldInvalid("containerPort", port.ContainerPort))
+			pErrs = append(pErrs, errs.NewFieldInvalid("containerPort", port.ContainerPort, ""))
 		}
 		if port.HostPort != 0 && !util.IsValidPortNum(port.HostPort) {
-			pErrs = append(pErrs, errs.NewFieldInvalid("hostPort", port.HostPort))
+			pErrs = append(pErrs, errs.NewFieldInvalid("hostPort", port.HostPort, ""))
 		}
 		if len(port.Protocol) == 0 {
 			port.Protocol = "TCP"
@@ -164,7 +164,7 @@ func validateEnv(vars []api.EnvVar) errs.ValidationErrorList {
 			vErrs = append(vErrs, errs.NewFieldRequired("name", ev.Name))
 		}
 		if !util.IsCIdentifier(ev.Name) {
-			vErrs = append(vErrs, errs.NewFieldInvalid("name", ev.Name))
+			vErrs = append(vErrs, errs.NewFieldInvalid("name", ev.Name, ""))
 		}
 		allErrs = append(allErrs, vErrs.PrefixIndex(i)...)
 	}
@@ -238,13 +238,18 @@ func validateHTTPGetAction(http *api.HTTPGetAction) errs.ValidationErrorList {
 }
 
 func validateHandler(handler *api.Handler) errs.ValidationErrorList {
+	numHandlers := 0
 	allErrors := errs.ValidationErrorList{}
 	if handler.Exec != nil {
+		numHandlers++
 		allErrors = append(allErrors, validateExecAction(handler.Exec).Prefix("exec")...)
-	} else if handler.HTTPGet != nil {
+	}
+	if handler.HTTPGet != nil {
+		numHandlers++
 		allErrors = append(allErrors, validateHTTPGetAction(handler.HTTPGet).Prefix("httpGet")...)
-	} else {
+	}
-		allErrors = append(allErrors, errs.NewFieldInvalid("", handler))
+	if numHandlers != 1 {
+		allErrors = append(allErrors, errs.NewFieldInvalid("", handler, "exactly 1 handler type is required"))
 	}
 	return allErrors
 }
@@ -271,7 +276,7 @@ func validateContainers(containers []api.Container, volumes util.StringSet) errs
 		if len(ctr.Name) == 0 {
 			cErrs = append(cErrs, errs.NewFieldRequired("name", ctr.Name))
 		} else if !util.IsDNSLabel(ctr.Name) {
-			cErrs = append(cErrs, errs.NewFieldInvalid("name", ctr.Name))
+			cErrs = append(cErrs, errs.NewFieldInvalid("name", ctr.Name, ""))
 		} else if allNames.Has(ctr.Name) {
 			cErrs = append(cErrs, errs.NewFieldDuplicate("name", ctr.Name))
 		} else if ctr.Privileged && !capabilities.AllowPrivileged {
@@ -338,7 +343,7 @@ func validateRestartPolicy(restartPolicy *api.RestartPolicy) errs.ValidationErro
 		restartPolicy.Always = &api.RestartPolicyAlways{}
 	}
 	if numPolicies > 1 {
-		allErrors = append(allErrors, errs.NewFieldInvalid("", restartPolicy))
+		allErrors = append(allErrors, errs.NewFieldInvalid("", restartPolicy, "only 1 policy is allowed"))
 	}
 	return allErrors
 }
@@ -355,7 +360,7 @@ func ValidatePod(pod *api.Pod) errs.ValidationErrorList {
 		allErrs = append(allErrs, errs.NewFieldRequired("name", pod.Name))
 	}
 	if !util.IsDNSSubdomain(pod.Namespace) {
-		allErrs = append(allErrs, errs.NewFieldInvalid("namespace", pod.Namespace))
+		allErrs = append(allErrs, errs.NewFieldInvalid("namespace", pod.Namespace, ""))
 	}
 	allErrs = append(allErrs, ValidatePodSpec(&pod.Spec).Prefix("spec")...)
 	allErrs = append(allErrs, validateLabels(pod.Labels)...)
@@ -392,16 +397,14 @@ func ValidatePodUpdate(newPod, oldPod *api.Pod) errs.ValidationErrorList {
 	allErrs := errs.ValidationErrorList{}
 
 	if newPod.Name != oldPod.Name {
-		allErrs = append(allErrs, errs.NewFieldInvalid("name", newPod.Name))
+		allErrs = append(allErrs, errs.NewFieldInvalid("name", newPod.Name, "field is immutable"))
 	}
 
 	if len(newPod.Spec.Containers) != len(oldPod.Spec.Containers) {
-		allErrs = append(allErrs, errs.NewFieldInvalid("spec.containers", newPod.Spec.Containers))
+		allErrs = append(allErrs, errs.NewFieldInvalid("spec.containers", newPod.Spec.Containers, "may not add or remove containers"))
 		return allErrs
 	}
 	pod := *newPod
-	pod.Labels = oldPod.Labels
-	pod.ResourceVersion = oldPod.ResourceVersion
 	// Tricky, we need to copy the container list so that we don't overwrite the update
 	var newContainers []api.Container
 	for ix, container := range pod.Spec.Containers {
@@ -410,7 +413,8 @@ func ValidatePodUpdate(newPod, oldPod *api.Pod) errs.ValidationErrorList {
 	}
 	pod.Spec.Containers = newContainers
 	if !reflect.DeepEqual(pod.Spec, oldPod.Spec) {
-		allErrs = append(allErrs, errs.NewFieldInvalid("spec.containers", newPod.Spec.Containers))
+		// TODO: a better error would include all immutable fields explicitly.
+		allErrs = append(allErrs, errs.NewFieldInvalid("spec.containers", newPod.Spec.Containers, "some fields are immutable"))
 	}
 	return allErrs
 }
@@ -421,13 +425,13 @@ func ValidateService(service *api.Service, lister ServiceLister, ctx api.Context
 	if len(service.Name) == 0 {
 		allErrs = append(allErrs, errs.NewFieldRequired("name", service.Name))
 	} else if !util.IsDNS952Label(service.Name) {
-		allErrs = append(allErrs, errs.NewFieldInvalid("name", service.Name))
+		allErrs = append(allErrs, errs.NewFieldInvalid("name", service.Name, ""))
 	}
 	if !util.IsDNSSubdomain(service.Namespace) {
-		allErrs = append(allErrs, errs.NewFieldInvalid("namespace", service.Namespace))
+		allErrs = append(allErrs, errs.NewFieldInvalid("namespace", service.Namespace, ""))
 	}
 	if !util.IsValidPortNum(service.Spec.Port) {
-		allErrs = append(allErrs, errs.NewFieldInvalid("spec.port", service.Spec.Port))
+		allErrs = append(allErrs, errs.NewFieldInvalid("spec.port", service.Spec.Port, ""))
 	}
 	if len(service.Spec.Protocol) == 0 {
 		service.Spec.Protocol = "TCP"
@@ -464,7 +468,7 @@ func ValidateReplicationController(controller *api.ReplicationController) errs.V
 		allErrs = append(allErrs, errs.NewFieldRequired("name", controller.Name))
 	}
 	if !util.IsDNSSubdomain(controller.Namespace) {
-		allErrs = append(allErrs, errs.NewFieldInvalid("namespace", controller.Namespace))
+		allErrs = append(allErrs, errs.NewFieldInvalid("namespace", controller.Namespace, ""))
 	}
 	allErrs = append(allErrs, ValidateReplicationControllerSpec(&controller.Spec).Prefix("spec")...)
 	allErrs = append(allErrs, validateLabels(controller.Labels)...)
@@ -480,7 +484,7 @@ func ValidateReplicationControllerSpec(spec *api.ReplicationControllerSpec) errs
 		allErrs = append(allErrs, errs.NewFieldRequired("selector", spec.Selector))
 	}
 	if spec.Replicas < 0 {
-		allErrs = append(allErrs, errs.NewFieldInvalid("replicas", spec.Replicas))
+		allErrs = append(allErrs, errs.NewFieldInvalid("replicas", spec.Replicas, ""))
 	}
 
 	if spec.Template == nil {
@@ -488,14 +492,15 @@ func ValidateReplicationControllerSpec(spec *api.ReplicationControllerSpec) errs
 	} else {
 		labels := labels.Set(spec.Template.Labels)
 		if !selector.Matches(labels) {
-			allErrs = append(allErrs, errs.NewFieldInvalid("template.labels", spec.Template.Labels))
+			allErrs = append(allErrs, errs.NewFieldInvalid("template.labels", spec.Template.Labels, "selector does not match template"))
 		}
 		allErrs = append(allErrs, validateLabels(spec.Template.Labels).Prefix("template.labels")...)
 		allErrs = append(allErrs, ValidatePodTemplateSpec(spec.Template).Prefix("template")...)
-		// TODO: Provide better error message, current message is not intuitive:
+		// RestartPolicy has already been first-order validated as per ValidatePodTemplateSpec().
-		// e.g. "spec.template.restartPolicy: invalid value '{<nil> <nil> 0xe68308}"
+		if spec.Template.Spec.RestartPolicy.Always == nil {
-		if spec.Template.Spec.RestartPolicy.OnFailure != nil || spec.Template.Spec.RestartPolicy.Never != nil {
+			// TODO: should probably be Unsupported
-			allErrs = append(allErrs, errs.NewFieldInvalid("template.restartPolicy", spec.Template.Spec.RestartPolicy))
+			// TODO: api.RestartPolicy should have a String() method for nicer printing
+			allErrs = append(allErrs, errs.NewFieldInvalid("template.restartPolicy", spec.Template.Spec.RestartPolicy, "must be Always"))
 		}
 	}
 	return allErrs
@@ -514,7 +519,7 @@ func ValidateReadOnlyPersistentDisks(volumes []api.Volume) errs.ValidationErrorL
 	for _, vol := range volumes {
 		if vol.Source.GCEPersistentDisk != nil {
 			if vol.Source.GCEPersistentDisk.ReadOnly == false {
-				allErrs = append(allErrs, errs.NewFieldInvalid("GCEPersistentDisk.ReadOnly", false))
+				allErrs = append(allErrs, errs.NewFieldInvalid("GCEPersistentDisk.ReadOnly", false, "must be true"))
 			}
 		}
 	}
@@ -524,10 +529,10 @@ func ValidateReadOnlyPersistentDisks(volumes []api.Volume) errs.ValidationErrorL
 // ValidateBoundPod tests if required fields on a bound pod are set.
 func ValidateBoundPod(pod *api.BoundPod) (errors []error) {
 	if !util.IsDNSSubdomain(pod.Name) {
-		errors = append(errors, errs.NewFieldInvalid("name", pod.Name))
+		errors = append(errors, errs.NewFieldInvalid("name", pod.Name, ""))
 	}
 	if !util.IsDNSSubdomain(pod.Namespace) {
-		errors = append(errors, errs.NewFieldInvalid("namespace", pod.Namespace))
+		errors = append(errors, errs.NewFieldInvalid("namespace", pod.Namespace, ""))
 	}
 	containerManifest := &api.ContainerManifest{
 		Version:       "v1beta2",

pkg/api/validation/validation_test.go

@@ -29,7 +29,7 @@ import (
 
 func expectPrefix(t *testing.T, prefix string, errs errors.ValidationErrorList) {
 	for i := range errs {
-		if f, p := errs[i].(errors.ValidationError).Field, prefix; !strings.HasPrefix(f, p) {
+		if f, p := errs[i].(*errors.ValidationError).Field, prefix; !strings.HasPrefix(f, p) {
 			t.Errorf("expected prefix '%s' for field '%s' (%v)", p, f, errs[i])
 		}
 	}
@@ -69,10 +69,10 @@ func TestValidateVolumes(t *testing.T) {
 			continue
 		}
 		for i := range errs {
-			if errs[i].(errors.ValidationError).Type != v.T {
+			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 {
+			if errs[i].(*errors.ValidationError).Field != v.F {
 				t.Errorf("%s: expected errors to have field %s: %v", k, v.F, errs[i])
 			}
 		}
@@ -125,10 +125,10 @@ func TestValidatePorts(t *testing.T) {
 			t.Errorf("expected failure for %s", k)
 		}
 		for i := range errs {
-			if errs[i].(errors.ValidationError).Type != v.T {
+			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 {
+			if errs[i].(*errors.ValidationError).Field != v.F {
 				t.Errorf("%s: expected errors to have field %s: %v", k, v.F, errs[i])
 			}
 		}
@@ -995,7 +995,7 @@ func TestValidateReplicationController(t *testing.T) {
 			t.Errorf("expected failure for %s", k)
 		}
 		for i := range errs {
-			field := errs[i].(errors.ValidationError).Field
+			field := errs[i].(*errors.ValidationError).Field
 			if !strings.HasPrefix(field, "spec.template.") &&
 				field != "name" &&
 				field != "namespace" &&
@@ -1078,7 +1078,7 @@ func TestValidateMinion(t *testing.T) {
 			t.Errorf("expected failure for %s", k)
 		}
 		for i := range errs {
-			field := errs[i].(errors.ValidationError).Field
+			field := errs[i].(*errors.ValidationError).Field
 			if field != "name" &&
 				field != "label" {
 				t.Errorf("%s: missing prefix for: %v", k, errs[i])

pkg/config/config.go

@@ -17,10 +17,13 @@ limitations under the License.
 package config
 
 import (
+	"fmt"
+
 	errs "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/meta"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/client"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/runtime"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
 )
 
 // ClientFunc returns the RESTClient defined for given resource
@@ -30,33 +33,33 @@ type ClientFunc func(mapping *meta.RESTMapping) (*client.RESTClient, error)
 // be valid API type. It requires ObjectTyper to parse the Version and Kind and
 // RESTMapper to get the resource URI and REST client that knows how to create
 // given type
-func CreateObjects(typer runtime.ObjectTyper, mapper meta.RESTMapper, clientFor ClientFunc, objects []runtime.Object) errs.ValidationErrorList {
+func CreateObjects(typer runtime.ObjectTyper, mapper meta.RESTMapper, clientFor ClientFunc, objects []runtime.Object) util.ErrorList {
-	allErrors := errs.ValidationErrorList{}
+	allErrors := util.ErrorList{}
 	for i, obj := range objects {
 		version, kind, err := typer.ObjectVersionAndKind(obj)
 		if err != nil {
-			reportError(&allErrors, i, errs.NewFieldInvalid("kind", obj))
+			allErrors = append(allErrors, fmt.Errorf("Config.item[%d] kind: %v", i, err))
 			continue
 		}
 
 		mapping, err := mapper.RESTMapping(version, kind)
 		if err != nil {
-			reportError(&allErrors, i, errs.NewFieldNotSupported("mapping", err))
+			allErrors = append(allErrors, fmt.Errorf("Config.item[%d] mapping: %v", i, err))
 			continue
 		}
 
 		client, err := clientFor(mapping)
 		if err != nil {
-			reportError(&allErrors, i, errs.NewFieldNotSupported("client", obj))
+			allErrors = append(allErrors, fmt.Errorf("Config.item[%d] client: %v", i, err))
 			continue
 		}
 
 		if err := CreateObject(client, mapping, obj); err != nil {
-			reportError(&allErrors, i, *err)
+			allErrors = append(allErrors, fmt.Errorf("Config.item[%d]: %v", i, err))
 		}
 	}
 
-	return allErrors.Prefix("Config")
+	return allErrors
 }
 
 // CreateObject creates the obj using the provided clients and the resource URI
@@ -65,28 +68,19 @@ func CreateObjects(typer runtime.ObjectTyper, mapper meta.RESTMapper, clientFor
 func CreateObject(client *client.RESTClient, mapping *meta.RESTMapping, obj runtime.Object) *errs.ValidationError {
 	name, err := mapping.MetadataAccessor.Name(obj)
 	if err != nil || name == "" {
-		e := errs.NewFieldRequired("name", err)
+		return errs.NewFieldRequired("name", err)
-		return &e
 	}
 
 	namespace, err := mapping.Namespace(obj)
 	if err != nil {
-		e := errs.NewFieldRequired("namespace", err)
+		return errs.NewFieldRequired("namespace", err)
-		return &e
 	}
 
 	// TODO: This should be using RESTHelper
 	err = client.Post().Path(mapping.Resource).Namespace(namespace).Body(obj).Do().Error()
 	if err != nil {
-		return &errs.ValidationError{errs.ValidationErrorTypeInvalid, name, err}
+		return errs.NewFieldInvalid(name, obj, err.Error())
 	}
 
 	return nil
 }
-
-// reportError reports the single item validation error and properly set the
-// prefix and index to match the Config item JSON index
-func reportError(allErrs *errs.ValidationErrorList, index int, err errs.ValidationError) {
-	i := errs.ValidationErrorList{}
-	*allErrs = append(*allErrs, append(i, err).PrefixIndex(index).Prefix("item")...)
-}

pkg/config/config_test.go

@@ -21,10 +21,10 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"strings"
 	"testing"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
-	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/latest"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/meta"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/client"
@@ -92,13 +92,9 @@ func TestCreateNoNameItem(t *testing.T) {
 		t.Errorf("Expected required value error for missing name")
 	}
 
-	e := errs[0].(errors.ValidationError)
+	errStr := errs[0].Error()
-	if errors.ValueOf(e.Type) != "required value" {
+	if !strings.Contains(errStr, "Config.item[0]: name") {
-		t.Errorf("Expected ValidationErrorTypeRequired error, got %#v", e)
+		t.Errorf("Expected 'Config.item[0]: name' in error string, got '%s'", errStr)
-	}
-
-	if e.Field != "Config.item[0].name" {
-		t.Errorf("Expected 'Config.item[0].name' as error field, got '%#v'", e.Field)
 	}
 }
 
@@ -121,13 +117,9 @@ func TestCreateInvalidItem(t *testing.T) {
 		t.Errorf("Expected invalid value error for kind")
 	}
 
-	e := errs[0].(errors.ValidationError)
+	errStr := errs[0].Error()
-	if errors.ValueOf(e.Type) != "invalid value" {
+	if !strings.Contains(errStr, "Config.item[0] kind") {
-		t.Errorf("Expected ValidationErrorTypeInvalid error, got %#v", e)
+		t.Errorf("Expected 'Config.item[0] kind' in error string, got '%s'", errStr)
-	}
-
-	if e.Field != "Config.item[0].kind" {
-		t.Errorf("Expected 'Config.item[0].kind' as error field, got '%#v'", e.Field)
 	}
 }
 
@@ -153,12 +145,8 @@ func TestCreateNoClientItems(t *testing.T) {
 		t.Errorf("Expected invalid value error for client")
 	}
 
-	e := errs[0].(errors.ValidationError)
+	errStr := errs[0].Error()
-	if errors.ValueOf(e.Type) != "unsupported value" {
+	if !strings.Contains(errStr, "Config.item[0] client") {
-		t.Errorf("Expected ValidationErrorTypeUnsupported error, got %#v", e)
+		t.Errorf("Expected 'Config.item[0] client' in error string, got '%s'", errStr)
-	}
-
-	if e.Field != "Config.item[0].client" {
-		t.Errorf("Expected 'Config.item[0].client' as error field, got '%#v'", e.Field)
 	}
 }

pkg/kubectl/cmd/createall.go

@@ -17,49 +17,44 @@ limitations under the License.
 package cmd
 
 import (
+	"fmt"
 	"io"
 
-	errs "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/meta"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/client"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/config"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/runtime"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
 	"github.com/golang/glog"
 	"github.com/spf13/cobra"
 	"gopkg.in/v1/yaml"
 )
 
 // DataToObjects converts the raw JSON data into API objects
-func DataToObjects(m meta.RESTMapper, t runtime.ObjectTyper, data []byte) (result []runtime.Object, errors errs.ValidationErrorList) {
+func DataToObjects(m meta.RESTMapper, t runtime.ObjectTyper, data []byte) (result []runtime.Object, errors util.ErrorList) {
 	configObj := []runtime.RawExtension{}
 
 	if err := yaml.Unmarshal(data, &configObj); err != nil {
-		errors = append(errors, errs.NewFieldInvalid("unmarshal", err))
+		errors = append(errors, fmt.Errorf("config unmarshal: %v", err))
-		return result, errors.Prefix("Config")
+		return result, errors
 	}
 
 	for i, in := range configObj {
 		version, kind, err := t.DataVersionAndKind(in.RawJSON)
 		if err != nil {
-			itemErrs := errs.ValidationErrorList{}
+			errors = append(errors, fmt.Errorf("item[%d] kind: %v", i, err))
-			itemErrs = append(itemErrs, errs.NewFieldInvalid("kind", string(in.RawJSON)))
-			errors = append(errors, itemErrs.PrefixIndex(i).Prefix("item")...)
 			continue
 		}
 
 		mapping, err := m.RESTMapping(version, kind)
 		if err != nil {
-			itemErrs := errs.ValidationErrorList{}
+			errors = append(errors, fmt.Errorf("item[%d] mapping: %v", i, err))
-			itemErrs = append(itemErrs, errs.NewFieldRequired("mapping", err))
-			errors = append(errors, itemErrs.PrefixIndex(i).Prefix("item")...)
 			continue
 		}
 
 		obj, err := mapping.Codec.Decode(in.RawJSON)
 		if err != nil {
-			itemErrs := errs.ValidationErrorList{}
+			errors = append(errors, fmt.Errorf("item[%d] decode: %v", i, err))
-			itemErrs = append(itemErrs, errs.NewFieldInvalid("decode", err))
-			errors = append(errors, itemErrs.PrefixIndex(i).Prefix("item")...)
 			continue
 		}
 		result = append(result, obj)

pkg/registry/service/rest.go

@@ -100,8 +100,7 @@ func (rs *REST) Create(ctx api.Context, obj runtime.Object) (<-chan apiserver.RE
 	} else {
 		// Try to respect the requested IP.
 		if err := rs.portalMgr.Allocate(net.ParseIP(service.Spec.PortalIP)); err != nil {
-			// TODO: Differentiate "IP already allocated" from real errors.
+			el := errors.ValidationErrorList{errors.NewFieldInvalid("spec.portalIP", service.Spec.PortalIP, err.Error())}
-			el := errors.ValidationErrorList{errors.NewFieldInvalid("spec.portalIP", service.Spec.PortalIP)}
 			return nil, errors.NewInvalid("service", service.Name, el)
 		}
 	}
@@ -222,9 +221,8 @@ func (rs *REST) Update(ctx api.Context, obj runtime.Object) (<-chan apiserver.RE
 		if err != nil {
 			return nil, err
 		}
-		if service.Spec.PortalIP != cur.Spec.PortalIP {
+		if service.Spec.PortalIP != "" && service.Spec.PortalIP != cur.Spec.PortalIP {
-			// TODO: Would be nice to pass "field is immutable" to users.
+			el := errors.ValidationErrorList{errors.NewFieldInvalid("spec.portalIP", service.Spec.PortalIP, "field is immutable")}
-			el := errors.ValidationErrorList{errors.NewFieldInvalid("spec.portalIP", service.Spec.PortalIP)}
 			return nil, errors.NewInvalid("service", service.Name, el)
 		}
 		// Copy over non-user fields.

pkg/tools/etcd_tools_watch.go

@@ -50,7 +50,8 @@ func ParseWatchResourceVersion(resourceVersion, kind string) (uint64, error) {
 	}
 	version, err := strconv.ParseUint(resourceVersion, 10, 64)
 	if err != nil {
-		return 0, errors.NewInvalid(kind, "", errors.ValidationErrorList{errors.NewFieldInvalid("resourceVersion", resourceVersion)})
+		// TODO: Does this need to be a ValidationErrorList?  I can't convince myself it does.
+		return 0, errors.NewInvalid(kind, "", errors.ValidationErrorList{errors.NewFieldInvalid("resourceVersion", resourceVersion, err.Error())})
 	}
 	return version + 1, nil
 }

test/integration/auth_test.go

@@ -273,7 +273,7 @@ func getTestRequests() []struct {
 		// Normal methods on services
 		{"GET", "/api/v1beta1/services", "", code200},
 		{"POST", "/api/v1beta1/services" + syncFlags, aService, code200},
-		{"PUT", "/api/v1beta1/services/a" + syncFlags, aService, code422}, // TODO: GET and put back server-provided fields to avoid a 422
+		{"PUT", "/api/v1beta1/services/a" + syncFlags, aService, code409}, // TODO: GET and put back server-provided fields to avoid a 422
 		{"GET", "/api/v1beta1/services", "", code200},
 		{"GET", "/api/v1beta1/services/a", "", code200},
 		{"DELETE", "/api/v1beta1/services/a" + syncFlags, "", code200},