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

datapath: Only NOTRACK proxy return traffic going to Cilium datapath #11899

Merged
merged 1 commit into from Jun 5, 2020

Conversation

jrajahalme
Copy link
Member

@jrajahalme jrajahalme commented Jun 4, 2020

Proxy return traffic accessed via a k8s NodePort will not be routed
back via Cilium bpf datapath, so such traffic needs to have possible
reverse NAT applied. Setting NOTRACK prevented this. Fix this by
setting NOTRACK only on packets heading back to the Cilium datapath
(-o lxc+ and -o cilium_host).

Fixes: #8971
Fixes: #8945
Signed-off-by: Jarno Rajahalme jarno@covalent.io

Proxy return traffic accessed via a k8s NodePort will not be routed
back via Cilium bpf datapath, so such traffic needs to have possible
reverse NAT applied. Setting NOTRACK prevented this. Fix this by
setting NOTRACK only on packets heading back to the Cilium datapath
(-o lxc+ and -o cilium_host).

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
@jrajahalme jrajahalme added kind/bug This is a bug in the Cilium logic. priority/release-blocker sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. release-note/bug This PR fixes an issue in a previous release of Cilium. labels Jun 4, 2020
@jrajahalme jrajahalme requested a review from a team June 4, 2020 19:08
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Jun 4, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.7.5 Jun 4, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.8.0 Jun 4, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.6.9 Jun 4, 2020
@jrajahalme jrajahalme marked this pull request as draft June 4, 2020 19:08
@jrajahalme
Copy link
Member Author

test-me-please

@jrajahalme
Copy link
Member Author

test-gke

@jrajahalme
Copy link
Member Author

This fixed nodeport with L7 on GKE on manual testing.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 36.932% when pulling 9b1e4dd on pr/jrajahalme/iptables-track-for-nodeport into f5a7682 on master.

@jrajahalme jrajahalme marked this pull request as ready for review June 4, 2020 22:50
@jrajahalme
Copy link
Member Author

1.17 flake #10231

@jrajahalme
Copy link
Member Author

jrajahalme commented Jun 5, 2020

K8s test suite run locally against a private cluster passed on GKE with this PR:

Ran 63 of 396 Specs in 5813.792 seconds
SUCCESS! -- 63 Passed | 0 Failed | 2 Pending | 331 Skipped
PASS

@jrajahalme
Copy link
Member Author

retest-4.19

@jrajahalme
Copy link
Member Author

retest-gke

@maintainer-s-little-helper 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 Jun 5, 2020
@joestringer
Copy link
Member

joestringer commented Jun 5, 2020

Change makes sense, probably just worth considering whether to further gate one of the new rules per my feedback above.

I don't think it makes sense to backport to v1.6, we're not hearing reports of issues on that release with eg. EKS. This fixes a known issue with the GKE mode which is the same on v1.7 and v1.8 so those backports make sense to me.

@maintainer-s-little-helper maintainer-s-little-helper bot removed this from Needs backport from master in 1.6.9 Jun 5, 2020
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should agree on the previous comment before merging:

https://github.com/cilium/cilium/pull/11899/files#r436051065

@joestringer
Copy link
Member

Merging despite GKE failure since the GKE job seems entirely broken but Jarno had manually validated the fix in a GKE environment.

@joestringer joestringer merged commit 02b43fc into master Jun 5, 2020
1.8.0 automation moved this from In progress to Merged Jun 5, 2020
@joestringer joestringer deleted the pr/jrajahalme/iptables-track-for-nodeport branch June 5, 2020 18:56
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.0 Jun 5, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.8 to Backport done to v1.8 in 1.8.0 Jun 8, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.8 to Backport done to v1.8 in 1.8.0 Jun 8, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.7 in 1.7.5 Jun 8, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.7 in 1.7.5 Jun 8, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.7 to Backport done to v1.7 in 1.7.5 Jun 10, 2020
brb added a commit that referenced this pull request Jul 8, 2020
#11899 made it possible to run
NodePort BPF with L7 policies.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
nebril pushed a commit that referenced this pull request Jul 8, 2020
#11899 made it possible to run
NodePort BPF with L7 policies.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. kind/bug This is a bug in the Cilium logic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. 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.5
Backport done to v1.7
1.8.0
  
Merged
1.8.0
Backport done to v1.8
5 participants