Skip to content

Conversation

jschwinger233
Copy link
Member

@jschwinger233 jschwinger233 commented Dec 4, 2023

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

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 jschwinger233 force-pushed the gray/tunnel-transparent-proxy branch from 9e022e4 to a2698af Compare December 12, 2023 04:31
@jschwinger233
Copy link
Member Author

/test

@jschwinger233 jschwinger233 force-pushed the gray/tunnel-transparent-proxy branch from a2698af to 2370f7d Compare December 13, 2023 11:10
@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 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 area/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
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 :)

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 jschwinger233 force-pushed the gray/tunnel-transparent-proxy branch from 2370f7d to aa84532 Compare December 13, 2023 16:44
@jschwinger233
Copy link
Member Author

/test

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 Requires further review before merging. needs-backport/1.15 labels Dec 13, 2023
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.

@julianwiedmann julianwiedmann removed the dont-merge/waiting-for-review Requires further review before merging. label Dec 14, 2023
@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
@julianwiedmann julianwiedmann mentioned this pull request Mar 5, 2024
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 labels Mar 5, 2024
@julianwiedmann julianwiedmann mentioned this pull request Mar 5, 2024
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 labels Mar 5, 2024
@julianwiedmann julianwiedmann mentioned this pull request Mar 5, 2024
7 tasks
@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 labels Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. 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.
Projects
No open projects
Status: Released
Status: Released
Status: Released
Development

Successfully merging this pull request may close these issues.

4 participants