Copilot commented on code in PR #13108: URL: https://github.com/apache/trafficserver/pull/13108#discussion_r4085069213
########## src/proxy/VirtualHost.cc: ########## @@ -0,0 +1,586 @@ +/** @file + + Virtual Host configuration implementation + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ + +#include <algorithm> +#include <cctype> +#include <cerrno> +#include <atomic> +#include <memory> +#include <mutex> +#include <set> +#include <string> +#include <string_view> +#include <strings.h> +#include <sys/stat.h> +#include <yaml-cpp/yaml.h> + +#include "proxy/VirtualHost.h" +#include "proxy/ReverseProxy.h" +#include "mgmt/config/ConfigContextDiags.h" +#include "mgmt/config/ConfigRegistry.h" +#include "records/RecCore.h" +#include "tscore/Filenames.h" +#include "tsutil/Convert.h" + +namespace +{ +DbgCtl dbg_ctl_virtualhost("virtualhost"); + +/** Serializes both reload paths against each other. + + The single-entry reload is a read-copy-modify-publish against the live config, so its read and + its publish must be atomic with respect to any other reload. The full reload has the same + requirement for a different reason: two full reloads that parse concurrently can publish in the + opposite order to their reads, so the older read wins and resurrects entries the newer one had + dropped. Both therefore hold this for the parse as well as the publish. Reload paths are + scheduled on ET_TASK with unrelated mutexes (a fresh one per @c ConfigRegistry::schedule_reload + and one per trigger record), so nothing else serializes them. Neither path is on the request + path, so serializing the parse costs nothing. + */ +std::mutex vhost_reconfigure_mutex; +} // namespace + +std::atomic<int> VirtualHost::_configid{0}; + +std::string +VirtualHostConfig::Entry::get_id() const +{ + return id; +} + +namespace +{ +const std::set<std::string> valid_vhost_keys = {"id", "domains", "remap"}; + +/** Check that @a name is composed of non-empty hostname labels. + + Without this, a documented-as-unsupported form such as `foo[0-9]+.example.com` is accepted as + an exact domain that no Host header can ever equal: the entry loads clean and never fires. + */ +bool +is_hostname(std::string_view name) +{ + // A bracketed IPv6 literal is matched verbatim against the Host header. + if (name.size() > 2 && name.front() == '[' && name.back() == ']') { + return true; + } + + size_t label_len = 0; + + for (char c : name) { + if (c == '.') { + if (label_len == 0) { + return false; + } + label_len = 0; + continue; + } + if (!isalnum(static_cast<unsigned char>(c)) && c != '-' && c != '_') { + return false; + } + ++label_len; + } + return label_len > 0; +} + +/** Decode a single `virtualhost` sequence element. + + Diagnostics go through @a ctx so the operator who asked for the reload sees them in the reload + task log, not just in diags.log. + */ +bool +decode_virtualhost_entry(YAML::Node const &node, VirtualHostConfig::Entry &item, ConfigContext ctx) +{ + if (!node["id"]) { + CfgLoadLog(ctx, DL_Error, "Virtualhost entry at line %d must provide `id`", node.Mark().line + 1); + return false; + } + item.id = node["id"].as<std::string>(); + + for (const auto &elem : node) { + auto key = elem.first.as<std::string>(); + if (!valid_vhost_keys.contains(key)) { + CfgLoadLog(ctx, DL_Error, "Virtualhost '%s' has unsupported key '%s' (line %d)", item.id.c_str(), key.c_str(), + elem.first.Mark().line + 1); + return false; + } + } + + auto domains = node["domains"]; + if (!domains || !domains.IsSequence() || domains.size() == 0) { + CfgLoadLog(ctx, DL_Error, "Virtualhost '%s' must provide at least one domain in a `domains` sequence (line %d)", + item.id.c_str(), node.Mark().line + 1); + return false; + } + item.exact_domains.clear(); + item.wildcard_domains.clear(); + + for (const auto &it : domains) { + auto domain_entry = it.as<std::string>(); + if (domain_entry.empty()) { + CfgLoadLog(ctx, DL_Error, "Virtualhost '%s' has an empty entry in `domains` (line %d)", item.id.c_str(), it.Mark().line + 1); + return false; + } + char domain[TS_MAX_HOST_NAME_LEN + 1]; + ts::transform_lower(domain_entry, domain); + + // Check if domain is wildcard, prefixed with * + if (domain[0] == '*') { + if (domain[1] != '.' || domain[2] == '\0' || domain[2] == '.' || strchr(domain + 2, '*') != nullptr) { + CfgLoadLog(ctx, DL_Error, "Virtualhost '%s' wildcard '%s' must match '*.[domain]' format (line %d)", item.id.c_str(), + domain, it.Mark().line + 1); + return false; + } + if (!is_hostname(domain + 2)) { + CfgLoadLog(ctx, DL_Error, "Virtualhost '%s' wildcard '%s' suffix is not a valid hostname; regex is not supported (line %d)", + item.id.c_str(), domain, it.Mark().line + 1); + return false; + } + item.wildcard_domains.emplace_back(domain + 2); + } else { + if (strchr(domain, '*') != nullptr) { + CfgLoadLog(ctx, DL_Error, "Virtualhost '%s' domain '%s' may only use a wildcard in the leading '*.[domain]' form (line %d)", + item.id.c_str(), domain, it.Mark().line + 1); + return false; + } + if (!is_hostname(domain)) { + CfgLoadLog(ctx, DL_Error, "Virtualhost '%s' domain '%s' is not a valid hostname; regex is not supported (line %d)", + item.id.c_str(), domain, it.Mark().line + 1); + return false; + } + item.exact_domains.emplace_back(domain); + } + } + + if (item.exact_domains.empty() && item.wildcard_domains.empty()) { + CfgLoadLog(ctx, DL_Error, "Virtualhost '%s' must have at least one domain defined (line %d)", item.id.c_str(), + node.Mark().line + 1); + return false; + } + + return true; +} + +bool +build_virtualhost_entry(YAML::Node const &node, Ptr<VirtualHostConfig::Entry> &entry, ConfigContext ctx) +{ + entry.clear(); + Ptr<VirtualHostConfig::Entry> vhost = make_ptr(new VirtualHostConfig::Entry); + auto &conf = *vhost; + try { + if (!decode_virtualhost_entry(node, conf, ctx)) { + return false; + } + } catch (YAML::Exception const &ex) { + CfgLoadLog(ctx, DL_Error, "Failed to parse virtualhost entry at line %d: %s", node.Mark().line + 1, ex.what()); + return false; + } + + // Build UrlRewrite table for remap rules + auto remap_node = node["remap"]; + if (remap_node) { + auto table = std::make_unique<UrlRewrite>(); + table->set_remap_yaml(true); + if (!table->load_table(conf.id, &remap_node, ctx)) { + CfgLoadLog(ctx, DL_Error, "Failed to load remap rules for virtualhost '%s' at line %d", conf.id.c_str(), + remap_node.Mark().line + 1); + return false; + } + conf.remap_table = make_managed_url_rewrite(std::move(table)); + } Review Comment: `UrlRewrite::load_table` is passed `conf.id` as the `config_file_path`. This becomes the effective 'path' for YAML remap parsing, which can break features that rely on the config file location (e.g., `include` directives resolving relative paths) and can also produce misleading diagnostics that report an id as a filename. Prefer passing the actual on-disk `virtualhost.yaml` path (or at least its directory) as the base path, and keep `id` only as a display label (e.g., add a separate `display_name` parameter or extend `ConfigContext` logs to include the vhost id). ########## src/proxy/VirtualHost.cc: ########## @@ -0,0 +1,586 @@ +/** @file + + Virtual Host configuration implementation + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ + +#include <algorithm> +#include <cctype> +#include <cerrno> +#include <atomic> +#include <memory> +#include <mutex> +#include <set> +#include <string> +#include <string_view> +#include <strings.h> +#include <sys/stat.h> +#include <yaml-cpp/yaml.h> + +#include "proxy/VirtualHost.h" +#include "proxy/ReverseProxy.h" +#include "mgmt/config/ConfigContextDiags.h" +#include "mgmt/config/ConfigRegistry.h" +#include "records/RecCore.h" +#include "tscore/Filenames.h" +#include "tsutil/Convert.h" + +namespace +{ +DbgCtl dbg_ctl_virtualhost("virtualhost"); + +/** Serializes both reload paths against each other. + + The single-entry reload is a read-copy-modify-publish against the live config, so its read and + its publish must be atomic with respect to any other reload. The full reload has the same + requirement for a different reason: two full reloads that parse concurrently can publish in the + opposite order to their reads, so the older read wins and resurrects entries the newer one had + dropped. Both therefore hold this for the parse as well as the publish. Reload paths are + scheduled on ET_TASK with unrelated mutexes (a fresh one per @c ConfigRegistry::schedule_reload + and one per trigger record), so nothing else serializes them. Neither path is on the request + path, so serializing the parse costs nothing. Review Comment: The mutex comment states that *both* reload paths hold the mutex for the parse and publish, but `VirtualHost::reconfigure(std::string_view id, ...)` currently parses the entry (`load_entry`) outside the lock. This mismatch makes the concurrency reasoning harder to trust/maintain. Either update the comment to reflect the current design (parse outside lock for single-entry reload; lock only for read-copy-modify-publish), or move the single-entry parse under the mutex if the intention is truly to serialize parsing too. ########## doc/admin-guide/files/virtualhost.yaml.en.rst: ########## @@ -0,0 +1,236 @@ + +.. Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. + +.. include:: ../../common.defs + +.. configfile:: virtualhost.yaml + +virtualhost.yaml +**************** + +The :file:`virtualhost.yaml` file defines configuration blocks that apply to a group of domains. +Each virtual host entry defines a set of domains and the remap rules associated with those domains. +Virtual host remap rules override global :file:`remap.yaml` rules but remain fully backward compatible +with existing configurations. If absent, ATS behaves exactly as before. + +Currently, this file only supports :file:`remap.yaml` overrides. Future versions will expand virtual +host support to additional configuration types (e.g. :file:`sni.yaml`, :file:`ssl_multicert.yaml`, +:file:`parent.config`, etc) + +By default this is named :file:`virtualhost.yaml`. The filename can be changed by setting +:ts:cv:`proxy.config.virtualhost.filename`. + + +Configuration +============= + +:file:`virtualhost.yaml` is YAML format with top level namespace **virtualhost** and a list of virtual host +entries. Each virtual host entry must provide an **id** and at least one domain defined in **domains**. + +An example configuration looks like: + +.. code-block:: yaml + + virtualhost: + - id: example + domains: + - example.com + + remap: + - type: map + from: + url: http://example.com + to: + url: http://origin.example.com/ + + +===================== ========================================================== +Field Name Description +===================== ========================================================== +``id`` Virtual host identifier to perform specific operations on +``domains`` List of domains to resolve a request to +``remap`` List of remap rules as defined in remap.yaml +===================== ========================================================== + +``domains`` + Domains can be defined as request domain name or subdomains using wildcard feature. + Wildcard support only allows a single left most ``*``. This does not support regex. + When matching to a virtual host entry, domains with exact match have precedence + over wildcard. If a domain matches to multiple wildcard domains, the most specific + (longest) suffix match is selected. + + For example: + Supported: + - ``foo.example.com`` + - ``*.example.com`` + - ``*.com`` + + NOT Supported: + - ``foo[0-9]+.example.com`` (regex) + - ``bar.*.example.net`` (``*`` in the middle) + - ``*.bar.*.com`` (multiple ``*``) + - ``*.*.baz.com`` (multiple ``*``) + - ``baz*.example.net`` (partial wildcard) + - ``*baz.example.net`` (partial wildcard) + - ``b*z.example.net`` (partial wildcard) + - ``*`` (global) + +Evaluation Order +---------------- + +|TS| evaluates a request using deterministic precedence in the following order: + +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. + 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. + +Only one virtual host entry may match a given request. Exact domain matches take precedence over wildcard matches. For wildcard matches, +ATS selects the most specific (longest) matching suffix (e.g. ``*.example.com`` before ``*.com``). + + +Granular Reload +=============== + +|TS| now supports granular configuration reloads for individual virtual hosts defined in :file:`virtualhost.yaml`. +In addition to reloading the entire |TS| configuration with :option:`traffic_ctl config reload`, users can +selectively reload a single virtual host entry without affecting other virtual host entries. + +By only updating the necessary changes, this reduces configuration deployment time and improves visibility on the changes made. + +To reload for a specific virtual host, use new reload directive: + +:: + + $ traffic_ctl config reload -D virtualhost.id=<id> + +Where **<id>** is the virtual host ID defined in :file:`virtualhost.yaml`. Only the **<id>** virtual host +configuration will be reloaded. This does not affect other virtual hosts or global configuration files. + +Example: + +:: + + $ traffic_ctl config reload -D virtualhost.id=foo + ✔ Reload scheduled [rpc-123456789] + + Monitor : traffic_ctl config reload -t rpc-123456789 -m + Details : traffic_ctl config reload -t rpc-123456789 -s -l + + $ traffic_ctl config reload -t rpc-123456789 -s -l + ✗ Token 'rpc-123456789' already in use + ✔ Reload [success] — rpc-123456789 + Started : 2026 May 19 19:20:20.691 + Finished: 2026 May 19 19:20:20.692 + Duration: 1ms + + ✔ 1 success ◌ 0 in-progress ✗ 0 failed (1 total) + + Tasks: + ✔ virtualhost ·································· 1ms + [Note] Reloaded virtualhost entry: foo + +The **<id>** must name an entry. An empty value is rejected rather than treated as a request to +reload the whole file, so a reload scoped to one entry never rebuilds the entire table from disk. + +A reload, whether of a single entry or of the whole file, fails and leaves the running +configuration in place if :file:`virtualhost.yaml` is missing, empty, or invalid. Only the initial +load at startup treats a missing or empty file as "no virtual hosts configured"; on a reload, +replacing a live routing table with an empty one is reported as a failure instead. + + +Examples +======== + +.. code-block:: yaml + + # virtualhost.yaml + virtualhost: + - id: example + domains: + - example.com + + remap: + - type: map + from: + url: http://example.com + to: + url: http://origin.example.com/ + + # remap.yaml + remap: + - type: map + from: + url: http://www.x.com + to: + url: http://other.example.com/ + +This rules translates in the following translation. Review Comment: Fix grammar: change 'This rules translates' to 'These rules translate' (or similar) for correct subject/verb agreement. ########## tests/gold_tests/remap/virtualhost_remap.test.py: ########## @@ -0,0 +1,255 @@ +''' +Verify that per-domain remap tables in virtualhost.yaml are applied to requests. +''' +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +Test.Summary = ''' +Verify virtualhost.yaml domain resolution and per-domain remap rules on the request path. +''' + +import os + +Test.ContinueOnFail = True +Test.testName = 'virtualhost_remap' + +ts = Test.MakeATSProcess("ts") + +# The origin is keyed on the request path only (the default lookup key), so each +# remap rule can be given a private target path. Which rule won is then decided +# by which body comes back, not merely by getting a 200. +server = Test.MakeOriginServer("server") + + +def add_origin_response(path: str, body: str) -> None: + """Register an origin response for `path` carrying a rule-specific body.""" + request_header = { + "headers": f"GET {path} HTTP/1.1\r\nHost: origin.example.com\r\n\r\n", + "timestamp": "1469733493.993", + "body": "" + } + response_header = {"headers": "HTTP/1.1 200 OK\r\nConnection: close\r\n\r\n", "timestamp": "1469733493.993", "body": body} + server.addResponse("sessionfile.log", request_header, response_header) + + +# One target path per remap rule that could plausibly fire. The bodies are chosen +# so that no expected body is a substring of another. +add_origin_response("/vhost-exact-domain/", "hit:vhost-exact-domain") +add_origin_response("/vhost-deep-wildcard/", "hit:vhost-deep-wildcard") +add_origin_response("/vhost-wide-wildcard/", "hit:vhost-wide-wildcard") +add_origin_response("/vhost-wildcard-precedence/", "hit:vhost-wildcard-precedence") +add_origin_response("/vhost-exact-precedence/", "hit:vhost-exact-precedence") +add_origin_response("/vhost-fallback-rule/", "hit:vhost-fallback-rule") +add_origin_response("/global-fallback/other/", "hit:global-fallback") +add_origin_response("/global-plain/", "hit:global-plain") +# Only reachable if the rejected reload below is wrongly published. +add_origin_response("/vhost-conflict/", "hit:vhost-conflict") + +ts.Disk.records_config.update({ + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'virtualhost|url_rewrite', +}) + +# The refused reload at the end of this test logs the conflict as an ERROR, which +# replaces the default diags expectations. +ts.Disk.diags_log.Content = Testers.ContainsExpression( + "is already claimed by virtualhost 'exact-only'", "The conflicting reload should name the virtualhost holding the domain") +ts.Disk.diags_log.Content += Testers.ExcludesExpression("FATAL:", "A refused reload must not be fatal") + +origin = f'127.0.0.1:{server.Variables.Port}' + +# Global table, in remap.config (legacy) format. The virtualhost tables below are +# always YAML, so this also covers the mixed case. +ts.Disk.remap_config.AddLines( + [ + f'map http://fallback.example.net/ http://{origin}/global-fallback/', + f'map http://none.example.net/ http://{origin}/global-plain/', + ]) + +vhost_config_lines = [ + 'virtualhost:', + # Plain exact-domain match. No wildcard in this config matches .example.org. + ' - id: exact-only', + ' domains:', + ' - exact.example.org', + ' remap:', + ' - type: map', + ' from:', + ' url: http://exact.example.org/', + ' to:', + f' url: http://{origin}/vhost-exact-domain/', + # x.deep.example.com matches both this wildcard and the wider one below. + # The longest (most specific) suffix must win. + ' - id: deep-wildcard', + ' domains:', + ' - "*.deep.example.com"', + ' remap:', + ' - type: map', + ' from:', + ' url: http://x.deep.example.com/', + ' to:', + f' url: http://{origin}/vhost-deep-wildcard/', + ' - id: wide-wildcard', + ' domains:', + ' - "*.example.com"', + ' remap:', + ' - type: map', + ' from:', + ' url: http://x.deep.example.com/', + ' to:', + f' url: http://{origin}/vhost-wide-wildcard/', + ' - type: map', + ' from:', + ' url: http://precedence.example.com/', + ' to:', + f' url: http://{origin}/vhost-wildcard-precedence/', + # precedence.example.com is claimed exactly here and by the wildcard + # above. The exact domain must win. + ' - id: exact-precedence', + ' domains:', + ' - precedence.example.com', + ' remap:', + ' - type: map', + ' from:', + ' url: http://precedence.example.com/', + ' to:', + f' url: http://{origin}/vhost-exact-precedence/', + # This virtualhost resolves for fallback.example.net but its only rule + # covers a different path, so requests elsewhere must fall back to the + # global table. + ' - id: path-miss', + ' domains:', + ' - fallback.example.net', + ' remap:', + ' - type: map', + ' from:', + ' url: http://fallback.example.net/only-here/', + ' to:', + f' url: http://{origin}/vhost-fallback-rule/', +] + +ts.Disk.virtualhost_yaml.AddLines(vhost_config_lines) + + +def add_request(name: str, host: str, path: str, expected: str, not_expected: str = "") -> 'TestRun': + """Send one request through the proxy and assert which remap rule served it. + + :param name: Test run name. + :param host: Host header, which selects the virtualhost. + :param path: Request path. + :param expected: Body of the rule that must have won. + :param not_expected: Body of the rule that must have lost, if any. + """ + tr = Test.AddTestRun(name) + tr.MakeCurlCommand(f'-s -H"Host: {host}" http://127.0.0.1:{ts.Variables.port}{path} --verbose', ts=ts) + tr.Processes.Default.ReturnCode = 0 + tr.Processes.Default.Streams.stdout = Testers.ContainsExpression(expected, f"{host}{path} should be served by {expected}") + if not_expected: + tr.Processes.Default.Streams.stdout += Testers.ExcludesExpression( + not_expected, f"{host}{path} must not be served by {not_expected}") + tr.StillRunningAfter = ts + tr.StillRunningAfter = server Review Comment: These two assignments will overwrite each other in normal Python attribute semantics, which can cause the test runner to only enforce one of the two processes as 'still running'. If the harness supports multiple dependencies, set them in a single assignment (e.g., a list/tuple) or use the harness’ documented API for adding multiple `StillRunningAfter` processes. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
