From a32fadebfa8b01dc0c66323eacfcb7bbcc1b7913 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Tue, 5 May 2026 14:09:38 +0200 Subject: [PATCH 01/11] Add `_reload` directive support for config reload framework. (#13110) * Add _reload directive support to config reload framework Config handlers need a way to receive operational parameters (e.g. scoping a reload to a single entry) without conflating them with config content. This adds a reserved _reload key inside the configs YAML node that the framework extracts before invoking handlers. Framework: ConfigContext gains reload_directives() getter; the _reload node is extracted in ConfigRegistry::execute_reload() and stripped from supplied_yaml(). Fixes stale _passed_configs entries not being erased after consumption. CLI: traffic_ctl config reload gains --directive (-D) flag using dot-notation (config_key.directive_key=value). Multiple directives are space-separated after a single -D. Tests: unit tests for parse_directive and ConfigContext directive propagation; autest coverage for directive RPC structure handling. Docs: developer guide and traffic_ctl reference updated. (cherry picked from commit 07813a3875b51a9191591fb12ad2f7409afb51f9) --- .../command-line/traffic_ctl.en.rst | 52 +++ .../config-reload-framework.en.rst | 82 +++++ include/mgmt/config/ConfigContext.h | 20 +- src/mgmt/config/ConfigContext.cc | 15 +- src/mgmt/config/ConfigRegistry.cc | 35 +- src/records/CMakeLists.txt | 1 + .../unit_tests/test_ReloadDirectives.cc | 319 ++++++++++++++++++ src/traffic_ctl/CtrlCommands.cc | 48 +++ src/traffic_ctl/traffic_ctl.cc | 2 + .../jsonrpc/config_reload_rpc.test.py | 94 ++++++ 10 files changed, 657 insertions(+), 11 deletions(-) create mode 100644 src/records/unit_tests/test_ReloadDirectives.cc diff --git a/doc/appendices/command-line/traffic_ctl.en.rst b/doc/appendices/command-line/traffic_ctl.en.rst index 762dfdfaad5..59dccd05b30 100644 --- a/doc/appendices/command-line/traffic_ctl.en.rst +++ b/doc/appendices/command-line/traffic_ctl.en.rst @@ -416,6 +416,58 @@ Display the current value of a configuration record. will return an error for the corresponding key. The JSONRPC response will contain per-key error details. + .. option:: --directive, -D + + Pass a reload directive to a specific config handler. Directives are operational parameters + that modify how the handler performs the reload — for example, scoping a reload to a single + entry or enabling a dry-run mode. They are distinct from config content (``-d``). + + The format is ``config_key.directive_key=value``, parsed by splitting on the first ``.`` + and the first ``=``: + + - ``config_key`` — the registry key (e.g. ``ip_allow``, ``sni``) + - ``directive_key`` — the directive name understood by that handler + - ``value`` — the directive value (always passed as a string on the wire) + + Multiple directives are passed as space-separated values after a single ``-D``: + + .. code-block:: bash + + # Single directive + $ traffic_ctl config reload -D myconfig.id=foo + + # Multiple directives for the same handler + $ traffic_ctl config reload -D myconfig.id=foo myconfig.dry_run=true + + # Directives for different handlers in the same reload + $ traffic_ctl config reload -D myconfig.id=foo sni.fqdn=example.com + + On the wire, ``-D myconfig.id=foo`` translates to: + + .. code-block:: json + + { "configs": { "myconfig": { "_reload": { "id": "foo" } } } } + + For complex or nested directive values, use ``-d`` with full YAML instead: + + .. code-block:: bash + + $ traffic_ctl config reload -d 'myconfig: { _reload: { id: foo, options: { strict: true } } }' + + .. note:: + + ``-D`` uses variable-argument parsing and must appear as the **last option** + on the command line. Any flags placed after ``-D`` will be consumed as directive + values. ``-D`` and ``-d`` cannot be combined in the same invocation due to this + same constraint. Use ``-d`` with full YAML when you need both directives and + inline content in a single reload request. + + .. note:: + + Available directives depend on the handler — consult each config's documentation for + supported directive keys. Directive values are strings on the wire; handlers use + yaml-cpp's ``as()`` to interpret them as needed. + .. option:: --force, -F Force a new reload even if one is already in progress. Without this flag, the server rejects diff --git a/doc/developer-guide/config-reload-framework.en.rst b/doc/developer-guide/config-reload-framework.en.rst index 3eceee058cf..c91f5dda339 100644 --- a/doc/developer-guide/config-reload-framework.en.rst +++ b/doc/developer-guide/config-reload-framework.en.rst @@ -267,8 +267,90 @@ supplied_yaml() Returns the YAML node supplied via the RPC ``-d`` flag or ``configs`` parameter. If no inline content was provided, the returned node is undefined (``operator bool()`` returns ``false``). + The framework strips the reserved ``_reload`` key from the supplied YAML before delivering it + to the handler, so ``supplied_yaml()`` always contains pure config data. + +reload_directives() + Returns the YAML map extracted from the ``_reload`` key in the RPC-supplied content. If no + directives were provided, the returned node is Undefined (``operator bool()`` returns ``false``). + + Directives are operational parameters that modify **how** the handler performs the reload — + they are distinct from config **content**. Common uses include scoping a reload to a single + entry, enabling a dry-run mode, or passing a version constraint. + + On the wire, directives are nested under ``_reload`` inside the handler's ``configs`` node: + + .. code-block:: json + + { + "configs": { + "myconfig": { + "_reload": { "id": "foo", "dry_run": "true" }, + "rules": ["rule1", "rule2"] + } + } + } + + The framework extracts ``_reload`` before the handler runs, so: + + - ``reload_directives()`` returns ``{ "id": "foo", "dry_run": "true" }`` + - ``supplied_yaml()`` returns the remaining content (without ``_reload``) + - If ``_reload`` was the only key, ``supplied_yaml()`` is undefined + + Directives and content can coexist. The handler decides how to combine them — the framework + delivers both without interpretation. + + **Recommended handler pattern:** + + .. code-block:: cpp + + void MyConfig::reconfigure(ConfigContext ctx) { + ctx.in_progress(); + + if (auto directives = ctx.reload_directives()) { + if (auto id_node = directives["id"]; id_node.IsDefined()) { + std::string id = id_node.as(); + if (!reload_single_entry(id)) { + ctx.fail("Unknown entry: " + id); + return; + } + ctx.complete("Reloaded entry: " + id); + return; + } + } + + if (auto yaml = ctx.supplied_yaml()) { + if (!load_from_yaml(yaml)) { + ctx.fail("Invalid inline content"); + return; + } + ctx.complete("Loaded from inline content"); + return; + } + + if (!load_from_file(config_filename)) { + ctx.fail("Failed to parse " + config_filename); + return; + } + ctx.complete("Loaded from file"); + } + + From :program:`traffic_ctl`, directives are passed via ``--directive`` (``-D``): + + .. code-block:: bash + + $ traffic_ctl config reload -D myconfig.id=foo + + See the ``--directive`` option in :ref:`traffic_ctl ` for details. + + .. note:: + + Directive values are strings on the wire (the JSONRPC transport serializes all values as + double-quoted strings). Handlers use yaml-cpp's ``as()`` to interpret them as needed. + add_dependent_ctx(description) Create a child sub-task. The parent aggregates status from all its children. + Child contexts inherit both ``supplied_yaml()`` and ``reload_directives()`` from the parent. All methods support ``swoc::bwprint`` format strings: diff --git a/include/mgmt/config/ConfigContext.h b/include/mgmt/config/ConfigContext.h index 30ebe886eb9..e788598872f 100644 --- a/include/mgmt/config/ConfigContext.h +++ b/include/mgmt/config/ConfigContext.h @@ -173,19 +173,35 @@ class ConfigContext [[nodiscard]] ConfigContext add_dependent_ctx(std::string_view description = "", std::string_view filename = ""); /// Get supplied YAML node (for RPC-based reloads). - /// A default-constructed YAML::Node is Undefined (operator bool() == false). + /// Returns Undefined when no content was provided (operator bool() == false). /// @code /// if (auto yaml = ctx.supplied_yaml()) { /* use yaml node */ } /// @endcode /// @return copy of the supplied YAML node (cheap — YAML::Node is internally reference-counted). [[nodiscard]] YAML::Node supplied_yaml() const; + /// Get reload directives extracted from the _reload key. + /// Directives are operational parameters that modify how the handler performs + /// the reload (e.g. scope to a single entry, dry-run) — distinct from config content. + /// The framework extracts _reload from the supplied node before passing content + /// to the handler, so supplied_yaml() never contains _reload. + /// Returns Undefined when no directives were provided (operator bool() == false). + /// @code + /// if (auto directives = ctx.reload_directives()) { /* use directives */ } + /// @endcode + /// @return copy of the directives YAML node (cheap — YAML::Node is internally reference-counted). + [[nodiscard]] YAML::Node reload_directives() const; + private: /// Set supplied YAML node. Only ConfigRegistry should call this during reload setup. void set_supplied_yaml(YAML::Node node); + /// Set reload directives. Only ConfigRegistry should call this during reload setup. + void set_reload_directives(YAML::Node node); + std::weak_ptr _task; - YAML::Node _supplied_yaml; ///< for no content, this will just be empty + YAML::Node _supplied_yaml{YAML::NodeType::Undefined}; + YAML::Node _reload_directives{YAML::NodeType::Undefined}; friend class ReloadCoordinator; friend class config::ConfigRegistry; diff --git a/src/mgmt/config/ConfigContext.cc b/src/mgmt/config/ConfigContext.cc index fc5d2772c7a..2f26c9b098a 100644 --- a/src/mgmt/config/ConfigContext.cc +++ b/src/mgmt/config/ConfigContext.cc @@ -149,7 +149,8 @@ ConfigContext::add_dependent_ctx(std::string_view description, std::string_view // child task will get the full content of the parent task // TODO: eventually we can have a "key" passed so child module // only gets their node of interest. - child._supplied_yaml = _supplied_yaml; + child._supplied_yaml = _supplied_yaml; + child._reload_directives = _reload_directives; return child; } return {}; @@ -167,6 +168,18 @@ ConfigContext::supplied_yaml() const return _supplied_yaml; } +void +ConfigContext::set_reload_directives(YAML::Node node) +{ + _reload_directives = node; +} + +YAML::Node +ConfigContext::reload_directives() const +{ + return _reload_directives; +} + namespace config { ConfigContext diff --git a/src/mgmt/config/ConfigRegistry.cc b/src/mgmt/config/ConfigRegistry.cc index 0a3382f8463..776fa265d2f 100644 --- a/src/mgmt/config/ConfigRegistry.cc +++ b/src/mgmt/config/ConfigRegistry.cc @@ -431,15 +431,17 @@ ConfigRegistry::execute_reload(const std::string &key) { Dbg(dbg_ctl, "Executing reload for config '%s'", key.c_str()); - // Single lock for both lookups: passed config (from RPC) and registry entry YAML::Node passed_config; + bool has_passed_config{false}; Entry entry_copy; { - std::shared_lock lock(_mutex); + std::unique_lock lock(_mutex); if (auto pc_it = _passed_configs.find(key); pc_it != _passed_configs.end()) { - passed_config = pc_it->second; - Dbg(dbg_ctl, "Retrieved passed config for '%s'", key.c_str()); + passed_config = pc_it->second; + has_passed_config = true; + _passed_configs.erase(pc_it); + Dbg(dbg_ctl, "Retrieved and consumed passed config for '%s'", key.c_str()); } if (auto it = _entries.find(key); it != _entries.end()) { @@ -455,14 +457,31 @@ ConfigRegistry::execute_reload(const std::string &key) // Create context with subtask tracking // For rpc reload: use key as description, no filename (source: rpc) // For file reload: use key as description, filename indicates source: file - std::string filename = passed_config.IsDefined() ? "" : entry_copy.resolve_filename(); + std::string filename = has_passed_config ? "" : entry_copy.resolve_filename(); auto ctx = ReloadCoordinator::Get_Instance().create_config_context(entry_copy.key, entry_copy.key, filename); ctx.in_progress(); - if (passed_config.IsDefined()) { - // Passed config mode: store YAML node directly for handler to use via supplied_yaml() + if (has_passed_config) { Dbg(dbg_ctl, "Config '%s' reloading from rpc-supplied content", entry_copy.key.c_str()); - ctx.set_supplied_yaml(passed_config); + + // Extract _reload directives before passing content to the handler. + // This keeps supplied_yaml() clean (pure config data) and provides + // reload_directives() as a separate accessor for operational parameters. + if (passed_config.IsMap() && passed_config["_reload"]) { + auto directives = passed_config["_reload"]; + if (!directives.IsMap()) { + Warning("Config '%s': _reload must be a YAML map, ignoring directives", entry_copy.key.c_str()); + } else { + Dbg(dbg_ctl, "Config '%s' has reload directives", entry_copy.key.c_str()); + ctx.set_reload_directives(directives); + } + passed_config.remove("_reload"); + } + + // After stripping _reload, pass remaining content (if any) as supplied_yaml + if (passed_config.size() > 0) { + ctx.set_supplied_yaml(passed_config); + } } else { Dbg(dbg_ctl, "Config '%s' reloading from file '%s'", entry_copy.key.c_str(), filename.c_str()); } diff --git a/src/records/CMakeLists.txt b/src/records/CMakeLists.txt index f5e643ae692..6fd4ce94874 100644 --- a/src/records/CMakeLists.txt +++ b/src/records/CMakeLists.txt @@ -54,6 +54,7 @@ if(BUILD_TESTING) unit_tests/test_ConfigReloadTask.cc unit_tests/test_ConfigRegistry.cc unit_tests/test_RecHiddenMetricLookup.cc + unit_tests/test_ReloadDirectives.cc ) target_link_libraries(test_records PRIVATE records configmanager inkevent Catch2::Catch2 ts::tscore libswoc::libswoc) add_catch2_test(NAME test_records COMMAND test_records) diff --git a/src/records/unit_tests/test_ReloadDirectives.cc b/src/records/unit_tests/test_ReloadDirectives.cc new file mode 100644 index 00000000000..5b90b607081 --- /dev/null +++ b/src/records/unit_tests/test_ReloadDirectives.cc @@ -0,0 +1,319 @@ +/** @file + + Unit tests for reload directives: ConfigContext directive accessors, + framework extraction logic, and CLI parse_directive() format. + + @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 + +#include "mgmt/config/ConfigContext.h" +#include "mgmt/config/ConfigReloadTrace.h" + +#include +#include +#include + +// ─── parse_directive: standalone copy of the traffic_ctl parsing logic ──────── +// +// The actual function lives in an anonymous namespace in CtrlCommands.cc. +// We reproduce the identical logic here so we can unit-test the format parsing +// without pulling in the full traffic_ctl binary and its dependencies. +namespace +{ +bool +parse_directive(std::string_view dir, YAML::Node &configs, std::string &error_out) +{ + auto dot = dir.find('.'); + if (dot == std::string_view::npos || dot == 0) { + error_out = "Invalid directive format '" + std::string(dir) + "'. Expected: config_key.directive_key=value"; + return false; + } + + auto eq = dir.find('=', dot + 1); + if (eq == std::string_view::npos || eq == dot + 1) { + error_out = "Invalid directive format '" + std::string(dir) + "'. Expected: config_key.directive_key=value"; + return false; + } + + std::string config_key{dir.substr(0, dot)}; + std::string directive_key{dir.substr(dot + 1, eq - dot - 1)}; + std::string value{dir.substr(eq + 1)}; + + configs[config_key]["_reload"][directive_key] = value; + return true; +} +} // namespace + +// ─── parse_directive format tests ───────────────────────────────────────────── + +TEST_CASE("parse_directive: valid single directive", "[config][directive][parse]") +{ + YAML::Node configs; + std::string err; + + REQUIRE(parse_directive("myconfig.id=foo", configs, err)); + REQUIRE(configs["myconfig"]["_reload"]["id"].as() == "foo"); +} + +TEST_CASE("parse_directive: value with equals signs", "[config][directive][parse]") +{ + YAML::Node configs; + std::string err; + + REQUIRE(parse_directive("plugin.url=http://x.com/a=b", configs, err)); + REQUIRE(configs["plugin"]["_reload"]["url"].as() == "http://x.com/a=b"); +} + +TEST_CASE("parse_directive: value with dots", "[config][directive][parse]") +{ + YAML::Node configs; + std::string err; + + REQUIRE(parse_directive("plugin.fqdn=foo.example.com", configs, err)); + REQUIRE(configs["plugin"]["_reload"]["fqdn"].as() == "foo.example.com"); +} + +TEST_CASE("parse_directive: empty value is allowed", "[config][directive][parse]") +{ + YAML::Node configs; + std::string err; + + REQUIRE(parse_directive("myconfig.flag=", configs, err)); + REQUIRE(configs["myconfig"]["_reload"]["flag"].as() == ""); +} + +TEST_CASE("parse_directive: multiple directives for same config", "[config][directive][parse]") +{ + YAML::Node configs; + std::string err; + + REQUIRE(parse_directive("myconfig.id=foo", configs, err)); + REQUIRE(parse_directive("myconfig.dry_run=true", configs, err)); + + REQUIRE(configs["myconfig"]["_reload"]["id"].as() == "foo"); + REQUIRE(configs["myconfig"]["_reload"]["dry_run"].as() == "true"); +} + +TEST_CASE("parse_directive: multiple directives for different configs", "[config][directive][parse]") +{ + YAML::Node configs; + std::string err; + + REQUIRE(parse_directive("myconfig.id=foo", configs, err)); + REQUIRE(parse_directive("sni.fqdn=example.com", configs, err)); + + REQUIRE(configs["myconfig"]["_reload"]["id"].as() == "foo"); + REQUIRE(configs["sni"]["_reload"]["fqdn"].as() == "example.com"); +} + +TEST_CASE("parse_directive: rejects missing dot", "[config][directive][parse]") +{ + YAML::Node configs; + std::string err; + + REQUIRE_FALSE(parse_directive("nodot", configs, err)); + REQUIRE(err.find("Invalid directive format") != std::string::npos); +} + +TEST_CASE("parse_directive: rejects leading dot", "[config][directive][parse]") +{ + YAML::Node configs; + std::string err; + + REQUIRE_FALSE(parse_directive(".key=value", configs, err)); + REQUIRE(err.find("Invalid directive format") != std::string::npos); +} + +TEST_CASE("parse_directive: rejects missing equals", "[config][directive][parse]") +{ + YAML::Node configs; + std::string err; + + REQUIRE_FALSE(parse_directive("config.key", configs, err)); + REQUIRE(err.find("Invalid directive format") != std::string::npos); +} + +TEST_CASE("parse_directive: rejects empty directive key", "[config][directive][parse]") +{ + YAML::Node configs; + std::string err; + + REQUIRE_FALSE(parse_directive("config.=value", configs, err)); + REQUIRE(err.find("Invalid directive format") != std::string::npos); +} + +// ─── ConfigContext directive accessor tests ─────────────────────────────────── + +TEST_CASE("ConfigContext: reload_directives on default context has no keys", "[config][context][directive]") +{ + ConfigContext ctx; + + // Members are initialized as Undefined, so operator bool() is false. + YAML::Node const directives = ctx.reload_directives(); + REQUIRE_FALSE(directives.IsDefined()); + REQUIRE_FALSE(directives); + REQUIRE_FALSE(directives["id"].IsDefined()); +} + +TEST_CASE("ConfigContext: supplied_yaml on default context has no content", "[config][context][directive]") +{ + ConfigContext ctx; + + auto yaml = ctx.supplied_yaml(); + REQUIRE_FALSE(yaml.IsDefined()); + REQUIRE_FALSE(yaml); + REQUIRE_FALSE(yaml.IsMap()); + REQUIRE_FALSE(yaml.IsSequence()); +} + +TEST_CASE("ConfigContext: reload_directives round-trip via task", "[config][context][directive]") +{ + auto task = std::make_shared("test-dir-1", "test", false, nullptr); + ConfigContext ctx(task, "test_handler"); + + YAML::Node directives; + directives["id"] = "foo"; + directives["dry_run"] = "true"; + + // Use the private setter via a ConfigContext that has a live task. + // Since set_reload_directives is private and friend-accessible only from + // ConfigRegistry/ReloadCoordinator, we test through the public interface + // after setting up the state that execute_reload would create. + + // Simulate what ConfigRegistry::execute_reload() does: + // Build a passed_config with _reload, then manually extract + YAML::Node passed_config; + passed_config["_reload"]["id"] = "foo"; + passed_config["_reload"]["dry_run"] = "true"; + passed_config["data"] = "some content"; + + // Extract _reload (same logic as execute_reload) + if (passed_config.IsMap() && passed_config["_reload"]) { + auto dir = passed_config["_reload"]; + if (dir.IsMap()) { + // We can't call set_reload_directives directly (private). + // But we can verify the extraction logic works on the YAML node. + REQUIRE(dir["id"].as() == "foo"); + REQUIRE(dir["dry_run"].as() == "true"); + passed_config.remove("_reload"); + } + } + + // After extraction, passed_config should only have "data" + REQUIRE_FALSE(passed_config["_reload"].IsDefined()); + REQUIRE(passed_config["data"].as() == "some content"); + REQUIRE(passed_config.size() == 1); +} + +TEST_CASE("ConfigContext: _reload extraction with directives only", "[config][context][directive]") +{ + YAML::Node passed_config; + passed_config["_reload"]["id"] = "bar"; + + // Extract + YAML::Node directives; + if (passed_config.IsMap() && passed_config["_reload"]) { + directives = passed_config["_reload"]; + passed_config.remove("_reload"); + } + + REQUIRE(directives.IsDefined()); + REQUIRE(directives["id"].as() == "bar"); + + // After extraction with only _reload, the map should be empty + REQUIRE(passed_config.size() == 0); +} + +TEST_CASE("ConfigContext: _reload extraction with content only", "[config][context][directive]") +{ + YAML::Node passed_config; + passed_config["rules"].push_back("rule1"); + passed_config["rules"].push_back("rule2"); + + bool extracted = false; + if (passed_config.IsMap() && passed_config["_reload"]) { + extracted = true; + passed_config.remove("_reload"); + } + + // No _reload key present — extraction did not fire + REQUIRE_FALSE(extracted); + + // Content untouched + REQUIRE(passed_config["rules"].size() == 2); +} + +TEST_CASE("ConfigContext: _reload non-map is rejected", "[config][context][directive]") +{ + YAML::Node passed_config; + passed_config["_reload"] = "scalar_value"; + passed_config["data"] = "content"; + + bool extracted = false; + bool rejected = false; + if (passed_config.IsMap() && passed_config["_reload"]) { + auto dir = passed_config["_reload"]; + if (!dir.IsMap()) { + rejected = true; + } else { + extracted = true; + } + passed_config.remove("_reload"); + } + + REQUIRE(rejected); + REQUIRE_FALSE(extracted); + // _reload is still removed even when rejected + REQUIRE_FALSE(passed_config["_reload"].IsDefined()); + REQUIRE(passed_config["data"].as() == "content"); +} + +// ─── Wire format integration: -D flag produces correct YAML structure ───────── + +TEST_CASE("Wire format: -D produces _reload nested under config key", "[config][directive][wire]") +{ + YAML::Node configs; + std::string err; + + parse_directive("myconfig.id=foo", configs, err); + + // Verify the structure matches what the server expects + REQUIRE(configs.IsMap()); + REQUIRE(configs["myconfig"].IsMap()); + REQUIRE(configs["myconfig"]["_reload"].IsMap()); + REQUIRE(configs["myconfig"]["_reload"]["id"].as() == "foo"); +} + +TEST_CASE("Wire format: -D combined with -d content", "[config][directive][wire]") +{ + YAML::Node configs; + + // Simulate -d providing content + configs["myconfig"]["rules"].push_back("rule1"); + + // Then -D adding directives + std::string err; + parse_directive("myconfig.id=foo", configs, err); + + // Both coexist under the same config key + REQUIRE(configs["myconfig"]["rules"].size() == 1); + REQUIRE(configs["myconfig"]["_reload"]["id"].as() == "foo"); +} diff --git a/src/traffic_ctl/CtrlCommands.cc b/src/traffic_ctl/CtrlCommands.cc index b90d14c43c9..ed15e90bc51 100644 --- a/src/traffic_ctl/CtrlCommands.cc +++ b/src/traffic_ctl/CtrlCommands.cc @@ -81,6 +81,33 @@ display_errors(BasePrinter *printer, std::vector co } } } + +/// Parse a single --directive (-D) argument "config_key.directive_key=value" +/// and inject into configs[config_key]["_reload"][directive_key]. +/// Returns true on success, sets error_out on parse failure. +bool +parse_directive(std::string_view dir, YAML::Node &configs, std::string &error_out) +{ + auto dot = dir.find('.'); + if (dot == std::string_view::npos || dot == 0) { + error_out = "Invalid directive format '" + std::string(dir) + "'. Expected: config_key.directive_key=value"; + return false; + } + + auto eq = dir.find('=', dot + 1); + if (eq == std::string_view::npos || eq == dot + 1) { + error_out = "Invalid directive format '" + std::string(dir) + "'. Expected: config_key.directive_key=value"; + return false; + } + + std::string config_key{dir.substr(0, dot)}; + std::string directive_key{dir.substr(dot + 1, eq - dot - 1)}; + std::string value{dir.substr(eq + 1)}; + + configs[config_key]["_reload"][directive_key] = value; + return true; +} + } // namespace BasePrinter::Options::FormatFlags @@ -557,6 +584,27 @@ ConfigCommand::config_reload() } } + // Parse --directive (-D) arguments into configs[key]["_reload"][directive] = value + auto dir_args = get_parsed_arguments()->get("directive"); + for (auto const &dir : dir_args) { + if (dir.empty()) { + continue; + } + if (dir[0] == '-') { + _printer->write_output("Error: '" + dir + + "' looks like a flag, not a directive. " + "Place -D as the last option on the command line."); + App_Exit_Status_Code = CTRL_EX_ERROR; + return; + } + std::string err; + if (!parse_directive(dir, configs, err)) { + _printer->write_output("Error: " + err); + App_Exit_Status_Code = CTRL_EX_ERROR; + return; + } + } + using ConfigError = config::reload::errors::ConfigReloadError; auto contains_error = [](std::vector const &errors, ConfigError error) -> bool { diff --git a/src/traffic_ctl/traffic_ctl.cc b/src/traffic_ctl/traffic_ctl.cc index ba704f1745b..478150d251f 100644 --- a/src/traffic_ctl/traffic_ctl.cc +++ b/src/traffic_ctl/traffic_ctl.cc @@ -164,6 +164,8 @@ main([[maybe_unused]] int argc, const char **argv) // -d @- - read config from stdin // -d "yaml: content" - inline yaml string .add_option("--data", "-d", "Inline config data (@file, @- for stdin, or yaml string)", "", MORE_THAN_ZERO_ARG_N, "") + .add_option("--directive", "-D", "Pass a reload directive to a config handler (format: config_key.directive_key=value)", "", + MORE_THAN_ZERO_ARG_N, "") .add_option( "--initial-wait", "-w", "Initial wait before first poll, giving the server time to schedule all handlers (seconds). Accepts fractional values", "", 1, diff --git a/tests/gold_tests/jsonrpc/config_reload_rpc.test.py b/tests/gold_tests/jsonrpc/config_reload_rpc.test.py index bf5f3265f6d..55d38103452 100644 --- a/tests/gold_tests/jsonrpc/config_reload_rpc.test.py +++ b/tests/gold_tests/jsonrpc/config_reload_rpc.test.py @@ -395,3 +395,97 @@ def validate_large_config(resp: Response): tr.Processes.Default.Streams.stdout = Testers.CustomJSONRPCResponse(validate_large_config) tr.StillRunningAfter = ts + +# ============================================================================ +# Test 11: Reload directive for registered FileOnly config (sni) +# Directive-only request — sni is FileOnly, so the RPC handler rejects with 6011. +# Verifies the _reload structure is handled gracefully through the RPC stack. +# ============================================================================ +tr = Test.AddTestRun("Reload directive for FileOnly config (sni)") +tr.DelayStart = 2 +tr.AddJsonRPCClientRequest(ts, Request.admin_config_reload(configs={"sni": {"_reload": {"fqdn": "*.example.com"}}})) + + +def validate_directive_fileonly(resp: Response): + '''sni is FileOnly — directive-only request rejected with 6011''' + result = resp.result + errors = result.get('errors', []) + + if not errors: + return (False, f"Expected rejection for FileOnly config, got: {result}") + + error_str = str(errors) + if '6011' in error_str: + return (True, f"Directive-only correctly rejected for FileOnly config: {errors}") + return (False, f"Expected error 6011, got: {errors}") + + +tr.Processes.Default.Streams.stdout = Testers.CustomJSONRPCResponse(validate_directive_fileonly) +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 12: Reload directive for unregistered config (virtualhost) +# virtualhost is not registered yet — should get 6010. +# This is the intended use case once the virtualhost handler is registered. +# ============================================================================ +tr = Test.AddTestRun("Reload directive for unregistered config (virtualhost)") +tr.DelayStart = 2 +tr.AddJsonRPCClientRequest(ts, Request.admin_config_reload(configs={"virtualhost": {"_reload": {"id": "myhost.example.com"}}})) + + +def validate_directive_unregistered(resp: Response): + '''virtualhost is not registered — rejected with 6010''' + result = resp.result + errors = result.get('errors', []) + + if not errors: + return (False, f"Expected error for unregistered config, got: {result}") + + error_str = str(errors) + if '6010' in error_str or 'not registered' in error_str: + return (True, f"Directive for unregistered config rejected: {errors}") + return (False, f"Expected error 6010, got: {errors}") + + +tr.Processes.Default.Streams.stdout = Testers.CustomJSONRPCResponse(validate_directive_unregistered) +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 13: Directives mixed with content for FileOnly config (ip_allow) +# _reload directives alongside actual config content — still rejected with 6011. +# ============================================================================ +tr = Test.AddTestRun("Directives mixed with content for FileOnly config") +tr.DelayStart = 2 +tr.AddJsonRPCClientRequest( + ts, + Request.admin_config_reload( + configs={ + "ip_allow": { + "_reload": { + "validate_only": "true" + }, + "rules": [{ + "apply": "in", + "ip_addrs": "0/0", + "action": "allow" + }] + } + })) + + +def validate_directive_mixed(resp: Response): + '''ip_allow is FileOnly — mixed directive+content rejected with 6011''' + result = resp.result + errors = result.get('errors', []) + + if not errors: + return (False, f"Expected rejection, got: {result}") + + error_str = str(errors) + if '6011' in error_str: + return (True, f"Mixed directive+content correctly rejected: {errors}") + return (False, f"Expected error 6011, got: {errors}") + + +tr.Processes.Default.Streams.stdout = Testers.CustomJSONRPCResponse(validate_directive_mixed) +tr.StillRunningAfter = ts From d2a21628e4fdceb70eacbeea28473b1e1797a2fd Mon Sep 17 00:00:00 2001 From: JosiahWI <41302989+JosiahWI@users.noreply.github.com> Date: Wed, 26 Aug 2026 09:40:20 -0500 Subject: [PATCH 02/11] Skip rate_limit AuTests when plugin not built (#13589) The rate_limit plugin is experimental at this time and not guaranteed to be in the build. Most of the rate_limit AuTests already handle this appropriately, but a few were missing the `SkipUnless` directive. (cherry picked from commit e23cedefb9cbbb8c43c4b6bd6a93040878edf8ba) --- tests/gold_tests/pluginTest/rate_limit/rate_limit.test.py | 1 + tests/gold_tests/pluginTest/rate_limit/rate_limit_iprep.test.py | 1 + tests/gold_tests/pluginTest/rate_limit/rate_limit_sni.test.py | 1 + 3 files changed, 3 insertions(+) diff --git a/tests/gold_tests/pluginTest/rate_limit/rate_limit.test.py b/tests/gold_tests/pluginTest/rate_limit/rate_limit.test.py index 573de3e7730..ed6abcc6a34 100644 --- a/tests/gold_tests/pluginTest/rate_limit/rate_limit.test.py +++ b/tests/gold_tests/pluginTest/rate_limit/rate_limit.test.py @@ -21,6 +21,7 @@ Test rate_limit plugin: concurrent limit enforcement, queue drain, and independent limiters. ''' +Test.SkipUnless(Condition.PluginExists('rate_limit.so')) Test.ContinueOnFail = True server = Test.MakeOriginServer("server", delay=3) diff --git a/tests/gold_tests/pluginTest/rate_limit/rate_limit_iprep.test.py b/tests/gold_tests/pluginTest/rate_limit/rate_limit_iprep.test.py index ba6003264d1..c22e064020c 100644 --- a/tests/gold_tests/pluginTest/rate_limit/rate_limit_iprep.test.py +++ b/tests/gold_tests/pluginTest/rate_limit/rate_limit_iprep.test.py @@ -28,6 +28,7 @@ Test rate_limit ip-rep initialization: reserve() vs resize() regression (Finding #108). ''' +Test.SkipUnless(Condition.PluginExists('rate_limit.so')) Test.ContinueOnFail = True server = Test.MakeOriginServer("server") diff --git a/tests/gold_tests/pluginTest/rate_limit/rate_limit_sni.test.py b/tests/gold_tests/pluginTest/rate_limit/rate_limit_sni.test.py index 5389bde7ed8..24be86bd77a 100644 --- a/tests/gold_tests/pluginTest/rate_limit/rate_limit_sni.test.py +++ b/tests/gold_tests/pluginTest/rate_limit/rate_limit_sni.test.py @@ -27,6 +27,7 @@ Test rate_limit SNI queue expiry: active counter underflow regression (Finding #109). ''' +Test.SkipUnless(Condition.PluginExists('rate_limit.so')) Test.ContinueOnFail = True server = Test.MakeOriginServer("server", delay=4) From 91f21fb4860f659e1cfbdd8e2b31787685cba13b Mon Sep 17 00:00:00 2001 From: Brian Neradt Date: Thu, 27 Aug 2026 17:32:11 -0500 Subject: [PATCH 03/11] Fix Clang 21 analyzer findings (#13593) Newer Clang releases expose latent ownership and error-handling issues, while the analyzer preset currently produces a GCC compilation database that Clang cannot reliably consume. This patch selects Clang explicitly for the analyzer preset, fixes the reported leaks, unchecked stream calls, and directory scanning under a mutex, and reshapes the remaining flagged code so the analyzer can follow it. That gives ATS a clean diagnostic baseline before the job moves to Ubuntu 26.04. Co-authored-by: Claude Opus 5 (cherry picked from commit c31517c86b12527ea1044d2fb6055c9ced35054d) --- CMakePresets.json | 2 + plugins/healthchecks/healthchecks.cc | 110 ++++++++++---------- src/iocore/hostdb/HostDB.cc | 4 + src/iocore/net/OCSPStapling.cc | 20 ++-- src/iocore/net/OpenSSLQUICNetVConnection.cc | 5 +- src/proxy/http/HttpBodyFactory.cc | 36 +++---- src/proxy/http/HttpProxyServerMain.cc | 29 ++++-- src/proxy/http3/Http3Frame.cc | 3 +- src/proxy/http3/Http3Session.cc | 5 +- src/proxy/http3/QPACK.cc | 6 +- src/proxy/logging/LogAccess.cc | 2 +- src/proxy/logging/LogFieldFallback.cc | 8 +- 12 files changed, 124 insertions(+), 106 deletions(-) diff --git a/CMakePresets.json b/CMakePresets.json index 68bbb2d4e4a..fd504ff15f4 100644 --- a/CMakePresets.json +++ b/CMakePresets.json @@ -267,6 +267,8 @@ "description": "CI Pipeline config for running clang-analyzer", "inherits": ["ci"], "cacheVariables": { + "CMAKE_C_COMPILER": "clang", + "CMAKE_CXX_COMPILER": "clang++", "CMAKE_EXPORT_COMPILE_COMMANDS": "ON", "ENABLE_CCACHE": "OFF", "ENABLE_EXAMPLE": "OFF", diff --git a/plugins/healthchecks/healthchecks.cc b/plugins/healthchecks/healthchecks.cc index c5da85da9da..ece4319aee2 100644 --- a/plugins/healthchecks/healthchecks.cc +++ b/plugins/healthchecks/healthchecks.cc @@ -140,9 +140,9 @@ load_status_file(HCFileInfo *info) if (nullptr != (fd = fopen(info->fname, "r"))) { data->exists = 1; - size_t bytes_read; - while ((bytes_read = fread(data->body, 1, MAX_BODY_LEN, fd)) > 0) { - data->b_len = static_cast(bytes_read); + data->b_len = static_cast(fread(data->body, 1, MAX_BODY_LEN, fd)); + if (ferror(fd)) { + data->b_len = 0; } fclose(fd); } @@ -333,68 +333,66 @@ parse_configs(const char *fname) return nullptr; } - while (!feof(fd)) { + while (fgets(buf, sizeof(buf) - 1, fd) != nullptr) { char *str, *save; char *ok = nullptr, *miss = nullptr, *mime = nullptr; - if (fgets(buf, sizeof(buf) - 1, fd)) { - finfo = new HCFileInfo(); - - str = strtok_r(buf, SEPARATORS, &save); - int state = 0; - while (nullptr != str) { - if (strlen(str) > 0) { - switch (state) { - case 0: - if ('/' == *str) { - ++str; - } - strncpy(finfo->path, str, PATH_NAME_MAX - 1); - finfo->path[PATH_NAME_MAX - 1] = '\0'; - finfo->p_len = strlen(finfo->path); - break; - case 1: - strncpy(finfo->fname, str, MAX_PATH_LEN - 1); - finfo->fname[MAX_PATH_LEN - 1] = '\0'; - finfo->basename = strrchr(finfo->fname, '/'); - if (finfo->basename) { - ++(finfo->basename); - finfo->basename_len = strlen(finfo->basename); - } - break; - case 2: - mime = str; - break; - case 3: - ok = str; - break; - case 4: - miss = str; - break; + finfo = new HCFileInfo(); + + str = strtok_r(buf, SEPARATORS, &save); + int state = 0; + while (nullptr != str) { + if (strlen(str) > 0) { + switch (state) { + case 0: + if ('/' == *str) { + ++str; + } + strncpy(finfo->path, str, PATH_NAME_MAX - 1); + finfo->path[PATH_NAME_MAX - 1] = '\0'; + finfo->p_len = strlen(finfo->path); + break; + case 1: + strncpy(finfo->fname, str, MAX_PATH_LEN - 1); + finfo->fname[MAX_PATH_LEN - 1] = '\0'; + finfo->basename = strrchr(finfo->fname, '/'); + if (finfo->basename) { + ++(finfo->basename); + finfo->basename_len = strlen(finfo->basename); } - ++state; + break; + case 2: + mime = str; + break; + case 3: + ok = str; + break; + case 4: + miss = str; + break; } - str = strtok_r(nullptr, SEPARATORS, &save); + ++state; } + str = strtok_r(nullptr, SEPARATORS, &save); + } - /* Fill in the info if everything was ok */ - if (state > 4) { - Dbg(dbg_ctl, "Parsed: %s %s %s %s %s", finfo->path, finfo->fname, mime, ok, miss); - finfo->ok = gen_header(ok, mime, &finfo->o_len); - finfo->miss = gen_header(miss, mime, &finfo->m_len); - finfo->set_data(load_status_file(finfo)); - - /* Add it the linked list */ - Dbg(dbg_ctl, "Adding path=%s to linked list", finfo->path); - if (nullptr == head_finfo) { - head_finfo = finfo; - } else { - prev_finfo->_next = finfo; - } - prev_finfo = finfo; + /* Fill in the info if everything was ok */ + if (state > 4) { + Dbg(dbg_ctl, "Parsed: %s %s %s %s %s", finfo->path, finfo->fname, mime, ok, miss); + finfo->ok = gen_header(ok, mime, &finfo->o_len); + finfo->miss = gen_header(miss, mime, &finfo->m_len); + finfo->set_data(load_status_file(finfo)); + + /* Add it the linked list */ + Dbg(dbg_ctl, "Adding path=%s to linked list", finfo->path); + if (nullptr == head_finfo) { + head_finfo = finfo; } else { - delete finfo; + prev_finfo->_next = finfo; } + prev_finfo = finfo; + } else { + delete finfo; } } fclose(fd); diff --git a/src/iocore/hostdb/HostDB.cc b/src/iocore/hostdb/HostDB.cc index 6abda699462..6011d24b6b0 100644 --- a/src/iocore/hostdb/HostDB.cc +++ b/src/iocore/hostdb/HostDB.cc @@ -953,6 +953,10 @@ HostDBContinuation::dnsEvent(int event, HostEnt *e) ts::LocalBuffer q_buf(valid_records); SRV **q = q_buf.data(); ink_assert(valid_records <= static_cast(hostdb_round_robin_max_count)); + // The loop below assigns every element, but ts::LocalBuffer hands back raw storage and the + // static analyzer cannot follow the loop well enough to see that. Pre-fill so the sort below + // is never reported as reading an uninitialized pointer. + std::fill_n(q, valid_records, nullptr); for (int i = 0; i < valid_records; ++i) { q[i] = &e->srv_hosts.hosts[i]; } diff --git a/src/iocore/net/OCSPStapling.cc b/src/iocore/net/OCSPStapling.cc index 715e68d2f90..d4b2e7cf98e 100644 --- a/src/iocore/net/OCSPStapling.cc +++ b/src/iocore/net/OCSPStapling.cc @@ -23,6 +23,7 @@ #include #include +#include #include #include @@ -898,19 +899,22 @@ ssl_stapling_init_cert(SSL_CTX *ctx, X509 *cert, const char *certname, const cha Dbg(dbg_ctl_ssl_ocsp, "using OCSP prefetched response file %s", rsp_file); FILE *fp = fopen(rsp_file, "r"); if (fp) { - fseek(fp, 0, SEEK_END); - long rsp_buf_len = ftell(fp); - if (rsp_buf_len >= 0) { - rewind(fp); - unsigned char *rsp_buf = static_cast(malloc(rsp_buf_len)); - auto read_len = fread(rsp_buf, 1, rsp_buf_len, fp); + long rsp_buf_len = -1; + + if (fseek(fp, 0, SEEK_END) == 0) { + rsp_buf_len = ftell(fp); + } + + if (rsp_buf_len > 0 && fseek(fp, 0, SEEK_SET) == 0) { + std::vector rsp_buf(rsp_buf_len); + auto read_len = fread(rsp_buf.data(), 1, rsp_buf.size(), fp); + if (read_len == static_cast(rsp_buf_len)) { - const unsigned char *p = rsp_buf; + const unsigned char *p = rsp_buf.data(); rsp = d2i_TS_OCSP_RESPONSE(nullptr, &p, rsp_buf_len); } else { Error("stapling_refresh_response: failed to read prefetched response file: %s", rsp_file); } - free(rsp_buf); } else { Error("stapling_refresh_response: failed to check the size of prefetched response file: %s", rsp_file); } diff --git a/src/iocore/net/OpenSSLQUICNetVConnection.cc b/src/iocore/net/OpenSSLQUICNetVConnection.cc index 9a06fb94d26..d71fefd4e64 100644 --- a/src/iocore/net/OpenSSLQUICNetVConnection.cc +++ b/src/iocore/net/OpenSSLQUICNetVConnection.cc @@ -389,7 +389,10 @@ QUICNetVConnection::acceptEvent(int event, Event *e) MUTEX_TRY_LOCK(lock, h->mutex, t); if (!lock.is_locked()) { - if (event == EVENT_NONE) { + // The event system always hands over a non-null Event, so @a e is only null if this is called + // directly, which pairs with EVENT_NONE. Reschedule on the thread in that case rather than + // dereferencing @a e. + if (event == EVENT_NONE || e == nullptr) { t->schedule_in(this, HRTIME_MSECONDS(net_retry_delay)); return EVENT_DONE; } else { diff --git a/src/proxy/http/HttpBodyFactory.cc b/src/proxy/http/HttpBodyFactory.cc index cdaaac582d8..fc91ce77825 100644 --- a/src/proxy/http/HttpBodyFactory.cc +++ b/src/proxy/http/HttpBodyFactory.cc @@ -261,6 +261,7 @@ HttpBodyFactory::reconfigure() unlock(); return; } // callbacks not setup right + unlock(); //////////////////////////////////////////// // extract relevant records.yaml values // @@ -272,14 +273,14 @@ HttpBodyFactory::reconfigure() // enable_customizations if records.yaml set auto e{RecGetRecordInt("proxy.config.body_factory.enable_customizations")}; - enable_customizations = (e.has_value() ? e.value() : 0); - all_found = all_found && e.has_value(); - Dbg(dbg_ctl_body_factory, "enable_customizations = %d (found = %d)", enable_customizations, e.has_value()); + int new_enable_customizations = (e.has_value() ? e.value() : 0); + all_found = all_found && e.has_value(); + Dbg(dbg_ctl_body_factory, "enable_customizations = %d (found = %d)", new_enable_customizations, e.has_value()); - e = RecGetRecordInt("proxy.config.body_factory.enable_logging"); - enable_logging = (e.has_value() ? (e.value() ? true : false) : false); - all_found = all_found && e.has_value(); - Dbg(dbg_ctl_body_factory, "enable_logging = %d (found = %d)", enable_logging, e.has_value()); + e = RecGetRecordInt("proxy.config.body_factory.enable_logging"); + bool new_enable_logging = (e.has_value() ? (e.value() ? true : false) : false); + all_found = all_found && e.has_value(); + Dbg(dbg_ctl_body_factory, "enable_logging = %d (found = %d)", new_enable_logging, e.has_value()); ats_scoped_str directory_of_template_sets; @@ -305,21 +306,16 @@ HttpBodyFactory::reconfigure() Warning("config changed, but can't fetch all proxy.config.body_factory values"); } - ///////////////////////////////////////////// - // clear out previous template hash tables // - ///////////////////////////////////////////// - - nuke_template_tables(); - - ///////////////////////////////////////////////////////////// - // at this point, the body hash table is gone, so we start // - // building a new one, by scanning the template directory. // - ///////////////////////////////////////////////////////////// - + std::unique_ptr new_table_of_sets; if (directory_of_template_sets) { - table_of_sets = load_sets_from_directory(directory_of_template_sets); + new_table_of_sets = load_sets_from_directory(directory_of_template_sets); } + lock(); + enable_customizations = new_enable_customizations; + enable_logging = new_enable_logging; + nuke_template_tables(); + table_of_sets = std::move(new_table_of_sets); unlock(); } @@ -727,7 +723,6 @@ HttpBodyFactory::nuke_template_tables() } } -// LOCKING: must be called with lock taken std::unique_ptr HttpBodyFactory::load_sets_from_directory(char *set_dir) { @@ -797,7 +792,6 @@ HttpBodyFactory::load_sets_from_directory(char *set_dir) return new_table_of_sets; } -// LOCKING: must be called with lock taken HttpBodySet * HttpBodyFactory::load_body_set_from_directory(char *set_name, char *tmpl_dir) { diff --git a/src/proxy/http/HttpProxyServerMain.cc b/src/proxy/http/HttpProxyServerMain.cc index 467fcad36a0..4701d9771cf 100644 --- a/src/proxy/http/HttpProxyServerMain.cc +++ b/src/proxy/http/HttpProxyServerMain.cc @@ -194,19 +194,30 @@ MakeHttpProxyAcceptor(HttpProxyAcceptor &acceptor, HttpProxyPort &port, unsigned // XXX the protocol probe should be a configuration option. - ProtocolProbeSessionAccept *probe = new ProtocolProbeSessionAccept(); + // A QUIC port is dispatched to the QUIC acceptor below, which has no probe fallback, so building a + // probe for one only leaks it. Every other port type ends up behind the probe, either directly or + // as the SSL acceptor's fallback. Without QUIC compiled in no port can be a QUIC port, so this is + // always true there. + bool const needs_probe = !port.isQUIC(); + + ProtocolProbeSessionAccept *probe = nullptr; HttpSessionAccept *http = nullptr; // don't allocate this unless it will be used. - probe->proxyPort = &port; - probe->proxy_protocol_ipmap = &HttpConfig::m_master.config_proxy_protocol_ip_addrs; - if (port.m_session_protocol_preference.intersects(HTTP_PROTOCOL_SET)) { - http = new HttpSessionAccept(accept_opt); - probe->registerEndpoint(ProtocolProbeSessionAccept::ProtoGroupKey::HTTP, http); - } + if (needs_probe) { + probe = new ProtocolProbeSessionAccept(); + probe->proxyPort = &port; + probe->proxy_protocol_ipmap = &HttpConfig::m_master.config_proxy_protocol_ip_addrs; + + if (port.m_session_protocol_preference.intersects(HTTP_PROTOCOL_SET)) { + http = new HttpSessionAccept(accept_opt); + probe->registerEndpoint(ProtocolProbeSessionAccept::ProtoGroupKey::HTTP, http); + } - if (port.m_session_protocol_preference.intersects(HTTP2_PROTOCOL_SET)) { - probe->registerEndpoint(ProtocolProbeSessionAccept::ProtoGroupKey::HTTP2, new Http2SessionAccept(accept_opt)); + if (port.m_session_protocol_preference.intersects(HTTP2_PROTOCOL_SET)) { + probe->registerEndpoint(ProtocolProbeSessionAccept::ProtoGroupKey::HTTP2, new Http2SessionAccept(accept_opt)); + } } + ProtocolSessionCreateMap.insert({TS_ALPN_PROTOCOL_INDEX_HTTP_1_0, create_h1_server_session}); ProtocolSessionCreateMap.insert({TS_ALPN_PROTOCOL_INDEX_HTTP_1_1, create_h1_server_session}); ProtocolSessionCreateMap.insert({TS_ALPN_PROTOCOL_INDEX_HTTP_2_0, create_h2_server_session}); diff --git a/src/proxy/http3/Http3Frame.cc b/src/proxy/http3/Http3Frame.cc index 2c15ee03ef3..54cd6355699 100644 --- a/src/proxy/http3/Http3Frame.cc +++ b/src/proxy/http3/Http3Frame.cc @@ -599,8 +599,7 @@ Http3FrameFactory::create_headers_frame(IOBufferReader *header_block_reader, siz { ats_unique_buf buf = ats_unique_malloc(header_block_len); - int64_t nread; - while ((nread = header_block_reader->read(buf.get(), header_block_len)) > 0) { + while (header_block_reader->read(buf.get(), header_block_len) > 0) { ; } diff --git a/src/proxy/http3/Http3Session.cc b/src/proxy/http3/Http3Session.cc index 1c92f031b21..ed62c55dd2e 100644 --- a/src/proxy/http3/Http3Session.cc +++ b/src/proxy/http3/Http3Session.cc @@ -65,11 +65,12 @@ HQSession::remove_transaction(HQTransaction *trans) void HQSession::_close_transactions() { - while (this->_transaction_list.head != nullptr) { - auto *transaction = this->_transaction_list.head; + for (auto *transaction = this->_transaction_list.head; transaction != nullptr;) { + auto *next = static_cast(transaction->link.next); transaction->do_io_close(); delete transaction; + transaction = next; } } diff --git a/src/proxy/http3/QPACK.cc b/src/proxy/http3/QPACK.cc index 45da0384942..38aa3a5630f 100644 --- a/src/proxy/http3/QPACK.cc +++ b/src/proxy/http3/QPACK.cc @@ -291,11 +291,13 @@ QPACK::decode(uint64_t stream_id, const uint8_t *header_block, size_t header_blo if (largest_reference != 0 && (this->_dynamic_table.is_empty() || this->_dynamic_table.largest_index() < largest_reference)) { // Blocked - if (this->_add_to_blocked_list( - new DecodeRequest(largest_reference, thread, cont, stream_id, header_block, header_block_len, hdr))) { + auto *decode_request = new DecodeRequest(largest_reference, thread, cont, stream_id, header_block, header_block_len, hdr); + + if (this->_add_to_blocked_list(decode_request)) { return 1; } else { // Number of blocked streams exceed the limit + delete decode_request; return -2; } } diff --git a/src/proxy/logging/LogAccess.cc b/src/proxy/logging/LogAccess.cc index 33d2f2f06c5..4471d11af8f 100644 --- a/src/proxy/logging/LogAccess.cc +++ b/src/proxy/logging/LogAccess.cc @@ -1627,7 +1627,7 @@ LogAccess::marshal_version_string(char *buf) int LogAccess::marshal_proxy_protocol_version(char *buf) { - const char *version_str = "-"; + const char *version_str; switch (m_data->get_pp_version()) { case 1: version_str = "V1"; diff --git a/src/proxy/logging/LogFieldFallback.cc b/src/proxy/logging/LogFieldFallback.cc index 876ac722060..9a98574e122 100644 --- a/src/proxy/logging/LogFieldFallback.cc +++ b/src/proxy/logging/LogFieldFallback.cc @@ -83,13 +83,13 @@ constexpr bool test_find_field_fallback_separator() { static_assert(find_field_fallback_separator("") == nullptr); - constexpr char const *text1 = "{field}??default"; + [[maybe_unused]] constexpr char text1[] = "{field}??default"; static_assert(find_field_fallback_separator(text1) == text1 + 7); - constexpr char const *text2 = "??default"; + [[maybe_unused]] constexpr char text2[] = "??default"; static_assert(find_field_fallback_separator(text2) == text2); - constexpr char const *text3 = "{field}??def??ault"; + [[maybe_unused]] constexpr char text3[] = "{field}??def??ault"; static_assert(find_field_fallback_separator(text3) == text3 + 7); - constexpr char const *text4 = "{field}??\"def??ault\""; + [[maybe_unused]] constexpr char text4[] = "{field}??\"def??ault\""; static_assert(find_field_fallback_separator(text4) == text4 + 7); return true; } From 9d8d8058ec08888fcd6a76b887f89df23cea62d0 Mon Sep 17 00:00:00 2001 From: Brian Neradt Date: Thu, 27 Aug 2026 18:05:37 -0500 Subject: [PATCH 04/11] Fix client certificate context updates (#13576) TSSslClientCertUpdate has been unable to find normally configured outbound client contexts since 7dbb6cb188 changed the lookup key from certificate-and-key paths to the resolved certificate path. The existing AuTest hid the regression because its lowercase Streams.all assignments did not register assertions. This patch updates every matching CA bucket using the stored certificate path, drops the cached certificate data so that contexts created later do not resurrect the pre-update PEM, preserves working contexts when a replacement cannot be built, and releases the SSL configuration after use. It also corrects the API documentation and strengthens the AuTest to verify every CA bucket and the expected certificate subjects. Fixes: #13575 (cherry picked from commit a2011c2fc4a110f9152aa2009e909e913cd239ea) --- .../functions/TSSslClientCertUpdate.en.rst | 11 +- .../api/functions/TSSslClientContext.en.rst | 4 +- src/api/InkAPI.cc | 132 ++++++++++++------ src/iocore/net/P_SSLConfig.h | 2 +- src/iocore/net/P_SSLSecret.h | 7 + src/iocore/net/SSLClientUtils.cc | 2 +- src/iocore/net/SSLSecret.cc | 10 ++ .../cert_update/cert_update.test.py | 75 +++++++++- .../cert_update/gold/client-cert-after.gold | 1 - .../cert_update/gold/client-cert-pre.gold | 1 - .../pluginTest/cert_update/gold/update.gold | 3 - 11 files changed, 188 insertions(+), 60 deletions(-) delete mode 100644 tests/gold_tests/pluginTest/cert_update/gold/client-cert-after.gold delete mode 100644 tests/gold_tests/pluginTest/cert_update/gold/client-cert-pre.gold delete mode 100644 tests/gold_tests/pluginTest/cert_update/gold/update.gold diff --git a/doc/developer-guide/api/functions/TSSslClientCertUpdate.en.rst b/doc/developer-guide/api/functions/TSSslClientCertUpdate.en.rst index f081acda953..66a12078d8a 100644 --- a/doc/developer-guide/api/functions/TSSslClientCertUpdate.en.rst +++ b/doc/developer-guide/api/functions/TSSslClientCertUpdate.en.rst @@ -35,6 +35,11 @@ Description =========== :func:`TSSslClientCertUpdate` updates existing client certificates configured in :file:`sni.yaml` or -`proxy.config.ssl.client.cert.filename`. :arg:`cert_path` should be exact match as provided in -configurations. :func:`TSSslClientCertUpdate` returns :enumerator:`TS_SUCCESS` only if :arg:`cert_path` exists -in configuration and reloaded to update the context. +`proxy.config.ssl.client.cert.filename`. :arg:`cert_path` must match the resolved certificate path used by +Traffic Server. Relative certificate names in the configuration are resolved against +`proxy.config.ssl.client.cert.path`. :func:`TSSslClientCertUpdate` returns :enumerator:`TS_SUCCESS` only if +:arg:`cert_path` exists in the configuration and is reloaded into every matching context. + +Any certificate data cached for :arg:`cert_path` and :arg:`key_path` is discarded as well, so client +contexts that Traffic Server creates after the update also use the new certificate rather than the +previously cached one. diff --git a/doc/developer-guide/api/functions/TSSslClientContext.en.rst b/doc/developer-guide/api/functions/TSSslClientContext.en.rst index 9f685b2d486..e427734e0ca 100644 --- a/doc/developer-guide/api/functions/TSSslClientContext.en.rst +++ b/doc/developer-guide/api/functions/TSSslClientContext.en.rst @@ -37,8 +37,8 @@ Description These functions are used to explore the client contexts that |TS| uses to connect to upstreams. :func:`TSSslClientContextsNamesGet` can be used to retrieve the entire client context mappings. Note -that in |TS|, client contexts are stored in a 2-level mapping with ca paths and cert/key -paths as keys. Hence every 2 null-terminated string in :arg:`result` can be used to lookup one context. +that in |TS|, client contexts are stored in a 2-level mapping with CA paths and the resolved certificate +path as keys. Hence every 2 null-terminated string in :arg:`result` can be used to lookup one context. :arg:`result` points to an user allocated array that will hold pointers to lookup key strings and :arg:`n` is the size for :arg:`result` array. :arg:`actual`, if valid, will be filled with actual number of lookup keys (2 for each context). diff --git a/src/api/InkAPI.cc b/src/api/InkAPI.cc index 158c06fa72b..5914d96313a 100644 --- a/src/api/InkAPI.cc +++ b/src/api/InkAPI.cc @@ -28,6 +28,7 @@ #include #include #include +#include #include "iocore/net/NetVConnection.h" #include "iocore/net/NetHandler.h" @@ -8240,59 +8241,106 @@ TSSslClientCertUpdate(const char *cert_path, const char *key_path) return TS_ERROR; } - std::string key; - shared_SSL_CTX client_ctx = nullptr; - SSLConfigParams *params = SSLConfig::acquire(); + // --- Pin the active SSL configuration --- + // + // Keep this configuration generation alive across every early return and + // release it automatically when the update finishes. + std::string key{cert_path}; + SSLConfig::scoped_config params; - // Generate second level key for client context lookup - swoc::bwprint(key, "{}:{}", cert_path, key_path); + // The client context map is keyed by the resolved certificate path. Dbg(dbg_ctl_ssl_cert_update, "TSSslClientCertUpdate(): Use %.*s as key for lookup", static_cast(key.size()), key.data()); - if (nullptr != params) { - // Try to update client contexts maps - auto &ca_paths_map = params->top_level_ctx_map; - auto &map_lock = params->ctxMapLock; - std::string ca_paths_key; - // First try to locate the client context and its CA path (by top level) - ink_mutex_acquire(&map_lock); - for (auto &ca_paths_pair : ca_paths_map) { - auto &ctx_map = ca_paths_pair.second; - auto iter = ctx_map.find(key); - if (iter != ctx_map.end() && iter->second != nullptr) { - ca_paths_key = ca_paths_pair.first; - break; - } + if (!params) { + return TS_ERROR; + } + + auto &ca_paths_map = params->top_level_ctx_map; + auto &map_lock = params->ctxMapLock; + std::vector ca_paths_keys; + + // --- Find every matching CA bucket --- + // + // A certificate can be used with more than one CA configuration. Snapshot + // all matching bucket keys while holding the map lock, then release it + // before performing the expensive context construction. + ink_mutex_acquire(&map_lock); + for (auto const &[ca_paths_key, ctx_map] : ca_paths_map) { + if (ctx_map.contains(key)) { + ca_paths_keys.push_back(ca_paths_key); } - ink_mutex_release(&map_lock); + } + ink_mutex_release(&map_lock); + + if (ca_paths_keys.empty()) { + return TS_ERROR; + } + + // --- Drop the cached certificate data --- + // + // getCTX() builds contexts from the cached secret data rather than from the + // files. Drop the cached copies so that a context built later for a CA + // bucket that does not exist yet also picks up the updated certificate + // instead of the pre-update PEM. + params->secrets.invalidateSecret(key); + if (key_path != nullptr && key_path[0] != '\0') { + params->secrets.invalidateSecret(key_path); + } - // Only update on existing - if (ca_paths_key.empty()) { + std::vector> client_contexts; + + // --- Build every replacement context --- + // + // Build all replacements before changing the live map. If any construction + // fails, the existing working contexts remain installed. + client_contexts.reserve(ca_paths_keys.size()); + for (auto const &ca_paths_key : ca_paths_keys) { + size_t sep = ca_paths_key.find(':'); + std::string ca_bundle_file = ca_paths_key.substr(0, sep); + std::string ca_bundle_path = ca_paths_key.substr(sep + 1); + shared_SSL_CTX client_ctx(SSLCreateClientContext(params, ca_bundle_file.empty() ? nullptr : ca_bundle_file.c_str(), + ca_bundle_path.empty() ? nullptr : ca_bundle_path.c_str(), cert_path, + key_path), + SSL_CTX_free); + + if (!client_ctx) { return TS_ERROR; } + client_contexts.emplace_back(ca_paths_key, std::move(client_ctx)); + } - // Extract CA related paths - size_t sep = ca_paths_key.find(':'); - std::string ca_bundle_file = ca_paths_key.substr(0, sep); - std::string ca_bundle_path = ca_paths_key.substr(sep + 1); - - // Build new client context - client_ctx = - shared_SSL_CTX(SSLCreateClientContext(params, ca_bundle_path.empty() ? nullptr : ca_bundle_path.c_str(), - ca_bundle_file.empty() ? nullptr : ca_bundle_file.c_str(), cert_path, key_path), - SSL_CTX_free); - - // Successfully generates a client context, update in the map - ink_mutex_acquire(&map_lock); - auto iter = ca_paths_map.find(ca_paths_key); - if (iter != ca_paths_map.end() && iter->second.count(key)) { - iter->second[key] = client_ctx; - } else { - client_ctx = nullptr; + std::vector targets; + + // --- Install all replacement contexts --- + // + // Reacquire the map lock and locate every target before overwriting any of + // them, so that the live map is either updated completely or left untouched. + targets.reserve(client_contexts.size()); + ink_mutex_acquire(&map_lock); + for (auto const &client_context : client_contexts) { + auto ca_iter = ca_paths_map.find(client_context.first); + + if (ca_iter == ca_paths_map.end()) { + break; + } + auto ctx_iter = ca_iter->second.find(key); + + if (ctx_iter == ca_iter->second.end()) { + break; + } + targets.push_back(&ctx_iter->second); + } + + bool const updated_all = targets.size() == client_contexts.size(); + + if (updated_all) { + for (std::size_t i = 0; i < targets.size(); ++i) { + *targets[i] = std::move(client_contexts[i].second); } - ink_mutex_release(&map_lock); } + ink_mutex_release(&map_lock); - return client_ctx ? TS_SUCCESS : TS_ERROR; + return updated_all ? TS_SUCCESS : TS_ERROR; } TSReturnCode diff --git a/src/iocore/net/P_SSLConfig.h b/src/iocore/net/P_SSLConfig.h index 48dc665d3e2..08dafff0d25 100644 --- a/src/iocore/net/P_SSLConfig.h +++ b/src/iocore/net/P_SSLConfig.h @@ -152,7 +152,7 @@ struct SSLConfigParams : public ConfigInfo { // Client contexts are held by 2-level map: // The first level maps from CA bundle file&path to next level map; - // The second level maps from cert&key to actual SSL_CTX; + // The second level maps from the resolved certificate path to the actual SSL_CTX; // The second level map owns the client SSL_CTX objects and is responsible for cleaning them up using CTX_MAP = std::unordered_map; mutable std::unordered_map top_level_ctx_map; diff --git a/src/iocore/net/P_SSLSecret.h b/src/iocore/net/P_SSLSecret.h index 292f99ca7a7..d6b28c7e912 100644 --- a/src/iocore/net/P_SSLSecret.h +++ b/src/iocore/net/P_SSLSecret.h @@ -34,6 +34,13 @@ class SSLSecret void setSecret(const std::string &name, std::string_view data); void getOrLoadSecret(const std::string &name1, const std::string &name2, std::string &data, std::string &data2); + /** Drop any cached data for @a name. + * + * The next getOrLoadSecret() for @a name reloads the data, either from a + * TS_LIFECYCLE_SSL_SECRET_HOOK plugin or from the file itself. + */ + void invalidateSecret(const std::string &name); + private: void loadSecret(const std::string &name1, const std::string &name2, std::string &data_item, std::string &data_item2); std::string loadFile(const std::string &name); diff --git a/src/iocore/net/SSLClientUtils.cc b/src/iocore/net/SSLClientUtils.cc index a91b86c5cfd..0087188ead1 100644 --- a/src/iocore/net/SSLClientUtils.cc +++ b/src/iocore/net/SSLClientUtils.cc @@ -338,7 +338,7 @@ SSLInitClientContext(const SSLConfigParams *params) } SSL_CTX * -SSLCreateClientContext(const struct SSLConfigParams *params, const char *ca_bundle_path, const char *ca_bundle_file, +SSLCreateClientContext(const struct SSLConfigParams *params, const char *ca_bundle_file, const char *ca_bundle_path, const char *cert_path, const char *key_path) { std::unique_ptr ctx(nullptr, &SSL_CTX_free); diff --git a/src/iocore/net/SSLSecret.cc b/src/iocore/net/SSLSecret.cc index fd8204a1c47..7249681aa28 100644 --- a/src/iocore/net/SSLSecret.cc +++ b/src/iocore/net/SSLSecret.cc @@ -119,6 +119,16 @@ SSLSecret::setSecret(const std::string &name, std::string_view data) Dbg(dbg_ctl_ssl_secret, "Set secret for %s to %.*s", name.c_str(), int(data.size() > 50 ? 50 : data.size()), data.data()); } +void +SSLSecret::invalidateSecret(const std::string &name) +{ + std::scoped_lock lock(secret_map_mutex); + + if (secret_map.erase(name) > 0) { + Dbg(dbg_ctl_ssl_secret, "Invalidated cached secret for %s", name.c_str()); + } +} + std::string SSLSecret::getSecret(const std::string &name) const { diff --git a/tests/gold_tests/pluginTest/cert_update/cert_update.test.py b/tests/gold_tests/pluginTest/cert_update/cert_update.test.py index 3d2766a4add..0191bc77336 100644 --- a/tests/gold_tests/pluginTest/cert_update/cert_update.test.py +++ b/tests/gold_tests/pluginTest/cert_update/cert_update.test.py @@ -26,7 +26,7 @@ Test.SkipIf(Condition.CurlUsingUnixDomainSocket()) Test.SkipUnless( Condition.HasProgram("openssl", "Openssl need to be installed on system for this test to work"), - Condition.PluginExists('cert_update.so')) + Condition.PluginExists('cert_update.so'), Condition.PluginExists('conf_remap.so')) # Set up origin server server = Test.MakeOriginServer("server") @@ -54,6 +54,8 @@ 'proxy.config.ssl.server.private_key.path': '{0}'.format(ts.Variables.SSLDir), 'proxy.config.ssl.client.cert.path': '{0}'.format(ts.Variables.SSLDir), 'proxy.config.ssl.client.private_key.path': '{0}'.format(ts.Variables.SSLDir), + 'proxy.config.ssl.client.CA.cert.path': '{0}'.format(ts.Variables.SSLDir), + 'proxy.config.ssl.client.verify.server.policy': 'PERMISSIVE', 'proxy.config.url_remap.pristine_host_hdr': 1 }) @@ -62,6 +64,14 @@ ts.Disk.remap_config.AddLines( [ 'map https://bar.com http://127.0.0.1:{0}'.format(server.Variables.Port), + 'map https://foo.com/override-ca https://127.0.0.1:{0} @plugin=conf_remap.so ' + '@pparam=proxy.config.ssl.client.cert.filename=client1.pem ' + '@pparam=proxy.config.ssl.client.CA.cert.filename=server1.pem'.format(ts.Variables.s_server_port), + # This CA configuration is only used after the certificate is updated so + # that its client context is created from scratch post-update. + 'map https://foo.com/late-ca https://127.0.0.1:{0} @plugin=conf_remap.so ' + '@pparam=proxy.config.ssl.client.cert.filename=client1.pem ' + '@pparam=proxy.config.ssl.client.CA.cert.filename=server2.pem'.format(ts.Variables.s_server_port), 'map https://foo.com https://127.0.0.1:{0}'.format(ts.Variables.s_server_port), ]) @@ -90,7 +100,8 @@ tr.Processes.Default.Env = ts.Env tr.Processes.Default.Command = ( '{0}/traffic_ctl plugin msg cert_update.server {1}/server2.pem'.format(ts.Variables.BINDIR, ts.Variables.SSLDir)) -ts.Disk.traffic_out.Content = "gold/update.gold" +ts.Disk.traffic_out.Content += Testers.ContainsExpression( + "Successfully updated server cert", "The server certificate context should be updated") ts.StillRunningAfter = server # Server-Cert-After @@ -104,7 +115,7 @@ ts.StillRunningAfter = server # Client-Cert-Pre -# s_server should see client (Traffic Server) as alice.com +# s_server should see client (Traffic Server) as alice.com with the default CA configuration. tr = Test.AddTestRun("Client-Cert-Pre") s_server = tr.Processes.Process( "s_server", "openssl s_server -www -key {0}/server1.pem -cert {0}/server1.pem -accept {1} -Verify 1 -msg".format( @@ -112,7 +123,23 @@ s_server.Ready = When.PortReady(ts.Variables.s_server_port) tr.MakeCurlCommand('--verbose --insecure --ipv4 --header "Host: foo.com" https://localhost:{}'.format(ts.Variables.ssl_port), ts=ts) tr.Processes.Default.StartBefore(s_server) -s_server.Streams.all = "gold/client-cert-pre.gold" +s_server.Streams.All = Testers.ContainsExpression( + "alice.com", "The default CA context should initially use the original client certificate") +tr.Processes.Default.ReturnCode = 0 +ts.StillRunningAfter = server + +# Client-Cert-Pre-CA-Override +# s_server should also see alice.com with the overridden CA configuration. +tr = Test.AddTestRun("Client-Cert-Pre-CA-Override") +s_server = tr.Processes.Process( + "s_server", "openssl s_server -www -key {0}/server1.pem -cert {0}/server1.pem -accept {1} -Verify 1 -msg".format( + ts.Variables.SSLDir, ts.Variables.s_server_port)) +s_server.Ready = When.PortReady(ts.Variables.s_server_port) +tr.MakeCurlCommand( + '--verbose --insecure --ipv4 --header "Host: foo.com" https://localhost:{}/override-ca'.format(ts.Variables.ssl_port), ts=ts) +tr.Processes.Default.StartBefore(s_server) +s_server.Streams.All = Testers.ContainsExpression( + "alice.com", "The CA override context should initially use the original client certificate") tr.Processes.Default.ReturnCode = 0 ts.StillRunningAfter = server @@ -122,7 +149,10 @@ tr.Processes.Default.Command = ( 'mv {0}/client2.pem {0}/client1.pem && {1}/traffic_ctl plugin msg cert_update.client {0}/client1.pem'.format( ts.Variables.SSLDir, ts.Variables.BINDIR)) -ts.Disk.traffic_out.Content = "gold/update.gold" +ts.Disk.traffic_out.Content += Testers.ContainsExpression( + "Successfully updated client cert", "The client certificate context should be updated") +ts.Disk.traffic_out.Content += Testers.ExcludesExpression( + "Failed to update client cert", "The client certificate context update should not fail") ts.StillRunningAfter = server # Client-Cert-After @@ -137,6 +167,39 @@ tr.MakeCurlCommand( '--verbose --insecure --ipv4 --header "Host: foo.com" https://localhost:{0}'.format(ts.Variables.ssl_port), ts=ts) tr.Processes.Default.StartBefore(s_server) -s_server.Streams.all = "gold/client-cert-after.gold" +s_server.Streams.All = Testers.ContainsExpression( + "bob.com", "The next outbound connection should use the replacement client certificate") +tr.Processes.Default.ReturnCode = 0 +ts.StillRunningAfter = server + +# Verify that the context under the overridden CA configuration was also updated. +tr = Test.AddTestRun("Client-Cert-After-CA-Override") +s_server = tr.Processes.Process( + "s_server", "openssl s_server -www -key {0}/server1.pem -cert {0}/server1.pem -accept {1} -Verify 1 -msg".format( + ts.Variables.SSLDir, ts.Variables.s_server_port)) +s_server.Ready = When.PortReady(ts.Variables.s_server_port) +tr.Processes.Default.Env = ts.Env +tr.MakeCurlCommand( + '--verbose --insecure --ipv4 --header "Host: foo.com" https://localhost:{0}/override-ca'.format(ts.Variables.ssl_port), ts=ts) +tr.Processes.Default.StartBefore(s_server) +s_server.Streams.All = Testers.ContainsExpression("bob.com", "The client certificate should be updated for every CA configuration") +tr.Processes.Default.ReturnCode = 0 +ts.StillRunningAfter = server + +# Client-Cert-After-Late-CA +# The /late-ca mapping has not been used yet, so its client context is built +# after the update. It must be built from the new certificate file rather than +# from the certificate data cached before the update. +tr = Test.AddTestRun("Client-Cert-After-Late-CA") +s_server = tr.Processes.Process( + "s_server", "openssl s_server -www -key {0}/server1.pem -cert {0}/server1.pem -accept {1} -Verify 1 -msg".format( + ts.Variables.SSLDir, ts.Variables.s_server_port)) +s_server.Ready = When.PortReady(ts.Variables.s_server_port) +tr.Processes.Default.Env = ts.Env +tr.MakeCurlCommand( + '--verbose --insecure --ipv4 --header "Host: foo.com" https://localhost:{0}/late-ca'.format(ts.Variables.ssl_port), ts=ts) +tr.Processes.Default.StartBefore(s_server) +s_server.Streams.All = Testers.ContainsExpression( + "bob.com", "A context created after the update should not use the cached pre-update certificate") tr.Processes.Default.ReturnCode = 0 ts.StillRunningAfter = server diff --git a/tests/gold_tests/pluginTest/cert_update/gold/client-cert-after.gold b/tests/gold_tests/pluginTest/cert_update/gold/client-cert-after.gold deleted file mode 100644 index fef60f68d27..00000000000 --- a/tests/gold_tests/pluginTest/cert_update/gold/client-cert-after.gold +++ /dev/null @@ -1 +0,0 @@ -``bob.com`` \ No newline at end of file diff --git a/tests/gold_tests/pluginTest/cert_update/gold/client-cert-pre.gold b/tests/gold_tests/pluginTest/cert_update/gold/client-cert-pre.gold deleted file mode 100644 index 6a94425920f..00000000000 --- a/tests/gold_tests/pluginTest/cert_update/gold/client-cert-pre.gold +++ /dev/null @@ -1 +0,0 @@ -``alice.com`` \ No newline at end of file diff --git a/tests/gold_tests/pluginTest/cert_update/gold/update.gold b/tests/gold_tests/pluginTest/cert_update/gold/update.gold deleted file mode 100644 index 4160bb7dbf7..00000000000 --- a/tests/gold_tests/pluginTest/cert_update/gold/update.gold +++ /dev/null @@ -1,3 +0,0 @@ -`` -``Successfully updated`` -`` \ No newline at end of file From 434e09f4879537e74019dcb300a12f32b69a5e6e Mon Sep 17 00:00:00 2001 From: Masaori Koshiba Date: Mon, 31 Aug 2026 09:02:41 +0900 Subject: [PATCH 05/11] Remove dead JSONRPC server_shutdown handler (#13578) Never registered nor called since it was added in #7478, and its void(YAML::Node) signature never matched the method handler shape it sat among. Shutdown already syncs the cache dir from the event thread in traffic_server.cc; doing it from an RPC thread would race the dir writers. Dropping it also removes mgmt's relative-path include of iocore's private P_CacheDir.h. (cherry picked from commit 2b4988ef3e83e3604cfa981b536871630317a8ef) --- include/mgmt/rpc/handlers/server/Server.h | 1 - src/mgmt/rpc/handlers/server/Server.cc | 7 ------- 2 files changed, 8 deletions(-) diff --git a/include/mgmt/rpc/handlers/server/Server.h b/include/mgmt/rpc/handlers/server/Server.h index fec31c66bfe..40e9b2ca059 100644 --- a/include/mgmt/rpc/handlers/server/Server.h +++ b/include/mgmt/rpc/handlers/server/Server.h @@ -26,7 +26,6 @@ namespace rpc::handlers::server { swoc::Rv server_start_drain(std::string_view const &id, YAML::Node const ¶ms); swoc::Rv server_stop_drain(std::string_view const &id, YAML::Node const &); -void server_shutdown(YAML::Node const &); swoc::Rv get_server_status(std::string_view const &id, YAML::Node const &); swoc::Rv get_connection_tracker_info(std::string_view const &id, YAML::Node const ¶ms); diff --git a/src/mgmt/rpc/handlers/server/Server.cc b/src/mgmt/rpc/handlers/server/Server.cc index f429d0787cc..c5e440d4b54 100644 --- a/src/mgmt/rpc/handlers/server/Server.cc +++ b/src/mgmt/rpc/handlers/server/Server.cc @@ -18,7 +18,6 @@ limitations under the License. */ -#include "../../../../iocore/cache/P_CacheDir.h" #include "iocore/eventsystem/EventProcessor.h" #include "iocore/net/ConnectionTracker.h" #include "mgmt/rpc/handlers/server/Server.h" @@ -176,12 +175,6 @@ server_stop_drain(std::string_view const & /* id ATS_UNUSED */, YAML::Node const return resp; } -void -server_shutdown(YAML::Node const &) -{ - sync_cache_dir_on_shutdown(); -} - swoc::Rv get_server_status(std::string_view const & /* params ATS_UNUSED */, YAML::Node const & /* params ATS_UNUSED */) { From 84f4573807783e5f3beb051cca41f2bac289f32d Mon Sep 17 00:00:00 2001 From: Sasank P <143941985+AceMeistr@users.noreply.github.com> Date: Tue, 1 Sep 2026 04:28:28 +0530 Subject: [PATCH 06/11] Fix data race in SSLCertContext copy & assignment (#13227) Resolve concurrent read/write data races by using std::scoped_lock in operator= and locking other.ctx_mutex at the start of the copy constructor. (cherry picked from commit 6106f6b57f466df2cc00fbb71d1621b0ce2ff615) --- src/iocore/net/SSLCertLookup.cc | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/iocore/net/SSLCertLookup.cc b/src/iocore/net/SSLCertLookup.cc index bf33d0294f6..fdc0f9fdddd 100644 --- a/src/iocore/net/SSLCertLookup.cc +++ b/src/iocore/net/SSLCertLookup.cc @@ -34,6 +34,7 @@ #include "P_SSLUtils.h" #include +#include #include #include #include @@ -234,24 +235,24 @@ ssl_create_ticket_keyblock(const char *ticket_key_path) SSLCertContext::SSLCertContext(SSLCertContext const &other) { + std::shared_lock lock(other.ctx_mutex); opt = other.opt; userconfig = other.userconfig; keyblock = other.keyblock; ctx_type = other.ctx_type; - std::shared_lock lock(other.ctx_mutex); - ctx = other.ctx; + ctx = other.ctx; } SSLCertContext & SSLCertContext::operator=(SSLCertContext const &other) { if (&other != this) { + std::scoped_lock lock(this->ctx_mutex, other.ctx_mutex); this->opt = other.opt; this->userconfig = other.userconfig; this->keyblock = other.keyblock; this->ctx_type = other.ctx_type; - std::shared_lock lock(other.ctx_mutex); - this->ctx = other.ctx; + this->ctx = other.ctx; } return *this; } From 8a195b30ea027a474378974edfc9d56c29ff3130 Mon Sep 17 00:00:00 2001 From: Brian Neradt Date: Thu, 3 Sep 2026 20:43:03 -0500 Subject: [PATCH 07/11] Proxy Verifier v3.2.0 (#13642) This includes a body delay feature Masaori worked on. See: https://github.com/yahoo/proxy-verifier#content-delay-specification (cherry picked from commit 0265a523cb1f83a5a1af58e7eebee90961e176a8) --- tests/proxy-verifier-checksum.txt | 2 +- tests/proxy-verifier-version.txt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/proxy-verifier-checksum.txt b/tests/proxy-verifier-checksum.txt index 9c2ec8528be..90b31901ff1 100644 --- a/tests/proxy-verifier-checksum.txt +++ b/tests/proxy-verifier-checksum.txt @@ -1 +1 @@ -342286244d441329c12de1122520da6c86c581fd +5485a3cea4359e86458bf6d369dc18cdcfbd433d diff --git a/tests/proxy-verifier-version.txt b/tests/proxy-verifier-version.txt index 66cfae52b28..6d260c3af09 100644 --- a/tests/proxy-verifier-version.txt +++ b/tests/proxy-verifier-version.txt @@ -1 +1 @@ -v3.1.3 +v3.2.0 From b880bea1ac7156ed0f1d2967b07591d66622e41d Mon Sep 17 00:00:00 2001 From: Javid Khan Date: Fri, 4 Sep 2026 07:43:05 +0530 Subject: [PATCH 08/11] dns: fix remaining-space tracking when copying address records (#13558) When decoding a DNS response, the path that copies an unaligned A or AAAA record into the host entry buffer moved the write position without updating the count of space remaining, so the two disagreed for the rest of the response. Update the count after the copy, the same way the name, CNAME and PTR paths already do. (cherry picked from commit 80e89e5130bb4adaea2624c527284dd803e3b0a3) --- src/iocore/dns/DNS.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/iocore/dns/DNS.cc b/src/iocore/dns/DNS.cc index 4c6cecdcca3..16e33e95f1f 100644 --- a/src/iocore/dns/DNS.cc +++ b/src/iocore/dns/DNS.cc @@ -1889,8 +1889,9 @@ dns_process(DNSHandler *handler, HostEnt *buf, int len) memcpy((*hap++ = bp), cp, n); Dbg(dbg_ctl_dns, "received %s = %s", QtypeName(type), inet_ntop(T_AAAA == type ? AF_INET6 : AF_INET, bp, ip_string, sizeof(ip_string))); - bp += n; - cp += n; + bp += n; + cp += n; + buflen = sizeof(buf->hostbuf) - (bp - buf->hostbuf); } } else { goto Lerror; From 028568e45a3c370467db26873e2830119f01daf0 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Mon, 7 Sep 2026 09:45:28 +0200 Subject: [PATCH 09/11] Argparser variable arg option parsing fix/improvement. (#13570) * Stop variable-arg options consuming later options ArgParser options declared with MORE_THAN_ZERO_ARG_N or MORE_THAN_ONE_ARG_N collected every remaining token, so an option written after one of them was silently swallowed as a value and never parsed. Collection now stops at a token naming another option of the same command, "--" ends option recognition so a value can still start with '-', and only the range actually consumed is erased. Separately, the --option=value path took the name up to the first '=' but the value from the last one, truncating any value containing '='. That made --directive=key.sub=val unusable, since directive values are key=value pairs by definition. Fixes: #13569 * Drop the -D placement workaround from traffic_ctl The guard rejecting directive values that start with '-' existed only because variable-argument parsing swallowed any option written after -D. That no longer happens, so the guard can only fire for a value the caller passed deliberately, and its advice to place -D last is now wrong. A malformed value is reported by the directive format check instead. Require values for both -D and -d. Supplying either with no values built a request identical to a plain reload, silently widening a scoped reload to every handler. Also document that -D may appear anywhere among the options and can be combined with -d, which the previous note said was impossible. * Add an at-most-one-argument arity to ArgParser An option whose value is optional had to be declared as taking zero or more values, the only variable arity available, so it also consumed the positional arguments of its own command. That is why traffic_ctl rejected "config get -c FILE RECORD" with an error naming get, and why --cold only worked written last or as --cold=FILE. Add AT_MOST_ONE_ARG_N, the equivalent of nargs='?' in Python argparse which this parser imitates, and declare --cold with it. The count check for the --option=value form now asks is_variable_arg_num() rather than comparing against the sentinels, so a third sentinel is not mistaken for a literal argument count. * Correct the variable-arg comment in handle_args The comment claimed the command's positional arguments were left in place, but collection only stops at a token naming another option, so positional tokens are still taken as values. Say so, and point at AT_MOST_ONE_ARG_N for an option whose value is optional. * Stop fixed-arity options consuming later options An option expecting a fixed number of values took whatever token followed it, so "traffic_ctl server debug enable -t -a" set the debug tags to the literal "-a" and wrote that to the running configuration. Apply the rule the variable-arity path already follows: a token naming another option of the same command is not a value, so the missing value is reported, and "--" still passes a value that starts with '-'. * Reject a repeated at-most-one-argument option is_variable_arg_num() exempts AT_MOST_ONE_ARG_N from the count check for the --option=value form, so --cold=a --cold=b silently kept the first and dropped the second, where the fixed arity equivalent is a usage error. * Correct the bare -c example in the traffic_ctl docs A bare -c before a record takes the record as the file name, which leaves config get with no records and exits with a usage error rather than reading a file named for the record. * Count both --cold spellings against the at-most-one limit The repetition check only counted the --option=value form, so "-c a -c b" silently kept the last file name, and mixing the two spellings put two values in an option that permits one. Fixed arity options keep their existing last-one-wins behaviour, which is a separate concern. * Say what "--" does to the options that follow it Option recognition stays off for the rest of a variable-length value list, so every later token becomes a value and any option written afterwards is swallowed. Neither the guide nor the traffic_ctl page said so, which made "--" look safe to use before other options. * Accumulate a repeated variable argument option Each occurrence reset the entry rather than adding to it, so "-D a -D b" kept only b and "-d f1 -d f2" reloaded only f2, silently dropping inline content the documentation says is merged. The --option=value form has always accumulated, so the two spellings of one option disagreed. A fixed arity option keeps its last-one-wins behaviour. The default command retry starts from a clean Arguments, since a global option is otherwise parsed twice and would collect its values twice. * Reject an empty value for an at-most-one-argument option An empty token was taken as the value, and traffic_ctl reads an empty --cold file name as a request for the default file, so a set whose file name came from an unset variable wrote to the live records.yaml and exited zero. The --option=value spelling already refuses an empty value, so the two spellings disagreed. The error names the option as it was written, matching that spelling. * Report the set symptom in the bare -c documentation A bare -c before a record takes the record as the file name, and the docs quoted only the error config get produces. config set is left short of its own arguments and reports a different one, so an operator who hit that did not find their message. * Drop the default command test that leaked into other tests set_default() writes a file scope default_command that nothing clears, so the test left every later parse in the same binary inserting "info" into its arguments. The mutex group and option dependency tests share that binary and failed on four platforms, while the file passed on its own. The retry path the test covered keeps its guard in parse(); it cannot be exercised without changing global state for the rest of the run. (cherry picked from commit d0fb283406931e502ec0737e5af8ddf0dcc71c57) --- .../command-line/traffic_ctl.en.rst | 67 ++++- .../internal-libraries/ArgParser.en.rst | 42 +++ include/tscore/ArgParser.h | 23 ++ src/traffic_ctl/CtrlCommands.cc | 44 ++- src/traffic_ctl/traffic_ctl.cc | 4 +- src/tscore/ArgParser.cc | 144 ++++++++-- src/tscore/unit_tests/test_ArgParser.cc | 268 ++++++++++++++++++ .../config_reload_directive_cli.test.py | 195 +++++++++++++ .../records/traffic_ctl_cold_config.test.py | 103 +++++++ .../traffic_ctl_server_debug.test.py | 18 ++ 10 files changed, 874 insertions(+), 34 deletions(-) create mode 100644 tests/gold_tests/jsonrpc/config_reload_directive_cli.test.py diff --git a/doc/appendices/command-line/traffic_ctl.en.rst b/doc/appendices/command-line/traffic_ctl.en.rst index 59dccd05b30..64276e05c6a 100644 --- a/doc/appendices/command-line/traffic_ctl.en.rst +++ b/doc/appendices/command-line/traffic_ctl.en.rst @@ -429,7 +429,8 @@ Display the current value of a configuration record. - ``directive_key`` — the directive name understood by that handler - ``value`` — the directive value (always passed as a string on the wire) - Multiple directives are passed as space-separated values after a single ``-D``: + Multiple directives are passed as space-separated values after a single ``-D``, or by + repeating the option. Both spellings accumulate, and they may be mixed: .. code-block:: bash @@ -442,6 +443,12 @@ Display the current value of a configuration record. # Directives for different handlers in the same reload $ traffic_ctl config reload -D myconfig.id=foo sni.fqdn=example.com + # The same, written as a repeated option + $ traffic_ctl config reload -D myconfig.id=foo -D sni.fqdn=example.com + + # Repeating it is how a directive is written after another option + $ traffic_ctl config reload -D myconfig.id=foo --monitor -D sni.fqdn=example.com + On the wire, ``-D myconfig.id=foo`` translates to: .. code-block:: json @@ -456,11 +463,23 @@ Display the current value of a configuration record. .. note:: - ``-D`` uses variable-argument parsing and must appear as the **last option** - on the command line. Any flags placed after ``-D`` will be consumed as directive - values. ``-D`` and ``-d`` cannot be combined in the same invocation due to this - same constraint. Use ``-d`` with full YAML when you need both directives and - inline content in a single reload request. + ``-D`` accepts values until the next option or the end of the command line, so it + may appear anywhere among the options and can be combined with ``-d`` — directives + and inline content merge under the same config key: + + .. code-block:: bash + + $ traffic_ctl config reload -D myconfig.id=foo --monitor + $ traffic_ctl config reload -D myconfig.id=foo -d 'myconfig: {rules: [a]}' + + To pass a directive value that begins with ``-``, place ``--`` before it. Option + recognition then stays off for the rest of the line, so every remaining token becomes + a directive value and any option written afterwards is swallowed. Use + ``--directive=-value`` instead when options still have to follow: + + .. code-block:: bash + + $ traffic_ctl config reload --directive=-weird.id=foo --monitor .. note:: @@ -563,6 +582,42 @@ Display the current value of a configuration record. Specifying the file name is not needed as `traffic_ctl` will try to use the build(or the runroot if used) information to figure out the path to the `records.yaml`. + ``-c`` accepts at most one file name, so it may be written before or after the record + names: + + .. code-block:: bash + + $ traffic_ctl config get -c records.yaml proxy.config.diags.debug.enabled + $ traffic_ctl config get proxy.config.diags.debug.enabled -c records.yaml + $ traffic_ctl config get --cold=records.yaml proxy.config.diags.debug.enabled + + When no file name is given, write ``-c`` last, or use the ``--cold=`` form for the + explicit file. A bare ``-c`` followed by a record name takes the record as the file name, + which leaves the command short of its own arguments. Each command reports this in terms of + what it was left without: + + .. code-block:: bash + + $ traffic_ctl config get proxy.config.diags.debug.enabled -c # default records.yaml + $ traffic_ctl config get -c proxy.config.diags.debug.enabled + Error: at least one argument expected by get + + $ traffic_ctl config set proxy.config.diags.debug.enabled 1 -c # default records.yaml + $ traffic_ctl config set -c proxy.config.diags.debug.enabled 1 + Error: 2 argument(s) expected by set + + An empty file name is not a file name, so it is reported rather than taken as a request for + the default file. This matters when the name comes from a variable that is unset, where + reading or writing the live :file:`records.yaml` is unlikely to be what was meant: + + .. code-block:: bash + + $ traffic_ctl config set -c "" proxy.config.diags.debug.enabled 1 + Error: missing argument for '-c' + + ``-c`` is also given at most once, so repeating it is a usage error rather than the last + file name silently winning. + If the file exists and is empty a new document will be created. If a file does not exist, an attempt to create a new file will be done. This option(only for the config file changes) lets you use the prefix `proxy.config.` or `ts.` for variable names, either would work. diff --git a/doc/developer-guide/internal-libraries/ArgParser.en.rst b/doc/developer-guide/internal-libraries/ArgParser.en.rst index c15a552ff5a..cdaf5cbe4f9 100644 --- a/doc/developer-guide/internal-libraries/ArgParser.en.rst +++ b/doc/developer-guide/internal-libraries/ArgParser.en.rst @@ -104,6 +104,48 @@ To add options to the parser or current command: This function call returns the new :class:`Option` instance. (0 is also number of arguments expected) +.. Note:: + + For options, the number of arguments may also be one of the following, which mirror the + ``nargs`` values of Python's ``argparse``: + + ================================ ======================================================= + Value Meaning + ================================ ======================================================= + ``AT_MOST_ONE_ARG_N`` Zero or one value (``argparse`` ``nargs='?'``) + ``MORE_THAN_ZERO_ARG_N`` Zero or more values (``argparse`` ``nargs='*'``) + ``MORE_THAN_ONE_ARG_N`` One or more values (``argparse`` ``nargs='+'``) + ================================ ======================================================= + + An option taking a variable number of values stops collecting when it reaches a token + naming another option of the same command, so options written afterwards keep their own + arguments. Use ``AT_MOST_ONE_ARG_N`` rather than ``MORE_THAN_ZERO_ARG_N`` for an option + whose value is optional, otherwise it also consumes the positional arguments of its + command. + + A token naming another option is not a value for a fixed number of arguments either. An + option written where a value is expected leaves the value missing, which is reported as a + usage error rather than the option being consumed and applied as the value. + + Because collection stops at the following option, an option taking an unbounded number of + values may be written more than once, and the occurrences accumulate. This matches the + ``--option=value`` form, which has always appended. An option taking a fixed number of + values keeps its last-one-wins behaviour instead, and ``AT_MOST_ONE_ARG_N`` reports a + repetition as a usage error since it permits only one value in total. + + An empty token is not a value for ``AT_MOST_ONE_ARG_N``. It is reported as a missing + argument rather than read as the option having been given without one, so a value taken + from an unset variable cannot silently select the declared default. + + A ``--`` token stops option recognition for the values being collected, which is how a + value beginning with ``-`` is passed. Note this differs from the POSIX ``--``: it does + not end the value list nor force the remainder to be positional arguments. + + Option recognition stays off for the rest of that collection, so for a variable number of + values every remaining token becomes a value and no later option is recognized. Use the + ``--option=value`` form instead when options still have to follow a value that begins with + ``-``. + We can also use the following chained way to add subcommand or option: .. code-block:: cpp diff --git a/include/tscore/ArgParser.h b/include/tscore/ArgParser.h index fdb6086eba0..1b29504a83f 100644 --- a/include/tscore/ArgParser.h +++ b/include/tscore/ArgParser.h @@ -34,10 +34,23 @@ constexpr unsigned MORE_THAN_ZERO_ARG_N = ~0; // more than one arguments constexpr unsigned MORE_THAN_ONE_ARG_N = ~0 - 1; +// zero or one argument +constexpr unsigned AT_MOST_ONE_ARG_N = ~0 - 2; // customizable indent for help message constexpr int INDENT_ONE = 32; constexpr int INDENT_TWO = 46; +/** Whether @a arg_num asks for a variable rather than a fixed number of values. + + Use this in preference to comparing against the sentinels, so that adding another + variable arity does not silently leave a sentinel being treated as a literal count. + */ +constexpr bool +is_variable_arg_num(unsigned arg_num) +{ + return arg_num == MORE_THAN_ZERO_ARG_N || arg_num == MORE_THAN_ONE_ARG_N || arg_num == AT_MOST_ONE_ARG_N; +} + namespace ts { using AP_StrVec = std::vector; @@ -89,6 +102,12 @@ class Arguments ~Arguments(); ArgumentData get(std::string const &name); + /** Whether @a name has an entry. + + @return @c true when the command or option has been parsed. Unlike get(), the called + flag is left alone, so this can be asked while parsing. + */ + bool has(std::string const &name) const noexcept; void append(std::string const &key, ArgumentData const &value); // Append value to the arg to the map of key @@ -222,6 +241,10 @@ class ArgParser void version_message() const; // Helper method for parse() void append_option_data(Arguments &ret, AP_StrVec &args, int index); + // Helper method to collect the values of an option or command into @a ret + std::string handle_args(Arguments &ret, AP_StrVec &args, std::string const &name, unsigned arg_num, unsigned &index) const; + // Whether @a token names an option registered on this command + bool is_registered_option(std::string const &token) const; // Helper method to validate mutually exclusive groups void validate_mutex_groups(Arguments &ret) const; // Helper method to validate option dependencies diff --git a/src/traffic_ctl/CtrlCommands.cc b/src/traffic_ctl/CtrlCommands.cc index ed15e90bc51..9a92ec3826a 100644 --- a/src/traffic_ctl/CtrlCommands.cc +++ b/src/traffic_ctl/CtrlCommands.cc @@ -51,6 +51,16 @@ const StringToFormatFlagsMap _Fmt_str_to_enum = { {"rpc", BasePrinter::Options::FormatFlags::RPC } }; +/// `-d ""`, or `-d $VAR` with VAR unset, yields an empty token that survives +/// ArgumentData::size() and is then skipped by the parse loops, so the reload would quietly +/// cover fewer configs than the operator asked for. Reported separately from the bare option +/// so the operator is told which of the two mistakes they made. +bool +has_empty_value(ts::ArgumentData const &args) +{ + return std::any_of(args.begin(), args.end(), [](std::string const &value) { return value.empty(); }); +} + constexpr std::string_view YAML_PREFIX{"records."}; constexpr std::string_view RECORD_PREFIX{"proxy.config."}; @@ -554,6 +564,21 @@ ConfigCommand::config_reload() _printer->write_output(""); } + // Without content the request would silently degrade to a full reload of every handler, + // which is the opposite of the scoped reload the operator asked for. + if (data_args) { + if (data_args.size() == 0) { + _printer->write_output("Error: --data (-d) requires content: @file, @- or a YAML string"); + App_Exit_Status_Code = CTRL_EX_ERROR; + return; + } + if (has_empty_value(data_args)) { + _printer->write_output("Error: --data (-d) received an empty value, so its config would be left out"); + App_Exit_Status_Code = CTRL_EX_ERROR; + return; + } + } + // Parse inline config data if provided (supports multiple -d arguments) YAML::Node configs; for (auto const &data_arg : data_args) { @@ -586,17 +611,22 @@ ConfigCommand::config_reload() // Parse --directive (-D) arguments into configs[key]["_reload"][directive] = value auto dir_args = get_parsed_arguments()->get("directive"); - for (auto const &dir : dir_args) { - if (dir.empty()) { - continue; + if (dir_args) { + if (dir_args.size() == 0) { + _printer->write_output("Error: --directive (-D) requires at least one config_key.directive_key=value"); + App_Exit_Status_Code = CTRL_EX_ERROR; + return; } - if (dir[0] == '-') { - _printer->write_output("Error: '" + dir + - "' looks like a flag, not a directive. " - "Place -D as the last option on the command line."); + if (has_empty_value(dir_args)) { + _printer->write_output("Error: --directive (-D) received an empty value, so its directive would be left out"); App_Exit_Status_Code = CTRL_EX_ERROR; return; } + } + for (auto const &dir : dir_args) { + if (dir.empty()) { + continue; + } std::string err; if (!parse_directive(dir, configs, err)) { _printer->write_output("Error: " + err); diff --git a/src/traffic_ctl/traffic_ctl.cc b/src/traffic_ctl/traffic_ctl.cc index 478150d251f..286461e556a 100644 --- a/src/traffic_ctl/traffic_ctl.cc +++ b/src/traffic_ctl/traffic_ctl.cc @@ -116,7 +116,7 @@ main([[maybe_unused]] int argc, const char **argv) .add_example_usage("traffic_ctl config get [OPTIONS] RECORD [RECORD ...]") .add_option("--cold", "-c", "Save the value in a configuration file. This does not save the value in TS. Local file change only", - "TS_RECORD_YAML", MORE_THAN_ZERO_ARG_N) + "TS_RECORD_YAML", AT_MOST_ONE_ARG_N) .add_option("--records", "", "Emit output in YAML format") .add_option("--default", "", "Include default value"); config_command.add_command("match", "Get configuration matching a regular expression", "", MORE_THAN_ONE_ARG_N, Command_Execute) @@ -184,7 +184,7 @@ main([[maybe_unused]] int argc, const char **argv) config_command.add_command("set", "Set a configuration value", "", 2, Command_Execute) .add_option("--cold", "-c", "Save the value in a configuration file. This does not save the value in TS. Local file change only", - "TS_RECORD_YAML", MORE_THAN_ZERO_ARG_N) + "TS_RECORD_YAML", AT_MOST_ONE_ARG_N) .add_option("--update", "-u", "Update a configuration value. [only relevant if --cold set]") .add_option( "--type", "-t", diff --git a/src/tscore/ArgParser.cc b/src/tscore/ArgParser.cc index 90c3c2f5e43..eff0c42e9ec 100644 --- a/src/tscore/ArgParser.cc +++ b/src/tscore/ArgParser.cc @@ -195,6 +195,9 @@ ArgParser::parse(const char **argv) if (!default_command.empty()) { args = _argv; args.insert(args.begin() + 1, default_command); + // The pass that failed may have collected options before it gave up. Those values would + // now accumulate on top of the ones the retry collects rather than be replaced. + ret = Arguments{}; _top_level_command.parse(ret, args); } }; @@ -418,6 +421,8 @@ ArgParser::Command::output_option() const return {" [ ...]"}; } else if (num == MORE_THAN_ONE_ARG_N) { return {" ..."}; + } else if (num == AT_MOST_ONE_ARG_N) { + return {" []"}; } else { return " ... "; } @@ -518,34 +523,113 @@ ArgParser::Command::output_option() const } } +bool +ArgParser::Command::is_registered_option(std::string const &token) const +{ + if (_option_list.find(token) != _option_list.end() || _option_map.find(token) != _option_map.end()) { + return true; + } + // The --option=value form. + if (token.size() > 2 && token[0] == '-' && token[1] == '-') { + if (auto const pos = token.find_first_of('='); pos != std::string::npos) { + return _option_list.find(token.substr(0, pos)) != _option_list.end(); + } + } + return false; +} + // helper method to handle the arguments and put them nicely in arguments // can be switched to ts::errata -static std::string -handle_args(Arguments &ret, AP_StrVec &args, std::string const &name, unsigned arg_num, unsigned &index) +std::string +ArgParser::Command::handle_args(Arguments &ret, AP_StrVec &args, std::string const &name, unsigned arg_num, unsigned &index) const { - ArgumentData data; - ret.append(name, data); + // A repeated option taking an unbounded number of values accumulates, as the --option=value + // form always has, so an entry already written by this pass keeps the values it collected. A + // fixed arity option keeps its last-one-wins behaviour, which is a separate concern. + bool const accumulates = MORE_THAN_ZERO_ARG_N == arg_num || MORE_THAN_ONE_ARG_N == arg_num; + + if (!accumulates || !ret.has(name)) { + ArgumentData data; + ret.append(name, data); + } // handle the args - if (arg_num == MORE_THAN_ZERO_ARG_N || arg_num == MORE_THAN_ONE_ARG_N) { - // infinite arguments - if (arg_num == MORE_THAN_ONE_ARG_N && args.size() <= index + 1) { - return "at least one argument expected by " + name; + if (arg_num == AT_MOST_ONE_ARG_N) { + // Zero or one value. A value is taken only when the following token does not name + // another option of this command, which leaves this command's positional arguments + // in place. A "--" token makes whatever follows it a value rather than an option. + unsigned j{index + 1}; + bool takes_value{false}; + + if (j < args.size()) { + if (args[j] == "--") { + ++j; + takes_value = j < args.size(); + } else { + takes_value = !is_registered_option(args[j]); + } } - for (unsigned j = index + 1; j < args.size(); j++) { + if (takes_value) { + ret.append_arg(name, args[j]); + ++j; + } + args.erase(args.begin() + index, args.begin() + j); + index -= 1; + return ""; + } + if (arg_num == MORE_THAN_ZERO_ARG_N || arg_num == MORE_THAN_ONE_ARG_N) { + // Variable number of arguments. Stop collecting at a token that names another option of this + // command, so options written afterwards keep their own values. Every other token is taken as + // a value, including a positional argument of the command, which is why an option whose value + // is optional wants AT_MOST_ONE_ARG_N rather than MORE_THAN_ZERO_ARG_N. A "--" token ends + // option recognition, which is how a value that starts with '-' can be passed. + unsigned j{index + 1}; + unsigned collected{0}; + bool recognize_options{true}; + + for (; j < args.size(); j++) { + if (recognize_options) { + if (args[j] == "--") { + recognize_options = false; + continue; + } + if (is_registered_option(args[j])) { + break; + } + } ret.append_arg(name, args[j]); + ++collected; } - args.erase(args.begin() + index, args.end()); + if (arg_num == MORE_THAN_ONE_ARG_N && collected == 0) { + return "at least one argument expected by " + name; + } + args.erase(args.begin() + index, args.begin() + j); + index -= 1; return ""; } - // finite number of argument handling - for (unsigned j = 0; j < arg_num; j++) { - if (args.size() < index + j + 2 || args[index + j + 1].empty()) { + // Fixed number of arguments. A token naming another option of this command is not a value, so + // the missing value is reported rather than the following option being consumed as one. A "--" + // token ends option recognition, which is how a value that starts with '-' is passed. + unsigned j{index + 1}; + bool recognize_options{true}; + + for (unsigned collected{0}; collected < arg_num; ++j) { + if (j >= args.size() || args[j].empty()) { return std::to_string(arg_num) + " argument(s) expected by " + name; } - ret.append_arg(name, args[index + j + 1]); + if (recognize_options) { + if (args[j] == "--") { + recognize_options = false; + continue; + } + if (is_registered_option(args[j])) { + return std::to_string(arg_num) + " argument(s) expected by " + name; + } + } + ret.append_arg(name, args[j]); + ++collected; } // erase the used arguments and append the data to the return structure - args.erase(args.begin() + index, args.begin() + index + arg_num + 1); + args.erase(args.begin() + index, args.begin() + j); index -= 1; return ""; } @@ -658,7 +742,7 @@ ArgParser::Command::append_option_data(Arguments &ret, AP_StrVec &args, int inde if (args[i][0] == '-' && args[i][1] == '-' && args[i].find('=') != std::string::npos) { // deal with --args= std::string option_name = args[i].substr(0, args[i].find_first_of('=')); - std::string value = args[i].substr(args[i].find_last_of('=') + 1); + std::string value = args[i].substr(args[i].find_first_of('=') + 1); if (value.empty()) { help_message("missing argument for '" + option_name + "'"); } @@ -705,6 +789,16 @@ ArgParser::Command::append_option_data(Arguments &ret, AP_StrVec &args, int inde } else { cur_option = _option_list.at(short_it->second); } + // Counted for the same repetition check as the --option=value form, so that an option + // taking at most one value cannot be given more by mixing the two spellings. + if (cur_option.arg_num == AT_MOST_ONE_ARG_N) { + check_map[cur_option.long_option] += 1; + // An empty token is not a value, and the --option=value spelling already refuses one, + // so refuse it here rather than silently falling back to the declared default. + if (i + 1 < args.size() && args[i + 1].empty()) { + help_message("missing argument for '" + args[i] + "'"); + } + } // handle the arguments std::string err = handle_args(ret, args, cur_option.key, cur_option.arg_num, i); if (!err.empty()) { @@ -720,9 +814,15 @@ ArgParser::Command::append_option_data(Arguments &ret, AP_StrVec &args, int inde } // check for wrong number of arguments for --arg=... for (const auto &it : check_map) { - unsigned num = _option_list.at(it.first).arg_num; - if (num != it.second && num < MORE_THAN_ONE_ARG_N) { - help_message(std::to_string(_option_list.at(it.first).arg_num) + " arguments expected by " + it.first); + unsigned const num = _option_list.at(it.first).arg_num; + if (num == AT_MOST_ONE_ARG_N) { + // At most one, so a repeated option is as wrong as a repeated fixed arity one, which + // is_variable_arg_num() would otherwise wave through. + if (it.second > 1) { + help_message("at most one argument expected by " + it.first); + } + } else if (num != it.second && !is_variable_arg_num(num)) { + help_message(std::to_string(num) + " arguments expected by " + it.first); } } } @@ -841,6 +941,12 @@ Arguments::get(std::string const &name) return ArgumentData(); } +bool +Arguments::has(std::string const &name) const noexcept +{ + return _data_map.find(name) != _data_map.end(); +} + void Arguments::append(std::string const &key, ArgumentData const &value) { diff --git a/src/tscore/unit_tests/test_ArgParser.cc b/src/tscore/unit_tests/test_ArgParser.cc index 0a32502e964..8803fa2eff1 100644 --- a/src/tscore/unit_tests/test_ArgParser.cc +++ b/src/tscore/unit_tests/test_ArgParser.cc @@ -217,3 +217,271 @@ TEST_CASE("with_required does not trigger on default values", "[parse]") REQUIRE(parsed.get("threshold").value() == "300"); REQUIRE(parsed.get("verbose") == true); } + +TEST_CASE("Variable argument option stops at a following option", "[parse]") +{ + ts::ArgParser parser; + parser.add_global_usage("test_prog [OPTIONS]"); + + ts::ArgParser::Command &cmd = parser.add_command("reload", "reload configs"); + cmd.add_option("--directive", "-D", "reload directives", "", MORE_THAN_ZERO_ARG_N, ""); + cmd.add_option("--token", "-t", "a token", "", 1, ""); + cmd.add_option("--monitor", "-m", "monitor progress"); + + // A flag after a variable argument option is not swallowed as a value. + const char *argv1[] = {"test_prog", "reload", "-D", "a.id=1", "b.id=2", "-m", nullptr}; + ts::Arguments parsed = parser.parse(argv1); + REQUIRE(parsed.get("directive").size() == 2); + REQUIRE(parsed.get("directive")[0] == "a.id=1"); + REQUIRE(parsed.get("directive")[1] == "b.id=2"); + REQUIRE(parsed.get("monitor") == true); + + // A following option keeps its own argument. + const char *argv2[] = {"test_prog", "reload", "-D", "a.id=1", "-t", "my_token", nullptr}; + parsed = parser.parse(argv2); + REQUIRE(parsed.get("directive").size() == 1); + REQUIRE(parsed.get("directive")[0] == "a.id=1"); + REQUIRE(parsed.get("token").value() == "my_token"); + + // The long form of the following option is recognized too. + const char *argv3[] = {"test_prog", "reload", "-D", "a.id=1", "--monitor", nullptr}; + parsed = parser.parse(argv3); + REQUIRE(parsed.get("directive").size() == 1); + REQUIRE(parsed.get("monitor") == true); + + // So is its --option=value form. + const char *argv4[] = {"test_prog", "reload", "-D", "a.id=1", "--token=my_token", nullptr}; + parsed = parser.parse(argv4); + REQUIRE(parsed.get("directive").size() == 1); + REQUIRE(parsed.get("token").value() == "my_token"); +} + +TEST_CASE("Double dash ends option recognition for variable argument options", "[parse]") +{ + ts::ArgParser parser; + parser.add_global_usage("test_prog [OPTIONS]"); + + ts::ArgParser::Command &cmd = parser.add_command("reload", "reload configs"); + cmd.add_option("--directive", "-D", "reload directives", "", MORE_THAN_ZERO_ARG_N, ""); + cmd.add_option("--monitor", "-m", "monitor progress"); + + // After "--" a token that looks like an option is taken as a value instead. + const char *argv[] = {"test_prog", "reload", "-D", "--", "-m", "a.id=1", nullptr}; + ts::Arguments parsed = parser.parse(argv); + REQUIRE(parsed.get("directive").size() == 2); + REQUIRE(parsed.get("directive")[0] == "-m"); + REQUIRE(parsed.get("directive")[1] == "a.id=1"); + REQUIRE(parsed.get("monitor") == false); +} + +TEST_CASE("Option value keeps embedded equal signs", "[parse]") +{ + ts::ArgParser parser; + parser.add_global_usage("test_prog [OPTIONS]"); + + ts::ArgParser::Command &cmd = parser.add_command("reload", "reload configs"); + cmd.add_option("--directive", "-D", "reload directives", "", MORE_THAN_ZERO_ARG_N, ""); + + // Only the first '=' separates the option from its value. + const char *argv[] = {"test_prog", "reload", "--directive=ip_allow.id=foo", nullptr}; + ts::Arguments parsed = parser.parse(argv); + REQUIRE(parsed.get("directive").size() == 1); + REQUIRE(parsed.get("directive")[0] == "ip_allow.id=foo"); +} + +TEST_CASE("An option taking at most one argument leaves the positional arguments alone", "[parse]") +{ + ts::ArgParser parser; + parser.add_global_usage("test_prog [OPTIONS]"); + + // Mirrors "traffic_ctl config get [--cold [FILE]] RECORD [RECORD ...]". + ts::ArgParser::Command &cmd = parser.add_command("get", "get values", "", MORE_THAN_ONE_ARG_N, nullptr); + cmd.add_option("--cold", "-c", "read from a file", "", AT_MOST_ONE_ARG_N); + cmd.add_option("--records", "", "yaml output"); + + // The option takes its single value and stops, so the command keeps its own arguments. + const char *argv1[] = {"test_prog", "get", "-c", "records.yaml", "proxy.config.x", nullptr}; + ts::Arguments parsed = parser.parse(argv1); + REQUIRE(parsed.get("cold").value() == "records.yaml"); + REQUIRE(parsed.get("get").size() == 1); + REQUIRE(parsed.get("get")[0] == "proxy.config.x"); + + // Several positional arguments are unaffected. + const char *argv2[] = {"test_prog", "get", "-c", "records.yaml", "proxy.config.x", "proxy.config.y", nullptr}; + parsed = parser.parse(argv2); + REQUIRE(parsed.get("cold").value() == "records.yaml"); + REQUIRE(parsed.get("get").size() == 2); + REQUIRE(parsed.get("get")[1] == "proxy.config.y"); + + // Trailing placement keeps working. + const char *argv3[] = {"test_prog", "get", "proxy.config.x", "-c", "records.yaml", nullptr}; + parsed = parser.parse(argv3); + REQUIRE(parsed.get("cold").value() == "records.yaml"); + REQUIRE(parsed.get("get").size() == 1); + + // The --option=value form is not mistaken for a fixed arity mismatch. + const char *argv4[] = {"test_prog", "get", "--cold=records.yaml", "proxy.config.x", nullptr}; + parsed = parser.parse(argv4); + REQUIRE(parsed.get("cold").value() == "records.yaml"); + REQUIRE(parsed.get("get").size() == 1); +} + +TEST_CASE("An option taking at most one argument accepts no value at all", "[parse]") +{ + ts::ArgParser parser; + parser.add_global_usage("test_prog [OPTIONS]"); + + ts::ArgParser::Command &cmd = parser.add_command("get", "get values", "", MORE_THAN_ONE_ARG_N, nullptr); + cmd.add_option("--cold", "-c", "read from a file", "", AT_MOST_ONE_ARG_N); + cmd.add_option("--records", "", "yaml output"); + + // Called with no value, so the caller falls back to its own default. + const char *argv1[] = {"test_prog", "get", "proxy.config.x", "-c", nullptr}; + ts::Arguments parsed = parser.parse(argv1); + REQUIRE(parsed.get("cold") == true); + REQUIRE(parsed.get("cold").size() == 0); + REQUIRE(parsed.get("cold").value().empty()); + REQUIRE(parsed.get("get").size() == 1); + + // A following option is never taken as the value. + const char *argv2[] = {"test_prog", "get", "-c", "--records", "proxy.config.x", nullptr}; + parsed = parser.parse(argv2); + REQUIRE(parsed.get("cold").size() == 0); + REQUIRE(parsed.get("records") == true); + REQUIRE(parsed.get("get").size() == 1); + REQUIRE(parsed.get("get")[0] == "proxy.config.x"); + + // After "--" even a token shaped like an option becomes the value. + const char *argv3[] = {"test_prog", "get", "-c", "--", "-weird-name.yaml", "proxy.config.x", nullptr}; + parsed = parser.parse(argv3); + REQUIRE(parsed.get("cold").value() == "-weird-name.yaml"); + REQUIRE(parsed.get("get").size() == 1); + REQUIRE(parsed.get("get")[0] == "proxy.config.x"); +} + +TEST_CASE("An option taking a fixed number of arguments can be given a value shaped like an option", "[parse]") +{ + ts::ArgParser parser; + parser.add_global_usage("test_prog [OPTIONS]"); + + // Mirrors "traffic_ctl server debug enable [--tags TAGS] [--append]". + ts::ArgParser::Command &cmd = parser.add_command("enable", "enable debug"); + cmd.add_option("--tags", "-t", "debug tags", "", 1); + cmd.add_option("--append", "-a", "append to the existing tags"); + + // A value that starts with '-' is passed after "--", which is otherwise taken as naming an + // option and reported as a missing value. + const char *argv1[] = {"test_prog", "enable", "-t", "--", "-a", nullptr}; + ts::Arguments parsed = parser.parse(argv1); + REQUIRE(parsed.get("tags").value() == "-a"); + REQUIRE(parsed.get("append") == false); + + // The --option=value form needs no escape. + const char *argv2[] = {"test_prog", "enable", "--tags=-a", nullptr}; + parsed = parser.parse(argv2); + REQUIRE(parsed.get("tags").value() == "-a"); + REQUIRE(parsed.get("append") == false); + + // An option written after the value keeps its own meaning. + const char *argv3[] = {"test_prog", "enable", "-t", "http", "-a", nullptr}; + parsed = parser.parse(argv3); + REQUIRE(parsed.get("tags").value() == "http"); + REQUIRE(parsed.get("append") == true); +} + +TEST_CASE("A repeated variable argument option accumulates its values", "[parse]") +{ + ts::ArgParser parser; + parser.add_global_usage("test_prog [OPTIONS]"); + + // Mirrors "traffic_ctl config reload [-D DIRECTIVE...] [-d SOURCE...] [-m]". + ts::ArgParser::Command &cmd = parser.add_command("reload", "reload configs"); + cmd.add_option("--directive", "-D", "reload directives", "", MORE_THAN_ZERO_ARG_N, ""); + cmd.add_option("--data", "-d", "inline config data", "", MORE_THAN_ZERO_ARG_N, ""); + cmd.add_option("--monitor", "-m", "monitor progress"); + + // Collection stops at the second -D, so the values of the first must survive it. + const char *argv1[] = {"test_prog", "reload", "-D", "a.id=1", "-D", "b.id=2", nullptr}; + ts::Arguments parsed = parser.parse(argv1); + REQUIRE(parsed.get("directive").size() == 2); + REQUIRE(parsed.get("directive")[0] == "a.id=1"); + REQUIRE(parsed.get("directive")[1] == "b.id=2"); + + // Each occurrence keeps every value it collected, in the order written. + const char *argv2[] = {"test_prog", "reload", "-D", "a.id=1", "b.id=2", "-D", "c.id=3", "d.id=4", nullptr}; + parsed = parser.parse(argv2); + REQUIRE(parsed.get("directive").size() == 4); + REQUIRE(parsed.get("directive")[0] == "a.id=1"); + REQUIRE(parsed.get("directive")[3] == "d.id=4"); + + // An unrelated option written between the two occurrences keeps its own meaning. + const char *argv3[] = {"test_prog", "reload", "-D", "a.id=1", "-m", "-D", "b.id=2", nullptr}; + parsed = parser.parse(argv3); + REQUIRE(parsed.get("directive").size() == 2); + REQUIRE(parsed.get("directive")[1] == "b.id=2"); + REQUIRE(parsed.get("monitor") == true); + + // The two spellings count against the same option, in either order. + const char *argv4[] = {"test_prog", "reload", "-D", "a.id=1", "--directive=b.id=2", nullptr}; + parsed = parser.parse(argv4); + REQUIRE(parsed.get("directive").size() == 2); + REQUIRE(parsed.get("directive")[0] == "a.id=1"); + REQUIRE(parsed.get("directive")[1] == "b.id=2"); + + const char *argv5[] = {"test_prog", "reload", "--directive=a.id=1", "--directive=b.id=2", nullptr}; + parsed = parser.parse(argv5); + REQUIRE(parsed.get("directive").size() == 2); + + // Repeated -d merges the same way, which is what the documented multi-source reload needs. + const char *argv6[] = {"test_prog", "reload", "-d", "@ip_allow.yaml", "-d", "@sni.yaml", nullptr}; + parsed = parser.parse(argv6); + REQUIRE(parsed.get("data").size() == 2); + REQUIRE(parsed.get("data")[0] == "@ip_allow.yaml"); + REQUIRE(parsed.get("data")[1] == "@sni.yaml"); + + // Two different options each keep their own values. + const char *argv7[] = {"test_prog", "reload", "-D", "a.id=1", "-d", "@f.yaml", "-D", "b.id=2", nullptr}; + parsed = parser.parse(argv7); + REQUIRE(parsed.get("directive").size() == 2); + REQUIRE(parsed.get("data").size() == 1); + REQUIRE(parsed.get("data")[0] == "@f.yaml"); +} + +TEST_CASE("A repeated option requiring at least one argument accumulates too", "[parse]") +{ + ts::ArgParser parser; + parser.add_global_usage("test_prog [OPTIONS]"); + + // Mirrors "traffic_ctl rpc invoke [--params PARAM...]". + ts::ArgParser::Command &cmd = parser.add_command("invoke", "invoke a method"); + cmd.add_option("--params", "-p", "request parameters", "", MORE_THAN_ONE_ARG_N, ""); + cmd.add_option("--format", "-f", "output format", "", 1, ""); + + const char *argv1[] = {"test_prog", "invoke", "-p", "one", "-p", "two", nullptr}; + ts::Arguments parsed = parser.parse(argv1); + REQUIRE(parsed.get("params").size() == 2); + REQUIRE(parsed.get("params")[0] == "one"); + REQUIRE(parsed.get("params")[1] == "two"); + + // The arity is satisfied by the first occurrence, so a later one is not left short. + const char *argv2[] = {"test_prog", "invoke", "-p", "one", "-f", "json", "-p", "two", nullptr}; + parsed = parser.parse(argv2); + REQUIRE(parsed.get("params").size() == 2); + REQUIRE(parsed.get("format").value() == "json"); +} + +TEST_CASE("A repeated option taking a fixed number of arguments keeps the last value", "[parse]") +{ + ts::ArgParser parser; + parser.add_global_usage("test_prog [OPTIONS]"); + + // Mirrors "traffic_ctl server debug enable [--tags TAGS]". Only an unbounded arity + // accumulates; a fixed one keeps the behaviour it has always had. + ts::ArgParser::Command &cmd = parser.add_command("enable", "enable debug"); + cmd.add_option("--tags", "-t", "debug tags", "", 1); + + const char *argv1[] = {"test_prog", "enable", "-t", "http", "-t", "cache", nullptr}; + ts::Arguments parsed = parser.parse(argv1); + REQUIRE(parsed.get("tags").size() == 1); + REQUIRE(parsed.get("tags").value() == "cache"); +} diff --git a/tests/gold_tests/jsonrpc/config_reload_directive_cli.test.py b/tests/gold_tests/jsonrpc/config_reload_directive_cli.test.py new file mode 100644 index 00000000000..0cb1d443281 --- /dev/null +++ b/tests/gold_tests/jsonrpc/config_reload_directive_cli.test.py @@ -0,0 +1,195 @@ +''' +Verify traffic_ctl command line parsing for the reload options that take a +variable number of values, --directive (-D) and --data (-d). + +Options declared with MORE_THAN_ZERO_ARG_N used to consume every remaining +token, so any option written after -D was silently swallowed as a directive +value and never parsed. -D therefore had to be the last option, and -D could +not be combined with -d. Once collection stops at the following option, the +option can be written more than once, and each occurrence has to keep the +values it collected rather than replace the ones before it. These runs assert +on the JSONRPC request that traffic_ctl builds (printed by -f rpc), because the +subject under test is the command line parsing rather than the server side +handling of the reload. +''' +# 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. + +Test.Summary = 'Verify traffic_ctl -D/-d argument parsing for config reload' +Test.ContinueOnFail = True + +ts = Test.MakeATSProcess("ts") +ts.StartupTimeout = 30 + +ts.Disk.records_config.update({ + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'rpc|config.reload', +}) + +ts.Disk.ip_allow_yaml.AddLines([ + 'ip_allow:', + '- apply: in', + ' ip_addrs: 0/0', + ' action: allow', + ' methods: ALL', +]) + +# ============================================================================ +# Test 1: an option written after -D keeps its own argument +# ============================================================================ +tr = Test.AddTestRun("Option after -D is not consumed as a directive value") +tr.Processes.Default.StartBefore(ts) +tr.Processes.Default.Command = "traffic_ctl config reload -D ip_allow.id=foo -t cli_token_1 -f rpc" +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"token": "cli_token_1"', "-t must survive after -D") +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"id": "foo"', "the directive must still be parsed") +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 2: several directives, then an option +# ============================================================================ +tr = Test.AddTestRun("Multiple directives followed by an option") +tr.Processes.Default.Command = "traffic_ctl config reload -D ip_allow.id=1 sni.id=2 -t cli_token_2 -f rpc" +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"token": "cli_token_2"', "-t must survive after -D") +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"ip_allow"', "first directive key must be present") +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"sni"', "second directive key must be present") +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 3: -D combined with -d, which the parser previously made impossible +# ============================================================================ +tr = Test.AddTestRun("-D can be combined with -d") +tr.Processes.Default.Command = "traffic_ctl config reload -D ip_allow.id=foo -d 'ip_allow: {rules: [x]}' -f rpc" +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"rules"', "inline content from -d must be present") +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"_reload"', "directives from -D must be present") +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 4: --directive=value keeps a value that itself contains '=' +# ============================================================================ +tr = Test.AddTestRun("--directive=value preserves embedded equal signs") +tr.Processes.Default.Command = "traffic_ctl config reload --directive=ip_allow.id=foo -f rpc" +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"id": "foo"', "the whole value must reach the request") +tr.Processes.Default.Streams.stdout += Testers.ExcludesExpression("Invalid directive format", "the value must parse cleanly") +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 5: "--" ends option recognition, so the value is taken literally and +# then rejected by the directive format check +# ============================================================================ +tr = Test.AddTestRun("A value after -- is taken literally") +tr.Processes.Default.Command = "traffic_ctl config reload -D -- -m" +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.ReturnCode = 2 +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression( + "Invalid directive format '-m'", "-m must be treated as a directive value, not as --monitor") +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 6: -D without any directive would silently reload every handler +# ============================================================================ +tr = Test.AddTestRun("-D requires at least one directive") +tr.Processes.Default.Command = "traffic_ctl config reload -D" +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.ReturnCode = 2 +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression("requires at least one", "-D must not be a silent no-op") +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 7: same for -d, where a silent full reload is especially misleading +# ============================================================================ +tr = Test.AddTestRun("-d requires content") +tr.Processes.Default.Command = "traffic_ctl config reload -d" +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.ReturnCode = 2 +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression("requires content", "-d must not be a silent no-op") +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 8: a repeated -D keeps the directives of every occurrence +# ============================================================================ +tr = Test.AddTestRun("A repeated -D accumulates its directives") +tr.Processes.Default.Command = "traffic_ctl config reload -D ip_allow.id=1 -D sni.id=2 -f rpc" +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"ip_allow"', "the first occurrence must survive the second") +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"sni"', "the second occurrence must be present") +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 9: repeating the option is how a directive is written after another +# option, since collection stops at the option rather than at the value +# ============================================================================ +tr = Test.AddTestRun("A repeated -D survives an option written between the two") +tr.Processes.Default.Command = "traffic_ctl config reload -D ip_allow.id=1 -t cli_token_8 -D sni.id=2 -f rpc" +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"token": "cli_token_8"', "-t must keep its own value") +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"ip_allow"', "the directive before -t must survive") +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"sni"', "the directive after -t must be parsed") +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 10: the documented multi source reload, where dropping one -d would +# leave its handler out of the reload without reporting anything +# ============================================================================ +tr = Test.AddTestRun("A repeated -d merges every source") +tr.Processes.Default.Command = ("traffic_ctl config reload -d 'ip_allow: {rules: [x]}' -d 'sni: {rules: [y]}' -f rpc") +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"ip_allow"', "content from the first -d must be present") +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"sni"', "content from the second -d must be present") +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 11: an empty -d token is no content. The token survives the argument +# count, so without the content check the request degrades to a full reload +# ============================================================================ +tr = Test.AddTestRun("-d with an empty argument is reported as empty") +tr.Processes.Default.Command = "traffic_ctl config reload -d ''" +# autest's shell detection indexes arg[0], so an empty argument needs the shell path. +tr.Processes.Default.ForceUseShell = True +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.ReturnCode = 2 +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression( + "received an empty value", "an empty -d must be reported as empty, not as a missing argument") +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 12: same for -D +# ============================================================================ +tr = Test.AddTestRun("-D with an empty argument is reported as empty") +tr.Processes.Default.Command = "traffic_ctl config reload -D ''" +tr.Processes.Default.ForceUseShell = True +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.ReturnCode = 2 +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression( + "received an empty value", "an empty -D must be reported as empty, not as a missing argument") +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 13: an empty token is refused even next to a real one. This is what an +# unset variable in a script written with several -d looks like, and accepting +# it would reload fewer configs than were asked for without saying so +# ============================================================================ +tr = Test.AddTestRun("An empty -d token is refused next to a real one") +tr.Processes.Default.Command = ("traffic_ctl config reload -d '' -d 'ip_allow: {rules: [x]}'") +tr.Processes.Default.ForceUseShell = True +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.ReturnCode = 2 +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression( + "received an empty value", "a partial reload must not happen silently") +tr.StillRunningAfter = ts diff --git a/tests/gold_tests/records/traffic_ctl_cold_config.test.py b/tests/gold_tests/records/traffic_ctl_cold_config.test.py index c27925baf2b..f605a4deac0 100644 --- a/tests/gold_tests/records/traffic_ctl_cold_config.test.py +++ b/tests/gold_tests/records/traffic_ctl_cold_config.test.py @@ -92,3 +92,106 @@ tr.Processes.Default.ReturnCode = 0 tr.Processes.Default.Env = ts.Env tr.Disk.File(file).Content = 'gold/records.yaml.cold_test5.gold' + +# --cold takes at most one file name, so it does not consume the record names that follow +# it. Before that was the case it had to be written after them, which the runs above do. +records_file = os.path.join(ts.Variables.CONFIGDIR, "records.yaml") + +# 6 +tr = Test.AddTestRun("Get a value with the file name given before the record") +tr.Processes.Default.Command = f'traffic_ctl config get -c {records_file} proxy.config.diags.debug.tags' +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression( + 'proxy.config.diags.debug.tags: http', 'The record must still be parsed as a record') + +# 7 +tr = Test.AddTestRun("Get several values with the file name given before them") +tr.Processes.Default.Command = ( + f'traffic_ctl config get -c {records_file} ' + 'proxy.config.diags.debug.tags proxy.config.cache.limits.http.max_alts') +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression( + 'proxy.config.diags.debug.tags: http', 'The first record must be parsed as a record') +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression( + 'proxy.config.cache.limits.http.max_alts: 1', 'The second record must be parsed as a record') + +# 8 +tr = Test.AddTestRun("Get a value using the --cold=FILE form") +tr.Processes.Default.Command = f'traffic_ctl config get --cold={records_file} proxy.config.diags.debug.tags' +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression( + 'proxy.config.diags.debug.tags: http', 'The record must still be parsed as a record') + +# 9 +file = os.path.join(ts.Variables.CONFIGDIR, "new_records3.yaml") +tr = Test.AddTestRun("Set a value with the file name given before the record and the value") +tr.Processes.Default.Command = f'traffic_ctl config set -c {file} proxy.config.cache.limits.http.max_alts 3' +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.Env = ts.Env +tr.Disk.File(file).Content = 'gold/records.yaml.cold_test5.gold' + +# 10 +tr = Test.AddTestRun("--cold takes at most one file name, so repeating it is an error") +tr.Processes.Default.Command = f'traffic_ctl config get --cold={records_file} --cold={records_file} proxy.config.diags.debug.tags' +tr.Processes.Default.ReturnCode = 64 # EX_USAGE - command line usage error +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.All = Testers.ContainsExpression( + 'at most one argument expected by --cold', 'A repeated --cold must be reported rather than the last one winning') + +# 11 +tr = Test.AddTestRun("A repeated --cold is an error in the space-separated form too") +tr.Processes.Default.Command = f'traffic_ctl config get -c {records_file} -c {records_file} proxy.config.diags.debug.tags' +tr.Processes.Default.ReturnCode = 64 # EX_USAGE - command line usage error +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.All = Testers.ContainsExpression( + 'at most one argument expected by --cold', 'A repeated -c must be reported rather than the last one winning') + +# 12 +tr = Test.AddTestRun("Mixing the two --cold spellings cannot smuggle in a second file name") +tr.Processes.Default.Command = f'traffic_ctl config get -c {records_file} --cold={records_file} proxy.config.diags.debug.tags' +tr.Processes.Default.ReturnCode = 64 # EX_USAGE - command line usage error +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.All = Testers.ContainsExpression( + 'at most one argument expected by --cold', 'The two forms must be counted together') + +# An empty file name reaches traffic_ctl when it is taken from a variable that is unset. The +# --cold=FILE form has always rejected it; the space-separated form used to fall through to the +# default records.yaml instead, so a run meant for another file read or wrote the live one. +# These runs go through "sh -c" because autest indexes the first character of every argument +# it splits, so an empty argument written straight into Command raises IndexError before the +# process starts. + +# 13 +tr = Test.AddTestRun("An empty file name is reported rather than taken as the default file") +tr.Processes.Default.Command = """sh -c 'traffic_ctl config get -c "" proxy.config.diags.debug.tags'""" +tr.Processes.Default.ReturnCode = 64 # EX_USAGE - command line usage error +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.All = Testers.ContainsExpression( + "missing argument for '-c'", 'An empty -c value must be reported, naming the option as written') + +# 14 +tr = Test.AddTestRun("An empty file name is reported before anything is written") +tr.Processes.Default.Command = """sh -c 'traffic_ctl config set -c "" proxy.config.cache.limits.http.max_alts 9'""" +tr.Processes.Default.ReturnCode = 64 # EX_USAGE - command line usage error +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.All = Testers.ContainsExpression( + "missing argument for '-c'", 'A set with an empty -c value must not fall back to the live records.yaml') + +# 15 +tr = Test.AddTestRun("The long spelling of an empty file name is reported as written") +tr.Processes.Default.Command = """sh -c 'traffic_ctl config get --cold "" proxy.config.diags.debug.tags'""" +tr.Processes.Default.ReturnCode = 64 # EX_USAGE - command line usage error +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.All = Testers.ContainsExpression( + "missing argument for '--cold'", 'The error must name the spelling the caller used') + +# 16 +tr = Test.AddTestRun("A bare -c before the record leaves set short of its own arguments") +tr.Processes.Default.Command = 'traffic_ctl config set -c proxy.config.cache.limits.http.max_alts 9' +tr.Processes.Default.ReturnCode = 64 # EX_USAGE - command line usage error +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.All = Testers.ContainsExpression( + r'2 argument\(s\) expected by set', 'set must report what it was left without, as the docs show') diff --git a/tests/gold_tests/traffic_ctl/traffic_ctl_server_debug.test.py b/tests/gold_tests/traffic_ctl/traffic_ctl_server_debug.test.py index f899f980708..30b781732b2 100644 --- a/tests/gold_tests/traffic_ctl/traffic_ctl_server_debug.test.py +++ b/tests/gold_tests/traffic_ctl/traffic_ctl_server_debug.test.py @@ -78,3 +78,21 @@ tr.Processes.Default.Streams.All = Testers.ContainsExpression( "Option \'--append\' requires \'--tags\' to be specified", "Should show error that --append requires --tags") tr.StillRunningAfter = traffic_ctl._ts + +# Test 15: An option written where the tags are expected leaves them missing, rather than being +# applied as the tags themselves. +tr = Test.AddTestRun("test --tags followed by another option") +tr.Processes.Default.Env = traffic_ctl._ts.Env +tr.Processes.Default.Command = "traffic_ctl server debug enable --tags --append" +tr.Processes.Default.ReturnCode = 64 # EX_USAGE - command line usage error +tr.Processes.Default.Streams.All = Testers.ContainsExpression( + "1 argument\\(s\\) expected by tags", "Should report the tags as missing") +tr.StillRunningAfter = traffic_ctl._ts + +# Test 16: Tags that are shaped like an option are passed after "--". +tr = Test.AddTestRun("test tags shaped like an option") +tr.Processes.Default.Env = traffic_ctl._ts.Env +tr.Processes.Default.Command = "traffic_ctl server debug enable --tags -- -a" +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.Streams.stdout = Testers.ContainsExpression('tags »"-a"«', "The value after -- must be taken as the tags") +tr.StillRunningAfter = traffic_ctl._ts From c2c2f17cf265d53aca111b99fbf4e0232d6b8999 Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Wed, 9 Sep 2026 10:13:59 -0500 Subject: [PATCH 10/11] Metrics: close the id lookup race and bounds gaps left by the lock revert (#13583) * Metrics: one gate for id validation, and fix the off-by-one valid(), lookup(IdType), name() and rename() each carried their own copy of the same range test, and the copies had drifted. valid() rejected an offset past MAX_SIZE; the other three did not. Since _splitID passes the low 16 bits of an id through unmasked and the offset check only applied when the id named the current blob, an id such as 0x0000FFFF indexed well past the end of a blob's 1024 entry arrays once a second blob existed. Ids reaching these accessors come from plugins through the TSStat* API, so they are untrusted. All four now go through Storage::_is_allocated(), which rejects a negative id, an offset no _makeId could have produced, an unallocated blob, and a slot at or past the allocation point. That last comparison also fixes an off-by-one: create() returns the id and then advances, so _cur_off is the next free slot, and the old <= / > tests accepted it. An increment there landed on the slot create() would hand out next, and since create() writes only the name and never the value, the next plugin to call TSStatCreate() received a metric already carrying someone else's count. Nothing depended on the loose bound: end() builds an id at the allocation point that is compared but never dereferenced, iterator::next() keeps the offset in range, and find() returns end() on a miss. * Metrics: publish the allocation point with release/acquire The lock removal in #13567 left the reader path reading _cur_blob, _cur_off and _blobs while a concurrent create() advances them, which is the data race #13310 took the mutex to close. Close it without the mutex instead. Making each counter atomic does not make the pair update atomically, and it does not need to. _cur_blob and _cur_off are publication points: each is written last, with a release store, after whatever it makes visible -- the blob pointer and the reset offset for _cur_blob, the slot's name for _cur_off. A reader acquires _cur_blob first, so observing a value for it also observes everything addBlob() wrote before releasing it. The torn pair a reader could otherwise see, a new blob index with the previous blob's stale offset, is unreachable rather than merely unlikely, so neither a packed word nor per-blob counters are needed. _blobs stays non-atomic. It is only read at an index no greater than _cur_blob, and that write is sequenced before the release store the reader acquired, so there is no race to close. Writers all hold the mutex and load relaxed. What remains is that a reader can observe an older _cur_blob with an already reset _cur_off and reject an id naming the previous blob, which drops an increment rather than misattributing one. Verified with a TSAN harness running eight readers validating and resolving ids across the whole space while a writer creates 2600 metrics across several blob boundaries: three reported races before this change, none after. * Metrics: cover concurrent id lookup, and make _extractType total Add a test that resolves ids from several threads while another registers metrics across a few blob boundaries. Nothing single threaded exercises the publication order the previous commit relies on; under the tsan preset, making either allocation counter non-atomic again reports a data race here. The test cannot catch a downgrade of the release/acquire pairs to relaxed -- atomics are race free at any ordering -- and says so, so the memory orders are not mistaken for tested. _extractType shifted a signed IdType, so _extractType(NOT_FOUND) sign extended to -4, a MetricType outside its enumeration, returned by Metrics::type(). Shifting unsigned is not enough on its own: the sign bit sits above the type field, so NOT_FOUND still yields 4. Mask to the single bit _makeId writes, which makes the function total for any input. * Add a ts::Metrics micro benchmark Nothing in tree measured the metric read paths, which is why a global mutex on the hottest one went unnoticed until it showed up in a production profile. Four cases, scaled by thread count: increment(id) what TSStatIntIncrement does, the path that regressed increment(ptr) what core and cripts do, the floor lookup(id) the lock free id resolution alone lookup(name) the same resolution through the mutex guarded name map lookup(name) is deliberately included as a positive control. It still takes the lock, so it must degrade with thread count; if it ever stops doing so, the harness is not loading the machine and the other three numbers mean nothing. Built only with ENABLE_BENCHMARKS, as with the rest of tools/benchmark. * Metrics: trim comments to the invariants State what holds rather than how it came to hold. Drops the explanations of which write order a comparison compensates for, what a reader would have seen otherwise, and what each benchmark case is meant to prove. Also shortens the createSpan boundary test's preamble, which describes the bug it covers at more length than the assertion needs. * Metrics: probe real ids in the concurrent lookup test The reader swept ids as consecutive integers, but an id packs the blob index above the offset, so 0..N only ever named blob 0 and everything from MAX_SIZE up decoded to an offset that validation rejects. Earlier cases in this file leave blob 0 full, so the reader was walking settled slots while the writer worked in a blob it never named. Take ids from what the writer has registered instead, and assert the ids span more than one blob so a future change cannot quietly confine the sweep again. Also assert the readers resolved something, since every id being skipped would otherwise pass. * Metrics: make _is_allocated private, tidy createSpan's index handling _is_allocated is only called by Storage's own accessors, so it does not belong in the public section; valid() remains the public gate. createSpan loaded _cur_off twice and _cur_blob once for its two guards, then re-read both unconditionally in case addBlob() had moved them. Load the pair once and refresh it only in the branch that grows a blob, which drops two atomic loads from the common path. Re-reading rather than adjusting the locals by hand keeps the caller from restating what addBlob() sets. * Publish the next free slot as one packed value _cur_blob and _cur_off were two atomics, and readers need the pair to be coherent. addBlob() reset the offset then bumped the blob, so a reader between the two saw the old blob with a zero offset and rejected every id in the just-completed blob. With ENABLE_FAST_SDK=OFF that reaches _TSReleaseAssert through TSStatInt*, so it aborts rather than losing a count. Reversing the stores only trades it for accepting ids in a blob nothing has been written to; two atomics have no coherent pair either way. One atomic holding the blob index above the offset, packed as an id is packed, fixes it: crossing a blob is a single release store. The value only ever increases, so an id is allocated exactly when it packs below the bound, which reduces the gate on every id based accessor to one acquire load and one compare. Acquiring the bound also acquires the blob install, so the null blob check goes away. It is also the id of the next free slot, which is what iteration wants for its end bound. Drop createSpan with it. It has no callers outside the tests, and it was the only path that could leave a blob partly filled -- it skipped to a fresh blob when a span did not fit, abandoning tail slots that were never handed out and that the packed bound would count as allocated. Without it, blobs fill contiguously and "packs below the bound" means exactly "was handed out". * Make the concurrent lookup test check what it claims Two ways it could pass without testing anything. Readers only published their tally on exit and nothing made them run before the writer finished, so on one CPU every reader could see stop and resolve nothing while resolved > 0 still held; it now publishes each resolution as it happens and the writer waits for one before stopping. And an id that lookup() clamps resolves to the reserved bad_id slot, whose name is not empty, so the name check could not detect a clamp; it now compares against the name that id must have. * Include and stop binding an unused offset Dropping createSpan took swoc/MemSpan.h with it, and that was what supplied for NOT_FOUND's numeric_limits. The header still compiles, through some other transitive path, which is exactly what makes it worth declaring. addBlob() destructured the packed value but only ever used the blob half. * Take the lock before touching a slot's name in rename() The name is the key _lookups is indexed by, so replacing it belongs entirely inside the lock. Nothing read the string outside it before -- binding a reference to it does not touch its bytes -- but computing that reference outside the lock made the boundary look wider than it is, and there is no reason for anything here to sit outside. * Metrics: trim comments, and drop ones about a check that is gone Two comments in the malformed-offset test explained that the null blob check, not the offset test, would reject those ids with only one blob allocated. The packed bound removed that check, so the reasoning no longer applied. * Remove rename() It mutated a slot's name while name() and lookup(id, &out_name) read that same std::string without the mutex and hand out views into it, which moonchen reproduced as a TSAN race. Locking rename() does not fix it; the readers are the lock free paths this PR exists to keep. Giving names immutable storage with its own lifetime rules would, but nothing outside the tests calls rename(). Without it a name is written once before the store that publishes it and never changes, so those readers are correct by construction. (cherry picked from commit c7af2e39b5c2a9fb3a26c2dc4bbb38d26d60c7e2) --- include/tsutil/Metrics.h | 107 ++++++------ src/records/unit_tests/test_RecRegister.cc | 4 +- src/tsutil/Metrics.cc | 129 ++++---------- src/tsutil/unit_tests/test_Metrics.cc | 187 +++++++++++++------- tools/benchmark/CMakeLists.txt | 3 + tools/benchmark/benchmark_Metrics.cc | 190 +++++++++++++++++++++ 6 files changed, 416 insertions(+), 204 deletions(-) create mode 100644 tools/benchmark/benchmark_Metrics.cc diff --git a/include/tsutil/Metrics.h b/include/tsutil/Metrics.h index 40cdef9522f..741b1e069a5 100644 --- a/include/tsutil/Metrics.h +++ b/include/tsutil/Metrics.h @@ -30,13 +30,12 @@ #include #include #include +#include #include #include #include #include -#include "swoc/MemSpan.h" - #include "tsutil/Assert.h" namespace ts @@ -85,8 +84,7 @@ class Metrics enum class MetricType : int { COUNTER = 0, GAUGE }; - using IdType = int32_t; // Could be a tuple, but one way or another, they have to be combined to an int32_t. - using SpanType = swoc::MemSpan; + using IdType = int32_t; // Could be a tuple, but one way or another, they have to be combined to an int32_t. static constexpr uint16_t MAX_BLOBS = 8192; static constexpr uint16_t MAX_SIZE = 1024; // For a total of 8M metrics @@ -147,12 +145,6 @@ class Metrics { return _storage->lookup(id, out_name, type); } - bool - rename(IdType id, const std::string_view name) - { - return _storage->rename(id, name); - } - AtomicType & operator[](IdType id) { @@ -267,9 +259,7 @@ class Metrics iterator end() const { - auto [blob, offset] = _storage->current(); - - return iterator(*this, _makeId(blob, offset, MetricType::COUNTER)); + return iterator(*this, _storage->next_free_id()); } iterator @@ -292,12 +282,6 @@ class Metrics return _storage->create(name, type); } - SpanType - _createSpan(size_t size, MetricType type, IdType *id = nullptr) - { - return _storage->createSpan(size, type, id); - } - // These are little helpers around managing the ID's static constexpr std::tuple _splitID(IdType value) @@ -308,7 +292,7 @@ class Metrics static constexpr MetricType _extractType(IdType value) { - return MetricType{value >> METRIC_TYPE_BITS}; + return MetricType{static_cast((static_cast(value) >> METRIC_TYPE_BITS) & 0x1)}; } static constexpr IdType @@ -318,13 +302,31 @@ class Metrics return (t << METRIC_TYPE_BITS | blob << 16 | offset); } + /// As @c _makeId, without the type bits. + static constexpr uint32_t + _pack(uint16_t blob, uint16_t offset) + { + return static_cast(blob) << 16 | offset; + } + + // A packed position must not reach the type bits, and an offset must fit its field. + static_assert(MAX_SIZE <= 0x10000); + static_assert(MAX_BLOBS <= (1 << (METRIC_TYPE_BITS - 16))); + class Storage { - BlobStorage _blobs; - uint16_t _cur_blob = 0; - uint16_t _cur_off = 0; - LookupTable _lookups; - mutable std::mutex _mutex; + /* The next free slot, packed as @c _makeId packs one. A single value because a reader that + * caught a new offset against an old blob index, or the reverse, would reject ids that exist + * or accept ids that do not. Release stored last, after the blob pointer or the slot's name it + * publishes. Only ever increases, so an id is allocated exactly when it packs below it. + * + * A slot's name is written once, before the store that publishes it, and never changes, which + * is what lets @c name and @c lookup hand out a view of it without the mutex. + */ + BlobStorage _blobs; + std::atomic _next_free{0}; + LookupTable _lookups; + mutable std::mutex _mutex; public: Storage(const Storage &) = delete; @@ -347,22 +349,43 @@ class Metrics AtomicType *lookup(Metrics::IdType id, std::string_view *out_name = nullptr, MetricType *out_type = nullptr) const; std::string_view name(IdType id) const; MetricType type(IdType id) const; - SpanType createSpan(size_t size, const MetricType type = MetricType::COUNTER, IdType *id = nullptr); - bool rename(IdType id, const std::string_view name); - std::pair - current() const + /// The id the next slot will get, which is also iteration's exclusive bound. + IdType + next_free_id() const { - std::lock_guard lock(_mutex); - return {_cur_blob, _cur_off}; + return static_cast(_next_free.load(std::memory_order_acquire)); } bool valid(IdType id) const { - auto [blob, entry] = _splitID(id); + return _is_allocated(id); + } + + private: + /** Whether @a id names an allocated slot. + * + * The gate for every id based accessor, since ids from the @c TSStat* API are untrusted. An id + * qualifies when it is non-negative, its offset is one @c _makeId could produce, and its slot + * has been handed out. + */ + bool + _is_allocated(IdType id) const + { + if (id < 0) { + return false; + } + + auto [blob_ix, offset] = _splitID(id); - return (id >= 0 && ((blob < _cur_blob && entry < MAX_SIZE) || (blob == _cur_blob && entry <= _cur_off))); + // Not implied below: an earlier blob can name an offset past MAX_SIZE and still pack under. + if (offset >= MAX_SIZE) { + return false; + } + + // Acquiring the bound acquires the blob install, so _blobs needs no check of its own. + return _pack(blob_ix, offset) < _next_free.load(std::memory_order_acquire); } }; @@ -376,7 +399,6 @@ class Metrics { public: using self_type = Gauge; - using SpanType = Metrics::SpanType; class AtomicType : public Metrics::AtomicType { @@ -452,14 +474,6 @@ class Metrics return reinterpret_cast(instance.lookup(instance._create(tmpname, MetricType::GAUGE))); } - static Metrics::Gauge::SpanType - createSpan(size_t size, IdType *id = nullptr) - { - auto &instance = Metrics::instance(); - - return instance._createSpan(size, MetricType::GAUGE, id); - } - static void increment(AtomicType *metric, uint64_t val = 1) { @@ -494,7 +508,6 @@ class Metrics { public: using self_type = Counter; - using SpanType = Metrics::SpanType; class AtomicType : public Metrics::AtomicType { @@ -570,14 +583,6 @@ class Metrics return reinterpret_cast(instance.lookup(instance._create(tmpname, MetricType::COUNTER))); } - static Metrics::Counter::SpanType - createSpan(size_t size, IdType *id = nullptr) - { - auto &instance = Metrics::instance(); - - return instance._createSpan(size, MetricType::COUNTER, id); - } - static void increment(AtomicType *metric, uint64_t val = 1) { diff --git a/src/records/unit_tests/test_RecRegister.cc b/src/records/unit_tests/test_RecRegister.cc index 38fb1adbf03..a406b6fbd6e 100644 --- a/src/records/unit_tests/test_RecRegister.cc +++ b/src/records/unit_tests/test_RecRegister.cc @@ -26,6 +26,7 @@ #include "test_Diags.h" #include +#include #include TEST_CASE("RecRegisterConfig - Type Dispatch", "[librecords][RecConfig]") @@ -103,7 +104,8 @@ TEST_CASE("RecLookupRecord - Concurrent metric registration", "[librecords][RecL std::atomic finished{false}; std::thread register_metrics([&]() { for (int i = 0; i < 100000; ++i) { - ts::Metrics::Counter::createSpan(1); + // Any registration will do; the point is to grow the store while lookups run. + ts::Metrics::Counter::create("proxy.test.concurrent.reg." + std::to_string(i)); } finished.store(true, std::memory_order_release); }); diff --git a/src/tsutil/Metrics.cc b/src/tsutil/Metrics.cc index c339eea2df8..1168c7d2e44 100644 --- a/src/tsutil/Metrics.cc +++ b/src/tsutil/Metrics.cc @@ -58,12 +58,16 @@ Metrics::Storage::addBlob() // The mutex must be held before calling this! { auto blob = std::make_unique(); + auto const cur_blob = static_cast(_next_free.load(std::memory_order_relaxed) >> 16); + debug_assert(blob); - // The write below is to _blobs[_cur_blob + 1], so the last usable blob index is MAX_BLOBS - 1. - release_assert(_cur_blob < MAX_BLOBS - 1); + // The write below is to _blobs[cur_blob + 1], so the last usable blob index is MAX_BLOBS - 1. + release_assert(cur_blob < MAX_BLOBS - 1); + + _blobs[cur_blob + 1] = std::move(blob); - _blobs[++_cur_blob] = std::move(blob); - _cur_off = 0; + // One store publishes both; the install above is sequenced before it. + _next_free.store(_pack(cur_blob + 1, 0), std::memory_order_release); } Metrics::IdType @@ -76,22 +80,27 @@ Metrics::Storage::create(std::string_view name, const MetricType type) return it->second; } - // The slot is written below and the bookkeeping only then advances, calling addBlob() once - // _cur_off reaches MAX_SIZE. Refusing the final slot of the final blob keeps addBlob() from - // ever being reached in an exhausted store, at a cost of one slot out of MAX_BLOBS * MAX_SIZE. - if (_cur_blob >= MAX_BLOBS - 1 && _cur_off >= MAX_SIZE - 1) { + // The slot is written below and the bookkeeping only then advances, calling addBlob() once the + // offset reaches MAX_SIZE. Refusing the final slot of the final blob keeps addBlob() from ever + // being reached in an exhausted store, at a cost of one slot out of MAX_BLOBS * MAX_SIZE. + auto const [cur_blob, cur_off] = _splitID(static_cast(_next_free.load(std::memory_order_relaxed))); + + if (cur_blob >= MAX_BLOBS - 1 && cur_off >= MAX_SIZE - 1) { return 0; // Slot 0 is the reserved bad_id. Cannot grow further. } - Metrics::IdType id = _makeId(_cur_blob, _cur_off, type); - Metrics::NamesAndAtomics *blob = _blobs[_cur_blob].get(); + Metrics::IdType id = _makeId(cur_blob, cur_off, type); + Metrics::NamesAndAtomics *blob = _blobs[cur_blob].get(); Metrics::NameStorage &names = std::get<0>(*blob); - names[_cur_off] = std::make_tuple(std::string(name), id); - _lookups.emplace(std::get<0>(names[_cur_off]), id); + names[cur_off] = std::make_tuple(std::string(name), id); + _lookups.emplace(std::get<0>(names[cur_off]), id); - if (++_cur_off >= MAX_SIZE) { - addBlob(); // This resets _cur_off to 0 as well + if (cur_off + 1 >= MAX_SIZE) { + addBlob(); + } else { + // Publishes the slot's name. + _next_free.store(_pack(cur_blob, cur_off + 1), std::memory_order_release); } return id; @@ -113,15 +122,16 @@ Metrics::Storage::lookup(const std::string_view name) const Metrics::AtomicType * Metrics::Storage::lookup(Metrics::IdType id, std::string_view *out_name, Metrics::MetricType *out_type) const { - auto [blob_ix, offset] = _splitID(id); - Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get(); + auto [blob_ix, offset] = _splitID(id); - // Do a sanity check on the ID, to make sure we don't index outside of the realm of possibility. - if (!blob || (blob_ix == _cur_blob && offset > _cur_off)) { - blob = _blobs[0].get(); - offset = 0; + // Anything not naming an allocated slot resolves to the reserved bad_id slot. + if (!_is_allocated(id)) { + blob_ix = 0; + offset = 0; } + Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get(); + if (out_name) { *out_name = std::get<0>(std::get<0>(*blob)[offset]); } @@ -159,15 +169,16 @@ Metrics::Storage::lookup(const std::string_view name, Metrics::IdType *out_id, M std::string_view Metrics::Storage::name(Metrics::IdType id) const { - auto [blob_ix, offset] = _splitID(id); - Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get(); + auto [blob_ix, offset] = _splitID(id); - // Do a sanity check on the ID, to make sure we don't index outside of the realm of possibility. - if (!blob || (blob_ix == _cur_blob && offset > _cur_off)) { - blob = _blobs[0].get(); - offset = 0; + // Anything not naming an allocated slot resolves to the reserved bad_id slot. + if (!_is_allocated(id)) { + blob_ix = 0; + offset = 0; } + Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get(); + const std::string &result = std::get<0>(std::get<0>(*blob)[offset]); return result; @@ -179,72 +190,6 @@ Metrics::Storage::type(IdType id) const return _extractType(id); } -Metrics::SpanType -Metrics::Storage::createSpan(size_t size, Metrics::MetricType type, Metrics::IdType *id) -{ - release_assert(size <= MAX_SIZE); - std::lock_guard lock(_mutex); - - // On the final blob there is nowhere left to grow, so refuse a span that would fill or overflow - // it rather than letting addBlob() assert. Same intent as the guard in create(), and the same - // cost: some slots of the last blob go unused. - if (_cur_blob >= MAX_BLOBS - 1 && _cur_off + size >= MAX_SIZE) { - if (id) { - *id = 0; // Slot 0 is the reserved bad_id. - } - return {}; - } - - // A span has to be contiguous, so one that does not fit in the current blob starts a new one. - if (_cur_off + size > MAX_SIZE) { - addBlob(); - } - - Metrics::IdType span_start = _makeId(_cur_blob, _cur_off, type); - Metrics::NamesAndAtomics *blob = _blobs[_cur_blob].get(); - Metrics::AtomicStorage &atomics = std::get<1>(*blob); - Metrics::SpanType span = Metrics::SpanType(&atomics[_cur_off], size); - - if (id) { - *id = span_start; - } - - _cur_off += size; - - // create() grows as soon as it consumes the last slot; do the same here. Otherwise a span ending - // exactly on the boundary leaves _cur_off at MAX_SIZE, and the next create() writes one past the - // end of the blob's name array. It also makes end() unreachable for iterator::next(), which - // wraps on ++offset == MAX_SIZE. - if (_cur_off >= MAX_SIZE) { - addBlob(); - } - - return span; -} - -bool -Metrics::Storage::rename(Metrics::IdType id, std::string_view name) -{ - auto [blob_ix, offset] = _splitID(id); - Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get(); - - // We can only rename Metrics that are already allocated - if (!blob || (blob_ix == _cur_blob && offset > _cur_off)) { - return false; - } - - std::string &cur = std::get<0>(std::get<0>(*blob)[offset]); - std::lock_guard lock(_mutex); - - if (cur.length() > 0) { - _lookups.erase(cur); - } - cur = name; - _lookups.emplace(cur, id); - - return true; -} - // Iterator implementation void Metrics::iterator::next() diff --git a/src/tsutil/unit_tests/test_Metrics.cc b/src/tsutil/unit_tests/test_Metrics.cc index 960324997b0..f267097807f 100644 --- a/src/tsutil/unit_tests/test_Metrics.cc +++ b/src/tsutil/unit_tests/test_Metrics.cc @@ -24,8 +24,10 @@ #include #include +#include #include #include +#include #include #include #include @@ -93,37 +95,6 @@ TEST_CASE("Metrics", "[libtsapi][Metrics]") REQUIRE(m[storeid].load() == 42); } - SECTION("Span allocation") - { - ts::Metrics::IdType span_id; - auto fooid = m.lookup("foo"); - auto span = Metrics::Counter::createSpan(17, &span_id); - - REQUIRE(span.size() == 17); - // Not fixed offsets: those only hold against a virgin store. Assert instead that the span - // was allocated above the earlier metric and that every id in it is valid. Both ids are - // counters, so they are directly comparable -- ids encode the metric type, and so are not - // ordered across differing types. - REQUIRE(fooid != ts::Metrics::NOT_FOUND); - REQUIRE(span_id != ts::Metrics::NOT_FOUND); - REQUIRE(span_id > fooid); - for (size_t i = 0; i < span.size(); ++i) { - REQUIRE(m.valid(span_id + static_cast(i))); - } - - m.rename(span_id + 0, "span.0"); - m.rename(span_id + 1, "span.1"); - m.rename(span_id + 2, "span.2"); - REQUIRE(m.name(fooid) == "foo"); - REQUIRE(m.name(span_id + 0) == "span.0"); - REQUIRE(m.name(span_id + 1) == "span.1"); - REQUIRE(m.name(span_id + 2) == "span.2"); - m.rename(fooid, "foo-new"); - REQUIRE(m.name(fooid) == "foo-new"); - REQUIRE(m.lookup("foo") == ts::Metrics::NOT_FOUND); - REQUIRE(m.lookup("foo-new") == fooid); - } - SECTION("lookup") { auto nm = m.lookup("notametric"); @@ -608,35 +579,131 @@ TEST_CASE("Metrics blob growth boundary", "[libtsapi][Metrics]") REQUIRE(std::adjacent_find(sorted_ptrs.begin(), sorted_ptrs.end()) == sorted_ptrs.end()); } -TEST_CASE("Metrics span lands exactly on a blob boundary", "[libtsapi][Metrics]") +TEST_CASE("Metrics malformed id offsets resolve to bad_id", "[libtsapi][Metrics]") +{ + // An id's offset field is 16 bits but a real offset is below MAX_SIZE, so a malformed one must + // not index past a blob's arrays. Filling past one blob puts the ids below in earlier blobs, + // where they pack under the bound and only the MAX_SIZE test rejects them. + auto &h = Metrics::hidden_instance(); + + for (int i = 0; i < Metrics::MAX_SIZE + 8; ++i) { + REQUIRE(Metrics::Counter::createHiddenPtr("f1.fill." + std::to_string(i)) != nullptr); + } + + auto const *bad = h.lookup(Metrics::IdType{0}); // the reserved bad_id slot + REQUIRE(bad != nullptr); + + for (Metrics::IdType id : {Metrics::IdType{0x0000FFFF}, Metrics::IdType{0x00000400}, Metrics::IdType{0x0001FFFF}}) { + REQUIRE(h.valid(id) == false); + REQUIRE(h.lookup(id) == bad); + REQUIRE(h.name(id) == h.name(Metrics::IdType{0})); + } +} + +TEST_CASE("Metrics id lookup is safe against concurrent creation", "[libtsapi][Metrics]") { - // A span has to be contiguous, so createSpan(MAX_SIZE) always starts a fresh blob and then fills - // it completely, whatever the current offset was. That makes this the one span size that reaches - // the boundary case deterministically: the offset ends up at MAX_SIZE, and unlike create(), - // createSpan used not to grow a new blob afterwards. The next create() then indexed one past the - // end of the blob's name array, and end() became an id that iterator::next() can never reach - // because it wraps at ++offset == MAX_SIZE. + // The id based read paths take no lock, so resolving an id races a concurrent create. Run both + // sides at once, over enough metrics to cross several blob boundaries. Under the tsan preset a + // non-atomic allocation counter reports a data race here; relaxing the memory orders does not, + // since atomics are race free at any ordering. // - // createSpan only ever targets the published store, so this necessarily allocates there. - Metrics::IdType span_id = Metrics::NOT_FOUND; - auto span = Metrics::Counter::createSpan(Metrics::MAX_SIZE, &span_id); - - REQUIRE(span.size() == Metrics::MAX_SIZE); - REQUIRE(span_id != Metrics::NOT_FOUND); - REQUIRE(span_id != 0); // 0 is the reserved bad_id, returned only when the store cannot grow. - - // The store must still be usable, and the new metric must be a real, resolvable entry rather - // than something written past the end of a blob. - auto p = Metrics::Counter::createPtr("span.boundary.after"); - REQUIRE(p != nullptr); - - auto &m = Metrics::instance(); - auto id = m.lookup("span.boundary.after"); - REQUIRE(id != Metrics::NOT_FOUND); - REQUIRE(m.valid(id)); - - // And it must behave like any other metric. - Metrics::Counter::increment(p, 7); - REQUIRE(Metrics::Counter::load(p) == 7); - REQUIRE(Metrics::Counter::createPtr("span.boundary.after") == p); + // Readers take ids from what the writer has registered rather than counting integers: an id packs + // the blob index above the offset, so consecutive integers only ever name the first blob. The + // store is relaxed, so a reader can pick up an id whose slot is not published yet, which is the + // case of interest. + constexpr int N_READERS = 4; + constexpr int N_CREATE = Metrics::MAX_SIZE * 2 + 64; + + auto &h = Metrics::hidden_instance(); + std::atomic stop{false}; + std::atomic ready{0}; + std::atomic mismatches{0}; + std::atomic resolved{0}; + + std::vector> created(N_CREATE); + + for (auto &c : created) { + c.store(Metrics::NOT_FOUND, std::memory_order_relaxed); + } + + // A clamped lookup resolves to the bad_id slot, whose name is not empty, so only the expected + // name detects one. + std::vector names; + + names.reserve(N_CREATE); + for (int i = 0; i < N_CREATE; ++i) { + names.push_back("pub.order." + std::to_string(i)); + } + + std::vector readers; + + for (int t = 0; t < N_READERS; ++t) { + readers.emplace_back([&]() { + ready.fetch_add(1, std::memory_order_release); + + while (!stop.load(std::memory_order_relaxed)) { + for (int i = 0; i < N_CREATE; ++i) { + auto const id = created[i].load(std::memory_order_relaxed); + + if (id == Metrics::NOT_FOUND || !h.valid(id)) { + continue; + } + + // valid() accepted the id, so lookup() must return that metric and not clamp. + std::string_view name; + Metrics::MetricType type; + auto *m = h.lookup(id, &name, &type); + + if (m == nullptr || name != names[i]) { + mismatches.fetch_add(1, std::memory_order_relaxed); + } + + // Published as it happens so the writer can wait for one. + resolved.fetch_add(1, std::memory_order_relaxed); + } + } + }); + } + + // Readers must be in their loops before the writer starts, or nothing races. + while (ready.load(std::memory_order_acquire) < N_READERS) { + std::this_thread::yield(); + } + + for (int i = 0; i < N_CREATE; ++i) { + REQUIRE(Metrics::Counter::createHiddenPtr(names[i]) != nullptr); + created[i].store(h.lookup(names[i]), std::memory_order_relaxed); + } + + // Being in the loop is not doing work: on one CPU the writer can finish first and every reader + // would then see stop. Wait for a real resolution so the check below cannot pass vacuously. + while (resolved.load(std::memory_order_relaxed) == 0) { + std::this_thread::yield(); + } + + stop.store(true, std::memory_order_relaxed); + for (auto &r : readers) { + r.join(); + } + + CHECK(mismatches.load() == 0); + CHECK(resolved.load() > 0); + + auto lo = std::numeric_limits::max(); + auto hi = std::numeric_limits::min(); + + for (int i = 0; i < N_CREATE; ++i) { + auto const id = h.lookup(names[i]); + + REQUIRE(id != Metrics::NOT_FOUND); + REQUIRE(h.valid(id)); + REQUIRE(h.name(id) == names[i]); + + lo = std::min(lo, id); + hi = std::max(hi, id); + } + + // More metrics than fit in one blob, so the ids must span blobs. A spread no wider than a blob + // would mean the sweep above never left the first one. + REQUIRE(hi - lo > Metrics::MAX_SIZE); } diff --git a/tools/benchmark/CMakeLists.txt b/tools/benchmark/CMakeLists.txt index e58e8dcf26b..ba3816cf6be 100644 --- a/tools/benchmark/CMakeLists.txt +++ b/tools/benchmark/CMakeLists.txt @@ -33,6 +33,9 @@ target_link_libraries(benchmark_ProxyAllocator PRIVATE Catch2::Catch2WithMain ts add_executable(benchmark_SharedMutex benchmark_SharedMutex.cc) target_link_libraries(benchmark_SharedMutex PRIVATE Catch2::Catch2 ts::tscore libswoc::libswoc) +add_executable(benchmark_Metrics benchmark_Metrics.cc) +target_link_libraries(benchmark_Metrics PRIVATE Catch2::Catch2 ts::tsutil libswoc::libswoc) + add_executable(benchmark_Random benchmark_Random.cc) target_link_libraries(benchmark_Random PRIVATE Catch2::Catch2WithMain ts::tscore) diff --git a/tools/benchmark/benchmark_Metrics.cc b/tools/benchmark/benchmark_Metrics.cc new file mode 100644 index 00000000000..e42b092a7f4 --- /dev/null +++ b/tools/benchmark/benchmark_Metrics.cc @@ -0,0 +1,190 @@ +/** @file + + Micro benchmark tool for ts::Metrics + + Metric values are lock free atomics; reaching one from an id is not. Four cases, scaled by thread + count: + + increment(id) valid() then lookup(id) then fetch_add, as TSStatIntIncrement does + increment(ptr) a bare fetch_add on a cached pointer, as core does + lookup(id) lock free id resolution + lookup(name) the same resolution through the mutex guarded name map + + increment(id) against increment(ptr) is what an id costs a plugin. lookup(id) against + lookup(name) isolates the mutex, and serves as a control: it must degrade with thread count, or + the harness is not loading the machine. + + @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. + */ + +#define CATCH_CONFIG_ENABLE_BENCHMARKING + +#include +#include +#include + +#include "tsutil/Metrics.h" + +#include +#include +#include +#include + +using ts::Metrics; + +namespace +{ +// Args +struct Conf { + int nthreads = 1; + int nops = 1000; + int nmetrics = 64; +}; + +Conf conf; + +/// The metrics every case operates on, created once. +struct Fixture { + std::vector ids; + std::vector ptrs; + std::vector names; + + Fixture() + { + auto &m = Metrics::instance(); + + ids.reserve(conf.nmetrics); + ptrs.reserve(conf.nmetrics); + names.reserve(conf.nmetrics); + + for (int i = 0; i < conf.nmetrics; ++i) { + names.push_back("benchmark.metrics." + std::to_string(i)); + + // Registers the name too, so the id and name lookups resolve to the same metric. + ptrs.push_back(Metrics::Counter::createPtr(names.back())); + ids.push_back(m.lookup(names.back())); + } + } +}; + +Fixture *fixture = nullptr; + +/// Run @a op on every thread, @c nops times each. The return value only defeats optimization. +template +int64_t +run(F &&op) +{ + std::vector threads; + std::atomic sink{0}; + + threads.reserve(conf.nthreads); + + for (int t = 0; t < conf.nthreads; ++t) { + threads.emplace_back([t, &sink, &op]() { + int64_t local = 0; + + for (int i = 0; i < conf.nops; ++i) { + // Stride per thread, or this measures cacheline ping-pong on one atomic. + local += op((t + i) % conf.nmetrics); + } + sink.fetch_add(local, std::memory_order_relaxed); + }); + } + + for (auto &th : threads) { + th.join(); + } + + return sink.load(); +} + +} // namespace + +TEST_CASE("Micro benchmark of ts::Metrics", "") +{ + auto &m = Metrics::instance(); + + SECTION("increment by id") + { + BENCHMARK("increment(id)") + { + return run([&m](int i) -> int64_t { + auto id = fixture->ids[i]; + + return m.valid(id) ? m.increment(id, 1) : 0; + }); + }; + } + + SECTION("increment by cached pointer") + { + // The floor: no resolution at all. + BENCHMARK("increment(ptr)") + { + return run([](int i) -> int64_t { + Metrics::Counter::increment(fixture->ptrs[i], 1); + + return 1; + }); + }; + } + + SECTION("lookup by id") + { + BENCHMARK("lookup(id)") + { + return run([&m](int i) -> int64_t { return m.lookup(fixture->ids[i]) != nullptr; }); + }; + } + + SECTION("lookup by name") + { + BENCHMARK("lookup(name)") + { + return run([&m](int i) -> int64_t { return m.lookup(fixture->names[i]) != Metrics::NOT_FOUND; }); + }; + } +} + +int +main(int argc, char *argv[]) +{ + Catch::Session session; + + using namespace Catch::Clara; + + // clang-format off + auto cli = session.cli() | + Opt(conf.nthreads, "")["--ts-nthreads"]("number of threads (default: 1)") | + Opt(conf.nops, "")["--ts-nops"]("operations per thread per run (default: 1000)") | + Opt(conf.nmetrics, "")["--ts-nmetrics"]("distinct metrics to spread across (default: 64)"); + // clang-format on + + session.cli(cli); + + int returnCode = session.applyCommandLine(argc, argv); + if (returnCode != 0) { + return returnCode; + } + + Fixture f; + fixture = &f; + + return session.run(); +} From 72c0d24230dd682ac50ec377725e735ffd138cbc Mon Sep 17 00:00:00 2001 From: Brian Neradt Date: Wed, 9 Sep 2026 14:07:37 -0500 Subject: [PATCH 11/11] Restore regex_remap's default PCRE2 match limit (#13652) Valid URLs with long query strings can miss regex_remap redirects. The old PCRE matcher used recursive calls for backtracking, so its recursion limit was reduced from 2047 to 1750 after stack crashes in #6819. The PCRE2 conversion in #12575 accidentally reused 1750 as a matching-work limit, causing ordinary long queries that previously matched to fail. This patch removes the work-limit override while retaining the per-instance match context and existing JIT stack behavior. PCRE2's normal work default is 10 million, allowing more worst-case CPU time per match while still bounding excessive backtracking. Its depth and heap limits remain intact. Since PCRE2 10.30, interpreter backtracking frames reside on the heap; JIT ignores the depth limit and uses a separately bounded stack. The old stack-derived value therefore does not translate into a suitable matching-work budget. This patch adds long-query redirect and capture-preservation coverage and extends the excessive-backtracking input to exercise the default work limit. The original 3 KB lookahead case remains a non-redirecting crash guard from #5762; its failure predates the PCRE2 conversion. Independent rule-specific log assertions preserve both checks. Fixes: #13651 Reported-by: Vinith Bindiganavale Co-authored-by: Codex Astra Medium (cherry picked from commit 7ed34a3d3c57aa46b721b0a5a578833ab755a3f4) --- plugins/esi/lib/IncludeUrlValidator.cc | 1 - plugins/regex_remap/regex_remap.cc | 6 +- .../pluginTest/regex_remap/long_query.conf | 20 ++ .../regex_remap/regex_remap.test.py | 30 ++- .../regex_remap_long_query.test.py | 19 ++ .../regex_remap/replay/long_query.replay.yaml | 230 ++++++++++++++++++ .../regex_remap/replay/yts-2819.replay.json | 2 +- 7 files changed, 292 insertions(+), 16 deletions(-) create mode 100644 tests/gold_tests/pluginTest/regex_remap/long_query.conf create mode 100644 tests/gold_tests/pluginTest/regex_remap/regex_remap_long_query.test.py create mode 100644 tests/gold_tests/pluginTest/regex_remap/replay/long_query.replay.yaml diff --git a/plugins/esi/lib/IncludeUrlValidator.cc b/plugins/esi/lib/IncludeUrlValidator.cc index 5bb9c0c51fe..33834b06fc4 100644 --- a/plugins/esi/lib/IncludeUrlValidator.cc +++ b/plugins/esi/lib/IncludeUrlValidator.cc @@ -39,7 +39,6 @@ namespace // Backtracking limit for the allowlist match. PCRE2 stops and reports // PCRE2_ERROR_MATCHLIMIT once this many match steps are taken, bounding // worst-case CPU per validation against attacker-influenced hostnames. - // Matches the value used by the regex_remap plugin. constexpr uint32_t ALLOW_REGEX_MATCH_LIMIT = 1750; bool diff --git a/plugins/regex_remap/regex_remap.cc b/plugins/regex_remap/regex_remap.cc index 86b2b2d2ba7..6b2d20d1636 100644 --- a/plugins/regex_remap/regex_remap.cc +++ b/plugins/regex_remap/regex_remap.cc @@ -55,9 +55,8 @@ static const char *PLUGIN_NAME = "regex_remap"; // Constants -static const int MATCHCOUNT = 15; // We support $0 - $9 x2 ints, and this needs to be 1.5x that -static const int MAX_SUBS = 32; // No more than 32 substitution variables in the subst string -static const int32_t REGEX_MATCH_LIMIT = 1750; // POOMA - also dependent on actual stack size. Crashes with previous value of 2047 +static const int MATCHCOUNT = 15; // We support $0 - $9 x2 ints, and this needs to be 1.5x that +static const int MAX_SUBS = 32; // No more than 32 substitution variables in the subst string // Substitutions other than regex matches enum ExtraSubstitutions { @@ -917,7 +916,6 @@ TSRemapNewInstance(int argc, char *argv[], void **ih, char * /* errbuf ATS_UNUSE if (!ri->rule_set) { return TS_ERROR; } - ri->match_context.set_match_limit(REGEX_MATCH_LIMIT); if (ri->profile) { ri->rule_hits.resize(ri->rule_set->rules().size()); } diff --git a/tests/gold_tests/pluginTest/regex_remap/long_query.conf b/tests/gold_tests/pluginTest/regex_remap/long_query.conf new file mode 100644 index 00000000000..d151b5b2541 --- /dev/null +++ b/tests/gold_tests/pluginTest/regex_remap/long_query.conf @@ -0,0 +1,20 @@ +# 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. + +# Match a token within the query and preserve both capture groups. +^/cms(\?.*)TOKEN(.*)$ https://redirect.example/cms$1TOKEN$2 @status=302 +# Make a skipped rule observable without contacting an origin. +^/cms.*$ https://fallback.example/ @status=307 diff --git a/tests/gold_tests/pluginTest/regex_remap/regex_remap.test.py b/tests/gold_tests/pluginTest/regex_remap/regex_remap.test.py index 91d4cb7ab3a..c6c30830127 100644 --- a/tests/gold_tests/pluginTest/regex_remap/regex_remap.test.py +++ b/tests/gold_tests/pluginTest/regex_remap/regex_remap.test.py @@ -45,6 +45,13 @@ # Define ATS and configure ts = Test.MakeATSProcess("ts", enable_cache=False) +# These two rules deliberately exercise resource limits. Replace the blanket +# error exclusion once, then append independent checks for each rule below. +ts.Disk.diags_log.Content = Testers.ExcludesExpression( + r'ERROR: (?!\[regex_remap\] Bad regular expression result ' + r'(?:-(?:46|47|53|63) .* from "\^/alpha/bravo/|-47 .* from "\^/match_limit/))', + "Only the deliberate resource-limit errors are allowed") + testName = "regex_remap" regex_remap_conf_path = os.path.join(ts.Variables.CONFIGDIR, 'regex_remap.conf') @@ -116,26 +123,29 @@ tr.Processes.Default.Streams.stdout = "gold/regex_remap_simple.gold" tr.StillRunningAfter = ts -# 3 Test - Match limit test 0 -tr = Test.AddTestRun("match limit 0") +# 3 Test - Preserve the original crash guard from #5762. This request must +# survive resource exhaustion without redirecting, regardless of which matching +# resource limit is reached (JIT stack, match work, depth, or heap). +tr = Test.AddTestRun("resource exhaustion does not crash ATS") creq = replay_txns[1]['client-request'] -tr.MakeCurlCommand(curl_and_args + \ - '--header "uuid: {}" '.format(creq["headers"]["fields"][1][1]) + '"{}"'.format(creq["url"]), ts=ts) +tr.MakeCurlCommand(curl_and_args + f"--header 'uuid: {creq['headers']['fields'][1][1]}' '{creq['url']}'", ts=ts) tr.Processes.Default.ReturnCode = 0 tr.Processes.Default.Streams.stdout = "gold/regex_remap_crash.gold" -ts.Disk.diags_log.Content = Testers.ContainsExpression( - 'ERROR: .regex_remap. Bad regular expression result -47', "Match limit exceeded") +ts.Disk.diags_log.Content += Testers.ContainsExpression( + r'ERROR: \[regex_remap\] Bad regular expression result -(?:46|47|53|63).*"\^/alpha/bravo/', + "The crash-guard rule must report resource exhaustion") tr.StillRunningAfter = ts -# 4 Test - Match limit test 1 -tr = Test.AddTestRun("match limit 1") +# 4 Test - The nested quantifiers must exceed PCRE2's default matching-work limit. +tr = Test.AddTestRun("excessive backtracking reaches the match limit") creq = replay_txns[2]['client-request'] tr.MakeCurlCommand(curl_and_args + \ '--header "uuid: {}" '.format(creq["headers"]["fields"][1][1]) + '"{}"'.format(creq["url"]), ts=ts) tr.Processes.Default.ReturnCode = 0 tr.Processes.Default.Streams.stdout = "gold/regex_remap_crash.gold" -ts.Disk.diags_log.Content = Testers.ContainsExpression( - 'ERROR: .regex_remap. Bad regular expression result -47', "Match limit exceeded") +ts.Disk.diags_log.Content += Testers.ContainsExpression( + r'ERROR: \[regex_remap\] Bad regular expression result -47.*\^/match_limit/', + "The excessive-backtracking rule must reach the match limit") tr.StillRunningAfter = ts diff --git a/tests/gold_tests/pluginTest/regex_remap/regex_remap_long_query.test.py b/tests/gold_tests/pluginTest/regex_remap/regex_remap_long_query.test.py new file mode 100644 index 00000000000..b430530da4e --- /dev/null +++ b/tests/gold_tests/pluginTest/regex_remap/regex_remap_long_query.test.py @@ -0,0 +1,19 @@ +# 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. + +Test.Summary = 'Verify regex_remap redirects with long query strings.' +Test.SkipUnless(Condition.PluginExists('regex_remap.so')) +Test.ATSReplayTest(replay_file='replay/long_query.replay.yaml') diff --git a/tests/gold_tests/pluginTest/regex_remap/replay/long_query.replay.yaml b/tests/gold_tests/pluginTest/regex_remap/replay/long_query.replay.yaml new file mode 100644 index 00000000000..2f6092ae426 --- /dev/null +++ b/tests/gold_tests/pluginTest/regex_remap/replay/long_query.replay.yaml @@ -0,0 +1,230 @@ +# 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. + +meta: + version: "1.0" + +autest: + description: 'Valid long query strings must not exhaust the match-work limit' + server: + name: 'server' + client: + name: 'client' + ats: + name: 'ts' + copy_to_config_dir: + - 'long_query.conf' + remap_config: + - from: 'http://example.com/' + to: 'http://127.0.0.1:{SERVER_HTTP_PORT}/' + plugins: + - name: 'regex_remap.so' + args: ['long_query.conf'] + log_validation: + diags_log: + excludes: + - expression: 'Bad regular expression result' + description: 'Simple query matching must not report any regex error' + +sessions: +- transactions: + - client-request: + method: GET + version: "1.1" + url: "/cms?partner=TOKEN&x=abc" + headers: + fields: + - [Host, example.com] + - [uuid, short-match] + proxy-request: + expect: absent + server-response: + status: 500 + headers: + fields: + - [Content-Length, "0"] + proxy-response: + status: 302 + headers: + fields: + - [Location, {value: "https://redirect.example/cms?partner=TOKEN&x=abc", as: equal}] + + # Regression for #13651: the 2021-byte subject exceeds the old 1750-work + # limit. Keep the 2000-byte suffix: shortening it can hide the regression. + # Only this case requires extensive backtracking; the other requests are + # controls for token placement and absence, not additional stress cases. + - client-request: + method: GET + version: "1.1" + url: "/cms?partner=TOKEN&x=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaa" + headers: + fields: + - [Host, example.com] + - [uuid, long-match] + proxy-request: + expect: absent + server-response: + status: 500 + headers: + fields: + - [Content-Length, "0"] + proxy-response: + status: 302 + headers: + fields: + - [Location, {value: "https://redirect.example/cms?partner=TOKEN&x=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", as: equal}] + + - client-request: + method: GET + version: "1.1" + url: "/cms?x=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaa&partner=TOKEN" + headers: + fields: + - [Host, example.com] + - [uuid, token-at-end] + proxy-request: + expect: absent + server-response: + status: 500 + headers: + fields: + - [Content-Length, "0"] + proxy-response: + status: 302 + headers: + fields: + - [Location, {value: "https://redirect.example/cms?x=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa&partner=TOKEN", as: equal}] + + - client-request: + method: GET + version: "1.1" + url: "/cms?partner=OTHER&x=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaa" + headers: + fields: + - [Host, example.com] + - [uuid, no-token] + proxy-request: + expect: absent + server-response: + status: 500 + headers: + fields: + - [Content-Length, "0"] + proxy-response: + status: 307 + headers: + fields: + - [Location, {value: "https://fallback.example/", as: equal}] diff --git a/tests/gold_tests/pluginTest/regex_remap/replay/yts-2819.replay.json b/tests/gold_tests/pluginTest/regex_remap/replay/yts-2819.replay.json index 4361a9800ff..d492eb0ba9c 100644 --- a/tests/gold_tests/pluginTest/regex_remap/replay/yts-2819.replay.json +++ b/tests/gold_tests/pluginTest/regex_remap/replay/yts-2819.replay.json @@ -163,7 +163,7 @@ "version": "1.1", "scheme": "http", "method": "GET", - "url": "http://example.one/match_limit/aaaaaaaaaaaaaaaaaaaf", + "url": "http://example.one/match_limit/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaf", "headers": { "fields": [ [