Re: [PATCH v2 12/27] network: support setting firewallBackend from network.conf

On 4/23/24 6:17 AM, Daniel P. Berrangé wrote: > On Sun, Apr 21, 2024 at 10:53:20PM -0400, Laine Stump wrote: > > It still can have only one useful value ("iptables"), but once a 2nd > > value is supported, it will be selectable by setting > > "firewall_backend=nftables" in /etc/libvirt/network.conf. > > > > If firewall_backend isn't set in network.conf, then libvirt will check > > to see if the iptables binary is present on the system and set > > firewallBackend to iptables; if not, it will be left as "unset", which > > (once multiple backends are available) will trigger an appropriate > > error message the first time we attempt to add a rule. > > > > > > > Signed-off-by: Laine Stump <laine(a)redhat.com&gt; > > --- > > src/network/bridge_driver.c | 22 +++++++------ > > src/network/bridge_driver_conf.c | 40 ++++++++++++++++++++++++ > > src/network/bridge_driver_conf.h | 3 ++ > > src/network/bridge_driver_linux.c | 6 ++-- > > src/network/bridge_driver_nop.c | 6 ++-- > > src/network/bridge_driver_platform.h | 6 ++-- > > src/network/libvirtd_network.aug | 5 ++- > > src/network/network.conf | 8 +++++ > > src/network/test_libvirtd_network.aug.in | 3 ++ > > tests/networkxml2firewalltest.c | 2 +- > > 10 files changed, 83 insertions(+), 18 deletions(-) > > > > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c > > index d89700c6ee..38e4ab84ad 100644 > > --- a/src/network/bridge_driver.c > > +++ b/src/network/bridge_driver.c > > > diff --git a/src/network/bridge_driver_conf.c b/src/network/bridge_driver_conf.c > > index a2edafa837..9769ee06b5 100644 > > --- a/src/network/bridge_driver_conf.c > > +++ b/src/network/bridge_driver_conf.c > > @@ -25,6 +25,7 @@ > > #include "datatypes.h" > > #include "virlog.h" > > #include "virerror.h" > > +#include "virfile.h" > > #include "virutil.h" > > #include "bridge_driver_conf.h" > > @@ -62,6 +63,7 @@ virNetworkLoadDriverConfig(virNetworkDriverConfig *cfg G_GNUC_UNUSED, > > const char *filename) > > { > > g_autoptr(virConf) conf = NULL; > > + g_autofree char *firewallBackendStr = NULL; > > /* if file doesn't exist or is unreadable, ignore the "error" */ > > if (access(filename, R_OK) == -1) > > @@ -73,6 +75,44 @@ virNetworkLoadDriverConfig(virNetworkDriverConfig *cfg G_GNUC_UNUSED, > > /* use virConfGetValue*(conf, ...) functions to read any settings into cfg */ > > + if (virConfGetValueString(conf, "firewall_backend", &firewallBackendStr) < 0) > > + return -1; > > + > > + if (firewallBackendStr) { > > + int backend = virFirewallBackendTypeFromString(firewallBackendStr); > > + > > + if (backend < 0) { > > + virReportError(VIR_ERR_INTERNAL_ERROR, > > + _("unknown value for 'firewall_backend' in network.conf: '%1$s'"), > > + firewallBackendStr); > > + return -1; > > + } > > + > > + cfg->firewallBackend = backend; > > + VIR_INFO("using firewall_backend setting from network.conf: '%s'", > > + virFirewallBackendTypeToString(cfg->firewallBackend)); > > Since the BACKEND enum has "unset" as a valid entry, and you're checking > 'backend < 0' here, Oops. That should have been <= 0, which is something we commonly do in the conf directory when we don't want to allow, e.g., formatting an "attr='default' for an attribute that hasn't been set. > this allows the user to explicitly do > > firewall_backend="UNSET" > > To me this is another reason to eliminate the "UNSET" backend enum value. > > > + > > + } else { > > + > > + /* no .conf setting, so see what this host supports by looking > > + * for binaries used by the backends, and set accordingly. > > + */ > > + g_autofree char *iptablesInPath = NULL; > > + > > + /* virFindFileInPath() uses g_find_program_in_path(), > > + * which allows absolute paths, and verifies that > > + * the file is executable. > > + */ > > + if ((iptablesInPath = virFindFileInPath(IPTABLES))) > > + cfg->firewallBackend = VIR_FIREWALL_BACKEND_IPTABLES; > > + > > + if (cfg->firewallBackend == VIR_FIREWALL_BACKEND_UNSET) > > Rather than checking against "UNSET", this could just be an 'else' > clause, and a fatal error. in the final version, it is: if (iptables is found) backend = iptables else if (nftables is found) backend = nftables if (backend == unset) INFO("couldn't find a backend" else INFO("backend set to %s", backend) > > Using a fatal error would mean the virnetworkd will fail to start, > since and journalctl will give the explanation. > > If we only report it later at time of first usage, then the app > user will see it, but they're not in a position to fix it. Showing > a failed service looks to give more attention to the admin. > > Of course they should not get into this situation in the first > place with a packaged distro, since the distro should have added > deps to pull in either iptables or nftables or both. Okay, I can do that (I was *kind of* (but not really :-/) following in the footsteps of the pre-existing networkSetupPrivateChains(), which saves off error messages and only reports them when a network is started. But now I realize that I'm not even doing that - just putting an INFO message in the logfile). I'm wary of completely failing to start if the network driver fails to find a firewall backend when it's reading its config, since that always happens, but it may be the case that nobody is actually using a virtual network, in which case maybe they don't care. However, I suppose since most people are now running split daemons rather than monolithic libvirtd, that wouldn't really matter so it should be okay (failure of virtnetworkd doesn't shut everything else down, right? And does virtnetworkd ever even get started if no guests have <interface type='network'>? I guess I'll find out when I make this change and try it :-))