From 298b5fd72781f409b6297dda8426ace9c5f15b0f Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Wed, 19 Aug 2026 12:36:55 +0200 Subject: [PATCH 1/2] 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 --- include/tscore/ArgParser.h | 4 ++ src/tscore/ArgParser.cc | 51 ++++++++++++++---- src/tscore/unit_tests/test_ArgParser.cc | 71 +++++++++++++++++++++++++ 3 files changed, 117 insertions(+), 9 deletions(-) diff --git a/include/tscore/ArgParser.h b/include/tscore/ArgParser.h index fdb6086eba0..57a2c0b6541 100644 --- a/include/tscore/ArgParser.h +++ b/include/tscore/ArgParser.h @@ -222,6 +222,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/tscore/ArgParser.cc b/src/tscore/ArgParser.cc index 90c3c2f5e43..9c49f51c8e4 100644 --- a/src/tscore/ArgParser.cc +++ b/src/tscore/ArgParser.cc @@ -518,23 +518,56 @@ 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); // 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; - } - for (unsigned j = index + 1; j < args.size(); j++) { + // Variable number of arguments. Stop collecting at a token that names another option + // of this command, so that following options and this command's own positional + // arguments are left in place for the caller. 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; + } + if (arg_num == MORE_THAN_ONE_ARG_N && collected == 0) { + return "at least one argument expected by " + name; } - args.erase(args.begin() + index, args.end()); + args.erase(args.begin() + index, args.begin() + j); + index -= 1; return ""; } // finite number of argument handling @@ -658,7 +691,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 + "'"); } diff --git a/src/tscore/unit_tests/test_ArgParser.cc b/src/tscore/unit_tests/test_ArgParser.cc index 0a32502e964..a3117e57e57 100644 --- a/src/tscore/unit_tests/test_ArgParser.cc +++ b/src/tscore/unit_tests/test_ArgParser.cc @@ -217,3 +217,74 @@ 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"); +} From 2c5f30d63b714ddd85c01361390f152e22ae0feb Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Wed, 19 Aug 2026 13:06:41 +0200 Subject: [PATCH 2/2] 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. --- .../command-line/traffic_ctl.en.rst | 16 ++- src/traffic_ctl/CtrlCommands.cc | 20 +-- .../config_reload_directive_cli.test.py | 119 ++++++++++++++++++ 3 files changed, 143 insertions(+), 12 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 67ba5801349..298cfe7193c 100644 --- a/doc/appendices/command-line/traffic_ctl.en.rst +++ b/doc/appendices/command-line/traffic_ctl.en.rst @@ -456,11 +456,17 @@ 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; every + token after ``--`` is taken as a value rather than an option. .. note:: diff --git a/src/traffic_ctl/CtrlCommands.cc b/src/traffic_ctl/CtrlCommands.cc index 88d29637ece..c4d539251bc 100644 --- a/src/traffic_ctl/CtrlCommands.cc +++ b/src/traffic_ctl/CtrlCommands.cc @@ -555,6 +555,14 @@ 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 && 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; + } + // Parse inline config data if provided (supports multiple -d arguments) YAML::Node configs; for (auto const &data_arg : data_args) { @@ -587,17 +595,15 @@ ConfigCommand::config_reload() // Parse --directive (-D) arguments into configs[key]["_reload"][directive] = value auto dir_args = get_parsed_arguments()->get("directive"); + if (dir_args && 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; + } 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); 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..b73efcb8562 --- /dev/null +++ b/tests/gold_tests/jsonrpc/config_reload_directive_cli.test.py @@ -0,0 +1,119 @@ +''' +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. 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