Copilot commented on code in PR #13108:
URL: https://github.com/apache/trafficserver/pull/13108#discussion_r4077496140
##########
src/proxy/http/remap/RemapYamlConfig.cc:
##########
@@ -1046,3 +1091,20 @@ remap_parse_yaml(const char *path, UrlRewrite *rewrite,
ConfigContext ctx)
return status;
}
+
+bool
+remap_parse_yaml(YAML::Node const *remap_node, UrlRewrite *rewrite,
ConfigContext ctx)
+{
+ BUILD_TABLE_INFO bti;
+
+ rewrite->pluginFactory.indicatePreReload();
Review Comment:
Each virtualhost invokes this YAML overload, but
`PluginFactory::indicatePreReload()` and `indicatePostReload()` broadcast over
the process-wide loaded-plugin list, while the `pluginUsed` map is local to the
current factory. With multiple virtualhost tables (or a global table plus a
virtualhost table), later builds send another reload notification and mark
plugins used only by earlier tables as
`TSREMAP_CONFIG_RELOAD_SUCCESS_PLUGIN_UNUSED`, so plugin reload callbacks can
reset live state. Aggregate one notification for the whole virtualhost rebuild
or make the callbacks table-scoped.
##########
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>();
Review Comment:
An empty `id` is accepted during startup, but the reload handler explicitly
rejects empty IDs, leaving a live virtualhost entry that cannot be addressed by
the documented single-entry reload. Reject empty identifiers while decoding the
entry.
This issue also appears in the following locations of the same file:
- line 471
- line 547
--
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]
