Skip to content

Commit

Permalink
Merge pull request #884 from hime/automated-cherry-pick-of-#864-upstr…
Browse files Browse the repository at this point in the history 
…eam-release-1.6

Automated cherry pick of #864: Properly unwrap gce-compute error code.
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Jun 14, 2024
2 parents cdaa196 + 8ee644b commit 71b89ba
Show file tree
Hide file tree
Showing 2 changed files with 132 additions and 52 deletions.
65 changes: 52 additions & 13 deletions pkg/cloud_provider/file/file.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"time"

"github.com/container-storage-interface/spec/lib/go/csi"
"github.com/googleapis/gax-go/v2/apierror"
"google.golang.org/api/googleapi"
"google.golang.org/api/option"
"google.golang.org/grpc/codes"
Expand Down Expand Up @@ -215,6 +216,14 @@ var (
shareUriRegex = regexp.MustCompile(`^projects/([^/]+)/locations/([^/]+)/instances/([^/]+)/shares/([^/]+)$`)
)

// userErrorCodeMap tells how API error types are translated to error codes.
var userErrorCodeMap = map[int]codes.Code{
http.StatusForbidden: codes.PermissionDenied,
http.StatusBadRequest: codes.InvalidArgument,
http.StatusTooManyRequests: codes.ResourceExhausted,
http.StatusNotFound: codes.NotFound,
}

func NewGCFSService(version string, client *http.Client, primaryFilestoreServiceEndpoint, testFilestoreServiceEndpoint string) (Service, error) {
ctx := context.Background()

Expand Down Expand Up @@ -631,23 +640,14 @@ func IsNotFoundErr(err error) bool {
// (2) http 403 Forbidden, returns grpc PermissionDenied,
// (3) http 404 Not Found, returns grpc NotFound
// (4) http 429 Too Many Requests, returns grpc ResourceExhausted
func isUserError(err error) *codes.Code {
func isUserOperationError(err error) *codes.Code {
// Upwrap the error
var apiErr *googleapi.Error
if !errors.As(err, &apiErr) {
// Fallback to check for expected error code in the error string
return containsUserErrStr(err)
}

userErrors := map[int]codes.Code{
http.StatusForbidden: codes.PermissionDenied,
http.StatusBadRequest: codes.InvalidArgument,
http.StatusTooManyRequests: codes.ResourceExhausted,
http.StatusNotFound: codes.NotFound,
}
if code, ok := userErrors[apiErr.Code]; ok {
return &code
}
return nil
}

Expand Down Expand Up @@ -692,17 +692,53 @@ func isContextError(err error) *codes.Code {

// existingErrorCode returns a pointer to the grpc error code for the passed in error.
// Returns nil if the error is nil, or if the error cannot be converted to a grpc status.
// Since github.com/googleapis/gax-go/v2/apierror now wraps googleapi errors (returned from
// GCE API calls), and sets their status error code to Unknown, we now have to make sure we
// only return existing error codes from errors that do not wrap googleAPI errors. Otherwise,
// we will return Unknown for all GCE API calls that return googleapi errors.
func existingErrorCode(err error) *codes.Code {
if err == nil {
return nil
}

if status, ok := status.FromError(err); ok {
return util.ErrCodePtr(status.Code())
// We want to make sure we catch other error types that are statusable.
// (eg. grpc-go/internal/status/status.go Error struct that wraps a status)
var googleErr *googleapi.Error
if !errors.As(err, &googleErr) {
if status, ok := status.FromError(err); ok {
return util.ErrCodePtr(status.Code())
}
}
return nil
}

// isGoogleAPIError returns the gRPC status code for the given googleapi error by mapping
// the googleapi error's HTTP code to the corresponding gRPC error code. If the error is
// wrapped in an APIError (github.com/googleapis/gax-go/v2/apierror), it maps the wrapped
// googleAPI error's HTTP code to the corresponding gRPC error code. Returns an error if
// the given error is not a googleapi error.
func isGoogleAPIError(err error) *codes.Code {
var googleErr *googleapi.Error
if !errors.As(err, &googleErr) {
return nil
}
var sourceCode int
var apiErr *apierror.APIError
if errors.As(err, &apiErr) {
// When googleapi.Err is used as a wrapper, we return the error code of the wrapped contents.
sourceCode = apiErr.HTTPCode()
} else {
// Rely on error code in googleapi.Err when it is our primary error.
sourceCode = googleErr.Code
}
// Map API error code to user error code.
if code, ok := userErrorCodeMap[sourceCode]; ok {
return util.ErrCodePtr(code)
}
// Map API error code to user error code.
return nil
}

// codeForError returns a pointer to the grpc error code that maps to the http
// error code for the passed in user googleapi error or context error. Returns
// codes.Internal if the given error is not a googleapi error caused by the user.
Expand All @@ -721,12 +757,15 @@ func codeForError(err error) *codes.Code {
if errCode := existingErrorCode(err); errCode != nil {
return errCode
}
if errCode := isUserError(err); errCode != nil {
if errCode := isUserOperationError(err); errCode != nil {
return errCode
}
if errCode := isContextError(err); errCode != nil {
return errCode
}
if errCode := isGoogleAPIError(err); errCode != nil {
return errCode
}

return util.ErrCodePtr(codes.Internal)
}
Expand Down
119 changes: 80 additions & 39 deletions pkg/cloud_provider/file/file_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"strings"
"testing"

"github.com/googleapis/gax-go/v2/apierror"
"google.golang.org/api/googleapi"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
Expand Down Expand Up @@ -536,11 +537,72 @@ func TestIsContextError(t *testing.T) {
}

func TestCodeForError(t *testing.T) {
getGoogleAPIWrappedError := func(err error) *googleapi.Error {
apierr, _ := apierror.ParseError(err, false)
wrappedError := &googleapi.Error{}
wrappedError.Wrap(apierr)

return wrappedError
}
getAPIError := func(err error) *apierror.APIError {
apierror, _ := apierror.ParseError(err, true)
return apierror
}
cases := []struct {
name string
err error
expectedErrCode *codes.Code
}{
{
name: "googleapi.Error that wraps apierror.APIError of http kind",
err: getGoogleAPIWrappedError(&googleapi.Error{
Code: 404,
Message: "data requested not found error",
}),
expectedErrCode: util.ErrCodePtr(codes.NotFound),
},
{
name: "googleapi.Error that wraps apierror.APIError of status kind",
err: getGoogleAPIWrappedError(status.New(
codes.Internal, "Internal status error",
).Err()),
expectedErrCode: util.ErrCodePtr(codes.Internal),
},
{
name: "apierror.APIError of status kind",
err: getAPIError(status.New(
codes.Canceled, "Internal status error",
).Err()),
expectedErrCode: util.ErrCodePtr(codes.Canceled),
},
{
name: "apierror.APIError of http kind",
err: getAPIError(&googleapi.Error{
Code: 404,
Message: "data requested not found error",
}),
expectedErrCode: util.ErrCodePtr(codes.NotFound),
},
{
name: "apierror.APIError wrapped 429 http error",
err: fmt.Errorf("got error: %w", &googleapi.Error{Code: http.StatusTooManyRequests}),
expectedErrCode: util.ErrCodePtr(codes.ResourceExhausted),
},
{
name: "apierror.APIError wrapped 400 http error",
err: fmt.Errorf("got error: %w", &googleapi.Error{Code: http.StatusBadRequest}),
expectedErrCode: util.ErrCodePtr(codes.InvalidArgument),
},
{
name: "apierror.APIError wrapped 403 http error",
err: fmt.Errorf("got error: %w", &googleapi.Error{Code: http.StatusForbidden}),
expectedErrCode: util.ErrCodePtr(codes.PermissionDenied),
},
{
name: "RESOURCE_EXHAUSTED error",
err: fmt.Errorf("got error: RESOURCE_EXHAUSTED: Operation rate exceeded"),
expectedErrCode: util.ErrCodePtr(codes.ResourceExhausted),
},
{
name: "deadline exceeded error",
err: context.DeadlineExceeded,
Expand Down Expand Up @@ -589,13 +651,15 @@ func TestCodeForError(t *testing.T) {
}

for _, test := range cases {
errCode := codeForError(test.err)
if (test.expectedErrCode == nil) != (errCode == nil) {
t.Errorf("test %v failed: got %v, expected %v", test.name, errCode, test.expectedErrCode)
}
if test.expectedErrCode != nil && *errCode != *test.expectedErrCode {
t.Errorf("test %v failed: got %v, expected %v", test.name, errCode, test.expectedErrCode)
}
t.Run(test.name, func(t *testing.T) {
errCode := codeForError(test.err)
if (test.expectedErrCode == nil) != (errCode == nil) {
t.Errorf("test %v failed: got %v, expected %v", test.name, errCode, test.expectedErrCode)
}
if test.expectedErrCode != nil && *errCode != *test.expectedErrCode {
t.Errorf("test %v failed: got %v, expected %v", test.name, errCode, test.expectedErrCode)
}
})
}
}

Expand Down Expand Up @@ -636,31 +700,6 @@ func TestIsUserError(t *testing.T) {
err: nil,
expectedErrCode: nil,
},
{
name: "404 http error",
err: &googleapi.Error{Code: http.StatusNotFound},
expectedErrCode: util.ErrCodePtr(codes.NotFound),
},
{
name: "wrapped 404 http error",
err: fmt.Errorf("got error: %w", &googleapi.Error{Code: http.StatusNotFound}),
expectedErrCode: util.ErrCodePtr(codes.NotFound),
},
{
name: "wrapped 429 http error",
err: fmt.Errorf("got error: %w", &googleapi.Error{Code: http.StatusTooManyRequests}),
expectedErrCode: util.ErrCodePtr(codes.ResourceExhausted),
},
{
name: "wrapped 400 http error",
err: fmt.Errorf("got error: %w", &googleapi.Error{Code: http.StatusBadRequest}),
expectedErrCode: util.ErrCodePtr(codes.InvalidArgument),
},
{
name: "wrapped 403 http error",
err: fmt.Errorf("got error: %w", &googleapi.Error{Code: http.StatusForbidden}),
expectedErrCode: util.ErrCodePtr(codes.PermissionDenied),
},
{
name: "RESOURCE_EXHAUSTED error",
err: fmt.Errorf("got error: RESOURCE_EXHAUSTED: Operation rate exceeded"),
Expand All @@ -684,12 +723,14 @@ func TestIsUserError(t *testing.T) {
}

for _, test := range cases {
errCode := isUserError(test.err)
if (test.expectedErrCode == nil) != (errCode == nil) {
t.Errorf("test %v failed: got %v, expected %v", test.name, errCode, test.expectedErrCode)
}
if test.expectedErrCode != nil && *errCode != *test.expectedErrCode {
t.Errorf("test %v failed: got %v, expected %v", test.name, errCode, test.expectedErrCode)
}
t.Run(test.name, func(t *testing.T) {
errCode := isUserOperationError(test.err)
if (test.expectedErrCode == nil) != (errCode == nil) {
t.Errorf("test %v failed: got %v, expected %v", test.name, errCode, test.expectedErrCode)
}
if test.expectedErrCode != nil && *errCode != *test.expectedErrCode {
t.Errorf("test %v failed: got %v, expected %v", test.name, errCode, test.expectedErrCode)
}
})
}
}

0 comments on commit 71b89ba

Please sign in to comment.