From 73ed4346d31681690bc25603f8a75e5979cab1b7 Mon Sep 17 00:00:00 2001 From: Rohit Ramkumar Date: Fri, 31 Aug 2018 10:40:31 -0700 Subject: [PATCH] Refactor LoadBalancerPool to use cloud listing snapshotter --- pkg/backends/backends.go | 4 +- pkg/controller/controller.go | 24 ++--- pkg/controller/controller_test.go | 2 +- pkg/controller/types.go | 2 + pkg/loadbalancers/interfaces.go | 6 +- pkg/loadbalancers/l7.go | 93 +++++++++---------- pkg/loadbalancers/l7s.go | 90 ++++++++++-------- pkg/loadbalancers/loadbalancers_test.go | 118 ++++++++++++------------ pkg/utils/namer.go | 10 ++ pkg/utils/namer_test.go | 27 ++++++ 10 files changed, 211 insertions(+), 165 deletions(-) diff --git a/pkg/backends/backends.go b/pkg/backends/backends.go index 523e961a8c..d22d6f8509 100644 --- a/pkg/backends/backends.go +++ b/pkg/backends/backends.go @@ -193,8 +193,8 @@ func (b *Backends) List() ([]interface{}, error) { return nil, err } var ret []interface{} - for _, _ = range backends { - ret = append(ret, true) + for _, x := range backends { + ret = append(ret, x) } return ret, nil } diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 8722798a8b..c05076e641 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -109,7 +109,7 @@ func NewLoadBalancerController( hasSynced: ctx.HasSynced, nodes: NewNodeController(ctx, instancePool), instancePool: instancePool, - l7Pool: loadbalancers.NewLoadBalancerPool(ctx.Cloud, ctx.ClusterNamer), + l7Pool: loadbalancers.NewLoadBalancerPool(ctx.Cloud, ctx.ClusterNamer, true), backendSyncer: backends.NewBackendSyncer(backendPool, healthChecker, ctx.ClusterNamer, ctx.BackendConfigEnabled), negLinker: backends.NewNEGLinker(backendPool, ctx.Cloud, ctx.ClusterNamer), igLinker: backends.NewInstanceGroupLinker(instancePool, backendPool, ctx.ClusterNamer), @@ -369,9 +369,12 @@ func (lbc *LoadBalancerController) SyncLoadBalancer(state interface{}) error { } // Create higher-level LB resources. - if err := lbc.l7Pool.Sync(lb); err != nil { + l7, err := lbc.l7Pool.Sync(lb) + if err != nil { return err } + syncState.l7 = l7 + return nil } @@ -400,16 +403,12 @@ func (lbc *LoadBalancerController) PostProcess(state interface{}) error { // Get the loadbalancer and update the ingress status. ing := syncState.ing - k, err := utils.KeyFunc(ing) - if err != nil { - return fmt.Errorf("cannot get key for Ingress %s/%s: %v", ing.Namespace, ing.Name, err) - } - l7, err := lbc.l7Pool.Get(k) - if err != nil { - return fmt.Errorf("unable to get loadbalancer: %v", err) + if syncState.l7 == nil { + return fmt.Errorf("sync state does not contain L7 spec") } - if err := lbc.updateIngressStatus(l7, ing); err != nil { + + if err := lbc.updateIngressStatus(syncState.l7, ing); err != nil { return fmt.Errorf("update ingress status error: %v", err) } return nil @@ -449,7 +448,10 @@ func (lbc *LoadBalancerController) sync(key string) error { // Bootstrap state for GCP sync. urlMap, errs := lbc.Translator.TranslateIngress(ing, lbc.ctx.DefaultBackendSvcPortID) - syncState := &syncState{urlMap, ing} + syncState := &syncState{ + urlMap: urlMap, + ing: ing, + } if errs != nil { return fmt.Errorf("error while evaluating the ingress spec: %v", utils.JoinErrs(errs)) } diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index eaab146960..e91526c338 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -64,7 +64,7 @@ func newLoadBalancerController() *LoadBalancerController { lbc := NewLoadBalancerController(ctx, stopCh) // TODO(rramkumar): Fix this so we don't have to override with our fake lbc.instancePool = instances.NewNodePool(instances.NewFakeInstanceGroups(sets.NewString(), namer), namer) - lbc.l7Pool = loadbalancers.NewLoadBalancerPool(loadbalancers.NewFakeLoadBalancers(clusterUID, namer), namer) + lbc.l7Pool = loadbalancers.NewLoadBalancerPool(loadbalancers.NewFakeLoadBalancers(clusterUID, namer), namer, false) lbc.instancePool.Init(&instances.FakeZoneLister{Zones: []string{"zone-a"}}) lbc.hasSynced = func() bool { return true } diff --git a/pkg/controller/types.go b/pkg/controller/types.go index 52bc12cc3d..6a982d9acd 100644 --- a/pkg/controller/types.go +++ b/pkg/controller/types.go @@ -17,6 +17,7 @@ limitations under the License. package controller import ( + "k8s.io/ingress-gce/pkg/loadbalancers" "k8s.io/ingress-gce/pkg/utils" extensions "k8s.io/api/extensions/v1beta1" @@ -31,5 +32,6 @@ type gcState struct { // syncState is used by the controller to maintain state for routines that sync GCP resources of an Ingress. type syncState struct { urlMap *utils.GCEURLMap + l7 *loadbalancers.L7 ing *extensions.Ingress } diff --git a/pkg/loadbalancers/interfaces.go b/pkg/loadbalancers/interfaces.go index 0aaa0d4f74..bd59775f5c 100644 --- a/pkg/loadbalancers/interfaces.go +++ b/pkg/loadbalancers/interfaces.go @@ -68,10 +68,12 @@ type LoadBalancers interface { // LoadBalancerPool is an interface to manage the cloud resources associated // with a gce loadbalancer. +// TODO(rramkumar): Break up this interface into 2: Pool & Syncer. type LoadBalancerPool interface { - Get(name string) (*L7, error) + // Note: Get is currrently only used for testing. + Get(name string) bool Delete(name string) error - Sync(ri *L7RuntimeInfo) error + Sync(ri *L7RuntimeInfo) (*L7, error) GC(names []string) error Shutdown() error } diff --git a/pkg/loadbalancers/l7.go b/pkg/loadbalancers/l7.go index 50ff77c06b..213c3beae9 100644 --- a/pkg/loadbalancers/l7.go +++ b/pkg/loadbalancers/l7.go @@ -176,68 +176,59 @@ func (l *L7) GetIP() string { return "" } -// Cleanup deletes resources specific to this l7 in the right order. +// Cleanup deletes resources specific to this l7 in the right order given a base name. // forwarding rule -> target proxy -> url map // This leaves backends and health checks, which are shared across loadbalancers. -func (l *L7) Cleanup() error { - if l.fw != nil { - glog.V(2).Infof("Deleting global forwarding rule %v", l.fw.Name) - if err := utils.IgnoreHTTPNotFound(l.cloud.DeleteGlobalForwardingRule(l.fw.Name)); err != nil { - return err - } - l.fw = nil +func Cleanup(name string, cloud LoadBalancers, namer *utils.Namer) error { + fwName := namer.ForwardingRule(name, "HTTP") + glog.V(2).Infof("Deleting global forwarding rule %v", fwName) + if err := utils.IgnoreHTTPNotFound(cloud.DeleteGlobalForwardingRule(fwName)); err != nil { + return err } - if l.fws != nil { - glog.V(2).Infof("Deleting global forwarding rule %v", l.fws.Name) - if err := utils.IgnoreHTTPNotFound(l.cloud.DeleteGlobalForwardingRule(l.fws.Name)); err != nil { - return err - } - l.fws = nil + fwsName := namer.ForwardingRule(name, "HTTPS") + glog.V(2).Infof("Deleting global forwarding rule %v", fwsName) + if err := utils.IgnoreHTTPNotFound(cloud.DeleteGlobalForwardingRule(fwsName)); err != nil { + return err } - if l.ip != nil { - glog.V(2).Infof("Deleting static IP %v(%v)", l.ip.Name, l.ip.Address) - if err := utils.IgnoreHTTPNotFound(l.cloud.DeleteGlobalAddress(l.ip.Name)); err != nil { - return err - } - l.ip = nil + + staticIPName := fwName + glog.V(2).Infof("Deleting static IP %v", staticIPName) + if err := utils.IgnoreHTTPNotFound(cloud.DeleteGlobalAddress(staticIPName)); err != nil { + return err } - if l.tps != nil { - glog.V(2).Infof("Deleting target https proxy %v", l.tps.Name) - if err := utils.IgnoreHTTPNotFound(l.cloud.DeleteTargetHttpsProxy(l.tps.Name)); err != nil { - return err - } - l.tps = nil + + tpsName := namer.TargetProxy(name, "HTTPS") + glog.V(2).Infof("Deleting target https proxy %v", tpsName) + if err := utils.IgnoreHTTPNotFound(cloud.DeleteTargetHttpsProxy(tpsName)); err != nil { + return err } - // Delete the SSL cert if it is from a secret, not referencing a pre-created GCE cert. - if len(l.sslCerts) != 0 && l.runtimeInfo.TLSName == "" { - var certErr error - for _, cert := range l.sslCerts { - glog.V(2).Infof("Deleting sslcert %s", cert.Name) - if err := utils.IgnoreHTTPNotFound(l.cloud.DeleteSslCertificate(cert.Name)); err != nil { - glog.Errorf("Old cert delete failed - %v", err) - certErr = err - } - } - l.sslCerts = nil - if certErr != nil { - return certErr - } + certs, err := cloud.ListSslCertificates() + if err != nil { + return err } - if l.tp != nil { - glog.V(2).Infof("Deleting target http proxy %v", l.tp.Name) - if err := utils.IgnoreHTTPNotFound(l.cloud.DeleteTargetHttpProxy(l.tp.Name)); err != nil { - return err + for _, c := range certs { + // Delete the SSL cert if it is from a secret, not referencing a pre-created GCE cert. + if namer.IsCertUsedForLB(name, c.Name) { + glog.V(2).Infof("Deleting sslcert %s", c.Name) + if err := utils.IgnoreHTTPNotFound(cloud.DeleteSslCertificate(c.Name)); err != nil { + return err + } } - l.tp = nil } - if l.um != nil { - glog.V(2).Infof("Deleting url map %v", l.um.Name) - if err := utils.IgnoreHTTPNotFound(l.cloud.DeleteUrlMap(l.um.Name)); err != nil { - return err - } - l.um = nil + + tpName := namer.TargetProxy(name, "HTTP") + glog.V(2).Infof("Deleting target http proxy %v", tpName) + if err := utils.IgnoreHTTPNotFound(cloud.DeleteTargetHttpProxy(tpName)); err != nil { + return err } + + umName := namer.UrlMap(name) + glog.V(2).Infof("Deleting url map %v", umName) + if err := utils.IgnoreHTTPNotFound(cloud.DeleteUrlMap(umName)); err != nil { + return err + } + return nil } diff --git a/pkg/loadbalancers/l7s.go b/pkg/loadbalancers/l7s.go index 0c1e54add2..6d960a4aba 100644 --- a/pkg/loadbalancers/l7s.go +++ b/pkg/loadbalancers/l7s.go @@ -18,9 +18,10 @@ package loadbalancers import ( "fmt" - "reflect" + "time" "github.com/golang/glog" + compute "google.golang.org/api/compute/v1" "k8s.io/apimachinery/pkg/util/sets" @@ -43,39 +44,45 @@ func (l *L7s) Namer() *utils.Namer { // NewLoadBalancerPool returns a new loadbalancer pool. // - cloud: implements LoadBalancers. Used to sync L7 loadbalancer resources // with the cloud. -func NewLoadBalancerPool(cloud LoadBalancers, namer *utils.Namer) LoadBalancerPool { - return &L7s{cloud, storage.NewInMemoryPool(), namer} +func NewLoadBalancerPool(cloud LoadBalancers, namer *utils.Namer, resyncWithCloud bool) LoadBalancerPool { + l7Pool := &L7s{ + cloud: cloud, + namer: namer, + } + if !resyncWithCloud { + l7Pool.snapshotter = storage.NewInMemoryPool() + } + keyFunc := func(i interface{}) (string, error) { + um := i.(*compute.UrlMap) + if !namer.NameBelongsToCluster(um.Name) { + return "", fmt.Errorf("unrecognized name %v", um.Name) + } + // Scrub out the UrlMap prefix of the name to get the base LB name. + return namer.ScrubUrlMapPrefix(um.Name), nil + } + l7Pool.snapshotter = storage.NewCloudListingPool("loadbalancers", keyFunc, l7Pool, 30*time.Second) + return l7Pool } -// Get returns the loadbalancer by name. -func (l *L7s) Get(name string) (*L7, error) { +// Get implements LoadBalancerPool. +// Note: This is currently only used for testing. +func (l *L7s) Get(name string) bool { name = l.namer.LoadBalancer(name) - lb, exists := l.snapshotter.Get(name) - if !exists { - return nil, fmt.Errorf("loadbalancer %v not in pool", name) - } - return lb.(*L7), nil + _, exists := l.snapshotter.Get(name) + return exists } -// Sync a load balancer with the given runtime info from the controller. -func (l *L7s) Sync(ri *L7RuntimeInfo) error { +// Sync implements LoadBalancerPool. +func (l *L7s) Sync(ri *L7RuntimeInfo) (*L7, error) { name := l.namer.LoadBalancer(ri.Name) - - lb, _ := l.Get(name) - if lb == nil { - glog.V(3).Infof("Creating l7 %v", name) - lb = &L7{ - runtimeInfo: ri, - Name: l.namer.LoadBalancer(ri.Name), - cloud: l.cloud, - namer: l.namer, - } - } else { - if !reflect.DeepEqual(lb.runtimeInfo, ri) { - glog.V(3).Infof("LB %v runtime info changed, old %+v new %+v", lb.Name, lb.runtimeInfo, ri) - lb.runtimeInfo = ri - } + glog.V(3).Infof("Sync: LB %s", name) + lb := &L7{ + runtimeInfo: ri, + Name: l.namer.LoadBalancer(ri.Name), + cloud: l.cloud, + namer: l.namer, } + // Add the lb to the pool, in case we create an UrlMap but run out // of quota in creating the ForwardingRule we still need to cleanup // the UrlMap during GC. @@ -86,28 +93,24 @@ func (l *L7s) Sync(ri *L7RuntimeInfo) error { // make it exist we need to create a collection of gce resources, done // through the edge hop. if err := lb.edgeHop(); err != nil { - return err + return lb, err } - return nil + return lb, nil } -// Delete deletes a load balancer by name. +// Delete implements LoadBalancerPool. func (l *L7s) Delete(name string) error { name = l.namer.LoadBalancer(name) - lb, err := l.Get(name) - if err != nil { - return err - } glog.V(3).Infof("Deleting lb %v", name) - if err := lb.Cleanup(); err != nil { + if err := Cleanup(name, l.cloud, l.namer); err != nil { return err } l.snapshotter.Delete(name) return nil } -// GC garbage collects loadbalancers not in the input list. +// GC implements LoadBalancerPool. func (l *L7s) GC(names []string) error { glog.V(4).Infof("GC(%v)", names) @@ -131,7 +134,7 @@ func (l *L7s) GC(names []string) error { return nil } -// Shutdown logs whether or not the pool is empty. +// Shutdown implemented LoadBalancerPool. func (l *L7s) Shutdown() error { if err := l.GC([]string{}); err != nil { return err @@ -139,3 +142,16 @@ func (l *L7s) Shutdown() error { glog.V(2).Infof("Loadbalancer pool shutdown.") return nil } + +// List lists all loadbalancers via listing all URLMap's. +func (l *L7s) List() ([]interface{}, error) { + urlMaps, err := l.cloud.ListUrlMaps() + if err != nil { + return nil, err + } + var ret []interface{} + for _, x := range urlMaps { + ret = append(ret, x) + } + return ret, nil +} diff --git a/pkg/loadbalancers/loadbalancers_test.go b/pkg/loadbalancers/loadbalancers_test.go index 32b2a468b5..d337e13f30 100644 --- a/pkg/loadbalancers/loadbalancers_test.go +++ b/pkg/loadbalancers/loadbalancers_test.go @@ -39,12 +39,12 @@ var ( testDefaultBeNodePort = utils.ServicePort{NodePort: 30000, Protocol: annotations.ProtocolHTTP} ) -func newFakeLoadBalancerPool(f LoadBalancers, t *testing.T, namer *utils.Namer) LoadBalancerPool { +func newFakeLoadBalancerPool(f LoadBalancers, t *testing.T, namer *utils.Namer, resyncWithCloud bool) LoadBalancerPool { fakeIGs := instances.NewFakeInstanceGroups(sets.NewString(), namer) nodePool := instances.NewNodePool(fakeIGs, namer) nodePool.Init(&instances.FakeZoneLister{Zones: []string{defaultZone}}) - return NewLoadBalancerPool(f, namer) + return NewLoadBalancerPool(f, namer, resyncWithCloud) } func TestCreateHTTPLoadBalancer(t *testing.T) { @@ -60,15 +60,14 @@ func TestCreateHTTPLoadBalancer(t *testing.T) { UrlMap: gceUrlMap, } f := NewFakeLoadBalancers(lbInfo.Name, namer) - pool := newFakeLoadBalancerPool(f, t, namer) + pool := newFakeLoadBalancerPool(f, t, namer, false) - if err := pool.Sync(lbInfo); err != nil { + if _, err := pool.Sync(lbInfo); err != nil { t.Fatalf("pool.Sync() = err %v", err) } - l7, err := pool.Get(lbInfo.Name) - if err != nil || l7 == nil { - t.Fatalf("Expected l7 not created, err: %v", err) + if !pool.Get(lbInfo.Name) { + t.Fatalf("Expected l7 not created") } verifyHTTPForwardingRuleAndProxyLinks(t, f) } @@ -87,14 +86,13 @@ func TestCreateHTTPSLoadBalancer(t *testing.T) { UrlMap: gceUrlMap, } f := NewFakeLoadBalancers(lbInfo.Name, namer) - pool := newFakeLoadBalancerPool(f, t, namer) + pool := newFakeLoadBalancerPool(f, t, namer, false) - if err := pool.Sync(lbInfo); err != nil { + if _, err := pool.Sync(lbInfo); err != nil { t.Fatalf("pool.Sync() = err %v", err) } - l7, err := pool.Get(lbInfo.Name) - if err != nil || l7 == nil { + if !pool.Get(lbInfo.Name) { t.Fatalf("Expected l7 not created") } verifyHTTPSForwardingRuleAndProxyLinks(t, f) @@ -159,10 +157,10 @@ func TestCertUpdate(t *testing.T) { } f := NewFakeLoadBalancers(lbInfo.Name, namer) - pool := newFakeLoadBalancerPool(f, t, namer) + pool := newFakeLoadBalancerPool(f, t, namer, false) // Sync first cert - if err := pool.Sync(lbInfo); err != nil { + if _, err := pool.Sync(lbInfo); err != nil { t.Fatalf("pool.Sync() = err %v", err) } @@ -173,7 +171,7 @@ func TestCertUpdate(t *testing.T) { // Sync with different cert lbInfo.TLS = []*TLSCerts{createCert("key2", "cert2", "name")} - if err := pool.Sync(lbInfo); err != nil { + if _, err := pool.Sync(lbInfo); err != nil { t.Fatalf("pool.Sync() = err %v", err) } expectCerts = map[string]string{certName2: lbInfo.TLS[0].Cert} @@ -198,10 +196,10 @@ func TestMultipleSecretsWithSameCert(t *testing.T) { UrlMap: gceUrlMap, } f := NewFakeLoadBalancers(lbInfo.Name, namer) - pool := newFakeLoadBalancerPool(f, t, namer) + pool := newFakeLoadBalancerPool(f, t, namer, false) // Sync first cert - if err := pool.Sync(lbInfo); err != nil { + if _, err := pool.Sync(lbInfo); err != nil { t.Fatalf("pool.Sync() = err %v", err) } certName := namer.SSLCertName(lbName, GetCertHash("cert")) @@ -226,7 +224,7 @@ func TestCertCreationWithCollision(t *testing.T) { UrlMap: gceUrlMap, } f := NewFakeLoadBalancers(lbInfo.Name, namer) - pool := newFakeLoadBalancerPool(f, t, namer) + pool := newFakeLoadBalancerPool(f, t, namer, false) // Have the same name used by orphaned cert // Since name of the cert is the same, the contents of Certificate have to be the same too, since name contains a @@ -238,7 +236,7 @@ func TestCertCreationWithCollision(t *testing.T) { }) // Sync first cert - if err := pool.Sync(lbInfo); err != nil { + if _, err := pool.Sync(lbInfo); err != nil { t.Fatalf("pool.Sync() = err %v", err) } @@ -255,7 +253,7 @@ func TestCertCreationWithCollision(t *testing.T) { // Sync with different cert lbInfo.TLS = []*TLSCerts{createCert("key2", "cert2", "name")} - if err := pool.Sync(lbInfo); err != nil { + if _, err := pool.Sync(lbInfo); err != nil { t.Fatalf("pool.Sync() = err %v", err) } expectCerts = map[string]string{certName2: "xyz"} @@ -287,7 +285,7 @@ func TestMultipleCertRetentionAfterRestart(t *testing.T) { expectCerts[certName1] = cert1.Cert f := NewFakeLoadBalancers(lbInfo.Name, namer) - firstPool := newFakeLoadBalancerPool(f, t, namer) + firstPool := newFakeLoadBalancerPool(f, t, namer, false) firstPool.Sync(lbInfo) verifyCertAndProxyLink(expectCerts, expectCerts, f, t) @@ -298,7 +296,7 @@ func TestMultipleCertRetentionAfterRestart(t *testing.T) { verifyCertAndProxyLink(expectCerts, expectCerts, f, t) // Restart of controller represented by a new pool - secondPool := newFakeLoadBalancerPool(f, t, namer) + secondPool := newFakeLoadBalancerPool(f, t, namer, false) secondPool.Sync(lbInfo) // Verify both certs are still present verifyCertAndProxyLink(expectCerts, expectCerts, f, t) @@ -329,7 +327,7 @@ func TestUpgradeToNewCertNames(t *testing.T) { lbInfo.TLS = []*TLSCerts{tlsCert} newCertName := namer.SSLCertName(lbName, tlsCert.CertHash) f := NewFakeLoadBalancers(lbInfo.Name, namer) - pool := newFakeLoadBalancerPool(f, t, namer) + pool := newFakeLoadBalancerPool(f, t, namer, false) // Manually create a target proxy and assign a legacy cert to it. sslCert := &compute.SslCertificate{Name: oldCertName, Certificate: "cert"} @@ -352,7 +350,7 @@ func TestUpgradeToNewCertNames(t *testing.T) { t.Fatalf("Expected cert with name %s, Got %s", oldCertName, proxyCerts[0].Name) } // Sync should replace this oldCert with one following the new naming scheme - if err := pool.Sync(lbInfo); err != nil { + if _, err := pool.Sync(lbInfo); err != nil { t.Fatalf("pool.Sync() = err %v", err) } // We expect to see only the new cert linked to the proxy and available in the load balancer. @@ -383,9 +381,9 @@ func TestMaxCertsUpload(t *testing.T) { UrlMap: gceUrlMap, } f := NewFakeLoadBalancers(lbInfo.Name, namer) - pool := newFakeLoadBalancerPool(f, t, namer) + pool := newFakeLoadBalancerPool(f, t, namer, false) - if err := pool.Sync(lbInfo); err != nil { + if _, err := pool.Sync(lbInfo); err != nil { t.Fatalf("pool.Sync() = err %v", err) } @@ -423,10 +421,10 @@ func TestIdenticalHostnameCerts(t *testing.T) { UrlMap: gceUrlMap, } f := NewFakeLoadBalancers(lbInfo.Name, namer) - pool := newFakeLoadBalancerPool(f, t, namer) + pool := newFakeLoadBalancerPool(f, t, namer, false) // Sync multiple times to make sure ordering is preserved for i := 0; i < 10; i++ { - if err := pool.Sync(lbInfo); err != nil { + if _, err := pool.Sync(lbInfo); err != nil { t.Fatalf("pool.Sync() = err %v", err) } verifyCertAndProxyLink(expectCerts, expectCerts, f, t) @@ -447,7 +445,7 @@ func TestIdenticalHostnameCertsPreShared(t *testing.T) { UrlMap: gceUrlMap, } f := NewFakeLoadBalancers(lbInfo.Name, namer) - pool := newFakeLoadBalancerPool(f, t, namer) + pool := newFakeLoadBalancerPool(f, t, namer, false) // Prepare pre-shared certs. preSharedCert1, _ := f.CreateSslCertificate(&compute.SslCertificate{ Name: "test-pre-shared-cert", @@ -470,7 +468,7 @@ func TestIdenticalHostnameCertsPreShared(t *testing.T) { lbInfo.TLSName = preSharedCert1.Name + "," + preSharedCert2.Name + "," + preSharedCert3.Name // Sync multiple times to make sure ordering is preserved for i := 0; i < 10; i++ { - if err := pool.Sync(lbInfo); err != nil { + if _, err := pool.Sync(lbInfo); err != nil { t.Fatalf("pool.Sync() = err %v", err) } verifyCertAndProxyLink(expectCerts, expectCerts, f, t) @@ -498,7 +496,7 @@ func TestPreSharedToSecretBasedCertUpdate(t *testing.T) { } f := NewFakeLoadBalancers(lbInfo.Name, namer) - pool := newFakeLoadBalancerPool(f, t, namer) + pool := newFakeLoadBalancerPool(f, t, namer, false) // Prepare pre-shared certs. preSharedCert1, _ := f.CreateSslCertificate(&compute.SslCertificate{ @@ -516,7 +514,7 @@ func TestPreSharedToSecretBasedCertUpdate(t *testing.T) { lbInfo.TLSName = preSharedCert1.Name + "," + preSharedCert2.Name // Sync pre-shared certs. - if err := pool.Sync(lbInfo); err != nil { + if _, err := pool.Sync(lbInfo); err != nil { t.Fatalf("pool.Sync() = err %v", err) } expectCerts := map[string]string{preSharedCert1.Name: preSharedCert1.Certificate, @@ -526,7 +524,7 @@ func TestPreSharedToSecretBasedCertUpdate(t *testing.T) { // Updates from pre-shared cert to secret based cert. lbInfo.TLS = []*TLSCerts{createCert("key", "cert", "name")} lbInfo.TLSName = "" - if err := pool.Sync(lbInfo); err != nil { + if _, err := pool.Sync(lbInfo); err != nil { t.Fatalf("pool.Sync() = err %v", err) } expectCerts[certName1] = lbInfo.TLS[0].Cert @@ -644,12 +642,11 @@ func TestCreateHTTPSLoadBalancerAnnotationCert(t *testing.T) { f.CreateSslCertificate(&compute.SslCertificate{ Name: tlsName, }) - pool := newFakeLoadBalancerPool(f, t, namer) - if err := pool.Sync(lbInfo); err != nil { + pool := newFakeLoadBalancerPool(f, t, namer, false) + if _, err := pool.Sync(lbInfo); err != nil { t.Fatalf("pool.Sync() = err %v", err) } - l7, err := pool.Get(lbInfo.Name) - if err != nil || l7 == nil { + if !pool.Get(lbInfo.Name) { t.Fatalf("Expected l7 not created") } verifyHTTPSForwardingRuleAndProxyLinks(t, f) @@ -670,13 +667,12 @@ func TestCreateBothLoadBalancers(t *testing.T) { UrlMap: gceUrlMap, } f := NewFakeLoadBalancers(lbInfo.Name, namer) - pool := newFakeLoadBalancerPool(f, t, namer) + pool := newFakeLoadBalancerPool(f, t, namer, false) - if err := pool.Sync(lbInfo); err != nil { + if _, err := pool.Sync(lbInfo); err != nil { t.Fatalf("pool.Sync() = err %v", err) } - l7, err := pool.Get(lbInfo.Name) - if err != nil || l7 == nil { + if !pool.Get(lbInfo.Name) { t.Fatalf("Expected l7 not created") } @@ -727,27 +723,25 @@ func TestUrlMapChange(t *testing.T) { lbInfo := &L7RuntimeInfo{Name: namer.LoadBalancer("test"), AllowHTTP: true, UrlMap: um1} f := NewFakeLoadBalancers(lbInfo.Name, namer) - pool := newFakeLoadBalancerPool(f, t, namer) - if err := pool.Sync(lbInfo); err != nil { + pool := newFakeLoadBalancerPool(f, t, namer, false) + if _, err := pool.Sync(lbInfo); err != nil { t.Fatalf("pool.Sync() = err %v", err) } - l7, err := pool.Get(lbInfo.Name) - if err != nil { - t.Fatalf("%v", err) + if !pool.Get(lbInfo.Name) { + t.Fatalf("Expected l7 not created") } - verifyURLMap(t, f, l7.UrlMap().Name, um1) + verifyURLMap(t, f, namer.UrlMap(lbInfo.Name), um1) // Change url map. lbInfo.UrlMap = um2 - if err = pool.Sync(lbInfo); err != nil { + if _, err := pool.Sync(lbInfo); err != nil { t.Fatalf("pool.Sync() = err %v", err) } - l7, err = pool.Get(lbInfo.Name) - if err != nil { - t.Fatalf("%v", err) + if !pool.Get(lbInfo.Name) { + t.Fatalf("Expected l7 does not exist") } - verifyURLMap(t, f, l7.UrlMap().Name, um2) + verifyURLMap(t, f, namer.UrlMap(lbInfo.Name), um2) } func TestPoolSyncNoChanges(t *testing.T) { @@ -771,13 +765,13 @@ func TestPoolSyncNoChanges(t *testing.T) { namer := utils.NewNamer("uid1", "fw1") lbInfo := &L7RuntimeInfo{Name: namer.LoadBalancer("test"), AllowHTTP: true, UrlMap: um1} f := NewFakeLoadBalancers(lbInfo.Name, namer) - pool := newFakeLoadBalancerPool(f, t, namer) - if err := pool.Sync(lbInfo); err != nil { + pool := newFakeLoadBalancerPool(f, t, namer, false) + if _, err := pool.Sync(lbInfo); err != nil { t.Fatalf("pool.Sync() = err %v", err) } lbInfo.UrlMap = um2 - if err := pool.Sync(lbInfo); err != nil { + if _, err := pool.Sync(lbInfo); err != nil { t.Fatalf("pool.Sync() = err %v", err) } @@ -819,12 +813,11 @@ func TestClusterNameChange(t *testing.T) { UrlMap: gceUrlMap, } f := NewFakeLoadBalancers(lbInfo.Name, namer) - pool := newFakeLoadBalancerPool(f, t, namer) - if err := pool.Sync(lbInfo); err != nil { + pool := newFakeLoadBalancerPool(f, t, namer, false) + if _, err := pool.Sync(lbInfo); err != nil { t.Fatalf("pool.Sync() = err %v", err) } - l7, err := pool.Get(lbInfo.Name) - if err != nil || l7 == nil { + if !pool.Get(lbInfo.Name) { t.Fatalf("Expected l7 not created") } verifyHTTPSForwardingRuleAndProxyLinks(t, f) @@ -836,11 +829,14 @@ func TestClusterNameChange(t *testing.T) { f.name = fmt.Sprintf("%v--%v", lbInfo.Name, newName) // Now the components should get renamed with the next suffix. - if err = pool.Sync(lbInfo); err != nil { + l7, err := pool.Sync(lbInfo) + if err != nil { t.Fatalf("pool.Sync() = err %v", err) } - l7, err = pool.Get(lbInfo.Name) - if err != nil || namer.ParseName(l7.Name).ClusterName != newName { + if !pool.Get(lbInfo.Name) { + t.Fatalf("Expected l7 does not exist") + } + if namer.ParseName(l7.Name).ClusterName != newName { t.Fatalf("Expected L7 name to change.") } verifyHTTPSForwardingRuleAndProxyLinks(t, f) diff --git a/pkg/utils/namer.go b/pkg/utils/namer.go index 0f033c54c6..c5479f1d61 100644 --- a/pkg/utils/namer.go +++ b/pkg/utils/namer.go @@ -363,6 +363,16 @@ func (n *Namer) UrlMap(lbName string) string { return truncate(fmt.Sprintf("%v-%v-%v", n.prefix, urlMapPrefix, lbName)) } +// ScrubUrlMapPrefix removes the "k8s-um" prefix from the UrlMap name. +func (n *Namer) ScrubUrlMapPrefix(name string) string { + prefix := fmt.Sprintf("%s-%s-", n.prefix, urlMapPrefix) + parts := strings.Split(name, prefix) + if len(parts) < 2 { + return "" + } + return parts[1] +} + // NamedPort returns the name for a named port. func (n *Namer) NamedPort(port int64) string { return fmt.Sprintf("port%v", port) diff --git a/pkg/utils/namer_test.go b/pkg/utils/namer_test.go index 9f8d26693d..846d18cd23 100644 --- a/pkg/utils/namer_test.go +++ b/pkg/utils/namer_test.go @@ -337,6 +337,33 @@ func TestNamerLoadBalancer(t *testing.T) { } } +func TestNamerScrubUrlMapPrefix(t *testing.T) { + namer := NewNamer("uid1", "fw1") + cases := []struct { + urlMapName string + scrubbedName string + }{ + { + urlMapName: "k8s-um-foo", + scrubbedName: "foo", + }, + { + urlMapName: "k8s-um-default-foo--clusterid", + scrubbedName: "default-foo--clusterid", + }, + { + urlMapName: "k8s-default-foo--clusterid", + }, + } + + for _, tc := range cases { + res := namer.ScrubUrlMapPrefix(tc.urlMapName) + if res != tc.scrubbedName { + t.Fatalf("namer.ScrubUrlMapPrefix() = %s, want %s", res, tc.scrubbedName) + } + } +} + // Ensure that a valid cert name is created if clusterName is empty. func TestNamerSSLCertName(t *testing.T) { secretHash := fmt.Sprintf("%x", sha256.Sum256([]byte("test123")))[:16]