Skip to content

Commit 26ce829

Browse files
hrefhref
authored
Merge pull request #29 from cloudscale-ch/denis/missing-uuid-fix
Fix CCM creating LBs ad infinitum, when encountering a missing UUID
2 parents ec00f2c + 42f6a05 commit 26ce829

File tree

6 files changed

+200
-74
lines changed

6 files changed

+200
-74
lines changed

‎helpers/run-in-test-cluster‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,16 +19,16 @@ function ensure-k8test() {
1919
# on the runners and cloning via SSH does not work.
2020
#
2121
# Therefore we run a separate install block that is only meant for GitHub.
22-
if [[ "${GITHUB_ACTIONS:-}" == "true" ]] then
22+
if [[ "${GITHUB_ACTIONS:-}" == "true" ]]; then
2323
if test -d k8test; then
24-
source k8test/venv/bin/activate
24+
source k8test/venv/bin/activate
2525
return
2626
fi
2727

2828
mkdir -p ~/.ssh/ && touch ~/.ssh/known_hosts
2929
git clone https://github.com/cloudscale-ch/k8test
3030
python3 -m venv k8test/venv
31-
source k8test/venv/bin/activate
31+
source k8test/venv/bin/activate
3232
pip install poetry
3333
poetry install --directory k8test
3434
return
@@ -74,7 +74,7 @@ function ensure-cluster() {
7474
if ! test -f k8test/cluster/ssh.pub; then
7575
ssh-keygen -t ed25519 -N '' -f k8test/cluster/ssh
7676
fi
77-
77+
7878
if ! test -f k8test/cluster/admin.conf; then
7979
zone="$(random-zone)"
8080

@@ -107,7 +107,7 @@ function ensure-cluster() {
107107
-e kubelet_extra_args='--cloud-provider=external' \
108108
-e kubernetes="${KUBERNETES}" \
109109
-e cluster_prefix="${CLUSTER_PREFIX}" \
110-
-e subnet="${SUBNET}"
110+
-e subnet="${SUBNET}"
111111
fi
112112
}
113113

‎pkg/cloudscale_ccm/lb_mapper.go‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,19 @@ func (l *lbMapper) findByServiceInfo(
2020
serviceInfo *serviceInfo,
2121
) *limiter.Limiter[cloudscale.LoadBalancer] {
2222

23+
// If we have a UUID, look for both the service and the UUID. Usually
24+
// we expect to only see one, but it is possible for the UUID to point
25+
// to another LB than the service name, in which case we return both
26+
// so the caller can decide if that is sane or not.
2327
if uuid := serviceInfo.annotation(LoadBalancerUUID); uuid != "" {
24-
return l.getByUUID(ctx, uuid)
28+
return limiter.Join(
29+
l.getByUUID(ctx, uuid),
30+
l.findByName(ctx, serviceInfo.annotation(LoadBalancerName)),
31+
).Unique(
32+
func(a *cloudscale.LoadBalancer, b *cloudscale.LoadBalancer) bool {
33+
return a.UUID == b.UUID
34+
},
35+
)
2536
}
2637

2738
return l.findByName(ctx, serviceInfo.annotation(LoadBalancerName))

‎pkg/cloudscale_ccm/lb_mapper_test.go‎

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,19 +37,42 @@ func TestFindLoadBalancer(t *testing.T) {
3737
lb, err := lbs.One()
3838
assert.NoError(t, err)
3939
assert.Equal(t, "foo", lb.Name)
40+
assert.Equal(t, 1, lbs.Count())
4041

4142
// Using an ambiguous name
4243
s.Annotations[LoadBalancerName] = "clone"
4344

4445
lbs = mapper.findByServiceInfo(t.Context(), i)
4546
_, err = lbs.One()
4647
assert.Error(t, err)
48+
assert.Equal(t, 2, lbs.Count())
4749

48-
// Using a uuid
50+
// Using a unique name and a mismatched uuid
51+
s.Annotations[LoadBalancerName] = "foo"
4952
s.Annotations[LoadBalancerUUID] = "85dffa20-8097-4d75-afa6-9e4372047ce6"
5053

54+
lbs = mapper.findByServiceInfo(t.Context(), i)
55+
_, err = lbs.One()
56+
assert.Error(t, err)
57+
assert.Equal(t, 2, lbs.Count())
58+
59+
// Using a unique name and a missing uuid
60+
s.Annotations[LoadBalancerName] = "foo"
61+
s.Annotations[LoadBalancerUUID] = "00000000-0000-0000-0000-000000000000"
62+
5163
lbs = mapper.findByServiceInfo(t.Context(), i)
5264
lb, err = lbs.One()
5365
assert.NoError(t, err)
54-
assert.Equal(t, "clone", lb.Name)
66+
assert.Equal(t, "foo", lb.Name)
67+
assert.Equal(t, 1, lbs.Count())
68+
69+
// Using a unique name and a matching uuid
70+
s.Annotations[LoadBalancerName] = "foo"
71+
s.Annotations[LoadBalancerUUID] = "c2e4aabd-8c91-46da-b069-71e01f439806"
72+
73+
lbs = mapper.findByServiceInfo(t.Context(), i)
74+
lb, err = lbs.One()
75+
assert.NoError(t, err)
76+
assert.Equal(t, "foo", lb.Name)
77+
assert.Equal(t, 1, lbs.Count())
5578
}

‎pkg/cloudscale_ccm/loadbalancer.go‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,12 +21,11 @@ import (
2121
// them, unless you know what you are doing.
2222
const (
2323
// LoadBalancerUUID uniquely identifes the loadbalancer. This annotation
24-
// should not be provided by the customer, unless the adoption of an
25-
// existing load balancer is desired.
24+
// should not be provided by the customer.
2625
//
27-
// In all other cases, this value is set by the CCM after creating the
28-
// load balancer, to ensure that we track it with a proper ID and not
29-
// a name that might change without our knowledge.
26+
// Instead, this value is set by the CCM after creating the load balancer,
27+
// to ensure that we track it with a proper ID and not a name that might
28+
// change without our knowledge.
3029
LoadBalancerUUID = "k8s.cloudscale.ch/loadbalancer-uuid"
3130

3231
// LoadBalancerConfigVersion is set by the CCM when it first handles a

‎pkg/cloudscale_ccm/reconcile_test.go‎

Lines changed: 99 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -202,79 +202,117 @@ func TestDesiredService(t *testing.T) {
202202
func TestActualState(t *testing.T) {
203203
t.Parallel()
204204

205-
server := testkit.NewMockAPIServer()
206-
server.WithLoadBalancers([]cloudscale.LoadBalancer{
205+
testCases := []struct {
206+
name string
207+
annotations map[string]string
208+
}{
207209
{
208-
UUID: "00000000-0000-0000-0000-000000000000",
209-
Name: "k8test-service-test",
210-
},
211-
})
212-
server.On("/v1/load-balancers/pools", 200, []cloudscale.LoadBalancerPool{
213-
{
214-
Name: "tcp/80",
215-
UUID: "00000000-0000-0000-0000-000000000001",
216-
LoadBalancer: cloudscale.LoadBalancerStub{
217-
UUID: "00000000-0000-0000-0000-000000000000",
210+
name: "by UUID only",
211+
annotations: map[string]string{
212+
LoadBalancerUUID: "00000000-0000-0000-0000-000000000000",
218213
},
219214
},
220-
})
221-
server.On("/v1/load-balancers/pools/00000000-0000-0000-0000-000000000001"+
222-
"/members", 200, []cloudscale.LoadBalancerPoolMember{
223215
{
224-
Name: "10.0.0.1:8080",
225-
Pool: cloudscale.LoadBalancerPoolStub{
226-
UUID: "00000000-0000-0000-0000-000000000001",
216+
name: "by Name only",
217+
annotations: map[string]string{
218+
LoadBalancerName: "k8test-service-test",
227219
},
228220
},
229-
})
230-
server.On("/v1/load-balancers/listeners", 200,
231-
[]cloudscale.LoadBalancerListener{
232-
{
233-
Name: "tcp/80",
234-
Pool: &cloudscale.LoadBalancerPoolStub{
235-
UUID: "00000000-0000-0000-0000-000000000001",
236-
},
221+
{
222+
name: "by matching UUID and Name",
223+
annotations: map[string]string{
224+
LoadBalancerUUID: "00000000-0000-0000-0000-000000000000",
225+
LoadBalancerName: "k8test-service-test",
237226
},
238227
},
239-
)
240-
server.On("/v1/load-balancers/health-monitors", 200,
241-
[]cloudscale.LoadBalancerHealthMonitor{
242-
{
243-
Type: "tcp",
244-
Pool: cloudscale.LoadBalancerPoolStub{
245-
UUID: "00000000-0000-0000-0000-000000000001",
246-
},
228+
{
229+
name: "by none-matching UUID and Name, name has precedence",
230+
annotations: map[string]string{
231+
LoadBalancerUUID: "00000000-0000-ffff-0000-000000000000",
232+
LoadBalancerName: "k8test-service-test",
247233
},
248234
},
249-
)
250-
server.On("/v1/floating-ips", 200,
251-
[]cloudscale.FloatingIP{},
252-
)
253-
server.Start()
254-
defer server.Close()
255-
256-
mapper := lbMapper{client: server.Client()}
257-
258-
s := testkit.NewService("service").V1()
259-
s.Annotations = make(map[string]string)
260-
s.Annotations[LoadBalancerUUID] = "00000000-0000-0000-0000-000000000000"
261-
262-
i := newServiceInfo(s, "")
235+
}
263236

264-
actual, err := actualLbState(t.Context(), &mapper, i)
265-
assert.NoError(t, err)
237+
for _, tc := range testCases {
238+
t.Run(tc.name, func(t *testing.T) {
239+
t.Parallel()
266240

267-
assert.Equal(t, "k8test-service-test", actual.lb.Name)
268-
assert.Len(t, actual.pools, 1)
269-
assert.Len(t, actual.members, 1)
270-
assert.Len(t, actual.listeners, 1)
271-
assert.Len(t, actual.monitors, 1)
272-
273-
p := actual.pools[0]
274-
assert.Equal(t, "tcp/80", p.Name)
275-
assert.Equal(t, "10.0.0.1:8080", actual.members[p][0].Name)
276-
assert.Equal(t, "tcp/80", actual.listeners[p][0].Name)
277-
assert.Equal(t, "tcp", actual.monitors[p][0].Type)
241+
server := testkit.NewMockAPIServer()
242+
server.WithLoadBalancers([]cloudscale.LoadBalancer{
243+
{
244+
UUID: "00000000-0000-0000-0000-000000000000",
245+
Name: "k8test-service-test",
246+
},
247+
})
248+
server.On("/v1/load-balancers/pools", 200,
249+
[]cloudscale.LoadBalancerPool{
250+
{
251+
Name: "tcp/80",
252+
UUID: "00000000-0000-0000-0000-000000000001",
253+
LoadBalancer: cloudscale.LoadBalancerStub{
254+
UUID: "00000000-0000-0000-0000-000000000000",
255+
},
256+
},
257+
})
258+
server.On("/v1/load-balancers/pools/"+
259+
"00000000-0000-0000-0000-000000000001/members", 200,
260+
[]cloudscale.LoadBalancerPoolMember{
261+
{
262+
Name: "10.0.0.1:8080",
263+
Pool: cloudscale.LoadBalancerPoolStub{
264+
UUID: "00000000-0000-0000-0000-000000000001",
265+
},
266+
},
267+
})
268+
server.On("/v1/load-balancers/listeners", 200,
269+
[]cloudscale.LoadBalancerListener{
270+
{
271+
Name: "tcp/80",
272+
Pool: &cloudscale.LoadBalancerPoolStub{
273+
UUID: "00000000-0000-0000-0000-000000000001",
274+
},
275+
},
276+
},
277+
)
278+
server.On("/v1/load-balancers/health-monitors", 200,
279+
[]cloudscale.LoadBalancerHealthMonitor{
280+
{
281+
Type: "tcp",
282+
Pool: cloudscale.LoadBalancerPoolStub{
283+
UUID: "00000000-0000-0000-0000-000000000001",
284+
},
285+
},
286+
},
287+
)
288+
server.On("/v1/floating-ips", 200,
289+
[]cloudscale.FloatingIP{},
290+
)
291+
server.Start()
292+
defer server.Close()
293+
294+
mapper := lbMapper{client: server.Client()}
295+
296+
s := testkit.NewService("service").V1()
297+
s.Annotations = tc.annotations
298+
i := newServiceInfo(s, "")
299+
300+
actual, err := actualLbState(t.Context(), &mapper, i)
301+
assert.NoError(t, err)
302+
303+
assert.Equal(t, "k8test-service-test", actual.lb.Name)
304+
assert.Len(t, actual.pools, 1)
305+
assert.Len(t, actual.members, 1)
306+
assert.Len(t, actual.listeners, 1)
307+
assert.Len(t, actual.monitors, 1)
308+
309+
p := actual.pools[0]
310+
assert.Equal(t, "tcp/80", p.Name)
311+
assert.Equal(t, "10.0.0.1:8080", actual.members[p][0].Name)
312+
assert.Equal(t, "tcp/80", actual.listeners[p][0].Name)
313+
assert.Equal(t, "tcp", actual.monitors[p][0].Type)
314+
})
315+
}
278316
}
279317

280318
func TestNextLbActionsInvalidCalls(t *testing.T) {

‎pkg/internal/limiter/limiter.go‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,52 @@ func New[T any](err error, elements ...T) *Limiter[T] {
1818
}
1919
}
2020

21+
func Join[T any](limiters ...*Limiter[T]) *Limiter[T] {
22+
joined := &Limiter[T]{
23+
Error: nil,
24+
elements: []T{},
25+
}
26+
27+
for _, limiter := range limiters {
28+
29+
// Keep the first error
30+
if joined.Error != nil && limiter.Error != nil {
31+
joined.Error = limiter.Error
32+
}
33+
34+
// Append the elements
35+
joined.elements = append(joined.elements, limiter.elements...)
36+
}
37+
38+
return joined
39+
}
40+
41+
// Unique excludes duplicate elements, according to a comparison function.
42+
// This is meant for small lists as the complexity is O(n^2).
43+
func (t *Limiter[T]) Unique(eq func(a *T, b *T) bool) *Limiter[T] {
44+
result := []T{}
45+
46+
for _, a := range t.elements {
47+
add := true
48+
49+
for _, b := range result {
50+
if eq(&a, &b) {
51+
add = false
52+
53+
break
54+
}
55+
}
56+
57+
if add {
58+
result = append(result, a)
59+
}
60+
}
61+
62+
t.elements = result
63+
64+
return t
65+
}
66+
2167
// All returns the full set of answers.
2268
func (t *Limiter[T]) All() ([]T, error) {
2369
if t.Error != nil {
@@ -68,3 +114,12 @@ func (t *Limiter[T]) AtMostOne() (*T, error) {
68114

69115
return &t.elements[0], nil
70116
}
117+
118+
// Count returns the number of elements, if there is no error.
119+
func (t *Limiter[T]) Count() int {
120+
if t.Error != nil {
121+
return 0
122+
}
123+
124+
return len(t.elements)
125+
}

0 commit comments

Comments
 (0)