From 0083fae4539a233301eac5fea7134ebeb0aabe21 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Thu, 31 Jul 2014 14:26:34 -0400 Subject: [PATCH] Provide helpers and tests for common error types Unify error handling in apiserver into a single path - RESTStorage objects must provide appropriate errors individually. Ensure ALL errors which can be traced to logical faults with RESTStorage are returned as api.Status objects. --- pkg/apiserver/apiserver.go | 56 ++++++----------- pkg/apiserver/apiserver_test.go | 91 ++++++++++++++++----------- pkg/apiserver/async.go | 15 +---- pkg/apiserver/errors.go | 108 ++++++++++++++++++++++++-------- pkg/apiserver/errors_test.go | 42 +++++++++++++ pkg/apiserver/watch.go | 3 +- 6 files changed, 202 insertions(+), 113 deletions(-) create mode 100644 pkg/apiserver/errors_test.go diff --git a/pkg/apiserver/apiserver.go b/pkg/apiserver/apiserver.go index ba8be538eb..cc0b0c9d2c 100644 --- a/pkg/apiserver/apiserver.go +++ b/pkg/apiserver/apiserver.go @@ -82,7 +82,7 @@ func New(storage map[string]RESTStorage, codec Codec, prefix string) *APIServer // Watch API handlers watchPrefix := path.Join(prefix, "watch") + "/" - mux.Handle(watchPrefix, http.StripPrefix(watchPrefix, &WatchHandler{storage})) + mux.Handle(watchPrefix, http.StripPrefix(watchPrefix, &WatchHandler{storage, codec})) // Support services for the apiserver logsPrefix := "/logs/" @@ -167,23 +167,19 @@ func (s *APIServer) handleRESTStorage(parts []string, req *http.Request, w http. case 1: selector, err := labels.ParseSelector(req.URL.Query().Get("labels")) if err != nil { - internalError(err, w) + errorJSON(err, s.codec, w) return } list, err := storage.List(selector) if err != nil { - internalError(err, w) + errorJSON(err, s.codec, w) return } writeJSON(http.StatusOK, s.codec, list, w) case 2: item, err := storage.Get(parts[1]) - if IsNotFound(err) { - notFound(w, req) - return - } if err != nil { - internalError(err, w) + errorJSON(err, s.codec, w) return } writeJSON(http.StatusOK, s.codec, item, w) @@ -198,26 +194,18 @@ func (s *APIServer) handleRESTStorage(parts []string, req *http.Request, w http. } body, err := readBody(req) if err != nil { - internalError(err, w) + errorJSON(err, s.codec, w) return } obj := storage.New() err = s.codec.DecodeInto(body, obj) - if IsNotFound(err) { - notFound(w, req) - return - } if err != nil { - internalError(err, w) + errorJSON(err, s.codec, w) return } out, err := storage.Create(obj) - if IsNotFound(err) { - notFound(w, req) - return - } if err != nil { - internalError(err, w) + errorJSON(err, s.codec, w) return } op := s.createOperation(out, sync, timeout) @@ -229,12 +217,8 @@ func (s *APIServer) handleRESTStorage(parts []string, req *http.Request, w http. return } out, err := storage.Delete(parts[1]) - if IsNotFound(err) { - notFound(w, req) - return - } if err != nil { - internalError(err, w) + errorJSON(err, s.codec, w) return } op := s.createOperation(out, sync, timeout) @@ -247,26 +231,18 @@ func (s *APIServer) handleRESTStorage(parts []string, req *http.Request, w http. } body, err := readBody(req) if err != nil { - internalError(err, w) + errorJSON(err, s.codec, w) return } obj := storage.New() err = s.codec.DecodeInto(body, obj) - if IsNotFound(err) { - notFound(w, req) - return - } if err != nil { - internalError(err, w) + errorJSON(err, s.codec, w) return } out, err := storage.Update(obj) - if IsNotFound(err) { - notFound(w, req) - return - } if err != nil { - internalError(err, w) + errorJSON(err, s.codec, w) return } op := s.createOperation(out, sync, timeout) @@ -320,7 +296,7 @@ func (s *APIServer) finishReq(op *Operation, w http.ResponseWriter) { func writeJSON(statusCode int, codec Codec, object interface{}, w http.ResponseWriter) { output, err := codec.Encode(object) if err != nil { - internalError(err, w) + errorJSON(err, codec, w) return } w.Header().Set("Content-Type", "application/json") @@ -328,11 +304,17 @@ func writeJSON(statusCode int, codec Codec, object interface{}, w http.ResponseW w.Write(output) } +// errorJSON renders an error to the response +func errorJSON(err error, codec Codec, w http.ResponseWriter) { + status := errToAPIStatus(err) + writeJSON(status.Code, codec, status, w) +} + // writeRawJSON writes a non-API object in JSON. func writeRawJSON(statusCode int, object interface{}, w http.ResponseWriter) { output, err := json.Marshal(object) if err != nil { - internalError(err, w) + http.Error(w, err.Error(), http.StatusInternalServerError) return } w.Header().Set("Content-Type", "application/json") diff --git a/pkg/apiserver/apiserver_test.go b/pkg/apiserver/apiserver_test.go index a75e347e30..51d6820129 100644 --- a/pkg/apiserver/apiserver_test.go +++ b/pkg/apiserver/apiserver_test.go @@ -25,6 +25,7 @@ import ( "net/http" "net/http/httptest" "reflect" + "strings" "sync" "testing" "time" @@ -433,26 +434,6 @@ func TestUpdateMissing(t *testing.T) { } } -func TestBadPath(t *testing.T) { - handler := New(map[string]RESTStorage{}, codec, "/prefix/version") - server := httptest.NewServer(handler) - client := http.Client{} - - request, err := http.NewRequest("GET", server.URL+"/foobar", nil) - if err != nil { - t.Errorf("unexpected error: %v", err) - } - - response, err := client.Do(request) - if err != nil { - t.Errorf("unexpected error: %v", err) - } - - if response.StatusCode != http.StatusNotFound { - t.Errorf("Unexpected response %#v", response) - } -} - func TestCreate(t *testing.T) { simpleStorage := &SimpleRESTStorage{} handler := New(map[string]RESTStorage{ @@ -588,33 +569,64 @@ func expectApiStatus(t *testing.T, method, url string, data []byte, code int) *a } response, err := client.Do(request) if err != nil { - t.Fatalf("unexpected error %#v", err) + t.Fatalf("unexpected error on %s %s: %v", method, url, err) return nil } var status api.Status _, err = extractBody(response, &status) if err != nil { - t.Fatalf("unexpected error %#v", err) + t.Fatalf("unexpected error on %s %s: %v", method, url, err) return nil } if code != response.StatusCode { - t.Fatalf("Expected %d, Got %d", code, response.StatusCode) + t.Fatalf("Expected %s %s to return %d, Got %d", method, url, code, response.StatusCode) } return &status } +func TestErrorsToAPIStatus(t *testing.T) { + cases := map[error]api.Status{ + NewAlreadyExistsErr("foo", "bar"): api.Status{ + Status: api.StatusFailure, + Code: http.StatusConflict, + Reason: "already_exists", + Message: "foo \"bar\" already exists", + Details: &api.StatusDetails{ + Kind: "foo", + ID: "bar", + }, + }, + NewConflictErr("foo", "bar", errors.New("failure")): api.Status{ + Status: api.StatusFailure, + Code: http.StatusConflict, + Reason: "conflict", + Message: "foo \"bar\" cannot be updated: failure", + Details: &api.StatusDetails{ + Kind: "foo", + ID: "bar", + }, + }, + } + for k, v := range cases { + actual := errToAPIStatus(k) + if !reflect.DeepEqual(actual, &v) { + t.Errorf("Expected %#v, Got %#v", v, actual) + } + } +} + func TestAsyncDelayReturnsError(t *testing.T) { storage := SimpleRESTStorage{ injectedFunction: func(obj interface{}) (interface{}, error) { - return nil, errors.New("error") + return nil, NewAlreadyExistsErr("foo", "bar") }, } handler := New(map[string]RESTStorage{"foo": &storage}, codec, "/prefix/version") handler.asyncOpWait = time.Millisecond / 2 server := httptest.NewServer(handler) - status := expectApiStatus(t, "DELETE", fmt.Sprintf("%s/prefix/version/foo/bar", server.URL), nil, http.StatusInternalServerError) - if status.Status != api.StatusFailure || status.Message != "error" || status.Details != nil { + 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 { t.Errorf("Unexpected status %#v", status) } } @@ -624,7 +636,7 @@ func TestAsyncCreateError(t *testing.T) { storage := SimpleRESTStorage{ injectedFunction: func(obj interface{}) (interface{}, error) { <-ch - return nil, errors.New("error") + return nil, NewAlreadyExistsErr("foo", "bar") }, } handler := New(map[string]RESTStorage{"foo": &storage}, codec, "/prefix/version") @@ -648,13 +660,22 @@ func TestAsyncCreateError(t *testing.T) { time.Sleep(time.Millisecond) finalStatus := expectApiStatus(t, "GET", fmt.Sprintf("%s/prefix/version/operations/%s?after=1", server.URL, status.Details.ID), []byte{}, http.StatusOK) + expectedErr := NewAlreadyExistsErr("foo", "bar") expectedStatus := &api.Status{ - Code: http.StatusInternalServerError, - Message: "error", Status: api.StatusFailure, + Code: http.StatusConflict, + Reason: "already_exists", + Message: expectedErr.Error(), + Details: &api.StatusDetails{ + Kind: "foo", + ID: "bar", + }, } if !reflect.DeepEqual(expectedStatus, finalStatus) { t.Errorf("Expected %#v, Got %#v", expectedStatus, finalStatus) + if finalStatus.Details != nil { + t.Logf("Details %#v, Got %#v", *expectedStatus.Details, *finalStatus.Details) + } } } @@ -665,14 +686,12 @@ func TestWriteJSONDecodeError(t *testing.T) { } writeJSON(http.StatusOK, api.Codec, &T{"Undecodable"}, w) })) - client := http.Client{} - resp, err := client.Get(server.URL) - if err != nil { - t.Errorf("unexpected error: %v", err) + status := expectApiStatus(t, "GET", server.URL, nil, http.StatusInternalServerError) + if status.Reason != api.ReasonTypeUnknown { + t.Errorf("unexpected reason %#v", status) } - - if resp.StatusCode != http.StatusInternalServerError { - t.Errorf("unexpected status code %d", resp.StatusCode) + if !strings.Contains(status.Message, "type apiserver.T is not registered") { + t.Errorf("unexpected message %#v", status) } } diff --git a/pkg/apiserver/async.go b/pkg/apiserver/async.go index 6e7d6df2d1..a87c9766a1 100644 --- a/pkg/apiserver/async.go +++ b/pkg/apiserver/async.go @@ -17,10 +17,6 @@ limitations under the License. package apiserver import ( - "net/http" - - "github.com/GoogleCloudPlatform/kubernetes/pkg/api" - "github.com/GoogleCloudPlatform/kubernetes/pkg/tools" "github.com/GoogleCloudPlatform/kubernetes/pkg/util" ) @@ -37,16 +33,7 @@ func MakeAsync(fn WorkFunc) <-chan interface{} { defer util.HandleCrash() obj, err := fn() if err != nil { - status := http.StatusInternalServerError - switch { - case tools.IsEtcdTestFailed(err): - status = http.StatusConflict - } - channel <- &api.Status{ - Status: api.StatusFailure, - Message: err.Error(), - Code: status, - } + channel <- errToAPIStatus(err) } else { channel <- obj } diff --git a/pkg/apiserver/errors.go b/pkg/apiserver/errors.go index 6b6cffee5b..1a20b793fe 100644 --- a/pkg/apiserver/errors.go +++ b/pkg/apiserver/errors.go @@ -19,50 +19,108 @@ package apiserver import ( "fmt" "net/http" + + "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + "github.com/GoogleCloudPlatform/kubernetes/pkg/tools" ) -// errNotFound is an error which indicates that a specified resource is not found. -type errNotFound string - -// Error returns a string representation of the err. -func (err errNotFound) Error() string { - return string(err) +// apiServerError is an error intended for consumption by a REST API server +type apiServerError struct { + api.Status } -// IsNotFound determines if the err is an error which indicates that a specified resource was not found. -func IsNotFound(err error) bool { - _, ok := err.(errNotFound) - return ok +// Error implements the Error interface. +func (e *apiServerError) Error() string { + return e.Status.Message } // NewNotFoundErr returns a new error which indicates that the resource of the kind and the name was not found. func NewNotFoundErr(kind, name string) error { - return errNotFound(fmt.Sprintf("%s %q not found", kind, name)) + return &apiServerError{api.Status{ + Status: api.StatusFailure, + Code: http.StatusNotFound, + Reason: api.ReasonTypeNotFound, + Details: &api.StatusDetails{ + Kind: kind, + ID: name, + }, + Message: fmt.Sprintf("%s %q not found", kind, name), + }} } -// errAlreadyExists is an error which indicates that a specified resource already exists. -type errAlreadyExists string +// NewAlreadyExistsErr returns an error indicating the item requested exists by that identifier +func NewAlreadyExistsErr(kind, name string) error { + return &apiServerError{api.Status{ + Status: api.StatusFailure, + Code: http.StatusConflict, + Reason: api.ReasonTypeAlreadyExists, + Details: &api.StatusDetails{ + Kind: kind, + ID: name, + }, + Message: fmt.Sprintf("%s %q already exists", kind, name), + }} +} -// Error returns a string representation of the err. -func (err errAlreadyExists) Error() string { - return string(err) +// NewConflictErr returns an error indicating the item can't be updated as provided. +func NewConflictErr(kind, name string, err error) error { + return &apiServerError{api.Status{ + Status: api.StatusFailure, + Code: http.StatusConflict, + Reason: api.ReasonTypeConflict, + Details: &api.StatusDetails{ + Kind: kind, + ID: name, + }, + Message: fmt.Sprintf("%s %q cannot be updated: %s", kind, name, err), + }} +} + +// IsNotFound returns true if the specified error was created by NewNotFoundErr +func IsNotFound(err error) bool { + return reasonForError(err) == api.ReasonTypeNotFound } // IsAlreadyExists determines if the err is an error which indicates that a specified resource already exists. func IsAlreadyExists(err error) bool { - _, ok := err.(errAlreadyExists) - return ok + return reasonForError(err) == api.ReasonTypeAlreadyExists } -// NewAlreadyExistsErr returns a new error which indicates that the resource of the kind and the name was not found. -func NewAlreadyExistsErr(kind, name string) error { - return errAlreadyExists(fmt.Sprintf("%s %q already exists", kind, name)) +// IsConflict determines if the err is an error which indicates the provided update conflicts +func IsConflict(err error) bool { + return reasonForError(err) == api.ReasonTypeConflict } -// internalError renders a generic error to the response -func internalError(err error, w http.ResponseWriter) { - w.WriteHeader(http.StatusInternalServerError) - fmt.Fprintf(w, "Internal Error: %#v", err) +func reasonForError(err error) api.ReasonType { + switch t := err.(type) { + case *apiServerError: + return t.Status.Reason + } + return api.ReasonTypeUnknown +} + +// errToAPIStatus converts an error to an api.Status object. +func errToAPIStatus(err error) *api.Status { + switch t := err.(type) { + case *apiServerError: + status := t.Status + status.Status = api.StatusFailure + //TODO: check for invalid responses + return &status + default: + status := http.StatusInternalServerError + switch { + //TODO: replace me with NewUpdateConflictErr + case tools.IsEtcdTestFailed(err): + status = http.StatusConflict + } + return &api.Status{ + Status: api.StatusFailure, + Code: status, + Reason: api.ReasonTypeUnknown, + Message: err.Error(), + } + } } // notFound renders a simple not found error diff --git a/pkg/apiserver/errors_test.go b/pkg/apiserver/errors_test.go new file mode 100644 index 0000000000..7c7cd7eb86 --- /dev/null +++ b/pkg/apiserver/errors_test.go @@ -0,0 +1,42 @@ +/* +Copyright 2014 Google Inc. All rights reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package apiserver + +import ( + "errors" + "testing" +) + +func TestErrorNew(t *testing.T) { + err := NewAlreadyExistsErr("test", "1") + if !IsAlreadyExists(err) { + t.Errorf("expected to be already_exists") + } + if IsConflict(err) { + t.Errorf("expected to not be confict") + } + if IsNotFound(err) { + t.Errorf("expected to not be not_found") + } + + if !IsConflict(NewConflictErr("test", "2", errors.New("message"))) { + t.Errorf("expected to be confict") + } + if !IsNotFound(NewNotFoundErr("test", "3")) { + t.Errorf("expected to be not found") + } +} diff --git a/pkg/apiserver/watch.go b/pkg/apiserver/watch.go index b2315b7856..36a0f0ccbf 100644 --- a/pkg/apiserver/watch.go +++ b/pkg/apiserver/watch.go @@ -31,6 +31,7 @@ import ( type WatchHandler struct { storage map[string]RESTStorage + codec Codec } func getWatchParams(query url.Values) (label, field labels.Selector, resourceVersion uint64) { @@ -64,7 +65,7 @@ func (h *WatchHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) { label, field, resourceVersion := getWatchParams(req.URL.Query()) watching, err := watcher.Watch(label, field, resourceVersion) if err != nil { - internalError(err, w) + errorJSON(err, h.codec, w) return }