Copilot commented on code in PR #13198:
URL: https://github.com/apache/trafficserver/pull/13198#discussion_r3293739699
##########
src/mgmt/rpc/server/unit_tests/test_rpcserver.cc:
##########
@@ -71,12 +76,56 @@ 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"};
-constexpr int default_backlog{5};
-constexpr int default_maxRetriesOnTransientErrors{64};
-constexpr size_t default_incoming_req_max_size{32000 * 3};
-DbgCtl dbg_ctl{"rpc.test.client"};
+constexpr std::string_view rpc_test_dir_template{"ats_rpc_XXXXXX"};
+constexpr std::string_view rpc_test_socket_name{"s"};
+constexpr std::string_view rpc_test_lock_name{"l"};
+constexpr size_t
max_rpc_socket_path_size{sizeof(sockaddr_un::sun_path) - 1};
+
+fs::path rpcTestDir;
+std::string sockPath;
+std::string lockPath;
+constexpr int default_backlog{5};
+constexpr int default_maxRetriesOnTransientErrors{64};
+constexpr size_t default_incoming_req_max_size{32000 * 3};
+DbgCtl dbg_ctl{"rpc.test.client"};
+
+bool
+try_setup_rpc_test_paths(fs::path const &base, std::string &error)
+{
+ auto const dir_template = (base / rpc_test_dir_template).string();
+ auto const socket_path = (fs::path{dir_template} /
rpc_test_socket_name).string();
+
+ if (socket_path.size() > max_rpc_socket_path_size) {
+ error = "JSONRPC test socket path is too long under " + base.string() + ":
" + socket_path;
+ return false;
+ }
+
+ std::vector<char> mutable_template{dir_template.begin(), dir_template.end()};
+ mutable_template.push_back('\0');
+
+ char *created_dir = mkdtemp(mutable_template.data());
+ if (created_dir == nullptr) {
+ error = "Failed to create JSONRPC test directory under " + base.string() +
": " + std::strerror(errno);
+ return false;
+ }
+
+ rpcTestDir = fs::path{created_dir};
+ sockPath = (rpcTestDir / rpc_test_socket_name).string();
+ lockPath = (rpcTestDir / rpc_test_lock_name).string();
+ return true;
+}
+
+bool
+setup_rpc_test_paths(std::string &error)
+{
+ if (try_setup_rpc_test_paths(fs::temp_directory_path(), error)) {
+ return true;
+ }
+ if (try_setup_rpc_test_paths(fs::path{"/tmp"}, error)) {
+ return true;
+ }
Review Comment:
If `fs::temp_directory_path()` fails the first attempt (e.g., path too long)
but the `/tmp` fallback succeeds, `error` will still contain the earlier
failure message. Because `INFO(setup_error)` is registered for the entire
scope, a later unrelated test failure could print a misleading setup error.
Clear `error` on success (or use a temporary error string per attempt and only
propagate it when all attempts fail).
##########
tests/gold_tests/autest-site/conditions.test.ext:
##########
@@ -20,6 +20,90 @@ import os
import subprocess
import json
import re
+import tempfile
+import time
+
+from ports import get_port_number
+
+OPENSSL_TLS_FLAGS = {
+ "1.0": "-tls1",
+ "1.1": "-tls1_1",
+ "1.2": "-tls1_2",
+ "1.3": "-tls1_3",
+}
+
+
+def _terminate_process(process):
+ if process.poll() is not None:
+ return
+ process.terminate()
+ try:
+ process.wait(timeout=2)
+ except subprocess.TimeoutExpired:
+ process.kill()
+ process.wait(timeout=2)
+
+
+def _probe_openssl_server(tls_version, client_probe):
+ tls_flag = OPENSSL_TLS_FLAGS.get(tls_version)
+ if tls_flag is None:
+ return False
+
+ with tempfile.TemporaryDirectory() as tmpdir:
+ cert_path = os.path.join(tmpdir, "cert.pem")
+ key_path = os.path.join(tmpdir, "key.pem")
+ result = subprocess.run(
+ [
+ "openssl",
+ "req",
+ "-x509",
+ "-newkey",
+ "rsa:2048",
+ "-nodes",
+ "-sha256",
+ "-keyout",
+ key_path,
+ "-out",
+ cert_path,
+ "-subj",
+ "/CN=localhost",
+ "-days",
+ "1",
+ ],
+ stdout=subprocess.DEVNULL,
+ stderr=subprocess.DEVNULL,
+ timeout=10,
+ )
+ if result.returncode != 0:
+ return False
+
+ port = get_port_number()
+ server = subprocess.Popen(
+ [
+ "openssl",
+ "s_server",
+ "-quiet",
+ "-accept",
+ f"127.0.0.1:{port}",
+ "-cert",
+ cert_path,
+ "-key",
+ key_path,
+ tls_flag,
+ "-cipher",
+ "DEFAULT@SECLEVEL=0",
+ "-www",
+ ],
+ stdout=subprocess.DEVNULL,
+ stderr=subprocess.DEVNULL,
+ )
+ try:
+ time.sleep(0.5)
+ if server.poll() is not None:
+ return False
+ return client_probe(port, tls_flag)
+ finally:
+ _terminate_process(server)
Review Comment:
`_probe_openssl_server()` can raise (e.g., `FileNotFoundError` if `openssl`
isn't installed, `subprocess.TimeoutExpired` from the `openssl req` timeout, or
`OSError` from `Popen`). Since these exceptions aren't caught here, they may
bubble out of the AuTest Condition predicate and hard-fail the test harness
instead of simply disabling legacy-TLS-dependent tests. Wrap the server/cert
creation section in a broad `try/except` (at least `OSError` /
`subprocess.SubprocessError` / `subprocess.TimeoutExpired`) and return `False`
on failures so the condition remains a safe probe.
--
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]
