From dc3147fefe539a6e60d9e6fa9f1124b7502699eb Mon Sep 17 00:00:00 2001 From: Spencer Hance Date: Thu, 8 Aug 2019 21:13:24 -0700 Subject: [PATCH 1/2] Refactor LB GC to better handle regional LBs --- pkg/composite/utils.go | 46 ------------- pkg/loadbalancers/features/features.go | 2 + pkg/loadbalancers/interfaces.go | 6 +- pkg/loadbalancers/l7.go | 3 +- pkg/loadbalancers/l7s.go | 93 ++++++++++++++++---------- 5 files changed, 63 insertions(+), 87 deletions(-) diff --git a/pkg/composite/utils.go b/pkg/composite/utils.go index 1577cb55ea..81f90a44e9 100644 --- a/pkg/composite/utils.go +++ b/pkg/composite/utils.go @@ -46,52 +46,6 @@ func CreateKey(gceCloud *gce.Cloud, name string, scope meta.KeyType) (*meta.Key, return nil, fmt.Errorf("invalid resource type: %s", scope) } -// TODO: (shance) generate this -// TODO: (shance) populate scope -// TODO: (shance) figure out a better way to get version -// ListAllUrlMaps() merges all configured List() calls into one list of composite UrlMaps. -// This function combines both global and regional resources into one slice -// so users must use ScopeFromSelfLink() to determine the correct scope -// before issuing an API call. -func ListAllUrlMaps(gceCloud *gce.Cloud) ([]*UrlMap, error) { - resultMap := map[string]*UrlMap{} - - // TODO(shance): this needs to be replaced with 'scope' and not key - globalKey, err := CreateKey(gceCloud, "", meta.Global) - if err != nil { - return nil, err - } - regionalKey, err := CreateKey(gceCloud, "", meta.Regional) - if err != nil { - return nil, err - } - - for _, vk := range []struct { - v meta.Version - k *meta.Key //TODO(shance): change this to scope - }{ - {meta.VersionGA, globalKey}, - {meta.VersionAlpha, regionalKey}, - } { - - list, err := ListUrlMaps(gceCloud, vk.k, vk.v) - if err != nil { - return nil, fmt.Errorf("error listing all urlmaps: %v", err) - } - for _, um := range list { - resultMap[um.SelfLink] = um - } - } - - // Convert map to slice - result := []*UrlMap{} - for _, um := range resultMap { - result = append(result, um) - } - - return result, nil -} - // TODO: (shance) generate this // TODO: (shance) populate scope // TODO: (shance) figure out a more accurate way to obtain version diff --git a/pkg/loadbalancers/features/features.go b/pkg/loadbalancers/features/features.go index 048d5e4775..7b88016369 100644 --- a/pkg/loadbalancers/features/features.go +++ b/pkg/loadbalancers/features/features.go @@ -28,6 +28,8 @@ const ( FeatureL7ILB = "L7ILB" ) +var GAResourceVersions = NewResourceVersions() + // ResourceVersions allows you to define all the versions required for each resource // for a feature. type ResourceVersions struct { diff --git a/pkg/loadbalancers/interfaces.go b/pkg/loadbalancers/interfaces.go index 70c1419b42..6a3b0c753b 100644 --- a/pkg/loadbalancers/interfaces.go +++ b/pkg/loadbalancers/interfaces.go @@ -18,14 +18,16 @@ package loadbalancers import ( "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta" + "k8s.io/ingress-gce/pkg/composite" + "k8s.io/ingress-gce/pkg/loadbalancers/features" ) // LoadBalancerPool is an interface to manage the cloud resources associated // with a gce loadbalancer. type LoadBalancerPool interface { Ensure(ri *L7RuntimeInfo) (*L7, error) - Delete(name string, version meta.Version, scope meta.KeyType) error + Delete(name string, versions *features.ResourceVersions, scope meta.KeyType) error GC(names []string) error Shutdown() error - List() ([]string, []meta.KeyType, error) + List(key *meta.Key, version meta.Version) ([]*composite.UrlMap, error) } diff --git a/pkg/loadbalancers/l7.go b/pkg/loadbalancers/l7.go index c7f99a3a1f..8dd5541b0f 100644 --- a/pkg/loadbalancers/l7.go +++ b/pkg/loadbalancers/l7.go @@ -223,10 +223,9 @@ func (l *L7) GetIP() string { // Cleanup deletes resources specific to this l7 in the right order. // forwarding rule -> target proxy -> url map // This leaves backends and health checks, which are shared across loadbalancers. -func (l *L7) Cleanup() error { +func (l *L7) Cleanup(versions *features.ResourceVersions) error { var key *meta.Key var err error - versions := l.Versions() fwName := l.namer.ForwardingRule(l.Name, utils.HTTPProtocol) klog.V(2).Infof("Deleting global forwarding rule %v", fwName) diff --git a/pkg/loadbalancers/l7s.go b/pkg/loadbalancers/l7s.go index da76945eaa..46f4869073 100644 --- a/pkg/loadbalancers/l7s.go +++ b/pkg/loadbalancers/l7s.go @@ -72,7 +72,7 @@ func (l *L7s) Ensure(ri *L7RuntimeInfo) (*L7, error) { } // Delete deletes a load balancer by name. -func (l *L7s) Delete(name string, version meta.Version, scope meta.KeyType) error { +func (l *L7s) Delete(name string, versions *features.ResourceVersions, scope meta.KeyType) error { lb := &L7{ runtimeInfo: &L7RuntimeInfo{Name: name}, Name: l.namer.LoadBalancer(name), @@ -82,46 +82,32 @@ func (l *L7s) Delete(name string, version meta.Version, scope meta.KeyType) erro } klog.V(3).Infof("Deleting lb %v", lb.Name) - if err := lb.Cleanup(); err != nil { + + if err := lb.Cleanup(versions); err != nil { return err } return nil } -// List returns a list of names of L7 resources, by listing all URL maps and -// deriving the Loadbalancer name from the URL map name -func (l *L7s) List() ([]string, []meta.KeyType, error) { - var names []string - var scopes []meta.KeyType - - var urlMaps []*composite.UrlMap - var err error - if flags.F.EnableL7Ilb { - urlMaps, err = composite.ListAllUrlMaps(l.cloud) - } else { - urlMaps, err = composite.ListUrlMaps(l.cloud, meta.GlobalKey(""), meta.VersionGA) - } +// List returns a list of urlMaps (the top level LB resource) that belong to the cluster +func (l *L7s) List(key *meta.Key, version meta.Version) ([]*composite.UrlMap, error) { + var result []*composite.UrlMap + urlMaps, err := composite.ListUrlMaps(l.cloud, key, version) if err != nil { - return nil, nil, err + return nil, err } for _, um := range urlMaps { if l.namer.NameBelongsToCluster(um.Name) { - nameParts := l.namer.ParseName(um.Name) - l7Name := l.namer.LoadBalancerFromLbName(nameParts.LbName) - names = append(names, l7Name) - scope, err := composite.ScopeFromSelfLink(um.SelfLink) - if err != nil { - return nil, nil, err - } - scopes = append(scopes, scope) + result = append(result, um) } } - return names, scopes, nil + return result, nil } // GC garbage collects loadbalancers not in the input list. +// TODO(shance): Update to handle regional and global LB with same name func (l *L7s) GC(names []string) error { klog.V(2).Infof("GC(%v)", names) @@ -129,27 +115,60 @@ func (l *L7s) GC(names []string) error { for _, n := range names { knownLoadBalancers.Insert(l.namer.LoadBalancer(n)) } - pool, scopes, err := l.List() + + // GC L7-ILB LBs if enabled + if flags.F.EnableL7Ilb { + key, err := composite.CreateKey(l.cloud, "", meta.Regional) + if err != nil { + return fmt.Errorf("error getting regional key: %v", err) + } + urlMaps, err := l.List(key, features.L7ILBVersions().UrlMap) + if err != nil { + return fmt.Errorf("error listing regional LBs: %v", err) + } + + if err := l.gc(urlMaps, knownLoadBalancers, features.L7ILBVersions()); err != nil { + return fmt.Errorf("error gc-ing regional LBs: %v", err) + } + } + + // TODO(shance): fix list taking a key + urlMaps, err := l.List(meta.GlobalKey(""), meta.VersionGA) if err != nil { - return err + return fmt.Errorf("error listing global LBs: %v", err) + } + + if errors := l.gc(urlMaps, knownLoadBalancers, features.GAResourceVersions); errors != nil { + return fmt.Errorf("error gcing global LBs: %v", errors) } + return nil +} + +// gc is a helper for GC +// TODO(shance): get versions from description +func (l *L7s) gc(urlMaps []*composite.UrlMap, knownLoadBalancers sets.String, versions *features.ResourceVersions) []error { + var errors []error + // Delete unknown loadbalancers - for i, name := range pool { - if knownLoadBalancers.Has(name) { + for _, um := range urlMaps { + nameParts := l.namer.ParseName(um.Name) + l7Name := l.namer.LoadBalancerFromLbName(nameParts.LbName) + + if knownLoadBalancers.Has(l7Name) { + klog.V(3).Infof("Load balancer %v is still valid, not GC'ing", l7Name) continue } - klog.V(2).Infof("GCing loadbalancer %v", name) - version := meta.VersionGA - // TODO: (shance) figure out a cleaner way to determine this - // Regional resources are alpha only - if scopes[i] != meta.Global { - version = meta.VersionAlpha + scope, err := composite.ScopeFromSelfLink(um.SelfLink) + if err != nil { + errors = append(errors, fmt.Errorf("error getting scope from self link for urlMap %v: %v", um, err)) + continue } - if err := l.Delete(name, version, scopes[i]); err != nil { - return err + klog.V(2).Infof("GCing loadbalancer %v", l7Name) + if err := l.Delete(l7Name, versions, scope); err != nil { + errors = append(errors, fmt.Errorf("error deleting loadbalancer %q", l7Name)) } } return nil From 0c126fb3a9e29fcc35d07fe033aeffe33fb038e7 Mon Sep 17 00:00:00 2001 From: Spencer Hance Date: Thu, 8 Aug 2019 21:13:37 -0700 Subject: [PATCH 2/2] Refactor LB unit tests --- pkg/loadbalancers/features/features_test.go | 4 +- pkg/loadbalancers/l7s_test.go | 88 ++++++++++++++------- pkg/loadbalancers/loadbalancers_test.go | 25 +++--- 3 files changed, 77 insertions(+), 40 deletions(-) diff --git a/pkg/loadbalancers/features/features_test.go b/pkg/loadbalancers/features/features_test.go index d7053f73db..2dd284519e 100644 --- a/pkg/loadbalancers/features/features_test.go +++ b/pkg/loadbalancers/features/features_test.go @@ -75,7 +75,7 @@ var ( } fakeFeatureToVersions = map[string]*ResourceVersions{ - fakeGaFeature: NewResourceVersions(), + fakeGaFeature: GAResourceVersions, fakeAlphaFeature: &fakeAlphaFeatureVersions, fakeBetaFeature: &fakeBetaFeatureVersions, fakeAlphaFeatureUrlMapOnly: &fakeAlphaFeatureUrlMapOnlyVersions, @@ -180,7 +180,7 @@ func TestVersionsFromFeatures(t *testing.T) { }{ { desc: "No Features", - expected: NewResourceVersions(), + expected: GAResourceVersions, }, { desc: "Alpha features", diff --git a/pkg/loadbalancers/l7s_test.go b/pkg/loadbalancers/l7s_test.go index 01229818de..ad3871a857 100644 --- a/pkg/loadbalancers/l7s_test.go +++ b/pkg/loadbalancers/l7s_test.go @@ -18,6 +18,9 @@ package loadbalancers import ( "fmt" + "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta" + "k8s.io/ingress-gce/pkg/composite" + "k8s.io/ingress-gce/pkg/loadbalancers/features" "net/http" "strings" "testing" @@ -166,13 +169,15 @@ func TestGC(t *testing.T) { generateKey(longName[:20], longName[:20]), } + versions := features.GAResourceVersions + for _, key := range otherKeys { - createFakeLoadbalancer(cloud, otherNamer, key) + createFakeLoadbalancer(cloud, otherNamer, key, versions, defaultScope) } for _, tc := range testCases { for _, key := range tc.gcpLBs { - createFakeLoadbalancer(l7sPool.cloud, l7sPool.namer, key) + createFakeLoadbalancer(l7sPool.cloud, l7sPool.namer, key, versions, defaultScope) } err := l7sPool.GC(tc.ingressLBs) @@ -182,7 +187,7 @@ func TestGC(t *testing.T) { // check if other LB are not deleted for _, key := range otherKeys { - if err := checkFakeLoadBalancer(l7sPool.cloud, otherNamer, key, true); err != nil { + if err := checkFakeLoadBalancer(l7sPool.cloud, otherNamer, key, versions, defaultScope, true); err != nil { t.Errorf("For case %q and key %q, do not expect err: %v", tc.desc, key, err) } } @@ -196,17 +201,17 @@ func TestGC(t *testing.T) { // check if the ones that are expected to be GC is actually GCed. expectRemovedLBs := sets.NewString(tc.gcpLBs...).Difference(sets.NewString(tc.expectLBs...)).Difference(sets.NewString(tc.ingressLBs...)) for _, key := range expectRemovedLBs.List() { - if err := checkFakeLoadBalancer(l7sPool.cloud, l7sPool.namer, key, false); err != nil { + if err := checkFakeLoadBalancer(l7sPool.cloud, l7sPool.namer, key, versions, defaultScope, false); err != nil { t.Errorf("For case %q and key %q, do not expect err: %v", tc.desc, key, err) } } // check if all expected LBs exists for _, key := range tc.expectLBs { - if err := checkFakeLoadBalancer(l7sPool.cloud, l7sPool.namer, key, true); err != nil { + if err := checkFakeLoadBalancer(l7sPool.cloud, l7sPool.namer, key, versions, defaultScope, true); err != nil { t.Errorf("For case %q and key %q, do not expect err: %v", tc.desc, key, err) } - removeFakeLoadBalancer(l7sPool.cloud, l7sPool.namer, key) + removeFakeLoadBalancer(l7sPool.cloud, l7sPool.namer, key, versions, defaultScope) } } } @@ -230,21 +235,23 @@ func TestDoNotGCWantedLB(t *testing.T) { l7sPool.cloud.CreateURLMap(&compute.UrlMap{Name: "random--name1"}) l7sPool.cloud.CreateURLMap(&compute.UrlMap{Name: "k8s-random-random--1111111111111111"}) + versions := features.GAResourceVersions + for _, tc := range testCases { - createFakeLoadbalancer(l7sPool.cloud, namer, tc.key) + createFakeLoadbalancer(l7sPool.cloud, namer, tc.key, versions, defaultScope) err := l7sPool.GC([]string{tc.key}) if err != nil { t.Errorf("For case %q, do not expect err: %v", tc.desc, err) } - if err := checkFakeLoadBalancer(l7sPool.cloud, namer, tc.key, true); err != nil { + if err := checkFakeLoadBalancer(l7sPool.cloud, namer, tc.key, versions, defaultScope, true); err != nil { t.Errorf("For case %q, do not expect err: %v", tc.desc, err) } urlMaps, _ := l7sPool.cloud.ListURLMaps() if len(urlMaps) != 1+numOfExtraUrlMap { t.Errorf("For case %q, expect %d urlmaps, but got %d.", tc.desc, 1+numOfExtraUrlMap, len(urlMaps)) } - removeFakeLoadBalancer(l7sPool.cloud, namer, tc.key) + removeFakeLoadBalancer(l7sPool.cloud, namer, tc.key, versions, defaultScope) } } @@ -264,20 +271,22 @@ func TestGCToLeakLB(t *testing.T) { testCases = append(testCases, testCase{fmt.Sprintf("Ingress Key is %d characters long.", i), generateKeyWithLength(i)}) } + versions := features.GAResourceVersions + for _, tc := range testCases { - createFakeLoadbalancer(l7sPool.cloud, namer, tc.key) + createFakeLoadbalancer(l7sPool.cloud, namer, tc.key, versions, defaultScope) err := l7sPool.GC([]string{}) if err != nil { t.Errorf("For case %q, do not expect err: %v", tc.desc, err) } if len(tc.key) >= resourceLeakLimit { - if err := checkFakeLoadBalancer(l7sPool.cloud, namer, tc.key, true); err != nil { + if err := checkFakeLoadBalancer(l7sPool.cloud, namer, tc.key, versions, defaultScope, true); err != nil { t.Errorf("For case %q, do not expect err: %v", tc.desc, err) } - removeFakeLoadBalancer(l7sPool.cloud, namer, tc.key) + removeFakeLoadBalancer(l7sPool.cloud, namer, tc.key, versions, defaultScope) } else { - if err := checkFakeLoadBalancer(l7sPool.cloud, namer, tc.key, false); err != nil { + if err := checkFakeLoadBalancer(l7sPool.cloud, namer, tc.key, versions, defaultScope, false); err != nil { t.Errorf("For case %q, do not expect err: %v", tc.desc, err) } } @@ -291,40 +300,63 @@ func newTestLoadBalancerPool() LoadBalancerPool { return NewLoadBalancerPool(fakeGCECloud, namer, ctx) } -func createFakeLoadbalancer(cloud *gce.Cloud, namer *utils.Namer, key string) { - lbName := namer.LoadBalancer(key) - cloud.CreateGlobalForwardingRule(&compute.ForwardingRule{Name: namer.ForwardingRule(lbName, utils.HTTPProtocol)}) - cloud.CreateTargetHTTPProxy(&compute.TargetHttpProxy{Name: namer.TargetProxy(lbName, utils.HTTPProtocol)}) - cloud.CreateURLMap(&compute.UrlMap{Name: namer.UrlMap(lbName)}) +func createFakeLoadbalancer(cloud *gce.Cloud, namer *utils.Namer, lbKey string, versions *features.ResourceVersions, scope meta.KeyType) { + lbName := namer.LoadBalancer(lbKey) + key, _ := composite.CreateKey(cloud, "", scope) + + key.Name = namer.ForwardingRule(lbName, utils.HTTPProtocol) + composite.CreateForwardingRule(cloud, key, &composite.ForwardingRule{Name: key.Name, Version: versions.ForwardingRule}) + + key.Name = namer.TargetProxy(lbName, utils.HTTPProtocol) + composite.CreateTargetHttpProxy(cloud, key, &composite.TargetHttpProxy{Name: key.Name, Version: versions.TargetHttpProxy}) + + key.Name = namer.UrlMap(lbName) + composite.CreateUrlMap(cloud, key, &composite.UrlMap{Name: key.Name, Version: versions.UrlMap}) + cloud.ReserveGlobalAddress(&compute.Address{Name: namer.ForwardingRule(lbName, utils.HTTPProtocol)}) + } -func removeFakeLoadBalancer(cloud *gce.Cloud, namer *utils.Namer, key string) { - lbName := namer.LoadBalancer(key) - cloud.DeleteGlobalForwardingRule(namer.ForwardingRule(lbName, utils.HTTPProtocol)) - cloud.DeleteTargetHTTPProxy(namer.TargetProxy(lbName, utils.HTTPProtocol)) - cloud.DeleteURLMap(namer.UrlMap(lbName)) +func removeFakeLoadBalancer(cloud *gce.Cloud, namer *utils.Namer, lbKey string, versions *features.ResourceVersions, scope meta.KeyType) { + lbName := namer.LoadBalancer(lbKey) + + key, _ := composite.CreateKey(cloud, "", scope) + key.Name = namer.ForwardingRule(lbName, utils.HTTPProtocol) + composite.DeleteForwardingRule(cloud, key, versions.ForwardingRule) + + key.Name = namer.TargetProxy(lbName, utils.HTTPProtocol) + composite.DeleteTargetHttpProxy(cloud, key, versions.TargetHttpProxy) + + key.Name = namer.UrlMap(lbName) + composite.DeleteUrlMap(cloud, key, versions.UrlMap) + cloud.DeleteGlobalAddress(namer.ForwardingRule(lbName, utils.HTTPProtocol)) } -func checkFakeLoadBalancer(cloud *gce.Cloud, namer *utils.Namer, key string, expectPresent bool) error { +func checkFakeLoadBalancer(cloud *gce.Cloud, namer *utils.Namer, lbKey string, versions *features.ResourceVersions, scope meta.KeyType, expectPresent bool) error { var err error - lbName := namer.LoadBalancer(key) - _, err = cloud.GetGlobalForwardingRule(namer.ForwardingRule(lbName, utils.HTTPProtocol)) + lbName := namer.LoadBalancer(lbKey) + key, _ := composite.CreateKey(cloud, namer.ForwardingRule(lbName, utils.HTTPProtocol), scope) + + _, err = composite.GetForwardingRule(cloud, key, versions.ForwardingRule) if expectPresent && err != nil { return err } if !expectPresent && err.(*googleapi.Error).Code != http.StatusNotFound { return fmt.Errorf("expect GlobalForwardingRule %q to not present: %v", key, err) } - _, err = cloud.GetTargetHTTPProxy(namer.TargetProxy(lbName, utils.HTTPProtocol)) + + key.Name = namer.TargetProxy(lbName, utils.HTTPProtocol) + _, err = composite.GetTargetHttpProxy(cloud, key, versions.TargetHttpProxy) if expectPresent && err != nil { return err } if !expectPresent && err.(*googleapi.Error).Code != http.StatusNotFound { return fmt.Errorf("expect TargetHTTPProxy %q to not present: %v", key, err) } - _, err = cloud.GetURLMap(namer.UrlMap(lbName)) + + key.Name = namer.UrlMap(lbName) + _, err = composite.GetUrlMap(cloud, key, versions.UrlMap) if expectPresent && err != nil { return err } diff --git a/pkg/loadbalancers/loadbalancers_test.go b/pkg/loadbalancers/loadbalancers_test.go index 229824a3ae..a32e9e03de 100644 --- a/pkg/loadbalancers/loadbalancers_test.go +++ b/pkg/loadbalancers/loadbalancers_test.go @@ -19,6 +19,7 @@ package loadbalancers import ( "context" "fmt" + "k8s.io/ingress-gce/pkg/loadbalancers/features" "net/http" "strconv" "strings" @@ -646,7 +647,7 @@ func TestIdenticalHostnameCerts(t *testing.T) { verifyCertAndProxyLink(expectCerts, expectCerts, j, t) // Fetch the target proxy certs and go through in order verifyProxyCertsInOrder(" foo.com", j, t) - j.pool.Delete(lbInfo.Name, defaultVersion, defaultScope) + j.pool.Delete(lbInfo.Name, features.GAResourceVersions, defaultScope) } } @@ -702,7 +703,7 @@ func TestIdenticalHostnameCertsPreShared(t *testing.T) { verifyCertAndProxyLink(expectCerts, expectCerts, j, t) // Fetch the target proxy certs and go through in order verifyProxyCertsInOrder(" foo.com", j, t) - j.pool.Delete(lbInfo.Name, defaultVersion, defaultScope) + j.pool.Delete(lbInfo.Name, features.GAResourceVersions, defaultScope) } } @@ -1191,20 +1192,24 @@ func TestList(t *testing.T) { } if _, err := j.pool.Ensure(lbInfo); err != nil { - t.Fatalf("j.pool.Ensure() = err %v", err) + t.Fatalf("j.pool.Ensure() = %v; want nil", err) } - lbNames, _, err := j.pool.List() + urlMaps, err := j.pool.List(key, defaultVersion) if err != nil { - t.Fatalf("j.pool.List() = err %v", err) + t.Fatalf("j.pool.List(%q, %q) = %v, want nil", key, defaultVersion, err) + } + var umNames []string + for _, um := range urlMaps { + umNames = append(umNames, um.Name) } - expected := []string{"old-l7--uid1", "test--uid1"} + expected := []string{"k8s-um-test--uid1", "k8s-um-old-l7--uid1"} - if len(lbNames) != 2 || - !slice.ContainsString(lbNames, expected[0], nil) || - !slice.ContainsString(lbNames, expected[1], nil) { - t.Fatalf("j.pool.List() returned %+v, want %+v", lbNames, expected) + for _, name := range expected { + if !slice.ContainsString(umNames, name, nil) { + t.Fatalf("j.pool.List(%q, %q) returned names %v, want %v", key, defaultVersion, umNames, expected) + } } }