Merge pull request #3124 from Ankitasw/elb-health-check

k8s-ci-robot · web-flow · commit 2d87f07bd4fa · 2022-03-01T01:23:49.000-08:00
Add support for custom protocol for ELB health checks
diff --git a/api/v1alpha3/awscluster_conversion.go b/api/v1alpha3/awscluster_conversion.go
@@ -61,6 +61,7 @@ func (r *AWSCluster) ConvertTo(dstRaw conversion.Hub) error {
 // Assumes restored and dst are non-nil.
 func restoreControlPlaneLoadBalancer(restored, dst *infrav1.AWSLoadBalancerSpec) {
 	dst.Name = restored.Name
+	dst.HealthCheckProtocol = restored.HealthCheckProtocol
 }
 
 // ConvertFrom converts the v1beta1 AWSCluster receiver to a v1alpha3 AWSCluster.
diff --git a/api/v1alpha3/zz_generated.conversion.go b/api/v1alpha3/zz_generated.conversion.go
diff --git a/api/v1alpha4/awscluster_conversion.go b/api/v1alpha4/awscluster_conversion.go
@@ -52,6 +52,7 @@ func (src *AWSCluster) ConvertTo(dstRaw conversion.Hub) error {
 // Assumes restored and dst are non-nil.
 func restoreControlPlaneLoadBalancer(restored, dst *infrav1.AWSLoadBalancerSpec) {
 	dst.Name = restored.Name
+	dst.HealthCheckProtocol = restored.HealthCheckProtocol
 }
 
 // ConvertFrom converts the v1beta1 AWSCluster receiver to a v1alpha4 AWSCluster.
diff --git a/api/v1alpha4/zz_generated.conversion.go b/api/v1alpha4/zz_generated.conversion.go
diff --git a/api/v1beta1/awscluster_types.go b/api/v1beta1/awscluster_types.go
@@ -177,6 +177,11 @@ type AWSLoadBalancerSpec struct {
 	// +optional
 	Subnets []string `json:"subnets,omitempty"`
 
+	// HealthCheckProtocol sets the protocol type for classic ELB health check target
+	// default value is ClassicELBProtocolSSL
+	// +optional
+	HealthCheckProtocol *ClassicELBProtocol `json:"healthCheckProtocol,omitempty"`
+
 	// AdditionalSecurityGroups sets the security groups used by the load balancer. Expected to be security group IDs
 	// This is optional - if not provided new security groups will be created for the load balancer
 	// +optional
diff --git a/api/v1beta1/awscluster_webhook.go b/api/v1beta1/awscluster_webhook.go
@@ -115,6 +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 !reflect.DeepEqual(newLoadBalancer.HealthCheckProtocol, existingLoadBalancer.HealthCheckProtocol) {
+			allErrs = append(allErrs,
+				field.Invalid(field.NewPath("spec", "controlPlaneLoadBalancer", "healthCheckProtocol"),
+					newLoadBalancer.HealthCheckProtocol, "field is immutable once set"),
+			)
+		}
 	}
 
 	if !reflect.DeepEqual(oldC.Spec.ControlPlaneEndpoint, clusterv1.APIEndpoint{}) &&
diff --git a/api/v1beta1/awscluster_webhook_test.go b/api/v1beta1/awscluster_webhook_test.go
@@ -46,20 +46,16 @@ func TestAWSCluster_ValidateCreate(t *testing.T) {
 		name    string
 		cluster *AWSCluster
 		wantErr bool
-		expect  func(t *testing.T, res *AWSLoadBalancerSpec)
+		expect  func(g *WithT, res *AWSLoadBalancerSpec)
 	}{
 		// The SSHKeyName tests were moved to sshkeyname_test.go
 		{
 			name: "Default nil scheme to `internet-facing`",
 			cluster: &AWSCluster{
 				Spec: AWSClusterSpec{},
 			},
-			expect: func(t *testing.T, res *AWSLoadBalancerSpec) {
-				t.Helper()
-
-				if res.Scheme.String() != ClassicELBSchemeInternetFacing.String() {
-					t.Error("Expected internet-facing defaulting for nil loadbalancer schemes")
-				}
+			expect: func(g *WithT, res *AWSLoadBalancerSpec) {
+				g.Expect(res.Scheme.String(), ClassicELBSchemeInternetFacing.String())
 			},
 			wantErr: false,
 		},
@@ -70,12 +66,8 @@ func TestAWSCluster_ValidateCreate(t *testing.T) {
 					ControlPlaneLoadBalancer: &AWSLoadBalancerSpec{Scheme: &ClassicELBSchemeIncorrectInternetFacing},
 				},
 			},
-			expect: func(t *testing.T, res *AWSLoadBalancerSpec) {
-				t.Helper()
-
-				if res.Scheme.String() != ClassicELBSchemeInternetFacing.String() {
-					t.Error("Expected internet-facing defaulting for supported incorrect scheme: Internet-facing")
-				}
+			expect: func(g *WithT, res *AWSLoadBalancerSpec) {
+				g.Expect(res.Scheme.String(), ClassicELBSchemeInternetFacing.String())
 			},
 			wantErr: false,
 		},
@@ -131,7 +123,7 @@ func TestAWSCluster_ValidateCreate(t *testing.T) {
 				return err == nil
 			}, 10*time.Second).Should(Equal(true))
 
-			tt.expect(t, c.Spec.ControlPlaneLoadBalancer)
+			tt.expect(g, c.Spec.ControlPlaneLoadBalancer)
 		})
 	}
 }
@@ -368,6 +360,56 @@ func TestAWSCluster_ValidateUpdate(t *testing.T) {
 			},
 			wantErr: true,
 		},
+		{
+			name: "Should fail if controlPlaneLoadBalancer healthcheckprotocol is updated",
+			oldCluster: &AWSCluster{
+				Spec: AWSClusterSpec{
+					ControlPlaneLoadBalancer: &AWSLoadBalancerSpec{
+						HealthCheckProtocol: &ClassicELBProtocolTCP,
+					},
+				},
+			},
+			newCluster: &AWSCluster{
+				Spec: AWSClusterSpec{
+					ControlPlaneLoadBalancer: &AWSLoadBalancerSpec{
+						HealthCheckProtocol: &ClassicELBProtocolSSL,
+					},
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name: "Should pass if controlPlaneLoadBalancer healthcheckprotocol is same after update",
+			oldCluster: &AWSCluster{
+				Spec: AWSClusterSpec{
+					ControlPlaneLoadBalancer: &AWSLoadBalancerSpec{
+						HealthCheckProtocol: &ClassicELBProtocolTCP,
+					},
+				},
+			},
+			newCluster: &AWSCluster{
+				Spec: AWSClusterSpec{
+					ControlPlaneLoadBalancer: &AWSLoadBalancerSpec{
+						HealthCheckProtocol: &ClassicELBProtocolTCP,
+					},
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "Should fail if controlPlaneLoadBalancer healthcheckprotocol is changed to non-default if it was not set before update",
+			oldCluster: &AWSCluster{
+				Spec: AWSClusterSpec{},
+			},
+			newCluster: &AWSCluster{
+				Spec: AWSClusterSpec{
+					ControlPlaneLoadBalancer: &AWSLoadBalancerSpec{
+						HealthCheckProtocol: &ClassicELBProtocolTCP,
+					},
+				},
+			},
+			wantErr: true,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
diff --git a/api/v1beta1/network_types.go b/api/v1beta1/network_types.go
@@ -54,6 +54,10 @@ func (e ClassicELBScheme) String() string {
 // ClassicELBProtocol defines listener protocols for a classic load balancer.
 type ClassicELBProtocol string
 
+func (e ClassicELBProtocol) String() string {
+	return string(e)
+}
+
 var (
 	// ClassicELBProtocolTCP defines the ELB API string representing the TCP protocol.
 	ClassicELBProtocolTCP = ClassicELBProtocol("TCP")
diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go
diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml
@@ -1651,6 +1651,10 @@ spec:
                       registered instances in its Availability Zone only. \n Defaults
                       to false."
                     type: boolean
+                  healthCheckProtocol:
+                    description: HealthCheckProtocol sets the protocol type for classic
+                      ELB health check target default value is ClassicELBProtocolSSL
+                    type: string
                   name:
                     description: Name sets the name of the classic ELB load balancer.
                       As per AWS, the name must be unique within your set of load
diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml
@@ -474,6 +474,11 @@ spec:
                               registered instances in its Availability Zone only.
                               \n Defaults to false."
                             type: boolean
+                          healthCheckProtocol:
+                            description: HealthCheckProtocol sets the protocol type
+                              for classic ELB health check target default value is
+                              ClassicELBProtocolSSL
+                            type: string
                           name:
                             description: Name sets the name of the classic ELB load
                               balancer. As per AWS, the name must be unique within
diff --git a/pkg/cloud/services/elb/loadbalancer.go b/pkg/cloud/services/elb/loadbalancer.go
@@ -434,7 +434,7 @@ func (s *Service) getAPIServerClassicELBSpec(elbName string) (*infrav1.ClassicEL
 			},
 		},
 		HealthCheck: &infrav1.ClassicELBHealthCheck{
-			Target:             fmt.Sprintf("%v:%d", infrav1.ClassicELBProtocolSSL, 6443),
+			Target:             fmt.Sprintf("%v:%d", s.getHealthCheckELBProtocol(), 6443),
 			Interval:           10 * time.Second,
 			Timeout:            5 * time.Second,
 			HealthyThreshold:   5,
@@ -784,6 +784,14 @@ func (s *Service) reconcileELBTags(lb *infrav1.ClassicELB, desiredTags map[strin
 	return nil
 }
 
+func (s *Service) getHealthCheckELBProtocol() *infrav1.ClassicELBProtocol {
+	controlPlaneELB := s.scope.ControlPlaneLoadBalancer()
+	if controlPlaneELB != nil && controlPlaneELB.HealthCheckProtocol != nil {
+		return controlPlaneELB.HealthCheckProtocol
+	}
+	return &infrav1.ClassicELBProtocolSSL
+}
+
 func fromSDKTypeToClassicELB(v *elb.LoadBalancerDescription, attrs *elb.LoadBalancerAttributes, tags []*elb.Tag) *infrav1.ClassicELB {
 	res := &infrav1.ClassicELB{
 		Name:             aws.StringValue(v.LoadBalancerName),
diff --git a/pkg/cloud/services/elb/loadbalancer_test.go b/pkg/cloud/services/elb/loadbalancer_test.go
@@ -28,6 +28,7 @@ import (
 	"github.com/aws/aws-sdk-go/service/elb"
 	rgapi "github.com/aws/aws-sdk-go/service/resourcegroupstaggingapi"
 	"github.com/golang/mock/gomock"
+	. "github.com/onsi/gomega"
 	"github.com/pkg/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -154,13 +155,13 @@ func TestGetAPIServerClassicELBSpec_ControlPlaneLoadBalancer(t *testing.T) {
 		name   string
 		lb     *infrav1.AWSLoadBalancerSpec
 		mocks  func(m *mock_ec2iface.MockEC2APIMockRecorder)
-		expect func(t *testing.T, res *infrav1.ClassicELB)
+		expect func(t *testing.T, g *WithT, res *infrav1.ClassicELB)
 	}{
 		{
 			name:  "nil load balancer config",
 			lb:    nil,
 			mocks: func(m *mock_ec2iface.MockEC2APIMockRecorder) {},
-			expect: func(t *testing.T, res *infrav1.ClassicELB) {
+			expect: func(t *testing.T, g *WithT, res *infrav1.ClassicELB) {
 				t.Helper()
 				if res.Attributes.CrossZoneLoadBalancing {
 					t.Error("Expected load balancer not to have cross-zone load balancing enabled")
@@ -173,7 +174,7 @@ func TestGetAPIServerClassicELBSpec_ControlPlaneLoadBalancer(t *testing.T) {
 				CrossZoneLoadBalancing: true,
 			},
 			mocks: func(m *mock_ec2iface.MockEC2APIMockRecorder) {},
-			expect: func(t *testing.T, res *infrav1.ClassicELB) {
+			expect: func(t *testing.T, g *WithT, res *infrav1.ClassicELB) {
 				t.Helper()
 				if !res.Attributes.CrossZoneLoadBalancing {
 					t.Error("Expected load balancer to have cross-zone load balancing enabled")
@@ -205,7 +206,7 @@ func TestGetAPIServerClassicELBSpec_ControlPlaneLoadBalancer(t *testing.T) {
 						},
 					}, nil)
 			},
-			expect: func(t *testing.T, res *infrav1.ClassicELB) {
+			expect: func(t *testing.T, g *WithT, res *infrav1.ClassicELB) {
 				t.Helper()
 				if len(res.SubnetIDs) != 2 {
 					t.Errorf("Expected load balancer to be configured for 2 subnets, got %v", len(res.SubnetIDs))
@@ -221,17 +222,40 @@ func TestGetAPIServerClassicELBSpec_ControlPlaneLoadBalancer(t *testing.T) {
 				AdditionalSecurityGroups: []string{"sg-00001", "sg-00002"},
 			},
 			mocks: func(m *mock_ec2iface.MockEC2APIMockRecorder) {},
-			expect: func(t *testing.T, res *infrav1.ClassicELB) {
+			expect: func(t *testing.T, g *WithT, res *infrav1.ClassicELB) {
 				t.Helper()
 				if len(res.SecurityGroupIDs) != 3 {
 					t.Errorf("Expected load balancer to be configured for 3 security groups, got %v", len(res.SecurityGroupIDs))
 				}
 			},
 		},
+		{
+			name: "Should create load balancer spec if elb health check protocol specified in config",
+			lb: &infrav1.AWSLoadBalancerSpec{
+				HealthCheckProtocol: &infrav1.ClassicELBProtocolTCP,
+			},
+			mocks: func(m *mock_ec2iface.MockEC2APIMockRecorder) {},
+			expect: func(t *testing.T, g *WithT, res *infrav1.ClassicELB) {
+				t.Helper()
+				expectedTarget := fmt.Sprintf("%v:%d", infrav1.ClassicELBProtocolTCP, 6443)
+				g.Expect(expectedTarget, res.HealthCheck.Target)
+			},
+		},
+		{
+			name:  "Should create load balancer spec with default elb health check protocol",
+			lb:    &infrav1.AWSLoadBalancerSpec{},
+			mocks: func(m *mock_ec2iface.MockEC2APIMockRecorder) {},
+			expect: func(t *testing.T, g *WithT, res *infrav1.ClassicELB) {
+				t.Helper()
+				expectedTarget := fmt.Sprintf("%v:%d", infrav1.ClassicELBProtocolTCP, 6443)
+				g.Expect(expectedTarget, res.HealthCheck.Target)
+			},
+		},
 	}
 
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
+			g := NewWithT(t)
 			mockCtrl := gomock.NewController(t)
 			defer mockCtrl.Finish()
 			ec2Mock := mock_ec2iface.NewMockEC2API(mockCtrl)
@@ -270,7 +294,7 @@ func TestGetAPIServerClassicELBSpec_ControlPlaneLoadBalancer(t *testing.T) {
 				t.Fatal(err)
 			}
 
-			tc.expect(t, spec)
+			tc.expect(t, g, spec)
 		})
 	}
 }
diff --git a/test/e2e/data/infrastructure-aws/kustomize_sources/limit-az/kustomization.yaml b/test/e2e/data/infrastructure-aws/kustomize_sources/limit-az/kustomization.yaml
@@ -2,3 +2,4 @@ resources:
   - ../default
 patchesStrategicMerge:
   - patches/limit-az.yaml
+  - patches/elb-health-check-protocol.yaml
diff --git a/test/e2e/data/infrastructure-aws/kustomize_sources/limit-az/patches/elb-health-check-protocol.yaml b/test/e2e/data/infrastructure-aws/kustomize_sources/limit-az/patches/elb-health-check-protocol.yaml
@@ -0,0 +1,8 @@
+---
+apiVersion: infrastructure.cluster.x-k8s.io/v1beta1
+kind: AWSCluster
+metadata:
+  name: "${CLUSTER_NAME}"
+spec:
+  controlPlaneLoadBalancer:
+    healthCheckProtocol: "TCP"

Original file line number	Diff line number	Diff line change
`@@ -61,6 +61,7 @@ func (r *AWSCluster) ConvertTo(dstRaw conversion.Hub) error {`
`61`	`61`	`// Assumes restored and dst are non-nil.`
`62`	`62`	`func restoreControlPlaneLoadBalancer(restored, dst *infrav1.AWSLoadBalancerSpec) {`
`63`	`63`	`dst.Name = restored.Name`
	`64`	`+ dst.HealthCheckProtocol = restored.HealthCheckProtocol`
`64`	`65`	`}`
`65`	`66`
`66`	`67`	`// ConvertFrom converts the v1beta1 AWSCluster receiver to a v1alpha3 AWSCluster.`
Original file line number	Diff line number	Diff line change
`@@ -52,6 +52,7 @@ func (src *AWSCluster) ConvertTo(dstRaw conversion.Hub) error {`
`52`	`52`	`// Assumes restored and dst are non-nil.`
`53`	`53`	`func restoreControlPlaneLoadBalancer(restored, dst *infrav1.AWSLoadBalancerSpec) {`
`54`	`54`	`dst.Name = restored.Name`
	`55`	`+ dst.HealthCheckProtocol = restored.HealthCheckProtocol`
`55`	`56`	`}`
`56`	`57`
`57`	`58`	`// ConvertFrom converts the v1beta1 AWSCluster receiver to a v1alpha4 AWSCluster.`
Original file line number	Diff line number	Diff line change
`@@ -115,6 +115,16 @@ func (r *AWSCluster) ValidateUpdate(old runtime.Object) error {`
`115`	`115`	`r.Spec.ControlPlaneLoadBalancer.Name, "field is immutable"),`
`116`	`116`	`)`
`117`	`117`	`}`
	`118`	`+`
	`119`	`+ // Block the update for HealthCheckProtocol :`
	`120`	`+ // - if it was not set in old spec but added in new spec`
	`121`	`+ // - if it was set in old spec but changed in new spec`
	`122`	`+ if !reflect.DeepEqual(newLoadBalancer.HealthCheckProtocol, existingLoadBalancer.HealthCheckProtocol) {`
	`123`	`+ allErrs = append(allErrs,`
	`124`	`+ field.Invalid(field.NewPath("spec", "controlPlaneLoadBalancer", "healthCheckProtocol"),`
	`125`	`+ newLoadBalancer.HealthCheckProtocol, "field is immutable once set"),`
	`126`	`+ )`
	`127`	`+ }`
`118`	`128`	`}`
`119`	`129`
`120`	`130`	`if !reflect.DeepEqual(oldC.Spec.ControlPlaneEndpoint, clusterv1.APIEndpoint{}) &&`