From d3be1ac92eb644e284915a55fe67942c33f88d4c Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Wed, 26 Jul 2017 21:43:24 -0400 Subject: [PATCH] Update generic errors with the new http package codes All of these errors are now part of the standard HTTP method. Formalize those into our error types and remove duplication and unclear separation. --- pkg/kubelet/server/server.go | 3 +- pkg/kubelet/server/server_test.go | 3 +- .../apimachinery/pkg/api/errors/errors.go | 45 +++++++++++-------- .../apimachinery/pkg/apis/meta/v1/types.go | 9 ++++ .../apiserver/pkg/endpoints/apiserver_test.go | 2 +- .../apiserver/pkg/endpoints/installer.go | 2 +- .../pkg/server/filters/maxinflight.go | 2 +- staging/src/k8s.io/client-go/rest/request.go | 2 +- .../src/k8s.io/client-go/rest/request_test.go | 6 +-- 9 files changed, 44 insertions(+), 30 deletions(-) diff --git a/pkg/kubelet/server/server.go b/pkg/kubelet/server/server.go index e728daaf784..74a5036828a 100644 --- a/pkg/kubelet/server/server.go +++ b/pkg/kubelet/server/server.go @@ -39,7 +39,6 @@ import ( "github.com/prometheus/client_golang/prometheus/promhttp" "k8s.io/api/core/v1" - apierrs "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -484,7 +483,7 @@ func (s *Server) getContainerLogs(request *restful.Request, response *restful.Re } logOptions.TypeMeta = metav1.TypeMeta{} if errs := validation.ValidatePodLogOptions(logOptions); len(errs) > 0 { - response.WriteError(apierrs.StatusUnprocessableEntity, fmt.Errorf(`{"message": "Invalid request."}`)) + response.WriteError(http.StatusUnprocessableEntity, fmt.Errorf(`{"message": "Invalid request."}`)) return } diff --git a/pkg/kubelet/server/server_test.go b/pkg/kubelet/server/server_test.go index 015947ddac4..b3b5ce9b46e 100644 --- a/pkg/kubelet/server/server_test.go +++ b/pkg/kubelet/server/server_test.go @@ -39,7 +39,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/api/core/v1" - apierrs "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/httpstream" @@ -1065,7 +1064,7 @@ func TestContainerLogsWithInvalidTail(t *testing.T) { t.Errorf("Got error GETing: %v", err) } defer resp.Body.Close() - if resp.StatusCode != apierrs.StatusUnprocessableEntity { + if resp.StatusCode != http.StatusUnprocessableEntity { t.Errorf("Unexpected non-error reading container logs: %#v", resp) } } diff --git a/staging/src/k8s.io/apimachinery/pkg/api/errors/errors.go b/staging/src/k8s.io/apimachinery/pkg/api/errors/errors.go index 560c889b9c1..6ede64718af 100644 --- a/staging/src/k8s.io/apimachinery/pkg/api/errors/errors.go +++ b/staging/src/k8s.io/apimachinery/pkg/api/errors/errors.go @@ -28,16 +28,6 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" ) -// HTTP Status codes not in the golang http package. -const ( - StatusUnprocessableEntity = 422 - StatusTooManyRequests = 429 - // StatusServerTimeout is an indication that a transient server error has - // occurred and the client *should* retry, with an optional Retry-After - // header to specify the back off window. - StatusServerTimeout = 504 -) - // StatusError is an error intended for consumption by a REST API server; it can also be // reconstructed by clients from a REST response. Public to allow easy type switches. type StatusError struct { @@ -189,7 +179,7 @@ func NewInvalid(qualifiedKind schema.GroupKind, name string, errs field.ErrorLis } return &StatusError{metav1.Status{ Status: metav1.StatusFailure, - Code: StatusUnprocessableEntity, // RFC 4918: StatusUnprocessableEntity + Code: http.StatusUnprocessableEntity, Reason: metav1.StatusReasonInvalid, Details: &metav1.StatusDetails{ Group: qualifiedKind.Group, @@ -211,6 +201,21 @@ func NewBadRequest(reason string) *StatusError { }} } +// NewTooManyRequests creates an error that indicates that the client must try again later because +// the specified endpoint is not accepting requests. More specific details should be provided +// if client should know why the failure was limited4. +func NewTooManyRequests(message string, retryAfterSeconds int) *StatusError { + return &StatusError{metav1.Status{ + Status: metav1.StatusFailure, + Code: http.StatusTooManyRequests, + Reason: metav1.StatusReasonTooManyRequests, + Message: message, + Details: &metav1.StatusDetails{ + RetryAfterSeconds: int32(retryAfterSeconds), + }, + }} +} + // NewServiceUnavailable creates an error that indicates that the requested service is unavailable. func NewServiceUnavailable(reason string) *StatusError { return &StatusError{metav1.Status{ @@ -276,7 +281,7 @@ func NewInternalError(err error) *StatusError { func NewTimeoutError(message string, retryAfterSeconds int) *StatusError { return &StatusError{metav1.Status{ Status: metav1.StatusFailure, - Code: StatusServerTimeout, + Code: http.StatusGatewayTimeout, Reason: metav1.StatusReasonTimeout, Message: fmt.Sprintf("Timeout: %s", message), Details: &metav1.StatusDetails{ @@ -313,14 +318,14 @@ func NewGenericServerResponse(code int, verb string, qualifiedResource schema.Gr case http.StatusMethodNotAllowed: reason = metav1.StatusReasonMethodNotAllowed message = "the server does not allow this method on the requested resource" - case StatusUnprocessableEntity: + case http.StatusUnprocessableEntity: reason = metav1.StatusReasonInvalid message = "the server rejected our request due to an error in our request" - case StatusServerTimeout: - reason = metav1.StatusReasonServerTimeout - message = "the server cannot complete the requested operation at this time, try again later" - case StatusTooManyRequests: + case http.StatusGatewayTimeout: reason = metav1.StatusReasonTimeout + message = "the server was unable to return a response in the time allotted, but may still be processing the request" + case http.StatusTooManyRequests: + reason = metav1.StatusReasonTooManyRequests message = "the server has received too many requests and has asked us to try again later" default: if code >= 500 { @@ -423,11 +428,13 @@ func IsInternalError(err error) bool { // IsTooManyRequests determines if err is an error which indicates that there are too many requests // that the server cannot handle. -// TODO: update IsTooManyRequests() when the TooManyRequests(429) error returned from the API server has a non-empty Reason field func IsTooManyRequests(err error) bool { + if reasonForError(err) == metav1.StatusReasonTooManyRequests { + return true + } switch t := err.(type) { case APIStatus: - return t.Status().Code == StatusTooManyRequests + return t.Status().Code == http.StatusTooManyRequests } return false } diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go index 14c7255c92b..8c1709e28a5 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go @@ -586,6 +586,15 @@ const ( // Status code 504 StatusReasonTimeout StatusReason = "Timeout" + // StatusReasonTooManyRequests means the server experienced too many requests within a + // given window and that the client must wait to perform the action again. A client may + // always retry the request that led to this error, although the client should wait at least + // the number of seconds specified by the retryAfterSeconds field. + // Details (optional): + // "retryAfterSeconds" int32 - the number of seconds before the operation should be retried + // Status code 429 + StatusReasonTooManyRequests StatusReason = "TooManyRequests" + // StatusReasonBadRequest means that the request itself was invalid, because the request // doesn't make any sense, for example deleting a read-only object. This is different than // StatusReasonInvalid above which indicates that the API call could possibly succeed, but the diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go index b28dbc1a1a1..01a4fb9ac07 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go @@ -3834,7 +3834,7 @@ func TestCreateTimeout(t *testing.T) { if err != nil { t.Errorf("unexpected error: %v", err) } - itemOut := expectApiStatus(t, "POST", server.URL+"/"+prefix+"/"+testGroupVersion.Group+"/"+testGroupVersion.Version+"/namespaces/default/foo?timeout=4ms", data, apierrs.StatusServerTimeout) + itemOut := expectApiStatus(t, "POST", server.URL+"/"+prefix+"/"+testGroupVersion.Group+"/"+testGroupVersion.Version+"/namespaces/default/foo?timeout=4ms", data, http.StatusGatewayTimeout) if itemOut.Status != metav1.StatusFailure || itemOut.Reason != metav1.StatusReasonTimeout { t.Errorf("Unexpected status %#v", itemOut) } diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/installer.go b/staging/src/k8s.io/apiserver/pkg/endpoints/installer.go index b8f6a038689..1e49cfa7d74 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/installer.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/installer.go @@ -509,7 +509,7 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag // http.StatusUnsupportedMediaType, http.StatusNotAcceptable, // http.StatusBadRequest, http.StatusUnauthorized, http.StatusForbidden, // http.StatusRequestTimeout, http.StatusConflict, http.StatusPreconditionFailed, - // 422 (StatusUnprocessableEntity), http.StatusInternalServerError, + // http.StatusUnprocessableEntity, http.StatusInternalServerError, // http.StatusServiceUnavailable // and api error codes // Note that if we specify a versioned Status object here, we may need to diff --git a/staging/src/k8s.io/apiserver/pkg/server/filters/maxinflight.go b/staging/src/k8s.io/apiserver/pkg/server/filters/maxinflight.go index 3e159c21f28..883a27dc355 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/filters/maxinflight.go +++ b/staging/src/k8s.io/apiserver/pkg/server/filters/maxinflight.go @@ -123,5 +123,5 @@ func WithMaxInFlightLimit( func tooManyRequests(req *http.Request, w http.ResponseWriter) { // Return a 429 status indicating "Too Many Requests" w.Header().Set("Retry-After", retryAfter) - http.Error(w, "Too many requests, please try again later.", errors.StatusTooManyRequests) + http.Error(w, "Too many requests, please try again later.", http.StatusTooManyRequests) } diff --git a/staging/src/k8s.io/client-go/rest/request.go b/staging/src/k8s.io/client-go/rest/request.go index 2d676cec06f..68ac4752682 100644 --- a/staging/src/k8s.io/client-go/rest/request.go +++ b/staging/src/k8s.io/client-go/rest/request.go @@ -889,7 +889,7 @@ func isTextResponse(resp *http.Response) bool { func checkWait(resp *http.Response) (int, bool) { switch r := resp.StatusCode; { // any 500 error code and 429 can trigger a wait - case r == errors.StatusTooManyRequests, r >= 500: + case r == http.StatusTooManyRequests, r >= 500: default: return 0, false } diff --git a/staging/src/k8s.io/client-go/rest/request_test.go b/staging/src/k8s.io/client-go/rest/request_test.go index fa016dfe1ce..0691fc7260c 100755 --- a/staging/src/k8s.io/client-go/rest/request_test.go +++ b/staging/src/k8s.io/client-go/rest/request_test.go @@ -1130,7 +1130,7 @@ func TestCheckRetryClosesBody(t *testing.T) { return } w.Header().Set("Retry-After", "1") - http.Error(w, "Too many requests, please try again later.", apierrors.StatusTooManyRequests) + http.Error(w, "Too many requests, please try again later.", http.StatusTooManyRequests) })) defer testServer.Close() @@ -1204,7 +1204,7 @@ func TestCheckRetryHandles429And5xx(t *testing.T) { return } w.Header().Set("Retry-After", "0") - w.WriteHeader([]int{apierrors.StatusTooManyRequests, 500, 501, 504}[count]) + w.WriteHeader([]int{http.StatusTooManyRequests, 500, 501, 504}[count]) count++ })) defer testServer.Close() @@ -1234,7 +1234,7 @@ func BenchmarkCheckRetryClosesBody(b *testing.B) { return } w.Header().Set("Retry-After", "0") - w.WriteHeader(apierrors.StatusTooManyRequests) + w.WriteHeader(http.StatusTooManyRequests) })) defer testServer.Close()