Merge pull request #4567 from vincepri/gc-earlier

Try to delete as many resources as possible, then requeue

controllers/awscluster_controller.go

@@ -26,6 +26,7 @@ import (
 	"github.com/pkg/errors"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/types"
+	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/client-go/tools/record"
 	"k8s.io/klog/v2"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -215,40 +216,48 @@ func (r *AWSClusterReconciler) reconcileDelete(ctx context.Context, clusterScope
 		}
 	}
 
+	// In this context we try to delete all the resources that we know about,
+	// and run the garbage collector to delete any resources that were tagged, if enabled.
+	//
+	// The reason the errors are collected and not returned immediately is that we want to
+	// try to delete as many resources as possible, and then return the errors.
+	// Resources like security groups, or load balancers can depende on each other, especially
+	// when external controllers might be using them.
+	allErrs := []error{}
+
+	if err := s3Service.DeleteBucket(); err != nil {
+		allErrs = append(allErrs, errors.Wrapf(err, "error deleting S3 Bucket"))
+	}
+
 	if err := elbsvc.DeleteLoadbalancers(); err != nil {
-		clusterScope.Error(err, "error deleting load balancer")
-		return err
+		allErrs = append(allErrs, errors.Wrapf(err, "error deleting load balancers"))
 	}
 
 	if err := ec2svc.DeleteBastion(); err != nil {
-		clusterScope.Error(err, "error deleting bastion")
-		return err
+		allErrs = append(allErrs, errors.Wrapf(err, "error deleting bastion"))
 	}
 
 	if err := sgService.DeleteSecurityGroups(); err != nil {
-		clusterScope.Error(err, "error deleting security groups")
-		return err
+		allErrs = append(allErrs, errors.Wrap(err, "error deleting security groups"))
 	}
 
 	if r.ExternalResourceGC {
 		gcSvc := gc.NewService(clusterScope, gc.WithGCStrategy(r.AlternativeGCStrategy))
 		if gcErr := gcSvc.ReconcileDelete(ctx); gcErr != nil {
-			return fmt.Errorf("failed delete reconcile for gc service: %w", gcErr)
+			allErrs = append(allErrs, fmt.Errorf("failed delete reconcile for gc service: %w", gcErr))
 		}
 	}
 
 	if err := networkSvc.DeleteNetwork(); err != nil {
-		clusterScope.Error(err, "error deleting network")
-		return err
+		allErrs = append(allErrs, errors.Wrap(err, "error deleting network"))
 	}
 
-	if err := s3Service.DeleteBucket(); err != nil {
-		return errors.Wrapf(err, "error deleting S3 Bucket")
+	if len(allErrs) > 0 {
+		return kerrors.NewAggregate(allErrs)
 	}
 
 	// Cluster is deleted so remove the finalizer.
 	controllerutil.RemoveFinalizer(clusterScope.AWSCluster, infrav1.ClusterFinalizer)
-
 	return nil
 }
 

controllers/awscluster_controller_unit_test.go

@@ -424,6 +424,9 @@ func TestAWSClusterReconcileOperations(t *testing.T) {
 				deleteCluster := func() {
 					t.Helper()
 					elbSvc.EXPECT().DeleteLoadbalancers().Return(expectedErr)
+					ec2Svc.EXPECT().DeleteBastion().Return(nil)
+					networkSvc.EXPECT().DeleteNetwork().Return(nil)
+					sgSvc.EXPECT().DeleteSecurityGroups().Return(nil)
 				}
 				awsCluster := getAWSCluster("test", "test")
 				awsCluster.Finalizers = []string{infrav1.ClusterFinalizer}
@@ -447,6 +450,8 @@ func TestAWSClusterReconcileOperations(t *testing.T) {
 				deleteCluster := func() {
 					ec2Svc.EXPECT().DeleteBastion().Return(expectedErr)
 					elbSvc.EXPECT().DeleteLoadbalancers().Return(nil)
+					networkSvc.EXPECT().DeleteNetwork().Return(nil)
+					sgSvc.EXPECT().DeleteSecurityGroups().Return(nil)
 				}
 				awsCluster := getAWSCluster("test", "test")
 				awsCluster.Finalizers = []string{infrav1.ClusterFinalizer}
@@ -471,6 +476,7 @@ func TestAWSClusterReconcileOperations(t *testing.T) {
 					ec2Svc.EXPECT().DeleteBastion().Return(nil)
 					elbSvc.EXPECT().DeleteLoadbalancers().Return(nil)
 					sgSvc.EXPECT().DeleteSecurityGroups().Return(expectedErr)
+					networkSvc.EXPECT().DeleteNetwork().Return(nil)
 				}
 				awsCluster := getAWSCluster("test", "test")
 				awsCluster.Finalizers = []string{infrav1.ClusterFinalizer}