Re: [PATCH] variables cleanup/fixup
On Tue, Aug 31, 2021 at 06:37:45PM +0200, Willy Tarreau wrote: > > As such: Your patches LGTM, thanks. Please proceed :-) > > Will do, and reference the issue above and update the doc regarding ifexist, > just mentioning that it's now ignored for legacy compatibility. I'll finally wait for Christopher next week to confirm that we can get rid of the equivalent in SPOE. By default it protects itself against registration of random variable names and there's a force-set-var option to bypass that for trusted agents. In my opinion this can go since we do not have this problem anymore, but let's confirm before risking to break setups. Cheers, Willy
Re: [PATCH] variables cleanup/fixup
On Tue, Aug 31, 2021 at 04:41:16PM +0200, Tim Düsterhus wrote: > Willy, > > On 8/31/21 9:07 AM, Willy Tarreau wrote: > > I've finally implemented the replacement of the global variables table > > Okay, please refer to issue #624 in the commit: > https://github.com/haproxy/haproxy/issues/624. I believe it should be > resolved afterwards. Ah yes, thank you, I forgot about this one! > > with a hash instead. However it now obviously breaks the "ifexist" > > argument that you added to Lua's set_var() that was designed to work > > around the growing table problem. > > > > Given that the limitation used to be made on the existence of a similarly > > named variable anywhere in the process (and not of the variable in the > > current list), I conclude that it was only used to preserve precious > > resources and never to conditionally set a variable in one's context. > > As such we could simply remove this test and silently ignore the ifexist > > argument now. Do you agree with this ? I'd really like it if we could > > definitely get rid of this old mess! > > For the record this is my the repository I care about: > > https://github.com/TimWolla/haproxy-auth-request > > It includes tests based on VTest. I just ran the tests with patches applied: > > > $ vtest -Dhaproxy_version=2.5.0 -q -k -t 10 -C test/*.vtc > > #top TEST test/no_variable_leak.vtc FAILED (0.203) exit=2 > > 1 tests failed, 0 tests skipped, 19 tests passed > > Naturally the no_variable_leak.vtc test > (https://github.com/TimWolla/haproxy-auth-request/blob/main/test/no_variable_leak.vtc) > fails now, as it specifically tests that my detection of the ifexist > parameter works as expected. I can simply exclude HAProxy 2.5 from that test > and all is well. Yeah that's the same now in 2.5-dev with the dedicated test. > In my case I only care about not bloating HAProxy's memory usage infinitely, > in case the backend sends sends headers with randomly generated names (these > are exposed as req.auth_response_header.*). The fact that variables are > unavailable to Lua is a side-effect of this, not a feature. That's what I remembered as well, thanks for confirming! > It would certainly be preferable for the user if they could simply use the > variables from Lua, without needing to reference them in the config. ... and without eating all the memory :-) > As such: Your patches LGTM, thanks. Please proceed :-) Will do, and reference the issue above and update the doc regarding ifexist, just mentioning that it's now ignored for legacy compatibility. Thanks! Willy
Re: [PATCH] variables cleanup/fixup
Willy, On 8/31/21 9:07 AM, Willy Tarreau wrote: I've finally implemented the replacement of the global variables table Okay, please refer to issue #624 in the commit: https://github.com/haproxy/haproxy/issues/624. I believe it should be resolved afterwards. with a hash instead. However it now obviously breaks the "ifexist" argument that you added to Lua's set_var() that was designed to work around the growing table problem. Given that the limitation used to be made on the existence of a similarly named variable anywhere in the process (and not of the variable in the current list), I conclude that it was only used to preserve precious resources and never to conditionally set a variable in one's context. As such we could simply remove this test and silently ignore the ifexist argument now. Do you agree with this ? I'd really like it if we could definitely get rid of this old mess! For the record this is my the repository I care about: https://github.com/TimWolla/haproxy-auth-request It includes tests based on VTest. I just ran the tests with patches applied: $ vtest -Dhaproxy_version=2.5.0 -q -k -t 10 -C test/*.vtc #top TEST test/no_variable_leak.vtc FAILED (0.203) exit=2 1 tests failed, 0 tests skipped, 19 tests passed Naturally the no_variable_leak.vtc test (https://github.com/TimWolla/haproxy-auth-request/blob/main/test/no_variable_leak.vtc) fails now, as it specifically tests that my detection of the ifexist parameter works as expected. I can simply exclude HAProxy 2.5 from that test and all is well. In my case I only care about not bloating HAProxy's memory usage infinitely, in case the backend sends sends headers with randomly generated names (these are exposed as req.auth_response_header.*). The fact that variables are unavailable to Lua is a side-effect of this, not a feature. It would certainly be preferable for the user if they could simply use the variables from Lua, without needing to reference them in the config. As such: Your patches LGTM, thanks. Please proceed :-) For reference I'm appending the current patch series. Best regards Tim Düsterhus
[PATCH] variables cleanup/fixup
Hi Tim,
I've finally implemented the replacement of the global variables table
with a hash instead. However it now obviously breaks the "ifexist"
argument that you added to Lua's set_var() that was designed to work
around the growing table problem.
Given that the limitation used to be made on the existence of a similarly
named variable anywhere in the process (and not of the variable in the
current list), I conclude that it was only used to preserve precious
resources and never to conditionally set a variable in one's context.
As such we could simply remove this test and silently ignore the ifexist
argument now. Do you agree with this ? I'd really like it if we could
definitely get rid of this old mess!
For reference I'm appending the current patch series.
Thanks!
Willy
>From 6abb2e9fd745311c091029933a86fe363d09a7fb Mon Sep 17 00:00:00 2001
From: Willy Tarreau
Date: Tue, 31 Aug 2021 08:13:25 +0200
Subject: MINOR: vars: rename vars_init() to vars_init_head()
The vars_init() name is particularly confusing as it does not initialize
the variables code but the head of a list of variables passed in
arguments. And we'll soon need to have proper initialization code, so
let's rename it now.
---
include/haproxy/vars.h | 2 +-
src/haproxy.c | 2 +-
src/http_ana.c | 6 +++---
src/session.c | 2 +-
src/stream.c | 6 +++---
src/tcpcheck.c | 2 +-
src/vars.c | 4 ++--
7 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/include/haproxy/vars.h b/include/haproxy/vars.h
index f809c62d5..fedc8ca15 100644
--- a/include/haproxy/vars.h
+++ b/include/haproxy/vars.h
@@ -29,7 +29,7 @@
extern struct vars proc_vars;
-void vars_init(struct vars *vars, enum vars_scope scope);
+void vars_init_head(struct vars *vars, enum vars_scope scope);
void var_accounting_diff(struct vars *vars, struct session *sess, struct
stream *strm, int size);
unsigned int var_clear(struct var *var);
void vars_prune(struct vars *vars, struct session *sess, struct stream *strm);
diff --git a/src/haproxy.c b/src/haproxy.c
index db957256e..32a08441d 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -1529,7 +1529,7 @@ static void init(int argc, char **argv)
hlua_init();
/* Initialize process vars */
- vars_init(&proc_vars, SCOPE_PROC);
+ vars_init_head(&proc_vars, SCOPE_PROC);
global.tune.options |= GTUNE_USE_SELECT; /* select() is always
available */
#if defined(USE_POLL)
diff --git a/src/http_ana.c b/src/http_ana.c
index b36054088..3443f7ec4 100644
--- a/src/http_ana.c
+++ b/src/http_ana.c
@@ -2955,7 +2955,7 @@ int http_eval_after_res_rules(struct stream *s)
if (s->vars_reqres.scope != SCOPE_RES) {
if (!LIST_ISEMPTY(&s->vars_reqres.head))
vars_prune(&s->vars_reqres, s->sess, s);
- vars_init(&s->vars_reqres, SCOPE_RES);
+ vars_init_head(&s->vars_reqres, SCOPE_RES);
}
ret = http_res_get_intercept_rule(s->be, &s->be->http_after_res_rules,
s);
@@ -5083,8 +5083,8 @@ struct http_txn *http_create_txn(struct stream *s)
txn->auth.method = HTTP_AUTH_UNKNOWN;
- vars_init(&s->vars_txn,SCOPE_TXN);
- vars_init(&s->vars_reqres, SCOPE_REQ);
+ vars_init_head(&s->vars_txn,SCOPE_TXN);
+ vars_init_head(&s->vars_reqres, SCOPE_REQ);
return txn;
}
diff --git a/src/session.c b/src/session.c
index 92d03eaed..7ad5cbb3e 100644
--- a/src/session.c
+++ b/src/session.c
@@ -47,7 +47,7 @@ struct session *session_new(struct proxy *fe, struct listener
*li, enum obj_type
sess->accept_date = date; /* user-visible date for logging */
sess->tv_accept = now; /* corrected date for internal use */
memset(sess->stkctr, 0, sizeof(sess->stkctr));
- vars_init(&sess->vars, SCOPE_SESS);
+ vars_init_head(&sess->vars, SCOPE_SESS);
sess->task = NULL;
sess->t_handshake = -1; /* handshake not done yet */
sess->t_idle = -1;
diff --git a/src/stream.c b/src/stream.c
index 132ee3abd..27062ea4b 100644
--- a/src/stream.c
+++ b/src/stream.c
@@ -451,8 +451,8 @@ struct stream *stream_new(struct session *sess, enum
obj_type *origin, struct bu
/* Initialise all the variables contexts even if not used.
* This permits to prune these contexts without errors.
*/
- vars_init(&s->vars_txn,SCOPE_TXN);
- vars_init(&s->vars_reqres, SCOPE_REQ);
+ vars_init_head(&s->vars_txn,SCOPE_TXN);
+ vars_init_head(&s->vars_reqres, SCOPE_REQ);
/* this part should be common with other protocols */
if (si_reset(&s->si[0]) < 0)
@@ -2201,7 +2201,7 @@ struct task *process_stream(struct task *t, void
*context, unsigned int state)
if (s->vars_reqres.scope != SCOPE_RES) {
if (!LIST_ISEMPTY(&s->vars_reqres.head))
