From: Arran Cudbard-Bell Date: Sat, 18 Sep 2021 03:14:19 +0000 (-0500) Subject: Update Redis to parse RESP3 types X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=15a889a3dc5ceef07e9c179c8e6c0df2873fd765;p=thirdparty%2Ffreeradius-server.git Update Redis to parse RESP3 types Convert redis xlat to be argument based --- diff --git a/src/lib/redis/base.h b/src/lib/redis/base.h index 583c250159..b804d07e9a 100644 --- a/src/lib/redis/base.h +++ b/src/lib/redis/base.h @@ -141,7 +141,8 @@ fr_redis_rcode_t fr_redis_command_status(fr_redis_conn_t *conn, redisReply *repl void fr_redis_reply_print(fr_log_lvl_t lvl, redisReply *reply, request_t *request, int idx); int fr_redis_reply_to_value_box(TALLOC_CTX *ctx, fr_value_box_t *out, redisReply *reply, - fr_type_t dst_type, fr_dict_attr_t const *dst_enumv); + fr_type_t dst_type, fr_dict_attr_t const *dst_enumv, + bool box_error, bool shallow); int fr_redis_reply_to_map(TALLOC_CTX *ctx, fr_map_list_t *out, request_t *request, redisReply *key, redisReply *op, redisReply *value); diff --git a/src/lib/redis/redis.c b/src/lib/redis/redis.c index cac657dd91..2393adc30a 100644 --- a/src/lib/redis/redis.c +++ b/src/lib/redis/redis.c @@ -184,19 +184,27 @@ void fr_redis_reply_print(fr_log_lvl_t lvl, redisReply *reply, request_t *reques * @note Any unsupported types will trigger an assert. You must check the * reply type prior to calling this function. * - * @param[in,out] ctx to allocate any buffers in. - * @param[out] out Where to write the cast type. - * @param[in] reply to process. - * @param[in] dst_type to convert to. - * @param[in] dst_enumv Used to convert string types to integers for attributes - * with enumerated values. + * @param[in,out] ctx to allocate any buffers in. + * @param[out] out Where to write the cast type. + * @param[in] reply to process. + * @param[in] dst_type to convert to. May be FR_TYPE_NULL + * to infer type. + * @param[in] dst_enumv Used to convert string types to + * integers for attribute with enumerated + * values. + * @param[in] box_error If true then REDIS_REPLY_ERROR will be + * copied to a box, otherwise we'll return + * and error with the contents of the error + * available on the thread local error stack. + * @param[in] shallow If true, we shallow copy strings. * @return - * - 1 if we received a NIL reply. Out will remain uninitialized. + * - 1 if we received a NIL reply. * - 0 on success. * - -1 on cast or parse failure. */ int fr_redis_reply_to_value_box(TALLOC_CTX *ctx, fr_value_box_t *out, redisReply *reply, - fr_type_t dst_type, fr_dict_attr_t const *dst_enumv) + fr_type_t dst_type, fr_dict_attr_t const *dst_enumv, + bool box_error, bool shallow) { fr_value_box_t in; @@ -204,6 +212,7 @@ int fr_redis_reply_to_value_box(TALLOC_CTX *ctx, fr_value_box_t *out, redisReply switch (reply->type) { case REDIS_REPLY_NIL: + fr_value_box_init(out, FR_TYPE_NULL, NULL, false); return 1; /* @@ -238,17 +247,94 @@ int fr_redis_reply_to_value_box(TALLOC_CTX *ctx, fr_value_box_t *out, redisReply } break; +#if HIREDIS_MAJOR >= 1 + case REDIS_REPLY_DOUBLE: + fr_value_box_shallow(&in, strtod(reply->str, NULL), true); + break; + + case REDIS_REPLY_BOOL: + fr_value_box_shallow(&in, (bool)reply->integer, true); + break; +#endif + + case REDIS_REPLY_ERROR: + if (!box_error) { + fr_strerror_printf("Redis error: %pV", + fr_box_strvalue_len(reply->str, reply->len)); + return -1; + } + FALL_THROUGH; + +#if HIREDIS_MAJOR >= 1 + case REDIS_REPLY_BIGNUM: /* FIXME - Could try and conver to integer ? */ +#endif + + case REDIS_REPLY_STRING: case REDIS_REPLY_STATUS: - case REDIS_REPLY_ERROR: - fr_value_box_bstrndup_shallow(&in, NULL, reply->str, reply->len, true); + if (shallow) { + fr_value_box_bstrndup_shallow(out, NULL, reply->str, reply->len, true); + } else { + if (fr_value_box_bstrndup(ctx, out, NULL, reply->str, reply->len, true) < 0) return -1; + } break; +#if HIREDIS_MAJOR >= 1 + case REDIS_REPLY_VERB: + { + fr_value_box_t *verb, *vtype; + + fr_value_box_init(out, FR_TYPE_GROUP, NULL, true); + + verb = fr_value_box_alloc(ctx, FR_TYPE_STRING, NULL, true); + if (unlikely(!verb)) { + fr_strerror_const("Out of memory"); + return -1; + } + if (fr_value_box_bstrndup(ctx, verb, NULL, reply->str, reply->len, true) < 0) return -1; + fr_dlist_insert_head(&out->vb_group, verb); + + vtype = fr_value_box_alloc(ctx, FR_TYPE_STRING, NULL, true); + if (unlikely(!vtype)) { + fr_strerror_const("Out of memory"); + talloc_free(verb); + return -1; + } + if (fr_value_box_strdup(ctx, vtype, NULL, reply->vtype, true) < 0) return -1; + fr_dlist_insert_head(&out->vb_group, vtype); + + } + break; +#endif + +#if HIREDIS_MAJOR >= 1 + case REDIS_REPLY_SET: + case REDIS_REPLY_MAP: + case REDIS_REPLY_PUSH: +#endif case REDIS_REPLY_ARRAY: - fr_assert(0); + { + fr_value_box_t *vb; + size_t i; + + fr_value_box_init(out, FR_TYPE_GROUP, NULL, true); + + for (i = 0; i < reply->elements; i++) { + vb = fr_value_box_alloc_null(ctx); + if (unlikely(!vb)) { + array_error: + fr_dlist_talloc_free(&out->vb_group); + return -1; + } + fr_dlist_insert_tail(&out->vb_group, vb); + + if (fr_redis_reply_to_value_box(vb, vb, reply->element[i], + FR_TYPE_NULL, NULL, box_error, shallow) < 0) goto array_error; + } + } } - if (fr_value_box_cast(ctx, out, dst_type, dst_enumv, &in) < 0) return -1; + if ((dst_type != FR_TYPE_NULL) && (fr_value_box_cast(ctx, out, dst_type, dst_enumv, &in) < 0)) return -1; return 0; } @@ -312,7 +398,7 @@ int fr_redis_reply_to_map(TALLOC_CTX *ctx, fr_map_list_t *out, request_t *reques /* Logs own errors */ if (fr_redis_reply_to_value_box(map, &vpt, value, - tmpl_da(map->lhs)->type, tmpl_da(map->lhs)) < 0) { + tmpl_da(map->lhs)->type, tmpl_da(map->lhs), false, false) < 0) { RPEDEBUG("Failed converting Redis data"); goto error; } diff --git a/src/modules/rlm_redis/rlm_redis.c b/src/modules/rlm_redis/rlm_redis.c index 530cecd3da..778e55bbf7 100644 --- a/src/modules/rlm_redis/rlm_redis.c +++ b/src/modules/rlm_redis/rlm_redis.c @@ -262,6 +262,11 @@ static xlat_action_t redis_node_xlat(TALLOC_CTX *ctx, fr_dcursor_t *out, return XLAT_ACTION_DONE; } +static xlat_arg_parser_t const redis_args[] = { + { .required = true, .variadic = true, .concat = true, .type = FR_TYPE_STRING }, + XLAT_ARG_PARSER_TERMINATOR +}; + /** Xlat to make calls to redis * @verbatim @@ -270,11 +275,14 @@ static xlat_action_t redis_node_xlat(TALLOC_CTX *ctx, fr_dcursor_t *out, * * @ingroup xlat_functions */ -static ssize_t redis_xlat(UNUSED TALLOC_CTX *ctx, char **out, size_t outlen, - void const *mod_inst, UNUSED void const *xlat_inst, - request_t *request, char const *fmt) +static xlat_action_t redis_xlat(TALLOC_CTX *ctx, fr_dcursor_t *out, + request_t *request, void const *xlat_inst, + UNUSED void *xlat_thread_inst, + fr_value_box_list_t *in) { - rlm_redis_t const *inst = mod_inst; + + rlm_redis_t const *inst = talloc_get_type_abort_const(*((void const * const *)xlat_inst), rlm_redis_t); + xlat_action_t action = XLAT_ACTION_DONE; fr_redis_conn_t *conn; bool read_only = false; @@ -283,79 +291,72 @@ static ssize_t redis_xlat(UNUSED TALLOC_CTX *ctx, char **out, size_t outlen, fr_redis_cluster_state_t state; fr_redis_rcode_t status; + redisReply *reply = NULL; int s_ret; - size_t len; - int ret; - - char const *p = fmt, *q; + fr_value_box_t *first = fr_dlist_head(in); + fr_sbuff_t sbuff = FR_SBUFF_IN(first->vb_strvalue, first->vb_length); - int argc; + int argc = 0; char const *argv[MAX_REDIS_ARGS]; - char argv_buf[MAX_REDIS_COMMAND_LEN]; + size_t arg_len[MAX_REDIS_ARGS]; - if (p[0] == '-') { - p++; - read_only = true; - } + fr_value_box_t *vb_out; + + if (fr_sbuff_next_if_char(&sbuff, '-')) read_only = true; /* * Hack to allow querying against a specific node for testing */ - if (p[0] == '@') { + if (fr_sbuff_next_if_char(&sbuff, '@')) { fr_socket_t node_addr; - fr_pool_t *pool; + fr_pool_t *pool; RDEBUG3("Overriding node selection"); - p++; - q = strchr(p, ' '); - if (!q) { - REDEBUG("Found node specifier but no command, format is [-][@[:port]] "); - return -1; - } - - if (fr_inet_pton_port(&node_addr.inet.dst_ipaddr, &node_addr.inet.dst_port, p, q - p, AF_UNSPEC, true, true) < 0) { + if (fr_inet_pton_port(&node_addr.inet.dst_ipaddr, &node_addr.inet.dst_port, + fr_sbuff_current(&sbuff), fr_sbuff_remaining(&sbuff), + AF_UNSPEC, true, true) < 0) { RPEDEBUG("Failed parsing node address"); - return -1; + return XLAT_ACTION_FAIL; } - p = q + 1; - if (fr_redis_cluster_pool_by_node_addr(&pool, inst->cluster, &node_addr, true) < 0) { RPEDEBUG("Failed locating cluster node"); - return -1; + return XLAT_ACTION_FAIL; } conn = fr_pool_connection_get(pool, request); if (!conn) { REDEBUG("No connections available for cluster node"); - return -1; + return XLAT_ACTION_FAIL; } - argc = rad_expand_xlat(request, p, MAX_REDIS_ARGS, argv, false, sizeof(argv_buf), argv_buf); - if (argc <= 0) { - RPEDEBUG("Invalid command: %s", p); - arg_error: - fr_pool_connection_release(pool, request, conn); - return -1; - } - if (argc >= (MAX_REDIS_ARGS - 1)) { - RPEDEBUG("Too many parameters; increase MAX_REDIS_ARGS and recompile: %s", p); - goto arg_error; + fr_dlist_talloc_free_head(in); /* Remove and free server arg */ + + fr_dlist_foreach(in, fr_value_box_t, vb) { + if (argc == NUM_ELEMENTS(argv)) { + REDEBUG("Too many arguments (%i)", argc); + REXDENT(); + goto fail; + } + + argv[argc] = vb->vb_strvalue; + arg_len[argc] = vb->vb_length; + argc++; } - RDEBUG2("Executing command: %s", argv[0]); + RDEBUG2("Executing command: %pV", fr_dlist_head(in)); if (argc > 1) { - RDEBUG2("With argments"); + RDEBUG2("With arguments"); RINDENT(); for (int i = 1; i < argc; i++) RDEBUG2("[%i] %s", i, argv[i]); REXDENT(); } if (!read_only) { - reply = redisCommandArgv(conn->handle, argc, argv, NULL); + reply = redisCommandArgv(conn->handle, argc, argv, arg_len); status = fr_redis_command_status(conn, reply); } else if (redis_command_read_only(&status, &reply, request, conn, argc, argv) == -2) { goto close_conn; @@ -368,9 +369,8 @@ static ssize_t redis_xlat(UNUSED TALLOC_CTX *ctx, char **out, size_t outlen, { fr_value_box_t vb; - if (fr_redis_reply_to_value_box(NULL, &vb, reply, FR_TYPE_STRING, NULL) == 0) { + if (fr_redis_reply_to_value_box(NULL, &vb, reply, FR_TYPE_STRING, NULL, false, true) == 0) { REDEBUG("Key served by a different node: %pV", &vb); - fr_value_box_clear(&vb); } goto fail; } @@ -381,32 +381,31 @@ static ssize_t redis_xlat(UNUSED TALLOC_CTX *ctx, char **out, size_t outlen, case REDIS_RCODE_RECONNECT: close_conn: fr_pool_connection_close(pool, request, conn); - ret = -1; + action = XLAT_ACTION_FAIL; goto finish; default: fail: fr_pool_connection_release(pool, request, conn); - ret = -1; + action = XLAT_ACTION_FAIL; goto finish; } } - /* - * Normal node selection and execution based on key - */ - argc = rad_expand_xlat(request, p, MAX_REDIS_ARGS, argv, false, sizeof(argv_buf), argv_buf); - if (argc <= 0) { - RPEDEBUG("Invalid command: %s", p); - ret = -1; - goto finish; - } + RDEBUG2("REDIS command arguments"); + RINDENT(); + fr_dlist_foreach(in, fr_value_box_t, vb) { + if (argc == NUM_ELEMENTS(argv)) { + REDEBUG("Too many arguments (%i)", argc); + REXDENT(); + goto finish; + } - if (argc >= (MAX_REDIS_ARGS - 1)) { - RPEDEBUG("Too many parameters; increase MAX_REDIS_ARGS and recompile: %s", p); - ret = -1; - goto finish; + argv[argc] = vb->vb_strvalue; + arg_len[argc] = vb->vb_length; + argc++; } + REXDENT(); /* * If we've got multiple arguments, the second one is usually the key. @@ -417,59 +416,49 @@ static ssize_t redis_xlat(UNUSED TALLOC_CTX *ctx, char **out, size_t outlen, */ if (argc > 1) { key = (uint8_t const *)argv[1]; - key_len = strlen((char const *)key); + key_len = arg_len[1]; } + for (s_ret = fr_redis_cluster_state_init(&state, &conn, inst->cluster, request, key, key_len, read_only); s_ret == REDIS_RCODE_TRY_AGAIN; /* Continue */ s_ret = fr_redis_cluster_state_next(&state, &conn, inst->cluster, request, status, &reply)) { - RDEBUG2("Executing command: %s", argv[0]); + RDEBUG2("Executing command: %pV", fr_dlist_head(in)); if (argc > 1) { RDEBUG2("With arguments"); RINDENT(); for (int i = 1; i < argc; i++) RDEBUG2("[%i] %s", i, argv[i]); REXDENT(); } + if (!read_only) { - reply = redisCommandArgv(conn->handle, argc, argv, NULL); + reply = redisCommandArgv(conn->handle, argc, argv, arg_len); status = fr_redis_command_status(conn, reply); } else if (redis_command_read_only(&status, &reply, request, conn, argc, argv) == -2) { state.close_conn = true; } } if (s_ret != REDIS_RCODE_SUCCESS) { - ret = -1; + action = XLAT_ACTION_FAIL; goto finish; } if (!fr_cond_assert(reply)) { - ret = -1; + action = XLAT_ACTION_FAIL; goto finish; } reply_parse: - switch (reply->type) { - case REDIS_REPLY_INTEGER: - ret = snprintf(*out, outlen, "%lld", reply->integer); - break; - - case REDIS_REPLY_STATUS: - case REDIS_REPLY_STRING: - len = (((size_t)reply->len) >= outlen) ? outlen - 1: (size_t) reply->len; - memcpy(*out, reply->str, len); - (*out)[len] = '\0'; - ret = reply->len; - break; - - default: - REDEBUG("Server returned non-value type \"%s\"", - fr_table_str_by_value(redis_reply_types, reply->type, "")); - ret = -1; - break; + MEM(vb_out = fr_value_box_alloc_null(ctx)); + if (fr_redis_reply_to_value_box(ctx, vb_out, reply, FR_TYPE_NULL, NULL, false, false) < 0) { + RPERROR("Failed processing reply"); + return XLAT_ACTION_FAIL; } + fr_dcursor_append(out, vb_out); finish: fr_redis_reply_free(&reply); - return ret; + + return action; } static int mod_bootstrap(void *instance, CONF_SECTION *conf) @@ -481,7 +470,9 @@ static int mod_bootstrap(void *instance, CONF_SECTION *conf) inst->name = cf_section_name2(conf); if (!inst->name) inst->name = cf_section_name1(conf); - xlat_register_legacy(inst, inst->name, redis_xlat, NULL, NULL, 0, XLAT_DEFAULT_BUF_LEN); + xlat = xlat_register(inst, inst->name, redis_xlat, false); + xlat_func_args(xlat, redis_args); + xlat_async_instantiate_set(xlat, redis_xlat_instantiate, rlm_redis_t *, NULL, inst); /* * %(redis_node:[ idx]) diff --git a/src/tests/modules/redis/cluster_key.unlang b/src/tests/modules/redis/cluster_key.unlang index 8ee3682990..9ef6c0907d 100644 --- a/src/tests/modules/redis/cluster_key.unlang +++ b/src/tests/modules/redis/cluster_key.unlang @@ -17,21 +17,21 @@ update control { } # Hashes to Redis cluster node master 1 (1) -if ("%{redis:SET b '%{control.Tmp-String-0}'}" == 'OK') { +if ("%(redis:SET b "%{control.Tmp-String-0}")" == 'OK') { test_pass } else { test_fail } # Hashes to Redis cluster node master 3 (2) -if ("%{redis:SET c '%{control.Tmp-String-1}'}" == 'OK') { +if ("%(redis:SET c "%{control.Tmp-String-1}")" == 'OK') { test_pass } else { test_fail } # Hashes to Redis cluster node master 2 (3) -if ("%{redis:SET d '%{control.Tmp-String-2}'}" == 'OK') { +if ("%(redis:SET d "%{control.Tmp-String-2}")" == 'OK') { test_pass } else { test_fail @@ -40,19 +40,19 @@ if ("%{redis:SET d '%{control.Tmp-String-2}'}" == 'OK') { # # Now check they are where we expect # -if ("%{redis:@%{redis_node:b 0} GET b}" == "%{control.Tmp-String-0}") { +if ("%(redis:@%(redis_node:b 0) GET b)" == "%{control.Tmp-String-0}") { test_pass } else { test_fail } -if ("%{redis:@%{redis_node:c 0} GET c}" == "%{control.Tmp-String-1}") { +if ("%(redis:@%(redis_node:c 0) GET c)" == "%{control.Tmp-String-1}") { test_pass } else { test_fail } -if ("%{redis:@%{redis_node:d 0} GET d}" == "%{control.Tmp-String-2}") { +if ("%(redis:@%(redis_node:d 0) GET d)" == "%{control.Tmp-String-2}") { test_pass } else { test_fail diff --git a/src/tests/modules/redis/cluster_node_fail.unlang b/src/tests/modules/redis/cluster_node_fail.unlang index 8b859f232b..fdd5c27d3a 100644 --- a/src/tests/modules/redis/cluster_node_fail.unlang +++ b/src/tests/modules/redis/cluster_node_fail.unlang @@ -4,7 +4,7 @@ $INCLUDE cluster_reset.inc # Hashes to Redis cluster node master 1 -if ("%{redis:SET b 'boom'}" == 'OK') { +if ("%(redis:SET b 'boom')" == 'OK') { test_pass } else { test_fail @@ -16,34 +16,34 @@ update request { } # Cause one of the redis cluster nodes to SEGV -if ("%{redis:@%{redis_node:b 0} DEBUG SEGFAULT}" != '') { +if ("%(redis:@%(redis_node:b 0) DEBUG SEGFAULT)" != '') { test_fail } else { test_pass } # Forcefully failover the slave for that node -if ("%{redis:@%{redis_node:b 1} CLUSTER FAILOVER TAKEOVER}" != 'OK') { +if ("%(redis:@%(redis_node:b 1) CLUSTER FAILOVER TAKEOVER)" != 'OK') { test_fail } else { test_pass } -if ("%{redis:GET b}" == 'boom') { +if ("%(redis:GET b)" == 'boom') { test_pass } else { test_fail } # Kill that one too -if ("%{redis:@%{redis_node:b 1} DEBUG SEGFAULT}" != '') { +if ("%(redis:@%(redis_node:b 1) DEBUG SEGFAULT)" != '') { test_fail } else { test_pass } # No alternatives... -if ("%{redis:GET b}" != 'boom') { +if ("%(redis:GET b)" != 'boom') { test_pass } else { test_fail diff --git a/src/tests/modules/redis/cluster_reset.inc b/src/tests/modules/redis/cluster_reset.inc index 8a8099ec2f..63bb51a3c7 100644 --- a/src/tests/modules/redis/cluster_reset.inc +++ b/src/tests/modules/redis/cluster_reset.inc @@ -48,7 +48,7 @@ update request { } if (!&Tmp-String-0 || (&Tmp-String-0 == '')) { update request { - &Tmp-String-0 := $ENV{REDIS_IPPOOL_TEST_SERVER} + &Tmp-String-0 := "$ENV{REDIS_IPPOOL_TEST_SERVER}" } } @@ -63,19 +63,19 @@ foreach &control.Tmp-Integer-0 { # # Force a remap as the slaves don't show up in the cluster immediately # - if ("%{redis_remap:%{Tmp-String-0}:30001}" == 'success') { + if ("%(redis_remap:%{Tmp-String-0}:30001)" == 'success') { # Hashes to Redis cluster node master 0 (1) - if (("%{redis:SET b '%{control.Tmp-String-0}'}" == 'OK') && \ - ("%{redis:SET c '%{control.Tmp-String-1}'}" == 'OK') && \ - ("%{redis:SET d '%{control.Tmp-String-2}'}" == 'OK')) { + if (("%(redis:SET b "%{control.Tmp-String-0}")" == 'OK') && \ + ("%(redis:SET c "%{control.Tmp-String-1}")" == 'OK') && \ + ("%(redis:SET d "%{control.Tmp-String-2}")" == 'OK')) { # # The actual node to keyslot mapping seems to be somewhat random # so we now need to figure out which slave each of those keys # ended up on. # - if (("%{redis:-@%{redis_node:b 1} GET b}" == "%{control.Tmp-String-0}") && \ - ("%{redis:-@%{redis_node:c 1} GET c}" == "%{control.Tmp-String-1}") && \ - ("%{redis:-@%{redis_node:d 1} GET d}" == "%{control.Tmp-String-2}")) { + if (("%(redis:-@%(redis_node:b 1) GET b)" == "%{control.Tmp-String-0}") && \ + ("%(redis:-@%(redis_node:c 1) GET c)" == "%{control.Tmp-String-1}") && \ + ("%(redis:-@%(redis_node:d 1) GET d)" == "%{control.Tmp-String-2}")) { break } }