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

[Eric Garver <eric@garver.life>]

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

Agreed. This would be very bad for usability.

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

How it stands today is exactly as you described above. firewalld relies
on the output (--echo) being in the same order as the input. At the
time, and I think still today, this was the _only_ way to reliably get
the rule handles. It's mostly due to the fact that input != output.

In the past we discussed allowing a user defined cookie/handle. This
would allow applications to perform in a write only manner. They would
not need to parse back the JSON since they already know the
cookie/handle. IMO, this would be ideal for firewalld's use case.