Skip to content

datapath: Accept proxy traffic if enable-endpoint-routes are enabled#11819

Merged
tgraf merged 2 commits intomasterfrom
pr/tgraf/fix-endpoint-routes-forward-chain
Jun 3, 2020
Merged

datapath: Accept proxy traffic if enable-endpoint-routes are enabled#11819
tgraf merged 2 commits intomasterfrom
pr/tgraf/fix-endpoint-routes-forward-chain

Conversation

@tgraf
Copy link
Copy Markdown
Contributor

@tgraf tgraf commented Jun 2, 2020 •
edited
Loading

The forward chain rules have been depended on the local delivery
interface which depending on the setting of enable-endpoint-routes is
either cilium_host or lxc+. This is sufficient for all regular
traffic. For proxy redirection traffic, all traffic still passes through
cilium_host regardless of the value of enable-endpoint-routes.

Eample of existing rules:

 -A CILIUM_FORWARD -o lxc+ -m comment --comment "cilium: any->cluster on lxc+ forward accept" -j ACCEPT
 -A CILIUM_FORWARD -i lxc+ -m comment --comment "cilium: cluster->any on lxc+ forward accept (nodeport)" -j ACCEPT
 -A CILIUM_FORWARD -i lxc+ -m comment --comment "cilium: cluster->any on lxc+ forward accept" -j ACCEPT

This problem was masked because Kubernetes would install these
wide-reaching rules:

-A KUBE-FORWARD -m comment --comment "kubernetes forwarding conntrack pod source rule" -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT
-A KUBE-FORWARD -m comment --comment "kubernetes forwarding conntrack pod destination rule" -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT

However, more recent versions of Kubernetes would install more
fine-grained rules when the PodCIDR is known too the host:

 -A KUBE-FORWARD -s 10.10.0.0/16 -m comment --comment "kubernetes forwarding conntrack pod source rule" -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT

This will still mask the problem if Cilium uses the PodCIDR for IP
allocation. However, in case Cilium does not use the announced PodCIDR
then these rules would no longer allow the proxy redirection traffic
causes proxy redirection to break.

Fixes: #11235
Fixes: #11807

@tgraf tgraf added kind/bug This is a bug in the Cilium logic. priority/release-blocker release-note/bug This PR fixes an issue in a previous release of Cilium. ci/flake This is a known failure that occurs in the tree. Please investigate me! labels Jun 2, 2020
@tgraf tgraf requested a review from a team June 2, 2020 12:11
@tgraf tgraf requested a review from a team as a code owner June 2, 2020 12:15
@tgraf
Copy link
Copy Markdown
Contributor Author

tgraf commented Jun 2, 2020

test-me-please

@tgraf
Copy link
Copy Markdown
Contributor Author

tgraf commented Jun 2, 2020 •
edited
Loading

test-gke K8sServicesTest*

EDIT: GKE tests passed

@coveralls
Copy link
Copy Markdown

coveralls commented Jun 2, 2020 •
edited
Loading

Coverage Status

Coverage decreased (-0.02%) to 36.903% when pulling 0fe4db3 on pr/tgraf/fix-endpoint-routes-forward-chain into a20dcd1 on master.

@jrajahalme jrajahalme force-pushed the pr/tgraf/fix-endpoint-routes-forward-chain branch from 43168d1 to 86a2832 Compare June 2, 2020 14:17
@jrajahalme
Copy link
Copy Markdown
Member

jrajahalme commented Jun 2, 2020

test-gke K8sServicesTest*

@jrajahalme
Copy link
Copy Markdown
Member

jrajahalme commented Jun 2, 2020

test-me-please

@jrajahalme
Copy link
Copy Markdown
Member

jrajahalme commented Jun 2, 2020

Testing this locally, still needed to manually add the last two rules to make nodeport pass.
Before:

vagrant@k8s2:~$ sudo iptables-save -c | grep -i forward
:FORWARD ACCEPT [5360:1318110]
:FORWARD DROP [7:420]
:CILIUM_FORWARD - [0:0]
:KUBE-FORWARD - [0:0]
[32:6449] -A FORWARD -m comment --comment "cilium-feeder: CILIUM_FORWARD" -j CILIUM_FORWARD
[7:420] -A FORWARD -m comment --comment "kubernetes forwarding rules" -j KUBE-FORWARD
[0:0] -A FORWARD -m conntrack --ctstate NEW -m comment --comment "kubernetes service portals" -j KUBE-SERVICES
[7:420] -A FORWARD -j DOCKER-USER
[7:420] -A FORWARD -j DOCKER-ISOLATION-STAGE-1
[0:0] -A FORWARD -o br-8ed96ba6747a -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT
[0:0] -A FORWARD -o br-8ed96ba6747a -j DOCKER
[0:0] -A FORWARD -i br-8ed96ba6747a ! -o br-8ed96ba6747a -j ACCEPT
[0:0] -A FORWARD -i br-8ed96ba6747a -o br-8ed96ba6747a -j ACCEPT
[0:0] -A FORWARD -o docker0 -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT
[0:0] -A FORWARD -o docker0 -j DOCKER
[0:0] -A FORWARD -i docker0 ! -o docker0 -j ACCEPT
[0:0] -A FORWARD -i docker0 -o docker0 -j ACCEPT
[14:5422] -A CILIUM_FORWARD -o cilium_host -m comment --comment "cilium: any->cluster on cilium_host forward accept" -j ACCEPT
[0:0] -A CILIUM_FORWARD -i cilium_host -m comment --comment "cilium: cluster->any on cilium_host forward accept (nodeport)" -j ACCEPT
[11:607] -A CILIUM_FORWARD -i lxc+ -m comment --comment "cilium: cluster->any on lxc+ forward accept" -j ACCEPT
[0:0] -A KUBE-FORWARD -m conntrack --ctstate INVALID -j DROP
[0:0] -A KUBE-FORWARD -m comment --comment "kubernetes forwarding rules" -m mark --mark 0x4000/0x4000 -j ACCEPT
[0:0] -A KUBE-FORWARD -s 10.10.0.0/16 -m comment --comment "kubernetes forwarding conntrack pod source rule" -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT
[0:0] -A KUBE-FORWARD -d 10.10.0.0/16 -m comment --comment "kubernetes forwarding conntrack pod destination rule" -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT

After:

vagrant@k8s2:~$ sudo iptables-save -c | grep -i forward
:FORWARD ACCEPT [5704:1398144]
:FORWARD DROP [0:0]
:CILIUM_FORWARD - [0:0]
:KUBE-FORWARD - [0:0]
[34:6576] -A FORWARD -m comment --comment "cilium-feeder: CILIUM_FORWARD" -j CILIUM_FORWARD
[4:958] -A FORWARD -m comment --comment "kubernetes forwarding rules" -j KUBE-FORWARD
[0:0] -A FORWARD -m conntrack --ctstate NEW -m comment --comment "kubernetes service portals" -j KUBE-SERVICES
[0:0] -A FORWARD -j DOCKER-USER
[0:0] -A FORWARD -j DOCKER-ISOLATION-STAGE-1
[0:0] -A FORWARD -o br-8ed96ba6747a -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT
[0:0] -A FORWARD -o br-8ed96ba6747a -j DOCKER
[0:0] -A FORWARD -i br-8ed96ba6747a ! -o br-8ed96ba6747a -j ACCEPT
[0:0] -A FORWARD -i br-8ed96ba6747a -o br-8ed96ba6747a -j ACCEPT
[0:0] -A FORWARD -o docker0 -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT
[0:0] -A FORWARD -o docker0 -j DOCKER
[0:0] -A FORWARD -i docker0 ! -o docker0 -j ACCEPT
[0:0] -A FORWARD -i docker0 -o docker0 -j ACCEPT
[18:4861] -A CILIUM_FORWARD -o cilium_host -m comment --comment "cilium: any->cluster on cilium_host forward accept" -j ACCEPT
[0:0] -A CILIUM_FORWARD -i cilium_host -m comment --comment "cilium: cluster->any on cilium_host forward accept (nodeport)" -j ACCEPT
[12:757] -A CILIUM_FORWARD -i lxc+ -m comment --comment "cilium: cluster->any on lxc+ forward accept" -j ACCEPT
[0:0] -A KUBE-FORWARD -m conntrack --ctstate INVALID -j DROP
[0:0] -A KUBE-FORWARD -m comment --comment "kubernetes forwarding rules" -m mark --mark 0x4000/0x4000 -j ACCEPT
[0:0] -A KUBE-FORWARD -s 10.10.0.0/16 -m comment --comment "kubernetes forwarding conntrack pod source rule" -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT
[0:0] -A KUBE-FORWARD -d 10.10.0.0/16 -m comment --comment "kubernetes forwarding conntrack pod destination rule" -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT
[4:958] -A KUBE-FORWARD -s 10.0.0.0/16 -m comment --comment "kubernetes forwarding conntrack pod source rule" -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT
[0:0] -A KUBE-FORWARD -d 10.0.0.0/16 -m comment --comment "kubernetes forwarding conntrack pod destination rule" -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT

It looks like the new rules were not added, i.e., ifName and localDeliveryInterface are the same (== cilium_host).

Logging the traffic that hits the added rule:

[ 3060.775993] KUBE-FORWARD: IN=cilium_net OUT=enp0s8 MAC=fa:0f:1a:59:d3:a0:fe:74:4d:d2:c0:06:08:00 SRC=10.0.1.135 DST=192.168.36.11 LEN=60 TOS=0x00 PREC=0x00 TTL=63 ID=0 DF PROTO=TCP SPT=80 DPT=36004 WINDOW=28960 RES=0x00 ACK SYN URGP=0 
[ 3060.777088] KUBE-FORWARD: IN=cilium_net OUT=enp0s8 MAC=fa:0f:1a:59:d3:a0:fe:74:4d:d2:c0:06:08:00 SRC=10.0.1.135 DST=192.168.36.11 LEN=52 TOS=0x00 PREC=0x00 TTL=63 ID=917 DF PROTO=TCP SPT=80 DPT=36004 WINDOW=227 RES=0x00 ACK URGP=0 
[ 3060.778450] KUBE-FORWARD: IN=cilium_net OUT=enp0s8 MAC=fa:0f:1a:59:d3:a0:fe:74:4d:d2:c0:06:08:00 SRC=10.0.1.135 DST=192.168.36.11 LEN=794 TOS=0x00 PREC=0x00 TTL=63 ID=918 DF PROTO=TCP SPT=80 DPT=36004 WINDOW=227 RES=0x00 ACK PSH URGP=0 
[ 3060.780645] KUBE-FORWARD: IN=cilium_net OUT=enp0s8 MAC=fa:0f:1a:59:d3:a0:fe:74:4d:d2:c0:06:08:00 SRC=10.0.1.135 DST=192.168.36.11 LEN=52 TOS=0x00 PREC=0x00 TTL=63 ID=919 DF PROTO=TCP SPT=80 DPT=36004 WINDOW=227 RES=0x00 ACK FIN URGP=0

Note the IN=cilium_net.

@tgraf tgraf marked this pull request as draft June 2, 2020 16:58
@tgraf @jrajahalme
datapath: Accept proxy traffic
467429b 
The forward chain rules have been depended on the local delivery
interface which depending on the setting of enable-endpoint-routes is
either `cilium_host` or `lxc+`. This is sufficient for all regular
traffic. For proxy redirection traffic, all traffic still passes through
cilium_host regardless of the value of enable-endpoint-routes.

Eample of existing rules:
```
 -A CILIUM_FORWARD -o lxc+ -m comment --comment "cilium: any->cluster on lxc+ forward accept" -j ACCEPT
 -A CILIUM_FORWARD -i lxc+ -m comment --comment "cilium: cluster->any on lxc+ forward accept (nodeport)" -j ACCEPT
 -A CILIUM_FORWARD -i lxc+ -m comment --comment "cilium: cluster->any on lxc+ forward accept" -j ACCEPT
```

This problem was masked because Kubernetes would install these
wide-reaching rules:
```
-A KUBE-FORWARD -m comment --comment "kubernetes forwarding conntrack pod source rule" -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT
-A KUBE-FORWARD -m comment --comment "kubernetes forwarding conntrack pod destination rule" -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT
```

However, more recent versions of Kubernetes would install more
fine-grained rules when the PodCIDR is known too the host:
```
 -A KUBE-FORWARD -s 10.10.0.0/16 -m comment --comment "kubernetes forwarding conntrack pod source rule" -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT
```

This will still mask the problem if Cilium uses the PodCIDR for IP
allocation. However, in case Cilium does not use the announced PodCIDR
then these rules would no longer allow the proxy redirection traffic
causes proxy redirection to break.

Fixes: #11235
Fixes: #11807

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
Signed-off-by: Thomas Graf <thomas@cilium.io>
@jrajahalme jrajahalme force-pushed the pr/tgraf/fix-endpoint-routes-forward-chain branch from 86a2832 to 3f787af Compare June 2, 2020 18:14
@tgraf @jrajahalme
test: Re-enable K8sServicesTest Checks service across nodes with L7 o…
0fe4db3 
…n GKE

Signed-off-by: Thomas Graf <thomas@cilium.io>
Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
@jrajahalme jrajahalme force-pushed the pr/tgraf/fix-endpoint-routes-forward-chain branch from 3f787af to 0fe4db3 Compare June 2, 2020 18:26
@jrajahalme
Copy link
Copy Markdown
Member

jrajahalme commented Jun 2, 2020

test-me-please

@jrajahalme
Copy link
Copy Markdown
Member

jrajahalme commented Jun 2, 2020

test-gke K8sServicesTest*

@jrajahalme
Copy link
Copy Markdown
Member

jrajahalme commented Jun 2, 2020

netnext hit by #11741

@jrajahalme
Copy link
Copy Markdown
Member

jrajahalme commented Jun 2, 2020

retest-net-next

@jrajahalme
Copy link
Copy Markdown
Member

jrajahalme commented Jun 2, 2020

GKE tests failed to start at all:

13:10:10      Kubernetes DNS did not become ready in time

Logs indicate that cilium-agent did not became ready, which would cause the DNS not to become ready either. Quick look on the cilium-agent logs did not reveal why it is not ready, though.

@michi-covalent
Copy link
Copy Markdown
Contributor

michi-covalent commented Jun 2, 2020

@jrajahalme it could be because of #11801

@jrajahalme jrajahalme marked this pull request as ready for review June 2, 2020 23:31
@tgraf tgraf merged commit 7e0469d into master Jun 3, 2020
@tgraf tgraf deleted the pr/tgraf/fix-endpoint-routes-forward-chain branch June 3, 2020 07:44
@tklauser tklauser mentioned this pull request Jun 3, 2020
Merged
@tklauser tklauser mentioned this pull request Jun 3, 2020
Merged
@tgraf tgraf mentioned this pull request Jun 3, 2020
Closed
This was referenced Jun 3, 2020
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aanm aanm aanm approved these changes

Assignees

No one assigned

Labels

ci/flake This is a known failure that occurs in the tree. Please investigate me! kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium.

Projects

None yet

Milestone

No milestone

7 participants

@tgraf @coveralls @jrajahalme @michi-covalent @aanm @tklauser @joestringer