From 90174ac8065fd6fc6d3835b32a7ab58b8375c26e Mon Sep 17 00:00:00 2001 From: "Alan T. DeKok" Date: Tue, 1 Jul 2025 17:39:46 -0400 Subject: [PATCH] more trigger fixes. allow back-ticks for exec, and run the exec tmpl type --- raddb/trigger.conf | 34 +++--- src/lib/server/trigger.c | 227 ++++++++++++++++++--------------------- src/lib/unlang/xlat.c | 40 ------- src/lib/unlang/xlat.h | 4 - 4 files changed, 122 insertions(+), 183 deletions(-) diff --git a/raddb/trigger.conf b/raddb/trigger.conf index d4f636c37a..40651c88e5 100644 --- a/raddb/trigger.conf +++ b/raddb/trigger.conf @@ -130,28 +130,30 @@ trigger { # # start:: The server has just started. # - start = "${snmptrap}::serverStart" + start = `${snmptrap}::serverStart` # # stop:: The server is about to stop. # - stop = "${snmptrap}::serverStop" + stop = `${snmptrap}::serverStop` # # max_requests:: The "max_requests" condition has been reached. # # This will trigger only once per 60 seconds. # - max_requests = "${snmptrap}::serverMaxRequests" + max_requests = `${snmptrap}::serverMaxRequests` } # # ### Module triggers # - # Triggers for specific modules. These are *not* in the module - # configuration because they are global to all instances of the - # module. You can have module-specific triggers, by placing a - # "trigger" subsection in the module configuration. + # Triggers for specific modules. These are *not* in the + # module configuration because they are global to all + # instances of the module. You can have module-specific + # triggers, by placing a `trigger` subsection in the module + # configuration. Not all modules support a `trigger` + # subsection. # modules { # @@ -171,22 +173,22 @@ trigger { # # open:: A new connection to the directory has been opened. # - open = "${snmptrap}::serverModuleConnectionUp ${args}" + open = `${snmptrap}::serverModuleConnectionUp ${args}` # # close:: A connection to the directory has been closed. # - close = "${snmptrap}::serverModuleConnectionDown ${args}" + close = `${snmptrap}::serverModuleConnectionDown ${args}` # # min:: Connection was released too quickly. # - min = "${snmptrap}::serverModuleConnectionReservedPeriodMin ${args}" + min = `${snmptrap}::serverModuleConnectionReservedPeriodMin ${args}` # # max:: Connection was held for too long. # - max = "${snmptrap}::serverModuleConnectionReservedPeriodMax ${args}" + max = `${snmptrap}::serverModuleConnectionReservedPeriodMax ${args}` } # @@ -201,27 +203,27 @@ trigger { # # open:: A new connection to the database has been opened. # - open = "${snmptrap}::serverModuleConnectionUp ${args}" + open = `${snmptrap}::serverModuleConnectionUp ${args}` # # close:: A connection to the database has been closed. # - close = "${snmptrap}::serverModuleConnectionDown ${args}" + close = `${snmptrap}::serverModuleConnectionDown ${args}` # # fail:: Failed to open a new connection to the database. # - fail = "${snmptrap}::serverModuleConnectionFail ${args}" + fail = `${snmptrap}::serverModuleConnectionFail ${args}` # # min:: A connection was released too quickly. # - min = "${snmptrap}::serverModuleConnectionReservedPeriodMin ${args}" + min = `${snmptrap}::serverModuleConnectionReservedPeriodMin ${args}` # # max:: A connection was held for too long. # - max = "${snmptrap}::serverModuleConnectionReservedPeriodMax ${args}" + max = `${snmptrap}::serverModuleConnectionReservedPeriodMax ${args}` } # diff --git a/src/lib/server/trigger.c b/src/lib/server/trigger.c index 6f70c88605..f9265b8e44 100644 --- a/src/lib/server/trigger.c +++ b/src/lib/server/trigger.c @@ -34,6 +34,7 @@ RCSID("$Id$") #include #include #include +#include #include @@ -132,82 +133,12 @@ bool trigger_enabled(void) } typedef struct { - char *command; //!< Name of the trigger. - xlat_exp_head_t *xlat; //!< xlat representation of the trigger args. fr_value_box_list_t args; //!< Arguments to pass to the trigger exec. fr_value_box_list_t out; //!< result of the xlap (which we ignore) unlang_result_t result; //!< the result of expansion - - fr_exec_state_t exec; //!< Used for asynchronous execution. - fr_time_delta_t timeout; //!< How long the trigger has to run. + int exec_status; } fr_trigger_t; -static unlang_action_t trigger_done(request_t *request, void *rctx) -{ - fr_trigger_t *trigger = talloc_get_type_abort(rctx, fr_trigger_t); - - if (trigger->exec.status == 0) { - RDEBUG2("Trigger \"%s\" done", trigger->command); - return UNLANG_ACTION_CALCULATE_RESULT; - } - - RERROR("Trigger \"%s\" failed", trigger->command); - - return UNLANG_ACTION_CALCULATE_RESULT; -} - -static unlang_action_t trigger_resume(request_t *request, void *rctx) -{ - fr_trigger_t *trigger = talloc_get_type_abort(rctx, fr_trigger_t); - - if (fr_value_box_list_empty(&trigger->args)) { - RERROR("Failed trigger \"%s\" - did not expand to anything", trigger->command); - return UNLANG_ACTION_FAIL; - } - - /* - * fr_exec_oneshot just calls request_resume when it's - * done. - */ - if (fr_exec_oneshot(request, &trigger->exec, request, - &trigger->args, - NULL, false, true, - false, - false, NULL, trigger->timeout) < 0) { - fail: - RPERROR("Failed running trigger \"%s\"", trigger->command); - return UNLANG_ACTION_FAIL; - } - - /* - * Swap out the repeat function so that when we're - * resumed the code path in the interpreter pops - * the frame. If we don't do this trigger_run just - * gets called repeatedly. - */ - if (unlang_function_repeat_set(request, trigger_done) < 0) { - fr_exec_oneshot_cleanup(&trigger->exec, SIGKILL); - goto fail; - } - - return UNLANG_ACTION_YIELD; -} - -static unlang_action_t trigger_run(request_t *request, void *uctx) -{ - fr_trigger_t *trigger = talloc_get_type_abort(uctx, fr_trigger_t); - - RDEBUG("Running trigger \"%s\"", trigger->command); - - if (unlang_xlat_push(request, NULL, &trigger->args, request, - trigger->xlat, UNLANG_SUB_FRAME) < 0) { - return UNLANG_ACTION_CALCULATE_RESULT; - } - - return UNLANG_ACTION_PUSHED_CHILD; -} - - /** Execute a trigger - call an executable to process an event * * @note Calls to this function will be ignored if #trigger_exec_init has not been called. @@ -239,6 +170,11 @@ int trigger_exec(unlang_interpret_t *intp, fr_trigger_t *trigger; ssize_t slen; + fr_event_list_t *el; + xlat_exp_head_t *xlat; + tmpl_rules_t t_rules; + fr_token_t quote; + /* * noop if trigger_exec_init was never called, or if * we're just checking the configuration. @@ -373,83 +309,128 @@ int trigger_exec(unlang_interpret_t *intp, MEM(trigger = talloc_zero(request, fr_trigger_t)); fr_value_box_list_init(&trigger->args); fr_value_box_list_init(&trigger->out); - trigger->command = talloc_strdup(trigger, value); - trigger->timeout = fr_time_delta_from_sec(5); /* @todo - Should be configurable? */ - if (value[0] != '/') { - slen = unlang_xlat_push_string(trigger, &trigger->result, &trigger->out, request, value); - if (slen < 0) goto parse_error; /* can't be zero! */ + el = unlang_interpret_event_list(request); + if (!el) el = main_loop_event_list(); + + t_rules = (tmpl_rules_t) { + .attr = { + .dict_def = request->local_dict, /* we can use local attributes */ + .list_def = request_attr_request, + }, + .xlat = { + .runtime_el = el, + }, + .at_runtime = true, + }; + + quote = cf_pair_value_quote(cp); + + /* + * Parse the xlat as appropriate. + */ + if (quote != T_BACK_QUOTED_STRING) { + slen = xlat_tokenize(trigger, &xlat, &FR_SBUFF_IN(value, talloc_array_length(value) - 1), NULL, &t_rules); } else { - slen = xlat_tokenize_argv(trigger, &trigger->xlat, - &FR_SBUFF_IN(trigger->command, talloc_array_length(trigger->command) - 1), - NULL, NULL, &(tmpl_rules_t) { - .attr = { - .dict_def = request->local_dict, - .list_def = request_attr_request, - .allow_unresolved = false, - }, - }, true); - if (slen <= 0) { - char *spaces, *text; - - parse_error: - fr_canonicalize_error(trigger, &spaces, &text, slen, trigger->command); - - cf_log_err(cp, "Failed parsing trigger command"); - cf_log_err(cp, "%s", text); - cf_log_perr(cp, "%s^", spaces); + slen = xlat_tokenize_argv(trigger, &xlat, &FR_SBUFF_IN(value, talloc_array_length(value) - 1), NULL, NULL, &t_rules, true); + } + if (slen <= 0) { + char *spaces, *text; + + fr_canonicalize_error(trigger, &spaces, &text, slen, value); + + cf_log_err(cp, "Failed parsing trigger command"); + cf_log_err(cp, "%s", text); + cf_log_perr(cp, "%s^", spaces); + + talloc_free(request); + talloc_free(spaces); + talloc_free(text); + return -1; + } + fr_assert(xlat != NULL); + + if (quote != T_BACK_QUOTED_STRING) { + if (unlang_xlat_push(trigger, &trigger->result, &trigger->out, request, xlat, true) < 0) { + fail_expand: + DEBUG("Failed expanding trigger - %s", fr_strerror()); talloc_free(request); - talloc_free(spaces); - talloc_free(text); return -1; } + } else { + tmpl_t *vpt; + + /* + * We need back-ticks, because it's just so much + * easier than anything else. + * + * @todo - define %exec.string() function, which + * splits the string, and THEN expands it. That + * would be much simpler than this stuff. + */ + MEM(vpt = tmpl_alloc(trigger, TMPL_TYPE_EXEC, quote, value, talloc_array_length(value))); + tmpl_set_xlat(vpt, xlat); + + if (unlang_tmpl_push(trigger, &trigger->out, request, vpt, + &(unlang_tmpl_args_t) { + .type = UNLANG_TMPL_ARGS_TYPE_EXEC, + .exec = { + .status_out = &trigger->exec_status, + .timeout = fr_time_delta_from_sec(1), + }, + }) < 0) { + goto fail_expand; + } } /* - * If we're not running it locally use the default - * interpreter for the thread. + * An interpreter was passed in, we can run the expansion + * asynchronously in that interpreter. And then the + * worker cleans up the detached request. */ if (intp) { unlang_interpret_set(request, intp); - if (unlang_subrequest_child_push_and_detach(request) < 0) { - error: - PERROR("Running trigger failed"); + + /* + * Don't allow the expansion to run for a long time. + * + * @todo - make the timeout configurable. + */ + if (unlang_interpret_set_timeout(request, fr_time_delta_from_sec(1)) < 0) { + DEBUG("Failed setting timeout on trigger %s", value); talloc_free(request); return -1; } - } - if (trigger->xlat) { - if (xlat_finalize(trigger->xlat, intp ? unlang_interpret_event_list(request) : main_loop_event_list()) < 0) { - fr_strerror_const("Failed performing ephemeral instantiation for xlat"); + if (unlang_subrequest_child_push_and_detach(request) < 0) { + PERROR("Running trigger failed"); talloc_free(request); return -1; } - - if (unlang_function_push(request, - trigger_run, - trigger_resume, - NULL, 0, - UNLANG_TOP_FRAME, - trigger) < 0) goto error; - } - - if (!intp) { + } else { /* - * Wait for the exec to finish too, - * so where there are global events - * the request processes don't race - * with something like the server - * shutting down. + * No interpreter, we MUST be running from the + * main loop. We then run the expansion + * synchronously. This allows the expansion / + * notification to finish before the server shuts + * down. + * + * If the expansion was async, then it may be + * possible for the server to exit before the + * expansion finishes. Arguably the worker + * thread should ensure that the server doesn't + * exit until all requests have acknowledged that + * they've exited. + * + * But those exits may be advisory. i.e. "please + * finish the request". This one here is + * mandatary to finish before the server exits. */ unlang_interpret_synchronous(NULL, request); talloc_free(request); } - /* - * Otherwise the worker cleans up the request request. - */ return 0; } diff --git a/src/lib/unlang/xlat.c b/src/lib/unlang/xlat.c index e71db732f9..a90773df51 100644 --- a/src/lib/unlang/xlat.c +++ b/src/lib/unlang/xlat.c @@ -305,46 +305,6 @@ int unlang_xlat_push_node(TALLOC_CTX *ctx, unlang_result_t *p_result, fr_value_b return unlang_xlat_push_internal(ctx, p_result, out, request, NULL, node, UNLANG_TOP_FRAME); } -/** Tokenize a string,and push the resulting xlat onto the stack for evaluation - * - * @param[in] ctx To allocate value boxes and values in. - * @param[out] p_result If set, and execution succeeds, true will be written - * here. If execution fails, false will be written. - * @param[out] out Where to write the result of the expansion. - * @param[in] request to push xlat onto. - * @param[in] string to tokenize and evaluate. - * @return - * - 0 on success. - * - -1 on failure. - */ -int unlang_xlat_push_string(TALLOC_CTX *ctx, unlang_result_t *p_result, fr_value_box_list_t *out, - request_t *request, char const *string) -{ - fr_slen_t slen; - xlat_exp_head_t *xlat; - fr_event_list_t *el; - - el = unlang_interpret_event_list(request); - if (!el) el = main_loop_event_list(); - - slen = xlat_tokenize(ctx, &xlat, &FR_SBUFF_IN(string, strlen(string)), NULL, - &(tmpl_rules_t) { - .attr = { - .dict_def = request->local_dict, /* we can use local attributes */ - .list_def = request_attr_request, - }, - .xlat = { - .runtime_el = el, - }, - .at_runtime = true, - }); - if (slen <= 0) { - return slen; - } - - return unlang_xlat_push_internal(ctx, p_result, out, request, xlat, xlat_exp_head(xlat), UNLANG_TOP_FRAME); -} - static unlang_action_t unlang_xlat_repeat(unlang_result_t *p_result, request_t *request, unlang_stack_frame_t *frame) { unlang_frame_state_xlat_t *state = talloc_get_type_abort(frame->state, unlang_frame_state_xlat_t); diff --git a/src/lib/unlang/xlat.h b/src/lib/unlang/xlat.h index 15a882b6df..2c9c45e392 100644 --- a/src/lib/unlang/xlat.h +++ b/src/lib/unlang/xlat.h @@ -506,10 +506,6 @@ int unlang_xlat_push(TALLOC_CTX *ctx, unlang_result_t *p_result, fr_value_box_l request_t *request, xlat_exp_head_t const *head, bool top_frame) CC_HINT(warn_unused_result); -int unlang_xlat_push_string(TALLOC_CTX *ctx, unlang_result_t *p_result, fr_value_box_list_t *out, - request_t *request, char const *string) - CC_HINT(warn_unused_result); - int unlang_xlat_eval(TALLOC_CTX *ctx, fr_value_box_list_t *out, request_t *request, xlat_exp_head_t const *head) CC_HINT(warn_unused_result); -- 2.47.3