On Thu, Apr 25, 2024 at 01:38:18AM -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 no iptables binary is found, that is
> considered a fatal error (since no networks can be started anyway), so
> an error is logged and startup of the network driver fails.
>
> NB: network.conf is itself created from network.conf.in at build time,
> and the advertised default setting of firewall_backend (in a commented
> out line) is set from the meson_options.txt setting
> "firewall_backend". This way the conf file will have correct
> information no matter what default backend is chosen at build time.
>
> Signed-off-by: Laine Stump <laine@xxxxxxxxxx>
> Reviewed-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> ---
> Changes from V2:
>
> * make failure to find iptables during network driver init fatal
>
> * recognize firewall_backend setting in meson_options.txt and
> propagate it through to:
>
> * meson-config.h (along with a numeric constant
> FIREWALL_BACKEND_DEFAULT_IPTABLES, which can be used for
> conditional compilation)
> * network.conf - default setting and comments use @FIREWALL_BACKEND@
> * test_libvirtd_network.aug.in so that default firewall backend
> for testing is set properly (this required calisthenics similar to
> qemu_user_group_hack_conf in the qemu driver's meson.build
> (see commit 64a7b8203bf)
>
> meson.build | 5 +++
> meson_options.txt | 1 +
> src/network/bridge_driver.c | 22 ++++++-----
> src/network/bridge_driver_conf.c | 50 ++++++++++++++++++++++++
> 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/meson.build | 22 ++++++++++-
> src/network/network.conf.in | 8 ++++
> src/network/test_libvirtd_network.aug.in | 3 ++
> tests/networkxml2firewalltest.c | 2 +-
> 13 files changed, 119 insertions(+), 20 deletions(-)
> diff --git a/src/network/bridge_driver_conf.c b/src/network/bridge_driver_conf.c
> index a2edafa837..25cbbf8933 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)
Not visible, but here we are about to do 'return 0'.
This means that if the config file doesn't exist at all, then
none of this method below will run.
The logic for detecting the "best" firewall backend in the
"} else {" clause below will thus never run. So without a
config file on disk, it will always implicitly get the
iptables backend set.
We should set all defaults upfront, which means detectnig
nft vs iptables. Then set an override later when reading
the config, if it exists.
> @@ -73,6 +75,54 @@ 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));
> +
> + } 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;
> + bool binaryFound = false;
> +
> + /* virFindFileInPath() uses g_find_program_in_path(),
> + * which allows absolute paths, and verifies that
> + * the file is executable.
> + */
> +#if defined(FIREWALL_BACKEND_DEFAULT_IPTABLES)
Do we need this check ? It should always be set, and if not, it'll
be a compile error anyway.
> + if ((iptablesInPath = virFindFileInPath(IPTABLES))) {
> + cfg->firewallBackend = VIR_FIREWALL_BACKEND_IPTABLES;
> + binaryFound = true;
> + }
> +#else
> +# error "No usable firewall_backend was set in build options"
> +#endif
> +
> + if (!binaryFound) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("firewall_backend not set, and no usable backend auto-detected"));
> + return -1;
> + }
> +
> + VIR_INFO("using auto-detected firewall_backend: '%s'",
> + virFirewallBackendTypeToString(cfg->firewallBackend));
> + }
> +
> return 0;
> }
>
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
