Skip to content

Commit

Permalink
Properly unwrap gce-compute error code.
Browse files Browse the repository at this point in the history
  • Loading branch information
@hime
hime committed May 28, 2024
1 parent 82c6deb commit 9e6aa5a
Show file tree
Hide file tree
Showing 3 changed files with 122 additions and 9 deletions.
61 changes: 52 additions & 9 deletions pkg/common/utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
Expand All @@ -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
}

Expand Down Expand Up @@ -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())
Expand Down
42 changes: 42 additions & 0 deletions pkg/common/utils_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand All @@ -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"},
Expand Down
28 changes: 28 additions & 0 deletions test/e2e/tests/single_zone_e2e_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 this fix.
// 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) {
Expand Down

0 comments on commit 9e6aa5a

Please sign in to comment.