Copilot commented on code in PR #13070:
URL: https://github.com/apache/trafficserver/pull/13070#discussion_r3054569944
##########
src/proxy/Plugin.cc:
##########
@@ -376,3 +405,256 @@ plugin_init(bool validateOnly)
}
return retVal;
}
+
+config::ConfigResult<PluginYAMLEntries>
+parse_plugin_yaml(const char *yaml_path)
+{
+ config::ConfigResult<PluginYAMLEntries> result;
+ YAML::Node root;
+
+ try {
+ root = YAML::LoadFile(yaml_path);
+ } catch (const YAML::Exception &e) {
+ result.errata.note("failed to parse: {}", e.what());
+ return result;
+ }
+
+ if (!root["plugins"] || !root["plugins"].IsSequence()) {
+ result.errata.note("missing or invalid 'plugins' sequence");
+ return result;
+ }
+
+ struct IndexedEntry {
+ int seq_idx;
+ PluginYAMLEntry entry;
+ };
+
+ std::vector<IndexedEntry> indexed;
+ int seq_idx = 0;
+
+ for (const auto &node : root["plugins"]) {
+ PluginYAMLEntry entry;
+
+ if (!node["path"]) {
+ result.errata.note("plugin entry #{} missing required 'path' field",
seq_idx + 1);
+ return result;
+ }
+ entry.path = node["path"].as<std::string>();
+
+ if (auto n = node["enabled"]; n) {
+ entry.enabled = n.as<bool>();
+ }
+ if (auto n = node["load_order"]; n) {
+ entry.load_order = n.as<int>();
+ }
+ if (auto n = node["params"]; n && n.IsSequence()) {
+ for (const auto &p : n) {
+ entry.params.emplace_back(p.as<std::string>());
+ }
+ }
+ if (auto n = node["config"]; n) {
+ if (n.IsScalar()) {
+ entry.config_literal = n.as<std::string>();
+ } else {
+ result.errata.note("plugin '{}': 'config' must be a scalar (use
literal block '|' for multi-line content)", entry.path);
+ return result;
+ }
Review Comment:
The `config` field is documented (and described in the PR) as only accepting
a literal block scalar (`|`), but the parser currently accepts any scalar style
(including single-line scalars / quoted scalars / folded `>`). This is either a
behavior bug (should reject non-literal scalars) or the docs/PR text need to be
updated to match the actual accepted input.
##########
src/config/unit_tests/test_plugin_config.cc:
##########
@@ -0,0 +1,206 @@
+/** @file
+
+ Unit tests for plugin.config parser and marshaller.
+
+ @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 <catch2/catch_test_macros.hpp>
+#include <filesystem>
+#include <fstream>
+
+#include <yaml-cpp/yaml.h>
+
+#include "config/plugin_config.h"
+
+namespace
+{
+
+class TempFile
+{
+public:
+ TempFile(std::string const &filename, std::string const &content)
+ {
+ _path = std::filesystem::temp_directory_path() / filename;
+ std::ofstream ofs(_path);
+ ofs << content;
+ }
Review Comment:
These unit tests create temp files with deterministic names (e.g.,
`plugin_basic.config`) in the global temp directory. With parallel test
execution, different processes can overwrite each other's files and cause
flakes. Prefer creating truly unique temp paths (e.g., `mkstemp`/random
suffix/PID) per test instance and validate the file write succeeded.
##########
src/proxy/Plugin.cc:
##########
@@ -134,7 +157,7 @@ plugin_dso_load(const char *path, void *&handle, void
*&init, std::string &error
return true;
}
-static bool
+bool
Review Comment:
`single_plugin_init` no longer has internal linkage (it was `static`
before). It isn't declared in a header and is only used within this TU, so
exporting it as a global symbol is unnecessary and increases the risk of symbol
collisions. Consider restoring `static` linkage.
```suggestion
static bool
```
##########
src/config/plugin_config.cc:
##########
@@ -0,0 +1,187 @@
+/** @file
+
+ Plugin configuration parsing and marshalling.
+
+ @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 "config/plugin_config.h"
+
+#include <fstream>
+#include <sstream>
+
+#include <yaml-cpp/yaml.h>
+
+namespace
+{
+
+constexpr char KEY_PLUGINS[] = "plugins";
+
+std::vector<std::string>
+tokenize_plugin_line(std::string_view line)
+{
+ std::vector<std::string> tokens;
+ std::size_t i = 0;
+
+ while (i < line.size()) {
+ while (i < line.size() && (line[i] == ' ' || line[i] == '\t')) {
+ ++i;
+ }
+ if (i >= line.size() || line[i] == '#') {
+ break;
+ }
+
+ if (line[i] == '"') {
+ ++i;
+ std::size_t start = i;
+ while (i < line.size() && line[i] != '"') {
+ ++i;
+ }
+ tokens.emplace_back(line.substr(start, i - start));
+ if (i < line.size()) {
+ ++i;
+ }
+ } else {
+ std::size_t start = i;
+ while (i < line.size() && line[i] != ' ' && line[i] != '\t' && line[i]
!= '#') {
+ ++i;
+ }
+ tokens.emplace_back(line.substr(start, i - start));
+ }
+ }
+ return tokens;
+}
+
+void
+emit_entry(YAML::Emitter &yaml, config::PluginConfigEntry const &entry)
+{
+ yaml << YAML::BeginMap;
+ yaml << YAML::Key << "path" << YAML::Value << entry.path;
+
+ if (!entry.enabled) {
+ yaml << YAML::Key << "enabled" << YAML::Value << false;
+ }
+
+ if (!entry.args.empty()) {
+ yaml << YAML::Key << "params" << YAML::Value << YAML::BeginSeq;
+ for (auto const &arg : entry.args) {
+ yaml << arg;
+ }
+ yaml << YAML::EndSeq;
+ }
+
+ yaml << YAML::EndMap;
+}
+
+} // namespace
+
+namespace config
+{
+
+ConfigResult<PluginConfigData>
+PluginConfigParser::parse(std::string const &filename)
+{
+ std::ifstream file(filename);
+
+ if (!file.is_open()) {
+ ConfigResult<PluginConfigData> result;
+ result.file_not_found = true;
+ result.errata.note("unable to open '{}'", filename);
+ return result;
+ }
+
+ std::ostringstream ss;
+ ss << file.rdbuf();
+ return parse_legacy(ss.str());
+}
+
+ConfigResult<PluginConfigData>
+PluginConfigParser::parse_legacy(std::string_view content)
+{
+ ConfigResult<PluginConfigData> result;
+ std::istringstream stream{std::string{content}};
+ std::string line;
+
+ while (std::getline(stream, line)) {
+ std::string_view sv{line};
+
+ std::size_t start = 0;
+ while (start < sv.size() && (sv[start] == ' ' || sv[start] == '\t')) {
+ ++start;
+ }
+ sv = sv.substr(start);
+
+ if (sv.empty()) {
+ continue;
+ }
+
+ bool commented = false;
+ if (sv[0] == '#') {
+ commented = true;
+ sv = sv.substr(1);
+ start = 0;
+ while (start < sv.size() && (sv[start] == ' ' || sv[start] == '\t')) {
+ ++start;
+ }
+ sv = sv.substr(start);
+ }
+
+ if (sv.empty()) {
+ continue;
+ }
+
+ auto tokens = tokenize_plugin_line(sv);
+ if (tokens.empty()) {
+ continue;
+ }
+
+ // Heuristic: a plugin line must have a .so path as first token
+ if (tokens[0].find(".so") == std::string::npos) {
+ continue;
+ }
+
Review Comment:
The legacy `plugin.config` loader accepts any first token as the plugin
path, but this parser silently skips lines unless the first token contains the
substring `.so`. That can drop valid plugin entries (e.g., absolute paths
without `.so`, platform-specific extensions, or other naming conventions)
during conversion. Consider removing this heuristic and instead only skip truly
empty/comment-only lines (or emit a warning when skipping).
```suggestion
```
##########
src/proxy/Plugin.cc:
##########
@@ -376,3 +405,256 @@ plugin_init(bool validateOnly)
}
return retVal;
}
+
+config::ConfigResult<PluginYAMLEntries>
+parse_plugin_yaml(const char *yaml_path)
+{
+ config::ConfigResult<PluginYAMLEntries> result;
+ YAML::Node root;
+
+ try {
+ root = YAML::LoadFile(yaml_path);
+ } catch (const YAML::Exception &e) {
+ result.errata.note("failed to parse: {}", e.what());
+ return result;
+ }
+
+ if (!root["plugins"] || !root["plugins"].IsSequence()) {
+ result.errata.note("missing or invalid 'plugins' sequence");
+ return result;
+ }
+
+ struct IndexedEntry {
+ int seq_idx;
+ PluginYAMLEntry entry;
+ };
+
+ std::vector<IndexedEntry> indexed;
+ int seq_idx = 0;
+
+ for (const auto &node : root["plugins"]) {
+ PluginYAMLEntry entry;
+
+ if (!node["path"]) {
+ result.errata.note("plugin entry #{} missing required 'path' field",
seq_idx + 1);
+ return result;
+ }
+ entry.path = node["path"].as<std::string>();
+
+ if (auto n = node["enabled"]; n) {
+ entry.enabled = n.as<bool>();
+ }
+ if (auto n = node["load_order"]; n) {
+ entry.load_order = n.as<int>();
+ }
+ if (auto n = node["params"]; n && n.IsSequence()) {
+ for (const auto &p : n) {
+ entry.params.emplace_back(p.as<std::string>());
+ }
+ }
+ if (auto n = node["config"]; n) {
+ if (n.IsScalar()) {
+ entry.config_literal = n.as<std::string>();
+ } else {
+ result.errata.note("plugin '{}': 'config' must be a scalar (use
literal block '|' for multi-line content)", entry.path);
+ return result;
+ }
+ }
+
+ indexed.push_back({seq_idx++, std::move(entry)});
+ }
+
+ std::stable_sort(indexed.begin(), indexed.end(), [](const IndexedEntry &a,
const IndexedEntry &b) {
+ const bool a_has = a.entry.load_order >= 0;
+ const bool b_has = b.entry.load_order >= 0;
+
+ if (a_has && b_has) {
+ return a.entry.load_order < b.entry.load_order;
+ }
+ return a_has && !b_has;
+ });
+
+ result.value.reserve(indexed.size());
+ for (auto &[_, entry] : indexed) {
+ result.value.emplace_back(std::move(entry));
+ }
+
+ return result;
+}
+
+/// Write inline config content to a temp file, returning the path on success.
+static std::optional<std::string>
+write_inline_config(const PluginYAMLEntry &entry, int index)
+{
+ char tmp_path[PATH_NAME_MAX];
+
+ std::string_view stem{entry.path};
+ if (auto pos = stem.rfind('/'); pos != std::string_view::npos) {
+ stem = stem.substr(pos + 1);
+ }
+ if (auto pos = stem.rfind('.'); pos != std::string_view::npos) {
+ stem = stem.substr(0, pos);
+ }
+
+ snprintf(tmp_path, sizeof(tmp_path), "%s/.%.*s_inline_%d.conf",
RecConfigReadConfigDir().c_str(), static_cast<int>(stem.size()),
+ stem.data(), index);
+
+ int fd = open(tmp_path, O_WRONLY | O_CREAT | O_TRUNC, 0644);
+ if (fd < 0) {
+ Error("%s: failed to create temp config for %s: %s",
ts::filename::PLUGIN_YAML, entry.path.c_str(), strerror(errno));
+ return std::nullopt;
+ }
+
+ auto n = write(fd, entry.config_literal.data(), entry.config_literal.size());
+ close(fd);
+
+ if (n < 0 || static_cast<size_t>(n) != entry.config_literal.size()) {
+ Error("%s: failed to write inline config for %s",
ts::filename::PLUGIN_YAML, entry.path.c_str());
+ return std::nullopt;
+ }
+
+ return std::string(tmp_path);
+}
+
+/// Build the argv for a single plugin: [path, inline_config_path?, params...,
$record expansions].
+static std::optional<std::vector<std::string>>
+build_plugin_args(const PluginYAMLEntry &entry, int index)
+{
+ std::vector<std::string> args;
+ args.emplace_back(entry.path);
+
+ if (!entry.config_literal.empty()) {
+ if (auto path = write_inline_config(entry, index); path) {
+ args.emplace_back(std::move(*path));
+ } else {
+ return std::nullopt;
+ }
+ }
+
+ for (const auto &p : entry.params) {
+ args.emplace_back(p);
+ }
+
+ return args;
+}
+
+static void
+log_plugin_load_summary(int loaded, int disabled)
+{
+ Note("%s: %d plugins loaded, %d disabled", ts::filename::PLUGIN_YAML,
loaded, disabled);
+
+ for (const auto &e : s_plugin_load_summary.entries) {
+ if (e.enabled) {
+ if (e.load_order >= 0) {
+ Note(" #%d %-30s load_order: %-5d loaded", e.index, e.path.c_str(),
e.load_order);
+ } else {
+ Note(" #%d %-30s loaded", e.index, e.path.c_str());
+ }
+ } else {
+ Note(" -- %-30s disabled", e.path.c_str());
+ }
+ }
+}
+
+static void
+cleanup_inline_configs()
+{
+ std::string config_dir = RecConfigReadConfigDir();
+ std::error_code ec;
+
+ try {
+ for (const auto &entry : std::filesystem::directory_iterator(config_dir,
ec)) {
+ if (!entry.is_regular_file()) {
+ continue;
+ }
+ auto name = entry.path().filename().string();
+ if (name.front() == '.' && name.find("_inline_") != std::string::npos &&
name.ends_with(".conf")) {
+ std::filesystem::remove(entry.path(), ec);
+ }
+ }
+ } catch (const std::exception &e) {
+ Error("%s: error cleaning up inline config files: %s",
ts::filename::PLUGIN_YAML, e.what());
+ }
+}
+
+bool
+plugin_yaml_init(bool validateOnly)
+{
+ plugin_dir_init();
+
+ ats_scoped_str yaml_path;
+
+ cleanup_inline_configs();
+
+ yaml_path = RecConfigReadConfigPath(nullptr, ts::filename::PLUGIN_YAML);
+ if (access(yaml_path, R_OK) != 0) {
+ return plugin_init(validateOnly);
+ }
+
+ Note("%s loading ...", ts::filename::PLUGIN_YAML);
+
+ auto result = parse_plugin_yaml(yaml_path.get());
+ if (!result.ok()) {
+ Error("%s: %s", ts::filename::PLUGIN_YAML,
std::string(result.errata.front().text()).c_str());
+ return false;
+ }
+
+ s_plugin_load_summary.source = ts::filename::PLUGIN_YAML;
+ s_plugin_load_summary.entries.clear();
+
+ bool retVal = true;
+ int index = 0;
+ int loaded = 0;
+ int disabled = 0;
+
+ for (const auto &entry : result.value) {
+ ++index;
+
+ if (!entry.enabled) {
+ Note("plugin #%d skipped: %s (enabled: false)", index,
entry.path.c_str());
+ s_plugin_load_summary.entries.push_back({entry.path, entry.load_order,
false, false, index});
+ ++disabled;
+ continue;
+ }
+
+ auto args = build_plugin_args(entry, index);
+ if (!args) {
+ return false;
+ }
+
+ std::vector<char *> argv_ptrs;
+ std::vector<char *> expanded;
+
+ for (auto &a : *args) {
+ char *var = plugin_expand(a.data(), ts::filename::PLUGIN_YAML);
+ expanded.emplace_back(var);
+ argv_ptrs.emplace_back(var ? var : a.data());
+ }
+ argv_ptrs.emplace_back(nullptr);
Review Comment:
Unlike the legacy `plugin.config` loader (which enforces `MAX_PLUGIN_ARGS`),
the YAML loader builds an argv vector with no upper bound. This can allow
unexpectedly large argv/heap usage and diverges from the existing safety limit.
Consider enforcing the same max-args limit (and returning a clear error) for
`plugin.yaml` as well.
##########
src/proxy/Plugin.cc:
##########
@@ -376,3 +405,256 @@ plugin_init(bool validateOnly)
}
return retVal;
}
+
+config::ConfigResult<PluginYAMLEntries>
+parse_plugin_yaml(const char *yaml_path)
+{
+ config::ConfigResult<PluginYAMLEntries> result;
+ YAML::Node root;
+
+ try {
+ root = YAML::LoadFile(yaml_path);
+ } catch (const YAML::Exception &e) {
+ result.errata.note("failed to parse: {}", e.what());
+ return result;
+ }
+
+ if (!root["plugins"] || !root["plugins"].IsSequence()) {
+ result.errata.note("missing or invalid 'plugins' sequence");
+ return result;
+ }
+
+ struct IndexedEntry {
+ int seq_idx;
+ PluginYAMLEntry entry;
+ };
+
+ std::vector<IndexedEntry> indexed;
+ int seq_idx = 0;
+
+ for (const auto &node : root["plugins"]) {
+ PluginYAMLEntry entry;
+
+ if (!node["path"]) {
+ result.errata.note("plugin entry #{} missing required 'path' field",
seq_idx + 1);
+ return result;
+ }
+ entry.path = node["path"].as<std::string>();
+
+ if (auto n = node["enabled"]; n) {
+ entry.enabled = n.as<bool>();
+ }
+ if (auto n = node["load_order"]; n) {
+ entry.load_order = n.as<int>();
+ }
+ if (auto n = node["params"]; n && n.IsSequence()) {
+ for (const auto &p : n) {
+ entry.params.emplace_back(p.as<std::string>());
+ }
+ }
+ if (auto n = node["config"]; n) {
+ if (n.IsScalar()) {
+ entry.config_literal = n.as<std::string>();
+ } else {
+ result.errata.note("plugin '{}': 'config' must be a scalar (use
literal block '|' for multi-line content)", entry.path);
+ return result;
+ }
+ }
+
+ indexed.push_back({seq_idx++, std::move(entry)});
+ }
+
+ std::stable_sort(indexed.begin(), indexed.end(), [](const IndexedEntry &a,
const IndexedEntry &b) {
+ const bool a_has = a.entry.load_order >= 0;
+ const bool b_has = b.entry.load_order >= 0;
+
+ if (a_has && b_has) {
+ return a.entry.load_order < b.entry.load_order;
+ }
+ return a_has && !b_has;
+ });
+
+ result.value.reserve(indexed.size());
+ for (auto &[_, entry] : indexed) {
+ result.value.emplace_back(std::move(entry));
+ }
+
+ return result;
+}
+
+/// Write inline config content to a temp file, returning the path on success.
+static std::optional<std::string>
+write_inline_config(const PluginYAMLEntry &entry, int index)
+{
+ char tmp_path[PATH_NAME_MAX];
+
+ std::string_view stem{entry.path};
+ if (auto pos = stem.rfind('/'); pos != std::string_view::npos) {
+ stem = stem.substr(pos + 1);
+ }
+ if (auto pos = stem.rfind('.'); pos != std::string_view::npos) {
+ stem = stem.substr(0, pos);
+ }
+
+ snprintf(tmp_path, sizeof(tmp_path), "%s/.%.*s_inline_%d.conf",
RecConfigReadConfigDir().c_str(), static_cast<int>(stem.size()),
+ stem.data(), index);
+
+ int fd = open(tmp_path, O_WRONLY | O_CREAT | O_TRUNC, 0644);
+ if (fd < 0) {
+ Error("%s: failed to create temp config for %s: %s",
ts::filename::PLUGIN_YAML, entry.path.c_str(), strerror(errno));
+ return std::nullopt;
+ }
+
+ auto n = write(fd, entry.config_literal.data(), entry.config_literal.size());
+ close(fd);
+
+ if (n < 0 || static_cast<size_t>(n) != entry.config_literal.size()) {
+ Error("%s: failed to write inline config for %s",
ts::filename::PLUGIN_YAML, entry.path.c_str());
+ return std::nullopt;
+ }
Review Comment:
`write_inline_config()` uses a single `write()` call and treats short writes
as a hard failure. `write()` is allowed to return a partial count, so this can
spuriously fail (and/or leave a truncated config file) under EINTR or other
conditions. Use the existing safe-write helper / loop until all bytes are
written, and consider cleaning up the temp file on failure.
##########
src/proxy/unit_tests/test_PluginYAML.cc:
##########
@@ -0,0 +1,324 @@
+/** @file
+
+ Unit tests for plugin.yaml parsing
+
+ @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 <catch2/catch_test_macros.hpp>
+#include <filesystem>
+#include <fstream>
+#include <string>
+
+#include "proxy/Plugin.h"
+
+namespace
+{
+class TempYAML
+{
+public:
+ explicit TempYAML(const std::string &content)
+ {
+ _path = std::filesystem::temp_directory_path() / "test_plugin_yaml.yaml";
+ std::ofstream f(_path);
+ f << content;
+ }
+
+ ~TempYAML() { std::filesystem::remove(_path); }
+
Review Comment:
This unit test helper writes to a fixed filename in the system temp
directory. When CTest runs test executables in parallel (`ctest -j`), multiple
processes can collide on the same temp path and cause flaky failures. Use a
unique filename per instance (e.g., PID + counter or `mkstemp`) and consider
checking `ofstream` open/write success.
```suggestion
#include <atomic>
#include <filesystem>
#include <fstream>
#include <string>
#include <system_error>
#include <unistd.h>
#include "proxy/Plugin.h"
namespace
{
std::atomic<unsigned int> temp_yaml_counter{0};
class TempYAML
{
public:
explicit TempYAML(const std::string &content)
{
auto const unique_id = temp_yaml_counter.fetch_add(1,
std::memory_order_relaxed);
_path = std::filesystem::temp_directory_path() /
("test_plugin_yaml_" + std::to_string(getpid()) + "_" +
std::to_string(unique_id) + ".yaml");
std::ofstream f(_path, std::ios::out | std::ios::trunc);
if (!f.is_open()) {
throw std::system_error(errno, std::generic_category(), "failed to
create temporary plugin.yaml");
}
f << content;
f.flush();
if (!f) {
throw std::runtime_error("failed to write temporary plugin.yaml");
}
}
~TempYAML()
{
std::error_code ec;
std::filesystem::remove(_path, ec);
}
```
##########
src/proxy/Plugin.cc:
##########
@@ -376,3 +405,256 @@ plugin_init(bool validateOnly)
}
return retVal;
}
+
+config::ConfigResult<PluginYAMLEntries>
+parse_plugin_yaml(const char *yaml_path)
+{
+ config::ConfigResult<PluginYAMLEntries> result;
+ YAML::Node root;
+
+ try {
+ root = YAML::LoadFile(yaml_path);
+ } catch (const YAML::Exception &e) {
+ result.errata.note("failed to parse: {}", e.what());
+ return result;
+ }
+
+ if (!root["plugins"] || !root["plugins"].IsSequence()) {
+ result.errata.note("missing or invalid 'plugins' sequence");
+ return result;
+ }
+
+ struct IndexedEntry {
+ int seq_idx;
+ PluginYAMLEntry entry;
+ };
+
+ std::vector<IndexedEntry> indexed;
+ int seq_idx = 0;
+
+ for (const auto &node : root["plugins"]) {
+ PluginYAMLEntry entry;
+
+ if (!node["path"]) {
+ result.errata.note("plugin entry #{} missing required 'path' field",
seq_idx + 1);
+ return result;
+ }
+ entry.path = node["path"].as<std::string>();
+
+ if (auto n = node["enabled"]; n) {
+ entry.enabled = n.as<bool>();
+ }
+ if (auto n = node["load_order"]; n) {
+ entry.load_order = n.as<int>();
+ }
+ if (auto n = node["params"]; n && n.IsSequence()) {
+ for (const auto &p : n) {
+ entry.params.emplace_back(p.as<std::string>());
+ }
+ }
+ if (auto n = node["config"]; n) {
+ if (n.IsScalar()) {
+ entry.config_literal = n.as<std::string>();
+ } else {
+ result.errata.note("plugin '{}': 'config' must be a scalar (use
literal block '|' for multi-line content)", entry.path);
+ return result;
+ }
+ }
+
+ indexed.push_back({seq_idx++, std::move(entry)});
+ }
+
+ std::stable_sort(indexed.begin(), indexed.end(), [](const IndexedEntry &a,
const IndexedEntry &b) {
+ const bool a_has = a.entry.load_order >= 0;
+ const bool b_has = b.entry.load_order >= 0;
+
+ if (a_has && b_has) {
+ return a.entry.load_order < b.entry.load_order;
+ }
+ return a_has && !b_has;
+ });
+
+ result.value.reserve(indexed.size());
+ for (auto &[_, entry] : indexed) {
+ result.value.emplace_back(std::move(entry));
+ }
+
+ return result;
+}
+
+/// Write inline config content to a temp file, returning the path on success.
+static std::optional<std::string>
+write_inline_config(const PluginYAMLEntry &entry, int index)
+{
+ char tmp_path[PATH_NAME_MAX];
+
+ std::string_view stem{entry.path};
+ if (auto pos = stem.rfind('/'); pos != std::string_view::npos) {
+ stem = stem.substr(pos + 1);
+ }
+ if (auto pos = stem.rfind('.'); pos != std::string_view::npos) {
+ stem = stem.substr(0, pos);
+ }
+
+ snprintf(tmp_path, sizeof(tmp_path), "%s/.%.*s_inline_%d.conf",
RecConfigReadConfigDir().c_str(), static_cast<int>(stem.size()),
+ stem.data(), index);
+
+ int fd = open(tmp_path, O_WRONLY | O_CREAT | O_TRUNC, 0644);
+ if (fd < 0) {
+ Error("%s: failed to create temp config for %s: %s",
ts::filename::PLUGIN_YAML, entry.path.c_str(), strerror(errno));
+ return std::nullopt;
+ }
+
+ auto n = write(fd, entry.config_literal.data(), entry.config_literal.size());
+ close(fd);
+
+ if (n < 0 || static_cast<size_t>(n) != entry.config_literal.size()) {
+ Error("%s: failed to write inline config for %s",
ts::filename::PLUGIN_YAML, entry.path.c_str());
+ return std::nullopt;
+ }
+
+ return std::string(tmp_path);
+}
+
+/// Build the argv for a single plugin: [path, inline_config_path?, params...,
$record expansions].
+static std::optional<std::vector<std::string>>
+build_plugin_args(const PluginYAMLEntry &entry, int index)
+{
+ std::vector<std::string> args;
+ args.emplace_back(entry.path);
+
+ if (!entry.config_literal.empty()) {
+ if (auto path = write_inline_config(entry, index); path) {
+ args.emplace_back(std::move(*path));
+ } else {
+ return std::nullopt;
+ }
+ }
+
+ for (const auto &p : entry.params) {
+ args.emplace_back(p);
+ }
+
+ return args;
+}
+
+static void
+log_plugin_load_summary(int loaded, int disabled)
+{
+ Note("%s: %d plugins loaded, %d disabled", ts::filename::PLUGIN_YAML,
loaded, disabled);
+
+ for (const auto &e : s_plugin_load_summary.entries) {
+ if (e.enabled) {
+ if (e.load_order >= 0) {
+ Note(" #%d %-30s load_order: %-5d loaded", e.index, e.path.c_str(),
e.load_order);
+ } else {
+ Note(" #%d %-30s loaded", e.index, e.path.c_str());
+ }
+ } else {
+ Note(" -- %-30s disabled", e.path.c_str());
+ }
+ }
+}
+
+static void
+cleanup_inline_configs()
+{
+ std::string config_dir = RecConfigReadConfigDir();
+ std::error_code ec;
+
+ try {
+ for (const auto &entry : std::filesystem::directory_iterator(config_dir,
ec)) {
+ if (!entry.is_regular_file()) {
+ continue;
+ }
+ auto name = entry.path().filename().string();
+ if (name.front() == '.' && name.find("_inline_") != std::string::npos &&
name.ends_with(".conf")) {
+ std::filesystem::remove(entry.path(), ec);
+ }
+ }
+ } catch (const std::exception &e) {
+ Error("%s: error cleaning up inline config files: %s",
ts::filename::PLUGIN_YAML, e.what());
+ }
+}
+
+bool
+plugin_yaml_init(bool validateOnly)
+{
+ plugin_dir_init();
+
+ ats_scoped_str yaml_path;
+
+ cleanup_inline_configs();
+
+ yaml_path = RecConfigReadConfigPath(nullptr, ts::filename::PLUGIN_YAML);
+ if (access(yaml_path, R_OK) != 0) {
+ return plugin_init(validateOnly);
+ }
+
Review Comment:
`cleanup_inline_configs()` is called unconditionally (even when
`plugin.yaml` is absent and even in validate-only mode). This makes
`traffic_server -V/--verify` and normal startup with only `plugin.config`
delete files in the config dir as a side effect. Consider only cleaning up when
`plugin.yaml` is actually being used (and ideally skip cleanup when
`validateOnly` is true).
```suggestion
yaml_path = RecConfigReadConfigPath(nullptr, ts::filename::PLUGIN_YAML);
if (access(yaml_path, R_OK) != 0) {
return plugin_init(validateOnly);
}
if (!validateOnly) {
cleanup_inline_configs();
}
```
##########
src/proxy/Plugin.cc:
##########
@@ -376,3 +405,256 @@ plugin_init(bool validateOnly)
}
return retVal;
}
+
+config::ConfigResult<PluginYAMLEntries>
+parse_plugin_yaml(const char *yaml_path)
+{
+ config::ConfigResult<PluginYAMLEntries> result;
+ YAML::Node root;
+
+ try {
+ root = YAML::LoadFile(yaml_path);
+ } catch (const YAML::Exception &e) {
+ result.errata.note("failed to parse: {}", e.what());
+ return result;
+ }
+
+ if (!root["plugins"] || !root["plugins"].IsSequence()) {
+ result.errata.note("missing or invalid 'plugins' sequence");
+ return result;
+ }
+
+ struct IndexedEntry {
+ int seq_idx;
+ PluginYAMLEntry entry;
+ };
+
+ std::vector<IndexedEntry> indexed;
+ int seq_idx = 0;
+
+ for (const auto &node : root["plugins"]) {
+ PluginYAMLEntry entry;
+
+ if (!node["path"]) {
+ result.errata.note("plugin entry #{} missing required 'path' field",
seq_idx + 1);
+ return result;
+ }
+ entry.path = node["path"].as<std::string>();
+
+ if (auto n = node["enabled"]; n) {
+ entry.enabled = n.as<bool>();
+ }
+ if (auto n = node["load_order"]; n) {
+ entry.load_order = n.as<int>();
+ }
+ if (auto n = node["params"]; n && n.IsSequence()) {
+ for (const auto &p : n) {
+ entry.params.emplace_back(p.as<std::string>());
+ }
+ }
+ if (auto n = node["config"]; n) {
+ if (n.IsScalar()) {
+ entry.config_literal = n.as<std::string>();
+ } else {
+ result.errata.note("plugin '{}': 'config' must be a scalar (use
literal block '|' for multi-line content)", entry.path);
+ return result;
+ }
+ }
+
+ indexed.push_back({seq_idx++, std::move(entry)});
+ }
+
+ std::stable_sort(indexed.begin(), indexed.end(), [](const IndexedEntry &a,
const IndexedEntry &b) {
+ const bool a_has = a.entry.load_order >= 0;
+ const bool b_has = b.entry.load_order >= 0;
+
+ if (a_has && b_has) {
+ return a.entry.load_order < b.entry.load_order;
+ }
+ return a_has && !b_has;
+ });
+
+ result.value.reserve(indexed.size());
+ for (auto &[_, entry] : indexed) {
+ result.value.emplace_back(std::move(entry));
+ }
+
+ return result;
+}
+
+/// Write inline config content to a temp file, returning the path on success.
+static std::optional<std::string>
+write_inline_config(const PluginYAMLEntry &entry, int index)
+{
+ char tmp_path[PATH_NAME_MAX];
+
+ std::string_view stem{entry.path};
+ if (auto pos = stem.rfind('/'); pos != std::string_view::npos) {
+ stem = stem.substr(pos + 1);
+ }
+ if (auto pos = stem.rfind('.'); pos != std::string_view::npos) {
+ stem = stem.substr(0, pos);
+ }
+
+ snprintf(tmp_path, sizeof(tmp_path), "%s/.%.*s_inline_%d.conf",
RecConfigReadConfigDir().c_str(), static_cast<int>(stem.size()),
+ stem.data(), index);
+
+ int fd = open(tmp_path, O_WRONLY | O_CREAT | O_TRUNC, 0644);
+ if (fd < 0) {
+ Error("%s: failed to create temp config for %s: %s",
ts::filename::PLUGIN_YAML, entry.path.c_str(), strerror(errno));
+ return std::nullopt;
+ }
+
+ auto n = write(fd, entry.config_literal.data(), entry.config_literal.size());
+ close(fd);
+
+ if (n < 0 || static_cast<size_t>(n) != entry.config_literal.size()) {
+ Error("%s: failed to write inline config for %s",
ts::filename::PLUGIN_YAML, entry.path.c_str());
+ return std::nullopt;
+ }
+
+ return std::string(tmp_path);
+}
+
+/// Build the argv for a single plugin: [path, inline_config_path?, params...,
$record expansions].
+static std::optional<std::vector<std::string>>
+build_plugin_args(const PluginYAMLEntry &entry, int index)
+{
+ std::vector<std::string> args;
+ args.emplace_back(entry.path);
+
+ if (!entry.config_literal.empty()) {
+ if (auto path = write_inline_config(entry, index); path) {
+ args.emplace_back(std::move(*path));
+ } else {
+ return std::nullopt;
+ }
+ }
+
+ for (const auto &p : entry.params) {
+ args.emplace_back(p);
+ }
+
+ return args;
+}
+
+static void
+log_plugin_load_summary(int loaded, int disabled)
+{
+ Note("%s: %d plugins loaded, %d disabled", ts::filename::PLUGIN_YAML,
loaded, disabled);
+
+ for (const auto &e : s_plugin_load_summary.entries) {
+ if (e.enabled) {
+ if (e.load_order >= 0) {
+ Note(" #%d %-30s load_order: %-5d loaded", e.index, e.path.c_str(),
e.load_order);
+ } else {
+ Note(" #%d %-30s loaded", e.index, e.path.c_str());
+ }
+ } else {
+ Note(" -- %-30s disabled", e.path.c_str());
+ }
+ }
+}
+
+static void
+cleanup_inline_configs()
+{
+ std::string config_dir = RecConfigReadConfigDir();
+ std::error_code ec;
+
+ try {
+ for (const auto &entry : std::filesystem::directory_iterator(config_dir,
ec)) {
+ if (!entry.is_regular_file()) {
+ continue;
+ }
+ auto name = entry.path().filename().string();
+ if (name.front() == '.' && name.find("_inline_") != std::string::npos &&
name.ends_with(".conf")) {
+ std::filesystem::remove(entry.path(), ec);
+ }
+ }
+ } catch (const std::exception &e) {
+ Error("%s: error cleaning up inline config files: %s",
ts::filename::PLUGIN_YAML, e.what());
+ }
+}
+
+bool
+plugin_yaml_init(bool validateOnly)
+{
+ plugin_dir_init();
+
+ ats_scoped_str yaml_path;
+
+ cleanup_inline_configs();
+
+ yaml_path = RecConfigReadConfigPath(nullptr, ts::filename::PLUGIN_YAML);
+ if (access(yaml_path, R_OK) != 0) {
+ return plugin_init(validateOnly);
+ }
Review Comment:
The `access(yaml_path, R_OK)` check falls back to `plugin.config` for *any*
error (including permission errors, EIO, etc.), but the stated precedence is
that `plugin.yaml` wins when it exists. If `plugin.yaml` exists but is
unreadable, silently ignoring it and using `plugin.config` can hide
misconfiguration. Consider only falling back on ENOENT, and otherwise emit an
error and fail initialization.
##########
src/traffic_ctl/ConvertConfigCommand.cc:
##########
@@ -145,3 +155,40 @@ ConvertConfigCommand::convert_storage()
_printer->write_output("Converted " + _input_file + " + " +
_volume_config_file + " -> " + _output_file);
}
}
+
+void
+ConvertConfigCommand::convert_plugin_config()
+{
+ config::PluginConfigParser parser;
+ config::ConfigResult<config::PluginConfigData> result =
parser.parse(_input_file);
+
+ if (!result.ok()) {
+ std::string error_msg = "Failed to parse input file '" + _input_file + "'";
+ if (!result.errata.empty()) {
+ error_msg += ": ";
+ error_msg += std::string(result.errata.front().text());
+ }
+ _printer->write_output(error_msg);
+ return;
+ }
+
+ if (_skip_disabled) {
+ std::erase_if(result.value, [](const config::PluginConfigEntry &e) {
return !e.enabled; });
+ }
Review Comment:
`std::erase_if` is used here but this file doesn't include `<algorithm>`.
Relying on transitive includes can break builds; add the explicit include.
##########
doc/appendices/command-line/traffic_ctl.en.rst:
##########
@@ -993,6 +1034,25 @@ traffic_ctl plugin
-------------------
.. program:: traffic_ctl plugin
+.. option:: list
+
+ Display the globally loaded plugins and their status. The output includes
+ each plugin's sequence index, path, ``load_order`` (if set), and whether the
+ plugin is enabled.
+
+ Example:
+
+ .. code-block:: bash
+
+ $ traffic_ctl plugin list
+ # index load_order path status
+ 1 100 certifier.so loaded
+ 2 - header_rewrite.so loaded
+ 3 - debug_plugin.so disabled
Review Comment:
The documented example output for `traffic_ctl plugin list` doesn't match
the current implementation in `PluginCommand::plugin_list()` (which prints a
`source:` line and uses a different column header/format). Update the example
to reflect the actual output format so operators can rely on the docs.
```suggestion
Display the globally loaded plugins and their status. The output includes
the source of the plugin configuration and the current list of configured
plugins.
Example:
.. code-block:: bash
$ traffic_ctl plugin list
source: /etc/trafficserver/plugin.config
IDX PLUGIN
1 certifier.so
2 header_rewrite.so
3 debug_plugin.so
```
##########
src/mgmt/rpc/handlers/plugins/Plugins.cc:
##########
@@ -89,4 +90,35 @@ plugin_send_basic_msg(std::string_view const & /* id
ATS_UNUSED */, YAML::Node c
return resp;
}
+swoc::Rv<YAML::Node>
+get_plugin_list(std::string_view const & /* id ATS_UNUSED */, YAML::Node const
& /* params ATS_UNUSED */)
+{
+ swoc::Rv<YAML::Node> resp;
+ try {
+ const auto &summary = get_plugin_load_summary();
+ YAML::Node data;
+
+ data["source"] = summary.source;
+
+ YAML::Node plugins;
+ for (const auto &e : summary.entries) {
+ YAML::Node plugin;
+
+ plugin["path"] = e.path;
+ plugin["enabled"] = e.enabled;
+ plugin["status"] = e.enabled ? "loaded" : "disabled";
Review Comment:
The JSONRPC response sets `status` based only on `enabled`, but the server
tracks actual load success via `PluginLoadSummary::Entry::loaded`. If a plugin
fails to load (and ATS continues running), `traffic_ctl plugin list` will
incorrectly report it as `loaded`. Use `e.loaded` to set `status` (e.g.,
`loaded` vs `failed` vs `disabled`).
```suggestion
plugin["status"] = !e.enabled ? "disabled" : (e.loaded ? "loaded" :
"failed");
```
--
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]
