diff --git a/pkg/api/errors/errors.go b/pkg/api/errors/errors.go index 70c43b40c9..149653e022 100644 --- a/pkg/api/errors/errors.go +++ b/pkg/api/errors/errors.go @@ -19,53 +19,85 @@ package errors import ( "fmt" "strings" + + "github.com/golang/glog" ) -// ValidationErrorEnum is a type of validation error. -type ValidationErrorEnum string +// ValidationErrorType is a machine readable value providing more detail about why +// a field is invalid. These values are expected to match 1-1 with +// CauseType in api/types.go. +type ValidationErrorType string -// These are known errors of validation. const ( - Invalid ValidationErrorEnum = "invalid value" - NotSupported ValidationErrorEnum = "unsupported value" - Duplicate ValidationErrorEnum = "duplicate value" - NotFound ValidationErrorEnum = "not found" + // ValidationErrorTypeNotFound is used to report failure to find a requested value + // (e.g. looking up an ID). + ValidationErrorTypeNotFound ValidationErrorType = "fieldValueNotFound" + // ValidationErrorTypeRequired is used to report required values that are not + // provided (e.g. empty strings, null values, or empty arrays). + ValidationErrorTypeRequired ValidationErrorType = "fieldValueRequired" + // ValidationErrorTypeDuplicate is used to report collisions of values that must be + // unique (e.g. unique IDs). + ValidationErrorTypeDuplicate ValidationErrorType = "fieldValueDuplicate" + // ValidationErrorTypeInvalid is used to report malformed values (e.g. failed regex + // match). + ValidationErrorTypeInvalid ValidationErrorType = "fieldValueInvalid" + // ValidationErrorTypeNotSupported is used to report valid (as per formatting rules) + // values that can not be handled (e.g. an enumerated string). + ValidationErrorTypeNotSupported ValidationErrorType = "fieldValueNotSupported" ) +func ValueOf(t ValidationErrorType) string { + switch t { + case ValidationErrorTypeNotFound: + return "not found" + case ValidationErrorTypeRequired: + return "required value" + case ValidationErrorTypeDuplicate: + return "duplicate value" + case ValidationErrorTypeInvalid: + return "invalid value" + case ValidationErrorTypeNotSupported: + return "unsupported value" + default: + glog.Errorf("unrecognized validation type: %#v", t) + return "" + } +} + // ValidationError is an implementation of the 'error' interface, which represents an error of validation. type ValidationError struct { - Type ValidationErrorEnum + Type ValidationErrorType Field string BadValue interface{} } func (v ValidationError) Error() string { - return fmt.Sprintf("%s: %v '%v'", v.Field, v.Type, v.BadValue) + return fmt.Sprintf("%s: %v '%v'", v.Field, ValueOf(v.Type), v.BadValue) } -// NewInvalid returns a ValidationError indicating "invalid value". Use this to -// report malformed values (e.g. failed regex match) or missing "required" fields. +// NewInvalid returns a ValidationError indicating "value required" +func NewRequired(field string, value interface{}) ValidationError { + return ValidationError{ValidationErrorTypeRequired, field, value} +} + +// NewInvalid returns a ValidationError indicating "invalid value" func NewInvalid(field string, value interface{}) ValidationError { - return ValidationError{Invalid, field, value} + return ValidationError{ValidationErrorTypeInvalid, field, value} } -// NewNotSupported returns a ValidationError indicating "unsuported value". Use -// this to report valid (as per formatting rules) values that can not be handled -// (e.g. an enumerated string). +// NewNotSupported returns a ValidationError indicating "unsupported value" func NewNotSupported(field string, value interface{}) ValidationError { - return ValidationError{NotSupported, field, value} + return ValidationError{ValidationErrorTypeNotSupported, field, value} } -// NewDuplicate returns a ValidationError indicating "duplicate value". Use this -// to report collisions of values that must be unique (e.g. unique IDs). +// NewDuplicate returns a ValidationError indicating "duplicate value" func NewDuplicate(field string, value interface{}) ValidationError { - return ValidationError{Duplicate, field, value} + return ValidationError{ValidationErrorTypeDuplicate, field, value} } -// NewNotFound returns a ValidationError indicating "value not found". Use this -// to report failure to find a requested value (e.g. looking up an ID). +// NewNotFound returns a ValidationError indicating "value not found" func NewNotFound(field string, value interface{}) ValidationError { - return ValidationError{NotFound, field, value} + return ValidationError{ValidationErrorTypeNotFound, field, value} } // ErrorList is a collection of errors. This does not implement the error @@ -74,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 @@ -97,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)) +} diff --git a/pkg/api/errors/errors_test.go b/pkg/api/errors/errors_test.go index d42345bbca..bd4a72a154 100644 --- a/pkg/api/errors/errors_test.go +++ b/pkg/api/errors/errors_test.go @@ -18,29 +18,34 @@ package errors import ( "fmt" + "strings" "testing" ) func TestMakeFuncs(t *testing.T) { testCases := []struct { fn func() ValidationError - expected ValidationErrorEnum + expected ValidationErrorType }{ { func() ValidationError { return NewInvalid("f", "v") }, - Invalid, + ValidationErrorTypeInvalid, }, { func() ValidationError { return NewNotSupported("f", "v") }, - NotSupported, + ValidationErrorTypeNotSupported, }, { func() ValidationError { return NewDuplicate("f", "v") }, - Duplicate, + ValidationErrorTypeDuplicate, }, { func() ValidationError { return NewNotFound("f", "v") }, - NotFound, + ValidationErrorTypeNotFound, + }, + { + func() ValidationError { return NewRequired("f", "v") }, + ValidationErrorTypeRequired, }, } @@ -52,8 +57,21 @@ func TestMakeFuncs(t *testing.T) { } } +func TestValidationError(t *testing.T) { + s := NewInvalid("foo", "bar").Error() + if !strings.Contains(s, "foo") || !strings.Contains(s, "bar") || !strings.Contains(s, ValueOf(ValidationErrorTypeInvalid)) { + t.Errorf("error message did not contain expected values, got %s", s) + } +} + func TestErrorList(t *testing.T) { errList := ErrorList{} + if a := errList.ToError(); a != nil { + t.Errorf("unexpected non-nil error for empty list: %v", a) + } + if a := errorListInternal(errList).Error(); a != "" { + t.Errorf("expected empty string, got %v", a) + } errList = append(errList, NewInvalid("field", "value")) // The fact that this compiles is the test. } @@ -83,3 +101,63 @@ func TestErrorListToError(t *testing.T) { } } } + +func TestErrListPrefix(t *testing.T) { + testCases := []struct { + Err ValidationError + Expected string + }{ + { + NewNotFound("[0].bar", "value"), + "foo[0].bar", + }, + { + NewInvalid("field", "value"), + "foo.field", + }, + { + NewDuplicate("", "value"), + "foo", + }, + } + for _, testCase := range testCases { + errList := ErrorList{testCase.Err} + prefix := errList.Prefix("foo") + if prefix == nil || len(prefix) != len(errList) { + t.Errorf("Prefix should return self") + } + if e, a := testCase.Expected, errList[0].(ValidationError).Field; e != a { + t.Errorf("expected %s, got %s", e, a) + } + } +} + +func TestErrListPrefixIndex(t *testing.T) { + testCases := []struct { + Err ValidationError + Expected string + }{ + { + NewNotFound("[0].bar", "value"), + "[1][0].bar", + }, + { + NewInvalid("field", "value"), + "[1].field", + }, + { + NewDuplicate("", "value"), + "[1]", + }, + } + for _, testCase := range testCases { + errList := ErrorList{testCase.Err} + prefix := errList.PrefixIndex(1) + if prefix == nil || len(prefix) != len(errList) { + t.Errorf("PrefixIndex should return self") + } + if e, a := testCase.Expected, errList[0].(ValidationError).Field; e != a { + t.Errorf("expected %s, got %s", e, a) + } + } +} diff --git a/pkg/api/types.go b/pkg/api/types.go index 1d290b4d42..b1dfaeb384 100644 --- a/pkg/api/types.go +++ b/pkg/api/types.go @@ -362,13 +362,13 @@ type Status struct { JSONBase `json:",inline" yaml:",inline"` // One of: "success", "failure", "working" (for operations not yet completed) Status string `json:"status,omitempty" yaml:"status,omitempty"` - // A human readable description of the status of this operation. + // A human-readable description of the status of this operation. Message string `json:"message,omitempty" yaml:"message,omitempty"` - // A machine readable description of why this operation is in the + // A machine-readable description of why this operation is in the // "failure" or "working" status. If this value is empty there // is no information available. A Reason clarifies an HTTP status // code but does not override it. - Reason ReasonType `json:"reason,omitempty" yaml:"reason,omitempty"` + Reason StatusReason `json:"reason,omitempty" yaml:"reason,omitempty"` // Extended data associated with the reason. Each reason may define its // own extended details. This field is optional and the data returned // is not guaranteed to conform to any schema except that defined by @@ -385,12 +385,15 @@ type Status struct { // and should assume that any attribute may be empty, invalid, or under // defined. type StatusDetails struct { - // The ID attribute of the resource associated with the status ReasonType + // The ID attribute of the resource associated with the status StatusReason // (when there is a single ID which can be described). ID string `json:"id,omitempty" yaml:"id,omitempty"` - // The kind attribute of the resource associated with the status ReasonType. + // The kind attribute of the resource associated with the status StatusReason. // On some operations may differ from the requested resource Kind. Kind string `json:"kind,omitempty" yaml:"kind,omitempty"` + // The Causes array includes more details associated with the StatusReason + // failure. Not all StatusReasons may provide detailed causes. + Causes []StatusCause `json:"causes,omitempty" yaml:"causes,omitempty"` } // Values of Status.Status @@ -400,19 +403,19 @@ const ( StatusWorking = "working" ) -// ReasonType is an enumeration of possible failure causes. Each ReasonType +// StatusReason is an enumeration of possible failure causes. Each StatusReason // must map to a single HTTP status code, but multiple reasons may map // to the same HTTP status code. // TODO: move to apiserver -type ReasonType string +type StatusReason string const ( - // ReasonTypeUnknown means the server has declined to indicate a specific reason. + // StatusReasonUnknown means the server has declined to indicate a specific reason. // The details field may contain other information about this error. // Status code 500. - ReasonTypeUnknown ReasonType = "" + StatusReasonUnknown StatusReason = "" - // ReasonTypeWorking means the server is processing this request and will complete + // StatusReasonWorking means the server is processing this request and will complete // at a future time. // Details (optional): // "kind" string - the name of the resource being referenced ("operation" today) @@ -422,7 +425,7 @@ const ( // "Location" - HTTP header populated with a URL that can retrieved the final // status of this operation. // Status code 202 - ReasonTypeWorking ReasonType = "working" + StatusReasonWorking StatusReason = "working" // ResourceTypeNotFound means one or more resources required for this operation // could not be found. @@ -432,21 +435,78 @@ const ( // resource. // "id" string - the identifier of the missing resource // Status code 404 - ReasonTypeNotFound ReasonType = "not_found" + StatusReasonNotFound StatusReason = "not_found" - // ReasonTypeAlreadyExists means the resource you are creating already exists. + // StatusReasonAlreadyExists means the resource you are creating already exists. // Details (optional): // "kind" string - the kind attribute of the conflicting resource // "id" string - the identifier of the conflicting resource // Status code 409 - ReasonTypeAlreadyExists ReasonType = "already_exists" + StatusReasonAlreadyExists StatusReason = "already_exists" // ResourceTypeConflict means the requested update operation cannot be completed // due to a conflict in the operation. The client may need to alter the request. // Each resource may define custom details that indicate the nature of the // conflict. // Status code 409 - ReasonTypeConflict ReasonType = "conflict" + StatusReasonConflict StatusReason = "conflict" + + // ResourceTypeInvalid means the requested create or update operation cannot be + // completed due to invalid data provided as part of the request. The client may + // need to alter the request. When set, the client may use the StatusDetails + // message field as a summary of the issues encountered. + // Details (optional): + // "kind" string - the kind attribute of the invalid resource + // "id" string - the identifier of the invalid resource + // "causes" - one or more StatusCause entries indicating the data in the + // provided resource that was invalid. The code, message, and + // field attributes will be set. + // Status code 422 + StatusReasonInvalid StatusReason = "invalid" +) + +// StatusCause provides more information about an api.Status failure, including +// cases when multiple errors are encountered. +type StatusCause struct { + // A machine-readable description of the cause of the error. If this value is + // empty there is no information available. + Type CauseType `json:"reason,omitempty" yaml:"reason,omitempty"` + // A human-readable description of the cause of the error. This field may be + // presented as-is to a reader. + Message string `json:"message,omitempty" yaml:"message,omitempty"` + // The field of the resource that has caused this error, as named by its JSON + // serialization. May include dot and postfix notation for nested attributes. + // Arrays are zero-indexed. Fields may appear more than once in an array of + // causes due to fields having multiple errors. + // Optional. + // + // Examples: + // "name" - the field "name" on the current resource + // "items[0].name" - the field "name" on the first array entry in "items" + Field string `json:"field,omitempty" yaml:"field,omitempty"` +} + +// CauseType is a machine readable value providing more detail about what +// occured in a status response. An operation may have multiple causes for a +// status (whether failure, success, or working). +type CauseType string + +const ( + // CauseTypeFieldValueNotFound is used to report failure to find a requested value + // (e.g. looking up an ID). + CauseTypeFieldValueNotFound CauseType = "fieldValueNotFound" + // CauseTypeFieldValueInvalid is used to report required values that are not + // provided (e.g. empty strings, null values, or empty arrays). + CauseTypeFieldValueRequired CauseType = "fieldValueRequired" + // CauseTypeFieldValueDuplicate is used to report collisions of values that must be + // unique (e.g. unique IDs). + CauseTypeFieldValueDuplicate CauseType = "fieldValueDuplicate" + // CauseTypeFieldValueInvalid is used to report malformed values (e.g. failed regex + // match). + CauseTypeFieldValueInvalid CauseType = "fieldValueInvalid" + // CauseTypeFieldValueNotSupported is used to report valid (as per formatting rules) + // values that can not be handled (e.g. an enumerated string). + CauseTypeFieldValueNotSupported CauseType = "fieldValueNotSupported" ) // ServerOp is an operation delivered to API clients. diff --git a/pkg/api/v1beta1/types.go b/pkg/api/v1beta1/types.go index a076b5986b..74468ce396 100644 --- a/pkg/api/v1beta1/types.go +++ b/pkg/api/v1beta1/types.go @@ -372,7 +372,7 @@ type Status struct { // "failure" or "working" status. If this value is empty there // is no information available. A Reason clarifies an HTTP status // code but does not override it. - Reason ReasonType `json:"reason,omitempty" yaml:"reason,omitempty"` + Reason StatusReason `json:"reason,omitempty" yaml:"reason,omitempty"` // Extended data associated with the reason. Each reason may define its // own extended details. This field is optional and the data returned // is not guaranteed to conform to any schema except that defined by @@ -389,12 +389,15 @@ type Status struct { // and should assume that any attribute may be empty, invalid, or under // defined. type StatusDetails struct { - // The ID attribute of the resource associated with the status ReasonType + // The ID attribute of the resource associated with the status StatusReason // (when there is a single ID which can be described). ID string `json:"id,omitempty" yaml:"id,omitempty"` - // The kind attribute of the resource associated with the status ReasonType. + // The kind attribute of the resource associated with the status StatusReason. // On some operations may differ from the requested resource Kind. Kind string `json:"kind,omitempty" yaml:"kind,omitempty"` + // The Causes array includes more details associated with the StatusReason + // failure. Not all StatusReasons may provide detailed causes. + Causes []StatusCause `json:"causes,omitempty" yaml:"causes,omitempty"` } // Values of Status.Status @@ -404,19 +407,19 @@ const ( StatusWorking = "working" ) -// ReasonType is an enumeration of possible failure causes. Each ReasonType +// StatusReason is an enumeration of possible failure causes. Each StatusReason // must map to a single HTTP status code, but multiple reasons may map // to the same HTTP status code. // TODO: move to apiserver -type ReasonType string +type StatusReason string const ( - // ReasonTypeUnknown means the server has declined to indicate a specific reason. + // StatusReasonUnknown means the server has declined to indicate a specific reason. // The details field may contain other information about this error. // Status code 500. - ReasonTypeUnknown ReasonType = "" + StatusReasonUnknown StatusReason = "" - // ReasonTypeWorking means the server is processing this request and will complete + // StatusReasonWorking means the server is processing this request and will complete // at a future time. // Details (optional): // "kind" string - the name of the resource being referenced ("operation" today) @@ -426,7 +429,7 @@ const ( // "Location" - HTTP header populated with a URL that can retrieved the final // status of this operation. // Status code 202 - ReasonTypeWorking ReasonType = "working" + StatusReasonWorking StatusReason = "working" // ResourceTypeNotFound means one or more resources required for this operation // could not be found. @@ -436,21 +439,65 @@ const ( // resource. // "id" string - the identifier of the missing resource // Status code 404 - ReasonTypeNotFound ReasonType = "notFound" + StatusReasonNotFound StatusReason = "notFound" - // ReasonTypeAlreadyExists means the resource you are creating already exists. + // StatusReasonAlreadyExists means the resource you are creating already exists. // Details (optional): // "kind" string - the kind attribute of the conflicting resource // "id" string - the identifier of the conflicting resource // Status code 409 - ReasonTypeAlreadyExists ReasonType = "alreadyExists" + StatusReasonAlreadyExists StatusReason = "alreadyExists" // ResourceTypeConflict means the requested update operation cannot be completed // due to a conflict in the operation. The client may need to alter the request. // Each resource may define custom details that indicate the nature of the // conflict. // Status code 409 - ReasonTypeConflict ReasonType = "conflict" + StatusReasonConflict StatusReason = "conflict" +) + +// StatusCause provides more information about an api.Status failure, including +// cases when multiple errors are encountered. +type StatusCause struct { + // A machine-readable description of the cause of the error. If this value is + // empty there is no information available. + Type CauseType `json:"reason,omitempty" yaml:"reason,omitempty"` + // A human-readable description of the cause of the error. This field may be + // presented as-is to a reader. + Message string `json:"message,omitempty" yaml:"message,omitempty"` + // The field of the resource that has caused this error, as named by its JSON + // serialization. May include dot and postfix notation for nested attributes. + // Arrays are zero-indexed. Fields may appear more than once in an array of + // causes due to fields having multiple errors. + // Optional. + // + // Examples: + // "name" - the field "name" on the current resource + // "items[0].name" - the field "name" on the first array entry in "items" + Field string `json:"field,omitempty" yaml:"field,omitempty"` +} + +// CauseType is a machine readable value providing more detail about what +// occured in a status response. An operation may have multiple causes for a +// status (whether failure, success, or working). +type CauseType string + +const ( + // CauseTypeFieldValueNotFound is used to report failure to find a requested value + // (e.g. looking up an ID). + CauseTypeFieldValueNotFound CauseType = "fieldValueNotFound" + // CauseTypeFieldValueInvalid is used to report required values that are not + // provided (e.g. empty strings, null values, or empty arrays). + CauseTypeFieldValueRequired CauseType = "fieldValueRequired" + // CauseTypeFieldValueDuplicate is used to report collisions of values that must be + // unique (e.g. unique IDs). + CauseTypeFieldValueDuplicate CauseType = "fieldValueDuplicate" + // CauseTypeFieldValueInvalid is used to report malformed values (e.g. failed regex + // match). + CauseTypeFieldValueInvalid CauseType = "fieldValueInvalid" + // CauseTypeFieldValueNotSupported is used to report valid (as per formatting rules) + // values that can not be handled (e.g. an enumerated string). + CauseTypeFieldValueNotSupported CauseType = "fieldValueNotSupported" ) // ServerOp is an operation delivered to API clients. diff --git a/pkg/api/validation.go b/pkg/api/validation.go index 70b098ea8c..0dd02dea64 100644 --- a/pkg/api/validation.go +++ b/pkg/api/validation.go @@ -34,17 +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 !util.IsDNSLabel(vol.Name) { - el = append(el, errs.NewInvalid("Volume.Name", vol.Name)) + if len(vol.Name) == 0 { + el = append(el, errs.NewRequired("name", vol.Name)) + } else if !util.IsDNSLabel(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 @@ -55,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 } @@ -70,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 } @@ -82,27 +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 !util.IsValidPortNum(port.ContainerPort) { - allErrs = append(allErrs, errs.NewInvalid("Port.ContainerPort", port.ContainerPort)) + if port.ContainerPort == 0 { + pErrs = append(pErrs, errs.NewRequired("containerPort", port.ContainerPort)) + } else if !util.IsValidPortNum(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 } @@ -111,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.NewInvalid("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 } @@ -126,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.NewInvalid("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.NewInvalid("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 @@ -145,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 } @@ -155,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]) @@ -162,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 } @@ -182,20 +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 !util.IsDNSLabel(ctr.Name) { - allErrs = append(allErrs, errs.NewInvalid("Container.Name", ctr.Name)) + if len(ctr.Name) == 0 { + cErrs = append(cErrs, errs.NewRequired("name", ctr.Name)) + } else if !util.IsDNSLabel(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)...) - allErrs = append(allErrs, validateEnv(ctr.Env)...) - allErrs = append(allErrs, validateVolumeMounts(ctr.VolumeMounts, volumes)...) + cErrs = append(cErrs, validatePorts(ctr.Ports).Prefix("ports")...) + cErrs = append(cErrs, validateEnv(ctr.Env).Prefix("env")...) + 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?) @@ -217,26 +233,24 @@ func ValidateManifest(manifest *ContainerManifest) errs.ErrorList { allErrs := errs.ErrorList{} if len(manifest.Version) == 0 { - allErrs = append(allErrs, errs.NewInvalid("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...) - } - allErrs = append(allErrs, validateContainers(manifest.Containers, allVolumes)...) + allErrs = append(allErrs, errs.Prefix("volumes")...) + allErrs = append(allErrs, validateContainers(manifest.Containers, allVolumes).Prefix("containers")...) 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 @@ -245,26 +259,26 @@ func ValidatePodState(podState *PodState) errs.ErrorList { // Pod tests if required fields in the pod are set. func ValidatePod(pod *Pod) errs.ErrorList { allErrs := errs.ErrorList{} - if pod.ID == "" { - allErrs = append(allErrs, errs.NewInvalid("Pod.ID", pod.ID)) + if len(pod.ID) == 0 { + allErrs = append(allErrs, errs.NewRequired("id", pod.ID)) } - allErrs = append(allErrs, ValidatePodState(&pod.DesiredState)...) + allErrs = append(allErrs, ValidatePodState(&pod.DesiredState).Prefix("desiredState")...) return allErrs } // ValidateService tests if required fields in the service are set. func ValidateService(service *Service) errs.ErrorList { allErrs := errs.ErrorList{} - if service.ID == "" { - allErrs = append(allErrs, errs.NewInvalid("Service.ID", service.ID)) + if len(service.ID) == 0 { + 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.NewInvalid("Service.Selector", service.Selector)) + allErrs = append(allErrs, errs.NewRequired("selector", service.Selector)) } return allErrs } @@ -272,20 +286,20 @@ func ValidateService(service *Service) errs.ErrorList { // ValidateReplicationController tests if required fields in the replication controller are set. func ValidateReplicationController(controller *ReplicationController) errs.ErrorList { allErrs := errs.ErrorList{} - if controller.ID == "" { - allErrs = append(allErrs, errs.NewInvalid("ReplicationController.ID", controller.ID)) + if len(controller.ID) == 0 { + allErrs = append(allErrs, errs.NewRequired("id", controller.ID)) } if labels.Set(controller.DesiredState.ReplicaSelector).AsSelector().Empty() { - allErrs = append(allErrs, errs.NewInvalid("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 } diff --git a/pkg/api/validation_test.go b/pkg/api/validation_test.go index 1e52c2869c..50c9cf4fab 100644 --- a/pkg/api/validation_test.go +++ b/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{ - "zero-length name": {{Name: ""}}, - "name > 63 characters": {{Name: strings.Repeat("a", 64)}}, - "name not a DNS label": {{Name: "a.b.c"}}, - "name not unique": {{Name: "abc"}, {Name: "abc"}}, + errorCases := map[string]struct { + V []Volume + T errors.ValidationErrorType + F string + }{ + "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{ - "name > 63 characters": {{Name: strings.Repeat("a", 64), ContainerPort: 80}}, - "name not a DNS label": {{Name: "a.b.c", ContainerPort: 80}}, - "name not unique": { + errorCases := map[string]struct { + P []Port + T errors.ValidationErrorType + 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}, - }, - "zero container port": {{ContainerPort: 0}}, - "invalid container port": {{ContainerPort: 65536}}, - "invalid host port": {{ContainerPort: 80, HostPort: 65536}}, - "invalid protocol": {{ContainerPort: 80, Protocol: "ICMP"}}, + }, errors.ValidationErrorTypeDuplicate, "[1].name"}, + "zero container port": {[]Port{{ContainerPort: 0}}, errors.ValidationErrorTypeRequired, "[0].containerPort"}, + "invalid container port": {[]Port{{ContainerPort: 65536}}, errors.ValidationErrorTypeInvalid, "[0].containerPort"}, + "invalid host port": {[]Port{{ContainerPort: 80, HostPort: 65536}}, errors.ValidationErrorTypeInvalid, "[0].hostPort"}, + "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]) + } + } } } diff --git a/pkg/apiserver/apiserver_test.go b/pkg/apiserver/apiserver_test.go index 59809d1d19..6f1ca93ffe 100644 --- a/pkg/apiserver/apiserver_test.go +++ b/pkg/apiserver/apiserver_test.go @@ -626,7 +626,7 @@ func TestAsyncDelayReturnsError(t *testing.T) { server := httptest.NewServer(handler) status := expectApiStatus(t, "DELETE", fmt.Sprintf("%s/prefix/version/foo/bar", server.URL), nil, http.StatusConflict) - if status.Status != api.StatusFailure || status.Message == "" || status.Details == nil || status.Reason != api.ReasonTypeAlreadyExists { + if status.Status != api.StatusFailure || status.Message == "" || status.Details == nil || status.Reason != api.StatusReasonAlreadyExists { t.Errorf("Unexpected status %#v", status) } } @@ -687,7 +687,7 @@ func TestWriteJSONDecodeError(t *testing.T) { writeJSON(http.StatusOK, api.Codec, &T{"Undecodable"}, w) })) status := expectApiStatus(t, "GET", server.URL, nil, http.StatusInternalServerError) - if status.Reason != api.ReasonTypeUnknown { + if status.Reason != api.StatusReasonUnknown { t.Errorf("unexpected reason %#v", status) } if !strings.Contains(status.Message, "type apiserver.T is not registered") { diff --git a/pkg/apiserver/errors.go b/pkg/apiserver/errors.go index 1a20b793fe..471b34f328 100644 --- a/pkg/apiserver/errors.go +++ b/pkg/apiserver/errors.go @@ -21,6 +21,7 @@ import ( "net/http" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" "github.com/GoogleCloudPlatform/kubernetes/pkg/tools" ) @@ -39,7 +40,7 @@ func NewNotFoundErr(kind, name string) error { return &apiServerError{api.Status{ Status: api.StatusFailure, Code: http.StatusNotFound, - Reason: api.ReasonTypeNotFound, + Reason: api.StatusReasonNotFound, Details: &api.StatusDetails{ Kind: kind, ID: name, @@ -53,7 +54,7 @@ func NewAlreadyExistsErr(kind, name string) error { return &apiServerError{api.Status{ Status: api.StatusFailure, Code: http.StatusConflict, - Reason: api.ReasonTypeAlreadyExists, + Reason: api.StatusReasonAlreadyExists, Details: &api.StatusDetails{ Kind: kind, ID: name, @@ -67,7 +68,7 @@ func NewConflictErr(kind, name string, err error) error { return &apiServerError{api.Status{ Status: api.StatusFailure, Code: http.StatusConflict, - Reason: api.ReasonTypeConflict, + Reason: api.StatusReasonConflict, Details: &api.StatusDetails{ Kind: kind, ID: name, @@ -76,27 +77,57 @@ func NewConflictErr(kind, name string, err error) error { }} } +// NewInvalidError returns an error indicating the item is invalid and cannot be processed. +func NewInvalidErr(kind, name string, errs errors.ErrorList) error { + causes := make([]api.StatusCause, 0, len(errs)) + for i := range errs { + if err, ok := errs[i].(errors.ValidationError); ok { + causes = append(causes, api.StatusCause{ + Type: api.CauseType(err.Type), + Message: err.Error(), + Field: err.Field, + }) + } + } + return &apiServerError{api.Status{ + Status: api.StatusFailure, + Code: 422, // RFC 4918 + Reason: api.StatusReasonInvalid, + Details: &api.StatusDetails{ + Kind: kind, + ID: name, + Causes: causes, + }, + Message: fmt.Sprintf("%s %q is invalid: %s", kind, name, errs.ToError()), + }} +} + // IsNotFound returns true if the specified error was created by NewNotFoundErr func IsNotFound(err error) bool { - return reasonForError(err) == api.ReasonTypeNotFound + return reasonForError(err) == api.StatusReasonNotFound } // IsAlreadyExists determines if the err is an error which indicates that a specified resource already exists. func IsAlreadyExists(err error) bool { - return reasonForError(err) == api.ReasonTypeAlreadyExists + return reasonForError(err) == api.StatusReasonAlreadyExists } // IsConflict determines if the err is an error which indicates the provided update conflicts func IsConflict(err error) bool { - return reasonForError(err) == api.ReasonTypeConflict + return reasonForError(err) == api.StatusReasonConflict } -func reasonForError(err error) api.ReasonType { +// IsInvalid determines if the err is an error which indicates the provided resource is not valid +func IsInvalid(err error) bool { + return reasonForError(err) == api.StatusReasonInvalid +} + +func reasonForError(err error) api.StatusReason { switch t := err.(type) { case *apiServerError: return t.Status.Reason } - return api.ReasonTypeUnknown + return api.StatusReasonUnknown } // errToAPIStatus converts an error to an api.Status object. @@ -110,14 +141,14 @@ func errToAPIStatus(err error) *api.Status { default: status := http.StatusInternalServerError switch { - //TODO: replace me with NewUpdateConflictErr + //TODO: replace me with NewConflictErr case tools.IsEtcdTestFailed(err): status = http.StatusConflict } return &api.Status{ Status: api.StatusFailure, Code: status, - Reason: api.ReasonTypeUnknown, + Reason: api.StatusReasonUnknown, Message: err.Error(), } } diff --git a/pkg/apiserver/errors_test.go b/pkg/apiserver/errors_test.go index 295560ffc4..d887faa987 100644 --- a/pkg/apiserver/errors_test.go +++ b/pkg/apiserver/errors_test.go @@ -19,9 +19,11 @@ package apiserver import ( "errors" "fmt" + "reflect" "testing" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + apierrors "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" ) func TestErrorNew(t *testing.T) { @@ -33,13 +35,108 @@ func TestErrorNew(t *testing.T) { t.Errorf("expected to not be confict") } if IsNotFound(err) { - t.Errorf(fmt.Sprintf("expected to not be %s", api.ReasonTypeNotFound)) + t.Errorf(fmt.Sprintf("expected to not be %s", api.StatusReasonNotFound)) + } + if IsInvalid(err) { + t.Errorf("expected to not be invalid") } if !IsConflict(NewConflictErr("test", "2", errors.New("message"))) { - t.Errorf("expected to be confict") + t.Errorf("expected to be conflict") } if !IsNotFound(NewNotFoundErr("test", "3")) { t.Errorf("expected to be not found") } + if !IsInvalid(NewInvalidErr("test", "2", nil)) { + t.Errorf("expected to be invalid") + } +} + +func TestNewInvalidErr(t *testing.T) { + testCases := []struct { + Err apierrors.ValidationError + Details *api.StatusDetails + }{ + { + apierrors.NewDuplicate("field[0].name", "bar"), + &api.StatusDetails{ + Kind: "kind", + ID: "name", + Causes: []api.StatusCause{{ + Type: api.CauseTypeFieldValueDuplicate, + Field: "field[0].name", + }}, + }, + }, + { + apierrors.NewInvalid("field[0].name", "bar"), + &api.StatusDetails{ + Kind: "kind", + ID: "name", + Causes: []api.StatusCause{{ + Type: api.CauseTypeFieldValueInvalid, + Field: "field[0].name", + }}, + }, + }, + { + apierrors.NewNotFound("field[0].name", "bar"), + &api.StatusDetails{ + Kind: "kind", + ID: "name", + Causes: []api.StatusCause{{ + Type: api.CauseTypeFieldValueNotFound, + Field: "field[0].name", + }}, + }, + }, + { + apierrors.NewNotSupported("field[0].name", "bar"), + &api.StatusDetails{ + Kind: "kind", + ID: "name", + Causes: []api.StatusCause{{ + Type: api.CauseTypeFieldValueNotSupported, + Field: "field[0].name", + }}, + }, + }, + { + apierrors.NewRequired("field[0].name", "bar"), + &api.StatusDetails{ + Kind: "kind", + ID: "name", + Causes: []api.StatusCause{{ + Type: api.CauseTypeFieldValueRequired, + Field: "field[0].name", + }}, + }, + }, + } + for i := range testCases { + vErr, expected := testCases[i].Err, testCases[i].Details + expected.Causes[0].Message = vErr.Error() + err := NewInvalidErr("kind", "name", apierrors.ErrorList{vErr}) + status := errToAPIStatus(err) + if status.Code != 422 || status.Reason != api.StatusReasonInvalid { + t.Errorf("unexpected status: %#v", status) + } + if !reflect.DeepEqual(expected, status.Details) { + t.Errorf("expected %#v, got %#v", expected, status.Details) + } + } +} + +func Test_errToAPIStatus(t *testing.T) { + err := &apiServerError{} + status := errToAPIStatus(err) + if status.Reason != api.StatusReasonUnknown || status.Status != api.StatusFailure { + t.Errorf("unexpected status object: %#v", status) + } +} + +func Test_reasonForError(t *testing.T) { + if e, a := api.StatusReasonUnknown, reasonForError(nil); e != a { + t.Errorf("unexpected reason type: %#v", a) + } } diff --git a/pkg/apiserver/operation.go b/pkg/apiserver/operation.go index 62677c1f33..7efa08341a 100644 --- a/pkg/apiserver/operation.go +++ b/pkg/apiserver/operation.go @@ -192,7 +192,7 @@ func (op *Operation) StatusOrResult() (description interface{}, finished bool) { if op.finished == nil { return &api.Status{ Status: api.StatusWorking, - Reason: api.ReasonTypeWorking, + Reason: api.StatusReasonWorking, Details: &api.StatusDetails{ID: op.ID, Kind: "operation"}, }, false } diff --git a/pkg/registry/controller/storage.go b/pkg/registry/controller/storage.go index f14951977e..79835aba93 100644 --- a/pkg/registry/controller/storage.go +++ b/pkg/registry/controller/storage.go @@ -60,7 +60,7 @@ func (rs *RegistryStorage) Create(obj interface{}) (<-chan interface{}, error) { // Pod Manifest ID should be assigned by the pod API controller.DesiredState.PodTemplate.DesiredState.Manifest.ID = "" if errs := api.ValidateReplicationController(controller); len(errs) > 0 { - return nil, fmt.Errorf("Validation errors: %v", errs) + return nil, apiserver.NewInvalidErr("replicationController", controller.ID, errs) } controller.CreationTimestamp = util.Now() @@ -117,7 +117,7 @@ func (rs *RegistryStorage) Update(obj interface{}) (<-chan interface{}, error) { return nil, fmt.Errorf("not a replication controller: %#v", obj) } if errs := api.ValidateReplicationController(controller); len(errs) > 0 { - return nil, fmt.Errorf("Validation errors: %v", errs) + return nil, apiserver.NewInvalidErr("replicationController", controller.ID, errs) } return apiserver.MakeAsync(func() (interface{}, error) { err := rs.registry.UpdateController(*controller) diff --git a/pkg/registry/controller/storage_test.go b/pkg/registry/controller/storage_test.go index 92c2b9eb90..d24eccb2ee 100644 --- a/pkg/registry/controller/storage_test.go +++ b/pkg/registry/controller/storage_test.go @@ -25,6 +25,7 @@ import ( "time" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + "github.com/GoogleCloudPlatform/kubernetes/pkg/apiserver" "github.com/GoogleCloudPlatform/kubernetes/pkg/labels" "github.com/GoogleCloudPlatform/kubernetes/pkg/registry/registrytest" ) @@ -272,8 +273,8 @@ func TestControllerStorageValidatesCreate(t *testing.T) { if c != nil { t.Errorf("Expected nil channel") } - if err == nil { - t.Errorf("Expected to get an error") + if !apiserver.IsInvalid(err) { + t.Errorf("Expected to get an invalid resource error, got %v", err) } } } @@ -302,8 +303,8 @@ func TestControllerStorageValidatesUpdate(t *testing.T) { if c != nil { t.Errorf("Expected nil channel") } - if err == nil { - t.Errorf("Expected to get an error") + if !apiserver.IsInvalid(err) { + t.Errorf("Expected to get an invalid resource error, got %v", err) } } } diff --git a/pkg/registry/pod/storage.go b/pkg/registry/pod/storage.go index eec4e143c2..56ad464b69 100644 --- a/pkg/registry/pod/storage.go +++ b/pkg/registry/pod/storage.go @@ -69,7 +69,7 @@ func (rs *RegistryStorage) Create(obj interface{}) (<-chan interface{}, error) { } pod.DesiredState.Manifest.ID = pod.ID if errs := api.ValidatePod(pod); len(errs) > 0 { - return nil, fmt.Errorf("Validation errors: %v", errs) + return nil, apiserver.NewInvalidErr("pod", pod.ID, errs) } pod.CreationTimestamp = util.Now() @@ -135,7 +135,7 @@ func (rs RegistryStorage) New() interface{} { func (rs *RegistryStorage) Update(obj interface{}) (<-chan interface{}, error) { pod := obj.(*api.Pod) if errs := api.ValidatePod(pod); len(errs) > 0 { - return nil, fmt.Errorf("Validation errors: %v", errs) + return nil, apiserver.NewInvalidErr("pod", pod.ID, errs) } return apiserver.MakeAsync(func() (interface{}, error) { if err := rs.registry.UpdatePod(*pod); err != nil { diff --git a/pkg/registry/pod/storage_test.go b/pkg/registry/pod/storage_test.go index 24ad2c46e9..1ffb128045 100644 --- a/pkg/registry/pod/storage_test.go +++ b/pkg/registry/pod/storage_test.go @@ -23,6 +23,7 @@ import ( "time" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + "github.com/GoogleCloudPlatform/kubernetes/pkg/apiserver" "github.com/GoogleCloudPlatform/kubernetes/pkg/cloudprovider/fake" "github.com/GoogleCloudPlatform/kubernetes/pkg/labels" "github.com/GoogleCloudPlatform/kubernetes/pkg/registry/registrytest" @@ -330,8 +331,8 @@ func TestPodStorageValidatesCreate(t *testing.T) { if c != nil { t.Errorf("Expected nil channel") } - if err == nil { - t.Errorf("Expected to get an error") + if !apiserver.IsInvalid(err) { + t.Errorf("Expected to get an invalid resource error, got %v", err) } } @@ -346,8 +347,8 @@ func TestPodStorageValidatesUpdate(t *testing.T) { if c != nil { t.Errorf("Expected nil channel") } - if err == nil { - t.Errorf("Expected to get an error") + if !apiserver.IsInvalid(err) { + t.Errorf("Expected to get an invalid resource error, got %v", err) } } diff --git a/pkg/registry/service/storage.go b/pkg/registry/service/storage.go index d5b4103dce..b73a17f731 100644 --- a/pkg/registry/service/storage.go +++ b/pkg/registry/service/storage.go @@ -48,7 +48,7 @@ func NewRegistryStorage(registry Registry, cloud cloudprovider.Interface, machin func (rs *RegistryStorage) Create(obj interface{}) (<-chan interface{}, error) { srv := obj.(*api.Service) if errs := api.ValidateService(srv); len(errs) > 0 { - return nil, fmt.Errorf("Validation errors: %v", errs) + return nil, apiserver.NewInvalidErr("service", srv.ID, errs) } srv.CreationTimestamp = util.Now() @@ -147,11 +147,8 @@ func GetServiceEnvironmentVariables(registry Registry, machine string) ([]api.En func (rs *RegistryStorage) Update(obj interface{}) (<-chan interface{}, error) { srv := obj.(*api.Service) - if srv.ID == "" { - return nil, fmt.Errorf("ID should not be empty: %#v", srv) - } if errs := api.ValidateService(srv); len(errs) > 0 { - return nil, fmt.Errorf("Validation errors: %v", errs) + return nil, apiserver.NewInvalidErr("service", srv.ID, errs) } return apiserver.MakeAsync(func() (interface{}, error) { // TODO: check to see if external load balancer status changed diff --git a/pkg/registry/service/storage_test.go b/pkg/registry/service/storage_test.go index d7d6f59792..37243465d3 100644 --- a/pkg/registry/service/storage_test.go +++ b/pkg/registry/service/storage_test.go @@ -21,6 +21,7 @@ import ( "testing" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + "github.com/GoogleCloudPlatform/kubernetes/pkg/apiserver" cloud "github.com/GoogleCloudPlatform/kubernetes/pkg/cloudprovider/fake" "github.com/GoogleCloudPlatform/kubernetes/pkg/labels" "github.com/GoogleCloudPlatform/kubernetes/pkg/registry/minion" @@ -78,8 +79,8 @@ func TestServiceStorageValidatesCreate(t *testing.T) { if c != nil { t.Errorf("Expected nil channel") } - if err == nil { - t.Errorf("Expected to get an error") + if !apiserver.IsInvalid(err) { + t.Errorf("Expected to get an invalid resource error, got %v", err) } } @@ -139,8 +140,8 @@ func TestServiceStorageValidatesUpdate(t *testing.T) { if c != nil { t.Errorf("Expected nil channel") } - if err == nil { - t.Errorf("Expected to get an error") + if !apiserver.IsInvalid(err) { + t.Errorf("Expected to get an invalid resource error, got %v", err) } } }