GCP: Fix uninstaller to not use hardcoded base path. #3702

rna-afk · 2020-06-01T18:26:04Z

The uninstaller currently hard codes the base path as the GCP
API base path has changed.

Change 1:
For the getZoneName function, the InstanceGroupAggregatedList,
InstanceAggregatedList and the disk list functions that calls
this to get the proper url all have a shortened URL as the key
in the list.Items map. Example of a shortened URL is
zones/us-central1-a.
Reusing the key value instead of hard coding the base path
and TrimLeft operation.

Change 2:
For the getInstanceGroupURL function, the call to the underlying
InstanceGroupAggregatedList to get the InstanceGroups already have
the URL to hit called the SelfLink which is now set to the returning
object and used for the URL, thus not requiring the call.

Change 3:
For the getInstanceNameAndZone, since the underlying object has only
the URL and no other relevant information, the base path is removed
and the TrimLeft operation is changed to Split based on the '/projects/'
URL query parameter instead of using the base path.

Previous commit information:
11c9773#diff-4781d154886c4da10a007bb219f27d8aL170
TODO: Find a better solution for change 3.

rna-afk · 2020-06-01T19:10:12Z

/test e2e-gcp

rna-afk · 2020-06-04T16:52:01Z

/test e2e-gcp

patrickdillon · 2020-06-04T19:53:56Z

@jstuever PTAL when you get a chance. I have spent time trying to review but I think I'm missing too much context to give high-level insight. I don't want to send @rna-afk too far in the wrong direction.

jstuever

Just a few recommendations....

jstuever · 2020-06-05T17:45:27Z

pkg/destroy/gcp/cloudcontroller.go

@@ -22,7 +22,7 @@ func (o *ClusterUninstaller) listCloudControllerInstanceGroups() ([]cloudResourc
 func (o *ClusterUninstaller) listCloudControllerBackendServices(instanceGroups []cloudResource) ([]cloudResource, error) {
 	urls := sets.NewString()
 	for _, instanceGroup := range instanceGroups {
-		urls.Insert(o.getInstanceGroupURL(instanceGroup))
+		urls.Insert(instanceGroup.url)


This is good... please delete the getInstanceGroupURL function definition as well. It doesn't appear to be used after this change.

installer/pkg/destroy/gcp/gcp.go

Lines 186 to 189 in a7f8c8d

// TODO: Find a better way to get the instance group URL to account for changes in base path

func (o *ClusterUninstaller) getInstanceGroupURL(ig cloudResource) string {

return fmt.Sprintf("%s%s/zones/%s/instanceGroups/%s", "https://www.googleapis.com/compute/v1/projects/", o.ProjectID, ig.zone, ig.name)

}

jstuever · 2020-06-05T17:49:04Z

pkg/destroy/gcp/disk.go

 			for _, item := range scopedList.Disks {
 				if filterFunc == nil || filterFunc != nil && filterFunc(item) {
-					zone := o.getZoneName(item.Zone)
+					zone := strings.Split(key, "/")[1]


Instead of this, let's update getZoneName to split based on /projects/ similar to how you did in instance.go. This allows us to update it in a single place if it changes again in the future.

Split(instanceURL, "/projects/")[1]

installer/pkg/destroy/gcp/gcp.go

Lines 168 to 175 in a7f8c8d

func (o *ClusterUninstaller) getZoneName(zoneURL string) string {

path := strings.TrimLeft(zoneURL, "https://www.googleapis.com/compute/v1/projects/")

parts := strings.Split(path, "/")

if len(parts) >= 3 {

return parts[2]

}

return ""

}

Would split based on /projects/ not cause a problem? I was worried that there might be something else named projects in the URL. That might cause some problems.

There might be a risk of that... probably less of a risk with the surrounding slashes. I also notice that the zone name is the last entity in strings.Split(path, "/").... so that might be better here.

parts:=strings.Split(path, "/") if len(parts) { return parts[len(parts)-1] }

jstuever · 2020-06-05T17:49:51Z

pkg/destroy/gcp/instance.go

 			for _, item := range scopedList.Instances {
 				if filterFunc == nil || filterFunc != nil && filterFunc(item) {
-					zoneName := o.getZoneName(item.Zone)
+					zoneName := strings.Split(key, "/")[1]


again, update getZoneName instead.

jstuever · 2020-06-05T17:50:03Z

pkg/destroy/gcp/instancegroup.go

 			for _, item := range scopedList.InstanceGroups {
 				if filterFunc == nil || filterFunc != nil && filterFunc(item) {
-					zoneName := o.getZoneName(item.Zone)
+					zoneName := strings.Split(key, "/")[1]


again, update getZoneName instead.

jstuever · 2020-06-08T23:26:49Z

/test e2e-gcp

rna-afk · 2020-06-09T13:28:21Z

/test e2e-gcp

rna-afk · 2020-06-10T13:05:42Z

/test e2e-gcp

The uninstaller currently hard codes the base path as the GCP API base path has changed. Change 1: For the getZoneName function, the InstanceGroupAggregatedList, InstanceAggregatedList and the disk list functions that calls this to get the proper url all have a shortened URL as the key in the list.Items map. Example of a shortened URL is zones/us-central1-a. Reusing the key value instead of hard coding the base path and TrimLeft operation. Change 2: For the getInstanceGroupURL function, the call to the underlying InstanceGroupAggregatedList to get the InstanceGroups already have the URL to hit called the SelfLink which is now set to the returning object and used for the URL, thus not requiring the call. Change 3: For the getInstanceNameAndZone, since the underlying object has only the URL and no other relevant information, the base path is removed and the TrimLeft operation is changed to Split based on the '/projects/' URL query parameter instead of using the base path. Previous commit information: openshift@11c9773#diff-4781d154886c4da10a007bb219f27d8aL170 TODO: Find a better solution for change 3.

jstuever · 2020-06-10T17:13:42Z

/test e2e-gcp

rna-afk · 2020-06-10T22:38:31Z

/test e2e-gcp

jstuever · 2020-06-11T17:11:21Z

/lgtm

jstuever · 2020-06-11T17:11:59Z

/approve

openshift-ci-robot · 2020-06-11T17:12:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jstuever

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/destroy/gcp/OWNERS~~ [jstuever]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2020-06-11T17:47:08Z

/retest