Skip to content

Add support for TCP_UDP to NLB TargetGroups and Listeners #2275

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Add support for TCP_UDP to NLB TargetGroups and Listeners #2275

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f78cf1e
fix the regression of IP mode support for fargate pods (#2158)
M00nF1sh Aug 4, 2021
f1ad76a
Add support for TCP_UDP to NLB TargetGroups and Listeners
amorey Sep 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions apis/elbv2/v1beta1/targetgroupbinding_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
50 changes: 47 additions & 3 deletions pkg/service/model_build_listener.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,35 @@ import (

func (t *defaultModelBuildTask) buildListeners(ctx context.Context, scheme elbv2model.LoadBalancerScheme) error {
cfg := t.buildListenerConfig(ctx)

// 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
}

Expand Down Expand Up @@ -171,3 +194,24 @@ func (t *defaultModelBuildTask) buildListenerConfig(ctx context.Context) listene
func (t *defaultModelBuildTask) buildListenerTags(ctx context.Context) (map[string]string, error) {
return t.buildAdditionalResourceTags(ctx)
}

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that if this is an nlb-ip LB, NodePort is the wrong test, as the code in model_build_target_group.go will be using the TargetPort as its destination, e.g. https://github.com/kubernetes-sigs/aws-load-balancer-controller/blob/main/pkg/ingress/model_build_target_group.go#L59-L62

The code that processes the annotations and determines the target type is happening much later (the call to buildTargetGroup via buildListener immediately after this method is called, so addressing this might require pulling the port-merging to later, or the annotation-reading earlier.

Copy link
Contributor

@TBBle TBBle Feb 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IP-target mode is going to introduce a difficult edge-case:

  • The target port can be named
  • The port names are unique in the service, i.e. a TCP and UDP port will have different names, even if they end up at the same port number on the target Pod.
  • Different pods can have different numeric values for the same target port

hence, it's possible to produce the situation that some of the pods have their TCP and UDP listeners on the same port, and some of the pods have their TCP and UDP listeners on different ports.

At this level, a target port equality test would never pass (Edit: I had the logic backwards earlier), as it'd be seeing the names, numeric lookup for named ports is done later ((m *defaultNetworkingManager) computeNumericalPorts for bindings, and implicitly by the Endpoints and consumed by (m *defaultResourceManager) registerPodEndpoints I think).

So I don't see a way to identify that any pair of named target ports in ip-target mode can be merged without some further hint from the user that, e.g. dns-udp and dns-tcp targetPorts will be the same port when resolved on all Pods in the Service; the same service port might be a good hint, but maybe not reliable?

Copy link
Contributor

@TBBle TBBle Feb 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be possible for the logic in (m *defaultResourceManager) registerPodEndpoints and the defaultEndpointResolver APIs it calls, to validate that when given a numeric port, that for TCP_UDP two ports exist on the service, and that for non-TCP_UDP, the right one is seen (which means adding the protocol to the ServiceRef in TargetGroupBinding as right now it's not specified if the numeric port is TCP or UDP).

When given a named port for TCP_UDP (if a way can be found to reliably match/merge them), it really should be a pair of names (as the TCP and UDP ports will have different names) and validate that both exist on the service.

That suggests that ServiceRef in TargetGroupBinding would need to be able to list multiple ports.

And then for TCP_UDP, exclude any resulting endpoints where the resulting port for TCP and UDP are different. Right now that won't be checked as for a numeric port, it'll take the first port name that has the right port number, irrespective of TCP or UDP) and for a named port, only one name is currently preserved through the pipeline.

Copy link
Contributor

@TBBle TBBle Feb 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The simplest way forward of-course is to initially limit TCP_UDP support to instance-target NLBs pending further design work. That still suffers from my first comment here, that we don't know at this point that it's an instance-target NLB.

Copy link
Contributor

@TBBle TBBle Feb 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to throw another small spanner in the works, spec.allocateLoadBalancerNodePorts may allow the user to make this value (port.NodePort and port0.NodePort) be 0.

That only makes sense for ip-target NLBs though, as instance-target NLBs rely on a NodePort. I'm not sure if the AWS Load Balancer Controller is going to be able to override that setting for instance-target NLBs, but a NodePort equality test should probably never consider two NodePort==0 ports as equal, to avoid making an invalid situation worse.

port0.Protocol = corev1.Protocol("TCP_UDP")
Copy link
Contributor

@TBBle TBBle Feb 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't add a comment there, but will this require flipping the test tgProtocol != elbv2model.ProtocolUDP in buildListenerSpec for TLS support to be tgProtocol == elbv2model.ProtocolTCP (not sure why the existing test would allow SCTP to be turned into TLS, but but perhaps !=UDP && != TCP_UDP is more consistent with current behaviour). Otherwise a merged TCP_UDP service would be transformed destructively (silently losing the UDP port) into a elbv2model.ProtocolTLS if provided certificate ARNS and it matched the SSL ports.

In fact, it might be better to throw an error if that case happens, as the merge is between a TCP service that should be converted to a TLS service, and a UDP service that cannot be so-converted, and I don't see anything in the docs that suggests a TLS_UDP protocol option, or a TLS_DTLS protocol option which would be even nicer.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TLS and UDP are on different levels. So that seems like a non-problem.

break
}
}
return port0
}
218 changes: 218 additions & 0 deletions pkg/service/model_build_listener_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"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"
"sigs.k8s.io/aws-load-balancer-controller/pkg/annotations"
elbv2model "sigs.k8s.io/aws-load-balancer-controller/pkg/model/elbv2"
)
Expand Down Expand Up @@ -88,3 +89,220 @@ func Test_defaultModelBuilderTask_buildListenerALPNPolicy(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)
}
45 changes: 34 additions & 11 deletions pkg/service/model_build_target_group.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,11 +431,39 @@ func (t *defaultModelBuildTask) buildPeersFromSourceRangesConfiguration(_ contex
func (t *defaultModelBuildTask) buildTargetGroupBindingNetworking(ctx context.Context, tgPort intstr.IntOrString, preserveClientIP bool,
hcPort intstr.IntOrString, port corev1.ServicePort, defaultSourceRanges []string, targetGroupIPAddressType elbv2model.TargetGroupIPAddressType) *elbv2model.TargetGroupBindingNetworking {
tgProtocol := port.Protocol
loadBalancerSubnetsSourceRanges := t.getLoadBalancerSubnetsSourceRanges(targetGroupIPAddressType)
networkingProtocol := 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
}
var trafficPorts []elbv2api.NetworkingPort
switch networkingProtocol {
case elbv2api.NetworkingProtocolTCP_UDP:
tcpProtocol := elbv2api.NetworkingProtocolTCP
udpProtocol := elbv2api.NetworkingProtocolUDP
trafficPorts = []elbv2api.NetworkingPort{
{
Port: &tgPort,
Protocol: &tcpProtocol,
},
{
Port: &tgPort,
Protocol: &udpProtocol,
},
}
default:
trafficPorts = []elbv2api.NetworkingPort{
{
Port: &tgPort,
Protocol: &networkingProtocol,
},
}
}
loadBalancerSubnetsSourceRanges := t.getLoadBalancerSubnetsSourceRanges(targetGroupIPAddressType)
trafficSource := loadBalancerSubnetsSourceRanges
customSourceRangesConfigured := false
if networkingProtocol == elbv2api.NetworkingProtocolUDP || preserveClientIP {
Expand All @@ -444,13 +472,8 @@ func (t *defaultModelBuildTask) buildTargetGroupBindingNetworking(ctx context.Co
tgbNetworking := &elbv2model.TargetGroupBindingNetworking{
Ingress: []elbv2model.NetworkingIngressRule{
{
From: trafficSource,
Ports: []elbv2api.NetworkingPort{
{
Port: &tgPort,
Protocol: &networkingProtocol,
},
},
From: trafficSource,
Ports: trafficPorts,
},
},
}
Expand Down
Loading