You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "traeak (via GitHub)" <gi...@apache.org> on 2026/10/16 20:45:22 UTC
[PR] nexthop: align strategy include path resolution [trafficserver]
traeak opened a new pull request, #13853:
URL: https://github.com/apache/trafficserver/pull/13853
Resolve relative strategies.yaml includes like remap.config and remap.yaml. For missing legacy includes with the config-directory prefix, retry without it and warn; absolute paths remain unchanged.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nexthop: align strategy include path resolution [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #13853:
URL: https://github.com/apache/trafficserver/pull/13853#discussion_r4312975256
##########
src/proxy/http/remap/unit-tests/test_NextHopStrategyFactory.cc:
##########
@@ -27,16 +27,61 @@
*/
#include <catch2/catch_test_macros.hpp> /* catch unit-test framework */
-#include <fstream> /* ofstream */
+#include <catch2/reporters/catch_reporter_event_listener.hpp>
+#include <catch2/reporters/catch_reporter_registrars.hpp>
+#include <chrono>
+#include <cstdlib>
+#include <filesystem>
+#include <fstream> /* ofstream */
#include <memory>
#include <utime.h>
#include <yaml-cpp/yaml.h>
-#include "nexthop_test_stubs.h"
+#include "tscore/Layout.h"
#include "proxy/http/remap/NextHopSelectionStrategy.h"
#include "proxy/http/remap/NextHopStrategyFactory.h"
#include "proxy/http/remap/NextHopConsistentHash.h"
-#include "proxy/http/remap/NextHopRoundRobin.h"
+
+// '#include' directives in strategy files resolve against the configuration
+// directory, so point it at a temporary configuration directory containing the
+// test fixtures.
+static std::filesystem::path config_root;
+
+struct ConfigDirListener : Catch::EventListenerBase {
+ using EventListenerBase::EventListenerBase;
+
+ void
+ testRunStarting(Catch::TestRunInfo const & /* testRunInfo ATS_UNUSED */) override
+ {
+ const auto temp_dir = std::filesystem::temp_directory_path();
+ const auto prefix = "nh_strategy_config_test-" + std::to_string(std::chrono::steady_clock::now().time_since_epoch().count());
+
+ for (unsigned int attempt = 0; attempt < 100; ++attempt) {
+ config_root = temp_dir / (prefix + "-" + std::to_string(attempt));
+ if (std::filesystem::create_directory(config_root)) {
+ break;
+ }
+ config_root.clear();
+ }
+ if (config_root.empty()) {
+ std::abort();
+ }
+
+ setenv("PROXY_CONFIG_CONFIG_DIR", "etc/trafficserver", 1);
+ Layout::create(config_root.string());
+ const auto config_dir = std::filesystem::path(Layout::get()->sysconfdir);
Review Comment:
issue: The fixture is copied to `Layout::get()->sysconfdir`, but the code under test reads `RecConfigReadConfigDir()`, which the `PROXY_CONFIG_CONFIG_DIR` setting above resolves to `<config_root>/etc/trafficserver`. The two only agree when `CMAKE_INSTALL_SYSCONFDIR` is the default `etc/trafficserver`.
Configured with `-DCMAKE_INSTALL_SYSCONFDIR=etc`, `test_NextHopStrategyFactory` fails on this branch and passes on the base commit. With an absolute sysconfdir, the listener creates that directory, copies `hosts.yaml` into it and leaves it there, and the next run exits in `copy_file` with "File exists".
Copying to the directory the factory reads keeps the fixture under `config_root` for any layout (it needs `#include "records/RecCore.h"`):
```cpp
const auto config_dir = std::filesystem::path(RecConfigReadConfigDir());
```
##########
src/proxy/http/remap/unit-tests/test_NextHopStrategyFactory.cc:
##########
@@ -64,9 +109,81 @@ SCENARIO("factory tests loading yaml configs", "[loadConfig]")
}
}
+ GIVEN("Loading a strategy.yaml outside the configuration directory with an absolute include.")
Review Comment:
suggestion: Nothing here tells "relative to the configuration directory" apart from "relative to the including file's directory". In the relative cases the including file and `hosts.yaml` sit side by side (the fixture in `unit-tests/`, and both files in `CONFIGDIR` in the autest). The legacy case is rescued by the fallback either way. If `f` is resolved against `std::filesystem::path(fileName).parent_path()` instead of the configuration directory, this whole file still passes.
A copy of this case with a relative include pins the documented behavior, because `hosts.yaml` then exists only in the configuration directory:
```cpp
out << "#include hosts.yaml\n";
```
It passes on this branch and fails under that alternative.
##########
src/proxy/http/remap/NextHopStrategyFactory.cc:
##########
@@ -23,15 +23,60 @@
#include <yaml-cpp/yaml.h>
+#include <filesystem>
#include <fstream>
#include <cstring>
#include <string_view>
+#include <utility>
+#include "records/RecCore.h"
+#include "tscore/Layout.h"
#include "proxy/http/remap/NextHopStrategyFactory.h"
#include "proxy/http/remap/NextHopConsistentHash.h"
#include "proxy/http/remap/NextHopRoundRobin.h"
#include <tsutil/YamlCfg.h>
+namespace
+{
+std::string
+legacy_config_dir_include_path(std::string_view include_path)
+{
+ std::filesystem::path const target{include_path};
+ if (target.is_absolute()) {
+ return {};
+ }
+
+ std::filesystem::path const config_dir{RecConfigReadConfigDir()};
+ std::filesystem::path const install_prefix{Layout::get()->prefix};
+ std::filesystem::path const config_dir_prefix = config_dir.lexically_relative(install_prefix);
+ if (config_dir_prefix.empty() || config_dir_prefix == "." || config_dir_prefix.is_absolute()) {
+ return {};
+ }
+ for (const auto &part : config_dir_prefix) {
+ if (part == "..") {
+ return {};
+ }
+ }
+
+ auto target_part = target.begin();
+ for (const auto &prefix_part : config_dir_prefix) {
+ if (target_part == target.end() || *target_part != prefix_part) {
+ return {};
+ }
+ ++target_part;
+ }
+ if (target_part == target.end()) {
+ return {};
+ }
+
+ std::filesystem::path stripped_path;
+ for (; target_part != target.end(); ++target_part) {
+ stripped_path /= *target_part;
+ }
+ return RecConfigReadConfigPath(nullptr, stripped_path.string().c_str());
Review Comment:
nitpick: The prefix match compares raw path elements. Two legacy spellings that the working-directory lookup used to accept now fail to load instead of taking the fallback: `#include ./etc/trafficserver/hosts.yaml`, and any legacy include when the configuration directory ends in `/` (an absolute `sysconfdir` in `runroot.yaml` is used as written). Normalizing the target and letting `lexically_relative` do the match covers both and replaces the two loops:
```cpp
std::filesystem::path const stripped_path = target.lexically_normal().lexically_relative(config_dir_prefix);
if (stripped_path.empty() || stripped_path == "." || *stripped_path.begin() == "..") {
return {};
}
return RecConfigReadConfigPath(nullptr, stripped_path.string().c_str());
```
##########
src/proxy/http/remap/NextHopStrategyFactory.cc:
##########
@@ -271,6 +332,10 @@ NextHopStrategyFactory::loadConfigFile(const std::string &fileName, std::strings
throw std::invalid_argument("Unable to load included file '" + f + "' from '" + fileName + "': " + ex.what());
}
}
+ if (used_legacy_path) {
+ const auto recommended_include = std::filesystem::path(f).lexically_relative(RecConfigReadConfigDir()).string();
+ NH_Warn("Deprecated '#include %s'; use '#include %s' instead.", include_target.c_str(), recommended_include.c_str());
Review Comment:
suggestion: The warning doesn't say which file holds the directive, so with nested includes the operator has to search for it. The error path above already names it (`from '<fileName>'`). The same here:
```cpp
NH_Warn("Deprecated '#include %s' in '%s'; use '#include %s' instead.", include_target.c_str(), fileName.c_str(),
recommended_include.c_str());
```
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nexthop: align strategy include path resolution [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13853:
URL: https://github.com/apache/trafficserver/pull/13853#discussion_r4312898881
##########
src/proxy/http/remap/NextHopStrategyFactory.cc:
##########
@@ -261,7 +307,22 @@ NextHopStrategyFactory::loadConfigFile(const std::string &fileName, std::strings
}
const auto include_line = line_view.substr(include_start);
const auto include_end = include_line.find_first_of(separators);
- const std::string f{include_line.substr(0, include_end)};
+ const std::string include_target{include_line.substr(0, include_end)};
+ std::string f = RecConfigReadConfigPath(nullptr, include_target.c_str());
+ bool used_legacy_path = false;
+
+ // Retry a missing include when it starts with the configuration directory's old
+ // install-prefix-relative spelling, e.g. etc/trafficserver/hosts.yaml.
+ struct stat include_stat;
+ if (stat(f.c_str(), &include_stat) == -1 && errno == ENOENT) {
+ std::string legacy_path = legacy_config_dir_include_path(include_target);
+ struct stat legacy_stat;
+ if (!legacy_path.empty() && stat(legacy_path.c_str(), &legacy_stat) == 0) {
Review Comment:
This code uses `struct stat`, `stat()`, `errno`, and `ENOENT` but the file-level includes shown in the diff do not explicitly include the standard headers that declare them. Relying on transitive includes is brittle and can break builds. Add the appropriate includes (typically `<sys/stat.h>` and `<cerrno>`) in this translation unit.
##########
tests/gold_tests/next_hop/strategies_include/strategies_include.test.py:
##########
@@ -0,0 +1,103 @@
+'''
+Verify '#include' handling in strategies.yaml.
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
Review Comment:
The entire file appears to be indented at top-level (e.g., the module docstring on line 1). In Python, leading indentation at top-level will raise `IndentationError: unexpected indent`. Remove the leading spaces so all top-level statements start at column 0.
##########
src/proxy/http/remap/unit-tests/test_NextHopStrategyFactory.cc:
##########
@@ -27,16 +27,61 @@
*/
#include <catch2/catch_test_macros.hpp> /* catch unit-test framework */
-#include <fstream> /* ofstream */
+#include <catch2/reporters/catch_reporter_event_listener.hpp>
+#include <catch2/reporters/catch_reporter_registrars.hpp>
+#include <chrono>
+#include <cstdlib>
+#include <filesystem>
+#include <fstream> /* ofstream */
#include <memory>
#include <utime.h>
#include <yaml-cpp/yaml.h>
-#include "nexthop_test_stubs.h"
+#include "tscore/Layout.h"
#include "proxy/http/remap/NextHopSelectionStrategy.h"
#include "proxy/http/remap/NextHopStrategyFactory.h"
#include "proxy/http/remap/NextHopConsistentHash.h"
-#include "proxy/http/remap/NextHopRoundRobin.h"
+
+// '#include' directives in strategy files resolve against the configuration
+// directory, so point it at a temporary configuration directory containing the
+// test fixtures.
+static std::filesystem::path config_root;
+
+struct ConfigDirListener : Catch::EventListenerBase {
+ using EventListenerBase::EventListenerBase;
+
+ void
+ testRunStarting(Catch::TestRunInfo const & /* testRunInfo ATS_UNUSED */) override
+ {
+ const auto temp_dir = std::filesystem::temp_directory_path();
+ const auto prefix = "nh_strategy_config_test-" + std::to_string(std::chrono::steady_clock::now().time_since_epoch().count());
+
+ for (unsigned int attempt = 0; attempt < 100; ++attempt) {
+ config_root = temp_dir / (prefix + "-" + std::to_string(attempt));
+ if (std::filesystem::create_directory(config_root)) {
+ break;
+ }
+ config_root.clear();
+ }
+ if (config_root.empty()) {
+ std::abort();
+ }
Review Comment:
`std::abort()` makes failures hard to diagnose in CI (no assertion message) and can bypass normal test reporting/cleanup. Prefer failing the test with Catch2 (e.g., `FAIL(...)`/`REQUIRE(...)`) or throwing a descriptive exception so the test output explains why the temp config directory could not be created.
##########
src/proxy/http/remap/NextHopStrategyFactory.cc:
##########
@@ -271,6 +332,10 @@ NextHopStrategyFactory::loadConfigFile(const std::string &fileName, std::strings
throw std::invalid_argument("Unable to load included file '" + f + "' from '" + fileName + "': " + ex.what());
}
}
+ if (used_legacy_path) {
+ const auto recommended_include = std::filesystem::path(f).lexically_relative(RecConfigReadConfigDir()).string();
+ NH_Warn("Deprecated '#include %s'; use '#include %s' instead.", include_target.c_str(), recommended_include.c_str());
Review Comment:
`recommended_include` can be surprising/empty in edge cases (e.g., if `lexically_relative` cannot compute a meaningful relative path). Consider guarding the warning so it never suggests an empty string (fallback to a known-safe suggestion like the basename, or skip the \"use ... instead\" portion when the relative path isn’t usable).
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org