Fix overridable configuration testing - #13516
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a flawed overridable-configuration regression test that required preserving out-of-range enum values, which in turn prevented proper range checking and could break connection-group hashing/equality in production. It replaces that coverage with Catch2 unit tests that validate the overridable config name/key/type map and representative setter/getter paths using valid values, and it restores correct clamping behavior for the per-server connection match converter.
Changes:
- Add Catch2 unit tests for the HTTP overridable configuration lookup and representative set/get APIs.
- Add focused unit test coverage for
ConnectionTrackerserver-match conversion, and clamp invalid integer values to the documented enum range. - Remove the legacy InkAPITest regression that enforced invalid round-trips and add the new API unit test target to the build.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/iocore/net/unit_tests/test_ConnectionTracker.cc | Adds Catch2 coverage for server match converter round-trips and clamping behavior. |
| src/iocore/net/ConnectionTracker.cc | Restores correct clamping of server match values in SERVER_MATCH_CONV. |
| src/iocore/net/CMakeLists.txt | Includes the new ConnectionTracker unit test source in the net test target. |
| src/api/unit_tests/test_HttpOverridableConfig.cc | Adds Catch2 tests for overridable config discovery and representative int/float/string set/get paths (including clamped enum). |
| src/api/InkAPITest.cc | Removes the legacy overridable-config regression test that depended on accepting arbitrary out-of-range values. |
| src/api/CMakeLists.txt | Adds a new Catch2 test executable/ctest entry for the overridable-config API unit tests. |
moonchen
left a comment
There was a problem hiding this comment.
The production fix is right — server_match feeds Group::Key hashing and MATCH_TYPE_NAME is sized MATCH_BOTH + 1, so an out-of-range value was reachable from a plugin.
Two things on the test side.
The old regression test round-tripped set/get over all 137 OVERRIDABLE_CONFIGS entries; the new one covers four. #4210 asks for in-range values, not for fewer variables. Dropping the loop also leaves nothing checking that a row's declared DATA_TYPE matches the member's actual C++ type — the new Find case compares type against descriptor.type, and both come from the same X-macro row. Could the loop stay, reading each config's current value and writing it back? That round-trips every converter with a value that is valid by construction.
OverridableConfigDefs.h still lists SDK_Overridable_Configs test array (InkAPITest.cc) as a generation target; that array is gone after this change.
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: apache#4210
9c8303b to
490f22e
Compare
|
Addressed the review in
Verification in
|
moonchen
left a comment
There was a problem hiding this comment.
Thanks—my earlier concerns are addressed, and the targeted tests pass locally.
moonchen
left a comment
There was a problem hiding this comment.
The new SSL string getter branches do not preserve the length accepted by TSHttpTxnConfigStringSet(). The setter only calls strlen when length is -1, so callers may provide a live, non-NUL-terminated buffer with an explicit length. Each new getter instead calls strlen on the stored pointer, which reads beyond that buffer; embedded NUL bytes also produce the wrong returned length.
I reproduced this deterministically by placing a three-byte buffer immediately before a protected guard page, calling TSHttpTxnConfigStringSet(txn, TS_CONFIG_SSL_CLIENT_SNI_POLICY, input, 3), and then TSHttpTxnConfigStringGet(). The getter terminated with SIGSEGV in strlen. The same issue applies to all of the newly added direct SSL getter cases.
The current round-trip test misses this because TestHttpTxn starts with null SSL defaults and writes back an empty value, making the setter a no-op. Please preserve the supplied lengths, or store owned NUL-terminated copies, and add coverage using a non-empty bounded buffer.
490f22e to
7a25aba
Compare
|
Addressed the bounded SSL string issue in The direct SSL override setters now copy explicit-length values into stable Added focused coverage that round-trips all eight direct SSL keys using a non-NUL-terminated three-byte value containing an embedded NUL. Verification in |
The standalone API Catch test could not resolve core symbols on macOS and only exercised representative configuration paths. Several string getters also rejected supported settings or lost explicit SSL string lengths, leaving default safety and bounded values unchecked. Link production API objects into the test, round-trip every overridable setting, and retain transaction-owned SSL strings with their API lengths. Cover constrained values, embedded NULs, and clears.
7a25aba to
6758d6c
Compare
There was a problem hiding this comment.
🟢 Approval recommended
Review details
Suppressed comments (1)
src/iocore/net/ConnectionTracker.cc:60
- The comment above METRIC_ENABLED_CONV / METRIC_AGGREGATE_CONV still says they don’t clamp “for the same reason as SERVER_MATCH_CONV above”, but SERVER_MATCH_CONV now clamps. This is now misleading for future maintainers; update the comment to reflect the current rationale for leaving the metric converters unclamped.
*static_cast<decltype(TxnConfig::server_match) *>(data) = static_cast<decltype(TxnConfig::server_match)>(value);
},
nullptr,
nullptr,
[](const void *data) -> std::string_view {
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
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