header_rewrite: reject bad run-plugin at config load

[bneradt]

This takes over #13425 from Leif Hedstrom (@zwoop), who authored the original change and the first commit in this PR.

A run-plugin whose target plugin failed to load left a null instance
that tripped a release assert and aborted the server on the first
request. Propagate the load failure as an exception so the rule is
rejected at config load time (a reload simply keeps the running
config), and guard exec() so a stray bad rule can never abort the
process.

[Copilot]

🟢 Ready to approve

The changes prevent a confirmed crash path by rejecting invalid run-plugin rules at config load time, add defensive runtime behavior, and include a targeted gold test to validate both startup and reload scenarios.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR hardens the header_rewrite plugin’s run-plugin operator so that a target plugin load failure is rejected during config parsing (startup/reload) instead of leaving a null instance that can later abort the process on first request, and adds an AuTest gold test to cover both startup-failure and reload-rejection behavior.

Changes:

• Add operator validity checking (Operator::is_valid()) and use it to reject invalid run-plugin operators during ruleset construction.
• Convert RuleSet::add_operator (and nested if handling) to use std::unique_ptr ownership to avoid manual deletes and make failure paths safer.
• Add an end-to-end gold test that verifies bad run-plugin configs fail cleanly at startup and are rejected on reload while keeping the prior running config.

File summaries

File	Description
tests/gold_tests/pluginTest/header_rewrite/header_rewrite_bad_run_plugin.test.py	New gold test covering startup rejection and reload rejection for a bad run-plugin target.
plugins/header_rewrite/ruleset.h	Update RuleSet::add_operator overload to take std::unique_ptr<Operator> for RAII ownership.
plugins/header_rewrite/ruleset.cc	Build operators via std::unique_ptr, reject invalid operators at parse time, and transfer ownership safely into operator chains.
plugins/header_rewrite/operator.h	Introduce Operator::is_valid() hook for post-initialize validation.
plugins/header_rewrite/operators.h	Implement OperatorRunPlugin::is_valid() to reflect whether the target plugin loaded successfully.
plugins/header_rewrite/operators.cc	Improve run-plugin error reporting with config location, remove request-time assert in favor of defensive exec, and simplify argv handling via std::vector.
plugins/header_rewrite/header_rewrite.cc	Use std::unique_ptr for nested if stack and transfer ownership into rule/operator chains without manual deletes.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 0
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟢 Ready to approve

The changes correctly prevent null run-plugin instances from reaching request-time execution, improve safety and ownership, and add targeted regression coverage.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟢 Ready to approve

It fixes a concrete crash path by rejecting invalid run-plugin rules at load time and includes a gold test that exercises both startup and reload scenarios.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[cmcfarlen]

The core change looks right. Converting initialize() failure into an exception routes it into the existing catch (std::runtime_error &) at header_rewrite.cc:413, so the rule is rejected at config load time and a reload simply keeps the running config. The unique_ptr conversions are real leak fixes rather than style: the old RuleSet::add_operator depended on an explicit delete on the is_hook_valid failure path, and the if_stack unwinding loop becomes unnecessary rather than merely correct.

Replacing the TSReleaseAssert in exec() with a guarded early return is sound belt-and-braces, and returning true is the right choice since remap pseudo-hook operators must return true (operator.h:92-94).

One thing to flag for the 10.2.0 backport. This PR is tracked at For v10.2.0 in the ATS v10.2.x project, but the new test calls Test.AddConfigReload(), which lives in tests/gold_tests/autest-site/config_reload.test.ext — a file that exists only on master. It is not on 10.2.x, so the test would fail to load there with AttributeError-style breakage at test collection time.

The good news is that this looks cheap to resolve, because only the autest extension is missing — every runtime dependency it needs is already on 10.2.x:

• traffic_ctl config reload already supports --monitor/-m, --token/-t, --initial-wait/-w, and --refresh-int/-r (src/traffic_ctl/traffic_ctl.cc:135-173).
• The get_reload_config_status RPC handler is present (src/mgmt/rpc/handlers/config/Configuration.cc:384, registered in RpcAdminPubHandlers.cc:43).
• jsonrpc.py resolves Request.get_reload_config_status dynamically via __getattr__, so it needs no explicit addition, and the CustomJSONRPCResponse tester already exists in jsonrpc_client.test.ext:164.

So the extension file should drop onto 10.2.x and work. The wrinkle is that the commit which introduced it, #13075 (70859231d8), is a 31-file change that also converts ~20 existing tests to the new helper and deletes wait_reload.sh — more churn than I would want to land on a release branch this close to the RC. Picking just tests/gold_tests/autest-site/config_reload.test.ext from that commit, without the test conversions, would give 10.2.x the API this test needs while leaving the existing passing tests alone.

Happy to do that as a small prerequisite pick if you and the RM agree that is the right shape.

[cmcfarlen]

Correction to my note above, and it changes the recommendation.

I said the autest extension was the only missing piece and suggested partial-picking just config_reload.test.ext. That was incomplete on two counts:

1. The monitor-mode plumbing I listed as "already on 10.2.x" came from ATS Configuration Reload with observability/tracing - Token model #12892 (the config-reload token model), which was indeed picked to 10.2.x as 5a9abc0630 via [10.2.x] ATS Configuration Reload with observability/tracing - Token model (#12892) #13354 — so that part is right, but it is not from Add Test.AddConfigReload() autest extension #13075.
2. Add Test.AddConfigReload() autest extension #13075 is not tests-only. It also widens ConfigContext::add_dependent_ctx() and ConfigReloadTask::add_child() with a filename parameter and updates callers such as SSLClientCoordinator.cc to pass ts::filename::SNI / ts::filename::SSL_MULTICERT. 10.2.x still has the older signatures (ConfigContext.h:173, ConfigReloadTrace.h:225), so that enrichment was never picked either.

So 10.2.x has the config-reload token-model feature but not its test-harness adoption or the filename enrichment that accompanies it. That looks like an oversight in the earlier backport rather than a deliberate scope decision, which makes picking #13075 in full the better fix — it restores parity with the feature already on the branch, the filename argument is defaulted and therefore source-compatible, and the ~20 converted tests move onto the same helper master has been exercising since June instead of leaving 10.2.x on divergent per-test reload logic.

Disregard the partial-pick suggestion; a normal cherry-pick -x of #13075 as the prerequisite is the cleaner path. Nothing here asks for a change to this PR — flagging only so the backport ordering is clear.