Re: [PATCH] BUG/MINOR: server: fix slowstart behavior
Hi Damien, On Tue, Apr 09, 2024 at 03:37:07PM +, Damien Claisse wrote: > We observed that a dynamic server which health check is down for longer > than slowstart delay at startup doesn't trigger the warmup phase, it > receives full traffic immediately. This has been confirmed by checking > haproxy UI, weight is immediately the full one (e.g. 75/75), without any > throttle applied. Further tests showed that it was similar if it was in > maintenance, and even when entering a down or maintenance state after > being up. > Another issue is that if the server is down for less time than > slowstart, when it comes back up, it briefly has a much higher weight > than expected for a slowstart. > > An easy way to reproduce is to do the following: > - Add a server with e.g. a 20s slowstart and a weight of 10 in config > file > - Put it in maintenance using CLI (set server be1/srv1 state maint) > - Wait more than 20s, enable it again (set server be1/srv1 state ready) > - Observe UI, weight will show 10/10 immediately. > If server was down for less than 20s, you'd briefly see a weight and > throttle value that is inconsistent, e.g. 50% throttle value and a > weight of 5 if server comes back up after 10s before going back to > 6% after a second or two. > > Code analysis shows that the logic in server_recalc_eweight stops the > warmup task by setting server's next state to SRV_ST_RUNNING if it > didn't change state for longer than the slowstart duration, regardless > of its current state. As a consequence, a server being down or disabled > for longer than the slowstart duration will never enter the warmup phase > when it will be up again. > > Regarding the weight when server comes back up, issue is that even if > the server is down, we still compute its next weight as if it was up, > hence when it comes back up, it can briefly have a much higher weight > than expected during slowstart, until the warmup task is called again > after last_change is updated. > > This patch aims to fix both issues. (...) You analysis makes a lot of sense, and I'm not much surprised that this has been revealed by dynamic servers, because the startup sequence before them was very stable and well established. So for sure if certain parts start in a different order it can have such visible effects. Good catch! It's now merged, thank you! Willy
[PATCH] BUG/MINOR: server: fix slowstart behavior
We observed that a dynamic server which health check is down for longer
than slowstart delay at startup doesn't trigger the warmup phase, it
receives full traffic immediately. This has been confirmed by checking
haproxy UI, weight is immediately the full one (e.g. 75/75), without any
throttle applied. Further tests showed that it was similar if it was in
maintenance, and even when entering a down or maintenance state after
being up.
Another issue is that if the server is down for less time than
slowstart, when it comes back up, it briefly has a much higher weight
than expected for a slowstart.
An easy way to reproduce is to do the following:
- Add a server with e.g. a 20s slowstart and a weight of 10 in config
file
- Put it in maintenance using CLI (set server be1/srv1 state maint)
- Wait more than 20s, enable it again (set server be1/srv1 state ready)
- Observe UI, weight will show 10/10 immediately.
If server was down for less than 20s, you'd briefly see a weight and
throttle value that is inconsistent, e.g. 50% throttle value and a
weight of 5 if server comes back up after 10s before going back to
6% after a second or two.
Code analysis shows that the logic in server_recalc_eweight stops the
warmup task by setting server's next state to SRV_ST_RUNNING if it
didn't change state for longer than the slowstart duration, regardless
of its current state. As a consequence, a server being down or disabled
for longer than the slowstart duration will never enter the warmup phase
when it will be up again.
Regarding the weight when server comes back up, issue is that even if
the server is down, we still compute its next weight as if it was up,
hence when it comes back up, it can briefly have a much higher weight
than expected during slowstart, until the warmup task is called again
after last_change is updated.
This patch aims to fix both issues.
---
src/server.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/server.c b/src/server.c
index ea93e1ff4..529fcf169 100644
--- a/src/server.c
+++ b/src/server.c
@@ -2279,15 +2279,17 @@ void server_recalc_eweight(struct server *sv, int
must_update)
unsigned w;
if (ns_to_sec(now_ns) < sv->last_change || ns_to_sec(now_ns) >=
sv->last_change + sv->slowstart) {
- /* go to full throttle if the slowstart interval is reached */
- if (sv->next_state == SRV_ST_STARTING)
+ /* go to full throttle if the slowstart interval is reached
unless server is currently down */
+ if ((sv->cur_state != SRV_ST_STOPPED) && (sv->next_state ==
SRV_ST_STARTING))
sv->next_state = SRV_ST_RUNNING;
}
/* We must take care of not pushing the server to full throttle during
slow starts.
* It must also start immediately, at least at the minimal step when
leaving maintenance.
*/
- if ((sv->next_state == SRV_ST_STARTING) && (px->lbprm.algo &
BE_LB_PROP_DYN))
+ if ((sv->cur_state == SRV_ST_STOPPED) && (sv->next_state ==
SRV_ST_STARTING) && (px->lbprm.algo & BE_LB_PROP_DYN))
+ w = 1;
+ else if ((sv->next_state == SRV_ST_STARTING) && (px->lbprm.algo &
BE_LB_PROP_DYN))
w = (px->lbprm.wdiv * (ns_to_sec(now_ns) - sv->last_change) +
sv->slowstart) / sv->slowstart;
else
w = px->lbprm.wdiv;
--
2.34.1
Re: [PATCH] BUG/MINOR: server: fix persistence cookie for dynamic servers
On Wed, Mar 27, 2024 at 02:34:25PM +, Damien Claisse wrote:
> When adding a server dynamically, we observe that when a backend has a
> dynamic persistence cookie, the new server has no cookie as we receive
> the following HTTP header:
> set-cookie: test-cookie=; Expires=Thu, 01-Jan-1970 00:00:01 GMT; path=/
> Whereas we were expecting to receive something like the following, which
> is what we receive for a server added in the config file:
> set-cookie: test-cookie=abcdef1234567890; path=/
> After investigating code path, srv_set_dyncookie() is never called when
> adding a server through CLI, it is only called when parsing config file
> or using "set server bkd1/srv1 addr".
> To fix this, call srv_set_dyncookie() inside cli_parse_add_server().
> This patch must be backported up to 2.4.
> ---
> src/server.c | 5 +
> 1 file changed, 5 insertions(+)
> diff --git a/src/server.c b/src/server.c
> index 555cae82c..a93798f03 100644
> --- a/src/server.c
> +++ b/src/server.c
> @@ -5732,6 +5732,11 @@ static int cli_parse_add_server(char **args, char
> *payload, struct appctx *appct
>*/
> srv->rid = (srv_id_reuse_cnt) ? (srv_id_reuse_cnt / 2) : 0;
>
> + /* generate new server's dynamic cookie if enabled on backend */
> + if (be->ck_opts & PR_CK_DYNAMIC) {
> + srv_set_dyncookie(srv);
> + }
> +
> /* adding server cannot fail when we reach this:
>* publishing EVENT_HDL_SUB_SERVER_ADD
>*/
> --
> 2.34.1
>
Thank you very much. This was merged in our development tree. In the
meantime, I also enabled "cookie" keyword for dynamic servers as nothing
prevented it.
--
Amaury Denoyelle
[PATCH] BUG/MINOR: server: fix persistence cookie for dynamic servers
When adding a server dynamically, we observe that when a backend has a
dynamic persistence cookie, the new server has no cookie as we receive
the following HTTP header:
set-cookie: test-cookie=; Expires=Thu, 01-Jan-1970 00:00:01 GMT; path=/
Whereas we were expecting to receive something like the following, which
is what we receive for a server added in the config file:
set-cookie: test-cookie=abcdef1234567890; path=/
After investigating code path, srv_set_dyncookie() is never called when
adding a server through CLI, it is only called when parsing config file
or using "set server bkd1/srv1 addr".
To fix this, call srv_set_dyncookie() inside cli_parse_add_server().
This patch must be backported up to 2.4.
---
src/server.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/src/server.c b/src/server.c
index 555cae82c..a93798f03 100644
--- a/src/server.c
+++ b/src/server.c
@@ -5732,6 +5732,11 @@ static int cli_parse_add_server(char **args, char
*payload, struct appctx *appct
*/
srv->rid = (srv_id_reuse_cnt) ? (srv_id_reuse_cnt / 2) : 0;
+ /* generate new server's dynamic cookie if enabled on backend */
+ if (be->ck_opts & PR_CK_DYNAMIC) {
+ srv_set_dyncookie(srv);
+ }
+
/* adding server cannot fail when we reach this:
* publishing EVENT_HDL_SUB_SERVER_ADD
*/
--
2.34.1
Re: [PATCH] BUG/MINOR: server: fix persistence cookie for dynamic servers
On Fri, Mar 22, 2024 at 09:45:59AM +, Damien Claisse wrote: > Hi Amaury! > Sorry for the HTML message, I have to use a *** well-known enterprise MUA :( > Le 22/03/2024 09:09, « Amaury Denoyelle » a écrit : >> This patch raises several interrogations. First, dynamic servers are >> currently intended to not support cookies, hence why the keyword is >> disabled for them. It has been done as a convenience but maybe it would >> be a good time to review it carefully and see if whole cookie support >> can be enabled. > Indeed, there could be an opportunity to revisit this. What we observed is > that, even with dynamic servers, calling “set server bkd1/srv1 addr a.b.c.d” > would re-add the dynamic cookie for this server, and calling “enable > dynamic-cookie backend bkd1” would re-compute cookie for all servers in the > backend. It is expected as in these calls code path there is a call to > srv_set_dyncookie(). So there currently is at least a partial support for > dynamic cookies on dynamic servers, even if it’s not expected :) >> Second, I'm unsure srv_set_dyncookie() should be called on >> _srv_parse_init(). This function is also called for configuration file >> servers. In particular, I do not know how we should handled duplicate >> cookie values in this case. > Not sure we really create duplicates here as we basically reset the same > cookie on the server, not on another one in the backend, at least I didn’t > observe such warnings in my logs while testing this patch yet. But I agree > that in the context of haproxy startup there would be 2 calls to > srv_set_dyncookie() per server which is useless and could create issues. > Maybe I could move that at the end of cli_parse_add_server()? Feel free to > suggest any better option. Okay, I had the time to review dynamic cookie handling. For me it's fine to use srv_set_dyncookie() for dynamic servers. I think however its new invocation should be placed near the end of cli_parse_add_server(). Maybe with an extra check (be->ck_opts & PR_CK_DYNAMIC) to be similar with invocation in check_config_validity(). This location will prevent duplicate invocation of srv_set_dyncookie() for static servers, as for them the function must be called on post parsing to ensure the backend key has been parsed. Could you please adjust your patch ? Also, as it is a bugfix, you can specify that it must be backported up to 2.4. In the meantime, I will continue code inspection to determine if cookie keyword can also be enabled for dynamic servers as well. Regards, -- Amaury Denoyelle
Re: [PATCH] BUG/MINOR: server: fix persistence cookie for dynamic servers
Hi Amaury! Sorry for the HTML message, I have to use a *** well-known enterprise MUA :( Le 22/03/2024 09:09, « Amaury Denoyelle » a écrit : This patch raises several interrogations. First, dynamic servers are currently intended to not support cookies, hence why the keyword is disabled for them. It has been done as a convenience but maybe it would be a good time to review it carefully and see if whole cookie support can be enabled. Indeed, there could be an opportunity to revisit this. What we observed is that, even with dynamic servers, calling “set server bkd1/srv1 addr a.b.c.d” would re-add the dynamic cookie for this server, and calling “enable dynamic-cookie backend bkd1” would re-compute cookie for all servers in the backend. It is expected as in these calls code path there is a call to srv_set_dyncookie(). So there currently is at least a partial support for dynamic cookies on dynamic servers, even if it’s not expected :) Second, I'm unsure srv_set_dyncookie() should be called on _srv_parse_init(). This function is also called for configuration file servers. In particular, I do not know how we should handled duplicate cookie values in this case. Not sure we really create duplicates here as we basically reset the same cookie on the server, not on another one in the backend, at least I didn’t observe such warnings in my logs while testing this patch yet. But I agree that in the context of haproxy startup there would be 2 calls to srv_set_dyncookie() per server which is useless and could create issues. Maybe I could move that at the end of cli_parse_add_server()? Feel free to suggest any better option. Cheers, Damien
Re: [PATCH] BUG/MINOR: server: fix persistence cookie for dynamic servers
On Thu, Mar 21, 2024 at 02:37:06PM +, Damien Claisse wrote: > When adding a server dynamically, we observe that when a backend has a > dynamic persistence cookie, the new server has no cookie as we receive > the following HTTP header: > set-cookie: test-cookie=; Expires=Thu, 01-Jan-1970 00:00:01 GMT; path=/ > Whereas we were expecting to receive something like the following, which > is what we receive for a server added in the config file: > set-cookie: test-cookie=abcdef1234567890; path=/ > After investigating code path, it seems srv_set_dyncookie() is never > called when adding a server through CLI, it is only called when parsing > config file or using "set server bkd1/srv1 addr". > To fix this, add a call to srv_set_dyncookie() inside _srv_parse_init() > which is used on the dynamic server initialization path. > --- > src/server.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > diff --git a/src/server.c b/src/server.c > index e22b33806..c954a8de3 100644 > --- a/src/server.c > +++ b/src/server.c > @@ -3306,7 +3306,9 @@ static int _srv_parse_init(struct server **srv, char > **args, int *cur_arg, > /* >* we don't need to lock the server here, because >* we are in the process of initializing. > - * > + */ > + srv_set_dyncookie(newsrv); > + /* >* Note that the server is not attached into the proxy tree if >* this is a dynamic server. >*/ > -- > 2.34.1 > Thanks for your contribution ! This patch raises several interrogations. First, dynamic servers are currently intended to not support cookies, hence why the keyword is disabled for them. It has been done as a convenience but maybe it would be a good time to review it carefully and see if whole cookie support can be enabled. Second, I'm unsure srv_set_dyncookie() should be called on _srv_parse_init(). This function is also called for configuration file servers. In particular, I do not know how we should handled duplicate cookie values in this case. -- Amaury Denoyelle
[PATCH] BUG/MINOR: server: fix persistence cookie for dynamic servers
When adding a server dynamically, we observe that when a backend has a dynamic persistence cookie, the new server has no cookie as we receive the following HTTP header: set-cookie: test-cookie=; Expires=Thu, 01-Jan-1970 00:00:01 GMT; path=/ Whereas we were expecting to receive something like the following, which is what we receive for a server added in the config file: set-cookie: test-cookie=abcdef1234567890; path=/ After investigating code path, it seems srv_set_dyncookie() is never called when adding a server through CLI, it is only called when parsing config file or using "set server bkd1/srv1 addr". To fix this, add a call to srv_set_dyncookie() inside _srv_parse_init() which is used on the dynamic server initialization path. --- src/server.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/server.c b/src/server.c index e22b33806..c954a8de3 100644 --- a/src/server.c +++ b/src/server.c @@ -3306,7 +3306,9 @@ static int _srv_parse_init(struct server **srv, char **args, int *cur_arg, /* * we don't need to lock the server here, because * we are in the process of initializing. -* +*/ + srv_set_dyncookie(newsrv); + /* * Note that the server is not attached into the proxy tree if * this is a dynamic server. */ -- 2.34.1
[PATCH] BUG/MEDIUM: Fix race condition between listener_accept and manage_proxy
Almost identical to 13e86d9 (BUG/MEDIUM: listener: Fix race condition
when updating the global mngmt task). When multiple `listener_accept`
threads attempt to schedule the proxy task which calls `manage_proxy`,
one listener thread may be sufficiently delayed to cause a race between
its call to `task_schedule` and `manage_proxy`'s clearing of the
`expire` field. This only occurs when `rate-limit sessions` is set and
the server is sufficiently loaded to cause the thread scheduling issues
resulting in the listener task delay. This results in the following
`BUG_ON` in the listener task:
FATAL: bug condition "task->expire == 0" matched at src/task.c:285
call trace(11):
| 0x56eb28 [0f 0b 66 0f 1f 44 00 00]: __task_queue+0xc8/0xca
| 0x553903 [eb 9c 0f 1f 00 ba 03 00]: main+0x1317e3
| 0x556de6 [8b 54 24 18 85 d2 0f 84]:
listener_accept+0x12c6/0x14f6
| 0x5ac348 [4d 8b 3c 24 49 01 df 64]:
fd_update_events+0x1a8/0x4a8
| 0x427d58 [48 39 eb 74 3b 66 66 66]: main+0x5c38
| 0x53801f [64 48 8b 04 25 00 00 00]: run_poll_loop+0x10f/0x647
| 0x5387f5 [49 c7 c4 c0 47 6a 00 49]: main+0x1166d5
| 0x7f7f662b4333 [e9 74 fe ff ff 48 8b 44]: libc:+0x8b333
| 0x7f7f66336efc [48 89 c7 b8 3c 00 00 00]: libc:+0x10defc
To fix it, we wrap the call to `task_schedule` in `listener_accept` and
the setting of `task->expire` in `manage_proxy` with the proxy's RW
lock. The same semantics are used as in 13e86d9. The `task_schedule`
call is read locked and the setting of `expire` to maybe-ETERNITY
is write locked.
This will fix issue #2289. It must be backported to 2.4 onward, and
should apply cleanly as the relevant code hasn't changed since then.
---
src/listener.c | 4
src/proxy.c| 2 ++
2 files changed, 6 insertions(+)
diff --git a/src/listener.c b/src/listener.c
index 86d0945da..594ed2c7f 100644
--- a/src/listener.c
+++ b/src/listener.c
@@ -1574,7 +1574,11 @@ void listener_accept(struct listener *l)
*/
limit_listener(l, >listener_queue);
if (p->task && tick_isset(expire))
+ {
+ HA_RWLOCK_RDLOCK(PROXY_LOCK, >lock);
task_schedule(p->task, expire);
+ HA_RWLOCK_RDUNLOCK(PROXY_LOCK, >lock);
+ }
goto end;
}
diff --git a/src/proxy.c b/src/proxy.c
index 6451cbb8a..25a6e4d99 100644
--- a/src/proxy.c
+++ b/src/proxy.c
@@ -2062,7 +2062,9 @@ struct task *manage_proxy(struct task *t, void
*context, unsigned int state)
/* The proxy is not limited so we can re-enable any waiting listener */
dequeue_proxy_listeners(p);
out:
+ HA_RWLOCK_WRLOCK(PROXY_LOCK, >lock);
t->expire = next;
+ HA_RWLOCK_WRUNLOCK(PROXY_LOCK, >lock);
task_queue(t);
return t;
}
--
2.43.0
[PATCH 1/1] BUG/MINOR: ssl: Update ssl_fc_curve/ssl_bc_curve to use SSL_get0_group_name
The function `smp_fetch_ssl_fc_ec` gets the curve name used during key
exchange. It currently uses the `SSL_get_negotiated_group`,
available since OpenSSLv3.0 to get the nid and derive the short name of the
curve from the nid. In OpenSSLv3.2, a new function,
`SSL_get0_group_name` was added that directly gives the curve name.
The function `smp_fetch_ssl_fc_ec` has been updated to use
`SSL_get0_group_name` if using OpenSSL>=3.2 and for versions >=3.0 and
< 3.2 use the old SSL_get_negotiated_group to get the curve name. Another
change made is to normalize the return value, so that
`smp_fetch_ssl_fc_ec` returns curve name in uppercase. (`SSL_get0_group_name`
returns the curve name in lowercase and
`SSL_get_negotiated_group` + `OBJ_nid2sn` returns curve name in uppercase).
---
src/ssl_sample.c | 36 +++-
1 file changed, 31 insertions(+), 5 deletions(-)
diff --git a/src/ssl_sample.c b/src/ssl_sample.c
index d7a7a09f9..38a931c95 100644
--- a/src/ssl_sample.c
+++ b/src/ssl_sample.c
@@ -1317,7 +1317,7 @@ smp_fetch_ssl_fc_ec(const struct arg *args, struct sample
*smp, const char *kw,
{
struct connection *conn;
SSL *ssl;
-int nid;
+char *curve_name;
if (obj_type(smp->sess->origin) == OBJ_TYPE_CHECK)
conn = (kw[4] == 'b') ? sc_conn(__objt_check(smp->sess->origin)->sc) :
NULL;
@@ -1329,10 +1329,36 @@ smp_fetch_ssl_fc_ec(const struct arg *args, struct
sample *smp, const char *kw,
if (!ssl)
return 0;
-nid = SSL_get_negotiated_group(ssl);
-if (!nid)
-return 0;
-smp->data.u.str.area = (char *)OBJ_nid2sn(nid);
+/*
+ * SSL_get0_group_name is a function to get the curve name and is
available from
+ * OpenSSL v3.2 onwards. For OpenSSL >=3.0 and <3.2, we will continue to
use
+ * SSL_get_negotiated_group to get the curve name.
+ */
+#if (HA_OPENSSL_VERSION_NUMBER >= 0x302fL)
+ curve_name = (char *)SSL_get0_group_name(ssl);
+ if (curve_name == NULL)
+ return 0;
+ else {
+ /**
+ * The curve name returned by SSL_get0_group_name is in
lowercase whereas the curve
+ * name returned when we use `SSL_get_negotiated_group` and
`OBJ_nid2sn` is the
+ * short name and is in upper case. To make the return value
consistent across the
+ * different functional calls and to make it consistent while
upgrading OpenSSL versions,
+ * will convert the curve name returned by SSL_get0_group_name
to upper case.
+ */
+ for (int i = 0; curve_name[i]; i++)
+ curve_name[i] = toupper(curve_name[i]);
+ }
+#else
+ int nid = SSL_get_negotiated_group(ssl);
+ if (!nid)
+ return 0;
+ curve_name = (char *)OBJ_nid2sn(nid);
+ if (curve_name == NULL)
+ return 0;
+#endif
+
+smp->data.u.str.area = curve_name;
if (!smp->data.u.str.area)
return 0;
--
2.39.2 (Apple Git-143)
[PATCH 1/1] BUG/MINOR: ssl: Update ssl_fc_curve/ssl_bc_curve to use SSL_get0_group_name
The function `smp_fetch_ssl_fc_ec` gets the curve name used during key
exchange. It currently uses the `SSL_get_negotiated_group`,
available since OpenSSLv3.0 to get the nid and derive the short name of the
curve from the nid. In OpenSSLv3.2, a new function,
`SSL_get0_group_name` was added that directly gives the curve name.
The function `smp_fetch_ssl_fc_ec` has been updated to use
`SSL_get0_group_name` if using OpenSSL>=3.2 and for versions >=3.0 and
< 3.2 use the old SSL_get_negotiated_group to get the curve name. Another
change made is to normalize the return value, so that
`smp_fetch_ssl_fc_ec` returns curve name in lowercase. (`SSL_get0_group_name`
returns the curve name in lowercase and
`SSL_get_negotiated_group` + `OBJ_nid2sn` returns curve name in uppercase).
Also updated the test case to reflect the changes.
---
reg-tests/ssl/ssl_curve_name.vtc | 4 ++--
src/ssl_sample.c | 33 +++-
2 files changed, 30 insertions(+), 7 deletions(-)
diff --git a/reg-tests/ssl/ssl_curve_name.vtc b/reg-tests/ssl/ssl_curve_name.vtc
index a285a8f86..c6242776f 100644
--- a/reg-tests/ssl/ssl_curve_name.vtc
+++ b/reg-tests/ssl/ssl_curve_name.vtc
@@ -44,8 +44,8 @@ client c1 -connect ${h1_clearlst_sock} {
txreq
rxresp
expect resp.status == 200
-expect resp.http.x-ssl-fc-curve-name == "X25519"
-expect resp.http.x-ssl-bc-curve-name == "X25519"
+expect resp.http.x-ssl-fc-curve-name == "x25519"
+expect resp.http.x-ssl-bc-curve-name == "x25519"
} -run
diff --git a/src/ssl_sample.c b/src/ssl_sample.c
index d7a7a09f9..a33188e92 100644
--- a/src/ssl_sample.c
+++ b/src/ssl_sample.c
@@ -1317,7 +1317,7 @@ smp_fetch_ssl_fc_ec(const struct arg *args, struct sample
*smp, const char *kw,
{
struct connection *conn;
SSL *ssl;
-int nid;
+char *curve_name;
if (obj_type(smp->sess->origin) == OBJ_TYPE_CHECK)
conn = (kw[4] == 'b') ? sc_conn(__objt_check(smp->sess->origin)->sc) :
NULL;
@@ -1329,10 +1329,33 @@ smp_fetch_ssl_fc_ec(const struct arg *args, struct
sample *smp, const char *kw,
if (!ssl)
return 0;
-nid = SSL_get_negotiated_group(ssl);
-if (!nid)
-return 0;
-smp->data.u.str.area = (char *)OBJ_nid2sn(nid);
+/*
+ * SSL_get0_group_name is a function to get the curve name and is
available from
+ * OpenSSL v3.2 onwards. For OpenSSL >=3.0 and <3.2, we will continue to
use
+ * SSL_get_negotiated_group to get the curve name.
+ */
+#if (HA_OPENSSL_VERSION_NUMBER >= 0x302fL)
+ curve_name = (char *)SSL_get0_group_name(ssl);
+#else
+ int nid = SSL_get_negotiated_group(ssl);
+ if (!nid)
+ return 0;
+ curve_name = (char *)OBJ_nid2sn(nid);
+ /**
+ * The curve name returned by SSL_get0_group_name is in lowercase
whereas the curve
+ * name returned when we use `SSL_get_negotiated_group` and `OBJ_nid2sn`
is the
+ * short name and is in upper case. The idea is to make the return value
consistent
+ * and stick to lowercase format.
+ */
+ if (curve_name == NULL)
+ return 0;
+ else {
+ for (int i = 0; curve_name[i]; i++)
+ curve_name[i] = tolower(curve_name[i]);
+ }
+#endif
+
+smp->data.u.str.area = curve_name;
if (!smp->data.u.str.area)
return 0;
--
2.39.2 (Apple Git-143)
[PR] BUG/MEDIUM: server-state: update server if the ports in config and state match
Dear list! Author: Steven Lu Number of patches: 1 This is an automated relay of the Github pull request: BUG/MEDIUM: server-state: update server if the ports in config and state match Patch title(s): BUG/MEDIUM: server-state: Only update server if the ports in config and state still match Link: https://github.com/haproxy/haproxy/pull/2370 Edit locally: wget https://github.com/haproxy/haproxy/pull/2370.patch && vi 2370.patch Apply locally: curl https://github.com/haproxy/haproxy/pull/2370.patch | git am - Description: BUG/MEDIUM: server-state: update server if the ports in config and state match When `load-server-state-from-file` is enabled, and change the backend port or check port in the config file (without altering the backend name and server name), restarting haproxy will not apply the port number changes from the new config file. This will result in users being unable to connect to the backend using the new port. The reason is that haproxy only uses the backend name and server name to decide whether to use state information. To fix the issue, we can only update the server info if the ports in the config and the state still match. This patch should solve the issue #2103. Instructions: This github pull request will be closed automatically; patch should be reviewed on the haproxy mailing list (haproxy@formilux.org). Everyone is invited to comment, even the patch's author. Please keep the author and list CCed in replies. Please note that in absence of any response this pull request will be lost.
[PATCH 2/3] BUG/MINOR: sample: Make the `word` converter compatible with `-m found`
Previously an expression like:
path,word(2,/) -m found
always returned `true`.
Bug exists since the `word` converter exists. That is:
c9a0f6d0232cf44d6b08d1964b9097a45a6c65f0
The same bug was previously fixed for the `field` converter in commit
4381d26edc03faa46401eb0fe82fd7be84be14fd.
The fix should be backported to 1.6+.
---
reg-tests/converter/word.vtc | 43
src/sample.c | 2 +-
2 files changed, 44 insertions(+), 1 deletion(-)
create mode 100644 reg-tests/converter/word.vtc
diff --git a/reg-tests/converter/word.vtc b/reg-tests/converter/word.vtc
new file mode 100644
index 00..acd46781f5
--- /dev/null
+++ b/reg-tests/converter/word.vtc
@@ -0,0 +1,43 @@
+varnishtest "word converter Test"
+
+feature ignore_unknown_macro
+
+server s1 {
+ rxreq
+ txresp -hdr "Connection: close"
+} -repeat 3 -start
+
+haproxy h1 -conf {
+defaults
+ mode http
+ timeout connect "${HAPROXY_TEST_TIMEOUT-5s}"
+ timeout client "${HAPROXY_TEST_TIMEOUT-5s}"
+ timeout server "${HAPROXY_TEST_TIMEOUT-5s}"
+
+frontend fe
+ bind "fd@${fe}"
+
+ requests
+ http-request set-var(txn.uri) path
+ http-response set-header Found %[var(txn.uri),word(2,/)] if {
var(txn.uri),word(2,/) -m found }
+
+ default_backend be
+
+backend be
+ server s1 ${s1_addr}:${s1_port}
+} -start
+
+client c1 -connect ${h1_fe_sock} {
+ txreq -url "/foo/bar/baz"
+ rxresp
+ expect resp.status == 200
+ expect resp.http.found == "bar"
+ txreq -url "/foo//bar/baz"
+ rxresp
+ expect resp.status == 200
+ expect resp.http.found == "bar"
+ txreq -url "/foo"
+ rxresp
+ expect resp.status == 200
+ expect resp.http.found == ""
+} -run
diff --git a/src/sample.c b/src/sample.c
index c8954ac476..29967e07d9 100644
--- a/src/sample.c
+++ b/src/sample.c
@@ -2964,7 +2964,7 @@ static int sample_conv_word(const struct arg *arg_p,
struct sample *smp, void *p
/* Field not found */
if (word != arg_p[0].data.sint) {
smp->data.u.str.data = 0;
- return 1;
+ return 0;
}
found:
smp->data.u.str.data = end - start;
--
2.42.0
Re: haproxy.org bug pages broken (missing html headers and footer?)
On Sat, Sep 30, 2023 at 11:06:28PM +0200, Willy Tarreau wrote: > On Sat, Sep 30, 2023 at 10:19:05AM +, Mathias Weiersmüller wrote: > > Hi Willy, > > > > > Argh, thanks for notifying us! Haproxy dev5 crashed leaving a huge core > > > that filled the FS (I hope it's complete, not checked yet), and the cron > > > job that rebuilds the bugs page miserably failed as you can see :-/ > > > > > > That's now fixed, thank you! > > > Willy > > > > the links to the respective bugs seem to be broken too, example: > > > > http://www.haproxy.org/bugs/://git.haproxy.org/?p=haproxy-2.6.git;a=commitdiff;h=dfa9730 > > > > it should be: > > https://git.haproxy.org/?p=haproxy-2.6.git;a=commitdiff;h=dfa9730 > > Actually it's different. Browsers changed "recently" regarding the handling > of schemeless URLs. In the past, using "://domain/path" would reuse the same > scheme (http or https) as the current page. Now it seems these are handled > as a relative path to the current page. I need to think about a simple way > for these to continue to work as in the past. OK, with href="//git.haproxy.org/..." (dropping just the colon), it seems to do the job. Will do this now. Willy
Re: haproxy.org bug pages broken (missing html headers and footer?)
On Sat, Sep 30, 2023 at 10:19:05AM +, Mathias Weiersmüller wrote: > Hi Willy, > > > Argh, thanks for notifying us! Haproxy dev5 crashed leaving a huge core > > that filled the FS (I hope it's complete, not checked yet), and the cron > > job that rebuilds the bugs page miserably failed as you can see :-/ > > > > That's now fixed, thank you! > > Willy > > the links to the respective bugs seem to be broken too, example: > > http://www.haproxy.org/bugs/://git.haproxy.org/?p=haproxy-2.6.git;a=commitdiff;h=dfa9730 > > it should be: > https://git.haproxy.org/?p=haproxy-2.6.git;a=commitdiff;h=dfa9730 Actually it's different. Browsers changed "recently" regarding the handling of schemeless URLs. In the past, using "://domain/path" would reuse the same scheme (http or https) as the current page. Now it seems these are handled as a relative path to the current page. I need to think about a simple way for these to continue to work as in the past. Thanks, Willy
AW: haproxy.org bug pages broken (missing html headers and footer?)
Hi Willy, > Argh, thanks for notifying us! Haproxy dev5 crashed leaving a huge core > that filled the FS (I hope it's complete, not checked yet), and the cron > job that rebuilds the bugs page miserably failed as you can see :-/ > > That's now fixed, thank you! > Willy the links to the respective bugs seem to be broken too, example: http://www.haproxy.org/bugs/://git.haproxy.org/?p=haproxy-2.6.git;a=commitdiff;h=dfa9730 it should be: https://git.haproxy.org/?p=haproxy-2.6.git;a=commitdiff;h=dfa9730 Best regards Matti
Re: haproxy.org bug pages broken (missing html headers and footer?)
Hi Lukas, On Wed, Sep 27, 2023 at 09:49:53PM +, Lukas Tribus wrote: > Hello, > > looks like the bug pages are broken; they contain the table of bugs > but there is really no formatting happening and it appears the entire > HTML header and footer is missing: > > Example: > http://www.haproxy.org/bugs/bugs-2.4.html > http://www.haproxy.org/bugs/bugs-2.6.2.html Argh, thanks for notifying us! Haproxy dev5 crashed leaving a huge core that filled the FS (I hope it's complete, not checked yet), and the cron job that rebuilds the bugs page miserably failed as you can see :-/ That's now fixed, thank you! Willy
Re: haproxy.org bug pages broken (missing html headers and footer?)
Hello, And https://www.haproxy.org/bugs/index.html is an empty document. There is a link for it on haproxy.org home page (as Known bugs). Le 27/09/2023 à 23:49, Lukas Tribus a écrit : Hello, looks like the bug pages are broken; they contain the table of bugs but there is really no formatting happening and it appears the entire HTML header and footer is missing: Example: http://www.haproxy.org/bugs/bugs-2.4.html http://www.haproxy.org/bugs/bugs-2.6.2.html BR, Lukas -- Best regards, Artur
haproxy.org bug pages broken (missing html headers and footer?)
Hello, looks like the bug pages are broken; they contain the table of bugs but there is really no formatting happening and it appears the entire HTML header and footer is missing: Example: http://www.haproxy.org/bugs/bugs-2.4.html http://www.haproxy.org/bugs/bugs-2.6.2.html BR, Lukas
Re: [PATCH] BUG/MINOR: promex: fix backend_agg_check_status
Le 12/09/2023 à 11:37, Cedric Paillet a écrit :
When a server is in maintenance, the check.status is no longer updated. Therefore,
we shouldn't consider check.status if the checks are not active. This check was
properly implemented in the haproxy_server_check_status metric, but wasn't carried
over to backend_agg_check_status, which introduced inconsistencies between the two
metrics."
---
addons/promex/service-prometheus.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/addons/promex/service-prometheus.c
b/addons/promex/service-prometheus.c
index e02e8c0c7..6885d202e 100644
--- a/addons/promex/service-prometheus.c
+++ b/addons/promex/service-prometheus.c
@@ -863,8 +863,10 @@ static int promex_dump_back_metrics(struct appctx *appctx,
struct htx *htx)
goto next_px;
sv = px->srv;
while (sv) {
- srv_check_status =
sv->check.status;
-
srv_check_count[srv_check_status] += 1;
+ if ((sv->check.state &
(CHK_ST_ENABLED|CHK_ST_PAUSED)) == CHK_ST_ENABLED) {
+ srv_check_status =
sv->check.status;
+
srv_check_count[srv_check_status] += 1;
+ }
sv = sv->next;
}
for (; ctx->obj_state < HCHK_STATUS_SIZE;
ctx->obj_state++) {
Thanks, merged now !
--
Christopher Faulet
[PATCH] BUG/MINOR: promex: fix backend_agg_check_status
When a server is in maintenance, the check.status is no longer updated.
Therefore, we shouldn't consider check.status if the checks are not active.
This check was properly implemented in the haproxy_server_check_status metric,
but wasn't carried over to backend_agg_check_status, which introduced
inconsistencies between the two metrics."
---
addons/promex/service-prometheus.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/addons/promex/service-prometheus.c
b/addons/promex/service-prometheus.c
index e02e8c0c7..6885d202e 100644
--- a/addons/promex/service-prometheus.c
+++ b/addons/promex/service-prometheus.c
@@ -863,8 +863,10 @@ static int promex_dump_back_metrics(struct appctx *appctx,
struct htx *htx)
goto next_px;
sv = px->srv;
while (sv) {
- srv_check_status =
sv->check.status;
-
srv_check_count[srv_check_status] += 1;
+ if ((sv->check.state &
(CHK_ST_ENABLED|CHK_ST_PAUSED)) == CHK_ST_ENABLED) {
+ srv_check_status =
sv->check.status;
+
srv_check_count[srv_check_status] += 1;
+ }
sv = sv->next;
}
for (; ctx->obj_state <
HCHK_STATUS_SIZE; ctx->obj_state++) {
--
2.25.1
RE: [PATCH] BUG/MINOR: promex: don't count servers in maintenance
Hi, Sorry, I now understand my problem better. When a server is in maintenance, the health checks are not performed and sv->check.state == CHK_ST_PAUSED. This is checked in the haproxy_server_check_status metric but not in backend_agg_check_status. I will check my new patch and push it afterwards. Thank you very much. -Message d'origine- De : Christopher Faulet Envoyé : lundi 11 septembre 2023 20:15 À : Cedric Paillet ; haproxy@formilux.org Objet : Re: [PATCH] BUG/MINOR: promex: don't count servers in maintenance Le 07/09/2023 à 16:50, Cedric Paillet a écrit : > >> And I guess we should also check the healthchecks are enabled for the >> server. It is not really an issue because call to get_check_status_result() >> will exclude neutral and unknown satuses. But there is no reason to count >> these servers. > > What we observed is that the health check of servers that have been put into > maintenance is no longer updated, and the status returned is the last known > one. I need to double-check, but I believe we even saw L7OK when putting a > "UP" server into maintenance. (What I'm sure of is that the majority of > servers in maintenance were in L7STS and not in UNK). > > If we add the PROMEX_FL_NO_MAINT_SR (which makes sense), we will continue to > display an incorrect backend_agg_server_status (and also > haproxy_server_check_status) for servers in maintenance for those who don't > set (no-maint=empty), and we probably then need another patch so that the > status of these servers is HCHK_STATUS_UNKNOWN?" > Health-checks for servers in maintenance are paused. So indeed, the last known status does not change anymore in this state. My purpose here was to also filter servers to only count those with health-checks enabled and running. When the server's metrics are dumped, the check status is already skipped for servers in maintenance. Thus it seems logical to not count them for the aggregated metric. In fact, this way, all servers in maintenance are skipped without checking the server's admin state. But it is probably cleaner to keep both checks for consistency. Except if I missed something. This part is not really clear for me anymore... -- Christopher Faulet
Re: [PATCH] BUG/MINOR: promex: don't count servers in maintenance
Le 07/09/2023 à 16:50, Cedric Paillet a écrit : And I guess we should also check the healthchecks are enabled for the server. It is not really an issue because call to get_check_status_result() will exclude neutral and unknown satuses. But there is no reason to count these servers. What we observed is that the health check of servers that have been put into maintenance is no longer updated, and the status returned is the last known one. I need to double-check, but I believe we even saw L7OK when putting a "UP" server into maintenance. (What I'm sure of is that the majority of servers in maintenance were in L7STS and not in UNK). If we add the PROMEX_FL_NO_MAINT_SR (which makes sense), we will continue to display an incorrect backend_agg_server_status (and also haproxy_server_check_status) for servers in maintenance for those who don't set (no-maint=empty), and we probably then need another patch so that the status of these servers is HCHK_STATUS_UNKNOWN?" Health-checks for servers in maintenance are paused. So indeed, the last known status does not change anymore in this state. My purpose here was to also filter servers to only count those with health-checks enabled and running. When the server's metrics are dumped, the check status is already skipped for servers in maintenance. Thus it seems logical to not count them for the aggregated metric. In fact, this way, all servers in maintenance are skipped without checking the server's admin state. But it is probably cleaner to keep both checks for consistency. Except if I missed something. This part is not really clear for me anymore... -- Christopher Faulet
Re: [PATCH] BUG/MINOR: promex: don't count servers in maintenance
Le 11/09/2023 à 15:09, Cedric Paillet a écrit :
The state of servers that were put in maintenance via the runtime API are
reported within the "backend_agg_check_status" metric, which
lead to inconsistent sums when compared to the "haproxy_server_check_status"
metric.
Now excluding them from this computation.
---
addons/promex/service-prometheus.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/addons/promex/service-prometheus.c
b/addons/promex/service-prometheus.c
index e02e8c0c7..7b6081fab 100644
--- a/addons/promex/service-prometheus.c
+++ b/addons/promex/service-prometheus.c
@@ -863,9 +863,12 @@ static int promex_dump_back_metrics(struct appctx *appctx,
struct htx *htx)
goto next_px;
sv = px->srv;
while (sv) {
+ if ((ctx->flags & PROMEX_FL_NO_MAINT_SRV)
&& (sv->cur_admin & SRV_ADMF_MAINT))
+ goto next_sv;
srv_check_status =
sv->check.status;
srv_check_count[srv_check_status] += 1;
- sv = sv->next;
+ next_sv:
+ sv = sv->next;
}
for (; ctx->obj_state < HCHK_STATUS_SIZE;
ctx->obj_state++) {
if
(get_check_status_result(ctx->obj_state) < CHK_RES_FAILED)
Thanks Cedric, I'll merge it ASAP.
--
Christopher Faulet
[PATCH] BUG/MINOR: promex: don't count servers in maintenance
The state of servers that were put in maintenance via the runtime API are
reported within the "backend_agg_check_status" metric, which
lead to inconsistent sums when compared to the "haproxy_server_check_status"
metric.
Now excluding them from this computation.
---
addons/promex/service-prometheus.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/addons/promex/service-prometheus.c
b/addons/promex/service-prometheus.c
index e02e8c0c7..7b6081fab 100644
--- a/addons/promex/service-prometheus.c
+++ b/addons/promex/service-prometheus.c
@@ -863,9 +863,12 @@ static int promex_dump_back_metrics(struct appctx *appctx,
struct htx *htx)
goto next_px;
sv = px->srv;
while (sv) {
+ if ((ctx->flags &
PROMEX_FL_NO_MAINT_SRV) && (sv->cur_admin & SRV_ADMF_MAINT))
+ goto next_sv;
srv_check_status =
sv->check.status;
srv_check_count[srv_check_status] += 1;
- sv = sv->next;
+ next_sv:
+ sv = sv->next;
}
for (; ctx->obj_state <
HCHK_STATUS_SIZE; ctx->obj_state++) {
if
(get_check_status_result(ctx->obj_state) < CHK_RES_FAILED)
--
2.25.1
RE: [PATCH] BUG/MINOR: promex: don't count servers in maintenance
> I guess you mean "backend_add_check_status". Because > "backend_agg_server_check_status" is a deprecated metric. It was replaced by > "backend_agg_server_status". Yes, sorry (and I should know, it's me who deprecated this value! )" In this patch both will be up updated, as it's the same code ! I will update the commit message. (we can deprecate it in 2.9 ? do you want a patch ?) > Here we should check for PROMEX_FL_NO_MAINT_SRV promex flag, no ? I mean: Yes, sure. >And I guess we should also check the healthchecks are enabled for the server. >It is not really an issue because call to get_check_status_result() will >exclude neutral and unknown satuses. But there is no reason to count these >servers. What we observed is that the health check of servers that have been put into maintenance is no longer updated, and the status returned is the last known one. I need to double-check, but I believe we even saw L7OK when putting a "UP" server into maintenance. (What I'm sure of is that the majority of servers in maintenance were in L7STS and not in UNK). If we add the PROMEX_FL_NO_MAINT_SR (which makes sense), we will continue to display an incorrect backend_agg_server_status (and also haproxy_server_check_status) for servers in maintenance for those who don't set (no-maint=empty), and we probably then need another patch so that the status of these servers is HCHK_STATUS_UNKNOWN?" Cédric
Re: [PATCH] BUG/MINOR: promex: don't count servers in maintenance
Hi,
Thanks Cedric and sorry for the delay. I have few comments.
Le 01/09/2023 à 08:22, Cedric Paillet a écrit :
The state of servers that were put in maintenance via the runtime API are
reported within the "backend_agg_server_check_status" metric, which
I guess you mean "backend_add_check_status". Because
"backend_agg_server_check_status" is a deprecated metric. It was replaced by
"backend_agg_server_status".
lead to inconsistent sums when compared to the "haproxy_server_check_status"
metric.
Now excluding them from this computation.
---
addons/promex/service-prometheus.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/addons/promex/service-prometheus.c
b/addons/promex/service-prometheus.c
index e02e8c0c7..7b61683dd 100644
--- a/addons/promex/service-prometheus.c
+++ b/addons/promex/service-prometheus.c
@@ -863,9 +863,12 @@ static int promex_dump_back_metrics(struct appctx *appctx,
struct htx *htx)
goto next_px;
sv = px->srv;
while (sv) {
+ if (sv->cur_admin &
SRV_ADMF_MAINT)
+ goto next_sv;
Here we should check for PROMEX_FL_NO_MAINT_SRV promex flag, no ? I mean:
if ((ctx->flags & PROMEX_FL_NO_MAINT_SRV) && (sv->cur_admin & SRV_ADMF_MAINT))
goto next_sv;
And I guess we should also check the healthchecks are enabled for the server. It
is not really an issue because call to get_check_status_result() will exclude
neutral and unknown satuses. But there is no reason to count these servers.
srv_check_status =
sv->check.status;
srv_check_count[srv_check_status] += 1;
- sv = sv->next;
+ next_sv:
+ sv = sv->next;
}
for (; ctx->obj_state < HCHK_STATUS_SIZE;
ctx->obj_state++) {
if
(get_check_status_result(ctx->obj_state) < CHK_RES_FAILED)
Otherwise, I'm ok with the fix.
--
Christopher Faulet
[PR] BUG/MEDIUM: h1-htx: Ensure chunked parsing with full output buffer
Dear list! Author: Chris Staite Number of patches: 1 This is an automated relay of the Github pull request: BUG/MEDIUM: h1-htx: Ensure chunked parsing with full output buffer Patch title(s): BUG/MEDIUM: h1-htx: Ensure chunked parsing with full output buffer Link: https://github.com/haproxy/haproxy/pull/2278 Edit locally: wget https://github.com/haproxy/haproxy/pull/2278.patch && vi 2278.patch Apply locally: curl https://github.com/haproxy/haproxy/pull/2278.patch | git am - Description: A previous fix to ensure that there is sufficient space on the output buffer to place parsed data (#2053) introduced an issue that if the output buffer is filled on a chunk boundary no data is parsed but the congested flag is not set due to the state not being H1_MSG_DATA. The check to ensure that there is sufficient space in the output buffer is actually already performed in all downstream functions before it is used. This makes the early optimisation that avoids the state transition to H1_MSG_DATA needless. Therefore, in order to allow the chunk parser to continue in this edge case we can simply remove the early check. This ensures that the state can progress and set the congested flag correctly in the caller. This patch fixes #2262. The upstream change that caused this logic error was backported as far as 2.5, therefore it makes sense to backport this fix back that far also. Instructions: This github pull request will be closed automatically; patch should be reviewed on the haproxy mailing list (haproxy@formilux.org). Everyone is invited to comment, even the patch's author. Please keep the author and list CCed in replies. Please note that in absence of any response this pull request will be lost.
[PATCH] BUG/MINOR: promex: don't count servers in maintenance
The state of servers that were put in maintenance via the runtime API are
reported within the "backend_agg_server_check_status" metric, which
lead to inconsistent sums when compared to the "haproxy_server_check_status"
metric.
Now excluding them from this computation.
---
addons/promex/service-prometheus.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/addons/promex/service-prometheus.c
b/addons/promex/service-prometheus.c
index e02e8c0c7..7b61683dd 100644
--- a/addons/promex/service-prometheus.c
+++ b/addons/promex/service-prometheus.c
@@ -863,9 +863,12 @@ static int promex_dump_back_metrics(struct appctx *appctx,
struct htx *htx)
goto next_px;
sv = px->srv;
while (sv) {
+ if (sv->cur_admin &
SRV_ADMF_MAINT)
+ goto next_sv;
srv_check_status =
sv->check.status;
srv_check_count[srv_check_status] += 1;
- sv = sv->next;
+ next_sv:
+ sv = sv->next;
}
for (; ctx->obj_state <
HCHK_STATUS_SIZE; ctx->obj_state++) {
if
(get_check_status_result(ctx->obj_state) < CHK_RES_FAILED)
--
2.25.1
Re: sc-set-gpt with expression: internal error, unexpected rule->from=0, please report this bug!
On Thu, Aug 10, 2023 at 01:59:34PM +0200, Johannes Naab wrote:
> On 8/9/23 17:53, Aurelien DARRAGON wrote:
> >> "http-request sc-set-gpt" does work, so does "tcp-request session". I.e.
> >> the bug seems to depend on "tcp-request connection".
> >>
> >
> > Indeed, according to both doc and code, sc-set-gpt and sc-set-gpt0 are
> > available from:
> >
> > - tcp-request session
> > - tcp-request content
> > - tcp-response content
> > - http-request
> > - http-response
> > - http-after-response
> >
> > But, according to the doc, they are also available from:
> > - tcp-request connection
> >
> > But the switch-cases in parse_set_gpt(), action_set_gpt(), and
> > action_set_gpt0() from stick_table.c don't allow this case, so it looks
> > like it was forgotten indeed when the expr support was added for
> > sc-set-gpt0 in 0d7712dff0 ("MINOR: stick-table: allow sc-set-gpt0 to set
> > value from an expression").
> >
> > We have the same issue for the sc-add-gpc action which was greatly
> > inspired from set-gpt, where the switch cases defined in parse_add_gpc()
> > and action_add_gpc() from stick_table.c don't allow tcp-request
> > connection as origin.
> >
> > Please find the attached patches that should help solve the above issues.
>
> Thanks. With those patches "tcp-request connection" does work as well,
> even with setting session variables.
Thanks to you two, I've applied your 3 patches now (Aurélien's code fixes
and Johannes' doc fixes).
Willy
Re: sc-set-gpt with expression: internal error, unexpected rule->from=0, please report this bug!
On 8/9/23 17:53, Aurelien DARRAGON wrote:
>> "http-request sc-set-gpt" does work, so does "tcp-request session". I.e.
>> the bug seems to depend on "tcp-request connection".
>>
>
> Indeed, according to both doc and code, sc-set-gpt and sc-set-gpt0 are
> available from:
>
> - tcp-request session
> - tcp-request content
> - tcp-response content
> - http-request
> - http-response
> - http-after-response
>
> But, according to the doc, they are also available from:
> - tcp-request connection
>
> But the switch-cases in parse_set_gpt(), action_set_gpt(), and
> action_set_gpt0() from stick_table.c don't allow this case, so it looks
> like it was forgotten indeed when the expr support was added for
> sc-set-gpt0 in 0d7712dff0 ("MINOR: stick-table: allow sc-set-gpt0 to set
> value from an expression").
>
> We have the same issue for the sc-add-gpc action which was greatly
> inspired from set-gpt, where the switch cases defined in parse_add_gpc()
> and action_add_gpc() from stick_table.c don't allow tcp-request
> connection as origin.
>
> Please find the attached patches that should help solve the above issues.
Thanks. With those patches "tcp-request connection" does work as well,
even with setting session variables.
Johannes
Re: sc-set-gpt with expression: internal error, unexpected rule->from=0, please report this bug!
>> I have no idea what causes it at the moment. A few things you could try,
>> in any order, to help locate the bug:
>>
>> - check if it accepts it using "http-request sc-set-gpt" instead of
>> "tcp-request connection" so that we know if it's related to the ruleset
>> or something else ;
>>
>
> Thanks, that seems to narrow the problem down.
>
> "http-request sc-set-gpt" does work, so does "tcp-request session". I.e.
> the bug seems to depend on "tcp-request connection".
>
> "session" works for me, for setting session variables it might even be
> necessary, but those might be avoidable by setting the conditional
> directly.
> (But not trivially since "sub()" only takes values or variables
> but not fetches and "-m int gt " only seem to takes direct
> values).
Indeed, according to both doc and code, sc-set-gpt and sc-set-gpt0 are
available from:
- tcp-request session
- tcp-request content
- tcp-response content
- http-request
- http-response
- http-after-response
But, according to the doc, they are also available from:
- tcp-request connection
But the switch-cases in parse_set_gpt(), action_set_gpt(), and
action_set_gpt0() from stick_table.c don't allow this case, so it looks
like it was forgotten indeed when the expr support was added for
sc-set-gpt0 in 0d7712dff0 ("MINOR: stick-table: allow sc-set-gpt0 to set
value from an expression").
We have the same issue for the sc-add-gpc action which was greatly
inspired from set-gpt, where the switch cases defined in parse_add_gpc()
and action_add_gpc() from stick_table.c don't allow tcp-request
connection as origin.
Please find the attached patches that should help solve the above issues.
AurelienFrom b66b401ddb36a4c686fa0df965492da204ba66a8 Mon Sep 17 00:00:00 2001
From: Aurelien DARRAGON
Date: Wed, 9 Aug 2023 17:39:29 +0200
Subject: [PATCH 2/2] BUG/MINOR: stktable: allow sc-add-gpc from tcp-request
connection
Following the previous commit's logic, we enable the use of sc-add-gpc
from tcp-request connection since it was probably forgotten in the first
place for sc-set-gpt0, and since sc-add-gpc was inspired from it, it also
lacks its.
As sc-add-gpc was implemented in 5a72d03a58 ("MINOR: stick-table: implement
the sc-add-gpc() action"), this should only be backported to 2.8
---
src/stick_table.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/stick_table.c b/src/stick_table.c
index 363269f01..b11e94961 100644
--- a/src/stick_table.c
+++ b/src/stick_table.c
@@ -2913,6 +2913,7 @@ static enum act_return action_add_gpc(struct act_rule *rule, struct proxy *px,
value = (unsigned int)(rule->arg.gpc.value);
else {
switch (rule->from) {
+ case ACT_F_TCP_REQ_CON: smp_opt_dir = SMP_OPT_DIR_REQ; break;
case ACT_F_TCP_REQ_SES: smp_opt_dir = SMP_OPT_DIR_REQ; break;
case ACT_F_TCP_REQ_CNT: smp_opt_dir = SMP_OPT_DIR_REQ; break;
case ACT_F_TCP_RES_CNT: smp_opt_dir = SMP_OPT_DIR_RES; break;
@@ -3013,6 +3014,7 @@ static enum act_parse_ret parse_add_gpc(const char **args, int *arg, struct prox
return ACT_RET_PRS_ERR;
switch (rule->from) {
+ case ACT_F_TCP_REQ_CON: smp_val = SMP_VAL_FE_CON_ACC; break;
case ACT_F_TCP_REQ_SES: smp_val = SMP_VAL_FE_SES_ACC; break;
case ACT_F_TCP_REQ_CNT: smp_val = SMP_VAL_FE_REQ_CNT; break;
case ACT_F_TCP_RES_CNT: smp_val = SMP_VAL_BE_RES_CNT; break;
--
2.34.1
From 0b3586a30a8181316477140daf56dd3309b1f6f1 Mon Sep 17 00:00:00 2001
From: Aurelien DARRAGON
Date: Wed, 9 Aug 2023 17:23:32 +0200
Subject: [PATCH 1/2] BUG/MINOR: stktable: allow sc-set-gpt(0) from tcp-request
connection
Both the documentation and original developer intents seem to suggest
that sc-set-gpt/sc-set-gpt0 actions should be available from
tcp-request connection.
Yet because it was probably forgotten when expr support was added to
sc-set-gpt0 in 0d7712dff0 ("MINOR: stick-table: allow sc-set-gpt0 to
set value from an expression") it doesn't work and will report this
kind of errors:
"internal error, unexpected rule->from=0, please report this bug!"
Fixing the code to comply with the documentation and the expected
behavior.
This must be backported to every stable versions.
[for < 2.5, as only sc-set-gpt0 existed back then, the patch must be
manually applied to skip irrelevant parts]
---
src/stick_table.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/stick_table.c b/src/stick_table.c
index a2aa9c451..363269f01 100644
--- a/src/stick_table.c
+++ b/src/stick_table.c
@@ -2656,6 +2656,7 @@ static enum act_return action_set_gpt(struct act_rule *rule, struct proxy *px,
value = (unsigned int)(rule->arg.gpt.value);
else {
switch (rule->from) {
+ case ACT_F_TCP_REQ_CON: smp_opt_dir = SMP_OPT_DIR_REQ; break;
case ACT_F_TCP_REQ_SES: smp_opt_dir = SMP_OPT_DIR_REQ; br
Re: sc-set-gpt with expression: internal error, unexpected rule->from=0, please report this bug!
Hi Willy,
On 8/9/23 13:48, Willy Tarreau wrote:
> Hi Johannes,
>
> On Wed, Aug 09, 2023 at 01:02:29PM +0200, Johannes Naab wrote:
>> Hi,
>>
>> I'm trying to use a stick table with general purpose tags (gpt) to do longer
>> term (beyond the window itself) maximum connection rate tracking:
>> - stick table with conn_rate and one gpt
>> - update/set gpt0 if the current conn_rate is greater than what is stored in
>> the gpt.
>>
>> But I have trouble setting the gpt even from a trivial sample expression,
>> erroring during config parsing with `internal error, unexpected rule->from=0,
>> please report this bug!`.
>
> At first glance I can't find a reason why your config would not work,
> so you've definitely discovered a bug.
>
> I have no idea what causes it at the moment. A few things you could try,
> in any order, to help locate the bug:
>
> - check if it accepts it using "http-request sc-set-gpt" instead of
> "tcp-request connection" so that we know if it's related to the ruleset
> or something else ;
>
Thanks, that seems to narrow the problem down.
"http-request sc-set-gpt" does work, so does "tcp-request session". I.e.
the bug seems to depend on "tcp-request connection".
"session" works for me, for setting session variables it might even be
necessary, but those might be avoidable by setting the conditional
directly.
(But not trivially since "sub()" only takes values or variables
but not fetches and "-m int gt " only seem to takes direct
values).
"tcp-request connection" state could be helpful to avoid TLS handshakes.
> - please also try sc0-set-gpt(0) instead of sc-set-gpt(0,0), maybe there
> is something wrong in the latter's parser.
>
That does not seem to make any difference.
> - does your other test with "int(1)" as the expression also fail or did
> it work ? If it did work, maybe forcing a cat to integer on the variable
> using "var(proc.baz),add(0)" could work.
>
Any expression fails in "tcp-request connection", even the more trivial
"int(1)", "var(proc.baz),add(0)" does fail as well.
> In any case some feedback on these points could be useful. The last two
> ones would be safe workarounds if they work.
>
>
For completeness a running/working config for tracking the max conn_rate
(https://xkcd.com/979/):
```
frontend foo
bind :::8080 v4v6
default_backend bar
tcp-request connection track-sc0 src table stick1
## track max conn_rate
tcp-request session set-var(sess.prev_conn_rate) sc_get_gpt(0,0,stick1)
tcp-request session set-var(sess.cur_conn_rate) sc_conn_rate(0,stick1)
tcp-request session sc-set-gpt(0,0) var(sess.cur_conn_rate) if {
var(sess.cur_conn_rate),sub(sess.prev_conn_rate) -m int gt 0 }
http-response set-header cur-conn-rate %[var(sess.cur_conn_rate)]
http-response set-header prev-conn-rate %[var(sess.prev_conn_rate)]
backend stick1
stick-table type ipv6 size 1m expire 1h store conn_rate(10s),gpt(1)
```
Thanks!
Johannes
>> Config, output, and haproxy -vv below.
>>
>> Should this work, or do I misunderstand what sc-set-gpt can achieve?
>
> For me it should work, and if there's a corner case that makes it
> impossible with your config, I'm not seeing it and we should report it
> in a much more user-friendly way!
>
> Thanks!
> Willy
>
Re: sc-set-gpt with expression: internal error, unexpected rule->from=0, please report this bug!
Hi Johannes, On Wed, Aug 09, 2023 at 01:02:29PM +0200, Johannes Naab wrote: > Hi, > > I'm trying to use a stick table with general purpose tags (gpt) to do longer > term (beyond the window itself) maximum connection rate tracking: > - stick table with conn_rate and one gpt > - update/set gpt0 if the current conn_rate is greater than what is stored in > the gpt. > > But I have trouble setting the gpt even from a trivial sample expression, > erroring during config parsing with `internal error, unexpected rule->from=0, > please report this bug!`. At first glance I can't find a reason why your config would not work, so you've definitely discovered a bug. I have no idea what causes it at the moment. A few things you could try, in any order, to help locate the bug: - check if it accepts it using "http-request sc-set-gpt" instead of "tcp-request connection" so that we know if it's related to the ruleset or something else ; - please also try sc0-set-gpt(0) instead of sc-set-gpt(0,0), maybe there is something wrong in the latter's parser. - does your other test with "int(1)" as the expression also fail or did it work ? If it did work, maybe forcing a cat to integer on the variable using "var(proc.baz),add(0)" could work. In any case some feedback on these points could be useful. The last two ones would be safe workarounds if they work. > Config, output, and haproxy -vv below. > > Should this work, or do I misunderstand what sc-set-gpt can achieve? For me it should work, and if there's a corner case that makes it impossible with your config, I'm not seeing it and we should report it in a much more user-friendly way! Thanks! Willy
sc-set-gpt with expression: internal error, unexpected rule->from=0, please report this bug!
Hi,
I'm trying to use a stick table with general purpose tags (gpt) to do longer
term (beyond the window itself) maximum connection rate tracking:
- stick table with conn_rate and one gpt
- update/set gpt0 if the current conn_rate is greater than what is stored in
the gpt.
But I have trouble setting the gpt even from a trivial sample expression,
erroring during config parsing with `internal error, unexpected rule->from=0,
please report this bug!`.
Config, output, and haproxy -vv below.
Should this work, or do I misunderstand what sc-set-gpt can achieve?
Best regards,
Johannes
config
```
global
log stdout format raw local0
stats socket /run/haproxy/admin.sock mode 660 level admin
stats timeout 30s
set-var proc.baz int(3)
defaults
log global
modehttp
timeout connect 5000
timeout client 5
timeout server 5
frontend foo
bind :::8080 v4v6
default_backend bar
tcp-request connection track-sc0 src table stick1
tcp-request connection sc-set-gpt(0,0) var(proc.baz)
# tcp-request connection sc-set-gpt(0,0) int(1)
http-response set-header conn-rate %[sc_get_gpt(0,0,stick1)]
## track max conn_rate
#tcp-request connection set-var(sess.prev_conn_rate)
sc_get_gpt(0,0,stick1)
#tcp-request connection set-var(sess.cur_conn_rate)
sc_conn_rate(0,stick1)
#tcp-request connection sc-set-gpt(0,0) var(sess.cur_conn_rate) if {
var(sess.cur_conn_rate),sub(sess.prev_conn_rate) -m int gt 0 }
backend bar
server localhost 127.0.0.1:80
backend stick1
stick-table type ipv6 size 1m expire 1h store conn_rate(10s),gpt(1)
```
error
```
# ./haproxy -f ~/haproxy.cfg
[NOTICE] (139304) : haproxy version is 2.9-dev2-227317-63
[NOTICE] (139304) : path to executable is ./haproxy
[ALERT](139304) : config : parsing [/root/haproxy.cfg:19] : internal error,
unexpected rule->from=0, please report this bug!
[ALERT](139304) : config : Error(s) found in configuration file :
/root/haproxy.cfg
[ALERT](139304) : config : Fatal errors found in configuration.
```
`haproxy -vv` (initally on 2.6, but it still occurs in recent git)
```
HAProxy version 2.9-dev2-227317-63 2023/08/09 - https://haproxy.org/
Status: development branch - not safe for use in production.
Known bugs: https://github.com/haproxy/haproxy/issues?q=is:issue+is:open
Running on: Linux 5.15.0-73-generic #80-Ubuntu SMP Mon May 15 15:18:26 UTC 2023
x86_64
Build options :
TARGET = linux-glibc
CPU = generic
CC = cc
CFLAGS = -O2 -g -Wall -Wextra -Wundef -Wdeclaration-after-statement
-Wfatal-errors -Wtype-limits -Wshift-negative-value -Wshift-overflow=2
-Wduplicated-cond -Wnull-dereference -fwrapv -Wno-address-of-packed-member
-Wno-unused-label -Wno-sign-compare -Wno-unused-parameter -Wno-clobbered
-Wno-missing-field-initializers -Wno-cast-function-type -Wno-string-plus-int
-Wno-atomic-alignment
OPTIONS = USE_OPENSSL=1 USE_SYSTEMD=1 USE_PCRE=1
DEBUG = -DDEBUG_STRICT -DDEBUG_MEMORY_POOLS
Feature list : -51DEGREES +ACCEPT4 +BACKTRACE -CLOSEFROM +CPU_AFFINITY +CRYPT_H
-DEVICEATLAS +DL -ENGINE +EPOLL -EVPORTS +GETADDRINFO -KQUEUE -LIBATOMIC
+LIBCRYPT +LINUX_SPLICE +LINUX_TPROXY -LUA -MATH -MEMORY_PROFILING +NETFILTER
+NS -OBSOLETE_LINKER +OPENSSL -OPENSSL_WOLFSSL -OT +PCRE -PCRE2 -PCRE2_JIT
-PCRE_JIT +POLL +PRCTL -PROCCTL -PROMEX -PTHREAD_EMULATION -QUIC
-QUIC_OPENSSL_COMPAT +RT +SHM_OPEN +SLZ +SSL -STATIC_PCRE -STATIC_PCRE2
+SYSTEMD +TFO +THREAD +THREAD_DUMP +TPROXY -WURFL -ZLIB
Default settings :
bufsize = 16384, maxrewrite = 1024, maxpollevents = 200
Built with multi-threading support (MAX_TGROUPS=16, MAX_THREADS=256, default=2).
Built with OpenSSL version : OpenSSL 3.0.2 15 Mar 2022
Running on OpenSSL version : OpenSSL 3.0.2 15 Mar 2022
OpenSSL library supports TLS extensions : yes
OpenSSL library supports SNI : yes
OpenSSL library supports : TLSv1.0 TLSv1.1 TLSv1.2 TLSv1.3
OpenSSL providers loaded : default
Built with network namespace support.
Built with libslz for stateless compression.
Compression algorithms supported : identity("identity"), deflate("deflate"),
raw-deflate("deflate"), gzip("gzip")
Built with transparent proxy support using: IP_TRANSPARENT IPV6_TRANSPARENT
IP_FREEBIND
Built with PCRE version : 8.39 2016-06-14
Running on PCRE version : 8.39 2016-06-14
PCRE library supports JIT : no (USE_PCRE_JIT not set)
Encrypted password support via crypt(3): yes
Built with gcc compiler version 11.4.0
Available polling systems :
epoll : pref=300, test result OK
poll : pref=200, test result OK
select : pref=150, test result OK
Total: 3 (3 usable), will use epoll.
Available multiplexer protocols :
(protocols marked as cannot be specified using 'proto' keyword)
h2 : mode=HTTP side=FE|BE mux=H2flags=HTX|HOL_RISK|NO_UPG
fcgi : mode=HTTP side=BE
Re: [PATCH 1/2] BUG/MINOR: server-state: Avoid warning on 'file not found'
Le 7/20/23 à 22:21, Marcos de Oliveira a écrit : From: Marcos de Oliveira On a clean installation, users might want to use server-state-file and the recommended zero-warning option. This caused a problem if server-state-file was not found, as a warning was emited, causing startup to fail. This will allow users to specify nonexistent server-state-file at first, and dump states to the file later. Fixes #2190 Thanks ! Both patches merged and backported to 2.8. -- Christopher Faulet
[PATCH 1/2] BUG/MINOR: server-state: Avoid warning on 'file not found'
From: Marcos de Oliveira
On a clean installation, users might want to use server-state-file and
the recommended zero-warning option. This caused a problem if
server-state-file was not found, as a warning was emited, causing
startup to fail.
This will allow users to specify nonexistent server-state-file at first,
and dump states to the file later.
Fixes #2190
---
src/server_state.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/src/server_state.c b/src/server_state.c
index d2cc4d820..93dbd20f1 100644
--- a/src/server_state.c
+++ b/src/server_state.c
@@ -798,7 +798,10 @@ void apply_server_state(void)
errno = 0;
f = fopen(file, "r");
if (!f) {
- ha_warning("config: Can't open global server state file '%s':
%s\n", file, strerror(errno));
+ if (errno == ENOENT)
+ ha_notice("config: Can't open global server state file
'%s': %s\n", file, strerror(errno));
+ else
+ ha_warning("config: Can't open global server state file
'%s': %s\n", file, strerror(errno));
goto no_globalfile;
}
@@ -866,8 +869,12 @@ void apply_server_state(void)
errno = 0;
f = fopen(file, "r");
if (!f) {
- ha_warning("Proxy '%s': Can't open server state file
'%s': %s.\n",
- curproxy->id, file, strerror(errno));
+ if (errno == ENOENT)
+ ha_notice("Proxy '%s': Can't open server state
file '%s': %s.\n",
+ curproxy->id, file, strerror(errno));
+ else
+ ha_warning("Proxy '%s': Can't open server state
file '%s': %s.\n",
+ curproxy->id, file, strerror(errno));
continue; /* next proxy */
}
--
2.25.1
[PATCH 2/2] BUG/MINOR: server-state: Ignore empty files
From: Marcos de Oliveira
Users might want to pre-create an empty file for later dumping
server-states. This commit allows for that by emiting a notice in case
file is empty and a warning if file is not empty, but version is unknown
Fix partially: #2190
---
src/server_state.c | 23 ---
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/src/server_state.c b/src/server_state.c
index 93dbd20f1..ebdcf3c69 100644
--- a/src/server_state.c
+++ b/src/server_state.c
@@ -522,6 +522,7 @@ static void srv_state_px_update(const struct proxy *px, int
vsn, struct eb_root
/*
* read next line from file and return the server state version if one
found.
+ * If file is empty, then -1 is returned
* If no version is found, then 0 is returned
* Note that this should be the first read on
*/
@@ -532,7 +533,7 @@ static int srv_state_get_version(FILE *f) {
/* first character of first line of the file must contain the version
of the export */
if (fgets(mybuf, SRV_STATE_LINE_MAXLEN, f) == NULL)
- return 0;
+ return -1;
vsn = strtol(mybuf, , 10);
if (endptr == mybuf || *endptr != '\n') {
@@ -806,9 +807,13 @@ void apply_server_state(void)
}
global_vsn = srv_state_get_version(f);
- if (global_vsn == 0) {
- ha_warning("config: Can't get version of the global server
state file '%s'.\n",
- file);
+ if (global_vsn < 1) {
+ if (global_vsn == -1)
+ ha_notice("config: Empty global server state file
'%s'.\n",
+ file);
+ if (global_vsn == 0)
+ ha_warning("config: Can't get version of the global
server state file '%s'.\n",
+ file);
goto close_globalfile;
}
@@ -880,9 +885,13 @@ void apply_server_state(void)
/* first character of first line of the file must contain the
version of the export */
local_vsn = srv_state_get_version(f);
- if (local_vsn == 0) {
- ha_warning("Proxy '%s': Can't get version of the server
state file '%s'.\n",
- curproxy->id, file);
+ if (local_vsn < 1) {
+ if (local_vsn == -1)
+ ha_notice("Proxy '%s': Empty server state file
'%s'.\n",
+ curproxy->id, file);
+ if (local_vsn == 0)
+ ha_warning("Proxy '%s': Can't get version of
the server state file '%s'.\n",
+ curproxy->id, file);
goto close_localfile;
}
--
2.25.1
Re: [PATCH] BUG/MINOR: Fix Lua's `get_stats` function
On Fri, Jun 02, 2023 at 10:11:36AM +0200, Tim Düsterhus wrote: > Hi > > On 6/2/23 08:42, Willy Tarreau wrote: > > Thank you for this. I've added a comment in the file about it, and a > > regtest to detect when this happens, since (null) appears in the header > > line of the "show stat" output in this case. > > Nice, thank you. I thought about including a test for that, but didn't have > the time to think about how to properly test this. The CLI-based test is > also much cleaner than any Lua-based test I would likely have created. Any test would fit anyway. What we want is just a safety belt to detect that the developer missed one place to update. If we face other similar ones in the future, that could be a nice way to add other similar guards against developers mistakes. Willy
Re: [PATCH] BUG/MINOR: Fix Lua's `get_stats` function
Hi On 6/2/23 08:42, Willy Tarreau wrote: Thank you for this. I've added a comment in the file about it, and a regtest to detect when this happens, since (null) appears in the header line of the "show stat" output in this case. Nice, thank you. I thought about including a test for that, but didn't have the time to think about how to properly test this. The CLI-based test is also much cleaner than any Lua-based test I would likely have created. Best regards Tim Düsterhus
Re: [PATCH] BUG/MINOR: Fix Lua's `get_stats` function
Hi Tim, On Thu, Jun 01, 2023 at 06:58:08PM +0200, Tim Duesterhus wrote: > Lua's `get_stats` function stopped working in > 4cfb0019e65bce79953164eddf54c1bbb61add62, due to the addition a new field > ST_F_PROTO without a corresponding entry in `stat_fields`. Thank you for this. I've added a comment in the file about it, and a regtest to detect when this happens, since (null) appears in the header line of the "show stat" output in this case. Willy
[PATCH] BUG/MINOR: Fix Lua's `get_stats` function
Lua's `get_stats` function stopped working in
4cfb0019e65bce79953164eddf54c1bbb61add62, due to the addition a new field
ST_F_PROTO without a corresponding entry in `stat_fields`.
Fix the issue by adding the entry, like
a46b142e8807ea640e041d3a29e3fd427844d559 did previously for a different field.
This patch fixes GitHub Issue #2174, it should be backported to 2.8.
---
src/stats.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/stats.c b/src/stats.c
index 68adde6c5..1d071e2c1 100644
--- a/src/stats.c
+++ b/src/stats.c
@@ -274,6 +274,7 @@ const struct name_desc stat_fields[ST_F_TOTAL_FIELDS] = {
[ST_F_H1REQ] = { .name = "h1req",
.desc = "Total number of HTTP/1 sessions processed by this object since
the worker process started" },
[ST_F_H2REQ] = { .name = "h2req",
.desc = "Total number of hTTP/2 sessions processed by this object since
the worker process started" },
[ST_F_H3REQ] = { .name = "h3req",
.desc = "Total number of HTTP/3 sessions processed by this object since
the worker process started" },
+ [ST_F_PROTO] = { .name = "proto",
.desc = "Protocol" },
};
/* one line of info */
--
2.40.1
[bug-report] if we should add check for memory allocation
Dear haproxy maintainers：
if we should add check for memory allocation?
I got 3 coredumps(haproxy version is 2.6.6):
bt 1
Program terminated with signal SIGSEGV, Segmentation fault.
#0 0x7f0a03a50a57 in BIO_meth_set_write (biom=0x0,
bwrite=bwrite@entry=0x55fbf4ac5260 ) at crypto/bio/bio_meth.c:92
92 biom->bwrite_old = bwrite;
(gdb) bt
#0 0x7f0a03a50a57 in BIO_meth_set_write (biom=0x0,
bwrite=bwrite@entry=0x55fbf4ac5260 ) at crypto/bio/bio_meth.c:92
#1 0x55fbf4ac50d5 in __ssl_sock_init () at src/ssl_sock.c:8150
#2 0x55fbf4aba0e6 in main (argc=3, argv=0x7ffe3895b718) at
src/haproxy.c:3075
bt 2
Program terminated with signal SIGSEGV, Segmentation fault.
#0 __memmove_evex_unaligned_erms () at
../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:307
307 VMOVU %VEC(0), (%rdi)
(gdb) bt
#0 __memmove_evex_unaligned_erms () at
../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:307
#1 0x56542cb0dba1 in memcpy (__len=46, __src=0x56542dbff1a0,
__dest=) at /usr/include/bits/string_fortified.h:29
#2 __b_putblk (b=, len=46, blk=0x56542dbff1a0 "register section
'log-forward': out of memory.\n") at include/haproxy/buf.h:552
#3 b_putblk (b=, len=46, blk=0x56542dbff1a0 "register section
'log-forward': out of memory.\n") at include/haproxy/buf.h:568
#4 usermsgs_put (msg=) at src/errors.c:50
#5 print_message (use_usermsgs_ctx=, label=,
fmt=, argp=) at src/errors.c:216
#6 0x56542cb0de0e in ha_alert (fmt=fmt@entry=0x56542cb581a8 "register
section '%s': out of memory.\n") at src/errors.c:253
#7 0x56542ca4bcfc in cfg_register_section (post_section_parser=0x0,
section_parser=0x56542ca444c0 ,
section_name=0x56542cb56e3a "log-forward") at src/cfgparse.c:4318
#8 cfg_register_section (section_name=0x56542cb56e3a "log-forward",
section_parser=0x56542ca444c0 , post_section_parser=0x0)
at src/cfgparse.c:4303
#9 0x56542c94f0e6 in main (argc=3, argv=0x7ffefb267388) at
src/haproxy.c:3075
(gdb) f 4
#4 usermsgs_put (msg=) at src/errors.c:50
50 b_putblk(_buf, msg->ptr, msg->len);
(gdb) p usermsgs_buf
$1 = {size = 1024, area = 0x0, data = 0, head = 0}
(gdb)
bt 3
Program terminated with signal SIGSEGV, Segmentation fault.
#0 __pthread_clockjoin_ex (threadid=1, thread_return=thread_return@entry=0x0,
clockid=clockid@entry=0, abstime=abstime@entry=0x0, block=block@entry=true) at
pthread_join_common.c:43
43 if (INVALID_NOT_TERMINATED_TD_P (pd))
(gdb) bt
#0 __pthread_clockjoin_ex (threadid=1, thread_return=thread_return@entry=0x0,
clockid=clockid@entry=0, abstime=abstime@entry=0x0, block=block@entry=true) at
pthread_join_common.c:43
#1 0x7ff1708aa043 in ___pthread_join (threadid=,
thread_return=thread_return@entry=0x0) at pthread_join.c:25
#2 0x55b6ed0c64d1 in preload_libgcc_s () at src/thread.c:950
#3 __thread_init () at src/thread.c:964
#4 0x55b6ecf10086 in main (argc=3, argv=0x7ffc1849dbc8) at
src/haproxy.c:3073
The attachments is my patches.
Looking forward to your response.
| |
eaglegai
|
|
eagle...@163.com
|
add-check-for-ha_meth.patch
Description: Binary data
add-check-for-usermsgs_buf.area-in-usermsgs_put.patch
Description: Binary data
add-check-for-pthread_create.patch
Description: Binary data
Re: [OPINIONS DESIRED] (was Re: [PATCH] BUG/MINOR: Fix typo in `TotalSplicedBytesOut` field name)
On Sun, Apr 23, 2023 at 02:25:06PM +0200, Lukas Tribus wrote: > On Sun, 23 Apr 2023 at 13:08, Willy Tarreau wrote: > > > > On Sun, Apr 23, 2023 at 12:39:25PM +0200, Tim Düsterhus, WoltLab GmbH wrote: > > > Willy, > > > > > > On 3/27/23 20:25, Willy Tarreau wrote: > > > > OK, let's see what other users and participants think about it. If I get > > > > at least one "please don't change it" I'll abide, otherwise it may make > > > > sense to fix it before it ossifies and annoys some future users. > > > > > > > > Anyone has any opinion here ? > > > > > > Wanted to bump this thread to make sure it is resolved one way or another > > > before the release. > > > > Ah thanks, I remembered there was something pending but didn't remember > > what. > > Yeah I think we should just fix it since nobody objected. I'll deal with > > that > > next week (I did too much of haproxy for this week-end). > > I agree that we should fix this, considering the possible breakage is > pretty unlikely and very much non fatal. Thank you Lukas, Willy
Re: [OPINIONS DESIRED] (was Re: [PATCH] BUG/MINOR: Fix typo in `TotalSplicedBytesOut` field name)
On Sun, 23 Apr 2023 at 13:08, Willy Tarreau wrote: > > On Sun, Apr 23, 2023 at 12:39:25PM +0200, Tim Düsterhus, WoltLab GmbH wrote: > > Willy, > > > > On 3/27/23 20:25, Willy Tarreau wrote: > > > OK, let's see what other users and participants think about it. If I get > > > at least one "please don't change it" I'll abide, otherwise it may make > > > sense to fix it before it ossifies and annoys some future users. > > > > > > Anyone has any opinion here ? > > > > Wanted to bump this thread to make sure it is resolved one way or another > > before the release. > > Ah thanks, I remembered there was something pending but didn't remember what. > Yeah I think we should just fix it since nobody objected. I'll deal with that > next week (I did too much of haproxy for this week-end). I agree that we should fix this, considering the possible breakage is pretty unlikely and very much non fatal. Lukas
Re: [OPINIONS DESIRED] (was Re: [PATCH] BUG/MINOR: Fix typo in `TotalSplicedBytesOut` field name)
On Sun, Apr 23, 2023 at 12:39:25PM +0200, Tim Düsterhus, WoltLab GmbH wrote: > Willy, > > On 3/27/23 20:25, Willy Tarreau wrote: > > OK, let's see what other users and participants think about it. If I get > > at least one "please don't change it" I'll abide, otherwise it may make > > sense to fix it before it ossifies and annoys some future users. > > > > Anyone has any opinion here ? > > Wanted to bump this thread to make sure it is resolved one way or another > before the release. Ah thanks, I remembered there was something pending but didn't remember what. Yeah I think we should just fix it since nobody objected. I'll deal with that next week (I did too much of haproxy for this week-end). Thanks, Willy
Re: [OPINIONS DESIRED] (was Re: [PATCH] BUG/MINOR: Fix typo in `TotalSplicedBytesOut` field name)
Willy, On 3/27/23 20:25, Willy Tarreau wrote: OK, let's see what other users and participants think about it. If I get at least one "please don't change it" I'll abide, otherwise it may make sense to fix it before it ossifies and annoys some future users. Anyone has any opinion here ? Wanted to bump this thread to make sure it is resolved one way or another before the release. Best regards Tim Düsterhus Developer WoltLab GmbH -- WoltLab GmbH Nedlitzer Str. 27B 14469 Potsdam Tel.: +49 331 96784338 duester...@woltlab.com www.woltlab.com Managing director: Marcel Werk AG Potsdam HRB 26795 P
Re: [PATCH] BUG/MINOR: ssl: Stop leaking `err` in ssl_sock_load_ocsp()
On Mon, Mar 27, 2023 at 03:43:22PM +0200, Remi Tricot-Le Breton wrote: > > On 27/03/2023 15:31, Tim Düsterhus wrote: > > Hi > > > > On 3/19/23 16:07, Tim Duesterhus wrote: > >> Previously performing a config check of `.github/h2spec.config` would > >> report a > >> 20 byte leak as reported in GitHub Issue #2082. > >> > >> The leak was introduced in a6c0a59e9af65180c3ff591b91855bea8d19b352, > >> which is > >> dev only. No backport needed. > > > > I believe you might've missed this patch. > > > > Best regards > > Tim Düsterhus > > Hi Tim, > > Sorry about that delay. The patch looks good to me. I'll let William > merge it when he has the time. > > Rémi > Thanks to both of you, merged. -- William Lallemand
[OPINIONS DESIRED] (was Re: [PATCH] BUG/MINOR: Fix typo in `TotalSplicedBytesOut` field name)
On Mon, Mar 27, 2023 at 07:08:24PM +0200, Tim Düsterhus, WoltLab GmbH wrote: > Willy, > > On 3/27/23 18:17, Willy Tarreau wrote: > > Hmmm that's embarrassing indeed. On the one hand we should consider > > that it's part of the visible API and should stay like this eternally > > (like "Referer" in HTTP). On the other hand, it was introduced only in > > 2.3, splicing is not the most commonly used feature (to say the least), > > and the likelihood that it had ever been used is extremely faint, so it > > is tempting to fix it before its use is widespread. This is also supported > > by search engines not reporting any occurrence of it aside a few rare > > dumps. > > > > However we are normally extremely careful about guaranteeing that a > > stable user can upgrade to the next LTS version. And here this would > > not be the case. So if we merge it into 2.8 I'd rather also backport > > it to 2.7 so that we know that all 2.7 users can safely upgrade to 2.8 > > (and if it causes any trouble then we know we'll just have to revert > > it from both places). > > > > What do you think ? > > I don't have an opinion on this. This was an accidental find, because I > noticed the typo while redacting the `info` output for this issue: > > https://github.com/haproxy/haproxy/issues/2091 > > We don't have any interest in this particular value, we don't track it in > monitoring. OK, let's see what other users and participants think about it. If I get at least one "please don't change it" I'll abide, otherwise it may make sense to fix it before it ossifies and annoys some future users. Anyone has any opinion here ? Thanks, Willy
Re: [PATCH] BUG/MINOR: Fix typo in `TotalSplicedBytesOut` field name
Willy, On 3/27/23 18:17, Willy Tarreau wrote: Hmmm that's embarrassing indeed. On the one hand we should consider that it's part of the visible API and should stay like this eternally (like "Referer" in HTTP). On the other hand, it was introduced only in 2.3, splicing is not the most commonly used feature (to say the least), and the likelihood that it had ever been used is extremely faint, so it is tempting to fix it before its use is widespread. This is also supported by search engines not reporting any occurrence of it aside a few rare dumps. However we are normally extremely careful about guaranteeing that a stable user can upgrade to the next LTS version. And here this would not be the case. So if we merge it into 2.8 I'd rather also backport it to 2.7 so that we know that all 2.7 users can safely upgrade to 2.8 (and if it causes any trouble then we know we'll just have to revert it from both places). What do you think ? I don't have an opinion on this. This was an accidental find, because I noticed the typo while redacting the `info` output for this issue: https://github.com/haproxy/haproxy/issues/2091 We don't have any interest in this particular value, we don't track it in monitoring. Best regards Tim Düsterhus Developer WoltLab GmbH -- WoltLab GmbH Nedlitzer Str. 27B 14469 Potsdam Tel.: +49 331 96784338 duester...@woltlab.com www.woltlab.com Managing director: Marcel Werk AG Potsdam HRB 26795 P
Re: [PATCH] BUG/MINOR: Fix typo in `TotalSplicedBytesOut` field name
Hi Tim, On Mon, Mar 27, 2023 at 03:26:51PM +0200, Tim Düsterhus, WoltLab GmbH wrote: > From d1c3bd09b95e6f68d8dc849b0637088b79856fbc Mon Sep 17 00:00:00 2001 > From: Tim Duesterhus > Date: Mon, 27 Mar 2023 15:23:44 +0200 > Subject: [PATCH] BUG/MINOR: Fix typo in `TotalSplicedBytesOut` field name > To: haproxy@formilux.org > Cc: w...@1wt.eu > > An additiona `d` slipped in there. > > This likely should not be backported, because scripts might rely on the > typoed name. Hmmm that's embarrassing indeed. On the one hand we should consider that it's part of the visible API and should stay like this eternally (like "Referer" in HTTP). On the other hand, it was introduced only in 2.3, splicing is not the most commonly used feature (to say the least), and the likelihood that it had ever been used is extremely faint, so it is tempting to fix it before its use is widespread. This is also supported by search engines not reporting any occurrence of it aside a few rare dumps. However we are normally extremely careful about guaranteeing that a stable user can upgrade to the next LTS version. And here this would not be the case. So if we merge it into 2.8 I'd rather also backport it to 2.7 so that we know that all 2.7 users can safely upgrade to 2.8 (and if it causes any trouble then we know we'll just have to revert it from both places). What do you think ? Thanks, Willy
Re: [PATCH] BUG/MINOR: ssl: Stop leaking `err` in ssl_sock_load_ocsp()
On 27/03/2023 15:31, Tim Düsterhus wrote: Hi On 3/19/23 16:07, Tim Duesterhus wrote: Previously performing a config check of `.github/h2spec.config` would report a 20 byte leak as reported in GitHub Issue #2082. The leak was introduced in a6c0a59e9af65180c3ff591b91855bea8d19b352, which is dev only. No backport needed. I believe you might've missed this patch. Best regards Tim Düsterhus Hi Tim, Sorry about that delay. The patch looks good to me. I'll let William merge it when he has the time. Rémi
Re: [PATCH] BUG/MINOR: ssl: Stop leaking `err` in ssl_sock_load_ocsp()
Hi On 3/19/23 16:07, Tim Duesterhus wrote: Previously performing a config check of `.github/h2spec.config` would report a 20 byte leak as reported in GitHub Issue #2082. The leak was introduced in a6c0a59e9af65180c3ff591b91855bea8d19b352, which is dev only. No backport needed. I believe you might've missed this patch. Best regards Tim Düsterhus
[PATCH] BUG/MINOR: Fix typo in `TotalSplicedBytesOut` field name
Hi
please find the patch attached.
This email address is not subscribed to the list, please keep it in Cc
when replying.
Best regards
Tim Düsterhus
Developer WoltLab GmbH
--
WoltLab GmbH
Nedlitzer Str. 27B
14469 Potsdam
Tel.: +49 331 96784338
duester...@woltlab.com
www.woltlab.com
Managing director:
Marcel Werk
AG Potsdam HRB 26795 PFrom d1c3bd09b95e6f68d8dc849b0637088b79856fbc Mon Sep 17 00:00:00 2001
From: Tim Duesterhus
Date: Mon, 27 Mar 2023 15:23:44 +0200
Subject: [PATCH] BUG/MINOR: Fix typo in `TotalSplicedBytesOut` field name
To: haproxy@formilux.org
Cc: w...@1wt.eu
An additiona `d` slipped in there.
This likely should not be backported, because scripts might rely on the typoed name.
---
src/stats.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/stats.c b/src/stats.c
index d9bb367f7d..3a878f15dc 100644
--- a/src/stats.c
+++ b/src/stats.c
@@ -150,7 +150,7 @@ const struct name_desc info_fields[INF_TOTAL_FIELDS] = {
[INF_BUSY_POLLING] = { .name = "BusyPolling", .desc = "1 if busy-polling is currently in use on the worker process, otherwise zero (config.busy-polling)" },
[INF_FAILED_RESOLUTIONS] = { .name = "FailedResolutions", .desc = "Total number of failed DNS resolutions in current worker process since started" },
[INF_TOTAL_BYTES_OUT]= { .name = "TotalBytesOut", .desc = "Total number of bytes emitted by current worker process since started" },
- [INF_TOTAL_SPLICED_BYTES_OUT]= { .name = "TotalSplicdedBytesOut", .desc = "Total number of bytes emitted by current worker process through a kernel pipe since started" },
+ [INF_TOTAL_SPLICED_BYTES_OUT]= { .name = "TotalSplicedBytesOut",.desc = "Total number of bytes emitted by current worker process through a kernel pipe since started" },
[INF_BYTES_OUT_RATE] = { .name = "BytesOutRate",.desc = "Number of bytes emitted by current worker process over the last second" },
[INF_DEBUG_COMMANDS_ISSUED] = { .name = "DebugCommandsIssued", .desc = "Number of debug commands issued on this process (anything > 0 is unsafe)" },
[INF_CUM_LOG_MSGS] = { .name = "CumRecvLogs", .desc = "Total number of log messages received by log-forwarding listeners on this worker process since started" },
--
2.40.0
Re: [PATCH] BUG/MINOR: illegal use of the malloc_trim() function if jemalloc is used
On Wed, Mar 22, 2023 at 02:39:15PM +0100, Miroslav Zagorac wrote: > On 22. 03. 2023. 14:33, Willy Tarreau wrote: > >> Also, in that case, when calling 'haproxy -vv', it is no longer printed > >> that > >> malloc_trim is enabled. > > > > I'm not sure what you mean here, because for me it's never printed that > > malloc_trim() is enabled when building with jemalloc. > > > > Hello Willy, > > without the patch: > > % make -j8 ADDLIB="-ljemalloc" TARGET="linux-glibc" IGNOREGIT=1 > USE_GETADDRINFO=1 USE_LIBCRYPT=1 USE_LUA=1 USE_MEMORY_PROFILING=1 > USE_MODULES=1 USE_NS=1 USE_OPENSSL=1 USE_PCRE=1 USE_PCRE_JIT=1 USE_TFO=1 > USE_THREAD=1 USE_ZLIB=1 > > % ldd haproxy | grep jemalloc > libjemalloc.so.2 => /lib/x86_64-linux-gnu/libjemalloc.so.2 > (0x7f1319e3a000) > > ./haproxy -vv | grep trim > Support for malloc_trim() is enabled. Ah thanks now I understand. For me it wasn't the case because I LD_PRELOADed it. However by doing so you've changed the semantics of this variable "using_default_allocator" whose role was exactly to detect if the allocator had changed from the built-in one. That's why it wasn't called "using_glibc_allocator" or so. Admittedly that part was still seriously lacking comments... I'd rather turn it back to what it was and fix the problem where the message is emitted instead. Anyway on latest changes we do emulate malloc_trim() using the equivalent in the other libs so in the end I'll just remove that message as it doesn't provide any value anymore. Thanks for the explanation! Willy
Re: [PATCH] BUG/MINOR: illegal use of the malloc_trim() function if jemalloc is used
On 22. 03. 2023. 14:33, Willy Tarreau wrote: >> Also, in that case, when calling 'haproxy -vv', it is no longer printed that >> malloc_trim is enabled. > > I'm not sure what you mean here, because for me it's never printed that > malloc_trim() is enabled when building with jemalloc. > Hello Willy, without the patch: % make -j8 ADDLIB="-ljemalloc" TARGET="linux-glibc" IGNOREGIT=1 USE_GETADDRINFO=1 USE_LIBCRYPT=1 USE_LUA=1 USE_MEMORY_PROFILING=1 USE_MODULES=1 USE_NS=1 USE_OPENSSL=1 USE_PCRE=1 USE_PCRE_JIT=1 USE_TFO=1 USE_THREAD=1 USE_ZLIB=1 % ldd haproxy | grep jemalloc libjemalloc.so.2 => /lib/x86_64-linux-gnu/libjemalloc.so.2 (0x7f1319e3a000) ./haproxy -vv | grep trim Support for malloc_trim() is enabled. -- Zaga What can change the nature of a man?
Re: [PATCH] BUG/MINOR: illegal use of the malloc_trim() function if jemalloc is used
Hi Miroslav, On Wed, Mar 22, 2023 at 01:11:53PM +0100, Miroslav Zagorac wrote: > Hello all, > > here is a patch that does not allow the use of malloc_trim() function if > HAProxy is linked with the malloc library. It was checked in some places in > the source, but not everywhere. Oh thanks for this, I remember noting it along a previous debugging session related to the patterns, and I would have sworn it was done! Interestingly we were speaking with William about this precise function 2 hours ago, saying that a more reliable long-term solution would be to intercept it and replace it so that any possible calls made from child libraries could be intercepted. This would then permit to get rid of the library compatibility check in dl_open(). > Also, in that case, when calling 'haproxy -vv', it is no longer printed that > malloc_trim is enabled. I'm not sure what you mean here, because for me it's never printed that malloc_trim() is enabled when building with jemalloc. Thanks! Willy
Re: [PATCH] BUG/MINOR: illegal use of the malloc_trim() function if jemalloc is used
On 22. 03. 2023. 13:11, Miroslav Zagorac wrote: > Hello all, > > here is a patch that does not allow the use of malloc_trim() function if > HAProxy is linked with the malloc library. It was checked in some places in .. jemalloc .., sorry for the typo. > the source, but not everywhere. > > Also, in that case, when calling 'haproxy -vv', it is no longer printed that > malloc_trim is enabled. > -- Miroslav Zagorac Senior Developer
[PATCH] BUG/MINOR: illegal use of the malloc_trim() function if jemalloc is used
Hello all,
here is a patch that does not allow the use of malloc_trim() function if
HAProxy is linked with the malloc library. It was checked in some places in
the source, but not everywhere.
Also, in that case, when calling 'haproxy -vv', it is no longer printed that
malloc_trim is enabled.
--
Miroslav Zagorac
Senior DeveloperFrom 33a84d19548baf46e8fccbb58a891a3e4c6b4895 Mon Sep 17 00:00:00 2001
From: Miroslav Zagorac
Date: Wed, 22 Mar 2023 12:52:19 +0100
Subject: [PATCH] BUG/MINOR: illegal use of the malloc_trim() function if
jemalloc is used
In the event that HAProxy is linked with the jemalloc library, it is still
shown that malloc_trim() is enabled when executing "haproxy -vv":
..
Support for malloc_trim() is enabled.
..
It's not so much a problem as it is that malloc_trim() is called in the
pat_ref_purge_range() function without any checking.
This was solved by setting the using_default_allocator variable to the
correct value in the detect_allocator() function and before calling
malloc_trim() it is checked whether the function should be called.
---
include/haproxy/pool.h | 1 +
src/pattern.c | 2 +-
src/pool.c | 9 -
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/include/haproxy/pool.h b/include/haproxy/pool.h
index 16146604b..a12990d24 100644
--- a/include/haproxy/pool.h
+++ b/include/haproxy/pool.h
@@ -101,6 +101,7 @@ extern int mem_poison_byte;
/* set of POOL_DBG_* flags */
extern uint pool_debugging;
+int is_trim_enabled(void);
void *pool_get_from_os(struct pool_head *pool);
void pool_put_to_os(struct pool_head *pool, void *ptr);
void *pool_alloc_nocache(struct pool_head *pool);
diff --git a/src/pattern.c b/src/pattern.c
index a2557dea1..71f25a43b 100644
--- a/src/pattern.c
+++ b/src/pattern.c
@@ -2084,7 +2084,7 @@ int pat_ref_purge_range(struct pat_ref *ref, uint from, uint to, int budget)
HA_RWLOCK_WRUNLOCK(PATEXP_LOCK, >lock);
#if defined(HA_HAVE_MALLOC_TRIM)
- if (done) {
+ if (done && is_trim_enabled()) {
malloc_trim(0);
}
#endif
diff --git a/src/pool.c b/src/pool.c
index 345681a0c..214d950fb 100644
--- a/src/pool.c
+++ b/src/pool.c
@@ -177,11 +177,10 @@ static void detect_allocator(void)
my_mallctl = mallctl;
#endif
-
- if (!my_mallctl) {
+ if (!my_mallctl)
my_mallctl = get_sym_curr_addr("mallctl");
- using_default_allocator = (my_mallctl == NULL);
- }
+
+ using_default_allocator = (my_mallctl == NULL);
if (!my_mallctl) {
#if defined(HA_HAVE_MALLOC_TRIM)
@@ -212,7 +211,7 @@ static void detect_allocator(void)
}
}
-static int is_trim_enabled(void)
+int is_trim_enabled(void)
{
return using_default_allocator;
}
--
2.37.1
[PATCH] BUG/MINOR: ssl: Stop leaking `err` in ssl_sock_load_ocsp()
Previously performing a config check of `.github/h2spec.config` would report a 20 byte leak as reported in GitHub Issue #2082. The leak was introduced in a6c0a59e9af65180c3ff591b91855bea8d19b352, which is dev only. No backport needed. --- src/ssl_sock.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/ssl_sock.c b/src/ssl_sock.c index 5993a72b06..801543cb77 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -1296,6 +1296,8 @@ static int ssl_sock_load_ocsp(const char *path, SSL_CTX *ctx, struct ckch_data * if (warn) free(warn); + free(err); + return ret; } -- 2.40.0
Re: [PR] BUG/MINOR: http-fetch: recognize IPv6 addresses in square brackets in req.hdr_ip()
Hi Oto! On Wed, Mar 01, 2023 at 01:23:02PM +0100, PR Bot wrote: > Dear list! > > Author: Oto Valek > Number of patches: 2 > > This is an automated relay of the Github pull request: >BUG/MINOR: http-fetch: recognize IPv6 addresses in square brackets in >req.hdr_ip() > > Patch title(s): >BUG/MINOR: http-fetch: recognize IPv6 addresses in square brackets in > req.hdr_ip() >REGTEST: added tests covering smp_fetch_hdr_ip() Look good now, I've merged it. Thank you! Willy
[PR] BUG/MINOR: http-fetch: recognize IPv6 addresses in square brackets in req.hdr_ip()
Dear list! Author: Oto Valek Number of patches: 2 This is an automated relay of the Github pull request: BUG/MINOR: http-fetch: recognize IPv6 addresses in square brackets in req.hdr_ip() Patch title(s): BUG/MINOR: http-fetch: recognize IPv6 addresses in square brackets in req.hdr_ip() REGTEST: added tests covering smp_fetch_hdr_ip() Link: https://github.com/haproxy/haproxy/pull/2063 Edit locally: wget https://github.com/haproxy/haproxy/pull/2063.patch && vi 2063.patch Apply locally: curl https://github.com/haproxy/haproxy/pull/2063.patch | git am - Description: Fixes haproxy/haproxy#2054 To comply with [RFC7239](https://www.rfc-editor.org/rfc/rfc7239.html#section-6.1) and [RFC3986](https://www.rfc-editor.org/rfc/rfc3986.html#section-3.2.2), the `req.hdr_ip()` should recognize IPv6 addresses in square brackets. As the `inet_pton()` call does not support this format, the `smp_fetch_hdr_ip()` function was changed to trim possible `'['` and `']'` before calling `inet_pton()`. New reg test cases were added to cover all 4 branches of smp_fetch_hdr_ip(). Instructions: This github pull request will be closed automatically; patch should be reviewed on the haproxy mailing list (haproxy@formilux.org). Everyone is invited to comment, even the patch's author. Please keep the author and list CCed in replies. Please note that in absence of any response this pull request will be lost.
[PR] BUG/MINOR: http-fetch: recognize IPv6 addresses in square brackets in req.hdr_ip()
Dear list! Author: Oto Valek Number of patches: 2 This is an automated relay of the Github pull request: BUG/MINOR: http-fetch: recognize IPv6 addresses in square brackets in req.hdr_ip() Patch title(s): BUG/MINOR: http-fetch: recognize IPv6 addresses in square brackets in req.hdr_ip() REGTEST: enclose one of the IPv6 address in square brackets Link: https://github.com/haproxy/haproxy/pull/2055 Edit locally: wget https://github.com/haproxy/haproxy/pull/2055.patch && vi 2055.patch Apply locally: curl https://github.com/haproxy/haproxy/pull/2055.patch | git am - Description: Fixes haproxy/haproxy#2054 To comply with [RFC7239](https://www.rfc-editor.org/rfc/rfc7239.html#section-6.1) and [RFC3986](https://www.rfc-editor.org/rfc/rfc3986.html#section-3.2.2), the `req.hdr_ip()` should recognize IPv6 addresses in square brackets. As the `inet_pton()` call does not support this format, the `smp_fetch_hdr_ip()` function was changed to trim possible `'['` and `']'` before calling `inet_pton()`. Reg tests were updated to cover this case. Instructions: This github pull request will be closed automatically; patch should be reviewed on the haproxy mailing list (haproxy@formilux.org). Everyone is invited to comment, even the patch's author. Please keep the author and list CCed in replies. Please note that in absence of any response this pull request will be lost.
Re: [PATCH] BUG/MINOR: mux-fcgi: Correctly set pathinfo
On Tue, Jan 17, 2023 at 09:44:11AM +1100, Paul Barnetta wrote: > Existing logic for checking whether a regex subexpression for pathinfo > is matched results in valid matches being ignored and non-matches having > a new zero length string stored in params->pathinfo. This patch reverses > the logic so params->pathinfo is set when the subexpression is matched. (...) Applied, thank you Paul! Willy
[PATCH] BUG/MINOR: mux-fcgi: Correctly set pathinfo
Existing logic for checking whether a regex subexpression for pathinfo is matched results in valid matches being ignored and non-matches having a new zero length string stored in params->pathinfo. This patch reverses the logic so params->pathinfo is set when the subexpression is matched. Without this patch the example configuration in the documentation: path-info ^(/.+\.php)(/.*)?$ does not result in PATH_INFO being sent to the FastCGI application, as expected, when the second subexpression is matched (in which case both pmatch[2].rm_so and pmatch[2].rm_eo will be non-negative integers). This patch may be backported as far as 2.2, the first release that made the capture of this second subexpression optional. --- src/mux_fcgi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mux_fcgi.c b/src/mux_fcgi.c index 597fbcbbd..9b4827174 100644 --- a/src/mux_fcgi.c +++ b/src/mux_fcgi.c @@ -1308,7 +1308,7 @@ static int fcgi_set_default_param(struct fcgi_conn *fconn, struct fcgi_strm *fst * captured */ params->scriptname = ist2(path.ptr + pmatch[1].rm_so, pmatch[1].rm_eo - pmatch[1].rm_so); - if (!(params->mask & FCGI_SP_PATH_INFO) && (pmatch[2].rm_so == -1 || pmatch[2].rm_eo == -1)) + if (!(params->mask & FCGI_SP_PATH_INFO) && !(pmatch[2].rm_so == -1 || pmatch[2].rm_eo == -1)) params->pathinfo = ist2(path.ptr + pmatch[2].rm_so, pmatch[2].rm_eo - pmatch[2].rm_so); check_index: -- 2.39.0
Re: [PATCH 1/1] BUG/MEDIUM: tests: use tmpdir to create UNIX socket
Hi Bertrand, On Sat, Dec 17, 2022 at 09:39:38PM +, Bertrand Jacquin wrote: > testdir can be a very long directory since it depends on source > directory path, this can lead to failure during tests when UNIX socket > path exceeds maximum allowed length of 97 characters as defined in > str2sa_range(). > > 16:48:14 [ALERT] *** h1debug|(10082) : config : parsing > [/tmp/haregtests-2022-12-17_16-47-39.4RNzIN/vtc.4850.5d0d728a/h1/cfg:19] : > 'bind' : socket path > 'unix@/local/p4clients/pkgbuild-bB20r/workspace/build/HAProxy/HAProxy-2.7.x.68.0/AL2_x86_64/DEV.STD.PTHREAD/build/private/HAProxy-2.7.x/src/reg-tests/lua/srv3' > too long (max 97) > > Also, it is not advisable to create UNIX socket in actual source > directory, but instead use dedicated temporary directory create for test > purpose. Ah good catch indeed! In addition this could also make tests randomly fail if multiple were run in parallel with different settings. Now merged, thank you! Willy
[PATCH 1/1] BUG/MEDIUM: tests: use tmpdir to create UNIX socket
testdir can be a very long directory since it depends on source
directory path, this can lead to failure during tests when UNIX socket
path exceeds maximum allowed length of 97 characters as defined in
str2sa_range().
16:48:14 [ALERT] *** h1debug|(10082) : config : parsing
[/tmp/haregtests-2022-12-17_16-47-39.4RNzIN/vtc.4850.5d0d728a/h1/cfg:19] :
'bind' : socket path
'unix@/local/p4clients/pkgbuild-bB20r/workspace/build/HAProxy/HAProxy-2.7.x.68.0/AL2_x86_64/DEV.STD.PTHREAD/build/private/HAProxy-2.7.x/src/reg-tests/lua/srv3'
too long (max 97)
Also, it is not advisable to create UNIX socket in actual source
directory, but instead use dedicated temporary directory create for test
purpose.
This should be backported to 2.6
---
reg-tests/lua/lua_httpclient.vtc | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/reg-tests/lua/lua_httpclient.vtc b/reg-tests/lua/lua_httpclient.vtc
index 0850ddb5f3f2..0a274932ab23 100644
--- a/reg-tests/lua/lua_httpclient.vtc
+++ b/reg-tests/lua/lua_httpclient.vtc
@@ -51,13 +51,13 @@ haproxy h1 -conf {
listen li1
mode http
- bind unix@${testdir}/srv3
+ bind unix@${tmpdir}/srv3
server srv3 ${s3_addr}:${s3_port}
} -start
client c0 -connect ${h1_fe1_sock} {
-txreq -url "/" -hdr "vtcport: ${s1_port}" -hdr "vtcport2: ${s2_port}" -hdr
"vtcport3: unix@${testdir}/srv3"
+txreq -url "/" -hdr "vtcport: ${s1_port}" -hdr "vtcport2: ${s2_port}" -hdr
"vtcport3: unix@${tmpdir}/srv3"
rxresp
expect resp.status == 200
} -run
Re: [PATCH 0/2] BUG/MINOR: promex: create haproxy_backend_agg_check_status
Le 12/8/22 à 10:16, Cedric Paillet a écrit : As described in github issue #1312, the first intention of patch 42d7c402d was to aggregate haproxy_server_check_status. But we aggregated haproxy_server_status instead. To fix that: - Deprecated haproxy_backend_agg_server_check_status. (Modify the metric description) - create new haproxy_backend_agg_server_status metric, aggregation of haproxy_backend_server_status. (to replace misnamed haproxy_backend_agg_server_check_status) - create new haproxy_backend_agg_check_status metric, aggregation of haproxy_backend_server_check_status. Cedric Paillet (2): BUG/MINOR: promex: create haproxy_backend_agg_server_status MINOR: promex: introduce haproxy_backend_agg_check_status addons/promex/service-prometheus.c | 30 +- include/haproxy/stats-t.h | 2 ++ src/stats.c| 10 -- 3 files changed, 39 insertions(+), 3 deletions(-) Thanks, both patches were merged ! I mentioned it could be backported as far as 2.4. -- Christopher Faulet
[PATCH 1/2] [PATCH] BUG/MINOR: promex: create haproxy_backend_agg_server_status
haproxy_backend_agg_server_check_status currently aggregates
haproxy_server_status instead of haproxy_server_check_status.
We deprecate this and create a new one,
haproxy_backend_agg_server_status to clarify what it really
does.
---
addons/promex/service-prometheus.c | 4 +++-
include/haproxy/stats-t.h | 1 +
src/stats.c| 6 --
3 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/addons/promex/service-prometheus.c
b/addons/promex/service-prometheus.c
index b54695c17..9330a860c 100644
--- a/addons/promex/service-prometheus.c
+++ b/addons/promex/service-prometheus.c
@@ -302,6 +302,7 @@ const struct promex_metric
promex_st_metrics[ST_F_TOTAL_FIELDS] = {
[ST_F_NEED_CONN_EST]= { .n = IST("need_connections_current"),
.type = PROMEX_MT_GAUGE,.flags = (
PROMEX_FL_SRV_METRIC) },
[ST_F_UWEIGHT] = { .n = IST("uweight"),
.type = PROMEX_MT_GAUGE,.flags = (
PROMEX_FL_BACK_METRIC | PROMEX_FL_SRV_METRIC) },
[ST_F_AGG_SRV_CHECK_STATUS] = { .n = IST("agg_server_check_status"),
.type = PROMEX_MT_GAUGE,.flags = (
PROMEX_FL_BACK_METRIC ) },
+ [ST_F_AGG_SRV_STATUS ] = { .n = IST("agg_server_status"),
.type = PROMEX_MT_GAUGE,.flags = (
PROMEX_FL_BACK_METRIC ) },
};
/* Description of overridden stats fields */
@@ -833,7 +834,8 @@ static int promex_dump_back_metrics(struct appctx *appctx,
struct htx *htx)
return -1;
switch (ctx->field_num) {
- case ST_F_AGG_SRV_CHECK_STATUS:
+ case ST_F_AGG_SRV_CHECK_STATUS: // DEPRECATED
+ case ST_F_AGG_SRV_STATUS:
if (!px->srv)
goto next_px;
sv = px->srv;
diff --git a/include/haproxy/stats-t.h b/include/haproxy/stats-t.h
index efa0ac383..253fb6c48 100644
--- a/include/haproxy/stats-t.h
+++ b/include/haproxy/stats-t.h
@@ -455,6 +455,7 @@ enum stat_field {
ST_F_USED_CONN_CUR,
ST_F_NEED_CONN_EST,
ST_F_UWEIGHT,
+ ST_F_AGG_SRV_STATUS,
ST_F_AGG_SRV_CHECK_STATUS,
/* must always be the last one */
diff --git a/src/stats.c b/src/stats.c
index 8ed5a7133..c15bc27ec 100644
--- a/src/stats.c
+++ b/src/stats.c
@@ -259,7 +259,8 @@ const struct name_desc stat_fields[ST_F_TOTAL_FIELDS] = {
[ST_F_USED_CONN_CUR] = { .name = "used_conn_cur",
.desc = "Current number of connections in use"},
[ST_F_NEED_CONN_EST] = { .name = "need_conn_est",
.desc = "Estimated needed number of connections"},
[ST_F_UWEIGHT] = { .name = "uweight",
.desc = "Server's user weight, or sum of active servers' user weights
for a backend" },
- [ST_F_AGG_SRV_CHECK_STATUS] = { .name =
"agg_server_check_status", .desc = "Backend's aggregated gauge of servers'
state check status" },
+ [ST_F_AGG_SRV_CHECK_STATUS] = { .name =
"agg_server_check_status", .desc = "[DEPRECATED] Backend's aggregated gauge
of servers' status" },
+ [ST_F_AGG_SRV_STATUS ] = { .name = "agg_server_status",
.desc = "Backend's aggregated gauge of servers' status" },
};
/* one line of info */
@@ -2673,7 +2674,8 @@ int stats_fill_be_stats(struct proxy *px, int flags,
struct field *stats, int le
chunk_appendf(out, " (%d/%d)", nbup,
nbsrv);
metric = mkf_str(FO_STATUS, fld);
break;
- case ST_F_AGG_SRV_CHECK_STATUS:
+ case ST_F_AGG_SRV_CHECK_STATUS: // DEPRECATED
+ case ST_F_AGG_SRV_STATUS:
metric = mkf_u32(FN_GAUGE, 0);
break;
case ST_F_WEIGHT:
--
2.25.1
[PATCH 0/2] BUG/MINOR: promex: create haproxy_backend_agg_check_status
As described in github issue #1312, the first intention of patch 42d7c402d was to aggregate haproxy_server_check_status. But we aggregated haproxy_server_status instead. To fix that: - Deprecated haproxy_backend_agg_server_check_status. (Modify the metric description) - create new haproxy_backend_agg_server_status metric, aggregation of haproxy_backend_server_status. (to replace misnamed haproxy_backend_agg_server_check_status) - create new haproxy_backend_agg_check_status metric, aggregation of haproxy_backend_server_check_status. Cedric Paillet (2): BUG/MINOR: promex: create haproxy_backend_agg_server_status MINOR: promex: introduce haproxy_backend_agg_check_status addons/promex/service-prometheus.c | 30 +- include/haproxy/stats-t.h | 2 ++ src/stats.c| 10 -- 3 files changed, 39 insertions(+), 3 deletions(-) -- 2.25.1
Re: [PATCH 0/2] BUG/MINOR: promex: fix haproxy_backend_agg_server_check_status
Le 12/6/22 à 15:13, Cedric Paillet a écrit : Sorry, indeed I mentioned the enum ID and not the promex name. I proposed to keep "haproxy_bakcned_agg_server_check_status" altering its description to announce it is deprecated . And then to add "haproxy_backend_agg_check_status" aggregate haproxy_server_check_status) and "haproxy_backend_agg_server_status" or "haproxy_backend_agg_srv_status" (to aggregate haproxy_server_status). To resume, is it ok for you ? - Deprecated haproxy_backend_agg_server_check_status. ( Modify the metric description ) - create new haproxy_backend_agg_server_status metric, aggregation of haproxy_backend_server_status. ( to replace misnamed haproxy_backend_agg_server_check_status ) - create new haproxy_backend_agg_check_status metric, aggregation of haproxy_backend_server_check_status. this will be merged/backported in 2.5/2.6/2.7/2.8-dev and haproxy_backend_agg_server_check_status will be removed in 2.9 ( or 2.10 ) Exactly. -- Christopher Faulet
RE: RE: [PATCH 0/2] BUG/MINOR: promex: fix haproxy_backend_agg_server_check_status
>Sorry, indeed I mentioned the enum ID and not the promex name. >I proposed to keep "haproxy_bakcned_agg_server_check_status" > altering its description to announce it is deprecated >. And then to add "haproxy_backend_agg_check_status" >aggregate haproxy_server_check_status) and >"haproxy_backend_agg_server_status" > or "haproxy_backend_agg_srv_status" (to aggregate haproxy_server_status). To resume, is it ok for you ? - Deprecated haproxy_backend_agg_server_check_status. ( Modify the metric description ) - create new haproxy_backend_agg_server_status metric, aggregation of haproxy_backend_server_status. ( to replace misnamed haproxy_backend_agg_server_check_status ) - create new haproxy_backend_agg_check_status metric, aggregation of haproxy_backend_server_check_status. this will be merged/backported in 2.5/2.6/2.7/2.8-dev and haproxy_backend_agg_server_check_status will be removed in 2.9 ( or 2.10 )
Re: [PATCH 0/2] BUG/MINOR: promex: fix haproxy_backend_agg_server_check_status
On Tue, Dec 6, 2022 at 3:27 AM Christopher Faulet wrote: > IMHO, we should keep the current metric and its description should be updated > to > mention it is deprecated. This way, we will be able to remove it in the 2.9 or > 3.0. This means we should find new names for the aggregated servers status and > the aggregated check status. "ST_F_AGG_SRV_STATUS" is pretty good for the > first > one. I may propose "ST_F_AGG_CHECK_STATUS" for the second one. > > I guess the two new metrics may be backported. They are not used by the stats > applet. > > What do you think about it? I am aligned with this plan of creating two new metrics Thanks, -- William
Re: RE: [PATCH 0/2] BUG/MINOR: promex: fix haproxy_backend_agg_server_check_status
Le 12/6/22 à 12:26, Cedric Paillet a écrit : Sorry for the delay. We were busy because of the 2.7.0 and then I forgot your patches :) And it was a bit late to add it into the 2.7.0. No problem. Because the metric name is indeed badly chosen but we can't break the compatibility, especially in a LTS version. Ok, I understand that. We need to find new names for the metric IMHO, we should keep the current metric and its description should be updated to mention it is deprecated. This way, we will be able to remove it in the 2.9 or 3.0. This means we should find new names for the aggregated servers status and the aggregated check status. "ST_F_AGG_SRV_STATUS" is pretty good for the first one. I may propose "ST_F_AGG_CHECK_STATUS" for the second one. Not very clear, the value to change to keep the compatibility is the metric name itself ? do you propose to use?: - haproxy_backend_agg_check_status ( to aggregate haproxy_server_check_status ) - haproxy_backend_agg_status ( to aggregate haproxy_server_status ) Or - haproxy_backend_agg_srv_check_status ( to aggregate haproxy_server_check_status - haproxy_backend_agg_srv_status ( to aggregate haproxy_server_status ) For reminder we currently have - haproxy_backend_agg_server_check_status ( to aggregate haproxy_server_status ) ( and we have also have haproxy_backend_status, for the status of the backend itself ) Cedric Paillet Sorry, indeed I mentioned the enum ID and not the promex name. I proposed to keep "haproxy_bakcned_agg_server_check_status" altering its description to announce it is deprecated. And then to add "haproxy_backend_agg_check_status" (aggregate haproxy_server_check_status) and "haproxy_backend_agg_server_status" or "haproxy_backend_agg_srv_status" (to aggregate haproxy_server_status). -- Christopher Faulet
RE: [PATCH 0/2] BUG/MINOR: promex: fix haproxy_backend_agg_server_check_status
> Sorry for the delay. We were busy because of the 2.7.0 and then I forgot your > patches :) And it was a bit late to add it into the 2.7.0. No problem. > Because the metric name is indeed badly chosen but we can't break the > compatibility, especially in a LTS version. Ok, I understand that. We need to find new names for the metric >IMHO, we should keep the current metric and its description should be updated >to mention it is deprecated. This way, we will be able to remove it in the 2.9 >or 3.0. This means we should find new names for the aggregated servers status >and the aggregated check status. "ST_F_AGG_SRV_STATUS" is pretty good for the >first one. I may propose "ST_F_AGG_CHECK_STATUS" for the second one. Not very clear, the value to change to keep the compatibility is the metric name itself ? do you propose to use?: - haproxy_backend_agg_check_status ( to aggregate haproxy_server_check_status ) - haproxy_backend_agg_status ( to aggregate haproxy_server_status ) Or - haproxy_backend_agg_srv_check_status ( to aggregate haproxy_server_check_status - haproxy_backend_agg_srv_status ( to aggregate haproxy_server_status ) For reminder we currently have - haproxy_backend_agg_server_check_status ( to aggregate haproxy_server_status ) ( and we have also have haproxy_backend_status, for the status of the backend itself ) Cedric Paillet
Re: [PATCH 0/2] BUG/MINOR: promex: fix haproxy_backend_agg_server_check_status
Le 11/28/22 à 08:44, Cedric Paillet a écrit : As described in github issue #1312, the first intention of patch 42d7c402d was to aggregate haproxy_server_check_status. But we aggregated haproxy_server_status instead. To fix that, rename haproxy_backend_agg_server_check_status to haproxy_backend_agg_server_status and use right data source for haproxy_backend_agg_server_check_status. Cedric Paillet (2): BUG/MINOR: promex: rename haproxy_backend_agg_server_check_status MINOR: promex: reintroduce haproxy_backend_agg_server_check_status addons/promex/service-prometheus.c | 28 +++- include/haproxy/stats-t.h | 1 + src/stats.c| 4 3 files changed, 32 insertions(+), 1 deletion(-) Hi Cedric, Sorry for the delay. We were busy because of the 2.7.0 and then I forgot your patches :) And it was a bit late to add it into the 2.7.0. So, I'm a bit bother. Because the metric name is indeed badly chosen but we can't break the compatibility, especially in a LTS version. The prometheus exporter is an addon. Thus the backward compatibility is less strict than for core code of HAProxy. However, it is probably the most used addon. IMHO, we should keep the current metric and its description should be updated to mention it is deprecated. This way, we will be able to remove it in the 2.9 or 3.0. This means we should find new names for the aggregated servers status and the aggregated check status. "ST_F_AGG_SRV_STATUS" is pretty good for the first one. I may propose "ST_F_AGG_CHECK_STATUS" for the second one. I guess the two new metrics may be backported. They are not used by the stats applet. What do you think about it? -- Christopher Faulet
RE: [PATCH 0/2] BUG/MINOR: promex: fix haproxy_backend_agg_server_check_status
Hi William, For me we it's not a problem to backport until 2.4. Even if it's a breaking change for haproxy_backend_agg_server_check_status, it's clearer and make more sense if it's aggregate server_check_status and not server_status. Cedric. -Message d'origine- De : William Dauchy Envoyé : mardi 29 novembre 2022 12:39 À : Cedric Paillet Cc : haproxy@formilux.org; Christopher Faulet Objet : Re: [PATCH 0/2] BUG/MINOR: promex: fix haproxy_backend_agg_server_check_status On Tue, Nov 29, 2022 at 11:52 AM William Dauchy wrote: > On Mon, Nov 28, 2022 at 8:46 AM Cedric Paillet wrote: > > As described in github issue #1312, the first intention of patch > > 42d7c402d was to aggregate haproxy_server_check_status. But we > > aggregated haproxy_server_status instead. > > To fix that, rename haproxy_backend_agg_server_check_status to > > haproxy_backend_agg_server_status and use right data source for > > haproxy_backend_agg_server_check_status. > > Thank you for looking into this. This has been on my todo list for too long. > I am fine with those changes as long as we backport them until the > version where this was introduced (this was introduced in v2.5 and > then backported in v2.4 if my checks are correct). > I understand that's a breaking change, some people started to build > tooling on top of that can't deal with behaviour change between > versions (I am thinking about dashboards in my case where we deal with > different versions). In that regard, I believe it can be ok to have to > do a one off change to fix this past mistake. If we are not aligned > with a backport, we will need to find another name. I realised I was not necessarily very clear on my position: - either we merge this patch, and then backport it until 2.4 - either we find a new name for the new metric and don't introduce a breaking change I am mostly worried about metrics collectors who are not able to deal with different behaviour from one version to another. In my own case, we have different haproxy versions running, and there is no global way we can adapt the collector depending on the software version it is collecting. The collector expects the same behaviour across versions. Having a breaking change which affects all versions is fine as it will require a unique change on our side, but having a situation in the middle is not acceptable for us. -- William
Re: [PATCH 0/2] BUG/MINOR: promex: fix haproxy_backend_agg_server_check_status
On Tue, Nov 29, 2022 at 11:52 AM William Dauchy wrote: > On Mon, Nov 28, 2022 at 8:46 AM Cedric Paillet wrote: > > As described in github issue #1312, the first intention of patch 42d7c402d > > was to aggregate haproxy_server_check_status. But we aggregated > > haproxy_server_status instead. > > To fix that, rename haproxy_backend_agg_server_check_status to > > haproxy_backend_agg_server_status and use right data source for > > haproxy_backend_agg_server_check_status. > > Thank you for looking into this. This has been on my todo list for too long. > I am fine with those changes as long as we backport them until the > version where this was introduced (this was introduced in v2.5 and > then backported in v2.4 if my checks are correct). > I understand that's a breaking change, some people started to build > tooling on top of that can't deal with behaviour change between > versions (I am thinking about dashboards in my case where we deal with > different versions). In that regard, I believe it can be ok to have to > do a one off change to fix this past mistake. If we are not aligned > with a backport, we will need to find another name. I realised I was not necessarily very clear on my position: - either we merge this patch, and then backport it until 2.4 - either we find a new name for the new metric and don't introduce a breaking change I am mostly worried about metrics collectors who are not able to deal with different behaviour from one version to another. In my own case, we have different haproxy versions running, and there is no global way we can adapt the collector depending on the software version it is collecting. The collector expects the same behaviour across versions. Having a breaking change which affects all versions is fine as it will require a unique change on our side, but having a situation in the middle is not acceptable for us. -- William
Re: [PATCH 0/2] BUG/MINOR: promex: fix haproxy_backend_agg_server_check_status
Hello Cedric, On Mon, Nov 28, 2022 at 8:46 AM Cedric Paillet wrote: > As described in github issue #1312, the first intention of patch 42d7c402d > was to aggregate haproxy_server_check_status. But we aggregated > haproxy_server_status instead. > To fix that, rename haproxy_backend_agg_server_check_status to > haproxy_backend_agg_server_status and use right data source for > haproxy_backend_agg_server_check_status. Thank you for looking into this. This has been on my todo list for too long. I am fine with those changes as long as we backport them until the version where this was introduced (this was introduced in v2.5 and then backported in v2.4 if my checks are correct). I understand that's a breaking change, some people started to build tooling on top of that can't deal with behaviour change between versions (I am thinking about dashboards in my case where we deal with different versions). In that regard, I believe it can be ok to have to do a one off change to fix this past mistake. If we are not aligned with a backport, we will need to find another name. -- William
[PATCH 1/2] [PATCH] BUG/MINOR: promex: rename haproxy_backend_agg_server_check_status
haproxy_backend_agg_server_check_status currently aggregates
haproxy_server_status instead of haproxy_server_check_status.
Rename the metric to haproxy_backend_agg_server_status to
clarify what it really does.
---
addons/promex/service-prometheus.c | 4 ++--
include/haproxy/stats-t.h | 2 +-
src/stats.c| 4 ++--
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/addons/promex/service-prometheus.c
b/addons/promex/service-prometheus.c
index d27aefaaa..c24f0895c 100644
--- a/addons/promex/service-prometheus.c
+++ b/addons/promex/service-prometheus.c
@@ -301,7 +301,7 @@ const struct promex_metric
promex_st_metrics[ST_F_TOTAL_FIELDS] = {
[ST_F_USED_CONN_CUR]= { .n = IST("used_connections_current"),
.type = PROMEX_MT_GAUGE,.flags = (
PROMEX_FL_SRV_METRIC) },
[ST_F_NEED_CONN_EST]= { .n = IST("need_connections_current"),
.type = PROMEX_MT_GAUGE,.flags = (
PROMEX_FL_SRV_METRIC) },
[ST_F_UWEIGHT] = { .n = IST("uweight"),
.type = PROMEX_MT_GAUGE,.flags = (
PROMEX_FL_BACK_METRIC | PROMEX_FL_SRV_METRIC) },
- [ST_F_AGG_SRV_CHECK_STATUS] = { .n = IST("agg_server_check_status"),
.type = PROMEX_MT_GAUGE,.flags = (
PROMEX_FL_BACK_METRIC ) },
+ [ST_F_AGG_SRV_STATUS ] = { .n = IST("agg_server_status"),
.type = PROMEX_MT_GAUGE,.flags = (
PROMEX_FL_BACK_METRIC ) },
};
/* Description of overridden stats fields */
@@ -833,7 +833,7 @@ static int promex_dump_back_metrics(struct appctx *appctx,
struct htx *htx)
return -1;
switch (ctx->field_num) {
- case ST_F_AGG_SRV_CHECK_STATUS:
+ case ST_F_AGG_SRV_STATUS :
if (!px->srv)
goto next_px;
sv = px->srv;
diff --git a/include/haproxy/stats-t.h b/include/haproxy/stats-t.h
index efa0ac383..babea31e1 100644
--- a/include/haproxy/stats-t.h
+++ b/include/haproxy/stats-t.h
@@ -455,7 +455,7 @@ enum stat_field {
ST_F_USED_CONN_CUR,
ST_F_NEED_CONN_EST,
ST_F_UWEIGHT,
- ST_F_AGG_SRV_CHECK_STATUS,
+ ST_F_AGG_SRV_STATUS ,
/* must always be the last one */
ST_F_TOTAL_FIELDS
diff --git a/src/stats.c b/src/stats.c
index 97e7fd865..dd3044b58 100644
--- a/src/stats.c
+++ b/src/stats.c
@@ -259,7 +259,7 @@ const struct name_desc stat_fields[ST_F_TOTAL_FIELDS] = {
[ST_F_USED_CONN_CUR] = { .name = "used_conn_cur",
.desc = "Current number of connections in use"},
[ST_F_NEED_CONN_EST] = { .name = "need_conn_est",
.desc = "Estimated needed number of connections"},
[ST_F_UWEIGHT] = { .name = "uweight",
.desc = "Server's user weight, or sum of active servers' user weights
for a backend" },
- [ST_F_AGG_SRV_CHECK_STATUS] = { .name =
"agg_server_check_status", .desc = "Backend's aggregated gauge of servers'
state check status" },
+ [ST_F_AGG_SRV_STATUS ] = { .name = "agg_server_status",
.desc = "Backend's aggregated gauge of servers' status" },
};
/* one line of info */
@@ -2673,7 +2673,7 @@ int stats_fill_be_stats(struct proxy *px, int flags,
struct field *stats, int le
chunk_appendf(out, " (%d/%d)", nbup,
nbsrv);
metric = mkf_str(FO_STATUS, fld);
break;
- case ST_F_AGG_SRV_CHECK_STATUS:
+ case ST_F_AGG_SRV_STATUS :
metric = mkf_u32(FN_GAUGE, 0);
break;
case ST_F_WEIGHT:
--
2.25.1
[PATCH 0/2] BUG/MINOR: promex: fix haproxy_backend_agg_server_check_status
As described in github issue #1312, the first intention of patch 42d7c402d was to aggregate haproxy_server_check_status. But we aggregated haproxy_server_status instead. To fix that, rename haproxy_backend_agg_server_check_status to haproxy_backend_agg_server_status and use right data source for haproxy_backend_agg_server_check_status. Cedric Paillet (2): BUG/MINOR: promex: rename haproxy_backend_agg_server_check_status MINOR: promex: reintroduce haproxy_backend_agg_server_check_status addons/promex/service-prometheus.c | 28 +++- include/haproxy/stats-t.h | 1 + src/stats.c| 4 3 files changed, 32 insertions(+), 1 deletion(-) -- 2.25.1
Re: [PATCH] BUG/MINOR: checks: update pgsql regex on auth packet
Le 9/26/22 à 17:27, Fatih Acar a écrit : This patch adds support to the following authentication methods: - AUTH_REQ_GSS (7) - AUTH_REQ_SSPI (9) - AUTH_REQ_SASL (10) Note that since AUTH_REQ_SASL allows multiple authentication mechanisms such as SCRAM-SHA-256 or SCRAM-SHA-256-PLUS, the auth payload length may vary since the method is sent in plaintext. In order to allow this, the regex now matches any payload length. This partially fixes Github issue #1508 since user authentication is still broken but should restore pre-2.2 behavior. This should be backported up to 2.2. Signed-off-by: Fatih Acar Merged, thanks ! -- Christopher Faulet
Re: [PATCH] BUG/MINOR: checks: update pgsql regex on auth packet
Le 9/26/22 à 17:27, Fatih Acar a écrit : This patch adds support to the following authentication methods: - AUTH_REQ_GSS (7) - AUTH_REQ_SSPI (9) - AUTH_REQ_SASL (10) Note that since AUTH_REQ_SASL allows multiple authentication mechanisms such as SCRAM-SHA-256 or SCRAM-SHA-256-PLUS, the auth payload length may vary since the method is sent in plaintext. In order to allow this, the regex now matches any payload length. This partially fixes Github issue #1508 since user authentication is still broken but should restore pre-2.2 behavior. This should be backported up to 2.2. Thanks ! Sorry for the delay. We will review it and merge it soon. -- Christopher Faulet
[PATCH] BUG/MINOR: checks: update pgsql regex on auth packet
This patch adds support to the following authentication methods:
- AUTH_REQ_GSS (7)
- AUTH_REQ_SSPI (9)
- AUTH_REQ_SASL (10)
Note that since AUTH_REQ_SASL allows multiple authentication mechanisms
such as SCRAM-SHA-256 or SCRAM-SHA-256-PLUS, the auth payload length may
vary since the method is sent in plaintext. In order to allow this, the
regex now matches any payload length.
This partially fixes Github issue #1508 since user authentication is
still broken but should restore pre-2.2 behavior.
This should be backported up to 2.2.
Signed-off-by: Fatih Acar
---
reg-tests/checks/pgsql-check.vtc | 16
src/tcpcheck.c | 2 +-
2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/reg-tests/checks/pgsql-check.vtc b/reg-tests/checks/pgsql-check.vtc
index 417932ee9..2c9c65b0e 100644
--- a/reg-tests/checks/pgsql-check.vtc
+++ b/reg-tests/checks/pgsql-check.vtc
@@ -23,6 +23,11 @@ server s3 {
send "Not a PostgreSQL response"
} -start
+server s4 {
+ recv 23
+ sendhex "520017000A534352414D2D5348412D323536"
+} -start
+
syslog S1 -level notice {
recv
expect ~ "[^:\\[ ]\\[${h1_pid}\\]: Health check for server be1/srv
succeeded, reason: Layer7 check passed.+info: \"PostgreSQL server is
ok\".+check duration: [[:digit:]]+ms, status: 1/1 UP."
@@ -38,6 +43,10 @@ syslog S3 -level notice {
expect ~ "[^:\\[ ]\\[${h1_pid}\\]: Health check for server be3/srv failed,
reason: Layer7 wrong status.+info: \"PostgreSQL unknown error\".+check
duration: [[:digit:]]+ms, status: 0/1 DOWN."
} -start
+syslog S4 -level notice {
+recv
+expect ~ "[^:\\[ ]\\[${h1_pid}\\]: Health check for server be4/srv
succeeded, reason: Layer7 check passed.+info: \"PostgreSQL server is
ok\".+check duration: [[:digit:]]+ms, status: 1/1 UP."
+} -start
haproxy h1 -conf {
defaults
@@ -64,6 +73,12 @@ haproxy h1 -conf {
option pgsql-check user postgres
server srv ${s3_addr}:${s3_port} check inter 1s rise 1 fall 1
+backend be4
+log ${S4_addr}:${S4_port} daemon
+option log-health-checks
+option pgsql-check user postgres
+server srv ${s4_addr}:${s4_port} check inter 1s rise 1 fall 1
+
listen pgsql1
bind "fd@${pgsql}"
tcp-request inspect-delay 100ms
@@ -75,3 +90,4 @@ haproxy h1 -conf {
syslog S1 -wait
syslog S2 -wait
syslog S3 -wait
+syslog S4 -wait
diff --git a/src/tcpcheck.c b/src/tcpcheck.c
index 5ef1c69cb..366a8d09c 100644
--- a/src/tcpcheck.c
+++ b/src/tcpcheck.c
@@ -4517,7 +4517,7 @@ int proxy_parse_pgsql_check_opt(char **args, int cur_arg,
struct proxy *curpx, c
chk->index = 2;
LIST_APPEND(>rules, >list);
- chk = parse_tcpcheck_expect((char *[]){"tcp-check", "expect",
"rbinary", "^5200(08|0A|0C)00(00|02|03|04|05|06)",
+ chk = parse_tcpcheck_expect((char *[]){"tcp-check", "expect",
"rbinary", "^5200[A-Z0-9]{2}00(00|02|03|04|05|06|07|09|0A)",
"min-recv", "9",
"error-status", "L7STS",
"on-success", "PostgreSQL server
is ok",
--
2.34.1
Re: [PATCH] BUG/MEDIUM: http-ana: fix crash or wrong header deletion by http-restrict-req-hdr-names
On Wed, Aug 17, 2022 at 01:05:55PM +, Mateusz Malek wrote: > On 17.08.2022 14:59, Mateusz Malek wrote: > > Sure - here you go: > > Sorry, wrong file. Patch in previous email had a typo (double /req9 > call instead of /req9 and /req10) in VTest test case. Perfect, thank you, now applied! I've updated the issue to remember the need to backport as far as 2.2 as you mentioned. I've just backported it to 2.6 already for the upcoming release and will handle other versions soon. Thanks again! Willy
Re: [PATCH] BUG/MEDIUM: http-ana: fix crash or wrong header deletion by http-restrict-req-hdr-names
On 17.08.2022 14:59, Mateusz Małek wrote:
> Sure - here you go:
Sorry, wrong file. Patch in previous email had a typo (double /req9
call instead of /req9 and /req10) in VTest test case.
Corrected patch below:
>From ce282735335e25ea90a88155e18d4adcb2b67e21 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mateusz=20Ma=C5=82ek?=
Date: Wed, 17 Aug 2022 14:22:09 +0200
Subject: [PATCH] BUG/MEDIUM: http-ana: fix crash or wrong header deletion by
http-restrict-req-hdr-names
When using `option http-restrict-req-hdr-names delete`, HAproxy may
crash or delete wrong header after receiving request containing multiple
forbidden characters in single header name; exact behavior depends on
number of request headers, number of forbidden characters and position
of header containing them.
This patch fixes GitHub issue #1822.
Must be backported as far as 2.2 (buggy feature got included in 2.2.25,
2.4.18 and 2.5.8).
---
.../http-rules/restrict_req_hdr_names.vtc | 62 +++
src/http_ana.c| 18 +++---
2 files changed, 73 insertions(+), 7 deletions(-)
diff --git a/reg-tests/http-rules/restrict_req_hdr_names.vtc
b/reg-tests/http-rules/restrict_req_hdr_names.vtc
index 071b9bded..4b26e33c6 100644
--- a/reg-tests/http-rules/restrict_req_hdr_names.vtc
+++ b/reg-tests/http-rules/restrict_req_hdr_names.vtc
@@ -35,6 +35,32 @@ server s5 {
txresp
} -start
+server s6 {
+rxreq
+expect req.http.x_my_hdr_with_lots_of_underscores ==
+txresp
+} -start
+
+server s7 {
+rxreq
+expect req.http.x_my_hdr-1 ==
+expect req.http.x-my-hdr-2 == on
+txresp
+} -start
+
+server s8 {
+rxreq
+expect req.http.x-my_hdr-1 ==
+expect req.http.x-my_hdr-2 ==
+txresp
+} -start
+
+server s9 {
+rxreq
+expect req.http.x-my-hdr-with-trailing-underscore_ ==
+txresp
+} -start
+
haproxy h1 -conf {
defaults
mode http
@@ -50,6 +76,10 @@ haproxy h1 -conf {
use_backend be-fcgi1 if { path /req4 }
use_backend be-fcgi2 if { path /req5 }
use_backend be-fcgi3 if { path /req6 }
+use_backend be-http4 if { path /req7 }
+use_backend be-http5 if { path /req8 }
+use_backend be-http6 if { path /req9 }
+use_backend be-http7 if { path /req10 }
backend be-http1
server s1 ${s1_addr}:${s1_port}
@@ -72,6 +102,22 @@ haproxy h1 -conf {
backend be-fcgi3
option http-restrict-req-hdr-names reject
+backend be-http4
+option http-restrict-req-hdr-names delete
+server s6 ${s6_addr}:${s6_port}
+
+backend be-http5
+option http-restrict-req-hdr-names delete
+server s7 ${s7_addr}:${s7_port}
+
+backend be-http6
+option http-restrict-req-hdr-names delete
+server s8 ${s8_addr}:${s8_port}
+
+backend be-http7
+option http-restrict-req-hdr-names delete
+server s9 ${s9_addr}:${s9_port}
+
defaults
mode http
timeout connect "${HAPROXY_TEST_TIMEOUT-5s}"
@@ -114,6 +160,22 @@ client c1 -connect ${h1_fe1_sock} {
txreq -req GET -url /req6 -hdr "X-my_hdr: on"
rxresp
expect resp.status == 403
+
+txreq -req GET -url /req7 -hdr "X_my_hdr_with_lots_of_underscores: on"
+rxresp
+expect resp.status == 200
+
+txreq -req GET -url /req8 -hdr "X_my_hdr-1: on" -hdr "X-my-hdr-2: on"
+rxresp
+expect resp.status == 200
+
+txreq -req GET -url /req9 -hdr "X-my_hdr-1: on" -hdr "X-my_hdr-2: on"
+rxresp
+expect resp.status == 200
+
+txreq -req GET -url /req10 -hdr "X-my-hdr-with-trailing-underscore_: on"
+rxresp
+expect resp.status == 200
} -run
client c2 -connect ${h1_fe2_sock} {
diff --git a/src/http_ana.c b/src/http_ana.c
index 4b74dd60d..2b2cfdc56 100644
--- a/src/http_ana.c
+++ b/src/http_ana.c
@@ -2645,17 +2645,21 @@ static enum rule_result
http_req_restrict_header_names(struct stream *s, struct
if (type == HTX_BLK_HDR) {
struct ist n = htx_get_blk_name(htx, blk);
- int i;
+ int i, end = istlen(n);
- for (i = 0; i < istlen(n); i++) {
+ for (i = 0; i < end; i++) {
if (!isalnum((unsigned char)n.ptr[i]) &&
n.ptr[i] != '-') {
- /* Block the request or remove the
header */
- if (px->options2 &
PR_O2_RSTRICT_REQ_HDR_NAMES_BLK)
- goto block;
- blk = htx_remove_blk(htx, blk);
- continue;
+ break;
}
}
+
+ if (i < end) {
+ /* Disallowed
Re: [PATCH] BUG/MEDIUM: http-ana: fix crash or wrong header deletion by http-restrict-req-hdr-names
On 17.08.2022 05:06, Willy Tarreau - w at 1wt.eu wrote:
> However here the goto at the beginning potentially maintains a
> problem: I'm wondering how we can be certain that htx_remove_blk()
> doesn't return a NULL if we remove the last block, in which case
> going back to the beginning of the loop without testing it can
> be a problem.
I guess that at least HTX_BLK_EOH should always follow headers, but
you're right it would be better not to take risk of unexpected NULL.
> Yep I agree with this. I was thinking that an even better option
> would be to remove the branches from the inner loop and use it only
> to spot unusual chars, something like: (...)
I agree with your suggestion, I've updated my patch.
> But if you can provide an updated patch for the loop, I'm fine with
> merging your patch as-is.
Sure - here you go:
>From d1129fd89e96d3d5d4abd06aff2289776883915d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mateusz=20Ma=C5=82ek?=
Date: Wed, 17 Aug 2022 14:22:09 +0200
Subject: [PATCH] BUG/MEDIUM: http-ana: fix crash or wrong header deletion by
http-restrict-req-hdr-names
When using `option http-restrict-req-hdr-names delete`, HAproxy may
crash or delete wrong header after receiving request containing multiple
forbidden characters in single header name; exact behavior depends on
number of request headers, number of forbidden characters and position
of header containing them.
This patch fixes GitHub issue #1822.
Must be backported as far as 2.2 (buggy feature got included in 2.2.25,
2.4.18 and 2.5.8).
---
.../http-rules/restrict_req_hdr_names.vtc | 62 +++
src/http_ana.c| 18 +++---
2 files changed, 73 insertions(+), 7 deletions(-)
diff --git a/reg-tests/http-rules/restrict_req_hdr_names.vtc
b/reg-tests/http-rules/restrict_req_hdr_names.vtc
index 071b9bded..713574f1c 100644
--- a/reg-tests/http-rules/restrict_req_hdr_names.vtc
+++ b/reg-tests/http-rules/restrict_req_hdr_names.vtc
@@ -35,6 +35,32 @@ server s5 {
txresp
} -start
+server s6 {
+rxreq
+expect req.http.x_my_hdr_with_lots_of_underscores ==
+txresp
+} -start
+
+server s7 {
+rxreq
+expect req.http.x_my_hdr-1 ==
+expect req.http.x-my-hdr-2 == on
+txresp
+} -start
+
+server s8 {
+rxreq
+expect req.http.x-my_hdr-1 ==
+expect req.http.x-my_hdr-2 ==
+txresp
+} -start
+
+server s9 {
+rxreq
+expect req.http.x-my-hdr-with-trailing-underscore_ ==
+txresp
+} -start
+
haproxy h1 -conf {
defaults
mode http
@@ -50,6 +76,10 @@ haproxy h1 -conf {
use_backend be-fcgi1 if { path /req4 }
use_backend be-fcgi2 if { path /req5 }
use_backend be-fcgi3 if { path /req6 }
+use_backend be-http4 if { path /req7 }
+use_backend be-http5 if { path /req8 }
+use_backend be-http6 if { path /req9 }
+use_backend be-http7 if { path /req10 }
backend be-http1
server s1 ${s1_addr}:${s1_port}
@@ -72,6 +102,22 @@ haproxy h1 -conf {
backend be-fcgi3
option http-restrict-req-hdr-names reject
+backend be-http4
+option http-restrict-req-hdr-names delete
+server s6 ${s6_addr}:${s6_port}
+
+backend be-http5
+option http-restrict-req-hdr-names delete
+server s7 ${s7_addr}:${s7_port}
+
+backend be-http6
+option http-restrict-req-hdr-names delete
+server s8 ${s8_addr}:${s8_port}
+
+backend be-http7
+option http-restrict-req-hdr-names delete
+server s9 ${s9_addr}:${s9_port}
+
defaults
mode http
timeout connect "${HAPROXY_TEST_TIMEOUT-5s}"
@@ -114,6 +160,22 @@ client c1 -connect ${h1_fe1_sock} {
txreq -req GET -url /req6 -hdr "X-my_hdr: on"
rxresp
expect resp.status == 403
+
+txreq -req GET -url /req7 -hdr "X_my_hdr_with_lots_of_underscores: on"
+rxresp
+expect resp.status == 200
+
+txreq -req GET -url /req8 -hdr "X_my_hdr-1: on" -hdr "X-my-hdr-2: on"
+rxresp
+expect resp.status == 200
+
+txreq -req GET -url /req9 -hdr "X-my_hdr-1: on" -hdr "X-my_hdr-2: on"
+rxresp
+expect resp.status == 200
+
+txreq -req GET -url /req9 -hdr "X-my-hdr-with-trailing-underscore_: on"
+rxresp
+expect resp.status == 200
} -run
client c2 -connect ${h1_fe2_sock} {
diff --git a/src/http_ana.c b/src/http_ana.c
index 4b74dd60d..2b2cfdc56 100644
--- a/src/http_ana.c
+++ b/src/http_ana.c
@@ -2645,17 +2645,21 @@ static enum rule_result
http_req_restrict_header_names(struct stream *s, struct
if (type == HTX_BLK_HDR) {
struct ist n = htx_get_blk_name(htx, blk);
- int i;
+ int i, end = istlen(n);
- for (i = 0; i < istlen(n); i++) {
+ for (i = 0; i < end; i++) {
Re: [RFC] [PATCH] BUG: http-ana: fix crash or wrong header deletion by http-restrict-req-hdr-names
Hi Mateusz,
On Tue, Aug 16, 2022 at 11:58:02PM +, hapr...@sl.damisa.net wrote:
> Hi,
>
> as suggested by Willy on GitHub, I'm submitting my patch for
> https://github.com/haproxy/haproxy/issues/1822.
Thank you!
> This is my first contribution, so I'm tagging it as RFC for now ;)
>
> I'm not entirely happy with using goto (suggested by Tim) to avoid
> hitting htx_get_next_blk call at the end of the loop, but I'm not
> familiar with HAproxy coding standards.
We're generally fine with gotos when they're the most efficient
solution (anyway, 1/5 of instructions executed by a CPU are jumps,
so it's pointless to hate gotos, it's just that using too many of
them will make the code less readable).
However here the goto at the beginning potentially maintains a
problem: I'm wondering how we can be certain that htx_remove_blk()
doesn't return a NULL if we remove the last block, in which case
going back to the beginning of the loop without testing it can
be a problem.
> I think it would be nicer to:
>
> 1. Introduce flag variable preserve_blk
> 2. Reset it to 1 at the beginning of the while (blk) loop
> 3. Replace htx_remove_blk + continue with preserve_blk = 0 + break
> 4. At the end of the loop, call htx_get_next_blk if preserve_blk is set
>or call htx_remove_blk otherwise
Yep I agree with this. I was thinking that an even better option
would be to remove the branches from the inner loop and use it only
to spot unusual chars, something like:
end = istlen(n);
for (i = 0; i < end; i++) {
if (!isalnum() ...)
break;
}
if (i < end) {
/* unwanted char found */
if (px->options...)
goto block;
blk = htx_remove_blk();
continue;
}
The for() loop could even be replaced with strcspn(). I just suspect
that for large sets it's slower if it needs to rebuild an internal map,
so we'd probably rather keep the for() loop as-is.
> I have not included severity of the patch, because on GitHub issue is
> still marked as `status: needs-triage`. I think MEDIUM would be
> appropriate.
Yes I think so as well.
> By the way, while writing VTest to cover this bug, I spotted something
> "suspicious" about reg tests for FCGI backends - my-fcgi-app FCGI app is
> defined, but it is not used anywhere? be-fcgi* backends look exactly
> like be-http* to me.
Indeed, I don't know either. I'm wondering if it's not needed just to
enable something by just being present, but I don't see what (and no
comment in the file indicates this). Maybe it's a leftover of an
initial attempt at testing fcgi, I don't know. Let's keep it for now,
we'll check with Christopher when he's back.
But if you can provide an updated patch for the loop, I'm fine with
merging your patch as-is.
Thanks!
Willy
[RFC] [PATCH] BUG: http-ana: fix crash or wrong header deletion by http-restrict-req-hdr-names
Hi,
as suggested by Willy on GitHub, I'm submitting my patch for
https://github.com/haproxy/haproxy/issues/1822.
This is my first contribution, so I'm tagging it as RFC for now ;)
I'm not entirely happy with using goto (suggested by Tim) to avoid
hitting htx_get_next_blk call at the end of the loop, but I'm not
familiar with HAproxy coding standards. I think it would be nicer to:
1. Introduce flag variable preserve_blk
2. Reset it to 1 at the beginning of the while (blk) loop
3. Replace htx_remove_blk + continue with preserve_blk = 0 + break
4. At the end of the loop, call htx_get_next_blk if preserve_blk is set
or call htx_remove_blk otherwise
I have not included severity of the patch, because on GitHub issue is
still marked as `status: needs-triage`. I think MEDIUM would be
appropriate.
By the way, while writing VTest to cover this bug, I spotted something
"suspicious" about reg tests for FCGI backends - my-fcgi-app FCGI app is
defined, but it is not used anywhere? be-fcgi* backends look exactly
like be-http* to me.
Best regards
Mateusz Małek
>From 17dcd8147fb7f48b3fcce5a7a51ff921f4f69848 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mateusz=20Ma=C5=82ek?=
Date: Wed, 17 Aug 2022 00:57:10 +0200
Subject: [PATCH] BUG: http-ana: fix crash or wrong header deletion by
http-restrict-req-hdr-names
When using `option http-restrict-req-hdr-names delete`, HAproxy may
crash or delete wrong header after receiving request containing multiple
forbidden characters in single header name; exact behavior depends on
number of request headers, number of forbidden characters and position
of header containing them.
This patch fixes GitHub issue #1822.
Must be backported as far as 2.2 (buggy feature got included in 2.2.25,
2.4.18 and 2.5.8).
---
.../http-rules/restrict_req_hdr_names.vtc | 47 +++
src/http_ana.c| 7 ++-
2 files changed, 53 insertions(+), 1 deletion(-)
diff --git a/reg-tests/http-rules/restrict_req_hdr_names.vtc
b/reg-tests/http-rules/restrict_req_hdr_names.vtc
index 071b9bded..eed90eab2 100644
--- a/reg-tests/http-rules/restrict_req_hdr_names.vtc
+++ b/reg-tests/http-rules/restrict_req_hdr_names.vtc
@@ -35,6 +35,26 @@ server s5 {
txresp
} -start
+server s6 {
+rxreq
+expect req.http.x_my_hdr_with_lots_of_underscores ==
+txresp
+} -start
+
+server s7 {
+rxreq
+expect req.http.x_my_hdr-1 ==
+expect req.http.x-my-hdr-2 == on
+txresp
+} -start
+
+server s8 {
+rxreq
+expect req.http.x-my_hdr-1 ==
+expect req.http.x-my_hdr-2 ==
+txresp
+} -start
+
haproxy h1 -conf {
defaults
mode http
@@ -50,6 +70,9 @@ haproxy h1 -conf {
use_backend be-fcgi1 if { path /req4 }
use_backend be-fcgi2 if { path /req5 }
use_backend be-fcgi3 if { path /req6 }
+use_backend be-http4 if { path /req7 }
+use_backend be-http5 if { path /req8 }
+use_backend be-http6 if { path /req9 }
backend be-http1
server s1 ${s1_addr}:${s1_port}
@@ -72,6 +95,18 @@ haproxy h1 -conf {
backend be-fcgi3
option http-restrict-req-hdr-names reject
+backend be-http4
+option http-restrict-req-hdr-names delete
+server s6 ${s6_addr}:${s6_port}
+
+backend be-http5
+option http-restrict-req-hdr-names delete
+server s7 ${s7_addr}:${s7_port}
+
+backend be-http6
+option http-restrict-req-hdr-names delete
+server s8 ${s8_addr}:${s8_port}
+
defaults
mode http
timeout connect "${HAPROXY_TEST_TIMEOUT-5s}"
@@ -114,6 +149,18 @@ client c1 -connect ${h1_fe1_sock} {
txreq -req GET -url /req6 -hdr "X-my_hdr: on"
rxresp
expect resp.status == 403
+
+txreq -req GET -url /req7 -hdr "X_my_hdr_with_lots_of_underscores: on"
+rxresp
+expect resp.status == 200
+
+txreq -req GET -url /req8 -hdr "X_my_hdr-1: on" -hdr "X-my-hdr-2: on"
+rxresp
+expect resp.status == 200
+
+txreq -req GET -url /req9 -hdr "X-my_hdr-1: on" -hdr "X-my_hdr-2: on"
+rxresp
+expect resp.status == 200
} -run
client c2 -connect ${h1_fe2_sock} {
diff --git a/src/http_ana.c b/src/http_ana.c
index 4b74dd60d..a2929cef5 100644
--- a/src/http_ana.c
+++ b/src/http_ana.c
@@ -2641,6 +2641,7 @@ static enum rule_result
http_req_restrict_header_names(struct stream *s, struct
blk = htx_get_first_blk(htx);
while (blk) {
+ next_iteration:
enum htx_blk_type type = htx_get_blk_type(blk);
if (type == HTX_BLK_HDR) {
@@ -2653,7 +2654,11 @@ static enum rule_result
http_req_restrict_header_names(struct stream *s, struct
if (px->options2 &
PR_O2_RSTRICT_REQ_HDR_NAMES_BLK)
goto block;
blk = htx_remove_blk(htx, blk);
-
Re: [PATCH] BUG/MEDIUM: sample: Fix adjusting size in word converter
On Wed, May 25, 2022 at 10:58:51PM -0600, astrotha...@gmail.com wrote: > From: Thayne McCombs > > Adjust the size of the sample buffer before we change the "area" > pointer. Otherwise, we end up not changing the size, because the area > pointer is already the same as "start" before we compute the difference > between the two. > > This is similar to the change in b28430591d18f7fda5bac2e0ea590b3a34f04601 > but for the word converter instead of field. Good catch, now merged, thank you Thayne! Willy
[PATCH] BUG/MEDIUM: sample: Fix adjusting size in word converter
From: Thayne McCombs Adjust the size of the sample buffer before we change the "area" pointer. Otherwise, we end up not changing the size, because the area pointer is already the same as "start" before we compute the difference between the two. This is similar to the change in b28430591d18f7fda5bac2e0ea590b3a34f04601 but for the word converter instead of field. --- src/sample.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/sample.c b/src/sample.c index 8a0a66b8c..50ae76b6e 100644 --- a/src/sample.c +++ b/src/sample.c @@ -2596,13 +2596,14 @@ static int sample_conv_word(const struct arg *arg_p, struct sample *smp, void *p if (!smp->data.u.str.data) return 1; - smp->data.u.str.area = start; /* Compute remaining size if needed Note: smp->data.u.str.size cannot be set to 0 */ if (smp->data.u.str.size) smp->data.u.str.size -= start - smp->data.u.str.area; + smp->data.u.str.area = start; + return 1; } -- 2.36.1
Re: [PATCH 1/2] BUG/MEDIUM: tools: Fix `inet_ntop` usage in sa2str
Thanks for catching that.
Re: [PATCH 1/2] BUG/MEDIUM: tools: Fix `inet_ntop` usage in sa2str
On Sun, May 22, 2022 at 01:06:27PM +0200, Tim Duesterhus wrote: > The given size must be the size of the destination buffer, not the size of the > (binary) address representation. > > This fixes GitHub issue #1599. > > The bug was introduced in 92149f9a82a9b55c598f1cc815bc330c555f3561 which is in > 2.4+. The fix must be backported there. Ah good catch, merged, thanks Tim! Willy
[PATCH 1/2] BUG/MEDIUM: tools: Fix `inet_ntop` usage in sa2str
The given size must be the size of the destination buffer, not the size of the (binary) address representation. This fixes GitHub issue #1599. The bug was introduced in 92149f9a82a9b55c598f1cc815bc330c555f3561 which is in 2.4+. The fix must be backported there. --- src/tools.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools.c b/src/tools.c index 9e629e5bd..b9a1121c6 100644 --- a/src/tools.c +++ b/src/tools.c @@ -1374,7 +1374,7 @@ char * sa2str(const struct sockaddr_storage *addr, int port, int map_ports) default: return NULL; } - inet_ntop(addr->ss_family, ptr, buffer, get_addr_len(addr)); + inet_ntop(addr->ss_family, ptr, buffer, sizeof(buffer)); if (map_ports) return memprintf(, "%s:%+d", buffer, port); else -- 2.36.1
Re: [PATCH] BUG/MEDIUM: lua: fix argument handling in data removal functions
Le 5/10/22 à 19:47, Boyang Li a écrit :
Lua API Channel.remove() and HTTPMessage.remove() expects 1 to 3
arguments (counting the manipulated object), with offset and length
being the 2nd and 3rd argument, respectively.
hlua_{channel,http_msg}_del_data() incorrectly gets the 3rd argument as
offset, and 4th (nonexistent) as length. hlua_http_msg_del_data() also
improperly checks arguments. This patch fixes argument handling in both.
Must be backported to 2.5.
Thanks ! Both patches were merged.
--
Christopher Faulet
[PATCH] BUG/MEDIUM: lua: fix argument handling in data removal functions
Lua API Channel.remove() and HTTPMessage.remove() expects 1 to 3
arguments (counting the manipulated object), with offset and length
being the 2nd and 3rd argument, respectively.
hlua_{channel,http_msg}_del_data() incorrectly gets the 3rd argument as
offset, and 4th (nonexistent) as length. hlua_http_msg_del_data() also
improperly checks arguments. This patch fixes argument handling in both.
Must be backported to 2.5.
---
src/hlua.c | 19 +--
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/src/hlua.c b/src/hlua.c
index 05f3a9053..2327553c8 100644
--- a/src/hlua.c
+++ b/src/hlua.c
@@ -3714,8 +3714,8 @@ __LJMP static int hlua_channel_del_data(lua_State *L)
WILL_LJMP(lua_error(L));
offset = output;
- if (lua_gettop(L) > 2) {
- offset = MAY_LJMP(luaL_checkinteger(L, 3));
+ if (lua_gettop(L) > 1) {
+ offset = MAY_LJMP(luaL_checkinteger(L, 2));
if (offset < 0)
offset = MAX(0, (int)input + offset);
offset += output;
@@ -3726,8 +3726,8 @@ __LJMP static int hlua_channel_del_data(lua_State *L)
}
len = output + input - offset;
- if (lua_gettop(L) == 4) {
- len = MAY_LJMP(luaL_checkinteger(L, 4));
+ if (lua_gettop(L) == 3) {
+ len = MAY_LJMP(luaL_checkinteger(L, 3));
if (!len)
goto end;
if (len == -1)
@@ -6704,8 +6704,7 @@ __LJMP static int hlua_http_msg_del_data(lua_State *L)
int offset, len;
if (lua_gettop(L) < 1 || lua_gettop(L) > 3)
- WILL_LJMP(luaL_error(L, "'insert' expects at most 2
arguments"));
- MAY_LJMP(check_args(L, 2, "insert"));
+ WILL_LJMP(luaL_error(L, "'remove' expects at most 2
arguments"));
msg = MAY_LJMP(hlua_checkhttpmsg(L, 1));
if (msg->msg_state < HTTP_MSG_DATA)
@@ -6716,8 +6715,8 @@ __LJMP static int hlua_http_msg_del_data(lua_State *L)
WILL_LJMP(lua_error(L));
offset = input + output;
- if (lua_gettop(L) > 2) {
- offset = MAY_LJMP(luaL_checkinteger(L, 3));
+ if (lua_gettop(L) > 1) {
+ offset = MAY_LJMP(luaL_checkinteger(L, 2));
if (offset < 0)
offset = MAX(0, (int)input + offset);
offset += output;
@@ -6728,8 +6727,8 @@ __LJMP static int hlua_http_msg_del_data(lua_State *L)
}
len = output + input - offset;
- if (lua_gettop(L) == 4) {
- len = MAY_LJMP(luaL_checkinteger(L, 4));
+ if (lua_gettop(L) == 3) {
+ len = MAY_LJMP(luaL_checkinteger(L, 3));
if (!len)
goto end;
if (len == -1)
--
2.35.1
Re: [PATCH] BUG/MINOR: Fix memory leak in resolvers_deinit()
On Tue, Apr 26, 2022 at 11:28:47PM +0200, Tim Duesterhus wrote: > A config like the following: > > global > stats socket /run/haproxy/admin.sock mode 660 level admin expose-fd > listeners > > resolvers unbound > nameserver unbound 127.0.0.1:53 > > will report the following leak when running a configuration check: > > ==241882== 6,991 (6,952 direct, 39 indirect) bytes in 1 blocks are > definitely lost in loss record 8 of 13 > ==241882==at 0x483DD99: calloc (in > /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) > ==241882==by 0x25938D: cfg_parse_resolvers (resolvers.c:3193) > ==241882==by 0x26A1E8: readcfgfile (cfgparse.c:2171) > ==241882==by 0x156D72: init (haproxy.c:2016) > ==241882==by 0x156D72: main (haproxy.c:3037) > > because the `.px` member of `struct resolvers` is not freed. > > The offending allocation was introduced in > c943799c865c04281454a7a54fd6c45c2b4d7e09 which is a reorganization that > happened during development of 2.4.x. This fix can likely be backported > without > issue to 2.4+ and is likely not needed for earlier versions as the leak > happens > during deinit only. Looks good, now merged, thanks Tim! Willy
[PATCH] BUG/MINOR: Fix memory leak in resolvers_deinit()
A config like the following: global stats socket /run/haproxy/admin.sock mode 660 level admin expose-fd listeners resolvers unbound nameserver unbound 127.0.0.1:53 will report the following leak when running a configuration check: ==241882== 6,991 (6,952 direct, 39 indirect) bytes in 1 blocks are definitely lost in loss record 8 of 13 ==241882==at 0x483DD99: calloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==241882==by 0x25938D: cfg_parse_resolvers (resolvers.c:3193) ==241882==by 0x26A1E8: readcfgfile (cfgparse.c:2171) ==241882==by 0x156D72: init (haproxy.c:2016) ==241882==by 0x156D72: main (haproxy.c:3037) because the `.px` member of `struct resolvers` is not freed. The offending allocation was introduced in c943799c865c04281454a7a54fd6c45c2b4d7e09 which is a reorganization that happened during development of 2.4.x. This fix can likely be backported without issue to 2.4+ and is likely not needed for earlier versions as the leak happens during deinit only. --- src/resolvers.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/resolvers.c b/src/resolvers.c index 0b7faf93d..3179073b5 100644 --- a/src/resolvers.c +++ b/src/resolvers.c @@ -2448,6 +2448,7 @@ static void resolvers_deinit(void) abort_resolution(res); } + free_proxy(resolvers->px); free(resolvers->id); free((char *)resolvers->conf.file); task_destroy(resolvers->t); -- 2.36.0
Re: Possible bug in stats page dark mode
On Sun, Apr 10, 2022 at 12:25:54PM -0600, Shawn Heisey wrote: > On the dark mode stats page served by version 2.6-dev5, the frontend or > backend description is grey text on a white background. It's very hard to > read. Ah indeed, a "th.desc" entry was missing to replace the background, now fixed and marked for backporting to 2.5. Thanks, Willy
Possible bug in stats page dark mode
On the dark mode stats page served by version 2.6-dev5, the frontend or backend description is grey text on a white background. It's very hard to read. This problem can't be seen on stats.haproxy.org, possibly because the frontend and backend configs do not have any descriptions. Thanks, Shawn
