bpf: nat: Make snat_v6_needed() get on par with snat_v4_prepare_state()

[qmonnet]

We recently implemented snat_v6_needed() in the datapath to determine whether we need SNAT for IPv6 masquerading; but the function was written long before it was merged and was based on snat_v4_needed(), which has since been consolidated and renamed as snat_v4_prepare_state(), and changed again to prepare for Egress Gateway.

This PR aims at bringing snat_v6_needed() as close to snat_v4_prepare_state() as possible. In the process, we also fix a condition for SNAT-ing packets from local endpoints, and the IP address to use for masquerading packets leaving through a tunnel.

Commits:

• bpf: nat: Move check on dest being outside cluster in snat_v6_needed()
• bpf: nat: Pass target directly to snat_v6_needed()
• bpf: nat: Do not skip IPv6 SNAT for packets from local endpoints
• bpf: nat: Rename snat_v6_needed() into snat_v6_prepare_state()
• bpf: nat: Use gateway IP for SNAT in the case of BPF overlay

bpf: Update IPv6 BPF masquerading code to bring it closer to IPv4's, fix SNAT for packets from local endpoints, for overlay

[qmonnet]

/test

[joestringer]

Is this really a kind/feature? What user-facing impact is there for this change?

At first I was a bit alarmed that a feature was being proposed two days before feature freeze, and still marked as draft the day before. But looking at the size of the change, the release note descrption, and the fact it's marked as release-note/misc, now I think maybe it's not actually a feature, just a nice-to-have code cleanup?

If you can get this into the tree tomorrow in coordination with the relevant code owners then I don't mind, but the window is starting to close on PR proposals for 1.14. We will be moving to a lower-churn phase of development shortly and at that point we'll be scrutinizing each change a bit more closely to determine whether it really matters to merge this around now in time for 1.14 or in a couple of weeks' time (for 1.15 dev cycle).

Woops, I mixed up my lists! The bit about churn still holds, but for now there's still some room for wiggle room around including enhancements, particularly if you think it'll help the 1.14 maintenance cycle post-release.

[qmonnet]

Thanks @joestringer for your comment and explanations! Yes, I'm aware of the feature freeze and lower churn phase. Our current labels don't allow us to make the distinction between “we absolutely need this for the release”, and “this is maybe not essential, but important enough that I would like some careful consideration on whether we should include this, and not let it slip unnoticed”. This PR would fall into the second category, I'd say.

I do believe it's an enhancement worth having, as it would bring IPv4 and IPv6 paths closer and make it easier to maintain in the future.

One additional consideration is that I'm not currently sure of whether the check on local_ep in the PR should be moved outside of the #ifdef ENABLE_MASQUERADE_IPV6, like the PR currently does (and like we do on the IPv4 path); I'm trying to understand if we need this and what the consequences are, and whether this is a bug or not in the current version of the code that we would address by merging this - but then, if there's a bug, we'll backport anyway. This is why this PR is still in draft status, by the way. [EDIT: Cleared it out with Gilberto, there's no bug, and we don't need this code to move outside of the #ifdef guard, so all is good.]

[qmonnet]

/test

[qmonnet]

/test
Rebase mistakes 🙄

[qmonnet]

/test

[julianwiedmann]

👋 thanks! Not sure I will manage to get in a proper review today, but some notes:

• when called with IS_BPF_OVERLAY, do we need a matching case for the SNAT with IPV4_GATEWAY? Looks like this is ROUTER_IP in the IPv6 world.
• it's a bit suspicious that we don't need to set target->from_local_endpoint. Did we miss to update snat_v6_nat_can_skip() ?

[qmonnet]

* when called with `IS_BPF_OVERLAY`, do we need a matching case for the SNAT with `IPV4_GATEWAY`? Looks like this is `ROUTER_IP` in the IPv6 world.

Yes, I think you're right. Otherwise we won't have the gateway/router IP when leaving through the tunnel. I've added a commit to fix this.

* it's a bit suspicious that we don't need to set `target->from_local_endpoint`. Did we miss to update `snat_v6_nat_can_skip()` ?

It is. And yes you're right again, I think this is because the IPv6 masquerading PR didn't pick up the update in that function. I've also added a commit to fix this.

Thanks a lot for the preliminary comments, I'm looking forward to the review 😄

[qmonnet]

Given the fixes mentioned above, I'll change the type for this PR to kind/bug.

[qmonnet]

Looks like I messed up when reordering commits, and I've got one that doesn't compile. I'll try to fix this in the evening.

[joestringer]

[EDIT: Cleared it out with Gilberto, there's no bug, and we don't need this code to move outside of the #ifdef guard, so all is good.]

Given the fixes mentioned above, I'll change the type for this PR to kind/bug.

I'm a bit confused. This is a refactor to align code right? The release note and release note label don't indicate anything about a bug being fixed.

At this point I'm willing to ship this PR in v1.14 if it lands before I branch (some time in the next few days), but I think we should remove the release-blocker label. It's not obvious to me that it's making sufficient ongoing progress to guarantee it lands in time, and it's about time we consider v1.14 development done and move on. If it turns out that this is still a bugfix and it doesn't land before branching, then we can make a call about whether to prepare+backport a more targeted fix or backport this PR when it's ready.

[qmonnet]

I'm a bit confused. This is a refactor to align code right? The release note and release note label don't indicate anything about a bug being fixed.

The confusion probably comes from my poor messaging, apologies. These two quotes refer to different things.

• This PR started as a refactor to align code.
• Doing so, I believed at first there might be a bug we might need to address with regards to the conditions under which we needed to run the checks on local_ep. But it fact no, we're good on this side.
• Then Julian raised that there were still some divergences between the IPv4 and IPv6 paths after the PR regarding conditions for SNAT-ing packets from local endpoints, and for overlay. These are bugs in the current implementation, and this PR now addresses them.

I did forget to update the release note. This is fixed now.

I'm not sure what expectations you have regarding the release note label. My understanding is that release-note/bug is for PRs addressing issues affecting previous releases, which is not the case at the moment (IPv6 masquerading was added recently).

At this point [...]

Sorry to hear about the insufficient progress - You know I've been busy with patch releases, but then I know you're making sure everything is in order for 1.14 so I guess it's fair. But the proposed resolution sounds good, let's target completion before branching, or consider a backport otherwise. Thanks a lot for your feedback!

[qmonnet]

/test

[borkmann]

Lgtm, both v4 and v6 variant are aligned logic-wise modulo the egress GW which is only for IPv4 at the moment.

The v4 code has this one:

# if defined(ENABLE_CLUSTER_AWARE_ADDRESSING) && defined(ENABLE_INTER_CLUSTER_SNAT)
	if (target->cluster_id != 0 &&
	    target->cluster_id != CLUSTER_ID) {
		target->addr = IPV4_INTER_CLUSTER_SNAT;
		return true;
	}
# endif

Is this again for v4-only? :/

[qmonnet]

Is this again for v4-only? :/

I thought so, because I only found IPV4_INTER_CLUSTER_SNAT and no IPv6 equivalent, plus I couldn't find where ENABLE_CLUSTER_AWARE_ADDRESSING and ENABLE_INTER_CLUSTER_SNAT were being set at first. But looking again, the IPv6 version was maybe missing only because IPv6 BPF masquerading was not here.

@YutaroHayakawa Could you please take a look and comment? Do we need the IPv6 equivalent to the IPv4 snippet quoted by Daniel now that we have IPv6 masquerading?

[EDIT: It looks like the changes for inter-cluster NAT are not trivial, so if we need them now, this would likely be for a follow-up PR.]

[julianwiedmann]

lgtm now, thank you Quentin! All good wrt inter-cluster-SNAT, that whole feature is currently IPv4-only.