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.

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

kuberesource: some improvements around the port-forwarder resource #1192

Merged
merged 4 commits into from
Jan 31, 2025

Conversation

burgerdev
Copy link
Contributor

  • Remove the unused port-forwarder for a single port.
  • Patch out the namespace if empty. This is mostly to align with the other namespaced wrappers.
  • Restrict to TCP ports when forwarding k8s services. The current socat script can't deal with UDP.
  • Handle SIGTERM by forwarding to the background children. This speeds up pod termination because we don't need to wait for the SIGKILL anymore.

@burgerdev burgerdev added the no changelog PRs not listed in the release notes label Jan 29, 2025
@burgerdev burgerdev marked this pull request as ready for review January 29, 2025 11:11
@burgerdev burgerdev requested a review from 3u13r January 29, 2025 11:11
Copy link
Member

@3u13r 3u13r left a comment

Choose a reason for hiding this comment

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

LGTM

internal/kuberesource/parts.go Outdated Show resolved Hide resolved
@@ -449,7 +449,7 @@ func ServiceForStatefulSet(s *applyappsv1.StatefulSetApplyConfiguration) *applyc

// PortForwarderForService creates a Pod that forwards network traffic to the given service.
//
// Port forwarders are named "port-forwarder-SVCNAME" and forward all ports in the ServiceSpec.
// Port forwarders are named "port-forwarder-SVCNAME" and forward all TCP ports in the ServiceSpec.
Copy link
Member

Choose a reason for hiding this comment

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

Is it less error prone if we error when we encounter UDP ports? On the other hand how do we want to forward the TCP part of something that has TCP and UDP ports. Just thinking out loud, no need to change something here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

UDP port forwarding is a bit iffy, and supporting TCP is enough for our use case. However, the implementation as is is just wrong if there is a UDP port, which is why I'm introducing the check.

On the other hand how do we want to forward the TCP part of something that has TCP and UDP ports.

You mean situations where both UDP and TCP need to be reachable for full functionality (like, a DNS server)? Fortunately, we don't need that.

Copy link
Member

Choose a reason for hiding this comment

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

I thought about that when using the function one might not read the documentation or just doesn't know if the port of a service is UDP or TCP and expecting a port forwarder. to put it another way: I thought about enforcing the comment in code by returning an error when we encounter non-TCP ports.

You mean situations where both UDP and TCP need to be reachable for full functionality (like, a DNS server)? Fortunately, we don't need that.

This was thinking about the consequences of return an error when we encounter non TCP ports. Then one would need to split every service that both has UDP and TCP ports into two in order to export the TCP ports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean, it's not like you can kubectl port-forward UDP, and that is the only reason we have this pod. If this were a public library function I'd tend to agree, but I don't think we need to be this careful for an e2e test utility.

That being said, there's a bug in the current implementation which I only discovered because of this thread: if there are only non-TCP ports, the list is empty and the port-forwarder likely crashloop. In that case, returning an error is probably justified.

@burgerdev burgerdev force-pushed the burgerdev/portforwarder branch 2 times, most recently from 7c90ea1 to 7d52a94 Compare January 30, 2025 14:29
@burgerdev burgerdev force-pushed the burgerdev/portforwarder branch from 7d52a94 to ab55d7f Compare January 30, 2025 16:51
@burgerdev burgerdev merged commit 96590d2 into main Jan 31, 2025
10 checks passed
@burgerdev burgerdev deleted the burgerdev/portforwarder branch January 31, 2025 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog PRs not listed in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants