Skip to content

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

Merged
joestringer merged 1 commit intomasterfrom
pr/jrajahalme/iptables-track-for-nodeport
Jun 5, 2020
Merged

datapath: Only NOTRACK proxy return traffic going to Cilium datapath#11899
joestringer merged 1 commit intomasterfrom
pr/jrajahalme/iptables-track-for-nodeport

Conversation

@jrajahalme
Copy link
Copy Markdown
Member

@jrajahalme jrajahalme commented Jun 4, 2020 •
edited
Loading

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

@jrajahalme
datapath: Only NOTRACK proxy return traffic going to Cilium datapath
9b1e4dd 
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 area/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
@jrajahalme jrajahalme marked this pull request as draft June 4, 2020 19:08
@jrajahalme
Copy link
Copy Markdown
Member Author

jrajahalme commented Jun 4, 2020

test-me-please

@jrajahalme
Copy link
Copy Markdown
Member Author

jrajahalme commented Jun 4, 2020

test-gke

@jrajahalme
Copy link
Copy Markdown
Member Author

jrajahalme commented Jun 4, 2020

This fixed nodeport with L7 on GKE on manual testing.

@coveralls
Copy link
Copy Markdown

coveralls commented Jun 4, 2020

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
Copy Markdown
Member Author

jrajahalme commented Jun 4, 2020

1.17 flake #10231

@jrajahalme
Copy link
Copy Markdown
Member Author

jrajahalme commented Jun 5, 2020 •
edited
Loading

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
Copy Markdown
Member Author

jrajahalme commented Jun 5, 2020

retest-4.19

@jrajahalme
Copy link
Copy Markdown
Member Author

jrajahalme commented Jun 5, 2020

retest-gke

joestringer
joestringer reviewed Jun 5, 2020
View reviewed changes
Comment thread pkg/datapath/iptables/iptables.go
@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
Copy Markdown
Member

joestringer commented Jun 5, 2020 •
edited
Loading

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.

joestringer
joestringer requested changes Jun 5, 2020
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member

joestringer commented Jun 5, 2020

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
@joestringer joestringer deleted the pr/jrajahalme/iptables-track-for-nodeport branch June 5, 2020 18:56
@christarazi christarazi mentioned this pull request Jun 5, 2020
Merged
@joestringer joestringer mentioned this pull request Jun 8, 2020
Merged
@jrajahalme jrajahalme mentioned this pull request Jun 24, 2020
Closed
@brb brb mentioned this pull request Jul 8, 2020
Merged
brb added a commit that referenced this pull request Jul 8, 2020
@brb
test: Test NodePort BPF with L7 proxy
c7ef841 
#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
@brb @nebril
test: Test NodePort BPF with L7 proxy
9cbe915 
#11899 made it possible to run
NodePort BPF with L7 policies.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@jrajahalme jrajahalme mentioned this pull request Jul 8, 2020
Closed
@joestringer joestringer mentioned this pull request Aug 3, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joestringer joestringer joestringer approved these changes

+1 more reviewer

@tgraf tgraf tgraf approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/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. 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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

NodePort BPF service cannot be reached when L7 policy is applied / L7 visibility is enabled CI: K8sServiceTest Tests NodePort BPF

5 participants

@jrajahalme @coveralls @joestringer @tgraf @christarazi