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.

Sign up for GitHub

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

Jump to bottom

proxy: opt-out from SNAT for L7 + Tunnel for some scenarios #29594

Merged
merged 2 commits into from Dec 14, 2023
Merged

proxy: opt-out from SNAT for L7 + Tunnel for some scenarios #29594

merged 2 commits into from Dec 14, 2023

Conversation

jschwinger233
Copy link
Member

@jschwinger233 jschwinger233 commented Dec 4, 2023 •
edited

Currently the L7 proxy performs SNAT for traffic when tunnel routing is enabled, even for cluster-internal traffic. This prevents cilium_host from detecting pod-level traffic, and we thus can't apply features.

Modify SupportsOriginalSourceAddr(), so that the proxy doesn't SNAT such traffic when some conditions are met.

Fixes proxy issues by opting out from SNAT for L7 + Tunnel.

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Dec 4, 2023
@jschwinger233
Copy link
Member Author

/test

@jschwinger233
proxy: opt-out from SNAT for L7 + Tunnel for some scenarios
5692a36 
Currently the L7 proxy performs SNAT for traffic when tunnel routing is
enabled, even for cluster-internal traffic. This prevents cilium_host from
detecting pod-level traffic, and we thus can't apply features.

Modify SupportsOriginalSourceAddr(), so that the proxy doesn't SNAT such
traffic when some conditions are met.

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
@jschwinger233
Copy link
Member Author

/test

@jschwinger233 jschwinger233 added area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. backport/author The backport will be carried out by the author of the PR. kind/bug This is a bug in the Cilium logic. needs-backport/1.12 needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch release-note/bug This PR fixes an issue in a previous release of Cilium. and removed area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. labels Dec 13, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Dec 13, 2023
@jschwinger233 jschwinger233 added the sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. label Dec 13, 2023
@jschwinger233
Copy link
Member Author

/test

@julianwiedmann julianwiedmann marked this pull request as ready for review December 13, 2023 13:31
@julianwiedmann julianwiedmann requested a review from a team as a code owner December 13, 2023 13:31
julianwiedmann
julianwiedmann requested changes Dec 13, 2023
View reviewed changes
Copy link
Member

@julianwiedmann julianwiedmann left a comment

Choose a reason for hiding this comment

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

One cosmetic update below, otherwise looks good :)

pkg/datapath/iptables/iptables.go Outdated Show resolved Hide resolved
@jschwinger233
iptables: filter table accepts from-proxy packets
aa84532 
GKE has DROP policy for filter table, so we have to explicitly accept
proxy traffic.

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
@jschwinger233
Copy link
Member Author

/test

julianwiedmann
julianwiedmann approved these changes Dec 13, 2023
View reviewed changes
Copy link
Member

@julianwiedmann julianwiedmann left a comment

Choose a reason for hiding this comment

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

ty!

@julianwiedmann julianwiedmann added dont-merge/waiting-for-review needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch labels Dec 13, 2023
jrajahalme
jrajahalme approved these changes Dec 14, 2023
View reviewed changes
Copy link
Member

@jrajahalme jrajahalme left a comment

Choose a reason for hiding this comment

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

The comment above the change should be updated to describe why original source addressing needs to be used with IPsec.

@jrajahalme
Copy link
Member

Comment on the PR description/commit message: While it may look like it, there is no SNAT involved with the L7 proxy. The forwarded connection is merely a new one originating from the host networking namespace.

@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 Dec 14, 2023
@aanm aanm added this pull request to the merge queue Dec 14, 2023
Merged via the queue into cilium:main with commit 244a5e9 Dec 14, 2023
62 checks passed
@julianwiedmann julianwiedmann mentioned this pull request Mar 5, 2024
Merged
3 tasks
@julianwiedmann julianwiedmann added backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. and removed needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch labels Mar 5, 2024
@julianwiedmann julianwiedmann mentioned this pull request Mar 5, 2024
Merged
6 tasks
@julianwiedmann julianwiedmann added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Mar 5, 2024
@julianwiedmann julianwiedmann mentioned this pull request Mar 5, 2024
Merged
7 tasks
@julianwiedmann julianwiedmann added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch needs-backport/1.12 labels Mar 5, 2024
@github-actions github-actions bot added backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Mar 6, 2024
@jrajahalme jrajahalme mentioned this pull request Mar 13, 2024
Merged
This was referenced Mar 13, 2024
Closed
Merged
Merged
@jschwinger233 jschwinger233 mentioned this pull request Apr 16, 2024
Open
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrajahalme jrajahalme jrajahalme approved these changes

@julianwiedmann julianwiedmann julianwiedmann approved these changes

Assignees
No one assigned
Labels
area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. backport/author The backport will be carried out by the author of the PR. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. 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
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jschwinger233 @jrajahalme @julianwiedmann @aanm