datapath: support Hybrid-DSR mode for Geneve dispatch #25553
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
julianwiedmann
added
sig/datapath
julianwiedmann
force-pushed
the
1.14-nodeport-geneve-hybrid
branch
from
b270d51
to
626ce31
Compare
|
/test |
The DSR validation in initKubeProxyReplacementOptions() only allowed Hybrid when used with IP-Option dispatch. But we can also support Hybrid mode with Geneve dispatch, so that UDP traffic goes down the NAT path. It then either gets get encapsulated without DSR info (tunnel routing), or leaves the node without encapsulation (native routing). This just requires a few small changes to the configuration of the ad-hoc tunnel. Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
Have all the nodeport modes grouped together, so it's easier to spot that we have more than just SNAT and full-DSR. Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
julianwiedmann
force-pushed
the
1.14-nodeport-geneve-hybrid
branch
from
626ce31
to
5bd8526
Compare
|
/test |
julianwiedmann
changed the title
julianwiedmann
added
kind/feature
julianwiedmann
requested review from
jibi,
viktor-kurchenko,
squeed,
lmb and
ysksuzuki
|
@ysksuzuki I don't think we discussed hybrid mode at all during #23890. Was there a reason we didn't enable it right away? 🤔 |
I think it was just an oversight🙈 |
lmb
reviewed
Jun 1, 2023
| @@ -143,7 +143,7 @@ func (h *HeaderfileWriter) WriteNodeConfig(w io.Writer, cfg *datapath.LocalNodeC | |||
| encapProto := option.Config.TunnelProtocol | |||
| if !option.Config.TunnelingEnabled() && | |||
| option.Config.EnableNodePort && | |||
| option.Config.NodePortMode == option.NodePortModeDSR && | |||
| option.Config.NodePortMode != option.NodePortModeSNAT && | |||
There was a problem hiding this comment.
This is more concise than == DSR || == Hybrid but maybe less future proof?
There was a problem hiding this comment.
Ack, not a big fan of it either - for now, I believe this is in sync with how the remaining code does it. But I can follow up with some NodePortUsesDSR() helper.
There was a problem hiding this comment.
+1. If we are going to make setting more logical, I also vote for not using the name NodePort to refer to everything related to forwarding traffic
squeed
approved these changes
Jun 1, 2023
julianwiedmann
added
the
ready-to-merge
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
We've initially had DSR with info dispatch via IP option/extension. Here we support "full" DSR, and also a hybrid mode where TCP connections use DSR, but UDP traffic continues to use NAT and its replies pass back through the LB node.
With #23890 we've added info dispatch via Geneve Options. Here DSR traffic gets Geneve-encapsulated and has DSR info added as needed. But this currently only supports "full" DSR.
Adding support for hybrid mode is easy enough - it's just a few changes in the agent's option processing, we don't even need to touch the BPF datapath.