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.

[bryancall]

Re-reviewed against the four commits pushed since my last review. Both blocking items are genuinely fixed, and Blocking 2 was addressed as a redesign rather than a type correction, which is what I asked for. Thank you for that.

Still requesting changes. Nothing below is either of the blocking items, and with one clearly marked exception everything below is in code this pull request is adding, not pre-existing behavior.

First, the setup facts my last review rested on have changed, so for anyone reading back: the branch is now mergeable, its base 02013679bb contains 709443e870, and the 15 green checks are dated 2026-08-25 against that base. Seven platform builds plus Clang-Analyzer. Last round the green said nothing because it predated the refcount change. This round it counts.

Blocking 1: the IP allow accept-check global. Confirmed fixed.

The setter call is gone from the inline parser and the comment that replaced it states the reason. I checked every clause in that comment against the code and all three hold:

• IpAllow::accept_check_p is a single static at src/proxy/IPAllow.cc:75, and tree-wide the only remaining writers are RemapConfig.cc:1555 and RemapYamlConfig.cc:1016, both global table paths.
• The accept time decision really is made before any host is known. The flag is consumed in IpAllow::match() at src/proxy/IPAllow.cc:194 under match_key_t::SRC_ADDR, keyed on a bare address, and its callers are HttpSessionAccept.cc:61, Http2SessionAccept.cc:63 and Http3SessionAccept.cc:56, all of which evaluate and can hard deny before new_connection() and therefore before a request byte is parsed. The virtualhost lookup needs a parsed Host, so the two are unambiguously ordered.
• The regression a re-add would cause is real. BUILD_TABLE_INFO::accept_check_p defaults true at RemapConfig.h:67, reset() does not clear it, and each virtualhost gets a fresh instance, so a virtualhost with no deactivate_filter would compute true and restore fast deny that the global config had switched off.

This is the best comment in the change. It documents a deletion, which is the highest rot risk kind of knowledge because there is no code for a future reader to inspect, and it names the specific global, the mechanism and the concrete regression. Please keep it verbatim.

Blocking 2: table lifetime. Confirmed fixed.

Four things convinced me this is a redesign and not a rename:

1. The deleter is shared. You extracted make_managed_url_rewrite(), exported it at include/proxy/ReverseProxy.h:55, moved both existing global call sites onto it, and used it for per virtualhost tables at src/proxy/VirtualHost.cc:137. Per domain tables now get the same UrlRewriteDeleter as the global table, so deferred teardown through new_Deleter and the deliberate post shutdown leak apply to both. One construction path instead of two. That is the part a textual merge would never have produced.
2. Removing the manual release from ~HttpSM() is correct, and it is not obvious. HttpSM::destroy() is a bare THREAD_FREE, which reads like it skips the destructor. It does not: ClassAllocator<HttpSM> takes the default Destruct_on_free_ = true at include/tscore/Allocator.h:332 and the THREAD_FREE macro opens with destroy_if_enabled at include/iocore/eventsystem/ProxyAllocator.h:96, while alloc() placement constructs. So Ptr<Entry> is constructed and released once per transaction. Had that gone the other way, a recycled state machine would have reused the previous transaction's virtualhost table, because init() reseeds m_remap but not m_virtualhost_entry.
3. Shutdown is handled and the claim is checkable. rewrite_table.load() can return null after shutdown_url_rewrite(), and setup_for_remap() null checks at src/proxy/http/remap/RemapProcessor.cc:54 before the first dereference.
4. The two attempt fallback leaves no inconsistent state. This was my main open question. Tracing every t_state write in setup_for_remap(): url_map's mapping is only set on the success paths in _mappingLookup, reverse_proxy and hh_info are recomputed identically by the second attempt, remap_redirect is not written there at all, and set_url_target_from_host_field() and mark_target_dirty() are gated on mapping_found. A failed first attempt leaves nothing for the second to trip over.

Correcting my own last review

I offered ConfigSource::FileOnly with a comment as "the deliberate alternative" for the supplied_yaml() item, pointing at IPAllow.cc:101. That was wrong and would have broken the feature. src/mgmt/rpc/handlers/config/Configuration.cc:300 gates on entry->source != ConfigSource::FileAndRpc and rejects the whole configs: entry with RPC_SOURCE_NOT_SUPPORTED before _reload is ever extracted, so FileOnly would have refused directive only requests too and taken granular reload with it. Keeping FileAndRpc and rejecting the body inside the handler is the only option that works. Your resolution is better than what I asked for.

Your two replies on the earlier threads are also both correct and hold up under checking. load() is only called on a fresh object, and reconfigure(std::string_view) does mutate a copy and return before configProcessor.set(), so a failed single entry reload cannot drop a live entry. I confirmed set_entry has no other callers.

---

Must fix

1. Four of the eight domain forms the documentation declares unsupported are silently accepted

src/proxy/VirtualHost.cc:92 gates all validation on if (domain[0] == '*'); the else at line 99 pushes straight into exact_domains with no checks.

• Correctly rejected: *baz.example.net, *.bar.*.com, *.*.baz.com, *
• Silently accepted as literal exact domains: foo[0-9]+.example.com, bar.*.example.net, baz*.example.net, b*z.example.net

Those become strings no real Host header can equal, so the entry loads clean, never fires, and produces no diagnostic. The operator gets a dead virtualhost with nothing to act on.

This is the mirror image of something I credited the change for last time. Wildcards genuinely are restricted to the single left most form, but the restriction is only reachable for strings that already begin with *. Note the contract is asserted in three places and enforced in none of them for this case: the admin guide list at doc/admin-guide/files/virtualhost.yaml.en.rst:84, the comment on the shipped default at configs/virtualhost.yaml.default:20 ("Only allow single left-most"), and my own last review. Reject any * that appears outside a leading *., with the id and line.

2. An unknown key does not fail the load, so a typo silently disables per-domain remap

src/proxy/VirtualHost.cc:63 warns on an unrecognized key and continues. So remaps: instead of remap: loads the entry with a null remap_table, build_virtualhost_entry skips the if (remap_node) block at line 130, and every request for that domain silently serves from the global table. The operator's per domain overrides are simply not in effect, traffic looks fine, and the only trace is one context free warning at startup with no id and no line number.

valid_vhost_keys is a closed set of three. An unknown key should be an error with the id and line, not a shrug.

3. The removed _remap_yaml guard now fails open and silent

The change deleted if (remap_node) { this->_remap_yaml = true; } from load_table() and moved it to the caller as set_remap_yaml(true). BuildTable() still branches only on is_remap_yaml() at src/proxy/http/remap/UrlRewrite.cc:845, so with a node supplied and the flag false the node is discarded and the virtualhost id is handed to remap_parse_config() as a file path. And src/proxy/http/remap/RemapConfig.cc:1115 is:

  if (ec.value() == ENOENT) { // a missing file is ok - treat as empty, no rules.
    return true;
  }

So it succeeds. Zero rules, 0 >= required_rules, _valid = true, the entry is accepted with an empty remap table, no diagnostic, and every request for that domain falls through to the global table with the operator's per domain policy not in effect.

Latent today, since the single caller does set the flag. But the trade was a structural guarantee for a convention whose violation fails silently and open, which is the wrong direction for a config loader, and set_remap_yaml() is a public unconditional setter at include/proxy/http/remap/UrlRewrite.h:111 so the flag can also be flipped after the table is built. It is also worth noting this went the opposite way from what the earlier review thread on UrlRewrite.cc:106 asked for, which was to make the flag deterministic.

Cheap fix: ink_release_assert(!remap_node || is_remap_yaml()); at the top of BuildTable(). Please use ink_release_assert and not ink_assert, which is debug only and would not catch this in a release build. Better fix: split the overloads so the illegal combination is unrepresentable, and have the node overload set the flag itself.

4. The diagnostics you added do not reach the operator who asked for the reload

include/proxy/VirtualHost.h:83 declares reconfigure() and reconfigure(std::string_view) with no ConfigContext, so nothing below the handler can write to the reload task log. Every new message is a bare Error(...), which is diags only, and what the caller gets back is Failed to load virtualhost config. Duplicate id, a domain claimed by another virtualhost, a malformed wildcard, a YAML syntax error with ex.what(), entry not found: all of it invisible over the RPC.

This is my earlier request landing on the diags side and not on the side that matters for the feature's own headline. Granular reload exists so an operator can change one virtualhost and be told what happened; being told only "failed" and having to go grep diags.log on the box is the workflow the reload status RPC exists to eliminate.

The change's own tests show the difference. Test 14 asserts its message through the task log because that one message goes through ctx.fail(). Test 15 has to scrape diags.log instead. reloadUrlRewrite shows the pattern: thread ctx down and use CfgLoadLog. The fix is mechanical, a defaulted ConfigContext parameter through reconfigure, load, load_entry and build_virtualhost_entry.

5. Changing proxy.config.virtualhost.filename fires two full reloads

VirtualHost::startup() registers that record through RecRegisterConfigUpdateCb and again as a ConfigRegistry trigger record, so one record change drives both VirtualHostConfigContinuation and RecordTriggeredReloadContinuation, each running a full reconfigure(). Since do_register also hands the file to FileManager, an ordinary file change reload hits both, re-parsing the file and rebuilding every virtualhost's remap table twice, including the plugin pre and post reload hooks.

The remap module deliberately avoids this overlap: it uses attach() for its filename records and the legacy callback only for proxy.config.reverse_proxy.enabled. Pick one mechanism.

6. Lost update between the two reload paths

reconfigure(id) reads the live config, copies it, mutates one entry and stores, with no serialization. Reload continuations get a fresh mutex each, VirtualHostConfigContinuation has none, and config_callback sits outside ReloadCoordinator, so concurrency here is reachable rather than theoretical, and item 5 above makes it more so.

Interleaved with a full reload that drops entries deleted from disk, the granular store can put them all back: the full reload publishes without entry b, then the granular path publishes its older snapshot plus its one edit, and b is live again despite being absent from the file. A mutex around the read, copy, modify and store in both overloads closes it. Static int _configid is also non atomic and costs nothing to make atomic.

I am raising this at the same weight as the blocking items last round for the same reason I gave then: getting per-domain config lifetime wrong under reload is the class of bug 709443e870 was fixing.

7. There is no test that sends a request through this feature

Across the whole tree, only two files under tests/ mention virtualhost: config_reload_rpc.test.py and the trafficserver.test.ext plumbing. All 16 runs in that file are JSONRPC calls. There is no origin server, no Host header, and no remap: block in any test config anywhere, so Entry::remap_table is never even constructed under test. There is no unit test either; src/proxy/CMakeLists.txt adds VirtualHost.cc to the library and registers no test source.

So exact domain matching, wildcard matching, the documented longest suffix rule, per domain precedence over the global table, and the fallback all have zero coverage. To be clear, "Add more config_reload_rpc tests" does not close this: tests 14, 15 and 16 are reload plumbing, and they are decent tests of reload plumbing, but they are a different thing from what the open review thread on HttpSM.cc:4715 is asking for.

Item 1 above is the concrete cost of this gap, and the fallback path runs setup_for_remap() twice against two different tables on the per transaction path for every request, which is not something I can sign off on by inspection alone.

The minimal test is one file, one ATS process, two origins, five requests, asserting which remap rule won rather than merely a 200:

1. Host: exact.example.com reaches the per domain rule, not the global table.
2. Host: x.deep.example.com with both *.deep.example.com and *.example.com configured reaches the deeper one. Nothing today covers the longest suffix rule.
3. A host matching both an exact domain and a wildcard reaches the exact one.
4. A host that resolves to a virtualhost whose rules do not match the path falls back to the global table. Highest value single assertion in the set.
5. A host with no virtualhost entry at all reaches the global table.

The Disk.virtualhost_yaml plumbing you already added in this change means writing this is cheap.

8. Test 16 cannot fail

At tests/gold_tests/jsonrpc/config_reload_rpc.test.py:634, both assertions are satisfied before the RPC is sent. VirtualHost::startup() calls reconfigure() then load(), which emits the byte identical Warning("Virtualhost configuration '%s' doesn't exist", ...) at src/proxy/VirtualHost.cc:151 and returns true, and it returns before YAML::LoadFile so bad file cannot appear either. Delete the stat and ENOENT guard you added to load_entry() and this test stays green.

Test 14 has the right pattern: pass a token= and query get_reload_config_status. A test that cannot fail is worse than no test, because it reads as coverage.

Related and worth fixing on its own: that same message text is emitted at VirtualHost.cc:151 where it is benign and returns true, and at VirtualHost.cc:217 where it is a failed reload and returns false. An operator grepping diags.log cannot tell which happened.

9. Two documentation examples are wrong

Both are in the new admin guide page, so they are the first thing an operator copies.

• virtualhost.yaml.en.rst:210 still has url: http:/foo.example.com/ with one slash. This is the third time it has been raised.
• The regex_map example at line 196 maps http://sub[0-9]+.example.com/ to http://origin$1.example.com/. There is no capture group in the pattern, so $1 binds nothing, yet the table below claims sub0 produces origin0. It needs sub([0-9]+).

---

Should fix

Diagnostics and levels, all in the new file:

• Warning is the wrong level for a missing optional config. Two precedents use Note for the identical stat and ENOENT pattern: src/proxy/http/remap/NextHopStrategyFactory.cc:55, which even carries the comment "missing config file is an acceptable runtime state", and src/proxy/logging/LogConfig.cc:779, "File doesn't exist, not a failure". load() returns true, so the code already treats it as benign, and this fires at every start and every reload for every deployment that does not use the feature.
• load_entry()'s empty file path at VirtualHost.cc:224 is still Dbg plus return false, the one terminal failure in that function left below Error. A config management tool that truncates the file to zero bytes gives the operator a failed reload and a silent log.
• The new inline parser uses Dbg for a non sequence remap: node at RemapYamlConfig.cc:1033 where the sibling file parser uses CfgLoadLog(ctx, DL_Error, ...) at line 999. A trailing empty remap: key parses as a defined null node, so this is reachable, and it ends in a startup Fatal whose cause needs a debug tag to see.
• stat failures other than ENOENT fall through to YAML::LoadFile, whose what() is bad file, so a permissions problem gets a useless message and then a Fatal. Your own Test 16 excludes that string, so you already know it is useless. Handle the general errno with strerror.
• proxy.config.url_remap.min_rules_required is now applied to per virtualhost tables, but doc/admin-guide/files/records.yaml.en.rst:4140 scopes it to remap.config, where it is a tripwire against a truncated global file. If an operator has set it, a small per domain block is rejected as if it had a syntax error, and the only line naming the cause is at Warning, prefixed [ReverseProxy], and never names the virtualhost, for a condition that then kills the process.
• Duplicate domains within a single entry are not detected separately, so the second occurrence trips the cross entry check and the message names the entry as its own conflicting claimant: "domain 'shop.example.com' in virtualhost 'shop' is already claimed by virtualhost 'shop'". A copy pasted line produces a fatal exit and a message that reads like an ATS bug.
• A domain longer than TS_MAX_HOST_NAME_LEN is silently truncated at VirtualHost.cc:88. ts::transform_lower clips rather than overflowing, so this is memory safe, but the entry then registers under a prefix that matches more broadly than what was written, with no diagnostic. Reject it.
• "expected toplevel 'virtualhost' key to be a sequence" also fires when the key is absent, since a missing key yields an undefined node. Someone who writes virtualhosts: is told their sequence is not a sequence.

Structure and API:

• set_entry() erases the existing entry and its domain claims before validating the replacement, so a mid loop conflict leaves domain maps pointing at an id no longer in _entries. It is safe today only because reconfigure(id) discards the copy, which is statement order in one function rather than anything the type requires. Validate first, mutate second. Also, whichever way you go, the failure message should tell the operator the disposition, something like "the previously loaded entry remains in effect", because that is the first thing they will ask.
• valid_vhost_keys at VirtualHost.cc:53 is a mutable namespace scope global with external linkage; the anonymous namespace closes at line 43. Should be const and inside it.
• Dbg(..., "%s", id.data()) on a std::string_view at VirtualHost.cc:432. You fixed exactly this twice in load_entry(), lines 250 and 254, and left this one.
• UrlRewrite::load_table(const std::string &config_file_path, ...) is called with the virtualhost id as the path at VirtualHost.cc:133. Harmless today because BuildTable ignores it on the inline path, but the parameter name is a lie at that call site, and item 3 above is what happens when it stops being ignored. If you split the overloads, name it label.
• HttpSM::m_virtualhost_entry is public and has no users outside HttpSM.cc and HttpSM.h. Free to make private now. The pinning comment's guarantee depends on nothing else writing the member and on set_virtualhost_entry() being a latch, and a public member does not enforce a latch. (m_remap cannot be made private; it is read from HttpTransact.cc and InkAPI.cc.)
• VirtualHostConfig's copy constructor at include/proxy/VirtualHost.h:35 needs a comment. RefCountObj deletes both copy operations at include/tscore/Ptr.h:50 and :53, so the implicit one would be deleted, and yours compiles only because its member init list omits ConfigInfo() and the base is default constructed with a fresh refcount. That is the right semantics for a clone about to be adopted by configProcessor.set(), and nothing says so. It is also silently staleable: add a fourth member and the clone loses it, compile clean, and since only reconfigure(id) copies, every test that does not drive a single entry reload still passes. operator= at line 41 has no callers and can retarget a config other threads hold pinned; delete it.
• scoped_config is constructed before the early returns in set_virtualhost_entry() at HttpSM.cc:4686, and the caller's guard is always true on the first call, so every transaction in every deployment pays a config processor acquire and release even with no virtualhost.yaml.

Tests:

• Test 15 at config_reload_rpc.test.py:600 uses Content =, which replaces all three default testers from trafficserver.test.ext:246, and line 602 restores only FATAL:. Dropping ERROR: is necessary there; losing Unrecognized configuration value looks accidental.
• Three validator docstrings claim assertions their functions do not make, each returning true unconditionally absent a synchronous error: config_reload_rpc.test.py:528, :615 and :655. The real assertions live in the following run or in a diags_log tester, so a maintainer who deletes that tester would believe the validator still covers it. The pattern predates your commits, but these three instances are new.

Comments, one clause each:

• The pinning comment at HttpSM.cc:4711 is correct only because Destruct_on_free is true. Declaring ClassAllocator<HttpSM, false> would leak both smart pointers across every recycle with no compiler complaint, so naming the dependency pins it.
• The null comment at HttpSM.cc:4726 names only setup_for_remap(), but finish_remap() null checks too at RemapProcessor.cc:173, which is what makes the later calls at HttpSM.cc:4611 and :8350 safe.

Remaining documentation:

• In the second example table, the bar.example.com row says only "No remap rule found in virtual host entry example". That is exactly where a reader looks for the fallback to the global table, which is the subtlest behavior in the feature.
• This rules translates in the following translation. appears twice verbatim.
• configs/virtualhost.yaml.default:20 ships - "*.com" as the wildcard example. The validator does accept it, but it claims every .com host and it is an unfortunate line to hand someone as a starting template. "*.example.com" would be safer.
• proxy.config.virtualhost.filename is RECU_DYNAMIC but its ts:cv:: entry in records.yaml.en.rst has no :reloadable: marker.

---

Out of scope, noted so nobody re-litigates

These are pre-existing and I am not asking you to fix them here.

• parse_yaml_remap_rule's errata is bound and discarded in both the file parser at RemapYamlConfig.cc:1009 and the new inline overload at :1049. The new overload copied the existing behavior faithfully, so this is an ATS-wide improvement, not something this change introduced.
• remap_parse_config returning true on ENOENT is long standing and correct for its own purpose. I only cite it above because it is what makes item 3 fail open rather than loudly.
• The return (True, ...)-on-no-error validator pattern is the house style throughout config_reload_rpc.test.py.
• On the open thread about YAML::Node in UrlRewrite.h: the compile failure claim there is wrong and I would close it. UrlRewrite.h:28 directly includes ConfigContext.h, which unconditionally includes yaml-cpp/node/node.h at ConfigContext.h:37, both above the uses at lines 82 and 89, so every consumer gets the complete type regardless of its own include order, and that include predates this change because load(ConfigContext ctx = {}) already required it. Seven platforms build. Adding the include directly is reasonable hygiene and matches what most ATS headers that name YAML::Node do, but it is a nit and not a blocker.

---

Credit

Two fixes in here nobody asked for. load() now clears _exact_domains_to_id and _wildcard_domains_to_id, without which a reload leaked stale domain to id mappings and would have produced phantom "already claimed" errors. And restructuring the directive check turned a real false success into a failure: the previous if (id_dir && id_dir.IsScalar()) fell through to a full reload and answered "Finished loading virtualhost config" for an operation nobody requested.

The comment quality in this round is genuinely good. I checked every explanatory comment you added against the code and did not find one that overstates what the code does. On a change like this that is worth more than it sounds, because it means the fixes came from understanding the ownership model rather than from matching the compiler's complaints.

The design work I credited last time still stands, and the two hard problems are solved. What is left is validation that does not enforce its own documented contract, silent failure modes on new config paths, one reload race, and the request path test.

[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.