From: Arran Cudbard-Bell Date: Fri, 9 May 2025 21:33:51 +0000 (-0600) Subject: Make the delay module transparent for rcodes X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=5a4e7121c80753784aa8537aa08533de1bb5ba9d;p=thirdparty%2Ffreeradius-server.git Make the delay module transparent for rcodes --- diff --git a/raddb/mods-available/delay b/raddb/mods-available/delay index 1164b2bff4f..96bc3c8938e 100644 --- a/raddb/mods-available/delay +++ b/raddb/mods-available/delay @@ -69,6 +69,24 @@ delay { # The default is `no`, which means that. # # relative = no + + # + # rcode:: Which rcode the delay module should return when it resumes + # this can be useful for detecting whether a `timeout { ... }` section + # expired, whilst the delay module was waiting. + # + # The default is `notset`, which means that the delay module will be + # transparent and not alter the section rcode. + # + # [NOTE] + # ==== + # The default section `rcode` in many places is `fail`. Care should be + # taken to set the `rcode` for the section a delay module appears in, if + # this configuration item is not set, and no other modules are present + # in the section. + # ==== + # +# rcode = 'notset' } # diff --git a/src/lib/server/rcode.c b/src/lib/server/rcode.c index fabba4793de..3293ed88cf7 100644 --- a/src/lib/server/rcode.c +++ b/src/lib/server/rcode.c @@ -39,9 +39,10 @@ fr_table_num_sorted_t const rcode_table[] = { { L("invalid"), RLM_MODULE_INVALID }, { L("noop"), RLM_MODULE_NOOP }, { L("notfound"), RLM_MODULE_NOTFOUND }, + { L("notset"), RLM_MODULE_NOT_SET }, { L("ok"), RLM_MODULE_OK }, { L("reject"), RLM_MODULE_REJECT }, { L("timeout"), RLM_MODULE_TIMEOUT }, - { L("updated"), RLM_MODULE_UPDATED }, + { L("updated"), RLM_MODULE_UPDATED } }; size_t rcode_table_len = NUM_ELEMENTS(rcode_table); diff --git a/src/lib/unlang/module.c b/src/lib/unlang/module.c index 3298346cfcd..5b50d837676 100644 --- a/src/lib/unlang/module.c +++ b/src/lib/unlang/module.c @@ -677,9 +677,10 @@ static unlang_action_t unlang_module_resume(rlm_rcode_t *p_result, request_t *re request->module = state->previous_module; break; - case UNLANG_ACTION_EXECUTE_NEXT: /* Not valid */ - fr_assert(0); - *p_result = RLM_MODULE_FAIL; + /* + * Module indicates we shouldn't process its rcode + */ + case UNLANG_ACTION_EXECUTE_NEXT: break; } @@ -941,9 +942,10 @@ static unlang_action_t unlang_module(rlm_rcode_t *p_result, request_t *request, *p_result = RLM_MODULE_FAIL; break; + /* + * Module indicates we shouldn't process its rcode + */ case UNLANG_ACTION_EXECUTE_NEXT: - fr_assert(0); - *p_result = RLM_MODULE_FAIL; break; } diff --git a/src/lib/unlang/timeout.c b/src/lib/unlang/timeout.c index 285d661597e..d9e6b6e2f65 100644 --- a/src/lib/unlang/timeout.c +++ b/src/lib/unlang/timeout.c @@ -112,7 +112,7 @@ static unlang_action_t unlang_timeout_resume_done(UNUSED rlm_rcode_t *p_result, * * Unless the next instruction is a "catch timeout", in which case we skip it. */ - if (!state->fired) return UNLANG_ACTION_CALCULATE_RESULT; + if (!state->fired) return UNLANG_ACTION_EXECUTE_NEXT; /* Don't modify the return code*/ RETURN_MODULE_TIMEOUT; } diff --git a/src/lib/unlang/unlang_priv.h b/src/lib/unlang/unlang_priv.h index 3812bc4deb5..1655e498bc3 100644 --- a/src/lib/unlang/unlang_priv.h +++ b/src/lib/unlang/unlang_priv.h @@ -570,7 +570,7 @@ static inline void frame_next(unlang_stack_t *stack, unlang_stack_frame_t *frame frame_cleanup(frame); frame->instruction = frame->next; - if (!frame->instruction) return; + if (!frame->instruction) return; /* No siblings, need to pop instead */ frame->next = frame->instruction->next; diff --git a/src/modules/rlm_delay/rlm_delay.c b/src/modules/rlm_delay/rlm_delay.c index 679be93d1e1..0e8bcd3fdee 100644 --- a/src/modules/rlm_delay/rlm_delay.c +++ b/src/modules/rlm_delay/rlm_delay.c @@ -26,15 +26,18 @@ RCSID("$Id$") #include #include +#include #include #include #include #include +#include typedef struct { tmpl_t *delay; //!< How long we delay for. bool relative; //!< Whether the delay is relative to the start of request processing. bool force_reschedule; //!< Whether we should force rescheduling of the request. + rlm_rcode_t rcode; //!< The return code to use when the delay is complete. } rlm_delay_t; /* @@ -44,6 +47,8 @@ static const conf_parser_t module_config[] = { { FR_CONF_OFFSET_HINT_TYPE("delay", FR_TYPE_TIME_DELTA, rlm_delay_t, delay) }, { FR_CONF_OFFSET("relative", rlm_delay_t, relative), .dflt = "no" }, { FR_CONF_OFFSET("force_reschedule", rlm_delay_t, force_reschedule), .dflt = "no" }, + { FR_CONF_OFFSET("rcode", rlm_delay_t, rcode), + .func = cf_table_parse_int, .uctx = &(cf_table_parse_ctx_t){ .table = rcode_table, .len = &rcode_table_len }, .dflt = "notset" }, CONF_PARSER_TERMINATOR }; @@ -121,6 +126,7 @@ static int delay_add(request_t *request, fr_time_t *resume_at, fr_time_t now, static unlang_action_t mod_delay_return(rlm_rcode_t *p_result, module_ctx_t const *mctx, request_t *request) { rlm_delay_retry_t *yielded = talloc_get_type_abort(mctx->rctx, rlm_delay_retry_t); + rlm_delay_t *inst = talloc_get_type_abort_const(mctx->mi->data, rlm_delay_t); /* * Print how long the delay *really* was. @@ -128,7 +134,11 @@ static unlang_action_t mod_delay_return(rlm_rcode_t *p_result, module_ctx_t cons RDEBUG3("Request delayed by %pV", fr_box_time_delta(fr_time_sub(fr_time(), yielded->when))); talloc_free(yielded); - RETURN_MODULE_OK; + /* + * Be transparent, don't alter the rcode + */ + if (inst->rcode == RLM_MODULE_NOT_SET) return UNLANG_ACTION_EXECUTE_NEXT; + RETURN_MODULE_RCODE(inst->rcode); } static unlang_action_t CC_HINT(nonnull) mod_delay(rlm_rcode_t *p_result, module_ctx_t const *mctx, request_t *request) diff --git a/src/tests/keywords/if-elsif-rcode-resume b/src/tests/keywords/if-elsif-rcode-resume new file mode 100644 index 00000000000..6954d482d51 --- /dev/null +++ b/src/tests/keywords/if-elsif-rcode-resume @@ -0,0 +1,26 @@ +# PRE: if if-else + +# This is a regression test. We saw the request->rcode being lost after the first condition + +noop +if (ok || notfound) { + test_fail +} elsif (!noop) { + test_fail +} elsif (noop) { + # Check nothing weird happens + reschedule_notfound +} + +if (!notfound) { + test_fail +} + +notfound +if (%expr.rcode('ok') || %expr.rcode('updated')) { + test_fail +} elsif (!%expr.rcode('notfound')) { + test_fail +} + +success diff --git a/src/tests/keywords/if-rcode b/src/tests/keywords/if-rcode new file mode 100644 index 00000000000..1289b80e986 --- /dev/null +++ b/src/tests/keywords/if-rcode @@ -0,0 +1,18 @@ +noop { + # 0 gives greatest chance of overwrite + noop = 1 +} + +# Verify that checking the rcode doesn't _alter_ the rcode +if (fail) { + test_fail +} +elsif (ok || updated) { + test_fail +} +# rcode should still be noop +elsif (noop) { + success +} else { + test_fail +} diff --git a/src/tests/keywords/parallel-yield b/src/tests/keywords/parallel-yield index e31d95bf801..dfb95dc9068 100644 --- a/src/tests/keywords/parallel-yield +++ b/src/tests/keywords/parallel-yield @@ -4,7 +4,10 @@ # Ensure if one module yields, the rest execute parallel { - reschedule + group { + ok # Set the rcode for the section, else it defaults to fail + reschedule + } group { parent.request.Filter-Id := 'foo' } diff --git a/src/tests/keywords/radius.conf b/src/tests/keywords/radius.conf index 06949e70193..3adc4da6ccd 100644 --- a/src/tests/keywords/radius.conf +++ b/src/tests/keywords/radius.conf @@ -9,6 +9,14 @@ modules { force_reschedule = yes } + # + # Reschedule the request, then return notfound + # + delay reschedule_notfound { + force_reschedule = yes + rcode = "notfound" + } + delay delay_10s { delay = 10 } diff --git a/src/tests/keywords/timeout-transparent-rcode b/src/tests/keywords/timeout-transparent-rcode new file mode 100644 index 00000000000..160008cbdc5 --- /dev/null +++ b/src/tests/keywords/timeout-transparent-rcode @@ -0,0 +1,20 @@ +# +# PRE: timeout +# + +# +# If the timeout doesn't fire, the rcode should not change, and the +# timeout block should be transparent. +# +timeout 1s { + if (ok) { + test_fail + } elsif (reject) { + notfound + } +} +if (!notfound) { + test_fail +} + +success