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)) }) }