From 1c5f571e977a6d3fe915c13fefae5a986dd81eec Mon Sep 17 00:00:00 2001 From: Matthew Cary Date: Thu, 20 Jul 2023 16:37:40 -0700 Subject: [PATCH 1/2] Replace cleanSelfLink with getResourceId Change-Id: I531dc90aa4640afc2d66b5ce57d698a56c7d4564 --- pkg/gce-cloud-provider/compute/fake-gce.go | 4 +- pkg/gce-pd-csi-driver/controller.go | 141 +++++++++++++++------ pkg/gce-pd-csi-driver/controller_test.go | 115 ++++++++++++++--- 3 files changed, 200 insertions(+), 60 deletions(-) diff --git a/pkg/gce-cloud-provider/compute/fake-gce.go b/pkg/gce-cloud-provider/compute/fake-gce.go index e67311400..c0a7fa385 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 9c682ebb1..5ffb0eaac 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{ @@ -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) { diff --git a/pkg/gce-pd-csi-driver/controller_test.go b/pkg/gce-pd-csi-driver/controller_test.go index d49809c77..c3c64dff5 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) + } } }) } From f2cfc817b0cf7ac650fa4ce7a5556698228d6ac7 Mon Sep 17 00:00:00 2001 From: Matthew Cary Date: Fri, 21 Jul 2023 15:15:30 -0700 Subject: [PATCH 2/2] Cherry-pick #1304 to release-1.9 Change-Id: I02a481e7759a502a7ab7117d58357e8139d75971 --- pkg/gce-pd-csi-driver/controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/gce-pd-csi-driver/controller.go b/pkg/gce-pd-csi-driver/controller.go index 5ffb0eaac..fba3a8a9b 100644 --- a/pkg/gce-pd-csi-driver/controller.go +++ b/pkg/gce-pd-csi-driver/controller.go @@ -1700,7 +1700,7 @@ func generateCreateVolumeResponse(disk *gce.CloudDisk, zones []string) (*csi.Cre createResp := &csi.CreateVolumeResponse{ Volume: &csi.Volume{ CapacityBytes: realDiskSizeBytes, - VolumeId: cleanSelfLink(disk.GetSelfLink()), + VolumeId: volumeId, VolumeContext: nil, AccessibleTopology: tops, },