chore: trim comments to describe the current state

Author: Herbaert

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()
 

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,

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 {

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())

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",

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)

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",

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: