On Tue, Apr 23, 2024 at 11:48:24AM -0400, Laine Stump wrote:
> 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@xxxxxxxxxx>
> > > ---
> > > 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
> :-))
It is socket activated on first use with systemd.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
