Merge pull request #54012 from corhere/firewalld-restore-on-stop

libnetwork: handle firewalld stopping or starting late

daemon/libnetwork/iptables/firewalld.go

@@ -78,30 +78,43 @@ func firewalldInit() error {
 	if connection, err = newConnection(); err != nil {
 		return fmt.Errorf("Failed to connect to D-Bus system bus: %v", err)
 	}
-	firewalldRunning.Store(checkRunning())
-	if !firewalldRunning.Load() {
+
+	// firewalld does not handle method calls until it has finished starting,
+	// so we can start calling its methods as soon as it owns its name.
+	running, err := connection.nameHasOwner()
+	if err != nil {
 		connection.sysconn.Close()
 		connection = nil
+		return err
 	}
-	if connection != nil {
-		go signalHandler()
-		zoneAdded, err := setupDockerZone()
-		if err != nil {
+	firewalldRunning.Store(running)
+	// Keep the connection when firewalld is not running, to start using
+	// firewalld if it is started later.
+	go signalHandler()
+	if !running {
+		return 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
 }
 
@@ -119,54 +132,79 @@ func newConnection() (*Conn, error) {
 	c.sysObj = c.sysconn.Object(dbusInterface, dbusPath)
 	c.sysConfObj = c.sysconn.Object(dbusInterface, dbusConfigPath)
 
-	rule := fmt.Sprintf("type='signal',path='%s',interface='%s',sender='%s',member='Reloaded'", dbusPath, dbusInterface, dbusInterface)
-	c.sysconn.BusObject().Call("org.freedesktop.DBus.AddMatch", 0, rule)
-
-	rule = fmt.Sprintf("type='signal',interface='org.freedesktop.DBus',member='NameOwnerChanged',path='/org/freedesktop/DBus',sender='org.freedesktop.DBus',arg0='%s'", dbusInterface)
-	c.sysconn.BusObject().Call("org.freedesktop.DBus.AddMatch", 0, rule)
-
 	c.signal = make(chan *dbus.Signal, 10)
 	c.sysconn.Signal(c.signal)
+
+	if err := c.sysconn.AddMatchSignal(
+		dbus.WithMatchSender(dbusInterface),
+		dbus.WithMatchObjectPath(dbusPath),
+		dbus.WithMatchInterface(dbusInterface),
+		dbus.WithMatchMember("Reloaded"),
+	); err != nil {
+		return nil, fmt.Errorf("failed to subscribe to firewalld reload signal: %w", err)
+	}
+	if err := c.sysconn.AddMatchSignal(
+		dbus.WithMatchSender("org.freedesktop.DBus"),
+		dbus.WithMatchObjectPath("/org/freedesktop/DBus"),
+		dbus.WithMatchInterface("org.freedesktop.DBus"),
+		dbus.WithMatchMember("NameOwnerChanged"),
+		dbus.WithMatchArg(0, dbusInterface),
+	); err != nil {
+		return nil, fmt.Errorf("failed to subscribe to firewalld name owner changes: %w", err)
+	}
 	return c, nil
 }
 
+// nameHasOwner reports whether firewalld owns its D-Bus name. Call it after
+// subscribing to NameOwnerChanged, so that no change of owner is missed
+// between the two.
+func (c *Conn) nameHasOwner() (bool, error) {
+	var hasOwner bool
+	if err := c.sysconn.BusObject().Call("org.freedesktop.DBus.NameHasOwner", 0, dbusInterface).Store(&hasOwner); err != nil {
+		return false, fmt.Errorf("failed to check whether firewalld is running: %w", err)
+	}
+	return hasOwner, nil
+}
+
 func signalHandler() {
 	for signal := range connection.signal {
-		switch {
-		case strings.Contains(signal.Name, "NameOwnerChanged"):
-			firewalldRunning.Store(checkRunning())
+		switch signal.Name {
+		case "org.freedesktop.DBus.NameOwnerChanged":
 			dbusConnectionChanged(signal.Body)
-
-		case strings.Contains(signal.Name, "Reloaded"):
+		case dbusInterface + ".Reloaded":
 			reloaded()
 		}
 	}
 }
 
 func dbusConnectionChanged(args []any) {
-	name := args[0].(string)
-	oldOwner := args[1].(string)
-	newOwner := args[2].(string)
+	var name, oldOwner, newOwner string
+	if err := dbus.Store(args, &name, &oldOwner, &newOwner); err != nil {
+		log.G(context.TODO()).WithError(err).Error("Could not parse D-Bus NameOwnerChanged signal")
+		return
+	}
 
 	if name != dbusInterface {
 		return
 	}
 
-	if newOwner != "" {
-		connectionEstablished()
-	} else if oldOwner != "" {
-		connectionLost()
+	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")
+		}
 	}
-}
-
-func connectionEstablished() {
+	// With CleanupOnExit=yes, firewalld deletes the iptables rules before it
+	// releases its D-Bus name. Restore our iptables rules when firewalld
+	// starts or exits.
 	reloaded()
 }
 
-func connectionLost() {
-	// Doesn't do anything for now. Libvirt also doesn't react to this.
-}
-
 // call all callbacks
 func reloaded() {
 	firewalldReloadMu.Lock()
@@ -185,16 +223,6 @@ func OnReloaded(callback func()) {
 	onReloaded = append(onReloaded, &callback)
 }
 
-// Call some remote method to see whether the service is actually running.
-func checkRunning() bool {
-	if connection == nil {
-		return false
-	}
-	var zone string
-	err := connection.sysObj.Call(dbusInterface+".getDefaultZone", 0).Store(&zone)
-	return err == nil
-}
-
 // passthrough method simply passes args through to iptables/ip6tables
 func passthrough(ipv IPVersion, args ...string) ([]byte, error) {
 	var output string

daemon/libnetwork/iptables/firewalld_test.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"net/netip"
+	"slices"
 	"strconv"
 	"testing"
 
@@ -20,9 +21,9 @@ func skipIfNoFirewalld(t *testing.T) {
 	if err != nil {
 		t.Skipf("cannot connect to D-bus system bus: %v", err)
 	}
-	var zone string
-	err = conn.Object(dbusInterface, dbusPath).Call(dbusInterface+".getDefaultZone", 0).Store(&zone)
-	if err != nil {
+	var running bool
+	err = conn.BusObject().Call("org.freedesktop.DBus.NameHasOwner", 0, dbusInterface).Store(&running)
+	if err != nil || !running {
 		t.Skipf("firewalld is not running: %v", err)
 	}
 }
@@ -89,6 +90,71 @@ 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) {
+	savedConn, savedCallbacks, savedRunning := connection, onReloaded, firewalldRunning.Load()
+	t.Cleanup(func() {
+		connection = savedConn
+		onReloaded = savedCallbacks
+		firewalldRunning.Store(savedRunning)
+	})
+
+	tests := []struct {
+		name       string
+		args       []any
+		wasRunning bool
+		expRunning bool
+		expCalls   int
+		expSetup   bool
+	}{
+		{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()
+
+			dbusConnectionChanged(tc.args)
+
+			assert.Check(t, is.Equal(firewalldRunning.Load(), tc.expRunning))
+			assert.Check(t, is.Equal(calls, tc.expCalls))
+			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))
+		})
+	}
+}
+
 func TestPassthrough(t *testing.T) {
 	skipIfNoFirewalld(t)
 	rule1 := []string{

integration/networking/firewall_linux_test.go

@@ -1,15 +1,26 @@
 package networking
 
 import (
+	"context"
 	"os"
+	"slices"
+	"strings"
 	"testing"
+	"time"
 
+	networktypes "github.com/moby/moby/api/types/network"
 	"github.com/moby/moby/client"
+	"github.com/moby/moby/v2/daemon/libnetwork/drivers/bridge"
+	"github.com/moby/moby/v2/integration/internal/container"
+	"github.com/moby/moby/v2/integration/internal/network"
 	"github.com/moby/moby/v2/integration/internal/testutils/networking"
 	"github.com/moby/moby/v2/internal/testutil/daemon"
 	"github.com/moby/moby/v2/internal/testutil/request"
 	"gotest.tools/v3/assert"
 	is "gotest.tools/v3/assert/cmp"
+	"gotest.tools/v3/icmd"
+	"gotest.tools/v3/poll"
+	"gotest.tools/v3/skip"
 )
 
 const defaultFirewallBackend = "iptables"
@@ -50,3 +61,159 @@ func TestInfoFirewallBackend(t *testing.T) {
 		assert.Check(t, is.Nil(info148.FirewallBackend))
 	})
 }
+
+// TestFirewalldStop checks that the daemon restores its iptables rules when
+// firewalld deletes them on stopping, and uses firewalld again when it is
+// started.
+func TestFirewalldStop(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)
+
+	d := daemon.New(t)
+	d.StartWithBusybox(ctx, t)
+	t.Cleanup(func() { d.Stop(t) })
+	c := d.NewClientT(t)
+	t.Cleanup(func() { c.Close() })
+
+	const bridgeName = "br-fwstop"
+	want := setupPublishedPort(ctx, t, c, bridgeName)
+
+	t.Cleanup(func() { startFirewalld(t) })
+	if !firewalldDeletesRules(t, func() { stopFirewalld(t) }) {
+		t.Skip("stopping firewalld does not delete iptables rules")
+	}
+	waitForRules(t, "after firewalld stopped", bridgeName, want)
+	assert.Check(t, is.Equal(d.FirewallBackendDriver(t), "iptables"))
+
+	deleted := firewalldDeletesRules(t, func() { startFirewalld(t) })
+	if deleted {
+		waitForRules(t, "after firewalld started", bridgeName, want)
+	}
+	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.
+func setupPublishedPort(ctx context.Context, t *testing.T, c client.APIClient, bridgeName string) []string {
+	t.Helper()
+	const netName = "testnet"
+	network.CreateNoError(ctx, t, c, netName,
+		network.WithDriver("bridge"),
+		network.WithOption(bridge.BridgeName, bridgeName),
+	)
+	t.Cleanup(func() { network.RemoveNoError(ctx, t, c, netName) })
+	ctrID := container.Run(ctx, t, c,
+		container.WithNetworkMode(netName),
+		container.WithExposedPorts("80/tcp"),
+		container.WithPortMap(networktypes.PortMap{
+			networktypes.MustParsePort("80/tcp"): {{HostPort: "8080"}},
+		}),
+	)
+	t.Cleanup(func() { c.ContainerRemove(ctx, ctrID, client.ContainerRemoveOptions{Force: true}) })
+
+	rules := bridgeRules(t, bridgeName)
+	assert.Assert(t, len(rules) > 0, "no iptables rules refer to %s", bridgeName)
+	return rules
+}
+
+// bridgeRules returns the iptables rules which refer to bridgeName.
+func bridgeRules(t *testing.T, bridgeName string) []string {
+	t.Helper()
+	var rules []string
+	for _, cmd := range []string{"iptables-save", "ip6tables-save"} {
+		res := icmd.RunCommand(cmd)
+		res.Assert(t, icmd.Success)
+		for _, l := range strings.Split(res.Stdout(), "\n") {
+			if strings.Contains(l, bridgeName) {
+				rules = append(rules, cmd+": "+l)
+			}
+		}
+	}
+	return rules
+}
+
+// waitForRules waits until every rule in want is present.
+func waitForRules(t *testing.T, when, bridgeName string, want []string) {
+	t.Helper()
+	poll.WaitOn(t, func(poll.LogT) poll.Result {
+		got := bridgeRules(t, bridgeName)
+		var missing []string
+		for _, r := range want {
+			if !slices.Contains(got, r) {
+				missing = append(missing, r)
+			}
+		}
+		if len(missing) > 0 {
+			return poll.Continue("%s, rules are missing:\n%s", when, strings.Join(missing, "\n"))
+		}
+		return poll.Success()
+	}, poll.WithTimeout(30*time.Second))
+}
+
+// firewalldDeletesRules reports whether f deletes an iptables chain that the
+// test adds before calling it.
+func firewalldDeletesRules(t *testing.T, f func()) bool {
+	t.Helper()
+	const markerChain = "TEST-FIREWALLD-MARKER"
+	icmd.RunCommand("iptables", "-N", markerChain).Assert(t, icmd.Success)
+	t.Cleanup(func() { icmd.RunCommand("iptables", "-X", markerChain) })
+	f()
+	return icmd.RunCommand("iptables", "-S", markerChain).ExitCode != 0
+}
+
+func stopFirewalld(t *testing.T) {
+	t.Helper()
+	icmd.RunCommand("systemctl", "stop", "firewalld").Assert(t, icmd.Success)
+}
+
+func startFirewalld(t *testing.T) {
+	t.Helper()
+	icmd.RunCommand("systemctl", "start", "firewalld").Assert(t, icmd.Success)
+	poll.WaitOn(t, func(poll.LogT) poll.Result {
+		if !networking.FirewalldRunning() {
+			return poll.Continue("firewalld is not running")
+		}
+		return poll.Success()
+	}, poll.WithTimeout(30*time.Second))
+}