1:37 a.m.
On 6/14/24 12:22 PM, 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.
> 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
I suppose this is true for now, but will hopefully some day become false :-)
This implementation allows things such as
-Dfirewall_backend_priority=nftables
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 haven't had the time to test this, or even look at it in detail, since
I'm leaving for a multi-week vacation on Sunday, but I thought I should
at least give my opinion on how I think it *should* work - maybe it
already *does* work as I describe)
What I would most prefer would be if you could list 1 or more backends
(i.e. not all backends need to be listed, and if a particular backend
isn't listed then it isn't checked for automatically (maybe isn't even
allowed to be set explicitly), and ideally wouldn't even be compiled in).
It would be nice if it gave an error if the same backend was given more
than once in the list, but since you can probably count the number of
people who will ever mess with this setting with the fingers on your two
hands (and maybe the addition of your toes), and they're hopefully
mentally aware enough to realize that naming the same backend multiple
time would be pointless, I don't think a check to prevent duplicates is
absolutely necessary (if fixing it would mean that you can't list fewer
than the total available backends - someone on Linux might want to only
enable nftables).
To say it by example, I think on a Linux system it should work to define
firewall_backend_prioty as:
nftables,iptables
iptables,nftables
nftables
iptables
null
Currently on FreeBSD the only acceptable setting would be "null", but in
the future it could be "ipfw,pf", or the opposite order, or just one or
the other.
> @@ -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.
I agree with Dan that we shouldn't prevent anyone from *defining* such a
network, we should just log an error and fail if they try to start such
a network on a system that doesn't have a non-null firewall backend.
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;