bpf: fix misconfigured nat to 0.0.0.0 on !masquerade config #14596

borkmann · 2021-01-12T23:57:03Z

(See commit msg for detailled analysis.)

borkmann · 2021-01-13T00:22:52Z

test-me-please

brb

Thanks for fixing this!

I think that the check is still error prone. Could we conditionally enable EnableBPFMasquerade in pkg/option/option.go by checking whether EnableIPv4 and Masquerade are true?

Also, I think that it fixes 0962c02 instead of other two linked commits in your msg. The problem came to the surface once we had decoupled NP and MASQ IP addrs.

borkmann · 2021-01-13T08:53:32Z

I think that the check is still error prone. Could we conditionally enable EnableBPFMasquerade in pkg/option/option.go by checking whether EnableIPv4 and Masquerade are true?

I don't think we could as this would mean that iptables-based masquerading rules won't be installed if we auto-enable the EnableBPFMasquerade config option.

Fwiw, the check is now in line with the rest of BPF masq tests:

root@apoc:~/go/src/github.com/cilium/cilium# git grep -n EnableBPFMasquerade | grep -n EnableIPv4Masquerade
1:daemon/cmd/daemon.go:518:	if option.Config.EnableIPv4Masquerade && option.Config.EnableBPFMasquerade &&
3:daemon/cmd/daemon.go:540:	if option.Config.EnableIPv4Masquerade && option.Config.EnableBPFMasquerade {
10:pkg/datapath/iptables/iptables.go:1185:		if option.Config.EnableIPv4Masquerade && !option.Config.EnableBPFMasquerade {
12:pkg/datapath/linux/config/config.go:425:		if option.Config.EnableIPv4Masquerade && option.Config.EnableBPFMasquerade {
13:pkg/datapath/linux/config/config.go:541:		if option.Config.EnableIPv4Masquerade && option.Config.EnableBPFMasquerade {
14:pkg/datapath/loader/loader.go:175:	if option.Config.EnableIPv4Masquerade && option.Config.EnableBPFMasquerade && bpfMasqIPv4Addrs != nil {
15:pkg/datapath/loader/template.go:232:		if option.Config.EnableIPv4Masquerade && option.Config.EnableBPFMasquerade {

Also, I think that it fixes 0962c02 instead of other two linked commits in your msg. The problem came to the surface once we had decoupled NP and MASQ IP addrs.

Ok, will update the commit msg.

borkmann · 2021-01-13T08:54:38Z

(CI green, changing commit msg only)

brb · 2021-01-13T09:00:07Z

I don't think we could as this would mean that iptables-based masquerading rules won't be installed if we auto-enable the EnableBPFMasquerade config option.

I meant in pkg/option/option.go to do option.Config.EnableBPFMasquerade = option.Config.EnableBPFMasquerade & option.Config.EnableIPv4 & option.Config.Masquerade. This would allow us to drop the check for option.Config.Masquerade in the most places.

pchaigno

LGTM, although Martynas's proposed change would make it harder to re-introduce this sort of error later on.

pchaigno · 2021-01-13T09:25:06Z

(CI green, changing commit msg only)

Not GKE. Failed due to scaling issues.

Thomas reported: When I do Pod to Pod I see this on the interface of the VM: 21:30:11.022617 IP 0.0.0.0.53336 > 10.240.0.22.8080: Flags [S], seq 2523807568, win 64240, options [mss 1460,sackOK,TS val 207396274 ecr 0,nop,wscale 7], length 0 When I do Host to Pod I see this on the interface of the same VM: 21:30:02.275063 IP 10.240.0.35.46236 > 10.240.0.22.8080: Flags [S], seq 1718112282, win 64240, options [mss 1460,sackOK,TS val 2667439682 ecr 0,nop,wscale 7], length 0 The NAT table indeed had a bunch of entries that would cause the 0.0.0.0 translation: [...] TCP OUT 10.240.0.16:60250 -> 10.0.222.20:443 XLATE_SRC 0.0.0.0:60250 Created=17376sec HostLocal=0 TCP OUT 10.240.0.16:46342 -> 10.240.0.49:9153 XLATE_SRC 0.0.0.0:46342 Created=17376sec HostLocal=0 [...] From the config, it turns out that the following was set: [...] enable-bpf-masquerade: "true" masquerade: "false" [...] And looking at the headers, ENABLE_MASQUERADE was enabled. This in fact meant that in snat_v4_needed() we'd look up the endpoint based on the src IPv4 and given neither ENABLE_IP_MASQ_AGENT nor ENCAP_IFINDEX was set, IPV4_MASQUERADE is set for the NAT address. However, the latter will be 0.0.0.0 in this case since IPV4_MASQUERADE is not defined from template side given it depends on ... if option.Config.EnableIPv4Masquerade && option.Config.EnableBPFMasquerade && bpfMasqIPv4Addrs != nil { if option.Config.EnableIPv4 { ipv4 := bpfMasqIPv4Addrs[ifName] opts["IPV4_MASQUERADE"] = byteorder.HostSliceToNetwork(ipv4, reflect.Uint32).(uint32) } } ... of which option.Config.EnableIPv4Masquerade is false. Yet ENABLE_MASQUERADE otoh is defined in a more relaxed manner, that is option.Config.EnableBPFMasquerade && option.Config.EnableIPv4, both of which were true in this case. Fix it by requiring option.Config.EnableIPv4Masquerade instead of option.Config.EnableIPv4 for the latter. The commit got uncovered by a0ec2ad ("datapath: Decouple IPV4_MASQUERADE from IPV4_NODEPORT"), but the original commit introducing the issue is 0962c02 ("datapath: Enable BPF MASQ for veth mode in IPv4"). Fixes: 0962c02 ("datapath: Enable BPF MASQ for veth mode in IPv4") Related: 5462ced ("datapath: Do not set exclusion CIDR if IPv4 is disabled") Related: a0ec2ad ("datapath: Decouple IPV4_MASQUERADE from IPV4_NODEPORT") Reported-by: Thomas Graf <thomas@cilium.io> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>

borkmann · 2021-01-13T09:54:25Z

I don't think we could as this would mean that iptables-based masquerading rules won't be installed if we auto-enable the EnableBPFMasquerade config option.

I meant in pkg/option/option.go to do option.Config.EnableBPFMasquerade = option.Config.EnableBPFMasquerade & option.Config.EnableIPv4 & option.Config.Masquerade. This would allow us to drop the check for option.Config.Masquerade in the most places.

The old config option option.Config.Masquerade does not exist anymore, we do have however option.Config.EnableIPv4Masquerade and option.Config.EnableIPv6Masquerade where at least the former implies the v4 + masq setting. I'm a bit hesitant to do something like option.Config.EnableBPFMasquerade = option.Config.EnableBPFMasquerade && option.Config.EnableIPv4Masquerade given we might need it for option.Config.EnableIPv6Masquerade as well in future to get rid of the iptables masq there and convert to BPF too.

pchaigno · 2021-01-13T09:56:42Z

retest-gke

Given the former is a more specific subflag of the latter for the agent, we should auto-disable it and log an info message in the agent. This needs to be adapted when we also implement v6 BPF-based masq. Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>

borkmann · 2021-01-13T11:31:09Z

test-me-please

borkmann added kind/bug This is a bug in the Cilium logic. pending-review sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. labels Jan 12, 2021

borkmann requested review from brb and a team January 12, 2021 23:57

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jan 12, 2021

maintainer-s-little-helper bot assigned brb Jan 12, 2021

maintainer-s-little-helper bot added this to In progress in 1.10.0 Jan 12, 2021

maintainer-s-little-helper bot added this to Needs backport from master in 1.9.2 Jan 12, 2021

borkmann added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Jan 12, 2021

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jan 12, 2021

brb reviewed Jan 13, 2021

View reviewed changes

pchaigno approved these changes Jan 13, 2021

View reviewed changes

borkmann force-pushed the pr/bpf-masq-fix branch from 362c597 to 845cfd7 Compare January 13, 2021 09:48

tgraf approved these changes Jan 13, 2021

View reviewed changes

borkmann requested a review from a team January 13, 2021 11:06

borkmann requested a review from brb January 13, 2021 12:15

brb approved these changes Jan 13, 2021

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 Jan 13, 2021

borkmann merged commit d678513 into master Jan 13, 2021

borkmann deleted the pr/bpf-masq-fix branch January 13, 2021 15:20

errordeveloper mentioned this pull request Jan 14, 2021

v1.9 backports 2021-01-14 #14613

Merged

errordeveloper added backport-pending/1.9 and removed needs-backport/1.9 labels Jan 14, 2021

maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.9 in 1.9.2 Jan 14, 2021

aanm added backport-done/1.9 and removed backport-pending/1.9 labels Jan 18, 2021

maintainer-s-little-helper bot moved this from Backport pending to v1.9 to Backport done to v1.9 in 1.9.2 Jan 18, 2021

aanm mentioned this pull request Jan 20, 2021

Prepare for release v1.9.2 #14666

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpf: fix misconfigured nat to 0.0.0.0 on !masquerade config #14596

bpf: fix misconfigured nat to 0.0.0.0 on !masquerade config #14596

borkmann commented Jan 12, 2021

borkmann commented Jan 13, 2021

brb left a comment

borkmann commented Jan 13, 2021 •

edited

Loading

borkmann commented Jan 13, 2021

brb commented Jan 13, 2021 •

edited

Loading

pchaigno left a comment

pchaigno commented Jan 13, 2021

borkmann commented Jan 13, 2021 •

edited

Loading

pchaigno commented Jan 13, 2021

borkmann commented Jan 13, 2021

bpf: fix misconfigured nat to 0.0.0.0 on !masquerade config #14596

bpf: fix misconfigured nat to 0.0.0.0 on !masquerade config #14596

Conversation

borkmann commented Jan 12, 2021

borkmann commented Jan 13, 2021

brb left a comment

Choose a reason for hiding this comment

borkmann commented Jan 13, 2021 • edited Loading

borkmann commented Jan 13, 2021

brb commented Jan 13, 2021 • edited Loading

pchaigno left a comment

Choose a reason for hiding this comment

pchaigno commented Jan 13, 2021

borkmann commented Jan 13, 2021 • edited Loading

pchaigno commented Jan 13, 2021

borkmann commented Jan 13, 2021

borkmann commented Jan 13, 2021 •

edited

Loading

brb commented Jan 13, 2021 •

edited

Loading

borkmann commented Jan 13, 2021 •

edited

Loading