On Tue, Apr 30, 2024 at 01:44:01PM -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>
I didn't give a R-B for this patch, and this still
has the problem I pointed out in v3, where if the
network.conf does not exist on disk at all, the
backend detction logic doesn't run.
> ---
> 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/meson.build b/meson.build
> index 1518afa1cb..02340c09dd 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1618,6 +1618,11 @@ endif
>
> if not get_option('driver_network').disabled() and conf.has('WITH_LIBVIRTD')
> conf.set('WITH_NETWORK', 1)
> + firewall_backend = get_option('firewall_backend')
> + conf.set_quoted('FIREWALL_BACKEND', firewall_backend)
> + if firewall_backend == 'iptables'
> + conf.set('FIREWALL_BACKEND_DEFAULT_IPTABLES', 1)
> + endif
> elif get_option('driver_network').enabled()
> error('libvirtd must be enabled to build the network driver')
> endif
> diff --git a/meson_options.txt b/meson_options.txt
> index ed91d97abf..367629f5dc 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -98,6 +98,7 @@ option('chrdev_lock_files', type: 'string', value: '', description: 'location fo
> option('dtrace', type: 'feature', value: 'auto', description: 'use dtrace for static probing')
> option('firewalld', type: 'feature', value: 'auto', description: 'firewalld support')
> option('firewalld_zone', type: 'feature', value: 'auto', description: 'whether to install firewalld libvirt zone')
> +option('firewall_backend', type: 'string', value: 'iptables', description: 'which firewall backend to use by default when none is specified')
> option('host_validate', type: 'feature', value: 'auto', description: 'build virt-host-validate')
> option('init_script', type: 'combo', choices: ['systemd', 'openrc', 'check', 'none'], value: 'check', description: 'Style of init script to install')
> option('loader_nvram', type: 'string', value: '', description: 'Pass list of pairs of <loader>:<nvram> paths. Both pairs and list items are separated by a colon.')
> 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
> @@ -1682,6 +1682,7 @@ static int
> networkReloadFirewallRulesHelper(virNetworkObj *obj,
> void *opaque G_GNUC_UNUSED)
> {
> + g_autoptr(virNetworkDriverConfig) cfg = virNetworkDriverGetConfig(networkGetDriver());
> VIR_LOCK_GUARD lock = virObjectLockGuard(obj);
> virNetworkDef *def = virNetworkObjGetDef(obj);
>
> @@ -1695,8 +1696,8 @@ networkReloadFirewallRulesHelper(virNetworkObj *obj,
> * network type, forward='open', doesn't need this because it
> * has no iptables rules.
> */
> - networkRemoveFirewallRules(def);
> - ignore_value(networkAddFirewallRules(def));
> + networkRemoveFirewallRules(def, cfg->firewallBackend);
> + ignore_value(networkAddFirewallRules(def, cfg->firewallBackend));
> break;
>
> case VIR_NETWORK_FORWARD_OPEN:
> @@ -1948,7 +1949,7 @@ networkStartNetworkVirtual(virNetworkDriverState *driver,
>
> /* Add "once per network" rules */
> if (def->forward.type != VIR_NETWORK_FORWARD_OPEN &&
> - networkAddFirewallRules(def) < 0)
> + networkAddFirewallRules(def, cfg->firewallBackend) < 0)
> goto error;
>
> firewalRulesAdded = true;
> @@ -2064,7 +2065,7 @@ networkStartNetworkVirtual(virNetworkDriverState *driver,
>
> if (firewalRulesAdded &&
> def->forward.type != VIR_NETWORK_FORWARD_OPEN)
> - networkRemoveFirewallRules(def);
> + networkRemoveFirewallRules(def, cfg->firewallBackend);
>
> virNetworkObjUnrefMacMap(obj);
>
> @@ -2076,7 +2077,8 @@ networkStartNetworkVirtual(virNetworkDriverState *driver,
>
>
> static int
> -networkShutdownNetworkVirtual(virNetworkObj *obj)
> +networkShutdownNetworkVirtual(virNetworkObj *obj,
> + virNetworkDriverConfig *cfg)
> {
> virNetworkDef *def = virNetworkObjGetDef(obj);
> pid_t dnsmasqPid;
> @@ -2102,7 +2104,7 @@ networkShutdownNetworkVirtual(virNetworkObj *obj)
> ignore_value(virNetDevSetOnline(def->bridge, false));
>
> if (def->forward.type != VIR_NETWORK_FORWARD_OPEN)
> - networkRemoveFirewallRules(def);
> + networkRemoveFirewallRules(def, cfg->firewallBackend);
>
> ignore_value(virNetDevBridgeDelete(def->bridge));
>
> @@ -2406,7 +2408,7 @@ networkShutdownNetwork(virNetworkDriverState *driver,
> case VIR_NETWORK_FORWARD_NAT:
> case VIR_NETWORK_FORWARD_ROUTE:
> case VIR_NETWORK_FORWARD_OPEN:
> - ret = networkShutdownNetworkVirtual(obj);
> + ret = networkShutdownNetworkVirtual(obj, cfg);
> break;
>
> case VIR_NETWORK_FORWARD_BRIDGE:
> @@ -3257,7 +3259,7 @@ networkUpdate(virNetworkPtr net,
> * old rules (and remember to load new ones after the
> * update).
> */
> - networkRemoveFirewallRules(def);
> + networkRemoveFirewallRules(def, cfg->firewallBackend);
> needFirewallRefresh = true;
> break;
> default:
> @@ -3285,14 +3287,14 @@ networkUpdate(virNetworkPtr net,
> parentIndex, xml,
> network_driver->xmlopt, flags) < 0) {
> if (needFirewallRefresh)
> - ignore_value(networkAddFirewallRules(def));
> + ignore_value(networkAddFirewallRules(def, cfg->firewallBackend));
> goto cleanup;
> }
>
> /* @def is replaced */
> def = virNetworkObjGetDef(obj);
>
> - if (needFirewallRefresh && networkAddFirewallRules(def) < 0)
> + if (needFirewallRefresh && networkAddFirewallRules(def, cfg->firewallBackend) < 0)
> goto cleanup;
>
> if (flags & VIR_NETWORK_UPDATE_AFFECT_CONFIG) {
> 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)
> @@ -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)
> + 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;
> }
>
> diff --git a/src/network/bridge_driver_conf.h b/src/network/bridge_driver_conf.h
> index 426c16198d..8f221f391e 100644
> --- a/src/network/bridge_driver_conf.h
> +++ b/src/network/bridge_driver_conf.h
> @@ -26,6 +26,7 @@
> #include "virdnsmasq.h"
> #include "virnetworkobj.h"
> #include "object_event.h"
> +#include "virfirewall.h"
>
> typedef struct _virNetworkDriverConfig virNetworkDriverConfig;
> struct _virNetworkDriverConfig {
> @@ -37,6 +38,8 @@ struct _virNetworkDriverConfig {
> char *stateDir;
> char *pidDir;
> char *dnsmasqStateDir;
> +
> + virFirewallBackend firewallBackend;
> };
>
> G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNetworkDriverConfig, virObjectUnref);
> diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c
> index 4914d5c903..c2ef27f251 100644
> --- a/src/network/bridge_driver_linux.c
> +++ b/src/network/bridge_driver_linux.c
> @@ -303,7 +303,8 @@ int networkCheckRouteCollision(virNetworkDef *def)
>
>
> int
> -networkAddFirewallRules(virNetworkDef *def)
> +networkAddFirewallRules(virNetworkDef *def,
> + virFirewallBackend firewallBackend G_GNUC_UNUSED)
> {
> if (virOnce(&createdOnce, networkSetupPrivateChains) < 0)
> return -1;
> @@ -394,7 +395,8 @@ networkAddFirewallRules(virNetworkDef *def)
>
>
> void
> -networkRemoveFirewallRules(virNetworkDef *def)
> +networkRemoveFirewallRules(virNetworkDef *def,
> + virFirewallBackend firewallBackend G_GNUC_UNUSED)
> {
> iptablesRemoveFirewallRules(def);
> }
> diff --git a/src/network/bridge_driver_nop.c b/src/network/bridge_driver_nop.c
> index 6eee6043e6..7d9a061e50 100644
> --- a/src/network/bridge_driver_nop.c
> +++ b/src/network/bridge_driver_nop.c
> @@ -36,11 +36,13 @@ int networkCheckRouteCollision(virNetworkDef *def G_GNUC_UNUSED)
> return 0;
> }
>
> -int networkAddFirewallRules(virNetworkDef *def G_GNUC_UNUSED)
> +int networkAddFirewallRules(virNetworkDef *def G_GNUC_UNUSED,
> + virFirewallBackend firewallBackend G_GNUC_UNUSED)
> {
> return 0;
> }
>
> -void networkRemoveFirewallRules(virNetworkDef *def G_GNUC_UNUSED)
> +void networkRemoveFirewallRules(virNetworkDef *def G_GNUC_UNUSED,
> + virFirewallBackend firewallBackend G_GNUC_UNUSED)
> {
> }
> diff --git a/src/network/bridge_driver_platform.h b/src/network/bridge_driver_platform.h
> index b720d343be..7443c3129f 100644
> --- a/src/network/bridge_driver_platform.h
> +++ b/src/network/bridge_driver_platform.h
> @@ -32,6 +32,8 @@ void networkPostReloadFirewallRules(bool startup);
>
> int networkCheckRouteCollision(virNetworkDef *def);
>
> -int networkAddFirewallRules(virNetworkDef *def);
> +int networkAddFirewallRules(virNetworkDef *def,
> + virFirewallBackend firewallBackend);
>
> -void networkRemoveFirewallRules(virNetworkDef *def);
> +void networkRemoveFirewallRules(virNetworkDef *def,
> + virFirewallBackend firewallBackend);
> diff --git a/src/network/libvirtd_network.aug b/src/network/libvirtd_network.aug
> index ae153d96a1..5d6d72dd92 100644
> --- a/src/network/libvirtd_network.aug
> +++ b/src/network/libvirtd_network.aug
> @@ -22,11 +22,14 @@ module Libvirtd_network =
> let int_entry (kw:string) = [ key kw . value_sep . int_val ]
> let str_array_entry (kw:string) = [ key kw . value_sep . str_array_val ]
>
> + let firewall_backend_entry = str_entry "firewall_backend"
> +
> (* Each entry in the config is one of the following *)
> + let entry = firewall_backend_entry
> let comment = [ label "#comment" . del /#[ \t]*/ "# " . store /([^ \t\n][^\n]*)?/ . del /\n/ "\n" ]
> let empty = [ label "#empty" . eol ]
>
> - let record = indent . eol
> + let record = indent . entry . eol
>
> let lns = ( record | comment | empty ) *
>
> diff --git a/src/network/meson.build b/src/network/meson.build
> index 0336435862..c79a36da22 100644
> --- a/src/network/meson.build
> +++ b/src/network/meson.build
> @@ -49,16 +49,34 @@ if conf.has('WITH_NETWORK')
> ],
> }
>
> + network_options_conf = configuration_data({
> + 'FIREWALL_BACKEND': firewall_backend,
> + })
> +
> network_conf = configure_file(
> input: 'network.conf.in',
> output: 'network.conf',
> - configuration: configmake_conf,
> + configuration: network_options_conf,
> )
> +
> + network_options_hack_conf = configuration_data({
> + 'FIREWALL_BACKEND': firewall_backend,
> + # This hack is necessary because the output file is going to be
> + # used as input for another configure_file() call later, which
> + # will take care of substituting @CONFIG@ with useful data
> + 'CONFIG': '@CONFIG@',
> + })
> + test_libvirtd_network_aug_tmp = configure_file(
> + input: 'test_libvirtd_network.aug.in',
> + output: 'test_libvirtd_network.aug.tmp',
> + configuration: network_options_hack_conf,
> + )
> +
> virt_conf_files += network_conf
> virt_aug_files += files('libvirtd_network.aug')
> virt_test_aug_files += {
> 'name': 'test_libvirtd_network.aug',
> - 'aug': files('test_libvirtd_network.aug.in'),
> + 'aug': test_libvirtd_network_aug_tmp,
> 'conf': network_conf,
> 'test_name': 'libvirtd_network',
> 'test_srcdir': meson.current_source_dir(),
> diff --git a/src/network/network.conf.in b/src/network/network.conf.in
> index 5c84003f6d..ec75e125d8 100644
> --- a/src/network/network.conf.in
> +++ b/src/network/network.conf.in
> @@ -1,3 +1,11 @@
> # Master configuration file for the network driver.
> # All settings described here are optional - if omitted, sensible
> # defaults are used.
> +
> +# firewall_backend:
> +#
> +# determines which subsystem to use to setup firewall packet
> +# filtering rules for virtual networks. Currently the only supported
> +# selection is "iptables".
> +#
> +#firewall_backend = "@FIREWALL_BACKEND@"
> diff --git a/src/network/test_libvirtd_network.aug.in b/src/network/test_libvirtd_network.aug.in
> index ffdca520ce..9e29a9192f 100644
> --- a/src/network/test_libvirtd_network.aug.in
> +++ b/src/network/test_libvirtd_network.aug.in
> @@ -1,2 +1,5 @@
> module Test_libvirtd_network =
> @CONFIG@
> +
> + test Libvirtd_network.lns get conf =
> +{ "firewall_backend" = "@FIREWALL_BACKEND@" }
> diff --git a/tests/networkxml2firewalltest.c b/tests/networkxml2firewalltest.c
> index cb66a26294..3a9f409e2a 100644
> --- a/tests/networkxml2firewalltest.c
> +++ b/tests/networkxml2firewalltest.c
> @@ -98,7 +98,7 @@ static int testCompareXMLToArgvFiles(const char *xml,
> if (!(def = virNetworkDefParse(NULL, xml, NULL, false)))
> return -1;
>
> - if (networkAddFirewallRules(def) < 0)
> + if (networkAddFirewallRules(def, VIR_FIREWALL_BACKEND_IPTABLES) < 0)
> return -1;
>
> actual = actualargv = virBufferContentAndReset(&buf);
> --
> 2.44.0
> _______________________________________________
> Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
> To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
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
