Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
kube-proxy consider endpoint readiness to delete UDP stale conntrack entries #106163
kube-proxy consider endpoint readiness to delete UDP stale conntrack entries #106163
Changes from all commits
909925b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So... this doesn't take
ProxyTerminatingEndpoints
into account...Consider the case where an
externalTrafficPolicy: Local
service has a single Serving-Terminating endpoint. Connections come in to that endpoint's node and are accepted and processed by the terminating pod. Then a new endpoint starts up and becomes Ready. Given the code here, that would be interpreted as "the service went from 0 endpoints to non-0 endpoints", and so the node with the Serving-Terminating endpoint would flush all conntrack entries for the service, breaking the existing connections to the Serving-Terminating pod.(Also, this patch changes the rules for
staleServices
, but there are terminating endpoints problems withstaleEndpoints
too; we used to delete conntrack entries to endpoints as soon as the endpoint become non-ready, but now we don't delete them until the pod is fully deleted...)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a regression that needs to be backported and ProxyTerminatingEndpoints is an alpha feature (no backports for alpha are allowed). Also, after the analysis you did in your related PR, I don't think that is easy to solve both problems at the same time 😅
it is UDP, in the sense that is not breaking the connection per se, since it is connectionless, the new packet will create a new entry looking at the iptables rules, that should still exist ... is less performant because you process the packet through the iptables list again but not a big deal (at least I can't see how this can break something, UDP is unreliable)
that is fixed by the
Equal
change to take into consideration Ready https://github.com/kubernetes/kubernetes/pull/106163/files#r743336980There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UDP is connectionless at L4, but not necessarily at L7. That's the main reason UDP conntrack records exist. Eg, anything using Datagram TLS (like QUIC / HTTP/3) won't survive being switched to a different endpoint mid-communication, because the new endpoint won't have the encryption key it needs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀 I can't argue against that, but seems we are going to have some fun soon 😄