12:04 p.m.
On 4/25/24 12:30 PM, Daniel P. Berrangé wrote:
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(a)redhat.com>
> Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com>
> ---
> 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.
oops. I hadn't noticed that since I always have a config file.
We should set all defaults upfront, which means detectnig
nft vs iptables. Then set an override later when reading
the config, if it exists.
Okay, how about this:
1) check for the existence of both backends and save that in a couple of
bools.
2) based on the the results of (1) plus meson_options.txt (or meson
commandline) setting of firewall_backend, do a preliminary setting of
backend (or fail if neither is found).
3) If the config file has a firewall_backend setting, look at the
results of (1) to make sure it's available, and set that (if it's *not*
available, do we fail? Or ignore? I'm inclined to say fail)
> @@ -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.
The way the logic in meson.build works, it looks at the setting of
firewall_backend, and if that is "iptables" then it adds
#define FIREWALL_BACKEND_DEFAULT_IPTABLES
to meson-config.h, or if it's set to "nftables" then it adds
#define FIREWALL_BACKEND_DEFAULT_NFTABLES
(the name in the #elif should have been ...NFTABLES, as I discovered
last night right after sending the patch mails - I hadn't received back
any of the mails to be able reply to myself then, so it had to wait
until this morning (I was already way too late getting to bed, so didn't
want to wait around any longer :-))
This all feels rather "hacky" (both because the
FIREWALL_BACKEND_DEFAULT_* constants are derived from the string setting
of firewall_backend, and also because the conditionally compiled code is
(short but) duplicated. But it was the simplest way I could see to get
conditional compilation of the order of the checks rather than needing
to do a string compare of the constant FIREWALL_BACKEND at runtime
(since C preprocessor expressions can't do string compares). If someone
wants to point out the obvious simple solution that I'm refusing to see,
I'd be happy to do that instead! (I don't want to require the person
doing the build to have to manually set more than a single option, and
the name of the backend seems like the simplest)
> + 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