and include it in BuildRequires and Requires of the rpm specfile to
make sure it's available when doing official distro builds.

Signed-off-by: Laine Stump <laine@redhat.com>
---
 libvirt.spec.in | 2 ++
 meson.build     | 1 +
 2 files changed, 3 insertions(+)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index ba73efb0b7..7b73b38af8 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -302,6 +302,7 @@ BuildRequires: libnl3-devel
 BuildRequires: libselinux-devel
 BuildRequires: iptables
 BuildRequires: ebtables
+BuildRequires: nftables
 BuildRequires: module-init-tools
 BuildRequires: cyrus-sasl-devel
 BuildRequires: polkit >= 0.112
@@ -541,6 +542,7 @@ Requires: libvirt-daemon-common = %{version}-%{release}
 Requires: libvirt-libs = %{version}-%{release}
 Requires: dnsmasq >= 2.41
 Requires: iptables
+Requires: nftables
 
 %description daemon-driver-network
 The network driver plugin for the libvirtd daemon, providing
diff --git a/meson.build b/meson.build
index 9a18767fbb..2d94acc226 100644
--- a/meson.build
+++ b/meson.build
@@ -801,6 +801,7 @@ optional_programs = [
   'mdevctl',
   'mm-ctl',
   'modprobe',
+  'nft',
   'ovs-vsctl',
   'passt',
   'pdwtags',
-- 
2.39.2

On Sun, Apr 30, 2023 at 11:19:30PM -0400, Laine Stump wrote:
> and include it in BuildRequires and Requires of the rpm specfile to
> make sure it's available when doing official distro builds.
> 
> Signed-off-by: Laine Stump <laine@redhat.com>
> ---
>  libvirt.spec.in | 2 ++
>  meson.build     | 1 +
>  2 files changed, 3 insertions(+)

This new dep will need libvirt.yml in libvirt-ci.git to be updated
and the dockerfiles then re-generated.

> 
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index ba73efb0b7..7b73b38af8 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -302,6 +302,7 @@ BuildRequires: libnl3-devel
>  BuildRequires: libselinux-devel
>  BuildRequires: iptables
>  BuildRequires: ebtables
> +BuildRequires: nftables
>  BuildRequires: module-init-tools
>  BuildRequires: cyrus-sasl-devel
>  BuildRequires: polkit >= 0.112
> @@ -541,6 +542,7 @@ Requires: libvirt-daemon-common = %{version}-%{release}
>  Requires: libvirt-libs = %{version}-%{release}
>  Requires: dnsmasq >= 2.41
>  Requires: iptables
> +Requires: nftables
>  
>  %description daemon-driver-network
>  The network driver plugin for the libvirtd daemon, providing
> diff --git a/meson.build b/meson.build
> index 9a18767fbb..2d94acc226 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -801,6 +801,7 @@ optional_programs = [
>    'mdevctl',
>    'mm-ctl',
>    'modprobe',
> +  'nft',
>    'ovs-vsctl',
>    'passt',
>    'pdwtags',
> -- 
> 2.39.2
> 

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

On Wed, May 03, 2023 at 04:26:21PM +0100, Daniel P. Berrangé wrote:
> On Sun, Apr 30, 2023 at 11:19:30PM -0400, Laine Stump wrote:
> > and include it in BuildRequires and Requires of the rpm specfile to
> > make sure it's available when doing official distro builds.
>
> This new dep will need libvirt.yml in libvirt-ci.git to be updated
> and the dockerfiles then re-generated.

I don't think we need the BuildRequires, or the build time detection,
at all. Just

  #define NFT "nft"

in the relevant file and be done with it. We'll locate the binary at
runtime, same as we're doing with most of them already.

The Requires is still needed, of course.


Maybe we also want to turn the iptables dependency into a Recommends?
That way you will be able to uninstall it for a pure nft-based setup.

... at some point. A lot of stuff seems to still depend on iptables
today, at least in Fedora.

-- 
Andrea Bolognani / Red Hat / Virtualization

On 5/4/23 4:33 AM, Andrea Bolognani wrote:
> On Wed, May 03, 2023 at 04:26:21PM +0100, Daniel P. Berrangé wrote:
>> On Sun, Apr 30, 2023 at 11:19:30PM -0400, Laine Stump wrote:
>>> and include it in BuildRequires and Requires of the rpm specfile to
>>> make sure it's available when doing official distro builds.
>>
>> This new dep will need libvirt.yml in libvirt-ci.git to be updated
>> and the dockerfiles then re-generated.
> 
> I don't think we need the BuildRequires, or the build time detection,
> at all. Just
> 
>    #define NFT "nft"
> 
> in the relevant file and be done with it. We'll locate the binary at
> runtime, same as we're doing with most of them already.

Are we? What's the huge list of "optional programs" in meson.build then?

I don't have any problem with doing all binary-location at runtime, as 
long as we don't think there's any potential security problem / bug that 
could arise from having a different binary with the same name added in 
some place earlier in $PATH (is that why we started canonicalizing 
binary paths during the build?) Thanks to the way 
g_find_program_in_path() was written, code later in this series that 
checks to see which binaries are available will work properly, whether 
or not the binary name was canonicalized during build, so making such a 
change won't have any effect on that.
> 
> The Requires is still needed, of course.
> 
> 
> Maybe we also want to turn the iptables dependency into a Recommends?
> That way you will be able to uninstall it for a pure nft-based setup.

I was being ultra-conservative about the change, making it opt-in for 
the distros for now at least. But I'm also fine with making it opt-out

> 
> ... at some point. A lot of stuff seems to still depend on iptables
> today, at least in Fedora.

Yeah, *somebody* has to start pulling the plug on it (actually firewalld 
has had nftables support for a long time, and I think it's probably the 
default although I haven't checked). It is really amazing how many 
people still automatically talk about iptables when they talk about 
filtering network traffic :-/

On Thu, May 04, 2023 at 02:21:57PM -0400, Laine Stump wrote:
> On 5/4/23 4:33 AM, Andrea Bolognani wrote:
> > I don't think we need the BuildRequires, or the build time detection,
> > at all. Just
> >
> >    #define NFT "nft"
> >
> > in the relevant file and be done with it. We'll locate the binary at
> > runtime, same as we're doing with most of them already.
>
> Are we? What's the huge list of "optional programs" in meson.build then?

Leftovers, that I intend to clean up At Some Point™ :)

> I don't have any problem with doing all binary-location at runtime, as long
> as we don't think there's any potential security problem / bug that could
> arise from having a different binary with the same name added in some place
> earlier in $PATH

If some malicious actor can alter root's $PATH, or inject binaries
into it, it's pretty much game over already.

> (is that why we started canonicalizing binary paths during
> the build?)

I think it was done more for feature detection purposes, e.g. only
enable the network driver if ifconfig is present or something.

But that gets in the way of packagers, who usually want to explicitly
enable/disable features anyway and to build in a minimal environment.
It also assumes same-host deployment, and locks the configuration too
early (what if I install ifconfig after building libvirt?).

Runtime detection has some drawbacks too, but overall is more
flexible and we've been moving in that direction.

> > Maybe we also want to turn the iptables dependency into a Recommends?
> > That way you will be able to uninstall it for a pure nft-based setup.
>
> I was being ultra-conservative about the change, making it opt-in for the
> distros for now at least. But I'm also fine with making it opt-out

I believe Dan argued for the nft backend to be made the default where
possible. I generally agree that we should adopt forward-looking
defaults whenever that can be done without breaking existing users.

Anyway, regardless of which one of the backends ends up being the
default one, maybe *both* nft and iptables should be Recommends? That
way you'll get both installed by default, but you'll be able to drop
the one that you're not using if you're aiming for a minimal
deployment.

-- 
Andrea Bolognani / Red Hat / Virtualization

On Fri, May 05, 2023 at 02:04:01AM -0700, Andrea Bolognani wrote:
> On Thu, May 04, 2023 at 02:21:57PM -0400, Laine Stump wrote:
> > On 5/4/23 4:33 AM, Andrea Bolognani wrote:
> > > I don't think we need the BuildRequires, or the build time detection,
> > > at all. Just
> > >
> > >    #define NFT "nft"
> > >
> > > in the relevant file and be done with it. We'll locate the binary at
> > > runtime, same as we're doing with most of them already.
> >
> > Are we? What's the huge list of "optional programs" in meson.build then?
> 
> Leftovers, that I intend to clean up At Some Point™ :)
> 
> > I don't have any problem with doing all binary-location at runtime, as long
> > as we don't think there's any potential security problem / bug that could
> > arise from having a different binary with the same name added in some place
> > earlier in $PATH
> 
> If some malicious actor can alter root's $PATH, or inject binaries
> into it, it's pretty much game over already.
> 
> > (is that why we started canonicalizing binary paths during
> > the build?)
> 
> I think it was done more for feature detection purposes, e.g. only
> enable the network driver if ifconfig is present or something.
> 
> But that gets in the way of packagers, who usually want to explicitly
> enable/disable features anyway and to build in a minimal environment.
> It also assumes same-host deployment, and locks the configuration too
> early (what if I install ifconfig after building libvirt?).
> 
> Runtime detection has some drawbacks too, but overall is more
> flexible and we've been moving in that direction.
> 
> > > Maybe we also want to turn the iptables dependency into a Recommends?
> > > That way you will be able to uninstall it for a pure nft-based setup.
> >
> > I was being ultra-conservative about the change, making it opt-in for the
> > distros for now at least. But I'm also fine with making it opt-out
> 
> I believe Dan argued for the nft backend to be made the default where
> possible. I generally agree that we should adopt forward-looking
> defaults whenever that can be done without breaking existing users.
> 
> Anyway, regardless of which one of the backends ends up being the
> default one, maybe *both* nft and iptables should be Recommends? That
> way you'll get both installed by default, but you'll be able to drop
> the one that you're not using if you're aiming for a minimal
> deployment.

Fedora has used nft kmod since at least Fedora 32 IIRC. While you could
potentially unload it and load the iptbles kmods I expect the users
doing that are minimal if any. Even if someone is doing that, I see no
reason why we can't exclusively have Requires: nft, and ignore iptables
as far as deps are concerned. The only "downside" is that someone who
has done the edge case of revertnig to iptables will have a redundant
'nft' userspace package installed. I think that's totally acceptable
for such a niche edge case. Same for RHEL >= 9.



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