Merge pull request #1708 from hime/master

Properly unwrap gce-compute error code.

Author: k8s-ci-robot

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())

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"},

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) {