From 1e35c81105d88c75108256209ddea410609cfca7 Mon Sep 17 00:00:00 2001 From: Minhan Xia Date: Thu, 14 Mar 2019 14:43:33 -0700 Subject: [PATCH 1/2] fix a bug where ingress GC will remove wanted LB if the namespace+name length is between 44-47 chars --- pkg/loadbalancers/l7s_test.go | 350 ++++++++++++++++++++++++++++++++++ pkg/utils/namer.go | 2 +- 2 files changed, 351 insertions(+), 1 deletion(-) create mode 100644 pkg/loadbalancers/l7s_test.go diff --git a/pkg/loadbalancers/l7s_test.go b/pkg/loadbalancers/l7s_test.go new file mode 100644 index 0000000000..9869454ab2 --- /dev/null +++ b/pkg/loadbalancers/l7s_test.go @@ -0,0 +1,350 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package loadbalancers + +import ( + "fmt" + "net/http" + "strings" + "testing" + + "google.golang.org/api/compute/v1" + "google.golang.org/api/googleapi" + "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/ingress-gce/pkg/context" + "k8s.io/ingress-gce/pkg/utils" + "k8s.io/kubernetes/pkg/cloudprovider/providers/gce" +) + +const ( + testClusterName = "0123456789abcedf" + + // resourceLeakLimit is the limit when ingress namespace and name are too long and ingress + // GC will leak the LB resources because the cluster uid got truncated. + resourceLeakLimit = 49 +) + +var longName = strings.Repeat("0123456789", 10) + +func TestGC(t *testing.T) { + t.Parallel() + pool := newTestLoadBalancerPool() + l7sPool := pool.(*L7s) + cloud := l7sPool.cloud + testCases := []struct { + desc string + ingressLBs []string + gcpLBs []string + expectLBs []string + }{ + { + desc: "empty", + ingressLBs: []string{}, + gcpLBs: []string{}, + expectLBs: []string{}, + }, + { + desc: "remove all lbs", + ingressLBs: []string{}, + gcpLBs: []string{ + generateKey(longName[:10], longName[:10]), + generateKey(longName[:20], longName[:20]), + generateKey(longName[:20], longName[:27]), + }, + expectLBs: []string{}, + }, + { + // resource leakage is due to the GC logic is doing naming matching. + // if the name is too long, it truncates the suffix which contains the cluster uid + // if cluster uid section of a name is too short or completely truncated, + // ingress controller will leak the resource instead. + desc: "remove all lbs but with leaks", + ingressLBs: []string{}, + gcpLBs: []string{ + generateKey(longName[:10], longName[:10]), + generateKey(longName[:20], longName[:20]), + generateKey(longName[:20], longName[:27]), + generateKey(longName[:21], longName[:27]), + generateKey(longName[:22], longName[:27]), + generateKey(longName[:50], longName[:30]), + }, + expectLBs: []string{ + generateKey(longName[:21], longName[:27]), + generateKey(longName[:22], longName[:27]), + generateKey(longName[:50], longName[:30]), + }, + }, + { + desc: "no LB exists", + ingressLBs: []string{ + generateKey(longName[:10], longName[:10]), + generateKey(longName[:20], longName[:20]), + generateKey(longName[:20], longName[:27]), + generateKey(longName[:50], longName[:30]), + }, + gcpLBs: []string{}, + expectLBs: []string{}, + }, + { + desc: "no LB get GCed", + ingressLBs: []string{ + generateKey(longName[:10], longName[:10]), + generateKey(longName[:20], longName[:20]), + generateKey(longName[:20], longName[:30]), + generateKey(longName, longName), + }, + gcpLBs: []string{ + generateKey(longName[:10], longName[:10]), + generateKey(longName[:20], longName[:20]), + generateKey(longName[:20], longName[:30]), + generateKey(longName, longName), + }, + expectLBs: []string{ + generateKey(longName[:10], longName[:10]), + generateKey(longName[:20], longName[:20]), + generateKey(longName[:20], longName[:30]), + generateKey(longName, longName), + }, + }, + { + desc: "some LB get GCed", + ingressLBs: []string{ + generateKey(longName[:10], longName[:10]), + generateKey(longName, longName), + }, + gcpLBs: []string{ + generateKey(longName[:10], longName[:10]), + generateKey(longName[:20], longName[:20]), + generateKey(longName[:20], longName[:27]), + generateKey(longName, longName), + }, + expectLBs: []string{ + generateKey(longName[:10], longName[:10]), + generateKey(longName, longName), + }, + }, + { + desc: "some LB get GCed and some leaked", + ingressLBs: []string{ + generateKey(longName[:10], longName[:10]), + generateKey(longName, longName), + }, + gcpLBs: []string{ + generateKey(longName[:10], longName[:10]), + generateKey(longName[:20], longName[:20]), + generateKey(longName[:20], longName[:27]), + generateKey(longName[:21], longName[:27]), + generateKey(longName, longName), + }, + expectLBs: []string{ + generateKey(longName[:10], longName[:10]), + generateKey(longName[:21], longName[:27]), + generateKey(longName, longName), + }, + }, + } + + otherNamer := utils.NewNamer("clusteruid", "fw1") + otherKeys := []string{ + "a/a", + "namespace/name", + generateKey(longName[:10], longName[:10]), + generateKey(longName[:20], longName[:20]), + } + + for _, key := range otherKeys { + createFakeLoadbalancer(cloud, otherNamer, key) + } + + for _, tc := range testCases { + for _, key := range tc.gcpLBs { + createFakeLoadbalancer(l7sPool.cloud, l7sPool.namer, key) + } + + err := l7sPool.GC(tc.ingressLBs) + if err != nil { + t.Errorf("For case %q, do not expect err: %v", tc.desc, err) + } + + // check if other LB are not deleted + for _, key := range otherKeys { + if err := checkFakeLoadBalancer(l7sPool.cloud, otherNamer, key, true); err != nil { + t.Errorf("For case %q and key %q, do not expect err: %v", tc.desc, key, err) + } + } + + // check if the total number of url maps are expected + urlMaps, _ := l7sPool.cloud.ListURLMaps() + if len(urlMaps) != len(tc.expectLBs)+len(otherKeys) { + t.Errorf("For case %q, expect %d urlmaps, but got %d.", tc.desc, len(tc.expectLBs)+len(otherKeys), len(urlMaps)) + } + + // 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 { + 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 { + t.Errorf("For case %q and key %q, do not expect err: %v", tc.desc, key, err) + } + removeFakeLoadBalancer(l7sPool.cloud, l7sPool.namer, key) + } + } +} + +func TestDoNotGCWantedLB(t *testing.T) { + t.Parallel() + pool := newTestLoadBalancerPool() + l7sPool := pool.(*L7s) + namer := l7sPool.namer + type testCase struct { + desc string + key string + } + testCases := []testCase{} + + for i := 3; i <= len(longName)*2+1; i++ { + testCases = append(testCases, testCase{fmt.Sprintf("Ingress Key is %d characters long.", i), generateKeyWithLength(i)}) + } + + numOfExtraUrlMap := 2 + l7sPool.cloud.CreateURLMap(&compute.UrlMap{Name: "random--name1"}) + l7sPool.cloud.CreateURLMap(&compute.UrlMap{Name: "k8s-random-random--1111111111111111"}) + + for _, tc := range testCases { + createFakeLoadbalancer(l7sPool.cloud, namer, tc.key) + 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 { + 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) + } +} + +// This should not leak at all, but verfies existing behavior +// TODO: remove this test after the GC resource leaking is fixed. +func TestGCToLeakLB(t *testing.T) { + t.Parallel() + pool := newTestLoadBalancerPool() + l7sPool := pool.(*L7s) + namer := l7sPool.namer + type testCase struct { + desc string + key string + } + testCases := []testCase{} + for i := 3; i <= len(longName)*2+1; i++ { + testCases = append(testCases, testCase{fmt.Sprintf("Ingress Key is %d characters long.", i), generateKeyWithLength(i)}) + } + + for _, tc := range testCases { + createFakeLoadbalancer(l7sPool.cloud, namer, tc.key) + 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 { + t.Errorf("For case %q, do not expect err: %v", tc.desc, err) + } + removeFakeLoadBalancer(l7sPool.cloud, namer, tc.key) + } else { + if err := checkFakeLoadBalancer(l7sPool.cloud, namer, tc.key, false); err != nil { + t.Errorf("For case %q, do not expect err: %v", tc.desc, err) + } + } + } +} + +func newTestLoadBalancerPool() LoadBalancerPool { + namer := utils.NewNamer(testClusterName, "fw1") + fakeGCECloud := gce.NewFakeGCECloud(gce.DefaultTestClusterValues()) + ctx := &context.ControllerContext{} + return NewLoadBalancerPool(fakeGCECloud, namer, ctx) +} + +func createFakeLoadbalancer(cloud LoadBalancers, 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)}) + cloud.ReserveGlobalAddress(&compute.Address{Name: namer.ForwardingRule(lbName, utils.HTTPProtocol)}) +} + +func removeFakeLoadBalancer(cloud LoadBalancers, 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)) + cloud.DeleteGlobalAddress(namer.ForwardingRule(lbName, utils.HTTPProtocol)) +} + +func checkFakeLoadBalancer(cloud LoadBalancers, namer *utils.Namer, key string, expectPresent bool) error { + var err error + lbName := namer.LoadBalancer(key) + _, err = cloud.GetGlobalForwardingRule(namer.ForwardingRule(lbName, utils.HTTPProtocol)) + 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)) + 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)) + if expectPresent && err != nil { + return err + } + if !expectPresent && err.(*googleapi.Error).Code != http.StatusNotFound { + return fmt.Errorf("expect URLMap %q to not present: %v", key, err) + } + return nil +} + +func generateKey(namespace, name string) string { + return fmt.Sprintf("%s/%s", namespace, name) +} + +func generateKeyWithLength(length int) string { + if length < 3 { + length = 3 + } + if length > 201 { + length = 201 + } + length = length - 1 + return fmt.Sprintf("%s/%s", longName[:length/2], longName[:length-length/2]) +} diff --git a/pkg/utils/namer.go b/pkg/utils/namer.go index 210c41b448..412482c240 100644 --- a/pkg/utils/namer.go +++ b/pkg/utils/namer.go @@ -343,7 +343,7 @@ func (n *Namer) LoadBalancer(key string) string { // LoadBalancerFromLbName reconstructs the full loadbalancer name, given the // lbName portion from NameComponents func (n *Namer) LoadBalancerFromLbName(lbName string) string { - return fmt.Sprintf("%v%v%v", lbName, clusterNameDelimiter, n.UID()) + return truncate(fmt.Sprintf("%v%v%v", lbName, clusterNameDelimiter, n.UID())) } // TargetProxy returns the name for target proxy given the load From 314783958ad05aabf8f1a31817a8b38bdf1936e3 Mon Sep 17 00:00:00 2001 From: Minhan Xia Date: Fri, 15 Mar 2019 11:00:12 -0700 Subject: [PATCH 2/2] convert LoadBalancers interfaces to follow release-1.5 --- pkg/loadbalancers/l7s_test.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/pkg/loadbalancers/l7s_test.go b/pkg/loadbalancers/l7s_test.go index 9869454ab2..7906503bba 100644 --- a/pkg/loadbalancers/l7s_test.go +++ b/pkg/loadbalancers/l7s_test.go @@ -188,7 +188,7 @@ func TestGC(t *testing.T) { } // check if the total number of url maps are expected - urlMaps, _ := l7sPool.cloud.ListURLMaps() + urlMaps, _ := l7sPool.cloud.ListUrlMaps() if len(urlMaps) != len(tc.expectLBs)+len(otherKeys) { t.Errorf("For case %q, expect %d urlmaps, but got %d.", tc.desc, len(tc.expectLBs)+len(otherKeys), len(urlMaps)) } @@ -227,8 +227,8 @@ func TestDoNotGCWantedLB(t *testing.T) { } numOfExtraUrlMap := 2 - l7sPool.cloud.CreateURLMap(&compute.UrlMap{Name: "random--name1"}) - l7sPool.cloud.CreateURLMap(&compute.UrlMap{Name: "k8s-random-random--1111111111111111"}) + l7sPool.cloud.CreateUrlMap(&compute.UrlMap{Name: "random--name1"}) + l7sPool.cloud.CreateUrlMap(&compute.UrlMap{Name: "k8s-random-random--1111111111111111"}) for _, tc := range testCases { createFakeLoadbalancer(l7sPool.cloud, namer, tc.key) @@ -240,7 +240,7 @@ func TestDoNotGCWantedLB(t *testing.T) { if err := checkFakeLoadBalancer(l7sPool.cloud, namer, tc.key, true); err != nil { t.Errorf("For case %q, do not expect err: %v", tc.desc, err) } - urlMaps, _ := l7sPool.cloud.ListURLMaps() + 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)) } @@ -286,7 +286,7 @@ func TestGCToLeakLB(t *testing.T) { func newTestLoadBalancerPool() LoadBalancerPool { namer := utils.NewNamer(testClusterName, "fw1") - fakeGCECloud := gce.NewFakeGCECloud(gce.DefaultTestClusterValues()) + fakeGCECloud := gce.FakeGCECloud(gce.DefaultTestClusterValues()) ctx := &context.ControllerContext{} return NewLoadBalancerPool(fakeGCECloud, namer, ctx) } @@ -294,16 +294,16 @@ func newTestLoadBalancerPool() LoadBalancerPool { func createFakeLoadbalancer(cloud LoadBalancers, 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)}) + cloud.CreateTargetHttpProxy(&compute.TargetHttpProxy{Name: namer.TargetProxy(lbName, utils.HTTPProtocol)}) + cloud.CreateUrlMap(&compute.UrlMap{Name: namer.UrlMap(lbName)}) cloud.ReserveGlobalAddress(&compute.Address{Name: namer.ForwardingRule(lbName, utils.HTTPProtocol)}) } func removeFakeLoadBalancer(cloud LoadBalancers, 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)) + cloud.DeleteTargetHttpProxy(namer.TargetProxy(lbName, utils.HTTPProtocol)) + cloud.DeleteUrlMap(namer.UrlMap(lbName)) cloud.DeleteGlobalAddress(namer.ForwardingRule(lbName, utils.HTTPProtocol)) } @@ -317,14 +317,14 @@ func checkFakeLoadBalancer(cloud LoadBalancers, namer *utils.Namer, key string, 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)) + _, err = cloud.GetTargetHttpProxy(namer.TargetProxy(lbName, utils.HTTPProtocol)) 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)) + _, err = cloud.GetUrlMap(namer.UrlMap(lbName)) if expectPresent && err != nil { return err }