Copilot commented on code in PR #13198:
URL: https://github.com/apache/trafficserver/pull/13198#discussion_r3293635240
##########
src/mgmt/rpc/server/unit_tests/test_rpcserver.cc:
##########
@@ -71,8 +72,32 @@ add_method_handler(const std::string &name, Func &&call)
namespace
{
-const std::string sockPath{"tests/var/jsonrpc20_test.sock"};
-const std::string lockPath{"tests/var/jsonrpc20_test.lock"};
+fs::path const &
+rpc_test_dir()
+{
+ static const fs::path dir = []() {
+ auto path = fs::temp_directory_path() /
fs::path("ats_jsonrpcserver_" + std::to_string(getpid()));
+ std::error_code ec;
+
+ fs::remove_all(path, ec);
+ ec.clear();
+ if (!fs::create_directories(path, ec, 0700) && ec) {
+ throw std::runtime_error{"Failed to create JSONRPC test directory: " +
ec.message()};
+ }
+ return path;
+ }();
+
+ return dir;
+}
+
+std::string
+rpc_test_path(std::string_view filename)
+{
+ return rpc_test_dir().string() + "/" + std::string{filename};
+}
+
+const std::string sockPath{rpc_test_path("jsonrpc20_test.sock")};
+const std::string lockPath{rpc_test_path("jsonrpc20_test.lock")};
Review Comment:
The socket path is now derived from fs::temp_directory_path() (which honors
TMPDIR/TMP/TEMPDIR). If those env vars point to a long path, sockPath may
exceed the AF_UNIX sun_path limit and IPCSocketServer will reject it (it
validates sockPathName.size() against sizeof(sockaddr_un::sun_path)). Add a
length check when building rpc_test_path (and fall back to a shorter base path
such as "/tmp" or a shorter directory name) to avoid hard-to-debug failures in
environments with long temp paths.
##########
tests/gold_tests/autest-site/conditions.test.ext:
##########
@@ -74,31 +163,32 @@ def HasLegacyTLSSupport(self):
"""
def check_tls1_support():
- try:
- # Connect to localhost on a port nothing is listening on.
- # This avoids external network dependency while still detecting
- # whether the crypto-policy allows TLSv1.0.
- result = subprocess.run(
- ['openssl', 's_client', '-tls1', '-connect', '127.0.0.1:1'],
- capture_output=True,
- text=True,
- timeout=5,
- input='' # Don't wait for interactive input
- )
- output = result.stdout + result.stderr
- # "no protocols available" means TLSv1 is disabled by crypto-policy
- if 'no protocols available' in output:
+
+ def client_probe(port, tls_flag):
Review Comment:
The HasLegacyTLSSupport docstring immediately above this function still
describes the previous closed-port probe logic ("connection refused" vs "no
protocols available"), but the implementation now starts a local openssl
s_server and performs a real handshake. Please update the docstring to match
the new behavior so future readers don't rely on outdated details.
##########
tests/gold_tests/tls/tls_check_cert_select_plugin.test.py:
##########
@@ -170,7 +170,6 @@
tr.ReturnCode = 60
tr.StillRunningAfter = server
tr.StillRunningAfter = ts
-tr.Processes.Default.Streams.All = Testers.ContainsExpression("unknown CA",
"Failed handshake")
-tr.Processes.Default.Streams.All += Testers.ExcludesExpression("CN=bar.com",
"Cert should contain bar.com")
+tr.Processes.Default.Streams.All = Testers.ContainsExpression(r"curl: \(60\)
SSL certificate", "Failed certificate verification")
tr.Processes.Default.Streams.All += Testers.ExcludesExpression("CN=foo.com",
"Cert should not contain foo.com")
tr.Processes.Default.Streams.All += Testers.ExcludesExpression("404", "Should
make an exchange")
Review Comment:
The PR description says this bad-CA case should confirm the selected
certificate is for bar.com, but this run no longer asserts that (it only checks
the generic curl(60) text and that CN=foo.com is absent). Add a stable
assertion that the presented/selected certificate corresponds to bar.com (e.g.,
match "CN=bar.com" or another stable marker) so the test actually verifies cert
selection in the failure path.
##########
src/mgmt/rpc/server/unit_tests/test_rpcserver.cc:
##########
@@ -71,8 +72,32 @@ add_method_handler(const std::string &name, Func &&call)
namespace
{
-const std::string sockPath{"tests/var/jsonrpc20_test.sock"};
-const std::string lockPath{"tests/var/jsonrpc20_test.lock"};
+fs::path const &
+rpc_test_dir()
+{
+ static const fs::path dir = []() {
+ auto path = fs::temp_directory_path() /
fs::path("ats_jsonrpcserver_" + std::to_string(getpid()));
+ std::error_code ec;
+
+ fs::remove_all(path, ec);
+ ec.clear();
+ if (!fs::create_directories(path, ec, 0700) && ec) {
+ throw std::runtime_error{"Failed to create JSONRPC test directory: " +
ec.message()};
+ }
Review Comment:
rpc_test_dir() performs filesystem mutation and can throw, and it's invoked
during static initialization via the global sockPath/lockPath constants. If
temp directory creation fails (permissions, sticky-bit collisions, etc.), the
test binary can terminate before Catch starts, making failures hard to
diagnose. Consider deferring temp dir creation to testRunStarting (and
reporting failures via Catch) and/or making rpc_test_dir() non-throwing and
returning an error that the test can assert on.
--
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]
