daemon: Do not require native routing CIDR if ipmasq-agent is enabled #27747
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
gandro
added
sig/datapath
|
/test |
christarazi
approved these changes
Aug 28, 2023
aanm
approved these changes
Aug 28, 2023
|
There seems to have been some infra related flakes:
|
pippolo84
approved these changes
Aug 29, 2023
When running with ipmasq-agent-based eBPF masquerading, there should be no need to mandatory require a native routing CIDR, since all CIDRs can be configured via the BPF map instead. This commit therefore does not set the IPV4_SNAT_EXCLUSION_DST_CIDR field if no native routing CIDR is set. Notably, it also does not fall back on the local pod CIDR like we do in regular BPF masquearding, since in many IPAM modes there is no local pod CIDR to begin with. Note that this change is backwards-compatible, because we have a check that ensures that the native routing CIDR is always set if BPF masquerading is enabled. So all existing users of ip-masq-agent already have a native routing CIDR set. The next commit will remove that check and therefore enabling the code path added by this commit. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
Cilium's built-in [ipmasq-agent replacement](https://docs.cilium.io/en/v1.14/network/concepts/masquerading/#ebpf-based) acts as a replacement for the native routing CIDR. Therefore, it does not make sense to require the native routing CIDR if the ipmasq-agent is enabled, since the two flags are basically mutually exclusive. The previous commit already prepared the implementation to not set the IPV4_SNAT_EXCLUSION_DST_CIDR in the datapath if the native routing CIDR is absent. In addition, this commit also slightly restructures and aligns the if condition with the error message, to make it a bit easier to read. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
With the previous commit, we removed the restriction that the user must set a native routing CIDR. This commit adjusts the multi-pool workflow to make use of that change (and incidentally also tests that the previous commit via CI). Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
gandro
force-pushed
the
pr/gandro/ipmasq-agent-does-not-need-native-routing-cidr
branch
from
798bae1
to
a2c3da2
Compare
|
/test |
qmonnet
approved these changes
Aug 29, 2023
ti-mo
approved these changes
Aug 29, 2023
christarazi
deleted the
pr/gandro/ipmasq-agent-does-not-need-native-routing-cidr
branch
maintainer-s-little-helper
bot
added
the
ready-to-merge
Good point. I think there is some experimentation needed first though |
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.
When running with ipmasq-agent-based eBPF masquerading, there should be no need to mandatory require a native routing CIDR, since all CIDRs can be configured via the BPF map instead.
This PR therefore does not set the
IPV4_SNAT_EXCLUSION_DST_CIDRfield if no native routing CIDR is set. Notably, it also does not fall back on the local pod CIDR like we do in regular BPF masquearding, since in many IPAM modes there is no local pod CIDR to begin with.Note that this change is backwards-compatible, because we have a check that ensures that the native routing CIDR is always set if BPF masquerading is enabled. So all existing users of ip-masq-agent already have a native routing CIDR set. This PR also removes that check, thus allowing future uses to not set a native routing CIDR when running with ipmasq-agent based masquerading.