Hi Laine,

(In reply to Laine Stump from comment #0)
> The output from "ebtables -L" in the the "original" ebtables package formats
> MAC addresses as:
> 
>     %x:%x:%x:%x:%x:%x
> 
> (e.g. "1:2:3:4:5:6") and shows IPv6 prefixes as a netmask (e.g:
> "/ffff:ffff:fc00::"), but the same output using the ebtables command from
> the iptables-ebtables package formats MAC addresses as:
> 
>    %0x:%0x:%0x:%0x:%0x:%0x
> 
> ("01:02:03:04:05:06") and shows IPv6 prefixes as.... prefixes (e.g.: "/22").
> 
> While the new format is arguably "nicer", it is a change in defacto API that
> makes the RHEL8 ebtables -L output incompatible with what was output in
> previous RHEL versions, thus making it likely that shell scripts that have
> been running successfully for years will suddenly fail after upgrading.

I would even go as far as considering the old output format to be a bug which
should be fixed in legacy ebtables. :)

Regarding breaking scripts, I can't imagine the differences in mac address
output to be relevant - you'd have to account for two digits per byte anyway and
in practice to many regexps a mac address simply looks like '*:*:*:*:*:*'.

The IPv6 address mask output definitely has potential to break scripts, the
output is completely different (and has to be interpreted as such). I wonder how
many swearing and custom written prefix to mask converters this has caused.

> iptables-ebtables should format MAC addresses and IPv6 prefixes (and
> everything else) exactly as it was formatted in the original ebtables.

I agree but reintroducing stupid things for that hurts, so I would like to
assess impact first:

> This was found by running the libvirt-tck nwfilter tests, which are in the
> package perl-Sys-Virt-TCK. A RHEL8 repo of all the necessary packages to
> install and run this test suite are here:

Does libvirt depend on the output formatting of ebtables (apart from verifiers
in testsuite)? If so, what is the deadline for you to get this resolved
(RHEL8.0, 8.0.1 (what-/whenever that is), 8.1)? If not, does adjusting the
testsuite to accept the changed output constitue a workaround?

Thanks, Phil

(In reply to Phil Sutter from comment #1)
> Hi Laine,
> 
> (In reply to Laine Stump from comment #0)
> > The output from "ebtables -L" in the the "original" ebtables package formats
> > MAC addresses as:
> > 
> >     %x:%x:%x:%x:%x:%x
> > 
> > (e.g. "1:2:3:4:5:6") and shows IPv6 prefixes as a netmask (e.g:
> > "/ffff:ffff:fc00::"), but the same output using the ebtables command from
> > the iptables-ebtables package formats MAC addresses as:
> > 
> >    %0x:%0x:%0x:%0x:%0x:%0x
> > 
> > ("01:02:03:04:05:06") and shows IPv6 prefixes as.... prefixes (e.g.: "/22").
> > 
> > While the new format is arguably "nicer", it is a change in defacto API that
> > makes the RHEL8 ebtables -L output incompatible with what was output in
> > previous RHEL versions, thus making it likely that shell scripts that have
> > been running successfully for years will suddenly fail after upgrading.
> 
> I would even go as far as considering the old output format to be a bug which
> should be fixed in legacy ebtables. :)
> 
> Regarding breaking scripts, I can't imagine the differences in mac address
> output to be relevant - you'd have to account for two digits per byte anyway
> and
> in practice to many regexps a mac address simply looks like '*:*:*:*:*:*'.

Yes, I think this change is really minor / insignificant

> The IPv6 address mask output definitely has potential to break scripts, the
> output is completely different (and has to be interpreted as such). I wonder
> how many swearing and custom written prefix to mask converters this has caused.

I don't disagree that the old format was unpleasant, but it has been like
this for years now, so admins/scripts are adapted to the pain already.

If the nft based *tables commands are really intended to be a drop-in
replacement for the original xtables based impls, then this format
change is a bad idea.

> > iptables-ebtables should format MAC addresses and IPv6 prefixes (and
> > everything else) exactly as it was formatted in the original ebtables.
> 
> I agree but reintroducing stupid things for that hurts, so I would like to
> assess impact first:
> 
> > This was found by running the libvirt-tck nwfilter tests, which are in the
> > package perl-Sys-Virt-TCK. A RHEL8 repo of all the necessary packages to
> > install and run this test suite are here:
> 
> Does libvirt depend on the output formatting of ebtables (apart from
> verifiers
> in testsuite)? If so, what is the deadline for you to get this resolved
> (RHEL8.0, 8.0.1 (what-/whenever that is), 8.1)? If not, does adjusting the
> testsuite to accept the changed output constitue a workaround?

It /only/ affects the libvirt test suite when we validate the rules that
were created match what we expected to get.

If it doesn't get resolved for RHEL-8.0 GA, then we're going to have to
put a workaround in our test suite regardless, at which point fixing the
bug isn't an issue from libvirt's POV anymore.

IOW, from libvirt's POV either this change is fixed for GA, or it can be closed wontfix now.

Hi Daniel,

(In reply to Daniel Berrange from comment #2)
> (In reply to Phil Sutter from comment #1)
[...]
> > Does libvirt depend on the output formatting of ebtables (apart from
> > verifiers
> > in testsuite)? If so, what is the deadline for you to get this resolved
> > (RHEL8.0, 8.0.1 (what-/whenever that is), 8.1)? If not, does adjusting the
> > testsuite to accept the changed output constitue a workaround?
> 
> It /only/ affects the libvirt test suite when we validate the rules that
> were created match what we expected to get.
> 
> If it doesn't get resolved for RHEL-8.0 GA, then we're going to have to
> put a workaround in our test suite regardless, at which point fixing the
> bug isn't an issue from libvirt's POV anymore.
> 
> IOW, from libvirt's POV either this change is fixed for GA, or it can be
> closed wontfix now.

Thanks for your validation. After studying RFC4291, I think representation of
IPv6 prefixes in netmask notation is invalid. Consequently I've sent a patch to
change this in legacy ebtables upstream:

https://marc.info/?l=netfilter-devel&m=155000828923204&w=2

If this is rejected, I'll attempt to align ebtables-nft output with legacy one.

Thanks, Phil

(In reply to Phil Sutter from comment #3)
> Hi Daniel,
> 
> (In reply to Daniel Berrange from comment #2)
> > (In reply to Phil Sutter from comment #1)
> [...]
> > > Does libvirt depend on the output formatting of ebtables (apart from
> > > verifiers
> > > in testsuite)? If so, what is the deadline for you to get this resolved
> > > (RHEL8.0, 8.0.1 (what-/whenever that is), 8.1)? If not, does adjusting the
> > > testsuite to accept the changed output constitue a workaround?
> > 
> > It /only/ affects the libvirt test suite when we validate the rules that
> > were created match what we expected to get.
> > 
> > If it doesn't get resolved for RHEL-8.0 GA, then we're going to have to
> > put a workaround in our test suite regardless, at which point fixing the
> > bug isn't an issue from libvirt's POV anymore.
> > 
> > IOW, from libvirt's POV either this change is fixed for GA, or it can be
> > closed wontfix now.
> 
> Thanks for your validation. After studying RFC4291, I think representation of
> IPv6 prefixes in netmask notation is invalid. Consequently I've sent a patch
> to
> change this in legacy ebtables upstream:
> 
> https://marc.info/?l=netfilter-devel&m=155000828923204&w=2
> 
> If this is rejected, I'll attempt to align ebtables-nft output with legacy
> one.

Upstream accepted my fix for legacy ebtables. Therefore I think we may burden
RHEL8 users with adjusting their scripts. (Which includes libvirt's testsuite.)

The upstream patch only "fixes" netmask/prefix formatting, but doesn't change the MAC address formatting. Do you plan to send a patch for that as well?

Hi Laine,

(In reply to Laine Stump from comment #5)
> The upstream patch only "fixes" netmask/prefix formatting, but doesn't
> change the MAC address formatting. Do you plan to send a patch for that as
> well?

As pointed out in comment 1, I consider this a minor issue. Do you think
fixing the formatting in legacy ebtables is feasible?

Thanks, Phil

I'm not advocating one way or the other, just wondering if you had forgotten about it, or if you purposefully intended to not change it. I'm fine with it either way.

(Well, I really think variable-length MAC address strings are ugly, and it complicates our tests to have to deal with variable and fixed length MACs.(since the way the test works is 1) apply some abstract rule at a higher level (in this case libvirt's nwfilter rules), then 2) diff (i.e. a straight strcmp, not a regexp match) the output of various subsets of ebtables -L (and iptables -L and ip6tables -L) with output that was captured during a "known good" run. If different versions of ebtables can produce different results, then we need to either do a regexp comparison (which makes understanding the tests, and what may have gone wrong, more complicated, or keep multiple versions of the test results. But really if it's just our tests then that's not a big deal (and anyway, even if you change ebtables behavior to match iptables-ebtables, we will still have to account for both styles in our tests at least until RHEL7 is EOL (unless the RHEL7 ebtables package is updated), which is a *long time* away :-). The whole intent of filing this BZ in the first place was just to make sure everyone was aware of the change, and had thought about the possible consequences.)

Hi Laine,

(In reply to Laine Stump from comment #7)
> I'm not advocating one way or the other, just wondering if you had forgotten
> about it, or if you purposefully intended to not change it. I'm fine with it
> either way.

My reason for sending a patch to legacy ebtables was to help decide if I should
adjust ebtables-nft to the non-conformant way of printing IPv6 prefixes. I didn't
do the same for MAC address formatting since the decision was clear there IMO.

> (Well, I really think variable-length MAC address strings are ugly, and it
> complicates our tests to have to deal with variable and fixed length
> MACs.(since the way the test works is 1) apply some abstract rule at a
> higher level (in this case libvirt's nwfilter rules), then 2) diff (i.e. a
> straight strcmp, not a regexp match) the output of various subsets of
> ebtables -L (and iptables -L and ip6tables -L) with output that was captured
> during a "known good" run. If different versions of ebtables can produce
> different results, then we need to either do a regexp comparison (which
> makes understanding the tests, and what may have gone wrong, more
> complicated, or keep multiple versions of the test results. But really if
> it's just our tests then that's not a big deal (and anyway, even if you
> change ebtables behavior to match iptables-ebtables, we will still have to
> account for both styles in our tests at least until RHEL7 is EOL (unless the
> RHEL7 ebtables package is updated), which is a *long time* away :-). The
> whole intent of filing this BZ in the first place was just to make sure
> everyone was aware of the change, and had thought about the possible
> consequences.)

I don't think changing ebtables output in RHEL7 is acceptable. But you're right,
this change should be documented.

Thanks, Phil