Merge pull request #5346 from richardcase/classic_elb_130_fix

🐛fix: classic elb fix for TLS issues

Author: k8s-ci-robot

api/v1beta2/awscluster_webhook.go

@@ -33,6 +33,11 @@ import (
 	"sigs.k8s.io/cluster-api/util/annotations"
 )
 
+const (
+	warningClassicELB                = "%s load balancer is using a classic elb which is deprecated & support will be removed in a future release, please consider using another type of load balancer instead"
+	warningHealthCheckProtocolNotSet = "healthcheck protocol is not set, the default value has changed from SSL to TCP. Health checks for existing clusters will be updated to TCP"
+)
+
 // log is for logging in this package.
 var _ = ctrl.Log.WithName("awscluster-resource")
 
@@ -53,15 +58,23 @@ var (
 // ValidateCreate implements webhook.Validator so a webhook will be registered for the type.
 func (r *AWSCluster) ValidateCreate() (admission.Warnings, error) {
 	var allErrs field.ErrorList
+	var allWarnings admission.Warnings
 
 	allErrs = append(allErrs, r.Spec.Bastion.Validate()...)
 	allErrs = append(allErrs, r.validateSSHKeyName()...)
 	allErrs = append(allErrs, r.Spec.AdditionalTags.Validate()...)
 	allErrs = append(allErrs, r.Spec.S3Bucket.Validate()...)
 	allErrs = append(allErrs, r.validateNetwork()...)
-	allErrs = append(allErrs, r.validateControlPlaneLBs()...)
 
-	return nil, aggregateObjErrors(r.GroupVersionKind().GroupKind(), r.Name, allErrs)
+	warnings, errs := r.validateControlPlaneLBs()
+	if len(errs) > 0 {
+		allErrs = append(allErrs, errs...)
+	}
+	if len(warnings) > 0 {
+		allWarnings = append(allWarnings, warnings...)
+	}
+
+	return allWarnings, aggregateObjErrors(r.GroupVersionKind().GroupKind(), r.Name, allErrs)
 }
 
 // ValidateDelete implements webhook.Validator so a webhook will be registered for the type.
@@ -72,6 +85,7 @@ func (r *AWSCluster) ValidateDelete() (admission.Warnings, error) {
 // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type.
 func (r *AWSCluster) ValidateUpdate(old runtime.Object) (admission.Warnings, error) {
 	var allErrs field.ErrorList
+	var allWarnings admission.Warnings
 
 	allErrs = append(allErrs, r.validateGCTasksAnnotation()...)
 
@@ -139,7 +153,23 @@ func (r *AWSCluster) ValidateUpdate(old runtime.Object) (admission.Warnings, err
 	allErrs = append(allErrs, r.Spec.AdditionalTags.Validate()...)
 	allErrs = append(allErrs, r.Spec.S3Bucket.Validate()...)
 
-	return nil, aggregateObjErrors(r.GroupVersionKind().GroupKind(), r.Name, allErrs)
+	if r.Spec.ControlPlaneLoadBalancer != nil {
+		if r.Spec.ControlPlaneLoadBalancer.LoadBalancerType == LoadBalancerTypeClassic {
+			allWarnings = append(allWarnings, fmt.Sprintf(warningClassicELB, "primary control plane"))
+		}
+	}
+
+	if r.Spec.SecondaryControlPlaneLoadBalancer != nil {
+		if r.Spec.SecondaryControlPlaneLoadBalancer.LoadBalancerType == LoadBalancerTypeClassic {
+			allWarnings = append(allWarnings, fmt.Sprintf(warningClassicELB, "secondary control plane"))
+		}
+	}
+
+	if r.Spec.ControlPlaneLoadBalancer == nil || r.Spec.ControlPlaneLoadBalancer.HealthCheckProtocol == nil {
+		allWarnings = append(allWarnings, fmt.Sprintf("%s. Existing load balancers will be updates", warningHealthCheckProtocolNotSet))
+	}
+
+	return allWarnings, aggregateObjErrors(r.GroupVersionKind().GroupKind(), r.Name, allErrs)
 }
 
 func (r *AWSCluster) validateControlPlaneLoadBalancerUpdate(oldlb, newlb *AWSLoadBalancerSpec) field.ErrorList {
@@ -184,11 +214,13 @@ func (r *AWSCluster) validateControlPlaneLoadBalancerUpdate(oldlb, newlb *AWSLoa
 		// Block the update for Protocol :
 		// - 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(newlb.HealthCheckProtocol, oldlb.HealthCheckProtocol) {
-			allErrs = append(allErrs,
-				field.Invalid(field.NewPath("spec", "controlPlaneLoadBalancer", "healthCheckProtocol"),
-					newlb.HealthCheckProtocol, "field is immutable once set"),
-			)
+		if oldlb.LoadBalancerType != LoadBalancerTypeClassic {
+			if !cmp.Equal(newlb.HealthCheckProtocol, oldlb.HealthCheckProtocol) {
+				allErrs = append(allErrs,
+					field.Invalid(field.NewPath("spec", "controlPlaneLoadBalancer", "healthCheckProtocol"),
+						newlb.HealthCheckProtocol, "field is immutable once set"),
+				)
+			}
 		}
 	}
 
@@ -301,8 +333,21 @@ func (r *AWSCluster) validateNetwork() field.ErrorList {
 	return allErrs
 }
 
-func (r *AWSCluster) validateControlPlaneLBs() field.ErrorList {
+func (r *AWSCluster) validateControlPlaneLBs() (admission.Warnings, field.ErrorList) {
 	var allErrs field.ErrorList
+	var allWarnings admission.Warnings
+
+	if r.Spec.ControlPlaneLoadBalancer != nil && r.Spec.ControlPlaneLoadBalancer.LoadBalancerType == LoadBalancerTypeClassic {
+		allWarnings = append(allWarnings, fmt.Sprintf(warningClassicELB, "primary control plane"))
+
+		if r.Spec.ControlPlaneLoadBalancer.HealthCheckProtocol == nil {
+			allWarnings = append(allWarnings, warningHealthCheckProtocolNotSet)
+		}
+
+		if r.Spec.ControlPlaneLoadBalancer.HealthCheckProtocol != nil && *r.Spec.ControlPlaneLoadBalancer.HealthCheckProtocol == ELBProtocolSSL {
+			allWarnings = append(allWarnings, "loadbalancer is using a classic elb with SSL health check, this causes issues with ciper suites with kubernetes v1.30+")
+		}
+	}
 
 	// If the secondary is defined, check that the name is not empty and different from the primary.
 	// Also, ensure that the secondary load balancer is an NLB
@@ -322,6 +367,9 @@ func (r *AWSCluster) validateControlPlaneLBs() field.ErrorList {
 		if r.Spec.SecondaryControlPlaneLoadBalancer.LoadBalancerType != LoadBalancerTypeNLB {
 			allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "secondaryControlPlaneLoadBalancer", "loadBalancerType"), r.Spec.SecondaryControlPlaneLoadBalancer.LoadBalancerType, "secondary control plane load balancer must be a Network Load Balancer"))
 		}
+		if r.Spec.SecondaryControlPlaneLoadBalancer.LoadBalancerType == LoadBalancerTypeClassic {
+			allWarnings = append(allWarnings, fmt.Sprintf(warningClassicELB, "secondary control plane"))
+		}
 	}
 
 	// Additional listeners are only supported for NLBs.
@@ -378,7 +426,7 @@ func (r *AWSCluster) validateControlPlaneLBs() field.ErrorList {
 		}
 	}
 
-	return allErrs
+	return allWarnings, allErrs
 }
 
 func (r *AWSCluster) validateIngressRule(rule IngressRule) field.ErrorList {

api/v1beta2/awscluster_webhook_test.go

@@ -32,12 +32,12 @@ import (
 
 	"sigs.k8s.io/cluster-api-provider-aws/v2/feature"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
-	utildefaulting "sigs.k8s.io/cluster-api/util/defaulting"
+	"sigs.k8s.io/cluster-api/util/defaulting"
 )
 
 func TestAWSClusterDefault(t *testing.T) {
 	cluster := &AWSCluster{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}}
-	t.Run("for AWSCluster", utildefaulting.DefaultValidateTest(cluster))
+	t.Run("for AWSCluster", defaultValidateTest(cluster, true))
 	cluster.Default()
 	g := NewWithT(t)
 	g.Expect(cluster.Spec.IdentityRef).NotTo(BeNil())
@@ -699,7 +699,7 @@ func TestAWSClusterValidateCreate(t *testing.T) {
 }
 
 func TestAWSClusterValidateUpdate(t *testing.T) {
-	var tests = []struct {
+	tests := []struct {
 		name       string
 		oldCluster *AWSCluster
 		newCluster *AWSCluster
@@ -967,23 +967,45 @@ func TestAWSClusterValidateUpdate(t *testing.T) {
 			wantErr: true,
 		},
 		{
-			name: "Should fail if controlPlaneLoadBalancer healthcheckprotocol is updated",
+			name: "Should fail if controlPlaneLoadBalancer healthcheckprotocol is updated and not classic elb",
 			oldCluster: &AWSCluster{
 				Spec: AWSClusterSpec{
 					ControlPlaneLoadBalancer: &AWSLoadBalancerSpec{
+						LoadBalancerType:    LoadBalancerTypeNLB,
 						HealthCheckProtocol: &ELBProtocolTCP,
 					},
 				},
 			},
 			newCluster: &AWSCluster{
 				Spec: AWSClusterSpec{
 					ControlPlaneLoadBalancer: &AWSLoadBalancerSpec{
+						LoadBalancerType:    LoadBalancerTypeNLB,
 						HealthCheckProtocol: &ELBProtocolSSL,
 					},
 				},
 			},
 			wantErr: true,
 		},
+		{
+			name: "Should pass if controlPlaneLoadBalancer healthcheckprotocol is updated and a classic elb",
+			oldCluster: &AWSCluster{
+				Spec: AWSClusterSpec{
+					ControlPlaneLoadBalancer: &AWSLoadBalancerSpec{
+						LoadBalancerType:    LoadBalancerTypeClassic,
+						HealthCheckProtocol: &ELBProtocolSSL,
+					},
+				},
+			},
+			newCluster: &AWSCluster{
+				Spec: AWSClusterSpec{
+					ControlPlaneLoadBalancer: &AWSLoadBalancerSpec{
+						LoadBalancerType:    LoadBalancerTypeClassic,
+						HealthCheckProtocol: &ELBProtocolTCP,
+					},
+				},
+			},
+			wantErr: false,
+		},
 		{
 			name: "Should pass if old secondary lb is absent",
 			oldCluster: &AWSCluster{
@@ -1017,19 +1039,43 @@ func TestAWSClusterValidateUpdate(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name: "Should fail if controlPlaneLoadBalancer healthcheckprotocol is changed to non-default if it was not set before update",
+			name: "Should fail if controlPlaneLoadBalancer healthcheckprotocol is changed to non-default if it was not set before update for non-classic elb",
 			oldCluster: &AWSCluster{
-				Spec: AWSClusterSpec{},
+				Spec: AWSClusterSpec{
+					ControlPlaneLoadBalancer: &AWSLoadBalancerSpec{
+						LoadBalancerType: LoadBalancerTypeNLB,
+					},
+				},
 			},
 			newCluster: &AWSCluster{
 				Spec: AWSClusterSpec{
 					ControlPlaneLoadBalancer: &AWSLoadBalancerSpec{
+						LoadBalancerType:    LoadBalancerTypeNLB,
 						HealthCheckProtocol: &ELBProtocolTCP,
 					},
 				},
 			},
 			wantErr: true,
 		},
+		{
+			name: "Should pass if controlPlaneLoadBalancer healthcheckprotocol is changed to non-default if it was not set before update for classic elb",
+			oldCluster: &AWSCluster{
+				Spec: AWSClusterSpec{
+					ControlPlaneLoadBalancer: &AWSLoadBalancerSpec{
+						LoadBalancerType: LoadBalancerTypeClassic,
+					},
+				},
+			},
+			newCluster: &AWSCluster{
+				Spec: AWSClusterSpec{
+					ControlPlaneLoadBalancer: &AWSLoadBalancerSpec{
+						LoadBalancerType:    LoadBalancerTypeNLB,
+						HealthCheckProtocol: &ELBProtocolTCP,
+					},
+				},
+			},
+			wantErr: false,
+		},
 		{
 			name: "correct GC tasks annotation",
 			oldCluster: &AWSCluster{
@@ -1357,3 +1403,48 @@ func TestAWSClusterDefaultAllowedCIDRBlocks(t *testing.T) {
 		})
 	}
 }
+
+// defaultValidateTest returns a new testing function to be used in tests to
+// make sure defaulting webhooks also pass validation tests on create,
+// update and delete.
+// NOTE: This is a copy of the DefaultValidateTest function in the cluster-api
+// package, but it has been modified to allow warnings to be returned.
+func defaultValidateTest(object defaulting.DefaultingValidator, allowWarnings bool) func(*testing.T) {
+	return func(t *testing.T) {
+		t.Helper()
+
+		createCopy := object.DeepCopyObject().(defaulting.DefaultingValidator)
+		updateCopy := object.DeepCopyObject().(defaulting.DefaultingValidator)
+		deleteCopy := object.DeepCopyObject().(defaulting.DefaultingValidator)
+		defaultingUpdateCopy := updateCopy.DeepCopyObject().(defaulting.DefaultingValidator)
+
+		t.Run("validate-on-create", func(t *testing.T) {
+			g := NewWithT(t)
+			createCopy.Default()
+			warnings, err := createCopy.ValidateCreate()
+			g.Expect(err).ToNot(HaveOccurred())
+			if !allowWarnings {
+				g.Expect(warnings).To(BeEmpty())
+			}
+		})
+		t.Run("validate-on-update", func(t *testing.T) {
+			g := NewWithT(t)
+			defaultingUpdateCopy.Default()
+			updateCopy.Default()
+			warnings, err := defaultingUpdateCopy.ValidateUpdate(updateCopy)
+			g.Expect(err).ToNot(HaveOccurred())
+			if !allowWarnings {
+				g.Expect(warnings).To(BeEmpty())
+			}
+		})
+		t.Run("validate-on-delete", func(t *testing.T) {
+			g := NewWithT(t)
+			deleteCopy.Default()
+			warnings, err := deleteCopy.ValidateDelete()
+			g.Expect(err).ToNot(HaveOccurred())
+			if !allowWarnings {
+				g.Expect(warnings).To(BeEmpty())
+			}
+		})
+	}
+}