Re: [PATCH nft v3 2/6] tests/shell: check and generate JSON dump files

[Phil Sutter <phil@nwl.cc>]

On Fri, Nov 17, 2023 at 06:06:16PM +0100, Thomas Haller wrote:
> On Fri, 2023-11-17 at 17:57 +0100, Phil Sutter wrote:
> > On Fri, Nov 17, 2023 at 05:36:23PM +0100, Pablo Neira Ayuso wrote:
> > > On Fri, Nov 17, 2023 at 05:16:02PM +0100, Thomas Haller wrote:
> > > > On Fri, 2023-11-17 at 00:00 +0100, Florian Westphal wrote:
> > > > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > > > Hi Thomas,
> > > > > > 
> > > > > > On Wed, Nov 15, 2023 at 01:36:40PM +0100, Thomas Haller
> > > > > > wrote:
> > > > > > > On Wed, 2023-11-15 at 13:30 +0100, Pablo Neira Ayuso wrote:
> > > > > > [...]
> > > > > > > > I see _lots_ of DUMP FAIL with kernel 5.4
> > > > > > > 
> > > > > > > Hi,
> > > > > > > 
> > > > > > > Could you provide more details?
> > > > > > > 
> > > > > > > For example,
> > > > > > > 
> > > > > > >     make -j && ./tests/shell/run-tests.sh
> > > > > > > tests/shell/testcases/include/0007glob_double_0 -x
> > > > > > >     grep ^ -a -R /tmp/nft-test.latest.*/
> > > > > > 
> > > > > > # cat [...]/ruleset-diff.json
> > > > > > --- testcases/include/dumps/0007glob_double_0.json-nft  2023-
> > > > > > 11-15
> > > > > > 13:27:20.272084254 +0100
> > > > > > +++ /tmp/nft-test.20231116-170617.584.lrZzMy/test-testcases-
> > > > > > include-0007glob_double_0.1/ruleset-after.json      2023-11-
> > > > > > 16
> > > > > > 17:06:18.332535411 +0100
> > > > > > @@ -1 +1 @@
> > > > > > -{"nftables": [{"metainfo": {"version": "VERSION",
> > > > > > "release_name":
> > > > > > "RELEASE_NAME", "json_schema_version": 1}}, {"table":
> > > > > > {"family":
> > > > > > "ip", "name": "x", "handle": 1}}, {"table": {"family": "ip",
> > > > > > "name": "y", "handle": 2}}]}
> > > > > > +{"nftables": [{"metainfo": {"version": "VERSION",
> > > > > > "release_name":
> > > > > > "RELEASE_NAME", "json_schema_version": 1}}, {"table":
> > > > > > {"family":
> > > > > > "ip", "name": "x", "handle": 158}}, {"table": {"family":
> > > > > > "ip",
> > > > > > "name": "y", "handle": 159}}]}
> > > > > > 
> > > > > > It seems that handles are a problem in this diff.
> > > > > 
> > > > > Are you running tests with -s option?
> > > > > 
> > > > > In that case, modules are removed after each test.
> > > > > 
> > > > > I suspect its because we can then hit -EAGAIN mid-transaction
> > > > > because module is missing (again), then replay logic does its
> > > > > thing.
> > > > > 
> > > > > But the handle generator isn't transaction aware,
> > > > > so it has advanced vs. the aborted partial transaction.
> > > > 
> > > > > I'm not sure what to do here.
> > > > 
> > > > a combination of:
> > > > 
> > > > a) make an effort, that kernel behavior is consistent and
> > > > reproducible.
> > > > Stable output seems important to me, and the automatic loading of
> > > > a
> > > > kernel module should not make a difference. This is IMO a bug.
> > > 
> > > This is not a bug in the kernel. The kernel guarantees that the
> > > handle
> > > is unique, but the handle allocation strategy is up to the kernel.
> > > Userspace cannot forecast what handle will get, such thing might
> > > lead
> > > to easy to break assumptions from userspace.
> > > 
> > > > b) let `nft -j list ruleset` honor (the lack of) `--handle`
> > > > option and
> > > > not print those handles. That bugfix would change behavior, so
> > > > maybe
> > > > instead add a "--no-handle" option for `nft -j` dumps.
> > 
> > 
> > > 
> > > Will honoring -a/--handle break firewalld? I think it is the main
> > > user
> > > of the JSON API. That might help disentangle if this makes sense or
> > > not and what the chances of breaking third party applications are.
> > > 
> > > I'd prefer not to see a --no-handle that will only work for JSON
> > > and
> > > that is only useful for this test infrastructure (noone else asked
> > > for
> > > this).
> > > 
> > > > c) sanitize the output with the sed command (my other mail).
> > > > 
> > > > This also means, that the .json-nft dumps won't work, if you run
> > > > without `unshare`. IMO, the mode without unshare should not be
> > > > supported anymore. But if it's deemed important, then it requires
> > > > b) or
> > > > c) or detect the case and skip the diffs with .json-nft.
> > 
> > What is the problem without unshare? Looking at your patch, it seems
> > possible to drop the handle attributes in json-sanitize-ruleset.sh.
> 
> Yes, (b) would suffice. I said "or" :)
> 
> No further problem, but without-unshare seems not a useful thing to
> support. The test-run takes significantly longer, interferes with the
> caller's netns and requires CAP_NET_ADMIN.

No, I was wondering why with option (c) "This also means, that the
.json-nft dumps won't work, if you run without `unshare`."

Because I vote for that option. ;)

> > 
> > > a) is no-go (kernel update to make test infrastructure or to allow
> > > userspace application to make fragile assumptions on how handles
> > > are
> > > allocated is not correct).
> > > 
> > > b) needs to evaluated, you maintain firewalld, let us know.
> > 
> > Given the inherent importance of the handle value for ruleset
> > manipulations, I assume *any* application will need to be updated to
> > pass --handle (or the libnftables-equivalent) to remain functional.
> 
> Right. So a "--no-handle" / NFT_CTX_OUTPUT_NO_HANDLE flag for JSON
> output?

Should not be needed. IIUC, the test infrastructure you're about to
introduce sanitizes the JSON output already anyway, right?