Re: [PATCH nft v2 1/1] src: enable output with "nft --echo --json" and nftables syntax

[Phil Sutter <phil@nwl.cc>]

Hi Pablo,

On Sat, Aug 01, 2020 at 09:27:30PM +0200, Pablo Neira Ayuso wrote:
> On Sat, Aug 01, 2020 at 02:02:13AM +0200, Phil Sutter wrote:
> > On Fri, Jul 31, 2020 at 07:30:28PM +0200, Pablo Neira Ayuso wrote:
> > > On Fri, Jul 31, 2020 at 03:48:28PM +0200, Phil Sutter wrote:
> > > > On Fri, Jul 31, 2020 at 02:58:25PM +0200, Pablo Neira Ayuso wrote:
> > > > > On Fri, Jul 31, 2020 at 02:33:42PM +0200, Phil Sutter wrote:
> > > [...]
> > > > The less predictable echo output behaves, the harder it is to write code
> > > > that makes use of it.
> > > 
> > > What is it making the output less predictible? The kernel should
> > > return an input that is equal to the output plus the handle. Other
> > > than that, it's a bug.
> > 
> > In tests/py, I see 330 lines explicitly stating the expected output as
> > it differs from the input ('grep "ok;" */*.t | wc -l'). Can we fix those
> > bugs first before we assume what the kernel returns is identical to user
> > input?
> 
> Semantically speaking those lines are equivalent, it's just that input
> and the output representation differ in some scenarios because the
> decompilation routine differ in the way it builds the expressions.

Obviously, yes, but irrelevant for this discussion. A script won't be
able to identify two different looking rules as identical because of
semantics.

> BTW, why do you qualify this as a bug?

I was just picking up your argument: Above, you wrote "Other than that,
it's a bug". I assume that in "return an input that is equal to the
output plus the handle", equal really means equal and not equivalent.

> > Say a script manages a rule (in JSON-equivalent) of:
> > 
> > | ip protocol tcp tcp dport '{ 22 - 23, 24 - 25}'
> > 
> > Both matches are elements in an array resembling the rule's "expr"
> > attribute. Nftables drops the first match, so if the script wants to
> > edit the ports in RHS of the second match, it won't find it anymore.
> > Also, the two port ranges are combined into a single one, so removing
> > one of the two ranges turns into a non-trivial problem.
> > 
> > Right now a script may apply its ruleset snippet and retrieve the
> > handles by:
> > 
> > | rc, ruleset, err = nftables.json_cmd(ruleset)
> > 
> > If the returned ruleset is not identical (apart from added attributes),
> > scripts will likely resort to a fire-n-forget type of usage pattern.
> 
> You mean, the user in that JSON script is comparing the input and
> output strings to find the rule handle?

I am assuming that a script that uses echo mode wants to update the
input ruleset (snippet) with handles for later reference to the added
rules. Other than copying output to input completely, it will have to
iterate through output and extract the handle properties, identifying
it's own rules based on current index. More or less what libnftables is
doing when updating JSON input with handles.

> If so, we should explore a better way to do this, eg. expose some user
> defined identifier in JSON that userspace sets on when sending the
> batch to identify the object coming back from the kernel.

Eric suggested to accept a "cookie" property with arbitrary value to
stay in place at least for echo output. He even suggested to accept this
as an alternative to the handle for rule referencing. The latter would
need kernel support, though.

> > > This is also saving quite a bit of code and streamlining this further:
> > > 
> > >  4 files changed, 49 insertions(+), 153 deletions(-)
> > 
> > Proudly presenting reduced code size by dropping functionality is
> > cheating. Assume nobody needs the JSON interface, easily drop 5k LoC.
> 
> The existing approach ignores the kernel echo netlink message almost
> entirely, it only takes the handle.

I know, I wrote the code.

> We need an unified way to deal with --json --echo, whether the input
> is native nft or json syntax.

We don't need, but seems we want. We have JSON output and JSON echo for
a while now and code for both is distinct. I fail to see why this was OK
but is no longer. From my perspective, Jose simply failed to see that
JSON output code should be used for JSON echo if input is not JSON.

I could come up with a patch implementing that if all this is merely
about the missing feature.

> If the problem is described in the question I made above, how will
> users passing native nft syntax and requesing json output will
> identify the rule? They cannot make string matching comparison in that
> case since there is no input JSON representation.

That is not a sensible use-case. For once, I would assume native syntax
to be used by humans, so if this is combined with JSON output, the goal
is translation. If input really comes from a script, it is likely not
retaining the input for later reuse but will take whatever it receives
back.

Cheers, Phil