bpf: nodeport: provide L4 ports for SNAT in LB egress path

[julianwiedmann]

For LB traffic that gets forwarded to a remote backend in non-DSR mode, tail_nodeport_nat_egress_ipv*() calls snat_v*_nat() to perform SNAT on the packet. Under the covers, this extracts a fresh CT tuple to look up / build a SNAT entry.

But for LB traffic we don't require any of the ICMP handling in that code path, and we already extract a CT tuple for building tunnel headers in XDP mode. So we can optimize this code path, and provide a fully populated CT tuple to the SNAT helper.

One additional benefit is that we fix handling for fragmented IPv4 packets, as lb4_extract_tuple() knows how to extract their L4 ports while snat_v4_nat() doesn't.

Fix SNAT by the N/S load-balancer for fragmented IPv4 requests.

[julianwiedmann]

/test

[julianwiedmann]

Related: #11180

[gentoo-root]

Looks good to me, but I suspect it may suffer from the same fragment double accounting issue as my #25340, because you also add a call of ipv4_handle_fragmentation. However, I stopped understanding how ipv4_handle_fragmentation leads to ct_lookup4. I'll take one more look tomorrow, maybe something changed in its implementation.

[julianwiedmann]

Looks good to me, but I suspect it may suffer from the same fragment double accounting issue as my #25340, because you also add a call of ipv4_handle_fragmentation. However, I stopped understanding how ipv4_handle_fragmentation leads to ct_lookup4. I'll take one more look tomorrow, maybe something changed in its implementation.

Ah, is that blocking you in #25340 ? It's most likely

cilium/bpf/lib/ipv4.h

Line 148 in 28de75d

update_metrics(ctx_full_len(ctx), dir, REASON_FRAG_PACKET);

then. That part is a bit silly, I wouldn't expect it to produce accurate metrics at the moment ... we can hardly expect that every packet needs just a single L4 port lookup.

It's probably fine to just push this metrics update into __ct_lookup4() instead - there we should have more confidence that a given CT tuple only gets looked up (== accounted) once. Let's try that in a follow-on PR?

[gentoo-root]

is that blocking you in #25340 ?

Yes.

It's most likely

Ah right, it's called twice, because ct_lookup4 also calls ct_extract_ports4.

I wouldn't expect it to produce accurate metrics at the moment

We have a test for that, which breaks after my change.

we can hardly expect that every packet needs just a single L4 port lookup.

Right, so I tried to write a test that would break even before my change, so far without luck, though.

It's probably fine to just push this metrics update into __ct_lookup4() instead - there we should have more confidence that a given CT tuple only gets looked up (== accounted) once.

I believe there is a scenario with host firewall where ct_lookup4 is called more than once. I couldn't trigger it yet for some reason, though.

[julianwiedmann]

It's probably fine to just push this metrics update into __ct_lookup4() instead - there we should have more confidence that a given CT tuple only gets looked up (== accounted) once.

I believe there is a scenario with host firewall where ct_lookup4 is called more than once.

Yep, I would fully expect that we still have cases where that's the case. But those we should fix over time :)

[gentoo-root]

But those we should fix over time :)

OK, if I don't manage to reproduce it with host firewall, I'll try to move update_metrics in my pull request to see if it unblocks the failing test. Thanks for the suggestion! I was looking to fix it once and for all, but probably you are right.

[julianwiedmann]

But those we should fix over time :)

OK, if I don't manage to reproduce it with host firewall, I'll try to move update_metrics in my pull request to see if it unblocks the failing test. Thanks for the suggestion! I was looking to fix it once and for all, but probably you are right.

Just FYI, I suspect that this might even produce slightly better metrics (more accurate direction variable, basically). I didn't have time to follow-up on #25880 yet.