Repository: kubernetes-sigs/cluster-api-provider-aws
Pull request #3967 (2023-01-09 15:44:13)

Apply #3871 "Update LoadBalancerReadyCondition on deletion" to release-1.5
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 <github@armesto.net>

<!-- Thanks for this PR! If this is your first PR please read the [contributing guide](../CONTRIBUTING.md) -->
<!-- If this PR is still work-in-progress and is being open for visibility please prefix the title with `WIP:` -->

**What type of PR is this?**
/kind bug
<!--
Add one of the following kinds:
/kind feature
/kind bug
/kind api-change
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind flake
/kind regression
/kind support
-->

**What this PR does / why we need it**:

This is #3871, but for the release-1.5. The original PR could not be backported due to conflicts.
<!-- Enter a description of the change and why this change is needed -->

**Special notes for your reviewer**:

**Checklist**:
<!-- Put an "X" character inside the brackets of each completed task. Some may be optional depending on the PR in which case these can be deleted -->

- [x] squashed commits
- [ ] includes documentation
- [x] adds unit tests
- [ ] adds or updates e2e tests



Patch:
diff --git a/controllers/awscluster_controller.go b/controllers/awscluster_controller.go
index e943d339d..4e30a95bb 100644
--- a/controllers/awscluster_controller.go
+++ b/controllers/awscluster_controller.go
@@ -170,6 +170,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())
diff --git a/pkg/cloud/services/elb/loadbalancer.go b/pkg/cloud/services/elb/loadbalancer.go
index b8b826b66..1f4392916 100644
--- a/pkg/cloud/services/elb/loadbalancer.go
+++ b/pkg/cloud/services/elb/loadbalancer.go
@@ -157,6 +157,8 @@ func (s *Service) deleteAPIServerELB() error {
 
 	apiELB, err := s.describeClassicELB(elbName)
 	if IsNotFound(err) {
+		s.scope.V(2).Info("Control plane load balancer not found, skipping deletion")
+		conditions.MarkFalse(s.scope.InfraCluster(), infrav1.LoadBalancerReadyCondition, clusterv1.DeletedReason, clusterv1.ConditionSeverityInfo, "")
 		return nil
 	}
 	if err != nil {
@@ -165,6 +167,7 @@ func (s *Service) deleteAPIServerELB() error {
 
 	if apiELB.IsUnmanaged(s.scope.Name()) {
 		s.scope.V(2).Info("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
 	}
 


Test patch:
diff --git a/pkg/cloud/services/elb/loadbalancer_test.go b/pkg/cloud/services/elb/loadbalancer_test.go
index 4225d6765..b76eee23b 100644
--- a/pkg/cloud/services/elb/loadbalancer_test.go
+++ b/pkg/cloud/services/elb/loadbalancer_test.go
@@ -39,6 +39,7 @@ import (
 	"sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/scope"
 	"sigs.k8s.io/cluster-api-provider-aws/test/mocks"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+	"sigs.k8s.io/cluster-api/util/conditions"
 )
 
 func TestELBName(t *testing.T) {
@@ -600,8 +601,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 +612,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 +661,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 +723,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 +787,8 @@ func TestDeleteAPIServerELB(t *testing.T) {
 			if err != nil {
 				t.Fatal(err)
 			}
+
+			tc.verifyAWSCluster(awsCluster)
 		})
 	}
 }
diff --git a/test/e2e/data/e2e_conf.yaml b/test/e2e/data/e2e_conf.yaml
index 53c8bc2ca..dbbed94df 100644
--- a/test/e2e/data/e2e_conf.yaml
+++ b/test/e2e/data/e2e_conf.yaml
@@ -189,7 +189,7 @@ variables:
   AWS_NODE_MACHINE_TYPE: t3.large
   AWS_MACHINE_TYPE_VCPU_USAGE: 2
   AWS_SSH_KEY_NAME: "cluster-api-provider-aws-sigs-k8s-io"
-  CONFORMANCE_CI_ARTIFACTS_KUBERNETES_VERSION: "v1.24.4"
+  CONFORMANCE_CI_ARTIFACTS_KUBERNETES_VERSION: "v1.24.0"
   CONFORMANCE_WORKER_MACHINE_COUNT: "5"
   CONFORMANCE_CONTROL_PLANE_MACHINE_COUNT: "1"
   ETCD_VERSION_UPGRADE_TO: "3.5.3-0"
@@ -214,7 +214,7 @@ intervals:
   default/wait-cluster: ["30m", "10s"]
   default/wait-control-plane: ["25m", "10s"]
   default/wait-worker-nodes: ["20m", "10s"]
-  conformance/wait-control-plane: ["35m", "10s"]
+  conformance/wait-control-plane: ["30m", "10s"]
   conformance/wait-worker-nodes: ["35m", "10s"]
   default/wait-controllers: ["5m", "10s"]
   default/wait-delete-cluster: ["20m", "10s"]
diff --git a/test/e2e/data/e2e_eks_conf.yaml b/test/e2e/data/e2e_eks_conf.yaml
index fadf7ecdf..b03072413 100644
--- a/test/e2e/data/e2e_eks_conf.yaml
+++ b/test/e2e/data/e2e_eks_conf.yaml
@@ -190,7 +190,7 @@ variables:
 
 intervals:
   default/wait-cluster: ["30m", "10s"]
-  default/wait-control-plane: ["35m", "10s"]
+  default/wait-control-plane: ["30m", "10s"]
   default/wait-worker-nodes: ["30m", "10s"]
   default/wait-controllers: ["5m", "10s"]
   default/wait-delete-cluster: ["35m", "30s"]
diff --git a/test/e2e/shared/aws.go b/test/e2e/shared/aws.go
index 90e03c4b5..b178591fc 100644
--- a/test/e2e/shared/aws.go
+++ b/test/e2e/shared/aws.go
@@ -759,7 +759,8 @@ func DumpCloudTrailEvents(e2eCtx *E2EContext) {
 // Kubernetes version in the e2econfig.
 func conformanceImageID(e2eCtx *E2EContext) string {
 	ver := e2eCtx.E2EConfig.GetVariable("CONFORMANCE_CI_ARTIFACTS_KUBERNETES_VERSION")
-	amiName := AMIPrefix + ver + "*"
+	strippedVer := strings.Replace(ver, "v", "", 1)
+	amiName := AMIPrefix + strippedVer + "*"
 
 	Byf("Searching for AMI: name=%s", amiName)
 	ec2Svc := ec2.New(e2eCtx.AWSSession)