Implement BPF-based masquerading for IPv6 (rebased) #23165
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
qmonnet
added
sig/datapath
qmonnet
force-pushed
the
pr/pchaigno/ipv6-bpf-masquerading
branch
from
127279e
to
a03848a
Compare
qmonnet
force-pushed
the
pr/pchaigno/ipv6-bpf-masquerading
branch
2 times, most recently
from
b03bbcd
to
edf6fb3
Compare
qmonnet
force-pushed
the
pr/pchaigno/ipv6-bpf-masquerading
branch
from
edf6fb3
to
4059d22
Compare
qmonnet
force-pushed
the
pr/pchaigno/ipv6-bpf-masquerading
branch
from
4059d22
to
415d684
Compare
qmonnet
force-pushed
the
pr/pchaigno/ipv6-bpf-masquerading
branch
6 times, most recently
from
9fee381
to
041e6d7
Compare
qmonnet
force-pushed
the
pr/pchaigno/ipv6-bpf-masquerading
branch
4 times, most recently
from
9018253
to
1d038fc
Compare
qmonnet
force-pushed
the
pr/pchaigno/ipv6-bpf-masquerading
branch
2 times, most recently
from
2772d17
to
b8a7a6c
Compare
|
/test |
|
/test-1.26-net-next |
qmonnet
force-pushed
the
pr/pchaigno/ipv6-bpf-masquerading
branch
2 times, most recently
from
c218e98
to
f759fcd
Compare
qmonnet
force-pushed
the
pr/pchaigno/ipv6-bpf-masquerading
branch
3 times, most recently
from
74d6a62
to
ad25279
Compare
|
/test-1.26-net-next |
|
Conformance E2E with changes from this PR to be checked at #26046 - Currently red because the connectivity tests involving 1.1.1.1 are failing (likely for an external reason, as I've seen them fail on multiple PRs/branches) |
This will allow us to easily define the IPv6 equivalent in a subsequent commit. This commit should include no functional changes. [ Quentin: Addressed minor conflict with commit 5ba20bd ("bpf: Use ct_extract_ports4 for ct_lookup4"). ] Signed-off-by: Paul Chaignon <paul@cilium.io> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
Rename and change function ipv6_addrcmp() into ipv6_addr_equals(). We're not using the comparison result anywhere other than for checking whether addresses are equal or not (testing whether one is "greater" than the other does not make much sense for IP addresses, plus the possible underflow from subtractions in the current implementation would make it tricky anyway). Beside improving readability, the motivation for this change is to address some cases where the verifier rejects programs doing these subtractions because "math between map_value pointer and register with unbounded min value is not allowed". Signed-off-by: Quentin Monnet <quentin@isovalent.com>
In the bpf package, where we process object files to load programs, we skip rewriting .bss references for object files without a .data section. Working on a new feature, this caused some older kernels to complain with "unrecognized bpf_ld_imm64 insn" (since they don't support pseudo-register loads), some newer kernels with other errors apparently related to the same missing references. A first attempt at fixing the issue consisted in delaying the check for the presence of the .data section, so that .bss references would be inlined [0]. This would indeed fix the errors observed in the verifier tests, but would instead break our BPF unit tests, where .bss is used to store mock-up value that we want to handle in a specific way. We need a long-term solution for dealing with the different sections in a way that works for all set-ups; ideally, we want to be able to use eBPF global variables everywhere we would need them, but this can't happen before we drop support for older kernels. In the meantime, let's adjust macros that can cause declarations to add variables to the .bss section, in prevision for the next commit. We explicitly store these variables into .data instead. [0] 50f8828 Suggested-by: Timo Beckers <timo@isovalent.com> Suggested-by: Dylan Reimerink <dylan.reimerink@isovalent.com> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
maintainer-s-little-helper
bot
added
the
ready-to-merge
In preparation for BPF IPv6 masquerading, rename ENABLE_MASQUERADE to make it IPv4-related. This is because we may enable masquerading for IPv4 only or IPv6 only in the agent, and in this case, we need to make a distinction between the two IP versions in the datapath. All occurrences of the variable are on IPv4-only paths. Signed-off-by: Quentin Monnet <quentin@isovalent.com>
This commit implements BPF masquerading for IPv6 by following the implementation for IPv4. The main function changed here is snat_v6_needed with almost the exact same steps as IPv4. Support for the ipmasq agent in IPv6 is not implemented here. [ Quentin: Addressed feedback from latest reviews, rebased on main branch. Used ENABLE_MASQUERADE_IPV6 instead of sharing ENABLE_MASQUERADE with IPv4. Addressed conflicts in nodeport.h and nat.h due to commits: - 0ea4341 ("bpf: Make union v6addr const where possible") - c62b209 ("bpf: nat: move snat_v{4,6}_needed into lib/nat.h") - b3747c8 ("bpf: Change ipcache lookup functions to take ClusterID") - 9f6fc7e ("bpf: nodeport: fix tracing for handle_nat_fwd()") - 54a8631 ("bpf: nodeport: handle revDNAT for local backends at to-netdev/to-overlay") - eee902f ("bpf: conntrack: introduce an optimized CT lookup for LB") - 05d084b ("bpf: nodeport: move DSR handling from bpf_lxc into from/to-netdev") - 306c2b4 ("bpf: nodeport: Extract DSR specific option from the Geneve header") - d749a59 ("bpf: Pass ext_err to ct_create4 and ct_create6") - 8b6aa6e ("bpf: nodeport: fix up trace point in to-overlay NAT paths") - 2ec713e ("bpf: rename remote_endpoint_info.sec_label to sec_identity") - 5ba20bd ("bpf: Use ct_extract_ports4 for ct_lookup4") - "bpf: Change ipv6_addrcmp() into ipv6_addr_equals()" from same PR ] Signed-off-by: Paul Chaignon <paul@cilium.io> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
This is the IPv6 port of commit 5ba20bd ("bpf: Use ct_extract_ports4 for ct_lookup4"). We wouldn't process ICMP in ct_extract_ports6() so far; this commit moves the more complete switch/case block from ct_lookup6() to ct_extract_ports6() instead, and makes the former function call the latter. This way, we get less duplicated code and ICMP support in ct_extract_ports6(). Signed-off-by: Quentin Monnet <quentin@isovalent.com>
Add ENABLE_MASQUERADE_IPV6 everywhere we already have ENABLE_MASQUERADE_IPV4, and ENABLE_IPV6 is set. Signed-off-by: Quentin Monnet <quentin@isovalent.com>
This commit refactors InitBPFMasqueradeAddrs to easily extend it for
IPv6 in a subsequent commit. This commit shouldn't have any functional
changes.
[ Quentin: Rebased on main branch, addressed minor conflicts. Also:
- Added firstGlobalAddr() stub to address_other.go, given that it is
now necessary to build integration tests for Darwin.
- Moved initMasqueradeAddrs (and V4 version) to address_linux.go
instead of assigning it in a variable in InitBPFMasqueradeAddrs,
because of the use of netlink.FAMILY_V4 which is not defined for
Darwin. ]
Signed-off-by: Paul Chaignon <paul@cilium.io>
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
This commit allows enabling BPF masquerading for IPv6 and defines all the proper macros for the datapath implementation. [ Quentin: Rebased on main branch, addressed minor conflicts on daemon.go, daemon_main.go, loader.go, and address.go (including from commit d10e8eb ("bpf: dynamic-width static data inlining")), adjusted to the changes introduced in previous commits. ] Signed-off-by: Paul Chaignon <paul@cilium.io> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
The host firewall had some code paths related to BPF masquerading for IPv4; now that we added IPv6 support for BPF masquerading, we need to take a deep look at the implications for this part of the datapath. Until we have done so, reject IPv6 BPF masquerading in the Agent. Also disable IPv6 BPF masquerading in some tests involving on the host firewall. Suggested-by: Julian Wiedmann <jwi@isovalent.com> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
BPF-based IPv6 masquerading is now supported, except for the ip-masq agent. This commit therefore updates the documentation accordingly. [ Quentin: Add the additional limitation on IPv6 BPF masquerading with host firewall, see previous commit for details. ] Signed-off-by: Paul Chaignon <paul@cilium.io> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
Since Cilium supports BPF masquerading for IPv6, we can now test it.
The upgrade/downgrade test needs a small exception because IPv6 BPF
masquerading is still not supported on v1.13, from which we upgrade.
The exception logic comes with an assertion, to ensure we don't forget
to remove the exception once the upgrade is performed from v1.14.
[ Quentin:
- Bump version number requirement in assert from 1.12 to 1.14,
switch fixed assert to versioncheck.
- Drop test updates for test/k8s/datapath_configuration.go, no
longer relevant after commits b5e0e12 ("test: Remove
K8sDatapathConfig some direct routing tests") and 8411627
("test: Remove K8sDatapathConfig some tunneling tests").
We don't even have to enable IPv6 masquerading in the datapath
conformance tests, because of commit 79aa8b1 ("gh/workflows:
re-enable masqueranding for EGW"). ]
Signed-off-by: Paul Chaignon <paul@cilium.io>
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
Now that BPF IPv6 masquerading is supported, align the configuration on IPv4's for net_policies test, because (as per the comment) "the request will fail if the reply packet's source address is rewritten when sending a request directly to the Pod from outside the cluster". Signed-off-by: Quentin Monnet <quentin@isovalent.com>
We know test masquerading in the conformance end-to-end workflow as well, which should eventually replace Jenkins. Let's update the workflow's description to use IPv6 in there, as well. Signed-off-by: Quentin Monnet <quentin@isovalent.com>
qmonnet
force-pushed
the
pr/pchaigno/ipv6-bpf-masquerading
branch
from
b4ae634
to
8c7dcfb
Compare
|
CI was all green at last, but e4d277c created a conflict on |
|
/test |
qmonnet
removed
the
ready-to-merge
maintainer-s-little-helper
bot
added
the
ready-to-merge
|
CI's all green, apart from checkpatch's false positives (I checked them). Updated Conformance E2E on the test PR is all green, too. All reviews are in. We're good to go! 🎉 |
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.
Summary of commits:
ct_is_reply4as a macro to facilitate implem. of IPv6 counterpart.ipv6_addrcmp()intoipv6_addr_equals().DEFINE_U*()macros use.datasection for declarations.ENABLE_MASQUERADEasENABLE_MASQUERADE_IPV4.ct_extract_ports6(), refactor to get closer to IPv4's version.ENABLE_MASQUERADE_IPV6to options lists for complexity tests.InitBPFMasqueradeAddrsto facilitate implem. of IPv6 counterpart.See commits for details.
Fixes: #14350.
This is a rebase of #18694 from Paul.