Merge pull request #1244 from sunnylovestiramisu/automated-cherry-pick-of-#1150-#1232-#1227-upstream-release-1.8

Automated cherry pick of #1150: fix bug where volume cloning topology requirements are
#1232: Use errors.As so we can detect wrapped errors, and check for
#1227: Adding new metric pdcsi_operation_errors to fetch error

Author: k8s-ci-robot

cmd/gce-pd-csi-driver/main.go

@@ -94,10 +94,14 @@ func handle() {
 	}
 	klog.V(2).Infof("Driver vendor version %v", version)
 
-	if *runControllerService && *httpEndpoint != "" && metrics.IsGKEComponentVersionAvailable() {
+	if *runControllerService && *httpEndpoint != "" {
 		mm := metrics.NewMetricsManager()
 		mm.InitializeHttpHandler(*httpEndpoint, *metricsPath)
-		mm.EmitGKEComponentVersion()
+		mm.RegisterPDCSIMetric()
+
+		if metrics.IsGKEComponentVersionAvailable() {
+			mm.EmitGKEComponentVersion()
+		}
 	}
 
 	if len(*extraVolumeLabelsStr) > 0 && !*runControllerService {

pkg/common/utils.go

@@ -17,14 +17,21 @@ limitations under the License.
 package common
 
 import (
+	"context"
+	"errors"
 	"fmt"
+	"net/http"
 	"regexp"
 	"strings"
 
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
+	"google.golang.org/api/googleapi"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 	"k8s.io/apimachinery/pkg/api/resource"
 	"k8s.io/apimachinery/pkg/util/sets"
 	volumehelpers "k8s.io/cloud-provider/volume/helpers"
+	"k8s.io/klog/v2"
 )
 
 const (
@@ -259,18 +266,6 @@ func ValidateSnapshotType(snapshotType string) error {
 	}
 }
 
-// ParseMachineType returns an extracted machineType from a URL, or empty if not found.
-// machineTypeUrl: Full or partial URL of the machine type resource, in the format:
-//
-//	zones/zone/machineTypes/machine-type
-func ParseMachineType(machineTypeUrl string) (string, error) {
-	machineType := machineTypeRegex.FindStringSubmatch(machineTypeUrl)
-	if machineType == nil {
-		return "", fmt.Errorf("failed to parse machineTypeUrl. Expected suffix: zones/{zone}/machineTypes/{machine-type}. Got: %s", machineTypeUrl)
-	}
-	return machineType[1], nil
-}
-
 // ConvertStringToInt64 converts a string to int64
 func ConvertStringToInt64(str string) (int64, error) {
 	quantity, err := resource.ParseQuantity(str)
@@ -288,3 +283,95 @@ func ConvertMiStringToInt64(str string) (int64, error) {
 	}
 	return volumehelpers.RoundUpToMiB(quantity)
 }
+
+// ParseMachineType returns an extracted machineType from a URL, or empty if not found.
+// machineTypeUrl: Full or partial URL of the machine type resource, in the format:
+//
+//	zones/zone/machineTypes/machine-type
+func ParseMachineType(machineTypeUrl string) (string, error) {
+	machineType := machineTypeRegex.FindStringSubmatch(machineTypeUrl)
+	if machineType == nil {
+		return "", fmt.Errorf("failed to parse machineTypeUrl. Expected suffix: zones/{zone}/machineTypes/{machine-type}. Got: %s", machineTypeUrl)
+	}
+	return machineType[1], nil
+}
+
+// CodeForError returns a pointer to the grpc error code that maps to the http
+// error code for the passed in user googleapi error or context error. Returns
+// codes.Internal if the given error is not a googleapi error caused by the user.
+// The following http error codes are considered user errors:
+// (1) http 400 Bad Request, returns grpc InvalidArgument,
+// (2) http 403 Forbidden, returns grpc PermissionDenied,
+// (3) http 404 Not Found, returns grpc NotFound
+// (4) http 429 Too Many Requests, returns grpc ResourceExhausted
+// The following errors are considered context errors:
+// (1) "context deadline exceeded", returns grpc DeadlineExceeded,
+// (2) "context canceled", returns grpc Canceled
+func CodeForError(err error) *codes.Code {
+	if err == nil {
+		return nil
+	}
+
+	if errCode := existingErrorCode(err); errCode != nil {
+		return errCode
+	}
+	if code := isContextError(err); code != nil {
+		return code
+	}
+
+	internalErrorCode := codes.Internal
+	// Upwrap the error
+	var apiErr *googleapi.Error
+	if !errors.As(err, &apiErr) {
+		return &internalErrorCode
+	}
+
+	userErrors := map[int]codes.Code{
+		http.StatusForbidden:       codes.PermissionDenied,
+		http.StatusBadRequest:      codes.InvalidArgument,
+		http.StatusTooManyRequests: codes.ResourceExhausted,
+		http.StatusNotFound:        codes.NotFound,
+	}
+	if code, ok := userErrors[apiErr.Code]; ok {
+		return &code
+	}
+
+	return &internalErrorCode
+}
+
+// isContextError returns a pointer to the grpc error code DeadlineExceeded
+// if the passed in error contains the "context deadline exceeded" string and returns
+// the grpc error code Canceled if the error contains the "context canceled" string.
+func isContextError(err error) *codes.Code {
+	if err == nil {
+		return nil
+	}
+
+	errStr := err.Error()
+	if strings.Contains(errStr, context.DeadlineExceeded.Error()) {
+		return errCodePtr(codes.DeadlineExceeded)
+	}
+	if strings.Contains(errStr, context.Canceled.Error()) {
+		return errCodePtr(codes.Canceled)
+	}
+	return nil
+}
+
+func existingErrorCode(err error) *codes.Code {
+	if err == nil {
+		return nil
+	}
+	if status, ok := status.FromError(err); ok {
+		return errCodePtr(status.Code())
+	}
+	return nil
+}
+
+func errCodePtr(code codes.Code) *codes.Code {
+	return &code
+}
+
+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

@@ -17,11 +17,17 @@ limitations under the License.
 package common
 
 import (
+	"context"
+	"errors"
 	"fmt"
+	"net/http"
 	"reflect"
 	"testing"
 
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
+	"google.golang.org/api/googleapi"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 )
 
 const (
@@ -578,60 +584,6 @@ func TestSnapshotStorageLocations(t *testing.T) {
 	}
 }
 
-func TestParseMachineType(t *testing.T) {
-	tests := []struct {
-		desc                string
-		inputMachineTypeUrl string
-		expectedMachineType string
-		expectError         bool
-	}{
-		{
-			desc:                "full URL machine type",
-			inputMachineTypeUrl: "https://www.googleapis.com/compute/v1/projects/my-project/zones/us-central1-c/machineTypes/c3-highcpu-4",
-			expectedMachineType: "c3-highcpu-4",
-		},
-		{
-			desc:                "partial URL machine type",
-			inputMachineTypeUrl: "zones/us-central1-c/machineTypes/n2-standard-4",
-			expectedMachineType: "n2-standard-4",
-		},
-		{
-			desc:                "custom partial URL machine type",
-			inputMachineTypeUrl: "zones/us-central1-c/machineTypes/e2-custom-2-4096",
-			expectedMachineType: "e2-custom-2-4096",
-		},
-		{
-			desc:                "incorrect URL",
-			inputMachineTypeUrl: "https://www.googleapis.com/compute/v1/projects/psch-gke-dev/zones/us-central1-c",
-			expectError:         true,
-		},
-		{
-			desc:                "incorrect partial URL",
-			inputMachineTypeUrl: "zones/us-central1-c/machineTypes/",
-			expectError:         true,
-		},
-		{
-			desc:                "missing zone",
-			inputMachineTypeUrl: "zones//machineTypes/n2-standard-4",
-			expectError:         true,
-		},
-	}
-	for _, tc := range tests {
-		t.Run(tc.desc, func(t *testing.T) {
-			actualMachineFamily, err := ParseMachineType(tc.inputMachineTypeUrl)
-			if err != nil && !tc.expectError {
-				t.Errorf("Got error %v parsing machine type %s; expect no error", err, tc.inputMachineTypeUrl)
-			}
-			if err == nil && tc.expectError {
-				t.Errorf("Got no error parsing machine type %s; expect an error", tc.inputMachineTypeUrl)
-			}
-			if err == nil && actualMachineFamily != tc.expectedMachineType {
-				t.Errorf("Got %s parsing machine type; expect %s", actualMachineFamily, tc.expectedMachineType)
-			}
-		})
-	}
-}
-
 func TestConvertStringToInt64(t *testing.T) {
 	tests := []struct {
 		desc        string
@@ -853,3 +805,163 @@ func TestConvertMiStringToInt64(t *testing.T) {
 		})
 	}
 }
+
+func TestParseMachineType(t *testing.T) {
+	tests := []struct {
+		desc                string
+		inputMachineTypeUrl string
+		expectedMachineType string
+		expectError         bool
+	}{
+		{
+			desc:                "full URL machine type",
+			inputMachineTypeUrl: "https://www.googleapis.com/compute/v1/projects/my-project/zones/us-central1-c/machineTypes/c3-highcpu-4",
+			expectedMachineType: "c3-highcpu-4",
+		},
+		{
+			desc:                "partial URL machine type",
+			inputMachineTypeUrl: "zones/us-central1-c/machineTypes/n2-standard-4",
+			expectedMachineType: "n2-standard-4",
+		},
+		{
+			desc:                "custom partial URL machine type",
+			inputMachineTypeUrl: "zones/us-central1-c/machineTypes/e2-custom-2-4096",
+			expectedMachineType: "e2-custom-2-4096",
+		},
+		{
+			desc:                "incorrect URL",
+			inputMachineTypeUrl: "https://www.googleapis.com/compute/v1/projects/psch-gke-dev/zones/us-central1-c",
+			expectError:         true,
+		},
+		{
+			desc:                "incorrect partial URL",
+			inputMachineTypeUrl: "zones/us-central1-c/machineTypes/",
+			expectError:         true,
+		},
+		{
+			desc:                "missing zone",
+			inputMachineTypeUrl: "zones//machineTypes/n2-standard-4",
+			expectError:         true,
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.desc, func(t *testing.T) {
+			actualMachineFamily, err := ParseMachineType(tc.inputMachineTypeUrl)
+			if err != nil && !tc.expectError {
+				t.Errorf("Got error %v parsing machine type %s; expect no error", err, tc.inputMachineTypeUrl)
+			}
+			if err == nil && tc.expectError {
+				t.Errorf("Got no error parsing machine type %s; expect an error", tc.inputMachineTypeUrl)
+			}
+			if err == nil && actualMachineFamily != tc.expectedMachineType {
+				t.Errorf("Got %s parsing machine type; expect %s", actualMachineFamily, tc.expectedMachineType)
+			}
+		})
+	}
+}
+
+func TestCodeForError(t *testing.T) {
+	internalErrorCode := codes.Internal
+	userErrorCode := codes.InvalidArgument
+	testCases := []struct {
+		name     string
+		inputErr error
+		expCode  *codes.Code
+	}{
+		{
+			name:     "Not googleapi.Error",
+			inputErr: errors.New("I am not a googleapi.Error"),
+			expCode:  &internalErrorCode,
+		},
+		{
+			name:     "User error",
+			inputErr: &googleapi.Error{Code: http.StatusBadRequest, Message: "User error with bad request"},
+			expCode:  &userErrorCode,
+		},
+		{
+			name:     "googleapi.Error but not a user error",
+			inputErr: &googleapi.Error{Code: http.StatusInternalServerError, Message: "Internal error"},
+			expCode:  &internalErrorCode,
+		},
+		{
+			name:     "context canceled error",
+			inputErr: context.Canceled,
+			expCode:  errCodePtr(codes.Canceled),
+		},
+		{
+			name:     "context deadline exceeded error",
+			inputErr: context.DeadlineExceeded,
+			expCode:  errCodePtr(codes.DeadlineExceeded),
+		},
+		{
+			name:     "status error with Aborted error code",
+			inputErr: status.Error(codes.Aborted, "aborted error"),
+			expCode:  errCodePtr(codes.Aborted),
+		},
+		{
+			name:     "nil error",
+			inputErr: nil,
+			expCode:  nil,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Logf("Running test: %v", tc.name)
+		errCode := CodeForError(tc.inputErr)
+		if (tc.expCode == nil) != (errCode == nil) {
+			t.Errorf("test %v failed: got %v, expected %v", tc.name, errCode, tc.expCode)
+		}
+		if tc.expCode != nil && *errCode != *tc.expCode {
+			t.Errorf("test %v failed: got %v, expected %v", tc.name, errCode, tc.expCode)
+		}
+	}
+}
+
+func TestIsContextError(t *testing.T) {
+	cases := []struct {
+		name            string
+		err             error
+		expectedErrCode *codes.Code
+	}{
+		{
+			name:            "deadline exceeded error",
+			err:             context.DeadlineExceeded,
+			expectedErrCode: errCodePtr(codes.DeadlineExceeded),
+		},
+		{
+			name:            "contains 'context deadline exceeded'",
+			err:             fmt.Errorf("got error: %w", context.DeadlineExceeded),
+			expectedErrCode: errCodePtr(codes.DeadlineExceeded),
+		},
+		{
+			name:            "context canceled error",
+			err:             context.Canceled,
+			expectedErrCode: errCodePtr(codes.Canceled),
+		},
+		{
+			name:            "contains 'context canceled'",
+			err:             fmt.Errorf("got error: %w", context.Canceled),
+			expectedErrCode: errCodePtr(codes.Canceled),
+		},
+		{
+			name:            "does not contain 'context canceled' or 'context deadline exceeded'",
+			err:             fmt.Errorf("unknown error"),
+			expectedErrCode: nil,
+		},
+		{
+			name:            "nil error",
+			err:             nil,
+			expectedErrCode: nil,
+		},
+	}
+
+	for _, test := range cases {
+		errCode := isContextError(test.err)
+		if (test.expectedErrCode == nil) != (errCode == nil) {
+			t.Errorf("test %v failed: got %v, expected %v", test.name, errCode, test.expectedErrCode)
+		}
+		if test.expectedErrCode != nil && *errCode != *test.expectedErrCode {
+			t.Errorf("test %v failed: got %v, expected %v", test.name, errCode, test.expectedErrCode)
+		}
+	}
+}