Copilot commented on code in PR #13108: URL: https://github.com/apache/trafficserver/pull/13108#discussion_r3815361607
########## src/proxy/VirtualHost.cc: ########## @@ -0,0 +1,468 @@ +/** @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 <cerrno> +#include <memory> +#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/ConfigRegistry.h" +#include "records/RecCore.h" +#include "tscore/Filenames.h" +#include "tsutil/Convert.h" + +namespace +{ +DbgCtl dbg_ctl_virtualhost("virtualhost"); +} + +int VirtualHost::_configid = 0; + +std::string +VirtualHostConfig::Entry::get_id() const +{ + return id; +} + +std::set<std::string> valid_vhost_keys = {"id", "domains", "remap"}; + +template <> struct YAML::convert<VirtualHostConfig::Entry> { + static bool + decode(const YAML::Node &node, VirtualHostConfig::Entry &item) + { + for (const auto &elem : node) { + if (std::none_of(valid_vhost_keys.begin(), valid_vhost_keys.end(), + [&elem](const std::string &s) { return s == elem.first.as<std::string>(); })) { + Warning("unsupported key '%s' in VirtualHost config", elem.first.as<std::string>().c_str()); + } + } + + if (!node["id"]) { + Error("Virtualhost entry at line %d must provide `id`", node.Mark().line + 1); + return false; + } + item.id = node["id"].as<std::string>(); + + auto domains = node["domains"]; + if (!domains || !domains.IsSequence() || domains.size() == 0) { + 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()) { + 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) { + Error("Virtualhost '%s' wildcard '%s' must match '*.[domain]' format (line %d)", item.id.c_str(), domain, + it.Mark().line + 1); + return false; + } + item.wildcard_domains.emplace_back(domain + 2); + } else { + item.exact_domains.emplace_back(domain); + } + } + + if (item.exact_domains.empty() && item.wildcard_domains.empty()) { + 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) +{ + entry.clear(); + Ptr<VirtualHostConfig::Entry> vhost = make_ptr(new VirtualHostConfig::Entry); + auto &conf = *vhost; + try { + if (!YAML::convert<VirtualHostConfig::Entry>::decode(node, conf)) { + return false; + } + } catch (YAML::Exception const &ex) { + 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>(); + 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; + } + conf.remap_table = make_managed_url_rewrite(std::move(table)); + } + entry = std::move(vhost); + return true; +} + +bool +VirtualHostConfig::load() +{ + _entries.clear(); + std::string config_path = RecConfigReadConfigPath("proxy.config.virtualhost.filename", ts::filename::VIRTUALHOST); + + struct stat sbuf; + if (stat(config_path.c_str(), &sbuf) == -1 && errno == ENOENT) { + Warning("Virtualhost configuration '%s' doesn't exist", config_path.c_str()); + return true; + } + + try { + YAML::Node config = YAML::LoadFile(config_path); + if (config.IsNull()) { + Dbg(dbg_ctl_virtualhost, "Empty virtualhost config: %s", config_path.c_str()); + return true; + } + + config = config["virtualhost"]; + if (config.IsNull() || !config.IsSequence()) { + Error("%s: expected toplevel 'virtualhost' key to be a sequence", config_path.c_str()); + return false; + } + + for (auto const &node : config) { + Ptr<Entry> entry; + if (!build_virtualhost_entry(node, entry)) { + return false; + } + + std::string vhost_id{entry->id}; + if (_entries.contains(vhost_id)) { + Error("%s: duplicate virtualhost id '%s' (line %d)", config_path.c_str(), vhost_id.c_str(), node.Mark().line + 1); + return false; + } + + for (auto const &domain : entry->exact_domains) { + if (_exact_domains_to_id.contains(domain)) { + Error("%s: domain '%s' in virtualhost '%s' is already claimed by virtualhost '%s'", config_path.c_str(), domain.c_str(), + vhost_id.c_str(), _exact_domains_to_id.at(domain).c_str()); + return false; + } + _exact_domains_to_id.emplace(domain, vhost_id); + } + + for (auto const &domain_suffix : entry->wildcard_domains) { + if (_wildcard_domains_to_id.contains(domain_suffix)) { + Error("%s: wildcard domain '*.%s' in virtualhost '%s' is already claimed by virtualhost '%s'", config_path.c_str(), + domain_suffix.c_str(), vhost_id.c_str(), _wildcard_domains_to_id.at(domain_suffix).c_str()); + return false; + } + _wildcard_domains_to_id.emplace(domain_suffix, vhost_id); + } + + _entries.emplace(vhost_id, std::move(entry)); + } + + } catch (std::exception &ex) { + Error("Failed to load %s: %s", config_path.c_str(), ex.what()); + return false; + } + return true; +} + +bool +VirtualHostConfig::load_entry(std::string_view id, Ptr<Entry> &entry) +{ + entry.clear(); + std::string config_path = RecConfigReadConfigPath("proxy.config.virtualhost.filename", ts::filename::VIRTUALHOST); + + try { + YAML::Node config = YAML::LoadFile(config_path); + if (config.IsNull()) { + Dbg(dbg_ctl_virtualhost, "Empty virtualhost config: %s", config_path.c_str()); + return false; + } + + config = config["virtualhost"]; + if (config.IsNull() || !config.IsSequence()) { + Error("%s: expected toplevel 'virtualhost' key to be a sequence", config_path.c_str()); + return false; + } + + for (auto const &node : config) { + auto config_id = node["id"]; + if (!config_id || config_id.as<std::string>() != id) { + continue; + } + + Ptr<Entry> vhost_entry; + if (!build_virtualhost_entry(node, vhost_entry)) { + return false; + } + entry = std::move(vhost_entry); + return true; + } + + } catch (std::exception &ex) { + Error("Failed to load virtualhost entry '%.*s' in %s: %s", static_cast<int>(id.size()), id.data(), config_path.c_str(), + ex.what()); + return false; + } + Error("%s: virtualhost with id '%.*s' not found", config_path.c_str(), static_cast<int>(id.size()), id.data()); + return false; +} + +bool +VirtualHostConfig::set_entry(std::string_view id, Ptr<Entry> &entry) +{ + std::string vhost_id{id}; + // If virtualhost entry already exists, remove current entry + if (auto it = _entries.find(vhost_id); it != _entries.end()) { + Ptr<Entry> curr_entry = std::move(it->second); + for (auto const &domain : curr_entry->exact_domains) { + _exact_domains_to_id.erase(domain); + } + for (auto const &domain : curr_entry->wildcard_domains) { + _wildcard_domains_to_id.erase(domain); + } + _entries.erase(vhost_id); + } + + // Add new entry into virtualhost config + if (entry) { + for (auto const &domain : entry->exact_domains) { + if (_exact_domains_to_id.contains(domain)) { + Error("Domain '%s' in virtualhost '%s' is already claimed by virtualhost '%s'", domain.c_str(), vhost_id.c_str(), + _exact_domains_to_id.at(domain).c_str()); + return false; + } + _exact_domains_to_id.emplace(domain, vhost_id); + } + + for (auto const &domain_suffix : entry->wildcard_domains) { + if (_wildcard_domains_to_id.contains(domain_suffix)) { + Error("Wildcard domain '*.%s' in virtualhost '%s' is already claimed by virtualhost '%s'", domain_suffix.c_str(), + vhost_id.c_str(), _wildcard_domains_to_id.at(domain_suffix).c_str()); + return false; + } Review Comment: `set_entry()` deletes the existing entry (and erases its domain claims) before validating that the replacement can be installed. If the new entry conflicts and returns `false`, the old entry is already removed, causing an unintended config drop. Consider validating conflicts first (or performing a two-phase update with rollback) so a failed single-entry reload leaves the previous virtualhost entry intact. ########## src/proxy/VirtualHost.cc: ########## @@ -0,0 +1,468 @@ +/** @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 <cerrno> +#include <memory> +#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/ConfigRegistry.h" +#include "records/RecCore.h" +#include "tscore/Filenames.h" +#include "tsutil/Convert.h" + +namespace +{ +DbgCtl dbg_ctl_virtualhost("virtualhost"); +} + +int VirtualHost::_configid = 0; + +std::string +VirtualHostConfig::Entry::get_id() const +{ + return id; +} + +std::set<std::string> valid_vhost_keys = {"id", "domains", "remap"}; + +template <> struct YAML::convert<VirtualHostConfig::Entry> { + static bool + decode(const YAML::Node &node, VirtualHostConfig::Entry &item) + { + for (const auto &elem : node) { + if (std::none_of(valid_vhost_keys.begin(), valid_vhost_keys.end(), + [&elem](const std::string &s) { return s == elem.first.as<std::string>(); })) { + Warning("unsupported key '%s' in VirtualHost config", elem.first.as<std::string>().c_str()); + } + } + + if (!node["id"]) { + Error("Virtualhost entry at line %d must provide `id`", node.Mark().line + 1); + return false; + } + item.id = node["id"].as<std::string>(); + + auto domains = node["domains"]; + if (!domains || !domains.IsSequence() || domains.size() == 0) { + 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()) { + 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) { + Error("Virtualhost '%s' wildcard '%s' must match '*.[domain]' format (line %d)", item.id.c_str(), domain, + it.Mark().line + 1); + return false; + } + item.wildcard_domains.emplace_back(domain + 2); + } else { + item.exact_domains.emplace_back(domain); + } + } + + if (item.exact_domains.empty() && item.wildcard_domains.empty()) { + 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) +{ + entry.clear(); + Ptr<VirtualHostConfig::Entry> vhost = make_ptr(new VirtualHostConfig::Entry); + auto &conf = *vhost; + try { + if (!YAML::convert<VirtualHostConfig::Entry>::decode(node, conf)) { + return false; + } + } catch (YAML::Exception const &ex) { + 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>(); + 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; + } + conf.remap_table = make_managed_url_rewrite(std::move(table)); + } + entry = std::move(vhost); + return true; +} + +bool +VirtualHostConfig::load() +{ + _entries.clear(); Review Comment: `VirtualHostConfig::load()` clears `_entries` but does not clear `_exact_domains_to_id` / `_wildcard_domains_to_id`. This will leave stale domain→id mappings across reloads and can cause false 'already claimed' errors or incorrect routing after reload. Clear both maps at the start of `load()` (alongside `_entries.clear()`). ########## src/proxy/http/remap/UrlRewrite.cc: ########## @@ -95,6 +95,15 @@ UrlRewrite::load(ConfigContext ctx) return false; } } + return load_table(std::string(config_file_path.get()), nullptr, ctx); +} + +bool +UrlRewrite::load_table(const std::string &config_file_path, YAML::Node const *remap_node, ConfigContext ctx) +{ + if (remap_node) { + this->_remap_yaml = true; + } Review Comment: `load_table()` sets `_remap_yaml` to `true` when `remap_node` is provided, but does not reset it when `remap_node` is null. If the same `UrlRewrite` instance is reused across loads, this can cause `BuildTable()` to parse a non-YAML remap file as YAML (or otherwise carry stale state). Set `_remap_yaml` deterministically for each call (e.g., `_remap_yaml = (remap_node != nullptr)` and/or derive from the file extension when `remap_node == nullptr`). -- 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]
