Skip to content
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

bpf: nat: use common set of rewrite helpers #27509

Merged

Conversation

julianwiedmann
Copy link
Member

@julianwiedmann julianwiedmann commented Aug 15, 2023

We've grown way too many variants of helpers that rewrite the packet
headers for NAT purposes. They all slightly differ in terms of what
input parameters they expect, and what types of ICMP packets they support.

This PR introduces a generic set of flexible helpers to rule them all, and converts over many of the old paths.

There's a few more places to convert, but I believe this is good enough for a first shot. I'll take on the others as a follow-on.

@julianwiedmann julianwiedmann added kind/enhancement This would improve or streamline existing functionality. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/misc This PR makes changes that have no direct user impact. feature/snat Relates to SNAT or Masquerading of traffic labels Aug 15, 2023
@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann
Copy link
Member Author

/ci-verifier

@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann
Copy link
Member Author

/test

We've grown way too many variants of helpers that rewrite the packet
headers for NAT purposes. They all slightly differ in terms of what
input parameters they expect, and what types of ICMP packets they support.

Introduce a generic set of flexible helpers to rule them all. Start by
converting the DSR RevDNAT path to these new helpers.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
We've previously removed the IPv6 variant of this code in ae197e6,
as `icmp_echoreply` would always cause us to skip SNAT for such packets.

The IPv4 path is slightly different: here we need to consider that
the EgressGW section could force us to SNAT the packet. But in practice
that's not a concern - the packet is a reply for an inbound ECHO, and
EgressGW policy only applies to outbound connections. Thus we can remove
this code.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
This starts exercising the ICMP support in the generic helpers. Thus we
need to pass down a variable `port_off` parameter from snat_v*_nat().

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
…pers

These ICMP types embed the original packet that exceeded the path MTU.
If this packet was SNATed on egress, we need to rewrite the inner headers
before passing the ICMP packet up. Otherwise the originating endpoint will
not be able to match it against the offending connection.

Instead of maintaining custom helpers for this rewrite, switch the handlers
over to the generic helpers. While at it also pass a `inner_l3_off`
parameter to make them a bit more agnostic of the outer ICMP packet type.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
…pers

We currently call csum_l4_replace() to apply the L3 NAT diff into the
L4 csum's pseudo-hdr component. But l4_modify_port() internally also
updates the L4 csum when rewriting the port.

Combine these two calls into one.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann julianwiedmann changed the title 1.15 bpf nat rewrite helpers bpf: nat: use common set of rewrite helpers Aug 16, 2023
@julianwiedmann julianwiedmann marked this pull request as ready for review August 16, 2023 09:32
@julianwiedmann julianwiedmann requested a review from a team as a code owner August 16, 2023 09:32
Copy link
Member

@YutaroHayakawa YutaroHayakawa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with nit.

bpf/lib/nat.h Show resolved Hide resolved
@maintainer-s-little-helper maintainer-s-little-helper bot added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Aug 17, 2023
@julianwiedmann julianwiedmann added this pull request to the merge queue Aug 17, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Aug 17, 2023
@julianwiedmann julianwiedmann added this pull request to the merge queue Aug 17, 2023
@aanm aanm removed this pull request from the merge queue due to the queue being cleared Aug 17, 2023
@julianwiedmann julianwiedmann merged commit 906ee17 into cilium:main Aug 17, 2023
59 checks passed
@julianwiedmann julianwiedmann deleted the 1.15-bpf-nat-rewrite-helpers branch August 17, 2023 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/snat Relates to SNAT or Masquerading of traffic kind/enhancement This would improve or streamline existing functionality. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants