Re: Inconsistent reading of txn vars from Lua script
Hi guys,
On Wed, May 12, 2021 at 09:44:28AM +0200, Willy Tarreau wrote:
> On Wed, May 12, 2021 at 09:12:25AM +0200, Tim Düsterhus wrote:
> > Willy,
> >
> > On 5/12/21 7:47 AM, Willy Tarreau wrote:
> > > Interestingly, the code for variables was initially made for the config,
> > > so it doesn't seem to destroy variable names when they're released since
> > > that was pointless with the config. I think that code should be revisited
> > > in 2.5 to improve the situation (e.g. by marking that the variable was
> > > dynamically allocated maybe), but I don't know this part well so I'll
> > > probably stop before starting to suggest stupidities :-)
> > >
> >
> > There's also this related issue from back when I implemented this additional
> > parameter:
> >
> > https://github.com/haproxy/haproxy/issues/624
>
> Yes, and it's still unclear to me how this storage is currently arranged,
> (i.e. why only store names?) I should have a look for 2.5 probably.
OK now I got a better view of it and there is some misunderstanding in
the way the names are being used to detect if a variable exists. For
example, calling this before calling the Lua code will make it always
succeed:
http-request set-var(proc.code) int(12)
Note that the variable name here is "proc.code", not "txn.code".
What happens is that there are unified names which are independent of
the variables themselves. The principle of the names is that when
looking for a variable, we need to compare its name, and instead of
storing the name into each and every copy of a variable, there's only
a pointer to a unified location storing names that have been encountered
at least one in the process so that a single word is used in combination
with millions of variables if needed. For this, only the suffix of the
variable's name is stored, its scope is not since we already know it
when looking up a variable.
This means that the "ifexist" option shouldn't be seen as "if this
variable exists anywhere else", but as "if any variable known to the
process already caused the same suffix to be allocated".
What happens here is that while the set_var("txn.code", true) call takes
care of *not* allocating a new entry to store "code" in the names table,
get_var("txn.code") isn't as careful and will finally create it, and
notice that the variable doesn't yet exist, so it returns nil. On a
subsequent call, set_var() will find a matching suffix name and will
then store the variable, which get_var() will then find.
If we had had a get_var("sess.code"), it would also have unblocked the
situation.
In my opinion we have multiple problems here.
The first one is that if the intent of the "ifexist" was to avoid
allocating variables that are not known to the config, it doesn't work
well due to the fact that it doesn't consider the scope, so it should
be stricter and check that the variable exists with the same scope.
But how? I don't know for now.
Second, the fact that get_var() does automatically cause the creation
of that variable is by far the biggest problem, because in order to
verify if it has been filled, this will cause an allocation which will
later ensure it is always filled. So we must make it support an "ifexist"
option as well so that it is possible to perform an existence lookup
without allocating.
I suspect the set_var() modification was done for a config which uses
Lua to set a variable and where the variable was read from the config,
but that this other case where get_var() is called from Lua was
overlooked.
Last point, overall I think that the "ifexist" mechanism remains of
very limited use due to the automaticity of some of the allocations,
which were initially designed only for referencement from the config
parser. Originally, I remember that Thierry introduced a "declare var"
directive in the global section, which we found painful to use and
unnecessary due to the fact that during the parsing we already get an
exhaustive list of the variables names. But maybe for variables known
to Lua only, we should use an explicit declaration (probably from the
Lua code itself).
Thinking about it, this could correspond to just a single call to
set_var(name, "") in the init code to declare that a variable will be
used by Lua. In this case the only missing part will be taking into
consideration the scope (we could later improve that by prepending
the enum to the string in the storage for example).
So in the end I think that for 2.4 we should simply change the Lua's
get_var() so that it always uses the ifexist variant. It will at
least stop creating random names on the fly and will continue to
work with variables that have been already created by the config or
with set_var(). I don't see a single case where it makes sense to
have get_var() create a variable in your back and return NIL because
set_var() wasn't called so that next time set_var() works.
Looking more closely at vars_get_by_name(), it's only used by Lua's
various get_var() and by the CLI's "get var" that I rece
Re: Inconsistent reading of txn vars from Lua script
Willy,
On 5/13/21 11:40 AM, Willy Tarreau wrote:
Yes, and it's still unclear to me how this storage is currently arranged,
(i.e. why only store names?) I should have a look for 2.5 probably.
OK now I got a better view of it and there is some misunderstanding in
the way the names are being used to detect if a variable exists. For
example, calling this before calling the Lua code will make it always
succeed:
http-request set-var(proc.code) int(12)
Note that the variable name here is "proc.code", not "txn.code".
What happens is that there are unified names which are independent of
the variables themselves. The principle of the names is that when
looking for a variable, we need to compare its name, and instead of
storing the name into each and every copy of a variable, there's only
a pointer to a unified location storing names that have been encountered
at least one in the process so that a single word is used in combination
with millions of variables if needed. For this, only the suffix of the
variable's name is stored, its scope is not since we already know it
when looking up a variable.
This means that the "ifexist" option shouldn't be seen as "if this
variable exists anywhere else", but as "if any variable known to the
process already caused the same suffix to be allocated".
What happens here is that while the set_var("txn.code", true) call takes
care of *not* allocating a new entry to store "code" in the names table,
get_var("txn.code") isn't as careful and will finally create it, and
notice that the variable doesn't yet exist, so it returns nil. On a
subsequent call, set_var() will find a matching suffix name and will
then store the variable, which get_var() will then find.
If we had had a get_var("sess.code"), it would also have unblocked the
situation.
In my opinion we have multiple problems here.
The first one is that if the intent of the "ifexist" was to avoid
allocating variables that are not known to the config, it doesn't work
well due to the fact that it doesn't consider the scope, so it should
be stricter and check that the variable exists with the same scope.
But how? I don't know for now.
I introduced it to not allocate the *variable name*, because they were
never cleaned up. Not allocating the variable would be a nice side effect.
My use case is haproxy-auth-request which uses variables to communicate
back the auth request's response headers:
https://github.com/TimWolla/haproxy-auth-request/blob/e7b6385b3f1f34e0090968464f19369b2b8d117c/auth-request.lua#L106-L108
Second, the fact that get_var() does automatically cause the creation
of that variable is by far the biggest problem, because in order to
verify if it has been filled, this will cause an allocation which will
later ensure it is always filled. So we must make it support an "ifexist"
option as well so that it is possible to perform an existence lookup
without allocating.
I suspect the set_var() modification was done for a config which uses
Lua to set a variable and where the variable was read from the config,
but that this other case where get_var() is called from Lua was
overlooked.
Yes, this is correct. I'd using Lua to set the variable and the
variables are expected to be read from the config for further processing.
Last point, overall I think that the "ifexist" mechanism remains of
very limited use due to the automaticity of some of the allocations,
which were initially designed only for referencement from the config
parser. Originally, I remember that Thierry introduced a "declare var"
directive in the global section, which we found painful to use and
unnecessary due to the fact that during the parsing we already get an
exhaustive list of the variables names. But maybe for variables known
to Lua only, we should use an explicit declaration (probably from the
Lua code itself).
Thinking about it, this could correspond to just a single call to
set_var(name, "") in the init code to declare that a variable will be
used by Lua. In this case the only missing part will be taking into
consideration the scope (we could later improve that by prepending
the enum to the string in the storage for example).
So in the end I think that for 2.4 we should simply change the Lua's
get_var() so that it always uses the ifexist variant. It will at
least stop creating random names on the fly and will continue to
work with variables that have been already created by the config or
with set_var(). I don't see a single case where it makes sense to
have get_var() create a variable in your back and return NIL because
set_var() wasn't called so that next time set_var() works.
I agree. If a variable never was created in the first place then
obviously any read will result in nothing being found. The implicit
creation sounds like a bug, because it will result in inconsistent behavior.
Looking more closely at vars_get_by_name(), it's only used by Lua's
various get_var() and by the CLI's "get var" that I recently added
without
Re: Inconsistent reading of txn vars from Lua script
On Thu, May 13, 2021 at 12:24:02PM +0200, Tim Düsterhus wrote: > > So what I'm proposing is to simply change vars_get_by_name() to call > > register_name() with alloc=0 in order to fix this mess. We can then > > check during 2.5 how to refine this to also consider the scope with > > the variable's name. It's just this, and fixes Joao's test case to > > always return 403: > > > > diff --git a/src/vars.c b/src/vars.c > > index 996141f5d..15dcb3c3d 100644 > > --- a/src/vars.c > > +++ b/src/vars.c > > @@ -583,7 +583,7 @@ int vars_get_by_name(const char *name, size_t len, > > struct sample *smp) > > enum vars_scope scope; > > /* Resolve name and scope. */ > > - name = register_name(name, len, &scope, 1, NULL); > > + name = register_name(name, len, &scope, 0, NULL); > > if (!name) > > return 0; > > Tim, do you agree with this analysis ? > > > > Yes, that change makes sense to me. Great, thanks for the fast response, I'm going to do that and mark it as a bugfix so that after some observation we can consider backporting it. > If you'd see my full use case then I > recommend taking a look at haproxy-auth-request. It's super simple and even > comes with VTest tests: > > https://github.com/TimWolla/haproxy-auth-request#usage > https://github.com/TimWolla/haproxy-auth-request/blob/main/auth-request.lua#L50 > https://github.com/TimWolla/haproxy-auth-request/tree/main/test Thanks. I vaguely remembered it was something simple but I really can't remember every use case that end up as a patch :-) Cheers, Willy
HashiCorp
Good Day, I would like to know you are interested in HashiCorp Users across a range of industries and geographic regions. If yes we can move forward. Who we are We are a global database providing company . Hope we get positive reply from your team. Thanks Judy Jones Access List
[PATCH] DOC: config: Fix configuration example for mqtt
Hello, This patch fixes the example for mqtt_is_valid(), which was missing curly braces within the ACL. Thanks, -- Daniel 0001-DOC-config-Fix-configuration-example-for-mqtt.patch Description: Binary data
Re: [PATCH] DOC: config: Fix configuration example for mqtt
On Thu, May 13, 2021 at 11:13:29AM -0400, Daniel Corbett wrote: > This patch fixes the example for mqtt_is_valid(), which was missing > curly braces within the ACL. Applied, thanks Daniel! Willy
DNS service discovery and consistent hashing
At Vimeo we have a custom tool since 2015 that monitors the membership of clusters of servers, templates out a config with servers assigned to backends, and manages reloading haproxy. We're looking into replacing this with something a bit more off-the-shelf, and one of the options is HAProxy's own DNS service discovery support. We're also using URI-based load balancing with consistent hashing, and the stability of that mapping is important to us. Temporary disagreements while membership is changing are inevitable, but we want the portion of the hash space that a backend server sees to change as little as possible during its lifetime, and for multiple haproxies running the same config, against the same cluster, to converge on the same mapping. Our existing tool assigns a persistent ID to each server, which is mapped to an "id" option in the server line, which has worked quite well. >From what we've seen in testing so far, using "server-template" with DNS *doesn't* give us the behavior we want — the assignment of servers to slots seems inconsistent, maybe depending on some combination of the order of answers in the DNS packet or the order that new server appearances are observed by haproxy. Long story short: 1. Is my interpretation right? 2. Would you be open to a patch to change that? I'm thinking of something like setting puid from a hash of the SRV name or the A address, "open addressing" style, with who goes first in case of a collision determined by lexicographic order — but I'm quite open to guidance. Or should I just look somewhere other than the DNS service discovery? Thanks, Andrew Rodland (Please CC, I'm not on the list.)
