On Fri, Jun 14, 2024 at 12:22:50PM -0400, Andrea Bolognani wrote:
On Fri, Jun 14, 2024 at 03:43:53PM GMT, Daniel P. Berrangé wrote:
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 | 15 ++++++++++++++- src/util/virfirewall.c | 6 ++++++ src/util/virfirewall.h | 1 + 7 files changed, 65 insertions(+), 14 deletions(-)
The test suite no longer passes after applying this. At the very least, you need to squash in the diff at the bottom of this message.
Well that's very wierd as I ran a CI pipeline with this commit which worked ! https://gitlab.com/berrange/libvirt/-/pipelines/1332837446 but you are correct that it fails tests, so huh ?
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
This implementation allows things such as
-Dfirewall_backend_priority=nftables
That's good, it allows preventing any fallback to iptables by default.
and
-Dfirewall_backend_priority=iptables,iptables
At least
-Dfirewall_backend_priority=iptables,nftables,iptables
will be blocked, but only because it results in a compilation error: meson will happily accept it.
Are we okay with that? It's IMO inferior to the much stricter checking that's performed today.
I don't think it really matters - if someone wants to pass a wierd config, so be it.
@@ -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;
This check (and the other ones you've introduced) are running too late, which leads to this behavior:
$ cat default-session.xml <network> <name>default-session</name> <forward mode='nat'/> <bridge name='virbr0' stp='on' delay='0'/> <ip address='192.168.123.1' netmask='255.255.255.0'> <dhcp> <range start='192.168.123.2' end='192.168.123.254'/> </dhcp> </ip> </network>
$ virsh -c qemu:///session net-define default-session.xml Network default-session defined from default-session.xml
$ virsh -c qemu:///session net-start default-session error: Failed to start network default-session error: error creating bridge interface virbr0: Operation not permitted
Since <forward mode='nat'/> requires firewall rules, we shouldn't allow a network that uses it to be defined in the first place.
This is the behaviour we already had before the nftables backend was created, and its not been a problem. There's no functional reason why we should refuse to allow it to be defined, if the binaries aren't needed until startup.
diff --git a/po/POTFILES b/po/POTFILES index 4bfbb91164..1ed4086d2c 100644 --- a/po/POTFILES +++ b/po/POTFILES @@ -143,6 +143,7 @@ src/lxc/lxc_process.c src/network/bridge_driver.c src/network/bridge_driver_conf.c src/network/bridge_driver_linux.c +src/network/bridge_driver_nop.c src/network/leaseshelper.c src/network/network_iptables.c src/network/network_nftables.c diff --git a/src/network/bridge_driver_nop.c b/src/network/bridge_driver_nop.c index 2114d521d1..323990cf17 100644 --- a/src/network/bridge_driver_nop.c +++ b/src/network/bridge_driver_nop.c @@ -49,8 +49,8 @@ int networkAddFirewallRules(virNetworkDef *def G_GNUC_UNUSED, */ if (firewallBackend != VIR_FIREWALL_BACKEND_NONE) { virReportError(VIR_ERR_NO_SUPPORT, - _("Firewall backend '%s' not available on this platform"), - virFirewallBackendTypeToString(firewallBackend)); + _("Firewall backend '%1$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 d374f54b64..090dbcdbed 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -817,7 +817,7 @@ virFirewallApplyCmd(virFirewall *firewall,
switch (virFirewallGetBackend(firewall)) { case VIR_FIREWALL_BACKEND_NONE: - virReportError(VIR_ERR_NO_SUPPORT, + virReportError(VIR_ERR_NO_SUPPORT, "%s", _("Firewall backend is not implemented")); return -1;
-- Andrea Bolognani / Red Hat / Virtualization
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 :|