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 :-))
+ 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@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
