header_rewrite: add POST_REMAP_HOOK support - #13426
Conversation
Add the POST_REMAP_HOOK keyword to the config parser so rulesets can target the real post-remap hook. Parser-only; runtime wiring follows.
Map the POST_REMAP event in the continuation, gather the post-remap request headers, and allow operators/conditions on the hook. Enables the global-plugin deployment model where rules act after remapping. Adds an end-to-end autest and documents the hook.
The earlier wording sold the hook as the window before the origin request, which SEND_REQUEST_HDR_HOOK already covers. The window that matters is before the cache lookup, and the gap is specific to global (plugin.config) rulesets: the read-request hooks run pre-remap, SEND_REQUEST_HDR_HOOK runs after the lookup and only on a forward to origin, and REMAP_PSEUDO_HOOK closes the window for remap.config only.
The old test set a header at POST_REMAP and asserted it on the request
forwarded to origin. Swapping the rule to SEND_REQUEST_HDR_HOOK passed
just as well, so the test demonstrated nothing that existing hooks
couldn't already do.
The rule now records the remapped host and echoes it into the client
response, and the replay adds a cache-hit transaction. That combination
pins both properties the hook uniquely provides:
- Fires on a cache hit, where no request goes to origin, so
SEND_REQUEST_HDR_HOOK never runs.
- Sees the remapped request, so the pre-remap hooks record the wrong
host.
Verified by rerunning the test with the rule moved to
SEND_REQUEST_HDR_HOOK, READ_REQUEST_HDR_HOOK, and
READ_REQUEST_PRE_REMAP_HOOK; each one fails.
Note %{CLIENT-URL} is the pristine URL by design, so the rule uses
%{URL:HOST} to read the in-flight request.
bneradt
left a comment
bneradt
left a comment
There was a problem hiding this comment.
Codex had one comment. Otherwise the PR looks good to me.
|
Adding TS_HTTP_POST_REMAP_HOOK only to Statement::initialize_hooks() does not affect derived operators that override initialize_hooks(). In particular, OperatorSetPluginCntl and OperatorSetStateFlag/Int8/Int16 each say they should be allowed everywhere, but their explicit hook lists omit the new hook. The session-state operators reuse those state classes, so they are affected too. As a result, a POST_REMAP_HOOK ruleset using set-plugin-cntl, set-state-, or set-session- is rejected during configuration parsing. Could these overrides add TS_HTTP_POST_REMAP_HOOK (or delegate to Statement::initialize_hooks()), with a configuration-level test covering one of these operators? |
3 participants
header_rewritecan attach rulesets to most transaction hooks, but not toTS_HTTP_POST_REMAP_HOOK. For a global (plugin.config) configuration, thatleaves no hook that sees the remapped request before the cache lookup:
READ_REQUEST_HDR_HOOK/READ_REQUEST_PRE_REMAP_HOOKrun before remapping,so they only ever see the pristine request.
REMAP_PSEUDO_HOOKsees the remapped request before the lookup, but is validonly in a remap context.
SEND_REQUEST_HDR_HOOKsees the remapped request, but fires after the lookupand only when the request is forwarded to an origin. It cannot influence the
lookup, and it never runs on a cache hit.
That window is where anything feeding the cache lookup has to run. The
cachekeyplugin registers
TS_HTTP_POST_REMAP_HOOKfor exactly this reason(
plugins/cachekey/plugin.cc:114): its remap-time path (TSRemapDoRemap)covers the per-remap case, and a global instance needs the remapped request
before
TSCacheUrlSetis consumed by the lookup.This is the hook a planned
header_rewritecache-key operator needs, for thesame reason: setting the cache key is only meaningful between remapping and the
cache lookup, and
SEND_REQUEST_HDR_HOOKis already too late.This PR adds a
POST_REMAP_HOOKhook condition and wires it end to end:POST_REMAP_HOOKkeyword in the parser.Covered by a parser unit test and an end-to-end autest.