d/libn/iptables: use firewalld when started later

The daemon uses firewalld only if firewalld is running when the daemon
starts. If firewalld is started later, the daemon does not react to it
starting or reloading. On firewalld configurations which delete other
applications' iptables rules when firewalld starts, the daemon's rules
stay deleted. The daemon's bridges are not added to the docker zone.

Keep the D-Bus connection when firewalld is not running at startup.
When firewalld acquires its D-Bus name, set up the docker zone and
forwarding policy before running the reload callbacks.

Signed-off-by: Cory Snider <csnider@mirantis.com>

daemon/libnetwork/iptables/firewalld.go

@@ -88,29 +88,33 @@ func firewalldInit() error {
 		return err
 	}
 	firewalldRunning.Store(running)
+	// Keep the connection when firewalld is not running, to start using
+	// firewalld if it is started later.
+	go signalHandler()
 	if !running {
-		connection.sysconn.Close()
-		connection = nil
+		return nil
 	}
-	if connection != nil {
-		go signalHandler()
-		zoneAdded, err := setupDockerZone()
-		if err != nil {
+	return setupFirewalld()
+}
+
+// setupFirewalld creates the docker zone and forwarding policy in firewalld,
+// if they do not already exist.
+func setupFirewalld() error {
+	zoneAdded, err := setupDockerZone()
+	if err != nil {
+		return err
+	}
+	policyAdded, policyAddErr := setupDockerForwardingPolicy()
+	if policyAddErr != nil {
+		// Log the error, but still reload firewalld if necessary.
+		log.G(context.TODO()).WithError(policyAddErr).Warnf("Firewalld: failed to add policy %s", dockerFwdPolicy)
+	}
+	if zoneAdded || policyAdded {
+		// Reload for changes to take effect.
+		if err := connection.sysObj.Call(dbusInterface+".reload", 0).Err; err != nil {
 			return err
 		}
-		policyAdded, policyAddErr := setupDockerForwardingPolicy()
-		if policyAddErr != nil {
-			// Log the error, but still reload firewalld if necessary.
-			log.G(context.TODO()).WithError(policyAddErr).Warnf("Firewalld: failed to add policy %s", dockerFwdPolicy)
-		}
-		if zoneAdded || policyAdded {
-			// Reload for changes to take effect.
-			if err := connection.sysObj.Call(dbusInterface+".reload", 0).Err; err != nil {
-				return err
-			}
-		}
 	}
-
 	return nil
 }
 
@@ -186,6 +190,15 @@ func dbusConnectionChanged(args []any) {
 
 	running := newOwner != ""
 	firewalldRunning.Store(running)
+	if running {
+		// firewalld may have been started for the first time since the daemon
+		// started, or its permanent configuration may have changed.
+		// firewalld does not handle method calls until it has finished starting,
+		// so we can set up the docker zone as soon as it owns its name.
+		if err := setupFirewalld(); err != nil {
+			log.G(context.TODO()).WithError(err).Error("Firewalld: failed to set up docker zone")
+		}
+	}
 	// With CleanupOnExit=yes, firewalld deletes the iptables rules before it
 	// releases its D-Bus name. Restore our iptables rules when firewalld
 	// starts or exits.

daemon/libnetwork/iptables/firewalld_test.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"net/netip"
+	"slices"
 	"strconv"
 	"testing"
 
@@ -89,9 +90,21 @@ func TestReloaded(t *testing.T) {
 	}
 }
 
+// fakeFirewalldObject records the methods called on it, and fails every call.
+type fakeFirewalldObject struct {
+	dbus.BusObject
+	methods []string
+}
+
+func (o *fakeFirewalldObject) Call(method string, _ dbus.Flags, _ ...any) *dbus.Call {
+	o.methods = append(o.methods, method)
+	return &dbus.Call{Err: errors.New("fake firewalld")}
+}
+
 func TestDBusConnectionChanged(t *testing.T) {
-	savedCallbacks, savedRunning := onReloaded, firewalldRunning.Load()
+	savedConn, savedCallbacks, savedRunning := connection, onReloaded, firewalldRunning.Load()
 	t.Cleanup(func() {
+		connection = savedConn
 		onReloaded = savedCallbacks
 		firewalldRunning.Store(savedRunning)
 	})
@@ -102,20 +115,25 @@ func TestDBusConnectionChanged(t *testing.T) {
 		wasRunning bool
 		expRunning bool
 		expCalls   int
+		expSetup   bool
 	}{
-		{name: "firewalld started", args: []any{dbusInterface, "", ":1.42"}, expRunning: true, expCalls: 1},
+		{name: "firewalld started", args: []any{dbusInterface, "", ":1.42"}, expRunning: true, expCalls: 1, expSetup: true},
 		{name: "firewalld stopped", args: []any{dbusInterface, ":1.42", ""}, wasRunning: true, expRunning: false, expCalls: 1},
 		{name: "other service stopped", args: []any{"org.example.Other", ":1.43", ""}, wasRunning: true, expRunning: true, expCalls: 0},
 		{name: "malformed signal", args: []any{dbusInterface, ":1.42"}, wasRunning: true, expRunning: true, expCalls: 0},
 	}
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
+			fw := &fakeFirewalldObject{}
+			connection = &Conn{sysObj: fw, sysConfObj: fw}
 			onReloaded = nil
 			var calls int
 			var runningInCallback bool
+			var methodsBeforeCallback []string
 			OnReloaded(func() {
 				calls++
 				runningInCallback = firewalldRunning.Load()
+				methodsBeforeCallback = slices.Clone(fw.methods)
 			})
 			firewalldRunning.Store(tc.wasRunning)
 			before := FirewalldReloadedAt()
@@ -127,6 +145,11 @@ func TestDBusConnectionChanged(t *testing.T) {
 			if tc.expCalls > 0 {
 				assert.Check(t, is.Equal(runningInCallback, tc.expRunning), "callbacks must see the new state")
 			}
+			if tc.expSetup {
+				assert.Check(t, is.Contains(methodsBeforeCallback, dbusInterface+".zone.getZones"), "the docker zone must be set up before the callbacks run")
+			} else {
+				assert.Check(t, is.Len(fw.methods, 0))
+			}
 			assert.Check(t, FirewalldReloadedAt().After(before) == (tc.expCalls > 0))
 		})
 	}

integration/networking/firewall_linux_test.go

@@ -93,6 +93,45 @@ func TestFirewalldStop(t *testing.T) {
 	assert.Check(t, is.Equal(d.FirewallBackendDriver(t), "iptables+firewalld"))
 }
 
+// TestFirewalldStartedAfterDaemon checks that the daemon starts using firewalld
+// when firewalld is started after the daemon.
+func TestFirewalldStartedAfterDaemon(t *testing.T) {
+	skip.If(t, testEnv.IsRootless, "rootless mode does not use firewalld")
+	skip.If(t, testEnv.FirewallBackendDriver() != "iptables+firewalld", "test is for the iptables backend with firewalld")
+	ctx := setupTest(t)
+
+	t.Cleanup(func() { startFirewalld(t) })
+	stopFirewalld(t)
+
+	d := daemon.New(t)
+	d.StartWithBusybox(ctx, t)
+	t.Cleanup(func() { d.Stop(t) })
+	c := d.NewClientT(t)
+	t.Cleanup(func() { c.Close() })
+	assert.Check(t, is.Equal(d.FirewallBackendDriver(t), "iptables"))
+
+	const bridgeName = "br-fwlate"
+	want := setupPublishedPort(ctx, t, c, bridgeName)
+
+	deleted := firewalldDeletesRules(t, func() { startFirewalld(t) })
+	poll.WaitOn(t, func(poll.LogT) poll.Result {
+		if drv := d.FirewallBackendDriver(t); drv != "iptables+firewalld" {
+			return poll.Continue("firewall backend driver is %q", drv)
+		}
+		return poll.Success()
+	})
+	poll.WaitOn(t, func(poll.LogT) poll.Result {
+		res := icmd.RunCommand("firewall-cmd", "--zone=docker", "--list-interfaces")
+		if !slices.Contains(strings.Fields(res.Stdout()), bridgeName) {
+			return poll.Continue("%s is not in the docker zone: %s", bridgeName, res.Combined())
+		}
+		return poll.Success()
+	})
+	if deleted {
+		waitForRules(t, "after firewalld started", bridgeName, want)
+	}
+}
+
 // setupPublishedPort creates a bridge network with the given bridge name, and
 // a container on it with a published port. It returns the iptables rules which
 // refer to the bridge.