Skip to content

Commit 54ac0d6

Browse files
committed
Prevent Load Balancer type annotation changes after creation
Adds validation to prevent users from changing the Load Balancer type annotation (service.beta.kubernetes.io/aws-load-balancer-type) after the load balancer has been created. This prevents undefined behavior and potential service disruptions. The validation detects the current load balancer type by analyzing the hostname pattern in the service's LoadBalancer status: - Classic Load Balancer: hostname ends with ".elb.amazonaws.com" - Network Load Balancer: hostname ends with ".elb.<region>.amazonaws.com" If a mismatch is detected between the annotation and the existing load balancer type, the controller returns a validation error preventing the update. Relatest to Issue 1254 Signed-off-by: Claude (AI Assistant) <noreply@anthropic.com> Co-Authored-By: Marco Braga <mrbraga@redhat.com>
1 parent d1c7c02 commit 54ac0d6

File tree

2 files changed

+128
-0
lines changed

2 files changed

+128
-0
lines changed

pkg/providers/v1/aws_validations.go

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package aws
1818

1919
import (
2020
"fmt"
21+
"strings"
2122

2223
v1 "k8s.io/api/core/v1"
2324
)
@@ -53,6 +54,25 @@ func ensureLoadBalancerValidation(v *awsValidationInput) error {
5354
func validateServiceAnnotations(v *awsValidationInput) error {
5455
isNLB := isNLB(v.annotations)
5556

57+
// ServiceAnnotationLoadBalancerType
58+
// Load Balancer Type annotation must not be updated after creation.
59+
// Classic Load Balancer: hostname ends with ".elb.amazonaws.com"
60+
// NLB: hostname ends with ".elb.<region>.amazonaws.com"
61+
{
62+
hostIsCreated := len(v.apiService.Status.LoadBalancer.Ingress) > 0
63+
if hostIsCreated {
64+
hostIsClassic := strings.HasSuffix(v.apiService.Status.LoadBalancer.Ingress[0].Hostname, ".elb.amazonaws.com")
65+
// If annotation is set to NLB and hostname has classic pattern, return an error.
66+
if isNLB && hostIsClassic {
67+
return fmt.Errorf("cannot update Load Balancer Type annotation %q after creation for NLB", ServiceAnnotationLoadBalancerType)
68+
}
69+
// If annotation is set to CLB and hostname has NLB pattern, return an error.
70+
if !isNLB && !hostIsClassic {
71+
return fmt.Errorf("cannot update Load Balancer Type annotation %q after creation for Classic Load Balancer", ServiceAnnotationLoadBalancerType)
72+
}
73+
}
74+
}
75+
5676
// ServiceAnnotationLoadBalancerSecurityGroups
5777
// NLB only: ensure the BYO annotations are not supported and return an error.
5878
// FIXME: the BYO SG for NLB implementation is blocked by https://github.com/kubernetes/cloud-provider-aws/pull/1209

pkg/providers/v1/aws_validations_test.go

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -269,11 +269,14 @@ func TestValidateServiceAnnotationTargetGroupAttributes(t *testing.T) {
269269

270270
func TestValidateServiceAnnotations(t *testing.T) {
271271
const byoSecurityGroupID = "sg-123456789"
272+
const classicLBHostname = "my-classic-lb-1234567890.us-east-1.elb.amazonaws.com"
273+
const nlbHostname = "my-nlb-1234567890.elb.us-east-1.amazonaws.com"
272274

273275
tests := []struct {
274276
name string
275277
annotations map[string]string
276278
servicePorts []v1.ServicePort
279+
ingressStatus []v1.LoadBalancerIngress
277280
expectedError string
278281
}{
279282
// Valid cases - CLB (Classic Load Balancer) should allow BYO security groups
@@ -420,6 +423,108 @@ func TestValidateServiceAnnotations(t *testing.T) {
420423
annotations: map[string]string{},
421424
expectedError: "",
422425
},
426+
427+
// No existing ingress - any type should be allowed
428+
{
429+
name: "NLB in new service with no ingress should be allowed",
430+
annotations: map[string]string{ServiceAnnotationLoadBalancerType: "nlb"},
431+
ingressStatus: nil,
432+
expectedError: "",
433+
},
434+
{
435+
name: "CLB in new service with no ingress should be allowed",
436+
annotations: map[string]string{},
437+
ingressStatus: nil,
438+
expectedError: "",
439+
},
440+
{
441+
name: "NLB in new service with empty ingress list should be allowed",
442+
annotations: map[string]string{ServiceAnnotationLoadBalancerType: "nlb"},
443+
ingressStatus: []v1.LoadBalancerIngress{},
444+
expectedError: "",
445+
},
446+
447+
// Existing Classic LB - same type should succeed
448+
{
449+
name: "CLB in existing service with no type annotation should be allowed",
450+
annotations: map[string]string{},
451+
ingressStatus: []v1.LoadBalancerIngress{
452+
{Hostname: classicLBHostname},
453+
},
454+
expectedError: "",
455+
},
456+
{
457+
name: "CLB in existing service with type annotation should be allowed",
458+
annotations: map[string]string{ServiceAnnotationLoadBalancerType: "clb"},
459+
ingressStatus: []v1.LoadBalancerIngress{
460+
{Hostname: classicLBHostname},
461+
},
462+
expectedError: "",
463+
},
464+
465+
// Existing NLB - same type should succeed
466+
{
467+
name: "NLB in existing service with type annotation should be allowed",
468+
annotations: map[string]string{ServiceAnnotationLoadBalancerType: "nlb"},
469+
ingressStatus: []v1.LoadBalancerIngress{
470+
{Hostname: nlbHostname},
471+
},
472+
expectedError: "",
473+
},
474+
475+
// Type change from CLB to NLB - should fail
476+
{
477+
name: "CLB in existing service with type annotation should not be allowed to change to NLB",
478+
annotations: map[string]string{ServiceAnnotationLoadBalancerType: "nlb"},
479+
ingressStatus: []v1.LoadBalancerIngress{
480+
{Hostname: classicLBHostname},
481+
},
482+
expectedError: "cannot update Load Balancer Type annotation",
483+
},
484+
485+
// Type change from NLB to CLB - should fail
486+
{
487+
name: "NLB in existing service with type annotation should not be allowed to change to CLB",
488+
annotations: map[string]string{},
489+
ingressStatus: []v1.LoadBalancerIngress{
490+
{Hostname: nlbHostname},
491+
},
492+
expectedError: "cannot update Load Balancer Type annotation",
493+
},
494+
{
495+
name: "NLB in existing service with type annotation should not be allowed to change to CLB",
496+
annotations: map[string]string{ServiceAnnotationLoadBalancerType: "clb"},
497+
ingressStatus: []v1.LoadBalancerIngress{
498+
{Hostname: nlbHostname},
499+
},
500+
expectedError: "cannot update Load Balancer Type annotation",
501+
},
502+
503+
// Edge cases with hostname patterns
504+
{
505+
name: "CLB in existing service with regional hostname should be allowed",
506+
annotations: map[string]string{},
507+
ingressStatus: []v1.LoadBalancerIngress{
508+
{Hostname: "internal-my-lb-123.eu-west-1.elb.amazonaws.com"},
509+
},
510+
expectedError: "",
511+
},
512+
{
513+
name: "NLB in existing service with different region hostname should be allowed",
514+
annotations: map[string]string{ServiceAnnotationLoadBalancerType: "nlb"},
515+
ingressStatus: []v1.LoadBalancerIngress{
516+
{Hostname: "my-nlb-abc123.elb.ap-southeast-1.amazonaws.com"},
517+
},
518+
expectedError: "",
519+
},
520+
{
521+
name: "NLB in existing service with regional hostname should not be allowed to change to CLB",
522+
annotations: map[string]string{},
523+
ingressStatus: []v1.LoadBalancerIngress{
524+
{Hostname: "my-nlb-abc123.elb.eu-central-1.amazonaws.com"},
525+
},
526+
expectedError: "cannot update Load Balancer Type annotation",
527+
},
423528
}
424529

425530
for _, tt := range tests {
@@ -436,6 +541,9 @@ func TestValidateServiceAnnotations(t *testing.T) {
436541
Ports: tt.servicePorts,
437542
},
438543
}
544+
if tt.ingressStatus != nil {
545+
service.Status.LoadBalancer.Ingress = tt.ingressStatus
546+
}
439547

440548
// Create validation input
441549
input := &awsValidationInput{

0 commit comments

Comments
 (0)