From 16175f689252b927338448fb2d6df15ab01c6ec0 Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Fri, 21 Aug 2026 06:47:49 -0500 Subject: [PATCH 1/4] Add non-HttpSM tests for `acquireSession` This patch includes a few simple test cases for `acquireSession`. They cover only the behavior which does not require an `HttpSM` object. --- include/proxy/http/HttpSessionManager.h | 35 +- src/proxy/http/unit_tests/CMakeLists.txt | 1 + .../unit_tests/test_HttpSessionManager.cc | 333 ++++++++++++++++++ 3 files changed, 361 insertions(+), 8 deletions(-) create mode 100644 src/proxy/http/unit_tests/test_HttpSessionManager.cc diff --git a/include/proxy/http/HttpSessionManager.h b/include/proxy/http/HttpSessionManager.h index 6c6523da407..daa1a566521 100644 --- a/include/proxy/http/HttpSessionManager.h +++ b/include/proxy/http/HttpSessionManager.h @@ -85,14 +85,33 @@ class ServerSessionPool : public Continuation static bool match(PoolableSession *ss, sockaddr const *addr, CryptoHash const &host_hash, TSServerSessionSharingMatchMask match_style); - /** Get a session from the pool. - - The session is selected based on @a match_style equivalently to @a match. If found the session - is removed from the pool. - - @return A pointer to the session or @c NULL if not matching session was found. - */ - HSMresult_t acquireSession(sockaddr const *addr, CryptoHash const &host_hash, TSServerSessionSharingMatchMask match_style, + /** Search the pool for a server session compatible with the given address, hostname, and state machine. + * + * The selection criteria are controlled by @p match_style. At least one of + * @c TS_SERVER_SESSION_SHARING_MATCH_MASK_IP or @c TS_SERVER_SESSION_SHARING_MATCH_MASK_HOSTONLY + * must be set for a match to be possible; if neither is set, @c HSMresult_t::NOT_FOUND is + * returned unconditionally. + * + * When a compatible session is found and does not support multiplexed streams, it is removed from + * the pool and ownership transfers to the caller. A session that supports multiplexed streams is + * not removed and remains available for subsequent acquisitions. + * + * @param[in] addr Remote address and port to match. + * @param[in] hostname_hash Cryptographic hash of the target hostname. + * @param[in] match_style Bitmask of @c TSServerSessionSharingMatchMask values specifying which + * attributes must agree between the candidate session and the request. + * @param[in] sm The requesting HTTP state machine; consulted for SNI and certificate + * validation when the corresponding bits are set in @p match_style. + * @param[out] server_session Set to the matched session when @c HSMresult_t::DONE is returned; + * @c nullptr otherwise. + * + * @return @c HSMresult_t::DONE if a matching session was found; + * @c HSMresult_t::NOT_FOUND otherwise. + * + * @par Thread Safety + * Not thread-safe. + */ + HSMresult_t acquireSession(sockaddr const *addr, CryptoHash const &hostname_hash, TSServerSessionSharingMatchMask match_style, HttpSM *sm, PoolableSession *&server_session); /** Release a session to the pool. diff --git a/src/proxy/http/unit_tests/CMakeLists.txt b/src/proxy/http/unit_tests/CMakeLists.txt index ca37b0246a9..a3ed5edeaf9 100644 --- a/src/proxy/http/unit_tests/CMakeLists.txt +++ b/src/proxy/http/unit_tests/CMakeLists.txt @@ -22,6 +22,7 @@ add_executable( test_ChunkedHandler.cc test_error_page_selection.cc test_ForwardedConfig.cc + test_HttpSessionManager.cc test_HttpTransact.cc test_HttpTransactHeaders.cc test_HttpUserAgent.cc diff --git a/src/proxy/http/unit_tests/test_HttpSessionManager.cc b/src/proxy/http/unit_tests/test_HttpSessionManager.cc new file mode 100644 index 00000000000..548f33c31be --- /dev/null +++ b/src/proxy/http/unit_tests/test_HttpSessionManager.cc @@ -0,0 +1,333 @@ +/** @file + + Unit tests for ServerSessionPool::acquireSession. + + @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 "proxy/http/HttpSessionManager.h" +#include "proxy/http/HttpConfig.h" + +#include + +#include +#include + +namespace +{ + +/** A minimal PoolableSession that can be pooled and matched. + * + * Only the remote address and the hostname hash participate in the match + * paths exercised here, so no NetVConnection is required. The session must + * not be multiplexing, otherwise acquireSession consults the HttpSM. + */ +class TestPoolableSession : public PoolableSession +{ +public: + TestPoolableSession(char const *addr_str, char const *hostname) + { + ink_release_assert(ats_ip_pton(addr_str, &_remote_addr) == 0); + this->attach_hostname(hostname); + } + + void + new_connection(NetVConnection *, MIOBuffer *, IOBufferReader *) override + { + } + void + start() override + { + } + void + release(ProxyTransaction *) override + { + } + void + destroy() override + { + } + void + free() override + { + } + void + increment_current_active_connections_stat() override + { + } + void + decrement_current_active_connections_stat() override + { + } + + int + get_transact_count() const override + { + return 0; + } + + const char * + get_protocol_string() const override + { + return "test"; + } + + IOBufferReader * + get_remote_reader() override + { + return nullptr; + } + + void + do_io_close(int /* lerrno ATS_UNUSED */ = -1) override + { + ++close_count; + } + + sockaddr const * + get_remote_addr() const override + { + return &_remote_addr.sa; + } + + int close_count = 0; + +private: + IpEndpoint _remote_addr; +}; + +/// Owns the test sessions and hands raw pointers to the pool under test. +class SessionFactory +{ +public: + TestPoolableSession * + make(char const *addr_str, char const *hostname) + { + return _sessions.emplace_back(std::make_unique(addr_str, hostname)).get(); + } + +private: + std::vector> _sessions; +}; + +/// The pool bookkeeping updates this gauge, which the test binary never registers. +void +init_metrics() +{ + if (http_rsb.pooled_server_connections == nullptr) { + http_rsb.pooled_server_connections = Metrics::Gauge::createPtr("proxy.process.http.pooled_server_connections"); + } +} + +CryptoHash +hash_of(char const *hostname) +{ + CryptoHash hash; + + CryptoContext().hash_immediate(hash, static_cast(static_cast(hostname)), strlen(hostname)); + return hash; +} + +/// A sockaddr that can be passed inline to acquireSession. +struct Addr { + Addr(char const *addr_str) { ink_release_assert(ats_ip_pton(addr_str, &_addr) == 0); } + + operator sockaddr const *() const { return &_addr.sa; } + +private: + IpEndpoint _addr; +}; + +constexpr auto MATCH_IP = TS_SERVER_SESSION_SHARING_MATCH_MASK_IP; +constexpr auto MATCH_HOSTONLY = TS_SERVER_SESSION_SHARING_MATCH_MASK_HOSTONLY; +constexpr auto MATCH_BOTH = static_cast(TS_SERVER_SESSION_SHARING_MATCH_MASK_IP | + TS_SERVER_SESSION_SHARING_MATCH_MASK_HOSTONLY); + +} // namespace + +TEST_CASE("ServerSessionPool::acquireSession", "[session_pool]") +{ + init_metrics(); + + // Declared before the pool so the sessions outlive it. + SessionFactory factory; + ServerSessionPool pool; + + PoolableSession *acquired = nullptr; + + SECTION("empty pool finds nothing") + { + CHECK(pool.acquireSession(Addr{"10.0.0.1:80"}, hash_of("one.example.com"), MATCH_IP, nullptr, acquired) == + HSMresult_t::NOT_FOUND); + CHECK(acquired == nullptr); + } + + SECTION("a match mask with neither IP nor host disables sharing") + { + pool.addSession(factory.make("10.0.0.1:80", "one.example.com")); + + CHECK(pool.acquireSession(Addr{"10.0.0.1:80"}, hash_of("one.example.com"), TS_SERVER_SESSION_SHARING_MATCH_MASK_NONE, nullptr, + acquired) == HSMresult_t::NOT_FOUND); + CHECK(acquired == nullptr); + CHECK(pool.count() == 1); + } + + SECTION("match on IP") + { + auto *session = factory.make("10.0.0.1:80", "one.example.com"); + + pool.addSession(session); + + SECTION("address and port match") + { + CHECK(pool.acquireSession(Addr{"10.0.0.1:80"}, hash_of("other.example.com"), MATCH_IP, nullptr, acquired) == + HSMresult_t::DONE); + CHECK(acquired == session); + + // A non-multiplexing session is handed off, not shared: it leaves both pools. + CHECK(pool.count() == 0); + CHECK(pool.acquireSession(Addr{"10.0.0.1:80"}, hash_of("one.example.com"), MATCH_HOSTONLY, nullptr, acquired) == + HSMresult_t::NOT_FOUND); + } + + SECTION("a different address does not match") + { + CHECK(pool.acquireSession(Addr{"10.0.0.2:80"}, hash_of("one.example.com"), MATCH_IP, nullptr, acquired) == + HSMresult_t::NOT_FOUND); + CHECK(acquired == nullptr); + CHECK(pool.count() == 1); + } + + SECTION("a different port does not match") + { + CHECK(pool.acquireSession(Addr{"10.0.0.1:81"}, hash_of("one.example.com"), MATCH_IP, nullptr, acquired) == + HSMresult_t::NOT_FOUND); + CHECK(acquired == nullptr); + CHECK(pool.count() == 1); + } + } + + SECTION("match on IP returns the most recently pooled session") + { + auto *first = factory.make("10.0.0.1:80", "one.example.com"); + auto *second = factory.make("10.0.0.1:80", "two.example.com"); + + pool.addSession(first); + pool.addSession(second); + + CHECK(pool.acquireSession(Addr{"10.0.0.1:80"}, hash_of("one.example.com"), MATCH_IP, nullptr, acquired) == HSMresult_t::DONE); + CHECK(acquired == second); + + CHECK(pool.acquireSession(Addr{"10.0.0.1:80"}, hash_of("one.example.com"), MATCH_IP, nullptr, acquired) == HSMresult_t::DONE); + CHECK(acquired == first); + } + + SECTION("match on host only ignores the address but not the port") + { + auto *session = factory.make("10.0.0.1:80", "one.example.com"); + + pool.addSession(session); + + SECTION("a different address with the same host and port matches") + { + CHECK(pool.acquireSession(Addr{"192.168.1.1:80"}, hash_of("one.example.com"), MATCH_HOSTONLY, nullptr, acquired) == + HSMresult_t::DONE); + CHECK(acquired == session); + CHECK(pool.count() == 0); + } + + SECTION("a different host does not match") + { + CHECK(pool.acquireSession(Addr{"10.0.0.1:80"}, hash_of("other.example.com"), MATCH_HOSTONLY, nullptr, acquired) == + HSMresult_t::NOT_FOUND); + CHECK(acquired == nullptr); + CHECK(pool.count() == 1); + } + + SECTION("a different port does not match") + { + CHECK(pool.acquireSession(Addr{"10.0.0.1:81"}, hash_of("one.example.com"), MATCH_HOSTONLY, nullptr, acquired) == + HSMresult_t::NOT_FOUND); + CHECK(acquired == nullptr); + CHECK(pool.count() == 1); + } + } + + SECTION("match on host only returns the most recently pooled session") + { + auto *first = factory.make("10.0.0.1:80", "one.example.com"); + auto *second = factory.make("10.0.0.2:80", "one.example.com"); + + pool.addSession(first); + pool.addSession(second); + + CHECK(pool.acquireSession(Addr{"10.0.0.3:80"}, hash_of("one.example.com"), MATCH_HOSTONLY, nullptr, acquired) == + HSMresult_t::DONE); + CHECK(acquired == second); + + CHECK(pool.acquireSession(Addr{"10.0.0.3:80"}, hash_of("one.example.com"), MATCH_HOSTONLY, nullptr, acquired) == + HSMresult_t::DONE); + CHECK(acquired == first); + } + + SECTION("match on both IP and host requires both to match") + { + auto *session = factory.make("10.0.0.1:80", "one.example.com"); + + pool.addSession(session); + + SECTION("both match") + { + CHECK(pool.acquireSession(Addr{"10.0.0.1:80"}, hash_of("one.example.com"), MATCH_BOTH, nullptr, acquired) == + HSMresult_t::DONE); + CHECK(acquired == session); + } + + SECTION("the address matches but the host does not") + { + CHECK(pool.acquireSession(Addr{"10.0.0.1:80"}, hash_of("other.example.com"), MATCH_BOTH, nullptr, acquired) == + HSMresult_t::NOT_FOUND); + CHECK(acquired == nullptr); + CHECK(pool.count() == 1); + } + + SECTION("the host matches but the address does not") + { + CHECK(pool.acquireSession(Addr{"192.168.1.1:80"}, hash_of("one.example.com"), MATCH_BOTH, nullptr, acquired) == + HSMresult_t::NOT_FOUND); + CHECK(acquired == nullptr); + CHECK(pool.count() == 1); + } + } + + SECTION("only sessions in the requested address bucket are considered") + { + auto *wrong_addr = factory.make("10.0.0.2:80", "one.example.com"); + auto *right_addr = factory.make("10.0.0.1:80", "one.example.com"); + + pool.addSession(wrong_addr); + pool.addSession(right_addr); + + CHECK(pool.acquireSession(Addr{"10.0.0.1:80"}, hash_of("one.example.com"), MATCH_BOTH, nullptr, acquired) == HSMresult_t::DONE); + CHECK(acquired == right_addr); + CHECK(pool.count() == 1); + } + + pool.purge(); +} From 628fd82594dde9829d6a5adfdc851339a6206658 Mon Sep 17 00:00:00 2001 From: JosiahWI <41302989+JosiahWI@users.noreply.github.com> Date: Fri, 21 Aug 2026 08:10:30 -0500 Subject: [PATCH 2/4] Reset metrics gauge between tests Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/proxy/http/unit_tests/test_HttpSessionManager.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/proxy/http/unit_tests/test_HttpSessionManager.cc b/src/proxy/http/unit_tests/test_HttpSessionManager.cc index 548f33c31be..295d0eabc6c 100644 --- a/src/proxy/http/unit_tests/test_HttpSessionManager.cc +++ b/src/proxy/http/unit_tests/test_HttpSessionManager.cc @@ -126,13 +126,15 @@ class SessionFactory std::vector> _sessions; }; -/// The pool bookkeeping updates this gauge, which the test binary never registers. +/// The pool bookkeeping updates this gauge, which is normally initialized via HttpConfig. +/// Create it here (when needed) and reset it between Catch2 runs so tests don't leak state. void init_metrics() { if (http_rsb.pooled_server_connections == nullptr) { http_rsb.pooled_server_connections = Metrics::Gauge::createPtr("proxy.process.http.pooled_server_connections"); } + Metrics::Gauge::store(http_rsb.pooled_server_connections, 0); } CryptoHash From 0884106c4eb4e96cd9ad57237ee3fb1bd9f20acd Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Fri, 21 Aug 2026 08:11:20 -0500 Subject: [PATCH 3/4] Include header for strlen --- src/proxy/http/unit_tests/test_HttpSessionManager.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/proxy/http/unit_tests/test_HttpSessionManager.cc b/src/proxy/http/unit_tests/test_HttpSessionManager.cc index 295d0eabc6c..c8caa14448d 100644 --- a/src/proxy/http/unit_tests/test_HttpSessionManager.cc +++ b/src/proxy/http/unit_tests/test_HttpSessionManager.cc @@ -26,6 +26,7 @@ #include +#include #include #include @@ -142,7 +143,8 @@ hash_of(char const *hostname) { CryptoHash hash; - CryptoContext().hash_immediate(hash, static_cast(static_cast(hostname)), strlen(hostname)); + CryptoContext().hash_immediate(hash, static_cast(static_cast(hostname)), + std::strlen(hostname)); return hash; } From c84caf217054ca9f28a8d2cfb7d1d012d8587713 Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Fri, 21 Aug 2026 12:09:46 -0500 Subject: [PATCH 4/4] Make changes suggested by Brian Neradt Reset metric after each subcase Improve `sm` parameter doc --- include/proxy/http/HttpSessionManager.h | 6 ++++-- src/proxy/http/unit_tests/test_HttpSessionManager.cc | 1 + 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/include/proxy/http/HttpSessionManager.h b/include/proxy/http/HttpSessionManager.h index daa1a566521..674ca977260 100644 --- a/include/proxy/http/HttpSessionManager.h +++ b/include/proxy/http/HttpSessionManager.h @@ -100,8 +100,10 @@ class ServerSessionPool : public Continuation * @param[in] hostname_hash Cryptographic hash of the target hostname. * @param[in] match_style Bitmask of @c TSServerSessionSharingMatchMask values specifying which * attributes must agree between the candidate session and the request. - * @param[in] sm The requesting HTTP state machine; consulted for SNI and certificate - * validation when the corresponding bits are set in @p match_style. + * @param[in] sm The requesting HTTP state machine. Must be non-null when any SNI, + * HOSTSNISYNC, or CERT bit is set in @p match_style, or when the pool + * may contain multiplexing sessions; when non-null, @c t_state.scheme + * and @c t_state.hdr_info.server_request must be initialized. * @param[out] server_session Set to the matched session when @c HSMresult_t::DONE is returned; * @c nullptr otherwise. * diff --git a/src/proxy/http/unit_tests/test_HttpSessionManager.cc b/src/proxy/http/unit_tests/test_HttpSessionManager.cc index c8caa14448d..0a0b8cc3468 100644 --- a/src/proxy/http/unit_tests/test_HttpSessionManager.cc +++ b/src/proxy/http/unit_tests/test_HttpSessionManager.cc @@ -334,4 +334,5 @@ TEST_CASE("ServerSessionPool::acquireSession", "[session_pool]") } pool.purge(); + Metrics::Gauge::store(http_rsb.pooled_server_connections, 0); }