diff --git a/pkg/cloud_provider/file/file.go b/pkg/cloud_provider/file/file.go index d14879296..5d5de1334 100644 --- a/pkg/cloud_provider/file/file.go +++ b/pkg/cloud_provider/file/file.go @@ -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" @@ -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() @@ -631,7 +640,7 @@ 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) { @@ -639,15 +648,6 @@ func isUserError(err error) *codes.Code { 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 } @@ -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. @@ -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) } diff --git a/pkg/cloud_provider/file/file_test.go b/pkg/cloud_provider/file/file_test.go index 914e7fb66..402150385 100644 --- a/pkg/cloud_provider/file/file_test.go +++ b/pkg/cloud_provider/file/file_test.go @@ -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" @@ -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, @@ -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) + } + }) } } @@ -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"), @@ -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) + } + }) } }