From: Aurelien DARRAGON Date: Tue, 30 May 2023 14:34:39 +0000 (+0200) Subject: MEDIUM: sample: introduce 'same' output type X-Git-Tag: v2.9-dev2~80 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=4f0e0f5a6510be08102c93b5c597b61c7d87585e;p=thirdparty%2Fhaproxy.git 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 --- diff --git a/include/haproxy/sample-t.h b/include/haproxy/sample-t.h index b2da0772b2..27cf4bab82 100644 --- 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 d52cdc00c8..6a41b1eb28 100644 --- 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 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 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 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 },