From e68ffbcc1d34946074dd470c7afbd9f507b9af68 Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Tue, 22 Sep 2026 15:53:57 +0200 Subject: [PATCH 1/4] refactor: move load balancer target pool management to the cluster controller The StackitCluster reconciler now owns the whole apiserver target pool and derives it from the control-plane Machines of the cluster. --- cloud/client.go | 6 +- cloud/fake/client.go | 54 +++++---- cloud/sdk_client.go | 90 ++++++-------- cloud/services/loadbalancer/loadbalancer.go | 125 +++++++++++++------- cloud/types.go | 6 +- controller/stackitcluster_controller.go | 19 +++ controller/stackitcluster_infrastructure.go | 47 +++++++- controller/stackitmachine_infrastructure.go | 80 ------------- 8 files changed, 216 insertions(+), 211 deletions(-) diff --git a/cloud/client.go b/cloud/client.go index cf962a2..7038356 100644 --- a/cloud/client.go +++ b/cloud/client.go @@ -51,9 +51,9 @@ type Client interface { ListAPIServerLoadBalancersByTags(ctx context.Context, tags map[string]string) ([]*LoadBalancer, error) - EnsureAPIServerLoadBalancerTarget(ctx context.Context, input LoadBalancerTargetInput) error - - DeleteAPIServerLoadBalancerTarget(ctx context.Context, input LoadBalancerTargetInput) error + // SetAPIServerLoadBalancerTargets replaces the contents of the API-server + // target pool. STACKIT rejects an empty pool. + SetAPIServerLoadBalancerTargets(ctx context.Context, loadBalancerID string, port int32, targets []LoadBalancerTargetInput) error DeleteAPIServerLoadBalancer(ctx context.Context, id string) error } diff --git a/cloud/fake/client.go b/cloud/fake/client.go index 0642e49..217e509 100644 --- a/cloud/fake/client.go +++ b/cloud/fake/client.go @@ -47,8 +47,7 @@ type Client struct { FailNextFindServer error FailNextEnsureLB error FailNextDeleteLB error - FailNextEnsureTarget error - FailNextDeleteTarget error + FailNextSetTargets error FailNextGetNetwork error FailNextEnsureBastion error FailNextDeleteBastion error @@ -483,36 +482,34 @@ func (c *Client) ListAPIServerLoadBalancersByTags( return loadBalancers, nil } -func (c *Client) EnsureAPIServerLoadBalancerTarget(_ context.Context, input cloud.LoadBalancerTargetInput) error { +func (c *Client) SetAPIServerLoadBalancerTargets( + _ context.Context, + loadBalancerID string, + port int32, + targets []cloud.LoadBalancerTargetInput, +) error { c.mu.Lock() defer c.mu.Unlock() - if err := consume(&c.FailNextEnsureTarget); err != nil { + if err := consume(&c.FailNextSetTargets); err != nil { return err } - entry, ok := c.loadBalancers[input.LoadBalancerID] - if !ok { - return fmt.Errorf("load balancer %q: %w", input.LoadBalancerID, cloud.ErrNotFound) - } - if entry.targets[bootstrapTargetName] == bootstrapTargetIP { - delete(entry.targets, bootstrapTargetName) + if loadBalancerID == "" || port <= 0 { + return fmt.Errorf("load balancer ID and target port are required: %w", cloud.ErrInvalidInput) } - entry.targets[input.Name] = input.IP - return nil -} - -func (c *Client) DeleteAPIServerLoadBalancerTarget(_ context.Context, input cloud.LoadBalancerTargetInput) error { - c.mu.Lock() - defer c.mu.Unlock() - - if err := consume(&c.FailNextDeleteTarget); err != nil { - return err + if len(targets) == 0 { + return fmt.Errorf("at least one target is required: %w", cloud.ErrInvalidInput) } - entry, ok := c.loadBalancers[input.LoadBalancerID] + entry, ok := c.loadBalancers[loadBalancerID] if !ok { - return fmt.Errorf("load balancer %q: %w", input.LoadBalancerID, cloud.ErrNotFound) + return fmt.Errorf("load balancer %q: %w", loadBalancerID, cloud.ErrNotFound) } - delete(entry.targets, input.Name) + replaced := make(map[string]string, len(targets)) + for _, target := range targets { + replaced[target.Name] = target.IP + } + entry.targets = replaced + entry.lb.Port = port return nil } @@ -604,6 +601,17 @@ func (c *Client) LoadBalancerTargetCount(id string) int { return len(entry.targets) } +// LoadBalancerTargetIPs returns one target pool as name to IP (test helper). +func (c *Client) LoadBalancerTargetIPs(id string) map[string]string { + c.mu.Lock() + defer c.mu.Unlock() + entry, ok := c.loadBalancers[id] + if !ok { + return nil + } + return copyTags(entry.targets) +} + func mapContains(haystack, needle map[string]string) bool { for k, v := range needle { if haystack[k] != v { diff --git a/cloud/sdk_client.go b/cloud/sdk_client.go index fc02cb2..a0d47c9 100644 --- a/cloud/sdk_client.go +++ b/cloud/sdk_client.go @@ -568,11 +568,20 @@ func (c *SDKClient) ListAPIServerLoadBalancersByTags( return matched, nil } -func (c *SDKClient) EnsureAPIServerLoadBalancerTarget(ctx context.Context, input LoadBalancerTargetInput) error { - if input.LoadBalancerID == "" || input.Name == "" || input.IP == "" { - return fmt.Errorf("%w: load balancer ID, target name, and target IP are required", ErrInvalidInput) +func (c *SDKClient) SetAPIServerLoadBalancerTargets( + ctx context.Context, + loadBalancerID string, + port int32, + targets []LoadBalancerTargetInput, +) error { + if loadBalancerID == "" || port <= 0 { + return fmt.Errorf("%w: load balancer ID and target port are required", ErrInvalidInput) } - loadBalancer, err := c.lbClient.DefaultAPI.GetLoadBalancer(ctx, c.projectID, c.region, input.LoadBalancerID).Execute() + // STACKIT NLB target pools must contain at least one target. + if len(targets) == 0 { + return fmt.Errorf("%w: at least one target is required", ErrInvalidInput) + } + loadBalancer, err := c.lbClient.DefaultAPI.GetLoadBalancer(ctx, c.projectID, c.region, loadBalancerID).Execute() if err != nil { return classifySDKError("get load balancer", err) } @@ -581,68 +590,43 @@ func (c *SDKClient) EnsureAPIServerLoadBalancerTarget(ctx context.Context, input return fmt.Errorf( "%w: load balancer %q has no %q target pool", ErrNotFound, - input.LoadBalancerID, + loadBalancerID, apiserverTargetPoolName, ) } - targets := withoutBootstrapTarget(targetPool.GetTargets()) - for i := range targets { - if targets[i].GetDisplayName() == input.Name || targets[i].GetIp() == input.IP { - targets[i].SetDisplayName(input.Name) - targets[i].SetIp(input.IP) - return c.updateAPIServerTargetPool(ctx, input.LoadBalancerID, targetPool, targets, input.Port) + desired := make([]lb.Target, 0, len(targets)) + for _, targetInput := range targets { + if targetInput.Name == "" || targetInput.IP == "" { + return fmt.Errorf("%w: target name and target IP are required", ErrInvalidInput) } + target := lb.NewTarget() + target.SetDisplayName(targetInput.Name) + target.SetIp(targetInput.IP) + desired = append(desired, *target) } - - target := lb.NewTarget() - target.SetDisplayName(input.Name) - target.SetIp(input.IP) - targets = append(targets, *target) - return c.updateAPIServerTargetPool(ctx, input.LoadBalancerID, targetPool, targets, input.Port) -} - -func withoutBootstrapTarget(targets []lb.Target) []lb.Target { - out := targets[:0] - for _, target := range targets { - if target.GetDisplayName() == bootstrapTargetName { - continue - } - out = append(out, target) + // Control plane status updates are frequent, and each one reaches this path. + if targetPool.GetTargetPort() == port && sameTargets(targetPool.GetTargets(), desired) { + return nil } - return out + return c.updateAPIServerTargetPool(ctx, loadBalancerID, targetPool, desired, port) } -func (c *SDKClient) DeleteAPIServerLoadBalancerTarget(ctx context.Context, input LoadBalancerTargetInput) error { - if input.LoadBalancerID == "" || input.Name == "" { - return fmt.Errorf("%w: load balancer ID and target name are required", ErrInvalidInput) - } - loadBalancer, err := c.lbClient.DefaultAPI.GetLoadBalancer(ctx, c.projectID, c.region, input.LoadBalancerID).Execute() - if err != nil { - return classifySDKError("get load balancer", err) +func sameTargets(current, desired []lb.Target) bool { + if len(current) != len(desired) { + return false } - targetPool := apiServerTargetPool(loadBalancer) - if targetPool == nil { - return nil + byName := make(map[string]string, len(current)) + for _, target := range current { + byName[target.GetDisplayName()] = target.GetIp() } - - targets := targetPool.GetTargets() - out := make([]lb.Target, 0, len(targets)) - for _, target := range targets { - if target.GetDisplayName() == input.Name { - continue + for _, target := range desired { + ip, ok := byName[target.GetDisplayName()] + if !ok || ip != target.GetIp() { + return false } - out = append(out, target) - } - if len(out) == len(targets) { - return nil } - if len(out) == 0 { - // STACKIT NLB target pools must contain at least one target. Leave the - // last target in place; deleting the load balancer removes it. - return nil - } - return c.updateAPIServerTargetPool(ctx, input.LoadBalancerID, targetPool, out, input.Port) + return true } func (c *SDKClient) findLoadBalancerByTags(ctx context.Context, tags map[string]string) (*LoadBalancer, error) { diff --git a/cloud/services/loadbalancer/loadbalancer.go b/cloud/services/loadbalancer/loadbalancer.go index f5d2f3a..81711e5 100644 --- a/cloud/services/loadbalancer/loadbalancer.go +++ b/cloud/services/loadbalancer/loadbalancer.go @@ -12,7 +12,11 @@ package loadbalancer import ( "context" + "crypto/sha256" + "encoding/hex" "fmt" + "slices" + "strings" clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" @@ -21,7 +25,14 @@ import ( "github.com/stackitcloud/cluster-api-provider-stackit/util" ) -const defaultAPIServerPort int32 = 6443 +const ( + defaultAPIServerPort int32 = 6443 + + // maxTargetNameLength is the STACKIT limit on a target display name. + maxTargetNameLength = 63 + + targetNameDigestLength = 7 +) func APIServerTags(stackitCluster *infrav1.StackitCluster) map[string]string { return util.ClusterTags(stackitCluster.Name, stackitCluster.Namespace, stackitCluster.Spec.AdditionalLabels) @@ -42,25 +53,81 @@ func APIServerInput( } } -func BootstrapTarget(ip string) cloud.LoadBalancerTargetInput { +func bootstrapTarget(ip string) cloud.LoadBalancerTargetInput { return cloud.LoadBalancerTargetInput{ Name: "capi-bootstrap-placeholder", IP: ip, - Port: defaultAPIServerPort, } } -func TargetForMachine(machineName string, addresses []cloud.Address) (cloud.LoadBalancerTargetInput, error) { - ip := FirstInternalIP(addresses) - if ip == "" { - return cloud.LoadBalancerTargetInput{}, fmt.Errorf("%w: server has no internal IP address", cloud.ErrTransient) +// APIServerTargets builds the desired API server target pool, sorted by machine +// name so the pool can be compared without spurious updates. Machines without an +// internal IP are still provisioning; while none has one, the bootstrap +// placeholder keeps the pool non-empty as STACKIT requires. +func APIServerTargets(machines []*clusterv1.Machine, bootstrapIP string) []cloud.LoadBalancerTargetInput { + sorted := make([]*clusterv1.Machine, 0, len(machines)) + for _, machine := range machines { + if machine != nil { + sorted = append(sorted, machine) + } + } + slices.SortFunc(sorted, func(a, b *clusterv1.Machine) int { + return strings.Compare(a.Name, b.Name) + }) + + targets := make([]cloud.LoadBalancerTargetInput, 0, len(sorted)) + // A target IP must be unique within the pool, and a replacement machine can + // transiently report the IP of the one it replaces. + seen := make(map[string]struct{}, len(sorted)) + for _, machine := range sorted { + ip := firstInternalIP(machine.Status.Addresses) + if ip == "" { + continue + } + if _, duplicate := seen[ip]; duplicate { + continue + } + seen[ip] = struct{}{} + targets = append(targets, cloud.LoadBalancerTargetInput{Name: targetName(machine.Name), IP: ip}) + } + if len(targets) == 0 { + return []cloud.LoadBalancerTargetInput{bootstrapTarget(bootstrapIP)} + } + return targets +} + +// targetName turns a machine name into a valid STACKIT target display name: +// letters, digits and inner hyphens, at most 63 characters. A qualifying name is +// returned unchanged; any other carries a digest so that two machines cannot +// collapse onto one target. +func targetName(machineName string) string { + var sanitized strings.Builder + previousHyphen := false + for _, r := range machineName { + switch { + case (r >= '0' && r <= '9') || (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z'): + sanitized.WriteRune(r) + previousHyphen = false + case !previousHyphen: + sanitized.WriteByte('-') + previousHyphen = true + } + } + name := strings.Trim(sanitized.String(), "-") + if name == machineName && len(name) <= maxTargetNameLength { + return name } - return cloud.LoadBalancerTargetInput{ - Name: machineName, - IP: ip, - Port: defaultAPIServerPort, - }, nil + sum := sha256.Sum256([]byte(machineName)) + digest := hex.EncodeToString(sum[:])[:targetNameDigestLength] + if name == "" { + return digest + } + suffix := "-" + digest + if len(name) > maxTargetNameLength-len(suffix) { + name = strings.TrimRight(name[:maxTargetNameLength-len(suffix)], "-") + } + return name + suffix } func ResolveID( @@ -85,39 +152,9 @@ func ResolveID( return loadBalancers[0].ID, nil } -func EnsureForMachine( - ctx context.Context, - cloudClient cloud.Client, - stackitCluster *infrav1.StackitCluster, - machineName string, - addresses []cloud.Address, -) (string, error) { - loadBalancerID, err := ResolveID(ctx, cloudClient, stackitCluster) - if err != nil { - return "", err - } - if loadBalancerID != "" { - return loadBalancerID, nil - } - - target, err := TargetForMachine(machineName, addresses) - if err != nil { - return "", err - } - - loadBalancer, err := cloudClient.EnsureAPIServerLoadBalancer(ctx, APIServerInput(stackitCluster, []cloud.LoadBalancerTargetInput{target})) - if err != nil { - return "", err - } - if loadBalancer == nil || loadBalancer.ID == "" { - return "", fmt.Errorf("%w: API server load balancer ID is empty", cloud.ErrTransient) - } - return loadBalancer.ID, nil -} - -func FirstInternalIP(addresses []cloud.Address) string { +func firstInternalIP(addresses []clusterv1.MachineAddress) string { for _, address := range addresses { - if address.Type == string(clusterv1.MachineInternalIP) && address.Address != "" { + if address.Type == clusterv1.MachineInternalIP && address.Address != "" { return address.Address } } diff --git a/cloud/types.go b/cloud/types.go index ddc5d6d..2b7bf0a 100644 --- a/cloud/types.go +++ b/cloud/types.go @@ -127,8 +127,6 @@ type LoadBalancerInput struct { // LoadBalancerTargetInput describes a VM target in the API-server load // balancer target pool. type LoadBalancerTargetInput struct { - LoadBalancerID string - Name string - IP string - Port int32 + Name string + IP string } diff --git a/controller/stackitcluster_controller.go b/controller/stackitcluster_controller.go index 2c4be47..27c533f 100644 --- a/controller/stackitcluster_controller.go +++ b/controller/stackitcluster_controller.go @@ -118,6 +118,24 @@ func (r *StackitClusterReconciler) stackitClusterRequestsForCluster(_ context.Co }} } +// stackitClusterRequestsForMachine keeps the API server load balancer target +// pool in step with the control plane. Worker machines never appear in it. +func (r *StackitClusterReconciler) stackitClusterRequestsForMachine(ctx context.Context, obj client.Object) []reconcile.Request { + machine, ok := obj.(*clusterv1.Machine) + if !ok || !clusterutil.IsControlPlaneMachine(machine) { + return nil + } + cluster, err := clusterutil.GetClusterFromMetadata(ctx, r.Client, machine.ObjectMeta) + if err != nil { + logf.FromContext(ctx).Error(err, "Failed to resolve Cluster for machine watch", "object", client.ObjectKeyFromObject(obj)) + return nil + } + if cluster == nil { + return nil + } + return r.stackitClusterRequestsForCluster(ctx, cluster) +} + func (r *StackitClusterReconciler) stackitClusterRequestsForCloudInitRef(ctx context.Context, obj client.Object) []reconcile.Request { kind := "" switch obj.(type) { @@ -156,6 +174,7 @@ func (r *StackitClusterReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). For(&infrav1.StackitCluster{}). Watches(&clusterv1.Cluster{}, handler.EnqueueRequestsFromMapFunc(r.stackitClusterRequestsForCluster)). + Watches(&clusterv1.Machine{}, handler.EnqueueRequestsFromMapFunc(r.stackitClusterRequestsForMachine)). Watches(&corev1.ConfigMap{}, handler.EnqueueRequestsFromMapFunc(r.stackitClusterRequestsForCloudInitRef)). Watches(&corev1.Secret{}, handler.EnqueueRequestsFromMapFunc(r.stackitClusterRequestsForCloudInitRef)). Named("stackitcluster"). diff --git a/controller/stackitcluster_infrastructure.go b/controller/stackitcluster_infrastructure.go index 2ba49c8..525b8b1 100644 --- a/controller/stackitcluster_infrastructure.go +++ b/controller/stackitcluster_infrastructure.go @@ -89,12 +89,26 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterS ) if stackitCluster.Spec.APIServerLoadBalancer.Enabled { + // STACKIT takes an explicit target list rather than label-based + // membership, so exactly one reconciler owns the pool. Machines being + // deleted drop out here, before the node is drained. + machines, err := collections.GetFilteredMachinesForCluster( + ctx, + r.Client, + clusterScope.Cluster, + collections.ControlPlaneMachines(clusterScope.Cluster.Name), + collections.ActiveMachines, + ) + if err != nil { + return ctrl.Result{}, fmt.Errorf("list control plane machines: %w", err) + } + // The same targets seed the creation, so a load balancer deleted out of + // band comes back with the real control plane behind it. + targets := loadbalancerservice.APIServerTargets(machines.UnsortedList(), bootstrapTargetIP(network)) + loadBalancer, err := cloudClient.EnsureAPIServerLoadBalancer( ctx, - loadbalancerservice.APIServerInput( - stackitCluster, - []cloud.LoadBalancerTargetInput{loadbalancerservice.BootstrapTarget(bootstrapTargetIP(network))}, - ), + loadbalancerservice.APIServerInput(stackitCluster, targets), ) if err != nil { stackitCluster.Status.Ready = false @@ -133,6 +147,31 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterS Port: defaultAPIServerPort, } clusterScope.SetAPIServerEndpoint(endpoint) + + if err := cloudClient.SetAPIServerLoadBalancerTargets(ctx, loadBalancer.ID, defaultAPIServerPort, targets); err != nil { + // A permanent failure neither requeues nor returns an error, so + // without an event it would pass unnoticed. + if !cloud.IsRetryable(err) && r.Recorder != nil { + r.Recorder.Eventf( + stackitCluster, nil, corev1.EventTypeWarning, "LoadBalancerTargetError", "Update", + "Cannot update API server load balancer target pool: %v", err, + ) + } + // Scoped to the load balancer condition on purpose: the machine + // reconciler stops while the cluster is not ready, so failing the + // cluster would block replacing the machine whose target is broken. + return util.CloudFailureResult( + &stackitCluster.Status.Conditions, + stackitCluster.Generation, + "LoadBalancerTargetError", + err, + retryableErrorRequeueAfter, + false, + infrav1.ClusterLoadBalancerReadyCondition, + ) + } + log.V(1).Info("Reconciled API server load balancer target pool", "targets", len(targets)) + clusterScope.SetConditions( metav1.ConditionTrue, "Available", diff --git a/controller/stackitmachine_infrastructure.go b/controller/stackitmachine_infrastructure.go index 114ff9d..0ac11de 100644 --- a/controller/stackitmachine_infrastructure.go +++ b/controller/stackitmachine_infrastructure.go @@ -28,7 +28,6 @@ import ( infrav1 "github.com/stackitcloud/cluster-api-provider-stackit/api/v1alpha1" "github.com/stackitcloud/cluster-api-provider-stackit/cloud" bastionservice "github.com/stackitcloud/cluster-api-provider-stackit/cloud/services/bastion" - loadbalancerservice "github.com/stackitcloud/cluster-api-provider-stackit/cloud/services/loadbalancer" "github.com/stackitcloud/cluster-api-provider-stackit/scope" "github.com/stackitcloud/cluster-api-provider-stackit/util" ) @@ -152,19 +151,6 @@ func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, machineS ) } - if err := r.reconcileAPIServerLoadBalancerTarget(ctx, cloudClient, machineScope, server); err != nil { - machineScope.SetNotReady("LoadBalancerTargetError", err.Error(), infrav1.MachineReadyCondition) - return util.CloudFailureResult( - &stackitMachine.Status.Conditions, - stackitMachine.Generation, - "LoadBalancerTargetError", - err, - retryableErrorRequeueAfter, - true, - infrav1.MachineReadyCondition, - ) - } - machineScope.SetReady() log.V(1).Info("StackitMachine ready", "providerID", providerID) return ctrl.Result{}, nil @@ -212,10 +198,6 @@ func (r *StackitMachineReconciler) reconcileDelete(ctx context.Context, machineS ) return resultErr } - if err := r.deleteAPIServerLoadBalancerTarget(ctx, cloudClient, machineScope); err != nil { - return err - } - instanceID, err := r.resolveServerForDeletion(ctx, cloudClient, machineScope) if err != nil { return err @@ -357,60 +339,6 @@ func (r *StackitMachineReconciler) reconcileBastionNodeSSHAccess( return err } -func (r *StackitMachineReconciler) reconcileAPIServerLoadBalancerTarget( - ctx context.Context, - cloudClient cloud.Client, - machineScope *scope.MachineScope, - server *cloud.Server, -) error { - if !isControlPlaneMachine(machineScope.Machine) || !machineScope.StackitCluster.Spec.APIServerLoadBalancer.Enabled { - return nil - } - loadBalancerID, err := loadbalancerservice.EnsureForMachine( - ctx, - cloudClient, - machineScope.StackitCluster, - machineScope.Machine.Name, - server.Addresses, - ) - if err != nil { - return err - } - - target, err := loadbalancerservice.TargetForMachine(machineScope.Machine.Name, server.Addresses) - if err != nil { - return err - } - target.LoadBalancerID = loadBalancerID - return cloudClient.EnsureAPIServerLoadBalancerTarget(ctx, target) -} - -func (r *StackitMachineReconciler) deleteAPIServerLoadBalancerTarget( - ctx context.Context, - cloudClient cloud.Client, - machineScope *scope.MachineScope, -) error { - if !isControlPlaneMachine(machineScope.Machine) || !machineScope.StackitCluster.Spec.APIServerLoadBalancer.Enabled { - return nil - } - loadBalancerID, err := loadbalancerservice.ResolveID(ctx, cloudClient, machineScope.StackitCluster) - if err != nil { - return err - } - if loadBalancerID == "" { - return nil - } - err = cloudClient.DeleteAPIServerLoadBalancerTarget(ctx, cloud.LoadBalancerTargetInput{ - LoadBalancerID: loadBalancerID, - Name: machineScope.Machine.Name, - Port: defaultAPIServerPort, - }) - if cloud.IsNotFound(err) { - return nil - } - return err -} - func (r *StackitMachineReconciler) getStackitCluster(ctx context.Context, cluster *clusterv1.Cluster) (*infrav1.StackitCluster, error) { if cluster.Spec.InfrastructureRef.Name == "" { return nil, nil @@ -440,11 +368,3 @@ func machineAddressesFromCloud(in []cloud.Address) []clusterv1.MachineAddress { } return out } - -func isControlPlaneMachine(machine *clusterv1.Machine) bool { - if machine == nil { - return false - } - _, ok := machine.Labels[clusterv1.MachineControlPlaneLabel] - return ok -} From 32f109ed174cf25dc9d0b2ac82d6b3e05fecbf65 Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Tue, 22 Sep 2026 15:54:32 +0200 Subject: [PATCH 2/4] test: cover target pool ownership in the cluster controller --- cloud/sdk_client_integration_test.go | 21 +-- cloud/sdk_client_test.go | 53 ++++-- .../loadbalancer/loadbalancer_test.go | 170 ++++++++++++++++++ controller/controller_test_helpers_test.go | 66 +++---- controller/stackitcluster_controller_test.go | 120 +++++++++++++ controller/stackitmachine_controller_test.go | 78 ++------ 6 files changed, 371 insertions(+), 137 deletions(-) create mode 100644 cloud/services/loadbalancer/loadbalancer_test.go diff --git a/cloud/sdk_client_integration_test.go b/cloud/sdk_client_integration_test.go index c4db3a0..0fd1594 100644 --- a/cloud/sdk_client_integration_test.go +++ b/cloud/sdk_client_integration_test.go @@ -69,7 +69,6 @@ func TestSDKClientLoadBalancerCreateDeleteIntegration(t *testing.T) { loadBalancer := createIntegrationLoadBalancer(t, client, networkID, LoadBalancerTargetInput{ Name: "capistackit-initial-target", IP: targetIP, - Port: 6443, }) if err := client.DeleteAPIServerLoadBalancer(context.Background(), loadBalancer.ID); err != nil { @@ -81,10 +80,10 @@ func TestSDKClientLoadBalancerTargetIntegration(t *testing.T) { client := newIntegrationClient(t) networkID := requiredIntegrationEnv(t, envIntegrationNetworkID) targetIP := requiredIntegrationEnv(t, envIntegrationTargetIP) + initialIP := integrationInitialTargetIP(targetIP) loadBalancer := createIntegrationLoadBalancer(t, client, networkID, LoadBalancerTargetInput{ Name: "capistackit-initial-target", - IP: integrationInitialTargetIP(targetIP), - Port: 6443, + IP: initialIP, }) t.Cleanup(func() { if err := client.DeleteAPIServerLoadBalancer(context.Background(), loadBalancer.ID); err != nil && !IsNotFound(err) { @@ -92,17 +91,15 @@ func TestSDKClientLoadBalancerTargetIntegration(t *testing.T) { } }) - target := LoadBalancerTargetInput{ - LoadBalancerID: loadBalancer.ID, - Name: "capistackit-integration-target", - IP: targetIP, - Port: 6443, + targets := []LoadBalancerTargetInput{ + {Name: "capistackit-initial-target", IP: initialIP}, + {Name: "capistackit-integration-target", IP: targetIP}, } - if err := client.EnsureAPIServerLoadBalancerTarget(context.Background(), target); err != nil { - t.Fatalf("EnsureAPIServerLoadBalancerTarget() error = %v", err) + if err := client.SetAPIServerLoadBalancerTargets(context.Background(), loadBalancer.ID, 6443, targets); err != nil { + t.Fatalf("SetAPIServerLoadBalancerTargets() error = %v", err) } - if err := client.DeleteAPIServerLoadBalancerTarget(context.Background(), target); err != nil { - t.Fatalf("DeleteAPIServerLoadBalancerTarget() error = %v", err) + if err := client.SetAPIServerLoadBalancerTargets(context.Background(), loadBalancer.ID, 6443, targets[:1]); err != nil { + t.Fatalf("SetAPIServerLoadBalancerTargets() shrink error = %v", err) } } diff --git a/cloud/sdk_client_test.go b/cloud/sdk_client_test.go index 11b0888..52ebf00 100644 --- a/cloud/sdk_client_test.go +++ b/cloud/sdk_client_test.go @@ -211,7 +211,7 @@ func TestSDKClientEnsureAPIServerLoadBalancerUsesBootstrapTargetWhenInitialTarge assertNestedStringField(t, createPayload, []string{"listeners", "0", "targetPool"}, apiserverTargetPoolName) } -func TestSDKClientLoadBalancerTargetUpdates(t *testing.T) { +func TestSDKClientSetsAPIServerTargetPool(t *testing.T) { var mu sync.Mutex targets := []any{ map[string]any{"displayName": "cp-0", "ip": "10.0.0.10"}, @@ -247,30 +247,57 @@ func TestSDKClientLoadBalancerTargetUpdates(t *testing.T) { })) client := newTestSDKClient(t, server.URL) - input := LoadBalancerTargetInput{ - LoadBalancerID: "apiserver-test", - Name: "cp-1", - IP: "10.0.0.11", - Port: 6443, + desired := []LoadBalancerTargetInput{ + {Name: "cp-0", IP: "10.0.0.10"}, + {Name: "cp-1", IP: "10.0.0.11"}, } - if err := client.EnsureAPIServerLoadBalancerTarget(context.Background(), input); err != nil { - t.Fatalf("EnsureAPIServerLoadBalancerTarget() error = %v", err) + if err := client.SetAPIServerLoadBalancerTargets(context.Background(), "apiserver-test", 6443, desired); err != nil { + t.Fatalf("SetAPIServerLoadBalancerTargets() error = %v", err) } - if err := client.DeleteAPIServerLoadBalancerTarget(context.Background(), input); err != nil { - t.Fatalf("DeleteAPIServerLoadBalancerTarget() error = %v", err) + if len(updatePayloads) != 1 { + t.Fatalf("got %d update payloads, want 1", len(updatePayloads)) } + assertNestedStringField(t, updatePayloads[0], []string{"targets", "0", "displayName"}, "cp-0") + assertNestedStringField(t, updatePayloads[0], []string{"targets", "1", "displayName"}, "cp-1") + assertNestedStringField(t, updatePayloads[0], []string{"targets", "1", "ip"}, "10.0.0.11") + // An unchanged pool must not turn into a write. + if err := client.SetAPIServerLoadBalancerTargets(context.Background(), "apiserver-test", 6443, desired); err != nil { + t.Fatalf("SetAPIServerLoadBalancerTargets() repeat error = %v", err) + } + if len(updatePayloads) != 1 { + t.Fatalf("got %d update payloads after an unchanged set, want 1", len(updatePayloads)) + } + + if err := client.SetAPIServerLoadBalancerTargets( + context.Background(), + "apiserver-test", + 6443, + desired[:1], + ); err != nil { + t.Fatalf("SetAPIServerLoadBalancerTargets() shrink error = %v", err) + } if len(updatePayloads) != 2 { t.Fatalf("got %d update payloads, want 2", len(updatePayloads)) } - assertNestedStringField(t, updatePayloads[0], []string{"targets", "1", "displayName"}, "cp-1") - assertNestedStringField(t, updatePayloads[0], []string{"targets", "1", "ip"}, "10.0.0.11") if got := nestedValue(t, updatePayloads[1], []string{"targets"}).([]any); len(got) != 1 { - t.Fatalf("delete target payload targets = %#v, want one remaining target", got) + t.Fatalf("shrunk target payload targets = %#v, want one remaining target", got) } assertNestedStringField(t, updatePayloads[1], []string{"targets", "0", "displayName"}, "cp-0") } +func TestSDKClientRejectsEmptyAPIServerTargetPool(t *testing.T) { + server := newSDKTestServer(t, http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + t.Fatalf("unexpected request: %s %s", r.Method, r.URL.String()) + })) + + client := newTestSDKClient(t, server.URL) + err := client.SetAPIServerLoadBalancerTargets(context.Background(), "apiserver-test", 6443, nil) + if !IsInvalidInput(err) { + t.Fatalf("SetAPIServerLoadBalancerTargets() error = %v, want ErrInvalidInput", err) + } +} + func TestSDKClientClassifiesHTTPStatusCodes(t *testing.T) { tests := []struct { name string diff --git a/cloud/services/loadbalancer/loadbalancer_test.go b/cloud/services/loadbalancer/loadbalancer_test.go new file mode 100644 index 0000000..6640255 --- /dev/null +++ b/cloud/services/loadbalancer/loadbalancer_test.go @@ -0,0 +1,170 @@ +/* +Copyright 2026. + +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 +*/ + +package loadbalancer + +import ( + "crypto/sha256" + "encoding/hex" + "regexp" + "strings" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" + + "github.com/stackitcloud/cluster-api-provider-stackit/cloud" +) + +func machineWithAddresses(name string, addresses ...clusterv1.MachineAddress) *clusterv1.Machine { + return &clusterv1.Machine{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Status: clusterv1.MachineStatus{Addresses: addresses}, + } +} + +func internalIP(ip string) clusterv1.MachineAddress { + return clusterv1.MachineAddress{Type: clusterv1.MachineInternalIP, Address: ip} +} + +func TestAPIServerTargets(t *testing.T) { + tests := []struct { + name string + machines []*clusterv1.Machine + want []cloud.LoadBalancerTargetInput + }{ + { + name: "sorts targets by machine name", + machines: []*clusterv1.Machine{ + machineWithAddresses("cp-2", internalIP("10.0.0.12")), + machineWithAddresses("cp-0", internalIP("10.0.0.10")), + machineWithAddresses("cp-1", internalIP("10.0.0.11")), + }, + want: []cloud.LoadBalancerTargetInput{ + {Name: "cp-0", IP: "10.0.0.10"}, + {Name: "cp-1", IP: "10.0.0.11"}, + {Name: "cp-2", IP: "10.0.0.12"}, + }, + }, + { + name: "skips machines that have no internal IP yet", + machines: []*clusterv1.Machine{ + machineWithAddresses("cp-0", internalIP("10.0.0.10")), + machineWithAddresses("cp-1"), + machineWithAddresses("cp-2", clusterv1.MachineAddress{ + Type: clusterv1.MachineExternalIP, + Address: "203.0.113.10", + }), + }, + want: []cloud.LoadBalancerTargetInput{{Name: "cp-0", IP: "10.0.0.10"}}, + }, + { + name: "drops a duplicate IP address", + machines: []*clusterv1.Machine{ + machineWithAddresses("cp-1", internalIP("10.0.0.10")), + machineWithAddresses("cp-0", internalIP("10.0.0.10")), + }, + want: []cloud.LoadBalancerTargetInput{{Name: "cp-0", IP: "10.0.0.10"}}, + }, + { + name: "falls back to the bootstrap placeholder without machines", + machines: nil, + want: []cloud.LoadBalancerTargetInput{{Name: "capi-bootstrap-placeholder", IP: "10.0.0.10"}}, + }, + { + name: "tolerates nil entries", + machines: []*clusterv1.Machine{nil, machineWithAddresses("cp-0", internalIP("10.0.0.11"))}, + want: []cloud.LoadBalancerTargetInput{{Name: "cp-0", IP: "10.0.0.11"}}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got := APIServerTargets(test.machines, "10.0.0.10") + if len(got) != len(test.want) { + t.Fatalf("APIServerTargets() = %#v, want %#v", got, test.want) + } + for i := range got { + if got[i] != test.want[i] { + t.Errorf("APIServerTargets()[%d] = %#v, want %#v", i, got[i], test.want[i]) + } + } + }) + } +} + +func TestTargetName(t *testing.T) { + longName := strings.Repeat("a", 70) + + tests := []struct { + name string + machineName string + want string + }{ + { + name: "leaves a valid name untouched", + machineName: "cluster-control-plane-abcde", + want: "cluster-control-plane-abcde", + }, + { + name: "replaces characters the load balancer API rejects", + machineName: "foo.bar-control-plane-abcde", + // SHA-256 of the machine name, first seven hex characters. + want: "foo-bar-control-plane-abcde-" + digestPrefix("foo.bar-control-plane-abcde"), + }, + { + name: "collapses a run of invalid characters and trims the edges", + machineName: ".foo..bar.", + want: "foo-bar-" + digestPrefix(".foo..bar."), + }, + { + name: "shortens an overlong name", + machineName: longName, + want: strings.Repeat("a", 55) + "-" + digestPrefix(longName), + }, + { + name: "falls back to the digest when nothing survives sanitizing", + machineName: "...", + want: digestPrefix("..."), + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got := targetName(test.machineName) + if got != test.want { + t.Fatalf("targetName(%q) = %q, want %q", test.machineName, got, test.want) + } + // One bad name makes the API reject the whole pool. + if !validTargetName.MatchString(got) { + t.Errorf("targetName(%q) = %q, which the load balancer API rejects", test.machineName, got) + } + }) + } +} + +// validTargetName is the pattern STACKIT enforces on a target display name. +var validTargetName = regexp.MustCompile(`^[0-9a-zA-Z](?:(?:[0-9a-zA-Z]|-){0,61}[0-9a-zA-Z])?$`) + +func TestTargetNameKeepsCollidingMachinesApart(t *testing.T) { + // Both names sanitize to the same string; only the digest tells them apart. + first := targetName("foo.bar-a") + second := targetName("foo-bar-a") + if first == second { + t.Fatalf("targetName() returned %q for both machine names", first) + } +} + +// digestPrefix recomputes the expected suffix so that the expectations do not +// derive from the code under test. +func digestPrefix(machineName string) string { + digest := sha256.Sum256([]byte(machineName)) + return hex.EncodeToString(digest[:])[:targetNameDigestLength] +} diff --git a/controller/controller_test_helpers_test.go b/controller/controller_test_helpers_test.go index d103137..42d828f 100644 --- a/controller/controller_test_helpers_test.go +++ b/controller/controller_test_helpers_test.go @@ -13,17 +13,16 @@ package controller import ( "context" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/reconcile" clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" infrav1 "github.com/stackitcloud/cluster-api-provider-stackit/api/v1alpha1" - "github.com/stackitcloud/cluster-api-provider-stackit/cloud" ) const ( @@ -140,52 +139,29 @@ func updateMachineControlPlaneLabel(ctx context.Context, name string) { Expect(k8sClient.Update(ctx, machine)).To(Succeed()) } -type loadBalancerEnsurer interface { - EnsureAPIServerLoadBalancer(context.Context, cloud.LoadBalancerInput) (*cloud.LoadBalancer, error) -} - -func createAPIServerLoadBalancer(ctx context.Context, cloudClient loadBalancerEnsurer) string { - lb, err := cloudClient.EnsureAPIServerLoadBalancer(ctx, cloud.LoadBalancerInput{ - Name: "apiserver", - ProjectID: testProjectID, - Region: "eu01", - NetworkID: testNetworkID, - Port: 6443, - Tags: map[string]string{"test": "apiserver"}, +// createControlPlaneMachine creates a control plane Machine and registers its +// cleanup. An empty ip leaves it without addresses, as while provisioning. +func createControlPlaneMachine(ctx context.Context, name, clusterName, ip string) { + createOwnerMachine(ctx, name, clusterName, "stackit-"+name) + DeferCleanup(func() { + deleteIfExists(ctx, &clusterv1.Machine{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "default"}}) }) - Expect(err).NotTo(HaveOccurred()) - return lb.ID -} - -func updateStackitClusterLoadBalancer(ctx context.Context, name, namespace, loadBalancerID string) { - stackitCluster := &infrav1.StackitCluster{} - Expect(k8sClient.Get(ctx, client.ObjectKey{Name: name, Namespace: namespace}, stackitCluster)).To(Succeed()) - stackitCluster.Spec.APIServerLoadBalancer.Enabled = true - Expect(k8sClient.Update(ctx, stackitCluster)).To(Succeed()) - Expect(k8sClient.Get(ctx, client.ObjectKey{Name: name, Namespace: namespace}, stackitCluster)).To(Succeed()) - stackitCluster.Status.APIServerLoadBalancerID = loadBalancerID - Expect(k8sClient.Status().Update(ctx, stackitCluster)).To(Succeed()) -} - -func enableStackitClusterLoadBalancer(ctx context.Context, name, namespace string) { - stackitCluster := &infrav1.StackitCluster{} - Expect(k8sClient.Get(ctx, client.ObjectKey{Name: name, Namespace: namespace}, stackitCluster)).To(Succeed()) - stackitCluster.Spec.APIServerLoadBalancer.Enabled = true - Expect(k8sClient.Update(ctx, stackitCluster)).To(Succeed()) + updateMachineControlPlaneLabel(ctx, name) + if ip != "" { + setMachineInternalIP(ctx, name, ip) + } } -func reconcileStackitClusterOnce(ctx context.Context, name, namespace string, cloudClient cloud.Client) { - reconciler := &StackitClusterReconciler{ - Client: k8sClient, - Scheme: k8sClient.Scheme(), - CloudClientFactory: func(context.Context, cloud.Credentials) (cloud.Client, error) { - return cloudClient, nil - }, - } - _, err := reconciler.Reconcile(ctx, reconcile.Request{ - NamespacedName: client.ObjectKey{Name: name, Namespace: namespace}, - }) - Expect(err).NotTo(HaveOccurred()) +// setMachineInternalIP stands in for the Cluster API machine controller, which +// copies status.addresses from the StackitMachine but does not run in envtest. +func setMachineInternalIP(ctx context.Context, name, ip string) { + machine := &clusterv1.Machine{} + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: name, Namespace: "default"}, machine)).To(Succeed()) + machine.Status.Addresses = []clusterv1.MachineAddress{{ + Type: clusterv1.MachineInternalIP, + Address: ip, + }} + Expect(k8sClient.Status().Update(ctx, machine)).To(Succeed()) } func expectCondition(conditions []metav1.Condition, conditionType string, status metav1.ConditionStatus, reason string) { diff --git a/controller/stackitcluster_controller_test.go b/controller/stackitcluster_controller_test.go index 1ca1de0..7a033bf 100644 --- a/controller/stackitcluster_controller_test.go +++ b/controller/stackitcluster_controller_test.go @@ -681,6 +681,126 @@ var _ = Describe("StackitCluster Controller", func() { Expect(got.Finalizers).To(ContainElement(infrav1.ClusterFinalizer)) }) + It("fills the target pool with the control plane machine addresses", func() { + createControlPlaneMachine(ctx, "cp-0-"+clusterName, clusterName, "10.0.0.11") + createControlPlaneMachine(ctx, "cp-1-"+clusterName, clusterName, "10.0.0.12") + + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + got := &infrav1.StackitCluster{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + Expect(fakeCloud.LoadBalancerTargetIPs(got.Status.APIServerLoadBalancerID)).To(Equal(map[string]string{ + "cp-0-" + clusterName: "10.0.0.11", + "cp-1-" + clusterName: "10.0.0.12", + })) + expectCondition(got.Status.Conditions, infrav1.ClusterLoadBalancerReadyCondition, metav1.ConditionTrue, "Available") + }) + + It("keeps the bootstrap placeholder while no control plane machine has an address", func() { + // STACKIT rejects an empty target pool. + createControlPlaneMachine(ctx, "cp-0-"+clusterName, clusterName, "") + + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + got := &infrav1.StackitCluster{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + Expect(fakeCloud.LoadBalancerTargetIPs(got.Status.APIServerLoadBalancerID)).To(Equal(map[string]string{ + "capi-bootstrap-placeholder": "10.0.0.1", + })) + }) + + It("keeps worker machines out of the target pool", func() { + machineName := "worker-" + clusterName + createOwnerMachine(ctx, machineName, clusterName, "stackit-"+machineName) + DeferCleanup(func() { + deleteIfExists(ctx, &clusterv1.Machine{ObjectMeta: metav1.ObjectMeta{Name: machineName, Namespace: namespace}}) + }) + setMachineInternalIP(ctx, machineName, "10.0.0.11") + + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + got := &infrav1.StackitCluster{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + Expect(fakeCloud.LoadBalancerTargetIPs(got.Status.APIServerLoadBalancerID)).To(Equal(map[string]string{ + "capi-bootstrap-placeholder": "10.0.0.1", + })) + }) + + It("drops a control plane machine that is being deleted from the target pool", func() { + machineName := "cp-0-" + clusterName + createControlPlaneMachine(ctx, machineName, clusterName, "10.0.0.11") + + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + got := &infrav1.StackitCluster{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + loadBalancerID := got.Status.APIServerLoadBalancerID + Expect(fakeCloud.LoadBalancerTargetCount(loadBalancerID)).To(Equal(1)) + + By("letting the Machine linger with a deletion timestamp") + machine := &clusterv1.Machine{} + machineKey := types.NamespacedName{Name: machineName, Namespace: namespace} + Expect(k8sClient.Get(ctx, machineKey, machine)).To(Succeed()) + machine.Finalizers = append(machine.Finalizers, "test.stackit.cloud/block-deletion") + Expect(k8sClient.Update(ctx, machine)).To(Succeed()) + Expect(k8sClient.Delete(ctx, machine)).To(Succeed()) + + _, err = reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + // The target goes before the node is drained, not after. + Expect(fakeCloud.LoadBalancerTargetIPs(loadBalancerID)).To(Equal(map[string]string{ + "capi-bootstrap-placeholder": "10.0.0.1", + })) + }) + + It("requeues without failing the cluster when the target pool update fails", func() { + createControlPlaneMachine(ctx, "cp-0-"+clusterName, clusterName, "10.0.0.11") + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + got := &infrav1.StackitCluster{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + Expect(got.Status.Ready).To(BeTrue()) + + fakeCloud.FailNextSetTargets = fmt.Errorf("update target pool timeout: %w", cloud.ErrTransient) + result, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(BeNumerically(">", 0)) + + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + expectCondition(got.Status.Conditions, infrav1.ClusterLoadBalancerReadyCondition, metav1.ConditionFalse, "LoadBalancerTargetError") + // The machine reconciler stops while the cluster is not ready, so a + // broken target pool must not take the cluster down with it. + Expect(got.Status.Ready).To(BeTrue()) + expectCondition(got.Status.Conditions, infrav1.ClusterReadyCondition, metav1.ConditionTrue, "Available") + }) + + It("maps control plane Machine events to StackitCluster reconcile requests", func() { + machineName := "cp-0-" + clusterName + createControlPlaneMachine(ctx, machineName, clusterName, "10.0.0.11") + + machine := &clusterv1.Machine{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: machineName, Namespace: namespace}, machine)).To(Succeed()) + Expect(reconciler.stackitClusterRequestsForMachine(ctx, machine)).To(Equal([]reconcile.Request{request})) + }) + + It("ignores worker Machine events", func() { + machineName := "worker-" + clusterName + createOwnerMachine(ctx, machineName, clusterName, "stackit-"+machineName) + DeferCleanup(func() { + deleteIfExists(ctx, &clusterv1.Machine{ObjectMeta: metav1.ObjectMeta{Name: machineName, Namespace: namespace}}) + }) + + machine := &clusterv1.Machine{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: machineName, Namespace: namespace}, machine)).To(Succeed()) + Expect(reconciler.stackitClusterRequestsForMachine(ctx, machine)).To(BeEmpty()) + }) + It("maps owning Cluster events to StackitCluster reconcile requests", func() { cluster := &clusterv1.Cluster{} Expect(k8sClient.Get(ctx, types.NamespacedName{Name: clusterName, Namespace: namespace}, cluster)).To(Succeed()) diff --git a/controller/stackitmachine_controller_test.go b/controller/stackitmachine_controller_test.go index 968f5f9..8fb254c 100644 --- a/controller/stackitmachine_controller_test.go +++ b/controller/stackitmachine_controller_test.go @@ -34,18 +34,17 @@ var _ = Describe("StackitMachine Controller", func() { const namespace = "default" var ( - ctx context.Context - clusterName string - machineName string - stackitName string - credentials string - fakeCloud *cloudfake.Client - reconciler *StackitMachineReconciler - request reconcile.Request - stackitKey types.NamespacedName - stackitMach *infrav1.StackitMachine - bootstrapName string - setupControlPlaneLoadBalancer func() + ctx context.Context + clusterName string + machineName string + stackitName string + credentials string + fakeCloud *cloudfake.Client + reconciler *StackitMachineReconciler + request reconcile.Request + stackitKey types.NamespacedName + stackitMach *infrav1.StackitMachine + bootstrapName string ) BeforeEach(func() { @@ -73,11 +72,6 @@ var _ = Describe("StackitMachine Controller", func() { createOwnerMachine(ctx, machineName, clusterName, stackitName) stackitMach = newStackitMachine(stackitName, namespace, machineName) Expect(k8sClient.Create(ctx, stackitMach)).To(Succeed()) - setupControlPlaneLoadBalancer = func() { - updateMachineControlPlaneLabel(ctx, machineName) - enableStackitClusterLoadBalancer(ctx, clusterName, namespace) - reconcileStackitClusterOnce(ctx, clusterName, namespace, fakeCloud) - } }) AfterEach(func() { @@ -338,49 +332,11 @@ var _ = Describe("StackitMachine Controller", func() { expectCondition(got.Status.Conditions, infrav1.MachineReadyCondition, metav1.ConditionFalse, "InstanceError") }) - It("registers control plane VMs as API server load balancer targets", func() { - setupControlPlaneLoadBalancer() - - result, err := reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - Expect(result).To(Equal(reconcile.Result{})) - Expect(fakeCloud.ServerCount()).To(Equal(1)) - Expect(fakeCloud.LoadBalancerCount()).To(Equal(1)) - - stackitCluster := &infrav1.StackitCluster{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Name: clusterName, Namespace: namespace}, stackitCluster)).To(Succeed()) - loadBalancerID := stackitCluster.Status.APIServerLoadBalancerID - Expect(loadBalancerID).NotTo(BeEmpty()) - Expect(fakeCloud.LoadBalancerTargetCount(loadBalancerID)).To(Equal(1)) - }) - - It("requeues when load balancer target registration returns a transient error", func() { - updateMachineControlPlaneLabel(ctx, machineName) - loadBalancerID := createAPIServerLoadBalancer(ctx, fakeCloud) - updateStackitClusterLoadBalancer(ctx, clusterName, namespace, loadBalancerID) - fakeCloud.FailNextEnsureTarget = fmt.Errorf("update target pool timeout: %w", cloud.ErrTransient) - - result, err := reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - Expect(result.RequeueAfter).To(BeNumerically(">", 0)) - - got := &infrav1.StackitMachine{} - Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) - expectCondition(got.Status.Conditions, infrav1.MachineReadyCondition, metav1.ConditionFalse, "LoadBalancerTargetError") - }) - It("deletes the VM and removes the finalizer", func() { - setupControlPlaneLoadBalancer() _, err := reconciler.Reconcile(ctx, request) Expect(err).NotTo(HaveOccurred()) Expect(fakeCloud.ServerCount()).To(Equal(1)) - stackitCluster := &infrav1.StackitCluster{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Name: clusterName, Namespace: namespace}, stackitCluster)).To(Succeed()) - loadBalancerID := stackitCluster.Status.APIServerLoadBalancerID - Expect(loadBalancerID).NotTo(BeEmpty()) - Expect(fakeCloud.LoadBalancerTargetCount(loadBalancerID)).To(Equal(1)) - got := &infrav1.StackitMachine{} Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) Expect(k8sClient.Delete(ctx, got)).To(Succeed()) @@ -388,7 +344,6 @@ var _ = Describe("StackitMachine Controller", func() { _, err = reconciler.Reconcile(ctx, request) Expect(err).NotTo(HaveOccurred()) Expect(fakeCloud.ServerCount()).To(Equal(0)) - Expect(fakeCloud.LoadBalancerTargetCount(loadBalancerID)).To(Equal(0)) Eventually(func() bool { err := k8sClient.Get(ctx, stackitKey, &infrav1.StackitMachine{}) return apierrors.IsNotFound(err) @@ -414,18 +369,10 @@ var _ = Describe("StackitMachine Controller", func() { }) It("removes the finalizer when the server is already gone at deletion time", func() { - setupControlPlaneLoadBalancer() - _, err := reconciler.Reconcile(ctx, request) Expect(err).NotTo(HaveOccurred()) Expect(fakeCloud.ServerCount()).To(Equal(1)) - stackitCluster := &infrav1.StackitCluster{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Name: clusterName, Namespace: namespace}, stackitCluster)).To(Succeed()) - loadBalancerID := stackitCluster.Status.APIServerLoadBalancerID - Expect(loadBalancerID).NotTo(BeEmpty()) - Expect(fakeCloud.LoadBalancerTargetCount(loadBalancerID)).To(Equal(1)) - provisioned := &infrav1.StackitMachine{} Expect(k8sClient.Get(ctx, stackitKey, provisioned)).To(Succeed()) instanceID := provisioned.Status.InstanceID @@ -443,8 +390,6 @@ var _ = Describe("StackitMachine Controller", func() { Expect(k8sClient.Get(ctx, stackitKey, degraded)).To(Succeed()) expectCondition(degraded.Status.Conditions, infrav1.MachineInstanceReadyCondition, metav1.ConditionFalse, "InstanceNotFound") Expect(degraded.Status.InstanceID).To(Equal(instanceID)) - Expect(fakeCloud.LoadBalancerTargetCount(loadBalancerID)).To(Equal(1), - "the deletion must still find the load balancer target to remove") By("deleting the object while the cloud reports the server as gone") // The fake deletes an unknown ID without an error, so the not-found answer @@ -459,7 +404,6 @@ var _ = Describe("StackitMachine Controller", func() { // so a nil field proves the call happened. Expect(fakeCloud.FailNextDeleteServer).ToNot(HaveOccurred(), "the machine deletion did not ask the cloud to remove the server") - Expect(fakeCloud.LoadBalancerTargetCount(loadBalancerID)).To(Equal(0)) Eventually(func() bool { err := k8sClient.Get(ctx, stackitKey, &infrav1.StackitMachine{}) return apierrors.IsNotFound(err) From 20ef7555996ee0ff9f4ac7a5b729bea2f136aed4 Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Tue, 22 Sep 2026 15:55:06 +0200 Subject: [PATCH 3/4] docs: move load balancer targets to the cluster reconciler responsibilities --- docs/src/development/architecture.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/src/development/architecture.md b/docs/src/development/architecture.md index 381ffe8..80c5327 100644 --- a/docs/src/development/architecture.md +++ b/docs/src/development/architecture.md @@ -56,6 +56,7 @@ balancer endpoint. - Reading credentials - Looking up the configured network - Managing the optional API server load balancer +- Maintaining its target pool from the control-plane machines of the cluster - Publishing failure domains - Updating readiness and contract status - Cleaning up provider-managed load balancers on deletion @@ -65,8 +66,7 @@ balancer endpoint. - Waiting for bootstrap data - Creating STACKIT servers - Setting provider IDs and addresses -- Registering control-plane machines as API server load balancer targets -- Deleting servers and load balancer targets on teardown +- Deleting servers on teardown Reconciliation must be idempotent. Re-running the same reconcile loop should be safe and should not create duplicate cloud resources. From 316cb4dd10dfadcfa6ea7f651644faef821d6a9f Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Tue, 22 Sep 2026 17:05:03 +0200 Subject: [PATCH 4/4] fix: keep the target pool intact when no control plane machine is active A cluster deletion filters out every machine at once, and the placeholder must not overwrite the pool while the API server is still needed. --- cloud/fake/client.go | 5 +- cloud/sdk_client.go | 41 ++++++------ cloud/services/loadbalancer/loadbalancer.go | 13 ++-- .../loadbalancer/loadbalancer_test.go | 7 +- controller/controller_test_helpers_test.go | 11 +++ controller/stackitcluster_controller.go | 34 ++++++++-- controller/stackitcluster_controller_test.go | 67 ++++++++++++++++--- controller/stackitcluster_infrastructure.go | 63 ++++++++++------- 8 files changed, 169 insertions(+), 72 deletions(-) diff --git a/cloud/fake/client.go b/cloud/fake/client.go index 217e509..a5d3e71 100644 --- a/cloud/fake/client.go +++ b/cloud/fake/client.go @@ -506,10 +506,13 @@ func (c *Client) SetAPIServerLoadBalancerTargets( } replaced := make(map[string]string, len(targets)) for _, target := range targets { + // The real API rejects these; accepting them would hide a regression. + if target.Name == "" || target.IP == "" { + return fmt.Errorf("target name and target IP are required: %w", cloud.ErrInvalidInput) + } replaced[target.Name] = target.IP } entry.targets = replaced - entry.lb.Port = port return nil } diff --git a/cloud/sdk_client.go b/cloud/sdk_client.go index a0d47c9..4d0cdb9 100644 --- a/cloud/sdk_client.go +++ b/cloud/sdk_client.go @@ -17,6 +17,7 @@ import ( "fmt" "net/http" "os" + "slices" "strings" "github.com/stackitcloud/stackit-sdk-go/core/config" @@ -581,6 +582,17 @@ func (c *SDKClient) SetAPIServerLoadBalancerTargets( if len(targets) == 0 { return fmt.Errorf("%w: at least one target is required", ErrInvalidInput) } + desired := make([]lb.Target, 0, len(targets)) + for _, targetInput := range targets { + if targetInput.Name == "" || targetInput.IP == "" { + return fmt.Errorf("%w: target name and target IP are required", ErrInvalidInput) + } + target := lb.NewTarget() + target.SetDisplayName(targetInput.Name) + target.SetIp(targetInput.IP) + desired = append(desired, *target) + } + loadBalancer, err := c.lbClient.DefaultAPI.GetLoadBalancer(ctx, c.projectID, c.region, loadBalancerID).Execute() if err != nil { return classifySDKError("get load balancer", err) @@ -595,16 +607,6 @@ func (c *SDKClient) SetAPIServerLoadBalancerTargets( ) } - desired := make([]lb.Target, 0, len(targets)) - for _, targetInput := range targets { - if targetInput.Name == "" || targetInput.IP == "" { - return fmt.Errorf("%w: target name and target IP are required", ErrInvalidInput) - } - target := lb.NewTarget() - target.SetDisplayName(targetInput.Name) - target.SetIp(targetInput.IP) - desired = append(desired, *target) - } // Control plane status updates are frequent, and each one reaches this path. if targetPool.GetTargetPort() == port && sameTargets(targetPool.GetTargets(), desired) { return nil @@ -616,17 +618,16 @@ func sameTargets(current, desired []lb.Target) bool { if len(current) != len(desired) { return false } - byName := make(map[string]string, len(current)) - for _, target := range current { - byName[target.GetDisplayName()] = target.GetIp() - } - for _, target := range desired { - ip, ok := byName[target.GetDisplayName()] - if !ok || ip != target.GetIp() { - return false - } + return slices.Equal(sortedTargetKeys(current), sortedTargetKeys(desired)) +} + +func sortedTargetKeys(targets []lb.Target) []string { + keys := make([]string, 0, len(targets)) + for _, target := range targets { + keys = append(keys, target.GetDisplayName()+"\x00"+target.GetIp()) } - return true + slices.Sort(keys) + return keys } func (c *SDKClient) findLoadBalancerByTags(ctx context.Context, tags map[string]string) (*LoadBalancer, error) { diff --git a/cloud/services/loadbalancer/loadbalancer.go b/cloud/services/loadbalancer/loadbalancer.go index 81711e5..1fedeb7 100644 --- a/cloud/services/loadbalancer/loadbalancer.go +++ b/cloud/services/loadbalancer/loadbalancer.go @@ -53,7 +53,8 @@ func APIServerInput( } } -func bootstrapTarget(ip string) cloud.LoadBalancerTargetInput { +// BootstrapTarget seeds a new load balancer; STACKIT rejects an empty pool. +func BootstrapTarget(ip string) cloud.LoadBalancerTargetInput { return cloud.LoadBalancerTargetInput{ Name: "capi-bootstrap-placeholder", IP: ip, @@ -61,10 +62,9 @@ func bootstrapTarget(ip string) cloud.LoadBalancerTargetInput { } // APIServerTargets builds the desired API server target pool, sorted by machine -// name so the pool can be compared without spurious updates. Machines without an -// internal IP are still provisioning; while none has one, the bootstrap -// placeholder keeps the pool non-empty as STACKIT requires. -func APIServerTargets(machines []*clusterv1.Machine, bootstrapIP string) []cloud.LoadBalancerTargetInput { +// name so the pool can be compared without spurious updates. The result is empty +// while no control plane machine has an internal IP yet. +func APIServerTargets(machines []*clusterv1.Machine) []cloud.LoadBalancerTargetInput { sorted := make([]*clusterv1.Machine, 0, len(machines)) for _, machine := range machines { if machine != nil { @@ -90,9 +90,6 @@ func APIServerTargets(machines []*clusterv1.Machine, bootstrapIP string) []cloud seen[ip] = struct{}{} targets = append(targets, cloud.LoadBalancerTargetInput{Name: targetName(machine.Name), IP: ip}) } - if len(targets) == 0 { - return []cloud.LoadBalancerTargetInput{bootstrapTarget(bootstrapIP)} - } return targets } diff --git a/cloud/services/loadbalancer/loadbalancer_test.go b/cloud/services/loadbalancer/loadbalancer_test.go index 6640255..d95ec1c 100644 --- a/cloud/services/loadbalancer/loadbalancer_test.go +++ b/cloud/services/loadbalancer/loadbalancer_test.go @@ -74,9 +74,10 @@ func TestAPIServerTargets(t *testing.T) { want: []cloud.LoadBalancerTargetInput{{Name: "cp-0", IP: "10.0.0.10"}}, }, { - name: "falls back to the bootstrap placeholder without machines", + // The placeholder belongs to the creation path, not here. + name: "stays empty without machines", machines: nil, - want: []cloud.LoadBalancerTargetInput{{Name: "capi-bootstrap-placeholder", IP: "10.0.0.10"}}, + want: nil, }, { name: "tolerates nil entries", @@ -87,7 +88,7 @@ func TestAPIServerTargets(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - got := APIServerTargets(test.machines, "10.0.0.10") + got := APIServerTargets(test.machines) if len(got) != len(test.want) { t.Fatalf("APIServerTargets() = %#v, want %#v", got, test.want) } diff --git a/controller/controller_test_helpers_test.go b/controller/controller_test_helpers_test.go index 42d828f..b8a77f9 100644 --- a/controller/controller_test_helpers_test.go +++ b/controller/controller_test_helpers_test.go @@ -152,6 +152,17 @@ func createControlPlaneMachine(ctx context.Context, name, clusterName, ip string } } +// markMachineDeleting leaves a Machine with a deletion timestamp and a +// finalizer, the state a control plane node is in while it drains. +func markMachineDeleting(ctx context.Context, name string) { + machine := &clusterv1.Machine{} + key := client.ObjectKey{Name: name, Namespace: "default"} + Expect(k8sClient.Get(ctx, key, machine)).To(Succeed()) + machine.Finalizers = append(machine.Finalizers, "test.stackit.cloud/block-deletion") + Expect(k8sClient.Update(ctx, machine)).To(Succeed()) + Expect(k8sClient.Delete(ctx, machine)).To(Succeed()) +} + // setMachineInternalIP stands in for the Cluster API machine controller, which // copies status.addresses from the StackitMachine but does not run in envtest. func setMachineInternalIP(ctx context.Context, name, ip string) { diff --git a/controller/stackitcluster_controller.go b/controller/stackitcluster_controller.go index 27c533f..4736619 100644 --- a/controller/stackitcluster_controller.go +++ b/controller/stackitcluster_controller.go @@ -18,7 +18,10 @@ package controller import ( "context" + "errors" "fmt" + "maps" + "slices" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -28,9 +31,12 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" clusterutil "sigs.k8s.io/cluster-api/util" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" infrav1 "github.com/stackitcloud/cluster-api-provider-stackit/api/v1alpha1" @@ -126,16 +132,30 @@ func (r *StackitClusterReconciler) stackitClusterRequestsForMachine(ctx context. return nil } cluster, err := clusterutil.GetClusterFromMetadata(ctx, r.Client, machine.ObjectMeta) - if err != nil { - logf.FromContext(ctx).Error(err, "Failed to resolve Cluster for machine watch", "object", client.ObjectKeyFromObject(obj)) + switch { + // A missing label or an already deleted Cluster is routine during teardown. + case errors.Is(err, clusterutil.ErrNoCluster) || apierrors.IsNotFound(err): return nil - } - if cluster == nil { + case err != nil: + logf.FromContext(ctx).Error(err, "Failed to get Cluster for Machine watch", "machine", client.ObjectKeyFromObject(machine)) return nil } return r.stackitClusterRequestsForCluster(ctx, cluster) } +// machineTargetPoolChanged drops the frequent control plane status updates that +// cannot change the target pool but would each cost a reconcile. +func machineTargetPoolChanged(e event.UpdateEvent) bool { + oldMachine, okOld := e.ObjectOld.(*clusterv1.Machine) + newMachine, okNew := e.ObjectNew.(*clusterv1.Machine) + if !okOld || !okNew { + return true + } + return !slices.Equal(oldMachine.Status.Addresses, newMachine.Status.Addresses) || + oldMachine.DeletionTimestamp.IsZero() != newMachine.DeletionTimestamp.IsZero() || + !maps.Equal(oldMachine.Labels, newMachine.Labels) +} + func (r *StackitClusterReconciler) stackitClusterRequestsForCloudInitRef(ctx context.Context, obj client.Object) []reconcile.Request { kind := "" switch obj.(type) { @@ -174,7 +194,11 @@ func (r *StackitClusterReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). For(&infrav1.StackitCluster{}). Watches(&clusterv1.Cluster{}, handler.EnqueueRequestsFromMapFunc(r.stackitClusterRequestsForCluster)). - Watches(&clusterv1.Machine{}, handler.EnqueueRequestsFromMapFunc(r.stackitClusterRequestsForMachine)). + Watches( + &clusterv1.Machine{}, + handler.EnqueueRequestsFromMapFunc(r.stackitClusterRequestsForMachine), + builder.WithPredicates(predicate.Funcs{UpdateFunc: machineTargetPoolChanged}), + ). Watches(&corev1.ConfigMap{}, handler.EnqueueRequestsFromMapFunc(r.stackitClusterRequestsForCloudInitRef)). Watches(&corev1.Secret{}, handler.EnqueueRequestsFromMapFunc(r.stackitClusterRequestsForCloudInitRef)). Named("stackitcluster"). diff --git a/controller/stackitcluster_controller_test.go b/controller/stackitcluster_controller_test.go index 7a033bf..c55c5b8 100644 --- a/controller/stackitcluster_controller_test.go +++ b/controller/stackitcluster_controller_test.go @@ -730,8 +730,9 @@ var _ = Describe("StackitCluster Controller", func() { }) It("drops a control plane machine that is being deleted from the target pool", func() { - machineName := "cp-0-" + clusterName - createControlPlaneMachine(ctx, machineName, clusterName, "10.0.0.11") + deleting := "cp-0-" + clusterName + createControlPlaneMachine(ctx, deleting, clusterName, "10.0.0.11") + createControlPlaneMachine(ctx, "cp-1-"+clusterName, clusterName, "10.0.0.12") _, err := reconciler.Reconcile(ctx, request) Expect(err).NotTo(HaveOccurred()) @@ -739,25 +740,45 @@ var _ = Describe("StackitCluster Controller", func() { got := &infrav1.StackitCluster{} Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) loadBalancerID := got.Status.APIServerLoadBalancerID - Expect(fakeCloud.LoadBalancerTargetCount(loadBalancerID)).To(Equal(1)) + Expect(fakeCloud.LoadBalancerTargetCount(loadBalancerID)).To(Equal(2)) - By("letting the Machine linger with a deletion timestamp") - machine := &clusterv1.Machine{} - machineKey := types.NamespacedName{Name: machineName, Namespace: namespace} - Expect(k8sClient.Get(ctx, machineKey, machine)).To(Succeed()) - machine.Finalizers = append(machine.Finalizers, "test.stackit.cloud/block-deletion") - Expect(k8sClient.Update(ctx, machine)).To(Succeed()) - Expect(k8sClient.Delete(ctx, machine)).To(Succeed()) + markMachineDeleting(ctx, deleting) _, err = reconciler.Reconcile(ctx, request) Expect(err).NotTo(HaveOccurred()) // The target goes before the node is drained, not after. Expect(fakeCloud.LoadBalancerTargetIPs(loadBalancerID)).To(Equal(map[string]string{ - "capi-bootstrap-placeholder": "10.0.0.1", + "cp-1-" + clusterName: "10.0.0.12", })) }) + It("keeps the target pool when every control plane machine is being deleted", func() { + // Cluster deletion removes the control plane before the infrastructure. + first := "cp-0-" + clusterName + second := "cp-1-" + clusterName + createControlPlaneMachine(ctx, first, clusterName, "10.0.0.11") + createControlPlaneMachine(ctx, second, clusterName, "10.0.0.12") + + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + got := &infrav1.StackitCluster{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + loadBalancerID := got.Status.APIServerLoadBalancerID + + markMachineDeleting(ctx, first) + markMachineDeleting(ctx, second) + + _, err = reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + Expect(fakeCloud.LoadBalancerTargetIPs(loadBalancerID)).To(Equal(map[string]string{ + first: "10.0.0.11", + second: "10.0.0.12", + }), "the pool must not collapse onto the bootstrap placeholder") + }) + It("requeues without failing the cluster when the target pool update fails", func() { createControlPlaneMachine(ctx, "cp-0-"+clusterName, clusterName, "10.0.0.11") _, err := reconciler.Reconcile(ctx, request) @@ -780,6 +801,30 @@ var _ = Describe("StackitCluster Controller", func() { expectCondition(got.Status.Conditions, infrav1.ClusterReadyCondition, metav1.ConditionTrue, "Available") }) + It("requeues when the first target pool update fails permanently", func() { + // Without the requeue nothing would drive another reconcile. + createControlPlaneMachine(ctx, "cp-0-"+clusterName, clusterName, "10.0.0.11") + fakeCloud.FailNextSetTargets = fmt.Errorf("target pool rejected: %w", cloud.ErrInvalidInput) + + result, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(BeNumerically(">", 0)) + + got := &infrav1.StackitCluster{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + expectCondition(got.Status.Conditions, infrav1.ClusterLoadBalancerReadyCondition, metav1.ConditionFalse, "LoadBalancerTargetError") + + By("recovering on the next attempt") + _, err = reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + Expect(got.Status.Ready).To(BeTrue()) + Expect(fakeCloud.LoadBalancerTargetIPs(got.Status.APIServerLoadBalancerID)).To(Equal(map[string]string{ + "cp-0-" + clusterName: "10.0.0.11", + })) + }) + It("maps control plane Machine events to StackitCluster reconcile requests", func() { machineName := "cp-0-" + clusterName createControlPlaneMachine(ctx, machineName, clusterName, "10.0.0.11") diff --git a/controller/stackitcluster_infrastructure.go b/controller/stackitcluster_infrastructure.go index 525b8b1..03d1f07 100644 --- a/controller/stackitcluster_infrastructure.go +++ b/controller/stackitcluster_infrastructure.go @@ -100,15 +100,27 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterS collections.ActiveMachines, ) if err != nil { + util.SetConditions( + &stackitCluster.Status.Conditions, + stackitCluster.Generation, + metav1.ConditionFalse, + "MachineListError", + err.Error(), + infrav1.ClusterLoadBalancerReadyCondition, + ) return ctrl.Result{}, fmt.Errorf("list control plane machines: %w", err) } - // The same targets seed the creation, so a load balancer deleted out of - // band comes back with the real control plane behind it. - targets := loadbalancerservice.APIServerTargets(machines.UnsortedList(), bootstrapTargetIP(network)) + targets := loadbalancerservice.APIServerTargets(machines.UnsortedList()) + // Seeding with the real targets lets a load balancer deleted out of band + // come back with the control plane behind it. + seed := targets + if len(seed) == 0 { + seed = []cloud.LoadBalancerTargetInput{loadbalancerservice.BootstrapTarget(bootstrapTargetIP(network))} + } loadBalancer, err := cloudClient.EnsureAPIServerLoadBalancer( ctx, - loadbalancerservice.APIServerInput(stackitCluster, targets), + loadbalancerservice.APIServerInput(stackitCluster, seed), ) if err != nil { stackitCluster.Status.Ready = false @@ -148,29 +160,32 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterS } clusterScope.SetAPIServerEndpoint(endpoint) - if err := cloudClient.SetAPIServerLoadBalancerTargets(ctx, loadBalancer.ID, defaultAPIServerPort, targets); err != nil { - // A permanent failure neither requeues nor returns an error, so - // without an event it would pass unnoticed. - if !cloud.IsRetryable(err) && r.Recorder != nil { - r.Recorder.Eventf( - stackitCluster, nil, corev1.EventTypeWarning, "LoadBalancerTargetError", "Update", - "Cannot update API server load balancer target pool: %v", err, + // An empty set never overwrites a populated pool: cluster deletion filters + // out every machine while the API server is still needed for drain. + if len(targets) > 0 { + if err := cloudClient.SetAPIServerLoadBalancerTargets(ctx, loadBalancer.ID, defaultAPIServerPort, targets); err != nil { + if !cloud.IsRetryable(err) && r.Recorder != nil { + r.Recorder.Eventf( + stackitCluster, nil, corev1.EventTypeWarning, "LoadBalancerTargetError", "Update", + "Cannot update API server load balancer target pool: %v", err, + ) + } + // Scoped to the load balancer condition: failing the cluster would + // stop the machine reconciler from replacing the broken machine. + util.SetConditions( + &stackitCluster.Status.Conditions, + stackitCluster.Generation, + metav1.ConditionFalse, + "LoadBalancerTargetError", + err.Error(), + infrav1.ClusterLoadBalancerReadyCondition, ) + // Requeue even on a permanent error: this return skips SetReady, so + // no machine exists yet whose events could retry it. + return ctrl.Result{RequeueAfter: retryableErrorRequeueAfter}, nil } - // Scoped to the load balancer condition on purpose: the machine - // reconciler stops while the cluster is not ready, so failing the - // cluster would block replacing the machine whose target is broken. - return util.CloudFailureResult( - &stackitCluster.Status.Conditions, - stackitCluster.Generation, - "LoadBalancerTargetError", - err, - retryableErrorRequeueAfter, - false, - infrav1.ClusterLoadBalancerReadyCondition, - ) + log.V(1).Info("Reconciled API server load balancer target pool", "targets", len(targets)) } - log.V(1).Info("Reconciled API server load balancer target pool", "targets", len(targets)) clusterScope.SetConditions( metav1.ConditionTrue,