Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions pkg/common/errors.go
Original file line number Diff line number Diff line change
@@ -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()
}
61 changes: 52 additions & 9 deletions pkg/common/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
Expand All @@ -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
}

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