Fix L7 ingress proxy iptables redirects in direct routing mode by joestringer · Pull Request #8864 · cilium/cilium

joestringer · 2019-08-09T22:28:48Z

Fix an issue where packets after the first inbound packet for a local pod with ingress proxy are dropped in the Linux forwarding logic:

In direct routing mode, when multiple nodes are involved in an HTTP
request and there is an ingress L7 policy applied at the destination, the
ACK for a TCP handshake inbound would previously be dropped early
in the forwarding stack, after the iptables PREROUTING tables:

CILIUM_PRE_raw:     IN=eth1 OUT= ... SYN
CILIUM_PRE_mangle:  IN=eth1 OUT= ... SYN
CILIUM_PRE_nat:     IN=eth1 OUT= ... SYN
CILIUM_FORWARD:     IN=eth1 OUT=lxc61b3fb1d7642 ... SYN
CILIUM_POST_mangle: IN= OUT=lxc61b3fb1d7642 ... SYN
CILIUM_POST_nat:    IN= OUT=lxc61b3fb1d7642 ... SYN
CILIUM_PRE_raw:     IN=cilium_host OUT= ... SYN
CILIUM_PRE_mangle:  IN=cilium_host OUT= ... SYN
INPUT:              IN=cilium_host OUT= ... SYN
CILIUM_OUTPUT_raw:  IN= OUT=cilium_host ... SYN ACK
OUTPUT:             IN= OUT=cilium_host ... SYN ACK
CILIUM_POST_mangle: IN= OUT=cilium_host ... SYN ACK
CILIUM_PRE_raw:     IN=cilium_net OUT= ... SYN ACK
CILIUM_PRE_mangle:  IN=cilium_net OUT= ... SYN ACK
CILIUM_FORWARD:     IN=cilium_net OUT=eth1 ... SYN ACK
CILIUM_POST_mangle: IN= OUT=eth1 ... SYN ACK
CILIUM_PRE_raw:     IN=eth1 OUT= ... ACK
CILIUM_PRE_mangle:  IN=eth1 OUT= ... ACK

(No subsequent evidence of iptables hits for the ACK)

Further investigation by Martynas and Daniel revealed that the traffic
was being dropped in the Linux kernel ip_forward() function, in the case
where the socket is assigned to the skb. If one disables early demux
(`sysctl -w net.ipv4.ip_early_demux=0`), then this behaviour is not
observed. Previous discussion which introduced this check can be seen on
the kernel lists [0], [1]. This drop case was introduced to highlight
cases where policy routing is not being properly applied, to drop
traffic early rather than potentially misrouting it within the stack.

One mitigation that was observed is that if the commit 0f7617569171
("Revert "cilium: fix up source address selection for cluster ip"") is
reverted, then SNAT will be applied to traffic that is being forwarded
towards the proxy after initial BPF processing in context of the
destination endpoint (during the CILIUM_POST_nat in the list above). As
a result, the tuple associated with the socket accepted by the proxy
would be distinct from the tuple for any incoming traffic from the eth1
device. Unfortunately, that rule relies upon a destination pod prefix
which cannot be relied upon in some datapath modes such as with
--enable-endpoint-routes (eg, with ENI integration).

Jarno noted that the upstream kernel TPROXY documentation[2] also
contains an iptables rule in the PREROUTING table which diverts traffic
that matches a local socket to a separate chain where a mark is applied
for policy routing, and the traffic is then accepted. We could
previously get away without having such a rule in most circumstances
because typically Cilium in tunnel mode handles all traffic within BPF
programs, tail-calling betweeen programs and with minimal involvement of
the rest of the stack. In other words, the early demux was being skipped
if traffic arrived to the node on a tunnel and hence the ACK would not
be dropped in a similar case in tunnel mode; we could forward through
BPF programs then apply the mark for policy routing in BPF, then pass
traffic to the stack which would then be forwarded correctly. However
for direct routing mode, we rely upon the stack's routing to forward
traffic so in this case the socket match rule is not optional.

Regarding the cost of this new rule, we realise that since the traffic
will be subject to early demux socket lookup anyway at this point in the
stack, there should be minimal additional cost associated with the new
socket lookup in the rule, since the stack would look up the socket
anyway.

[0] https://patchwork.ozlabs.org/patch/459585/
[1] https://patchwork.ozlabs.org/patch/460836/
[2] https://www.kernel.org/doc/Documentation/networking/tproxy.txt

Related: #8813

Follow-up: #8930

This change is

joestringer · 2019-08-09T22:29:36Z

test-me-please

coveralls · 2019-08-09T22:56:37Z

Coverage decreased (-0.009%) to 44.151% when pulling b833876 on joestringer:submit/l7-proxy-ingress-endpoint-routes into 9c36f26 on cilium:master.

joestringer · 2019-08-13T18:43:42Z

test-me-please

EDIT: The build never kicked off on Jenkins. Will retry.

joestringer · 2019-08-13T21:38:23Z

test-me-please

EDIT: Jenkins ignored me again. Retrying again.

joestringer · 2019-08-13T22:13:45Z

test-me-please

EDIT: Ran out of space on jenkins node:

https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-Validated/14302/execution/node/7/log/

joestringer · 2019-08-13T23:22:07Z

test-me-please

EDIT: Same problem, same node jenkins-fixed-new-0.

joestringer · 2019-08-13T23:43:58Z

test-me-please

EDIT: The only failure is in the mode that this PR is trying to fix..

https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-Validated/14308/execution/node/151/log/?consoleFull

18:06:38  Summarizing 1 Failure:
18:06:38  
18:06:38  [Fail] K8sDatapathConfig PerEndpointRoute [It] Check connectivity with per endpoint routes 
18:06:38  /home/jenkins/workspace/Cilium-PR-Ginkgo-Tests-Validated/k8s-1.15-gopath/src/github.com/cilium/cilium/test/k8sT/assertionHelpers.go:121
18:06:38  
18:06:38  Ran 52 of 365 Specs in 2918.250 seconds
18:06:38  FAIL! -- 51 Passed | 1 Failed | 0 Pending | 313 Skipped
18:06:38  --- FAIL: TestTest (2918.25s)
18:06:38  FAIL
18:06:38  
18:06:38  Ginkgo ran 1 suite in 48m45.638190146s
18:06:38  Test Suite Failed

However, it's not in the actual test, but in the bootstrapping / health endpoints setup:

17:55:12  ===================== TEST FAILED =====================
17:55:13  Cilium is not ready yet: connectivity health is failing: Cluster connectivity is unhealthy on 'cilium-5cvwn': Exitcode: 255

I wonder if the per-endpoint routes are correctly configured for the health endpoints.

tgraf · 2019-08-14T17:41:41Z

test-me-please

joestringer · 2019-08-14T18:39:37Z

New patch to fix cilium-health connectivity.

joestringer · 2019-08-14T18:39:45Z

test-me-please

EDIT: Hit infra issue:

13:41:22  Cannot contact jenkins-node-3: java.lang.InterruptedException

joestringer · 2019-08-14T21:26:37Z

test-me-please

EDIT: Interrupted again..
https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-Validated/14363/execution/node/154/log/

Cannot contact jenkins-node-11: java.lang.InterruptedException

joestringer · 2019-08-16T04:18:07Z

Superseded by #8870. Unfortunately we lose the helpful git log here but functionally it should be the same as the version already in the tree.

Refactor this into one place to make it easier and tidier to wrap creation of these rules. For more detail on why they are necessary, see cilium#8864. Signed-off-by: Joe Stringer <joe@cilium.io>

Refactor this into one place to make it easier and tidier to wrap creation of these rules. For more detail on why they are necessary, see #8864. Signed-off-by: Joe Stringer <joe@cilium.io>

[ upstream commit 4c3f84c ] Refactor this into one place to make it easier and tidier to wrap creation of these rules. For more detail on why they are necessary, see #8864. Signed-off-by: Joe Stringer <joe@cilium.io> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

'--no-wildcard' allows the socket match to find zero-bound (listening) sockets, which we do not want, as this may intercept (reply) traffic intended for other nodes when an ephemeral source port number allocated in one node happens to be the same as the allocated proxy port number in 'this' node (the node doing the iptables socket match changed here). Fixes: #12241 Related: #8864 Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

[ upstream commit ca767ee ] '--no-wildcard' allows the socket match to find zero-bound (listening) sockets, which we do not want, as this may intercept (reply) traffic intended for other nodes when an ephemeral source port number allocated in one node happens to be the same as the allocated proxy port number in 'this' node (the node doing the iptables socket match changed here). Fixes: #12241 Related: #8864 Signed-off-by: Jarno Rajahalme <jarno@covalent.io> Signed-off-by: Chris Tarazi <chris@isovalent.com>

[ upstream commit ca767ee ] '--no-wildcard' allows the socket match to find zero-bound (listening) sockets, which we do not want, as this may intercept (reply) traffic intended for other nodes when an ephemeral source port number allocated in one node happens to be the same as the allocated proxy port number in 'this' node (the node doing the iptables socket match changed here). Fixes: #12241 Related: #8864 Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

joestringer added pending-review area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. labels Aug 9, 2019

joestringer requested review from a team and jrajahalme August 9, 2019 22:28

joestringer mentioned this pull request Aug 9, 2019

Fix DNS egress L7 policy in ENI mode #8837

Merged

joestringer added wip and removed pending-review labels Aug 9, 2019

joestringer commented Aug 9, 2019

View reviewed changes

Comment thread bpf/init.sh Outdated

joestringer force-pushed the submit/l7-proxy-ingress-endpoint-routes branch from d1924c5 to fd3fb65 Compare August 13, 2019 18:39

joestringer changed the title ~~Fix local pod-pod ingress L7 proxying with --enable-endpoint-routes mode~~ Fix L7 ingress proxy in direct routing mode Aug 13, 2019

joestringer added pending-review and removed wip labels Aug 13, 2019

joestringer added the needs-backport/1.6 label Aug 13, 2019

joestringer commented Aug 13, 2019

View reviewed changes

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

joestringer mentioned this pull request Aug 13, 2019

bpf: Support proxy upstream connections with original source address/port #8870

Merged

joestringer force-pushed the submit/l7-proxy-ingress-endpoint-routes branch from fd3fb65 to 0ae4ee9 Compare August 14, 2019 18:39

joestringer requested a review from a team August 14, 2019 18:39

tgraf approved these changes Aug 14, 2019

View reviewed changes

joestringer closed this Aug 16, 2019

ianvernon removed the needs-backport/1.6 label Aug 16, 2019

jrajahalme mentioned this pull request Jun 23, 2020

iptables: Remove '--nowildcard' from socket match #12248

Merged

Conversation

joestringer commented Aug 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joestringer commented Aug 9, 2019

Uh oh!

coveralls commented Aug 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

joestringer commented Aug 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joestringer commented Aug 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joestringer commented Aug 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joestringer commented Aug 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joestringer commented Aug 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

tgraf commented Aug 14, 2019

Uh oh!

joestringer commented Aug 14, 2019

Uh oh!

joestringer commented Aug 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joestringer commented Aug 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joestringer commented Aug 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

joestringer commented Aug 9, 2019 •

edited

Loading

coveralls commented Aug 9, 2019 •

edited

Loading

joestringer commented Aug 13, 2019 •

edited

Loading

joestringer commented Aug 13, 2019 •

edited

Loading

joestringer commented Aug 13, 2019 •

edited

Loading

joestringer commented Aug 13, 2019 •

edited

Loading

joestringer commented Aug 13, 2019 •

edited

Loading

joestringer commented Aug 14, 2019 •

edited

Loading

joestringer commented Aug 14, 2019 •

edited

Loading

joestringer commented Aug 16, 2019 •

edited

Loading