Merge pull request #1325 from mtulio/fix-1254-lb-leak

fix: lb leak preventing changes in Load Balancer type annotation after creation

Author: k8s-ci-robot

pkg/providers/v1/aws_validations.go

@@ -18,6 +18,7 @@ package aws
 
 import (
 	"fmt"
+	"strings"
 
 	v1 "k8s.io/api/core/v1"
 )
@@ -53,6 +54,25 @@ func ensureLoadBalancerValidation(v *awsValidationInput) error {
 func validateServiceAnnotations(v *awsValidationInput) error {
 	isNLB := isNLB(v.annotations)
 
+	// ServiceAnnotationLoadBalancerType
+	// Load Balancer Type annotation must not be updated after creation.
+	// Classic Load Balancer: hostname ends with ".elb.amazonaws.com"
+	// NLB: hostname ends with ".elb.<region>.amazonaws.com"
+	{
+		hostIsCreated := len(v.apiService.Status.LoadBalancer.Ingress) > 0
+		if hostIsCreated {
+			hostIsClassic := strings.HasSuffix(v.apiService.Status.LoadBalancer.Ingress[0].Hostname, ".elb.amazonaws.com")
+			// If annotation is set to NLB and hostname has classic pattern, return an error.
+			if isNLB && hostIsClassic {
+				return fmt.Errorf("cannot update Load Balancer Type annotation %q after creation for NLB", ServiceAnnotationLoadBalancerType)
+			}
+			// If annotation is set to CLB and hostname has NLB pattern, return an error.
+			if !isNLB && !hostIsClassic {
+				return fmt.Errorf("cannot update Load Balancer Type annotation %q after creation for Classic Load Balancer", ServiceAnnotationLoadBalancerType)
+			}
+		}
+	}
+
 	// ServiceAnnotationLoadBalancerSecurityGroups
 	// NLB only: ensure the BYO annotations are not supported and return an error.
 	// 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

@@ -269,11 +269,14 @@ func TestValidateServiceAnnotationTargetGroupAttributes(t *testing.T) {
 
 func TestValidateServiceAnnotations(t *testing.T) {
 	const byoSecurityGroupID = "sg-123456789"
+	const classicLBHostname = "my-classic-lb-1234567890.us-east-1.elb.amazonaws.com"
+	const nlbHostname = "my-nlb-1234567890.elb.us-east-1.amazonaws.com"
 
 	tests := []struct {
 		name          string
 		annotations   map[string]string
 		servicePorts  []v1.ServicePort
+		ingressStatus []v1.LoadBalancerIngress
 		expectedError string
 	}{
 		// Valid cases - CLB (Classic Load Balancer) should allow BYO security groups
@@ -420,6 +423,108 @@ func TestValidateServiceAnnotations(t *testing.T) {
 			annotations:   map[string]string{},
 			expectedError: "",
 		},
+
+		// No existing ingress - any type should be allowed
+		{
+			name:          "NLB in new service with no ingress should be allowed",
+			annotations:   map[string]string{ServiceAnnotationLoadBalancerType: "nlb"},
+			ingressStatus: nil,
+			expectedError: "",
+		},
+		{
+			name:          "CLB in new service with no ingress should be allowed",
+			annotations:   map[string]string{},
+			ingressStatus: nil,
+			expectedError: "",
+		},
+		{
+			name:          "NLB in new service with empty ingress list should be allowed",
+			annotations:   map[string]string{ServiceAnnotationLoadBalancerType: "nlb"},
+			ingressStatus: []v1.LoadBalancerIngress{},
+			expectedError: "",
+		},
+
+		// Existing Classic LB - same type should succeed
+		{
+			name:        "CLB in existing service with no type annotation should be allowed",
+			annotations: map[string]string{},
+			ingressStatus: []v1.LoadBalancerIngress{
+				{Hostname: classicLBHostname},
+			},
+			expectedError: "",
+		},
+		{
+			name:        "CLB in existing service with type annotation should be allowed",
+			annotations: map[string]string{ServiceAnnotationLoadBalancerType: "clb"},
+			ingressStatus: []v1.LoadBalancerIngress{
+				{Hostname: classicLBHostname},
+			},
+			expectedError: "",
+		},
+
+		// Existing NLB - same type should succeed
+		{
+			name:        "NLB in existing service with type annotation should be allowed",
+			annotations: map[string]string{ServiceAnnotationLoadBalancerType: "nlb"},
+			ingressStatus: []v1.LoadBalancerIngress{
+				{Hostname: nlbHostname},
+			},
+			expectedError: "",
+		},
+
+		// Type change from CLB to NLB - should fail
+		{
+			name:        "CLB in existing service with type annotation should not be allowed to change to NLB",
+			annotations: map[string]string{ServiceAnnotationLoadBalancerType: "nlb"},
+			ingressStatus: []v1.LoadBalancerIngress{
+				{Hostname: classicLBHostname},
+			},
+			expectedError: "cannot update Load Balancer Type annotation",
+		},
+
+		// Type change from NLB to CLB - should fail
+		{
+			name:        "NLB in existing service with type annotation should not be allowed to change to CLB",
+			annotations: map[string]string{},
+			ingressStatus: []v1.LoadBalancerIngress{
+				{Hostname: nlbHostname},
+			},
+			expectedError: "cannot update Load Balancer Type annotation",
+		},
+		{
+			name:        "NLB in existing service with type annotation should not be allowed to change to CLB",
+			annotations: map[string]string{ServiceAnnotationLoadBalancerType: "clb"},
+			ingressStatus: []v1.LoadBalancerIngress{
+				{Hostname: nlbHostname},
+			},
+			expectedError: "cannot update Load Balancer Type annotation",
+		},
+
+		// Edge cases with hostname patterns
+		{
+			name:        "CLB in existing service with regional hostname should be allowed",
+			annotations: map[string]string{},
+			ingressStatus: []v1.LoadBalancerIngress{
+				{Hostname: "internal-my-lb-123.eu-west-1.elb.amazonaws.com"},
+			},
+			expectedError: "",
+		},
+		{
+			name:        "NLB in existing service with different region hostname should be allowed",
+			annotations: map[string]string{ServiceAnnotationLoadBalancerType: "nlb"},
+			ingressStatus: []v1.LoadBalancerIngress{
+				{Hostname: "my-nlb-abc123.elb.ap-southeast-1.amazonaws.com"},
+			},
+			expectedError: "",
+		},
+		{
+			name:        "NLB in existing service with regional hostname should not be allowed to change to CLB",
+			annotations: map[string]string{},
+			ingressStatus: []v1.LoadBalancerIngress{
+				{Hostname: "my-nlb-abc123.elb.eu-central-1.amazonaws.com"},
+			},
+			expectedError: "cannot update Load Balancer Type annotation",
+		},
 	}
 
 	for _, tt := range tests {
@@ -436,6 +541,9 @@ func TestValidateServiceAnnotations(t *testing.T) {
 					Ports: tt.servicePorts,
 				},
 			}
+			if tt.ingressStatus != nil {
+				service.Status.LoadBalancer.Ingress = tt.ingressStatus
+			}
 
 			// Create validation input
 			input := &awsValidationInput{