From a7834389b4475c15bfbe958f1bc642f5e7a16a97 Mon Sep 17 00:00:00 2001 From: Billie Cleek Date: Tue, 11 May 2021 16:41:20 -0700 Subject: [PATCH] check APIStatus.Code in Is* family of functions Check the Code in the Is family of functions where it makes sense. This was already done in a couple of functions, but there were many others where checking the code makes sense. Consider known reasons in the Is* family of functions for checking errors based on StatusReason and code. Maintain backward compatibility by not checkng APIStatus for an unknown reason in either IsTooManyRequests or IsRequestEntityToLargeError Add tests for error types by code Add tests to exercise error checks using both reasons and codes. --- .../apimachinery/pkg/api/errors/errors.go | 170 ++++++++++++++++-- .../pkg/api/errors/errors_test.go | 92 ++++++++++ 2 files changed, 243 insertions(+), 19 deletions(-) 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 31eddfc1ea0..959885cfbbe 100644 --- a/staging/src/k8s.io/apimachinery/pkg/api/errors/errors.go +++ b/staging/src/k8s.io/apimachinery/pkg/api/errors/errors.go @@ -44,6 +44,28 @@ type APIStatus interface { var _ error = &StatusError{} +var knownReasons = map[metav1.StatusReason]struct{}{ + // metav1.StatusReasonUnknown : {} + metav1.StatusReasonUnauthorized: {}, + metav1.StatusReasonForbidden: {}, + metav1.StatusReasonNotFound: {}, + metav1.StatusReasonAlreadyExists: {}, + metav1.StatusReasonConflict: {}, + metav1.StatusReasonGone: {}, + metav1.StatusReasonInvalid: {}, + metav1.StatusReasonServerTimeout: {}, + metav1.StatusReasonTimeout: {}, + metav1.StatusReasonTooManyRequests: {}, + metav1.StatusReasonBadRequest: {}, + metav1.StatusReasonMethodNotAllowed: {}, + metav1.StatusReasonNotAcceptable: {}, + metav1.StatusReasonRequestEntityTooLarge: {}, + metav1.StatusReasonUnsupportedMediaType: {}, + metav1.StatusReasonInternalError: {}, + metav1.StatusReasonExpired: {}, + metav1.StatusReasonServiceUnavailable: {}, +} + // Error implements the Error interface. func (e *StatusError) Error() string { return e.ErrStatus.Message @@ -478,7 +500,14 @@ func NewGenericServerResponse(code int, verb string, qualifiedResource schema.Gr // IsNotFound returns true if the specified error was created by NewNotFound. // It supports wrapped errors. func IsNotFound(err error) bool { - return ReasonForError(err) == metav1.StatusReasonNotFound + reason, code := reasonAndCodeForError(err) + if reason == metav1.StatusReasonNotFound { + return true + } + if _, ok := knownReasons[reason]; !ok && code == http.StatusNotFound { + return true + } + return false } // IsAlreadyExists determines if the err is an error which indicates that a specified resource already exists. @@ -490,19 +519,40 @@ func IsAlreadyExists(err error) bool { // IsConflict determines if the err is an error which indicates the provided update conflicts. // It supports wrapped errors. func IsConflict(err error) bool { - return ReasonForError(err) == metav1.StatusReasonConflict + reason, code := reasonAndCodeForError(err) + if reason == metav1.StatusReasonConflict { + return true + } + if _, ok := knownReasons[reason]; !ok && code == http.StatusConflict { + return true + } + return false } // IsInvalid determines if the err is an error which indicates the provided resource is not valid. // It supports wrapped errors. func IsInvalid(err error) bool { - return ReasonForError(err) == metav1.StatusReasonInvalid + reason, code := reasonAndCodeForError(err) + if reason == metav1.StatusReasonInvalid { + return true + } + if _, ok := knownReasons[reason]; !ok && code == http.StatusUnprocessableEntity { + return true + } + return false } // IsGone is true if the error indicates the requested resource is no longer available. // It supports wrapped errors. func IsGone(err error) bool { - return ReasonForError(err) == metav1.StatusReasonGone + reason, code := reasonAndCodeForError(err) + if reason == metav1.StatusReasonGone { + return true + } + if _, ok := knownReasons[reason]; !ok && code == http.StatusGone { + return true + } + return false } // IsResourceExpired is true if the error indicates the resource has expired and the current action is @@ -515,77 +565,147 @@ func IsResourceExpired(err error) bool { // IsNotAcceptable determines if err is an error which indicates that the request failed due to an invalid Accept header // It supports wrapped errors. func IsNotAcceptable(err error) bool { - return ReasonForError(err) == metav1.StatusReasonNotAcceptable + reason, code := reasonAndCodeForError(err) + if reason == metav1.StatusReasonNotAcceptable { + return true + } + if _, ok := knownReasons[reason]; !ok && code == http.StatusNotAcceptable { + return true + } + return false } // IsUnsupportedMediaType determines if err is an error which indicates that the request failed due to an invalid Content-Type header // It supports wrapped errors. func IsUnsupportedMediaType(err error) bool { - return ReasonForError(err) == metav1.StatusReasonUnsupportedMediaType + reason, code := reasonAndCodeForError(err) + if reason == metav1.StatusReasonUnsupportedMediaType { + return true + } + if _, ok := knownReasons[reason]; !ok && code == http.StatusUnsupportedMediaType { + return true + } + return false } // IsMethodNotSupported determines if the err is an error which indicates the provided action could not // be performed because it is not supported by the server. // It supports wrapped errors. func IsMethodNotSupported(err error) bool { - return ReasonForError(err) == metav1.StatusReasonMethodNotAllowed + reason, code := reasonAndCodeForError(err) + if reason == metav1.StatusReasonMethodNotAllowed { + return true + } + if _, ok := knownReasons[reason]; !ok && code == http.StatusMethodNotAllowed { + return true + } + return false } // IsServiceUnavailable is true if the error indicates the underlying service is no longer available. // It supports wrapped errors. func IsServiceUnavailable(err error) bool { - return ReasonForError(err) == metav1.StatusReasonServiceUnavailable + reason, code := reasonAndCodeForError(err) + if reason == metav1.StatusReasonServiceUnavailable { + return true + } + if _, ok := knownReasons[reason]; !ok && code == http.StatusServiceUnavailable { + return true + } + return false } // IsBadRequest determines if err is an error which indicates that the request is invalid. // It supports wrapped errors. func IsBadRequest(err error) bool { - return ReasonForError(err) == metav1.StatusReasonBadRequest + reason, code := reasonAndCodeForError(err) + if reason == metav1.StatusReasonBadRequest { + return true + } + if _, ok := knownReasons[reason]; !ok && code == http.StatusBadRequest { + return true + } + return false } // IsUnauthorized determines if err is an error which indicates that the request is unauthorized and // requires authentication by the user. // It supports wrapped errors. func IsUnauthorized(err error) bool { - return ReasonForError(err) == metav1.StatusReasonUnauthorized + reason, code := reasonAndCodeForError(err) + if reason == metav1.StatusReasonUnauthorized { + return true + } + if _, ok := knownReasons[reason]; !ok && code == http.StatusUnauthorized { + return true + } + return false } // IsForbidden determines if err is an error which indicates that the request is forbidden and cannot // be completed as requested. // It supports wrapped errors. func IsForbidden(err error) bool { - return ReasonForError(err) == metav1.StatusReasonForbidden + reason, code := reasonAndCodeForError(err) + if reason == metav1.StatusReasonForbidden { + return true + } + if _, ok := knownReasons[reason]; !ok && code == http.StatusForbidden { + return true + } + return false } // IsTimeout determines if err is an error which indicates that request times out due to long // processing. // It supports wrapped errors. func IsTimeout(err error) bool { - return ReasonForError(err) == metav1.StatusReasonTimeout + reason, code := reasonAndCodeForError(err) + if reason == metav1.StatusReasonTimeout { + return true + } + if _, ok := knownReasons[reason]; !ok && code == http.StatusGatewayTimeout { + return true + } + return false } // IsServerTimeout determines if err is an error which indicates that the request needs to be retried // by the client. // It supports wrapped errors. func IsServerTimeout(err error) bool { + // do not check the status code, because no https status code exists that can + // be scoped to retryable timeouts. return ReasonForError(err) == metav1.StatusReasonServerTimeout } // IsInternalError determines if err is an error which indicates an internal server error. // It supports wrapped errors. func IsInternalError(err error) bool { - return ReasonForError(err) == metav1.StatusReasonInternalError + reason, code := reasonAndCodeForError(err) + if reason == metav1.StatusReasonInternalError { + return true + } + if _, ok := knownReasons[reason]; !ok && code == http.StatusInternalServerError { + return true + } + return false } // IsTooManyRequests determines if err is an error which indicates that there are too many requests // that the server cannot handle. // It supports wrapped errors. func IsTooManyRequests(err error) bool { - if ReasonForError(err) == metav1.StatusReasonTooManyRequests { + reason, code := reasonAndCodeForError(err) + if reason == metav1.StatusReasonTooManyRequests { return true } - if status := APIStatus(nil); errors.As(err, &status) { - return status.Status().Code == http.StatusTooManyRequests + + // IsTooManyRequests' checking of code predates the checking of the code in + // the other Is* functions. In order to maintain backward compatibility, this + // does not check that the reason is unknown. + if code == http.StatusTooManyRequests { + return true } return false } @@ -594,11 +714,16 @@ func IsTooManyRequests(err error) bool { // the request entity is too large. // It supports wrapped errors. func IsRequestEntityTooLargeError(err error) bool { - if ReasonForError(err) == metav1.StatusReasonRequestEntityTooLarge { + reason, code := reasonAndCodeForError(err) + if reason == metav1.StatusReasonRequestEntityTooLarge { return true } - if status := APIStatus(nil); errors.As(err, &status) { - return status.Status().Code == http.StatusRequestEntityTooLarge + + // IsRequestEntityTooLargeError's checking of code predates the checking of + // the code in the other Is* functions. In order to maintain backward + // compatibility, this does not check that the reason is unknown. + if code == http.StatusRequestEntityTooLarge { + return true } return false } @@ -653,6 +778,13 @@ func ReasonForError(err error) metav1.StatusReason { return metav1.StatusReasonUnknown } +func reasonAndCodeForError(err error) (metav1.StatusReason, int32) { + if status := APIStatus(nil); errors.As(err, &status) { + return status.Status().Reason, status.Status().Code + } + return metav1.StatusReasonUnknown, 0 +} + // ErrorReporter converts generic errors into runtime.Object errors without // requiring the caller to take a dependency on meta/v1 (where Status lives). // This prevents circular dependencies in core watch code. diff --git a/staging/src/k8s.io/apimachinery/pkg/api/errors/errors_test.go b/staging/src/k8s.io/apimachinery/pkg/api/errors/errors_test.go index 6b9e3b81093..ffba3f3aa54 100644 --- a/staging/src/k8s.io/apimachinery/pkg/api/errors/errors_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/api/errors/errors_test.go @@ -478,3 +478,95 @@ func TestSuggestsClientDelaySupportsWrapping(t *testing.T) { }) } } + +func TestIsErrorTypesByReasonAndCode(t *testing.T) { + testCases := []struct { + name string + knownReason metav1.StatusReason + otherReason metav1.StatusReason + otherReasonConsidered bool + code int32 + fn func(error) bool + }{ + { + name: "IsRequestEntityTooLarge", + knownReason: metav1.StatusReasonRequestEntityTooLarge, + otherReason: metav1.StatusReasonForbidden, + otherReasonConsidered: false, + code: http.StatusRequestEntityTooLarge, + fn: IsRequestEntityTooLargeError, + }, { + name: "TooManyRequests", + knownReason: metav1.StatusReasonTooManyRequests, + otherReason: metav1.StatusReasonForbidden, + otherReasonConsidered: false, + code: http.StatusTooManyRequests, + fn: IsTooManyRequests, + }, { + name: "Forbidden", + knownReason: metav1.StatusReasonForbidden, + otherReason: metav1.StatusReasonNotFound, + otherReasonConsidered: true, + code: http.StatusForbidden, + fn: IsForbidden, + }, { + name: "NotFound", + knownReason: metav1.StatusReasonNotFound, + otherReason: metav1.StatusReasonForbidden, + otherReasonConsidered: true, + code: http.StatusNotFound, + fn: IsNotFound, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Run("by known reason", func(t *testing.T) { + err := &StatusError{ + metav1.Status{ + Reason: tc.knownReason, + }, + } + + got := tc.fn(err) + if !got { + t.Errorf("expected reason %s to match", tc.knownReason) + } + }) + + t.Run("by code and unknown reason", func(t *testing.T) { + err := &StatusError{ + metav1.Status{ + Reason: metav1.StatusReasonUnknown, // this could be _any_ reason that isn't in knownReasons. + Code: tc.code, + }, + } + + got := tc.fn(err) + if !got { + t.Errorf("expected code %d with reason %s to match", tc.code, tc.otherReason) + } + }) + + if !tc.otherReasonConsidered { + return + } + + t.Run("by code and other known reason", func(t *testing.T) { + err := &StatusError{ + metav1.Status{ + Reason: tc.otherReason, + Code: tc.code, + }, + } + + got := tc.fn(err) + if got { + t.Errorf("expected code %d with reason %s to not match", tc.code, tc.otherReason) + } + }) + + }) + + } +}