Merge pull request #11329 from a-robinson/targets

Fix issue of comparing instance URLs with different project ID representations in GCE target pools

pkg/cloudprovider/gce/gce.go

@@ -219,12 +219,25 @@ func (gce *GCECloud) Routes() (cloudprovider.Routes, bool) {
 	return gce, true
 }
 
-func makeHostLink(projectID, zone, host string) string {
+func makeHostURL(projectID, zone, host string) string {
 	host = canonicalizeInstanceName(host)
 	return fmt.Sprintf("https://www.googleapis.com/compute/v1/projects/%s/zones/%s/instances/%s",
 		projectID, zone, host)
 }
 
+func makeComparableHostPath(zone, host string) string {
+	host = canonicalizeInstanceName(host)
+	return fmt.Sprintf("/zones/%s/instances/%s", zone, host)
+}
+
+func hostURLToComparablePath(hostURL string) string {
+	idx := strings.Index(hostURL, "/zones/")
+	if idx < 0 {
+		return ""
+	}
+	return hostURL[idx:]
+}
+
 // Session Affinity Type string
 type GCEAffinityType string
 
@@ -240,7 +253,7 @@ const (
 func (gce *GCECloud) makeTargetPool(name, region string, hosts []string, affinityType GCEAffinityType) error {
 	var instances []string
 	for _, host := range hosts {
-		instances = append(instances, makeHostLink(gce.projectID, gce.zone, host))
+		instances = append(instances, makeHostURL(gce.projectID, gce.zone, host))
 	}
 	pool := &compute.TargetPool{
 		Name:            name,
@@ -427,12 +440,15 @@ func (gce *GCECloud) UpdateTCPLoadBalancer(name, region string, hosts []string)
 	if err != nil {
 		return err
 	}
-	existing := util.NewStringSet(pool.Instances...)
+	existing := util.NewStringSet()
+	for _, instance := range pool.Instances {
+		existing.Insert(hostURLToComparablePath(instance))
+	}
 
 	var toAdd []*compute.InstanceReference
 	var toRemove []*compute.InstanceReference
 	for _, host := range hosts {
-		link := makeHostLink(gce.projectID, gce.zone, host)
+		link := makeComparableHostPath(gce.zone, host)
 		if !existing.Has(link) {
 			toAdd = append(toAdd, &compute.InstanceReference{link})
 		}

pkg/cloudprovider/gce/gce_test.go

@@ -60,3 +60,65 @@ func TestGetHostTag(t *testing.T) {
 		}
 	}
 }
+
+func TestComparingHostURLs(t *testing.T) {
+	tests := []struct {
+		host1       string
+		zone        string
+		name        string
+		expectEqual bool
+	}{
+		{
+			host1:       "https://www.googleapis.com/compute/v1/projects/1234567/zones/us-central1-f/instances/kubernetes-node-fhx1",
+			zone:        "us-central1-f",
+			name:        "kubernetes-node-fhx1",
+			expectEqual: true,
+		},
+		{
+			host1:       "https://www.googleapis.com/compute/v1/projects/cool-project/zones/us-central1-f/instances/kubernetes-node-fhx1",
+			zone:        "us-central1-f",
+			name:        "kubernetes-node-fhx1",
+			expectEqual: true,
+		},
+		{
+			host1:       "https://www.googleapis.com/compute/v23/projects/1234567/zones/us-central1-f/instances/kubernetes-node-fhx1",
+			zone:        "us-central1-f",
+			name:        "kubernetes-node-fhx1",
+			expectEqual: true,
+		},
+		{
+			host1:       "https://www.googleapis.com/compute/v24/projects/1234567/regions/us-central1/zones/us-central1-f/instances/kubernetes-node-fhx1",
+			zone:        "us-central1-f",
+			name:        "kubernetes-node-fhx1",
+			expectEqual: true,
+		},
+		{
+			host1:       "https://www.googleapis.com/compute/v1/projects/1234567/zones/us-central1-f/instances/kubernetes-node-fhx1",
+			zone:        "us-central1-c",
+			name:        "kubernetes-node-fhx1",
+			expectEqual: false,
+		},
+		{
+			host1:       "https://www.googleapis.com/compute/v1/projects/1234567/zones/us-central1-f/instances/kubernetes-node-fhx",
+			zone:        "us-central1-f",
+			name:        "kubernetes-node-fhx1",
+			expectEqual: false,
+		},
+		{
+			host1:       "https://www.googleapis.com/compute/v1/projects/1234567/zones/us-central1-f/instances/kubernetes-node-fhx1",
+			zone:        "us-central1-f",
+			name:        "kubernetes-node-fhx",
+			expectEqual: false,
+		},
+	}
+
+	for _, test := range tests {
+		link1 := hostURLToComparablePath(test.host1)
+		link2 := makeComparableHostPath(test.zone, test.name)
+		if test.expectEqual && link1 != link2 {
+			t.Errorf("expected link1 and link2 to be equal, got %s and %s", link1, link2)
+		} else if !test.expectEqual && link1 == link2 {
+			t.Errorf("expected link1 and link2 not to be equal, got %s and %s", link1, link2)
+		}
+	}
+}