-
Notifications
You must be signed in to change notification settings - Fork 124
firewall: flush stale UDP conntrack entries on port_forward setup/teardown #1362
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
The unit tests seem to be failing because conntrack-tools is not installed in CI/CD |
f61d2f3 to
0731a48
Compare
0731a48 to
82cb21c
Compare
|
Ephemeral COPR build failed. @containers/packit-build please check. |
2 similar comments
|
Ephemeral COPR build failed. @containers/packit-build please check. |
|
Ephemeral COPR build failed. @containers/packit-build please check. |
|
I moved the conntrack entries flushing code to the end of all setup_port_forward and teardown_port_forward functions, instead of keeping it at the start, to avoid a potential race condition where a new conntrack entry is created after the flush but before the port forwarding rules are completely set up. |
82cb21c to
86d1881
Compare
Yes we can do that. I make that happen. |
Needed for containers/netavark#1362 Signed-off-by: Paul Holzinger <[email protected]>
425517c to
107bf6b
Compare
|
#1363 this should include conntrack now (once that merges you can rebase) |
|
#1363 was merged so CI should have access to conntrack I hope. |
107bf6b to
e6104d8
Compare
|
|
b07535a to
bf8e6dc
Compare
|
I have an error message nit, but on the whole LGTM |
| rand = "0.9.2" | ||
| sha2 = "0.10.9" | ||
| netlink-packet-route = "0.25.1" | ||
| netlink-packet-netfilter = { git = "https://github.com/shivkr6/netlink-packet-netfilter.git", branch = "conntrack-new" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just so it is not overlooked, we cannot merge this while the upstream lib isn't merged so I mark as changes requested
|
Forgot to say overall great work! |
bf8e6dc to
bcb10cb
Compare
Thanks! The appreciation from you and Matt really keeps my motivation and confidence high |
|
It's extremely weird that the |
bcb10cb to
343b730
Compare
…rdown Add a new netlink_netfilter module to interact with the kernel's conntrack table using the netlink_packet_netfilter crate. This module allows dumping and deleting conntrack entries. All firewall drivers now call the new flush_udp_conntrack() function during port forwarding setup and teardown. When a container with a UDP port mapping is started, stale conntrack entries can prevent traffic from reaching the new container instance. This change proactively deletes these stale entries for the mapped UDP ports, ensuring that new connections are not dropped by the kernel. Added an integration test for the same and unit tests for dump_conntrack and del_conntrack. Fixes: containers#1045 Signed-off-by: Shivang K Raghuvanshi <[email protected]>
343b730 to
49b6f1d
Compare
|
I skipped the |
|
Tests green, just waiting on that upstream library now. Nice work @shivkr6 |
|
Thanks @mheon. The bigger PR, rust-netlink/netlink-packet-netfilter#12, got merged. The remaining smaller PRs yet to be merged are rust-netlink/netlink-packet-netfilter#14 and rust-netlink/netlink-packet-netfilter#15 CC @Luap99 |

Add a new netlink_netfilter module to interact with the kernel's conntrack table using the netlink_packet_netfilter crate. This module allows dumping and deleting conntrack entries. All firewall drivers now call the new flush_udp_conntrack() function during port forwarding setup and teardown.
When a container with a UDP port mapping is started, stale conntrack entries can prevent traffic from reaching the new container instance. This change proactively deletes these stale entries for the mapped UDP ports, ensuring that new connections are not dropped by the kernel.
Fixes: #1045
NOTE: This PR cannot be merged right now because:
2) I have to write integration tests to test this functionality.[DONE]CC: @Luap99 @mheon