Re: [PATCH v2] network: introduce a "none" firewall backend type

On Fri, Jun 14, 2024 at 07:52:55PM GMT, Daniel P. Berrangé wrote: > 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: > > > @@ -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. But in the case of qemu:///session, or when we're not on Linux, we already know that there is no way for the network that's being defined to ever work. To me that's the same as allowing a guest to be defined, when the corresponding QEMU binary doesn't support some of the devices. Or when a firmware binary satisfying the constraints isn't available. We reject such configurations for guests, and it would just be consistent to do the same for networks.

Besides, even if we decide that we want to allow such networks to be defined, we should report a more meaningful error for the failed startup, one which points out that mode=nat will never work. It shouldn't be the same error message that I was getting on FreeBSD because of a kernel driver that wasn't loaded, as the user's ability to make the error go away is completely different in the two scenarios.