[PATCH 0/3] ci: update the CI dashboard
by Daniel P. Berrangé
Daniel P. Berrangé (3):
docs: trim many projects from CI dashboard
docs: fix link to virttools-web pipeline status
docs: add libosinfo & virt-viewer to CI dashboard
docs/ci-dashboard.rst | 78 ++++++++++++++++++-------------------------
1 file changed, 32 insertions(+), 46 deletions(-)
--
2.45.1
5 months, 1 week
[PATCH] gitlab: add missing job inheritance for codestyle
by Daniel P. Berrangé
The previous fix:
commit b069efe29c950d1a45e88ef7dc924d3ee223103a
Author: Daniel P. Berrangé <berrange(a)redhat.com>
Date: Fri Jun 14 19:57:06 2024 +0100
gitlab: fix codestyle CI job
was incomplete, as the job inheritance was also
broken.
Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com>
---
.gitlab-ci.yml | 1 +
1 file changed, 1 insertion(+)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 635e731f82..6de867e1f2 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -106,6 +106,7 @@ pages:
codestyle_job:
stage: sanity_checks
+ extends: .gitlab_native_build_job
needs:
- job: x86_64-opensuse-leap-15-container
optional: true
--
2.45.1
5 months, 1 week
[PATCH 0/2] Make affinity setting a bit more debug friendly
by Michal Privoznik
*** BLURB HERE ***
Michal Prívozník (2):
qemu_process: Issue an info message when subtracting isolcpus
virprocess: Debug affinity map in virProcessSetAffinity()
src/qemu/qemu_process.c | 6 ++++++
src/util/virprocess.c | 6 ++++--
2 files changed, 10 insertions(+), 2 deletions(-)
--
2.43.2
5 months, 1 week
[PATCH] gitlab: fix codestyle CI job
by Daniel P. Berrangé
Jobs whose names start with a '.' as treated as templates, so
not actually run in a pipeline.
Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com>
---
.gitlab-ci.yml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index d9d8b1e3cd..635e731f82 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -104,7 +104,7 @@ pages:
paths:
- public
-.codestyle_job:
+codestyle_job:
stage: sanity_checks
needs:
- job: x86_64-opensuse-leap-15-container
--
2.45.1
5 months, 1 week
Re: [libvirt PATCH 00/28] native support for nftables in virtual
network driver
by Roman Bogorodskiy
Laine Stump wrote:
> On 6/10/24 2:54 PM, Roman Bogorodskiy wrote:
> > Laine Stump wrote:
> >
> > > This patch series enables libvirt to use nftables rules rather than
> > > iptables *when setting up virtual networks* (it does *not* add
> > > nftables support to the nwfilter driver). It accomplishes this by
> > > abstracting several iptables functions (from viriptables.[ch] called
> > > by the virtual network driver into a rudimentary "virNetfilter API"
> > > (in virnetfilter.[ch], having the virtual network driver call the
> > > virNetFilter API rather than calling the existing iptables functions
> > > directly, and then finally adding an equivalent virNftables backend
> > > that can be used instead of iptables (selected manually via a
> > > network.conf setting, or automatically if iptables isn't found on the
> > > host).
> >
> > Hi,
> >
> > Apparently, I'm late to the discussion.
> >
> > I noticed that now I cannot use the bridge driver on FreeBSD as it's
> > failing to initialize both iptables and nftables backends (which is
> > expect).
>
> Yeah, previously we wouldn't check if iptables was available until someone
> tried to start a network that would need to use it, and would then log an
> error (and just fail starting that network, but the network driver would
> remain running). But now we figure out which firewall backend to use
> immediately when the driver is loaded, and if we fail to fin a workable
> backend we fail the entire driver init.r
>
> How did you use the network driver before? With a <forward mode='open'/>
> network? Truthfully I hadn't ever considered the case of someone using it
> with only network types that didn't need firewall rules. I wonder if there
> are other platforms we support that have a usable network driver for
> <forward mode='open'/> (MacOS?)
I'm using it with the following network configuration:
virsh # net-dumpxml default
<network>
<name>default</name>
<uuid>2a1415c9-325b-41e4-82c6-e805162d8934</uuid>
<forward mode='nat'/>
<bridge name='virbr0' stp='on' delay='0'/>
<mac address='52:54:00:24:fa:43'/>
<ip address='192.168.122.1' netmask='255.255.255.0'>
<dhcp>
<range start='192.168.122.2' end='192.168.122.254'/>
</dhcp>
</ip>
</network>
So basically all the mechanics like creating tap devices, bridges,
serving dhcp, etc, all these work for me. On top of that I had a few
iterations of manual firewall configurations (with both ipfw and pf)
to implement NAT on guests.
Unfortunately, I don't have access to that setup anymore and I haven't
re-created it yet. IIRC, it could probably show some warnings about
missing iptables, but it didn't affect anything for me.
> >
> > What would be a good way to address that? I see at least two options:
> >
> > 1. Add a Noop firewall driver
> > 2. Implement a "real" FreeBSD driver based either on pf or ipfw (that's
> > been on my TODO list forever, but I somehow got stuck on the very first
> > step on choosing between pf and ipfw).
>
> Why not both? :-)
>
> > This obviously will take much
> > more time.
> >
> > Maybe there are other options I'm missing.
>
> Obviously (2) would be nicest, but I guess in the short term some variation
> of (1) would be quicker.
>
> Another possibility could be to restore the old behavior of saving the error
> and only reporting it when a network requiring a firewall is loaded, but I
> think I remember a discussion about this during review of an earlier
> revision of the patches, and we agreed that it made the problem easier to
> find if it was reported immediately and the driver load failed.
>
> I suppose in the long run the build-time option firewall_backend_priority
> should be used to control which backends are included in the build (rather
> than just which ones are checked at runtime), so that FreeBSD could
> completely skip all the iptables and nftables code (and firewalld when
> that's done), and Linux platforms could skip pf and ipfw.
>
> >
> > What do you think?
>
> I'm about to be offline for 3 weeks, but in the meantime if you'd like to
> try making a NULL backend that is only an option if it's listed in
> firewall_backend_priority (you'll need to remove the compile-time check that
> all possible backends are accounted for - I think that is the first of the
> two G_STATIC_ASSERTS at the top of virNetworkLoadDriverConfig()), always
> initializes successfully in bridge_driver_conf.c if it is listed in the
> options, and then in networkAddFirewallRules add a check to log an error and
> fail if backend == NULL (something about attempting to start a network type
> that would require firewall rules, but the system not having any of the
> supported types of firewallbackend or something - it's too late now and my
> brain is too fried and sleepy to think of good wording :-)). As long as it
> isn't a valid selection on Linux builds that are done with
> firewall_backend_priority=nftables,iptables, but *is* a valid selection if
> the setting is "firewall_backend_priority=null" that shouldn't be *too*
> controversial.
Ok, I think I can try making the NULL backend.
> Later we can talk about pf and ipfw backends :-)
Yeah, that sounds good. My main problem with the choice is that ipfw is
the most actively supported firewall, but it relies quite heavily on the
rule numbering, which makes it a little hard to integrate with
user-specific rules (i.e. defined outside of libvirt). The "pf" seem to
be better in this regard (at least to my taste), but it's not "native"
FreeBSD firewall and is not as active (at least, to my impression).
Roman
5 months, 1 week
[PATCH 0/3] Couple of memleak fixes
by Michal Privoznik
The third patch MIGHT fix the following issue:
https://issues.redhat.com/browse/RHEL-22574
but at this point it's still unclear. I'll append appropriate
'Resolves:' line when I learn more.
Michal Prívozník (3):
virfirewall: Fir a memleak in virFirewallParseXML()
virnetworkobj: Free fwRemoval before setting another one in
virNetworkObjSetFwRemoval()
remote_daemon_dispatch: Unref sasl session when closing client
connection
src/conf/virnetworkobj.c | 1 +
src/remote/remote_daemon_dispatch.c | 4 ++++
src/util/virfirewall.c | 2 +-
3 files changed, 6 insertions(+), 1 deletion(-)
--
2.44.2
5 months, 1 week
[PATCH] network: introduce a "none" firewall backend type
by Daniel P. Berrangé
There are two scenarios identified after the recent firewall backend
selection was introduced, which result in libvirtd failing to startup
due to an inability to find either iptables/nftables
- On Linux if running unprivileged with $PATH lacking the dir
containing iptables/nftables
- On non-Linux where iptables/nftables never existed
In the former case, it is preferrable to restore the behaviour whereby
the driver starts successfully. Users will get an error reported when
attempting to start any virtual network, due to the lack of permissions
needed to create bridge devices. This makes the missing firewall backend
irrelevant.
In the latter case, the network driver calls the 'nop' platform
implementation which does not attempt to implement any firewall logic,
just allowing the network to start without firewall rules.
To solve this are number of changes are required
* Introduce VIR_FIREWALL_BACKEND_NONE, which does nothing except
report a fatal error from virFirewallApply(). This code path
is unreachable, since we'll never create a virFirewall
object with with VIR_FIREWALL_BACKEND_NONE, so the error reporting
is just a sanity check.
* Ignore the compile time backend defaults and assume use of
the 'none' backend if running unprivileged.
This fixes the first regression, avoiding the failure to start
libvirtd on Linux in unprivileged context, instead allowing use
of the driver and expecting a permission denied when creating a
bridge.
* Reject the use of compile time backend defaults no non-Linux
and hardcode the 'none' backend. The non-Linux platforms have
no firewall implementation at all currently, so there's no
reason to permit the use of 'firewall_backend_priority'
meson option.
This fixes the second regression, avoiding the failure to start
libvirtd on non-Linux hosts due to non-existant Linux binaries.
* Change the Linux platform backend to raise an error if the
firewall backend is 'none'. Again this code path is unreachable
by default since we'll fail to create the bridge before getting
here, but if someone modified network.conf to request the 'none'
backend, this will stop further progress.
* Change the nop platform backend to raise an error if the
firewall backend is 'iptables' or 'nftables'. Again this code
path is unreachable, since we should already have failed to
find the iptables/nftables binaries on non-Linux hosts, so
this is just a sanity check.
* 'none' is not permited as a value in 'firewall_backend_priority'
meson option, since it is conceptually meaningless to ask for
that on Linux.
Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com>
---
meson.build | 26 +++++++++++++++++++-------
meson_options.txt | 2 +-
src/network/bridge_driver_conf.c | 19 ++++++++++++++-----
src/network/bridge_driver_linux.c | 10 ++++++++++
src/network/bridge_driver_nop.c | 13 ++++++++++++-
src/util/virfirewall.c | 6 ++++++
src/util/virfirewall.h | 1 +
7 files changed, 63 insertions(+), 14 deletions(-)
diff --git a/meson.build b/meson.build
index 5c7cd7ec2e..2e8b87280d 100644
--- a/meson.build
+++ b/meson.build
@@ -1647,15 +1647,27 @@ if not get_option('driver_network').disabled() and conf.has('WITH_LIBVIRTD')
conf.set('WITH_NETWORK', 1)
firewall_backend_priority = get_option('firewall_backend_priority')
- if (not firewall_backend_priority.contains('nftables') or
- not firewall_backend_priority.contains('iptables') or
- firewall_backend_priority.length() != 2)
- error('invalid value for firewall_backend_priority option')
+ if firewall_backend_priority.length() == 0
+ if host_machine.system() == 'linux'
+ firewall_backend_priority = ['nftables', 'iptables']
+ else
+ # No firewall impl on non-Linux so far, so force 'none'
+ # as placeholder
+ firewall_backend_priority = ['none']
+ endif
+ else
+ if host_machine.system() != 'linux'
+ error('firewall backend priority only supported on linux hosts')
+ endif
endif
- conf.set('FIREWALL_BACKEND_PRIORITY_0', 'VIR_FIREWALL_BACKEND_' + firewall_backend_priority[0].to_upper())
- conf.set('FIREWALL_BACKEND_PRIORITY_1', 'VIR_FIREWALL_BACKEND_' + firewall_backend_priority[1].to_upper())
- conf.set('FIREWALL_BACKEND_PRIORITY_NUM', firewall_backend_priority.length())
+ backends = []
+ foreach backend: firewall_backend_priority
+ backend = 'VIR_FIREWALL_BACKEND_' + backend.to_upper()
+ backends += backend
+ endforeach
+
+ conf.set('FIREWALL_BACKENDS', ', '.join(backends))
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 50d71427cb..2d440c63d8 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -117,7 +117,7 @@ option('dtrace', type: 'feature', value: 'auto', description: 'use dtrace for st
option('firewalld', type: 'feature', value: 'auto', description: 'firewalld support')
# dep:firewalld
option('firewalld_zone', type: 'feature', value: 'auto', description: 'whether to install firewalld libvirt zone')
-option('firewall_backend_priority', type: 'array', choices: ['nftables', 'iptables'], description: 'order in which to try firewall backends')
+option('firewall_backend_priority', type: 'array', choices: ['nftables', 'iptables'], value: [], description: 'order in which to try firewall backends')
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_conf.c b/src/network/bridge_driver_conf.c
index e2f3613a41..9da5e790b7 100644
--- a/src/network/bridge_driver_conf.c
+++ b/src/network/bridge_driver_conf.c
@@ -61,6 +61,7 @@ networkGetDnsmasqCaps(virNetworkDriverState *driver)
static int
virNetworkLoadDriverConfig(virNetworkDriverConfig *cfg G_GNUC_UNUSED,
+ bool privileged,
const char *filename)
{
g_autoptr(virConf) conf = NULL;
@@ -68,13 +69,17 @@ virNetworkLoadDriverConfig(virNetworkDriverConfig *cfg G_GNUC_UNUSED,
bool fwBackendSelected = false;
size_t i;
int fwBackends[] = {
- FIREWALL_BACKEND_PRIORITY_0,
- FIREWALL_BACKEND_PRIORITY_1,
+ FIREWALL_BACKENDS
};
- G_STATIC_ASSERT(G_N_ELEMENTS(fwBackends) == VIR_FIREWALL_BACKEND_LAST);
- G_STATIC_ASSERT(G_N_ELEMENTS(fwBackends) == FIREWALL_BACKEND_PRIORITY_NUM);
+ G_STATIC_ASSERT(G_N_ELEMENTS(fwBackends) > 0 &&
+ G_N_ELEMENTS(fwBackends) <= VIR_FIREWALL_BACKEND_LAST);
int nFwBackends = G_N_ELEMENTS(fwBackends);
+ if (!privileged) {
+ fwBackends[0] = VIR_FIREWALL_BACKEND_NONE;
+ nFwBackends = 1;
+ }
+
if (access(filename, R_OK) == 0) {
conf = virConfReadFile(filename, 0);
@@ -104,6 +109,10 @@ virNetworkLoadDriverConfig(virNetworkDriverConfig *cfg G_GNUC_UNUSED,
for (i = 0; i < nFwBackends && !fwBackendSelected; i++) {
switch ((virFirewallBackend)fwBackends[i]) {
+ case VIR_FIREWALL_BACKEND_NONE:
+ fwBackendSelected = true;
+ break;
+
case VIR_FIREWALL_BACKEND_IPTABLES: {
g_autofree char *iptablesInPath = virFindFileInPath(IPTABLES);
@@ -187,7 +196,7 @@ virNetworkDriverConfigNew(bool privileged)
configfile = g_strconcat(configdir, "/network.conf", NULL);
- if (virNetworkLoadDriverConfig(cfg, configfile) < 0)
+ if (virNetworkLoadDriverConfig(cfg, privileged, configfile) < 0)
return NULL;
if (g_mkdir_with_parents(cfg->stateDir, 0777) < 0) {
diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c
index 35e6bd1154..fe7c6e193c 100644
--- a/src/network/bridge_driver_linux.c
+++ b/src/network/bridge_driver_linux.c
@@ -47,6 +47,11 @@ networkFirewallSetupPrivateChains(virFirewallBackend backend,
virFirewallLayer layer)
{
switch (backend) {
+ case VIR_FIREWALL_BACKEND_NONE:
+ virReportError(VIR_ERR_NO_SUPPORT, "%s",
+ _("No firewall backend is available"));
+ return -1;
+
case VIR_FIREWALL_BACKEND_IPTABLES:
return iptablesSetupPrivateChains(layer);
@@ -417,6 +422,11 @@ networkAddFirewallRules(virNetworkDef *def,
}
switch (firewallBackend) {
+ case VIR_FIREWALL_BACKEND_NONE:
+ virReportError(VIR_ERR_NO_SUPPORT, "%s",
+ _("No firewall backend is available"));
+ return -1;
+
case VIR_FIREWALL_BACKEND_IPTABLES:
return iptablesAddFirewallRules(def, fwRemoval);
diff --git a/src/network/bridge_driver_nop.c b/src/network/bridge_driver_nop.c
index 537b9234f8..7797be1ba8 100644
--- a/src/network/bridge_driver_nop.c
+++ b/src/network/bridge_driver_nop.c
@@ -37,9 +37,20 @@ int networkCheckRouteCollision(virNetworkDef *def G_GNUC_UNUSED)
}
int networkAddFirewallRules(virNetworkDef *def G_GNUC_UNUSED,
- virFirewallBackend firewallBackend G_GNUC_UNUSED,
+ virFirewallBackend firewallBackend,
virFirewall **fwRemoval G_GNUC_UNUSED)
{
+ /*
+ * Shouldn't be possible, since virNetworkLoadDriverConfig
+ * ought to fail to find the required binaries when loading,
+ * so this is just a sanity check
+ */
+ if (firewallBackend != VIR_FIREWALL_NONE) {
+ virReportError(VIR_ERR_NO_SUPPORT, "%s",
+ _("Firewall backend '%s' not available on this platform"),
+ virFirewallBackendTypeToSTring(firewallBackend));
+ return -1;
+ }
return 0;
}
diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
index 2219506b18..d374f54b64 100644
--- a/src/util/virfirewall.c
+++ b/src/util/virfirewall.c
@@ -37,6 +37,7 @@ VIR_LOG_INIT("util.firewall");
VIR_ENUM_IMPL(virFirewallBackend,
VIR_FIREWALL_BACKEND_LAST,
+ "none",
"iptables",
"nftables");
@@ -815,6 +816,11 @@ virFirewallApplyCmd(virFirewall *firewall,
}
switch (virFirewallGetBackend(firewall)) {
+ case VIR_FIREWALL_BACKEND_NONE:
+ virReportError(VIR_ERR_NO_SUPPORT,
+ _("Firewall backend is not implemented"));
+ return -1;
+
case VIR_FIREWALL_BACKEND_IPTABLES:
if (virFirewallCmdIptablesApply(firewall, fwCmd, &output) < 0)
return -1;
diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h
index 302a6a4e5b..bce51259d2 100644
--- a/src/util/virfirewall.h
+++ b/src/util/virfirewall.h
@@ -44,6 +44,7 @@ typedef enum {
} virFirewallLayer;
typedef enum {
+ VIR_FIREWALL_BACKEND_NONE, /* Always fails */
VIR_FIREWALL_BACKEND_IPTABLES,
VIR_FIREWALL_BACKEND_NFTABLES,
--
2.45.1
5 months, 1 week
[PATCH 0/2] network: fix network driver to gracefully skip startup
by Daniel P. Berrangé
We should gracefully skip startup when:
* No network.conf firewall_backend is explicitly set, and
neither iptables/nftables are present
* Running unprivileged
The former fixes libvirtd startup on non-Linux, or minimal linux
installs without firewall tools.
The latter skips pointless initialization that creates a driver
that cannot do anything useful
Daniel P. Berrangé (2):
network: skip network driver init if no firewall backend is present
network: don't attempt to initialize if non-privileged
src/network/bridge_driver.c | 14 +++++++++++++-
src/network/bridge_driver_conf.c | 8 ++++----
2 files changed, 17 insertions(+), 5 deletions(-)
--
2.45.1
5 months, 1 week
[PATCH] tools: fix paths in PKI validation error messages
by Daniel P. Berrangé
A couple of paths passed in the error messages, didnt match the paths
that were actually being tested.
Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com>
---
tools/virt-pki-validate.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/virt-pki-validate.c b/tools/virt-pki-validate.c
index 656f29fdc5..e693ffaed6 100644
--- a/tools/virt-pki-validate.c
+++ b/tools/virt-pki-validate.c
@@ -184,7 +184,7 @@ virPKIValidateIdentity(bool isServer, bool system, const char *path)
_("Checking system cert dir access"),
0, 0, 0755,
_("The system cert dir %1$s must be accessible to all users. As root, run: chown root.root; chmod 0755 %2$s"),
- LIBVIRT_PKI_DIR, LIBVIRT_PKI_DIR);
+ LIBVIRT_CERT_DIR, LIBVIRT_CERT_DIR);
FILE_REQUIRE_EXISTS(scope,
LIBVIRT_KEY_DIR,
@@ -197,7 +197,7 @@ virPKIValidateIdentity(bool isServer, bool system, const char *path)
_("Checking system key dir access"),
0, 0, 0755,
_("The system key dir %1$s must be accessible to all users. As root, run: chown root.root; chmod 0755 %2$s"),
- LIBVIRT_KEY_DIR, LIBVIRT_PKI_DIR);
+ LIBVIRT_KEY_DIR, LIBVIRT_KEY_DIR);
} else if (path) {
virNetTLSConfigCustomTrust(path,
&cacert,
--
2.45.1
5 months, 1 week
[libvirt PATCH 00/28] native support for nftables in virtual network driver
by Laine Stump
This patch series enables libvirt to use nftables rules rather than
iptables *when setting up virtual networks* (it does *not* add
nftables support to the nwfilter driver). It accomplishes this by
abstracting several iptables functions (from viriptables.[ch] called
by the virtual network driver into a rudimentary "virNetfilter API"
(in virnetfilter.[ch], having the virtual network driver call the
virNetFilter API rather than calling the existing iptables functions
directly, and then finally adding an equivalent virNftables backend
that can be used instead of iptables (selected manually via a
network.conf setting, or automatically if iptables isn't found on the
host).
A first look at the result may have you thinking that it's filled with
a lot of bad decisions. While I would agree with that in many cases, I
think that overall they are the "least bad" decisions, or at least
"bad within acceptable limits / no worse than something else", and
point out that it's been done in a way that minimizes (actually
eliminates) the need for immediate changes to nwfilter (the other
consumer of iptables, which *also* needs to be updated to use native
nftables), and makes it much easier to change our mind about the
details in the future.
When I first started on this (long, protracted, repeatedly interrupted
for extended periods - many of these patches are > a year old) task, I
considered doing an all-at-once complete replacement of iptables with
nftables, since all the Linux distros we support have had nftables for
several years, and I'm pretty sure nobody has it disabled (not even
sure if it's possible to disable nftables while still enabling
iptables, since they both use xtables in the kernel). But due to
libvirt's use of "-t mangle -j CHECKSUM --checksum-fill" (see commit
fd5b15ff all the way back in July 2010 for details) which has no
equivalent in nftables rules (and we don't *want* it to!!), and the
desire to be able to easily switch back to iptables in case of an
unforeseen regression, we decided that both iptables and nftables need
to be supported (for now), with the default (for now) remaining as
iptables.
Just allowing for dual backends complicated matters, since it means
that we have to have a config file, a setting, detection of which
backends are available, and of course some sort of concept of an
abstracted frontend that can use either backend based on the config
setting (and/or auto-detection). Combining that with the fact that it
would just be "too big" of a project to switch over nwfilter's
iptables usage at the same time means that we have to keep around a
lot of existing code for compatibility's sake rather than just wiping
it all away and starting over.
So, what I've ended up with is:
1) a network.conf file (didn't exist before) with a single setting
"firewall_backend". If unset, the network driver tries to use iptables
on the backend, and if that's missing, then tries to use nftables.
2) a new (internal-only, so transient!) virNetFilterXXX API that is
used by the network driver in place of the iptablesXXX API, and calls
either iptablesXXX or:
3) a virNftablesXXX API that exactly replicates the filtering rules of
the existing iptablesXXX API (except in the custom "libvirt" base
table rather than the system "filter" and "nat" tables). This means
that:
4) when the nftables backend is used, the rules added are *exactly the
same* (functionally speaking) as we currently add for iptables (except
they are in the "libvirt" table).
We had spent some time in IRC discussing different ways of using new
functionality available in nftables to make a more
efficient/performant implemention of the desired filtering, and there
are some really great possibilities that need to be explored, but in
the end there were too many details up in the air, and I decided that
it would be more "accomplishable" (coined a new word there!) to first
replicate existing behavior with nftables, but do it inside a
framework that makes it easy to modify the details in the future (in
particular making it painless to switch back and forth between builds
with differing filter models at runtime) - this way we'll be able to
separate the infrastructure work from the details of the rules (which
we can then more easily work on and experiment with). (This implies
that the main objective right now is "get rid of iptables
dependencies", not "make the filtering faster and more efficient").
Notable features of this patchset:
* allows switching between iptables/nftables backends without
rebooting or restarting networks/guests.
Because the commands required to remove a network's filter rules are
now saved in the network status XML, each time libvirtd (or
virtnetworkd) is restarted, it will execute exactly the commands
needed to remove the filter rules that had been added by the
previous libvirtd/virtnetworkd (rather than just making a guess, as
we've always done up until now), and then add new rules using the
current backend+binary's set of rules (while also saving the info
needed for future removal of these new rules back into the network's
status XML).
* firewall_backend can be explicitly set in (new)
/etc/libvirt/network.conf, but if it's not explicitly set, libvirt
will default to the iptables backend if the iptables binary is
found, and otherwise fall back to nftables as long as the nft
binary is found; otherwise the first attempt to start a network will
fail with an appropriate error.
Things that seem ugly / that I would like to clean up / that I think
are just fine as they are:
* virFirewall does *not* provide a backend-agnostic interface [this is fine]
* We need to maintain a backward-compatible API for virFirewall so
that we don't have to touch nwfilter code. Trying to make its API
backend-agnostic would require individually considering/changing
every nwfilter use of virFirewall.
* instead virFirewall objects are just a way to build a collection
of commands to execute to build a firewall, then execute them
while collecting info for and building a collection of commands
that will tear down that firewall in the future.
Do I want to "fix" this in the future by making virFirewall a higher
level interface that accepts tokens describing the type of rule to
add (rather than backend-specific arguments to a backend-specific
command)? No. I think I like the way virFirewall works (as
described in that previous bullet-point), instead I'm thinking that
it is just slightly mis-named - I've lately been thinking of it as a
"virNetFilterCmdList". Similarly, the virFirewallRules that it has a
list of aren't really "rules", they are better described as commands
or actions, so maybe they should be renamed to virNetfilterCmd or
virNetfilterAction. But that is just cosmetic, so I didn't want to
get into it in these patches (especially in case someone disagrees,
or has a better idea for naming).
* Speaking of renaming - I should probably rename all the
"iptablesXXX" functions to "virIptablesXXX" to be consistent with so
much of our other code. I lost the ambition to deal with it right
now though, so I'm leaving that for later cleanup (or I could do it
now if it really makes someone's day :-).
* I could have chosen a higher place in the callchain to make the
virNetfilter abstraction, e.g. at the level of
"networkAddXXXFirewallRules()" rather than at the lower level of
iptablesXXX(). That is actually probably what will happen in the
future (since it will be necessary in order for an nftables-based
firewall to be significantly different in structure from an
iptables-based firewall). But that's the beauty of an API being
private - we can freely add/remove things as needed. the important
thing is that we now have the basic structure there.
For now, the split is just above the existing iptablesXXX API
(util/viriptables.[ch], which seems like a "narrow" enough
place. Most iptablesXXX functions are written in terms of just 10
*other* iptablesXXX functions that add iptables-specific commands -
I've just moved those functions into virnetfilter.[ch]
(appropriately renamed), and changed them to call the 10
virNetfilterXXX functions that will in-turn call those 10
iptablesXXX (or equivalent virNftablesXXX) functions.
* Some people may dislike that the 10 virNetfilterXXX functions are
each written with a switch statement that has cases to directly call
each backend, rather than each backend driver having a table of
pointers to API functions, with the virNetfilter API function
calling backends[fwBackend]->XXX() (ie the pattern for so many
drivers in libvirt). But for just 2 backends, that really seemed
like overkill and unnecessary obfuscation.
* As implemented here, I am storing a "<fwRemoval>" element in the
network status XML - it contains a serialized virFirewall object
that directly contains the commands necessary to remove the
firewall. I could instead just store "<firewall>", which would
include all the commands that were used to *create* the firewall in
addition to the commands needed to remove the firewall. The way it's
done currently takes up less space; switching to storing the full
firewall *might* be more informative to somebody, but on the other
hand would make the network status XML *very* long. If anybody has
an opinion about this, now is the time to bring it up - do you think
it's worth having a separate list of all the commands that were used
to create a network's firewall (keeping in mind that there is no
public API to access it)? Or is it enough to just store what's
needed to remove the firewall?
* Several months ago Eric Garver posted patches for a pure firewalld
backend, and I requested that they not be pushed because I wanted
that to be integrated with my nftables backend support. Due to the
fact that the firewalld backend is almost entirely implemented by
putting the bridge into a new firewalld "zone", with no individual
rules added, that won't happen as just another backend driver file
in parallel to iptables and nftables; it will instead work by
checking firewall_backend at a higher level in the network driver,
thus avoiding the calls to virNetfilterXXX() entirely. I have
locally merged Eric's patches over the top of these patches, and
there are surprisingly few conflicts, but since his patches didn't
account for a user-settable config (but instead just always used the
firewalld backend if firewalld was active), some of the patches are
going to require a bit of rework, which I'll take care of after
getting these patches in.
Laine Stump (28):
util: add -w/--concurrent when applying the rule rather than when
building it
util: new virFirewallRuleGet*() APIs
util: determine ignoreErrors value when creating rule, not when
applying
util: rename iptables helpers that will become the frontend for
ip&nftables
util: move backend-agnostic virNetfilter*() functions to their own
file
util: make netfilter action a proper typedefed (virFirewall) enum
util: #define the names used for private packet filter chains
util: move/rename virFirewallApplyRuleDirect to
virIptablesApplyFirewallRule
util/network: reintroduce virFirewallBackend, but different
network: add (empty) network.conf file to distribution files
network: allow setting firewallBackend from network.conf
network: do not add DHCP checksum mangle rule unless using iptables
network: call backend agnostic function to init private filter chains
util: setup functions in virnetfilter which will call appropriate
backend
build: add nft to the list of binaries we attempt to locate
util: add nftables backend to virnetfilter API used by network driver
tests: test cases for nftables backend
util: new functions to support adding individual rollback rules
util: check for 0 args when applying iptables rule
util: implement rollback rule autosave for iptables backend
util: implement rollback rule autosave for nftables backend
network: turn on auto-rollback for the rules added for virtual
networks
util: new function virFirewallNewFromRollback()
util: new functions virFirewallParseXML() and virFirewallFormat()
conf: add a virFirewall object to virNetworkObj
network: use previously saved list of firewall rules when removing
network: save network status when firewall rules are reloaded
network: improve log message when reloading virtual network firewall
rules
libvirt.spec.in | 5 +
meson.build | 1 +
po/POTFILES | 2 +
src/conf/virnetworkobj.c | 40 +
src/conf/virnetworkobj.h | 11 +
src/libvirt_private.syms | 68 +-
src/network/bridge_driver.c | 40 +-
src/network/bridge_driver_conf.c | 44 +
src/network/bridge_driver_conf.h | 3 +
src/network/bridge_driver_linux.c | 241 +++--
src/network/bridge_driver_nop.c | 6 +-
src/network/bridge_driver_platform.h | 6 +-
src/network/libvirtd_network.aug | 39 +
src/network/meson.build | 11 +
src/network/network.conf | 24 +
src/network/test_libvirtd_network.aug.in | 5 +
src/nwfilter/nwfilter_ebiptables_driver.c | 16 +-
src/util/meson.build | 2 +
src/util/virebtables.c | 4 +-
src/util/virfirewall.c | 490 ++++++++--
src/util/virfirewall.h | 51 +-
src/util/viriptables.c | 762 ++++-----------
src/util/viriptables.h | 222 ++---
src/util/virnetfilter.c | 892 ++++++++++++++++++
src/util/virnetfilter.h | 159 ++++
src/util/virnftables.c | 698 ++++++++++++++
src/util/virnftables.h | 118 +++
.../{base.args => base.iptables} | 0
tests/networkxml2firewalldata/base.nftables | 256 +++++
...-linux.args => nat-default-linux.iptables} | 0
.../nat-default-linux.nftables | 248 +++++
...pv6-linux.args => nat-ipv6-linux.iptables} | 0
.../nat-ipv6-linux.nftables | 384 ++++++++
...rgs => nat-ipv6-masquerade-linux.iptables} | 0
.../nat-ipv6-masquerade-linux.nftables | 456 +++++++++
...linux.args => nat-many-ips-linux.iptables} | 0
.../nat-many-ips-linux.nftables | 472 +++++++++
...-linux.args => nat-no-dhcp-linux.iptables} | 0
.../nat-no-dhcp-linux.nftables | 384 ++++++++
...ftp-linux.args => nat-tftp-linux.iptables} | 0
.../nat-tftp-linux.nftables | 274 ++++++
...inux.args => route-default-linux.iptables} | 0
.../route-default-linux.nftables | 162 ++++
tests/networkxml2firewalltest.c | 56 +-
tests/virfirewalltest.c | 20 +-
45 files changed, 5718 insertions(+), 954 deletions(-)
create mode 100644 src/network/libvirtd_network.aug
create mode 100644 src/network/network.conf
create mode 100644 src/network/test_libvirtd_network.aug.in
create mode 100644 src/util/virnetfilter.c
create mode 100644 src/util/virnetfilter.h
create mode 100644 src/util/virnftables.c
create mode 100644 src/util/virnftables.h
rename tests/networkxml2firewalldata/{base.args => base.iptables} (100%)
create mode 100644 tests/networkxml2firewalldata/base.nftables
rename tests/networkxml2firewalldata/{nat-default-linux.args => nat-default-linux.iptables} (100%)
create mode 100644 tests/networkxml2firewalldata/nat-default-linux.nftables
rename tests/networkxml2firewalldata/{nat-ipv6-linux.args => nat-ipv6-linux.iptables} (100%)
create mode 100644 tests/networkxml2firewalldata/nat-ipv6-linux.nftables
rename tests/networkxml2firewalldata/{nat-ipv6-masquerade-linux.args => nat-ipv6-masquerade-linux.iptables} (100%)
create mode 100644 tests/networkxml2firewalldata/nat-ipv6-masquerade-linux.nftables
rename tests/networkxml2firewalldata/{nat-many-ips-linux.args => nat-many-ips-linux.iptables} (100%)
create mode 100644 tests/networkxml2firewalldata/nat-many-ips-linux.nftables
rename tests/networkxml2firewalldata/{nat-no-dhcp-linux.args => nat-no-dhcp-linux.iptables} (100%)
create mode 100644 tests/networkxml2firewalldata/nat-no-dhcp-linux.nftables
rename tests/networkxml2firewalldata/{nat-tftp-linux.args => nat-tftp-linux.iptables} (100%)
create mode 100644 tests/networkxml2firewalldata/nat-tftp-linux.nftables
rename tests/networkxml2firewalldata/{route-default-linux.args => route-default-linux.iptables} (100%)
create mode 100644 tests/networkxml2firewalldata/route-default-linux.nftables
--
2.39.2
5 months, 1 week
