Revert fix: let deletion proceed when an owning object is already gone

Author: Herbaert

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
 	}
 

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

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(

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

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

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.
 //