Skip to content

Commit

Permalink
Merge pull request kubernetes#722 from smarterclayton/improve_errors
Browse files Browse the repository at this point in the history 
Normalize apiserver error handling of standard errors
  • Loading branch information
@lavalamp
lavalamp committed Aug 11, 2014
2 parents 9436748 + 0083fae commit 9050c81
Show file tree
Hide file tree
Showing 6 changed files with 202 additions and 113 deletions.
56 changes: 19 additions & 37 deletions pkg/apiserver/apiserver.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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/"
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -320,19 +296,25 @@ 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")
w.WriteHeader(statusCode)
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")
Expand Down
91 changes: 55 additions & 36 deletions pkg/apiserver/apiserver_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"net/http"
"net/http/httptest"
"reflect"
"strings"
"sync"
"testing"
"time"
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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)
}
}
Expand All @@ -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")
Expand All @@ -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)
}
}
}

Expand All @@ -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)
}
}

Expand Down
15 changes: 1 addition & 14 deletions pkg/apiserver/async.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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
}
Expand Down
Loading

0 comments on commit 9050c81

Please sign in to comment.