From 6bad482ff41b817f6ed6040afefce3582c682a26 Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Wed, 19 Aug 2026 12:14:16 +0000 Subject: [PATCH 01/15] chore: Use speaking variable names in the cluster and machine reconcilers (cherry picked from commit bbb27119d1111fbf84de4bfd74aef7e77e5c09ec) --- controller/stackitcluster_bastion.go | 53 ++--- controller/stackitcluster_infrastructure.go | 106 +++++----- controller/stackitmachine_infrastructure.go | 223 +++++++++++--------- scope/cluster_scope.go | 8 +- scope/machine_scope.go | 4 +- 5 files changed, 209 insertions(+), 185 deletions(-) diff --git a/controller/stackitcluster_bastion.go b/controller/stackitcluster_bastion.go index 5e07cb4..57807d5 100644 --- a/controller/stackitcluster_bastion.go +++ b/controller/stackitcluster_bastion.go @@ -39,25 +39,25 @@ func (r *StackitClusterReconciler) reconcileBastion( cloudClient cloud.Client, clusterScope *scope.ClusterScope, ) (ctrl.Result, bool, error) { - cluster := clusterScope.StackitCluster - input := bastionservice.Input(cluster, nil) + stackitCluster := clusterScope.StackitCluster + input := bastionservice.Input(stackitCluster, nil) status := cloud.Bastion{ - ServerID: cluster.Status.Bastion.ServerID, - PublicIPID: cluster.Status.Bastion.PublicIPID, - PublicIP: cluster.Status.Bastion.PublicIP, - SecurityGroupID: cluster.Status.Bastion.SecurityGroupID, + ServerID: stackitCluster.Status.Bastion.ServerID, + PublicIPID: stackitCluster.Status.Bastion.PublicIPID, + PublicIP: stackitCluster.Status.Bastion.PublicIP, + SecurityGroupID: stackitCluster.Status.Bastion.SecurityGroupID, } - if !cluster.Spec.Bastion.Enabled { + if !stackitCluster.Spec.Bastion.Enabled { // The condition carries this reason only after a cleanup has succeeded, // so anything else means we may still own bastion resources — including // the case where EnsureBastion succeeded but its status patch was lost. // Keying on it instead of on the status keeps the tag-based cleanup to // once per cluster rather than once per reconcile, which matters because // this path runs for every cluster without a bastion. - condition := meta.FindStatusCondition(cluster.Status.Conditions, infrav1.ClusterBastionReadyCondition) + condition := meta.FindStatusCondition(stackitCluster.Status.Conditions, infrav1.ClusterBastionReadyCondition) if condition == nil || condition.Reason != bastionDisabledReason { - if err := cloudClient.DeleteNodeSSHAccess(ctx, bastionservice.NodeSSHAccessTags(cluster)); err != nil { + if err := cloudClient.DeleteNodeSSHAccess(ctx, bastionservice.NodeSSHAccessTags(stackitCluster)); err != nil { return ctrl.Result{}, false, err } if err := cloudClient.DeleteBastion(ctx, input, status); err != nil { @@ -65,7 +65,7 @@ func (r *StackitClusterReconciler) reconcileBastion( } clusterScope.ClearBastionStatus() if r.Recorder != nil { - r.Recorder.Eventf(cluster, nil, corev1.EventTypeNormal, "BastionDeleted", "Delete", "Deleted bastion") + r.Recorder.Eventf(stackitCluster, nil, corev1.EventTypeNormal, "BastionDeleted", "Delete", "Deleted bastion") } } clusterScope.SetConditions( @@ -77,7 +77,7 @@ func (r *StackitClusterReconciler) reconcileBastion( return ctrl.Result{}, true, nil } - if err := validateBastionSpec(cluster.Spec.Bastion); err != nil { + if err := validateBastionSpec(stackitCluster.Spec.Bastion); err != nil { clusterScope.SetNotReady( "InvalidBastionSpec", err.Error(), @@ -87,7 +87,7 @@ func (r *StackitClusterReconciler) reconcileBastion( return ctrl.Result{}, false, nil } - cloudInit, err := r.resolveBastionCloudInit(ctx, cluster) + cloudInit, err := r.resolveBastionCloudInit(ctx, stackitCluster) if err != nil { clusterScope.SetNotReady( "CloudInitRefError", @@ -99,25 +99,30 @@ func (r *StackitClusterReconciler) reconcileBastion( } input.CloudInit = cloudInit - if bastionNeedsRecreate(cluster, cloudInit) { - if err := cloudClient.DeleteNodeSSHAccess(ctx, bastionservice.NodeSSHAccessTags(cluster)); err != nil && !cloud.IsNotFound(err) { + if bastionNeedsRecreate(stackitCluster, cloudInit) { + if err := cloudClient.DeleteNodeSSHAccess(ctx, bastionservice.NodeSSHAccessTags(stackitCluster)); err != nil && !cloud.IsNotFound(err) { return ctrl.Result{}, false, err } if err := cloudClient.DeleteBastion(ctx, input, status); err != nil && !cloud.IsNotFound(err) { return ctrl.Result{}, false, err } clusterScope.ClearBastionStatus() - clusterScope.SetNotReady("Recreating", "recreating bastion because cloudInitRef content changed", infrav1.ClusterBastionReadyCondition, infrav1.ClusterReadyCondition) + clusterScope.SetNotReady( + "Recreating", + "recreating bastion because cloudInitRef content changed", + infrav1.ClusterBastionReadyCondition, + infrav1.ClusterReadyCondition, + ) if r.Recorder != nil { r.Recorder.Eventf( - cluster, nil, corev1.EventTypeNormal, "BastionRecreating", "Recreate", + stackitCluster, nil, corev1.EventTypeNormal, "BastionRecreating", "Recreate", "Recreating bastion because cloudInitRef content changed", ) } return ctrl.Result{RequeueAfter: retryableErrorRequeueAfter}, false, nil } - hadBastionStatus := hasBastionStatus(cluster.Status.Bastion) + hadBastionStatus := hasBastionStatus(stackitCluster.Status.Bastion) bastion, err := cloudClient.EnsureBastion(ctx, input) if err != nil { return ctrl.Result{}, false, err @@ -125,7 +130,7 @@ func (r *StackitClusterReconciler) reconcileBastion( clusterScope.SetBastionStatus(bastion, bastionCloudInitHash(cloudInit)) if !hadBastionStatus && r.Recorder != nil { r.Recorder.Eventf( - cluster, nil, corev1.EventTypeNormal, "BastionCreated", "Create", "Created bastion %s", bastion.ServerID, + stackitCluster, nil, corev1.EventTypeNormal, "BastionCreated", "Create", "Created bastion %s", bastion.ServerID, ) } if bastion.ServerState != "" && bastion.ServerState != "ACTIVE" { @@ -180,11 +185,11 @@ func hasBastionStatus(status infrav1.StackitBastionStatus) bool { return status.ServerID != "" || status.PublicIPID != "" || status.PublicIP != "" || status.SecurityGroupID != "" } -func bastionNeedsRecreate(sc *infrav1.StackitCluster, cloudInit []byte) bool { - if !hasBastionStatus(sc.Status.Bastion) { +func bastionNeedsRecreate(stackitCluster *infrav1.StackitCluster, cloudInit []byte) bool { + if !hasBastionStatus(stackitCluster.Status.Bastion) { return false } - return sc.Status.Bastion.CloudInitHash != bastionCloudInitHash(cloudInit) + return stackitCluster.Status.Bastion.CloudInitHash != bastionCloudInitHash(cloudInit) } func bastionCloudInitHash(cloudInit []byte) string { @@ -194,12 +199,12 @@ func bastionCloudInitHash(cloudInit []byte) string { return fmt.Sprintf("%x", sha256.Sum256(cloudInit)) } -func (r *StackitClusterReconciler) resolveBastionCloudInit(ctx context.Context, sc *infrav1.StackitCluster) ([]byte, error) { - ref := sc.Spec.Bastion.CloudInitRef +func (r *StackitClusterReconciler) resolveBastionCloudInit(ctx context.Context, stackitCluster *infrav1.StackitCluster) ([]byte, error) { + ref := stackitCluster.Spec.Bastion.CloudInitRef if ref == nil { return nil, nil } - key := types.NamespacedName{Namespace: sc.Namespace, Name: ref.Name} + key := types.NamespacedName{Namespace: stackitCluster.Namespace, Name: ref.Name} switch ref.Kind { case "ConfigMap": configMap := &corev1.ConfigMap{} diff --git a/controller/stackitcluster_infrastructure.go b/controller/stackitcluster_infrastructure.go index 1380c0c..8d7b4b7 100644 --- a/controller/stackitcluster_infrastructure.go +++ b/controller/stackitcluster_infrastructure.go @@ -33,19 +33,19 @@ import ( func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterScope *scope.ClusterScope) (ctrl.Result, error) { log := logf.FromContext(ctx) - cluster := clusterScope.StackitCluster + stackitCluster := clusterScope.StackitCluster - if !controllerutil.ContainsFinalizer(cluster, infrav1.ClusterFinalizer) { - controllerutil.AddFinalizer(cluster, infrav1.ClusterFinalizer) + if !controllerutil.ContainsFinalizer(stackitCluster, infrav1.ClusterFinalizer) { + controllerutil.AddFinalizer(stackitCluster, infrav1.ClusterFinalizer) } - cluster.Status.FailureDomains = stackitFailureDomains(cluster.Spec.Region) + stackitCluster.Status.FailureDomains = stackitFailureDomains(stackitCluster.Spec.Region) - cloudClient, err := util.BuildCloudClient(ctx, r.Client, r.CloudClientFactory, cluster) + cloudClient, err := util.BuildCloudClient(ctx, r.Client, r.CloudClientFactory, stackitCluster) if err != nil { - cluster.Status.Ready = false + stackitCluster.Status.Ready = false return util.CredentialFailureResult( - &cluster.Status.Conditions, - cluster.Generation, + &stackitCluster.Status.Conditions, + stackitCluster.Generation, err, infrav1.ClusterCredentialsReadyCondition, infrav1.ClusterReadyCondition, @@ -58,12 +58,12 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterS infrav1.ClusterCredentialsReadyCondition, ) - network, err := cloudClient.GetNetwork(ctx, cluster.Spec.Network.ID) + network, err := cloudClient.GetNetwork(ctx, stackitCluster.Spec.Network.ID) if err != nil { - cluster.Status.Ready = false + stackitCluster.Status.Ready = false return util.CloudFailureResult( - &cluster.Status.Conditions, - cluster.Generation, + &stackitCluster.Status.Conditions, + stackitCluster.Generation, "NetworkNotFound", err, retryableErrorRequeueAfter, @@ -79,19 +79,19 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterS infrav1.ClusterNetworkReadyCondition, ) - if cluster.Spec.APIServerLoadBalancer.Enabled { - lb, err := cloudClient.EnsureAPIServerLoadBalancer( + if stackitCluster.Spec.APIServerLoadBalancer.Enabled { + loadBalancer, err := cloudClient.EnsureAPIServerLoadBalancer( ctx, loadbalancerservice.APIServerInput( - cluster, + stackitCluster, []cloud.LoadBalancerTargetInput{loadbalancerservice.BootstrapTarget(bootstrapTargetIP(network))}, ), ) if err != nil { - cluster.Status.Ready = false + stackitCluster.Status.Ready = false return util.CloudFailureResult( - &cluster.Status.Conditions, - cluster.Generation, + &stackitCluster.Status.Conditions, + stackitCluster.Generation, "LoadBalancerError", err, retryableErrorRequeueAfter, @@ -100,17 +100,17 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterS infrav1.ClusterReadyCondition, ) } - hadLoadBalancerID := cluster.Status.APIServerLoadBalancerID != "" - if lb != nil { - cluster.Status.APIServerLoadBalancerID = lb.ID - if !hadLoadBalancerID && lb.ID != "" && r.Recorder != nil { + hadLoadBalancerID := stackitCluster.Status.APIServerLoadBalancerID != "" + if loadBalancer != nil { + stackitCluster.Status.APIServerLoadBalancerID = loadBalancer.ID + if !hadLoadBalancerID && loadBalancer.ID != "" && r.Recorder != nil { r.Recorder.Eventf( - cluster, nil, corev1.EventTypeNormal, "LoadBalancerCreated", "Create", - "Created API server load balancer %s", lb.ID, + stackitCluster, nil, corev1.EventTypeNormal, "LoadBalancerCreated", "Create", + "Created API server load balancer %s", loadBalancer.ID, ) } } - if lb == nil || lb.IP == "" { + if loadBalancer == nil || loadBalancer.IP == "" { clusterScope.SetNotReady( "Provisioning", "waiting for API server load balancer IP address", @@ -120,7 +120,7 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterS return ctrl.Result{RequeueAfter: 10 * time.Second}, nil } endpoint := clusterv1.APIEndpoint{ - Host: lb.IP, + Host: loadBalancer.IP, Port: defaultAPIServerPort, } clusterScope.SetAPIServerEndpoint(endpoint) @@ -132,12 +132,12 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterS ) if r.Recorder != nil { r.Recorder.Eventf( - cluster, nil, corev1.EventTypeNormal, "LoadBalancerReady", "SetReady", - "API server load balancer is ready at %s", lb.IP, + stackitCluster, nil, corev1.EventTypeNormal, "LoadBalancerReady", "SetReady", + "API server load balancer is ready at %s", loadBalancer.IP, ) } - } else if cluster.Spec.ControlPlaneEndpoint.Host != "" { - cluster.Status.APIServerEndpoint = cluster.Spec.ControlPlaneEndpoint + } else if stackitCluster.Spec.ControlPlaneEndpoint.Host != "" { + stackitCluster.Status.APIServerEndpoint = stackitCluster.Spec.ControlPlaneEndpoint clusterScope.SetConditions( metav1.ConditionTrue, "Skipped", @@ -155,10 +155,10 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterS } if result, ready, err := r.reconcileBastion(ctx, cloudClient, clusterScope); err != nil { - cluster.Status.Ready = false + stackitCluster.Status.Ready = false return util.CloudFailureResult( - &cluster.Status.Conditions, - cluster.Generation, + &stackitCluster.Status.Conditions, + stackitCluster.Generation, "BastionError", err, retryableErrorRequeueAfter, @@ -171,7 +171,7 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterS } clusterScope.SetReady() - log.V(1).Info("StackitCluster ready", "endpoint", cluster.Status.APIServerEndpoint) + log.V(1).Info("StackitCluster ready", "endpoint", stackitCluster.Status.APIServerEndpoint) return ctrl.Result{}, nil } @@ -223,7 +223,7 @@ func bootstrapTargetIP(network *cloud.Network) string { } func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, clusterScope *scope.ClusterScope) error { - cluster := clusterScope.StackitCluster + stackitCluster := clusterScope.StackitCluster // Cleanup runs unconditionally. Neither spec nor status is a trustworthy // record of what exists in the cloud: a resource can be created before its @@ -232,7 +232,7 @@ func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, clusterS // DeleteNodeSSHAccess all fall back to tag lookups and tolerate NotFound, so // asking for everything costs a handful of list calls once per cluster and // removes every combination in which a resource could be missed. - cloudClient, err := util.BuildCloudClient(ctx, r.Client, r.CloudClientFactory, cluster) + cloudClient, err := util.BuildCloudClient(ctx, r.Client, r.CloudClientFactory, stackitCluster) if err != nil { // A missing credentials Secret can never be recovered from — it commonly // disappears first during namespace teardown. Blocking here would strand @@ -241,16 +241,16 @@ func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, clusterS // retrying for those. if apierrors.IsNotFound(err) { if r.Recorder != nil { - r.Recorder.Eventf(cluster, nil, corev1.EventTypeWarning, "CleanupSkipped", "Delete", + r.Recorder.Eventf(stackitCluster, nil, corev1.EventTypeWarning, "CleanupSkipped", "Delete", "Credentials Secret is gone; finalizing without cloud cleanup. "+ - "Any remaining STACKIT resources for this cluster must be removed manually: %v", err) + "Any remaining STACKIT resources for this stackitCluster must be removed manually: %v", err) } - controllerutil.RemoveFinalizer(cluster, infrav1.ClusterFinalizer) + controllerutil.RemoveFinalizer(stackitCluster, infrav1.ClusterFinalizer) return nil } util.SetConditions( - &cluster.Status.Conditions, - cluster.Generation, + &stackitCluster.Status.Conditions, + stackitCluster.Generation, metav1.ConditionFalse, "CredentialsInvalid", err.Error(), @@ -258,7 +258,7 @@ func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, clusterS ) return err } - loadBalancerID, err := loadbalancerservice.ResolveID(ctx, cloudClient, cluster) + loadBalancerID, err := loadbalancerservice.ResolveID(ctx, cloudClient, stackitCluster) if err != nil { return err } @@ -266,31 +266,31 @@ func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, clusterS if err := cloudClient.DeleteAPIServerLoadBalancer(ctx, loadBalancerID); err != nil && !cloud.IsNotFound(err) { return err } - cluster.Status.APIServerLoadBalancerID = "" + stackitCluster.Status.APIServerLoadBalancerID = "" if r.Recorder != nil { r.Recorder.Eventf( - cluster, nil, corev1.EventTypeNormal, "LoadBalancerDeleted", "Delete", + stackitCluster, nil, corev1.EventTypeNormal, "LoadBalancerDeleted", "Delete", "Deleted API server load balancer %s", loadBalancerID, ) } } - if err := cloudClient.DeleteNodeSSHAccess(ctx, bastionservice.NodeSSHAccessTags(cluster)); err != nil && !cloud.IsNotFound(err) { + if err := cloudClient.DeleteNodeSSHAccess(ctx, bastionservice.NodeSSHAccessTags(stackitCluster)); err != nil && !cloud.IsNotFound(err) { return err } - if err := cloudClient.DeleteBastion(ctx, bastionservice.Input(cluster, nil), cloud.Bastion{ - ServerID: cluster.Status.Bastion.ServerID, - PublicIPID: cluster.Status.Bastion.PublicIPID, - PublicIP: cluster.Status.Bastion.PublicIP, - SecurityGroupID: cluster.Status.Bastion.SecurityGroupID, + if err := cloudClient.DeleteBastion(ctx, bastionservice.Input(stackitCluster, nil), cloud.Bastion{ + ServerID: stackitCluster.Status.Bastion.ServerID, + PublicIPID: stackitCluster.Status.Bastion.PublicIPID, + PublicIP: stackitCluster.Status.Bastion.PublicIP, + SecurityGroupID: stackitCluster.Status.Bastion.SecurityGroupID, }); err != nil && !cloud.IsNotFound(err) { return err } - if hasBastionStatus(cluster.Status.Bastion) { + if hasBastionStatus(stackitCluster.Status.Bastion) { clusterScope.ClearBastionStatus() if r.Recorder != nil { - r.Recorder.Eventf(cluster, nil, corev1.EventTypeNormal, "BastionDeleted", "Delete", "Deleted bastion") + r.Recorder.Eventf(stackitCluster, nil, corev1.EventTypeNormal, "BastionDeleted", "Delete", "Deleted bastion") } } - controllerutil.RemoveFinalizer(cluster, infrav1.ClusterFinalizer) + controllerutil.RemoveFinalizer(stackitCluster, infrav1.ClusterFinalizer) return nil } diff --git a/controller/stackitmachine_infrastructure.go b/controller/stackitmachine_infrastructure.go index 863b4e2..b515ce3 100644 --- a/controller/stackitmachine_infrastructure.go +++ b/controller/stackitmachine_infrastructure.go @@ -32,51 +32,61 @@ import ( "github.com/stackitcloud/cluster-api-provider-stackit/util" ) -func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, s *scope.MachineScope) (ctrl.Result, error) { +func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, machineScope *scope.MachineScope) (ctrl.Result, error) { log := logf.FromContext(ctx) - sm := s.StackitMachine + stackitMachine := machineScope.StackitMachine - if !controllerutil.ContainsFinalizer(sm, infrav1.MachineFinalizer) { - controllerutil.AddFinalizer(sm, infrav1.MachineFinalizer) + if !controllerutil.ContainsFinalizer(stackitMachine, infrav1.MachineFinalizer) { + controllerutil.AddFinalizer(stackitMachine, infrav1.MachineFinalizer) } - if !s.StackitCluster.Status.Ready { - s.SetNotReady("InfrastructureNotReady", "waiting for StackitCluster to be ready", infrav1.MachineReadyCondition) + if !machineScope.StackitCluster.Status.Ready { + machineScope.SetNotReady("InfrastructureNotReady", "waiting for StackitCluster to be ready", infrav1.MachineReadyCondition) return ctrl.Result{}, nil } - if err := validateMachineAvailabilityZone(s); err != nil { - s.SetNotReady("InvalidFailureDomain", err.Error(), infrav1.MachineInstanceReadyCondition, infrav1.MachineReadyCondition) + if err := validateMachineAvailabilityZone(machineScope); err != nil { + machineScope.SetNotReady( + "InvalidFailureDomain", + err.Error(), + infrav1.MachineInstanceReadyCondition, + infrav1.MachineReadyCondition, + ) return ctrl.Result{}, nil } - bootstrapData, condStatus, reason, msg := r.fetchBootstrapData(ctx, s.Machine) - s.SetConditions(condStatus, reason, msg, infrav1.MachineBootstrapReadyCondition) - if condStatus != metav1.ConditionTrue { - s.SetConditions(metav1.ConditionFalse, reason, msg, infrav1.MachineReadyCondition) + bootstrapData, conditionStatus, reason, message := r.fetchBootstrapData(ctx, machineScope.Machine) + machineScope.SetConditions(conditionStatus, reason, message, infrav1.MachineBootstrapReadyCondition) + if conditionStatus != metav1.ConditionTrue { + machineScope.SetConditions(metav1.ConditionFalse, reason, message, infrav1.MachineReadyCondition) if reason == util.BootstrapReasonInvalid { return ctrl.Result{}, nil } return ctrl.Result{RequeueAfter: retryableErrorRequeueAfter}, nil } - cloudClient, err := util.BuildCloudClient(ctx, r.Client, r.CloudClientFactory, s.StackitCluster) + cloudClient, err := util.BuildCloudClient(ctx, r.Client, r.CloudClientFactory, machineScope.StackitCluster) if err != nil { return util.CredentialFailureResult( - &sm.Status.Conditions, - sm.Generation, + &stackitMachine.Status.Conditions, + stackitMachine.Generation, err, infrav1.MachineCredentialsReadyCondition, infrav1.MachineReadyCondition, ) } - s.SetConditions(metav1.ConditionTrue, "Available", "", infrav1.MachineCredentialsReadyCondition) + machineScope.SetConditions(metav1.ConditionTrue, "Available", "", infrav1.MachineCredentialsReadyCondition) - server, created, err := r.ensureServer(ctx, cloudClient, s, bootstrapData) + server, created, err := r.ensureServer(ctx, cloudClient, machineScope, bootstrapData) if err != nil { - s.SetNotReady("InstanceError", err.Error(), infrav1.MachineInstanceReadyCondition, infrav1.MachineReadyCondition) + machineScope.SetNotReady( + "InstanceError", + err.Error(), + infrav1.MachineInstanceReadyCondition, + infrav1.MachineReadyCondition, + ) return util.CloudFailureResult( - &sm.Status.Conditions, - sm.Generation, + &stackitMachine.Status.Conditions, + stackitMachine.Generation, "InstanceError", err, retryableErrorRequeueAfter, @@ -87,24 +97,29 @@ func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, s *scope } if created && r.Recorder != nil { r.Recorder.Eventf( - sm, nil, corev1.EventTypeNormal, "InstanceCreated", "Create", "Created instance %s", server.ID, + stackitMachine, nil, corev1.EventTypeNormal, "InstanceCreated", "Create", "Created instance %s", server.ID, ) } - sm.Status.InstanceState = server.State - sm.Status.Addresses = machineAddressesFromCloud(server.Addresses) - providerID := s.SetInstance(server) + stackitMachine.Status.InstanceState = server.State + stackitMachine.Status.Addresses = machineAddressesFromCloud(server.Addresses) + providerID := machineScope.SetInstance(server) if server.State != "" && server.State != "ACTIVE" { - s.SetNotReady("Provisioning", fmt.Sprintf("server state is %s", server.State), infrav1.MachineInstanceReadyCondition, infrav1.MachineReadyCondition) + machineScope.SetNotReady( + "Provisioning", + fmt.Sprintf("server state is %s", server.State), + infrav1.MachineInstanceReadyCondition, + infrav1.MachineReadyCondition, + ) return ctrl.Result{RequeueAfter: 15 * time.Second}, nil } - if err := r.reconcileBastionNodeSSHAccess(ctx, cloudClient, s, server); err != nil { - s.SetNotReady("BastionSSHAccessError", err.Error(), infrav1.MachineReadyCondition) + if err := r.reconcileBastionNodeSSHAccess(ctx, cloudClient, machineScope, server); err != nil { + machineScope.SetNotReady("BastionSSHAccessError", err.Error(), infrav1.MachineReadyCondition) return util.CloudFailureResult( - &sm.Status.Conditions, - sm.Generation, + &stackitMachine.Status.Conditions, + stackitMachine.Generation, "BastionSSHAccessError", err, retryableErrorRequeueAfter, @@ -113,11 +128,11 @@ func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, s *scope ) } - if err := r.reconcileAPIServerLoadBalancerTarget(ctx, cloudClient, s, server); err != nil { - s.SetNotReady("LoadBalancerTargetError", err.Error(), infrav1.MachineReadyCondition) + if err := r.reconcileAPIServerLoadBalancerTarget(ctx, cloudClient, machineScope, server); err != nil { + machineScope.SetNotReady("LoadBalancerTargetError", err.Error(), infrav1.MachineReadyCondition) return util.CloudFailureResult( - &sm.Status.Conditions, - sm.Generation, + &stackitMachine.Status.Conditions, + stackitMachine.Generation, "LoadBalancerTargetError", err, retryableErrorRequeueAfter, @@ -126,17 +141,17 @@ func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, s *scope ) } - s.SetReady() + machineScope.SetReady() log.V(1).Info("StackitMachine ready", "providerID", providerID) return ctrl.Result{}, nil } -func validateMachineAvailabilityZone(s *scope.MachineScope) error { - availabilityZone := s.StackitMachine.Spec.AvailabilityZone - if availabilityZone == "" || len(s.StackitCluster.Status.FailureDomains) == 0 { +func validateMachineAvailabilityZone(machineScope *scope.MachineScope) error { + availabilityZone := machineScope.StackitMachine.Spec.AvailabilityZone + if availabilityZone == "" || len(machineScope.StackitCluster.Status.FailureDomains) == 0 { return nil } - for _, failureDomain := range s.StackitCluster.Status.FailureDomains { + for _, failureDomain := range machineScope.StackitCluster.Status.FailureDomains { if failureDomain.Name == availabilityZone { return nil } @@ -144,47 +159,47 @@ func validateMachineAvailabilityZone(s *scope.MachineScope) error { return fmt.Errorf("availabilityZone %q is not published in StackitCluster status.failureDomains", availabilityZone) } -func (r *StackitMachineReconciler) reconcileDelete(ctx context.Context, s *scope.MachineScope) error { - sm := s.StackitMachine - needsLoadBalancerCleanup := isControlPlaneMachine(s.Machine) && - s.StackitCluster.Spec.APIServerLoadBalancer.Enabled && - s.StackitCluster.Status.APIServerLoadBalancerID != "" - if sm.Status.InstanceID == "" && !needsLoadBalancerCleanup { - controllerutil.RemoveFinalizer(sm, infrav1.MachineFinalizer) +func (r *StackitMachineReconciler) reconcileDelete(ctx context.Context, machineScope *scope.MachineScope) error { + stackitMachine := machineScope.StackitMachine + needsLoadBalancerCleanup := isControlPlaneMachine(machineScope.Machine) && + machineScope.StackitCluster.Spec.APIServerLoadBalancer.Enabled && + machineScope.StackitCluster.Status.APIServerLoadBalancerID != "" + if stackitMachine.Status.InstanceID == "" && !needsLoadBalancerCleanup { + controllerutil.RemoveFinalizer(stackitMachine, infrav1.MachineFinalizer) if r.Recorder != nil { - r.Recorder.Eventf(sm, nil, corev1.EventTypeNormal, "InstanceDeleted", "Delete", "Deleted instance") + r.Recorder.Eventf(stackitMachine, nil, corev1.EventTypeNormal, "InstanceDeleted", "Delete", "Deleted instance") } return nil } - cloudClient, err := util.BuildCloudClient(ctx, r.Client, r.CloudClientFactory, s.StackitCluster) + cloudClient, err := util.BuildCloudClient(ctx, r.Client, r.CloudClientFactory, machineScope.StackitCluster) if err != nil { _, resultErr := util.CredentialFailureResult( - &sm.Status.Conditions, - sm.Generation, + &stackitMachine.Status.Conditions, + stackitMachine.Generation, err, infrav1.MachineCredentialsReadyCondition, ) return resultErr } - if err := r.deleteAPIServerLoadBalancerTarget(ctx, cloudClient, s); err != nil { + if err := r.deleteAPIServerLoadBalancerTarget(ctx, cloudClient, machineScope); err != nil { return err } - if sm.Status.InstanceID == "" { - controllerutil.RemoveFinalizer(sm, infrav1.MachineFinalizer) + if stackitMachine.Status.InstanceID == "" { + controllerutil.RemoveFinalizer(stackitMachine, infrav1.MachineFinalizer) if r.Recorder != nil { - r.Recorder.Eventf(sm, nil, corev1.EventTypeNormal, "InstanceDeleted", "Delete", "Deleted instance") + r.Recorder.Eventf(stackitMachine, nil, corev1.EventTypeNormal, "InstanceDeleted", "Delete", "Deleted instance") } return nil } - instanceID := sm.Status.InstanceID + instanceID := stackitMachine.Status.InstanceID if err := cloudClient.DeleteServer(ctx, instanceID); err != nil && !cloud.IsNotFound(err) { return err } - s.ClearInstance() - controllerutil.RemoveFinalizer(sm, infrav1.MachineFinalizer) + machineScope.ClearInstance() + controllerutil.RemoveFinalizer(stackitMachine, infrav1.MachineFinalizer) if r.Recorder != nil { r.Recorder.Eventf( - sm, nil, corev1.EventTypeNormal, "InstanceDeleted", "Delete", "Deleted instance %s", instanceID, + stackitMachine, nil, corev1.EventTypeNormal, "InstanceDeleted", "Delete", "Deleted instance %s", instanceID, ) } return nil @@ -211,14 +226,14 @@ func (r *StackitMachineReconciler) fetchBootstrapData(ctx context.Context, machi func (r *StackitMachineReconciler) ensureServer( ctx context.Context, - c cloud.Client, - s *scope.MachineScope, + cloudClient cloud.Client, + machineScope *scope.MachineScope, userData []byte, ) (*cloud.Server, bool, error) { - sm := s.StackitMachine - tags := s.Tags() - if sm.Status.InstanceID != "" { - server, err := c.GetServer(ctx, sm.Status.InstanceID) + stackitMachine := machineScope.StackitMachine + tags := machineScope.Tags() + if stackitMachine.Status.InstanceID != "" { + server, err := cloudClient.GetServer(ctx, stackitMachine.Status.InstanceID) if err == nil { return server, false, nil } @@ -226,7 +241,7 @@ func (r *StackitMachineReconciler) ensureServer( return nil, false, err } } - if server, err := c.FindServerByTags(ctx, tags); err == nil { + if server, err := cloudClient.FindServerByTags(ctx, tags); err == nil { return server, false, nil } else if !cloud.IsNotFound(err) { return nil, false, err @@ -239,32 +254,32 @@ func (r *StackitMachineReconciler) ensureServer( // deleted server (same IP). Neither restores the cluster, and both consume // another VM silently. Surface it instead and let Cluster API decide to // replace the Machine. - if sm.Status.Initialization.Provisioned { + if stackitMachine.Status.Initialization.Provisioned { return nil, false, fmt.Errorf( "%w: server %s for already-provisioned machine no longer exists; the Machine must be replaced", - cloud.ErrNotFound, sm.Status.InstanceID, + cloud.ErrNotFound, stackitMachine.Status.InstanceID, ) } deleteOnTermination := true - if sm.Spec.RootVolume.DeleteOnTermination != nil { - deleteOnTermination = *sm.Spec.RootVolume.DeleteOnTermination - } - server, err := c.CreateServer(ctx, cloud.CreateServerInput{ - Name: sm.Name, - ProjectID: s.StackitCluster.Spec.ProjectID, - Region: s.StackitCluster.Spec.Region, - ImageID: sm.Spec.ImageID, - MachineType: sm.Spec.MachineType, - AvailabilityZone: sm.Spec.AvailabilityZone, - SSHKeyName: sm.Spec.SSHKeyName, - NetworkID: sm.Spec.Network.ID, - SecurityGroups: sm.Spec.SecurityGroups, + if stackitMachine.Spec.RootVolume.DeleteOnTermination != nil { + deleteOnTermination = *stackitMachine.Spec.RootVolume.DeleteOnTermination + } + server, err := cloudClient.CreateServer(ctx, cloud.CreateServerInput{ + Name: stackitMachine.Name, + ProjectID: machineScope.StackitCluster.Spec.ProjectID, + Region: machineScope.StackitCluster.Spec.Region, + ImageID: stackitMachine.Spec.ImageID, + MachineType: stackitMachine.Spec.MachineType, + AvailabilityZone: stackitMachine.Spec.AvailabilityZone, + SSHKeyName: stackitMachine.Spec.SSHKeyName, + NetworkID: stackitMachine.Spec.Network.ID, + SecurityGroups: stackitMachine.Spec.SecurityGroups, UserData: userData, Tags: tags, RootVolume: cloud.RootVolumeInput{ - SizeGiB: sm.Spec.RootVolume.SizeGiB, - PerformanceClass: sm.Spec.RootVolume.PerformanceClass, + SizeGiB: stackitMachine.Spec.RootVolume.SizeGiB, + PerformanceClass: stackitMachine.Spec.RootVolume.PerformanceClass, DeleteOnTermination: deleteOnTermination, }, }) @@ -273,70 +288,74 @@ func (r *StackitMachineReconciler) ensureServer( func (r *StackitMachineReconciler) reconcileBastionNodeSSHAccess( ctx context.Context, - c cloud.Client, - s *scope.MachineScope, + cloudClient cloud.Client, + machineScope *scope.MachineScope, server *cloud.Server, ) error { - if !s.StackitCluster.Spec.Bastion.Enabled { + if !machineScope.StackitCluster.Spec.Bastion.Enabled { return nil } - if s.StackitCluster.Status.Bastion.SecurityGroupID == "" { + if machineScope.StackitCluster.Status.Bastion.SecurityGroupID == "" { return fmt.Errorf("%w: bastion security group ID is empty", cloud.ErrTransient) } if server == nil || server.ID == "" { return fmt.Errorf("%w: server ID is empty", cloud.ErrTransient) } - _, err := c.EnsureNodeSSHAccess(ctx, cloud.NodeSSHAccessInput{ - Name: s.StackitCluster.Name + "-node-ssh", + _, err := cloudClient.EnsureNodeSSHAccess(ctx, cloud.NodeSSHAccessInput{ + Name: machineScope.StackitCluster.Name + "-node-ssh", ServerID: server.ID, - BastionSecurityGroupID: s.StackitCluster.Status.Bastion.SecurityGroupID, - Tags: bastionservice.NodeSSHAccessTags(s.StackitCluster), + BastionSecurityGroupID: machineScope.StackitCluster.Status.Bastion.SecurityGroupID, + Tags: bastionservice.NodeSSHAccessTags(machineScope.StackitCluster), }) return err } func (r *StackitMachineReconciler) reconcileAPIServerLoadBalancerTarget( ctx context.Context, - c cloud.Client, - s *scope.MachineScope, + cloudClient cloud.Client, + machineScope *scope.MachineScope, server *cloud.Server, ) error { - if !isControlPlaneMachine(s.Machine) || !s.StackitCluster.Spec.APIServerLoadBalancer.Enabled { + if !isControlPlaneMachine(machineScope.Machine) || !machineScope.StackitCluster.Spec.APIServerLoadBalancer.Enabled { return nil } loadBalancerID, err := loadbalancerservice.EnsureForMachine( ctx, - c, - s.StackitCluster, - s.Machine.Name, + cloudClient, + machineScope.StackitCluster, + machineScope.Machine.Name, server.Addresses, ) if err != nil { return err } - target, err := loadbalancerservice.TargetForMachine(s.Machine.Name, server.Addresses) + target, err := loadbalancerservice.TargetForMachine(machineScope.Machine.Name, server.Addresses) if err != nil { return err } target.LoadBalancerID = loadBalancerID - return c.EnsureAPIServerLoadBalancerTarget(ctx, target) + return cloudClient.EnsureAPIServerLoadBalancerTarget(ctx, target) } -func (r *StackitMachineReconciler) deleteAPIServerLoadBalancerTarget(ctx context.Context, c cloud.Client, s *scope.MachineScope) error { - if !isControlPlaneMachine(s.Machine) || !s.StackitCluster.Spec.APIServerLoadBalancer.Enabled { +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, c, s.StackitCluster) + loadBalancerID, err := loadbalancerservice.ResolveID(ctx, cloudClient, machineScope.StackitCluster) if err != nil { return err } if loadBalancerID == "" { return nil } - err = c.DeleteAPIServerLoadBalancerTarget(ctx, cloud.LoadBalancerTargetInput{ + err = cloudClient.DeleteAPIServerLoadBalancerTarget(ctx, cloud.LoadBalancerTargetInput{ LoadBalancerID: loadBalancerID, - Name: s.Machine.Name, + Name: machineScope.Machine.Name, Port: defaultAPIServerPort, }) if cloud.IsNotFound(err) { diff --git a/scope/cluster_scope.go b/scope/cluster_scope.go index 772499a..f8d67ea 100644 --- a/scope/cluster_scope.go +++ b/scope/cluster_scope.go @@ -41,17 +41,17 @@ type ClusterScope struct { func NewClusterScope( k8sClient client.Client, cluster *clusterv1.Cluster, - sc *infrav1.StackitCluster, + stackitCluster *infrav1.StackitCluster, ) (*ClusterScope, error) { - ph, err := patch.NewHelper(sc, k8sClient) + patchHelper, err := patch.NewHelper(stackitCluster, k8sClient) if err != nil { return nil, err } return &ClusterScope{ Client: k8sClient, Cluster: cluster, - StackitCluster: sc, - patchHelper: ph, + StackitCluster: stackitCluster, + patchHelper: patchHelper, }, nil } diff --git a/scope/machine_scope.go b/scope/machine_scope.go index 0e982e7..23918cd 100644 --- a/scope/machine_scope.go +++ b/scope/machine_scope.go @@ -44,7 +44,7 @@ func NewMachineScope( stackitCluster *infrav1.StackitCluster, stackitMachine *infrav1.StackitMachine, ) (*MachineScope, error) { - ph, err := patch.NewHelper(stackitMachine, k8sClient) + patchHelper, err := patch.NewHelper(stackitMachine, k8sClient) if err != nil { return nil, err } @@ -54,7 +54,7 @@ func NewMachineScope( Machine: machine, StackitCluster: stackitCluster, StackitMachine: stackitMachine, - patchHelper: ph, + patchHelper: patchHelper, }, nil } From 5ecc8528d75ce65f22620380a5c824c619668dec Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Wed, 19 Aug 2026 12:32:03 +0000 Subject: [PATCH 02/15] fix: resolve the owning Cluster in the StackitCluster to StackitMachine watch --- controller/stackitmachine_controller_test.go | 44 ++++++++++++++++++++ controller/stackitmachine_watches.go | 22 +++++++++- 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/controller/stackitmachine_controller_test.go b/controller/stackitmachine_controller_test.go index ed72106..b72a008 100644 --- a/controller/stackitmachine_controller_test.go +++ b/controller/stackitmachine_controller_test.go @@ -402,6 +402,50 @@ var _ = Describe("StackitMachine Controller", func() { Expect(requests).To(ConsistOf(request)) }) + // Regression test for debug/watch-wiring-bug.md defect 2: the mapper matched + // Machine.spec.clusterName against the StackitCluster name, so it enqueued + // nothing as soon as the two differed. Every other spec here hides the bug + // because createOwnerCluster gives the Cluster and its infrastructureRef the + // same name; a ClusterClass-generated infrastructureRef never does. + It("maps StackitCluster events when the StackitCluster name differs from the Cluster name", func() { + suffix := time.Now().UnixNano() + ownerClusterName := fmt.Sprintf("owner-%d", suffix) + infraClusterName := ownerClusterName + "-infra" + otherMachineName := fmt.Sprintf("other-machine-%d", suffix) + otherStackitName := fmt.Sprintf("other-stackit-machine-%d", suffix) + + By("creating a Cluster whose infrastructureRef points at a differently named StackitCluster") + ownerCluster := &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{Name: ownerClusterName, Namespace: namespace}, + Spec: clusterv1.ClusterSpec{ + InfrastructureRef: clusterv1.ContractVersionedObjectReference{ + APIGroup: infrav1.GroupVersion.Group, + Kind: "StackitCluster", + Name: infraClusterName, + }, + }, + } + Expect(k8sClient.Create(ctx, ownerCluster)).To(Succeed()) + + infraCluster := newStackitCluster(infraClusterName, namespace, false) + infraCluster.OwnerReferences[0].Name = ownerClusterName + Expect(k8sClient.Create(ctx, infraCluster)).To(Succeed()) + + createOwnerMachine(ctx, otherMachineName, namespace, ownerClusterName, otherStackitName, nil) + + DeferCleanup(func() { + deleteIfExists(ctx, &clusterv1.Machine{ObjectMeta: metav1.ObjectMeta{Name: otherMachineName, Namespace: namespace}}) + deleteIfExists(ctx, infraCluster) + deleteIfExists(ctx, ownerCluster) + }) + + By("mapping the StackitCluster event onto the Machine owned by its Cluster") + requests := reconciler.stackitMachineRequestsForStackitCluster(ctx, infraCluster) + Expect(requests).To(ConsistOf(reconcile.Request{ + NamespacedName: types.NamespacedName{Namespace: namespace, Name: otherStackitName}, + }), "the StackitCluster watch must resolve its owning Cluster instead of matching on its own name") + }) + It("maps bootstrap Secret events to StackitMachine reconcile requests", func() { updateMachineBootstrapSecret(ctx, machineName, bootstrapName) secret := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: bootstrapName, Namespace: namespace}} diff --git a/controller/stackitmachine_watches.go b/controller/stackitmachine_watches.go index 266adfc..9fa4b0c 100644 --- a/controller/stackitmachine_watches.go +++ b/controller/stackitmachine_watches.go @@ -14,8 +14,10 @@ import ( "context" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" + clusterutil "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/controller-runtime/pkg/client" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -37,14 +39,30 @@ func (r *StackitMachineReconciler) stackitMachineRequestsForStackitCluster(ctx c return nil } + // Machine.spec.clusterName names the owning Cluster, not the StackitCluster, + // and nothing forces the two to share a name — a ClusterClass-generated + // infrastructureRef carries a random suffix. Resolve the owning Cluster + // first, then match against its name. + cluster, err := clusterutil.GetOwnerCluster(ctx, r.Client, stackitCluster.ObjectMeta) + switch { + case apierrors.IsNotFound(err) || cluster == nil: + return nil + case err != nil: + logf.FromContext(ctx).Error(err, "Failed to get owning Cluster for StackitCluster watch", "stackitCluster", client.ObjectKeyFromObject(stackitCluster)) + return nil + } + machines := &clusterv1.MachineList{} - if err := r.List(ctx, machines, client.InNamespace(stackitCluster.Namespace)); err != nil { + if err := r.List(ctx, machines, + client.InNamespace(stackitCluster.Namespace), + client.MatchingLabels{clusterv1.ClusterNameLabel: cluster.Name}, + ); err != nil { logf.FromContext(ctx).Error(err, "Failed to list Machines for StackitCluster watch", "stackitCluster", client.ObjectKeyFromObject(stackitCluster)) return nil } return stackitMachineRequestsForMachines(machines.Items, func(machine clusterv1.Machine) bool { - return machine.Spec.ClusterName == stackitCluster.Name + return machine.Spec.ClusterName == cluster.Name }) } From ba5af15e539154e6ba80ac2e07bb563ff1e7fe22 Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Wed, 19 Aug 2026 13:58:32 +0000 Subject: [PATCH 03/15] fix: keep the StackitCluster finalizer until all its Machines are gone --- controller/constants.go | 5 ++ controller/controller_test_helpers_test.go | 11 ++-- controller/stackitcluster_controller.go | 3 +- controller/stackitcluster_controller_test.go | 62 ++++++++++++++++++++ controller/stackitcluster_infrastructure.go | 44 +++++++++++--- controller/stackitmachine_controller_test.go | 4 +- 6 files changed, 111 insertions(+), 18 deletions(-) diff --git a/controller/constants.go b/controller/constants.go index 6a27a27..cf19128 100644 --- a/controller/constants.go +++ b/controller/constants.go @@ -18,4 +18,9 @@ const ( cloudInitRefKindSecret = "Secret" retryableErrorRequeueAfter = 5 * time.Second + + // deleteRequeueAfter paces the wait for dependent objects to disappear + // during deletion. Matches what Cluster API and the other infrastructure + // providers use for the same purpose. + deleteRequeueAfter = 5 * time.Second ) diff --git a/controller/controller_test_helpers_test.go b/controller/controller_test_helpers_test.go index fd95fb3..110e266 100644 --- a/controller/controller_test_helpers_test.go +++ b/controller/controller_test_helpers_test.go @@ -87,15 +87,14 @@ func createOwnerCluster(ctx context.Context, name string) { Expect(k8sClient.Create(ctx, cluster)).To(Succeed()) } -func createOwnerMachine(ctx context.Context, name, namespace, clusterName, stackitMachineName string, bootstrapSecretName *string) { - if bootstrapSecretName == nil { - empty := "" - bootstrapSecretName = &empty - } +// Machines are created without bootstrap data; the specs that need it attach +// it afterwards via updateMachineBootstrapSecret. +func createOwnerMachine(ctx context.Context, name, clusterName, stackitMachineName string) { + bootstrapSecretName := new("") machine := &clusterv1.Machine{ ObjectMeta: metav1.ObjectMeta{ Name: name, - Namespace: namespace, + Namespace: "default", Labels: map[string]string{ clusterv1.ClusterNameLabel: clusterName, }, diff --git a/controller/stackitcluster_controller.go b/controller/stackitcluster_controller.go index 47ddd09..a7eaa81 100644 --- a/controller/stackitcluster_controller.go +++ b/controller/stackitcluster_controller.go @@ -54,6 +54,7 @@ type StackitClusterReconciler struct { // +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=stackitclusters/status,verbs=get;update;patch // +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=stackitclusters/finalizers,verbs=update // +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=clusters,verbs=get;list;watch +// +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machines,verbs=get;list;watch // +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch // +kubebuilder:rbac:groups="",resources=configmaps,verbs=get;list;watch // +kubebuilder:rbac:groups="",resources=events,verbs=create;patch @@ -94,7 +95,7 @@ func (r *StackitClusterReconciler) Reconcile(ctx context.Context, req ctrl.Reque util.SetPausedCondition(&stackitCluster.Status.Conditions, stackitCluster.Generation, false, "") if !stackitCluster.DeletionTimestamp.IsZero() { - return ctrl.Result{}, r.reconcileDelete(ctx, clusterScope) + return r.reconcileDelete(ctx, clusterScope) } return r.reconcileNormal(ctx, clusterScope) } diff --git a/controller/stackitcluster_controller_test.go b/controller/stackitcluster_controller_test.go index ef24ac0..ad6a529 100644 --- a/controller/stackitcluster_controller_test.go +++ b/controller/stackitcluster_controller_test.go @@ -580,6 +580,68 @@ var _ = Describe("StackitCluster Controller", func() { }).Should(BeTrue()) }) + // Regression tests for debug/deletion-bug.md: the finalizer used to go away + // regardless of remaining Machines. Their controllers reach credentials and + // project context through this StackitCluster, so once it is gone they can + // neither delete their servers nor drop their own finalizers — the VMs are + // orphaned and the Machines hang. Cluster API orders this correctly when the + // deletion starts at the Cluster, but a namespace teardown or a direct + // delete of this object bypasses that ordering. + It("keeps the finalizer while Machines still exist for the cluster", func() { + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + machineName := "machine-" + clusterName + createOwnerMachine(ctx, machineName, clusterName, "stackit-"+machineName) + DeferCleanup(func() { + deleteIfExists(ctx, &clusterv1.Machine{ObjectMeta: metav1.ObjectMeta{Name: machineName, Namespace: namespace}}) + }) + + got := &infrav1.StackitCluster{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + Expect(fakeCloud.LoadBalancerCount()).To(Equal(1)) + Expect(k8sClient.Delete(ctx, got)).To(Succeed()) + + By("requeueing instead of tearing down shared infrastructure") + result, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(Equal(deleteRequeueAfter)) + + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + Expect(got.Finalizers).To(ContainElement(infrav1.ClusterFinalizer), + "the finalizer must survive while a Machine still needs the StackitCluster") + Expect(fakeCloud.LoadBalancerCount()).To(Equal(1), + "the load balancer must not be deleted while Machines still exist") + }) + + It("removes the finalizer once the last Machine is gone", func() { + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + machineName := "machine-" + clusterName + createOwnerMachine(ctx, machineName, clusterName, "stackit-"+machineName) + + got := &infrav1.StackitCluster{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + Expect(k8sClient.Delete(ctx, got)).To(Succeed()) + + result, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(Equal(deleteRequeueAfter)) + + By("deleting the last Machine") + deleteIfExists(ctx, &clusterv1.Machine{ObjectMeta: metav1.ObjectMeta{Name: machineName, Namespace: namespace}}) + + _, err = reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + Expect(fakeCloud.LoadBalancerCount()).To(Equal(0)) + Eventually(func() bool { + err := k8sClient.Get(ctx, stackitKey, &infrav1.StackitCluster{}) + return apierrors.IsNotFound(err) + }).Should(BeTrue()) + }) + It("keeps the finalizer when load balancer deletion returns a transient error", func() { _, err := reconciler.Reconcile(ctx, request) Expect(err).NotTo(HaveOccurred()) diff --git a/controller/stackitcluster_infrastructure.go b/controller/stackitcluster_infrastructure.go index 8d7b4b7..437183f 100644 --- a/controller/stackitcluster_infrastructure.go +++ b/controller/stackitcluster_infrastructure.go @@ -12,6 +12,7 @@ package controller import ( "context" + "fmt" "net/netip" "time" @@ -20,6 +21,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" logf "sigs.k8s.io/controller-runtime/pkg/log" @@ -222,9 +224,33 @@ func bootstrapTargetIP(network *cloud.Network) string { return "10.0.0.1" } -func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, clusterScope *scope.ClusterScope) error { +func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, clusterScope *scope.ClusterScope) (ctrl.Result, error) { stackitCluster := clusterScope.StackitCluster + // Machines resolve their credentials and project context through this + // StackitCluster, so removing the finalizer while any of them remain leaves + // them unable to delete their own servers. Cluster API orders this correctly + // when deletion starts at the Cluster, but a namespace teardown or a direct + // delete of this object bypasses that ordering entirely. + // + // Selects the same way util/collections.GetFilteredMachinesForCluster does, + // inlined because that package pulls in the kubeadm bootstrap API for a + // query this short. Cluster API sets the label on every Machine it owns. + machines := &clusterv1.MachineList{} + if err := r.List(ctx, machines, + client.InNamespace(clusterScope.Cluster.Namespace), + client.MatchingLabels{clusterv1.ClusterNameLabel: clusterScope.Cluster.Name}, + ); err != nil { + return ctrl.Result{}, fmt.Errorf("list Machines for cluster %s: %w", clusterScope.Cluster.Name, err) + } + if len(machines.Items) > 0 { + logf.FromContext(ctx).Info( + "Waiting for Machines to be deleted before removing the StackitCluster finalizer", + "remainingMachines", len(machines.Items), + ) + return ctrl.Result{RequeueAfter: deleteRequeueAfter}, nil + } + // Cleanup runs unconditionally. Neither spec nor status is a trustworthy // record of what exists in the cloud: a resource can be created before its // status patch lands, and disabling the load balancer or the bastion leaves @@ -243,10 +269,10 @@ func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, clusterS if r.Recorder != nil { r.Recorder.Eventf(stackitCluster, nil, corev1.EventTypeWarning, "CleanupSkipped", "Delete", "Credentials Secret is gone; finalizing without cloud cleanup. "+ - "Any remaining STACKIT resources for this stackitCluster must be removed manually: %v", err) + "Any remaining STACKIT resources for this cluster must be removed manually: %v", err) } controllerutil.RemoveFinalizer(stackitCluster, infrav1.ClusterFinalizer) - return nil + return ctrl.Result{}, nil } util.SetConditions( &stackitCluster.Status.Conditions, @@ -256,15 +282,15 @@ func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, clusterS err.Error(), infrav1.ClusterCredentialsReadyCondition, ) - return err + return ctrl.Result{}, err } loadBalancerID, err := loadbalancerservice.ResolveID(ctx, cloudClient, stackitCluster) if err != nil { - return err + return ctrl.Result{}, err } if loadBalancerID != "" { if err := cloudClient.DeleteAPIServerLoadBalancer(ctx, loadBalancerID); err != nil && !cloud.IsNotFound(err) { - return err + return ctrl.Result{}, err } stackitCluster.Status.APIServerLoadBalancerID = "" if r.Recorder != nil { @@ -275,7 +301,7 @@ func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, clusterS } } if err := cloudClient.DeleteNodeSSHAccess(ctx, bastionservice.NodeSSHAccessTags(stackitCluster)); err != nil && !cloud.IsNotFound(err) { - return err + return ctrl.Result{}, err } if err := cloudClient.DeleteBastion(ctx, bastionservice.Input(stackitCluster, nil), cloud.Bastion{ ServerID: stackitCluster.Status.Bastion.ServerID, @@ -283,7 +309,7 @@ func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, clusterS PublicIP: stackitCluster.Status.Bastion.PublicIP, SecurityGroupID: stackitCluster.Status.Bastion.SecurityGroupID, }); err != nil && !cloud.IsNotFound(err) { - return err + return ctrl.Result{}, err } if hasBastionStatus(stackitCluster.Status.Bastion) { clusterScope.ClearBastionStatus() @@ -292,5 +318,5 @@ func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, clusterS } } controllerutil.RemoveFinalizer(stackitCluster, infrav1.ClusterFinalizer) - return nil + return ctrl.Result{}, nil } diff --git a/controller/stackitmachine_controller_test.go b/controller/stackitmachine_controller_test.go index b72a008..6da4260 100644 --- a/controller/stackitmachine_controller_test.go +++ b/controller/stackitmachine_controller_test.go @@ -69,7 +69,7 @@ var _ = Describe("StackitMachine Controller", func() { createCredentialsSecret(ctx, credentials, namespace, testProjectID) createOwnerCluster(ctx, clusterName) createReadyStackitCluster(ctx, clusterName, namespace, credentials) - createOwnerMachine(ctx, machineName, namespace, clusterName, stackitName, nil) + createOwnerMachine(ctx, machineName, clusterName, stackitName) stackitMach = newStackitMachine(stackitName, namespace, machineName) Expect(k8sClient.Create(ctx, stackitMach)).To(Succeed()) }) @@ -431,7 +431,7 @@ var _ = Describe("StackitMachine Controller", func() { infraCluster.OwnerReferences[0].Name = ownerClusterName Expect(k8sClient.Create(ctx, infraCluster)).To(Succeed()) - createOwnerMachine(ctx, otherMachineName, namespace, ownerClusterName, otherStackitName, nil) + createOwnerMachine(ctx, otherMachineName, ownerClusterName, otherStackitName) DeferCleanup(func() { deleteIfExists(ctx, &clusterv1.Machine{ObjectMeta: metav1.ObjectMeta{Name: otherMachineName, Namespace: namespace}}) From 0922daf3105c0e3407cf7f62fde32a4453a23cf4 Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Wed, 19 Aug 2026 19:27:27 +0000 Subject: [PATCH 04/15] fix: look the server up by tags before dropping the StackitMachine finalizer --- controller/stackitmachine_controller_test.go | 38 ++++++++++ controller/stackitmachine_infrastructure.go | 79 ++++++++++++++------ 2 files changed, 93 insertions(+), 24 deletions(-) diff --git a/controller/stackitmachine_controller_test.go b/controller/stackitmachine_controller_test.go index 6da4260..bff4339 100644 --- a/controller/stackitmachine_controller_test.go +++ b/controller/stackitmachine_controller_test.go @@ -386,6 +386,44 @@ var _ = Describe("StackitMachine Controller", func() { }).Should(BeTrue()) }) + // Regression test for debug/deletion-bug.md section A: an empty + // status.instanceID was taken as proof that no VM had ever been created, so + // the finalizer went away without a single cloud call. If CreateServer had + // succeeded and the status patch had not, that server kept running, tagged + // and unreferenced by any object. + It("deletes a tagged server whose instance ID was lost from the status", func() { + updateMachineBootstrapSecret(ctx, machineName, bootstrapName) + createBootstrapSecret(ctx, bootstrapName) + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(fakeCloud.ServerCount()).To(Equal(1)) + + By("losing the persisted instance ID, as if the status patch never landed") + got := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + got.Status.InstanceID = "" + got.Status.ProviderID = "" + Expect(k8sClient.Status().Update(ctx, got)).To(Succeed()) + + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + got.Spec.ProviderID = nil + Expect(k8sClient.Update(ctx, got)).To(Succeed()) + + By("deleting the StackitMachine") + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + Expect(k8sClient.Delete(ctx, got)).To(Succeed()) + + _, err = reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + Expect(fakeCloud.ServerCount()).To(Equal(0), + "the tagged server leaked because deletion trusted the empty status") + Eventually(func() bool { + err := k8sClient.Get(ctx, stackitKey, &infrav1.StackitMachine{}) + return apierrors.IsNotFound(err) + }).Should(BeTrue()) + }) + It("maps owning Machine events to StackitMachine reconcile requests", func() { machine := &clusterv1.Machine{} Expect(k8sClient.Get(ctx, types.NamespacedName{Name: machineName, Namespace: namespace}, machine)).To(Succeed()) diff --git a/controller/stackitmachine_infrastructure.go b/controller/stackitmachine_infrastructure.go index b515ce3..34bbe6b 100644 --- a/controller/stackitmachine_infrastructure.go +++ b/controller/stackitmachine_infrastructure.go @@ -161,18 +161,26 @@ func validateMachineAvailabilityZone(machineScope *scope.MachineScope) error { func (r *StackitMachineReconciler) reconcileDelete(ctx context.Context, machineScope *scope.MachineScope) error { stackitMachine := machineScope.StackitMachine - needsLoadBalancerCleanup := isControlPlaneMachine(machineScope.Machine) && - machineScope.StackitCluster.Spec.APIServerLoadBalancer.Enabled && - machineScope.StackitCluster.Status.APIServerLoadBalancerID != "" - if stackitMachine.Status.InstanceID == "" && !needsLoadBalancerCleanup { - controllerutil.RemoveFinalizer(stackitMachine, infrav1.MachineFinalizer) - if r.Recorder != nil { - r.Recorder.Eventf(stackitMachine, nil, corev1.EventTypeNormal, "InstanceDeleted", "Delete", "Deleted instance") - } - return nil - } + + // The cloud client is built unconditionally: an empty status.instanceID is + // not proof that no server exists, so every deletion has to ask the cloud + // before it may drop the finalizer. cloudClient, err := util.BuildCloudClient(ctx, r.Client, r.CloudClientFactory, machineScope.StackitCluster) if err != nil { + // A missing credentials Secret can never be recovered from, and it + // commonly disappears first during namespace teardown. Blocking here + // would strand the Machine in Terminating forever, so finalize and make + // the possible leak loud instead — the same trade the cluster side + // makes. Any other credentials problem is fixable, so keep retrying. + if apierrors.IsNotFound(err) { + if r.Recorder != nil { + r.Recorder.Eventf(stackitMachine, nil, corev1.EventTypeWarning, "CleanupSkipped", "Delete", + "Credentials Secret is gone; finalizing without cloud cleanup. "+ + "Any remaining STACKIT server for this machine must be removed manually: %v", err) + } + controllerutil.RemoveFinalizer(stackitMachine, infrav1.MachineFinalizer) + return nil + } _, resultErr := util.CredentialFailureResult( &stackitMachine.Status.Conditions, stackitMachine.Generation, @@ -184,27 +192,50 @@ func (r *StackitMachineReconciler) reconcileDelete(ctx context.Context, machineS if err := r.deleteAPIServerLoadBalancerTarget(ctx, cloudClient, machineScope); err != nil { return err } - if stackitMachine.Status.InstanceID == "" { - controllerutil.RemoveFinalizer(stackitMachine, infrav1.MachineFinalizer) + + instanceID, err := r.resolveServerForDeletion(ctx, cloudClient, machineScope) + if err != nil { + return err + } + if instanceID != "" { + if err := cloudClient.DeleteServer(ctx, instanceID); err != nil && !cloud.IsNotFound(err) { + return err + } + machineScope.ClearInstance() if r.Recorder != nil { - r.Recorder.Eventf(stackitMachine, nil, corev1.EventTypeNormal, "InstanceDeleted", "Delete", "Deleted instance") + r.Recorder.Eventf( + stackitMachine, nil, corev1.EventTypeNormal, "InstanceDeleted", "Delete", "Deleted instance %s", instanceID, + ) } - return nil } - instanceID := stackitMachine.Status.InstanceID - if err := cloudClient.DeleteServer(ctx, instanceID); err != nil && !cloud.IsNotFound(err) { - return err - } - machineScope.ClearInstance() controllerutil.RemoveFinalizer(stackitMachine, infrav1.MachineFinalizer) - if r.Recorder != nil { - r.Recorder.Eventf( - stackitMachine, nil, corev1.EventTypeNormal, "InstanceDeleted", "Delete", "Deleted instance %s", instanceID, - ) - } return nil } +// resolveServerForDeletion reports the ID of the server backing this machine, or +// an empty string once the cloud confirms none exists. +// +// status.instanceID alone is not enough: CreateServer can succeed and the status +// patch can be lost, leaving a tagged server that no field on the object refers +// to. The tags carry the Machine UID and therefore still identify that server. +func (r *StackitMachineReconciler) resolveServerForDeletion( + ctx context.Context, + cloudClient cloud.Client, + machineScope *scope.MachineScope, +) (string, error) { + if instanceID := machineScope.StackitMachine.Status.InstanceID; instanceID != "" { + return instanceID, nil + } + server, err := cloudClient.FindServerByTags(ctx, machineScope.Tags()) + if err != nil { + if cloud.IsNotFound(err) { + return "", nil + } + return "", err + } + return server.ID, nil +} + func (r *StackitMachineReconciler) fetchBootstrapData(ctx context.Context, machine *clusterv1.Machine) ([]byte, metav1.ConditionStatus, string, string) { if machine.Spec.Bootstrap.DataSecretName == nil || *machine.Spec.Bootstrap.DataSecretName == "" { return nil, metav1.ConditionFalse, "BootstrapDataSecretMissing", "Machine.spec.bootstrap.dataSecretName is empty" From ea6ee6f922b90efe14e1d88e29b7b8079c4314c1 Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Thu, 20 Aug 2026 07:27:02 +0000 Subject: [PATCH 05/15] fix: keep Secret data out of the manager's informer cache --- cmd/manager/main.go | 42 ++++++++++++++++++++ cmd/manager/main_test.go | 83 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 125 insertions(+) create mode 100644 cmd/manager/main_test.go diff --git a/cmd/manager/main.go b/cmd/manager/main.go index ca4905e..2688d4b 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -25,11 +25,14 @@ import ( // to ensure that exec-entrypoint and run can make use of them. _ "k8s.io/client-go/plugin/pkg/client/auth" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" clientgoscheme "k8s.io/client-go/kubernetes/scheme" clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/cache" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/healthz" "sigs.k8s.io/controller-runtime/pkg/log/zap" "sigs.k8s.io/controller-runtime/pkg/metrics/filters" @@ -56,6 +59,43 @@ func init() { // +kubebuilder:scaffold:scheme } +// managerCacheOptions keeps Secret data out of the shared informer cache. Both +// controllers watch Secrets — credentials, bootstrap data and the bastion +// cloud-init — and a watch only needs an object's identity to enqueue a +// reconcile, never its contents, so caching credential bytes in memory buys +// nothing and exposes them to anything that can read the process. +// +// Deliberately no label selector on the entry, unlike Cluster API's own +// equivalent in internal/setup: the Secrets this provider watches carry no +// common label, so restricting the cache by one would silently stop the +// watches from firing rather than only hardening them. +func managerCacheOptions() cache.Options { + return cache.Options{ + ByObject: map[client.Object]cache.ByObject{ + &corev1.Secret{}: { + Transform: func(in any) (any, error) { + if secret, ok := in.(*corev1.Secret); ok { + secret.Data = nil + } + return in, nil + }, + }, + }, + } +} + +// managerClientOptions turns every Secret read into a live lookup. The cache no +// longer holds Secret data (see managerCacheOptions), so the reads that need +// the real bytes — util.BuildCloudClient and the bootstrap-data fetch — have to +// bypass it. +func managerClientOptions() client.Options { + return client.Options{ + Cache: &client.CacheOptions{ + DisableFor: []client.Object{&corev1.Secret{}}, + }, + } +} + // nolint:gocyclo func main() { var metricsAddr string @@ -160,6 +200,8 @@ func main() { mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{ Scheme: scheme, + Cache: managerCacheOptions(), + Client: managerClientOptions(), Metrics: metricsServerOptions, WebhookServer: webhookServer, HealthProbeBindAddress: probeAddr, diff --git a/cmd/manager/main_test.go b/cmd/manager/main_test.go new file mode 100644 index 0000000..0ca1843 --- /dev/null +++ b/cmd/manager/main_test.go @@ -0,0 +1,83 @@ +/* +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 main + +import ( + "testing" + + corev1 "k8s.io/api/core/v1" + "sigs.k8s.io/controller-runtime/pkg/cache" +) + +// secretCacheConfig returns the cache configuration registered for Secrets. +// ByObject is keyed by a sample object rather than by type, so the entry has to +// be found by the key's type — a freshly allocated &corev1.Secret{} is a +// different pointer and would never match as a map key. +func secretCacheConfig(t *testing.T) cache.ByObject { + t.Helper() + + for obj, byObject := range managerCacheOptions().ByObject { + if _, ok := obj.(*corev1.Secret); ok { + return byObject + } + } + t.Fatal("no cache configuration registered for Secrets") + return cache.ByObject{} +} + +func TestManagerCacheOptionsStripsSecretData(t *testing.T) { + transform := secretCacheConfig(t).Transform + if transform == nil { + t.Fatal("Secret cache configuration has no Transform") + } + + out, err := transform(&corev1.Secret{ + Data: map[string][]byte{"credentials": []byte("service-account-key")}, + }) + if err != nil { + t.Fatalf("Transform returned an error: %v", err) + } + secret, ok := out.(*corev1.Secret) + if !ok { + t.Fatalf("Transform returned %T, want *corev1.Secret", out) + } + if secret.Data != nil { + t.Errorf("Transform left Secret data in the cache: %v", secret.Data) + } +} + +// The Transform runs on everything committed to the Secret informer, including +// the tombstones the cache emits on deletion, so it has to pass anything that +// is not a Secret straight through instead of dropping it. +func TestManagerCacheOptionsPassesThroughNonSecrets(t *testing.T) { + in := &corev1.ConfigMap{Data: map[string]string{"key": "value"}} + + out, err := secretCacheConfig(t).Transform(in) + if err != nil { + t.Fatalf("Transform returned an error: %v", err) + } + if out != any(in) { + t.Errorf("Transform returned %v, want the input unchanged", out) + } +} + +func TestManagerClientOptionsDisablesSecretCache(t *testing.T) { + cacheOptions := managerClientOptions().Cache + if cacheOptions == nil { + t.Fatal("client options have no cache configuration") + } + for _, obj := range cacheOptions.DisableFor { + if _, ok := obj.(*corev1.Secret); ok { + return + } + } + t.Errorf("Secrets are not in DisableFor: %v", cacheOptions.DisableFor) +} From ec1762f5ab3126b03de8fb9297bcbc140c6b6700 Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Thu, 20 Aug 2026 07:42:07 +0000 Subject: [PATCH 06/15] fix: re-reconcile the StackitCluster when its credentials Secret changes --- controller/stackitcluster_controller.go | 39 ++++++++++++++++++ controller/stackitcluster_controller_test.go | 43 ++++++++++++++++++++ 2 files changed, 82 insertions(+) diff --git a/controller/stackitcluster_controller.go b/controller/stackitcluster_controller.go index a7eaa81..f5fc9a8 100644 --- a/controller/stackitcluster_controller.go +++ b/controller/stackitcluster_controller.go @@ -150,6 +150,44 @@ func (r *StackitClusterReconciler) stackitClusterRequestsForCloudInitRef(ctx con return requests } +// stackitClusterRequestsForCredentialsSecret enqueues every StackitCluster whose +// credentialsSecretRef points at the given Secret. +// +// Without it, correcting an invalid credentials Secret never reaches the +// cluster: CredentialFailureResult deliberately returns without a requeue, +// because retrying invalid credentials in a hot loop helps nobody — which only +// works if fixing them triggers a reconcile. +func (r *StackitClusterReconciler) stackitClusterRequestsForCredentialsSecret(ctx context.Context, obj client.Object) []reconcile.Request { + secret, ok := obj.(*corev1.Secret) + if !ok { + return nil + } + + // Listed across all namespaces on purpose: CredentialsSecretRef.Namespace is + // optional, so the Secret may well live somewhere other than the + // StackitCluster that references it. + clusters := &infrav1.StackitClusterList{} + if err := r.List(ctx, clusters); err != nil { + logf.FromContext(ctx).Error(err, "Failed to list StackitClusters for credentials Secret watch", "secret", client.ObjectKeyFromObject(secret)) + return nil + } + + secretKey := client.ObjectKeyFromObject(secret) + requests := make([]reconcile.Request, 0, len(clusters.Items)) + for _, cluster := range clusters.Items { + if util.CredentialsSecretKey(&cluster) != secretKey { + continue + } + requests = append(requests, reconcile.Request{ + NamespacedName: types.NamespacedName{ + Namespace: cluster.Namespace, + Name: cluster.Name, + }, + }) + } + return requests +} + // SetupWithManager registers the controller with the manager. func (r *StackitClusterReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). @@ -157,6 +195,7 @@ func (r *StackitClusterReconciler) SetupWithManager(mgr ctrl.Manager) error { Watches(&clusterv1.Cluster{}, handler.EnqueueRequestsFromMapFunc(r.stackitClusterRequestsForCluster)). Watches(&corev1.ConfigMap{}, handler.EnqueueRequestsFromMapFunc(r.stackitClusterRequestsForCloudInitRef)). Watches(&corev1.Secret{}, handler.EnqueueRequestsFromMapFunc(r.stackitClusterRequestsForCloudInitRef)). + Watches(&corev1.Secret{}, handler.EnqueueRequestsFromMapFunc(r.stackitClusterRequestsForCredentialsSecret)). Named("stackitcluster"). Complete(r) } diff --git a/controller/stackitcluster_controller_test.go b/controller/stackitcluster_controller_test.go index ad6a529..83fb60f 100644 --- a/controller/stackitcluster_controller_test.go +++ b/controller/stackitcluster_controller_test.go @@ -22,6 +22,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/reconcile" infrav1 "github.com/stackitcloud/cluster-api-provider-stackit/api/v1alpha1" @@ -700,6 +701,48 @@ var _ = Describe("StackitCluster Controller", func() { Expect(requests).To(Equal([]reconcile.Request{request})) }) + It("maps credentials Secret events to StackitCluster reconcile requests", func() { + secret := &corev1.Secret{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: credentials, Namespace: namespace}, secret)).To(Succeed()) + + requests := reconciler.stackitClusterRequestsForCredentialsSecret(ctx, secret) + Expect(requests).To(Equal([]reconcile.Request{request})) + }) + + It("maps credentials Secret events from a different namespace than the StackitCluster", func() { + otherNamespace := "credentials-elsewhere" + Expect(client.IgnoreAlreadyExists(k8sClient.Create(ctx, &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: otherNamespace}, + }))).To(Succeed()) + + otherCredentials := credentials + "-elsewhere" + createCredentialsSecret(ctx, otherCredentials, otherNamespace, testProjectID) + DeferCleanup(func() { + deleteIfExists(ctx, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: otherCredentials, Namespace: otherNamespace}}) + }) + + got := &infrav1.StackitCluster{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + got.Spec.CredentialsSecretRef = corev1.SecretReference{Name: otherCredentials, Namespace: otherNamespace} + Expect(k8sClient.Update(ctx, got)).To(Succeed()) + + secret := &corev1.Secret{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: otherCredentials, Namespace: otherNamespace}, secret)).To(Succeed()) + + requests := reconciler.stackitClusterRequestsForCredentialsSecret(ctx, secret) + Expect(requests).To(Equal([]reconcile.Request{request}), + "credentialsSecretRef.namespace is optional, so the mapper must not be limited to the Secret's own namespace") + }) + + It("ignores Secret events that no StackitCluster uses as credentials", func() { + secret := &corev1.Secret{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: credentials, Namespace: namespace}, secret)).To(Succeed()) + secret.Name = credentials + "-unused" + + requests := reconciler.stackitClusterRequestsForCredentialsSecret(ctx, secret) + Expect(requests).To(BeEmpty()) + }) + It("ignores Cluster events for other infrastructure providers", func() { cluster := &clusterv1.Cluster{ ObjectMeta: metav1.ObjectMeta{Name: "other", Namespace: namespace}, From bc837499d44d5ebe513b890d6c2b87915db7c578 Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Thu, 20 Aug 2026 08:30:49 +0000 Subject: [PATCH 07/15] fix: let deletion proceed when an owning object is already gone --- controller/stackitcluster_controller.go | 19 ++++-- controller/stackitcluster_controller_test.go | 50 +++++++++++++++ controller/stackitcluster_infrastructure.go | 26 +++++++- controller/stackitmachine_controller.go | 67 +++++++++++++------- controller/stackitmachine_controller_test.go | 38 +++++++++++ controller/stackitmachine_infrastructure.go | 30 +++++++++ 6 files changed, 198 insertions(+), 32 deletions(-) diff --git a/controller/stackitcluster_controller.go b/controller/stackitcluster_controller.go index f5fc9a8..3d95a11 100644 --- a/controller/stackitcluster_controller.go +++ b/controller/stackitcluster_controller.go @@ -69,12 +69,19 @@ func (r *StackitClusterReconciler) Reconcile(ctx context.Context, req ctrl.Reque return ctrl.Result{}, err } - cluster, err := clusterutil.GetOwnerCluster(ctx, r.Client, stackitCluster.ObjectMeta) - if err != nil { - return ctrl.Result{}, fmt.Errorf("get owner cluster: %w", err) - } - if cluster == nil { - log.Info("StackitCluster has no owning Cluster yet, requeueing") + deleting := !stackitCluster.DeletionTimestamp.IsZero() + cluster, ownerErr := clusterutil.GetOwnerCluster(ctx, r.Client, stackitCluster.ObjectMeta) + switch { + case ownerGone(ownerErr) && deleting: + // The owning Cluster is already gone and can never come back, so + // returning the error here would retry forever without this object ever + // reaching reconcileDelete. Deletion must not be blocked by a + // precondition only the normal path needs. + log.Info("Owning Cluster is gone, continuing deletion without it") + case ownerErr != nil: + return ctrl.Result{}, fmt.Errorf("get owner cluster: %w", ownerErr) + case cluster == nil && !deleting: + log.Info("StackitCluster has no owning Cluster yet, waiting") return ctrl.Result{}, nil } diff --git a/controller/stackitcluster_controller_test.go b/controller/stackitcluster_controller_test.go index 83fb60f..48b3791 100644 --- a/controller/stackitcluster_controller_test.go +++ b/controller/stackitcluster_controller_test.go @@ -643,6 +643,56 @@ var _ = Describe("StackitCluster Controller", func() { }).Should(BeTrue()) }) + // GetOwnerCluster returns an error once the owning Cluster is gone, and that + // error was returned from Reconcile. Since the Cluster can never come back, + // the StackitCluster retried forever without ever reaching reconcileDelete + // and stayed in Terminating for good. + It("finalizes deletion when the owning Cluster is already gone", func() { + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(fakeCloud.LoadBalancerCount()).To(Equal(1)) + + deleteIfExists(ctx, &clusterv1.Cluster{ObjectMeta: metav1.ObjectMeta{Name: clusterName, Namespace: namespace}}) + + got := &infrav1.StackitCluster{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + Expect(k8sClient.Delete(ctx, got)).To(Succeed()) + + _, err = reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(fakeCloud.LoadBalancerCount()).To(Equal(0)) + Eventually(func() bool { + err := k8sClient.Get(ctx, stackitKey, &infrav1.StackitCluster{}) + return apierrors.IsNotFound(err) + }).Should(BeTrue()) + }) + + It("still waits for Machines when the owning Cluster is already gone", func() { + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + machineName := "machine-" + clusterName + createOwnerMachine(ctx, machineName, clusterName, "stackit-"+machineName) + DeferCleanup(func() { + deleteIfExists(ctx, &clusterv1.Machine{ObjectMeta: metav1.ObjectMeta{Name: machineName, Namespace: namespace}}) + }) + + deleteIfExists(ctx, &clusterv1.Cluster{ObjectMeta: metav1.ObjectMeta{Name: clusterName, Namespace: namespace}}) + + got := &infrav1.StackitCluster{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + Expect(k8sClient.Delete(ctx, got)).To(Succeed()) + + result, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(Equal(deleteRequeueAfter), + "the Machine name has to come from the ownerReference once the Cluster is gone") + + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + Expect(got.Finalizers).To(ContainElement(infrav1.ClusterFinalizer)) + Expect(fakeCloud.LoadBalancerCount()).To(Equal(1)) + }) + It("keeps the finalizer when load balancer deletion returns a transient error", func() { _, err := reconciler.Reconcile(ctx, request) Expect(err).NotTo(HaveOccurred()) diff --git a/controller/stackitcluster_infrastructure.go b/controller/stackitcluster_infrastructure.go index 437183f..e8089d1 100644 --- a/controller/stackitcluster_infrastructure.go +++ b/controller/stackitcluster_infrastructure.go @@ -224,6 +224,17 @@ func bootstrapTargetIP(network *cloud.Network) string { return "10.0.0.1" } +// ownerClusterName reports the name of the Cluster this StackitCluster belongs +// to, without needing that Cluster to still exist. +func ownerClusterName(stackitCluster *infrav1.StackitCluster) string { + for _, ref := range stackitCluster.OwnerReferences { + if ref.Kind == "Cluster" { + return ref.Name + } + } + return "" +} + func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, clusterScope *scope.ClusterScope) (ctrl.Result, error) { stackitCluster := clusterScope.StackitCluster @@ -236,12 +247,21 @@ func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, clusterS // Selects the same way util/collections.GetFilteredMachinesForCluster does, // inlined because that package pulls in the kubeadm bootstrap API for a // query this short. Cluster API sets the label on every Machine it owns. + // + // The Cluster itself may already be gone — a namespace teardown deletes it + // in no particular order — so the name is taken from the ownerReference, + // which outlives it. Owner references are always same-namespace, so the + // StackitCluster's own namespace is the right one either way. + clusterName := ownerClusterName(stackitCluster) + if clusterScope.Cluster != nil { + clusterName = clusterScope.Cluster.Name + } machines := &clusterv1.MachineList{} if err := r.List(ctx, machines, - client.InNamespace(clusterScope.Cluster.Namespace), - client.MatchingLabels{clusterv1.ClusterNameLabel: clusterScope.Cluster.Name}, + client.InNamespace(stackitCluster.Namespace), + client.MatchingLabels{clusterv1.ClusterNameLabel: clusterName}, ); err != nil { - return ctrl.Result{}, fmt.Errorf("list Machines for cluster %s: %w", clusterScope.Cluster.Name, err) + return ctrl.Result{}, fmt.Errorf("list Machines for cluster %s: %w", clusterName, err) } if len(machines.Items) > 0 { logf.FromContext(ctx).Info( diff --git a/controller/stackitmachine_controller.go b/controller/stackitmachine_controller.go index d1aa1d1..c4d9de0 100644 --- a/controller/stackitmachine_controller.go +++ b/controller/stackitmachine_controller.go @@ -18,6 +18,7 @@ package controller import ( "context" + "errors" "fmt" corev1 "k8s.io/api/core/v1" @@ -66,31 +67,31 @@ func (r *StackitMachineReconciler) Reconcile(ctx context.Context, req ctrl.Reque return ctrl.Result{}, err } - machine, err := clusterutil.GetOwnerMachine(ctx, r.Client, stackitMachine.ObjectMeta) - if err != nil { - return ctrl.Result{}, fmt.Errorf("get owner machine: %w", err) - } - if machine == nil { - log.Info("StackitMachine has no owning Machine yet, requeueing") - return ctrl.Result{}, nil - } - - cluster, err := clusterutil.GetClusterFromMetadata(ctx, r.Client, machine.ObjectMeta) - if err != nil { - return ctrl.Result{}, fmt.Errorf("get cluster from machine metadata: %w", err) + // The owning objects are resolved as far as they still exist. While the + // object is being deleted an owner that is simply gone is tolerated, so the + // checks that need one sit behind the deletion branch below rather than in + // front of it. Every other error still fails, so a transient API problem + // cannot be mistaken for a missing owner and drop the finalizer over a + // server that is still running. + deleting := !stackitMachine.DeletionTimestamp.IsZero() + + machine, ownerErr := clusterutil.GetOwnerMachine(ctx, r.Client, stackitMachine.ObjectMeta) + if ownerErr != nil && (!deleting || !ownerGone(ownerErr)) { + return ctrl.Result{}, fmt.Errorf("get owner machine: %w", ownerErr) } - if cluster == nil { - log.Info("Machine has no owning Cluster yet, requeueing") - return ctrl.Result{}, nil - } - - stackitCluster, err := r.getStackitCluster(ctx, cluster) - if err != nil { - return ctrl.Result{}, err + var cluster *clusterv1.Cluster + if machine != nil { + cluster, ownerErr = clusterutil.GetClusterFromMetadata(ctx, r.Client, machine.ObjectMeta) + if ownerErr != nil && (!deleting || !ownerGone(ownerErr)) { + return ctrl.Result{}, fmt.Errorf("get cluster from machine metadata: %w", ownerErr) + } } - if stackitCluster == nil { - log.Info("StackitCluster not found, requeueing") - return ctrl.Result{}, nil + var stackitCluster *infrav1.StackitCluster + if cluster != nil { + stackitCluster, err = r.getStackitCluster(ctx, cluster) + if err != nil { + return ctrl.Result{}, err + } } machineScope, err := scope.NewMachineScope(r.Client, cluster, machine, stackitCluster, stackitMachine) @@ -112,9 +113,29 @@ func (r *StackitMachineReconciler) Reconcile(ctx context.Context, req ctrl.Reque if !stackitMachine.DeletionTimestamp.IsZero() { return ctrl.Result{}, r.reconcileDelete(ctx, machineScope) } + + switch { + case machine == nil: + log.Info("StackitMachine has no owning Machine yet, waiting") + return ctrl.Result{}, nil + case cluster == nil: + log.Info("Machine has no owning Cluster yet, waiting") + return ctrl.Result{}, nil + case stackitCluster == nil: + log.Info("StackitCluster not found, waiting") + return ctrl.Result{}, nil + } return r.reconcileNormal(ctx, machineScope) } +// ownerGone reports whether err means the owning object no longer exists, as +// opposed to not being readable right now. GetOwnerMachine and GetOwnerCluster +// surface a plain NotFound; GetClusterFromMetadata reports ErrNoCluster when the +// Machine has lost the label naming its Cluster. +func ownerGone(err error) bool { + return apierrors.IsNotFound(err) || errors.Is(err, clusterutil.ErrNoCluster) +} + // SetupWithManager registers the controller with the manager. func (r *StackitMachineReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). diff --git a/controller/stackitmachine_controller_test.go b/controller/stackitmachine_controller_test.go index bff4339..0d39c70 100644 --- a/controller/stackitmachine_controller_test.go +++ b/controller/stackitmachine_controller_test.go @@ -424,6 +424,44 @@ var _ = Describe("StackitMachine Controller", func() { }).Should(BeTrue()) }) + // Every one of these owning objects used to be checked before the + // DeletionTimestamp branch and returned without a requeue, so a + // StackitMachine whose owner disappeared first — a namespace teardown + // deletes in no particular order — could never be deleted again. + DescribeTable("finalizes deletion when an owning object is already gone", + func(deleteOwner func()) { + updateMachineBootstrapSecret(ctx, machineName, bootstrapName) + createBootstrapSecret(ctx, bootstrapName) + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(fakeCloud.ServerCount()).To(Equal(1)) + + deleteOwner() + + got := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + Expect(k8sClient.Delete(ctx, got)).To(Succeed()) + + _, err = reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(fakeCloud.ServerCount()).To(Equal(1), + "without the owning objects there are no credentials and no tags, so the server cannot be reached") + Eventually(func() bool { + err := k8sClient.Get(ctx, stackitKey, &infrav1.StackitMachine{}) + return apierrors.IsNotFound(err) + }).Should(BeTrue()) + }, + Entry("owning Machine", func() { + deleteIfExists(ctx, &clusterv1.Machine{ObjectMeta: metav1.ObjectMeta{Name: machineName, Namespace: namespace}}) + }), + Entry("owning Cluster", func() { + deleteIfExists(ctx, &clusterv1.Cluster{ObjectMeta: metav1.ObjectMeta{Name: clusterName, Namespace: namespace}}) + }), + Entry("owning StackitCluster", func() { + deleteIfExists(ctx, &infrav1.StackitCluster{ObjectMeta: metav1.ObjectMeta{Name: clusterName, Namespace: namespace}}) + }), + ) + It("maps owning Machine events to StackitMachine reconcile requests", func() { machine := &clusterv1.Machine{} Expect(k8sClient.Get(ctx, types.NamespacedName{Name: machineName, Namespace: namespace}, machine)).To(Succeed()) diff --git a/controller/stackitmachine_infrastructure.go b/controller/stackitmachine_infrastructure.go index 34bbe6b..a1da5a9 100644 --- a/controller/stackitmachine_infrastructure.go +++ b/controller/stackitmachine_infrastructure.go @@ -162,6 +162,22 @@ func validateMachineAvailabilityZone(machineScope *scope.MachineScope) error { func (r *StackitMachineReconciler) reconcileDelete(ctx context.Context, machineScope *scope.MachineScope) error { stackitMachine := machineScope.StackitMachine + // Without one of the owning objects the server can no longer be reached at + // all: the StackitCluster carries the credentials, the project and the + // region, and the Cluster and Machine names make up the tags that identify + // the server. Blocking here would leave the StackitMachine in Terminating + // forever, so finalize and make the possible leak loud instead — the same + // trade the missing-credentials path below makes. + if missing := missingOwner(machineScope); missing != "" { + if r.Recorder != nil { + r.Recorder.Eventf(stackitMachine, nil, corev1.EventTypeWarning, "CleanupSkipped", "Delete", + "%s is gone; finalizing without cloud cleanup. "+ + "Any remaining STACKIT server for this machine must be removed manually.", missing) + } + controllerutil.RemoveFinalizer(stackitMachine, infrav1.MachineFinalizer) + return nil + } + // The cloud client is built unconditionally: an empty status.instanceID is // not proof that no server exists, so every deletion has to ask the cloud // before it may drop the finalizer. @@ -212,6 +228,20 @@ func (r *StackitMachineReconciler) reconcileDelete(ctx context.Context, machineS return nil } +// missingOwner names the first owning object that no longer exists, or an empty +// string once all of them are present. +func missingOwner(machineScope *scope.MachineScope) string { + switch { + case machineScope.Machine == nil: + return "Owning Machine" + case machineScope.Cluster == nil: + return "Owning Cluster" + case machineScope.StackitCluster == nil: + return "Owning StackitCluster" + } + return "" +} + // resolveServerForDeletion reports the ID of the server backing this machine, or // an empty string once the cloud confirms none exists. // From 7a9cd2b717ef981be3142668dfda3a35836dfd9c Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Thu, 20 Aug 2026 08:37:50 +0000 Subject: [PATCH 08/15] chore: drop debug/ folder references from test comments --- controller/stackitcluster_controller_test.go | 24 +++++++-------- controller/stackitmachine_controller_test.go | 31 ++++++++++---------- 2 files changed, 27 insertions(+), 28 deletions(-) diff --git a/controller/stackitcluster_controller_test.go b/controller/stackitcluster_controller_test.go index 48b3791..0ff9633 100644 --- a/controller/stackitcluster_controller_test.go +++ b/controller/stackitcluster_controller_test.go @@ -204,11 +204,11 @@ var _ = Describe("StackitCluster Controller", func() { }) It("cleans up bastion resources during deletion even when bastion status was never persisted", func() { - // Regression test for debug/deletion-bug.md: the cloud-cleanup block used - // to be gated on persisted status for the bastion, while the load - // balancer was gated on its spec flag. A bastion created without its - // status patch landing (process restart, conflict) therefore skipped - // cleanup entirely and leaked server, public IP and security group. + // The cloud-cleanup block used to be gated on persisted status for the + // bastion, while the load balancer was gated on its spec flag. A bastion + // created without its status patch landing (process restart, conflict) + // therefore skipped cleanup entirely and leaked server, public IP and + // security group. createOwnerCluster(ctx, clusterName+"-nolb") defer deleteIfExists(ctx, &clusterv1.Cluster{ ObjectMeta: metav1.ObjectMeta{Name: clusterName + "-nolb", Namespace: namespace}, @@ -581,13 +581,13 @@ var _ = Describe("StackitCluster Controller", func() { }).Should(BeTrue()) }) - // Regression tests for debug/deletion-bug.md: the finalizer used to go away - // regardless of remaining Machines. Their controllers reach credentials and - // project context through this StackitCluster, so once it is gone they can - // neither delete their servers nor drop their own finalizers — the VMs are - // orphaned and the Machines hang. Cluster API orders this correctly when the - // deletion starts at the Cluster, but a namespace teardown or a direct - // delete of this object bypasses that ordering. + // The finalizer used to go away regardless of remaining Machines. Their + // controllers reach credentials and project context through this + // StackitCluster, so once it is gone they can neither delete their servers + // nor drop their own finalizers — the VMs are orphaned and the Machines + // hang. Cluster API orders this correctly when the deletion starts at the + // Cluster, but a namespace teardown or a direct delete of this object + // bypasses that ordering. It("keeps the finalizer while Machines still exist for the cluster", func() { _, err := reconciler.Reconcile(ctx, request) Expect(err).NotTo(HaveOccurred()) diff --git a/controller/stackitmachine_controller_test.go b/controller/stackitmachine_controller_test.go index 0d39c70..8b40cd6 100644 --- a/controller/stackitmachine_controller_test.go +++ b/controller/stackitmachine_controller_test.go @@ -132,12 +132,12 @@ var _ = Describe("StackitMachine Controller", func() { }) It("does not silently recreate the server of an already-provisioned machine", func() { - // Regression test for debug/machine-recreate-bug.md: when the backing - // server disappears out-of-band, ensureServer used to call CreateServer - // again, replaying the original bootstrap data. The replacement either - // never rejoins (different IP) or rejoins while Machine and Node keep - // pointing at the deleted server (same IP) — neither restores the - // cluster, and both consume another VM unnoticed. + // When the backing server disappears out-of-band, ensureServer used to + // call CreateServer again, replaying the original bootstrap data — which + // is pinned to the previous identity. The replacement either never + // rejoins (different IP) or rejoins while Machine and Node keep pointing + // at the deleted server (same IP) — neither restores the cluster, and + // both consume another VM unnoticed. updateMachineBootstrapSecret(ctx, machineName, bootstrapName) createBootstrapSecret(ctx, bootstrapName) @@ -386,11 +386,10 @@ var _ = Describe("StackitMachine Controller", func() { }).Should(BeTrue()) }) - // Regression test for debug/deletion-bug.md section A: an empty - // status.instanceID was taken as proof that no VM had ever been created, so - // the finalizer went away without a single cloud call. If CreateServer had - // succeeded and the status patch had not, that server kept running, tagged - // and unreferenced by any object. + // An empty status.instanceID used to be taken as proof that no VM had ever + // been created, so the finalizer went away without a single cloud call. If + // CreateServer had succeeded and the status patch had not, that server kept + // running, tagged and unreferenced by any object. It("deletes a tagged server whose instance ID was lost from the status", func() { updateMachineBootstrapSecret(ctx, machineName, bootstrapName) createBootstrapSecret(ctx, bootstrapName) @@ -478,11 +477,11 @@ var _ = Describe("StackitMachine Controller", func() { Expect(requests).To(ConsistOf(request)) }) - // Regression test for debug/watch-wiring-bug.md defect 2: the mapper matched - // Machine.spec.clusterName against the StackitCluster name, so it enqueued - // nothing as soon as the two differed. Every other spec here hides the bug - // because createOwnerCluster gives the Cluster and its infrastructureRef the - // same name; a ClusterClass-generated infrastructureRef never does. + // The mapper used to match Machine.spec.clusterName against the + // StackitCluster name, so it enqueued nothing as soon as the two differed. + // Every other spec here hides the bug because createOwnerCluster gives the + // Cluster and its infrastructureRef the same name; a ClusterClass-generated + // infrastructureRef never does. It("maps StackitCluster events when the StackitCluster name differs from the Cluster name", func() { suffix := time.Now().UnixNano() ownerClusterName := fmt.Sprintf("owner-%d", suffix) From 79689f7fc6f9090d971ad46179c8737d091895fa Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Thu, 20 Aug 2026 08:46:45 +0000 Subject: [PATCH 09/15] fix: persist the finalizer before the first cloud call --- cloud/fake/client.go | 14 +++++++++++ controller/stackitcluster_controller_test.go | 22 +++++++++++++++++ controller/stackitcluster_infrastructure.go | 8 +++++++ controller/stackitmachine_controller_test.go | 25 ++++++++++++++++++++ controller/stackitmachine_infrastructure.go | 8 +++++++ 5 files changed, 77 insertions(+) diff --git a/cloud/fake/client.go b/cloud/fake/client.go index 0d08966..fd843d7 100644 --- a/cloud/fake/client.go +++ b/cloud/fake/client.go @@ -55,6 +55,14 @@ type Client struct { FailNextEnsureNodeSSH error FailNextDeleteNodeSSH error + // Before* hooks, if non-nil, run before the call they belong to does any + // work. They let a test observe API server state at the exact moment a cloud + // call is about to happen — for instance to assert a finalizer was persisted + // before the first resource could be created. Unlike FailNext*, they are not + // consumed and fire on every call. + BeforeCreateServer func() + BeforeGetNetwork func() + // CreateServerCalls counts successful CreateServer calls (for idempotency // assertions). CreateServerCalls int @@ -171,6 +179,9 @@ func (c *Client) CreateServer(_ context.Context, input cloud.CreateServerInput) c.mu.Lock() defer c.mu.Unlock() + if c.BeforeCreateServer != nil { + c.BeforeCreateServer() + } if err := consume(&c.FailNextCreateServer); err != nil { return nil, err } @@ -215,6 +226,9 @@ func (c *Client) GetNetwork(_ context.Context, id string) (*cloud.Network, error c.mu.Lock() defer c.mu.Unlock() + if c.BeforeGetNetwork != nil { + c.BeforeGetNetwork() + } if err := consume(&c.FailNextGetNetwork); err != nil { return nil, err } diff --git a/controller/stackitcluster_controller_test.go b/controller/stackitcluster_controller_test.go index 0ff9633..1472757 100644 --- a/controller/stackitcluster_controller_test.go +++ b/controller/stackitcluster_controller_test.go @@ -581,6 +581,28 @@ var _ = Describe("StackitCluster Controller", func() { }).Should(BeTrue()) }) + // AddFinalizer only mutated the object in memory, and the write to etcd + // happened in the deferred PatchObject at the end of Reconcile — after the + // load balancer and the bastion had been created. A process dying in between + // left those resources behind an object with no finalizer to clean them up. + // GetNetwork is the first cloud call of the reconcile, so a hook there + // proves the ordering rather than only the end result. + It("persists the finalizer before the first cloud call", func() { + var finalizersAtFirstCall []string + fakeCloud.BeforeGetNetwork = func() { + got := &infrav1.StackitCluster{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + finalizersAtFirstCall = got.Finalizers + } + + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(fakeCloud.LoadBalancerCount()).To(Equal(1)) + + Expect(finalizersAtFirstCall).To(ContainElement(infrav1.ClusterFinalizer), + "cloud resources were created while the API server had no finalizer to clean them up") + }) + // The finalizer used to go away regardless of remaining Machines. Their // controllers reach credentials and project context through this // StackitCluster, so once it is gone they can neither delete their servers diff --git a/controller/stackitcluster_infrastructure.go b/controller/stackitcluster_infrastructure.go index e8089d1..d7c5da8 100644 --- a/controller/stackitcluster_infrastructure.go +++ b/controller/stackitcluster_infrastructure.go @@ -39,6 +39,14 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterS if !controllerutil.ContainsFinalizer(stackitCluster, infrav1.ClusterFinalizer) { controllerutil.AddFinalizer(stackitCluster, infrav1.ClusterFinalizer) + // Persisted immediately, before anything can create a cloud resource. + // AddFinalizer only mutates the object in memory; it otherwise reaches + // etcd through the deferred PatchObject at the end of Reconcile, and a + // process that dies in between leaves a load balancer or a bastion + // running behind an object that carries no finalizer to clean it up. + if err := clusterScope.PatchObject(ctx); err != nil { + return ctrl.Result{}, fmt.Errorf("persist finalizer: %w", err) + } } stackitCluster.Status.FailureDomains = stackitFailureDomains(stackitCluster.Spec.Region) diff --git a/controller/stackitmachine_controller_test.go b/controller/stackitmachine_controller_test.go index 8b40cd6..eb35ae6 100644 --- a/controller/stackitmachine_controller_test.go +++ b/controller/stackitmachine_controller_test.go @@ -386,6 +386,31 @@ var _ = Describe("StackitMachine Controller", func() { }).Should(BeTrue()) }) + // AddFinalizer only mutated the object in memory, and the write to etcd + // happened in the deferred PatchObject at the end of Reconcile — after + // CreateServer. A process dying in between left a running server behind an + // object with no finalizer to clean it up. The hook observes API server + // state from inside the cloud call, so it proves the ordering rather than + // only the end result. + It("persists the finalizer before creating the server", func() { + updateMachineBootstrapSecret(ctx, machineName, bootstrapName) + createBootstrapSecret(ctx, bootstrapName) + + var finalizersAtCreate []string + fakeCloud.BeforeCreateServer = func() { + got := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + finalizersAtCreate = got.Finalizers + } + + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(fakeCloud.ServerCount()).To(Equal(1)) + + Expect(finalizersAtCreate).To(ContainElement(infrav1.MachineFinalizer), + "the server was created while the API server had no finalizer to clean it up") + }) + // An empty status.instanceID used to be taken as proof that no VM had ever // been created, so the finalizer went away without a single cloud call. If // CreateServer had succeeded and the status patch had not, that server kept diff --git a/controller/stackitmachine_infrastructure.go b/controller/stackitmachine_infrastructure.go index a1da5a9..a6fc3e2 100644 --- a/controller/stackitmachine_infrastructure.go +++ b/controller/stackitmachine_infrastructure.go @@ -38,6 +38,14 @@ func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, machineS if !controllerutil.ContainsFinalizer(stackitMachine, infrav1.MachineFinalizer) { controllerutil.AddFinalizer(stackitMachine, infrav1.MachineFinalizer) + // Persisted immediately, before CreateServer can run. AddFinalizer only + // mutates the object in memory; it otherwise reaches etcd through the + // deferred PatchObject at the end of Reconcile, and a process that dies + // in between leaves a server running behind an object that carries no + // finalizer to clean it up. + if err := machineScope.PatchObject(ctx); err != nil { + return ctrl.Result{}, fmt.Errorf("persist finalizer: %w", err) + } } if !machineScope.StackitCluster.Status.Ready { From b5f1948af60fa616ee424f24414659bf6ba94b75 Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Thu, 20 Aug 2026 09:37:19 +0000 Subject: [PATCH 10/15] docs: correct the Cluster API contract version in the README --- README.md | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 2b56182..47eec19 100644 --- a/README.md +++ b/README.md @@ -61,9 +61,13 @@ Check out the [Quick Start](./quick-start.md) for launching a cluster on STACKIT This provider's versions are compatible with the following versions of Cluster API and support all Kubernetes versions that is supported by its compatible Cluster API version: -| | Cluster API v1alpha4 (v0.4) | Cluster API v1beta1 (v1.x) | -| ------------------------ | :-------------------------: | :------------------------: | -| CAPSTK v1alpha1 `(main)` | x | ✓ | +| | Cluster API v1alpha4 (v0.4) | Cluster API v1beta1 | Cluster API v1beta2 | +| ------------------------ | :-------------------------: | :-----------------: | :-----------------: | +| CAPSTK v1alpha1 `(main)` | x | x | ✓ | + +This provider implements the **v1beta2** contract, as declared in +[`metadata.yaml`](metadata.yaml), and is built against `sigs.k8s.io/cluster-api` +v1.13.2. (See [Kubernetes support matrix](https://cluster-api.sigs.k8s.io/reference/versions.html) of Cluster API versions). From 472953c028f0516f7bec1a47bb3e51852b4539a5 Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Thu, 20 Aug 2026 11:03:51 +0000 Subject: [PATCH 11/15] fix: strip managed fields from cached Secrets as well --- cmd/manager/main.go | 12 ++++++++---- cmd/manager/main_test.go | 7 +++++++ 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/cmd/manager/main.go b/cmd/manager/main.go index 2688d4b..46ecfd0 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -63,12 +63,15 @@ func init() { // controllers watch Secrets — credentials, bootstrap data and the bastion // cloud-init — and a watch only needs an object's identity to enqueue a // reconcile, never its contents, so caching credential bytes in memory buys -// nothing and exposes them to anything that can read the process. +// nothing and exposes them to anything that can read the process. Managed +// fields go with them: nothing here reads them, and they are usually the +// largest part of what is left once the data is gone. // // Deliberately no label selector on the entry, unlike Cluster API's own -// equivalent in internal/setup: the Secrets this provider watches carry no -// common label, so restricting the cache by one would silently stop the -// watches from firing rather than only hardening them. +// equivalent in internal/setup. Cluster API only watches Secrets it stamps +// itself, whereas the credentials and bastion cloud-init Secrets here are +// created by the user and carry no labels, so a selector would silently stop +// those watches from firing rather than only hardening the cache. func managerCacheOptions() cache.Options { return cache.Options{ ByObject: map[client.Object]cache.ByObject{ @@ -76,6 +79,7 @@ func managerCacheOptions() cache.Options { Transform: func(in any) (any, error) { if secret, ok := in.(*corev1.Secret); ok { secret.Data = nil + secret.SetManagedFields(nil) } return in, nil }, diff --git a/cmd/manager/main_test.go b/cmd/manager/main_test.go index 0ca1843..64b2692 100644 --- a/cmd/manager/main_test.go +++ b/cmd/manager/main_test.go @@ -14,6 +14,7 @@ import ( "testing" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/cache" ) @@ -40,6 +41,9 @@ func TestManagerCacheOptionsStripsSecretData(t *testing.T) { } out, err := transform(&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + ManagedFields: []metav1.ManagedFieldsEntry{{Manager: "kubectl", Operation: metav1.ManagedFieldsOperationUpdate}}, + }, Data: map[string][]byte{"credentials": []byte("service-account-key")}, }) if err != nil { @@ -52,6 +56,9 @@ func TestManagerCacheOptionsStripsSecretData(t *testing.T) { if secret.Data != nil { t.Errorf("Transform left Secret data in the cache: %v", secret.Data) } + if secret.ManagedFields != nil { + t.Errorf("Transform left managed fields in the cache: %v", secret.ManagedFields) + } } // The Transform runs on everything committed to the Secret informer, including From b5ba35ae2c9f62934e697345f4e614559e4bda0d Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Thu, 20 Aug 2026 13:52:05 +0000 Subject: [PATCH 12/15] Revert fix: let deletion proceed when an owning object is already gone --- controller/stackitcluster_controller.go | 19 ++---- controller/stackitcluster_controller_test.go | 50 --------------- controller/stackitcluster_infrastructure.go | 35 ++-------- controller/stackitmachine_controller.go | 67 +++++++------------- controller/stackitmachine_controller_test.go | 38 ----------- controller/stackitmachine_infrastructure.go | 30 --------- 6 files changed, 34 insertions(+), 205 deletions(-) diff --git a/controller/stackitcluster_controller.go b/controller/stackitcluster_controller.go index 3d95a11..f5fc9a8 100644 --- a/controller/stackitcluster_controller.go +++ b/controller/stackitcluster_controller.go @@ -69,19 +69,12 @@ func (r *StackitClusterReconciler) Reconcile(ctx context.Context, req ctrl.Reque return ctrl.Result{}, err } - deleting := !stackitCluster.DeletionTimestamp.IsZero() - cluster, ownerErr := clusterutil.GetOwnerCluster(ctx, r.Client, stackitCluster.ObjectMeta) - switch { - case ownerGone(ownerErr) && deleting: - // The owning Cluster is already gone and can never come back, so - // returning the error here would retry forever without this object ever - // reaching reconcileDelete. Deletion must not be blocked by a - // precondition only the normal path needs. - log.Info("Owning Cluster is gone, continuing deletion without it") - case ownerErr != nil: - return ctrl.Result{}, fmt.Errorf("get owner cluster: %w", ownerErr) - case cluster == nil && !deleting: - log.Info("StackitCluster has no owning Cluster yet, waiting") + cluster, err := clusterutil.GetOwnerCluster(ctx, r.Client, stackitCluster.ObjectMeta) + if err != nil { + return ctrl.Result{}, fmt.Errorf("get owner cluster: %w", err) + } + if cluster == nil { + log.Info("StackitCluster has no owning Cluster yet, requeueing") return ctrl.Result{}, nil } diff --git a/controller/stackitcluster_controller_test.go b/controller/stackitcluster_controller_test.go index 1472757..11501f9 100644 --- a/controller/stackitcluster_controller_test.go +++ b/controller/stackitcluster_controller_test.go @@ -665,56 +665,6 @@ var _ = Describe("StackitCluster Controller", func() { }).Should(BeTrue()) }) - // GetOwnerCluster returns an error once the owning Cluster is gone, and that - // error was returned from Reconcile. Since the Cluster can never come back, - // the StackitCluster retried forever without ever reaching reconcileDelete - // and stayed in Terminating for good. - It("finalizes deletion when the owning Cluster is already gone", func() { - _, err := reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - Expect(fakeCloud.LoadBalancerCount()).To(Equal(1)) - - deleteIfExists(ctx, &clusterv1.Cluster{ObjectMeta: metav1.ObjectMeta{Name: clusterName, Namespace: namespace}}) - - got := &infrav1.StackitCluster{} - Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) - Expect(k8sClient.Delete(ctx, got)).To(Succeed()) - - _, err = reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - Expect(fakeCloud.LoadBalancerCount()).To(Equal(0)) - Eventually(func() bool { - err := k8sClient.Get(ctx, stackitKey, &infrav1.StackitCluster{}) - return apierrors.IsNotFound(err) - }).Should(BeTrue()) - }) - - It("still waits for Machines when the owning Cluster is already gone", func() { - _, err := reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - - machineName := "machine-" + clusterName - createOwnerMachine(ctx, machineName, clusterName, "stackit-"+machineName) - DeferCleanup(func() { - deleteIfExists(ctx, &clusterv1.Machine{ObjectMeta: metav1.ObjectMeta{Name: machineName, Namespace: namespace}}) - }) - - deleteIfExists(ctx, &clusterv1.Cluster{ObjectMeta: metav1.ObjectMeta{Name: clusterName, Namespace: namespace}}) - - got := &infrav1.StackitCluster{} - Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) - Expect(k8sClient.Delete(ctx, got)).To(Succeed()) - - result, err := reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - Expect(result.RequeueAfter).To(Equal(deleteRequeueAfter), - "the Machine name has to come from the ownerReference once the Cluster is gone") - - Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) - Expect(got.Finalizers).To(ContainElement(infrav1.ClusterFinalizer)) - Expect(fakeCloud.LoadBalancerCount()).To(Equal(1)) - }) - It("keeps the finalizer when load balancer deletion returns a transient error", func() { _, err := reconciler.Reconcile(ctx, request) Expect(err).NotTo(HaveOccurred()) diff --git a/controller/stackitcluster_infrastructure.go b/controller/stackitcluster_infrastructure.go index d7c5da8..8aebb98 100644 --- a/controller/stackitcluster_infrastructure.go +++ b/controller/stackitcluster_infrastructure.go @@ -232,44 +232,19 @@ func bootstrapTargetIP(network *cloud.Network) string { return "10.0.0.1" } -// ownerClusterName reports the name of the Cluster this StackitCluster belongs -// to, without needing that Cluster to still exist. -func ownerClusterName(stackitCluster *infrav1.StackitCluster) string { - for _, ref := range stackitCluster.OwnerReferences { - if ref.Kind == "Cluster" { - return ref.Name - } - } - return "" -} - func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, clusterScope *scope.ClusterScope) (ctrl.Result, error) { stackitCluster := clusterScope.StackitCluster // Machines resolve their credentials and project context through this - // StackitCluster, so removing the finalizer while any of them remain leaves - // them unable to delete their own servers. Cluster API orders this correctly - // when deletion starts at the Cluster, but a namespace teardown or a direct - // delete of this object bypasses that ordering entirely. - // + // StackitCluster, so the finalizer has to stay while any of them remain. // Selects the same way util/collections.GetFilteredMachinesForCluster does, - // inlined because that package pulls in the kubeadm bootstrap API for a - // query this short. Cluster API sets the label on every Machine it owns. - // - // The Cluster itself may already be gone — a namespace teardown deletes it - // in no particular order — so the name is taken from the ownerReference, - // which outlives it. Owner references are always same-namespace, so the - // StackitCluster's own namespace is the right one either way. - clusterName := ownerClusterName(stackitCluster) - if clusterScope.Cluster != nil { - clusterName = clusterScope.Cluster.Name - } + // inlined because that package pulls in the kubeadm bootstrap API. machines := &clusterv1.MachineList{} if err := r.List(ctx, machines, - client.InNamespace(stackitCluster.Namespace), - client.MatchingLabels{clusterv1.ClusterNameLabel: clusterName}, + client.InNamespace(clusterScope.Cluster.Namespace), + client.MatchingLabels{clusterv1.ClusterNameLabel: clusterScope.Cluster.Name}, ); err != nil { - return ctrl.Result{}, fmt.Errorf("list Machines for cluster %s: %w", clusterName, err) + return ctrl.Result{}, fmt.Errorf("list Machines for cluster %s: %w", clusterScope.Cluster.Name, err) } if len(machines.Items) > 0 { logf.FromContext(ctx).Info( diff --git a/controller/stackitmachine_controller.go b/controller/stackitmachine_controller.go index c4d9de0..d1aa1d1 100644 --- a/controller/stackitmachine_controller.go +++ b/controller/stackitmachine_controller.go @@ -18,7 +18,6 @@ package controller import ( "context" - "errors" "fmt" corev1 "k8s.io/api/core/v1" @@ -67,31 +66,31 @@ func (r *StackitMachineReconciler) Reconcile(ctx context.Context, req ctrl.Reque return ctrl.Result{}, err } - // The owning objects are resolved as far as they still exist. While the - // object is being deleted an owner that is simply gone is tolerated, so the - // checks that need one sit behind the deletion branch below rather than in - // front of it. Every other error still fails, so a transient API problem - // cannot be mistaken for a missing owner and drop the finalizer over a - // server that is still running. - deleting := !stackitMachine.DeletionTimestamp.IsZero() - - machine, ownerErr := clusterutil.GetOwnerMachine(ctx, r.Client, stackitMachine.ObjectMeta) - if ownerErr != nil && (!deleting || !ownerGone(ownerErr)) { - return ctrl.Result{}, fmt.Errorf("get owner machine: %w", ownerErr) + machine, err := clusterutil.GetOwnerMachine(ctx, r.Client, stackitMachine.ObjectMeta) + if err != nil { + return ctrl.Result{}, fmt.Errorf("get owner machine: %w", err) } - var cluster *clusterv1.Cluster - if machine != nil { - cluster, ownerErr = clusterutil.GetClusterFromMetadata(ctx, r.Client, machine.ObjectMeta) - if ownerErr != nil && (!deleting || !ownerGone(ownerErr)) { - return ctrl.Result{}, fmt.Errorf("get cluster from machine metadata: %w", ownerErr) - } + if machine == nil { + log.Info("StackitMachine has no owning Machine yet, requeueing") + return ctrl.Result{}, nil } - var stackitCluster *infrav1.StackitCluster - if cluster != nil { - stackitCluster, err = r.getStackitCluster(ctx, cluster) - if err != nil { - return ctrl.Result{}, err - } + + cluster, err := clusterutil.GetClusterFromMetadata(ctx, r.Client, machine.ObjectMeta) + if err != nil { + return ctrl.Result{}, fmt.Errorf("get cluster from machine metadata: %w", err) + } + if cluster == nil { + log.Info("Machine has no owning Cluster yet, requeueing") + return ctrl.Result{}, nil + } + + stackitCluster, err := r.getStackitCluster(ctx, cluster) + if err != nil { + return ctrl.Result{}, err + } + if stackitCluster == nil { + log.Info("StackitCluster not found, requeueing") + return ctrl.Result{}, nil } machineScope, err := scope.NewMachineScope(r.Client, cluster, machine, stackitCluster, stackitMachine) @@ -113,29 +112,9 @@ func (r *StackitMachineReconciler) Reconcile(ctx context.Context, req ctrl.Reque if !stackitMachine.DeletionTimestamp.IsZero() { return ctrl.Result{}, r.reconcileDelete(ctx, machineScope) } - - switch { - case machine == nil: - log.Info("StackitMachine has no owning Machine yet, waiting") - return ctrl.Result{}, nil - case cluster == nil: - log.Info("Machine has no owning Cluster yet, waiting") - return ctrl.Result{}, nil - case stackitCluster == nil: - log.Info("StackitCluster not found, waiting") - return ctrl.Result{}, nil - } return r.reconcileNormal(ctx, machineScope) } -// ownerGone reports whether err means the owning object no longer exists, as -// opposed to not being readable right now. GetOwnerMachine and GetOwnerCluster -// surface a plain NotFound; GetClusterFromMetadata reports ErrNoCluster when the -// Machine has lost the label naming its Cluster. -func ownerGone(err error) bool { - return apierrors.IsNotFound(err) || errors.Is(err, clusterutil.ErrNoCluster) -} - // SetupWithManager registers the controller with the manager. func (r *StackitMachineReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). diff --git a/controller/stackitmachine_controller_test.go b/controller/stackitmachine_controller_test.go index eb35ae6..860cf4b 100644 --- a/controller/stackitmachine_controller_test.go +++ b/controller/stackitmachine_controller_test.go @@ -448,44 +448,6 @@ var _ = Describe("StackitMachine Controller", func() { }).Should(BeTrue()) }) - // Every one of these owning objects used to be checked before the - // DeletionTimestamp branch and returned without a requeue, so a - // StackitMachine whose owner disappeared first — a namespace teardown - // deletes in no particular order — could never be deleted again. - DescribeTable("finalizes deletion when an owning object is already gone", - func(deleteOwner func()) { - updateMachineBootstrapSecret(ctx, machineName, bootstrapName) - createBootstrapSecret(ctx, bootstrapName) - _, err := reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - Expect(fakeCloud.ServerCount()).To(Equal(1)) - - deleteOwner() - - got := &infrav1.StackitMachine{} - Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) - Expect(k8sClient.Delete(ctx, got)).To(Succeed()) - - _, err = reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - Expect(fakeCloud.ServerCount()).To(Equal(1), - "without the owning objects there are no credentials and no tags, so the server cannot be reached") - Eventually(func() bool { - err := k8sClient.Get(ctx, stackitKey, &infrav1.StackitMachine{}) - return apierrors.IsNotFound(err) - }).Should(BeTrue()) - }, - Entry("owning Machine", func() { - deleteIfExists(ctx, &clusterv1.Machine{ObjectMeta: metav1.ObjectMeta{Name: machineName, Namespace: namespace}}) - }), - Entry("owning Cluster", func() { - deleteIfExists(ctx, &clusterv1.Cluster{ObjectMeta: metav1.ObjectMeta{Name: clusterName, Namespace: namespace}}) - }), - Entry("owning StackitCluster", func() { - deleteIfExists(ctx, &infrav1.StackitCluster{ObjectMeta: metav1.ObjectMeta{Name: clusterName, Namespace: namespace}}) - }), - ) - It("maps owning Machine events to StackitMachine reconcile requests", func() { machine := &clusterv1.Machine{} Expect(k8sClient.Get(ctx, types.NamespacedName{Name: machineName, Namespace: namespace}, machine)).To(Succeed()) diff --git a/controller/stackitmachine_infrastructure.go b/controller/stackitmachine_infrastructure.go index a6fc3e2..f355a7b 100644 --- a/controller/stackitmachine_infrastructure.go +++ b/controller/stackitmachine_infrastructure.go @@ -170,22 +170,6 @@ func validateMachineAvailabilityZone(machineScope *scope.MachineScope) error { func (r *StackitMachineReconciler) reconcileDelete(ctx context.Context, machineScope *scope.MachineScope) error { stackitMachine := machineScope.StackitMachine - // Without one of the owning objects the server can no longer be reached at - // all: the StackitCluster carries the credentials, the project and the - // region, and the Cluster and Machine names make up the tags that identify - // the server. Blocking here would leave the StackitMachine in Terminating - // forever, so finalize and make the possible leak loud instead — the same - // trade the missing-credentials path below makes. - if missing := missingOwner(machineScope); missing != "" { - if r.Recorder != nil { - r.Recorder.Eventf(stackitMachine, nil, corev1.EventTypeWarning, "CleanupSkipped", "Delete", - "%s is gone; finalizing without cloud cleanup. "+ - "Any remaining STACKIT server for this machine must be removed manually.", missing) - } - controllerutil.RemoveFinalizer(stackitMachine, infrav1.MachineFinalizer) - return nil - } - // The cloud client is built unconditionally: an empty status.instanceID is // not proof that no server exists, so every deletion has to ask the cloud // before it may drop the finalizer. @@ -236,20 +220,6 @@ func (r *StackitMachineReconciler) reconcileDelete(ctx context.Context, machineS return nil } -// missingOwner names the first owning object that no longer exists, or an empty -// string once all of them are present. -func missingOwner(machineScope *scope.MachineScope) string { - switch { - case machineScope.Machine == nil: - return "Owning Machine" - case machineScope.Cluster == nil: - return "Owning Cluster" - case machineScope.StackitCluster == nil: - return "Owning StackitCluster" - } - return "" -} - // resolveServerForDeletion reports the ID of the server backing this machine, or // an empty string once the cloud confirms none exists. // From 087fb45b084a3ba1fdab02c2d8dad13d65998bd4 Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Thu, 20 Aug 2026 13:53:55 +0000 Subject: [PATCH 13/15] Revert fix: keep Secret data out of the manager's informer cache --- cmd/manager/main.go | 46 -------------------- cmd/manager/main_test.go | 90 ---------------------------------------- 2 files changed, 136 deletions(-) delete mode 100644 cmd/manager/main_test.go diff --git a/cmd/manager/main.go b/cmd/manager/main.go index 46ecfd0..ca4905e 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -25,14 +25,11 @@ import ( // to ensure that exec-entrypoint and run can make use of them. _ "k8s.io/client-go/plugin/pkg/client/auth" - corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" clientgoscheme "k8s.io/client-go/kubernetes/scheme" clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/cache" - "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/healthz" "sigs.k8s.io/controller-runtime/pkg/log/zap" "sigs.k8s.io/controller-runtime/pkg/metrics/filters" @@ -59,47 +56,6 @@ func init() { // +kubebuilder:scaffold:scheme } -// managerCacheOptions keeps Secret data out of the shared informer cache. Both -// controllers watch Secrets — credentials, bootstrap data and the bastion -// cloud-init — and a watch only needs an object's identity to enqueue a -// reconcile, never its contents, so caching credential bytes in memory buys -// nothing and exposes them to anything that can read the process. Managed -// fields go with them: nothing here reads them, and they are usually the -// largest part of what is left once the data is gone. -// -// Deliberately no label selector on the entry, unlike Cluster API's own -// equivalent in internal/setup. Cluster API only watches Secrets it stamps -// itself, whereas the credentials and bastion cloud-init Secrets here are -// created by the user and carry no labels, so a selector would silently stop -// those watches from firing rather than only hardening the cache. -func managerCacheOptions() cache.Options { - return cache.Options{ - ByObject: map[client.Object]cache.ByObject{ - &corev1.Secret{}: { - Transform: func(in any) (any, error) { - if secret, ok := in.(*corev1.Secret); ok { - secret.Data = nil - secret.SetManagedFields(nil) - } - return in, nil - }, - }, - }, - } -} - -// managerClientOptions turns every Secret read into a live lookup. The cache no -// longer holds Secret data (see managerCacheOptions), so the reads that need -// the real bytes — util.BuildCloudClient and the bootstrap-data fetch — have to -// bypass it. -func managerClientOptions() client.Options { - return client.Options{ - Cache: &client.CacheOptions{ - DisableFor: []client.Object{&corev1.Secret{}}, - }, - } -} - // nolint:gocyclo func main() { var metricsAddr string @@ -204,8 +160,6 @@ func main() { mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{ Scheme: scheme, - Cache: managerCacheOptions(), - Client: managerClientOptions(), Metrics: metricsServerOptions, WebhookServer: webhookServer, HealthProbeBindAddress: probeAddr, diff --git a/cmd/manager/main_test.go b/cmd/manager/main_test.go deleted file mode 100644 index 64b2692..0000000 --- a/cmd/manager/main_test.go +++ /dev/null @@ -1,90 +0,0 @@ -/* -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 main - -import ( - "testing" - - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/cache" -) - -// secretCacheConfig returns the cache configuration registered for Secrets. -// ByObject is keyed by a sample object rather than by type, so the entry has to -// be found by the key's type — a freshly allocated &corev1.Secret{} is a -// different pointer and would never match as a map key. -func secretCacheConfig(t *testing.T) cache.ByObject { - t.Helper() - - for obj, byObject := range managerCacheOptions().ByObject { - if _, ok := obj.(*corev1.Secret); ok { - return byObject - } - } - t.Fatal("no cache configuration registered for Secrets") - return cache.ByObject{} -} - -func TestManagerCacheOptionsStripsSecretData(t *testing.T) { - transform := secretCacheConfig(t).Transform - if transform == nil { - t.Fatal("Secret cache configuration has no Transform") - } - - out, err := transform(&corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - ManagedFields: []metav1.ManagedFieldsEntry{{Manager: "kubectl", Operation: metav1.ManagedFieldsOperationUpdate}}, - }, - Data: map[string][]byte{"credentials": []byte("service-account-key")}, - }) - if err != nil { - t.Fatalf("Transform returned an error: %v", err) - } - secret, ok := out.(*corev1.Secret) - if !ok { - t.Fatalf("Transform returned %T, want *corev1.Secret", out) - } - if secret.Data != nil { - t.Errorf("Transform left Secret data in the cache: %v", secret.Data) - } - if secret.ManagedFields != nil { - t.Errorf("Transform left managed fields in the cache: %v", secret.ManagedFields) - } -} - -// The Transform runs on everything committed to the Secret informer, including -// the tombstones the cache emits on deletion, so it has to pass anything that -// is not a Secret straight through instead of dropping it. -func TestManagerCacheOptionsPassesThroughNonSecrets(t *testing.T) { - in := &corev1.ConfigMap{Data: map[string]string{"key": "value"}} - - out, err := secretCacheConfig(t).Transform(in) - if err != nil { - t.Fatalf("Transform returned an error: %v", err) - } - if out != any(in) { - t.Errorf("Transform returned %v, want the input unchanged", out) - } -} - -func TestManagerClientOptionsDisablesSecretCache(t *testing.T) { - cacheOptions := managerClientOptions().Cache - if cacheOptions == nil { - t.Fatal("client options have no cache configuration") - } - for _, obj := range cacheOptions.DisableFor { - if _, ok := obj.(*corev1.Secret); ok { - return - } - } - t.Errorf("Secrets are not in DisableFor: %v", cacheOptions.DisableFor) -} From b9df8992b2065718c43e62e36e67c6968f2b5189 Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Thu, 20 Aug 2026 14:00:46 +0000 Subject: [PATCH 14/15] fix: requeue invalid credentials instead of watching the Secret --- controller/constants.go | 8 ++- controller/stackitcluster_controller.go | 39 ------------ controller/stackitcluster_controller_test.go | 62 ++++++-------------- controller/stackitcluster_infrastructure.go | 1 + controller/stackitmachine_controller_test.go | 16 ++++- controller/stackitmachine_infrastructure.go | 2 + util/conditions.go | 8 ++- 7 files changed, 46 insertions(+), 90 deletions(-) diff --git a/controller/constants.go b/controller/constants.go index cf19128..cc4e652 100644 --- a/controller/constants.go +++ b/controller/constants.go @@ -19,8 +19,10 @@ const ( retryableErrorRequeueAfter = 5 * time.Second - // deleteRequeueAfter paces the wait for dependent objects to disappear - // during deletion. Matches what Cluster API and the other infrastructure - // providers use for the same purpose. + // deleteRequeueAfter paces the wait for dependent objects to disappear during deletion. deleteRequeueAfter = 5 * time.Second + + // credentialsRetryRequeueAfter paces retries of invalid credentials, which + // need an operator to fix the Secret before they can succeed. + credentialsRetryRequeueAfter = time.Minute ) diff --git a/controller/stackitcluster_controller.go b/controller/stackitcluster_controller.go index f5fc9a8..a7eaa81 100644 --- a/controller/stackitcluster_controller.go +++ b/controller/stackitcluster_controller.go @@ -150,44 +150,6 @@ func (r *StackitClusterReconciler) stackitClusterRequestsForCloudInitRef(ctx con return requests } -// stackitClusterRequestsForCredentialsSecret enqueues every StackitCluster whose -// credentialsSecretRef points at the given Secret. -// -// Without it, correcting an invalid credentials Secret never reaches the -// cluster: CredentialFailureResult deliberately returns without a requeue, -// because retrying invalid credentials in a hot loop helps nobody — which only -// works if fixing them triggers a reconcile. -func (r *StackitClusterReconciler) stackitClusterRequestsForCredentialsSecret(ctx context.Context, obj client.Object) []reconcile.Request { - secret, ok := obj.(*corev1.Secret) - if !ok { - return nil - } - - // Listed across all namespaces on purpose: CredentialsSecretRef.Namespace is - // optional, so the Secret may well live somewhere other than the - // StackitCluster that references it. - clusters := &infrav1.StackitClusterList{} - if err := r.List(ctx, clusters); err != nil { - logf.FromContext(ctx).Error(err, "Failed to list StackitClusters for credentials Secret watch", "secret", client.ObjectKeyFromObject(secret)) - return nil - } - - secretKey := client.ObjectKeyFromObject(secret) - requests := make([]reconcile.Request, 0, len(clusters.Items)) - for _, cluster := range clusters.Items { - if util.CredentialsSecretKey(&cluster) != secretKey { - continue - } - requests = append(requests, reconcile.Request{ - NamespacedName: types.NamespacedName{ - Namespace: cluster.Namespace, - Name: cluster.Name, - }, - }) - } - return requests -} - // SetupWithManager registers the controller with the manager. func (r *StackitClusterReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). @@ -195,7 +157,6 @@ func (r *StackitClusterReconciler) SetupWithManager(mgr ctrl.Manager) error { Watches(&clusterv1.Cluster{}, handler.EnqueueRequestsFromMapFunc(r.stackitClusterRequestsForCluster)). Watches(&corev1.ConfigMap{}, handler.EnqueueRequestsFromMapFunc(r.stackitClusterRequestsForCloudInitRef)). Watches(&corev1.Secret{}, handler.EnqueueRequestsFromMapFunc(r.stackitClusterRequestsForCloudInitRef)). - Watches(&corev1.Secret{}, handler.EnqueueRequestsFromMapFunc(r.stackitClusterRequestsForCredentialsSecret)). Named("stackitcluster"). Complete(r) } diff --git a/controller/stackitcluster_controller_test.go b/controller/stackitcluster_controller_test.go index 11501f9..e115d44 100644 --- a/controller/stackitcluster_controller_test.go +++ b/controller/stackitcluster_controller_test.go @@ -22,7 +22,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" - "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/reconcile" infrav1 "github.com/stackitcloud/cluster-api-provider-stackit/api/v1alpha1" @@ -500,19 +499,34 @@ var _ = Describe("StackitCluster Controller", func() { expectCondition(got.Status.Conditions, infrav1.ClusterReadyCondition, metav1.ConditionFalse, "NetworkNotFound") }) - It("marks credentials invalid without requeueing on unauthorized credentials", func() { + // The requeue is what lets a corrected Secret take effect: nothing else + // enqueues the cluster once the credentials are rejected. + It("requeues on unauthorized credentials and recovers once they are corrected", func() { reconciler.CloudClientFactory = func(context.Context, cloud.Credentials) (cloud.Client, error) { return nil, fmt.Errorf("authenticate: %w", cloud.ErrUnauthorized) } result, err := reconciler.Reconcile(ctx, request) Expect(err).NotTo(HaveOccurred()) - Expect(result).To(Equal(reconcile.Result{})) + Expect(result.RequeueAfter).To(Equal(credentialsRetryRequeueAfter)) got := &infrav1.StackitCluster{} Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + Expect(got.Status.Ready).To(BeFalse()) expectCondition(got.Status.Conditions, infrav1.ClusterCredentialsReadyCondition, metav1.ConditionFalse, "CredentialsInvalid") expectCondition(got.Status.Conditions, infrav1.ClusterReadyCondition, metav1.ConditionFalse, "CredentialsInvalid") + + By("correcting the credentials") + reconciler.CloudClientFactory = func(context.Context, cloud.Credentials) (cloud.Client, error) { + return fakeCloud, nil + } + + _, err = reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + Expect(got.Status.Ready).To(BeTrue()) + expectCondition(got.Status.Conditions, infrav1.ClusterCredentialsReadyCondition, metav1.ConditionTrue, "Available") }) It("does not call the cloud API when the owning Cluster is paused", func() { @@ -723,48 +737,6 @@ var _ = Describe("StackitCluster Controller", func() { Expect(requests).To(Equal([]reconcile.Request{request})) }) - It("maps credentials Secret events to StackitCluster reconcile requests", func() { - secret := &corev1.Secret{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Name: credentials, Namespace: namespace}, secret)).To(Succeed()) - - requests := reconciler.stackitClusterRequestsForCredentialsSecret(ctx, secret) - Expect(requests).To(Equal([]reconcile.Request{request})) - }) - - It("maps credentials Secret events from a different namespace than the StackitCluster", func() { - otherNamespace := "credentials-elsewhere" - Expect(client.IgnoreAlreadyExists(k8sClient.Create(ctx, &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{Name: otherNamespace}, - }))).To(Succeed()) - - otherCredentials := credentials + "-elsewhere" - createCredentialsSecret(ctx, otherCredentials, otherNamespace, testProjectID) - DeferCleanup(func() { - deleteIfExists(ctx, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: otherCredentials, Namespace: otherNamespace}}) - }) - - got := &infrav1.StackitCluster{} - Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) - got.Spec.CredentialsSecretRef = corev1.SecretReference{Name: otherCredentials, Namespace: otherNamespace} - Expect(k8sClient.Update(ctx, got)).To(Succeed()) - - secret := &corev1.Secret{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Name: otherCredentials, Namespace: otherNamespace}, secret)).To(Succeed()) - - requests := reconciler.stackitClusterRequestsForCredentialsSecret(ctx, secret) - Expect(requests).To(Equal([]reconcile.Request{request}), - "credentialsSecretRef.namespace is optional, so the mapper must not be limited to the Secret's own namespace") - }) - - It("ignores Secret events that no StackitCluster uses as credentials", func() { - secret := &corev1.Secret{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Name: credentials, Namespace: namespace}, secret)).To(Succeed()) - secret.Name = credentials + "-unused" - - requests := reconciler.stackitClusterRequestsForCredentialsSecret(ctx, secret) - Expect(requests).To(BeEmpty()) - }) - It("ignores Cluster events for other infrastructure providers", func() { cluster := &clusterv1.Cluster{ ObjectMeta: metav1.ObjectMeta{Name: "other", Namespace: namespace}, diff --git a/controller/stackitcluster_infrastructure.go b/controller/stackitcluster_infrastructure.go index 8aebb98..66f7284 100644 --- a/controller/stackitcluster_infrastructure.go +++ b/controller/stackitcluster_infrastructure.go @@ -57,6 +57,7 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterS &stackitCluster.Status.Conditions, stackitCluster.Generation, err, + credentialsRetryRequeueAfter, infrav1.ClusterCredentialsReadyCondition, infrav1.ClusterReadyCondition, ) diff --git a/controller/stackitmachine_controller_test.go b/controller/stackitmachine_controller_test.go index 860cf4b..4b0dee3 100644 --- a/controller/stackitmachine_controller_test.go +++ b/controller/stackitmachine_controller_test.go @@ -220,7 +220,7 @@ var _ = Describe("StackitMachine Controller", func() { expectCondition(got.Status.Conditions, infrav1.MachineReadyCondition, metav1.ConditionFalse, "InvalidFailureDomain") }) - It("marks credentials invalid without requeueing on unauthorized credentials", func() { + It("requeues on unauthorized credentials and recovers once they are corrected", func() { updateMachineBootstrapSecret(ctx, machineName, bootstrapName) createBootstrapSecret(ctx, bootstrapName) reconciler.CloudClientFactory = func(context.Context, cloud.Credentials) (cloud.Client, error) { @@ -229,13 +229,25 @@ var _ = Describe("StackitMachine Controller", func() { result, err := reconciler.Reconcile(ctx, request) Expect(err).NotTo(HaveOccurred()) - Expect(result).To(Equal(reconcile.Result{})) + Expect(result.RequeueAfter).To(Equal(credentialsRetryRequeueAfter)) Expect(fakeCloud.ServerCount()).To(Equal(0)) got := &infrav1.StackitMachine{} Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) expectCondition(got.Status.Conditions, infrav1.MachineCredentialsReadyCondition, metav1.ConditionFalse, "CredentialsInvalid") expectCondition(got.Status.Conditions, infrav1.MachineReadyCondition, metav1.ConditionFalse, "CredentialsInvalid") + + By("correcting the credentials") + reconciler.CloudClientFactory = func(context.Context, cloud.Credentials) (cloud.Client, error) { + return fakeCloud, nil + } + + _, err = reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(fakeCloud.ServerCount()).To(Equal(1)) + + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + expectCondition(got.Status.Conditions, infrav1.MachineCredentialsReadyCondition, metav1.ConditionTrue, "Available") }) It("does not call the cloud API when the owning Cluster is paused", func() { diff --git a/controller/stackitmachine_infrastructure.go b/controller/stackitmachine_infrastructure.go index f355a7b..711017c 100644 --- a/controller/stackitmachine_infrastructure.go +++ b/controller/stackitmachine_infrastructure.go @@ -78,6 +78,7 @@ func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, machineS &stackitMachine.Status.Conditions, stackitMachine.Generation, err, + credentialsRetryRequeueAfter, infrav1.MachineCredentialsReadyCondition, infrav1.MachineReadyCondition, ) @@ -193,6 +194,7 @@ func (r *StackitMachineReconciler) reconcileDelete(ctx context.Context, machineS &stackitMachine.Status.Conditions, stackitMachine.Generation, err, + credentialsRetryRequeueAfter, infrav1.MachineCredentialsReadyCondition, ) return resultErr diff --git a/util/conditions.go b/util/conditions.go index 858e4f4..f60b2bf 100644 --- a/util/conditions.go +++ b/util/conditions.go @@ -81,15 +81,21 @@ func SetConditions( } } +// CredentialFailureResult records the failure and decides how to retry. +// +// Invalid credentials need an operator to fix the Secret, so they requeue on a +// slow timer rather than returning an error and spinning through the controller +// backoff. The reconcile reads the Secret again, which is what picks up the fix. func CredentialFailureResult( conditions *[]metav1.Condition, generation int64, err error, + requeueAfter time.Duration, conditionTypes ...string, ) (ctrl.Result, error) { SetConditions(conditions, generation, metav1.ConditionFalse, "CredentialsInvalid", err.Error(), conditionTypes...) if cloud.IsUnauthorized(err) || cloud.IsInvalidInput(err) || errors.Is(err, ErrCredentialsInvalid) { - return ctrl.Result{}, nil + return ctrl.Result{RequeueAfter: requeueAfter}, nil } return ctrl.Result{}, err } From 6fab208d66f91ce96e3e2088c10cf2557c9982c9 Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Fri, 21 Aug 2026 07:44:29 +0000 Subject: [PATCH 15/15] chore: trim comments to describe the current state --- cloud/fake/client.go | 8 ++-- controller/controller_test_helpers_test.go | 6 +-- controller/stackitcluster_bastion.go | 9 ++-- controller/stackitcluster_controller_test.go | 47 +++++++------------- controller/stackitcluster_infrastructure.go | 27 +++++------ controller/stackitmachine_controller_test.go | 31 +++++-------- controller/stackitmachine_infrastructure.go | 38 ++++++---------- controller/stackitmachine_watches.go | 4 +- 8 files changed, 57 insertions(+), 113 deletions(-) diff --git a/cloud/fake/client.go b/cloud/fake/client.go index fd843d7..0642e49 100644 --- a/cloud/fake/client.go +++ b/cloud/fake/client.go @@ -55,11 +55,9 @@ type Client struct { FailNextEnsureNodeSSH error FailNextDeleteNodeSSH error - // Before* hooks, if non-nil, run before the call they belong to does any - // work. They let a test observe API server state at the exact moment a cloud - // call is about to happen — for instance to assert a finalizer was persisted - // before the first resource could be created. Unlike FailNext*, they are not - // consumed and fire on every call. + // Before* hooks, if non-nil, run before the call they belong to. They let a + // test observe API server state at the exact moment a cloud call would + // happen. Unlike FailNext*, they are not consumed. BeforeCreateServer func() BeforeGetNetwork func() diff --git a/controller/controller_test_helpers_test.go b/controller/controller_test_helpers_test.go index 110e266..c861246 100644 --- a/controller/controller_test_helpers_test.go +++ b/controller/controller_test_helpers_test.go @@ -87,10 +87,7 @@ func createOwnerCluster(ctx context.Context, name string) { Expect(k8sClient.Create(ctx, cluster)).To(Succeed()) } -// Machines are created without bootstrap data; the specs that need it attach -// it afterwards via updateMachineBootstrapSecret. func createOwnerMachine(ctx context.Context, name, clusterName, stackitMachineName string) { - bootstrapSecretName := new("") machine := &clusterv1.Machine{ ObjectMeta: metav1.ObjectMeta{ Name: name, @@ -102,7 +99,8 @@ func createOwnerMachine(ctx context.Context, name, clusterName, stackitMachineNa Spec: clusterv1.MachineSpec{ ClusterName: clusterName, Bootstrap: clusterv1.Bootstrap{ - DataSecretName: bootstrapSecretName, + // Specs that need bootstrap data attach it with updateMachineBootstrapSecret. + DataSecretName: new(""), }, InfrastructureRef: clusterv1.ContractVersionedObjectReference{ APIGroup: infrav1.GroupVersion.Group, diff --git a/controller/stackitcluster_bastion.go b/controller/stackitcluster_bastion.go index 57807d5..3a3927d 100644 --- a/controller/stackitcluster_bastion.go +++ b/controller/stackitcluster_bastion.go @@ -49,12 +49,9 @@ func (r *StackitClusterReconciler) reconcileBastion( } if !stackitCluster.Spec.Bastion.Enabled { - // The condition carries this reason only after a cleanup has succeeded, - // so anything else means we may still own bastion resources — including - // the case where EnsureBastion succeeded but its status patch was lost. - // Keying on it instead of on the status keeps the tag-based cleanup to - // once per cluster rather than once per reconcile, which matters because - // this path runs for every cluster without a bastion. + // The reason is set only after a cleanup has succeeded, so anything else + // means bastion resources may still exist. Keying on it rather than on + // the status keeps the tag-based cleanup to once per cluster. condition := meta.FindStatusCondition(stackitCluster.Status.Conditions, infrav1.ClusterBastionReadyCondition) if condition == nil || condition.Reason != bastionDisabledReason { if err := cloudClient.DeleteNodeSSHAccess(ctx, bastionservice.NodeSSHAccessTags(stackitCluster)); err != nil { diff --git a/controller/stackitcluster_controller_test.go b/controller/stackitcluster_controller_test.go index e115d44..4d391ad 100644 --- a/controller/stackitcluster_controller_test.go +++ b/controller/stackitcluster_controller_test.go @@ -203,11 +203,8 @@ var _ = Describe("StackitCluster Controller", func() { }) It("cleans up bastion resources during deletion even when bastion status was never persisted", func() { - // The cloud-cleanup block used to be gated on persisted status for the - // bastion, while the load balancer was gated on its spec flag. A bastion - // created without its status patch landing (process restart, conflict) - // therefore skipped cleanup entirely and leaked server, public IP and - // security group. + // A bastion whose status patch never landed must still be cleaned up, so + // cleanup follows the spec flag rather than persisted status. createOwnerCluster(ctx, clusterName+"-nolb") defer deleteIfExists(ctx, &clusterv1.Cluster{ ObjectMeta: metav1.ObjectMeta{Name: clusterName + "-nolb", Namespace: namespace}, @@ -247,10 +244,8 @@ var _ = Describe("StackitCluster Controller", func() { }) It("finalizes deletion when the credentials Secret is already gone", func() { - // A missing credentials Secret cannot be recovered from, and it commonly - // disappears first during namespace teardown. Broadening the delete gate - // to spec.Bastion.Enabled made a working cloud client mandatory for every - // bastion cluster, which would strand such a cluster in Terminating. + // The Secret commonly disappears first during namespace teardown, and + // without it no cloud client can be built at all. createOwnerCluster(ctx, clusterName+"-nocreds") defer deleteIfExists(ctx, &clusterv1.Cluster{ ObjectMeta: metav1.ObjectMeta{Name: clusterName + "-nocreds", Namespace: namespace}, @@ -284,10 +279,8 @@ var _ = Describe("StackitCluster Controller", func() { }) It("tears the bastion down when disabled even if its status was never persisted", func() { - // Counterpart to the deletion path: disabling the bastion used to be - // gated on hasBastionStatus alone. With the status lost, nothing was torn - // down while the condition reported "bastion disabled" — leaving port 22 - // open for the rest of the cluster's life. + // With the status lost, a status-gated teardown would report the bastion + // as disabled while leaving port 22 open. got := &infrav1.StackitCluster{} Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) got.Spec.Bastion = validBastionSpec() @@ -316,8 +309,7 @@ var _ = Describe("StackitCluster Controller", func() { It("tears the bastion down when disabled even if only its status was lost", func() { // Narrower than the case above: the condition survives and still reports - // the bastion as available, only the bastion status fields are gone. - // Gating the cleanup on hasBastionStatus left the server running here. + // the bastion as available, only the status fields are gone. got := &infrav1.StackitCluster{} Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) got.Spec.Bastion = validBastionSpec() @@ -345,10 +337,8 @@ var _ = Describe("StackitCluster Controller", func() { }) It("cleans up the load balancer during deletion when it was disabled and its status was lost", func() { - // Counterpart to the bastion case: flipping apiServerLoadBalancer.enabled - // off neither deletes the load balancer nor clears its ID, so with the - // status patch lost the deletion gate matched nothing and the load - // balancer stayed behind. + // Flipping apiServerLoadBalancer.enabled off neither deletes the load + // balancer nor clears its ID, so deletion cannot rely on either. _, err := reconciler.Reconcile(ctx, request) Expect(err).NotTo(HaveOccurred()) Expect(fakeCloud.LoadBalancerCount()).To(Equal(1)) @@ -595,12 +585,8 @@ var _ = Describe("StackitCluster Controller", func() { }).Should(BeTrue()) }) - // AddFinalizer only mutated the object in memory, and the write to etcd - // happened in the deferred PatchObject at the end of Reconcile — after the - // load balancer and the bastion had been created. A process dying in between - // left those resources behind an object with no finalizer to clean them up. - // GetNetwork is the first cloud call of the reconcile, so a hook there - // proves the ordering rather than only the end result. + // GetNetwork is the first cloud call of the reconcile, so a hook there proves + // the ordering rather than only the end result. It("persists the finalizer before the first cloud call", func() { var finalizersAtFirstCall []string fakeCloud.BeforeGetNetwork = func() { @@ -617,13 +603,10 @@ var _ = Describe("StackitCluster Controller", func() { "cloud resources were created while the API server had no finalizer to clean them up") }) - // The finalizer used to go away regardless of remaining Machines. Their - // controllers reach credentials and project context through this - // StackitCluster, so once it is gone they can neither delete their servers - // nor drop their own finalizers — the VMs are orphaned and the Machines - // hang. Cluster API orders this correctly when the deletion starts at the - // Cluster, but a namespace teardown or a direct delete of this object - // bypasses that ordering. + // Machine controllers reach credentials and project context through this + // StackitCluster, so it has to outlive them. Cluster API orders this + // correctly when deletion starts at the Cluster, but a namespace teardown or + // a direct delete of this object bypasses that ordering. It("keeps the finalizer while Machines still exist for the cluster", func() { _, err := reconciler.Reconcile(ctx, request) Expect(err).NotTo(HaveOccurred()) diff --git a/controller/stackitcluster_infrastructure.go b/controller/stackitcluster_infrastructure.go index 66f7284..5e0c56d 100644 --- a/controller/stackitcluster_infrastructure.go +++ b/controller/stackitcluster_infrastructure.go @@ -39,11 +39,9 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterS if !controllerutil.ContainsFinalizer(stackitCluster, infrav1.ClusterFinalizer) { controllerutil.AddFinalizer(stackitCluster, infrav1.ClusterFinalizer) - // Persisted immediately, before anything can create a cloud resource. - // AddFinalizer only mutates the object in memory; it otherwise reaches - // etcd through the deferred PatchObject at the end of Reconcile, and a - // process that dies in between leaves a load balancer or a bastion - // running behind an object that carries no finalizer to clean it up. + // Persisted immediately, before any cloud resource can be created, to + // ensure nothing is running behind an object that carries no finalizer to + // clean it up. if err := clusterScope.PatchObject(ctx); err != nil { return ctrl.Result{}, fmt.Errorf("persist finalizer: %w", err) } @@ -255,20 +253,15 @@ func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, clusterS return ctrl.Result{RequeueAfter: deleteRequeueAfter}, nil } - // Cleanup runs unconditionally. Neither spec nor status is a trustworthy - // record of what exists in the cloud: a resource can be created before its - // status patch lands, and disabling the load balancer or the bastion leaves - // the running resource behind. ResolveID, DeleteBastion and - // DeleteNodeSSHAccess all fall back to tag lookups and tolerate NotFound, so - // asking for everything costs a handful of list calls once per cluster and - // removes every combination in which a resource could be missed. + // Cleanup runs unconditionally: neither spec nor status is a trustworthy + // record of what exists in the cloud. ResolveID, DeleteBastion and + // DeleteNodeSSHAccess fall back to tag lookups and tolerate NotFound. cloudClient, err := util.BuildCloudClient(ctx, r.Client, r.CloudClientFactory, stackitCluster) if err != nil { - // A missing credentials Secret can never be recovered from — it commonly - // disappears first during namespace teardown. Blocking here would strand - // the cluster in Terminating forever, so finalize and make the possible - // leak loud instead. Any other credentials problem is fixable, so keep - // retrying for those. + // A missing credentials Secret cannot be recovered from and commonly + // disappears first during namespace teardown, so finalize and make the + // possible leak loud rather than stranding the cluster in Terminating. + // Every other credentials problem is fixable and keeps retrying. if apierrors.IsNotFound(err) { if r.Recorder != nil { r.Recorder.Eventf(stackitCluster, nil, corev1.EventTypeWarning, "CleanupSkipped", "Delete", diff --git a/controller/stackitmachine_controller_test.go b/controller/stackitmachine_controller_test.go index 4b0dee3..0dfb3e2 100644 --- a/controller/stackitmachine_controller_test.go +++ b/controller/stackitmachine_controller_test.go @@ -132,12 +132,9 @@ var _ = Describe("StackitMachine Controller", func() { }) It("does not silently recreate the server of an already-provisioned machine", func() { - // When the backing server disappears out-of-band, ensureServer used to - // call CreateServer again, replaying the original bootstrap data — which - // is pinned to the previous identity. The replacement either never - // rejoins (different IP) or rejoins while Machine and Node keep pointing - // at the deleted server (same IP) — neither restores the cluster, and - // both consume another VM unnoticed. + // Recreating it would replay bootstrap data pinned to the previous + // identity: the replacement either never rejoins or rejoins while Machine + // and Node still point at the deleted server. updateMachineBootstrapSecret(ctx, machineName, bootstrapName) createBootstrapSecret(ctx, bootstrapName) @@ -398,12 +395,8 @@ var _ = Describe("StackitMachine Controller", func() { }).Should(BeTrue()) }) - // AddFinalizer only mutated the object in memory, and the write to etcd - // happened in the deferred PatchObject at the end of Reconcile — after - // CreateServer. A process dying in between left a running server behind an - // object with no finalizer to clean it up. The hook observes API server - // state from inside the cloud call, so it proves the ordering rather than - // only the end result. + // The hook observes API server state from inside the cloud call, so it proves + // the ordering rather than only the end result. It("persists the finalizer before creating the server", func() { updateMachineBootstrapSecret(ctx, machineName, bootstrapName) createBootstrapSecret(ctx, bootstrapName) @@ -423,10 +416,8 @@ var _ = Describe("StackitMachine Controller", func() { "the server was created while the API server had no finalizer to clean it up") }) - // An empty status.instanceID used to be taken as proof that no VM had ever - // been created, so the finalizer went away without a single cloud call. If - // CreateServer had succeeded and the status patch had not, that server kept - // running, tagged and unreferenced by any object. + // A lost status patch leaves a running, tagged server that no field on the + // object names, so an empty status.instanceID is not proof that none exists. It("deletes a tagged server whose instance ID was lost from the status", func() { updateMachineBootstrapSecret(ctx, machineName, bootstrapName) createBootstrapSecret(ctx, bootstrapName) @@ -476,11 +467,9 @@ var _ = Describe("StackitMachine Controller", func() { Expect(requests).To(ConsistOf(request)) }) - // The mapper used to match Machine.spec.clusterName against the - // StackitCluster name, so it enqueued nothing as soon as the two differed. - // Every other spec here hides the bug because createOwnerCluster gives the - // Cluster and its infrastructureRef the same name; a ClusterClass-generated - // infrastructureRef never does. + // Machine.spec.clusterName names the Cluster, not the StackitCluster, and a + // ClusterClass-generated infrastructureRef never shares its Cluster's name. + // Every other spec here uses matching names and would miss that. It("maps StackitCluster events when the StackitCluster name differs from the Cluster name", func() { suffix := time.Now().UnixNano() ownerClusterName := fmt.Sprintf("owner-%d", suffix) diff --git a/controller/stackitmachine_infrastructure.go b/controller/stackitmachine_infrastructure.go index 711017c..c549df3 100644 --- a/controller/stackitmachine_infrastructure.go +++ b/controller/stackitmachine_infrastructure.go @@ -38,11 +38,8 @@ func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, machineS if !controllerutil.ContainsFinalizer(stackitMachine, infrav1.MachineFinalizer) { controllerutil.AddFinalizer(stackitMachine, infrav1.MachineFinalizer) - // Persisted immediately, before CreateServer can run. AddFinalizer only - // mutates the object in memory; it otherwise reaches etcd through the - // deferred PatchObject at the end of Reconcile, and a process that dies - // in between leaves a server running behind an object that carries no - // finalizer to clean it up. + // Persisted immediately, before CreateServer can run, to ensure no servers + // are running behind an object that carries no finalizer to clean it up. if err := machineScope.PatchObject(ctx); err != nil { return ctrl.Result{}, fmt.Errorf("persist finalizer: %w", err) } @@ -171,16 +168,14 @@ func validateMachineAvailabilityZone(machineScope *scope.MachineScope) error { func (r *StackitMachineReconciler) reconcileDelete(ctx context.Context, machineScope *scope.MachineScope) error { stackitMachine := machineScope.StackitMachine - // The cloud client is built unconditionally: an empty status.instanceID is - // not proof that no server exists, so every deletion has to ask the cloud - // before it may drop the finalizer. + // An empty status.instanceID is not proof that no server exists, so every + // deletion asks the cloud before dropping the finalizer. cloudClient, err := util.BuildCloudClient(ctx, r.Client, r.CloudClientFactory, machineScope.StackitCluster) if err != nil { - // A missing credentials Secret can never be recovered from, and it - // commonly disappears first during namespace teardown. Blocking here - // would strand the Machine in Terminating forever, so finalize and make - // the possible leak loud instead — the same trade the cluster side - // makes. Any other credentials problem is fixable, so keep retrying. + // A missing credentials Secret cannot be recovered from and commonly + // disappears first during namespace teardown, so finalize and make the + // possible leak loud rather than stranding the Machine in Terminating. + // Every other credentials problem is fixable and keeps retrying. if apierrors.IsNotFound(err) { if r.Recorder != nil { r.Recorder.Eventf(stackitMachine, nil, corev1.EventTypeWarning, "CleanupSkipped", "Delete", @@ -223,11 +218,9 @@ func (r *StackitMachineReconciler) reconcileDelete(ctx context.Context, machineS } // resolveServerForDeletion reports the ID of the server backing this machine, or -// an empty string once the cloud confirms none exists. -// -// status.instanceID alone is not enough: CreateServer can succeed and the status -// patch can be lost, leaving a tagged server that no field on the object refers -// to. The tags carry the Machine UID and therefore still identify that server. +// an empty string once the cloud confirms none exists. It falls back to the tags, +// which carry the Machine UID, because a lost status patch can leave a running +// server that status.instanceID no longer names. func (r *StackitMachineReconciler) resolveServerForDeletion( ctx context.Context, cloudClient cloud.Client, @@ -288,13 +281,8 @@ func (r *StackitMachineReconciler) ensureServer( return nil, false, err } - // The machine had already been provisioned and its server has since - // disappeared. Recreating it here would replay the original bootstrap data, - // which is pinned to the previous identity: the replacement either never - // rejoins (different IP) or rejoins while Machine/Node keep pointing at the - // deleted server (same IP). Neither restores the cluster, and both consume - // another VM silently. Surface it instead and let Cluster API decide to - // replace the Machine. + // Recreating the server would replay bootstrap data pinned to the previous + // identity, so surface the loss and let Cluster API replace the Machine. if stackitMachine.Status.Initialization.Provisioned { return nil, false, fmt.Errorf( "%w: server %s for already-provisioned machine no longer exists; the Machine must be replaced", diff --git a/controller/stackitmachine_watches.go b/controller/stackitmachine_watches.go index 9fa4b0c..8853c7a 100644 --- a/controller/stackitmachine_watches.go +++ b/controller/stackitmachine_watches.go @@ -40,9 +40,7 @@ func (r *StackitMachineReconciler) stackitMachineRequestsForStackitCluster(ctx c } // Machine.spec.clusterName names the owning Cluster, not the StackitCluster, - // and nothing forces the two to share a name — a ClusterClass-generated - // infrastructureRef carries a random suffix. Resolve the owning Cluster - // first, then match against its name. + // and a ClusterClass-generated infrastructureRef never shares its name. cluster, err := clusterutil.GetOwnerCluster(ctx, r.Client, stackitCluster.ObjectMeta) switch { case apierrors.IsNotFound(err) || cluster == nil: