On Fri, May 24, 2019 at 11:06:34PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> > On Sun, May 19, 2019 at 07:18:38PM +0200, Florian Westphal wrote:
> > > convert "ip saddr { 1.1.1.1 }" to "ip saddr 1.1.1.1".
> > > Both do the same, but second form is faster since no single-element
> > > anon set is created.
> > >
> > > Fix up the remaining test cases to expect transformations of the form
> > > "meta l4proto { 33-55}" to "meta l4proto 33-55".
> >
> > Last time we discussed this I think we agreed to spew a warning for
> > this to educate people on this.
>
> I decided against it.
> Why adding a warning? We do not change what the rule does, and we do
> not collapse different rules into one.
Yes, there is no semantic change.
Should we also do transparent transformation for
ct state { established,new }
I have seen rulesets like this too.
> > My concern is: This is an optimization, are we going to do transparent
> > optimizations of the ruleset? I would like to explore at some point
> > automatic transformations for rulesets, also spot shadowed rules,
> > overlaps, and other sort of inconsistencies.
> >
> > Are we going to do all that transparently?
>
> I think it could be done on a case-by-case basis.
OK.
> > Asking this because this is an optimization after all, and I'm not
> > sure I want to step in into making optimizations transparently. Even
> > if this one is fairly trivial.
> >
> > I also don't like this path because we introduce one more assymmetry
> > between what the user adds a what the user fetches from the kernel.
>
> nft add rule ip filter input ip protocol tcp tcp dport 22
> -> tcp dport 22
>
> nft add rule inet filter input tcp dport 22 tcp sport 55
> -> tcp sport 55 tcp dport 22
>
> I don't think there is any point in warning about this, so
> why warn about tcp dport { 22 } ?
>
> A warning should, IMO, be reserved for something where we
> detect a actual problem, e.g.
>
> tcp dport 22 tcp dport 55
>
> and then we could print a 'will never match' warning for nft -f,
> and maybe even refuse it for "nft add rule .."
>
> Full ruleset optimization should, IMO, be done transparently as
> well if we're confident such transformations are done correctly.
>
> I would however only do it for nft -f, not for "nft add rule ..."
Why only for nft -f and not for "nft add rule" ?
> I see rulesets appearing that are very iptables like, e.g.
>
> tcp dport 22 accept
> tcp dport 80 accept
>
> and so on, and I think it makes sense to allow nft to compact this.
Yes, we can do transformations for that case too.
> We could add optimization level support to nft, i.e. only do basic
> per-rule and do cross-rule optimizations only with a command line parameter.
We can add a new parameter to optimize rulesets, we can start with
something simple, ie.
* collapse consecutive several rules that come with the same
selectors, only values change.
* turn { 22 } into 22.
* turn ct state {new, established } into ct new,established.
> I think a trivial one such as s/{ 22 }/22/ should just be done automatically.
We could, yes. But I would prefer if we place all optimizations under
some new option and we document them.
I was expecting to have a discussion on this during the NFWS :-).
