From 0c126fb3a9e29fcc35d07fe033aeffe33fb038e7 Mon Sep 17 00:00:00 2001 From: Spencer Hance Date: Thu, 8 Aug 2019 21:13:37 -0700 Subject: [PATCH] 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) + } } }