Merge pull request #307 from kubernetes/revert-304-links

Revert "Use cloud ResourceID for URL parsing and generation"

pkg/backends/backends.go

@@ -154,7 +154,14 @@ func ensureDescription(be *BackendService, description string) (needsUpdate bool
 func ensureHealthCheckLink(be *BackendService, hcLink string) (needsUpdate bool) {
 	existingHCLink := getHealthCheckLink(be)
 
-	if utils.EqualResourceID(existingHCLink, hcLink) {
+	// Compare health check name instead of health check link.
+	// This is because health check link contains api version.
+	// For NEG, the api version for health check will be alpha.
+	// Hence, it will cause the health check links to be always different
+	// TODO (mixia): compare health check link directly once NEG is GA
+	existingHCName := retrieveObjectName(existingHCLink)
+	expectedHCName := retrieveObjectName(hcLink)
+	if existingHCName == expectedHCName {
 		return false
 	}
 
@@ -307,11 +314,7 @@ func (b *Backends) ensureBackendService(sp utils.ServicePort, igLinks []string)
 // edgeHop checks the links of the given backend by executing an edge hop.
 // It fixes broken links and updates the Backend accordingly.
 func (b *Backends) edgeHop(be *BackendService, igLinks []string) error {
-	addIGs, err := getInstanceGroupsToAdd(be, igLinks)
-	if err != nil {
-		return err
-	}
-
+	addIGs := getInstanceGroupsToAdd(be, igLinks)
 	if len(addIGs) == 0 {
 		return nil
 	}
@@ -425,32 +428,32 @@ func getBackendsForNEGs(negs []*computealpha.NetworkEndpointGroup) []*computealp
 	return backends
 }
 
-func getInstanceGroupsToAdd(be *BackendService, igLinks []string) ([]string, error) {
-	existingIGs := sets.String{}
+func getInstanceGroupsToAdd(be *BackendService, igLinks []string) []string {
+	beName := be.Name
+	beIGs := sets.String{}
 	for _, existingBe := range be.Backends {
-		path, err := utils.ResourcePath(existingBe.Group)
-		if err != nil {
-			return nil, fmt.Errorf("failed to parse instance group: %v", err)
-		}
-		existingIGs.Insert(path)
+		beIGs.Insert(comparableGroupPath(existingBe.Group))
 	}
 
-	wantIGs := sets.String{}
+	expectedIGs := sets.String{}
 	for _, igLink := range igLinks {
-		path, err := utils.ResourcePath(igLink)
-		if err != nil {
-			return nil, fmt.Errorf("failed to parse instance group: %v", err)
-		}
-		wantIGs.Insert(path)
+		expectedIGs.Insert(comparableGroupPath(igLink))
 	}
 
-	missingIGs := wantIGs.Difference(existingIGs)
-	if missingIGs.Len() > 0 {
-		glog.V(2).Infof("Backend service %q has instance groups %+v, want %+v",
-			be.Name, existingIGs.List(), wantIGs.List())
+	if beIGs.IsSuperset(expectedIGs) {
+		return nil
 	}
+	glog.V(2).Infof("Expected igs for backend service %v: %+v, current igs %+v",
+		beName, expectedIGs.List(), beIGs.List())
 
-	return missingIGs.List(), nil
+	var addIGs []string
+	for _, igLink := range igLinks {
+		if !beIGs.Has(comparableGroupPath(igLink)) {
+			addIGs = append(addIGs, igLink)
+		}
+	}
+
+	return addIGs
 }
 
 // GC garbage collects services corresponding to ports in the given list.
@@ -578,3 +581,16 @@ func applyProbeSettingsToHC(p *v1.Probe, hc *healthchecks.HealthCheck) {
 		hc.CheckIntervalSec = int64(p.PeriodSeconds) + int64(healthchecks.DefaultHealthCheckInterval.Seconds())
 	}
 }
+
+//retrieveObjectName takes a GCE object link and return the last part of the url as object name
+func retrieveObjectName(url string) string {
+	splited := strings.Split(url, "/")
+	return splited[len(splited)-1]
+}
+
+// comparableGroupPath trims project and compute version from the SelfLink
+// /zones/[ZONE_NAME]/instanceGroups/[IG_NAME]
+func comparableGroupPath(url string) string {
+	path_parts := strings.Split(url, "/zones/")
+	return fmt.Sprintf("/zones/%s", path_parts[1])
+}

pkg/backends/backends_test.go

@@ -355,7 +355,7 @@ func TestBackendPoolChaosMonkey(t *testing.T) {
 	// Mess up the link between backend service and instance group.
 	// This simulates a user doing foolish things through the UI.
 	be.Backends = []*compute.Backend{
-		{Group: "zones/us-central1-c/instanceGroups/edge-hop-test"},
+		{Group: "/zones/edge-hop-test"},
 	}
 
 	// Add hook to keep track of how many calls are made.
@@ -384,17 +384,16 @@ func TestBackendPoolChaosMonkey(t *testing.T) {
 	if err != nil {
 		t.Fatalf("Failed to find instance group %v", defaultNamer.InstanceGroup())
 	}
-	groupPath, err := utils.ResourcePath(gotGroup.SelfLink)
-	if err != nil {
-		t.Fatalf("Failed to get resource path from %q", gotGroup.SelfLink)
-	}
-
+	backendLinks := sets.NewString()
 	for _, be := range gotBackend.Backends {
-		if be.Group == groupPath {
-			return
-		}
+		backendLinks.Insert(be.Group)
+	}
+	if !backendLinks.Has(gotGroup.SelfLink) {
+		t.Fatalf(
+			"Broken instance group link, got: %+v expected: %v",
+			backendLinks.List(),
+			gotGroup.SelfLink)
 	}
-	t.Fatalf("Failed to find %q in backend groups %+v", groupPath, be.Backends)
 }
 
 func TestBackendPoolSync(t *testing.T) {
@@ -721,8 +720,8 @@ func TestBackendInstanceGroupClobbering(t *testing.T) {
 	// Simulate another controller updating the same backend service with
 	// a different instance group
 	newGroups := []*compute.Backend{
-		{Group: fmt.Sprintf("zones/%s/instanceGroups/%s", defaultZone, "k8s-ig-bar")},
-		{Group: fmt.Sprintf("zones/%s/instanceGroups/%s", defaultZone, "k8s-ig-foo")},
+		{Group: fmt.Sprintf("/zones/%s/instanceGroups/%s", defaultZone, "k8s-ig-bar")},
+		{Group: fmt.Sprintf("/zones/%s/instanceGroups/%s", defaultZone, "k8s-ig-foo")},
 	}
 	be.Backends = append(be.Backends, newGroups...)
 	if err = fakeGCE.UpdateGlobalBackendService(be); err != nil {
@@ -741,21 +740,13 @@ func TestBackendInstanceGroupClobbering(t *testing.T) {
 	}
 	gotGroups := sets.NewString()
 	for _, g := range be.Backends {
-		igPath, err := utils.ResourcePath(g.Group)
-		if err != nil {
-			t.Fatalf("%v", err)
-		}
-		gotGroups.Insert(igPath)
+		gotGroups.Insert(comparableGroupPath(g.Group))
 	}
 
 	// seed expectedGroups with the first group native to this controller
-	expectedGroups := sets.NewString(fmt.Sprintf("zones/%s/instanceGroups/%s", defaultZone, "k8s-ig--uid1"))
+	expectedGroups := sets.NewString(fmt.Sprintf("/zones/%s/instanceGroups/%s", defaultZone, "k8s-ig--uid1"))
 	for _, newGroup := range newGroups {
-		igPath, err := utils.ResourcePath(newGroup.Group)
-		if err != nil {
-			t.Fatalf("%v", err)
-		}
-		expectedGroups.Insert(igPath)
+		expectedGroups.Insert(comparableGroupPath(newGroup.Group))
 	}
 	if !expectedGroups.Equal(gotGroups) {
 		t.Fatalf("Expected %v Got %v", expectedGroups, gotGroups)
@@ -887,9 +878,65 @@ func TestLinkBackendServiceToNEG(t *testing.T) {
 	}
 
 	for _, be := range bs.Backends {
-		neg := "networkEndpointGroups"
+		neg := "NetworkEndpointGroup"
 		if !strings.Contains(be.Group, neg) {
-			t.Errorf("Got backend link %q, want containing %q", be.Group, neg)
+			t.Errorf("Expect backend to be a NEG, but got %q", be.Group)
+		}
+	}
+}
+
+func TestRetrieveObjectName(t *testing.T) {
+	testCases := []struct {
+		url    string
+		expect string
+	}{
+		{
+			"",
+			"",
+		},
+		{
+			"a/b/c/d/",
+			"",
+		},
+		{
+			"a/b/c/d",
+			"d",
+		},
+		{
+			"compute",
+			"compute",
+		},
+	}
+
+	for _, tc := range testCases {
+		if retrieveObjectName(tc.url) != tc.expect {
+			t.Errorf("expect %q, but got %q", tc.expect, retrieveObjectName(tc.url))
+		}
+	}
+}
+
+func TestComparableGroupPath(t *testing.T) {
+	testCases := []struct {
+		igPath   string
+		expected string
+	}{
+		{
+			"https://www.googleapis.com/compute/beta/projects/project-id/zones/us-central1-a/instanceGroups/example-group",
+			"/zones/us-central1-a/instanceGroups/example-group",
+		},
+		{
+			"https://www.googleapis.com/compute/alpha/projects/project-id/zones/us-central1-b/instanceGroups/test-group",
+			"/zones/us-central1-b/instanceGroups/test-group",
+		},
+		{
+			"https://www.googleapis.com/compute/v1/projects/project-id/zones/us-central1-c/instanceGroups/another-group",
+			"/zones/us-central1-c/instanceGroups/another-group",
+		},
+	}
+
+	for _, tc := range testCases {
+		if comparableGroupPath(tc.igPath) != tc.expected {
+			t.Errorf("expected %s, but got %s", tc.expected, comparableGroupPath(tc.igPath))
 		}
 	}
 }

pkg/healthchecks/fakes.go

@@ -21,8 +21,6 @@ import (
 	compute "google.golang.org/api/compute/v1"
 
 	"k8s.io/ingress-gce/pkg/utils"
-	"k8s.io/kubernetes/pkg/cloudprovider/providers/gce/cloud"
-	"k8s.io/kubernetes/pkg/cloudprovider/providers/gce/cloud/meta"
 )
 
 // NewFakeHealthCheckProvider returns a new FakeHealthChecks.
@@ -42,7 +40,7 @@ type FakeHealthCheckProvider struct {
 // CreateHttpHealthCheck fakes out http health check creation.
 func (f *FakeHealthCheckProvider) CreateHttpHealthCheck(hc *compute.HttpHealthCheck) error {
 	v := *hc
-	v.SelfLink = cloud.NewHttpHealthChecksResourceID("mock-project", hc.Name).SelfLink(meta.VersionGA)
+	v.SelfLink = "https://fake.google.com/compute/httpHealthChecks/" + hc.Name
 	f.http[hc.Name] = v
 	return nil
 }
@@ -79,17 +77,17 @@ func (f *FakeHealthCheckProvider) UpdateHttpHealthCheck(hc *compute.HttpHealthCh
 // CreateHealthCheck fakes out http health check creation.
 func (f *FakeHealthCheckProvider) CreateHealthCheck(hc *compute.HealthCheck) error {
 	v := *hc
-	v.SelfLink = cloud.NewHealthChecksResourceID("mock-project", hc.Name).SelfLink(meta.VersionGA)
-	alphaHC, _ := toAlphaHealthCheck(&v)
+	v.SelfLink = "https://fake.google.com/compute/healthChecks/" + hc.Name
+	alphaHC, _ := toAlphaHealthCheck(hc)
 	f.generic[hc.Name] = *alphaHC
 	return nil
 }
 
 // CreateHealthCheck fakes out http health check creation.
 func (f *FakeHealthCheckProvider) CreateAlphaHealthCheck(hc *computealpha.HealthCheck) error {
 	v := *hc
-	v.SelfLink = cloud.NewHealthChecksResourceID("mock-project", hc.Name).SelfLink(meta.VersionAlpha)
-	f.generic[hc.Name] = v
+	v.SelfLink = "https://fake.google.com/compute/healthChecks/" + hc.Name
+	f.generic[hc.Name] = *hc
 	return nil
 }
 

pkg/instances/fakes.go

@@ -22,8 +22,6 @@ import (
 
 	compute "google.golang.org/api/compute/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
-	"k8s.io/kubernetes/pkg/cloudprovider/providers/gce/cloud"
-	"k8s.io/kubernetes/pkg/cloudprovider/providers/gce/cloud/meta"
 
 	"k8s.io/ingress-gce/pkg/utils"
 )
@@ -83,7 +81,7 @@ func (f *FakeInstanceGroups) GetInstanceGroup(name, zone string) (*compute.Insta
 
 // CreateInstanceGroup fakes instance group creation.
 func (f *FakeInstanceGroups) CreateInstanceGroup(ig *compute.InstanceGroup, zone string) error {
-	ig.SelfLink = cloud.NewInstanceGroupsResourceID("mock-project", zone, ig.Name).SelfLink(meta.VersionGA)
+	ig.SelfLink = fmt.Sprintf("/zones/%s/instanceGroups/%s", zone, ig.Name)
 	ig.Zone = zone
 	f.instanceGroups = append(f.instanceGroups, ig)
 	return nil

pkg/instances/instances.go

@@ -19,6 +19,7 @@ package instances
 import (
 	"fmt"
 	"net/http"
+	"strings"
 
 	"github.com/golang/glog"
 
@@ -185,11 +186,10 @@ func (i *Instances) list(name string) (sets.String, error) {
 			return nodeNames, err
 		}
 		for _, ins := range instances {
-			name, err := utils.KeyName(ins.Instance)
-			if err != nil {
-				return nodeNames, err
-			}
-			nodeNames.Insert(name)
+			// TODO: If round trips weren't so slow one would be inclided
+			// to GetInstance using this url and get the name.
+			parts := strings.Split(ins.Instance, "/")
+			nodeNames.Insert(parts[len(parts)-1])
 		}
 	}
 	return nodeNames, nil

pkg/loadbalancers/certificates.go

@@ -159,19 +159,14 @@ func (l *L7) populateSSLCert() error {
 	if len(l.sslCerts) == 0 {
 		// Check for legacy cert since that follows a different naming convention
 		glog.V(4).Infof("Looking for legacy ssl certs")
-		expectedCertLinks := l.getSslCertLinkInUse()
-		for _, link := range expectedCertLinks {
+		expectedCertNames := l.getSslCertLinkInUse()
+		for _, link := range expectedCertNames {
 			// Retrieve the certificate and ignore error if certificate wasn't found
-			name, err := utils.KeyName(link)
-			if err != nil {
-				glog.Warningf("error getting certificate name: %v", link)
-				continue
-			}
-
+			name := getResourceNameFromLink(link)
 			if !l.namer.IsLegacySSLCert(l.Name, name) {
 				continue
 			}
-			cert, _ := l.cloud.GetSslCertificate(name)
+			cert, _ := l.cloud.GetSslCertificate(getResourceNameFromLink(name))
 			if cert != nil {
 				glog.V(4).Infof("Populating legacy ssl cert %s for l7 %s", cert.Name, l.Name)
 				l.sslCerts = append(l.sslCerts, cert)
@@ -210,19 +205,14 @@ func (l *L7) compareCerts(certLinks []string) bool {
 		glog.V(4).Infof("Loadbalancer has %d certs, target proxy has %d certs", len(certsMap), len(certLinks))
 		return false
 	}
-
-	for _, link := range certLinks {
-		certName, err := utils.KeyName(link)
-		if err != nil {
-			glog.Warningf("Cannot get cert name from URL: %v", link)
-			return false
-		}
-
+	var certName string
+	for _, linkName := range certLinks {
+		certName = getResourceNameFromLink(linkName)
 		if cert, ok := certsMap[certName]; !ok {
 			glog.V(4).Infof("Cannot find cert with name %s in certsMap %+v", certName, certsMap)
 			return false
-		} else if ok && !utils.EqualResourceID(link, cert.SelfLink) {
-			glog.V(4).Infof("Selflink compare failed for certs - %s in loadbalancer, %s in targetproxy", cert.SelfLink, link)
+		} else if ok && !utils.CompareLinks(linkName, cert.SelfLink) {
+			glog.V(4).Infof("Selflink compare failed for certs - %s in loadbalancer, %s in targetproxy", cert.SelfLink, linkName)
 			return false
 		}
 	}