Update LoadBalancerReadyCondition on deletion

The LoadBalancerReadyCondition is sometimes explicitly patched and other
times it only updates the awsCluster object. Furthermore the condition
won't be patched by the patch helper in the AWSClusterReconciler,
because it's not in the `patch.WithOwnedConditions` list.
This change puts the condition in the list and also updates the
condition reason to `Deleted` when the LB is not found. Without setting
the condition in that case, the reconciler will set condition to
`Deleting` and it will never go back to `Deleted`.

Co-authored-by: Jose Armesto <[email protected]>

Author: Mario Nitchev

controllers/awscluster_controller.go

@@ -38,6 +38,13 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 	"sigs.k8s.io/controller-runtime/pkg/source"
 
+	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+	"sigs.k8s.io/cluster-api/util"
+	capiannotations "sigs.k8s.io/cluster-api/util/annotations"
+	"sigs.k8s.io/cluster-api/util/conditions"
+	"sigs.k8s.io/cluster-api/util/patch"
+	"sigs.k8s.io/cluster-api/util/predicates"
+
 	infrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2"
 	"sigs.k8s.io/cluster-api-provider-aws/v2/feature"
 	"sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/scope"
@@ -51,12 +58,6 @@ import (
 	"sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/services/securitygroup"
 	"sigs.k8s.io/cluster-api-provider-aws/v2/pkg/logger"
 	infrautilconditions "sigs.k8s.io/cluster-api-provider-aws/v2/util/conditions"
-	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
-	"sigs.k8s.io/cluster-api/util"
-	capiannotations "sigs.k8s.io/cluster-api/util/annotations"
-	"sigs.k8s.io/cluster-api/util/conditions"
-	"sigs.k8s.io/cluster-api/util/patch"
-	"sigs.k8s.io/cluster-api/util/predicates"
 )
 
 var defaultAWSSecurityGroupRoles = []infrav1.SecurityGroupRole{
@@ -171,6 +172,7 @@ func (r *AWSClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 			patch.WithOwnedConditions{Conditions: []clusterv1.ConditionType{
 				infrav1.PrincipalCredentialRetrievedCondition,
 				infrav1.PrincipalUsageAllowedCondition,
+				infrav1.LoadBalancerReadyCondition,
 			}})
 		if e != nil {
 			fmt.Println(e.Error())

pkg/cloud/services/elb/loadbalancer.go

@@ -31,15 +31,16 @@ import (
 	"github.com/pkg/errors"
 	"k8s.io/apimachinery/pkg/util/sets"
 
+	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+	"sigs.k8s.io/cluster-api/util/conditions"
+
 	infrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2"
 	"sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/awserrors"
 	"sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/converters"
 	"sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/scope"
 	"sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/services/wait"
 	"sigs.k8s.io/cluster-api-provider-aws/v2/pkg/hash"
 	"sigs.k8s.io/cluster-api-provider-aws/v2/pkg/record"
-	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
-	"sigs.k8s.io/cluster-api/util/conditions"
 )
 
 // ResourceGroups are filtered by ARN identifier: https://docs.aws.amazon.com/general/latest/gr/aws-arns-and-namespaces.html#arns-syntax
@@ -148,6 +149,8 @@ func (s *Service) deleteAPIServerELB() error {
 
 	apiELB, err := s.describeClassicELB(elbName)
 	if IsNotFound(err) {
+		s.scope.Debug("Control plane load balancer not found, skipping deletion")
+		conditions.MarkFalse(s.scope.InfraCluster(), infrav1.LoadBalancerReadyCondition, clusterv1.DeletedReason, clusterv1.ConditionSeverityInfo, "")
 		return nil
 	}
 	if err != nil {
@@ -156,6 +159,7 @@ func (s *Service) deleteAPIServerELB() error {
 
 	if apiELB.IsUnmanaged(s.scope.Name()) {
 		s.scope.Debug("Found unmanaged classic load balancer for apiserver, skipping deletion", "api-server-elb-name", apiELB.Name)
+		conditions.MarkFalse(s.scope.InfraCluster(), infrav1.LoadBalancerReadyCondition, clusterv1.DeletedReason, clusterv1.ConditionSeverityInfo, "")
 		return nil
 	}
 

pkg/cloud/services/elb/loadbalancer_test.go

@@ -35,10 +35,12 @@ import (
 	"k8s.io/utils/pointer"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
+	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+	"sigs.k8s.io/cluster-api/util/conditions"
+
 	infrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2"
 	"sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/scope"
 	"sigs.k8s.io/cluster-api-provider-aws/v2/test/mocks"
-	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 )
 
 func TestELBName(t *testing.T) {
@@ -600,8 +602,9 @@ func TestDeleteAPIServerELB(t *testing.T) {
 	clusterName := "bar" //nolint:goconst // does not need to be a package-level const
 	elbName := "bar-apiserver"
 	tests := []struct {
-		name        string
-		elbAPIMocks func(m *mocks.MockELBAPIMockRecorder)
+		name             string
+		elbAPIMocks      func(m *mocks.MockELBAPIMockRecorder)
+		verifyAWSCluster func(*infrav1.AWSCluster)
 	}{
 		{
 			name: "if control plane ELB is not found, do nothing",
@@ -610,6 +613,16 @@ func TestDeleteAPIServerELB(t *testing.T) {
 					LoadBalancerNames: aws.StringSlice([]string{elbName}),
 				})).Return(nil, awserr.New(elb.ErrCodeAccessPointNotFoundException, "", nil))
 			},
+			verifyAWSCluster: func(awsCluster *infrav1.AWSCluster) {
+				loadBalancerConditionReady := conditions.IsTrue(awsCluster, infrav1.LoadBalancerReadyCondition)
+				if loadBalancerConditionReady {
+					t.Fatalf("Expected LoadBalancerReady condition to be False, but was True")
+				}
+				loadBalancerConditionReason := conditions.GetReason(awsCluster, infrav1.LoadBalancerReadyCondition)
+				if loadBalancerConditionReason != clusterv1.DeletedReason {
+					t.Fatalf("Expected LoadBalancerReady condition reason to be Deleted, but was %s", loadBalancerConditionReason)
+				}
+			},
 		},
 		{
 			name: "if control plane ELB is found, and it is not managed, do nothing",
@@ -649,6 +662,16 @@ func TestDeleteAPIServerELB(t *testing.T) {
 					nil,
 				)
 			},
+			verifyAWSCluster: func(awsCluster *infrav1.AWSCluster) {
+				loadBalancerConditionReady := conditions.IsTrue(awsCluster, infrav1.LoadBalancerReadyCondition)
+				if loadBalancerConditionReady {
+					t.Fatalf("Expected LoadBalancerReady condition to be False, but was True")
+				}
+				loadBalancerConditionReason := conditions.GetReason(awsCluster, infrav1.LoadBalancerReadyCondition)
+				if loadBalancerConditionReason != clusterv1.DeletedReason {
+					t.Fatalf("Expected LoadBalancerReady condition reason to be Deleted, but was %s", loadBalancerConditionReason)
+				}
+			},
 		},
 		{
 			name: "if control plane ELB is found, and it is managed, delete the ELB",
@@ -701,6 +724,16 @@ func TestDeleteAPIServerELB(t *testing.T) {
 					nil,
 				)
 			},
+			verifyAWSCluster: func(awsCluster *infrav1.AWSCluster) {
+				loadBalancerConditionReady := conditions.IsTrue(awsCluster, infrav1.LoadBalancerReadyCondition)
+				if loadBalancerConditionReady {
+					t.Fatalf("Expected LoadBalancerReady condition to be False, but was True")
+				}
+				loadBalancerConditionReason := conditions.GetReason(awsCluster, infrav1.LoadBalancerReadyCondition)
+				if loadBalancerConditionReason != clusterv1.DeletedReason {
+					t.Fatalf("Expected LoadBalancerReady condition reason to be Deleted, but was %s", loadBalancerConditionReason)
+				}
+			},
 		},
 	}
 
@@ -755,6 +788,8 @@ func TestDeleteAPIServerELB(t *testing.T) {
 			if err != nil {
 				t.Fatal(err)
 			}
+
+			tc.verifyAWSCluster(awsCluster)
 		})
 	}
 }