Skip to content

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

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
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
Loading 

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. area/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. labels Nov 20, 2023
@julianwiedmann
Copy link
Member Author

julianwiedmann commented Nov 20, 2023 •
edited
Loading

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

markpash
markpash reviewed Nov 27, 2023
View reviewed changes
@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
@julianwiedmann julianwiedmann deleted the 1.15-ct-gc-dsr branch November 28, 2023 07:08
@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
@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 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 labels 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. backport-done/1.13 The backport for Cilium 1.13.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. backport-pending/1.13 labels Dec 6, 2023
@julianwiedmann julianwiedmann mentioned this pull request Dec 7, 2023
Merged
@julianwiedmann julianwiedmann added backport-pending/1.12 backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. and removed backport-pending/1.12 labels Dec 7, 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
1 more reviewer

@markpash markpash markpash 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/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.
Projects
No open projects
cilium v1.13.10
Status: Released
cilium v1.14.5
Status: Released
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@julianwiedmann @markpash @nbusseneau