diff --git a/pkg/backends/backends.go b/pkg/backends/backends.go index f107d9728b..862aa30b2b 100644 --- a/pkg/backends/backends.go +++ b/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]) +} diff --git a/pkg/backends/backends_test.go b/pkg/backends/backends_test.go index f764bf9d55..98cc3fd3e0 100644 --- a/pkg/backends/backends_test.go +++ b/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)) } } } diff --git a/pkg/healthchecks/fakes.go b/pkg/healthchecks/fakes.go index 0e79acf67f..4ada4cd252 100644 --- a/pkg/healthchecks/fakes.go +++ b/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,8 +77,8 @@ 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 } @@ -88,8 +86,8 @@ func (f *FakeHealthCheckProvider) CreateHealthCheck(hc *compute.HealthCheck) err // 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 } diff --git a/pkg/instances/fakes.go b/pkg/instances/fakes.go index ab49cf7e3b..f388661a3b 100644 --- a/pkg/instances/fakes.go +++ b/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 diff --git a/pkg/instances/instances.go b/pkg/instances/instances.go index 2323c34813..5e43bb5492 100644 --- a/pkg/instances/instances.go +++ b/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 diff --git a/pkg/loadbalancers/certificates.go b/pkg/loadbalancers/certificates.go index 964668728a..5f4ed082d0 100644 --- a/pkg/loadbalancers/certificates.go +++ b/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 } } diff --git a/pkg/loadbalancers/fakes.go b/pkg/loadbalancers/fakes.go index 77df351e12..a339d81af6 100644 --- a/pkg/loadbalancers/fakes.go +++ b/pkg/loadbalancers/fakes.go @@ -23,8 +23,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" ) @@ -136,7 +134,7 @@ func (f *FakeLoadBalancers) CreateGlobalForwardingRule(rule *compute.ForwardingR if rule.IPAddress == "" { rule.IPAddress = fmt.Sprintf(testIPManager.ip()) } - rule.SelfLink = cloud.NewGlobalForwardingRulesResourceID("mock-project", rule.Name).SelfLink(meta.VersionGA) + rule.SelfLink = rule.Name f.Fw = append(f.Fw, rule) return nil } @@ -198,7 +196,7 @@ func (f *FakeLoadBalancers) GetUrlMap(name string) (*compute.UrlMap, error) { func (f *FakeLoadBalancers) CreateUrlMap(urlMap *compute.UrlMap) error { glog.V(4).Infof("CreateUrlMap %+v", urlMap) f.calls = append(f.calls, "CreateUrlMap") - urlMap.SelfLink = cloud.NewUrlMapsResourceID("mock-project", urlMap.Name).SelfLink(meta.VersionGA) + urlMap.SelfLink = urlMap.Name f.Um = append(f.Um, urlMap) return nil } @@ -254,7 +252,7 @@ func (f *FakeLoadBalancers) GetTargetHttpProxy(name string) (*compute.TargetHttp // CreateTargetHttpProxy fakes creating a target http proxy. func (f *FakeLoadBalancers) CreateTargetHttpProxy(proxy *compute.TargetHttpProxy) error { f.calls = append(f.calls, "CreateTargetHttpProxy") - proxy.SelfLink = cloud.NewTargetHttpProxiesResourceID("mock-project", proxy.Name).SelfLink(meta.VersionGA) + proxy.SelfLink = proxy.Name f.Tp = append(f.Tp, proxy) return nil } @@ -303,7 +301,7 @@ func (f *FakeLoadBalancers) GetTargetHttpsProxy(name string) (*compute.TargetHtt // CreateTargetHttpsProxy fakes creating a target http proxy. func (f *FakeLoadBalancers) CreateTargetHttpsProxy(proxy *compute.TargetHttpsProxy) error { f.calls = append(f.calls, "CreateTargetHttpsProxy") - proxy.SelfLink = cloud.NewTargetHttpProxiesResourceID("mock-project", proxy.Name).SelfLink(meta.VersionGA) + proxy.SelfLink = proxy.Name f.Tps = append(f.Tps, proxy) return nil } @@ -461,7 +459,7 @@ func (f *FakeLoadBalancers) ListSslCertificates() ([]*compute.SslCertificate, er // CreateSslCertificate fakes out certificate creation. func (f *FakeLoadBalancers) CreateSslCertificate(cert *compute.SslCertificate) (*compute.SslCertificate, error) { f.calls = append(f.calls, "CreateSslCertificate") - cert.SelfLink = cloud.NewSslCertificatesResourceID("mock-project", cert.Name).SelfLink(meta.VersionGA) + cert.SelfLink = cert.Name if len(f.Certs) == TargetProxyCertLimit { // Simulate cert creation failure return nil, fmt.Errorf("Unable to create cert, Exceeded cert limit of %d.", TargetProxyCertLimit) diff --git a/pkg/loadbalancers/forwarding_rules.go b/pkg/loadbalancers/forwarding_rules.go index 2104c99be1..6b951627cd 100644 --- a/pkg/loadbalancers/forwarding_rules.go +++ b/pkg/loadbalancers/forwarding_rules.go @@ -18,6 +18,7 @@ package loadbalancers import ( "fmt" + "strings" "github.com/golang/glog" compute "google.golang.org/api/compute/v1" @@ -69,7 +70,8 @@ func (l *L7) checkForwardingRule(name, proxyLink, ip, portRange string) (fw *com fw = nil } if fw == nil { - glog.V(3).Infof("Creating forwarding rule for proxy %q and ip %v:%v", proxyLink, ip, portRange) + parts := strings.Split(proxyLink, "/") + glog.V(3).Infof("Creating forwarding rule for proxy %v and ip %v:%v", parts[len(parts)-1:], ip, portRange) rule := &compute.ForwardingRule{ Name: name, IPAddress: ip, @@ -86,7 +88,7 @@ func (l *L7) checkForwardingRule(name, proxyLink, ip, portRange string) (fw *com } } // TODO: If the port range and protocol don't match, recreate the rule - if utils.EqualResourceID(fw.Target, proxyLink) { + if utils.CompareLinks(fw.Target, proxyLink) { glog.V(4).Infof("Forwarding rule %v already exists", fw.Name) } else { glog.V(3).Infof("Forwarding rule %v has the wrong proxy, setting %v overwriting %v", diff --git a/pkg/loadbalancers/l7.go b/pkg/loadbalancers/l7.go index 7692a252a7..2aafae80c8 100644 --- a/pkg/loadbalancers/l7.go +++ b/pkg/loadbalancers/l7.go @@ -118,6 +118,24 @@ func (l *L7) UrlMap() *compute.UrlMap { return l.um } +// Returns the name portion of a link - which is the last section +func getResourceNameFromLink(link string) string { + s := strings.Split(link, "/") + if len(s) == 0 { + return "" + } + return s[len(s)-1] +} + +func (l *L7) getSslCertLinkInUse() []string { + proxyName := l.namer.TargetProxy(l.Name, utils.HTTPSProtocol) + proxy, _ := l.cloud.GetTargetHttpsProxy(proxyName) + if proxy != nil && len(proxy.SslCertificates) > 0 { + return proxy.SslCertificates + } + return nil +} + func (l *L7) edgeHop() error { if err := l.assertUrlMapExists(); err != nil { return err diff --git a/pkg/loadbalancers/loadbalancers_test.go b/pkg/loadbalancers/loadbalancers_test.go index 9f10f04e10..ca4989aa2c 100644 --- a/pkg/loadbalancers/loadbalancers_test.go +++ b/pkg/loadbalancers/loadbalancers_test.go @@ -548,11 +548,10 @@ func verifyProxyCertsInOrder(hostname string, f *FakeLoadBalancers, t *testing.T count := 0 tmp := "" - for _, link := range tps.SslCertificates { - certName, _ := utils.KeyName(link) - cert, err := f.GetSslCertificate(certName) + for _, linkName := range tps.SslCertificates { + cert, err := f.GetSslCertificate(getResourceNameFromLink(linkName)) if err != nil { - t.Fatalf("Failed to fetch certificate from link %s - %v", link, err) + t.Fatalf("Failed to fetch certificate from link %s - %v", linkName, err) } if strings.HasSuffix(cert.Certificate, hostname) { // cert contents will be of the form "cert- ", we want the certs with the smaller number @@ -603,9 +602,8 @@ func verifyCertAndProxyLink(expectCerts map[string]string, expectCertsProxy map[ if len(tps.SslCertificates) != len(expectCertsProxy) { t.Fatalf("Expected https proxy to have %d certs, actual %d", len(expectCertsProxy), len(tps.SslCertificates)) } - for _, link := range tps.SslCertificates { - certName, _ := utils.KeyName(link) - if _, ok := expectCerts[certName]; !ok { + for _, linkName := range tps.SslCertificates { + if _, ok := expectCerts[getResourceNameFromLink(linkName)]; !ok { t.Fatalf("unexpected ssl certificate linked in target proxy; Expected : %v; Target Proxy Certs: %v", expectCertsProxy, tps.SslCertificates) } diff --git a/pkg/loadbalancers/target_proxies.go b/pkg/loadbalancers/target_proxies.go index 504a3ac572..98b3384b5f 100644 --- a/pkg/loadbalancers/target_proxies.go +++ b/pkg/loadbalancers/target_proxies.go @@ -51,7 +51,7 @@ func (l *L7) checkProxy() (err error) { l.tp = proxy return nil } - if !utils.EqualResourceID(proxy.UrlMap, l.um.SelfLink) { + if !utils.CompareLinks(proxy.UrlMap, l.um.SelfLink) { glog.V(3).Infof("Proxy %v has the wrong url map, setting %v overwriting %v", proxy.Name, l.um, proxy.UrlMap) if err := l.cloud.SetUrlMapForTargetHttpProxy(proxy, l.um); err != nil { @@ -96,7 +96,7 @@ func (l *L7) checkHttpsProxy() (err error) { l.tps = proxy return nil } - if !utils.EqualResourceID(proxy.UrlMap, l.um.SelfLink) { + if !utils.CompareLinks(proxy.UrlMap, l.um.SelfLink) { glog.V(3).Infof("Https proxy %v has the wrong url map, setting %v overwriting %v", proxy.Name, l.um, proxy.UrlMap) if err := l.cloud.SetUrlMapForTargetHttpsProxy(proxy, l.um); err != nil { @@ -119,12 +119,3 @@ func (l *L7) checkHttpsProxy() (err error) { l.tps = proxy return nil } - -func (l *L7) getSslCertLinkInUse() []string { - proxyName := l.namer.TargetProxy(l.Name, utils.HTTPSProtocol) - proxy, _ := l.cloud.GetTargetHttpsProxy(proxyName) - if proxy != nil && len(proxy.SslCertificates) > 0 { - return proxy.SslCertificates - } - return nil -} diff --git a/pkg/loadbalancers/url_maps.go b/pkg/loadbalancers/url_maps.go index 739fca1188..04a792728a 100644 --- a/pkg/loadbalancers/url_maps.go +++ b/pkg/loadbalancers/url_maps.go @@ -205,7 +205,7 @@ func (l *L7) getBackendNames() []string { } func mapsEqual(a, b *compute.UrlMap) bool { - if !utils.EqualResourcePaths(a.DefaultService, b.DefaultService) { + if utils.BackendServiceComparablePath(a.DefaultService) != utils.BackendServiceComparablePath(b.DefaultService) { return false } if len(a.HostRules) != len(b.HostRules) { @@ -235,7 +235,7 @@ func mapsEqual(a, b *compute.UrlMap) bool { for i := range a.PathMatchers { a := a.PathMatchers[i] b := b.PathMatchers[i] - if !utils.EqualResourcePaths(a.DefaultService, b.DefaultService) { + if utils.BackendServiceComparablePath(a.DefaultService) != utils.BackendServiceComparablePath(b.DefaultService) { return false } if a.Description != b.Description { @@ -260,7 +260,7 @@ func mapsEqual(a, b *compute.UrlMap) bool { } // Trim down the url's for a.Service and b.Service to a comparable structure // We do this because we update the UrlMap with relative links (not full) to backends. - if !utils.EqualResourcePaths(a.Service, b.Service) { + if utils.BackendServiceComparablePath(a.Service) != utils.BackendServiceComparablePath(b.Service) { return false } } diff --git a/pkg/neg/fakes.go b/pkg/neg/fakes.go index 1c357a1092..17c677406a 100644 --- a/pkg/neg/fakes.go +++ b/pkg/neg/fakes.go @@ -23,8 +23,6 @@ import ( computealpha "google.golang.org/api/compute/v0.alpha" "k8s.io/apimachinery/pkg/util/sets" - "k8s.io/kubernetes/pkg/cloudprovider/providers/gce/cloud" - "k8s.io/kubernetes/pkg/cloudprovider/providers/gce/cloud/meta" ) const ( @@ -84,10 +82,10 @@ func NewFakeNetworkEndpointGroupCloud(subnetwork, network string) networkEndpoin var NotFoundError = fmt.Errorf("Not Found") -func (f *FakeNetworkEndpointGroupCloud) GetNetworkEndpointGroup(name string, zone string) (*computealpha.NetworkEndpointGroup, error) { - f.mu.Lock() - defer f.mu.Unlock() - negs, ok := f.NetworkEndpointGroups[zone] +func (cloud *FakeNetworkEndpointGroupCloud) GetNetworkEndpointGroup(name string, zone string) (*computealpha.NetworkEndpointGroup, error) { + cloud.mu.Lock() + defer cloud.mu.Unlock() + negs, ok := cloud.NetworkEndpointGroups[zone] if ok { for _, neg := range negs { if neg.Name == name { @@ -102,35 +100,35 @@ func networkEndpointKey(name, zone string) string { return fmt.Sprintf("%s-%s", zone, name) } -func (f *FakeNetworkEndpointGroupCloud) ListNetworkEndpointGroup(zone string) ([]*computealpha.NetworkEndpointGroup, error) { - f.mu.Lock() - defer f.mu.Unlock() - return f.NetworkEndpointGroups[zone], nil +func (cloud *FakeNetworkEndpointGroupCloud) ListNetworkEndpointGroup(zone string) ([]*computealpha.NetworkEndpointGroup, error) { + cloud.mu.Lock() + defer cloud.mu.Unlock() + return cloud.NetworkEndpointGroups[zone], nil } -func (f *FakeNetworkEndpointGroupCloud) AggregatedListNetworkEndpointGroup() (map[string][]*computealpha.NetworkEndpointGroup, error) { - f.mu.Lock() - defer f.mu.Unlock() - return f.NetworkEndpointGroups, nil +func (cloud *FakeNetworkEndpointGroupCloud) AggregatedListNetworkEndpointGroup() (map[string][]*computealpha.NetworkEndpointGroup, error) { + cloud.mu.Lock() + defer cloud.mu.Unlock() + return cloud.NetworkEndpointGroups, nil } -func (f *FakeNetworkEndpointGroupCloud) CreateNetworkEndpointGroup(neg *computealpha.NetworkEndpointGroup, zone string) error { - f.mu.Lock() - defer f.mu.Unlock() - neg.SelfLink = cloud.NewNetworkEndpointGroupsResourceID("mock-project", zone, neg.Name).SelfLink(meta.VersionAlpha) - if _, ok := f.NetworkEndpointGroups[zone]; !ok { - f.NetworkEndpointGroups[zone] = []*computealpha.NetworkEndpointGroup{} +func (cloud *FakeNetworkEndpointGroupCloud) CreateNetworkEndpointGroup(neg *computealpha.NetworkEndpointGroup, zone string) error { + cloud.mu.Lock() + defer cloud.mu.Unlock() + neg.SelfLink = fmt.Sprintf("https://fake.google.com/compute/%s/NetworkEndpointGroup/%s", zone, neg.Name) + if _, ok := cloud.NetworkEndpointGroups[zone]; !ok { + cloud.NetworkEndpointGroups[zone] = []*computealpha.NetworkEndpointGroup{} } - f.NetworkEndpointGroups[zone] = append(f.NetworkEndpointGroups[zone], neg) - f.NetworkEndpoints[networkEndpointKey(neg.Name, zone)] = []*computealpha.NetworkEndpoint{} + cloud.NetworkEndpointGroups[zone] = append(cloud.NetworkEndpointGroups[zone], neg) + cloud.NetworkEndpoints[networkEndpointKey(neg.Name, zone)] = []*computealpha.NetworkEndpoint{} return nil } -func (f *FakeNetworkEndpointGroupCloud) DeleteNetworkEndpointGroup(name string, zone string) error { - f.mu.Lock() - defer f.mu.Unlock() - delete(f.NetworkEndpoints, networkEndpointKey(name, zone)) - negs := f.NetworkEndpointGroups[zone] +func (cloud *FakeNetworkEndpointGroupCloud) DeleteNetworkEndpointGroup(name string, zone string) error { + cloud.mu.Lock() + defer cloud.mu.Unlock() + delete(cloud.NetworkEndpoints, networkEndpointKey(name, zone)) + negs := cloud.NetworkEndpointGroups[zone] newList := []*computealpha.NetworkEndpointGroup{} found := false for _, neg := range negs { @@ -143,22 +141,22 @@ func (f *FakeNetworkEndpointGroupCloud) DeleteNetworkEndpointGroup(name string, if !found { return NotFoundError } - f.NetworkEndpointGroups[zone] = newList + cloud.NetworkEndpointGroups[zone] = newList return nil } -func (f *FakeNetworkEndpointGroupCloud) AttachNetworkEndpoints(name, zone string, endpoints []*computealpha.NetworkEndpoint) error { - f.mu.Lock() - defer f.mu.Unlock() - f.NetworkEndpoints[networkEndpointKey(name, zone)] = append(f.NetworkEndpoints[networkEndpointKey(name, zone)], endpoints...) +func (cloud *FakeNetworkEndpointGroupCloud) AttachNetworkEndpoints(name, zone string, endpoints []*computealpha.NetworkEndpoint) error { + cloud.mu.Lock() + defer cloud.mu.Unlock() + cloud.NetworkEndpoints[networkEndpointKey(name, zone)] = append(cloud.NetworkEndpoints[networkEndpointKey(name, zone)], endpoints...) return nil } -func (f *FakeNetworkEndpointGroupCloud) DetachNetworkEndpoints(name, zone string, endpoints []*computealpha.NetworkEndpoint) error { - f.mu.Lock() - defer f.mu.Unlock() +func (cloud *FakeNetworkEndpointGroupCloud) DetachNetworkEndpoints(name, zone string, endpoints []*computealpha.NetworkEndpoint) error { + cloud.mu.Lock() + defer cloud.mu.Unlock() newList := []*computealpha.NetworkEndpoint{} - for _, ne := range f.NetworkEndpoints[networkEndpointKey(name, zone)] { + for _, ne := range cloud.NetworkEndpoints[networkEndpointKey(name, zone)] { found := false for _, remove := range endpoints { if reflect.DeepEqual(*ne, *remove) { @@ -171,15 +169,15 @@ func (f *FakeNetworkEndpointGroupCloud) DetachNetworkEndpoints(name, zone string } newList = append(newList, ne) } - f.NetworkEndpoints[networkEndpointKey(name, zone)] = newList + cloud.NetworkEndpoints[networkEndpointKey(name, zone)] = newList return nil } -func (f *FakeNetworkEndpointGroupCloud) ListNetworkEndpoints(name, zone string, showHealthStatus bool) ([]*computealpha.NetworkEndpointWithHealthStatus, error) { - f.mu.Lock() - defer f.mu.Unlock() +func (cloud *FakeNetworkEndpointGroupCloud) ListNetworkEndpoints(name, zone string, showHealthStatus bool) ([]*computealpha.NetworkEndpointWithHealthStatus, error) { + cloud.mu.Lock() + defer cloud.mu.Unlock() ret := []*computealpha.NetworkEndpointWithHealthStatus{} - nes, ok := f.NetworkEndpoints[networkEndpointKey(name, zone)] + nes, ok := cloud.NetworkEndpoints[networkEndpointKey(name, zone)] if !ok { return nil, NotFoundError } @@ -189,10 +187,10 @@ func (f *FakeNetworkEndpointGroupCloud) ListNetworkEndpoints(name, zone string, return ret, nil } -func (f *FakeNetworkEndpointGroupCloud) NetworkURL() string { - return f.Network +func (cloud *FakeNetworkEndpointGroupCloud) NetworkURL() string { + return cloud.Network } -func (f *FakeNetworkEndpointGroupCloud) SubnetworkURL() string { - return f.Subnetwork +func (cloud *FakeNetworkEndpointGroupCloud) SubnetworkURL() string { + return cloud.Subnetwork } diff --git a/pkg/neg/syncer.go b/pkg/neg/syncer.go index 10ebfc7c6e..0c054f9fb8 100644 --- a/pkg/neg/syncer.go +++ b/pkg/neg/syncer.go @@ -33,7 +33,6 @@ import ( "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/tools/cache" "k8s.io/client-go/tools/record" - "k8s.io/ingress-gce/pkg/utils" "k8s.io/kubernetes/pkg/cloudprovider/providers/gce" ) @@ -253,8 +252,8 @@ func (s *syncer) ensureNetworkEndpointGroups() error { needToCreate := false if neg == nil { needToCreate = true - } else if !utils.EqualResourceID(neg.LoadBalancer.Network, s.cloud.NetworkURL()) || - !utils.EqualResourceID(neg.LoadBalancer.Subnetwork, s.cloud.SubnetworkURL()) { + } else if retrieveName(neg.LoadBalancer.Network) != retrieveName(s.cloud.NetworkURL()) || + retrieveName(neg.LoadBalancer.Subnetwork) != retrieveName(s.cloud.SubnetworkURL()) { // Only compare network and subnetwork names to avoid api endpoint differences that cause deleting NEG accidentally. // TODO: change to compare network/subnetwork url instead of name when NEG API reach GA. needToCreate = true @@ -509,6 +508,11 @@ func calculateDifference(targetMap, currentMap map[string]sets.String) (map[stri return addSet, removeSet } +func retrieveName(url string) string { + strs := strings.Split(url, "/") + return strs[len(strs)-1] +} + // getService retrieves service object from serviceLister based on the input namespace and name func getService(serviceLister cache.Indexer, namespace, name string) *apiv1.Service { service, exists, err := serviceLister.GetByKey(serviceKeyFunc(namespace, name)) diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go index dcf9bb05d1..d7afb4d1e0 100644 --- a/pkg/utils/utils.go +++ b/pkg/utils/utils.go @@ -26,7 +26,6 @@ import ( compute "google.golang.org/api/compute/v1" "google.golang.org/api/googleapi" "k8s.io/apimachinery/pkg/types" - "k8s.io/kubernetes/pkg/cloudprovider/providers/gce/cloud" ) const ( @@ -107,6 +106,16 @@ func IsForbiddenError(err error) bool { return IsHTTPErrorCode(err, http.StatusForbidden) } +// CompareLinks returns true if the 2 self links are equal. +func CompareLinks(l1, l2 string) bool { + // TODO: These can be partial links + return l1 == l2 && l1 != "" +} + +// PrimitivePathMap is a convenience type used by multiple submodules +// that share the same testing methods. +type PrimitivePathMap map[string]map[string]string + // trimFieldsEvenly trims the fields evenly and keeps the total length // <= max. Truncation is spread in ratio with their original length, // meaning smaller fields will be truncated less than longer ones. @@ -163,67 +172,15 @@ func BackendServiceRelativeResourcePath(name string) string { return fmt.Sprintf("global/backendServices/%v", name) } -// KeyName returns the name portion from a full or partial GCP resource URL. -// Example: -// https://googleapis.com/v1/compute/projects/my-project/global/backendServices/my-backend -// Output: my-backend -func KeyName(url string) (string, error) { - id, err := cloud.ParseResourceURL(url) - if err != nil { - return "", err +// BackendServiceComparablePath trims project and compute version from the SelfLink +// for a global BackendService. +// global/backendServices/[BACKEND_SERVICE_NAME] +func BackendServiceComparablePath(url string) string { + path_parts := strings.Split(url, "global/") + if len(path_parts) != 2 { + return "" } - - if id.Key == nil { - // Resource is projects - return id.ProjectID, nil - } - - return id.Key.Name, nil -} - -// ResourcePath returns the location, resource and name portion from a -// full or partial GCP resource URL. This removes the endpoint prefix, version, and project. -// Example: -// https://googleapis.com/v1/compute/projects/my-project/global/backendServices/my-backend -// Output: global/backendServices/my-backend -func ResourcePath(url string) (string, error) { - resID, err := cloud.ParseResourceURL(url) - if err != nil { - return "", err - } - return resID.ResourcePath(), nil -} - -// EqualResourcePaths returns true if a and b have equal ResourcePaths. Resource paths -// entail the location, resource type, and resource name. -func EqualResourcePaths(a, b string) bool { - aPath, err := ResourcePath(a) - if err != nil { - return false - } - - bPath, err := ResourcePath(b) - if err != nil { - return false - } - - return aPath == bPath -} - -// EqualResourceID returns true if a and b have equal ResourceNames. Relative resource names -// entail the project, location, resource type, and resource name. -func EqualResourceID(a, b string) bool { - aId, err := cloud.ParseResourceURL(a) - if err != nil { - return false - } - - bId, err := cloud.ParseResourceURL(b) - if err != nil { - return false - } - - return aId.Equal(bId) + return fmt.Sprintf("global/%s", path_parts[1]) } // IGLinks returns a list of links extracted from the passed in list of diff --git a/pkg/utils/utils_test.go b/pkg/utils/utils_test.go index 7707016753..84cf76b082 100644 --- a/pkg/utils/utils_test.go +++ b/pkg/utils/utils_test.go @@ -84,7 +84,7 @@ func TestTrimFieldsEvenly(t *testing.T) { for i := range res { totalLen += len(res[i]) if res[i] != tc.expect[i] { - t.Errorf("%s: the %d field is want to be %q, but got %q", tc.desc, i, tc.expect[i], res[i]) + t.Errorf("%s: the %d field is expected to be %q, but got %q", tc.desc, i, tc.expect[i], res[i]) } } @@ -94,10 +94,10 @@ func TestTrimFieldsEvenly(t *testing.T) { } } -func TestResourcePathOfURL(t *testing.T) { +func TestBackendServiceComparablePath(t *testing.T) { testCases := []struct { - url string - want string + url string + expected string }{ { "global/backendServices/foo", @@ -108,15 +108,15 @@ func TestResourcePathOfURL(t *testing.T) { "global/backendServices/foo", }, { - "https://www.googleapis.com/compute/v1/projects/foo/BAD-INPUT/zones/us-central1-c/backendServices/foo", + "https://www.googleapis.com/compute/v1/projects/foo/zones/us-central1-c/backendServices/foo", "", }, } for _, tc := range testCases { - res, _ := ResourcePath(tc.url) - if res != tc.want { - t.Errorf("ResourcePath(%q) = %q, want %q", tc.url, res, tc.want) + res := BackendServiceComparablePath(tc.url) + if res != tc.expected { + t.Errorf("Expected result after url trim to be %v, but got %v", tc.expected, res) } } } @@ -156,117 +156,3 @@ func TestToNamespacedName(t *testing.T) { }) } } - -func TestEqualResourcePaths(t *testing.T) { - testCases := map[string]struct { - a string - b string - want bool - }{ - "partial vs full": { - a: "https://www.googleapis.com/compute/beta/projects/project-id/zones/us-central1-a/instanceGroups/example-group", - b: "zones/us-central1-a/instanceGroups/example-group", - want: true, - }, - "full vs full": { - a: "https://www.googleapis.com/compute/beta/projects/project-id/zones/us-central1-a/instanceGroups/example-group", - b: "https://www.googleapis.com/compute/beta/projects/project-id/zones/us-central1-a/instanceGroups/example-group", - want: true, - }, - "diff projects and versions": { - a: "https://www.googleapis.com/compute/v1/projects/project-A/zones/us-central1-a/instanceGroups/example-group", - b: "https://www.googleapis.com/compute/beta/projects/project-B/zones/us-central1-a/instanceGroups/example-group", - want: true, - }, - "diff name": { - a: "https://www.googleapis.com/compute/v1/projects/project-A/zones/us-central1-a/instanceGroups/example-groupA", - b: "https://www.googleapis.com/compute/beta/projects/project-B/zones/us-central1-a/instanceGroups/example-groupB", - want: false, - }, - "diff location": { - a: "https://www.googleapis.com/compute/v1/projects/project-A/zones/us-central1-a/instanceGroups/example-group", - b: "https://www.googleapis.com/compute/beta/projects/project-B/zones/us-central1-b/instanceGroups/example-group", - want: false, - }, - "diff resource": { - a: "https://www.googleapis.com/compute/v1/projects/project-A/zones/us-central1-a/backendServices/example-group", - b: "https://www.googleapis.com/compute/beta/projects/project-B/zones/us-central1-b/instanceGroups/example-group", - want: false, - }, - "bad input a": { - a: "/project-A/zones/us-central1-a/backendServices/example-group", - b: "https://www.googleapis.com/compute/beta/projects/project-B/zones/us-central1-b/instanceGroups/example-group", - want: false, - }, - "bad input b": { - a: "https://www.googleapis.com/compute/beta/projects/project-B/zones/us-central1-b/instanceGroups/example-group", - b: "/project-A/zones/us-central1-a/backendServices/example-group", - want: false, - }, - } - - for name, tc := range testCases { - t.Run(name, func(t *testing.T) { - if got := EqualResourcePaths(tc.a, tc.b); got != tc.want { - t.Errorf("EqualResourcePathsOfURLs(%q, %q) = %v, want %v", tc.a, tc.b, got, tc.want) - } - }) - } -} - -func TestEqualResourceID(t *testing.T) { - testCases := map[string]struct { - a string - b string - want bool - }{ - "partial vs full": { - a: "https://www.googleapis.com/compute/beta/projects/project-id/zones/us-central1-a/instanceGroups/example-group", - b: "projects/project-id/zones/us-central1-a/instanceGroups/example-group", - want: true, - }, - "full vs full": { - a: "https://www.googleapis.com/compute/beta/projects/project-id/zones/us-central1-a/instanceGroups/example-group", - b: "https://www.googleapis.com/compute/beta/projects/project-id/zones/us-central1-a/instanceGroups/example-group", - want: true, - }, - "diff versions": { - a: "https://www.googleapis.com/compute/v1/projects/project-A/zones/us-central1-a/instanceGroups/example-group", - b: "https://www.googleapis.com/compute/beta/projects/project-A/zones/us-central1-a/instanceGroups/example-group", - want: true, - }, - "diff name": { - a: "https://www.googleapis.com/compute/v1/projects/project-A/zones/us-central1-a/instanceGroups/example-groupA", - b: "https://www.googleapis.com/compute/beta/projects/project-B/zones/us-central1-a/instanceGroups/example-groupB", - want: false, - }, - "diff location": { - a: "https://www.googleapis.com/compute/v1/projects/project-A/zones/us-central1-a/instanceGroups/example-group", - b: "https://www.googleapis.com/compute/beta/projects/project-B/zones/us-central1-b/instanceGroups/example-group", - want: false, - }, - "diff resource": { - a: "https://www.googleapis.com/compute/v1/projects/project-A/zones/us-central1-a/backendServices/example-group", - b: "https://www.googleapis.com/compute/beta/projects/project-B/zones/us-central1-b/instanceGroups/example-group", - want: false, - }, - "bad input a": { - a: "/project-A/zones/us-central1-a/backendServices/example-group", - b: "https://www.googleapis.com/compute/beta/projects/project-B/zones/us-central1-b/instanceGroups/example-group", - want: false, - }, - "bad input b": { - a: "https://www.googleapis.com/compute/beta/projects/project-B/zones/us-central1-b/instanceGroups/example-group", - b: "/project-A/zones/us-central1-a/backendServices/example-group", - want: false, - }, - } - - for name, tc := range testCases { - t.Run(name, func(t *testing.T) { - if got := EqualResourceID(tc.a, tc.b); got != tc.want { - t.Errorf("EqualResourcePathsOfURLs(%q, %q) = %v, want %v", tc.a, tc.b, got, tc.want) - } - }) - } -}