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]

Pull request overview

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

Suppressed comments (5)

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

src/proxy/VirtualHost.cc:54

• valid_vhost_keys is defined at global namespace scope with external linkage, which is unnecessary and risks symbol collisions across TUs. Make it static const (and ideally keep it in the anonymous namespace) so it has internal linkage.

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

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

• The example URL has a typo: http:/foo.example.com/ is missing a slash and is not a valid URL.

            url: http:/foo.example.com/

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

• The evaluation-order text mixes remap.config and remap.yaml as the global fallback. The implementation falls back to the global remap table regardless of whether it came from remap.yaml or remap.config, so the docs should describe both consistently.

   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.

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

• Grammar: "This rules translates" should be plural.

This rules translates in the following translation.

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

• UrlRewrite.h now references YAML::Node in the public method signatures, but this header neither includes yaml-cpp nor forward-declares YAML::Node. This makes compilation depend on include order and can fail for translation units that include UrlRewrite.h without having already included <yaml-cpp/yaml.h>. Add a forward declaration (e.g. namespace YAML { class Node; }) near the top of the header, or include yaml-cpp explicitly.

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

  /** Build the internal url write tables.
   *
   * @param path Path to configuration file.
   * @param ctx  ConfigContext for reload status tracking.
   * @return 0 on success, non-zero error code on failure.
   */
  int BuildTable(const char *path, YAML::Node const *remap_node = nullptr, ConfigContext ctx = {});

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