Skip to content

ctmap,daemon: Exclude host IPs from conntrack GC during startup #19998

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

Merged
merged 1 commit into from
Jun 7, 2022

Conversation

pchaigno
Copy link
Member

@pchaigno pchaigno commented May 29, 2022

When the host firewall is enabled, we start tracking (and potentially enforcing policies) on all connections to and from the host IP addresses. Thus, we also need to avoid GCing the host IPs. If we don't it can cause established connections to be broken on agent restart.

Fixes: #19367.

Fix bug where established host connections would be interrupted on agent restart if the host firewall was enabled.

@pchaigno pchaigno added release-note/bug This PR fixes an issue in a previous release of Cilium. area/host-firewall Impacts the host firewall or the host endpoint. needs-backport/1.11 labels May 29, 2022
@pchaigno pchaigno force-pushed the exclude-host-firewall-ct-gc branch from 66583b7 to 721ef47 Compare May 30, 2022 09:48
@zuzzas
Copy link
Contributor

zuzzas commented May 30, 2022

@pchaigno
No CT entry drop observed. No connection terminated. Works great!

@pchaigno pchaigno marked this pull request as ready for review May 30, 2022 10:30
@pchaigno pchaigno requested review from a team, YutaroHayakawa and ldelossa May 30, 2022 10:30
Copy link
Member

@YutaroHayakawa YutaroHayakawa left a comment

Choose a reason for hiding this comment

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

I found some typos, but otherwise, looks good to me.

@pchaigno pchaigno force-pushed the exclude-host-firewall-ct-gc branch from 721ef47 to 1841658 Compare May 31, 2022 07:01
@@ -30,7 +31,8 @@ type EndpointManager interface {
}

// Enable enables the connection tracking garbage collection.
func Enable(ipv4, ipv6 bool, restoredEndpoints []*endpoint.Endpoint, mgr EndpointManager) {
func Enable(ipv4, ipv6 bool, restoredEndpoints []*endpoint.Endpoint, mgr EndpointManager,
nodeAddressing types.NodeAddressing) {
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit of a nit but it would be nice to comment or document which of these arguments are used for GC filtering.

When the host firewall is enabled, we start tracking (and potentially
enforcing policies) on all connections to and from the host IP addresses.
Thus, we also need to avoid GCing the host IPs. If we don't it can cause
established connections to be broken on agent restart.

Reported-by: Andrey Klimentyev <andrey.klimentyev@flant.com>
Reported-by: Jarno Rajahalme <jarno@isovalent.com>
Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno pchaigno force-pushed the exclude-host-firewall-ct-gc branch from 1841658 to 722cf3e Compare June 5, 2022 18:02
@pchaigno
Copy link
Member Author

pchaigno commented Jun 5, 2022

/test

@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 Jun 5, 2022
@joamaki joamaki merged commit 1873163 into cilium:master Jun 7, 2022
@joestringer
Copy link
Member

🤔 I'm confused, how do we garbage collect host IPs now? Won't this just lead to the CT table being full of expired entries corresponding to host connections?

@pchaigno pchaigno deleted the exclude-host-firewall-ct-gc branch June 9, 2022 17:08
@pchaigno
Copy link
Member Author

pchaigno commented Jun 9, 2022

@joestringer We only skip host IPs for the initial scan (initialScan); then we'll GC them as usual. Same as for restored endpoints.

@joestringer joestringer changed the title ctmap,daemon: Exclude host IPs from conntrack GC ctmap,daemon: Exclude host IPs from conntrack GC during startup Jun 9, 2022
@joestringer joestringer added the backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. label Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/host-firewall Impacts the host firewall or the host endpoint. backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. 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
None yet
Development

Successfully merging this pull request may close these issues.

Conntrack table reset on agent restart
6 participants