d/libn/iptables: track firewalld by its D-Bus name

The daemon checks whether firewalld is running by calling
getDefaultZone at startup, and again whenever firewalld's D-Bus name
changes owner. The check infers firewalld's state from whether a
method call succeeds.

firewalld does not handle method calls until it has finished starting.
It is ready as soon as it owns its name. Ask the bus whether the name
has an owner at startup, after subscribing to NameOwnerChanged so that
no change of owner is missed, and take the state from each
NameOwnerChanged signal after that.

Signed-off-by: Cory Snider <csnider@mirantis.com>
(cherry picked from commit 3e896ecd6a)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

daemon/libnetwork/iptables/firewalld.go

@@ -78,8 +78,17 @@ 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
+	}
+	firewalldRunning.Store(running)
+	if !running {
 		connection.sysconn.Close()
 		connection = nil
 	}
@@ -119,37 +128,64 @@ 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)
+	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
 	}
 
+	running := newOwner != ""
+	firewalldRunning.Store(running)
 	// With CleanupOnExit=yes, firewalld deletes the iptables rules before it
 	// releases its D-Bus name. Restore our iptables rules when firewalld
 	// starts or exits.
@@ -174,16 +210,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

@@ -20,9 +20,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)
 	}
 }
@@ -90,29 +90,43 @@ func TestReloaded(t *testing.T) {
 }
 
 func TestDBusConnectionChanged(t *testing.T) {
-	saved := onReloaded
-	t.Cleanup(func() { onReloaded = saved })
+	savedCallbacks, savedRunning := onReloaded, firewalldRunning.Load()
+	t.Cleanup(func() {
+		onReloaded = savedCallbacks
+		firewalldRunning.Store(savedRunning)
+	})
 
 	tests := []struct {
-		name               string
-		service            string
-		oldOwner, newOwner string
-		expCalls           int
+		name       string
+		args       []any
+		wasRunning bool
+		expRunning bool
+		expCalls   int
 	}{
-		{name: "firewalld started", service: dbusInterface, newOwner: ":1.42", expCalls: 1},
-		{name: "firewalld stopped", service: dbusInterface, oldOwner: ":1.42", expCalls: 1},
-		{name: "other service stopped", service: "org.example.Other", oldOwner: ":1.43", expCalls: 0},
+		{name: "firewalld started", args: []any{dbusInterface, "", ":1.42"}, expRunning: true, expCalls: 1},
+		{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) {
 			onReloaded = nil
 			var calls int
-			OnReloaded(func() { calls++ })
+			var runningInCallback bool
+			OnReloaded(func() {
+				calls++
+				runningInCallback = firewalldRunning.Load()
+			})
+			firewalldRunning.Store(tc.wasRunning)
 			before := FirewalldReloadedAt()
 
-			dbusConnectionChanged([]any{tc.service, tc.oldOwner, tc.newOwner})
+			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")
+			}
 			assert.Check(t, FirewalldReloadedAt().After(before) == (tc.expCalls > 0))
 		})
 	}