Skip to content
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

Add support for TCP_UDP to NLB TargetGroups and Listeners (rebase) #3807

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+916 −261
Open

Add support for TCP_UDP to NLB TargetGroups and Listeners (rebase) #3807

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bccf5fb
fix the regression of IP mode support for fargate pods (#2158)
M00nF1sh Aug 4, 2021
3a0cb2f
Add support for TCP_UDP to NLB TargetGroups and Listeners
amorey Sep 5, 2021
0c80a24
Attempt to address remaining errors
lyda Oct 14, 2024
df59201
Remove spurious tabs
lyda Nov 5, 2024
77afb20
Don't silently ignore errors
lyda Dec 5, 2024
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
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,4 @@ site
*~
*.bak
scripts/aws_sdk_model_override/*
/gomock_reflect*
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"
Copy link
Collaborator

Choose a reason for hiding this comment

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

we don't need this.
the networking rules here is meant for security groups(which don't have a tcp_udp rule protocol)

instead of TCP_UDP, we should just generate two rules in TGB's networking, one for TCP and another one for UDP.

Copy link
Author

Choose a reason for hiding this comment

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

I'm sorry, but we do. This is for AWS load balancer target groups, not security groups, and as much I find it annoying, TCP_UDP is a protocol for AWS load balancers. It's used in the single case where the TCP and UDP port numbers are the same. So, for example, if you're implementing a domain name server and need to listen on port 53 for both TCP and UDP, the load balancer must be configured to have a target group protocol of TCP_UDP. I have no idea why they did this, but they did.

)

// NetworkingPort defines the port and protocol for networking rules.
Expand Down
240 changes: 120 additions & 120 deletions config/webhook/manifests.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,128 +4,128 @@ kind: MutatingWebhookConfiguration
metadata:
name: webhook
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
5 changes: 3 additions & 2 deletions pkg/ingress/model_build_target_group.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
Expand Down
59 changes: 51 additions & 8 deletions pkg/service/model_build_listener.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package service
import (
"context"
"fmt"
"slices"
"strconv"

"github.com/aws/aws-sdk-go-v2/aws"
Expand All @@ -19,16 +20,41 @@ 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 {
Copy link
Collaborator

@M00nF1sh M00nF1sh Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

why not just loop over portMap instead of t.service.Spec.Ports. (we don't need this portMap[key] check this way)

Copy link
Author

Choose a reason for hiding this comment

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

Because it's trying to find the case of the same port number being listened to on TCP and UDP. portMap is empty and is being used to find duplicate port numbers.

key := port.Port
if vals, exists := portMap[key]; exists {
if len(vals) > 1 {
port, err = mergeServicePortsForListener(vals)
if err != nil {
return err
}
} 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 {
Expand All @@ -39,7 +65,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)
Expand Down Expand Up @@ -149,7 +175,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)
Expand All @@ -160,7 +186,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 {
Expand Down Expand Up @@ -191,7 +217,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
}
Expand Down Expand Up @@ -234,3 +260,20 @@ func (t *defaultModelBuildTask) buildListenerAttributes(ctx context.Context, svc
}
return attributes, nil
}

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)
}
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)
}
}
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
}
Loading