From 4d07b05851b044324b02388d1e8936885b8a61fc Mon Sep 17 00:00:00 2001 From: Matthew Cary Date: Thu, 20 Jul 2023 16:37:40 -0700 Subject: [PATCH] Replace cleanSelfLink with getResourceId Change-Id: I81e79dd89d7bab65c7ccab3cd490b173593a888f --- pkg/gce-cloud-provider/compute/fake-gce.go | 4 +- pkg/gce-pd-csi-driver/controller.go | 140 +++++++++++++++------ pkg/gce-pd-csi-driver/controller_test.go | 115 ++++++++++++++--- 3 files changed, 198 insertions(+), 61 deletions(-) diff --git a/pkg/gce-cloud-provider/compute/fake-gce.go b/pkg/gce-cloud-provider/compute/fake-gce.go index 82a5e18fb..4efc3fad9 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 d27937b0b..0fd00aa32 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" @@ -138,6 +138,14 @@ const ( // 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) { @@ -319,7 +327,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, params), nil + return generateCreateVolumeResponse(existingDisk, zones, params) } snapshotID := "" @@ -434,7 +442,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, params), nil + return generateCreateVolumeResponse(disk, zones, params) } @@ -869,13 +877,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, @@ -988,6 +1006,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 { @@ -1006,7 +1028,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, @@ -1035,6 +1057,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 { @@ -1053,7 +1079,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, @@ -1065,9 +1091,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("failed to get source id from %s: %w", image.SourceDisk, err) + } + _, sourceKey, err := common.VolumeIDToKey(sourceId) if err != nil { - return fmt.Errorf("fail to get source disk key %s, %w", image.SourceDisk, err) + return fmt.Errorf("failed to get source disk key %s: %w", image.SourceDisk, err) } if sourceKey.String() != volKey.String() { @@ -1116,7 +1146,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) } @@ -1159,7 +1193,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 } @@ -1350,7 +1384,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) } @@ -1372,6 +1406,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. @@ -1380,8 +1423,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, }, @@ -1397,35 +1440,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, }, } @@ -1691,7 +1722,12 @@ func extractVolumeContext(context map[string]string) (*PDCSIContext, error) { return info, nil } -func generateCreateVolumeResponse(disk *gce.CloudDisk, zones []string, params common.DiskParameters) *csi.CreateVolumeResponse { +func generateCreateVolumeResponse(disk *gce.CloudDisk, zones []string, params common.DiskParameters) (*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{ @@ -1702,7 +1738,7 @@ func generateCreateVolumeResponse(disk *gce.CloudDisk, zones []string, params co createResp := &csi.CreateVolumeResponse{ Volume: &csi.Volume{ CapacityBytes: realDiskSizeBytes, - VolumeId: cleanSelfLink(disk.GetSelfLink()), + VolumeId: volumeId, VolumeContext: paramsToVolumeContext(params), AccessibleTopology: tops, }, @@ -1741,12 +1777,36 @@ func generateCreateVolumeResponse(disk *gce.CloudDisk, zones []string, params co } 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 d97815f10..fbe0a06ac 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), }) } @@ -3252,11 +3261,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", @@ -3274,24 +3284,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", }, { @@ -3304,13 +3328,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) + } } }) }