MEDIUM: sample: introduce 'same' output type

Thierry Fournier reported an annoying side-effect when using the debug()
converter.

Consider the following examples:

[1]  http-request set-var(txn.test) bool(true),ipmask(24)
[2]  http-request redirect location /match if { bool(true),ipmask(32) }

When starting haproxy with [1] example we get:

   config : parsing [test.conf:XX] : error detected in frontend 'fe' while parsing 'http-request set-var(txn.test)' rule : converter 'ipmask' cannot be applied.

With [2], we get:

  config : parsing [test.conf:XX] : error detected in frontend 'fe' while parsing 'http-request redirect' rule : error in condition: converter 'ipmask' cannot be applied in ACL expression 'bool(true),ipmask(32)'.

Now consider the following examples which are based on [1] and [2]
but with the debug() sample conv inserted in-between those incompatible
sample types:

[1*] http-request set-var(txn.test) bool(true),debug,ipmask(24)
[2*] http-request redirect location /match if { bool(true),debug,ipmask(32) }

According to the documentation, "it is safe to insert the debug converter
anywhere in a chain, even with non-printable sample types".
Thus we don't expect any side-effect from using it within a chain.

However in current implementation, because of debug() returning SMP_T_ANY
type which is a pseudo type (only resolved at runtime), the sample
compatibility checks performed at parsing time are completely uneffective.
(haproxy will start and no warning will be emitted)

The indesirable effect of this is that debug() prevents haproxy from
warning you about impossible type conversions, hiding potential errors
in the configuration that could result to unexpected evaluation or logic
while serving live traffic. We better let haproxy warn you about this kind
of errors when it has the chance.

With our previous examples, this could cause some inconveniences. Let's
say for example that you are testing a configuration prior to deploying
it. When testing the config you might want to use debug() converter from
time to time to check how the conversion chain behaves.

Now after deploying the exact same conf, you might want to remove those
testing debug() statements since they are no longer relevant.. but
removing them could "break" the config and suddenly prevent haproxy from
starting upon reload/restart. (this is the expected behavior, but it
comes a bit too late because of debug() hiding errors during tests..)

To fix this, we introduce a new output type for sample expressions:
  SMP_T_SAME - may only be used as "expected" out_type (parsing time)
               for sample converters.

As it name implies, it is a way for the developpers to indicate that the
resulting converter's output type is guaranteed to match the type of the
sample that is presented on the converter's input side.
(converter may alter data content, but data type must not be changed)

What it does is that it tells haproxy that if switching to the converter
(by looking at the converter's input only, since outype is SAME) is
conversion-free, then the converter type can safely be ignored for type
compatibility checks within the chain.

debug()'s out_type is thus set to SMP_T_SAME instead of ANY, which allows
it to fully comply with the doc in the sense that it does not impact the
conversion chain when inserted between sample items.

Co-authored-by: Thierry Fournier <thierry.f.78@gmail.com>

diff --git a/include/haproxy/sample-t.h b/include/haproxy/sample-t.h
index b2da0772b2417a5ca550f7724f14b758f2875bcb..27cf4bab820a3dd5d9ca14aa7352344ade73cc90 100644 (file)
--- a/include/haproxy/sample-t.h
+++ b/include/haproxy/sample-t.h
@@ -34,6 +34,7 @@
  */
 enum {
        SMP_T_ANY = 0,   /* pseudo type: any type */
+       SMP_T_SAME,      /* special: output type hint for converters that don't alter input type (out == in) */
        SMP_T_BOOL,      /* boolean */
        SMP_T_SINT,      /* signed 64bits integer type */
        SMP_T_ADDR,      /* pseudo type: could be ipv4 or ipv6 */

diff --git a/src/sample.c b/src/sample.c
index d52cdc00c88d1f5eb48e3c59504d180f47533bcf..6a41b1eb28dfa95a19973b4c1acbc755f54c54f7 100644 (file)
--- a/src/sample.c
+++ b/src/sample.c
@@ -49,6 +49,7 @@
 /* sample type names */
 const char *smp_to_type[SMP_TYPES] = {
        [SMP_T_ANY]  = "any",
+       [SMP_T_SAME] = "same",
        [SMP_T_BOOL] = "bool",
        [SMP_T_SINT] = "sint",
        [SMP_T_ADDR] = "addr",
@@ -331,20 +332,48 @@ static const char *fetch_ckp_names[SMP_CKP_ENTRIES] = {
        [SMP_CKP_CLI_PARSER] = "CLI parser",
 };
 
-/* This function returns the type of the data returned by the sample_expr.
- * It assumes that the <expr> and all of its converters are properly
- * initialized.
+/* This function returns the most accurate expected type of the data returned
+ * by the sample_expr. It assumes that the <expr> and all of its converters are
+ * properly initialized.
  */
-inline
 int smp_expr_output_type(struct sample_expr *expr)
 {
-       struct sample_conv_expr *smp_expr;
+       struct sample_conv_expr *cur_smp = NULL;
+       int cur_type = SMP_T_ANY;  /* current type in the chain */
+       int next_type = SMP_T_ANY; /* next type in the chain */
 
        if (!LIST_ISEMPTY(&expr->conv_exprs)) {
-               smp_expr = LIST_PREV(&expr->conv_exprs, struct sample_conv_expr *, list);
-               return smp_expr->conv->out_type;
+               /* Ignore converters that output SMP_T_SAME if switching to them is
+                * conversion-free. (such converter's output match with input, thus only
+                * their input is considered)
+                *
+                * We start looking at the end of conv list and then loop back until the
+                * sample fetch for better performance (it is more likely to find the last
+                * effective output type near the end of the chain)
+                */
+               do {
+                       struct list *cur_head = (cur_smp) ? &cur_smp->list : &expr->conv_exprs;
+
+                       cur_smp = LIST_PREV(cur_head, struct sample_conv_expr *, list);
+                       if (cur_smp->conv->out_type != SMP_T_SAME) {
+                               /* current converter has effective out_type */
+                               cur_type = cur_smp->conv->out_type;
+                               goto out;
+                       }
+                       else if (sample_casts[cur_type][next_type] != c_none)
+                               return next_type; /* switching to next type is not conversion-free */
+
+                       next_type = cur_smp->conv->in_type;
+               } while (cur_smp != LIST_NEXT(&expr->conv_exprs, struct sample_conv_expr *, list));
        }
-       return expr->fetch->out_type;
+       /* conv list empty or doesn't have effective out_type,
+        * falling back to sample fetch out_type
+        */
+       cur_type = expr->fetch->out_type;
+ out:
+       if (sample_casts[cur_type][next_type] != c_none)
+               return next_type; /* switching to next type is not conversion-free */
+       return cur_type;
 }
 
 
@@ -993,16 +1022,17 @@ static int c_bool2bin(struct sample *smp)
 /*****************************************************************/
 
 sample_cast_fct sample_casts[SMP_TYPES][SMP_TYPES] = {
-/*            to:  ANY     BOOL       SINT       ADDR        IPV4      IPV6        STR         BIN         METH */
-/* from:  ANY */ { c_none, c_pseudo,  c_pseudo,  c_pseudo,   c_pseudo, c_pseudo,   c_pseudo,   c_pseudo,   c_pseudo,   },
-/*       BOOL */ { c_none, c_none,    c_none,    NULL,       NULL,     NULL,       c_int2str,  c_bool2bin, NULL,       },
-/*       SINT */ { c_none, c_none,    c_none,    c_int2ip,   c_int2ip, c_int2ipv6, c_int2str,  c_int2bin,  NULL,       },
-/*       ADDR */ { c_none, NULL,      NULL,      c_pseudo,   c_pseudo, c_pseudo,   c_pseudo,   c_pseudo,   NULL,       },
-/*       IPV4 */ { c_none, NULL,      c_ip2int,  c_none,     c_none,   c_ip2ipv6,  c_ip2str,   c_addr2bin, NULL,       },
-/*       IPV6 */ { c_none, NULL,      NULL,      c_none,     c_ipv62ip,c_none,     c_ipv62str, c_addr2bin, NULL,       },
-/*        STR */ { c_none, c_str2int, c_str2int, c_str2addr, c_str2ip, c_str2ipv6, c_none,     c_none,     c_str2meth, },
-/*        BIN */ { c_none, NULL,      NULL,      NULL,       NULL,     NULL,       c_bin2str,  c_none,     c_str2meth, },
-/*       METH */ { c_none, NULL,      NULL,      NULL,       NULL,     NULL,       c_meth2str, c_meth2str, c_none,     }
+/*            to:  ANY     SAME      BOOL       SINT       ADDR        IPV4       IPV6        STR         BIN         METH */
+/* from:  ANY */ { c_none, NULL,     c_pseudo,  c_pseudo,  c_pseudo,   c_pseudo,  c_pseudo,   c_pseudo,   c_pseudo,   c_pseudo   },
+/*       SAME */ { NULL,   NULL,     NULL,      NULL,      NULL,       NULL,      NULL,       NULL,       NULL,       NULL       },
+/*       BOOL */ { c_none, NULL,     c_none,    c_none,    NULL,       NULL,      NULL,       c_int2str,  c_bool2bin, NULL       },
+/*       SINT */ { c_none, NULL,     c_none,    c_none,    c_int2ip,   c_int2ip,  c_int2ipv6, c_int2str,  c_int2bin,  NULL       },
+/*       ADDR */ { c_none, NULL,     NULL,      NULL,      c_pseudo,   c_pseudo,  c_pseudo,   c_pseudo,   c_pseudo,   NULL       },
+/*       IPV4 */ { c_none, NULL,     NULL,      c_ip2int,  c_none,     c_none,    c_ip2ipv6,  c_ip2str,   c_addr2bin, NULL       },
+/*       IPV6 */ { c_none, NULL,     NULL,      NULL,      c_none,     c_ipv62ip, c_none,     c_ipv62str, c_addr2bin, NULL       },
+/*        STR */ { c_none, NULL,     c_str2int, c_str2int, c_str2addr, c_str2ip,  c_str2ipv6, c_none,     c_none,     c_str2meth },
+/*        BIN */ { c_none, NULL,     NULL,      NULL,      NULL,       NULL,      NULL,       c_bin2str,  c_none,     c_str2meth },
+/*       METH */ { c_none, NULL,     NULL,      NULL,      NULL,       NULL,      NULL,       c_meth2str, c_meth2str, c_none     }
 };
 
 /* Process the converters (if any) for a sample expr after the first fetch
@@ -1103,7 +1133,15 @@ int sample_parse_expr_cnv(char **str, int *idx, char **endptr, char **err_msg, s
                        goto out_error;
                }
 
-               prev_type = conv->out_type;
+               /* Ignore converters that output SMP_T_SAME if switching to them is
+                * conversion-free. (such converter's output match with input, thus only
+                * their input is considered)
+                */
+               if (conv->out_type != SMP_T_SAME)
+                       prev_type = conv->out_type;
+               else if (sample_casts[prev_type][conv->in_type] != c_none)
+                       prev_type = conv->in_type;
+
                conv_expr = calloc(1, sizeof(*conv_expr));
                if (!conv_expr)
                        goto out_error;
@@ -4488,7 +4526,7 @@ INITCALL1(STG_REGISTER, sample_register_fetches, &smp_kws);
 /* Note: must not be declared <const> as its list will be overwritten */
 static struct sample_conv_kw_list sample_conv_kws = {ILH, {
        { "add_item",sample_conv_add_item,     ARG3(2,STR,STR,STR),   smp_check_add_item,       SMP_T_STR,  SMP_T_STR  },
-       { "debug",   sample_conv_debug,        ARG2(0,STR,STR),       smp_check_debug,          SMP_T_ANY,  SMP_T_ANY  },
+       { "debug",   sample_conv_debug,        ARG2(0,STR,STR),       smp_check_debug,          SMP_T_ANY,  SMP_T_SAME },
        { "b64dec",  sample_conv_base642bin,   0,                     NULL,                     SMP_T_STR,  SMP_T_BIN  },
        { "base64",  sample_conv_bin2base64,   0,                     NULL,                     SMP_T_BIN,  SMP_T_STR  },
        { "concat",  sample_conv_concat,       ARG3(1,STR,STR,STR),   smp_check_concat,         SMP_T_STR,  SMP_T_STR  },