On Wed, Aug 14, 2024 at 06:00:55PM +0200, Phil Sutter wrote:
> On Wed, Aug 14, 2024 at 01:51:21PM +0200, Pablo Neira Ayuso wrote:
> > Bail out if rate are used:
> >
> > ruleset.nft:5:77-106: Error: Wrong rate format, expecting bytes or kbytes or mbytes
> > add rule netdev firewall PROTECTED_IPS update @quota_temp_before { ip daddr quota over 45000 mbytes/second } add @quota_trigger { ip daddr }
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> > improve error reporting while at this.
> >
> > Fixes: 6615676d825e ("src: add per-bytes limit")
> > Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> > ---
> > v2: - change patch subject
> > - use strndup() to fetch units in rate_parse() so limit rate does not break.
> >
> > src/datatype.c | 20 +++++++++++++-------
> > 1 file changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/datatype.c b/src/datatype.c
> > index d398a9c8c618..297c5d0409d5 100644
> > --- a/src/datatype.c
> > +++ b/src/datatype.c
> > @@ -1485,14 +1485,14 @@ static struct error_record *time_unit_parse(const struct location *loc,
> > struct error_record *data_unit_parse(const struct location *loc,
> > const char *str, uint64_t *rate)
> > {
> > - if (strncmp(str, "bytes", strlen("bytes")) == 0)
> > + if (strcmp(str, "bytes") == 0)
> > *rate = 1ULL;
> > - else if (strncmp(str, "kbytes", strlen("kbytes")) == 0)
> > + else if (strcmp(str, "kbytes") == 0)
> > *rate = 1024;
> > - else if (strncmp(str, "mbytes", strlen("mbytes")) == 0)
> > + else if (strcmp(str, "mbytes") == 0)
> > *rate = 1024 * 1024;
> > else
> > - return error(loc, "Wrong rate format");
> > + return error(loc, "Wrong unit format, expecting bytes, kbytes or mbytes");
> >
> > return NULL;
> > }
>
> I have local commits which introduce KBYTES and MBYTES keywords and
> thereby kill the need for quota_unit and limit_bytes cases in
> parser_bison.y. It still needs testing and is surely not solving all
> issues there are, but I find it nicer than the partially redundant code
> we have right now.
Is this allowing for compact representation? ie. kbytes/second,
because I remember this was the issue to follow this poor man
approach.
> My motivation for this was to maybe improve parser's ability to handle
> lack of spaces in input. I still see the scanner fall into the generic
> "string" token case which requires manual dissection in the parser.
>
> What is your motivation for the above changes?
A user that accidentally used:
quota over 10mbytes/second
which is currently accepted, the /second part is misleading, as the
commit described, this is now rejected after this patch.
> Maybe we could collect parser limitations around these units and see
> what helps "the most"?
I am fine with handling this from the parser instead, there is now
flex start conditions that can help to enable these tokens on demand.
