Merge pull request #722 from smarterclayton/improve_errors

Normalize apiserver error handling of standard errors

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")

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)
+	status := expectApiStatus(t, "DELETE", fmt.Sprintf("%s/prefix/version/foo/bar", server.URL), nil, http.StatusConflict)
-	if status.Status != api.StatusFailure || status.Message != "error" || status.Details != nil {
+	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{}
+	status := expectApiStatus(t, "GET", server.URL, nil, http.StatusInternalServerError)
-	resp, err := client.Get(server.URL)
+	if status.Reason != api.ReasonTypeUnknown {
-	if err != nil {
+		t.Errorf("unexpected reason %#v", status)
-		t.Errorf("unexpected error: %v", err)
 	}
-
+	if !strings.Contains(status.Message, "type apiserver.T is not registered") {
-	if resp.StatusCode != http.StatusInternalServerError {
+		t.Errorf("unexpected message %#v", status)
-		t.Errorf("unexpected status code %d", resp.StatusCode)
 	}
 }
 

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
+			channel <- errToAPIStatus(err)
-			switch {
-			case tools.IsEtcdTestFailed(err):
-				status = http.StatusConflict
-			}
-			channel <- &api.Status{
-				Status:  api.StatusFailure,
-				Message: err.Error(),
-				Code:    status,
-			}
 		} else {
 			channel <- obj
 		}

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.
+// apiServerError is an error intended for consumption by a REST API server
-type errNotFound string
+type apiServerError struct {
-
+	api.Status
-// Error returns a string representation of the err.
-func (err errNotFound) Error() string {
-	return string(err)
 }
 
-// IsNotFound determines if the err is an error which indicates that a specified resource was not found.
+// Error implements the Error interface.
-func IsNotFound(err error) bool {
+func (e *apiServerError) Error() string {
-	_, ok := err.(errNotFound)
+	return e.Status.Message
-	return ok
 }
 
 // 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.
+// NewAlreadyExistsErr returns an error indicating the item requested exists by that identifier
-type errAlreadyExists string
+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.
+// NewConflictErr returns an error indicating the item can't be updated as provided.
-func (err errAlreadyExists) Error() string {
+func NewConflictErr(kind, name string, err error) error {
-	return string(err)
+	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 reasonForError(err) == api.ReasonTypeAlreadyExists
-	return ok
 }
 
-// NewAlreadyExistsErr returns a new error which indicates that the resource of the kind and the name was not found.
+// IsConflict determines if the err is an error which indicates the provided update conflicts
-func NewAlreadyExistsErr(kind, name string) error {
+func IsConflict(err error) bool {
-	return errAlreadyExists(fmt.Sprintf("%s %q already exists", kind, name))
+	return reasonForError(err) == api.ReasonTypeConflict
 }
 
-// internalError renders a generic error to the response
+func reasonForError(err error) api.ReasonType {
-func internalError(err error, w http.ResponseWriter) {
+	switch t := err.(type) {
-	w.WriteHeader(http.StatusInternalServerError)
+	case *apiServerError:
-	fmt.Fprintf(w, "Internal Error: %#v", err)
+		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

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")
+	}
+}

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) {
@@ -66,7 +67,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
 		}