On Thu, Feb 06, 2020 at 03:33:45PM +0100, Tim Düsterhus wrote:
> Willy,
> Am 06.02.20 um 15:24 schrieb Willy Tarreau:
> > I was surprized because I discovered that we've always accepted empty
> > expressions thus"if" without even a condition or "if or" are valid, which
> > adds to the confusion. So your patch does indeed make sense. However we
> > don't need to test for "||" because it's already invalid as an ACL name
> > and the existing message is more precise about it (invalid chars in the
> > name).
> I added the test for '||' for completeness in case there are adjustments
> to valid characters later on. I am aware that it will never trigger and
> being only run on configuration parsing the performance impact should be
> a non-issue.

It's not a matter of performance impact but that the message is misleading
in this case. It says that "||" is forbidden as an ACL name, which sort
of implies a lot of other ones are valid. It's not "||" that's forbidden,
it's anything not made exclusively of "[0-9a-zA-Z_.:-]", and actually your
message scared me and made me recheck the doc to be sure.

> > Also I'd rather have the "goto out" statement after setting the error,
> > otherwise it becomes confusing and will certainly trap someone adding
> > yet another test later.
> So you want to make using 'or' as an ACL name fatal instead of a warning?

Ah silly me! I misread it, I read only the first one as a warning and
the second one as an error. However you're making me think now :-)

We should likely make it an error in 2.2 and a warning in existing branches.
We know that people don't read warnings, so if we keep it forever, such
a config might last. But we must not break an existing config in a stable
branch otherwise users roll back by lack of time to fix the conf.

> Feel free to either modify the patch yourself to make the adjustments
> you want to see or let me know if you want me to make them. It might
> require a bit more back and forth, though :-)

OK I guess I can do them, sure.


Reply via email to