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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
