Skip to content

Conversation

qmonnet
Copy link
Member

@qmonnet qmonnet commented Jun 8, 2023

For testing only.

Run modified workflow for conformance-e2e on top of #23165.

@qmonnet qmonnet added dont-merge/preview-only Only for preview or testing, don't merge it. release-note/misc This PR makes changes that have no direct user impact. labels Jun 8, 2023
@qmonnet qmonnet force-pushed the pr/pchaigno/ipv6-bpf-masquerading_test-conform-e2e branch 6 times, most recently from 8a07e1d to dbb9bf9 Compare June 13, 2023 10:34
pchaigno and others added 15 commits June 14, 2023 00:13
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>
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>
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
@qmonnet qmonnet force-pushed the pr/pchaigno/ipv6-bpf-masquerading_test-conform-e2e branch from dbb9bf9 to 1521dff Compare June 13, 2023 23:18
@qmonnet qmonnet closed this Jun 14, 2023
@qmonnet qmonnet deleted the pr/pchaigno/ipv6-bpf-masquerading_test-conform-e2e branch June 14, 2023 01:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dont-merge/preview-only Only for preview or testing, don't merge it. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants