Merge pull request #1840 from hime/automated-cherry-pick-of-#1708-upstream-release-1.12

Automated cherry pick of #1708: Properly unwrap gce-compute error code.

Author: k8s-ci-robot

pkg/common/errors.go

@@ -0,0 +1,55 @@
+/*
+Copyright 2024 The Kubernetes Authors.
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+    http://www.apache.org/licenses/LICENSE-2.0
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package common
+
+import (
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
+)
+
+// TemporaryError wraps an error with a temporary error code.
+// It implements the error interface. Do not return TemporaryError
+// directly from CSI Spec API calls, as CSI Spec API calls MUST
+// return a standard gRPC status. If TemporaryErrors are returned from
+// helper functions within a CSI Spec API method, make sure the outer CSI
+// Spec API method returns a standard gRPC status. (e.g. LoggedError(tempErr) )
+type TemporaryError struct {
+	err  error
+	code codes.Code
+}
+
+// Unwrap extracts the original error.
+func (te *TemporaryError) Unwrap() error {
+	return te.err
+}
+
+// GRPCStatus extracts the underlying gRPC Status error.
+// This method is necessary to fulfill the grpcstatus interface
+// described in https://pkg.go.dev/google.golang.org/grpc/status#FromError.
+// FromError is used in CodeForError to get existing error codes from status errors.
+func (te *TemporaryError) GRPCStatus() *status.Status {
+	if te.err == nil {
+		return status.New(codes.OK, "")
+	}
+	return status.New(te.code, te.err.Error())
+}
+
+func NewTemporaryError(code codes.Code, err error) *TemporaryError {
+	return &TemporaryError{err: err, code: code}
+}
+
+// Error returns a readable representation of the TemporaryError.
+func (te *TemporaryError) Error() string {
+	return te.err.Error()
+}

pkg/common/utils.go

@@ -25,6 +25,7 @@ import (
 	"strings"
 
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
+	"github.com/googleapis/gax-go/v2/apierror"
 	"google.golang.org/api/googleapi"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
@@ -334,7 +335,6 @@ func CodeForError(sourceError error) codes.Code {
 	if sourceError == nil {
 		return codes.Internal
 	}
-
 	if code, err := isUserMultiAttachError(sourceError); err == nil {
 		return code
 	}
@@ -347,12 +347,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
 	}
 
@@ -403,16 +398,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

@@ -26,6 +26,7 @@ import (
 	"testing"
 
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
+	"github.com/googleapis/gax-go/v2/apierror"
 	"google.golang.org/api/googleapi"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
@@ -976,6 +977,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
@@ -991,6 +1003,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"
 	extremeDiskType                         = "pd-extreme"
@@ -268,6 +269,33 @@ var _ = Describe("GCE PD CSI Driver", func() {
 		}
 	})
 
+	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) {
 			testContext := getRandomTestContext()