Re: [PATCH] MINOR: acl: Warn when an ACL is named 'or' or '||'
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 '||'
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 '||'
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 '||'
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 '||'
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 '||'
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 '||'
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
[PATCH] MINOR: acl: Warn when an ACL is named 'or' or '||'
Willy,
after a user seeked support in IRC and me seeing the error message they were
facing I just *had to*
try something :-)
<*redacted*> Can anyone tell me if there a way to do multi variable in this
rspirep line? Seems
to be a syntax problem with the AND path_subdomain.
rspirep ^Location:\ (.*) Location:\ /subdomainXYZ/\1 if
hdr_location AND path_subdomain
[...]
Also the 'AND' is implicit. Just leave it out.
That will probably solve your syntax issue.
<*redacted*> Ahhh thanks, it seems that removing the AND was enough to get it
working
<*redacted*> was admittedly confused when I saw the error 'no such ACL: AND'
*redacted*, don't make me try out stupid things. Quick poll (answer
without trying it out!).
Will this add a 'Foo' header to the response or not?
acl t always_true
acl or always_false
http-response set-header Foo Bar if t or t
<*redacted*> maybe.. no?
<*redacted2*> you're supposed to use || and or for OR, so I'd guess cfgparse
should fail here, does it not ?
It does not. I'm just preparing a patch.
<*redacted2*> :-)
<*redacted3*> TimWolla: famoous last words
<*redacted3*> me is unfair
Apply with `git am --scissors` to automatically cut the commit message.
-- >8 --
Subject: [PATCH] MINOR: acl: Warn when an ACL is named 'or' or '||'
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.
May be backported to older branches, it should not break anything
and might improve the users' lifes.
---
src/cfgparse-listen.c | 7 ++-
src/fcgi-app.c| 10 +-
src/flt_spoe.c| 6 ++
3 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/src/cfgparse-listen.c b/src/cfgparse-listen.c
index 3f16a2517..7220b50ec 100644
--- a/src/cfgparse-listen.c
+++ b/src/cfgparse-listen.c
@@ -806,7 +806,12 @@ int cfg_parse_listen(const char *file, int linenum, char
**args, int kwm)
err_code |= ERR_ALERT | ERR_FATAL;
goto out;
}
-
+ if (strcasecmp(args[1], "or") == 0 || strcasecmp(args[1], "||")
== 0) {
+ ha_warning("parsing [%s:%d] : acl name '%s' will never
match. 'or' and '||' are used to express a "
+ "logical disjunction within a condition.\n",
+ file, linenum, args[1]);
+ err_code |= ERR_WARN;
+ }
if (parse_acl((const char **)args + 1, >acl, ,
>conf.args, file, linenum) == NULL) {
ha_alert("parsing [%s:%d] : error detected while
parsing ACL '%s' : %s.\n",
file, linenum, args[1], errmsg);
diff --git a/src/fcgi-app.c b/src/fcgi-app.c
index f7108c376..873fc0f13 100644
--- a/src/fcgi-app.c
+++ b/src/fcgi-app.c
@@ -885,11 +885,19 @@ static int cfg_parse_fcgi_app(const char *file, int
linenum, char **args, int kw
ha_alert("parsing [%s:%d] : character '%c' is not
permitted in acl name '%s'.\n",
file, linenum, *err, args[1]);
err_code |= ERR_ALERT | ERR_FATAL;
+ goto out;
+ }
+ if (strcasecmp(args[1], "or") == 0 || strcasecmp(args[1], "||")
== 0) {
+ ha_warning("parsing [%s:%d] : acl name '%s' will never
match. 'or' and '||' are used to express a "
+ "logical disjunction within a condition.\n",
+ file, linenum, args[1]);
+ err_code |= ERR_WARN;
}
- else if (parse_acl((const char **)args+1, >acls,
, >conf.args, file, linenum) == NULL) {
+ if (parse_acl((const char **)args+1, >acls, ,
>conf.args, file, linenum) == NULL) {
ha_alert("parsing [%s:%d] : error detected while
parsing ACL '%s' : %s.\n",
file, linenum, args[1], errmsg);
err_code |= ERR_ALERT | ERR_FATAL;
+ goto out;
}
}
else if (!strcmp(args[0], "set-param")) {
diff --git a/src/flt_spoe.c b/src/flt_spoe.c
index e3328cc01..3951e6527 100644
--- a/src/flt_spoe.c
+++ b/src/flt_spoe.c
@@ -3991,6 +3991,12 @@ cfg_parse_spoe_message(const char *file, int linenum,
char **args, int kwm)
err_code |= ERR_ALERT | ERR_FATAL;
goto out;
}
+ if (strcasecmp(args[1], "or") == 0 || strcasecmp(args[1], "
