From 8909a215d2d9659c257b94668af7e97535ad1a8b Mon Sep 17 00:00:00 2001 From: M00nF1sh Date: Wed, 4 Aug 2021 11:12:56 -0700 Subject: [PATCH 1/7] fix the regression of IP mode support for fargate pods (#2158) From 0edfd5ed8d9294abcaa7c029716afe09ea912b70 Mon Sep 17 00:00:00 2001 From: Andres Morey Date: Sun, 5 Sep 2021 15:55:53 +0300 Subject: [PATCH 2/7] Add support for TCP_UDP to NLB TargetGroups and Listeners Previously, aws-load-balancer-controller ignored extra overlapping ServicePorts defined in the Kubernetes Service spec if the external port numbers were the same even if the protocols were different (e.g. TCP:53, UDP:53). This behavior prevented users from exposing services that support TCP and UDP on the same external load balancer port number. This patch solves the problem by detecting when a user defines multiple ServicePorts for the same external load balancer port number but using TCP and UDP protocols separately. In such situations, a TCP_UDP TargetGroup and Listener are created and SecurityGroup rules are updated accordingly. If more than two ServicePorts are defined, only the first two mergeable ServicePorts are used. Otherwise, the first ServicePort is used. Note: rebasing errors would be my fault -- Kevin Lyda Signed-off-by: Kevin Lyda --- .gitignore | 1 + .../elbv2/v1beta1/targetgroupbinding_types.go | 3 + config/webhook/manifests.yaml | 242 ++++++++-------- pkg/ingress/model_build_target_group.go | 5 +- pkg/service/model_build_listener.go | 59 +++- pkg/service/model_build_listener_test.go | 219 ++++++++++++++- pkg/service/model_build_target_group.go | 51 +++- pkg/service/model_build_target_group_test.go | 62 ++++- pkg/service/model_builder_test.go | 258 ++++++++++++++++++ 9 files changed, 754 insertions(+), 146 deletions(-) diff --git a/.gitignore b/.gitignore index d9917959af..21a3d67398 100644 --- a/.gitignore +++ b/.gitignore @@ -25,3 +25,4 @@ site *~ *.bak scripts/aws_sdk_model_override/* +/gomock_reflect* diff --git a/apis/elbv2/v1beta1/targetgroupbinding_types.go b/apis/elbv2/v1beta1/targetgroupbinding_types.go index 7a273a1d43..08345a99a4 100644 --- a/apis/elbv2/v1beta1/targetgroupbinding_types.go +++ b/apis/elbv2/v1beta1/targetgroupbinding_types.go @@ -87,6 +87,9 @@ const ( // NetworkingProtocolUDP is the UDP protocol. NetworkingProtocolUDP NetworkingProtocol = "UDP" + + // NetworkingProtocolTCP_UDP is the TCP_UDP protocol. + NetworkingProtocolTCP_UDP NetworkingProtocol = "TCP_UDP" ) // NetworkingPort defines the port and protocol for networking rules. diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml index 0b1a24bfed..9d1898df1f 100644 --- a/config/webhook/manifests.yaml +++ b/config/webhook/manifests.yaml @@ -2,130 +2,130 @@ apiVersion: admissionregistration.k8s.io/v1 kind: MutatingWebhookConfiguration metadata: - name: webhook + name: mutating-webhook-configuration webhooks: - - admissionReviewVersions: - - v1beta1 - clientConfig: - service: - name: webhook-service - namespace: system - path: /mutate-v1-pod - failurePolicy: Fail - name: mpod.elbv2.k8s.aws - rules: - - apiGroups: - - "" - apiVersions: - - v1 - operations: - - CREATE - resources: - - pods - sideEffects: None - - admissionReviewVersions: - - v1beta1 - clientConfig: - service: - name: webhook-service - namespace: system - path: /mutate-v1-service - failurePolicy: Fail - name: mservice.elbv2.k8s.aws - rules: - - apiGroups: - - "" - apiVersions: - - v1 - operations: - - CREATE - resources: - - services - sideEffects: None - - admissionReviewVersions: - - v1beta1 - clientConfig: - service: - name: webhook-service - namespace: system - path: /mutate-elbv2-k8s-aws-v1beta1-targetgroupbinding - failurePolicy: Fail - name: mtargetgroupbinding.elbv2.k8s.aws - rules: - - apiGroups: - - elbv2.k8s.aws - apiVersions: - - v1beta1 - operations: - - CREATE - - UPDATE - resources: - - targetgroupbindings - sideEffects: None +- admissionReviewVersions: + - v1beta1 + clientConfig: + service: + name: webhook-service + namespace: system + path: /mutate-v1-pod + failurePolicy: Fail + name: mpod.elbv2.k8s.aws + rules: + - apiGroups: + - "" + apiVersions: + - v1 + operations: + - CREATE + resources: + - pods + sideEffects: None +- admissionReviewVersions: + - v1beta1 + clientConfig: + service: + name: webhook-service + namespace: system + path: /mutate-v1-service + failurePolicy: Fail + name: mservice.elbv2.k8s.aws + rules: + - apiGroups: + - "" + apiVersions: + - v1 + operations: + - CREATE + resources: + - services + sideEffects: None +- admissionReviewVersions: + - v1beta1 + clientConfig: + service: + name: webhook-service + namespace: system + path: /mutate-elbv2-k8s-aws-v1beta1-targetgroupbinding + failurePolicy: Fail + name: mtargetgroupbinding.elbv2.k8s.aws + rules: + - apiGroups: + - elbv2.k8s.aws + apiVersions: + - v1beta1 + operations: + - CREATE + - UPDATE + resources: + - targetgroupbindings + sideEffects: None --- apiVersion: admissionregistration.k8s.io/v1 kind: ValidatingWebhookConfiguration metadata: - name: webhook + name: validating-webhook-configuration webhooks: - - admissionReviewVersions: - - v1beta1 - clientConfig: - service: - name: webhook-service - namespace: system - path: /validate-elbv2-k8s-aws-v1beta1-ingressclassparams - failurePolicy: Fail - name: vingressclassparams.elbv2.k8s.aws - rules: - - apiGroups: - - elbv2.k8s.aws - apiVersions: - - v1beta1 - operations: - - CREATE - - UPDATE - resources: - - ingressclassparams - sideEffects: None - - admissionReviewVersions: - - v1beta1 - clientConfig: - service: - name: webhook-service - namespace: system - path: /validate-elbv2-k8s-aws-v1beta1-targetgroupbinding - failurePolicy: Fail - name: vtargetgroupbinding.elbv2.k8s.aws - rules: - - apiGroups: - - elbv2.k8s.aws - apiVersions: - - v1beta1 - operations: - - CREATE - - UPDATE - resources: - - targetgroupbindings - sideEffects: None - - admissionReviewVersions: - - v1beta1 - clientConfig: - service: - name: webhook-service - namespace: system - path: /validate-networking-v1-ingress - failurePolicy: Fail - matchPolicy: Equivalent - name: vingress.elbv2.k8s.aws - rules: - - apiGroups: - - networking.k8s.io - apiVersions: - - v1 - operations: - - CREATE - - UPDATE - resources: - - ingresses - sideEffects: None +- admissionReviewVersions: + - v1beta1 + clientConfig: + service: + name: webhook-service + namespace: system + path: /validate-elbv2-k8s-aws-v1beta1-ingressclassparams + failurePolicy: Fail + name: vingressclassparams.elbv2.k8s.aws + rules: + - apiGroups: + - elbv2.k8s.aws + apiVersions: + - v1beta1 + operations: + - CREATE + - UPDATE + resources: + - ingressclassparams + sideEffects: None +- admissionReviewVersions: + - v1beta1 + clientConfig: + service: + name: webhook-service + namespace: system + path: /validate-elbv2-k8s-aws-v1beta1-targetgroupbinding + failurePolicy: Fail + name: vtargetgroupbinding.elbv2.k8s.aws + rules: + - apiGroups: + - elbv2.k8s.aws + apiVersions: + - v1beta1 + operations: + - CREATE + - UPDATE + resources: + - targetgroupbindings + sideEffects: None +- admissionReviewVersions: + - v1beta1 + clientConfig: + service: + name: webhook-service + namespace: system + path: /validate-networking-v1-ingress + failurePolicy: Fail + matchPolicy: Equivalent + name: vingress.elbv2.k8s.aws + rules: + - apiGroups: + - networking.k8s.io + apiVersions: + - v1 + operations: + - CREATE + - UPDATE + resources: + - ingresses + sideEffects: None diff --git a/pkg/ingress/model_build_target_group.go b/pkg/ingress/model_build_target_group.go index 381fd39721..3f6a221a15 100644 --- a/pkg/ingress/model_build_target_group.go +++ b/pkg/ingress/model_build_target_group.go @@ -5,10 +5,11 @@ import ( "crypto/sha256" "encoding/hex" "fmt" - awssdk "github.com/aws/aws-sdk-go-v2/aws" "regexp" "strconv" + awssdk "github.com/aws/aws-sdk-go-v2/aws" + "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -95,7 +96,7 @@ func (t *defaultModelBuildTask) buildTargetGroupBindingSpec(ctx context.Context, }, nil } -func (t *defaultModelBuildTask) buildTargetGroupBindingNetworking(ctx context.Context, targetPort intstr.IntOrString, healthCheckPort intstr.IntOrString) *elbv2model.TargetGroupBindingNetworking { +func (t *defaultModelBuildTask) buildTargetGroupBindingNetworking(_ context.Context, targetPort intstr.IntOrString, healthCheckPort intstr.IntOrString) *elbv2model.TargetGroupBindingNetworking { if t.backendSGIDToken == nil { return nil } diff --git a/pkg/service/model_build_listener.go b/pkg/service/model_build_listener.go index 36aafb697d..68c6efa658 100644 --- a/pkg/service/model_build_listener.go +++ b/pkg/service/model_build_listener.go @@ -19,16 +19,38 @@ func (t *defaultModelBuildTask) buildListeners(ctx context.Context, scheme elbv2 return err } + // group by listener port number + portMap := make(map[int32][]corev1.ServicePort) for _, port := range t.service.Spec.Ports { - _, err := t.buildListener(ctx, port, *cfg, scheme) - if err != nil { - return err + key := port.Port + if vals, exists := portMap[key]; exists { + portMap[key] = append(vals, port) + } else { + portMap[key] = []corev1.ServicePort{port} } } + + // execute build listener + for _, port := range t.service.Spec.Ports { + key := port.Port + if vals, exists := portMap[key]; exists { + if len(vals) > 1 { + port = mergeServicePortsForListener(vals) + } else { + port = vals[0] + } + _, err := t.buildListener(ctx, port, cfg, scheme) + if err != nil { + return err + } + delete(portMap, key) + } + } + return nil } -func (t *defaultModelBuildTask) buildListener(ctx context.Context, port corev1.ServicePort, cfg listenerConfig, +func (t *defaultModelBuildTask) buildListener(ctx context.Context, port corev1.ServicePort, cfg *listenerConfig, scheme elbv2model.LoadBalancerScheme) (*elbv2model.Listener, error) { lsSpec, err := t.buildListenerSpec(ctx, port, cfg, scheme) if err != nil { @@ -39,7 +61,7 @@ func (t *defaultModelBuildTask) buildListener(ctx context.Context, port corev1.S return ls, nil } -func (t *defaultModelBuildTask) buildListenerSpec(ctx context.Context, port corev1.ServicePort, cfg listenerConfig, +func (t *defaultModelBuildTask) buildListenerSpec(ctx context.Context, port corev1.ServicePort, cfg *listenerConfig, scheme elbv2model.LoadBalancerScheme) (elbv2model.ListenerSpec, error) { tgProtocol := elbv2model.Protocol(port.Protocol) listenerProtocol := elbv2model.Protocol(port.Protocol) @@ -149,7 +171,7 @@ func validateTLSPortsSet(rawTLSPorts []string, ports []corev1.ServicePort) error return nil } -func (t *defaultModelBuildTask) buildTLSPortsSet(_ context.Context) (sets.String, error) { +func (t *defaultModelBuildTask) buildTLSPortsSet(_ context.Context) (sets.Set[string], error) { var rawTLSPorts []string _ = t.annotationParser.ParseStringSliceAnnotation(annotations.SvcLBSuffixSSLPorts, &rawTLSPorts, t.service.Annotations) @@ -160,7 +182,7 @@ func (t *defaultModelBuildTask) buildTLSPortsSet(_ context.Context) (sets.String return nil, err } - return sets.NewString(rawTLSPorts...), nil + return sets.New[string](rawTLSPorts...), nil } func (t *defaultModelBuildTask) buildBackendProtocol(_ context.Context) string { @@ -191,7 +213,7 @@ func (t *defaultModelBuildTask) buildListenerALPNPolicy(ctx context.Context, lis type listenerConfig struct { certificates []elbv2model.Certificate - tlsPortsSet sets.String + tlsPortsSet sets.Set[string] sslPolicy *string backendProtocol string } @@ -234,3 +256,24 @@ func (t *defaultModelBuildTask) buildListenerAttributes(ctx context.Context, svc } return attributes, nil } + +func mergeServicePortsForListener(ports []corev1.ServicePort) corev1.ServicePort { + port0 := ports[0] + mergeableProtocols := map[corev1.Protocol]bool{ + corev1.ProtocolTCP: true, + corev1.ProtocolUDP: true, + } + if _, ok := mergeableProtocols[port0.Protocol]; !ok { + return port0 + } + for _, port := range ports[1:] { + if _, ok := mergeableProtocols[port.Protocol]; !ok { + continue + } + if port.NodePort == port0.NodePort && port.Protocol != port0.Protocol { + port0.Protocol = corev1.Protocol("TCP_UDP") + break + } + } + return port0 +} diff --git a/pkg/service/model_build_listener_test.go b/pkg/service/model_build_listener_test.go index 9fd455afa0..cbf3686dbc 100644 --- a/pkg/service/model_build_listener_test.go +++ b/pkg/service/model_build_listener_test.go @@ -159,7 +159,7 @@ func Test_defaultModelBuilderTask_buildListenerConfig(t *testing.T) { }, want: &listenerConfig{ certificates: ([]elbv2model.Certificate)(nil), - tlsPortsSet: sets.NewString("83"), + tlsPortsSet: sets.New[string]("83"), sslPolicy: new(string), backendProtocol: "", }, @@ -301,3 +301,220 @@ func Test_defaultModelBuilderTask_buildListenerAttributes(t *testing.T) { }) } } + +func Test_mergeServicePortsForListener(t *testing.T) { + tests := []struct { + name string + ports []corev1.ServicePort + want corev1.ServicePort + }{ + { + name: "one port", + ports: []corev1.ServicePort{ + { + Name: "p1", + Port: 80, + TargetPort: intstr.FromInt(80), + Protocol: corev1.ProtocolTCP, + NodePort: 31223, + }, + }, + want: corev1.ServicePort{ + Name: "p1", + Port: 80, + TargetPort: intstr.FromInt(80), + Protocol: corev1.ProtocolTCP, + NodePort: 31223, + }, + }, + { + name: "two tcp ports, different target and node ports", + ports: []corev1.ServicePort{ + { + Name: "p1", + Port: 80, + TargetPort: intstr.FromInt(80), + Protocol: corev1.ProtocolTCP, + NodePort: 31223, + }, + { + Name: "p2", + Port: 80, + TargetPort: intstr.FromInt(8888), + Protocol: corev1.ProtocolTCP, + NodePort: 31224, + }, + }, + want: corev1.ServicePort{ + Name: "p1", + Port: 80, + TargetPort: intstr.FromInt(80), + Protocol: corev1.ProtocolTCP, + NodePort: 31223, + }, + }, + { + name: "two udp ports, different target and node ports", + ports: []corev1.ServicePort{ + { + Name: "p1", + Port: 80, + TargetPort: intstr.FromInt(80), + Protocol: corev1.ProtocolUDP, + NodePort: 31223, + }, + { + Name: "p2", + Port: 80, + TargetPort: intstr.FromInt(8888), + Protocol: corev1.ProtocolUDP, + NodePort: 31224, + }, + }, + want: corev1.ServicePort{ + Name: "p1", + Port: 80, + TargetPort: intstr.FromInt(80), + Protocol: corev1.ProtocolUDP, + NodePort: 31223, + }, + }, + { + name: "one tcp and one udp, different target and node ports", + ports: []corev1.ServicePort{ + { + Name: "p1", + Port: 80, + TargetPort: intstr.FromInt(80), + Protocol: corev1.ProtocolTCP, + NodePort: 31223, + }, + { + Name: "p2", + Port: 80, + TargetPort: intstr.FromInt(8888), + Protocol: corev1.ProtocolUDP, + NodePort: 31224, + }, + }, + want: corev1.ServicePort{ + Name: "p1", + Port: 80, + TargetPort: intstr.FromInt(80), + Protocol: corev1.ProtocolTCP, + NodePort: 31223, + }, + }, + { + name: "one tcp and one udp, same target and node ports", + ports: []corev1.ServicePort{ + { + Name: "p1", + Port: 80, + TargetPort: intstr.FromInt(80), + Protocol: corev1.ProtocolTCP, + NodePort: 31223, + }, + { + Name: "p2", + Port: 80, + TargetPort: intstr.FromInt(80), + Protocol: corev1.ProtocolUDP, + NodePort: 31223, + }, + }, + want: corev1.ServicePort{ + Name: "p1", + Port: 80, + TargetPort: intstr.FromInt(80), + Protocol: corev1.Protocol("TCP_UDP"), + NodePort: 31223, + }, + }, + { + name: "one udp and one tcp, same target and node ports", + ports: []corev1.ServicePort{ + { + Name: "p1", + Port: 80, + TargetPort: intstr.FromInt(80), + Protocol: corev1.ProtocolUDP, + NodePort: 31223, + }, + { + Name: "p2", + Port: 80, + TargetPort: intstr.FromInt(80), + Protocol: corev1.ProtocolTCP, + NodePort: 31223, + }, + }, + want: corev1.ServicePort{ + Name: "p1", + Port: 80, + TargetPort: intstr.FromInt(80), + Protocol: corev1.Protocol("TCP_UDP"), + NodePort: 31223, + }, + }, + { + name: "one tcp and one udp, same node port, different target port", + ports: []corev1.ServicePort{ + { + Name: "p1", + Port: 80, + TargetPort: intstr.FromInt(80), + Protocol: corev1.ProtocolTCP, + NodePort: 31223, + }, + { + Name: "p2", + Port: 80, + TargetPort: intstr.FromInt(8888), + Protocol: corev1.ProtocolUDP, + NodePort: 31223, + }, + }, + want: corev1.ServicePort{ + Name: "p1", + Port: 80, + TargetPort: intstr.FromInt(80), + Protocol: corev1.Protocol("TCP_UDP"), + NodePort: 31223, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + port := mergeServicePortsForListener(tt.ports) + assert.Equal(t, port.Name, tt.want.Name) + assert.Equal(t, port.Port, tt.want.Port) + assert.Equal(t, port.TargetPort.IntVal, tt.want.TargetPort.IntVal) + assert.Equal(t, port.Protocol, tt.want.Protocol) + assert.Equal(t, port.NodePort, tt.want.NodePort) + }) + } + + // test that function returns new ServicePort instance + p1 := corev1.ServicePort{ + Name: "p1", + Port: 80, + TargetPort: intstr.FromInt(80), + Protocol: corev1.ProtocolTCP, + NodePort: 31223, + } + p2 := corev1.ServicePort{ + Name: "p2", + Port: 80, + TargetPort: intstr.FromInt(80), + Protocol: corev1.ProtocolUDP, + NodePort: 31223, + } + ports := []corev1.ServicePort{p1, p2} + mergedPort := mergeServicePortsForListener(ports) + + assert.Equal(t, corev1.ProtocolTCP, p1.Protocol) + assert.Equal(t, corev1.Protocol("TCP_UDP"), mergedPort.Protocol) + assert.NotEqual(t, &p1, &mergedPort) +} diff --git a/pkg/service/model_build_target_group.go b/pkg/service/model_build_target_group.go index 831d6c6bcd..4ac0495086 100644 --- a/pkg/service/model_build_target_group.go +++ b/pkg/service/model_build_target_group.go @@ -553,15 +553,24 @@ func (t *defaultModelBuildTask) buildTargetGroupBindingNetworkingLegacy(ctx cont return nil, nil } tgProtocol := port.Protocol - networkingProtocol := elbv2api.NetworkingProtocolTCP healthCheckProtocol := elbv2api.NetworkingProtocolTCP - if tgProtocol == corev1.ProtocolUDP { + var networkingProtocol elbv2api.NetworkingProtocol + switch tgProtocol { + case corev1.ProtocolUDP: networkingProtocol = elbv2api.NetworkingProtocolUDP + case corev1.Protocol("TCP_UDP"): + networkingProtocol = elbv2api.NetworkingProtocolTCP_UDP + default: + networkingProtocol = elbv2api.NetworkingProtocolTCP } loadBalancerSubnetCIDRs := t.getLoadBalancerSubnetsSourceRanges(targetGroupIPAddressType) trafficSource := loadBalancerSubnetCIDRs defaultRangeUsed := false - if networkingProtocol == elbv2api.NetworkingProtocolUDP || t.preserveClientIP { + var trafficPorts []elbv2api.NetworkingPort + switch networkingProtocol { + case elbv2api.NetworkingProtocolTCP_UDP: + tcpProtocol := elbv2api.NetworkingProtocolTCP + udpProtocol := elbv2api.NetworkingProtocolUDP trafficSource = t.getLoadBalancerSourceRanges(ctx) if len(trafficSource) == 0 { trafficSource, err = t.getDefaultIPSourceRanges(ctx, targetGroupIPAddressType, port.Protocol, scheme) @@ -570,17 +579,39 @@ func (t *defaultModelBuildTask) buildTargetGroupBindingNetworkingLegacy(ctx cont } defaultRangeUsed = true } + trafficPorts = []elbv2api.NetworkingPort{ + { + Port: &tgPort, + Protocol: &tcpProtocol, + }, + { + Port: &tgPort, + Protocol: &udpProtocol, + }, + } + default: + if networkingProtocol == elbv2api.NetworkingProtocolUDP || t.preserveClientIP { + trafficSource = t.getLoadBalancerSourceRanges(ctx) + if len(trafficSource) == 0 { + trafficSource, err = t.getDefaultIPSourceRanges(ctx, targetGroupIPAddressType, port.Protocol, scheme) + if err != nil { + return nil, err + } + defaultRangeUsed = true + } + } + trafficPorts = []elbv2api.NetworkingPort{ + { + Port: &tgPort, + Protocol: &networkingProtocol, + }, + } } tgbNetworking := &elbv2model.TargetGroupBindingNetworking{ Ingress: []elbv2model.NetworkingIngressRule{ { - From: t.buildPeersFromSourceRangeCIDRs(ctx, trafficSource), - Ports: []elbv2api.NetworkingPort{ - { - Port: &tgPort, - Protocol: &networkingProtocol, - }, - }, + From: t.buildPeersFromSourceRangeCIDRs(ctx, trafficSource), + Ports: trafficPorts, }, }, } diff --git a/pkg/service/model_build_target_group_test.go b/pkg/service/model_build_target_group_test.go index fbf65903da..f1909957fe 100644 --- a/pkg/service/model_build_target_group_test.go +++ b/pkg/service/model_build_target_group_test.go @@ -3,15 +3,16 @@ package service import ( "context" "errors" - ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types" - "github.com/golang/mock/gomock" - "sigs.k8s.io/aws-load-balancer-controller/pkg/model/core" - "sigs.k8s.io/aws-load-balancer-controller/pkg/networking" "sort" "strconv" "testing" "github.com/aws/aws-sdk-go-v2/aws" + ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types" + "github.com/golang/mock/gomock" + "sigs.k8s.io/aws-load-balancer-controller/pkg/model/core" + "sigs.k8s.io/aws-load-balancer-controller/pkg/networking" + "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -1228,6 +1229,59 @@ func Test_defaultModelBuilderTask_buildTargetGroupBindingNetworkingLegacy(t *tes ipAddressType: elbv2.TargetGroupIPAddressTypeIPv4, want: nil, }, + { + name: "tcpudp-service with no source ranges configuration", + svc: &corev1.Service{}, + tgPort: port80, + hcPort: port808, + scheme: elbv2.LoadBalancerSchemeInternetFacing, + subnets: []ec2types.Subnet{ + { + CidrBlock: aws.String("172.16.0.0/19"), + SubnetId: aws.String("az-1"), + }, + }, + tgProtocol: corev1.Protocol("TCP_UDP"), + ipAddressType: elbv2.TargetGroupIPAddressTypeIPv4, + want: &elbv2.TargetGroupBindingNetworking{ + Ingress: []elbv2.NetworkingIngressRule{ + { + From: []elbv2.NetworkingPeer{ + { + IPBlock: &elbv2api.IPBlock{ + CIDR: "172.16.0.0/19", + }, + }, + }, + Ports: []elbv2api.NetworkingPort{ + { + Protocol: &networkingProtocolTCP, + Port: &port80, + }, + { + Protocol: &networkingProtocolUDP, + Port: &port80, + }, + }, + }, + { + From: []elbv2.NetworkingPeer{ + { + IPBlock: &elbv2api.IPBlock{ + CIDR: "172.16.0.0/19", + }, + }, + }, + Ports: []elbv2api.NetworkingPort{ + { + Protocol: &networkingProtocolTCP, + Port: &port808, + }, + }, + }, + }, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/service/model_builder_test.go b/pkg/service/model_builder_test.go index 44db576aef..576a92e64a 100644 --- a/pkg/service/model_builder_test.go +++ b/pkg/service/model_builder_test.go @@ -2250,6 +2250,264 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { listLoadBalancerCalls: []listLoadBalancerCall{listLoadBalancerCallForEmptyLB}, wantError: true, }, + { + testName: "Instance mode, TCP/UDP same port test", + svc: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "tcpudp-protocol", + Namespace: "app", + Annotations: map[string]string{ + "service.beta.kubernetes.io/aws-load-balancer-type": "external", + "service.beta.kubernetes.io/aws-load-balancer-scheme": "internet-facing", + "service.beta.kubernetes.io/aws-load-balancer-nlb-target-type": "instance", + }, + UID: "2dc098f0-ae33-4378-af7b-83e2a0424495", + }, + Spec: corev1.ServiceSpec{ + ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyTypeCluster, + Type: corev1.ServiceTypeLoadBalancer, + Selector: map[string]string{"app": "hello"}, + HealthCheckNodePort: 29123, + Ports: []corev1.ServicePort{ + { + Name: "p1", + Port: 80, + TargetPort: intstr.FromInt(80), + Protocol: corev1.ProtocolTCP, + NodePort: 31223, + }, + { + Name: "p2", + Port: 80, + TargetPort: intstr.FromInt(80), + Protocol: corev1.ProtocolUDP, + NodePort: 31223, + }, + { + Name: "alt2", + Port: 83, + TargetPort: intstr.FromInt(8883), + Protocol: corev1.ProtocolTCP, + NodePort: 32323, + }, + }, + }, + }, + resolveViaDiscoveryCalls: []resolveViaDiscoveryCall{resolveViaDiscoveryCallForThreeSubnet}, + listLoadBalancerCalls: []listLoadBalancerCall{listLoadBalancerCallForEmptyLB}, + wantError: false, + wantValue: ` +{ + "id":"app/tcpudp-protocol", + "resources":{ + "AWS::ElasticLoadBalancingV2::Listener":{ + "80":{ + "spec":{ + "loadBalancerARN":{ + "$ref":"#/resources/AWS::ElasticLoadBalancingV2::LoadBalancer/LoadBalancer/status/loadBalancerARN" + }, + "port":80, + "protocol":"TCP_UDP", + "defaultActions":[ + { + "type":"forward", + "forwardConfig":{ + "targetGroups":[ + { + "targetGroupARN":{ + "$ref":"#/resources/AWS::ElasticLoadBalancingV2::TargetGroup/app/tcpudp-protocol:80/status/targetGroupARN" + } + } + ] + } + } + ] + } + }, + "83":{ + "spec":{ + "loadBalancerARN":{ + "$ref":"#/resources/AWS::ElasticLoadBalancingV2::LoadBalancer/LoadBalancer/status/loadBalancerARN" + }, + "port":83, + "protocol":"TCP", + "defaultActions":[ + { + "type":"forward", + "forwardConfig":{ + "targetGroups":[ + { + "targetGroupARN":{ + "$ref":"#/resources/AWS::ElasticLoadBalancingV2::TargetGroup/app/tcpudp-protocol:83/status/targetGroupARN" + } + } + ] + } + } + ] + } + } + }, + "AWS::ElasticLoadBalancingV2::LoadBalancer":{ + "LoadBalancer":{ + "spec":{ + "name":"k8s-app-tcpudppr-2af705447d", + "type":"network", + "scheme":"internet-facing", + "ipAddressType":"ipv4", + "subnetMapping":[ + { + "subnetID":"subnet-1" + }, + { + "subnetID":"subnet-2" + }, + { + "subnetID":"subnet-3" + } + ] + } + } + }, + "AWS::ElasticLoadBalancingV2::TargetGroup":{ + "app/tcpudp-protocol:80":{ + "spec":{ + "ipAddressType":"ipv4", + "name":"k8s-app-tcpudppr-2213a0d759", + "targetType":"instance", + "port":31223, + "protocol":"TCP_UDP", + "healthCheckConfig":{ + "port":"traffic-port", + "protocol":"TCP", + "unhealthyThresholdCount":3, + "healthyThresholdCount":3, + "intervalSeconds":10 + }, + "targetGroupAttributes":[ + { + "key":"proxy_protocol_v2.enabled", + "value":"false" + } + ] + } + }, + "app/tcpudp-protocol:83":{ + "spec":{ + "ipAddressType":"ipv4", + "name":"k8s-app-tcpudppr-c200165858", + "targetType":"instance", + "port":32323, + "protocol":"TCP", + "healthCheckConfig":{ + "port":"traffic-port", + "protocol":"TCP", + "unhealthyThresholdCount":3, + "healthyThresholdCount":3, + "intervalSeconds":10 + }, + "targetGroupAttributes":[ + { + "key":"proxy_protocol_v2.enabled", + "value":"false" + } + ] + } + } + }, + "K8S::ElasticLoadBalancingV2::TargetGroupBinding":{ + "app/tcpudp-protocol:80":{ + "spec":{ + "template":{ + "metadata":{ + "creationTimestamp":null, + "name":"k8s-app-tcpudppr-2213a0d759", + "namespace":"app" + }, + "spec":{ + "targetGroupARN":{ + "$ref":"#/resources/AWS::ElasticLoadBalancingV2::TargetGroup/app/tcpudp-protocol:80/status/targetGroupARN" + }, + "targetType":"instance", + "serviceRef":{ + "name":"tcpudp-protocol", + "port":80 + }, + "ipAddressType": "ipv4", + "networking":{ + "ingress":[ + { + "from":[ + { + "ipBlock":{ + "cidr":"0.0.0.0/0" + } + } + ], + "ports":[ + { + "protocol":"TCP", + "port":31223 + }, + { + "protocol":"UDP", + "port":31223 + } + ] + } + ] + } + } + } + } + }, + "app/tcpudp-protocol:83":{ + "spec":{ + "template":{ + "metadata":{ + "name":"k8s-app-tcpudppr-c200165858", + "namespace":"app", + "creationTimestamp":null + }, + "spec":{ + "targetGroupARN":{ + "$ref":"#/resources/AWS::ElasticLoadBalancingV2::TargetGroup/app/tcpudp-protocol:83/status/targetGroupARN" + }, + "targetType":"instance", + "serviceRef":{ + "name":"tcpudp-protocol", + "port":83 + }, + "ipAddressType": "ipv4", + "networking":{ + "ingress":[ + { + "from":[ + { + "ipBlock":{ + "cidr":"0.0.0.0/0" + } + } + ], + "ports":[ + { + "protocol":"TCP", + "port":32323 + } + ] + } + ] + } + } + } + } + } + } + } +} +`, + wantNumResources: 7, + }, { testName: "list load balancers error", svc: &corev1.Service{ From 08431ef831b3c520a2a918b081647f99ce474ec3 Mon Sep 17 00:00:00 2001 From: Kevin Lyda Date: Mon, 14 Oct 2024 12:51:01 +0100 Subject: [PATCH 3/7] Attempt to address remaining errors --- pkg/service/model_build_target_group.go | 27 +++----- pkg/service/model_builder_test.go | 92 ++++++++++++++++++++----- 2 files changed, 83 insertions(+), 36 deletions(-) diff --git a/pkg/service/model_build_target_group.go b/pkg/service/model_build_target_group.go index 4ac0495086..318964ea2d 100644 --- a/pkg/service/model_build_target_group.go +++ b/pkg/service/model_build_target_group.go @@ -484,6 +484,8 @@ func (t *defaultModelBuildTask) buildTargetGroupBindingNetworking(_ context.Cont Protocol: &protocolTCP, Port: &tgPort, }) + case corev1.Protocol("TCP_UDP"): + fallthrough case corev1.ProtocolUDP: ports = append(ports, elbv2api.NetworkingPort{ Protocol: &protocolUDP, @@ -567,18 +569,19 @@ func (t *defaultModelBuildTask) buildTargetGroupBindingNetworkingLegacy(ctx cont trafficSource := loadBalancerSubnetCIDRs defaultRangeUsed := false var trafficPorts []elbv2api.NetworkingPort - switch networkingProtocol { - case elbv2api.NetworkingProtocolTCP_UDP: - tcpProtocol := elbv2api.NetworkingProtocolTCP - udpProtocol := elbv2api.NetworkingProtocolUDP + if networkingProtocol == elbv2api.NetworkingProtocolUDP || t.preserveClientIP { trafficSource = t.getLoadBalancerSourceRanges(ctx) if len(trafficSource) == 0 { - trafficSource, err = t.getDefaultIPSourceRanges(ctx, targetGroupIPAddressType, port.Protocol, scheme) + trafficSource, err = t.getDefaultIPSourceRanges(ctx, targetGroupIPAddressType, tgProtocol, scheme) if err != nil { return nil, err } defaultRangeUsed = true } + } + if networkingProtocol == elbv2api.NetworkingProtocolTCP_UDP { + tcpProtocol := elbv2api.NetworkingProtocolTCP + udpProtocol := elbv2api.NetworkingProtocolUDP trafficPorts = []elbv2api.NetworkingPort{ { Port: &tgPort, @@ -589,17 +592,7 @@ func (t *defaultModelBuildTask) buildTargetGroupBindingNetworkingLegacy(ctx cont Protocol: &udpProtocol, }, } - default: - if networkingProtocol == elbv2api.NetworkingProtocolUDP || t.preserveClientIP { - trafficSource = t.getLoadBalancerSourceRanges(ctx) - if len(trafficSource) == 0 { - trafficSource, err = t.getDefaultIPSourceRanges(ctx, targetGroupIPAddressType, port.Protocol, scheme) - if err != nil { - return nil, err - } - defaultRangeUsed = true - } - } + } else { trafficPorts = []elbv2api.NetworkingPort{ { Port: &tgPort, @@ -640,7 +633,7 @@ func (t *defaultModelBuildTask) getDefaultIPSourceRanges(ctx context.Context, ta if targetGroupIPAddressType == elbv2model.TargetGroupIPAddressTypeIPv6 { defaultSourceRanges = t.defaultIPv6SourceRanges } - if (protocol == corev1.ProtocolUDP || t.preserveClientIP) && scheme == elbv2model.LoadBalancerSchemeInternal { + if (protocol == corev1.Protocol("TCP_UDP") || protocol == corev1.ProtocolUDP || t.preserveClientIP) && scheme == elbv2model.LoadBalancerSchemeInternal { vpcInfo, err := t.vpcInfoProvider.FetchVPCInfo(ctx, t.vpcID, networking.FetchVPCInfoWithoutCache()) if err != nil { return nil, err diff --git a/pkg/service/model_builder_test.go b/pkg/service/model_builder_test.go index 576a92e64a..66a18d03f3 100644 --- a/pkg/service/model_builder_test.go +++ b/pkg/service/model_builder_test.go @@ -2,11 +2,12 @@ package service import ( "context" - ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types" - elbv2types "github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2/types" "testing" "time" + ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types" + elbv2types "github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2/types" + awssdk "github.com/aws/aws-sdk-go-v2/aws" "github.com/go-logr/logr" "github.com/golang/mock/gomock" @@ -2300,6 +2301,46 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { { "id":"app/tcpudp-protocol", "resources":{ + "AWS::EC2::SecurityGroup":{ + "ManagedLBSecurityGroup":{ + "spec":{ + "description":"[k8s] Managed SecurityGroup for LoadBalancer", + "groupName":"k8s-app-tcpudppr-06a9156bf8", + "ingress":[ + { + "fromPort":80, + "ipProtocol":"tcp", + "ipRanges":[ + { + "cidrIP":"0.0.0.0/0" + } + ], + "toPort":80 + }, + { + "fromPort":80, + "ipProtocol":"udp", + "ipRanges":[ + { + "cidrIP":"0.0.0.0/0" + } + ], + "toPort":80 + }, + { + "fromPort":83, + "ipProtocol":"tcp", + "ipRanges":[ + { + "cidrIP":"0.0.0.0/0" + } + ], + "toPort":83 + } + ] + } + } + }, "AWS::ElasticLoadBalancingV2::Listener":{ "80":{ "spec":{ @@ -2354,6 +2395,11 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "name":"k8s-app-tcpudppr-2af705447d", "type":"network", "scheme":"internet-facing", + "securityGroups":[ + { + "$ref":"#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" + } + ], "ipAddressType":"ipv4", "subnetMapping":[ { @@ -2380,6 +2426,7 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "healthCheckConfig":{ "port":"traffic-port", "protocol":"TCP", + "timeoutSeconds":10, "unhealthyThresholdCount":3, "healthyThresholdCount":3, "intervalSeconds":10 @@ -2402,6 +2449,7 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "healthCheckConfig":{ "port":"traffic-port", "protocol":"TCP", + "timeoutSeconds":10, "unhealthyThresholdCount":3, "healthyThresholdCount":3, "intervalSeconds":10 @@ -2429,6 +2477,7 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "$ref":"#/resources/AWS::ElasticLoadBalancingV2::TargetGroup/app/tcpudp-protocol:80/status/targetGroupARN" }, "targetType":"instance", + "vpcID":"vpc-xxx", "serviceRef":{ "name":"tcpudp-protocol", "port":80 @@ -2439,18 +2488,20 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { { "from":[ { - "ipBlock":{ - "cidr":"0.0.0.0/0" + "securityGroup": { + "groupID": { + "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" + } } } ], "ports":[ { - "protocol":"TCP", + "protocol":"UDP", "port":31223 }, { - "protocol":"UDP", + "protocol":"TCP", "port":31223 } ] @@ -2474,6 +2525,7 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "$ref":"#/resources/AWS::ElasticLoadBalancingV2::TargetGroup/app/tcpudp-protocol:83/status/targetGroupARN" }, "targetType":"instance", + "vpcID":"vpc-xxx", "serviceRef":{ "name":"tcpudp-protocol", "port":83 @@ -2484,8 +2536,10 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { { "from":[ { - "ipBlock":{ - "cidr":"0.0.0.0/0" + "securityGroup": { + "groupID": { + "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" + } } } ], @@ -2506,7 +2560,7 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { } } `, - wantNumResources: 7, + wantNumResources: 8, }, { testName: "list load balancers error", @@ -3645,7 +3699,7 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { ], "ports":[ { - "port": 80, + "port": 80, "protocol":"TCP" } ] @@ -3899,12 +3953,12 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { } ], "ports":[ - { - "port": 80, + { + "port": 80, "protocol":"TCP" }, - { - "port": 8888, + { + "port": 8888, "protocol":"TCP" } ] @@ -4540,9 +4594,9 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "from":[ { "securityGroup":{ - "groupID": { - "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" - } + "groupID": { + "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" + } } } ], @@ -4584,8 +4638,8 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { { "securityGroup":{ "groupID": { - "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" - } + "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" + } } } ], From c00e52cfa872bf717f33ae50a4d65bdacb20e487 Mon Sep 17 00:00:00 2001 From: Kevin Lyda Date: Tue, 5 Nov 2024 13:23:42 +0000 Subject: [PATCH 4/7] Remove spurious tabs --- pkg/service/model_builder_test.go | 340 +++++++++++++++--------------- 1 file changed, 170 insertions(+), 170 deletions(-) diff --git a/pkg/service/model_builder_test.go b/pkg/service/model_builder_test.go index 66a18d03f3..d735981058 100644 --- a/pkg/service/model_builder_test.go +++ b/pkg/service/model_builder_test.go @@ -560,8 +560,8 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "healthyThresholdCount":2, "unhealthyThresholdCount":2, "matcher":{ - "httpCode": "200-399" - } + "httpCode": "200-399" + } }, "targetGroupAttributes":[ { @@ -587,8 +587,8 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "healthyThresholdCount":2, "unhealthyThresholdCount":2, "matcher":{ - "httpCode": "200-399" - } + "httpCode": "200-399" + } }, "targetGroupAttributes":[ { @@ -908,8 +908,8 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "healthyThresholdCount":2, "unhealthyThresholdCount":2, "matcher":{ - "httpCode": "200-399" - } + "httpCode": "200-399" + } }, "targetGroupAttributes":[ { @@ -935,8 +935,8 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "healthyThresholdCount":2, "unhealthyThresholdCount":2, "matcher":{ - "httpCode": "200-399" - } + "httpCode": "200-399" + } }, "targetGroupAttributes":[ { @@ -1526,8 +1526,8 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "healthyThresholdCount":2, "unhealthyThresholdCount":2, "matcher":{ - "httpCode": "200-399" - } + "httpCode": "200-399" + } }, "targetGroupAttributes":[ { @@ -1553,8 +1553,8 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "healthyThresholdCount":2, "unhealthyThresholdCount":2, "matcher":{ - "httpCode": "200-399" - } + "httpCode": "200-399" + } }, "targetGroupAttributes":[ { @@ -2145,21 +2145,21 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { }, "networking": { "ingress": [ - { - "from":[ - { - "ipBlock":{ - "cidr":"192.168.0.0/19" - } - } - ], - "ports":[ - { - "protocol":"TCP", - "port":80 - } - ] - } + { + "from":[ + { + "ipBlock":{ + "cidr":"192.168.0.0/19" + } + } + ], + "ports":[ + { + "protocol":"TCP", + "port":80 + } + ] + } ] }, "serviceRef": { @@ -2301,46 +2301,46 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { { "id":"app/tcpudp-protocol", "resources":{ - "AWS::EC2::SecurityGroup":{ - "ManagedLBSecurityGroup":{ - "spec":{ - "description":"[k8s] Managed SecurityGroup for LoadBalancer", - "groupName":"k8s-app-tcpudppr-06a9156bf8", - "ingress":[ - { - "fromPort":80, - "ipProtocol":"tcp", - "ipRanges":[ - { - "cidrIP":"0.0.0.0/0" - } - ], - "toPort":80 - }, - { - "fromPort":80, - "ipProtocol":"udp", - "ipRanges":[ - { - "cidrIP":"0.0.0.0/0" - } - ], - "toPort":80 - }, - { - "fromPort":83, - "ipProtocol":"tcp", - "ipRanges":[ - { - "cidrIP":"0.0.0.0/0" - } - ], - "toPort":83 - } - ] - } - } - }, + "AWS::EC2::SecurityGroup":{ + "ManagedLBSecurityGroup":{ + "spec":{ + "description":"[k8s] Managed SecurityGroup for LoadBalancer", + "groupName":"k8s-app-tcpudppr-06a9156bf8", + "ingress":[ + { + "fromPort":80, + "ipProtocol":"tcp", + "ipRanges":[ + { + "cidrIP":"0.0.0.0/0" + } + ], + "toPort":80 + }, + { + "fromPort":80, + "ipProtocol":"udp", + "ipRanges":[ + { + "cidrIP":"0.0.0.0/0" + } + ], + "toPort":80 + }, + { + "fromPort":83, + "ipProtocol":"tcp", + "ipRanges":[ + { + "cidrIP":"0.0.0.0/0" + } + ], + "toPort":83 + } + ] + } + } + }, "AWS::ElasticLoadBalancingV2::Listener":{ "80":{ "spec":{ @@ -2395,11 +2395,11 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "name":"k8s-app-tcpudppr-2af705447d", "type":"network", "scheme":"internet-facing", - "securityGroups":[ - { - "$ref":"#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" - } - ], + "securityGroups":[ + { + "$ref":"#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" + } + ], "ipAddressType":"ipv4", "subnetMapping":[ { @@ -2426,7 +2426,7 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "healthCheckConfig":{ "port":"traffic-port", "protocol":"TCP", - "timeoutSeconds":10, + "timeoutSeconds":10, "unhealthyThresholdCount":3, "healthyThresholdCount":3, "intervalSeconds":10 @@ -2449,7 +2449,7 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "healthCheckConfig":{ "port":"traffic-port", "protocol":"TCP", - "timeoutSeconds":10, + "timeoutSeconds":10, "unhealthyThresholdCount":3, "healthyThresholdCount":3, "intervalSeconds":10 @@ -2477,7 +2477,7 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "$ref":"#/resources/AWS::ElasticLoadBalancingV2::TargetGroup/app/tcpudp-protocol:80/status/targetGroupARN" }, "targetType":"instance", - "vpcID":"vpc-xxx", + "vpcID":"vpc-xxx", "serviceRef":{ "name":"tcpudp-protocol", "port":80 @@ -2488,10 +2488,10 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { { "from":[ { - "securityGroup": { - "groupID": { - "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" - } + "securityGroup": { + "groupID": { + "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" + } } } ], @@ -2525,7 +2525,7 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "$ref":"#/resources/AWS::ElasticLoadBalancingV2::TargetGroup/app/tcpudp-protocol:83/status/targetGroupARN" }, "targetType":"instance", - "vpcID":"vpc-xxx", + "vpcID":"vpc-xxx", "serviceRef":{ "name":"tcpudp-protocol", "port":83 @@ -2536,10 +2536,10 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { { "from":[ { - "securityGroup": { - "groupID": { - "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" - } + "securityGroup": { + "groupID": { + "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" + } } } ], @@ -3451,10 +3451,10 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { ], "securityGroups": [ { - "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" + "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" }, "sg-backend" - ] + ] } } }, @@ -3635,10 +3635,10 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { ], "securityGroups": [ { - "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" + "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" }, "sg-backend" - ] + ] } } }, @@ -3699,7 +3699,7 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { ], "ports":[ { - "port": 80, + "port": 80, "protocol":"TCP" } ] @@ -3859,10 +3859,10 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { ], "securityGroups": [ { - "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" + "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" }, "sg-backend" - ] + ] } } }, @@ -3883,8 +3883,8 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "healthyThresholdCount":2, "unhealthyThresholdCount":2, "matcher":{ - "httpCode": "200-399" - } + "httpCode": "200-399" + } }, "targetGroupAttributes":[ { @@ -3910,8 +3910,8 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "healthyThresholdCount":2, "unhealthyThresholdCount":2, "matcher":{ - "httpCode": "200-399" - } + "httpCode": "200-399" + } }, "targetGroupAttributes":[ { @@ -3953,12 +3953,12 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { } ], "ports":[ - { - "port": 80, + { + "port": 80, "protocol":"TCP" }, - { - "port": 8888, + { + "port": 8888, "protocol":"TCP" } ] @@ -4000,11 +4000,11 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { ], "ports":[ { - "port": 80, + "port": 80, "protocol":"TCP" }, - { - "port": 8888, + { + "port": 8888, "protocol":"TCP" } ] @@ -4200,10 +4200,10 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { ], "securityGroups": [ { - "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" + "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" }, "sg-backend" - ] + ] } } }, @@ -4224,8 +4224,8 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "healthyThresholdCount":2, "unhealthyThresholdCount":2, "matcher":{ - "httpCode": "200-399" - } + "httpCode": "200-399" + } }, "targetGroupAttributes":[ { @@ -4251,8 +4251,8 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "healthyThresholdCount":2, "unhealthyThresholdCount":2, "matcher":{ - "httpCode": "200-399" - } + "httpCode": "200-399" + } }, "targetGroupAttributes":[ { @@ -4295,7 +4295,7 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { ], "ports":[ { - "port": 80, + "port": 80, "protocol":"TCP" } ] @@ -4337,11 +4337,11 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { ], "ports":[ { - "port": 8883, + "port": 8883, "protocol":"TCP" }, - { - "port": 80, + { + "port": 80, "protocol":"TCP" } ] @@ -4514,9 +4514,9 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { ], "securityGroups": [ { - "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" + "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" } - ] + ] } } }, @@ -4594,9 +4594,9 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "from":[ { "securityGroup":{ - "groupID": { - "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" - } + "groupID": { + "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" + } } } ], @@ -4638,8 +4638,8 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { { "securityGroup":{ "groupID": { - "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" - } + "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" + } } } ], @@ -4847,9 +4847,9 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { ], "securityGroups": [ { - "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" + "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" } - ] + ] } } }, @@ -4864,14 +4864,14 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "healthCheckConfig":{ "port": 29123, "protocol":"HTTP", - "path":"/healthz", + "path":"/healthz", "intervalSeconds":10, "timeoutSeconds":6, "healthyThresholdCount":2, "unhealthyThresholdCount":2, "matcher":{ - "httpCode": "200-399" - } + "httpCode": "200-399" + } }, "targetGroupAttributes":[ { @@ -4891,14 +4891,14 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "healthCheckConfig":{ "port": 29123, "protocol":"HTTP", - "path":"/healthz", + "path":"/healthz", "intervalSeconds":10, "timeoutSeconds":6, "healthyThresholdCount":2, "unhealthyThresholdCount":2, "matcher":{ - "httpCode": "200-399" - } + "httpCode": "200-399" + } }, "targetGroupAttributes":[ { @@ -4936,18 +4936,18 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { { "securityGroup":{ "groupID": { - "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" - } + "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" + } } } ], "ports":[ { - "port": 31223, + "port": 31223, "protocol":"TCP" }, - { - "port": 29123, + { + "port": 29123, "protocol":"TCP" } ] @@ -4983,19 +4983,19 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "from":[ { "securityGroup":{ - "groupID": { - "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" - } + "groupID": { + "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" + } } } ], "ports":[ { - "port": 32323, + "port": 32323, "protocol":"TCP" }, - { - "port": 29123, + { + "port": 29123, "protocol":"TCP" } ] @@ -5198,9 +5198,9 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { ], "securityGroups": [ { - "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" + "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" } - ] + ] } } }, @@ -5215,14 +5215,14 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "healthCheckConfig":{ "port": 29123, "protocol":"HTTP", - "path":"/healthz", + "path":"/healthz", "intervalSeconds":10, "timeoutSeconds":6, "healthyThresholdCount":2, "unhealthyThresholdCount":2, "matcher":{ - "httpCode": "200-399" - } + "httpCode": "200-399" + } }, "targetGroupAttributes":[ { @@ -5242,14 +5242,14 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "healthCheckConfig":{ "port": 29123, "protocol":"HTTP", - "path":"/healthz", + "path":"/healthz", "intervalSeconds":10, "timeoutSeconds":6, "healthyThresholdCount":2, "unhealthyThresholdCount":2, "matcher":{ - "httpCode": "200-399" - } + "httpCode": "200-399" + } }, "targetGroupAttributes":[ { @@ -5287,18 +5287,18 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { { "securityGroup":{ "groupID": { - "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" - } + "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" + } } } ], "ports":[ - { - "port": 31223, + { + "port": 31223, "protocol":"UDP" }, - { - "port": 29123, + { + "port": 29123, "protocol":"TCP" } ] @@ -5334,19 +5334,19 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "from":[ { "securityGroup":{ - "groupID": { - "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" - } + "groupID": { + "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" + } } } ], "ports":[ - { - "port": 32323, + { + "port": 32323, "protocol":"UDP" }, - { - "port": 29123, + { + "port": 29123, "protocol":"TCP" } ] @@ -5480,7 +5480,7 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "port":"traffic-port", "protocol":"TCP", "intervalSeconds":10, - "timeoutSeconds": 10, + "timeoutSeconds": 10, "healthyThresholdCount":3, "unhealthyThresholdCount":3 }, @@ -5660,7 +5660,7 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "securityGroup":{ "groupID":"sg-backend" } - } + } ], "ports": [ { @@ -5698,10 +5698,10 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { ], "securityGroups": [ { - "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" + "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" }, "sg-backend" - ], + ], "scheme": "internal", "type": "network" } @@ -5719,7 +5719,7 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "unhealthyThresholdCount": 3, "protocol": "TCP", "port": "traffic-port", - "timeoutSeconds": 10, + "timeoutSeconds": 10, "intervalSeconds": 10 }, "targetGroupAttributes": [ @@ -5875,7 +5875,7 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "port": "traffic-port", "protocol": "TCP", "intervalSeconds": 10, - "timeoutSeconds": 10, + "timeoutSeconds": 10, "healthyThresholdCount": 3, "unhealthyThresholdCount": 3 }, @@ -6060,7 +6060,7 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "port": "traffic-port", "protocol": "TCP", "intervalSeconds": 10, - "timeoutSeconds": 10, + "timeoutSeconds": 10, "healthyThresholdCount": 3, "unhealthyThresholdCount": 3 }, @@ -6226,7 +6226,7 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { ], "securityGroups": [ { - "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" + "$ref": "#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID" } ] } @@ -6244,7 +6244,7 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "port": "traffic-port", "protocol": "TCP", "intervalSeconds": 10, - "timeoutSeconds": 10, + "timeoutSeconds": 10, "healthyThresholdCount": 3, "unhealthyThresholdCount": 3 }, @@ -6440,7 +6440,7 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "port": "traffic-port", "protocol": "TCP", "intervalSeconds": 10, - "timeoutSeconds": 10, + "timeoutSeconds": 10, "healthyThresholdCount": 3, "unhealthyThresholdCount": 3 }, @@ -6581,7 +6581,7 @@ func Test_defaultModelBuilderTask_Build(t *testing.T) { "port": "traffic-port", "protocol": "TCP", "intervalSeconds": 10, - "timeoutSeconds": 10, + "timeoutSeconds": 10, "healthyThresholdCount": 3, "unhealthyThresholdCount": 3 }, From 64864d4b6b7c228165000116ae7f1dcd7b176292 Mon Sep 17 00:00:00 2001 From: Kevin Lyda Date: Thu, 5 Dec 2024 14:40:47 +0000 Subject: [PATCH 5/7] Don't silently ignore errors --- config/webhook/manifests.yaml | 2 +- pkg/service/model_build_listener.go | 34 ++++++++++++++--------------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml index 9d1898df1f..e589c77fbe 100644 --- a/config/webhook/manifests.yaml +++ b/config/webhook/manifests.yaml @@ -2,7 +2,7 @@ apiVersion: admissionregistration.k8s.io/v1 kind: MutatingWebhookConfiguration metadata: - name: mutating-webhook-configuration + name: webhook webhooks: - admissionReviewVersions: - v1beta1 diff --git a/pkg/service/model_build_listener.go b/pkg/service/model_build_listener.go index 68c6efa658..1229c46f6f 100644 --- a/pkg/service/model_build_listener.go +++ b/pkg/service/model_build_listener.go @@ -3,6 +3,7 @@ package service import ( "context" "fmt" + "slices" "strconv" "github.com/aws/aws-sdk-go-v2/aws" @@ -35,7 +36,10 @@ func (t *defaultModelBuildTask) buildListeners(ctx context.Context, scheme elbv2 key := port.Port if vals, exists := portMap[key]; exists { if len(vals) > 1 { - port = mergeServicePortsForListener(vals) + port, err = mergeServicePortsForListener(vals) + if err != nil { + return err + } } else { port = vals[0] } @@ -257,23 +261,19 @@ func (t *defaultModelBuildTask) buildListenerAttributes(ctx context.Context, svc return attributes, nil } -func mergeServicePortsForListener(ports []corev1.ServicePort) corev1.ServicePort { - port0 := ports[0] - mergeableProtocols := map[corev1.Protocol]bool{ - corev1.ProtocolTCP: true, - corev1.ProtocolUDP: true, +func mergeServicePortsForListener(ports []corev1.ServicePort) (corev1.ServicePort, error) { + if len(ports) != 2 { + return corev1.ServicePort{}, fmt.Errorf("Can only merge two ports, not %d (%+v)", len(ports), ports) } - if _, ok := mergeableProtocols[port0.Protocol]; !ok { - return port0 - } - for _, port := range ports[1:] { - if _, ok := mergeableProtocols[port.Protocol]; !ok { - continue - } - if port.NodePort == port0.NodePort && port.Protocol != port0.Protocol { - port0.Protocol = corev1.Protocol("TCP_UDP") - break + for _, port := range ports { + if !slices.Contains([]string{"TCP", "UDP"}, string(port.Protocol)) { + return corev1.ServicePort{}, fmt.Errorf("Unsupported protocol for merging: %s", port.Protocol) } } - return port0 + if ports[0].Protocol == ports[1].Protocol { + return corev1.ServicePort{}, fmt.Errorf("Protocols can't match for merging: %s", ports[0].Protocol) + } + port := ports[0] + port.Protocol = corev1.Protocol("TCP_UDP") + return port, nil } From 01bbd24fcf7d81b024bbf51413ea39f00a691854 Mon Sep 17 00:00:00 2001 From: Kevin Lyda Date: Wed, 8 Jan 2025 16:51:22 +0000 Subject: [PATCH 6/7] Clean up tests The original PR had a test for mergeServicePortsForListener tacked on. That test has been moved into the test for mergeServicePortsForListener. This also fixes the test to deal with the error checking that was added to mergeServicePortsForListener. --- pkg/service/model_build_listener_test.go | 72 +++++++++++++++--------- 1 file changed, 45 insertions(+), 27 deletions(-) diff --git a/pkg/service/model_build_listener_test.go b/pkg/service/model_build_listener_test.go index cbf3686dbc..b72093a9d3 100644 --- a/pkg/service/model_build_listener_test.go +++ b/pkg/service/model_build_listener_test.go @@ -304,9 +304,10 @@ func Test_defaultModelBuilderTask_buildListenerAttributes(t *testing.T) { func Test_mergeServicePortsForListener(t *testing.T) { tests := []struct { - name string - ports []corev1.ServicePort - want corev1.ServicePort + name string + ports []corev1.ServicePort + want corev1.ServicePort + success bool }{ { name: "one port", @@ -326,6 +327,7 @@ func Test_mergeServicePortsForListener(t *testing.T) { Protocol: corev1.ProtocolTCP, NodePort: 31223, }, + success: true, }, { name: "two tcp ports, different target and node ports", @@ -352,6 +354,7 @@ func Test_mergeServicePortsForListener(t *testing.T) { Protocol: corev1.ProtocolTCP, NodePort: 31223, }, + success: true, }, { name: "two udp ports, different target and node ports", @@ -378,6 +381,7 @@ func Test_mergeServicePortsForListener(t *testing.T) { Protocol: corev1.ProtocolUDP, NodePort: 31223, }, + success: true, }, { name: "one tcp and one udp, different target and node ports", @@ -404,6 +408,7 @@ func Test_mergeServicePortsForListener(t *testing.T) { Protocol: corev1.ProtocolTCP, NodePort: 31223, }, + success: true, }, { name: "one tcp and one udp, same target and node ports", @@ -430,6 +435,7 @@ func Test_mergeServicePortsForListener(t *testing.T) { Protocol: corev1.Protocol("TCP_UDP"), NodePort: 31223, }, + success: true, }, { name: "one udp and one tcp, same target and node ports", @@ -456,6 +462,7 @@ func Test_mergeServicePortsForListener(t *testing.T) { Protocol: corev1.Protocol("TCP_UDP"), NodePort: 31223, }, + success: true, }, { name: "one tcp and one udp, same node port, different target port", @@ -470,7 +477,7 @@ func Test_mergeServicePortsForListener(t *testing.T) { { Name: "p2", Port: 80, - TargetPort: intstr.FromInt(8888), + TargetPort: intstr.FromInt(80), Protocol: corev1.ProtocolUDP, NodePort: 31223, }, @@ -482,12 +489,45 @@ func Test_mergeServicePortsForListener(t *testing.T) { Protocol: corev1.Protocol("TCP_UDP"), NodePort: 31223, }, + success: true, + }, + { + name: "one tcp and one udp, same node port, different target port", + ports: []corev1.ServicePort{ + { + Name: "p1", + Port: 80, + TargetPort: intstr.FromInt(80), + Protocol: corev1.ProtocolTCP, + NodePort: 31223, + }, + { + Name: "p2", + Port: 80, + TargetPort: intstr.FromInt(80), + Protocol: corev1.ProtocolUDP, + NodePort: 31223, + }, + { + Name: "p2", + Port: 80, + TargetPort: intstr.FromInt(80), + Protocol: corev1.ProtocolSCTP, + NodePort: 31223, + }, + }, + want: corev1.ServicePort{}, + success: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - port := mergeServicePortsForListener(tt.ports) + port, err := mergeServicePortsForListener(tt.ports) + if !tt.success { + assert.NotNil(t, err) + return + } assert.Equal(t, port.Name, tt.want.Name) assert.Equal(t, port.Port, tt.want.Port) assert.Equal(t, port.TargetPort.IntVal, tt.want.TargetPort.IntVal) @@ -495,26 +535,4 @@ func Test_mergeServicePortsForListener(t *testing.T) { assert.Equal(t, port.NodePort, tt.want.NodePort) }) } - - // test that function returns new ServicePort instance - p1 := corev1.ServicePort{ - Name: "p1", - Port: 80, - TargetPort: intstr.FromInt(80), - Protocol: corev1.ProtocolTCP, - NodePort: 31223, - } - p2 := corev1.ServicePort{ - Name: "p2", - Port: 80, - TargetPort: intstr.FromInt(80), - Protocol: corev1.ProtocolUDP, - NodePort: 31223, - } - ports := []corev1.ServicePort{p1, p2} - mergedPort := mergeServicePortsForListener(ports) - - assert.Equal(t, corev1.ProtocolTCP, p1.Protocol) - assert.Equal(t, corev1.Protocol("TCP_UDP"), mergedPort.Protocol) - assert.NotEqual(t, &p1, &mergedPort) } From b60ef73d5668c13405ee209557a6edfd29239517 Mon Sep 17 00:00:00 2001 From: Kevin Lyda Date: Wed, 8 Jan 2025 17:20:35 +0000 Subject: [PATCH 7/7] Add kubebulder setting for TCP_UDP --- apis/elbv2/v1beta1/targetgroupbinding_types.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apis/elbv2/v1beta1/targetgroupbinding_types.go b/apis/elbv2/v1beta1/targetgroupbinding_types.go index 08345a99a4..f6453bbbc7 100644 --- a/apis/elbv2/v1beta1/targetgroupbinding_types.go +++ b/apis/elbv2/v1beta1/targetgroupbinding_types.go @@ -77,7 +77,7 @@ type NetworkingPeer struct { SecurityGroup *SecurityGroup `json:"securityGroup,omitempty"` } -// +kubebuilder:validation:Enum=TCP;UDP +// +kubebuilder:validation:Enum=TCP;UDP;TCP_UDP // NetworkingProtocol defines the protocol for networking rules. type NetworkingProtocol string