From 384acf8c7811e0ca47b87f01c3e28c679c66762e Mon Sep 17 00:00:00 2001 From: Jaime Bohorquez Date: Tue, 28 May 2024 22:25:12 +0000 Subject: [PATCH] Properly unwrap gce-compute error code. --- pkg/common/utils.go | 61 ++++++++++++++++++++++---- pkg/common/utils_test.go | 42 ++++++++++++++++++ test/e2e/tests/single_zone_e2e_test.go | 28 ++++++++++++ 3 files changed, 122 insertions(+), 9 deletions(-) diff --git a/pkg/common/utils.go b/pkg/common/utils.go index 17a75bd96..25f8a6552 100644 --- a/pkg/common/utils.go +++ b/pkg/common/utils.go @@ -26,6 +26,7 @@ import ( "time" "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta" + "github.com/googleapis/gax-go/v2/apierror" "golang.org/x/time/rate" "google.golang.org/api/googleapi" "google.golang.org/grpc/codes" @@ -423,7 +424,6 @@ func CodeForError(sourceError error) codes.Code { if sourceError == nil { return codes.Internal } - if code, err := isUserMultiAttachError(sourceError); err == nil { return code } @@ -436,12 +436,7 @@ func CodeForError(sourceError error) codes.Code { if code, err := isConnectionResetError(sourceError); err == nil { return code } - - var apiErr *googleapi.Error - if !errors.As(sourceError, &apiErr) { - return codes.Internal - } - if code, ok := userErrorCodeMap[apiErr.Code]; ok { + if code, err := isGoogleAPIError(sourceError); err == nil { return code } @@ -492,16 +487,64 @@ func isUserMultiAttachError(err error) (codes.Code, error) { return codes.Unknown, fmt.Errorf("Not a user multiattach error: %w", err) } +// existingErrorCode returns the existing gRPC Status error code for the given error, if one exists, +// or an error if one doesn't exist. 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 are either TemporaryErrors, or 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, error) { if err == nil { return codes.Unknown, fmt.Errorf("null error") } - if status, ok := status.FromError(err); ok { - return status.Code(), nil + var tmpError *TemporaryError + // This explicitly checks our error is a temporary error before extracting its + // status, as there can be other errors that can qualify as statusable + // while not necessarily being temporary. + if errors.As(err, &tmpError) { + if status, ok := status.FromError(err); ok { + return status.Code(), nil + } + } + // 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 status.Code(), nil + } } + return codes.Unknown, fmt.Errorf("no existing error code for %w", err) } +// 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, error) { + var googleErr *googleapi.Error + if !errors.As(err, &googleErr) { + return codes.Unknown, fmt.Errorf("error %w is not a googleapi.Error", err) + } + 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 code, nil + } + + return codes.Unknown, fmt.Errorf("googleapi.Error %w does not map to any known errors", err) +} + func LoggedError(msg string, err error) error { klog.Errorf(msg+"%v", err.Error()) return status.Errorf(CodeForError(err), msg+"%v", err.Error()) diff --git a/pkg/common/utils_test.go b/pkg/common/utils_test.go index 0776202d0..e0c14f295 100644 --- a/pkg/common/utils_test.go +++ b/pkg/common/utils_test.go @@ -27,6 +27,7 @@ import ( "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta" "github.com/google/go-cmp/cmp" + "github.com/googleapis/gax-go/v2/apierror" "google.golang.org/api/googleapi" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" @@ -1206,6 +1207,17 @@ func TestParseMachineType(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 + } testCases := []struct { name string inputErr error @@ -1221,6 +1233,36 @@ func TestCodeForError(t *testing.T) { inputErr: &googleapi.Error{Code: http.StatusBadRequest, Message: "User error with bad request"}, expCode: codes.InvalidArgument, }, + { + name: "googleapi.Error that wraps apierror.APIError of http kind", + inputErr: getGoogleAPIWrappedError(&googleapi.Error{ + Code: 404, + Message: "data requested not found error", + }), + expCode: codes.NotFound, + }, + { + name: "googleapi.Error that wraps apierror.APIError of status kind", + inputErr: getGoogleAPIWrappedError(status.New( + codes.Internal, "Internal status error", + ).Err()), + expCode: codes.Internal, + }, + { + name: "apierror.APIError of http kind", + inputErr: getAPIError(&googleapi.Error{ + Code: 404, + Message: "data requested not found error", + }), + expCode: codes.NotFound, + }, + { + name: "apierror.APIError of status kind", + inputErr: getAPIError(status.New( + codes.Canceled, "Internal status error", + ).Err()), + expCode: codes.Canceled, + }, { name: "googleapi.Error but not a user error", inputErr: &googleapi.Error{Code: http.StatusInternalServerError, Message: "Internal error"}, diff --git a/test/e2e/tests/single_zone_e2e_test.go b/test/e2e/tests/single_zone_e2e_test.go index 6b3cfea74..99d30f3d8 100644 --- a/test/e2e/tests/single_zone_e2e_test.go +++ b/test/e2e/tests/single_zone_e2e_test.go @@ -50,6 +50,7 @@ const ( defaultRepdSizeGb int64 = 200 defaultMwSizeGb int64 = 200 defaultVolumeLimit int64 = 127 + invalidSizeGb int64 = 66000 readyState = "READY" standardDiskType = "pd-standard" ssdDiskType = "pd-ssd" @@ -268,6 +269,33 @@ var _ = Describe("GCE PD CSI Driver", func() { Expect(err).To(BeNil(), "Could not find disk in correct zone") } }) + // TODO(hime): Enable this test once all release branches contain the fix from PR#1708. + // It("Should return InvalidArgument when disk size exceeds limit", func() { + // // If this returns a different error code (like Unknown), the error wrapping logic in #1708 has regressed. + // Expect(testContexts).ToNot(BeEmpty()) + // testContext := getRandomTestContext() + + // zones := []string{"us-central1-c", "us-central1-b", "us-central1-a"} + + // for _, zone := range zones { + // volName := testNamePrefix + string(uuid.NewUUID()) + // topReq := &csi.TopologyRequirement{ + // Requisite: []*csi.Topology{ + // { + // Segments: map[string]string{common.TopologyKeyZone: zone}, + // }, + // }, + // } + // volume, err := testContext.Client.CreateVolume(volName, nil, invalidSizeGb, topReq, nil) + // Expect(err).ToNot(BeNil(), "Failed to fetch error from create volume.") + // Expect(err.Error()).To(ContainSubstring("InvalidArgument"), "Failed to verify error code matches InvalidArgument.") + // defer func() { + // if volume != nil { + // testContext.Client.DeleteVolume(volume.VolumeId) + // } + // }() + // } + // }) DescribeTable("Should complete entire disk lifecycle with underspecified volume ID", func(diskType string) {