Re: [libvirt] [PATCH 4/5] network: regain guest network connectivity after firewalld switch to nftables

On 1/15/19 12:54 PM, Daniel P. Berrangé wrote: > On Tue, Jan 15, 2019 at 12:43:05PM -0500, Laine Stump wrote: > > On 1/15/19 11:39 AM, Daniel P. Berrangé wrote: > > > I wonder if we should have an configure time check so that we do > > > *not* install this file if built agains an old firewalld version. > > > By unconditionally installing it we're probably causing firewalld > > > to emit an error message in syslog which some sysadmin is almost > > > certainly to report a bug about > > 1) see the comment above about the unreliability of version checks. > > > > > > 2) The version / build of firewalld present at build time is really > > irrelevant. As a matter of fact, we currently don't require firewalld to be > > present *at all* at build time (or at runtime - we just use it if it's there > > and active, otherwise we go directly to iptables/ebtables). > If configure installs it by default, but provides a switch, we can > use --with-firewall-zone / --without-firewalld-zone explicitly in > the RPM specs. We know exactly which Fedora / RHEL versions need > this in so can avoid installing it where we know if isn't wanted. To make sure I'm understanding properly (because I think I misunderstood the 1st time I read it) you're suggesting that we not check the version of firewalld during configure/build, but have a configure option that always defaults to "with", and can be manually set to "without"?

Since the extraneous error message that you want to get rid of is one that's logged by firewalld when it fails to parse the libvirt zone file (I think we can avoid all other extraneous logs at runtime by checking the firewalld backend, and checking for existence of the libvirt zone), I guess this configure change would only be useful for indicating whether or not the libvirt zone file should be installed (and not really essential for any runtime code in  libvirt), but that's something that can't really be affected by configure (aside from having it modify the libvirt.spec file, but that's only useful for RHEL/Fedora, and I don't think RHEL or Fedora will be updating libvirt without updating firewalld). So I'm not sure there's a point to supporting --without_firewalld_zone in the configure.

> > 3) I actually *want* the bug reported to distro maintainers, to push them to > > update their firewalld sooner rather than later. > If we install it based on version where nftables supprot was first > introduced in firewalld, rather than when it was fixed, we would > still get useful bug reports without spamming people with errors on > distros where nftables isn't even supported by firewalld. Our aim is to avoid the error log caused by firewalld failing to parse the libvirt zone (if that failure is due to a lack of support for rule priorities). To do that, we need to make sure the libvirt zone file isn't installed on systems where firewalld lacks support for rule priorities. We can either do that by modifying the package contents to include or not include libvirt.zone, or we can do it by modifying the install-time scripts to copy/not copy libvirt.zone into its place. If we modify the package contents at build time, then updating firewalld to a version supporting nftables+priorities would necessitate a new build *and* install of the libvirt packages.

Hmm, but I guess if the *only* change when building/installing with --without_firewalld_zones was that libvirt.zone wasn't installed (i.e. we would still compile the code that logs a libvirt error if the backend is nftables but there is no libvirt zone), then I guess the failure wouldn't be silent. So I retract my statement about a silent failure, but it still would mean the libvirt packages would need to be rebuilt/reinstalled to get things working after a firewalld upgrade.

Since it's only a transient thing (it will only be a problem for somebody who upgrades to new libvirt, but keeps their firewalld at something older than 0.7.0), I don't think we should do anything at all complicated to prevent it - we'd just end up with a bunch of obscure code that won't be used any time after 6 months from now, and will continue confusing people long into the future. I think it's better in the end to just live with a few extra (and harmless) error messages during the short period when some distros have libvirt >= 5.1.0 and firewalld < 0.7.0, rather than having silent (i.e. no error message) failures if a distro updates firewalld without rebuilding libvirt.