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 = {});