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

[Phil Sutter <phil@nwl.cc>]

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:
> > Hi,
> > 
> > On Fri, Jul 31, 2020 at 11:22:12AM +0200, Pablo Neira Ayuso wrote:
> > > On Fri, Jul 31, 2020 at 02:00:22AM +0200, Jose M. Guisado Gomez wrote:
> > > > diff --git a/src/parser_json.c b/src/parser_json.c
> > > > index 59347168..237b6f3e 100644
> > > > --- a/src/parser_json.c
> > > > +++ b/src/parser_json.c
> > > > @@ -3884,11 +3884,15 @@ int json_events_cb(const struct nlmsghdr *nlh, struct netlink_mon_handler *monh)
> > > >
> > > >  void json_print_echo(struct nft_ctx *ctx)
> > > >  {
> > > > -	if (!ctx->json_root)
> > > > +	if (!ctx->json_echo)
> > > >		return;
> > 
> > Why not reuse json_root?
> 
> Now that json_root is released from nft_parse_json_buffer() that is
> possible, yes.
> 
> However, it is only possible to reuse ctx->json_root if the
> ctx->json_root is released right after the parsing.
> 
> Otherwise the semantics of ctx->json_root starts getting confusing.

Hmm. Maybe I miss something, I just noticed that json_print_echo()
doesn't make use of json_root anymore.

[...]
> > > @Phil, I think the entire assoc code can just go away? Maybe you can also
> > > run firewalld tests to make sure v3 works fine?  IIRC that is a heavy user
> > > of --echo and --json.
> > 
> > Keeping JSON input in place and merely updating it with handles
> > retrieved from kernel was a deliberate choice to make sure scripts can
> > rely upon echo output to not differ from input unexpectedly.
> 
> Hm, this is not trusting what the kernel is sending to us via echo.
> And this approach differs from what it is done for --echo with native
> nft syntax.

We agreed that regular nft output is made for humans, not machines.
Hence why we have libnftables and JSON API. Very simple scripts may get
by with regular output, grepping for 'handle <NUM>' and ignoring the
rest. When loading more than a single command, this quickly becomes
inconvenient.

> > Given that output often deviates from input due to rule optimizing
> > or loss of information, I'd say this code change will break that
> > promise. Can't we enable JSON echo with non-JSON input while
> > upholding it?
> 
> I would prefer to remove this code. What is your concern?

The less predictable echo output behaves, the harder it is to write code
that makes use of it. The whole handle semantics assumes scripts will be
able to fetch handles from echo output. With output being identical to
input apart from added handles, scripts may just replace their input
with received output and will find everything where it is supposed to
be. The alternative means extracting handles from output and updating
the stored input based on array indexes. Basically libnftables does this
updating for users as a service, and the code in commit 50b5b71ebeee3
("parser_json: Rewrite echo support") is probably more efficient than
what any Python script could do.

Maybe I am over-estimating the importance of handle usability. The fact
that people have to use a rule's handle in order to remove it means to
me that they will either find a way to get the handle of the rule they
added or fall back into a write-only usage-pattern which means dropping
the whole chain/table/ruleset for each change. Basically what iptables
does internally.

I'm assuming scripts will work directly with the Python data structures
that are later passed to libnftables as JSON. If they want to change a
rule, e.g. add a statement, it is no use if other statements disappear
or new ones are added by the commit->retrieve action.

Maybe Eric can shed some light on how Firewalld uses echo mode and
whether my concerns are relevant or not.

Cheers, Phil