From 78ddfecd0867cfccc450893985e149ada39666c5 Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Wed, 21 Oct 2026 14:11:27 -0400 Subject: [PATCH] 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 (cherry picked from commit 3e896ecd6ad32db05c20d3a4c2d9b1fa75211e31) Signed-off-by: Sebastiaan van Stijn --- daemon/libnetwork/iptables/firewalld.go | 74 +++++++++++++------- daemon/libnetwork/iptables/firewalld_test.go | 42 +++++++---- 2 files changed, 78 insertions(+), 38 deletions(-) diff --git a/daemon/libnetwork/iptables/firewalld.go b/daemon/libnetwork/iptables/firewalld.go index bf6431963f..4d18c4d779 100644 --- a/daemon/libnetwork/iptables/firewalld.go +++ b/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 diff --git a/daemon/libnetwork/iptables/firewalld_test.go b/daemon/libnetwork/iptables/firewalld_test.go index a7c4cf0b09..254010e1ce 100644 --- a/daemon/libnetwork/iptables/firewalld_test.go +++ b/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)) }) }