From c841001bebd30e746f8e0b9a0067d96231d1082a Mon Sep 17 00:00:00 2001 From: bneradt Date: Fri, 7 Aug 2026 13:53:01 -0500 Subject: [PATCH 1/2] Fix overridable configuration testing The overridable configuration regression test writes arbitrary values to each setting and expects exact round trips. That forced the server match converter to retain out-of-range enum values, which can break connection-group hashing and equality in production. This patch replaces the regression test with Catch coverage that checks the full name/key/type map and representative public setter and getter paths with valid values. It also adds focused converter coverage and clamps server match values to its documented enum range, removing the production workaround for the flawed test. Fixes: #4210 --- src/api/CMakeLists.txt | 33 ++++ src/api/InkAPITest.cc | 139 +---------------- .../unit_tests/test_HttpOverridableConfig.cc | 144 ++++++++++++++++++ src/iocore/net/CMakeLists.txt | 1 + src/iocore/net/ConnectionTracker.cc | 10 +- .../net/unit_tests/test_ConnectionTracker.cc | 63 ++++++++ 6 files changed, 248 insertions(+), 142 deletions(-) create mode 100644 src/api/unit_tests/test_HttpOverridableConfig.cc create mode 100644 src/iocore/net/unit_tests/test_ConnectionTracker.cc diff --git a/src/api/CMakeLists.txt b/src/api/CMakeLists.txt index ca3ea344862..fcfc12b67dc 100644 --- a/src/api/CMakeLists.txt +++ b/src/api/CMakeLists.txt @@ -63,4 +63,37 @@ if(APPLE) target_link_options(tsapi PRIVATE -undefined dynamic_lookup) endif() +if(BUILD_TESTING) + add_executable(test_api unit_tests/test_HttpOverridableConfig.cc) + target_link_libraries( + test_api + PRIVATE Catch2::Catch2WithMain + ts::tscore + ts::tsapi + ts::overridable_txn_vars + ts::tsutil + ts::http + ts::http_remap + ts::http2 + ts::logging + ts::hdrs + ts::diagsconfig + ts::inkutils + ts::inkdns + ts::inkhostdb + ts::inkcache + ts::aio + ts::proxy + ts::inknet + ts::records + ts::inkevent + libswoc::libswoc + ts::jsonrpc_protocol + ts::jsonrpc_server + ts::rpcpublichandlers + ts::configmanager + ) + add_catch2_test(NAME test_api COMMAND test_api) +endif() + clang_tidy_check(tsapi) diff --git a/src/api/InkAPITest.cc b/src/api/InkAPITest.cc index eac60ccd3fc..3739da52399 100644 --- a/src/api/InkAPITest.cc +++ b/src/api/InkAPITest.cc @@ -54,8 +54,6 @@ #include "records/RecHttp.h" #include "proxy/http/HttpSM.h" -#include "proxy/http/OverridableConfigDefs.h" -#include "iocore/net/ConnectionTracker.h" #include "tscore/TestBox.h" namespace @@ -94,7 +92,8 @@ DbgCtl dbg_ctl_SockClient{"SockClient"}; #define ERROR_BODY "TESTING ERROR PAGE" #define TRANSFORM_APPEND_STRING "This is a transformed response" -extern int dns_failover_period; +extern int dns_failover_period; +extern ClassAllocator httpSMAllocator; ////////////////////////////////////////////////////////////////////////////// // STRUCTURES @@ -8704,140 +8703,6 @@ EXCLUSIVE_REGRESSION_TEST(SDK_API_TSHttpConnectServerIntercept)(RegressionTest * return; } -//////////////////////////////////////////////// -// SDK_API_OVERRIDABLE_CONFIGS -// -// Unit Test for API: TSHttpTxnConfigFind -// TSHttpTxnConfigIntSet -// TSHttpTxnConfigIntGet -// TSHttpTxnConfigFloatSet -// TSHttpTxnConfigFloatGet -// TSHttpTxnConfigStringSet -// TSHttpTxnConfigStringGet -//////////////////////////////////////////////// - -// Generate the SDK_Overridable_Configs array from the X-macro. -// The order MUST match TSOverridableConfigKey enum order (enforced by static_assert). -// clang-format off -#define X_SDK_CONFIG(CONFIG_KEY, MEMBER, RECORD_NAME, DATA_TYPE, CONV) RECORD_NAME, -std::array SDK_Overridable_Configs = {{ - OVERRIDABLE_CONFIGS(X_SDK_CONFIG) -}}; -#undef X_SDK_CONFIG -// clang-format on - -static_assert(SDK_Overridable_Configs.size() == TS_CONFIG_LAST_ENTRY, - "SDK_Overridable_Configs size must match TS_CONFIG_LAST_ENTRY"); - -extern ClassAllocator httpSMAllocator; - -REGRESSION_TEST(SDK_API_OVERRIDABLE_CONFIGS)(RegressionTest *test, int /* atype ATS_UNUSED */, int *pstatus) -{ - TSOverridableConfigKey key; - TSRecordDataType type; - HttpSM *s = THREAD_ALLOC(httpSMAllocator, this_thread()); - bool success = true; - TSHttpTxn txnp = reinterpret_cast(s); - InkRand generator(17); - TSMgmtInt ival_read, ival_rand; - TSMgmtFloat fval_read, fval_rand; - const char *sval_read; - const char *test_string = "The Apache Traffic Server"; - int len; - - s->init(); - s->mutex = new_ProxyMutex(); - SCOPED_MUTEX_LOCK(lock, s->mutex, this_ethread()); - - HttpCacheSM *c_sm = &(s->get_cache_sm()); - c_sm->init(s, s->mutex); - - *pstatus = REGRESSION_TEST_INPROGRESS; - for (int i = 0; i < static_cast(SDK_Overridable_Configs.size()); ++i) { - std::string_view conf{SDK_Overridable_Configs[i]}; - - if (TS_SUCCESS == TSHttpTxnConfigFind(conf.data(), -1, &key, &type)) { - if (key != i) { - SDK_RPRINT(test, "TSHttpTxnConfigFind", "TestCase1", TC_FAIL, "Failed on %s, expected %d, got %d", conf.data(), i, key); - success = false; - continue; - } - } else { - SDK_RPRINT(test, "TSHttpTxnConfigFind", "TestCase1", TC_FAIL, "Call returned unexpected TS_ERROR for %s", conf.data()); - success = false; - continue; - } - - if (TS_SUCCESS == TSHttpTxnConfigFind(conf.data(), conf.size(), &key, &type)) { - if (key != i) { - SDK_RPRINT(test, "TSHttpTxnConfigFind", "TestCase1", TC_FAIL, "Failed on %s, expected %d, got %d", conf.data(), i, key); - success = false; - continue; - } - } else { - SDK_RPRINT(test, "TSHttpTxnConfigFind", "TestCase1", TC_FAIL, "Call returned unexpected TS_ERROR for %s", conf.data()); - success = false; - continue; - } - - // Now check the getters / setters - switch (type) { - case TS_RECORDDATATYPE_INT: - ival_rand = generator.random() % 126; // to fit in a signed byte - TSHttpTxnConfigIntSet(txnp, key, ival_rand); - TSHttpTxnConfigIntGet(txnp, key, &ival_read); - if (ival_rand != ival_read) { - SDK_RPRINT(test, "TSHttpTxnConfigIntSet", "TestCase1", TC_FAIL, "Failed on %s, %d != %d", conf.data(), ival_read, - ival_rand); - success = false; - continue; - } - break; - - case TS_RECORDDATATYPE_FLOAT: - fval_rand = generator.random(); - TSHttpTxnConfigFloatSet(txnp, key, fval_rand); - TSHttpTxnConfigFloatGet(txnp, key, &fval_read); - if (fval_rand != fval_read) { - SDK_RPRINT(test, "TSHttpTxnConfigFloatSet", "TestCase1", TC_FAIL, "Failed on %s, %f != %f", conf.data(), fval_read, - fval_rand); - success = false; - continue; - } - break; - - case TS_RECORDDATATYPE_STRING: - TSHttpTxnConfigStringSet(txnp, key, test_string, -1); - TSHttpTxnConfigStringGet(txnp, key, &sval_read, &len); - // Compare string content, not pointers - the implementation may store - // a copy of the string (e.g., in ParsedConfigCache for efficiency). - if (sval_read == nullptr || std::string_view(test_string) != std::string_view(sval_read, len)) { - SDK_RPRINT(test, "TSHttpTxnConfigStringSet", "TestCase1", TC_FAIL, "Failed on %s, %s != %s", conf.data(), - sval_read ? sval_read : "(null)", test_string); - success = false; - continue; - } - break; - - default: - break; - } - } - - s->destroy(); - if (success) { - *pstatus = REGRESSION_TEST_PASSED; - SDK_RPRINT(test, "TSHttpTxnConfigFind", "TestCase1", TC_PASS, "ok"); - SDK_RPRINT(test, "TSHttpTxnConfigIntSet", "TestCase1", TC_PASS, "ok"); - SDK_RPRINT(test, "TSHttpTxnConfigFloatSet", "TestCase1", TC_PASS, "ok"); - SDK_RPRINT(test, "TSHttpTxnConfigStringSet", "TestCase1", TC_PASS, "ok"); - } else { - *pstatus = REGRESSION_TEST_FAILED; - } - - return; -} - //////////////////////////////////////////////// // SDK_API_TXN_HTTP_INFO_INFO_GET // diff --git a/src/api/unit_tests/test_HttpOverridableConfig.cc b/src/api/unit_tests/test_HttpOverridableConfig.cc new file mode 100644 index 00000000000..4166ee38eee --- /dev/null +++ b/src/api/unit_tests/test_HttpOverridableConfig.cc @@ -0,0 +1,144 @@ +/** @file + + Catch based unit tests for HTTP overridable configuration APIs. + + @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 "iocore/eventsystem/ConfigProcessor.h" +#include "iocore/net/ConnectionTracker.h" +#include "proxy/http/HttpConfig.h" +#include "proxy/http/HttpSM.h" +#include "proxy/http/OverridableConfigDefs.h" +#include "ts/ts.h" + +#include + +#include +#include + +using namespace std::literals; + +namespace +{ +struct ConfigDescriptor { + std::string_view name; + TSOverridableConfigKey key; + TSRecordDataType type; +}; + +// clang-format off +static constexpr std::array CONFIG_DESCRIPTORS{{ +#define X_CONFIG_DESCRIPTOR(CONFIG_KEY, MEMBER, RECORD_NAME, DATA_TYPE, CONV) \ + {RECORD_NAME, TS_CONFIG_##CONFIG_KEY, TS_RECORDDATATYPE_##DATA_TYPE}, + OVERRIDABLE_CONFIGS(X_CONFIG_DESCRIPTOR) +#undef X_CONFIG_DESCRIPTOR +}}; +// clang-format on + +class TestHttpTxn +{ +public: + TestHttpTxn() + { + if (HttpConfig::m_id == 0) { + HttpConfig::m_id = configProcessor.set(HttpConfig::m_id, new HttpConfigParams); + } + + _sm.magic = HttpSmMagic_t::ALIVE; + _sm.t_state.http_config_param = HttpConfig::acquire(); + REQUIRE(_sm.t_state.http_config_param != nullptr); + _sm.t_state.txn_conf = &_sm.t_state.http_config_param->oride; + } + + operator TSHttpTxn() { return reinterpret_cast(&_sm); } + +private: + HttpSM _sm; +}; +} // namespace + +TEST_CASE("Find HTTP overridable configurations", "[api][overridable-config]") +{ + for (auto const &descriptor : CONFIG_DESCRIPTORS) { + TSOverridableConfigKey key; + TSRecordDataType type; + + INFO(descriptor.name); + + REQUIRE(TSHttpTxnConfigFind(descriptor.name.data(), -1, &key, &type) == TS_SUCCESS); + CHECK(key == descriptor.key); + CHECK(type == descriptor.type); + + REQUIRE(TSHttpTxnConfigFind(descriptor.name.data(), descriptor.name.size(), &key, &type) == TS_SUCCESS); + CHECK(key == descriptor.key); + CHECK(type == descriptor.type); + } + + TSOverridableConfigKey key; + TSRecordDataType type; + + CHECK(TSHttpTxnConfigFind("proxy.config.invalid", -1, &key, &type) == TS_ERROR); +} + +TEST_CASE("Set and get HTTP overridable configurations", "[api][overridable-config]") +{ + TestHttpTxn txn; + + SECTION("integer") + { + static constexpr TSMgmtInt expected = 0; + TSMgmtInt actual; + + REQUIRE(TSHttpTxnConfigIntSet(txn, TS_CONFIG_HTTP_CACHE_HTTP, expected) == TS_SUCCESS); + REQUIRE(TSHttpTxnConfigIntGet(txn, TS_CONFIG_HTTP_CACHE_HTTP, &actual) == TS_SUCCESS); + CHECK(actual == expected); + } + + SECTION("float") + { + static constexpr TSMgmtFloat expected = 0.25; + TSMgmtFloat actual; + + REQUIRE(TSHttpTxnConfigFloatSet(txn, TS_CONFIG_HTTP_CACHE_HEURISTIC_LM_FACTOR, expected) == TS_SUCCESS); + REQUIRE(TSHttpTxnConfigFloatGet(txn, TS_CONFIG_HTTP_CACHE_HEURISTIC_LM_FACTOR, &actual) == TS_SUCCESS); + CHECK(actual == expected); + } + + SECTION("constrained integer") + { + TSMgmtInt actual; + + REQUIRE(TSHttpTxnConfigIntSet(txn, TS_CONFIG_HTTP_PER_SERVER_CONNECTION_MATCH, 95) == TS_SUCCESS); + REQUIRE(TSHttpTxnConfigIntGet(txn, TS_CONFIG_HTTP_PER_SERVER_CONNECTION_MATCH, &actual) == TS_SUCCESS); + CHECK(actual == TS_SERVER_OUTBOUND_MATCH_BOTH); + } + + SECTION("string") + { + static constexpr auto expected = "Catch test"sv; + const char *actual; + int length; + + REQUIRE(TSHttpTxnConfigStringSet(txn, TS_CONFIG_HTTP_RESPONSE_SERVER_STR, expected.data(), expected.size()) == TS_SUCCESS); + REQUIRE(TSHttpTxnConfigStringGet(txn, TS_CONFIG_HTTP_RESPONSE_SERVER_STR, &actual, &length) == TS_SUCCESS); + REQUIRE(actual != nullptr); + CHECK(std::string_view(actual, length) == expected); + } +} diff --git a/src/iocore/net/CMakeLists.txt b/src/iocore/net/CMakeLists.txt index b317e26b3ea..dba611fabb4 100644 --- a/src/iocore/net/CMakeLists.txt +++ b/src/iocore/net/CMakeLists.txt @@ -144,6 +144,7 @@ if(BUILD_TESTING) test_net libinknet_stub.cc NetVCTest.cc + unit_tests/test_ConnectionTracker.cc unit_tests/test_ProxyProtocol.cc unit_tests/test_SSLCertLookup.cc unit_tests/test_SSLNetVConnectionAsyncEp.cc diff --git a/src/iocore/net/ConnectionTracker.cc b/src/iocore/net/ConnectionTracker.cc index 45ce7e60f07..e6246e544b5 100644 --- a/src/iocore/net/ConnectionTracker.cc +++ b/src/iocore/net/ConnectionTracker.cc @@ -26,6 +26,8 @@ #include "records/RecCore.h" #include "swoc/IPAddr.h" +#include + using namespace std::literals; ConnectionTracker::TableSingleton ConnectionTracker::_inbound_table; @@ -49,11 +51,9 @@ const MgmtConverter ConnectionTracker::MIN_SERVER_CONV( const MgmtConverter ConnectionTracker::SERVER_MATCH_CONV{ [](const void *data) -> MgmtInt { return static_cast(*static_cast(data)); }, [](void *data, MgmtInt i) -> void { - // Problem - the InkAPITest requires being able to set an arbitrary value, so this can either - // correctly clamp or pass the regression tests. Currently it passes the tests. - // *static_cast(data) = std::clamp(static_cast(i), MATCH_IP, - // MATCH_BOTH); - *static_cast(data) = static_cast(i); + auto const value = std::clamp(i, static_cast(MATCH_IP), static_cast(MATCH_BOTH)); + + *static_cast(data) = static_cast(value); }, nullptr, nullptr, diff --git a/src/iocore/net/unit_tests/test_ConnectionTracker.cc b/src/iocore/net/unit_tests/test_ConnectionTracker.cc new file mode 100644 index 00000000000..ccd9d169d6f --- /dev/null +++ b/src/iocore/net/unit_tests/test_ConnectionTracker.cc @@ -0,0 +1,63 @@ +/** @file + + Catch based unit tests for connection tracking configuration. + + @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 "iocore/net/ConnectionTracker.h" + +#include + +#include + +TEST_CASE("Connection tracker server match conversion", "[libinknet][ConnectionTracker]") +{ + auto const &converter = ConnectionTracker::SERVER_MATCH_CONV; + auto match = ConnectionTracker::MATCH_IP; + + REQUIRE(converter.store_int != nullptr); + REQUIRE(converter.load_int != nullptr); + + SECTION("valid values round trip") + { + static constexpr std::array valid_values{ + ConnectionTracker::MATCH_IP, + ConnectionTracker::MATCH_PORT, + ConnectionTracker::MATCH_HOST, + ConnectionTracker::MATCH_BOTH, + }; + + for (auto const expected : valid_values) { + converter.store_int(&match, static_cast(expected)); + + CHECK(match == expected); + CHECK(converter.load_int(&match) == static_cast(expected)); + } + } + + SECTION("invalid values are clamped") + { + converter.store_int(&match, -1); + CHECK(match == ConnectionTracker::MATCH_IP); + + converter.store_int(&match, 95); + CHECK(match == ConnectionTracker::MATCH_BOTH); + } +} From 9c8303bfe0b94fec900c6cb47abcc1c1a4236e70 Mon Sep 17 00:00:00 2001 From: bneradt Date: Mon, 10 Aug 2026 19:59:11 -0500 Subject: [PATCH 2/2] Make API unit test self-contained The API Catch test loaded libtsapi on macOS, where unresolved core symbols are resolved by traffic_server at runtime. The standalone test therefore aborted in dyld before Catch ran because ET_UDP was absent. This patch builds the API implementation as reusable position-independent objects and links those objects into both libtsapi and the unit test. The test now exercises production API code while allowing platform linkers to resolve all static ATS dependencies directly. --- src/api/CMakeLists.txt | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/api/CMakeLists.txt b/src/api/CMakeLists.txt index fcfc12b67dc..8ef4f8d44f6 100644 --- a/src/api/CMakeLists.txt +++ b/src/api/CMakeLists.txt @@ -16,7 +16,15 @@ ####################### # plugin api *only* -add_library(tsapi SHARED InkAPI.cc InkAPIInternal.cc InkIOCoreAPI.cc) +add_library(tsapi_objects OBJECT InkAPI.cc InkAPIInternal.cc InkIOCoreAPI.cc) +set_target_properties(tsapi_objects PROPERTIES POSITION_INDEPENDENT_CODE ON) +target_link_libraries(tsapi_objects PRIVATE libswoc::libswoc yaml-cpp::yaml-cpp OpenSSL::SSL) +if(ENABLE_PROBES) + target_link_libraries(tsapi_objects PRIVATE systemtap::systemtap) +endif() + +add_library(tsapi SHARED) +target_link_libraries(tsapi PRIVATE tsapi_objects) if(BUILD_REGRESSION_TESTING) target_sources(tsapi PRIVATE InkAPITest.cc) endif() @@ -69,7 +77,7 @@ if(BUILD_TESTING) test_api PRIVATE Catch2::Catch2WithMain ts::tscore - ts::tsapi + tsapi_objects ts::overridable_txn_vars ts::tsutil ts::http @@ -96,4 +104,4 @@ if(BUILD_TESTING) add_catch2_test(NAME test_api COMMAND test_api) endif() -clang_tidy_check(tsapi) +clang_tidy_check(tsapi_objects)