feat(LB): improve immutable changes error message (#253)

* feat: add annotation to `resultImmutableChanged` struct and include it in the error message if set

* fix: also set extra labels for existing LBs

* fix lint

* Update pkg/ccm/loadbalancer.go

Co-authored-by: Raphael Grömmer <38243306+RAPSNX@users.noreply.github.com>

---------

Co-authored-by: Raphael Grömmer <38243306+RAPSNX@users.noreply.github.com>

Author: Kumm-Kai

pkg/ccm/loadbalancer.go

@@ -106,7 +106,7 @@ func (l *LoadBalancer) GetLoadBalancerName(_ context.Context, _ string, service
 // load balancer is not ready yet (e.g., it is still being provisioned) and
 // polling at a fixed rate is preferred over backing off exponentially in
 // order to minimize latency.
-func (l *LoadBalancer) EnsureLoadBalancer(
+func (l *LoadBalancer) EnsureLoadBalancer( //nolint:gocyclo // not really complex
 	ctx context.Context,
 	clusterName string,
 	service *corev1.Service,
@@ -126,7 +126,7 @@ func (l *LoadBalancer) EnsureLoadBalancer(
 		return nil, fmt.Errorf("reconcile metricsRemoteWrite: %w", err)
 	}
 
-	spec, events, err := lbSpecFromService(service, nodes, l.networkID, observabilityOptions)
+	spec, events, err := lbSpecFromService(service, nodes, l.networkID, l.extraLabels, observabilityOptions)
 	if err != nil {
 		return nil, fmt.Errorf("invalid load balancer specification: %w", err)
 	}
@@ -137,7 +137,11 @@ func (l *LoadBalancer) EnsureLoadBalancer(
 
 	fulfills, immutableChanged := compareLBwithSpec(lb, spec)
 	if immutableChanged != nil {
-		return nil, fmt.Errorf("updated to load balancer cannot be fulfilled. Load balancer API doesn't support changing %q", immutableChanged.field)
+		changeStr := fmt.Sprintf("%q", immutableChanged.field)
+		if immutableChanged.annotation != "" {
+			changeStr += fmt.Sprintf(" (%q)", immutableChanged.annotation)
+		}
+		return nil, fmt.Errorf("update to load balancer cannot be fulfilled: API doesn't support changing %s", changeStr)
 	}
 	if !fulfills {
 		credentialsRefBeforeUpdate := getMetricsRemoteWriteRef(lb)
@@ -202,7 +206,7 @@ func (l *LoadBalancer) createLoadBalancer(ctx context.Context, clusterName strin
 		return nil, fmt.Errorf("reconcile metricsRemoteWrite: %w", err)
 	}
 
-	spec, events, err := lbSpecFromService(service, nodes, l.networkID, metricsRemoteWrite)
+	spec, events, err := lbSpecFromService(service, nodes, l.networkID, l.extraLabels, metricsRemoteWrite)
 	if err != nil {
 		return nil, fmt.Errorf("invalid load balancer specification: %w", err)
 	}
@@ -234,7 +238,7 @@ func (l *LoadBalancer) createLoadBalancer(ctx context.Context, clusterName strin
 // It is not called on controller start-up. EnsureLoadBalancer must also ensure to update targets.
 func (l *LoadBalancer) UpdateLoadBalancer(ctx context.Context, clusterName string, service *corev1.Service, nodes []*corev1.Node) error {
 	// only TargetPools are used from spec
-	spec, events, err := lbSpecFromService(service, nodes, l.networkID, nil)
+	spec, events, err := lbSpecFromService(service, nodes, l.networkID, l.extraLabels, nil)
 	if err != nil {
 		return fmt.Errorf("invalid service: %w", err)
 	}

pkg/ccm/loadbalancer_spec.go

@@ -10,6 +10,7 @@ import (
 	"github.com/stackitcloud/stackit-sdk-go/core/utils"
 	"github.com/stackitcloud/stackit-sdk-go/services/loadbalancer"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/utils/ptr"
 
 	"github.com/stackitcloud/cloud-provider-stackit/pkg/cmp"
 )
@@ -244,6 +245,7 @@ func lbSpecFromService( //nolint:funlen,gocyclo // It is long but not complex.
 	service *corev1.Service,
 	nodes []*corev1.Node,
 	networkID string,
+	extraLabels map[string]string,
 	observability *loadbalancer.LoadbalancerOptionObservability,
 ) (*loadbalancer.CreateLoadBalancerPayload, []Event, error) {
 	lb := &loadbalancer.CreateLoadBalancerPayload{
@@ -275,6 +277,14 @@ func lbSpecFromService( //nolint:funlen,gocyclo // It is long but not complex.
 		}
 	}
 
+	// Add extraLabels if set
+	if extraLabels != nil {
+		lb.Labels = ptr.To(extraLabels)
+	}
+
+	// Add metric metricsRemoteWrite settings
+	lb.Options.Observability = observability
+
 	events := make([]Event, 0)
 
 	// Parse private network from annotations.
@@ -342,9 +352,6 @@ func lbSpecFromService( //nolint:funlen,gocyclo // It is long but not complex.
 		lb.ExternalAddress = &externalIP
 	}
 
-	// Add metric metricsRemoteWrite settings
-	lb.Options.Observability = observability
-
 	// Parse TCP idle timeout from annotations.
 	// TODO: Split into separate function.
 	tcpIdleTimeout := defaultTCPIdleTimeout
@@ -576,7 +583,8 @@ func checkUnsupportedAnnotations(service *corev1.Service) *Event {
 // resultImmutableChanged denotes that at least one property that cannot be changed did change.
 // Attempting an update will fail.
 type resultImmutableChanged struct {
-	field string
+	field      string
+	annotation string
 }
 
 // compareLBwithSpec checks whether the load balancer fulfills the specification.
@@ -587,7 +595,7 @@ func compareLBwithSpec(lb *loadbalancer.LoadBalancer, spec *loadbalancer.CreateL
 	fulfills = true
 
 	if cmp.UnpackPtr(cmp.UnpackPtr(lb.Options).PrivateNetworkOnly) != cmp.UnpackPtr(cmp.UnpackPtr(spec.Options).PrivateNetworkOnly) {
-		return false, &resultImmutableChanged{field: ".options.privateNetworkOnly"}
+		return false, &resultImmutableChanged{field: ".options.privateNetworkOnly", annotation: internalLBAnnotation}
 	}
 
 	if !cmp.PtrValEqualFn(
@@ -620,7 +628,7 @@ func compareLBwithSpec(lb *loadbalancer.LoadBalancer, spec *loadbalancer.CreateL
 		// lb.ExternalAddress is set to the ephemeral IP if the load balancer is ephemeral, while spec will never contain an ephemeral IP.
 		// So we only compare them if the spec has a static IP.
 		if !cmp.PtrValEqual(lb.ExternalAddress, spec.ExternalAddress) {
-			return false, &resultImmutableChanged{field: ".externalAddress"}
+			return false, &resultImmutableChanged{field: ".externalAddress", annotation: externalIPAnnotation}
 		}
 		if cmp.UnpackPtr(cmp.UnpackPtr(lb.Options).EphemeralAddress) {
 			// Promote an ephemeral IP to a static IP.
@@ -629,7 +637,7 @@ func compareLBwithSpec(lb *loadbalancer.LoadBalancer, spec *loadbalancer.CreateL
 	} else if !cmp.UnpackPtr(cmp.UnpackPtr(lb.Options).PrivateNetworkOnly) &&
 		!cmp.UnpackPtr(cmp.UnpackPtr(lb.Options).EphemeralAddress) {
 		// Demotion is not allowed by the load balancer API.
-		return false, &resultImmutableChanged{field: ".options.ephemeralAddress"}
+		return false, &resultImmutableChanged{field: ".options.ephemeralAddress", annotation: externalIPAnnotation}
 	}
 
 	if cmp.LenSlicePtr(lb.Listeners) != cmp.LenSlicePtr(spec.Listeners) {
@@ -664,16 +672,16 @@ func compareLBwithSpec(lb *loadbalancer.LoadBalancer, spec *loadbalancer.CreateL
 	}
 
 	if cmp.LenSlicePtr(lb.Networks) != cmp.LenSlicePtr(spec.Networks) {
-		return false, &resultImmutableChanged{field: "len(.networks)"}
+		return false, &resultImmutableChanged{field: "len(.networks)", annotation: listenerNetworkAnnotation}
 	}
 	if cmp.LenSlicePtr(lb.Networks) > 0 {
 		for i, x := range *lb.Networks {
 			y := (*spec.Networks)[i]
 			if !cmp.PtrValEqual(x.NetworkId, y.NetworkId) {
-				return false, &resultImmutableChanged{field: fmt.Sprintf(".networks[%d].networkId", i)}
+				return false, &resultImmutableChanged{field: fmt.Sprintf(".networks[%d].networkId", i), annotation: listenerNetworkAnnotation}
 			}
 			if !cmp.PtrValEqual(x.Role, y.Role) {
-				return false, &resultImmutableChanged{field: fmt.Sprintf(".networks[%d].role", i)}
+				return false, &resultImmutableChanged{field: fmt.Sprintf(".networks[%d].role", i), annotation: listenerNetworkAnnotation}
 			}
 		}
 	}