virtualhost.yaml with remap rules

[serrislew]

V2 of #12669 but including remap.yaml (#12997)

• Resolves to a single virtualhost entry
  • Remap rule logic remains the same. Order ranking still applies (i.e. rules defined first have priority)
  • Virtualhost remap rules have priority. If no remap rule matches or virtualhost is found, existing remap.yaml or .config is then considered
• Virtualhost domains support exact and wildcard (only single left-most "*") matching
• Supports traffic_ctl config reload of single virtualhost entry using reload directives
  • $ traffic_ctl config reload -D virtualhost.id=foo
  • Uses token model reload

[brbzull0]

I was talking with @serrislew about some parts of this PR, specially related to the interaction with the reload handler. I think #13110 is the plumbing that the id base reloading could benefit from.

[Copilot]

Pull request overview

This PR introduces virtualhost.yaml as a new configuration file that maps request hostnames (exact and wildcard) to a single virtual host entry, enabling per-virtualhost remap rule overrides (in remap.yaml format) with support for granular reload via reload directives / JSONRPC.

Changes:

• Add virtualhost.yaml configuration + record proxy.config.virtualhost.filename, default config stub, and admin-guide documentation.
• Integrate virtualhost lookup into HttpSM::do_remap_request() so virtualhost remap rules are attempted before global remap rules, with fallback to the global remap table when no match is found.
• Extend remap.yaml handling so UrlRewrite / remap parser can build tables from an inline YAML node (used by virtualhost remap blocks) and enable reload-directive routing to the virtualhost handler.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
tests/gold_tests/jsonrpc/config_reload_rpc.test.py	Updates JSONRPC reload-directive test to expect virtualhost directives to route to the handler.
src/records/RecordsConfig.cc	Adds proxy.config.virtualhost.filename dynamic record.
src/proxy/VirtualHost.cc	Implements virtualhost config loading, domain matching, per-entry reload, and config registry registration.
src/proxy/ReverseProxy.cc	Calls VirtualHost::startup() during reverse proxy initialization.
src/proxy/http/remap/UrlRewrite.cc	Factors out load_table() and allows table building from an inline YAML node.
src/proxy/http/remap/RemapYamlConfig.cc	Adds overloads to parse inline remap YAML sequences into remap tables.
src/proxy/http/HttpSM.cc	Adds per-transaction virtualhost entry selection and remap table override/fallback logic.
src/proxy/CMakeLists.txt	Adds VirtualHost.cc to the proxy library build.
include/tscore/Filenames.h	Adds virtualhost.yaml to known filenames.
include/proxy/VirtualHost.h	Declares virtualhost config/entry types and VirtualHost API.
include/proxy/http/remap/UrlRewrite.h	Declares load_table() and updated BuildTable() signature.
include/proxy/http/remap/RemapYamlConfig.h	Declares new inline YAML parsing overloads.
include/proxy/http/HttpSM.h	Adds virtualhost state to HttpSM and declares helper method.
doc/admin-guide/files/virtualhost.yaml.en.rst	New documentation for virtualhost.yaml, evaluation order, and granular reload.
doc/admin-guide/files/records.yaml.en.rst	Documents the new proxy.config.virtualhost.filename record.
doc/admin-guide/files/index.en.rst	Adds virtualhost.yaml to the admin-guide files index.
configs/virtualhost.yaml.default	Adds a default/example virtualhost.yaml template.

[brbzull0]

Looks good to me.
I'll approve it but I'd wait for @bryancall to do his review as well before merging it.

[bryancall]

I read the full diff across all 17 files and traced the new code against current master. The design work here is real: the domain resolution is deterministic and validated at load time, duplicate ids and duplicate exact and wildcard domains are all rejected across entries, wildcards are restricted to a single left-most *. form, and find_by_domain walks dot-suffixes longest to shortest so the documented "most specific wildcard wins" rule is actually what the code does. It follows the ConfigProcessor/ConfigRegistry idiom closely, it is opt-in and backward compatible, and it ships a full admin-guide page rather than a stub.

Requesting changes. Two blocking items, one of which means the PR cannot build against master as it stands.

Blocking 1: the inline remap parser clobbers the process-global IP allow accept-check flag

src/proxy/http/remap/RemapYamlConfig.cc:~1057

The new inline-node parser ends with IpAllow::enableAcceptCheck(bti->accept_check_p). IpAllow::accept_check_p is a single process-wide static (src/proxy/IPAllow.cc:75, setter at include/proxy/IPAllow.h:398-403), written from exactly three places: RemapConfig.cc:1555, the existing file parser at RemapYamlConfig.cc:1016, and now this.

The ordering makes it reachable. init_reverse_proxy() calls initial_table->load() first, and this PR appends VirtualHost::startup() at the very end of the same function, so every virtualhost table is parsed after the authoritative global table. build_virtualhost_entry to UrlRewrite::load_table to BuildTable to remap_parse_yaml constructs a fresh BUILD_TABLE_INFO whose accept_check_p defaults to true (include/proxy/http/remap/RemapConfig.h:67) and is only lowered by a rule inside that virtualhost.

So a global remap.yaml containing deactivate_filter: ip_allow, which is documented at remap.yaml.en.rst:1035, leaves accept_check_p false, and then the last virtualhost parsed resets it to true. A per-domain config silently rewrites process-wide IP access-control enforcement, last writer wins, at startup and on every granular reload. That is a security-relevant global being set from a per-domain scope.

Blocking 2: the refcount handling targets an ownership model that no longer exists

src/proxy/http/HttpSM.cc:4578-4633 and include/proxy/http/HttpSM.h:307-311

Master commit 709443e870 ("Fix race in remap table refcount during reload") removed UrlRewrite's RefCountObj base. On current master, include/proxy/http/remap/UrlRewrite.h has no acquire, release or RefCountObj; HttpSM.h:315 is std::shared_ptr<UrlRewrite> m_remap and every call site uses m_remap.get(). ReverseProxy.cc now exposes AtomicSharedPtr<UrlRewrite> rewrite_table with a custom deleter and a shutdown path that stores nullptr.

This PR still declares UrlRewrite *m_remap and calls acquire()/release() on UrlRewrite in four places, and rewrite_table.load()->acquire() is both a compile error and a null-dereference hazard during shutdown. GitHub reports the branch as conflicting, and the 15 green checks were run against the pre-709443e870 base, so they say nothing about the current state.

I want to flag that this is not a textual merge. The virtualhost table lifetime needs redesigning against the new shared-pointer ownership, and that redesign is worth doing deliberately, since getting per-domain table lifetime wrong under reload is exactly the class of race 709443e870 was fixing.

Should fix

src/proxy/VirtualHost.cc:385 The config is registered as ConfigSource::FileAndRpc, but the reload handler never reads ctx.supplied_yaml(). It reads only ctx.reload_directives() looking for id, then re-reads the on-disk file in both branches and calls ctx.complete(). Configuration.cc:300 rejects a pushed body only when the source is not FileAndRpc, and ConfigRegistry::execute_reload calls ctx.set_supplied_yaml(passed_config) before invoking the handler, with the registry comment at line 489 stating the contract that the handler is supposed to check it. So an admin_config_reload carrying virtualhost content is accepted, silently discarded, and answered with "Finished loading virtualhost config". IPAllow.cc:101 shows the deliberate alternative: register FileOnly with a comment saying why.

src/proxy/VirtualHost.cc:140 The YAML exception handler is catch (YAML::Exception const &ex) { Dbg(dbg_ctl_virtualhost, "Failed to parse virtualhost entry"); return false; }. Fixed string, ex bound and unused, no entry id, no line number. Every validation failure in convert<Entry>::decode (missing id, empty domains, malformed wildcard) and every failure in VirtualHostConfig::load (non-sequence top level, duplicate id, duplicate domain) is debug-only; only the unknown-key case uses Warning. The failure then surfaces as Fatal("failed to load %s") at startup with no cause attached. An operator with a typo in virtualhost.yaml gets a fatal exit and nothing to act on. RemapYamlConfig.cc routes the same class of failure through CfgLoadLog(ctx, DL_Error, ...) with ex.what(), which is the model to follow.

Smaller items

• src/proxy/VirtualHost.cc:72 std::set<std::string> valid_vhost_keys is a mutable namespace-scope global with external linkage in a .cc file. Should be const and in an anonymous namespace.
• src/proxy/VirtualHost.cc:257 Dbg(..., "%s", id.data()) is called on a std::string_view in three places. Not guaranteed NUL-terminated.
• include/proxy/VirtualHost.h:56-58 Entry::acquire()/release() hand-roll refcounting that Ptr<Entry> already provides, with dead if (self) null checks after a const_cast of this.
• src/proxy/VirtualHost.cc:148 UrlRewrite::load_table(const std::string &config_file_path, ...) is called with the virtualhost id as the config file path, which then flows into BuildTable as a path.
• src/proxy/http/HttpSM.cc:4578-4582 set_virtualhost_entry constructs VirtualHost::scoped_config, a config processor get plus a refcount, before the early-return checks, so every transaction pays for it even when no virtualhost is configured.
• doc/admin-guide/files/virtualhost.yaml.en.rst:212 The second example still has url: http:/foo.example.com/ with a single slash. Copilot raised this last round.
• configs/virtualhost.yaml.default:21 The shipped default uses - "*.com" as its wildcard example, which is an unfortunate thing to have someone uncomment.
• tests/gold_tests/jsonrpc/config_reload_rpc.test.py:440 The docstring of validate_directive_routed still says virtualhost is not registered and is rejected with 6010, contradicting the assertions directly below it.

Two things I initially suspected and then ruled out, so nobody re-litigates them: internal redirects do not leave a stale virtualhost table in a way that matters here, and the missing acl_filters section in the inline parser is not actually a gap.

[Copilot]

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.

Suppressed comments (6)

src/proxy/VirtualHost.cc:135

• For inline remap YAML, load_table() is passed conf.id as config_file_path. If remap rules use features that rely on an actual source path (e.g., include directives resolved relative to a file location, or path-based diagnostics), using the virtualhost id as a 'path' can produce incorrect behavior or confusing logs. Consider passing the actual virtualhost.yaml path (or a base directory) separately from a human-readable label, so inline parsing has a correct filesystem context.

  // Build UrlRewrite table for remap rules
  auto remap_node = node["remap"];
  if (remap_node) {
    auto table = std::make_unique<UrlRewrite>();
    if (!table->load_table(conf.id, &remap_node)) {
      Error("Failed to load remap rules for virtualhost '%s' at line %d", conf.id.c_str(), remap_node.Mark().line + 1);
      return false;
    }

src/proxy/VirtualHost.cc:316

• find_by_domain() allocates a temporary std::string{domain} to lowercase, and then performs map lookups using a char* key on std::unordered_map<std::string, ...> (which typically constructs a temporary std::string for lookup). This runs on every request, so the extra allocations can add measurable overhead. Consider lowercasing without allocating (if an overload exists) and/or enabling heterogeneous lookup (transparent hash/equal) so lookups can be done with std::string_view/char* without constructing a std::string.

  char lower_domain[TS_MAX_HOST_NAME_LEN + 1];
  ts::transform_lower(std::string{domain}, lower_domain);

  // Check for exact match domains first
  auto id = _exact_domains_to_id.find(lower_domain);
  if (id != _exact_domains_to_id.end()) {

tests/gold_tests/jsonrpc/config_reload_rpc.test.py:438

• The docstring for validate_directive_routed contradicts the updated test intent (virtualhost is now registered and should be routed/accepted). Update the docstring to reflect the new expected behavior so the test remains self-describing.

def validate_directive_routed(resp: Response):
    '''virtualhost is not registered — rejected with 6010'''
    result = resp.result

tests/gold_tests/jsonrpc/config_reload_rpc.test.py:449

• result.get('message', []) defaults to a list, but message is typically a string in JSON-RPC responses. Using a consistent default type (e.g., empty string) makes the intent clearer and avoids surprising truthiness/type behavior in validations.

    tasks = result.get('tasks', [])
    message = result.get('message', [])
    if tasks or message:

doc/admin-guide/files/virtualhost.yaml.en.rst:210

• The example URL is malformed (http:/... should be http://...). Since this is a copy/paste-able config example, it should be corrected to prevent user misconfiguration.

            url: http:/foo.example.com/

doc/admin-guide/files/virtualhost.yaml.en.rst:178

• Fix grammar: 'This rules translates' should be 'These rules translate'.

This rules translates in the following translation.

[Copilot]

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.

Suppressed comments (6)

Previously missed (4) — in code that hasn't changed since the last review.

src/proxy/VirtualHost.cc:426

• VirtualHost::reconfigure(std::string_view) logs id.data() with %s. std::string_view::data() is not guaranteed to be NUL-terminated, so this can over-read or print garbage for non-string-backed views. Use a length-limited format (%.*s).

  VirtualHost::scoped_config vhost_config;
  Dbg(dbg_ctl_virtualhost, "Reconfiguring virtualhost entry: %s", id.data());
  // Reconfigure all vhosts if id not specified

src/proxy/VirtualHost.cc:54

• valid_vhost_keys is a non-static namespace-scope variable, giving it external linkage. This is easy to avoid and prevents potential link-time name collisions. Make it static const (or place it in the existing anonymous namespace).

std::set<std::string> valid_vhost_keys = {"id", "domains", "remap"};

doc/admin-guide/files/virtualhost.yaml.en.rst:210

• Doc example has a malformed URL (http:/foo.example.com/), which is easy to copy/paste into configs and will fail to parse. Fix it to http://foo.example.com/.

         - type: map
           from:
            url: http:/foo.example.com/
           to:
            url: http://foo.origin.com/

doc/admin-guide/files/virtualhost.yaml.en.rst:214

• Grammar: "This rules translates in the following translation." should be corrected (it reads awkwardly and is duplicated wording).

This rules translates in the following translation.

tests/gold_tests/jsonrpc/config_reload_rpc.test.py:439

• The validator docstring still says virtualhost is "not registered" even though this test now expects the directive to be routed to the registered handler. Update the docstring to match the new behavior so failures are easier to interpret.

def validate_directive_routed(resp: Response):
    '''virtualhost is not registered — rejected with 6010'''
    result = resp.result

doc/admin-guide/files/virtualhost.yaml.en.rst:104

• The evaluation-order text mixes remap.config and remap.yaml as the global fallback, but the code falls back to the global remap table (which can come from either). Document the fallback as remap.yaml (if present) or remap.config (otherwise) consistently.

   b. Check for a wildcard domain match. If any virtual host wildcard domains define a subdomain of the request hostname in the form ``*.[domain]``, that virtual host is selected.
   c. If no matching virtual host exists, the request proceeds using global configuration (i.e :file:`remap.config`). Skip to step 3.
2. Within selected virtual host config, use virtual host remap rules.
   a. Follow existing :file:`remap.yaml` rules and matching orders. If a matching remap rule is found, that remap rule is selected.
3. If neither virtual host nor remap rules match, ATS falls back to global :file:`remap.yaml` resolution.

[Copilot]

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.

Suppressed comments (6)

Previously missed (4) — in code that hasn't changed since the last review.

src/proxy/VirtualHost.cc:433

• VirtualHost::reconfigure(std::string_view id) logs id.data() with %s. Since id is a std::string_view, it is not guaranteed to be NUL-terminated; passing id.data() to %s can read past the end of the view.

  VirtualHost::scoped_config vhost_config;
  Dbg(dbg_ctl_virtualhost, "Reconfiguring virtualhost entry: %s", id.data());
  // Reconfigure all vhosts if id not specified

doc/admin-guide/files/virtualhost.yaml.en.rst:102

• The evaluation order describes falling back to global config as remap.config, but this PR adds/remains compatible with remap.yaml as well. The docs should mention both remap.yaml and remap.config here to avoid implying YAML is skipped.

This issue also appears on line 103 of the same file.

1. Resolve to a single virtualhost
   a. Check for an exact domain match. If any virtual host lists the request hostname explicitly, that virtual host is selected.
   b. Check for a wildcard domain match. If any virtual host wildcard domains define a subdomain of the request hostname in the form ``*.[domain]``, that virtual host is selected.
   c. If no matching virtual host exists, the request proceeds using global configuration (i.e :file:`remap.config`). Skip to step 3.
2. Within selected virtual host config, use virtual host remap rules.

doc/admin-guide/files/virtualhost.yaml.en.rst:212

• The example URL has only a single slash after http: (http:/foo.example.com/), which is not a valid URL and will confuse users copying the snippet.

           from:
            url: http:/foo.example.com/
           to:
            url: http://foo.origin.com/

src/proxy/VirtualHost.cc:322

• find_by_domain() unnecessarily allocates a temporary std::string just to lower-case the input. ts::transform_lower already accepts std::string_view, so this can be done without an allocation on the hot path.

  char lower_domain[TS_MAX_HOST_NAME_LEN + 1];
  ts::transform_lower(std::string{domain}, lower_domain);

include/proxy/http/remap/UrlRewrite.h:84

• UrlRewrite.h now exposes APIs that take YAML::Node pointers, but this header neither includes <yaml-cpp/yaml.h> nor forward-declares YAML::Node. Any TU that includes UrlRewrite.h without already including yaml-cpp will fail to compile (unknown type YAML). Add a forward declaration (preferred, since this is only a pointer type) or include yaml-cpp in this header.

  bool load(ConfigContext ctx = {});

  bool load_table(const std::string &config_file_path, YAML::Node const *remap_node, ConfigContext ctx = {});

doc/admin-guide/files/virtualhost.yaml.en.rst:105

• This line says ATS falls back to global remap.yaml resolution, but if remap.yaml is absent ATS falls back to remap.config. Update the wording to reflect both global remap sources.

   a. Follow existing :file:`remap.yaml` rules and matching orders. If a matching remap rule is found, that remap rule is selected.
3. If neither virtual host nor remap rules match, ATS falls back to global :file:`remap.yaml` resolution.

[Copilot]

Copilot review overview

🔵 Needs a closer look

include/proxy/http/remap/UrlRewrite.h now references YAML::Node without a forward declaration or include, which can break compilation in translation units that include this header.

Review effort: Lite
Findings: 1 · 1

Open (2)

• UrlRewrite.h now exposes YAML::Node in the public API (load_table / BuildTable) but the…
• This change adds new runtime behavior (virtualhost selection + per-virtualhost remap table override…

Resolved since last review (2)

• load_table() sets _remap_yaml to true when remap_node is provided, but does not reset it…
• set_entry() deletes the existing entry (and erases its domain claims) before validating that the…

[bryancall]

❌ Request changes

Six of the nine must-fix items from my last round are cleanly done, the documentation fixes are in, and the granular reload race I raised is genuinely closed. What holds this up is that the same race survives one path over, in the function the new comment does not claim to cover, and that a missing or empty config file still replaces a live routing table and reports success.

A note on how I read this, since the branch was force-pushed: comparing the old head against the new one shows 618 files and +33687 lines, nearly all of it the rebase. I reconstructed the version I reviewed onto the current base so the two were directly comparable, which gives the honest delta: 7 files, +413/-127. Everything below is from that.

Verified: five of the six code asks

The unsupported domain forms now reject with the id, the domain and the line (VirtualHost.cc:100-104). You implemented exactly what I prescribed, and I need to correct my own ask rather than yours: I named four silently accepted forms and then prescribed a fix covering only the three that contain a *. foo[0-9]+.example.com has no *, so it is still accepted as a literal exact domain that no Host header can ever equal, loading clean and never firing. The admin guide still lists it under NOT Supported, so the contract and the code still disagree on that one case. That is my specification error, not a failure on your part, and I am raising it as non-blocking below rather than counting it against this round. An unknown key is now an error that names the key's own line (:76-83), with the id parsed first so the message can carry it. The _remap_yaml guard is back as ink_release_assert with a comment naming the silent-drop consequence (UrlRewrite.cc:842), and I confirmed it cannot misfire: the only node-passing caller sets the flag on a fresh instance one line earlier. ConfigContext is threaded through every load path, so duplicate id, claimed domain, malformed wildcard and YAML syntax errors all reach the reload task log. The duplicate reload is gone with RecRegisterConfigUpdateCb, and the trigger record still catches file changes.

Test 16 can now fail, which was the point of that ask. I checked the mutation rather than taking it on faith: delete the ENOENT guard in load_entry and the new task-log assertion fails; delete the one in load() and startup goes fatal and trips the exclusion. Both halves are covered.

Verified: the granular reload race is closed

reconfigure(id, ctx) now takes the lock first and reads the live config inside it, and the comment at :480-482 explains exactly why the read had to move. That is the bug I reported and it is properly fixed.

Still blocking: the full reload still reads outside the lock

The comment on the new mutex states the requirement precisely:

The single-entry reload is a read-copy-modify-publish against the live config, so the read and the publish must be atomic with respect to any other reload.

The granular path honours that. The full path does not. At VirtualHost.cc:448, config->load(ctx) performs the entire disk read and parse outside the lock, and only configProcessor.set at :455 is inside it.

So two full reloads can publish in the opposite order to their reads. An operator edits the file and reloads; that pass reads {a,b,c}. They edit again to delete b and reload; the second pass reads {a,c}, takes the lock and publishes. The first pass then finishes parsing, takes the lock, and publishes {a,b,c}. The deleted entry is live again, and nothing corrects it until the next reload.

I verified this is reachable rather than theoretical. ConfigRegistry::schedule_reload at ConfigRegistry.cc:497-503 does Ptr<ProxyMutex> mutex(new_ProxyMutex()); per call, so two reloads of the same key run concurrently on ET_TASK with nothing serializing them. That is the same reasoning the new comment gives for why the granular path needed the lock.

The fix is to widen the existing block to cover load(). Neither path is on the request path, so serialising the parse costs nothing. Whichever way you go, the comment at :48-55 should say which reloads it actually serialises, because as written it reads as though both are covered.

Still blocking: a missing or empty config replaces the live table and reports success

load() treats both an absent file (VirtualHost.cc:178-181) and a file that parses to Null (:184-188) as success: it logs, returns true, and reconfigure() then publishes a config whose _entries and both domain maps were cleared at the top of the function. The reload task completes, traffic_ctl config status shows success, and every per-domain remap table is gone. Requests that were routed per domain now fall through to the global table.

The empty-file case is the quieter of the two: its only diagnostic is a Dbg under a tag that is off in production.

What makes this a finding rather than a design choice is that you fixed the identical conditions on the single-entry path in this very commit. load_entry now returns DL_Error and false for both a missing file (:246-250) and an empty one (:254-258). The full reload, which replaces the entire table rather than one entry, was left reporting success.

Tolerating an absent file at startup is right. Tolerating it on a live reload is not, and the two are already distinguishable: ConfigContext is falsy at startup and truthy under a reload task. Refusing to swap in an empty table when a reload task is live, and saying how many live entries it would have dropped, would close both cases.

There is a related path worth checking while you are in there. proxy.config.virtualhost.filename is a trigger record, so traffic_ctl config set with a typo in the path fires a reload with no task attached, meaning every ctx.log and ctx.complete is discarded. The traffic_ctl call returns success, the status output is empty, and virtualhost routing is silently gone. The no-op context on the record path predates this pull request, but this is the commit that claims to fix reload diagnostics, and that is the path most likely to fire unattended.

Still blocking: _reload: {id: ""} silently performs a full reload

The handler rejects a non-scalar id (VirtualHost.cc:405-408), but "" is a scalar, so it passes. reconfigure(std::string_view id, ...) then hits if (id.empty()) at :466 and reinterprets the request as a full reload from disk.

An operator who asked to swap one entry gets the whole table rebuilt from whatever is currently on disk, including every unrelated half-finished edit in the file. The success message is "Reloaded virtualhost entry: " with a blank id, so the status output does not reveal what happened. A narrowly scoped request performing a wide scoped mutation is worth refusing rather than reinterpreting: fail the directive and say that omitting it entirely reloads the whole file.

Still blocking: nothing tests that a rejected reload leaves the old table serving

The two test files are disjoint in the least useful way. virtualhost_remap.test.py makes five requests and performs zero reloads. config_reload_rpc.test.py performs sixteen reloads and makes zero requests. So the distinction between the reload was refused and the previous routing table is still serving is never made anywhere in the tree.

That distinction is the entire subject of the reload-safety half of this commit. The three failure paths that are now asserted all fail in load_entry or in the handler, before the critical section is ever entered. The one failure that runs inside it, set_entry returning false on a domain conflict, has no test at all, in either file.

I should say plainly that the new request-path test would pass unchanged against the pre-fix code. Every config line in it is valid under both the old and the new validator, and no routing or matching logic changed in this delta. That is fine, it closes a different ask and it was the right thing to write, but it should not be read as evidence that this commit works.

Closing this is cheap now that the request-path test exists: after the five passing requests, rewrite the file so one entry claims a domain another already holds, reload that entry, assert the task fails, then re-issue the earlier request and assert it still returns the same body.

Non-blocking: the validation fixes have no tests of their own

The unknown-key change turns a warning into a load failure that ends in Fatal at startup, so an existing file with a stray key now stops the process on restart. That is the behaviour I asked for and I still think it is right, but neither half of the contract is pinned by a test. The same is true of the wildcard placement fix. Both are a few lines each on the assertion helper you already wrote.

Non-blocking: two test-hygiene items

config_reload_rpc.test.py:694 changed Content += to Content =, which drops all three default testers. Dropping ERROR: is now necessary, since the reload legitimately logs one. Losing Unrecognized configuration value is collateral, and this is the instance where a records typo would go unnoticed. One line puts it back. This is the same thing I raised about test 15 last round; the difference is that this one is new in this commit.

The fallback case in the new request test asserts the global table was reached, but a request that never resolved the virtualhost at all produces byte-identical output, so it does not pin the two-attempt fallback that is the behaviour under test. One more run, asserting that the same virtualhost's own rule still fires for a path it does have, makes the pair discriminating.

Non-blocking: the regex domain form, and what the documentation does not say

Two related gaps, both cheap.

The regex form above needs either code or documentation to move. Rejecting anything outside [a-z0-9.-] in domains would make the NOT Supported list true as written. Softening the doc to say that only a leading *. is recognised and anything else is treated as a literal that simply will not match would also be honest. Either is fine; having the page promise a guarantee the validator does not make is not.

Separately, the page never says what happens when a reload fails, and the behaviour is worth documenting because it is asymmetric. A file that fails to load at startup is fatal and the server does not start. The same file at runtime is rejected, the previous configuration keeps running, and the error surfaces through the reload status. An operator reading this page has no way to learn that, and it is exactly the thing they need to know before touching the file on a live box.

Non-blocking: items from last round still open

All thirteen of the should-fix items I raised are unchanged in the current head. None is individually blocking and I am not asking for them all now, but two are worth naming because they are the cheapest on the list: a missing optional config still logs at Warning where the sibling loaders use Note, and a stat failure that is not ENOENT still falls through to YAML::LoadFile and surfaces as bad file, which is so unhelpful that test 16 explicitly excludes it.

The one I would least like to see merge is set_entry still erasing the entry and its domain claims before validating the replacement. It is safe today only because the caller discards the copy on failure, which is a property of one call site rather than of the function.

Agreed: what is good here

Full-reload atomicity on the failure path is correct: every failure returns before configProcessor.set, so a validation failure leaves the previous config fully intact. The granular path is atomic the same way, mutating a copy that is destroyed on failure. No entry is silently dropped on a full reload; a collision rejects the whole load, which is the right choice.

The new request-path test is well built, and I want that said plainly. Keying the origin on the path so each rule gets a private target, and identifying the winning rule by body rather than by status, is the right design, and three of the five runs are genuinely discriminating because they assert the loser's body is absent. Mixing a legacy remap.config global table with YAML per-domain tables covers the mixed case for free.

Rejecting pushed RPC content explicitly, rather than ignoring it, is a call I rarely see made correctly.

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate issues affect virtualhost validation, reload behavior, remapping, and plugin lifecycle.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 3 · 1

Open (4)

• Reject empty virtualhost identifiers during startup · New
• Scope plugin reload notifications to each rebuild · New
• UrlRewrite.h now exposes YAML::Node in the public API (load_table / BuildTable) but the…
• This change adds new runtime behavior (virtualhost selection + per-virtualhost remap table override…

Previously missed (2)

In code that hasn't changed since last review

Preserve virtualhost reverse mappings during forward fallback

src/​proxy/​http/​HttpSM.cc:4752

m_remap is changed to the global table whenever the virtualhost table has no forward match. That also selects the table used later by response_url_remap() for Location headers, so a virtualhost containing only reverse_map rules (or a request that misses its virtualhost forward rules) never applies those per-domain reverse mappings. Keep the virtualhost reverse table available while falling back for forward lookup, or compose the two tables.

Incorrect subject-verb agreement

doc/​admin-guide/​files/​virtualhost.yaml.en.rst:186

This sentence has incorrect subject/verb agreement and number: use “These rules produce the following translations.”

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

Review effort: Lite
Findings: 4 · 3 · 1

Open (8)

• UrlRewrite::load_table is passed conf.id as the config_file_path. This becomes the effective… · New
• Scope plugin reload notifications to each rebuild
• Reject empty virtualhost identifiers during startup
• UrlRewrite.h now exposes YAML::Node in the public API (load_table / BuildTable) but the…
• The mutex comment states that both reload paths hold the mutex for the parse and publish, but… · New
• These two assignments will overwrite each other in normal Python attribute semantics, which can… · New
• This change adds new runtime behavior (virtualhost selection + per-virtualhost remap table override…
• Fix grammar: change 'This rules translates' to 'These rules translate' (or similar) for correct… · New

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved reload synchronization, validation, lifecycle, dependency-tracking, and test-fixture issues remain.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 3 · 1

Open (4)

• Reject overlong domain entries before hostname transformation · New
• Serialize global and virtualhost remap plugin reloads · New
• UrlRewrite.h now exposes YAML::Node in the public API (load_table / BuildTable) but the…
• This change adds new runtime behavior (virtualhost selection + per-virtualhost remap table override…

Resolved since last review (6)

• UrlRewrite::load_table is passed conf.id as the config_file_path. This becomes the effective…
• Scope plugin reload notifications to each rebuild
• Reject empty virtualhost identifiers during startup
• These two assignments will overwrite each other in normal Python attribute semantics, which can…
• The mutex comment states that both reload paths hold the mutex for the parse and publish, but…
• Fix grammar: change 'This rules translates' to 'These rules translate' (or similar) for correct…

Previously missed (2)

In code that hasn't changed since last review

Reject unsupported virtualhost reload directive keys

src/​proxy/​VirtualHost.cc:522

If _reload contains a typo or any unsupported key, this block finds no id and silently falls through to VirtualHost::reconfigure(ctx), turning a scoped request such as virtualhost.ident=foo into a full-file reload. Since the documented directive set only supports id, reject unknown directive keys instead of widening the operation.

Track inline remap includes for virtualhost reloads

src/​proxy/​http/​remap/​RemapYamlConfig.cc:1103

Inline remap rules can contain include, and the existing parse_yaml_remap_fragment() registers included files under ts::filename::REMAP_YAML. That dependency schedules only the global remap-yaml handler, so a change to a file included by a virtualhost rule leaves this table stale until virtualhost.yaml itself is reloaded. Associate inline includes with the virtualhost reload handler, or explicitly reject/document includes for this path.

[bryancall]

❌ Request changes

This is close. Since my last review on 87e97f9ce, two commits (cb8db3e86 and 6834b8212) have closed all four of my blockers. They also fixed the single-entry lock ordering that Copilot flagged, and they grouped the remap plugin notifications. One small gap still holds this up: a _reload directive with any key other than id still becomes a full reload that reports success. It is the same case as the empty id you fixed, reached through a different key, and it's a few lines to close.

I reviewed 87e97f9ce..6834b8212. CI is 14 of 14 green.

Fixed: both reload paths now hold the lock from read to publish

Full reloads take vhost_reconfigure_mutex before config->load() (VirtualHost.cc:574). The single-entry reload now takes it before load_entry (:600), so a single-entry reload that read the file before a full reload removed its id can no longer publish that stale entry on top of the result. The first paragraph of the mutex comment now describes both paths accurately. One sentence in it claims too much; see below.

Fixed: a missing or empty file on reload is refused

load() fails on a missing file (:250) and on a file that parses to Null (:262) unless it is the startup load. Each message reports how many live entries were kept. Startup passes true explicitly and the header default is the safe value. The record-triggered path I raised is covered too: CfgLoadLog always writes to diags, so a mistyped proxy.config.virtualhost.filename now logs an error and keeps the table. The admin guide says so (virtualhost.yaml.en.rst:154-157). Test 18 (config_reload_rpc.test.py:819) fails on the old code.

Fixed: an empty id is rejected, in the request and in the file

The handler rejects _reload: {id: ""} with a message saying how to ask for a full reload, and reconfigure(id) refuses an empty id rather than widening it. Test 17 (:759) fails on the old code. decode_virtualhost_entry now also rejects id: "" in the file (VirtualHost.cc:120-124), so every entry that loads can be reloaded by id.

Fixed: a refused reload is shown to leave the old table serving

virtualhost_remap.test.py:186-255 rewrites deep-wildcard to claim a domain exact-only already holds. It reloads only that entry and expects exit code 2 through the polling helper, then checks both hosts. It adds an origin response that is reachable only if the bad config gets published (:59). If the conflicting entry were published, the erase-first copy leaked, or the table were lost, one of the two requests would catch it. To be clear about what it proves: the old reconfigure(id) already returned before publishing when set_entry failed, so this pins behavior that already worked. That is still what I asked for.

Fixed: one plugin notification pair per rebuild

VirtualHostPluginReload replaces the per-table pre/post pairs with one pair per rebuild, and reports "used" against every table that will be live. I walked every return path in both reconfigure overloads and in startup(). Each failure sends exactly one failure post, and each success sends exactly one success post. lock is declared before plugin_reload, so the destructor's failure post always runs under the lock. The old per-table posts told plugins that only an earlier table used that they were unused, and txn_box clears its cache on every post, so this is a real improvement. Passing "virtualhost" instead of a factory UUID is fine: LoadedPlugins uses that argument only in two debug lines (PluginDso.cc:382, :394).

The StillRunningAfter += change at virtualhost_remap.test.py:164 also works: in autest 1.10.6, += goes through TesterSet.__iadd__ and keeps both checks, where the old = replaced the first.

Still blocking: a directive key other than id still becomes a full reload

The handler only acts on directives["id"] (VirtualHost.cc:520-540). A directives map with any other key skips that block and runs VirtualHost::reconfigure(ctx) at :543, which reports "Finished loading virtualhost config". Nothing upstream checks the keys: traffic_ctl passes any -D key through, and ConfigRegistry just stores them.

So -D virtualhost.ID=foo or -D virtualhost.name=foo rebuilds the whole table from disk, including any half-finished edits elsewhere in the file. The operator meant to reload one entry. Your comment at :529-530 says exactly why that is wrong for an empty id.

Fix: inside if (directives), fail on any key other than id and name the key in the message. An empty _reload: {} should still mean a full reload, since Test 18 relies on it.

Non-blocking: the mutex comment claims too much about plugin notifications

VirtualHost.cc:56-57 says the mutex "keeps the remap plugin reload notifications of one rebuild from interleaving with another's". That holds between two virtualhost rebuilds only. reloadUrlRewrite (ReverseProxy.cc:202-232) sends its own pre/post pairs to the same loaded plugins without this mutex, and LoadedPlugins locks only for one call at a time. So a remap.config reload can still land between a virtualhost pre and post. Rewording it to say "two virtualhost rebuilds" is enough here. The related issue is that each side's post reports the other side's plugins as unused. That predates this commit and is better as a follow-up issue than a change in this PR.

Non-blocking: id: with no value loads as a virtualhost named null

yaml-cpp converts a null node to the string "null" (lib/yamlcpp/include/yaml-cpp/node/impl.h:145-146), so id:, id: ~ and id: null all get past the new empty check and load with the id null. Checking node["id"].IsScalar() before as<std::string>() at :119 closes it.

Non-blocking: make the plugin reload object required

load() and load_entry() take VirtualHostPluginReload *plugin_reload = nullptr, and build_virtualhost_entry skips begin() when it is null. Both real callers pass it. Now that remap_parse_yaml(node, ...) no longer sends notifications itself, the default lets a future caller build remap tables that notify nobody. A required reference, or just dropping the = nullptr, removes that trap. The load_table doc in UrlRewrite.h should also say that the inline mode sends no notifications, since the file mode still does.

Non-blocking: tests that would pin the rest

Only the empty-id request, the missing-file refusal and the refused-reload requests have tests. These are cheap:

• Grouped notifications. No plugin is needed. LoadedPlugins always writes done reloading by factory '<id>' under the plugin_dso debug tag. Add plugin_dso to the tags in virtualhost_remap.test.py and assert traffic.out contains done reloading by factory 'virtualhost', which fails if this commit is reverted.
• Empty id in the file. Rewrite the file with id: "", reload expecting failure, and assert the "non-empty id" message.
• An empty file at startup. configs/virtualhost.yaml.default contains only comments, so every default install takes the Null branch at :260. Nothing in AuTest does, because min_cfg/ has no virtualhost.yaml. One instance whose file holds only a comment line, excluding FATAL: and making one request, covers it.
• An empty file on a full reload, and a refused full reload with live entries. Test 18 runs with zero entries, so "keeping the 0 live entry(s)" doesn't show anything was kept.
• is_hostname in both directions. Cover foo[0-9]+.example.com on the reject side, and an IPv4 literal, a _ label and a bracketed IPv6 literal on the accept side. I confirmed that host_get() keeps the brackets, so matching [::1] verbatim is right. The bracket branch accepts anything between brackets, though, so [foo[0-9]+] passes.

Non-blocking: docs

• virtualhost.yaml.en.rst:222 still says "This rules translates"; :186 was fixed.
• :43 should say the id must be non-empty.
• :154-157 covers the runtime half. It should also say that invalid content at startup is fatal (VirtualHost::startup() calls Fatal at :555).
• Now that deleting the file is refused, say that virtualhost: [] is how to remove every virtual host on purpose. One quirk is worth a sentence: FileManager treats a deleted file as a change only once (FileManager.cc:394-403). So the first reload after deleting it fails, and later reloads report success while the old entries keep serving until the next restart drops them.

Non-blocking: Copilot's new comments

• Domain truncation at :150-151: right, low impact. transform_lower clips to TS_MAX_HOST_NAME_LEN before is_hostname runs. Two names that differ only past that point hit the duplicate-domain error rather than colliding silently, and a valid DNS name is at most 253 characters. Rejecting an overlong domain_entry before lowering would make it explicit.
• Interleaving with the global remap reload at :575: right. It is the mutex comment item above, and it predates this commit.

Still open from earlier rounds

set_entry still erases the old entry before validating the new one. The refused-reload test shows this is safe only because the caller discards the copy, so a comment on set_entry saying callers must pass a private copy would keep it that way. Content = at virtualhost_remap.test.py:68 and config_reload_rpc.test.py:694 still drops the default Unrecognized configuration value exclusion. Adding one += line restores it in each file. The fallback case at virtualhost_remap.test.py:180-182 can still pass without the virtualhost resolving.

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical reload safety and related virtualhost reload correctness issues remain.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 4 · 1

Open (5)

• Reject empty virtualhost sequences during reload · New
• Serialize global and virtualhost remap plugin reloads
• Reject overlong domain entries before hostname transformation
• UrlRewrite.h now exposes YAML::Node in the public API (load_table / BuildTable) but the…
• This change adds new runtime behavior (virtualhost selection + per-virtualhost remap table override…

Previously missed (2)

In code that hasn't changed since last review

Notify plugins when remap entries are replaced

src/​proxy/​VirtualHost.cc:210

When a reload replaces an existing entry that had remap with one that has no remap, this branch never calls VirtualHostPluginReload::begin(). The old table's plugin instances are then removed without the pre/post reload callbacks that notify loaded remap DSOs of the successful/unused configuration, so plugins can retain stale reload state. Start the notification for this replacement/removal as well, while still emitting only one pair for a whole rebuild.

Reject duplicate IDs in scoped reloads

src/​proxy/​VirtualHost.cc:356

The full-file loader rejects duplicate IDs at lines 282-287, but this scoped loader returns the first matching node and silently accepts a file containing duplicate id entries. A single-entry reload is therefore ambiguous and can apply a different definition than the operator intended; detect multiple matches (and fail) instead of returning on the first one.

[bryancall]

✅ Approve

I reviewed 6834b8212..c374a3b5d: one commit, VirtualHost.cc +8 and config_reload_rpc.test.py +60. CI is 14 of 14 green. That closes the last blocker, so this is ready from my side. Thanks for sticking with it. The reload paths are in much better shape than where we started.

Fixed: an unrecognized directive key no longer becomes a full reload

The handler now walks the directives map and fails on any key other than id, naming the key (VirtualHost.cc:521-528). I checked the edges:

• An empty _reload: {} runs the loop zero times and still falls through to the full reload, so Test 18 is unaffected.
• A mixed map such as {id: x, ID: y} fails and names ID.
• A null key is not a scalar, so it's reported as <non-scalar> and still fails.
• as<std::string>() only runs on scalar keys, so it cannot throw.
• directives is always a map by the time it gets here: ConfigRegistry::apply_passed_config only stores _reload when it is a map (ConfigRegistry.cc:514-519).

Verified: Test 19 fails on the old code

Test 19 (config_reload_rpc.test.py:881) sends _reload: {ID: ...} to the instance that has a valid virtualhost.yaml. On 6834b8212 that request does a full reload that succeeds, so the "directive 'ID' is not supported" line the test looks for never appears and the check fails. The status check also requires fail, the same way Test 17 does. It's the last test in the file and the refused path never touches the live table, so it leaves nothing behind for later runs.

Non-blocking: a _reload that isn't a map still becomes a full reload

If _reload arrives as a scalar or a sequence, apply_passed_config logs a Warning, drops it (ConfigRegistry.cc:514-516), and the handler goes on to a full reload that reports success. It's the same widening this commit closes, reached through the shape of _reload rather than a key. It only happens through raw JSON-RPC, since traffic_ctl -D always builds a map, and it's framework code this PR doesn't touch. Failing the task there instead of warning would be a good small follow-up.

The non-blocking items from my 09-23 review (the mutex comment's plugin sentence, id: loading as null, the optional plugin reload pointer, the extra tests and the doc fixes) are all still worth doing. None of them needs to hold this PR.