Merge pull request #3832 from Ankitasw/cleanup-internet-facing-logic

Clean up internet-facing loadbalancer scheme logic
kubernetes-sigs · Nov 10, 2022 · 1317a26 · 1317a26
2 parents 5eb7ee3 + ce55e2f
commit 1317a26
Show file tree

Hide file tree

Showing 5 changed files with 18 additions and 57 deletions.
diff --git a/api/v1beta2/awscluster_webhook.go b/api/v1beta2/awscluster_webhook.go
@@ -81,11 +81,15 @@ func (r *AWSCluster) ValidateUpdate(old runtime.Object) error {
 	}
 
 	newLoadBalancer := &AWSLoadBalancerSpec{}
+	existingLoadBalancer := &AWSLoadBalancerSpec{}
 
 	if r.Spec.ControlPlaneLoadBalancer != nil {
 		newLoadBalancer = r.Spec.ControlPlaneLoadBalancer.DeepCopy()
 	}
 
+	if oldC.Spec.ControlPlaneLoadBalancer != nil {
+		existingLoadBalancer = oldC.Spec.ControlPlaneLoadBalancer.DeepCopy()
+	}
 	if oldC.Spec.ControlPlaneLoadBalancer == nil {
 		// If old scheme was nil, the only value accepted here is the default value: internet-facing
 		if newLoadBalancer.Scheme != nil && newLoadBalancer.Scheme.String() != ClassicELBSchemeInternetFacing.String() {
@@ -96,16 +100,11 @@ func (r *AWSCluster) ValidateUpdate(old runtime.Object) error {
 		}
 	} else {
 		// If old scheme was not nil, the new scheme should be the same.
-		existingLoadBalancer := oldC.Spec.ControlPlaneLoadBalancer.DeepCopy()
 		if !cmp.Equal(existingLoadBalancer.Scheme, newLoadBalancer.Scheme) {
-			// Only allow changes from Internet-facing scheme to internet-facing.
-			if !(existingLoadBalancer.Scheme.String() == ClassicELBSchemeIncorrectInternetFacing.String() &&
-				newLoadBalancer.Scheme.String() == ClassicELBSchemeInternetFacing.String()) {
-				allErrs = append(allErrs,
-					field.Invalid(field.NewPath("spec", "controlPlaneLoadBalancer", "scheme"),
-						r.Spec.ControlPlaneLoadBalancer.Scheme, "field is immutable"),
-				)
-			}
+			allErrs = append(allErrs,
+				field.Invalid(field.NewPath("spec", "controlPlaneLoadBalancer", "scheme"),
+					r.Spec.ControlPlaneLoadBalancer.Scheme, "field is immutable"),
+			)
 		}
 		// The name must be defined when the AWSCluster is created. If it is not defined,
 		// then the controller generates a default name at runtime, but does not store it,
@@ -116,16 +115,16 @@ func (r *AWSCluster) ValidateUpdate(old runtime.Object) error {
 					r.Spec.ControlPlaneLoadBalancer.Name, "field is immutable"),
 			)
 		}
+	}
 
-		// Block the update for HealthCheckProtocol :
-		// - if it was not set in old spec but added in new spec
-		// - if it was set in old spec but changed in new spec
-		if !cmp.Equal(newLoadBalancer.HealthCheckProtocol, existingLoadBalancer.HealthCheckProtocol) {
-			allErrs = append(allErrs,
-				field.Invalid(field.NewPath("spec", "controlPlaneLoadBalancer", "healthCheckProtocol"),
-					newLoadBalancer.HealthCheckProtocol, "field is immutable once set"),
-			)
-		}
+	// Block the update for HealthCheckProtocol :
+	// - if it was not set in old spec but added in new spec
+	// - if it was set in old spec but changed in new spec
+	if !cmp.Equal(newLoadBalancer.HealthCheckProtocol, existingLoadBalancer.HealthCheckProtocol) {
+		allErrs = append(allErrs,
+			field.Invalid(field.NewPath("spec", "controlPlaneLoadBalancer", "healthCheckProtocol"),
+				newLoadBalancer.HealthCheckProtocol, "field is immutable once set"),
+		)
 	}
 
 	if !cmp.Equal(oldC.Spec.ControlPlaneEndpoint, clusterv1.APIEndpoint{}) &&

diff --git a/api/v1beta2/awscluster_webhook_test.go b/api/v1beta2/awscluster_webhook_test.go
@@ -52,29 +52,7 @@ func TestAWSCluster_ValidateCreate(t *testing.T) {
 	}{
 		// The SSHKeyName tests were moved to sshkeyname_test.go
 		{
-			name: "Default nil scheme to `internet-facing`",
-			cluster: &AWSCluster{
-				Spec: AWSClusterSpec{},
-			},
-			expect: func(g *WithT, res *AWSLoadBalancerSpec) {
-				g.Expect(res.Scheme.String(), ClassicELBSchemeInternetFacing.String())
-			},
-			wantErr: false,
-		},
-		{
-			name: "Internet-facing ELB scheme is defaulted to internet-facing during creation",
-			cluster: &AWSCluster{
-				Spec: AWSClusterSpec{
-					ControlPlaneLoadBalancer: &AWSLoadBalancerSpec{Scheme: &ClassicELBSchemeIncorrectInternetFacing},
-				},
-			},
-			expect: func(g *WithT, res *AWSLoadBalancerSpec) {
-				g.Expect(res.Scheme.String(), ClassicELBSchemeInternetFacing.String())
-			},
-			wantErr: false,
-		},
-		{
-			name: "Supported schemes are 'internet-facing, Internet-facing, internal, or nil', rest will be rejected",
+			name: "Supported schemes are 'internet-facing, internal, or nil', rest will be rejected",
 			cluster: &AWSCluster{
 				Spec: AWSClusterSpec{
 					ControlPlaneLoadBalancer: &AWSLoadBalancerSpec{Scheme: &unsupportedIncorrectScheme},

diff --git a/api/v1beta2/defaults.go b/api/v1beta2/defaults.go
@@ -60,12 +60,8 @@ func SetDefaults_AWSClusterSpec(s *AWSClusterSpec) { //nolint:golint,stylecheck
 			Name: AWSClusterControllerIdentityName,
 		}
 	}
-
-	// 	If ELB scheme is set to Internet-facing due to an API bug in versions > v0.6.6 and v0.7.0, default it to internet-facing.
 	if s.ControlPlaneLoadBalancer == nil {
 		s.ControlPlaneLoadBalancer = &AWSLoadBalancerSpec{Scheme: &ClassicELBSchemeInternetFacing}
-	} else if s.ControlPlaneLoadBalancer.Scheme != nil && s.ControlPlaneLoadBalancer.Scheme.String() == ClassicELBSchemeIncorrectInternetFacing.String() {
-		s.ControlPlaneLoadBalancer.Scheme = &ClassicELBSchemeInternetFacing
 	}
 }
 

diff --git a/api/v1beta2/network_types.go b/api/v1beta2/network_types.go
@@ -42,9 +42,6 @@ var (
 	// ClassicELBSchemeInternal defines an internal-only facing
 	// load balancer internal to an ELB.
 	ClassicELBSchemeInternal = ClassicELBScheme("internal")
-
-	// ClassicELBSchemeIncorrectInternetFacing was inaccurately used to define an internet-facing LB in v0.6 releases > v0.6.6 and v0.7.0 release.
-	ClassicELBSchemeIncorrectInternetFacing = ClassicELBScheme("Internet-facing")
 )
 
 func (e ClassicELBScheme) String() string {

diff --git a/pkg/cloud/services/elb/loadbalancer.go b/pkg/cloud/services/elb/loadbalancer.go
@@ -54,15 +54,6 @@ const maxELBsDescribeTagsRequest = 20
 func (s *Service) ReconcileLoadbalancers() error {
 	s.scope.Debug("Reconciling load balancers")
 
-	// If ELB scheme is set to Internet-facing due to an API bug in versions > v0.6.6 and v0.7.0, change it to internet-facing and patch.
-	if s.scope.ControlPlaneLoadBalancerScheme().String() == infrav1.ClassicELBSchemeIncorrectInternetFacing.String() {
-		s.scope.ControlPlaneLoadBalancer().Scheme = &infrav1.ClassicELBSchemeInternetFacing
-		if err := s.scope.PatchObject(); err != nil {
-			return err
-		}
-		s.scope.Trace("Patched control plane load balancer scheme")
-	}
-
 	// Generate a default control plane load balancer name. The load balancer name cannot be
 	// generated by the defaulting webhook, because it is derived from the cluster name, and that
 	// name is undefined at defaulting time when generateName is used.