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

Thu Jan 17 09:51:01 UTC 2019

On Wed, Jan 16, 2019 at 09:43:58PM -0500, Laine Stump wrote:
> 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"?

Yes that's correct.

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

It isn't only about the versions of firewalld that support nftables, but
lack priority.

Firewalld has existed in RHEL/Fedora and other distros for many years
now, and only the yet-to-be release 0.7.0 version supports the new priority
syntax our zone file is using. So essentially every distro that exists
today will see the error from our zone file if we install it unconditionally. 

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

I don't see that as a real problem. Very few distros will rebase firewalld
in an existing stream. In a development stream, a rebuild of libvirt will
happen by chance for other reasons alrady. Dynamically copying files around
at time of RPM install is never very nice as it becomes content that is not
tracked by RPM.

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

Yes, I'd expect a rebuild of libvirt packages to take advantage of a newer
firewalld. 

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

It isn't a transient thing though - the majority of distros will never
upgrade their firewalld to 0.7.0 - they'll just wait until their next
distro release.

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