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 knowing about this unexpected behavior either. 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. 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 Best regards Tim Düsterhus
