diff --git a/pkg/api/errors/errors.go b/pkg/api/errors/errors.go index d1172d03fa..69d0cf8bb4 100644 --- a/pkg/api/errors/errors.go +++ b/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{ - api.Status{ - Status: api.StatusFailure, - Code: http.StatusBadRequest, - Reason: api.StatusReasonBadRequest, - Details: &api.StatusDetails{ - Causes: []api.StatusCause{ - {Message: reason}, - }, + return &StatusError{api.Status{ + Status: api.StatusFailure, + Code: http.StatusBadRequest, + Reason: api.StatusReasonBadRequest, + Details: &api.StatusDetails{ + Causes: []api.StatusCause{ + {Message: reason}, }, }, - } + }} } // NewInternalError returns an error indicating the item is invalid and cannot be processed. diff --git a/pkg/api/errors/errors_test.go b/pkg/api/errors/errors_test.go index 1e56ef94ac..d3f765c1da 100644 --- a/pkg/api/errors/errors_test.go +++ b/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", diff --git a/pkg/api/errors/validation.go b/pkg/api/errors/validation.go index a9d711466a..0c05a4a1a3 100644 --- a/pkg/api/errors/validation.go +++ b/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 { - return fmt.Sprintf("%s: %v '%v'", v.Field, ValueOf(v.Type), v.BadValue) +var _ error = &ValidationError{} + +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" -func NewFieldRequired(field string, value interface{}) ValidationError { - return ValidationError{ValidationErrorTypeRequired, field, value} +// NewFieldRequired returns a *ValidationError indicating "value required" +func NewFieldRequired(field string, value interface{}) *ValidationError { + return &ValidationError{ValidationErrorTypeRequired, field, value, ""} } -// NewFieldInvalid returns a ValidationError indicating "invalid value" -func NewFieldInvalid(field string, value interface{}) ValidationError { - return ValidationError{ValidationErrorTypeInvalid, field, value} +// NewFieldInvalid returns a *ValidationError indicating "invalid value" +func NewFieldInvalid(field string, value interface{}, detail string) *ValidationError { + return &ValidationError{ValidationErrorTypeInvalid, field, value, detail} } -// NewFieldNotSupported returns a ValidationError indicating "unsupported value" -func NewFieldNotSupported(field string, value interface{}) ValidationError { - return ValidationError{ValidationErrorTypeNotSupported, field, value} +// NewFieldNotSupported returns a *ValidationError indicating "unsupported value" +func NewFieldNotSupported(field string, value interface{}) *ValidationError { + return &ValidationError{ValidationErrorTypeNotSupported, field, value, ""} } -// NewFieldForbidden returns a ValidationError indicating "forbidden" -func NewFieldForbidden(field string, value interface{}) ValidationError { - return ValidationError{ValidationErrorTypeForbidden, field, value} +// NewFieldForbidden returns a *ValidationError indicating "forbidden" +func NewFieldForbidden(field string, value interface{}) *ValidationError { + return &ValidationError{ValidationErrorTypeForbidden, field, value, ""} } -// NewFieldDuplicate returns a ValidationError indicating "duplicate value" -func NewFieldDuplicate(field string, value interface{}) ValidationError { - return ValidationError{ValidationErrorTypeDuplicate, field, value} +// NewFieldDuplicate returns a *ValidationError indicating "duplicate value" +func NewFieldDuplicate(field string, value interface{}) *ValidationError { + return &ValidationError{ValidationErrorTypeDuplicate, field, value, ""} } -// NewFieldNotFound returns a ValidationError indicating "value not found" -func NewFieldNotFound(field string, value interface{}) ValidationError { - return ValidationError{ValidationErrorTypeNotFound, field, value} +// NewFieldNotFound returns a *ValidationError indicating "value not found" +func NewFieldNotFound(field string, value interface{}) *ValidationError { + 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 { diff --git a/pkg/api/errors/validation_test.go b/pkg/api/errors/validation_test.go index 0ed29359a3..3b2493e842 100644 --- a/pkg/api/errors/validation_test.go +++ b/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() - if !strings.Contains(s, "foo") || !strings.Contains(s, "bar") || !strings.Contains(s, ValueOf(ValidationErrorTypeInvalid)) { + s := NewFieldInvalid("foo", "bar", "deet").Error() + 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) } } diff --git a/pkg/api/validation/events.go b/pkg/api/validation/events.go index 2ae27ea357..96f62d1b2f 100644 --- a/pkg/api/validation/events.go +++ b/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 } diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index 6cba612952..580d451caf 100644 --- a/pkg/api/validation/validation.go +++ b/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: - // e.g. "spec.template.restartPolicy: invalid value '{ 0xe68308}" - if spec.Template.Spec.RestartPolicy.OnFailure != nil || spec.Template.Spec.RestartPolicy.Never != nil { - allErrs = append(allErrs, errs.NewFieldInvalid("template.restartPolicy", spec.Template.Spec.RestartPolicy)) + // RestartPolicy has already been first-order validated as per ValidatePodTemplateSpec(). + if spec.Template.Spec.RestartPolicy.Always == nil { + // TODO: should probably be Unsupported + // 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", diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go index bedc22d226..b2a3ee107d 100644 --- a/pkg/api/validation/validation_test.go +++ b/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]) diff --git a/pkg/config/config.go b/pkg/config/config.go index a73c84d452..930aa0f408 100644 --- a/pkg/config/config.go +++ b/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 { - allErrors := errs.ValidationErrorList{} +func CreateObjects(typer runtime.ObjectTyper, mapper meta.RESTMapper, clientFor ClientFunc, objects []runtime.Object) util.ErrorList { + 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 &e + return errs.NewFieldRequired("name", err) } namespace, err := mapping.Namespace(obj) if err != nil { - e := errs.NewFieldRequired("namespace", err) - return &e + return errs.NewFieldRequired("namespace", err) } // 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")...) -} diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 0c318f4e21..799ae67fec 100644 --- a/pkg/config/config_test.go +++ b/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) - if errors.ValueOf(e.Type) != "required value" { - t.Errorf("Expected ValidationErrorTypeRequired error, got %#v", e) - } - - if e.Field != "Config.item[0].name" { - t.Errorf("Expected 'Config.item[0].name' as error field, got '%#v'", e.Field) + errStr := errs[0].Error() + if !strings.Contains(errStr, "Config.item[0]: name") { + t.Errorf("Expected 'Config.item[0]: name' in error string, got '%s'", errStr) } } @@ -121,13 +117,9 @@ func TestCreateInvalidItem(t *testing.T) { t.Errorf("Expected invalid value error for kind") } - e := errs[0].(errors.ValidationError) - if errors.ValueOf(e.Type) != "invalid value" { - t.Errorf("Expected ValidationErrorTypeInvalid error, got %#v", e) - } - - if e.Field != "Config.item[0].kind" { - t.Errorf("Expected 'Config.item[0].kind' as error field, got '%#v'", e.Field) + errStr := errs[0].Error() + if !strings.Contains(errStr, "Config.item[0] kind") { + t.Errorf("Expected 'Config.item[0] kind' in error string, got '%s'", errStr) } } @@ -153,12 +145,8 @@ func TestCreateNoClientItems(t *testing.T) { t.Errorf("Expected invalid value error for client") } - e := errs[0].(errors.ValidationError) - if errors.ValueOf(e.Type) != "unsupported value" { - t.Errorf("Expected ValidationErrorTypeUnsupported error, got %#v", e) - } - - if e.Field != "Config.item[0].client" { - t.Errorf("Expected 'Config.item[0].client' as error field, got '%#v'", e.Field) + errStr := errs[0].Error() + if !strings.Contains(errStr, "Config.item[0] client") { + t.Errorf("Expected 'Config.item[0] client' in error string, got '%s'", errStr) } } diff --git a/pkg/kubectl/cmd/createall.go b/pkg/kubectl/cmd/createall.go index c0b87ab5d7..80e145a641 100644 --- a/pkg/kubectl/cmd/createall.go +++ b/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)) - return result, errors.Prefix("Config") + errors = append(errors, fmt.Errorf("config unmarshal: %v", err)) + return result, errors } for i, in := range configObj { version, kind, err := t.DataVersionAndKind(in.RawJSON) if err != nil { - itemErrs := errs.ValidationErrorList{} - itemErrs = append(itemErrs, errs.NewFieldInvalid("kind", string(in.RawJSON))) - errors = append(errors, itemErrs.PrefixIndex(i).Prefix("item")...) + errors = append(errors, fmt.Errorf("item[%d] kind: %v", i, err)) continue } mapping, err := m.RESTMapping(version, kind) if err != nil { - itemErrs := errs.ValidationErrorList{} - itemErrs = append(itemErrs, errs.NewFieldRequired("mapping", err)) - errors = append(errors, itemErrs.PrefixIndex(i).Prefix("item")...) + errors = append(errors, fmt.Errorf("item[%d] mapping: %v", i, err)) continue } obj, err := mapping.Codec.Decode(in.RawJSON) if err != nil { - itemErrs := errs.ValidationErrorList{} - itemErrs = append(itemErrs, errs.NewFieldInvalid("decode", err)) - errors = append(errors, itemErrs.PrefixIndex(i).Prefix("item")...) + errors = append(errors, fmt.Errorf("item[%d] decode: %v", i, err)) continue } result = append(result, obj) diff --git a/pkg/registry/service/rest.go b/pkg/registry/service/rest.go index 64febd65a3..56a662a88d 100644 --- a/pkg/registry/service/rest.go +++ b/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)} + el := errors.ValidationErrorList{errors.NewFieldInvalid("spec.portalIP", service.Spec.PortalIP, err.Error())} 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 { - // TODO: Would be nice to pass "field is immutable" to users. - el := errors.ValidationErrorList{errors.NewFieldInvalid("spec.portalIP", service.Spec.PortalIP)} + if service.Spec.PortalIP != "" && service.Spec.PortalIP != cur.Spec.PortalIP { + el := errors.ValidationErrorList{errors.NewFieldInvalid("spec.portalIP", service.Spec.PortalIP, "field is immutable")} return nil, errors.NewInvalid("service", service.Name, el) } // Copy over non-user fields. diff --git a/pkg/tools/etcd_tools_watch.go b/pkg/tools/etcd_tools_watch.go index c736fec305..eab0b1753c 100644 --- a/pkg/tools/etcd_tools_watch.go +++ b/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 } diff --git a/test/integration/auth_test.go b/test/integration/auth_test.go index 858c192489..6916fae267 100644 --- a/test/integration/auth_test.go +++ b/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},