From 430412c2bd94583e9f9643e09b7db435dd842131 Mon Sep 17 00:00:00 2001 From: Mario Nitchev Date: Tue, 22 Nov 2022 17:58:53 +0200 Subject: [PATCH 1/2] 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 --- controllers/awscluster_controller.go | 14 ++++--- pkg/cloud/services/elb/loadbalancer.go | 8 +++- pkg/cloud/services/elb/loadbalancer_test.go | 41 +++++++++++++++++++-- 3 files changed, 52 insertions(+), 11 deletions(-) diff --git a/controllers/awscluster_controller.go b/controllers/awscluster_controller.go index b68eb8fb0a..525f22afb9 100644 --- a/controllers/awscluster_controller.go +++ b/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()) diff --git a/pkg/cloud/services/elb/loadbalancer.go b/pkg/cloud/services/elb/loadbalancer.go index f3aca6d64f..f8ccc7657e 100644 --- a/pkg/cloud/services/elb/loadbalancer.go +++ b/pkg/cloud/services/elb/loadbalancer.go @@ -31,6 +31,9 @@ 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" @@ -38,8 +41,6 @@ import ( "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 } diff --git a/pkg/cloud/services/elb/loadbalancer_test.go b/pkg/cloud/services/elb/loadbalancer_test.go index f38a27a01b..287e32750a 100644 --- a/pkg/cloud/services/elb/loadbalancer_test.go +++ b/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) }) } } From cd7273fa7b46453cf366e9e84c8313f5488bcf4e Mon Sep 17 00:00:00 2001 From: Mario Nitchev Date: Wed, 23 Nov 2022 09:56:05 +0200 Subject: [PATCH 2/2] Make gci linter happy --- controllers/awscluster_controller.go | 13 ++++++------- pkg/cloud/services/elb/loadbalancer.go | 5 ++--- pkg/cloud/services/elb/loadbalancer_test.go | 5 ++--- 3 files changed, 10 insertions(+), 13 deletions(-) diff --git a/controllers/awscluster_controller.go b/controllers/awscluster_controller.go index 525f22afb9..f091068cf7 100644 --- a/controllers/awscluster_controller.go +++ b/controllers/awscluster_controller.go @@ -38,13 +38,6 @@ 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" @@ -58,6 +51,12 @@ 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{ diff --git a/pkg/cloud/services/elb/loadbalancer.go b/pkg/cloud/services/elb/loadbalancer.go index f8ccc7657e..0db1f15033 100644 --- a/pkg/cloud/services/elb/loadbalancer.go +++ b/pkg/cloud/services/elb/loadbalancer.go @@ -31,9 +31,6 @@ 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" @@ -41,6 +38,8 @@ import ( "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 diff --git a/pkg/cloud/services/elb/loadbalancer_test.go b/pkg/cloud/services/elb/loadbalancer_test.go index 287e32750a..f225354a75 100644 --- a/pkg/cloud/services/elb/loadbalancer_test.go +++ b/pkg/cloud/services/elb/loadbalancer_test.go @@ -35,12 +35,11 @@ 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" + "sigs.k8s.io/cluster-api/util/conditions" ) func TestELBName(t *testing.T) {