Add more specific error handling and handle generic errors more effectively

The client library should type more generic errors from the server.

pkg/api/errors/errors.go

@@ -105,6 +105,21 @@ func NewAlreadyExists(kind, name string) error {
 	}}
 }
 
+// NewUnauthorized returns an error indicating the client is not authorized to perform the requested
+// action.
+func NewUnauthorized(reason string) error {
+	message := reason
+	if len(message) == 0 {
+		message = "not authorized"
+	}
+	return &StatusError{api.Status{
+		Status:  api.StatusFailure,
+		Code:    http.StatusUnauthorized,
+		Reason:  api.StatusReasonUnauthorized,
+		Message: message,
+	}}
+}
+
 // NewForbidden returns an error indicating the requested action was forbidden
 func NewForbidden(kind, name string, err error) error {
 	return &StatusError{api.Status{
@@ -183,14 +198,15 @@ func NewMethodNotSupported(kind, action string) error {
 
 // NewServerTimeout returns an error indicating the requested action could not be completed due to a
 // transient error, and the client should try again.
-func NewServerTimeout(kind, operation string) error {
+func NewServerTimeout(kind, operation string, retryAfter int) error {
 	return &StatusError{api.Status{
 		Status: api.StatusFailure,
 		Code:   http.StatusInternalServerError,
 		Reason: api.StatusReasonServerTimeout,
 		Details: &api.StatusDetails{
-			Kind: kind,
-			ID:   operation,
+			Kind:       kind,
+			ID:         operation,
+			RetryAfter: retryAfter,
 		},
 		Message: fmt.Sprintf("The %s operation against %s could not be completed at this time, please try again.", operation, kind),
 	}}
@@ -211,12 +227,15 @@ func NewInternalError(err error) error {
 
 // NewTimeoutError returns an error indicating that a timeout occurred before the request
 // could be completed.  Clients may retry, but the operation may still complete.
-func NewTimeoutError(message string) error {
+func NewTimeoutError(message string, retryAfter int) error {
 	return &StatusError{api.Status{
 		Status:  api.StatusFailure,
 		Code:    StatusServerTimeout,
 		Reason:  api.StatusReasonTimeout,
 		Message: fmt.Sprintf("Timeout: %s", message),
+		Details: &api.StatusDetails{
+			RetryAfter: retryAfter,
+		},
 	}}
 }
 
@@ -251,6 +270,12 @@ func IsBadRequest(err error) bool {
 	return reasonForError(err) == api.StatusReasonBadRequest
 }
 
+// IsUnauthorized determines if err is an error which indicates that the request is unauthorized and
+// requires authentication by the user.
+func IsUnauthorized(err error) bool {
+	return reasonForError(err) == api.StatusReasonUnauthorized
+}
+
 // IsForbidden determines if err is an error which indicates that the request is forbidden and cannot
 // be completed as requested.
 func IsForbidden(err error) bool {
@@ -275,6 +300,21 @@ func IsUnexpectedObjectError(err error) bool {
 	return ok
 }
 
+// SuggestsClientDelay returns true if this error suggests a client delay as well as the
+// suggested seconds to wait, or false if the error does not imply a wait.
+func SuggestsClientDelay(err error) (int, bool) {
+	switch t := err.(type) {
+	case *StatusError:
+		if t.Status().Details != nil {
+			switch t.Status().Reason {
+			case api.StatusReasonServerTimeout, api.StatusReasonTimeout:
+				return t.Status().Details.RetryAfter, true
+			}
+		}
+	}
+	return 0, false
+}
+
 func reasonForError(err error) api.StatusReason {
 	switch t := err.(type) {
 	case *StatusError:

pkg/api/errors/errors_test.go

@@ -69,9 +69,18 @@ func TestErrorNew(t *testing.T) {
 	if !IsForbidden(NewForbidden("test", "2", errors.New("reason"))) {
 		t.Errorf("expected to be %s", api.StatusReasonForbidden)
 	}
-	if !IsServerTimeout(NewServerTimeout("test", "reason")) {
+	if !IsUnauthorized(NewUnauthorized("reason")) {
+		t.Errorf("expected to be %s", api.StatusReasonUnauthorized)
+	}
+	if !IsServerTimeout(NewServerTimeout("test", "reason", 0)) {
 		t.Errorf("expected to be %s", api.StatusReasonServerTimeout)
 	}
+	if time, ok := SuggestsClientDelay(NewServerTimeout("test", "doing something", 10)); time != 10 || !ok {
+		t.Errorf("expected to be %s", api.StatusReasonServerTimeout)
+	}
+	if time, ok := SuggestsClientDelay(NewTimeoutError("test reason", 10)); time != 10 || !ok {
+		t.Errorf("expected to be %s", api.StatusReasonTimeout)
+	}
 	if !IsMethodNotSupported(NewMethodNotSupported("foo", "delete")) {
 		t.Errorf("expected to be %s", api.StatusReasonMethodNotAllowed)
 	}

pkg/api/rest/create.go

@@ -84,7 +84,7 @@ func CheckGeneratedNameError(strategy RESTCreateStrategy, err error, obj runtime
 		return err
 	}
 
-	return errors.NewServerTimeout(kind, "POST")
+	return errors.NewServerTimeout(kind, "POST", 0)
 }
 
 // objectMetaAndKind retrieves kind and ObjectMeta from a runtime object, or returns an error.

pkg/api/types.go

@@ -1200,6 +1200,8 @@ type StatusDetails struct {
 	// The Causes array includes more details associated with the StatusReason
 	// failure. Not all StatusReasons may provide detailed causes.
 	Causes []StatusCause `json:"causes,omitempty"`
+	// If specified, the time in seconds before the operation should be retried.
+	RetryAfter int `json:"retryAfter,omitempty"`
 }
 
 // Values of Status.Status
@@ -1220,6 +1222,13 @@ const (
 	// Status code 500.
 	StatusReasonUnknown StatusReason = ""
 
+	// StatusReasonUnauthorized means the server can be reached and understood the request, but requires
+	// the user to present appropriate authorization credentials (identified by the WWW-Authenticate header)
+	// in order for the action to be completed. If the user has specified credentials on the request, the
+	// server considers them insufficient.
+	// Status code 401
+	StatusReasonUnauthorized StatusReason = "Unauthorized"
+
 	// StatusReasonForbidden means the server can be reached and understood the request, but refuses
 	// to take any further action.  It is the result of the server being configured to deny access for some reason
 	// to the requested resource by the client.
@@ -1276,12 +1285,17 @@ const (
 	// Details (optional):
 	//   "kind" string - the kind attribute of the resource being acted on.
 	//   "id"   string - the operation that is being attempted.
+	//   "retryAfter" int - the number of seconds before the operation should be retried
 	// Status code 500
 	StatusReasonServerTimeout StatusReason = "ServerTimeout"
 
 	// StatusReasonTimeout means that the request could not be completed within the given time.
-	// Clients can get this response only when they specified a timeout param in the request.
-	// The request might succeed with an increased value of timeout param.
+	// Clients can get this response only when they specified a timeout param in the request,
+	// or if the server cannot complete the operation within a reasonable amount of time.
+	// The request might succeed with an increased value of timeout param. The client *should*
+	// wait at least the number of seconds specified by the retryAfter field.
+	// Details (optional):
+	//   "retryAfter" int - the number of seconds before the operation should be retried
 	// Status code 504
 	StatusReasonTimeout StatusReason = "Timeout"
 

pkg/api/v1beta1/types.go

@@ -1014,6 +1014,8 @@ type StatusDetails struct {
 	// The Causes array includes more details associated with the StatusReason
 	// failure. Not all StatusReasons may provide detailed causes.
 	Causes []StatusCause `json:"causes,omitempty" description:"the Causes array includes more details associated with the StatusReason failure; not all StatusReasons may provide detailed causes"`
+	// If specified, the time in seconds before the operation should be retried.
+	RetryAfter int `json:"retryAfter,omitempty" description:"the number of seconds before the client should attempt to retry this operation"`
 }
 
 // Values of Status.Status

pkg/api/v1beta2/types.go

@@ -1028,6 +1028,8 @@ type StatusDetails struct {
 	// The Causes array includes more details associated with the StatusReason
 	// failure. Not all StatusReasons may provide detailed causes.
 	Causes []StatusCause `json:"causes,omitempty" description:"the Causes array includes more details associated with the StatusReason failure; not all StatusReasons may provide detailed causes"`
+	// If specified, the time in seconds before the operation should be retried.
+	RetryAfter int `json:"retryAfter,omitempty" description:"the number of seconds before the client should attempt to retry this operation"`
 }
 
 // Values of Status.Status

pkg/api/v1beta3/types.go

@@ -1187,6 +1187,8 @@ type StatusDetails struct {
 	// The Causes array includes more details associated with the StatusReason
 	// failure. Not all StatusReasons may provide detailed causes.
 	Causes []StatusCause `json:"causes,omitempty" description:"the Causes array includes more details associated with the StatusReason failure; not all StatusReasons may provide detailed causes"`
+	// If specified, the time in seconds before the operation should be retried.
+	RetryAfter int `json:"retryAfter,omitempty" description:"the number of seconds before the client should attempt to retry this operation"`
 }
 
 // Values of Status.Status

pkg/apiserver/resthandler.go

@@ -436,7 +436,7 @@ func finishRequest(timeout time.Duration, fn resultFunc) (result runtime.Object,
 	case err = <-errCh:
 		return nil, err
 	case <-time.After(timeout):
-		return nil, errors.NewTimeoutError("request did not complete within allowed duration")
+		return nil, errors.NewTimeoutError("request did not complete within allowed duration", 0)
 	}
 }
 

pkg/client/request.go

@@ -22,6 +22,7 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
+	"mime"
 	"net/http"
 	"net/url"
 	"path"
@@ -621,12 +622,15 @@ func (r *Request) transformResponse(body []byte, resp *http.Response, req *http.
 		// no-op, we've been upgraded
 	case resp.StatusCode < http.StatusOK || resp.StatusCode > http.StatusPartialContent:
 		if !isStatusResponse {
-			var err error
-			err = &UnexpectedStatusError{
+			var err error = &UnexpectedStatusError{
 				Request:  req,
 				Response: resp,
 				Body:     string(body),
 			}
+			message := "unknown"
+			if isTextResponse(resp) {
+				message = strings.TrimSpace(string(body))
+			}
 			// TODO: handle other error classes we know about
 			switch resp.StatusCode {
 			case http.StatusConflict:
@@ -638,7 +642,21 @@ func (r *Request) transformResponse(body []byte, resp *http.Response, req *http.
 			case http.StatusNotFound:
 				err = errors.NewNotFound(r.resource, r.resourceName)
 			case http.StatusBadRequest:
-				err = errors.NewBadRequest(err.Error())
+				err = errors.NewBadRequest(message)
+			case http.StatusUnauthorized:
+				err = errors.NewUnauthorized(message)
+			case http.StatusForbidden:
+				err = errors.NewForbidden(r.resource, r.resourceName, err)
+			case errors.StatusUnprocessableEntity:
+				err = errors.NewInvalid(r.resource, r.resourceName, nil)
+			case errors.StatusServerTimeout:
+				retryAfter, _ := retryAfter(resp)
+				err = errors.NewServerTimeout(r.resource, r.verb, retryAfter)
+			case errors.StatusTooManyRequests:
+				retryAfter, _ := retryAfter(resp)
+				err = errors.NewServerTimeout(r.resource, r.verb, retryAfter)
+			case http.StatusInternalServerError:
+				err = errors.NewInternalError(fmt.Errorf(message))
 			}
 			return nil, false, err
 		}
@@ -657,6 +675,30 @@ func (r *Request) transformResponse(body []byte, resp *http.Response, req *http.
 	return body, created, nil
 }
 
+// isTextResponse returns true if the response appears to be a textual media type.
+func isTextResponse(resp *http.Response) bool {
+	contentType := resp.Header.Get("Content-Type")
+	if len(contentType) == 0 {
+		return true
+	}
+	media, _, err := mime.ParseMediaType(contentType)
+	if err != nil {
+		return false
+	}
+	return strings.HasPrefix(media, "text/")
+}
+
+// retryAfter returns the value of the Retry-After header and true, or 0 and false if
+// the header was missing or not a valid number.
+func retryAfter(resp *http.Response) (int, bool) {
+	if h := resp.Header.Get("Retry-After"); len(h) > 0 {
+		if i, err := strconv.Atoi(h); err == nil {
+			return i, true
+		}
+	}
+	return 0, false
+}
+
 // Result contains the result of calling Request.Do().
 type Result struct {
 	body    []byte

pkg/client/request_test.go

@@ -222,6 +222,7 @@ func TestTransformResponse(t *testing.T) {
 		Data     []byte
 		Created  bool
 		Error    bool
+		ErrFn    func(err error) bool
 	}{
 		{Response: &http.Response{StatusCode: 200}, Data: []byte{}},
 		{Response: &http.Response{StatusCode: 201}, Data: []byte{}, Created: true},
@@ -230,6 +231,30 @@ func TestTransformResponse(t *testing.T) {
 		{Response: &http.Response{StatusCode: 422}, Error: true},
 		{Response: &http.Response{StatusCode: 409}, Error: true},
 		{Response: &http.Response{StatusCode: 404}, Error: true},
+		{Response: &http.Response{StatusCode: 401}, Error: true},
+		{
+			Response: &http.Response{
+				StatusCode: 401,
+				Header:     http.Header{"Content-Type": []string{"application/json"}},
+				Body:       ioutil.NopCloser(bytes.NewReader(invalid)),
+			},
+			Error: true,
+			ErrFn: func(err error) bool {
+				return err.Error() != "aaaaa" && apierrors.IsUnauthorized(err)
+			},
+		},
+		{
+			Response: &http.Response{
+				StatusCode: 401,
+				Header:     http.Header{"Content-Type": []string{"text/any"}},
+				Body:       ioutil.NopCloser(bytes.NewReader(invalid)),
+			},
+			Error: true,
+			ErrFn: func(err error) bool {
+				return err.Error() == "aaaaa" && apierrors.IsUnauthorized(err)
+			},
+		},
+		{Response: &http.Response{StatusCode: 403}, Error: true},
 		{Response: &http.Response{StatusCode: 200, Body: ioutil.NopCloser(bytes.NewReader(invalid))}, Data: invalid},
 		{Response: &http.Response{StatusCode: 200, Body: ioutil.NopCloser(bytes.NewReader(invalid))}, Data: invalid},
 	}
@@ -246,6 +271,18 @@ func TestTransformResponse(t *testing.T) {
 		hasErr := err != nil
 		if hasErr != test.Error {
 			t.Errorf("%d: unexpected error: %t %v", i, test.Error, err)
+		} else if hasErr && test.Response.StatusCode > 399 {
+			status, ok := err.(APIStatus)
+			if !ok {
+				t.Errorf("%d: response should have been transformable into APIStatus: %v", i, err)
+				continue
+			}
+			if status.Status().Code != test.Response.StatusCode {
+				t.Errorf("%d: status code did not match response: %#v", i, status.Status())
+			}
+		}
+		if test.ErrFn != nil && !test.ErrFn(err) {
+			t.Errorf("%d: error function did not match: %v", i, err)
 		}
 		if !(test.Data == nil && response == nil) && !api.Semantic.DeepDerivative(test.Data, response) {
 			t.Errorf("%d: unexpected response: %#v %#v", i, test.Data, response)

pkg/client/restclient_test.go

@@ -225,7 +225,13 @@ func TestDoRequestSuccess(t *testing.T) {
 }
 
 func TestDoRequestFailed(t *testing.T) {
-	status := &api.Status{Status: api.StatusFailure, Reason: api.StatusReasonInvalid, Details: &api.StatusDetails{ID: "test", Kind: "test"}}
+	status := &api.Status{
+		Code:    http.StatusNotFound,
+		Status:  api.StatusFailure,
+		Reason:  api.StatusReasonNotFound,
+		Message: " \"\" not found",
+		Details: &api.StatusDetails{},
+	}
 	expectedBody, _ := latest.Codec.Encode(status)
 	fakeHandler := util.FakeHandler{
 		StatusCode:   404,
@@ -253,7 +259,7 @@ func TestDoRequestFailed(t *testing.T) {
 	}
 	actual := ss.Status()
 	if !reflect.DeepEqual(status, &actual) {
-		t.Errorf("Unexpected mis-match. Expected %#v.  Saw %#v", status, actual)
+		t.Errorf("Unexpected mis-match: %s", util.ObjectDiff(status, &actual))
 	}
 }