From c1b64fbbdb57469a1043d92d4c8f7376d3442b37 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 16 Aug 2023 01:03:49 +0200 Subject: [PATCH] libnetwork/iptables: register callbacks on firewalldConnection Register callbacks on the firewalldConnection instance instead of a global variable. These callbacks should only be needed if firewalld is initialized and running, so associate them with the firewalldConnection that we created. Signed-off-by: Sebastiaan van Stijn --- libnetwork/iptables/firewalld.go | 90 +++++++++++++++++++-------- libnetwork/iptables/firewalld_test.go | 10 ++- 2 files changed, 72 insertions(+), 28 deletions(-) diff --git a/libnetwork/iptables/firewalld.go b/libnetwork/iptables/firewalld.go index a09d78e7f5..ffd1880658 100644 --- a/libnetwork/iptables/firewalld.go +++ b/libnetwork/iptables/firewalld.go @@ -23,6 +23,7 @@ const ( ) const ( + // See https://github.com/firewalld/firewalld/blob/v1.3.3/doc/xml/firewalld.dbus.xml#L49-L64 dbusInterface = "org.fedoraproject.FirewallD1" dbusPath = "/org/fedoraproject/FirewallD1" dbusConfigPath = "/org/fedoraproject/FirewallD1/config" @@ -36,13 +37,11 @@ type firewalldConnection struct { sysObj dbus.BusObject sysConfObj dbus.BusObject signal chan *dbus.Signal + + onReloaded []*func() // callbacks to run when Firewalld is reloaded. } -var ( - firewalld *firewalldConnection - - onReloaded []*func() // callbacks when Firewalld has been reloaded -) +var firewalld *firewalldConnection // firewalldInit initializes firewalld management code. func firewalldInit() (*firewalldConnection, error) { @@ -109,54 +108,93 @@ func (fwd *firewalldConnection) handleSignals() { case strings.Contains(signal.Name, "NameOwnerChanged"): // re-check if firewalld is still running. fwd.checkRunning() - dbusConnectionChanged(signal.Body) + fwd.onConnectionChange(signal.Body) case strings.Contains(signal.Name, "Reloaded"): - reloaded() + fwd.onReload() } } }() } -func dbusConnectionChanged(args []interface{}) { - name := args[0].(string) - oldOwner := args[1].(string) - newOwner := args[2].(string) - - if name != dbusInterface { +func (fwd *firewalldConnection) onConnectionChange(args []any) { + if name := args[0].(string); name != dbusInterface { return } - + oldOwner := args[1].(string) + newOwner := args[2].(string) if len(newOwner) > 0 { - connectionEstablished() + fwd.onConnectionEstablished() } else if len(oldOwner) > 0 { - connectionLost() + fwd.onConnectionLost() } } -func connectionEstablished() { - reloaded() +func (fwd *firewalldConnection) onConnectionEstablished() { + fwd.onReload() } -func connectionLost() { +func (fwd *firewalldConnection) onConnectionLost() { // Doesn't do anything for now. Libvirt also doesn't react to this. } -// call all callbacks -func reloaded() { - for _, pf := range onReloaded { +// onReload executes all registered callbacks. +func (fwd *firewalldConnection) onReload() { + // Note that we're not checking if firewalld is running here, + // as the previous code did not check for this, and (technically), + // the callbacks may not require firewalld. So we're assuming here + // that all callback can be executed if this onReload function + // is triggered (either from a D-Bus event, or otherwise). + // + // TODO(thaJeztah): verify if these should always be run, or only if firewalld is running. + if fwd == nil { + return + } + for _, pf := range fwd.onReloaded { (*pf)() } } -// OnReloaded add callback -func OnReloaded(callback func()) { - for _, pf := range onReloaded { +// registerReloadCallback adds a callback to be executed when firewalld +// is reloaded. Adding a callback is idempotent; it ignores the given +// callback if it's already registered. +// +// It is a no-op if firewalld is not running or firewalldConnection is not +// initialized. +func (fwd *firewalldConnection) registerReloadCallback(callback func()) { + // Note that we're not checking if firewalld is running here, + // as the previous code did not check for this, and (technically), + // the callbacks may not require firewalld, or may be registered + // when firewalld is not running. + // + // We're assuming here that all callback can be executed if this + // onReload function is triggered (either from a D-Bus event, or + // otherwise). + // + // TODO(thaJeztah): verify if these should always be run, or only if firewalld is running at the moment the callback is registered. + if fwd == nil { + return + } + for _, pf := range fwd.onReloaded { if pf == &callback { return } } - onReloaded = append(onReloaded, &callback) + fwd.onReloaded = append(fwd.onReloaded, &callback) +} + +// OnReloaded adds a callback to be executed when firewalld is reloaded. +// Adding a callback is idempotent; it ignores the given callback if it's +// already registered. +// +// Callbacks can be registered regardless if firewalld is currently running, +// but it will initialize firewalld before executing. +func OnReloaded(callback func()) { + // Make sure firewalld is initialized before we register callbacks. + // This function is also called from setupArrangeUserFilterRule, + // which is called during controller initialization. + _ = initCheck() + firewalld.registerReloadCallback(callback) } // checkRunning checks if firewalld is running. diff --git a/libnetwork/iptables/firewalld_test.go b/libnetwork/iptables/firewalld_test.go index 62490153a0..6021afe5b3 100644 --- a/libnetwork/iptables/firewalld_test.go +++ b/libnetwork/iptables/firewalld_test.go @@ -53,12 +53,17 @@ func TestReloaded(t *testing.T) { const port = 1234 const proto = "tcp" + // create a dummy firewalldConnection and mark it as "running", because + // OnReloaded (registerReloadCallback), + fwd := &firewalldConnection{} + fwd.running.Store(true) + err = fwdChain.Link(Append, ip1, ip2, port, proto, bridgeName) if err != nil { t.Fatal(err) } else { // to be re-called again later - OnReloaded(func() { fwdChain.Link(Append, ip1, ip2, port, proto, bridgeName) }) + fwd.registerReloadCallback(func() { fwdChain.Link(Append, ip1, ip2, port, proto, bridgeName) }) } rule1 := []string{ @@ -78,7 +83,7 @@ func TestReloaded(t *testing.T) { // flush all rules fwdChain.Remove() - reloaded() + fwd.onReload() // make sure the rules have been recreated if !iptable.Exists(fwdChain.Table, fwdChain.Name, rule1...) { @@ -120,4 +125,5 @@ func TestFirewalldUninitialized(t *testing.T) { if err != nil { t.Errorf("unexpected error when calling delInterface on an uninitialized firewalldConnection: %v", err) } + fwd.registerReloadCallback(func() {}) }