MINOR: hlua: add option to preserve bool type from smp to lua

As discussed in GH #2814, there is an ambiguity in hlua implementation
that causes haproxy smp boolean type to be pushed as an integer on the
Lua stack. On the other hand, when doing Lua to haproxy smp conversion,
the boolean type is properly perserved. Of course this situation is not
desirable and can lead to unexpected results. However we cannot simply
fix the behavior because in Lua boolean and integer types are not
are completely distinct types and cannot be used interchangeably. So in
order to prevent breaking existing scripts logic, in this patch we add a
dedicated lua tunable named "tune.lua.smp-preserve-bool" which can take
the following values:

  - "on" : when converting haproxy smp to lua, boolean type is preserved
  - "off": when converting haproxy smp to lua, boolean is converted to
           integer (legacy behavior)

For now, the tunable defaults to "off" to preserve historical behavior.
However, when the option isn't set explicitly and lua is used, a warning
will be emitted in order to raise user's awareness about this ambiguity.
It is expected that the tunable could default to "on" in future versions,
thus it is recommended to avoid setting it to "off" except when using
existing Lua scripts that still rely on the old behavior regarding boolean
smp to Lua conversion, and that they cannot be fixed easily.

This should solve issue GH #2814. It may be relevant to backport this in
haproxy 3.1.

diff --git a/doc/configuration.txt b/doc/configuration.txt
index af815305cf353ccc48f5915e8d9ff63f940b0897..d7ad7888bf48b76b174f697338afb4f3714f09a0 100644 (file)
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -1670,6 +1670,7 @@ The following keywords are supported in the "global" section :
    - tune.lua.maxmem
    - tune.lua.service-timeout
    - tune.lua.session-timeout
+   - tune.lua.smp-preserve-bool
    - tune.lua.task-timeout
    - tune.max-checks-per-thread
    - tune.maxaccept
@@ -3989,6 +3990,25 @@ tune.lua.task-timeout <timeout>
   remain alive during of the lifetime of HAProxy. For example, a task used to
   check servers.
 
+tune.lua.smp-preserve-bool { on | off }
+  Explicitly tell haproxy how haproxy sample objects should be handled when
+  pushed to Lua. Indeed, when using native converters or sample fetches from
+  Lua script, haproxy converts the smp type to equivalent Lua type. Because
+  of historical implementation, there is an ambiguity around boolean
+  handling: when doing Lua -> haproxy smp conversion, booleans are properly
+  preserved, but when doing haproxy smp -> Lua conversion, booleans were
+  converted to integers. This means that a sample fetch or converter returning
+  a boolean would return an integer 0 or 1 when leveraged from Lua.
+  Unfortunately, in Lua, booleans and integers are not interchangeable. Thus,
+  to avoid ambiguities, "tune.lua.smp-preserve-bool" must explicitly be set to
+  either "on" (which means dropping the historical behavior for better
+  consistency) or "off" (enforce historical behavior to prevent existing script
+  logic from misbehaving). If the option is not set explicitly and a Lua script
+  is loaded from the configuration, haproxy will emit a warning, and the option
+  will implicitly default to "off" to match with the historical behavior. When
+  possible, it is recommended to set this option to "on" after ensuring that
+  in-use Lua scripts are properly handling bool haproxy samples as booleans.
+
 tune.max-checks-per-thread <number>
   Sets the number of active checks per thread above which a thread will
   actively try to search a less loaded thread to run the health check, or

diff --git a/src/hlua.c b/src/hlua.c
index e1a8b431a6469c58e1ceec2ac008fc53d7b57113..4889b6776790ed5994419a5d9a822e45281aa21a 100644 (file)
--- a/src/hlua.c
+++ b/src/hlua.c
@@ -84,6 +84,18 @@ enum hlua_log_opt {
 /* default log options, made of flags in hlua_log_opt */
 static uint hlua_log_opts = HLUA_LOG_LOGGERS_ON | HLUA_LOG_STDERR_AUTO;
 
+#define HLUA_SMP_PRESERVE_BOOL_UNK 0x0
+#define HLUA_SMP_PRESERVE_BOOL_ON  0x1
+#define HLUA_SMP_PRESERVE_BOOL_OFF 0X2
+
+/* used to know how to deal with smp2lua bool handling, option implicitly
+ * defaults to historical behavior (off), but it has to be set explicitly
+ * to avoid ambiguity, else a warning will be emitted
+ *
+ * FIXME: make it default to ON in 3.3??
+ */
+static uint8_t hlua_smp_preserve_bool = HLUA_SMP_PRESERVE_BOOL_UNK;
+
 /* Lua uses longjmp to perform yield or throwing errors. This
  * macro is used only for identifying the function that can
  * not return because a longjmp is executed.
@@ -1085,9 +1097,14 @@ __LJMP static int hlua_smp2lua(lua_State *L, struct sample *smp)
 {
        switch (smp->data.type) {
        case SMP_T_SINT:
-       case SMP_T_BOOL:
                lua_pushinteger(L, smp->data.u.sint);
                break;
+       case SMP_T_BOOL:
+               if (hlua_smp_preserve_bool == HLUA_SMP_PRESERVE_BOOL_ON)
+                       lua_pushboolean(L, !!smp->data.u.sint);
+               else
+                       lua_pushinteger(L, smp->data.u.sint);
+               break;
 
        case SMP_T_BIN:
        case SMP_T_STR:
@@ -12936,6 +12953,24 @@ static int hlua_cfg_parse_log_stderr(char **args, int section_type, struct proxy
        return 0;
 }
 
+static int hlua_cfg_parse_smp_preserve_bool(char **args, int section_type, struct proxy *curpx,
+                                            const struct proxy *defpx, const char *file, int line,
+                                            char **err)
+{
+       if (too_many_args(1, args, err, NULL))
+               return -1;
+
+       if (strcmp(args[1], "on") == 0)
+               hlua_smp_preserve_bool = HLUA_SMP_PRESERVE_BOOL_ON;
+       else if (strcmp(args[1], "off") == 0)
+               hlua_smp_preserve_bool = HLUA_SMP_PRESERVE_BOOL_OFF;
+       else {
+               memprintf(err, "'%s' expects either 'on' or 'off' but got '%s'.", args[0], args[1]);
+               return -1;
+       }
+       return 0;
+}
+
 /* This function is called by the main configuration key "lua-load". It loads and
  * execute an lua file during the parsing of the HAProxy configuration file. It is
  * the main lua entry point.
@@ -12955,6 +12990,21 @@ static int hlua_load_state(char **args, lua_State *L, char **err)
        int error;
        int nargs;
 
+       if (ONLY_ONCE()) {
+               /* we know we get there if "lua-load" or "lua-load-per-thread" was
+                * used in the config
+                */
+               if (hlua_smp_preserve_bool == HLUA_SMP_PRESERVE_BOOL_UNK) {
+                       /* hlua_smp_preserve_bool tunable must be explicitly set to
+                        * avoid ambiguity, so we raise a warning (but only if lua
+                        * is actually used
+                        */
+                       ha_warning("hlua: please set \"tune.lua.smp-preserve-bool\" tunable to "
+                                  "either \"on\" or \"off\" explicitly to avoid ambiguities. "
+                                  "Defaulting to \"off\".\n");
+               }
+       }
+
        /* Just load and compile the file. */
        error = luaL_loadfile(L, args[0]);
        if (error) {
@@ -13186,6 +13236,7 @@ static struct cfg_kw_list cfg_kws = {{ },{
        { CFG_GLOBAL, "tune.lua.maxmem",          hlua_parse_maxmem },
        { CFG_GLOBAL, "tune.lua.log.loggers",     hlua_cfg_parse_log_loggers },
        { CFG_GLOBAL, "tune.lua.log.stderr",      hlua_cfg_parse_log_stderr },
+       { CFG_GLOBAL, "tune.lua.smp-preserve-bool", hlua_cfg_parse_smp_preserve_bool },
        { 0, NULL, NULL },
 }};