fix: persist the finalizer before the first cloud call

Author: Herbaert

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
 	}

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

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)
 

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

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 {