Re: [PATCH 0/6] Lua variable handling
Le 19/05/2020 à 13:49, Tim Duesterhus a écrit : Christopher, Thierry, this patch series implements the solution discussed in GitHub issue #624: https://github.com/haproxy/haproxy/issues/624#issuecomment-630590470 I felt pretty dirty having to copy and paste my changes to all three versions of `set_var` and `unset_var`. This should be refactored one day to use a single generic implementation. All three versions only differ slightly with regard to where they take their `appctx` from. I did not refactor that, because I don't really feel confident doing that large changes within Lua, yet :-) I hope I didn't miss anything to adjust. I've added a new reg-test that verifies the intended behavior and made sure that all the Lua reg-tests still pass. Best regards Tim Düsterhus (6): REGTESTS: Add missing OPENSSL to REQUIRE_OPTIONS for lua/txn_get_priv MINOR: lua: Use vars_unset_by_name_ifexist() CLEANUP: vars: Remove void vars_unset_by_name(const char*, size_t, struct sample*) MINOR: vars: Make vars_(un|)set_by_name(_ifexist|) return a success value MINOR: lua: Make `set_var()` and `unset_var()` return success MEDIUM: lua: Add `ifexist` parameter to `set_var` doc/lua-api/index.rst | 18 ++-- include/proto/vars.h | 7 ++--- reg-tests/lua/set_var.lua | 25 + reg-tests/lua/set_var.vtc | 51 ++ reg-tests/lua/txn_get_priv.vtc | 2 +- src/hlua.c | 48 ++-- src/vars.c | 40 +- 7 files changed, 141 insertions(+), 50 deletions(-) create mode 100644 reg-tests/lua/set_var.lua create mode 100644 reg-tests/lua/set_var.vtc All the patchset is now merged. Thanks ! -- Christopher Faulet
Re: [PATCH 0/6] Lua variable handling
On Tue, May 19, 2020 at 05:39:25PM +0200, Christopher Faulet wrote: > Le 19/05/2020 à 16:06, Tim Düsterhus a écrit : > > > > > > Now back on topic. Instead of adding more parameters to set_var(), I'd > > > prefer a warning instead. > > > > > > If someone is using set_var() from Lua, and that variable is never used > > > or set in the rest of the config, we would know that. Additionally, one > > > can set "zero-warning" option to prevent abuse by Lua scripts or to > > > prevent bugs. > > > > This would break my use case for haproxy-auth-request. The plan is that > > the Lua script unconditionally sets are variable for all response > > headers and that HAProxy with my patches applied drops the variables > > that are never going to be read. I specifically want to avoid that the > > administrator needs to configure the headers to expose. Details are in: > > https://github.com/TimWolla/haproxy-auth-request/pull/13 > > > > I agree with Tim. It is pretty handy to let the set_var silently fail. This > way, a script can expose many variables and let the user choose those he > needs. The same is already done in the SPOE. > Then, we've chosen to be backward compatible and let any variable be > dynamically registered from LUA. There is a warning in the LUA > documentation. It is now the developers responsibility to be careful. I > doubt it will be a source of problems. But if so, it could be changed. Well, it's exactly the same in shell scripts, you set and read variables and if you misspell them it's the developer's problem. And this is very convenient and flexible. Willy
Re: [PATCH 0/6] Lua variable handling
Le 19/05/2020 à 16:06, Tim Düsterhus a écrit : Now back on topic. Instead of adding more parameters to set_var(), I'd prefer a warning instead. If someone is using set_var() from Lua, and that variable is never used or set in the rest of the config, we would know that. Additionally, one can set "zero-warning" option to prevent abuse by Lua scripts or to prevent bugs. This would break my use case for haproxy-auth-request. The plan is that the Lua script unconditionally sets are variable for all response headers and that HAProxy with my patches applied drops the variables that are never going to be read. I specifically want to avoid that the administrator needs to configure the headers to expose. Details are in: https://github.com/TimWolla/haproxy-auth-request/pull/13 I agree with Tim. It is pretty handy to let the set_var silently fail. This way, a script can expose many variables and let the user choose those he needs. The same is already done in the SPOE. Then, we've chosen to be backward compatible and let any variable be dynamically registered from LUA. There is a warning in the LUA documentation. It is now the developers responsibility to be careful. I doubt it will be a source of problems. But if so, it could be changed. -- Christopher Faulet
Re: [PATCH 0/6] Lua variable handling
Le 19/05/2020 à 13:49, Tim Duesterhus a écrit : Christopher, Thierry, this patch series implements the solution discussed in GitHub issue #624: https://github.com/haproxy/haproxy/issues/624#issuecomment-630590470 I felt pretty dirty having to copy and paste my changes to all three versions of `set_var` and `unset_var`. This should be refactored one day to use a single generic implementation. All three versions only differ slightly with regard to where they take their `appctx` from. I did not refactor that, because I don't really feel confident doing that large changes within Lua, yet :-) I hope I didn't miss anything to adjust. I've added a new reg-test that verifies the intended behavior and made sure that all the Lua reg-tests still pass. Best regards Thanks Tim, I'll review your patches but at first glance, it seems to be good. -- Christopher Faulet
Re: [PATCH 0/6] Lua variable handling
Adis, Am 19.05.20 um 15:57 schrieb Adis Nezirovic: > However, nothing in the code checks that variable name is formed > correctly (with correct prefix, such as "txn."), or that the rest of the > variable name is correct. > > Lua code using set_var() with wrong name silently fails, the variable is > not set, and nothing is logged > . > > > With your proposal set_var() now can return result, and we could even > log warning for "variable name syntax error". > Have a look at the reg-test in patch 5/6. I specifically added a test that checks the return value for incorrect variable names. > ---8<--- > > Now back on topic. Instead of adding more parameters to set_var(), I'd > prefer a warning instead. > > If someone is using set_var() from Lua, and that variable is never used > or set in the rest of the config, we would know that. Additionally, one > can set "zero-warning" option to prevent abuse by Lua scripts or to > prevent bugs. This would break my use case for haproxy-auth-request. The plan is that the Lua script unconditionally sets are variable for all response headers and that HAProxy with my patches applied drops the variables that are never going to be read. I specifically want to avoid that the administrator needs to configure the headers to expose. Details are in: https://github.com/TimWolla/haproxy-auth-request/pull/13 > For your use case, maybe you could use maps from Lua? They should be > backed by ebtree and be faster right? > To my understanding Maps are not scoped per request and thus would not be usable for haproxy-auth-request. Best regards Tim Düsterhus
Re: [PATCH 0/6] Lua variable handling
Tim, First, not a a direct comment on your patches, but before I forget, I'd like to point out that there is one more thing which need fixing in regard to Lua variables: TXN.set_var(TXN, var, value) where "var" is noted as: The var name according with the HAProxy syntax However, nothing in the code checks that variable name is formed correctly (with correct prefix, such as "txn."), or that the rest of the variable name is correct. Lua code using set_var() with wrong name silently fails, the variable is not set, and nothing is logged . With your proposal set_var() now can return result, and we could even log warning for "variable name syntax error". ---8<--- Now back on topic. Instead of adding more parameters to set_var(), I'd prefer a warning instead. If someone is using set_var() from Lua, and that variable is never used or set in the rest of the config, we would know that. Additionally, one can set "zero-warning" option to prevent abuse by Lua scripts or to prevent bugs. For your use case, maybe you could use maps from Lua? They should be backed by ebtree and be faster right? Best regards, -- Adis Nezirovic Software Engineer HAProxy Technologies - Powering your uptime! 375 Totten Pond Road, Suite 302 | Waltham, MA 02451, US +1 (844) 222-4340 | https://www.haproxy.com
[PATCH 0/6] Lua variable handling
Christopher, Thierry, this patch series implements the solution discussed in GitHub issue #624: https://github.com/haproxy/haproxy/issues/624#issuecomment-630590470 I felt pretty dirty having to copy and paste my changes to all three versions of `set_var` and `unset_var`. This should be refactored one day to use a single generic implementation. All three versions only differ slightly with regard to where they take their `appctx` from. I did not refactor that, because I don't really feel confident doing that large changes within Lua, yet :-) I hope I didn't miss anything to adjust. I've added a new reg-test that verifies the intended behavior and made sure that all the Lua reg-tests still pass. Best regards Tim Düsterhus (6): REGTESTS: Add missing OPENSSL to REQUIRE_OPTIONS for lua/txn_get_priv MINOR: lua: Use vars_unset_by_name_ifexist() CLEANUP: vars: Remove void vars_unset_by_name(const char*, size_t, struct sample*) MINOR: vars: Make vars_(un|)set_by_name(_ifexist|) return a success value MINOR: lua: Make `set_var()` and `unset_var()` return success MEDIUM: lua: Add `ifexist` parameter to `set_var` doc/lua-api/index.rst | 18 ++-- include/proto/vars.h | 7 ++--- reg-tests/lua/set_var.lua | 25 + reg-tests/lua/set_var.vtc | 51 ++ reg-tests/lua/txn_get_priv.vtc | 2 +- src/hlua.c | 48 ++-- src/vars.c | 40 +- 7 files changed, 141 insertions(+), 50 deletions(-) create mode 100644 reg-tests/lua/set_var.lua create mode 100644 reg-tests/lua/set_var.vtc -- 2.26.2
