Reduce the amount of repeating code in CT #25356

gentoo-root · 2023-05-10T13:37:25Z

Pure refactoring, no behavioral change.

The switch in ct_lookup4 is almost identical to ct_extract_ports4. Reuse the existing function with a slight modification. Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>

qmonnet

Looks good, thanks!

Nit: In 2nd commit, some comment blocks could be re-wrapped now that they have less left-indent.

ICMP_FRAG_NEEDED handling in snat_v4_rev_nat is pretty self-contained, and three indentation levels can easily be saved by moving this code into a separate function with just slight modifications. Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>

gentoo-root · 2023-05-10T18:51:45Z

Nit: In 2nd commit, some comment blocks could be re-wrapped now that they have less left-indent.

Done.

julianwiedmann

💯 on the ct_extract_ports4() patch!

I'm less convinced that the ICMP_FRAG_NEEDED patch is truly making it easier to understand things, but not feeling strongly about it either. But should we give the ICMPV6_PKT_TOOBIG part the same treatment then?

julianwiedmann · 2023-05-11T09:42:26Z

bpf/lib/nat.h

+	struct ipv4_ct_tuple tuple = {};
+	struct ipv4_nat_entry *state;
+	struct iphdr iphdr;


Just for context - when initially adding this code (a short while back), there was concern whether we should share the CT tuple (for processing the inner & outer packet), or have it cleanly separated. And how it would impact complexity.

Thanks, I wasn't aware of this context. What was the conclusion of that discussion? Shall I pass a pointer to tuple_buf to this function to reuse the one from the caller? Or there wasn't a clear conclusion?

What was the conclusion of that discussion?

We tried to share it as much as possible - but I don't think there was a final experiment on the latest code whether having it fully split up would trigger the verifier. So if we can do a clean split and still pass the verifier, I think that would be preferable.

julianwiedmann · 2023-05-11T09:48:21Z

bpf/lib/nat.h

+	/* Switch back to the outer header. */
+	tuple.nexthdr = IPPROTO_ICMP;
+
+	return snat_v4_rewrite_ingress(ctx, &tuple, state, off);
+}


I'm slightly concerned that this part (how we NAT the outer headers) will over time drift apart from what we do in the "main" path. How bad would it be to only handle the inner SNAT here, and return to the main function for handling the outer SNAT?

You mean just this function call return snat_v4_rewrite_ingress(ctx, &tuple, state, off);? I don't see what can drift here, rewriting the outer headers is already contained in a separate function, so that it can be reused from two different flows.

I can return 0 from here and goto rewrite_ingress in the caller on non-errors, but that's going to be uglier, and I don't see much point. Please tell me what you think.

You mean just this function call return snat_v4_rewrite_ingress(ctx, &tuple, state, off);? I don't see what can drift here, rewriting the outer headers is already contained in a separate function, so that it can be reused from two different flows.

The bad scenario would be that at some point we only modify the main path, without realizing that there's a second user buried somewhere else.

I can return 0 from here and goto rewrite_ingress in the caller on non-errors, but that's going to be uglier, and I don't see much point. Please tell me what you think.

Then let's not change it and keep your current approach :)

gentoo-root

should we give the ICMPV6_PKT_TOOBIG part the same treatment then?

Good idea, I didn't notice IPv6 had a similar flow, because I mainly worked on IPv4 code for my bugfix. Let me do that too.

BTW, why don't we have ICMPV6_PKT_TOOBIG in snat_v6_nat, but only have it in revNAT?

gentoo-root · 2023-05-11T18:40:06Z

bpf/lib/nat.h

+	/* Switch back to the outer header. */
+	tuple.nexthdr = IPPROTO_ICMP;
+
+	return snat_v4_rewrite_ingress(ctx, &tuple, state, off);
+}


You mean just this function call return snat_v4_rewrite_ingress(ctx, &tuple, state, off);? I don't see what can drift here, rewriting the outer headers is already contained in a separate function, so that it can be reused from two different flows.

I can return 0 from here and goto rewrite_ingress in the caller on non-errors, but that's going to be uglier, and I don't see much point. Please tell me what you think.

gentoo-root · 2023-05-11T18:41:40Z

bpf/lib/nat.h

+	struct ipv4_ct_tuple tuple = {};
+	struct ipv4_nat_entry *state;
+	struct iphdr iphdr;


Thanks, I wasn't aware of this context. What was the conclusion of that discussion? Shall I pass a pointer to tuple_buf to this function to reuse the one from the caller? Or there wasn't a clear conclusion?

ICMPV6_PKT_TOOBIG handling in snat_v4_rev_nat is pretty self-contained, and two indentation levels can easily be saved by moving this code into a separate function with just slight modifications. Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>

julianwiedmann · 2023-05-15T15:42:11Z

should we give the ICMPV6_PKT_TOOBIG part the same treatment then?

Good idea, I didn't notice IPv6 had a similar flow, because I mainly worked on IPv4 code for my bugfix. Let me do that too.

BTW, why don't we have ICMPV6_PKT_TOOBIG in snat_v6_nat, but only have it in revNAT?

Good question, I don't see it being discussed in #25054.

gentoo-root · 2023-05-17T10:51:30Z

/test

aditighag · 2023-05-17T15:44:12Z

Ah! The read-to-merge label was added by mlh. /cc @julianwiedmann Not sure if you all your review comments were addressed (looks that way), please double check.

julianwiedmann · 2023-05-19T07:49:43Z

Ah! The read-to-merge label was added by mlh. /cc @julianwiedmann Not sure if you all your review comments were addressed (looks that way), please double check.

All good, thanks! If it's green, merge we must.

gentoo-root added the release-note/misc This PR makes changes that have no direct user impact. label May 10, 2023

gentoo-root mentioned this pull request May 10, 2023

bpf: Track reply packets of egress gateway and SNATed host-local connections #25112

Merged

bpf: Use ct_extract_ports4 for ct_lookup4

f491afc

The switch in ct_lookup4 is almost identical to ct_extract_ports4. Reuse the existing function with a slight modification. Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>

gentoo-root force-pushed the ct-cleanup branch from 4dced4b to 15e05b8 Compare May 10, 2023 17:04

gentoo-root marked this pull request as ready for review May 10, 2023 17:09

gentoo-root requested a review from a team as a code owner May 10, 2023 17:09

gentoo-root requested review from qmonnet and julianwiedmann May 10, 2023 17:09

qmonnet added sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. kind/cleanup This includes no functional changes. labels May 10, 2023

qmonnet approved these changes May 10, 2023

View reviewed changes

gentoo-root force-pushed the ct-cleanup branch from 15e05b8 to 004c9a4 Compare May 10, 2023 18:50

julianwiedmann reviewed May 11, 2023

View reviewed changes

gentoo-root commented May 11, 2023

View reviewed changes

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 17, 2023

aditighag merged commit 00429c1 into cilium:main May 17, 2023
57 checks passed

gentoo-root added the needs-backport/1.12 label May 24, 2023

maintainer-s-little-helper bot added this to Needs backport from main in 1.12.10 May 24, 2023

gentoo-root removed the needs-backport/1.12 label May 25, 2023

maintainer-s-little-helper bot removed this from Needs backport from main in 1.12.10 May 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce the amount of repeating code in CT #25356

Reduce the amount of repeating code in CT #25356

gentoo-root commented May 10, 2023

qmonnet left a comment

gentoo-root commented May 10, 2023

julianwiedmann left a comment

julianwiedmann May 11, 2023

gentoo-root May 11, 2023

julianwiedmann May 15, 2023 •

edited

julianwiedmann May 11, 2023

gentoo-root May 11, 2023

julianwiedmann May 15, 2023 •

edited

gentoo-root left a comment •

edited

gentoo-root May 11, 2023

gentoo-root May 11, 2023

julianwiedmann commented May 15, 2023

gentoo-root commented May 17, 2023

aditighag commented May 17, 2023

julianwiedmann commented May 19, 2023

Reduce the amount of repeating code in CT #25356

Reduce the amount of repeating code in CT #25356

Conversation

gentoo-root commented May 10, 2023

qmonnet left a comment

Choose a reason for hiding this comment

gentoo-root commented May 10, 2023

julianwiedmann left a comment

Choose a reason for hiding this comment

julianwiedmann May 11, 2023

Choose a reason for hiding this comment

gentoo-root May 11, 2023

Choose a reason for hiding this comment

julianwiedmann May 15, 2023 • edited

Choose a reason for hiding this comment

julianwiedmann May 11, 2023

Choose a reason for hiding this comment

gentoo-root May 11, 2023

Choose a reason for hiding this comment

julianwiedmann May 15, 2023 • edited

Choose a reason for hiding this comment

gentoo-root left a comment • edited

Choose a reason for hiding this comment

gentoo-root May 11, 2023

Choose a reason for hiding this comment

gentoo-root May 11, 2023

Choose a reason for hiding this comment

julianwiedmann commented May 15, 2023

gentoo-root commented May 17, 2023

aditighag commented May 17, 2023

julianwiedmann commented May 19, 2023

julianwiedmann May 15, 2023 •

edited

julianwiedmann May 15, 2023 •

edited

gentoo-root left a comment •

edited