Stop firewalld reload re-creating rules for deleted networks by robmry · Pull Request #49728 · moby/moby

robmry · 2025-04-01T19:38:33Z

- What I did

Improved restoration of iptables rules following a firewalld-reload ... only restore rules for current networks, and make sure networks aren't being updated while that happens.

- How I did it

- Restore iptables for current networks on firewalld reload

Using iptables.OnReloaded to restore individual per-network rules on firewalld reload means rules for deleted networks pop back in to existence (because there was no way to delete the callbacks on network-delete).

So, on firewalld reload, walk over current networks and ask them to restore their iptables rules.

- Report firewalld reload time in Info.FirewallBackend

To make it possible to write tests that wait for the reload to complete.

- Test that firewalld reload doesn't re-create deleted iptables rules

- How to verify it

Existing and new tests.

Checked that, without the fix, the new reload test fails.

Before the fix ...

# docker network create b4
e62823d3d7dcdc7e0c69d39af30175a2afd0a85c5dcbecac1450759bff9ce632
# docker run -d --network b4 -p 1234:1234 alpine
56ae89f8965dc9d83f7d7d565544d76b66059928fc072a13fd6ba1c8778e28c6
# docker network rm b4
b4

# iptables -S
-P INPUT ACCEPT
-P FORWARD ACCEPT
-P OUTPUT ACCEPT
-N DOCKER
-N DOCKER-BRIDGE
-N DOCKER-CT
-N DOCKER-FORWARD
-N DOCKER-ISOLATION-STAGE-1
-N DOCKER-ISOLATION-STAGE-2
-N DOCKER-USER
-A FORWARD -j DOCKER-USER
-A FORWARD -j DOCKER-FORWARD
-A DOCKER ! -i docker0 -o docker0 -j DROP
-A DOCKER-BRIDGE -o docker0 -j DOCKER
-A DOCKER-CT -o docker0 -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT
-A DOCKER-FORWARD -j DOCKER-CT
-A DOCKER-FORWARD -j DOCKER-ISOLATION-STAGE-1
-A DOCKER-FORWARD -j DOCKER-BRIDGE
-A DOCKER-FORWARD -i docker0 -j ACCEPT
-A DOCKER-ISOLATION-STAGE-1 -i docker0 ! -o docker0 -j DOCKER-ISOLATION-STAGE-2
-A DOCKER-ISOLATION-STAGE-2 -o docker0 -j DROP
-A DOCKER-USER -j RETURN

# firewall-cmd --reload
success

# iptables -S
-P INPUT ACCEPT
-P FORWARD ACCEPT
-P OUTPUT ACCEPT
-N DOCKER
-N DOCKER-BRIDGE
-N DOCKER-CT
-N DOCKER-FORWARD
-N DOCKER-ISOLATION-STAGE-1
-N DOCKER-ISOLATION-STAGE-2
-N DOCKER-USER
-A FORWARD -j DOCKER-USER
-A FORWARD -j DOCKER-FORWARD
-A DOCKER ! -i docker0 -o docker0 -j DROP
-A DOCKER ! -i br-e62823d3d7dc -o br-e62823d3d7dc -j DROP
-A DOCKER-BRIDGE -o docker0 -j DOCKER
-A DOCKER-BRIDGE -o br-e62823d3d7dc -j DOCKER
-A DOCKER-CT -o docker0 -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT
-A DOCKER-CT -o br-e62823d3d7dc -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT
-A DOCKER-FORWARD -j DOCKER-CT
-A DOCKER-FORWARD -j DOCKER-ISOLATION-STAGE-1
-A DOCKER-FORWARD -j DOCKER-BRIDGE
-A DOCKER-FORWARD -i docker0 -j ACCEPT
-A DOCKER-FORWARD -i br-e62823d3d7dc -j ACCEPT
-A DOCKER-ISOLATION-STAGE-1 -i docker0 ! -o docker0 -j DOCKER-ISOLATION-STAGE-2
-A DOCKER-ISOLATION-STAGE-1 -i br-e62823d3d7dc ! -o br-e62823d3d7dc -j DOCKER-ISOLATION-STAGE-2
-A DOCKER-ISOLATION-STAGE-2 -o br-e62823d3d7dc -j DROP
-A DOCKER-ISOLATION-STAGE-2 -o docker0 -j DROP
-A DOCKER-USER -j RETURN

- Human readable description for the release notes

- Fix an issue that could lead to unwanted iptables rules being restored and never deleted following a firewalld reload.

aepifanov

LGTM

akerouanton

Two small nits, otherwise LGTM.

libnetwork/drivers/bridge/bridge_linux.go

api/types/system/info.go

libnetwork/iptables/firewalld.go

libnetwork/controller_linux.go

thaJeztah

LGTM, thanks!

thaJeztah · 2025-04-08T08:46:13Z

libnetwork/drivers/bridge/bridge_linux.go

+	}
+
+	if err := nw.iptablesNetwork.reapplyNetworkLevelRules(); err != nil {
+		log.G(context.Background()).WithFields(log.Fields{


Curious; are the locations where we'd ultimately would want the context to be passed? (which could be a context.WithoutCancel).

Probably fine for later, as these are not exported, just curious if passing context could help with tracing etc.

Yes, potentially ... this function's called when a dbus message is received to say firewalld did-something, or on a signal. So, the context should be created a few calls up the stack from here, but Background seems appropriate in the meantime. (It will be an exported function in a couple of PR's time, but only from an internal package.)

thaJeztah · 2025-04-08T08:49:53Z

libnetwork/controller_linux.go

+		if !reloadedAt.IsZero() {
+			info.Info = append(info.Info, [2]string{"ReloadedAt", reloadedAt.Format(time.RFC3339)})


So, we're mostly using this information to know whether a reload happened (and if so, when it happened), right? Would there be value in knowing when it was first applied, or is that not relevant for debugging purposes? (thinking; "first loaded at X", and a reload "2 seconds later").

Yes - it's useful for debug. If we can see at-least one firewalld reload has happened, maybe it did something surprising. Or, if no reload has happened, perhaps it should have. And, it's a way for the daemon to signal to tests that they can continue after triggering a reload - a change in the timestamp means rules deleted by firewalld have been re-created by the daemon.

A few iptables rules are created on daemon init, and more at network creation or gateway update time. So, there's no single "load" time.

At some point, we could add a reload duration or triggered-at time, to report how long it takes to re-create the rules.

thaJeztah · 2025-04-08T08:55:34Z

internal/testutils/networking/iptables.go

+			return poll.Success()
+		}
+		return poll.Continue("firewalld reload not complete")
+	}, poll.WithDelay(100*time.Millisecond), poll.WithTimeout(20*time.Second))


FWIW; default is 100 miliseconds interval (so we don't need to set it), and 10 seconds timeout. If 10 seconds would still be enough for this one, we could remove this config.

(not a blocker for sure; mostly that I removed a bunch of these at some point where the custom options were the same as the default)

Ok, I can remove them ... ten seconds should be enough, we can always increase if it does lead to flaky tests, but that seems unlikely.

(Not sure removing it is good for at-a-glance readability though. I'm unlikely to remember the defaults, so will have to go and look them up if a test's misbehaving.)

Using iptables.OnReloaded to restore individual per-network rules on firewalld reload means rules for deleted networks pop back in to existence (because there was no way to delete the callbacks on network-delete). So, on firewalld reload, walk over current networks and ask them to restore their iptables rules. Signed-off-by: Rob Murray <rob.murray@docker.com>

Signed-off-by: Rob Murray <rob.murray@docker.com>

robmry self-assigned this Apr 1, 2025

robmry force-pushed the bridge_driver_firewalld_reload branch from 23232f5 to 0927988 Compare April 2, 2025 08:13

robmry changed the title ~~Stop firewalld reload re-creating rules for deleted networks/containers~~ Stop firewalld reload re-creating rules for deleted networks Apr 2, 2025

robmry added impact/changelog kind/bugfix PR's that fix bugs version/20.10 area/networking/firewalling Networking area/networking/d/bridge Networking area/networking/firewalld Networking labels Apr 2, 2025

robmry added this to the 28.1.0 milestone Apr 2, 2025

robmry force-pushed the bridge_driver_firewalld_reload branch 2 times, most recently from 5490f9c to 9d482e0 Compare April 2, 2025 10:01

robmry marked this pull request as ready for review April 2, 2025 10:36

robmry requested a review from akerouanton April 2, 2025 10:36

aepifanov approved these changes Apr 2, 2025

View reviewed changes

akerouanton approved these changes Apr 2, 2025

View reviewed changes

libnetwork/drivers/bridge/bridge_linux.go Outdated Show resolved Hide resolved

libnetwork/drivers/bridge/bridge_linux.go Outdated Show resolved Hide resolved

robmry force-pushed the bridge_driver_firewalld_reload branch from 9d482e0 to ea7aca7 Compare April 2, 2025 17:18

robmry requested a review from thaJeztah April 2, 2025 17:19

thaJeztah reviewed Apr 3, 2025

View reviewed changes

api/types/system/info.go Show resolved Hide resolved

thaJeztah reviewed Apr 3, 2025

View reviewed changes

libnetwork/iptables/firewalld.go Outdated Show resolved Hide resolved

libnetwork/iptables/firewalld.go Show resolved Hide resolved

libnetwork/iptables/firewalld.go Outdated Show resolved Hide resolved

libnetwork/controller_linux.go Outdated Show resolved Hide resolved

robmry force-pushed the bridge_driver_firewalld_reload branch from ea7aca7 to 13707b5 Compare April 7, 2025 09:11

robmry marked this pull request as draft April 7, 2025 09:12

robmry force-pushed the bridge_driver_firewalld_reload branch 2 times, most recently from 83cef04 to 0b0108c Compare April 7, 2025 17:03

robmry marked this pull request as ready for review April 7, 2025 17:05

robmry requested a review from thaJeztah April 7, 2025 17:05

thaJeztah approved these changes Apr 8, 2025

View reviewed changes

robmry added 3 commits April 8, 2025 10:26

Report firewalld reload time in Info.FirewallBackend

dbea045

Signed-off-by: Rob Murray <rob.murray@docker.com>

Test that firewalld reload doesn't re-create deleted iptables rules

c3fa7c1

Signed-off-by: Rob Murray <rob.murray@docker.com>

robmry force-pushed the bridge_driver_firewalld_reload branch from 0b0108c to c3fa7c1 Compare April 8, 2025 09:27

robmry merged commit 1b187e0 into moby:master Apr 8, 2025
148 checks passed

robmry deleted the bridge_driver_firewalld_reload branch April 8, 2025 10:34

corhere mentioned this pull request Jul 17, 2025

[25.0 backport] libnetwork/d/{bridge,overlay}: fix firewalld reload handling #50445

Merged

This was referenced Aug 4, 2025

Deadlock when firewalld is reloaded while the bridge driver is initializing #50619

Closed

libnet/d/bridge: handleFirewalldReloadNw: fix deadlock #50620

Merged

nh250146 mentioned this pull request Feb 13, 2026

Multiple CVEs found in latest release aenix-io/dnsmasq-controller#9

Open

		if !reloadedAt.IsZero() {
		info.Info = append(info.Info, [2]string{"ReloadedAt", reloadedAt.Format(time.RFC3339)})

Conversation

robmry commented Apr 1, 2025 • edited by vvoland Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

- Restore iptables for current networks on firewalld reload

- Report firewalld reload time in Info.FirewallBackend

- Test that firewalld reload doesn't re-create deleted iptables rules

Uh oh!

aepifanov left a comment

Choose a reason for hiding this comment

Uh oh!

akerouanton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

robmry Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

thaJeztah Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

robmry Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

thaJeztah Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

robmry Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

robmry commented Apr 1, 2025 •

edited by vvoland

Loading