From 1a1f6cef8c222e6f2ca2e1b484fa3d3494d0308f Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Tue, 21 Apr 2026 14:02:10 +0200 Subject: [PATCH 1/3] 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. --- .../command-line/traffic_ctl.en.rst | 50 +++ .../config-reload-framework.en.rst | 82 +++++ include/mgmt/config/ConfigContext.h | 14 +- src/mgmt/config/ConfigContext.cc | 15 +- src/mgmt/config/ConfigRegistry.cc | 27 +- src/records/CMakeLists.txt | 1 + .../unit_tests/test_ReloadDirectives.cc | 318 ++++++++++++++++++ src/traffic_ctl/CtrlCommands.cc | 41 +++ src/traffic_ctl/traffic_ctl.cc | 2 + .../jsonrpc/config_reload_rpc.test.py | 94 ++++++ 10 files changed, 637 insertions(+), 7 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 dc560fe9cc7..2b18da43eda 100644 --- a/doc/appendices/command-line/traffic_ctl.en.rst +++ b/doc/appendices/command-line/traffic_ctl.en.rst @@ -414,6 +414,56 @@ 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`` and ``-d`` cannot be combined in the same invocation due to argument + parsing constraints. 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 5b115078940..d7de29e2027 100644 --- a/doc/developer-guide/config-reload-framework.en.rst +++ b/doc/developer-guide/config-reload-framework.en.rst @@ -266,8 +266,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" }, + "myconfig": [ ... actual content ... ] + } + } + } + + 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 (directives["id"]) { + std::string id = directives["id"].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 :option:`traffic_ctl config reload --directive` 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 0dbce853a1e..dde7aa85aa6 100644 --- a/include/mgmt/config/ConfigContext.h +++ b/include/mgmt/config/ConfigContext.h @@ -177,12 +177,24 @@ class ConfigContext /// @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. + /// @return copy of the directives YAML node (undefined if none were provided). + [[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; ///< for no content, this will just be empty + YAML::Node _reload_directives; ///< operational parameters from _reload key friend class ReloadCoordinator; friend class config::ConfigRegistry; diff --git a/src/mgmt/config/ConfigContext.cc b/src/mgmt/config/ConfigContext.cc index c23f3e27604..d6bbf1290d3 100644 --- a/src/mgmt/config/ConfigContext.cc +++ b/src/mgmt/config/ConfigContext.cc @@ -131,7 +131,8 @@ ConfigContext::add_dependent_ctx(std::string_view description) // 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 {}; @@ -149,6 +150,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..85cdfdd2f81 100644 --- a/src/mgmt/config/ConfigRegistry.cc +++ b/src/mgmt/config/ConfigRegistry.cc @@ -431,15 +431,15 @@ 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; 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_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()) { @@ -460,9 +460,26 @@ ConfigRegistry::execute_reload(const std::string &key) ctx.in_progress(); if (passed_config.IsDefined()) { - // Passed config mode: store YAML node directly for handler to use via supplied_yaml() 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 6412c6cee9c..ac93f681737 100644 --- a/src/records/CMakeLists.txt +++ b/src/records/CMakeLists.txt @@ -52,6 +52,7 @@ if(BUILD_TESTING) unit_tests/test_RecRegister.cc unit_tests/test_ConfigReloadTask.cc unit_tests/test_ConfigRegistry.cc + unit_tests/test_ReloadDirectives.cc unit_tests/test_RecDumpRecords.cc ) target_link_libraries(test_records PRIVATE records configmanager inkevent Catch2::Catch2 ts::tscore libswoc::libswoc) diff --git a/src/records/unit_tests/test_ReloadDirectives.cc b/src/records/unit_tests/test_ReloadDirectives.cc new file mode 100644 index 00000000000..cd7edbf1295 --- /dev/null +++ b/src/records/unit_tests/test_ReloadDirectives.cc @@ -0,0 +1,318 @@ +/** @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; + + // Default-constructed YAML::Node is Null (IsDefined()==true). + // The handler pattern checks specific keys, which are undefined: + auto directives = ctx.reload_directives(); + REQUIRE(directives.IsNull()); + 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(yaml.IsNull()); + 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 21700b3154d..0bdcdc9a0da 100644 --- a/src/traffic_ctl/CtrlCommands.cc +++ b/src/traffic_ctl/CtrlCommands.cc @@ -79,6 +79,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 @@ -533,6 +560,20 @@ 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; + } + 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 6fbfc7f01d9..d548a2a1ec7 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 587522d3a710e96294ca6fbe2874db227400b5ed Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Tue, 21 Apr 2026 14:40:11 +0200 Subject: [PATCH 2/3] Fix docs --- doc/developer-guide/config-reload-framework.en.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/developer-guide/config-reload-framework.en.rst b/doc/developer-guide/config-reload-framework.en.rst index d7de29e2027..287860adaef 100644 --- a/doc/developer-guide/config-reload-framework.en.rst +++ b/doc/developer-guide/config-reload-framework.en.rst @@ -285,7 +285,7 @@ reload_directives() "configs": { "myconfig": { "_reload": { "id": "foo", "dry_run": "true" }, - "myconfig": [ ... actual content ... ] + "rules": ["rule1", "rule2"] } } } @@ -340,7 +340,7 @@ reload_directives() $ traffic_ctl config reload -D myconfig.id=foo - See :option:`traffic_ctl config reload --directive` for details. + See the ``--directive`` option in :ref:`traffic_ctl ` for details. .. note:: From ed6fd54fd6bcc3284e1d4a0b7134ec3066d0f90d Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Wed, 22 Apr 2026 12:45:37 +0200 Subject: [PATCH 3/3] Fixes after review --- doc/appendices/command-line/traffic_ctl.en.rst | 8 +++++--- doc/developer-guide/config-reload-framework.en.rst | 6 +++--- include/mgmt/config/ConfigContext.h | 12 ++++++++---- src/mgmt/config/ConfigRegistry.cc | 8 +++++--- src/records/unit_tests/test_ReloadDirectives.cc | 11 ++++++----- src/traffic_ctl/CtrlCommands.cc | 7 +++++++ 6 files changed, 34 insertions(+), 18 deletions(-) diff --git a/doc/appendices/command-line/traffic_ctl.en.rst b/doc/appendices/command-line/traffic_ctl.en.rst index 2b18da43eda..198d7008122 100644 --- a/doc/appendices/command-line/traffic_ctl.en.rst +++ b/doc/appendices/command-line/traffic_ctl.en.rst @@ -454,9 +454,11 @@ Display the current value of a configuration record. .. note:: - ``-D`` and ``-d`` cannot be combined in the same invocation due to argument - parsing constraints. Use ``-d`` with full YAML when you need both directives - and inline content in a single reload request. + ``-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:: diff --git a/doc/developer-guide/config-reload-framework.en.rst b/doc/developer-guide/config-reload-framework.en.rst index 287860adaef..162f3f1038d 100644 --- a/doc/developer-guide/config-reload-framework.en.rst +++ b/doc/developer-guide/config-reload-framework.en.rst @@ -271,7 +271,7 @@ supplied_yaml() 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 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 @@ -307,8 +307,8 @@ reload_directives() ctx.in_progress(); if (auto directives = ctx.reload_directives()) { - if (directives["id"]) { - std::string id = directives["id"].as(); + 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; diff --git a/include/mgmt/config/ConfigContext.h b/include/mgmt/config/ConfigContext.h index dde7aa85aa6..2a4ee20733a 100644 --- a/include/mgmt/config/ConfigContext.h +++ b/include/mgmt/config/ConfigContext.h @@ -170,7 +170,7 @@ class ConfigContext [[nodiscard]] ConfigContext add_dependent_ctx(std::string_view description = ""); /// 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 @@ -182,7 +182,11 @@ class ConfigContext /// 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. - /// @return copy of the directives YAML node (undefined if none were provided). + /// 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: @@ -193,8 +197,8 @@ class ConfigContext 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 _reload_directives; ///< operational parameters from _reload key + 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/ConfigRegistry.cc b/src/mgmt/config/ConfigRegistry.cc index 85cdfdd2f81..776fa265d2f 100644 --- a/src/mgmt/config/ConfigRegistry.cc +++ b/src/mgmt/config/ConfigRegistry.cc @@ -432,12 +432,14 @@ ConfigRegistry::execute_reload(const std::string &key) Dbg(dbg_ctl, "Executing reload for config '%s'", key.c_str()); YAML::Node passed_config; + bool has_passed_config{false}; Entry entry_copy; { std::unique_lock lock(_mutex); if (auto pc_it = _passed_configs.find(key); pc_it != _passed_configs.end()) { - passed_config = pc_it->second; + 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()); } @@ -455,11 +457,11 @@ 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()) { + if (has_passed_config) { Dbg(dbg_ctl, "Config '%s' reloading from rpc-supplied content", entry_copy.key.c_str()); // Extract _reload directives before passing content to the handler. diff --git a/src/records/unit_tests/test_ReloadDirectives.cc b/src/records/unit_tests/test_ReloadDirectives.cc index cd7edbf1295..5b90b607081 100644 --- a/src/records/unit_tests/test_ReloadDirectives.cc +++ b/src/records/unit_tests/test_ReloadDirectives.cc @@ -166,10 +166,10 @@ TEST_CASE("ConfigContext: reload_directives on default context has no keys", "[c { ConfigContext ctx; - // Default-constructed YAML::Node is Null (IsDefined()==true). - // The handler pattern checks specific keys, which are undefined: - auto directives = ctx.reload_directives(); - REQUIRE(directives.IsNull()); + // 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()); } @@ -178,7 +178,8 @@ TEST_CASE("ConfigContext: supplied_yaml on default context has no content", "[co ConfigContext ctx; auto yaml = ctx.supplied_yaml(); - REQUIRE(yaml.IsNull()); + REQUIRE_FALSE(yaml.IsDefined()); + REQUIRE_FALSE(yaml); REQUIRE_FALSE(yaml.IsMap()); REQUIRE_FALSE(yaml.IsSequence()); } diff --git a/src/traffic_ctl/CtrlCommands.cc b/src/traffic_ctl/CtrlCommands.cc index 0bdcdc9a0da..d3ce489a810 100644 --- a/src/traffic_ctl/CtrlCommands.cc +++ b/src/traffic_ctl/CtrlCommands.cc @@ -566,6 +566,13 @@ ConfigCommand::config_reload() 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);