datapath/iptables: Masquerade hairpin traffic that traversed the stack by tgraf · Pull Request #10928 · cilium/cilium

tgraf · 2020-04-10T00:45:40Z

This PR builds on #10926

The traffic is sent to the stack and hairpin'ed back into a local pod
after a component on the stack has applied a DNAT rule, the traffic must
be SNATed to ensure the reverse NAT can take place. This can happen if
portmap or kiam is being used and redirection happens to a local
destination.

The masquerade filter must be limited as not all DNAT traffic may be
affected. NodePort traffic from a non-local source must remain
unmasqueraded in order for trafficPolicy=local to continue working.

Also, when EnableEndpointRoutes is enabled, traffic always traverses the
stack and must not be masqueraded either.

Fixes: #9784
Requires: #10926

coveralls · 2020-04-10T01:18:15Z

Coverage decreased (-0.02%) to 46.812% when pulling 02115cf on pr/tgraf/fix-hairpin-via-stack into 9fbd820 on master.

tgraf · 2020-04-10T01:19:33Z

test-me-please

00:19:58.206  There was an error while executing `VBoxManage`, a CLI used by Vagrant
00:19:58.206  for controlling VirtualBox. The command and stderr is shown below.
00:19:58.206  
00:19:58.206  Command: ["list", "hostonlyifs"]
00:19:58.206  
00:19:58.206  Stderr: VBoxManage: error: Failed to create the VirtualBox object!

tgraf · 2020-04-10T01:55:48Z

test-me-please

Edit: provisioning failure

19:06:36  Checking for older boxes...
19:06:36  Removing box 'cilium/ubuntu-next' (v50) with provider 'virtualbox'...
19:06:37  VBoxManage: error: Failed to create the VirtualBox object!
19:06:37  VBoxManage: error: Document is empty.
19:06:37  VBoxManage: error: Location: '/root/.config/VirtualBox/VirtualBox.xml', line 1 (0), column 1.
19:06:37  VBoxManage: error: /home/vbox/vbox-6.0.18/src/VBox/Main/src-server/VirtualBoxImpl.cpp[624] (nsresult VirtualBox::init())
19:06:37  VBoxManage: error: Details: code NS_ERROR_FAILURE (0x80004005), component VirtualBoxWrap, interface IVirtualBox

christarazi · 2020-04-10T06:12:56Z

test-me-please

00:24:46.741  + cd /home/jenkins/workspace/Cilium-PR-Ginkgo-Tests-Validated/runtime-gopath/src/github.com/cilium/cilium/test
00:24:46.741  + vagrant destroy runtime --force
00:24:52.567  There was an error while executing `VBoxManage`, a CLI used by Vagrant
00:24:52.567  for controlling VirtualBox. The command and stderr is shown below.
00:24:52.567  
00:24:52.567  Command: ["list", "hostonlyifs"]
00:24:52.567  
00:24:52.567  Stderr: VBoxManage: error: Failed to create the VirtualBox object!
00:24:52.567  VBoxManage: error: Document is empty.
00:24:52.567  VBoxManage: error: Location: '/root/.config/VirtualBox/VirtualBox.xml', line 1 (0), column 1.
00:24:52.567  VBoxManage: error: /home/vbox/vbox-6.0.18/src/VBox/Main/src-server/VirtualBoxImpl.cpp[624] (nsresult VirtualBox::init())
00:24:52.567  VBoxManage: error: Details: code NS_ERROR_FAILURE (0x80004005), component VirtualBoxWrap, interface IVirtualBox

tgraf · 2020-04-10T17:17:03Z

test-me-please

Hit #10118

tgraf · 2020-04-11T08:41:12Z

test-me-please

borkmann

Given we haven't seen this in non-chaining scenarios so far, should we gate this to avoid adding the rule for native Cilium setups?

qmonnet

Looks good to me, with minor nits.

tgraf · 2020-04-15T20:33:46Z

Given we haven't seen this in non-chaining scenarios so far, should we gate this to avoid adding the rule for native Cilium setups?

This can happen in direct routing mode as well if ENABLE_ROUTING is enabled.

tgraf · 2020-04-16T10:10:44Z

test-me-please

IP frag test failed:

/home/jenkins/workspace/Cilium-PR-Ginkgo-Tests-Validated/k8s-1.11-gopath/src/github.com/cilium/cilium/test/ginkgo-ext/scopes.go:430
Failed to account for IPv4 fragments (in)
Expected
    <[]int | len:2, cap:2>: [16, 17]
To satisfy at least one of these matchers: [%!s(*matchers.EqualMatcher=&{[16 16]}) %!s(*matchers.EqualMatcher=&{[20 12]})]
/home/jenkins/workspace/Cilium-PR-Ginkgo-Tests-Validated/k8s-1.11-gopath/src/github.com/cilium/cilium/test/k8sT/Services.go:559

joestringer

Change looks good, do we have a small repro we can integrate into the CI? This type of bug seems easy to break by just rearranging rules a bit.

Packets to a host IP are currently redirected via cilium_host/cilium_net. The reason for this is mostly historic. For other packets where routing by the kernel routing tables is desired, packets are already passed on via TC_ACT_OK to the stack directly. The two cases where this redirection is needed are: * For proxy redirection due to a kernel limitation on passing the routing tables multiple times. This case is left untouched. * For the HOST_REDIRECT_TO_INGRESS case, e.g. flannel integration. This case is left untouched. The IPv4 and IPv6 case is brought in line to not accidently lose this logic later on. A side effect of this is that the skb gets scrubbed including the skb->mark. The presence of the identity in the skb->mark is being relied on in a follow-up fix however. Therfore, pass packets via the stack using TC_ACT_OK. This is faster, simpler, and allows for the identity to be carried in the mark. Fixes: #9784 Signed-off-by: Thomas Graf <thomas@cilium.io>

The traffic is sent to the stack and hairpin'ed back into a local pod after a component on the stack has applied a DNAT rule, the traffic must be SNATed to ensure the reverse NAT can take place. This can happen if portmap or kiam is being used and redirection happens to a local destination. The masquerade filter must be limited as not all DNAT traffic may be affected. NodePort traffic from a non-local source must remain unmasqueraded in order for trafficPolicy=local to continue working. Also, when EnableEndpointRoutes is enabled, traffic always traverses the stack and must not be masqueraded either. Fixes: #9784 Signed-off-by: Thomas Graf <thomas@cilium.io>

Add checks for multi-node and intra-node connectivity via hostport. These tests will remain unready if hostport/portmap is not enabled. Signed-off-by: Thomas Graf <thomas@cilium.io>

Requires the following tests to pass: ``` NAME READY STATUS RESTARTS AGE echo-a-5f555bbc8b-9cxv9 1/1 Running 0 41s echo-b-659766fb56-zw2wl 1/1 Running 0 41s echo-b-host-65d7db76d8-5wmhm 1/1 Running 0 41s host-to-b-multi-node-clusterip-c7557d4f8-gv6ws 1/1 Running 0 41s host-to-b-multi-node-headless-5dfcdf9b76-9hcqn 1/1 Running 0 41s pod-to-a-6cf58894b7-mqg67 1/1 Running 0 40s pod-to-a-allowed-cnp-5898f7d8c9-bdfxz 1/1 Running 0 41s pod-to-a-external-1111-5779fb7cb9-tgdlh 1/1 Running 0 39s pod-to-a-l3-denied-cnp-74b9566cc7-zjhhh 1/1 Running 0 41s pod-to-b-intra-node-77b485d996-xfv45 1/1 Running 0 40s pod-to-b-intra-node-hostport-6c55bf8459-vddt2 1/1 Running 0 40s pod-to-b-multi-node-clusterip-75f5c78f68-2lk8x 1/1 Running 0 40s pod-to-b-multi-node-headless-5df88f9bd4-f5jlt 1/1 Running 0 40s pod-to-b-multi-node-hostport-d7c8d659f-4xqpt 1/1 Running 0 39s pod-to-external-fqdn-allow-google-cnp-74466b4c6f-dxvlq 1/1 Running 0 39s ``` Signed-off-by: Thomas Graf <thomas@cilium.io>

tgraf · 2020-04-17T13:42:21Z

Change looks good, do we have a small repro we can integrate into the CI? This type of bug seems easy to break by just rearranging rules a bit.

Good point. I've extended the connectivity-check YAML to include hostport and added it to the CI. This uncovered one more case that needed fixing which required an additional commit.

tgraf · 2020-04-17T13:42:45Z

test-me-please

One test failed with:

00:48:24.124  getting vagrant kubeconfig from provisioned vagrant cluster
00:48:26.959  checking whether kubeconfig works for vagrant cluster
00:48:26.959  Unable to connect to the server: tls: first record does not look like a TLS handshake

GKE and -with-kernel tests all passed

borkmann · 2020-04-17T21:38:30Z

        imagePullPolicy: IfNotPresent
+        ports:
+        - containerPort: 80
+          hostPort: 40000


Also added one here for services (ebd7392) but only for the BPF case so far. I wonder if we somewhat could consolidate both, just a thought; not needed for this series in here.

joestringer · 2020-04-17T23:03:58Z

Tests were interrupted, retrying:
https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-Validated/18901/execution/node/198/log/?consoleFull

joestringer · 2020-04-17T23:04:06Z

restart-ginkgo

tgraf · 2020-04-20T15:09:02Z

restart-ginkgo

tgraf added kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. labels Apr 10, 2020

tgraf requested a review from a team April 10, 2020 00:45

tgraf mentioned this pull request Apr 10, 2020

Request time out from container to other container using hostIP:hostPort on same host with portmap CNI chained #9784

Closed

tgraf added needs-backport/1.7 labels Apr 10, 2020

tgraf mentioned this pull request Apr 11, 2020

CI: K8sServicesTest Checks service across nodes with L7 policy Tests NodePort with L7 Policy: tftp connect via IPv6 failed #10118

Closed

borkmann approved these changes Apr 15, 2020

View reviewed changes

qmonnet reviewed Apr 15, 2020

View reviewed changes

Comment thread pkg/datapath/iptables/iptables.go Outdated

Comment thread pkg/datapath/linux/linux_defaults/mark.go

tgraf force-pushed the pr/tgraf/fix-hairpin-via-stack branch from 6986f4b to 5483585 Compare April 16, 2020 10:08

joestringer reviewed Apr 16, 2020

View reviewed changes

tgraf added 4 commits April 17, 2020 15:41

connectivity-check: Add hostport checks

c71dd1c

Add checks for multi-node and intra-node connectivity via hostport. These tests will remain unready if hostport/portmap is not enabled. Signed-off-by: Thomas Graf <thomas@cilium.io>

tgraf force-pushed the pr/tgraf/fix-hairpin-via-stack branch from 5483585 to 02115cf Compare April 17, 2020 13:41

tgraf requested a review from a team as a code owner April 17, 2020 13:41

tgraf requested a review from a team April 17, 2020 13:41

tgraf requested a review from borkmann April 17, 2020 13:42

borkmann reviewed Apr 17, 2020

View reviewed changes

borkmann approved these changes Apr 17, 2020

View reviewed changes

joestringer added the priority/release-blocker label Apr 17, 2020

aanm approved these changes Apr 20, 2020

View reviewed changes

tgraf merged commit 9499596 into master Apr 20, 2020

tgraf deleted the pr/tgraf/fix-hairpin-via-stack branch April 20, 2020 21:06

jrfastab added backport-pending/1.7 and removed needs-backport/1.7 labels Apr 22, 2020

jrfastab mentioned this pull request Apr 22, 2020

v1.7 backports 2020-04-22 #11109

Merged

jrfastab mentioned this pull request Apr 29, 2020

v1.7 backport 2020-04-29 bpf #11239

Merged

christarazi mentioned this pull request Apr 30, 2020

v1.6 backports 2020-04-30 #11266

Merged

joestringer added backport-done/1.7 and removed backport-pending/1.7 labels May 7, 2020

qmonnet mentioned this pull request May 12, 2020

v1.6 backports 2020-05-12 #11496

Merged

qmonnet added backport-pending/1.6 and removed needs-backport/1.6 labels May 12, 2020

joestringer added backport-done/1.6 and removed backport-pending/1.6 labels May 26, 2020

Conversation

tgraf commented Apr 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Apr 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgraf commented Apr 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgraf commented Apr 10, 2020 • edited by christarazi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christarazi commented Apr 10, 2020 • edited by tgraf Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgraf commented Apr 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgraf commented Apr 11, 2020

Uh oh!

borkmann left a comment

Choose a reason for hiding this comment

Uh oh!

qmonnet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tgraf commented Apr 15, 2020

Uh oh!

tgraf commented Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joestringer left a comment

Choose a reason for hiding this comment

Uh oh!

tgraf commented Apr 17, 2020

Uh oh!

tgraf commented Apr 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

borkmann Apr 17, 2020

Choose a reason for hiding this comment

Uh oh!

joestringer commented Apr 17, 2020

Uh oh!

joestringer commented Apr 17, 2020

Uh oh!

tgraf commented Apr 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

tgraf commented Apr 10, 2020 •

edited

Loading

coveralls commented Apr 10, 2020 •

edited

Loading

tgraf commented Apr 10, 2020 •

edited

Loading

tgraf commented Apr 10, 2020 •

edited by christarazi

Loading

christarazi commented Apr 10, 2020 •

edited by tgraf

Loading

tgraf commented Apr 10, 2020 •

edited

Loading

tgraf commented Apr 16, 2020 •

edited

Loading

tgraf commented Apr 17, 2020 •

edited

Loading