Skip to content

Commit d84515a

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #692 from freehan/cherry-pick
Cherry pick #688 into release.15
2 parents cff9722 + 3147839 commit d84515a

File tree

2 files changed

+351
-1
lines changed

2 files changed

+351
-1
lines changed

‎pkg/loadbalancers/l7s_test.go‎

Lines changed: 350 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,350 @@
1+
/*
2+
Copyright 2019 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package loadbalancers
18+
19+
import (
20+
"fmt"
21+
"net/http"
22+
"strings"
23+
"testing"
24+
25+
"google.golang.org/api/compute/v1"
26+
"google.golang.org/api/googleapi"
27+
"k8s.io/apimachinery/pkg/util/sets"
28+
"k8s.io/ingress-gce/pkg/context"
29+
"k8s.io/ingress-gce/pkg/utils"
30+
"k8s.io/kubernetes/pkg/cloudprovider/providers/gce"
31+
)
32+
33+
const (
34+
testClusterName = "0123456789abcedf"
35+
36+
// resourceLeakLimit is the limit when ingress namespace and name are too long and ingress
37+
// GC will leak the LB resources because the cluster uid got truncated.
38+
resourceLeakLimit = 49
39+
)
40+
41+
var longName = strings.Repeat("0123456789", 10)
42+
43+
func TestGC(t *testing.T) {
44+
t.Parallel()
45+
pool := newTestLoadBalancerPool()
46+
l7sPool := pool.(*L7s)
47+
cloud := l7sPool.cloud
48+
testCases := []struct {
49+
desc string
50+
ingressLBs []string
51+
gcpLBs []string
52+
expectLBs []string
53+
}{
54+
{
55+
desc: "empty",
56+
ingressLBs: []string{},
57+
gcpLBs: []string{},
58+
expectLBs: []string{},
59+
},
60+
{
61+
desc: "remove all lbs",
62+
ingressLBs: []string{},
63+
gcpLBs: []string{
64+
generateKey(longName[:10], longName[:10]),
65+
generateKey(longName[:20], longName[:20]),
66+
generateKey(longName[:20], longName[:27]),
67+
},
68+
expectLBs: []string{},
69+
},
70+
{
71+
// resource leakage is due to the GC logic is doing naming matching.
72+
// if the name is too long, it truncates the suffix which contains the cluster uid
73+
// if cluster uid section of a name is too short or completely truncated,
74+
// ingress controller will leak the resource instead.
75+
desc: "remove all lbs but with leaks",
76+
ingressLBs: []string{},
77+
gcpLBs: []string{
78+
generateKey(longName[:10], longName[:10]),
79+
generateKey(longName[:20], longName[:20]),
80+
generateKey(longName[:20], longName[:27]),
81+
generateKey(longName[:21], longName[:27]),
82+
generateKey(longName[:22], longName[:27]),
83+
generateKey(longName[:50], longName[:30]),
84+
},
85+
expectLBs: []string{
86+
generateKey(longName[:21], longName[:27]),
87+
generateKey(longName[:22], longName[:27]),
88+
generateKey(longName[:50], longName[:30]),
89+
},
90+
},
91+
{
92+
desc: "no LB exists",
93+
ingressLBs: []string{
94+
generateKey(longName[:10], longName[:10]),
95+
generateKey(longName[:20], longName[:20]),
96+
generateKey(longName[:20], longName[:27]),
97+
generateKey(longName[:50], longName[:30]),
98+
},
99+
gcpLBs: []string{},
100+
expectLBs: []string{},
101+
},
102+
{
103+
desc: "no LB get GCed",
104+
ingressLBs: []string{
105+
generateKey(longName[:10], longName[:10]),
106+
generateKey(longName[:20], longName[:20]),
107+
generateKey(longName[:20], longName[:30]),
108+
generateKey(longName, longName),
109+
},
110+
gcpLBs: []string{
111+
generateKey(longName[:10], longName[:10]),
112+
generateKey(longName[:20], longName[:20]),
113+
generateKey(longName[:20], longName[:30]),
114+
generateKey(longName, longName),
115+
},
116+
expectLBs: []string{
117+
generateKey(longName[:10], longName[:10]),
118+
generateKey(longName[:20], longName[:20]),
119+
generateKey(longName[:20], longName[:30]),
120+
generateKey(longName, longName),
121+
},
122+
},
123+
{
124+
desc: "some LB get GCed",
125+
ingressLBs: []string{
126+
generateKey(longName[:10], longName[:10]),
127+
generateKey(longName, longName),
128+
},
129+
gcpLBs: []string{
130+
generateKey(longName[:10], longName[:10]),
131+
generateKey(longName[:20], longName[:20]),
132+
generateKey(longName[:20], longName[:27]),
133+
generateKey(longName, longName),
134+
},
135+
expectLBs: []string{
136+
generateKey(longName[:10], longName[:10]),
137+
generateKey(longName, longName),
138+
},
139+
},
140+
{
141+
desc: "some LB get GCed and some leaked",
142+
ingressLBs: []string{
143+
generateKey(longName[:10], longName[:10]),
144+
generateKey(longName, longName),
145+
},
146+
gcpLBs: []string{
147+
generateKey(longName[:10], longName[:10]),
148+
generateKey(longName[:20], longName[:20]),
149+
generateKey(longName[:20], longName[:27]),
150+
generateKey(longName[:21], longName[:27]),
151+
generateKey(longName, longName),
152+
},
153+
expectLBs: []string{
154+
generateKey(longName[:10], longName[:10]),
155+
generateKey(longName[:21], longName[:27]),
156+
generateKey(longName, longName),
157+
},
158+
},
159+
}
160+
161+
otherNamer := utils.NewNamer("clusteruid", "fw1")
162+
otherKeys := []string{
163+
"a/a",
164+
"namespace/name",
165+
generateKey(longName[:10], longName[:10]),
166+
generateKey(longName[:20], longName[:20]),
167+
}
168+
169+
for _, key := range otherKeys {
170+
createFakeLoadbalancer(cloud, otherNamer, key)
171+
}
172+
173+
for _, tc := range testCases {
174+
for _, key := range tc.gcpLBs {
175+
createFakeLoadbalancer(l7sPool.cloud, l7sPool.namer, key)
176+
}
177+
178+
err := l7sPool.GC(tc.ingressLBs)
179+
if err != nil {
180+
t.Errorf("For case %q, do not expect err: %v", tc.desc, err)
181+
}
182+
183+
// check if other LB are not deleted
184+
for _, key := range otherKeys {
185+
if err := checkFakeLoadBalancer(l7sPool.cloud, otherNamer, key, true); err != nil {
186+
t.Errorf("For case %q and key %q, do not expect err: %v", tc.desc, key, err)
187+
}
188+
}
189+
190+
// check if the total number of url maps are expected
191+
urlMaps, _ := l7sPool.cloud.ListUrlMaps()
192+
if len(urlMaps) != len(tc.expectLBs)+len(otherKeys) {
193+
t.Errorf("For case %q, expect %d urlmaps, but got %d.", tc.desc, len(tc.expectLBs)+len(otherKeys), len(urlMaps))
194+
}
195+
196+
// check if the ones that are expected to be GC is actually GCed.
197+
expectRemovedLBs := sets.NewString(tc.gcpLBs...).Difference(sets.NewString(tc.expectLBs...)).Difference(sets.NewString(tc.ingressLBs...))
198+
for _, key := range expectRemovedLBs.List() {
199+
if err := checkFakeLoadBalancer(l7sPool.cloud, l7sPool.namer, key, false); err != nil {
200+
t.Errorf("For case %q and key %q, do not expect err: %v", tc.desc, key, err)
201+
}
202+
}
203+
204+
// check if all expected LBs exists
205+
for _, key := range tc.expectLBs {
206+
if err := checkFakeLoadBalancer(l7sPool.cloud, l7sPool.namer, key, true); err != nil {
207+
t.Errorf("For case %q and key %q, do not expect err: %v", tc.desc, key, err)
208+
}
209+
removeFakeLoadBalancer(l7sPool.cloud, l7sPool.namer, key)
210+
}
211+
}
212+
}
213+
214+
func TestDoNotGCWantedLB(t *testing.T) {
215+
t.Parallel()
216+
pool := newTestLoadBalancerPool()
217+
l7sPool := pool.(*L7s)
218+
namer := l7sPool.namer
219+
type testCase struct {
220+
desc string
221+
key string
222+
}
223+
testCases := []testCase{}
224+
225+
for i := 3; i <= len(longName)*2+1; i++ {
226+
testCases = append(testCases, testCase{fmt.Sprintf("Ingress Key is %d characters long.", i), generateKeyWithLength(i)})
227+
}
228+
229+
numOfExtraUrlMap := 2
230+
l7sPool.cloud.CreateUrlMap(&compute.UrlMap{Name: "random--name1"})
231+
l7sPool.cloud.CreateUrlMap(&compute.UrlMap{Name: "k8s-random-random--1111111111111111"})
232+
233+
for _, tc := range testCases {
234+
createFakeLoadbalancer(l7sPool.cloud, namer, tc.key)
235+
err := l7sPool.GC([]string{tc.key})
236+
if err != nil {
237+
t.Errorf("For case %q, do not expect err: %v", tc.desc, err)
238+
}
239+
240+
if err := checkFakeLoadBalancer(l7sPool.cloud, namer, tc.key, true); err != nil {
241+
t.Errorf("For case %q, do not expect err: %v", tc.desc, err)
242+
}
243+
urlMaps, _ := l7sPool.cloud.ListUrlMaps()
244+
if len(urlMaps) != 1+numOfExtraUrlMap {
245+
t.Errorf("For case %q, expect %d urlmaps, but got %d.", tc.desc, 1+numOfExtraUrlMap, len(urlMaps))
246+
}
247+
removeFakeLoadBalancer(l7sPool.cloud, namer, tc.key)
248+
}
249+
}
250+
251+
// This should not leak at all, but verfies existing behavior
252+
// TODO: remove this test after the GC resource leaking is fixed.
253+
func TestGCToLeakLB(t *testing.T) {
254+
t.Parallel()
255+
pool := newTestLoadBalancerPool()
256+
l7sPool := pool.(*L7s)
257+
namer := l7sPool.namer
258+
type testCase struct {
259+
desc string
260+
key string
261+
}
262+
testCases := []testCase{}
263+
for i := 3; i <= len(longName)*2+1; i++ {
264+
testCases = append(testCases, testCase{fmt.Sprintf("Ingress Key is %d characters long.", i), generateKeyWithLength(i)})
265+
}
266+
267+
for _, tc := range testCases {
268+
createFakeLoadbalancer(l7sPool.cloud, namer, tc.key)
269+
err := l7sPool.GC([]string{})
270+
if err != nil {
271+
t.Errorf("For case %q, do not expect err: %v", tc.desc, err)
272+
}
273+
274+
if len(tc.key) >= resourceLeakLimit {
275+
if err := checkFakeLoadBalancer(l7sPool.cloud, namer, tc.key, true); err != nil {
276+
t.Errorf("For case %q, do not expect err: %v", tc.desc, err)
277+
}
278+
removeFakeLoadBalancer(l7sPool.cloud, namer, tc.key)
279+
} else {
280+
if err := checkFakeLoadBalancer(l7sPool.cloud, namer, tc.key, false); err != nil {
281+
t.Errorf("For case %q, do not expect err: %v", tc.desc, err)
282+
}
283+
}
284+
}
285+
}
286+
287+
func newTestLoadBalancerPool() LoadBalancerPool {
288+
namer := utils.NewNamer(testClusterName, "fw1")
289+
fakeGCECloud := gce.FakeGCECloud(gce.DefaultTestClusterValues())
290+
ctx := &context.ControllerContext{}
291+
return NewLoadBalancerPool(fakeGCECloud, namer, ctx)
292+
}
293+
294+
func createFakeLoadbalancer(cloud LoadBalancers, namer *utils.Namer, key string) {
295+
lbName := namer.LoadBalancer(key)
296+
cloud.CreateGlobalForwardingRule(&compute.ForwardingRule{Name: namer.ForwardingRule(lbName, utils.HTTPProtocol)})
297+
cloud.CreateTargetHttpProxy(&compute.TargetHttpProxy{Name: namer.TargetProxy(lbName, utils.HTTPProtocol)})
298+
cloud.CreateUrlMap(&compute.UrlMap{Name: namer.UrlMap(lbName)})
299+
cloud.ReserveGlobalAddress(&compute.Address{Name: namer.ForwardingRule(lbName, utils.HTTPProtocol)})
300+
}
301+
302+
func removeFakeLoadBalancer(cloud LoadBalancers, namer *utils.Namer, key string) {
303+
lbName := namer.LoadBalancer(key)
304+
cloud.DeleteGlobalForwardingRule(namer.ForwardingRule(lbName, utils.HTTPProtocol))
305+
cloud.DeleteTargetHttpProxy(namer.TargetProxy(lbName, utils.HTTPProtocol))
306+
cloud.DeleteUrlMap(namer.UrlMap(lbName))
307+
cloud.DeleteGlobalAddress(namer.ForwardingRule(lbName, utils.HTTPProtocol))
308+
}
309+
310+
func checkFakeLoadBalancer(cloud LoadBalancers, namer *utils.Namer, key string, expectPresent bool) error {
311+
var err error
312+
lbName := namer.LoadBalancer(key)
313+
_, err = cloud.GetGlobalForwardingRule(namer.ForwardingRule(lbName, utils.HTTPProtocol))
314+
if expectPresent && err != nil {
315+
return err
316+
}
317+
if !expectPresent && err.(*googleapi.Error).Code != http.StatusNotFound {
318+
return fmt.Errorf("expect GlobalForwardingRule %q to not present: %v", key, err)
319+
}
320+
_, err = cloud.GetTargetHttpProxy(namer.TargetProxy(lbName, utils.HTTPProtocol))
321+
if expectPresent && err != nil {
322+
return err
323+
}
324+
if !expectPresent && err.(*googleapi.Error).Code != http.StatusNotFound {
325+
return fmt.Errorf("expect TargetHTTPProxy %q to not present: %v", key, err)
326+
}
327+
_, err = cloud.GetUrlMap(namer.UrlMap(lbName))
328+
if expectPresent && err != nil {
329+
return err
330+
}
331+
if !expectPresent && err.(*googleapi.Error).Code != http.StatusNotFound {
332+
return fmt.Errorf("expect URLMap %q to not present: %v", key, err)
333+
}
334+
return nil
335+
}
336+
337+
func generateKey(namespace, name string) string {
338+
return fmt.Sprintf("%s/%s", namespace, name)
339+
}
340+
341+
func generateKeyWithLength(length int) string {
342+
if length < 3 {
343+
length = 3
344+
}
345+
if length > 201 {
346+
length = 201
347+
}
348+
length = length - 1
349+
return fmt.Sprintf("%s/%s", longName[:length/2], longName[:length-length/2])
350+
}

‎pkg/utils/namer.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -343,7 +343,7 @@ func (n *Namer) LoadBalancer(key string) string {
343343
// LoadBalancerFromLbName reconstructs the full loadbalancer name, given the
344344
// lbName portion from NameComponents
345345
func (n *Namer) LoadBalancerFromLbName(lbName string) string {
346-
return fmt.Sprintf("%v%v%v", lbName, clusterNameDelimiter, n.UID())
346+
return truncate(fmt.Sprintf("%v%v%v", lbName, clusterNameDelimiter, n.UID()))
347347
}
348348

349349
// TargetProxy returns the name for target proxy given the load

0 commit comments

Comments
 (0)