Re: [PATCH] MINOR: acl: Warn when an ACL is named 'or' or '||'

2020-02-06 Thread Tim Düsterhus
Willy,

Am 06.02.20 um 16:37 schrieb Willy Tarreau:
> Maybe you didn't reload the page or ended on an older tab ?
> 

Apparently it was my browser cache. Hitting reload several times made it
show up. All good now!

Best regards
Tim Düsterhus



Re: [PATCH] MINOR: acl: Warn when an ACL is named 'or' or '||'

2020-02-06 Thread Willy Tarreau
On Thu, Feb 06, 2020 at 04:31:36PM +0100, Tim Düsterhus wrote:
> The committed patch is looking good. I'm not seeing anything in 2.1
> though, did you forget to push that branch?

No here it's pushed:

  commit 9007a8d65d56e670aad97f60b66c36a52aab22c5 (HEAD -> master, flx04/master)
  Author: Tim Duesterhus 
  Date:   Wed Feb 5 21:00:50 2020 +0100

MINOR: acl: Warn when an ACL is named 'or'

(flx04 is the remote's name). And I'm seeing it at the top here:

http://git.haproxy.org/?p=haproxy-2.1.git;a=summary

Maybe you didn't reload the page or ended on an older tab ?

Willy



Re: [PATCH] MINOR: acl: Warn when an ACL is named 'or' or '||'

2020-02-06 Thread Tim Düsterhus
Willy,

Am 06.02.20 um 16:19 schrieb Willy Tarreau:
> On Thu, Feb 06, 2020 at 03:42:17PM +0100, Willy Tarreau wrote:
>>> 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.
> 
> I did what I mentioned and backported it as far as 1.8 (with warnings
> for stable releases).

The committed patch is looking good. I'm not seeing anything in 2.1
though, did you forget to push that branch?

Best regards
Tim Düsterhus



Re: [PATCH] MINOR: acl: Warn when an ACL is named 'or' or '||'

2020-02-06 Thread Willy Tarreau
On Thu, Feb 06, 2020 at 03:42:17PM +0100, Willy Tarreau wrote:
> > 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.

I did what I mentioned and backported it as far as 1.8 (with warnings
for stable releases).

Thanks,
Willy



Re: [PATCH] MINOR: acl: Warn when an ACL is named 'or' or '||'

2020-02-06 Thread Willy Tarreau
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.

Thanks!
Willy



Re: [PATCH] MINOR: acl: Warn when an ACL is named 'or' or '||'

2020-02-06 Thread Tim Düsterhus
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.

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

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

Best regards
Tim Düsterhus



Re: [PATCH] MINOR: acl: Warn when an ACL is named 'or' or '||'

2020-02-06 Thread Willy Tarreau
Hi Tim,

On Wed, Feb 05, 2020 at 09:00:50PM +0100, Tim Duesterhus wrote:
(...)
> Consider a configuration like this:
> 
> > acl t always_true
> > acl or always_false
> >
> > http-response set-header Foo Bar if t or t
> 
> The 'or' within the condition will be treated as a logical disjunction
> and the header will be set, despite the ACL 'or' being falsy.

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

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.

> May be backported to older branches, it should not break anything
> and might improve the users' lifes.

I also think we should, indeed.

Willy