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@redhat.com>
---
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
@@ -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..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));
+
+ } 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)
+ 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;
}
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/network.conf b/src/network/network.conf
index 5c84003f6d..74c79e4cc6 100644
--- a/src/network/network.conf
+++ b/src/network/network.conf
@@ -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 = "iptables"
diff --git a/src/network/test_libvirtd_network.aug.in b/src/network/test_libvirtd_network.aug.in
index ffdca520ce..3aa7b4cc22 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" = "iptables" }
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@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
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@redhat.com>
> ---
> 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
--
|: 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@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
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@redhat.com>
>> ---
>> 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 :-))
>
>> + 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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
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@redhat.com>
> > > ---
> > > 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@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
© 2016 - 2026 Red Hat, Inc.