added validation for dummy ports introduced in the aws-load-balancer-ssl-ports annotation (#3067)

ahrakos · web-flow · commit a6c935243a5a · 2023-03-20T08:39:20.000-07:00
diff --git a/pkg/service/model_build_listener.go b/pkg/service/model_build_listener.go
@@ -14,9 +14,13 @@ import (
 )
 
 func (t *defaultModelBuildTask) buildListeners(ctx context.Context, scheme elbv2model.LoadBalancerScheme) error {
-	cfg := t.buildListenerConfig(ctx)
+	cfg, err := t.buildListenerConfig(ctx)
+	if err != nil {
+		return err
+	}
+
 	for _, port := range t.service.Spec.Ports {
-		_, err := t.buildListener(ctx, port, cfg, scheme)
+		_, err := t.buildListener(ctx, port, *cfg, scheme)
 		if err != nil {
 			return err
 		}
@@ -115,10 +119,43 @@ func (t *defaultModelBuildTask) buildListenerCertificates(_ context.Context) []e
 	return certificates
 }
 
-func (t *defaultModelBuildTask) buildTLSPortsSet(_ context.Context) sets.String {
+func validateTLSPortsSet(rawTLSPorts []string, ports []corev1.ServicePort) error {
+	unusedPorts := make([]string, 0)
+
+	for _, tlsPort := range rawTLSPorts {
+		isPortUsed := false
+		for _, portObj := range ports {
+			if portObj.Name == tlsPort || strconv.Itoa(int(portObj.Port)) == tlsPort {
+				isPortUsed = true
+				break
+			}
+		}
+
+		if !isPortUsed {
+			unusedPorts = append(unusedPorts, tlsPort)
+		}
+	}
+
+	if len(unusedPorts) > 0 {
+		unusedPortErr := errors.Errorf("Unused port in ssl-ports annotation %v", unusedPorts)
+		return unusedPortErr
+	}
+
+	return nil
+}
+
+func (t *defaultModelBuildTask) buildTLSPortsSet(_ context.Context) (sets.String, error) {
 	var rawTLSPorts []string
+
 	_ = t.annotationParser.ParseStringSliceAnnotation(annotations.SvcLBSuffixSSLPorts, &rawTLSPorts, t.service.Annotations)
-	return sets.NewString(rawTLSPorts...)
+
+	err := validateTLSPortsSet(rawTLSPorts, t.service.Spec.Ports)
+
+	if err != nil {
+		return nil, err
+	}
+
+	return sets.NewString(rawTLSPorts...), nil
 }
 
 func (t *defaultModelBuildTask) buildBackendProtocol(_ context.Context) string {
@@ -154,18 +191,22 @@ type listenerConfig struct {
 	backendProtocol string
 }
 
-func (t *defaultModelBuildTask) buildListenerConfig(ctx context.Context) listenerConfig {
+func (t *defaultModelBuildTask) buildListenerConfig(ctx context.Context) (*listenerConfig, error) {
 	certificates := t.buildListenerCertificates(ctx)
-	tlsPortsSet := t.buildTLSPortsSet(ctx)
+	tlsPortsSet, err := t.buildTLSPortsSet(ctx)
+	if err != nil {
+		return nil, err
+	}
+
 	backendProtocol := t.buildBackendProtocol(ctx)
 	sslPolicy := t.buildSSLNegotiationPolicy(ctx)
 
-	return listenerConfig{
+	return &listenerConfig{
 		certificates:    certificates,
 		tlsPortsSet:     tlsPortsSet,
 		sslPolicy:       sslPolicy,
 		backendProtocol: backendProtocol,
-	}
+	}, nil
 }
 
 func (t *defaultModelBuildTask) buildListenerTags(ctx context.Context) (map[string]string, error) {
diff --git a/pkg/service/model_build_listener_test.go b/pkg/service/model_build_listener_test.go
@@ -4,9 +4,12 @@ import (
 	"context"
 	"testing"
 
+	"github.com/pkg/errors"
 	"github.com/stretchr/testify/assert"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/intstr"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"sigs.k8s.io/aws-load-balancer-controller/pkg/annotations"
 	elbv2model "sigs.k8s.io/aws-load-balancer-controller/pkg/model/elbv2"
 )
@@ -88,3 +91,94 @@ func Test_defaultModelBuilderTask_buildListenerALPNPolicy(t *testing.T) {
 		})
 	}
 }
+
+func Test_defaultModelBuilderTask_buildListenerConfig(t *testing.T) {
+	tests := []struct {
+		name    string
+		svc     *corev1.Service
+		wantErr error
+		want    *listenerConfig
+	}{
+		{
+			name: "Service with unused ports in the ssl-ports annotation, Unused ports provided",
+			svc: &corev1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						"service.beta.kubernetes.io/aws-load-balancer-ssl-ports": "80, 85, 90, arbitrary-name",
+					},
+				},
+				Spec: corev1.ServiceSpec{
+					Type: corev1.ServiceTypeLoadBalancer,
+					Ports: []corev1.ServicePort{
+						{
+							Name:       "http",
+							Port:       80,
+							TargetPort: intstr.FromInt(80),
+							Protocol:   corev1.ProtocolTCP,
+							NodePort:   31223,
+						},
+						{
+							Name:       "alt2",
+							Port:       83,
+							TargetPort: intstr.FromInt(8883),
+							Protocol:   corev1.ProtocolTCP,
+							NodePort:   32323,
+						},
+					},
+				},
+			},
+			wantErr: errors.New("Unused port in ssl-ports annotation [85 90 arbitrary-name]"),
+		},
+		{
+			name: "Service with unused ports in the ssl-ports annotation, No unused ports",
+			svc: &corev1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						"service.beta.kubernetes.io/aws-load-balancer-ssl-ports": "83",
+					},
+				},
+				Spec: corev1.ServiceSpec{
+					Type: corev1.ServiceTypeLoadBalancer,
+					Ports: []corev1.ServicePort{
+						{
+							Name:       "http",
+							Port:       80,
+							TargetPort: intstr.FromInt(80),
+							Protocol:   corev1.ProtocolTCP,
+							NodePort:   31223,
+						},
+						{
+							Name:       "alt2",
+							Port:       83,
+							TargetPort: intstr.FromInt(8883),
+							Protocol:   corev1.ProtocolTCP,
+							NodePort:   32323,
+						},
+					},
+				},
+			},
+			want: &listenerConfig{
+				certificates:    ([]elbv2model.Certificate)(nil),
+				tlsPortsSet:     sets.NewString("83"),
+				sslPolicy:       new(string),
+				backendProtocol: "",
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			parser := annotations.NewSuffixAnnotationParser("service.beta.kubernetes.io")
+			builder := &defaultModelBuildTask{
+				annotationParser: parser,
+				service:          tt.svc,
+			}
+			got, err := builder.buildListenerConfig(context.Background())
+			if tt.wantErr != nil {
+				assert.EqualError(t, err, tt.wantErr.Error())
+			} else {
+				assert.Equal(t, tt.want, got)
+			}
+		})
+	}
+}
diff --git a/test/e2e/service/nlb_ip_target_test.go b/test/e2e/service/nlb_ip_target_test.go
@@ -217,6 +217,18 @@ var _ = Describe("k8s service reconciled by the aws load balancer", func() {
 							TargetPort: intstr.FromInt(80),
 							Protocol:   corev1.ProtocolTCP,
 						},
+						{
+							Port:       443,
+							Name:       "https",
+							TargetPort: intstr.FromInt(443),
+							Protocol:   corev1.ProtocolTCP,
+						},
+						{
+							Port:       333,
+							Name:       "arbitrary-port",
+							TargetPort: intstr.FromInt(333),
+							Protocol:   corev1.ProtocolTCP,
+						},
 					},
 				},
 			}
@@ -245,10 +257,14 @@ var _ = Describe("k8s service reconciled by the aws load balancer", func() {
 					Scheme:     "internet-facing",
 					TargetType: "ip",
 					Listeners: map[string]string{
-						"80": "TLS",
+						"80":  "TLS",
+						"443": "TLS",
+						"333": "TLS",
 					},
 					TargetGroups: map[string]string{
-						"80": "TCP",
+						"80":  "TCP",
+						"443": "TCP",
+						"333": "TCP",
 					},
 					NumTargets: int(numReplicas),
 				})
@@ -272,10 +288,14 @@ var _ = Describe("k8s service reconciled by the aws load balancer", func() {
 					Scheme:     "internet-facing",
 					TargetType: "ip",
 					Listeners: map[string]string{
-						"80": "TCP",
+						"80":  "TCP",
+						"443": "TLS",
+						"333": "TLS",
 					},
 					TargetGroups: map[string]string{
-						"80": "TCP",
+						"80":  "TCP",
+						"443": "TCP",
+						"333": "TCP",
 					},
 					NumTargets: int(numReplicas),
 				})
