Netfilter-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Pablo Neira Ayuso <pablo@netfilter.org>
To: Phil Sutter <phil@nwl.cc>,
	"Jose M. Guisado Gomez" <guigom@riseup.net>,
	netfilter-devel@vger.kernel.org, Eric Garver <erig@erig.me>
Subject: Re: [PATCH nft v2 1/1] src: enable output with "nft --echo --json" and nftables syntax
Date: Sat, 1 Aug 2020 21:27:30 +0200
Message-ID: <20200801192730.GA5485@salvia> (raw)
In-Reply-To: <20200801000213.GN13697@orbyte.nwl.cc>

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.

BTW, why do you qualify this as a bug?

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

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.

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

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

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.

Thanks.

  reply index

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-30 19:53 [PATCH nft] " Jose M. Guisado Gomez
2020-07-31  0:00 ` [PATCH nft v2 0/1] " Jose M. Guisado Gomez
2020-07-31  0:00 ` [PATCH nft v2 1/1] " Jose M. Guisado Gomez
2020-07-31  9:22   ` Pablo Neira Ayuso
2020-07-31 10:49     ` [PATCH nft v3] " Jose M. Guisado Gomez
2020-08-04 10:38       ` [PATCH nft v4] src: enable json echo output when reading native syntax Jose M. Guisado Gomez
2020-08-04 11:05         ` Pablo Neira Ayuso
2020-08-04 12:13           ` Jose M. Guisado
2020-08-04 12:15             ` Pablo Neira Ayuso
2020-08-04 12:37         ` Phil Sutter
2020-08-04 13:05           ` Jose M. Guisado
2020-08-04 13:14             ` Phil Sutter
2020-08-04 13:44               ` Jose M. Guisado
2020-08-04 14:04                 ` Pablo Neira Ayuso
2020-08-04 14:17                   ` Pablo Neira Ayuso
2020-08-04 14:20                   ` Phil Sutter
2020-08-04 15:47                     ` Jose M. Guisado
2020-08-04 12:57         ` Eric Garver
2020-07-31 12:33     ` [PATCH nft v2 1/1] src: enable output with "nft --echo --json" and nftables syntax Phil Sutter
2020-07-31 12:58       ` Pablo Neira Ayuso
2020-07-31 13:48         ` Phil Sutter
2020-07-31 14:17           ` Eric Garver
2020-07-31 17:19             ` Pablo Neira Ayuso
2020-07-31 18:36               ` Eric Garver
2020-07-31 20:14                 ` Eric Garver
2020-07-31 17:30           ` Pablo Neira Ayuso
2020-08-01  0:02             ` Phil Sutter
2020-08-01 19:27               ` Pablo Neira Ayuso [this message]
2020-08-03 12:52                 ` Phil Sutter
2020-08-04 10:20                   ` Jose M. Guisado
2020-08-04 10:32                     ` Phil Sutter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200801192730.GA5485@salvia \
    --to=pablo@netfilter.org \
    --cc=erig@erig.me \
    --cc=guigom@riseup.net \
    --cc=netfilter-devel@vger.kernel.org \
    --cc=phil@nwl.cc \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Netfilter-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netfilter-devel/0 netfilter-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netfilter-devel netfilter-devel/ https://lore.kernel.org/netfilter-devel \
		netfilter-devel@vger.kernel.org
	public-inbox-index netfilter-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netfilter-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git