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

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,

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.

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.

> + VIR_INFO("firewall_backend not set, and no usable backend auto-detected"); > + else > + VIR_INFO("using auto-detected firewall_backend: '%s'", > + virFirewallBackendTypeToString(cfg->firewallBackend)); > + } > + > return 0; > } > With regards, Daniel