You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "bneradt (via GitHub)" <gi...@apache.org> on 2026/08/04 21:47:14 UTC
[PR] header_rewrite: reject bad run-plugin at config load [trafficserver]
bneradt opened a new pull request, #13493:
URL: https://github.com/apache/trafficserver/pull/13493
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.
The review follow-up replaces the exception propagation described in the original message with explicit operator validity and RAII ownership. This avoids introducing a new throw/catch path while preserving config-load rejection and defensive execution behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] header_rewrite: reject bad run-plugin at config load [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on PR #13493:
URL: https://github.com/apache/trafficserver/pull/13493#issuecomment-5196110740
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](https://github.com/orgs/apache/projects/573), 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] header_rewrite: reject bad run-plugin at config load [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt merged PR #13493:
URL: https://github.com/apache/trafficserver/pull/13493
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] header_rewrite: reject bad run-plugin at config load [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on PR #13493:
URL: https://github.com/apache/trafficserver/pull/13493#issuecomment-5196286560
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 #12892 (the config-reload token model), which was indeed picked to 10.2.x as `5a9abc0630` via #13354 — so that part is right, but it is not from #13075.
2. #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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] header_rewrite: reject bad run-plugin at config load [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on PR #13493:
URL: https://github.com/apache/trafficserver/pull/13493#issuecomment-5207868919
Cherry-picked to the 10.2.x branch as 1986d64d7e4e9fc082f70b866487fd4992d97f20 for the 10.2.0 release.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org