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

ctmap: consider CT entry's .dsr flag in PurgeOrphanNATEntries() #29098

Merged
merged 1 commit into from
Nov 28, 2023
Merged

ctmap: consider CT entry's .dsr flag in PurgeOrphanNATEntries() #29098

merged 1 commit into from
Nov 28, 2023

Conversation

julianwiedmann
Copy link
Member

@julianwiedmann julianwiedmann commented Nov 10, 2023 •
edited 

The BPF datapath potentially re-creates a CT entry, in particular when a
DSR connection gets re-opened as local connection. Such a re-purposed CT
entry then leaves a DSR NAT entry behind.

Currently we wouldn't clean up such NAT entries (as the matching CT entry
still exists). But once we look at the CT entry's .dsr flag, we understand
that the CT entry is actually no longer a match for the NAT entry.

@julianwiedmann julianwiedmann added kind/bug This is a bug in the Cilium logic. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/bug This PR fixes an issue in a previous release of Cilium. feature/conntrack area/loadbalancing Impacts load-balancing and Kubernetes service implementations labels Nov 10, 2023
@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann julianwiedmann force-pushed the 1.15-ct-gc-dsr branch 2 times, most recently from 191d479 to 9bc424c Compare November 20, 2023 11:38
@julianwiedmann
ctmap: consider CT entry's .dsr flag in PurgeOrphanNATEntries()
fe6a305 
The BPF datapath potentially re-creates a CT entry, in particular when a
DSR connection gets re-opened as local connection. Such a re-purposed CT
entry then leaves a DSR NAT entry behind.

Currently we wouldn't clean up such NAT entries (as the matching CT entry
still exists). But once we look at the CT entry's .dsr flag, we understand
that the CT entry is actually no longer a match for the NAT entry.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann julianwiedmann marked this pull request as ready for review November 20, 2023 14:12
@julianwiedmann julianwiedmann requested a review from a team as a code owner November 20, 2023 14:12
@julianwiedmann julianwiedmann added needs-backport/1.12 backport/author The backport will be carried out by the author of the PR. 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 labels Nov 20, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.12.17 Nov 20, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.5 Nov 20, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.13.10 Nov 20, 2023
@julianwiedmann
Copy link
Member Author

julianwiedmann commented Nov 20, 2023 •
edited

#27607 might be good for context here. Note that we were re-creating CT entries even before that PR, it was just more hacky 🙂 .

@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 Nov 27, 2023
@julianwiedmann julianwiedmann added this pull request to the merge queue Nov 28, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 28, 2023
@julianwiedmann julianwiedmann added this pull request to the merge queue Nov 28, 2023
Merged via the queue into cilium:main with commit dfbae95 Nov 28, 2023
61 checks passed
@julianwiedmann julianwiedmann removed needs-backport/1.12 backport/author The backport will be carried out by the author of the PR. labels Nov 29, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed this from Needs backport from main in 1.12.17 Nov 29, 2023
@julianwiedmann
Copy link
Member Author

The 1.13 / 1.14 backports apply cleanly, so let's get those in. I'll need to stare at the 1.12 backport a bit more, but it's on my radar.

@nbusseneau nbusseneau mentioned this pull request Dec 5, 2023
Merged
5 tasks
@nbusseneau nbusseneau 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 labels Dec 5, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.13 in 1.13.10 Dec 5, 2023
@nbusseneau nbusseneau mentioned this pull request Dec 5, 2023
Merged
10 tasks
@nbusseneau nbusseneau 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 Dec 5, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.14 in 1.14.5 Dec 5, 2023
@github-actions github-actions bot added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Dec 6, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.14 to Backport done to v1.14 in 1.14.5 Dec 6, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed this from Backport done to v1.14 in 1.14.5 Dec 6, 2023
@github-actions github-actions bot added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Dec 6, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Backport done to v1.14 in 1.14.5 Dec 6, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.13 to Backport done to v1.13 in 1.13.10 Dec 6, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed this from Backport done to v1.13 in 1.13.10 Dec 6, 2023
@julianwiedmann julianwiedmann mentioned this pull request Dec 7, 2023
Merged
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Backport pending to v1.12 in 1.12.17 Dec 7, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Backport done to v1.13 in 1.13.10 Dec 7, 2023
@julianwiedmann julianwiedmann added backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. and removed backport-pending/1.12 labels Dec 11, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.12 to Backport done to v1.12 in 1.12.17 Dec 11, 2023
This was referenced Dec 11, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markpash markpash markpash approved these changes

Assignees
No one assigned
Labels
area/loadbalancing Impacts load-balancing and Kubernetes service implementations backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. 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. feature/conntrack 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.12.17
Backport done to v1.12
1.13.10
Backport done to v1.13
1.14.5
Backport done to v1.14
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@julianwiedmann @markpash @nbusseneau