Skip to content

Commit 9a459b0

Browse files
richardcaserichardcase
committed
feat: classic elb fix for TLS issues
There is an issue when creating clusters (or upgrading clusters) with kubernetes versions v1.30+ and using a classic elb with an SSL health check (which the default for new clusters). The problem is that Kubernetes v1.30+ switched to Go 1.22 which removed the RSA ciphers. This then causes the ELB health check to fail. There are a number of workarounds including manually specifying the cipher suites to use for the api server. This commit does the following: - Adds warnings to the AWSCluster webhook to alert users that: - their cluster is using a classic elb and this is now deprecated - their cluster is using the default health check protocol which warnings previously SSL and that now the default has changed to TCP. - Will update the health check to TCP if the load balancer is "classic" and the health check protocol is not set. Signed-off-by: Richard Case <[email protected]>
1 parent ac3cc61 commit 9a459b0

File tree

15 files changed

+560
-38
lines changed

15 files changed

+560
-38
lines changed

‎api/v1beta2/awscluster_webhook.go

Lines changed: 51 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,11 @@ import (
3333
"sigs.k8s.io/cluster-api/util/annotations"
3434
)
3535

36+
const (
37+
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"
38+
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"
39+
)
40+
3641
// log is for logging in this package.
3742
var _ = ctrl.Log.WithName("awscluster-resource")
3843

@@ -53,15 +58,23 @@ var (
5358
// ValidateCreate implements webhook.Validator so a webhook will be registered for the type.
5459
func (r *AWSCluster) ValidateCreate() (admission.Warnings, error) {
5560
var allErrs field.ErrorList
61+
var allWarnings admission.Warnings
5662

5763
allErrs = append(allErrs, r.Spec.Bastion.Validate()...)
5864
allErrs = append(allErrs, r.validateSSHKeyName()...)
5965
allErrs = append(allErrs, r.Spec.AdditionalTags.Validate()...)
6066
allErrs = append(allErrs, r.Spec.S3Bucket.Validate()...)
6167
allErrs = append(allErrs, r.validateNetwork()...)
62-
allErrs = append(allErrs, r.validateControlPlaneLBs()...)
6368

64-
return nil, aggregateObjErrors(r.GroupVersionKind().GroupKind(), r.Name, allErrs)
69+
warnings, errs := r.validateControlPlaneLBs()
70+
if len(errs) > 0 {
71+
allErrs = append(allErrs, errs...)
72+
}
73+
if len(warnings) > 0 {
74+
allWarnings = append(allWarnings, warnings...)
75+
}
76+
77+
return allWarnings, aggregateObjErrors(r.GroupVersionKind().GroupKind(), r.Name, allErrs)
6578
}
6679

6780
// ValidateDelete implements webhook.Validator so a webhook will be registered for the type.
@@ -72,6 +85,7 @@ func (r *AWSCluster) ValidateDelete() (admission.Warnings, error) {
7285
// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type.
7386
func (r *AWSCluster) ValidateUpdate(old runtime.Object) (admission.Warnings, error) {
7487
var allErrs field.ErrorList
88+
var allWarnings admission.Warnings
7589

7690
allErrs = append(allErrs, r.validateGCTasksAnnotation()...)
7791

@@ -139,7 +153,23 @@ func (r *AWSCluster) ValidateUpdate(old runtime.Object) (admission.Warnings, err
139153
allErrs = append(allErrs, r.Spec.AdditionalTags.Validate()...)
140154
allErrs = append(allErrs, r.Spec.S3Bucket.Validate()...)
141155

142-
return nil, aggregateObjErrors(r.GroupVersionKind().GroupKind(), r.Name, allErrs)
156+
if r.Spec.ControlPlaneLoadBalancer != nil {
157+
if r.Spec.ControlPlaneLoadBalancer.LoadBalancerType == LoadBalancerTypeClassic {
158+
allWarnings = append(allWarnings, fmt.Sprintf(warningClassicELB, "primary control plane"))
159+
}
160+
}
161+
162+
if r.Spec.SecondaryControlPlaneLoadBalancer != nil {
163+
if r.Spec.SecondaryControlPlaneLoadBalancer.LoadBalancerType == LoadBalancerTypeClassic {
164+
allWarnings = append(allWarnings, fmt.Sprintf(warningClassicELB, "secondary control plane"))
165+
}
166+
}
167+
168+
if r.Spec.ControlPlaneLoadBalancer == nil || r.Spec.ControlPlaneLoadBalancer.HealthCheckProtocol == nil {
169+
allWarnings = append(allWarnings, fmt.Sprintf("%s. Existing load balancers will be updates", warningHealthCheckProtocolNotSet))
170+
}
171+
172+
return allWarnings, aggregateObjErrors(r.GroupVersionKind().GroupKind(), r.Name, allErrs)
143173
}
144174

145175
func (r *AWSCluster) validateControlPlaneLoadBalancerUpdate(oldlb, newlb *AWSLoadBalancerSpec) field.ErrorList {
@@ -180,16 +210,6 @@ func (r *AWSCluster) validateControlPlaneLoadBalancerUpdate(oldlb, newlb *AWSLoa
180210
newlb.Name, "field is immutable"),
181211
)
182212
}
183-
184-
// Block the update for Protocol :
185-
// - if it was not set in old spec but added in new spec
186-
// - if it was set in old spec but changed in new spec
187-
if !cmp.Equal(newlb.HealthCheckProtocol, oldlb.HealthCheckProtocol) {
188-
allErrs = append(allErrs,
189-
field.Invalid(field.NewPath("spec", "controlPlaneLoadBalancer", "healthCheckProtocol"),
190-
newlb.HealthCheckProtocol, "field is immutable once set"),
191-
)
192-
}
193213
}
194214

195215
return allErrs
@@ -301,8 +321,21 @@ func (r *AWSCluster) validateNetwork() field.ErrorList {
301321
return allErrs
302322
}
303323

304-
func (r *AWSCluster) validateControlPlaneLBs() field.ErrorList {
324+
func (r *AWSCluster) validateControlPlaneLBs() (admission.Warnings, field.ErrorList) {
305325
var allErrs field.ErrorList
326+
var allWarnings admission.Warnings
327+
328+
if r.Spec.ControlPlaneLoadBalancer != nil && r.Spec.ControlPlaneLoadBalancer.LoadBalancerType == LoadBalancerTypeClassic {
329+
allWarnings = append(allWarnings, fmt.Sprintf(warningClassicELB, "primary control plane"))
330+
331+
if r.Spec.ControlPlaneLoadBalancer.HealthCheckProtocol == nil {
332+
allWarnings = append(allWarnings, warningHealthCheckProtocolNotSet)
333+
}
334+
335+
if r.Spec.ControlPlaneLoadBalancer.HealthCheckProtocol != nil && *r.Spec.ControlPlaneLoadBalancer.HealthCheckProtocol == ELBProtocolSSL {
336+
allWarnings = append(allWarnings, "loadbalancer is using a classic elb with SSL health check, this causes issues with ciper suites with kubernetes v1.30+")
337+
}
338+
}
306339

307340
// If the secondary is defined, check that the name is not empty and different from the primary.
308341
// Also, ensure that the secondary load balancer is an NLB
@@ -322,6 +355,9 @@ func (r *AWSCluster) validateControlPlaneLBs() field.ErrorList {
322355
if r.Spec.SecondaryControlPlaneLoadBalancer.LoadBalancerType != LoadBalancerTypeNLB {
323356
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"))
324357
}
358+
if r.Spec.SecondaryControlPlaneLoadBalancer.LoadBalancerType == LoadBalancerTypeClassic {
359+
allWarnings = append(allWarnings, fmt.Sprintf(warningClassicELB, "secondary control plane"))
360+
}
325361
}
326362

327363
// Additional listeners are only supported for NLBs.
@@ -378,7 +414,7 @@ func (r *AWSCluster) validateControlPlaneLBs() field.ErrorList {
378414
}
379415
}
380416

381-
return allErrs
417+
return allWarnings, allErrs
382418
}
383419

384420
func (r *AWSCluster) validateIngressRule(rule IngressRule) field.ErrorList {

‎pkg/cloud/services/elb/loadbalancer.go

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -545,6 +545,14 @@ func (s *Service) reconcileClassicLoadBalancer() error {
545545
}
546546
}
547547

548+
if !cmp.Equal(spec.HealthCheck, apiELB.HealthCheck) {
549+
s.scope.Debug("Reconciling health check for apiserver load balancer", "health-check", spec.HealthCheck)
550+
err := s.configureHealthCheck(apiELB.Name, spec.HealthCheck)
551+
if err != nil {
552+
return err
553+
}
554+
}
555+
548556
if err := s.reconcileELBTags(apiELB, spec.Tags); err != nil {
549557
return errors.Wrapf(err, "failed to reconcile tags for apiserver load balancer %q", apiELB.Name)
550558
}
@@ -587,6 +595,22 @@ func (s *Service) reconcileClassicLoadBalancer() error {
587595
return nil
588596
}
589597

598+
func (s *Service) configureHealthCheck(name string, healthCheck *infrav1.ClassicELBHealthCheck) error {
599+
if _, err := s.ELBClient.ConfigureHealthCheck(&elb.ConfigureHealthCheckInput{
600+
LoadBalancerName: aws.String(name),
601+
HealthCheck: &elb.HealthCheck{
602+
Target: aws.String(healthCheck.Target),
603+
Interval: aws.Int64(int64(healthCheck.Interval.Seconds())),
604+
Timeout: aws.Int64(int64(healthCheck.Timeout.Seconds())),
605+
HealthyThreshold: aws.Int64(healthCheck.HealthyThreshold),
606+
UnhealthyThreshold: aws.Int64(healthCheck.UnhealthyThreshold),
607+
},
608+
}); err != nil {
609+
return errors.Wrapf(err, "failed to configure health check for classic load balancer: %s", name)
610+
}
611+
return nil
612+
}
613+
590614
func (s *Service) deleteAPIServerELB() error {
591615
s.scope.Debug("Deleting control plane load balancer")
592616

@@ -1732,7 +1756,7 @@ func (s *Service) createTargetGroup(ln infrav1.Listener, tags map[string]string)
17321756

17331757
func (s *Service) getHealthCheckTarget() string {
17341758
controlPlaneELB := s.scope.ControlPlaneLoadBalancer()
1735-
protocol := &infrav1.ELBProtocolSSL
1759+
protocol := &infrav1.ELBProtocolTCP
17361760
if controlPlaneELB != nil && controlPlaneELB.HealthCheckProtocol != nil {
17371761
protocol = controlPlaneELB.HealthCheckProtocol
17381762
if protocol.String() == infrav1.ELBProtocolHTTP.String() || protocol.String() == infrav1.ELBProtocolHTTPS.String() {

‎pkg/cloud/services/elb/loadbalancer_test.go

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1572,7 +1572,8 @@ func TestReconcileTargetGroupsAndListeners(t *testing.T) {
15721572
Port: aws.Int64(infrav1.DefaultAPIServerPort),
15731573
Protocol: aws.String("TCP"),
15741574
},
1575-
}}, nil)
1575+
},
1576+
}, nil)
15761577
},
15771578
check: func(t *testing.T, tgs []*elbv2.TargetGroup, listeners []*elbv2.Listener, err error) {
15781579
t.Helper()
@@ -2332,7 +2333,8 @@ func TestReconcileV2LB(t *testing.T) {
23322333
Matcher: &elbv2.Matcher{},
23332334
TargetGroupArn: aws.String(tgArn),
23342335
TargetGroupName: aws.String("targetGroup"),
2335-
}},
2336+
},
2337+
},
23362338
}, nil)
23372339
m.ModifyLoadBalancerAttributes(&elbv2.ModifyLoadBalancerAttributesInput{
23382340
LoadBalancerArn: aws.String(elbArn),
@@ -2341,7 +2343,8 @@ func TestReconcileV2LB(t *testing.T) {
23412343
Key: aws.String("load_balancing.cross_zone.enabled"),
23422344
Value: aws.String("false"),
23432345
},
2344-
}}).
2346+
},
2347+
}).
23452348
Return(&elbv2.ModifyLoadBalancerAttributesOutput{}, nil)
23462349

23472350
m.CreateTargetGroup(helpers.PartialMatchCreateTargetGroupInput(t, &elbv2.CreateTargetGroupInput{
@@ -2443,7 +2446,8 @@ func TestReconcileV2LB(t *testing.T) {
24432446
Port: aws.Int64(infrav1.DefaultAPIServerPort),
24442447
Protocol: aws.String("TCP"),
24452448
},
2446-
}}, nil)
2449+
},
2450+
}, nil)
24472451
m.DescribeLoadBalancerAttributes(&elbv2.DescribeLoadBalancerAttributesInput{LoadBalancerArn: aws.String(elbArn)}).Return(
24482452
&elbv2.DescribeLoadBalancerAttributesOutput{
24492453
Attributes: []*elbv2.LoadBalancerAttribute{
@@ -3484,7 +3488,7 @@ func TestGetHealthCheckProtocol(t *testing.T) {
34843488
{
34853489
"default case",
34863490
&infrav1.AWSLoadBalancerSpec{},
3487-
"SSL:6443",
3491+
"TCP:6443",
34883492
},
34893493
{
34903494
"protocol http",

‎test/e2e/data/e2e_conf.yaml

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ images:
1717
- name: gcr.io/k8s-staging-cluster-api/capa-manager:e2e
1818
loadBehavior: mustLoad
1919

20-
## PLEASE KEEP THESE UP TO DATE WITH THE COMPONENTS
20+
## PLEASE KEEP THESE UP TO DATE WITH THE COMPONENTS
2121

2222
# Cluster API v1beta1 Preloads
2323
- name: quay.io/jetstack/cert-manager-cainjector:v1.15.1
@@ -123,7 +123,17 @@ providers:
123123
files:
124124
- sourcePath: "./shared/v1beta1_provider/metadata.yaml"
125125
- sourcePath: "./infrastructure-aws/capi-upgrades/v1beta1/cluster-template.yaml"
126-
- name: v2.0.99
126+
- name: "v2.7.1"
127+
value: "https://github.com/kubernetes-sigs/cluster-api-provider-aws/releases/download/v2.7.1/infrastructure-components.yaml"
128+
type: "url"
129+
contract: v1beta2
130+
replacements:
131+
- old: --metrics-addr=127.0.0.1:8080
132+
new: --metrics-addr=:8080
133+
files:
134+
- sourcePath: "./shared/v1beta2_provider/v2.7/metadata.yaml"
135+
- sourcePath: "./infrastructure-aws/withoutclusterclass/generated/cluster-template-classicelb-upgrade.yaml"
136+
- name: v9.9.99
127137
# Use manifest from source files
128138
value: ../../../config/default
129139
# Do not add contract field for v1beta1 --> v1beta2 clusterctl upgrades test to work.
@@ -208,6 +218,8 @@ variables:
208218
EXP_KUBEADM_BOOTSTRAP_FORMAT_IGNITION: "true"
209219
EXP_EXTERNAL_RESOURCE_GC: "true"
210220
GC_WORKLOAD: "../../data/gcworkload.yaml"
221+
CLASSICELB_TEST_KUBERNETES_VERSION_FROM: "v1.29.9"
222+
CLASSICELB_TEST_KUBERNETES_VERSION_TO: "v1.30.8"
211223

212224
intervals:
213225
default/wait-cluster: ["35m", "10s"]
@@ -223,8 +235,10 @@ intervals:
223235
default/wait-failed-machine-status: ["2m", "10s"]
224236
default/wait-infra-subnets: ["5m", "30s"]
225237
default/wait-machine-pool-nodes: ["40m", "10s"]
226-
default/wait-machine-pool-upgrade: [ "50m", "10s" ]
238+
default/wait-machine-pool-upgrade: ["50m", "10s"]
227239
default/wait-create-identity: ["1m", "10s"]
228240
default/wait-job: ["10m", "10s"]
229241
default/wait-deployment-ready: ["5m", "10s"]
230242
default/wait-loadbalancer-ready: ["5m", "30s"]
243+
default/wait-classic-elb-health-check-short: ["1m", "10s"]
244+
default/wait-classic-elb-health-check-long: ["10m", "10s"]

‎test/e2e/data/infrastructure-aws/withoutclusterclass/kustomize_sources/classicelb-upgrade/kustomization.yaml

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
apiVersion: kustomize.config.k8s.io/v1beta1
2+
kind: Kustomization
3+
4+
resources:
5+
- ../default
6+
7+
patches:
8+
- path: nocontrolplane.yaml
9+
target:
10+
group: infrastructure.cluster.x-k8s.io
11+
kind: AWSCluster
12+
version: v1beta2

‎test/e2e/data/infrastructure-aws/withoutclusterclass/kustomize_sources/classicelb-upgrade/nocontrolplane.yaml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
- op: remove
2+
path: /spec/controlPlaneLoadBalancer

‎test/e2e/data/shared/v1beta2_provider/metadata.yaml

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,3 +41,27 @@ releaseSeries:
4141
- major: 2
4242
minor: 0
4343
contract: v1beta1
44+
- major: 2
45+
minor: 1
46+
contract: v1beta1
47+
- major: 2
48+
minor: 2
49+
contract: v1beta1
50+
- major: 2
51+
minor: 3
52+
contract: v1beta1
53+
- major: 2
54+
minor: 4
55+
contract: v1beta1
56+
- major: 2
57+
minor: 5
58+
contract: v1beta1
59+
- major: 2
60+
minor: 6
61+
contract: v1beta1
62+
- major: 2
63+
minor: 7
64+
contract: v1beta1
65+
- major: 9
66+
minor: 9
67+
contract: v1beta1

‎test/e2e/data/shared/v1beta2_provider/v2.7/metadata.yaml

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
# maps release series of major.minor to cluster-api contract version
2+
# the contract version may change between minor or major versions, but *not*
3+
# between patch versions.
4+
#
5+
# update this file only when a new major or minor version is released
6+
apiVersion: clusterctl.cluster.x-k8s.io/v1alpha3
7+
releaseSeries:
8+
- major: 0
9+
minor: 4
10+
contract: v1alpha2
11+
- major: 0
12+
minor: 5
13+
contract: v1alpha3
14+
- major: 0
15+
minor: 6
16+
contract: v1alpha3
17+
- major: 0
18+
minor: 7
19+
contract: v1alpha4
20+
- major: 1
21+
minor: 0
22+
contract: v1beta1
23+
- major: 1
24+
minor: 1
25+
contract: v1beta1
26+
- major: 1
27+
minor: 2
28+
contract: v1beta1
29+
- major: 1
30+
minor: 3
31+
contract: v1beta1
32+
- major: 1
33+
minor: 4
34+
contract: v1beta1
35+
- major: 1
36+
minor: 5
37+
contract: v1beta1
38+
- major: 1
39+
minor: 6
40+
contract: v1beta1
41+
- major: 2
42+
minor: 0
43+
contract: v1beta1
44+
- major: 2
45+
minor: 1
46+
contract: v1beta1
47+
- major: 2
48+
minor: 2
49+
contract: v1beta1
50+
- major: 2
51+
minor: 3
52+
contract: v1beta1
53+
- major: 2
54+
minor: 4
55+
contract: v1beta1
56+
- major: 2
57+
minor: 5
58+
contract: v1beta1
59+
- major: 2
60+
minor: 6
61+
contract: v1beta1
62+
- major: 2
63+
minor: 7
64+
contract: v1beta1

0 commit comments

Comments
 (0)