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: fix nodeport to avoid sending loopback address out to wire #10841

Merged
merged 1 commit into from Apr 3, 2020

Conversation

borkmann
Copy link
Member

@borkmann borkmann commented Apr 3, 2020

See commit msg.

@borkmann borkmann added pending-review sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/bug This PR fixes an issue in a previous release of Cilium. labels Apr 3, 2020
@borkmann borkmann requested review from brb and a team April 3, 2020 08:27
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Apr 3, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.7.2 Apr 3, 2020
@borkmann
Copy link
Member Author

borkmann commented Apr 3, 2020

test-me-please

While testing recently, I've noticed the case where we start to use
the 169.254.42.1 loopback address for requests from outside:

  # tcpdump -i any port 30042 or 80 -n
  [...]
  09:50:15.835618 IP 192.168.178.28.80 > 169.254.42.1.32882: Flags [S.], seq 689888661, ack 2221057376, win 65160, options [mss 1460,sackOK,TS val 3644467179 ecr 3644467179,nop,wscale 7], length 0
  09:50:16.863069 IP 192.168.178.28.80 > 169.254.42.1.32882: Flags [S.], seq 689888661, ack 2221057376, win 65160, options [mss 1460,sackOK,TS val 3644468207 ecr 3644467179,nop,wscale 7], length 0
  [...]

This can happen if the backend IP (192.168.178.28) is the same IP that
is doing the request to the frontend:

  # ./cilium/cilium service list
  ID   Frontend               Service Type   Backend
  1    192.168.178.29:30042   NodePort       1 => 192.168.178.28:80

Then we're hitting the codepath where we replace the 192.168.178.28 src
address with 169.254.42.1 (IPV4_LOOPBACK) and try to send it back out
the NodePort device, which is just wrong. That code was only intended
for node local Pod to Pod ClusterIP handling we had before socket LB.

For NodePort requests from outside the node this should not be done. We
can handle this situation just fine in case of BPF-based SNAT, and in
case of DSR it is expected to not work and we should not try to do any
special NAT'ing or such. For east-west traffic on Cilium-managed nodes,
we are simply using the socket LB anyway for everything, so we won't
run into this corner case either.

There is a unused DISABLE_LOOPBACK_LB define, to compile this out. Fix
it by specifying this define from init.sh.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
@borkmann
Copy link
Member Author

borkmann commented Apr 3, 2020

test-me-please

@coveralls
Copy link

coveralls commented Apr 3, 2020

Coverage Status

Coverage increased (+0.02%) to 45.476% when pulling 490fda5 on pr/nodeport-fix-loopback into ad9ccef on master.

@borkmann borkmann merged commit b9ef15f into master Apr 3, 2020
1.8.0 automation moved this from In progress to Merged Apr 3, 2020
@borkmann borkmann deleted the pr/nodeport-fix-loopback branch April 3, 2020 10:32
@joestringer joestringer removed this from Needs backport from master in 1.7.2 Apr 3, 2020
@joestringer joestringer added this to Needs backport from master in 1.7.3 Apr 3, 2020
@joestringer joestringer moved this from Needs backport from master to Backport done to v1.7 in 1.7.3 Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/bug This PR fixes an issue in a previous release of Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
No open projects
1.7.3
Backport done to v1.7
1.8.0
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

5 participants