Repository: kubernetes-sigs/cluster-api-provider-aws

Issue:
Cluster deletion stuck due to AWSCluster finalizer issue
/kind bug

**What steps did you take and what happened:**

We have CAPI objects deployed using Helm chart. Thus when `helm uninstall <release>` is executed all objects get deleted simultaneously.

Sometimes finalizer on `AWSCluster` object (and object itself) is removed before all `AWSMachine` resources are removed. This is causing `AWSMachines` to stuck forever, because awsmachine controller tries to patch `AWSCluster` somehow.

```
I0726 16:19:31.225463       1 awscluster_controller.go:208] "Reconciling AWSCluster delete" controller="awscluster" controllerGroup="infrastructure.cluster.x-k8s.io" controllerKind="AWSCluster" AWSCluster="defau
lt/aws-cl-1" namespace="default" name="aws-cl-1" reconcileID="da967d9f-4c3d-47a1-953a-cedf44e4d8d0" cluster="default/aws-cl-1"
I0726 16:19:33.955431       1 securitygroups.go:320] "Deleted security group" controller="awscluster" controllerGroup="infrastructure.cluster.x-k8s.io" controllerKind="AWSCluster" AWSCluster="default/aws-cl-1" n
amespace="default" name="aws-cl-1" reconcileID="da967d9f-4c3d-47a1-953a-cedf44e4d8d0" cluster="default/aws-cl-1" security-group-id="sg-068b633aae83d2e19" kind="cluster managed"
I0726 16:19:34.432437       1 securitygroups.go:320] "Deleted security group" controller="awscluster" controllerGroup="infrastructure.cluster.x-k8s.io" controllerKind="AWSCluster" AWSCluster="default/aws-cl-1" n
amespace="default" name="aws-cl-1" reconcileID="da967d9f-4c3d-47a1-953a-cedf44e4d8d0" cluster="default/aws-cl-1" security-group-id="sg-05fe37ab8f0a3ab15" kind="cluster managed"
I0726 16:19:36.516438       1 vpc.go:550] "Deleted VPC" controller="awscluster" controllerGroup="infrastructure.cluster.x-k8s.io" controllerKind="AWSCluster" AWSCluster="default/aws-cl-1" namespace="default" nam
e="aws-cl-1" reconcileID="da967d9f-4c3d-47a1-953a-cedf44e4d8d0" cluster="default/aws-cl-1" vpc-id="vpc-03b7241ad6eae9ab1"
E0726 16:19:36.632931       1 controller.go:329] "Reconciler error" err="failed to patch AWSCluster default/aws-cl-1: awsclusters.infrastructure.cluster.x-k8s.io \"aws-cl-1\" not found" controller="awscluster" c
ontrollerGroup="infrastructure.cluster.x-k8s.io" controllerKind="AWSCluster" AWSCluster="default/aws-cl-1" namespace="default" name="aws-cl-1" reconcileID="da967d9f-4c3d-47a1-953a-cedf44e4d8d0"
I0726 16:19:51.603067       1 awsmachine_controller.go:198] "AWSCluster or AWSManagedControlPlane is not ready yet" controller="awsmachine" controllerGroup="infrastructure.cluster.x-k8s.io" controllerKind="AWSMa
chine" AWSMachine="default/aws-cl-1-md-xczh6-bfjvv" namespace="default" name="aws-cl-1-md-xczh6-bfjvv" reconcileID="c0acf9c4-8be9-413f-a906-483b59563d9f" machine="default/aws-cl-1-md-xczh6-bfjvv" cluster="defaul
t/aws-cl-1"
I0726 16:19:52.434829       1 awsmachine_controller.go:198] "AWSCluster or AWSManagedControlPlane is not ready yet" controller="awsmachine" controllerGroup="infrastructure.cluster.x-k8s.io" controllerKind="AWSMa
chine" AWSMachine="default/aws-cl-1-cp-0" namespace="default" name="aws-cl-1-cp-0" reconcileID="dac57c28-e165-472b-b20f-fa0521e4b2f1" machine="default/aws-cl-1-cp-0" cluster="default/aws-cl-1"
I0726 16:19:59.970099       1 awsmachine_controller.go:198] "AWSCluster or AWSManagedControlPlane is not ready yet" controller="awsmachine" controllerGroup="infrastructure.cluster.x-k8s.io" controllerKind="AWSMa
chine" AWSMachine="default/aws-cl-1-cp-0" namespace="default" name="aws-cl-1-cp-0" reconcileID="df4dc07d-5e1b-4d28-88a3-0f30fa7a76f8" machine="default/aws-cl-1-cp-0" cluster="default/aws-cl-1"
I0726 16:19:59.970270       1 awsmachine_controller.go:198] "AWSCluster or AWSManagedControlPlane is not ready yet" controller="awsmachine" controllerGroup="infrastructure.cluster.x-k8s.io" controllerKind="AWSMa
chine" AWSMachine="default/aws-cl-1-md-xczh6-bfjvv" namespace="default" name="aws-cl-1-md-xczh6-bfjvv" reconcileID="120b3a56-1855-4d44-8333-8502e8d04981" machine="default/aws-cl-1-md-xczh6-bfjvv" cluster="defaul
t/aws-cl-1"
I0726 16:22:36.109923       1 awsmachine_controller.go:198] "AWSCluster or AWSManagedControlPlane is not ready yet" controller="awsmachine" controllerGroup="infrastructure.cluster.x-k8s.io" controllerKind="AWSMa
chine" AWSMachine="default/aws-cl-1-cp-0" namespace="default" name="aws-cl-1-cp-0" reconcileID="4c0f6242-2f77-4194-b9dc-c1aed2034184" machine="default/aws-cl-1-cp-0" cluster="default/aws-cl-1"
I0726 16:22:36.110149       1 awsmachine_controller.go:198] "AWSCluster or AWSManagedControlPlane is not ready yet" controller="awsmachine" controllerGroup="infrastructure.cluster.x-k8s.io" controllerKind="AWSMa
chine" AWSMachine="default/aws-cl-1-md-xczh6-bfjvv" namespace="default" name="aws-cl-1-md-xczh6-bfjvv" reconcileID="90ca119f-d6c2-457f-9d79-be35c1ae70a8" machine="default/aws-cl-1-md-xczh6-bfjvv" cluster="defaul
t/aws-cl-1"
I0726 16:29:30.719506       1 awsmachine_controller.go:198] "AWSCluster or AWSManagedControlPlane is not ready yet" controller="awsmachine" controllerGroup="infrastructure.cluster.x-k8s.io" controllerKind="AWSMa
chine" AWSMachine="default/aws-cl-1-cp-0" namespace="default" name="aws-cl-1-cp-0" reconcileID="d9d8a14a-8e04-43ab-b596-b3bc9083af81" machine="default/aws-cl-1-cp-0" cluster="default/aws-cl-1"
I0726 16:29:30.719543       1 awsmachine_controller.go:198] "AWSCluster or AWSManagedControlPlane is not ready yet" controller="awsmachine" controllerGroup="infrastructure.cluster.x-k8s.io" controllerKind="AWSMa
chine" AWSMachine="default/aws-cl-1-md-xczh6-bfjvv" namespace="default" name="aws-cl-1-md-xczh6-bfjvv" reconcileID="7bba88c3-0555-4536-bb12-d366df08e338" machine="default/aws-cl-1-md-xczh6-bfjvv" cluster="defaul
t/aws-cl-1"
I0726 16:32:52.252957       1 awsmachine_controller.go:198] "AWSCluster or AWSManagedControlPlane is not ready yet" controller="awsmachine" controllerGroup="infrastructure.cluster.x-k8s.io" controllerKind="AWSMa
chine" AWSMachine="default/aws-cl-1-md-xczh6-bfjvv" namespace="default" name="aws-cl-1-md-xczh6-bfjvv" reconcileID="2ba19b22-ddb1-4323-aa07-4bd170f5e49b" machine="default/aws-cl-1-md-xczh6-bfjvv" cluster="defaul
t/aws-cl-1"
I0726 16:32:52.253183       1 awsmachine_controller.go:198] "AWSCluster or AWSManagedControlPlane is not ready yet" controller="awsmachine" controllerGroup="infrastructure.cluster.x-k8s.io" controllerKind="AWSMa
chine" AWSMachine="default/aws-cl-1-cp-0" namespace="default" name="aws-cl-1-cp-0" reconcileID="36079ee5-a4ec-407a-a181-1d7a3ca1058f" machine="default/aws-cl-1-cp-0" cluster="default/aws-cl-1"
```

Ultimately this makes cluster deletion to stuck indefinitely.  The `Cluster` object will then stuck in the `Deleting` state.

To fix this operator should manually delete finalizers on all `AWSMachine` objects which are left in the cluster. 

*Note that this is intermittent issue, but happens pretty often in my tests (~7 times out of 10). Also I should note that I AWS resources seem to be properly cleaned up*

**What did you expect to happen:**

`AWSCluster` object finalizer should only get removed when no depended objects (like `AWSMachine`) are present.

**Environment:**

- Cluster-api-provider-aws version: `v2.6.1`
- Kubernetes version: (use `kubectl version`): `v1.30.2+k0s`
- OS (e.g. from `/etc/os-release`): `Amazon Linux 2`

Discussion:
@k8s-ci-robot: This issue is currently awaiting triage.

If CAPA/CAPI contributors determines this is a relevant issue, they will accept it by applying the `triage/accepted` label and provide further guidance.

The `triage/accepted` label can be added by org members by writing `/triage accepted` in a comment.


<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
</details>@a13x5: @randybias FYI@fiunchinho: We also use Helm charts to deploy CAPA resources and we've also ran into issues when deleting clusters, but I believe we were doing the same mistake that you seem to be doing now. Let me try to explain our journey. Maintainers or other folks with more experience can correct me if I'm wrong, because I may be :)

CAPI and CAPA need to take care of deleting a cluster, specifically they need to follow certain order in which the different resources are removed. First worker nodes are removed (i.e. `MachinePools`), then control plane is removed (i.e. `KubeadmControlplane`). And only after all of these are removed, then the `AWSCluster` is removed. Finally, when the infrastructure cluster is removed, the parent `Cluster` is removed.

When using Helm charts, helm hijacks the whole deletion process, deleting all objects at once. The `AWSCluster` deletion logic is triggered before the workers have finished being deleted. This is your current issue if I understand correctly.
To fix this problem and allow CAPI and CAPA to manage the deletion of the resources, we now add the helm annotation `helm.sh/resource-policy: keep` to all resources but the `Cluster` CR. This means that when a chart with a cluster is deleted, the `Cluster` will be marked for deletion, and CAPI / CAPA will start the deletion process in the order they need. Once the process is completed, all the finalizers in the `Cluster` CR will be removed, and the `Cluster` CR will be garbage collected.

This may not apply to you, but as a final note, we ended up removing the `helm.sh/resource-policy: keep` annotation from `MachinePools` because we wanted to be able to change the name of the node pools. If we would keep the annotation, a new `MachinePool` would be rendered by Helm, but the old one would be kept. Because workers are the first thing CAPI / CAPA wants to remove, we decided it was safe to remove the `helm.sh/resource-policy: keep` annotation in this case.

I hope that helps.

So I believe this is working as intended. Maybe we could add documentation about this particular use case.@a13x5: Thanks for your answer and suggestions, but I will have to disagree.

Kubernetes is a **declarative** system, so when operating on objects I shouldn't maintain any order of execution. 
The controller should maintain the required order of execution. For example it would be a shame if I could delete PVC when it's still being used by a pod or namespace deleted when it's still has objects.

CAPA in this case fails to maintain it's own state and order of execution it requires. It deletes the finalizer on it's own resource which still has dependent resources present. All these resources are owned (reconciled) by a CAPA controller. So this is clearly a bug.

I'm considering the solution with annotations (thanks for the hint btw) only as a workaround, but not as proper fix.@fiunchinho: > Thanks for your answer and suggestions, but I will have to disagree.
> 
> Kubernetes is a **declarative** system, so when operating on objects I shouldn't maintain any order of execution. The controller should maintain the required order of execution. For example it would be a shame if I could delete PVC when it's still being used by a pod or namespace deleted when it's still has objects.
> 
> CAPA in this case fails to maintain it's own state and order of execution it requires. It deletes the finalizer on it's own resource which still has dependent resources present. All these resources are owned (reconciled) by a CAPA controller. So this is clearly a bug.
> 
> I'm considering the solution with annotations (thanks for the hint btw) only as a workaround, but not as proper fix.

Fair enough. Let's see if that could be improved, because it would be a good improvement to all users, not just the ones using Helm.@k8s-triage-robot: The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:
- After 90d of inactivity, `lifecycle/stale` is applied
- After 30d of inactivity since `lifecycle/stale` was applied, `lifecycle/rotten` is applied
- After 30d of inactivity since `lifecycle/rotten` was applied, the issue is closed

You can:
- Mark this issue as fresh with `/remove-lifecycle stale`
- Close this issue with `/close`
- Offer to help out with [Issue Triage][1]

Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community).

/lifecycle stale

[1]: https://www.kubernetes.dev/docs/guide/issue-triage/@k8s-triage-robot: The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:
- After 90d of inactivity, `lifecycle/stale` is applied
- After 30d of inactivity since `lifecycle/stale` was applied, `lifecycle/rotten` is applied
- After 30d of inactivity since `lifecycle/rotten` was applied, the issue is closed

You can:
- Mark this issue as fresh with `/remove-lifecycle rotten`
- Close this issue with `/close`
- Offer to help out with [Issue Triage][1]

Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community).

/lifecycle rotten

[1]: https://www.kubernetes.dev/docs/guide/issue-triage/@joshfrench: /remove-lifecycle rotten@richardcase: This is not a CAPA specific thing but for the whole of CAPI. In CAPI the only supported way to delete a cluster is by deleting the `Cluster` resource. If you try and delete all the resources then you will end up with orphaned resources. Please see: https://cluster-api.sigs.k8s.io/user/quick-start.html?highlight=delete#clean-up:

```
IMPORTANT: In order to ensure a proper cleanup of your infrastructure you must always delete the cluster object. Deleting the entire cluster template with kubectl delete -f capi-quickstart.yaml might lead to pending resources to be cleaned up manually.
```

If you use Helm for the cluster definitions then as @fiunchinho you will need to use `helm.sh/resource-policy: keep`

Solution patch:
diff --git a/controllers/awscluster_controller.go b/controllers/awscluster_controller.go
index dbc131f6b..0389681f9 100644
--- a/controllers/awscluster_controller.go
+++ b/controllers/awscluster_controller.go
@@ -58,6 +58,10 @@ import (
 	"sigs.k8s.io/cluster-api/util/predicates"
 )
 
+const (
+	deleteRequeueAfter = 20 * time.Second
+)
+
 var defaultAWSSecurityGroupRoles = []infrav1.SecurityGroupRole{
 	infrav1.SecurityGroupAPIServerLB,
 	infrav1.SecurityGroupLB,
@@ -191,21 +195,34 @@ func (r *AWSClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 
 	// Handle deleted clusters
 	if !awsCluster.DeletionTimestamp.IsZero() {
-		return ctrl.Result{}, r.reconcileDelete(ctx, clusterScope)
+		return r.reconcileDelete(ctx, clusterScope)
 	}
 
 	// Handle non-deleted clusters
 	return r.reconcileNormal(clusterScope)
 }
 
-func (r *AWSClusterReconciler) reconcileDelete(ctx context.Context, clusterScope *scope.ClusterScope) error {
+func (r *AWSClusterReconciler) reconcileDelete(ctx context.Context, clusterScope *scope.ClusterScope) (ctrl.Result, error) {
 	if !controllerutil.ContainsFinalizer(clusterScope.AWSCluster, infrav1.ClusterFinalizer) {
 		clusterScope.Info("No finalizer on AWSCluster, skipping deletion reconciliation")
-		return nil
+		return reconcile.Result{}, nil
 	}
 
 	clusterScope.Info("Reconciling AWSCluster delete")
 
+	numDependencies, err := r.dependencyCount(ctx, clusterScope)
+	if err != nil {
+		clusterScope.Error(err, "error getting AWSCluster dependencies")
+		return reconcile.Result{}, err
+	}
+
+	if numDependencies > 0 {
+		clusterScope.Info("AWSCluster cluster still has dependencies - requeue needed", "dependencyCount", numDependencies)
+		return reconcile.Result{RequeueAfter: deleteRequeueAfter}, nil
+	}
+
+	clusterScope.Info("AWSCluster has no dependent CAPI resources, proceeding with its deletion")
+
 	ec2svc := r.getEC2Service(clusterScope)
 	elbsvc := r.getELBService(clusterScope)
 	networkSvc := r.getNetworkService(*clusterScope)
@@ -257,12 +274,12 @@ func (r *AWSClusterReconciler) reconcileDelete(ctx context.Context, clusterScope
 	}
 
 	if len(allErrs) > 0 {
-		return kerrors.NewAggregate(allErrs)
+		return reconcile.Result{}, kerrors.NewAggregate(allErrs)
 	}
 
 	// Cluster is deleted so remove the finalizer.
 	controllerutil.RemoveFinalizer(clusterScope.AWSCluster, infrav1.ClusterFinalizer)
-	return nil
+	return reconcile.Result{}, nil
 }
 
 func (r *AWSClusterReconciler) reconcileLoadBalancer(clusterScope *scope.ClusterScope, awsCluster *infrav1.AWSCluster) (*time.Duration, error) {
@@ -484,3 +501,24 @@ func (r *AWSClusterReconciler) checkForExternalControlPlaneLoadBalancer(clusterS
 		return nil
 	}
 }
+
+func (r *AWSClusterReconciler) dependencyCount(ctx context.Context, clusterScope *scope.ClusterScope) (int, error) {
+	clusterName := clusterScope.Name()
+	namespace := clusterScope.Namespace()
+
+	clusterScope.Info("Looking for AWSCluster dependencies")
+
+	listOptions := []client.ListOption{
+		client.InNamespace(namespace),
+		client.MatchingLabels(map[string]string{clusterv1.ClusterNameLabel: clusterName}),
+	}
+
+	machines := &infrav1.AWSMachineList{}
+	if err := r.Client.List(ctx, machines, listOptions...); err != nil {
+		return 0, fmt.Errorf("failed to list machines for cluster %s/%s: %w", namespace, clusterName, err)
+	}
+
+	clusterScope.Debug("Found dependent machines", "count", len(machines.Items))
+
+	return len(machines.Items), nil
+}



Test patch:
diff --git a/controllers/awscluster_controller_test.go b/controllers/awscluster_controller_test.go
index a40a6ed1b..474abaaf9 100644
--- a/controllers/awscluster_controller_test.go
+++ b/controllers/awscluster_controller_test.go
@@ -568,7 +568,7 @@ func TestAWSClusterReconcilerIntegrationTests(t *testing.T) {
 		_, err = reconciler.reconcileNormal(cs)
 		g.Expect(err.Error()).To(ContainSubstring("The maximum number of VPCs has been reached"))
 
-		err = reconciler.reconcileDelete(ctx, cs)
+		_, err = reconciler.reconcileDelete(ctx, cs)
 		g.Expect(err).To(BeNil())
 	})
 	t.Run("Should successfully delete AWSCluster with managed VPC", func(t *testing.T) {
@@ -646,7 +646,7 @@ func TestAWSClusterReconcilerIntegrationTests(t *testing.T) {
 			return sgSvc
 		}
 
-		err = reconciler.reconcileDelete(ctx, cs)
+		_, err = reconciler.reconcileDelete(ctx, cs)
 		g.Expect(err).To(BeNil())
 		expectAWSClusterConditions(g, cs.AWSCluster, []conditionAssertion{
 			{infrav1.LoadBalancerReadyCondition, corev1.ConditionFalse, clusterv1.ConditionSeverityInfo, clusterv1.DeletedReason},
diff --git a/controllers/awscluster_controller_unit_test.go b/controllers/awscluster_controller_unit_test.go
index be59c0a67..0ea2bfaed 100644
--- a/controllers/awscluster_controller_unit_test.go
+++ b/controllers/awscluster_controller_unit_test.go
@@ -420,7 +420,7 @@ func TestAWSClusterReconcileOperations(t *testing.T) {
 					},
 				)
 				g.Expect(err).To(BeNil())
-				err = reconciler.reconcileDelete(ctx, cs)
+				_, err = reconciler.reconcileDelete(ctx, cs)
 				g.Expect(err).To(BeNil())
 				g.Expect(awsCluster.GetFinalizers()).ToNot(ContainElement(infrav1.ClusterFinalizer))
 			})
@@ -449,7 +449,7 @@ func TestAWSClusterReconcileOperations(t *testing.T) {
 					},
 				)
 				g.Expect(err).To(BeNil())
-				err = reconciler.reconcileDelete(ctx, cs)
+				_, err = reconciler.reconcileDelete(ctx, cs)
 				g.Expect(err).ToNot(BeNil())
 				g.Expect(awsCluster.GetFinalizers()).To(ContainElement(infrav1.ClusterFinalizer))
 			})
@@ -474,7 +474,7 @@ func TestAWSClusterReconcileOperations(t *testing.T) {
 					},
 				)
 				g.Expect(err).To(BeNil())
-				err = reconciler.reconcileDelete(ctx, cs)
+				_, err = reconciler.reconcileDelete(ctx, cs)
 				g.Expect(err).ToNot(BeNil())
 				g.Expect(awsCluster.GetFinalizers()).To(ContainElement(infrav1.ClusterFinalizer))
 			})
@@ -499,7 +499,7 @@ func TestAWSClusterReconcileOperations(t *testing.T) {
 					},
 				)
 				g.Expect(err).To(BeNil())
-				err = reconciler.reconcileDelete(ctx, cs)
+				_, err = reconciler.reconcileDelete(ctx, cs)
 				g.Expect(err).ToNot(BeNil())
 				g.Expect(awsCluster.GetFinalizers()).To(ContainElement(infrav1.ClusterFinalizer))
 			})
@@ -524,7 +524,7 @@ func TestAWSClusterReconcileOperations(t *testing.T) {
 					},
 				)
 				g.Expect(err).To(BeNil())
-				err = reconciler.reconcileDelete(ctx, cs)
+				_, err = reconciler.reconcileDelete(ctx, cs)
 				g.Expect(err).ToNot(BeNil())
 				g.Expect(awsCluster.GetFinalizers()).To(ContainElement(infrav1.ClusterFinalizer))
 			})