FreeRADIUS 3.2: Add fail_on_errors option to rlm_sqlippool to make sure it returns 'fail' on queries

[AlexAT]

rlm_sqlippool in FreeRADIUS 3.2 returns 'noop' when pool IP allocation queries fail, while it should return failure.

This makes it impossible to use rlm_sqlippool in redundant blocks for i.e. clustered SQL server redundancy or handling database failures otherwise.

This PR adds an extra boolean option called 'fail_on_errors' (disabled by default) which alters module behavior to return 'fail' instead of 'noop' in case some query fails.

[claude]

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

[alandekok]

? The redundant block falls through to the next item on fail. So I don't understand the problem you're seeing

[AlexAT]

Yes, but the problem is rlm_sqlippool returns 'fail' only in case 'handle' is null after query execution.
And this is not the case if query happened but failed with SQL error (this ends in non-zero return code with a valid handle).
And thus rlm_sqlippool continues executing and then ends at 'noop' after failing to find IP address if 'pool check' query does not fail or is not defined.

[AlexAT]

I.e. easy sample: only 'find' query is defined with calling stored procedure. The procedure fails with SQL error.

retval = data->sql_inst->sql_select_query(data->sql_inst, request, handle, expanded); results in non-zero code with valid handle. Then execution path returns a length of 0 to the find query caller, but here the check is just !handle to return 'fail'. Otherwise it proceeds, ends with no IP address found and returns noop.

[AlexAT]

Lemme 'outline' the flow a little bit.

	/*
	 *	If no IP found, look for a free one
	 */
	if (allocation_len == 0) {
		allocation_len = sqlippool_query1(allocation, sizeof(allocation),
						  inst->allocate_find, &handle,
						  inst, request, (char *) NULL, 0);

// HERE the sqlippool_query1 is called
////////////
// ...
	retval = data->sql_inst->sql_select_query(data->sql_inst, request, handle, expanded);
// THIS results in retval != 0 but valid handle
	talloc_free(expanded);

// SO we get there
	if ((retval != 0) || !*handle) {
		REDEBUG("database query error on '%s'", query);
// AND return length of 0
		return 0;
	}
// ...
///////////
// NOW sqlippool_query1 returns with result length of 0 and valid handle

		if (!handle) return RLM_MODULE_FAIL;
// AS handle is valid, RLM_MODULE_FAIL is not returned and the execution proceeds further
	}

// ...

// EVENTUALLY ending here (as the returned length remains 0)
		REDEBUG("IP address could not be allocated");
		return do_logging(request, inst->log_failed, RLM_MODULE_NOOP);

// ...

[AlexAT]

For the exact practical problem, sometimes pool transactions over InnoDB (especially Galera clusters) do deadlock.
This obviously causes SQL deadlock error to be reported to module.
But alas, module does not fail on it, returning noop instead and so redundant section never acts further than the first attempt.

[alandekok]

I really don't understand what you're getting at. The initial report says it returns "fail" and implies that's wrong, because v3 returns "noop". But the later comments say it returns "noop" when it should return "fail".

And the patch makes it return "fail".

Since this is v4 and it's unreleased, we don't need new configuration options to control changing behavior. If the module should do something different, then it should just be patched to be correct.

[AlexAT]

Um, @alandekok I've corrected the initial report. Seems like I have described it rather ambiguously.

This patch is against 3.2, not 4.0, and this new option brings behavior of 3.2 to at least something around what is expected and 4.0 does.