From 52923a134876f4b5432b24be7504ec0eb9911267 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Thu, 21 Aug 2014 20:24:53 -0400 Subject: [PATCH] Return validation errors from storage create/update --- pkg/api/errors/errors_test.go | 74 +++++++++++++++++++++++ pkg/apiserver/errors.go | 18 ++++-- pkg/apiserver/errors_test.go | 79 ++++++++++++++++++++++++- pkg/registry/controller/storage.go | 4 +- pkg/registry/controller/storage_test.go | 9 +-- pkg/registry/pod/storage.go | 4 +- pkg/registry/pod/storage_test.go | 9 +-- pkg/registry/service/storage.go | 7 +-- pkg/registry/service/storage_test.go | 9 +-- 9 files changed, 187 insertions(+), 26 deletions(-) diff --git a/pkg/api/errors/errors_test.go b/pkg/api/errors/errors_test.go index 17fc22e627e..bd4a72a1541 100644 --- a/pkg/api/errors/errors_test.go +++ b/pkg/api/errors/errors_test.go @@ -18,6 +18,7 @@ package errors import ( "fmt" + "strings" "testing" ) @@ -56,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. } @@ -87,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/apiserver/errors.go b/pkg/apiserver/errors.go index 20e0a503f49..f1f6682623f 100644 --- a/pkg/apiserver/errors.go +++ b/pkg/apiserver/errors.go @@ -78,15 +78,25 @@ func NewConflictErr(kind, name string, err error) error { } // NewInvalidError returns an error indicating the item is invalid and cannot be processed. -func NewInvalidError(kind, name string, errs errors.ErrorList) error { +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{ + Reason: api.CauseReasonType(err.Type), + Message: err.Error(), + Field: err.Field, + }) + } + } return &apiServerError{api.Status{ Status: api.StatusFailure, Code: 422, // RFC 4918 Reason: api.ReasonTypeInvalid, Details: &api.StatusDetails{ - Kind: kind, - ID: name, - // TODO: causes + Kind: kind, + ID: name, + Causes: causes, }, Message: fmt.Sprintf("%s %q is invalid: %s", kind, name, errs.ToError()), }} diff --git a/pkg/apiserver/errors_test.go b/pkg/apiserver/errors_test.go index a53161933e2..213b9581702 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) { @@ -45,11 +47,86 @@ func TestErrorNew(t *testing.T) { if !IsNotFound(NewNotFoundErr("test", "3")) { t.Errorf("expected to be not found") } - if !IsInvalid(NewInvalidError("test", "2", nil)) { + 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{{ + Reason: api.CauseReasonTypeFieldValueDuplicate, + Field: "field[0].name", + }}, + }, + }, + { + apierrors.NewInvalid("field[0].name", "bar"), + &api.StatusDetails{ + Kind: "kind", + ID: "name", + Causes: []api.StatusCause{{ + Reason: api.CauseReasonTypeFieldValueInvalid, + Field: "field[0].name", + }}, + }, + }, + { + apierrors.NewNotFound("field[0].name", "bar"), + &api.StatusDetails{ + Kind: "kind", + ID: "name", + Causes: []api.StatusCause{{ + Reason: api.CauseReasonTypeFieldValueNotFound, + Field: "field[0].name", + }}, + }, + }, + { + apierrors.NewNotSupported("field[0].name", "bar"), + &api.StatusDetails{ + Kind: "kind", + ID: "name", + Causes: []api.StatusCause{{ + Reason: api.CauseReasonTypeFieldValueNotSupported, + Field: "field[0].name", + }}, + }, + }, + { + apierrors.NewRequired("field[0].name", "bar"), + &api.StatusDetails{ + Kind: "kind", + ID: "name", + Causes: []api.StatusCause{{ + Reason: api.CauseReasonTypeFieldValueRequired, + 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.ReasonTypeInvalid { + 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) diff --git a/pkg/registry/controller/storage.go b/pkg/registry/controller/storage.go index f14951977e0..79835aba932 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 92c2b9eb905..d24eccb2eef 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 eec4e143c2d..56ad464b699 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 24ad2c46e95..1ffb1280452 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 d5b4103dce6..b73a17f7312 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 d7d6f597923..37243465d3e 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) } } }