Repository: kubernetes-sigs/gcp-compute-persistent-disk-csi-driver
Pull request #1308 (2023-07-21 22:16:37)

Cherry-pick #1304 to release-1.9
Cherry-pick #1304 to release-1.9

/kind bug


```release-note
Fix resource parsing when the gcp project name ends with alpha, beta or v1
```



Patch:
diff --git a/pkg/gce-cloud-provider/compute/fake-gce.go b/pkg/gce-cloud-provider/compute/fake-gce.go
index e6731140..c0a7fa38 100644
--- a/pkg/gce-cloud-provider/compute/fake-gce.go
+++ b/pkg/gce-cloud-provider/compute/fake-gce.go
@@ -229,10 +229,10 @@ func (cloud *FakeCloudProvider) InsertDisk(ctx context.Context, project string,
 	switch volKey.Type() {
 	case meta.Zonal:
 		computeDisk.Zone = volKey.Zone
-		computeDisk.SelfLink = fmt.Sprintf("projects/%s/zones/%s/disks/%s", project, volKey.Zone, volKey.Name)
+		computeDisk.SelfLink = fmt.Sprintf("%sprojects/%s/zones/%s/disks/%s", BasePath, project, volKey.Zone, volKey.Name)
 	case meta.Regional:
 		computeDisk.Region = volKey.Region
-		computeDisk.SelfLink = fmt.Sprintf("projects/%s/regions/%s/disks/%s", project, volKey.Region, volKey.Name)
+		computeDisk.SelfLink = fmt.Sprintf("%sprojects/%s/regions/%s/disks/%s", BasePath, project, volKey.Region, volKey.Name)
 	default:
 		return fmt.Errorf("could not create disk, key was neither zonal nor regional, instead got: %v", volKey.String())
 	}
diff --git a/pkg/gce-pd-csi-driver/controller.go b/pkg/gce-pd-csi-driver/controller.go
index 9c682ebb..fba3a8a9 100644
--- a/pkg/gce-pd-csi-driver/controller.go
+++ b/pkg/gce-pd-csi-driver/controller.go
@@ -19,7 +19,7 @@ import (
 	"errors"
 	"fmt"
 	"math/rand"
-	"regexp"
+	neturl "net/url"
 	"sort"
 	"strings"
 	"time"
@@ -130,6 +130,17 @@ const (
 	// but 500 is a good proxy (gives ~8KB of data per ListVolumesResponse#Entry)
 	// See https://github.com/grpc/grpc/blob/master/include/grpc/impl/codegen/grpc_types.h#L503)
 	maxListVolumesResponseEntries = 500
+
+	// Keys in the volume context.
+	contextForceAttach = "force-attach"
+
+	resourceApiScheme  = "https"
+	resourceApiService = "compute"
+	resourceProject    = "projects"
+)
+
+var (
+	validResourceApiVersions = map[string]bool{"v1": true, "alpha": true, "beta": true}
 )
 
 func isDiskReady(disk *gce.CloudDisk) (bool, error) {
@@ -306,7 +317,7 @@ func (gceCS *GCEControllerServer) CreateVolume(ctx context.Context, req *csi.Cre
 
 		// If there is no validation error, immediately return success
 		klog.V(4).Infof("CreateVolume succeeded for disk %v, it already exists and was compatible", volKey)
-		return generateCreateVolumeResponse(existingDisk, zones), nil
+		return generateCreateVolumeResponse(existingDisk, zones)
 	}
 
 	snapshotID := ""
@@ -421,7 +432,7 @@ func (gceCS *GCEControllerServer) CreateVolume(ctx context.Context, req *csi.Cre
 	}
 
 	klog.V(4).Infof("CreateVolume succeeded for disk %v", volKey)
-	return generateCreateVolumeResponse(disk, zones), nil
+	return generateCreateVolumeResponse(disk, zones)
 
 }
 
@@ -856,13 +867,23 @@ func (gceCS *GCEControllerServer) ListVolumes(ctx context.Context, req *csi.List
 	entries := []*csi.ListVolumesResponse_Entry{}
 	for i := 0; i+offset < len(gceCS.disks) && i < maxEntries; i++ {
 		d := gceCS.disks[i+offset]
+		diskRsrc, err := getResourceId(d.SelfLink)
+		if err != nil {
+			klog.Warningf("Bad ListVolumes disk resource %s, skipped: %v (%+v)", d.SelfLink, err, d)
+			continue
+		}
 		users := []string{}
 		for _, u := range d.Users {
-			users = append(users, cleanSelfLink(u))
+			rsrc, err := getResourceId(u)
+			if err != nil {
+				klog.Warningf("Bad ListVolumes user %s, skipped: %v", u, err)
+			} else {
+				users = append(users, rsrc)
+			}
 		}
 		entries = append(entries, &csi.ListVolumesResponse_Entry{
 			Volume: &csi.Volume{
-				VolumeId: cleanSelfLink(d.SelfLink),
+				VolumeId: diskRsrc,
 			},
 			Status: &csi.ListVolumesResponse_VolumeStatus{
 				PublishedNodeIds: users,
@@ -975,6 +996,10 @@ func (gceCS *GCEControllerServer) createPDSnapshot(ctx context.Context, project
 			return nil, common.LoggedError("Failed to create snapshot: ", err)
 		}
 	}
+	snapshotId, err := getResourceId(snapshot.SelfLink)
+	if err != nil {
+		return nil, common.LoggedError(fmt.Sprintf("Cannot extract resource id from snapshot %s", snapshot.SelfLink), err)
+	}
 
 	err = gceCS.validateExistingSnapshot(snapshot, volKey)
 	if err != nil {
@@ -993,7 +1018,7 @@ func (gceCS *GCEControllerServer) createPDSnapshot(ctx context.Context, project
 
 	return &csi.Snapshot{
 		SizeBytes:      common.GbToBytes(snapshot.DiskSizeGb),
-		SnapshotId:     cleanSelfLink(snapshot.SelfLink),
+		SnapshotId:     snapshotId,
 		SourceVolumeId: volumeID,
 		CreationTime:   timestamp,
 		ReadyToUse:     ready,
@@ -1022,6 +1047,10 @@ func (gceCS *GCEControllerServer) createImage(ctx context.Context, project strin
 			return nil, common.LoggedError("Failed to create image: ", err)
 		}
 	}
+	imageId, err := getResourceId(image.SelfLink)
+	if err != nil {
+		return nil, common.LoggedError(fmt.Sprintf("Cannot extract resource id from snapshot %s", image.SelfLink), err)
+	}
 
 	err = gceCS.validateExistingImage(image, volKey)
 	if err != nil {
@@ -1040,7 +1069,7 @@ func (gceCS *GCEControllerServer) createImage(ctx context.Context, project strin
 
 	return &csi.Snapshot{
 		SizeBytes:      common.GbToBytes(image.DiskSizeGb),
-		SnapshotId:     cleanSelfLink(image.SelfLink),
+		SnapshotId:     imageId,
 		SourceVolumeId: volumeID,
 		CreationTime:   timestamp,
 		ReadyToUse:     ready,
@@ -1052,9 +1081,13 @@ func (gceCS *GCEControllerServer) validateExistingImage(image *compute.Image, vo
 		return fmt.Errorf("disk does not exist")
 	}
 
-	_, sourceKey, err := common.VolumeIDToKey(cleanSelfLink(image.SourceDisk))
+	sourceId, err := getResourceId(image.SourceDisk)
 	if err != nil {
-		return fmt.Errorf("fail to get source disk key %s, %w", image.SourceDisk, err)
+		return fmt.Errorf("failed to get source id from %s: %w", image.SourceDisk, err)
+	}
+	_, sourceKey, err := common.VolumeIDToKey(sourceId)
+	if err != nil {
+		return fmt.Errorf("failed to get source disk key %s: %w", image.SourceDisk, err)
 	}
 
 	if sourceKey.String() != volKey.String() {
@@ -1103,7 +1136,11 @@ func (gceCS *GCEControllerServer) validateExistingSnapshot(snapshot *compute.Sna
 		return fmt.Errorf("disk does not exist")
 	}
 
-	_, sourceKey, err := common.VolumeIDToKey(cleanSelfLink(snapshot.SourceDisk))
+	sourceId, err := getResourceId(snapshot.SourceDisk)
+	if err != nil {
+		return fmt.Errorf("failed to get source id from %s: %w", snapshot.SourceDisk, err)
+	}
+	_, sourceKey, err := common.VolumeIDToKey(sourceId)
 	if err != nil {
 		return fmt.Errorf("fail to get source disk key %s, %w", snapshot.SourceDisk, err)
 	}
@@ -1146,7 +1183,7 @@ func (gceCS *GCEControllerServer) DeleteSnapshot(ctx context.Context, req *csi.D
 	if err != nil {
 		// Cannot get snapshot ID from the passing request
 		// This is a success according to the spec
-		klog.Warningf("Snapshot id does not have the correct format %s", snapshotID)
+		klog.Warningf("Snapshot id does not have the correct format %s: %v", snapshotID, err)
 		return &csi.DeleteSnapshotResponse{}, nil
 	}
 
@@ -1337,7 +1374,7 @@ func (gceCS *GCEControllerServer) getSnapshotByID(ctx context.Context, snapshotI
 				return &csi.ListSnapshotsResponse{}, nil
 			}
 		}
-		e, err := generateImageEntry(image)
+		e, err := generateDiskImageEntry(image)
 		if err != nil {
 			return nil, fmt.Errorf("failed to generate image entry: %w", err)
 		}
@@ -1359,6 +1396,15 @@ func generateDiskSnapshotEntry(snapshot *compute.Snapshot) (*csi.ListSnapshotsRe
 		return nil, fmt.Errorf("Failed to covert creation timestamp: %w", err)
 	}
 
+	snapshotId, err := getResourceId(snapshot.SelfLink)
+	if err != nil {
+		return nil, fmt.Errorf("failed to get snapshot id from %s: %w", snapshot.SelfLink, err)
+	}
+	sourceId, err := getResourceId(snapshot.SourceDisk)
+	if err != nil {
+		return nil, fmt.Errorf("failed to get source id from %s: %w", snapshot.SourceDisk, err)
+	}
+
 	// We ignore the error intentionally here since we are just listing snapshots
 	// TODO: If the snapshot is in "FAILED" state we need to think through what this
 	// should actually look like.
@@ -1367,8 +1413,8 @@ func generateDiskSnapshotEntry(snapshot *compute.Snapshot) (*csi.ListSnapshotsRe
 	entry := &csi.ListSnapshotsResponse_Entry{
 		Snapshot: &csi.Snapshot{
 			SizeBytes:      common.GbToBytes(snapshot.DiskSizeGb),
-			SnapshotId:     cleanSelfLink(snapshot.SelfLink),
-			SourceVolumeId: cleanSelfLink(snapshot.SourceDisk),
+			SnapshotId:     snapshotId,
+			SourceVolumeId: sourceId,
 			CreationTime:   tp,
 			ReadyToUse:     ready,
 		},
@@ -1384,35 +1430,23 @@ func generateDiskImageEntry(image *compute.Image) (*csi.ListSnapshotsResponse_En
 		return nil, fmt.Errorf("failed to covert creation timestamp: %w", err)
 	}
 
-	ready, _ := isImageReady(image.Status)
-
-	entry := &csi.ListSnapshotsResponse_Entry{
-		Snapshot: &csi.Snapshot{
-			SizeBytes:      common.GbToBytes(image.DiskSizeGb),
-			SnapshotId:     cleanSelfLink(image.SelfLink),
-			SourceVolumeId: cleanSelfLink(image.SourceDisk),
-			CreationTime:   tp,
-			ReadyToUse:     ready,
-		},
+	imageId, err := getResourceId(image.SelfLink)
+	if err != nil {
+		return nil, fmt.Errorf("cannot get image id from %s: %w", image.SelfLink, err)
 	}
-	return entry, nil
-}
-
-func generateImageEntry(image *compute.Image) (*csi.ListSnapshotsResponse_Entry, error) {
-	timestamp, err := parseTimestamp(image.CreationTimestamp)
+	sourceId, err := getResourceId(image.SourceDisk)
 	if err != nil {
-		return nil, fmt.Errorf("Failed to covert creation timestamp: %w", err)
+		return nil, fmt.Errorf("cannot get source id from %s: %w", image.SourceDisk, err)
 	}
 
-	// ignore the error intentionally here since we are just listing images
 	ready, _ := isImageReady(image.Status)
 
 	entry := &csi.ListSnapshotsResponse_Entry{
 		Snapshot: &csi.Snapshot{
 			SizeBytes:      common.GbToBytes(image.DiskSizeGb),
-			SnapshotId:     cleanSelfLink(image.SelfLink),
-			SourceVolumeId: cleanSelfLink(image.SourceDisk),
-			CreationTime:   timestamp,
+			SnapshotId:     imageId,
+			SourceVolumeId: sourceId,
+			CreationTime:   tp,
 			ReadyToUse:     ready,
 		},
 	}
@@ -1650,7 +1684,12 @@ func getDefaultZonesInRegion(ctx context.Context, gceCS *GCEControllerServer, ex
 	return ret, nil
 }
 
-func generateCreateVolumeResponse(disk *gce.CloudDisk, zones []string) *csi.CreateVolumeResponse {
+func generateCreateVolumeResponse(disk *gce.CloudDisk, zones []string) (*csi.CreateVolumeResponse, error) {
+	volumeId, err := getResourceId(disk.GetSelfLink())
+	if err != nil {
+		return nil, fmt.Errorf("cannot get volume id from %s: %w", disk.GetSelfLink(), err)
+	}
+
 	tops := []*csi.Topology{}
 	for _, zone := range zones {
 		tops = append(tops, &csi.Topology{
@@ -1661,7 +1700,7 @@ func generateCreateVolumeResponse(disk *gce.CloudDisk, zones []string) *csi.Crea
 	createResp := &csi.CreateVolumeResponse{
 		Volume: &csi.Volume{
 			CapacityBytes:      realDiskSizeBytes,
-			VolumeId:           cleanSelfLink(disk.GetSelfLink()),
+			VolumeId:           volumeId,
 			VolumeContext:      nil,
 			AccessibleTopology: tops,
 		},
@@ -1700,12 +1739,36 @@ func generateCreateVolumeResponse(disk *gce.CloudDisk, zones []string) *csi.Crea
 		}
 		createResp.Volume.ContentSource = contentSource
 	}
-	return createResp
+	return createResp, nil
 }
 
-func cleanSelfLink(selfLink string) string {
-	r, _ := regexp.Compile("https:\\/\\/www.*apis.com\\/.*(v1|beta|alpha)\\/")
-	return r.ReplaceAllString(selfLink, "")
+func getResourceId(resourceLink string) (string, error) {
+	url, err := neturl.Parse(resourceLink)
+	if err != nil {
+		return "", fmt.Errorf("Could not parse resource %s: %w", resourceLink, err)
+	}
+	if url.Scheme != resourceApiScheme {
+		return "", fmt.Errorf("Unexpected API scheme for resource %s", resourceLink)
+	}
+
+	// Note that the resource host can basically be anything, if we are running in
+	// a distributed cloud or trusted partner environment.
+
+	// The path should be /compute/VERSION/project/....
+	elts := strings.Split(url.Path, "/")
+	if len(elts) < 4 {
+		return "", fmt.Errorf("Short resource path %s", resourceLink)
+	}
+	if elts[1] != resourceApiService {
+		return "", fmt.Errorf("Bad resource service %s in %s", elts[1], resourceLink)
+	}
+	if _, ok := validResourceApiVersions[elts[2]]; !ok {
+		return "", fmt.Errorf("Bad version %s in %s", elts[2], resourceLink)
+	}
+	if elts[3] != resourceProject {
+		return "", fmt.Errorf("Expected %v to start with %s in resource %s", elts[3:], resourceProject, resourceLink)
+	}
+	return strings.Join(elts[3:], "/"), nil
 }
 
 func createRegionalDisk(ctx context.Context, cloudProvider gce.GCECompute, name string, zones []string, params common.DiskParameters, capacityRange *csi.CapacityRange, capBytes int64, snapshotID string, volumeContentSourceVolumeID string, multiWriter bool) (*gce.CloudDisk, error) {


Test patch:
diff --git a/pkg/gce-pd-csi-driver/controller_test.go b/pkg/gce-pd-csi-driver/controller_test.go
index d49809c7..c3c64dff 100644
--- a/pkg/gce-pd-csi-driver/controller_test.go
+++ b/pkg/gce-pd-csi-driver/controller_test.go
@@ -930,7 +930,11 @@ func TestListVolumePagination(t *testing.T) {
 			var d []*gce.CloudDisk
 			for i := 0; i < tc.diskCount; i++ {
 				// Create diskCount dummy disks
-				d = append(d, gce.CloudDiskFromV1(&compute.Disk{Name: fmt.Sprintf("%v", i)}))
+				name := fmt.Sprintf("disk-%v", i)
+				d = append(d, gce.CloudDiskFromV1(&compute.Disk{
+					Name:     name,
+					SelfLink: fmt.Sprintf("https://www.googleapis.com/compute/v1/projects/project/zones/zone/disk/%s", name),
+				}))
 			}
 			gceDriver := initGCEDriver(t, d)
 			tok := ""
@@ -988,7 +992,11 @@ func TestListVolumeArgs(t *testing.T) {
 			var d []*gce.CloudDisk
 			for i := 0; i < diskCount; i++ {
 				// Create 600 dummy disks
-				d = append(d, gce.CloudDiskFromV1(&compute.Disk{Name: fmt.Sprintf("%v", i)}))
+				name := fmt.Sprintf("disk-%v", i)
+				d = append(d, gce.CloudDiskFromV1(&compute.Disk{
+					Name:     name,
+					SelfLink: fmt.Sprintf("https://www.googleapis.com/compute/v1/projects/project/zones/zone/disk/%s", name),
+				}))
 			}
 			gceDriver := initGCEDriver(t, d)
 			lvr := &csi.ListVolumesRequest{
@@ -1887,7 +1895,8 @@ func TestCreateVolumeRandomRequisiteTopology(t *testing.T) {
 
 func createZonalCloudDisk(name string) *gce.CloudDisk {
 	return gce.CloudDiskFromV1(&compute.Disk{
-		Name: name,
+		Name:     name,
+		SelfLink: fmt.Sprintf("https://www.googleapis.com/compute/v1/projects/project/zones/zone/name/%s", name),
 	})
 }
 
@@ -3231,11 +3240,12 @@ func TestControllerPublishBackoff(t *testing.T) {
 	}
 }
 
-func TestCleanSelfLink(t *testing.T) {
+func TestGetResource(t *testing.T) {
 	testCases := []struct {
-		name string
-		in   string
-		want string
+		name  string
+		in    string
+		want  string
+		error bool
 	}{
 		{
 			name: "v1 full standard w/ endpoint prefix",
@@ -3253,24 +3263,38 @@ func TestCleanSelfLink(t *testing.T) {
 			want: "projects/project/zones/zone/disks/disk",
 		},
 		{
-			name: "no prefix",
-			in:   "projects/project/zones/zone/disks/disk",
-			want: "projects/project/zones/zone/disks/disk",
+			name:  "no prefix",
+			in:    "projects/project/zones/zone/disks/disk",
+			error: true,
 		},
-
 		{
-			name: "no prefix + project omitted",
-			in:   "zones/zone/disks/disk",
-			want: "zones/zone/disks/disk",
+			name:  "no prefix + project omitted",
+			in:    "zones/zone/disks/disk",
+			error: true,
 		},
 		{
-			name: "Compute prefix, google api",
+			name: "Compute prefix, www google api",
 			in:   "https://www.compute.googleapis.com/compute/v1/projects/project/zones/zone/disks/disk",
 			want: "projects/project/zones/zone/disks/disk",
 		},
 		{
 			name: "Compute prefix, partner api",
-			in:   "https://www.compute.PARTNERapis.com/compute/v1/projects/project/zones/zone/disks/disk",
+			in:   "https://www.compute.partnerapis.com/compute/v1/projects/project/zones/zone/disks/disk",
+			want: "projects/project/zones/zone/disks/disk",
+		},
+		{
+			name: "Compute, alternate googleapis host",
+			in:   "https://content-compute.googleapis.com/compute/v1/projects/project/zones/zone/disks/disk",
+			want: "projects/project/zones/zone/disks/disk",
+		},
+		{
+			name: "Compute, partner host",
+			in:   "https://compute.blahapis.com/compute/v1/projects/project/zones/zone/disks/disk",
+			want: "projects/project/zones/zone/disks/disk",
+		},
+		{
+			name: "Alternate partner host with mtls domain",
+			in:   "https://content-compute.us-central1.rep.mtls.googleapis.com/compute/v1/projects/project/zones/zone/disks/disk",
 			want: "projects/project/zones/zone/disks/disk",
 		},
 		{
@@ -3283,13 +3307,66 @@ func TestCleanSelfLink(t *testing.T) {
 			in:   "https://www.partnerapis.com/compute/alpha/projects/project/zones/zone/disks/disk",
 			want: "projects/project/zones/zone/disks/disk",
 		},
+		{
+			name: "alpha project",
+			in:   "https://www.googleapis.com/compute/alpha/projects/alphaproject/zones/zone/disks/disk",
+			want: "projects/alphaproject/zones/zone/disks/disk",
+		},
+		{
+			name: "beta project",
+			in:   "https://www.googleapis.com/compute/alpha/projects/betabeta/zones/zone/disks/disk",
+			want: "projects/betabeta/zones/zone/disks/disk",
+		},
+		{
+			name: "v1 project",
+			in:   "https://www.googleapis.com/compute/alpha/projects/projectv1/zones/zone/disks/disk",
+			want: "projects/projectv1/zones/zone/disks/disk",
+		},
+		{
+			name: "random host",
+			in:   "https://npr.org/compute/v1/projects/project/zones/zone/disks/disk",
+			want: "projects/project/zones/zone/disks/disk",
+		},
+		{
+			name:  "no prefix",
+			in:    "projects/project/zones/zone/disks/disk",
+			error: true,
+		},
+		{
+			name:  "bad scheme",
+			in:    "ftp://www.googleapis.com/compute/v1/projects/project/zones/zone/disks/disk",
+			error: true,
+		},
+		{
+			name:  "insecure scheme",
+			in:    "http://www.googleapis.com/compute/v1/projects/project/zones/zone/disks/disk",
+			error: true,
+		},
+		{
+			name:  "bad service",
+			in:    "https://www.googleapis.com/computers/v1/projects/project/zones/zone/disks/disk",
+			error: true,
+		},
+		{
+			name:  "bad version",
+			in:    "https://www..googleapis.com/compute/zeta/projects/project/zones/zone/disks/disk",
+			error: true,
+		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			got := cleanSelfLink(tc.in)
-			if got != tc.want {
-				t.Errorf("Expected cleaned self link: %v, got: %v", tc.want, got)
+			got, err := getResourceId(tc.in)
+			if tc.error {
+				if err == nil {
+					t.Errorf("Expected error, but got none")
+				}
+			} else {
+				if err != nil {
+					t.Errorf("Unexpected error %v", err)
+				} else if got != tc.want {
+					t.Errorf("Expected cleaned self link: %v, got: %v", tc.want, got)
+				}
 			}
 		})
 	}