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 :|
