From 30358e8b047d76a9570d34e8010e078b9874c625 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Mon, 23 Mar 2015 22:56:22 -0400 Subject: [PATCH] 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 | 48 ++++++++++++++++++++++++++++++++--- pkg/api/errors/errors_test.go | 11 +++++++- pkg/api/rest/create.go | 2 +- pkg/api/types.go | 18 +++++++++++-- pkg/api/v1beta1/types.go | 2 ++ pkg/api/v1beta2/types.go | 2 ++ pkg/api/v1beta3/types.go | 2 ++ pkg/apiserver/resthandler.go | 2 +- pkg/client/request.go | 48 ++++++++++++++++++++++++++++++++--- pkg/client/request_test.go | 37 +++++++++++++++++++++++++++ pkg/client/restclient_test.go | 10 ++++++-- 11 files changed, 168 insertions(+), 14 deletions(-) diff --git a/pkg/api/errors/errors.go b/pkg/api/errors/errors.go index f0c3ed9c898..816faf4241a 100644 --- a/pkg/api/errors/errors.go +++ b/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: diff --git a/pkg/api/errors/errors_test.go b/pkg/api/errors/errors_test.go index f73f1c99ad6..1c257a51f08 100644 --- a/pkg/api/errors/errors_test.go +++ b/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) } diff --git a/pkg/api/rest/create.go b/pkg/api/rest/create.go index 27f0d34c397..f6bd48220f9 100644 --- a/pkg/api/rest/create.go +++ b/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. diff --git a/pkg/api/types.go b/pkg/api/types.go index 273e59ad09d..b8d2b7f34b1 100644 --- a/pkg/api/types.go +++ b/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" diff --git a/pkg/api/v1beta1/types.go b/pkg/api/v1beta1/types.go index a8cda9ca232..426960b281e 100644 --- a/pkg/api/v1beta1/types.go +++ b/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 diff --git a/pkg/api/v1beta2/types.go b/pkg/api/v1beta2/types.go index 325dd419bbe..f2352074f5c 100644 --- a/pkg/api/v1beta2/types.go +++ b/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 diff --git a/pkg/api/v1beta3/types.go b/pkg/api/v1beta3/types.go index ca8105bdcf9..b243a14e1d0 100644 --- a/pkg/api/v1beta3/types.go +++ b/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 diff --git a/pkg/apiserver/resthandler.go b/pkg/apiserver/resthandler.go index df363a6fba3..25ae6c98a0d 100644 --- a/pkg/apiserver/resthandler.go +++ b/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) } } diff --git a/pkg/client/request.go b/pkg/client/request.go index 58a1a8477e9..d3e6e795834 100644 --- a/pkg/client/request.go +++ b/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 diff --git a/pkg/client/request_test.go b/pkg/client/request_test.go index f0267026304..1ce71dbaca4 100644 --- a/pkg/client/request_test.go +++ b/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) diff --git a/pkg/client/restclient_test.go b/pkg/client/restclient_test.go index e9c3ede61ce..e5e1706d3da 100644 --- a/pkg/client/restclient_test.go +++ b/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)) } }