Avoid unnecesary LB creation if UUID is missing

When the k8s.cloudscale.ch/loadbalancer-uuid annotation was present on
a service, but the UUID could not be found in via API, the CCM would
start creating LBs in a loop until it was killed.

The cause of this was the LBs were looked up. The CCM aassumed that if
a UUID could not be found, that the LB must not exist, and tried to
create one. This would happen in a loop without any way for the CCM to
detect that it should stop.

This assumption has now been changed. We now check for the LB both by
name and UUID, and expect to find at most one. If the service name and
the UUID both match different LBs (as might happen if one manipulates
the service annotations manually), the CCM will return an error.

Ultimately, the CCM has no state it can call its own and infers
state from the APIs it talks to (i.e., Kubernetes and the cloudscale.ch
API).

So if someone were to change both the UUID and the name of the LB using
annotations, the CCM would have no choice but to create a new LB.

Manipulating the UUID was at one point thought to be a possible feature:

One could bring an existing LB into the managament of the CCM. Though
there is no code that would prevent this, the docs no longer mention
this option, as this has the potential to cause havoc.

Changing the name is already discouraged. It is probably savest to
not touch the name, nor the UUID, during the lifetime of a service.

Author: href

pkg/cloudscale_ccm/lb_mapper.go

@@ -20,8 +20,19 @@ func (l *lbMapper) findByServiceInfo(
 	serviceInfo *serviceInfo,
 ) *limiter.Limiter[cloudscale.LoadBalancer] {
 
+	// If we have a UUID, look for both the service and the UUID. Usually
+	// we expect to only see one, but it is possible for the UUID to point
+	// to another LB than the service name, in which case we return both
+	// so the caller can decide if that is sane or not.
 	if uuid := serviceInfo.annotation(LoadBalancerUUID); uuid != "" {
-		return l.getByUUID(ctx, uuid)
+		return limiter.Join(
+			l.getByUUID(ctx, uuid),
+			l.findByName(ctx, serviceInfo.annotation(LoadBalancerName)),
+		).Unique(
+			func(a *cloudscale.LoadBalancer, b *cloudscale.LoadBalancer) bool {
+				return a.UUID == b.UUID
+			},
+		)
 	}
 
 	return l.findByName(ctx, serviceInfo.annotation(LoadBalancerName))

pkg/cloudscale_ccm/lb_mapper_test.go

@@ -37,19 +37,42 @@ func TestFindLoadBalancer(t *testing.T) {
 	lb, err := lbs.One()
 	assert.NoError(t, err)
 	assert.Equal(t, "foo", lb.Name)
+	assert.Equal(t, 1, lbs.Count())
 
 	// Using an ambiguous name
 	s.Annotations[LoadBalancerName] = "clone"
 
 	lbs = mapper.findByServiceInfo(t.Context(), i)
 	_, err = lbs.One()
 	assert.Error(t, err)
+	assert.Equal(t, 2, lbs.Count())
 
-	// Using a uuid
+	// Using a unique name and a mismatched uuid
+	s.Annotations[LoadBalancerName] = "foo"
 	s.Annotations[LoadBalancerUUID] = "85dffa20-8097-4d75-afa6-9e4372047ce6"
 
+	lbs = mapper.findByServiceInfo(t.Context(), i)
+	_, err = lbs.One()
+	assert.Error(t, err)
+	assert.Equal(t, 2, lbs.Count())
+
+	// Using a unique name and a missing uuid
+	s.Annotations[LoadBalancerName] = "foo"
+	s.Annotations[LoadBalancerUUID] = "00000000-0000-0000-0000-000000000000"
+
 	lbs = mapper.findByServiceInfo(t.Context(), i)
 	lb, err = lbs.One()
 	assert.NoError(t, err)
-	assert.Equal(t, "clone", lb.Name)
+	assert.Equal(t, "foo", lb.Name)
+	assert.Equal(t, 1, lbs.Count())
+
+	// Using a unique name and a matching uuid
+	s.Annotations[LoadBalancerName] = "foo"
+	s.Annotations[LoadBalancerUUID] = "c2e4aabd-8c91-46da-b069-71e01f439806"
+
+	lbs = mapper.findByServiceInfo(t.Context(), i)
+	lb, err = lbs.One()
+	assert.NoError(t, err)
+	assert.Equal(t, "foo", lb.Name)
+	assert.Equal(t, 1, lbs.Count())
 }

pkg/cloudscale_ccm/loadbalancer.go

@@ -21,12 +21,11 @@ import (
 // them, unless you know what you are doing.
 const (
 	// LoadBalancerUUID uniquely identifes the loadbalancer. This annotation
-	// should not be provided by the customer, unless the adoption of an
-	// existing load balancer is desired.
+	// should not be provided by the customer.
 	//
-	// In all other cases, this value is set by the CCM after creating the
-	// load balancer, to ensure that we track it with a proper ID and not
-	// a name that might change without our knowledge.
+	// Instead, this value is set by the CCM after creating the load balancer,
+	// to ensure that we track it with a proper ID and not a name that might
+	// change without our knowledge.
 	LoadBalancerUUID = "k8s.cloudscale.ch/loadbalancer-uuid"
 
 	// LoadBalancerConfigVersion is set by the CCM when it first handles a

pkg/internal/limiter/limiter.go

@@ -18,6 +18,52 @@ func New[T any](err error, elements ...T) *Limiter[T] {
 	}
 }
 
+func Join[T any](limiters ...*Limiter[T]) *Limiter[T] {
+	joined := &Limiter[T]{
+		Error:    nil,
+		elements: []T{},
+	}
+
+	for _, limiter := range limiters {
+
+		// Keep the first error
+		if joined.Error != nil && limiter.Error != nil {
+			joined.Error = limiter.Error
+		}
+
+		// Append the elements
+		joined.elements = append(joined.elements, limiter.elements...)
+	}
+
+	return joined
+}
+
+// Unique excludes duplicate elements, according to a comparison function.
+// This is meant for small lists as the complexity is O(n^2).
+func (t *Limiter[T]) Unique(eq func(a *T, b *T) bool) *Limiter[T] {
+	result := []T{}
+
+	for _, a := range t.elements {
+		add := true
+
+		for _, b := range result {
+			if eq(&a, &b) {
+				add = false
+
+				break
+			}
+		}
+
+		if add {
+			result = append(result, a)
+		}
+	}
+
+	t.elements = result
+
+	return t
+}
+
 // All returns the full set of answers.
 func (t *Limiter[T]) All() ([]T, error) {
 	if t.Error != nil {
@@ -68,3 +114,12 @@ func (t *Limiter[T]) AtMostOne() (*T, error) {
 
 	return &t.elements[0], nil
 }
+
+// Count returns the number of elements, if there is no error.
+func (t *Limiter[T]) Count() int {
+	if t.Error != nil {
+		return 0
+	}
+
+	return len(t.elements)
+}