From 512922d27c99c9e12f5c8ad3549d18eebac76feb Mon Sep 17 00:00:00 2001 From: Ankita Swamy Date: Tue, 1 Feb 2022 11:58:23 +0530 Subject: [PATCH] Use TCP instead of SSL for ELB health checks --- api/v1alpha3/awscluster_conversion.go | 1 + api/v1alpha3/zz_generated.conversion.go | 1 + api/v1alpha4/awscluster_conversion.go | 1 + api/v1alpha4/zz_generated.conversion.go | 1 + api/v1beta1/awscluster_types.go | 5 ++ api/v1beta1/awscluster_webhook.go | 10 +++ api/v1beta1/awscluster_webhook_test.go | 70 +++++++++++++++---- api/v1beta1/network_types.go | 4 ++ api/v1beta1/zz_generated.deepcopy.go | 5 ++ ...tructure.cluster.x-k8s.io_awsclusters.yaml | 4 ++ ....cluster.x-k8s.io_awsclustertemplates.yaml | 5 ++ pkg/cloud/services/elb/loadbalancer.go | 10 ++- pkg/cloud/services/elb/loadbalancer_test.go | 36 ++++++++-- .../limit-az/kustomization.yaml | 1 + .../patches/elb-health-check-protocol.yaml | 8 +++ 15 files changed, 141 insertions(+), 21 deletions(-) create mode 100644 test/e2e/data/infrastructure-aws/kustomize_sources/limit-az/patches/elb-health-check-protocol.yaml diff --git a/api/v1alpha3/awscluster_conversion.go b/api/v1alpha3/awscluster_conversion.go index 96694aa3b7..abd18f90b6 100644 --- 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 index 753b6074b2..edff3fd470 100644 --- a/api/v1alpha3/zz_generated.conversion.go +++ b/api/v1alpha3/zz_generated.conversion.go @@ -1056,6 +1056,7 @@ func autoConvert_v1beta1_AWSLoadBalancerSpec_To_v1alpha3_AWSLoadBalancerSpec(in out.Scheme = (*ClassicELBScheme)(unsafe.Pointer(in.Scheme)) out.CrossZoneLoadBalancing = in.CrossZoneLoadBalancing out.Subnets = *(*[]string)(unsafe.Pointer(&in.Subnets)) + // WARNING: in.HealthCheckProtocol requires manual conversion: does not exist in peer-type out.AdditionalSecurityGroups = *(*[]string)(unsafe.Pointer(&in.AdditionalSecurityGroups)) return nil } diff --git a/api/v1alpha4/awscluster_conversion.go b/api/v1alpha4/awscluster_conversion.go index 82345a690a..983f9285cc 100644 --- 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 index 5f475d0d99..2d398170b2 100644 --- a/api/v1alpha4/zz_generated.conversion.go +++ b/api/v1alpha4/zz_generated.conversion.go @@ -1219,6 +1219,7 @@ func autoConvert_v1beta1_AWSLoadBalancerSpec_To_v1alpha4_AWSLoadBalancerSpec(in out.Scheme = (*ClassicELBScheme)(unsafe.Pointer(in.Scheme)) out.CrossZoneLoadBalancing = in.CrossZoneLoadBalancing out.Subnets = *(*[]string)(unsafe.Pointer(&in.Subnets)) + // WARNING: in.HealthCheckProtocol requires manual conversion: does not exist in peer-type out.AdditionalSecurityGroups = *(*[]string)(unsafe.Pointer(&in.AdditionalSecurityGroups)) return nil } diff --git a/api/v1beta1/awscluster_types.go b/api/v1beta1/awscluster_types.go index 70c3a030f8..984d366257 100644 --- 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 index 31d9301008..d827a2fe36 100644 --- 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 index ed776473a9..42614b3fb1 100644 --- a/api/v1beta1/awscluster_webhook_test.go +++ b/api/v1beta1/awscluster_webhook_test.go @@ -46,7 +46,7 @@ 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 { @@ -54,12 +54,8 @@ func TestAWSCluster_ValidateCreate(t *testing.T) { 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 index 46224ff8be..65c0fbfa52 100644 --- 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 index 4bc96f4d87..839659a1b0 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -558,6 +558,11 @@ func (in *AWSLoadBalancerSpec) DeepCopyInto(out *AWSLoadBalancerSpec) { *out = make([]string, len(*in)) copy(*out, *in) } + if in.HealthCheckProtocol != nil { + in, out := &in.HealthCheckProtocol, &out.HealthCheckProtocol + *out = new(ClassicELBProtocol) + **out = **in + } if in.AdditionalSecurityGroups != nil { in, out := &in.AdditionalSecurityGroups, &out.AdditionalSecurityGroups *out = make([]string, len(*in)) diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml index 5698d7af6d..248d52b686 100644 --- 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 index 9af2c5b64e..7d46209438 100644 --- 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 index 50252e597d..bb07da7575 100644 --- a/pkg/cloud/services/elb/loadbalancer.go +++ b/pkg/cloud/services/elb/loadbalancer.go @@ -431,7 +431,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, @@ -781,6 +781,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 index 556e392d89..416252dbee 100644 --- 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 index e5399ad3a5..fc5f76bee0 100644 --- 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 new file mode 100644 index 0000000000..42f5c9853f --- /dev/null +++ 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"