From 298b5fd72781f409b6297dda8426ace9c5f15b0f Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Wed, 19 Aug 2026 12:36:55 +0200 Subject: [PATCH 01/14] 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 02/14] 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 From d8520e0e6103063f64f9dc367be01424a35f9311 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Mon, 24 Aug 2026 13:15:49 +0200 Subject: [PATCH 03/14] 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. --- .../command-line/traffic_ctl.en.rst | 19 +++++ .../internal-libraries/ArgParser.en.rst | 23 ++++++ include/tscore/ArgParser.h | 13 ++++ src/traffic_ctl/traffic_ctl.cc | 4 +- src/tscore/ArgParser.cc | 27 ++++++- src/tscore/unit_tests/test_ArgParser.cc | 70 +++++++++++++++++++ .../records/traffic_ctl_cold_config.test.py | 40 +++++++++++ 7 files changed, 193 insertions(+), 3 deletions(-) diff --git a/doc/appendices/command-line/traffic_ctl.en.rst b/doc/appendices/command-line/traffic_ctl.en.rst index 298cfe7193c..f0f2aeeea6c 100644 --- a/doc/appendices/command-line/traffic_ctl.en.rst +++ b/doc/appendices/command-line/traffic_ctl.en.rst @@ -569,6 +569,25 @@ 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 is ambiguous, because the record + name is taken as the file name: + + .. 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 # wrong, reads a file + # named for the record + 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..b780656d82e 100644 --- a/doc/developer-guide/internal-libraries/ArgParser.en.rst +++ b/doc/developer-guide/internal-libraries/ArgParser.en.rst @@ -104,6 +104,29 @@ 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 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. + 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 57a2c0b6541..c0a428a38c0 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; diff --git a/src/traffic_ctl/traffic_ctl.cc b/src/traffic_ctl/traffic_ctl.cc index 1bda6e8521d..c077b130934 100644 --- a/src/traffic_ctl/traffic_ctl.cc +++ b/src/traffic_ctl/traffic_ctl.cc @@ -118,7 +118,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) @@ -186,7 +186,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 9c49f51c8e4..d295b36ea29 100644 --- a/src/tscore/ArgParser.cc +++ b/src/tscore/ArgParser.cc @@ -418,6 +418,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 " ... "; } @@ -541,6 +543,29 @@ ArgParser::Command::handle_args(Arguments &ret, AP_StrVec &args, std::string con ArgumentData data; ret.append(name, data); // handle the args + 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]); + } + } + 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 that following options and this command's own positional @@ -754,7 +779,7 @@ 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) { + if (num != it.second && !is_variable_arg_num(num)) { help_message(std::to_string(_option_list.at(it.first).arg_num) + " arguments expected by " + it.first); } } diff --git a/src/tscore/unit_tests/test_ArgParser.cc b/src/tscore/unit_tests/test_ArgParser.cc index a3117e57e57..bba84323dca 100644 --- a/src/tscore/unit_tests/test_ArgParser.cc +++ b/src/tscore/unit_tests/test_ArgParser.cc @@ -288,3 +288,73 @@ TEST_CASE("Option value keeps embedded equal signs", "[parse]") 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"); +} 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..b9034b3c511 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,43 @@ 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' From 087d7b6506db01519275dfbad45f134292453963 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Wed, 26 Aug 2026 09:41:41 +0200 Subject: [PATCH 04/14] 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. --- src/tscore/ArgParser.cc | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/tscore/ArgParser.cc b/src/tscore/ArgParser.cc index d295b36ea29..495edf8ac0c 100644 --- a/src/tscore/ArgParser.cc +++ b/src/tscore/ArgParser.cc @@ -567,10 +567,11 @@ ArgParser::Command::handle_args(Arguments &ret, AP_StrVec &args, std::string con 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 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. + // 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}; From 5367a988fdac0f2a1195b865c6cca9cdc15e426d Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Wed, 26 Aug 2026 14:14:38 +0200 Subject: [PATCH 05/14] 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 '-'. --- .../internal-libraries/ArgParser.en.rst | 4 +++ src/tscore/ArgParser.cc | 25 ++++++++++++---- src/tscore/unit_tests/test_ArgParser.cc | 30 +++++++++++++++++++ .../traffic_ctl_server_debug.test.py | 18 +++++++++++ 4 files changed, 72 insertions(+), 5 deletions(-) diff --git a/doc/developer-guide/internal-libraries/ArgParser.en.rst b/doc/developer-guide/internal-libraries/ArgParser.en.rst index b780656d82e..0171c4a6908 100644 --- a/doc/developer-guide/internal-libraries/ArgParser.en.rst +++ b/doc/developer-guide/internal-libraries/ArgParser.en.rst @@ -123,6 +123,10 @@ This function call returns the new :class:`Option` instance. (0 is also number o 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. + 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. diff --git a/src/tscore/ArgParser.cc b/src/tscore/ArgParser.cc index 495edf8ac0c..b2c0b29b7b3 100644 --- a/src/tscore/ArgParser.cc +++ b/src/tscore/ArgParser.cc @@ -596,15 +596,30 @@ ArgParser::Command::handle_args(Arguments &ret, AP_StrVec &args, std::string con 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 ""; } diff --git a/src/tscore/unit_tests/test_ArgParser.cc b/src/tscore/unit_tests/test_ArgParser.cc index bba84323dca..dc10cc4e86b 100644 --- a/src/tscore/unit_tests/test_ArgParser.cc +++ b/src/tscore/unit_tests/test_ArgParser.cc @@ -358,3 +358,33 @@ TEST_CASE("An option taking at most one argument accepts no value at all", "[par 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); +} 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 c09f1aa0f81860ce21883cd8855e3888965c9163 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Fri, 28 Aug 2026 11:18:15 +0200 Subject: [PATCH 06/14] 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. --- src/tscore/ArgParser.cc | 12 +++++++++--- .../records/traffic_ctl_cold_config.test.py | 8 ++++++++ 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/tscore/ArgParser.cc b/src/tscore/ArgParser.cc index b2c0b29b7b3..a572b72f488 100644 --- a/src/tscore/ArgParser.cc +++ b/src/tscore/ArgParser.cc @@ -794,9 +794,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 && !is_variable_arg_num(num)) { - 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); } } } 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 b9034b3c511..aed15c53878 100644 --- a/tests/gold_tests/records/traffic_ctl_cold_config.test.py +++ b/tests/gold_tests/records/traffic_ctl_cold_config.test.py @@ -132,3 +132,11 @@ 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') From c94ce9145db3cffbb16b2a9b01b29dad65ee770b Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Fri, 28 Aug 2026 11:18:15 +0200 Subject: [PATCH 07/14] 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. --- doc/appendices/command-line/traffic_ctl.en.rst | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/doc/appendices/command-line/traffic_ctl.en.rst b/doc/appendices/command-line/traffic_ctl.en.rst index f0f2aeeea6c..e2169c1e1e2 100644 --- a/doc/appendices/command-line/traffic_ctl.en.rst +++ b/doc/appendices/command-line/traffic_ctl.en.rst @@ -579,14 +579,17 @@ Display the current value of a configuration record. $ 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 is ambiguous, because the record - name is taken as the file name: + explicit file. A bare ``-c`` followed by a record name takes the record as the file name, + which leaves the command with no records of its own and is reported as a usage error: .. 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 # wrong, reads a file - # named for the record + $ traffic_ctl config get -c proxy.config.diags.debug.enabled + Error: at least one argument expected by get + + ``-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. From 73d26466c53728d164ae853d505540e4b0d08690 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Fri, 28 Aug 2026 13:18:45 +0200 Subject: [PATCH 08/14] 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. --- src/tscore/ArgParser.cc | 5 +++++ .../records/traffic_ctl_cold_config.test.py | 16 ++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/src/tscore/ArgParser.cc b/src/tscore/ArgParser.cc index a572b72f488..090c48ae0e6 100644 --- a/src/tscore/ArgParser.cc +++ b/src/tscore/ArgParser.cc @@ -779,6 +779,11 @@ 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; + } // handle the arguments std::string err = handle_args(ret, args, cur_option.key, cur_option.arg_num, i); if (!err.empty()) { 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 aed15c53878..219a9a7b766 100644 --- a/tests/gold_tests/records/traffic_ctl_cold_config.test.py +++ b/tests/gold_tests/records/traffic_ctl_cold_config.test.py @@ -140,3 +140,19 @@ 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') From 68e57089ac46497267b195bf136e8951b9383908 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Fri, 28 Aug 2026 13:18:46 +0200 Subject: [PATCH 09/14] 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. --- doc/appendices/command-line/traffic_ctl.en.rst | 10 ++++++++-- .../internal-libraries/ArgParser.en.rst | 5 +++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/doc/appendices/command-line/traffic_ctl.en.rst b/doc/appendices/command-line/traffic_ctl.en.rst index e2169c1e1e2..fbe180995b0 100644 --- a/doc/appendices/command-line/traffic_ctl.en.rst +++ b/doc/appendices/command-line/traffic_ctl.en.rst @@ -465,8 +465,14 @@ Display the current value of a configuration record. $ 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. + 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:: diff --git a/doc/developer-guide/internal-libraries/ArgParser.en.rst b/doc/developer-guide/internal-libraries/ArgParser.en.rst index 0171c4a6908..c47801a98b5 100644 --- a/doc/developer-guide/internal-libraries/ArgParser.en.rst +++ b/doc/developer-guide/internal-libraries/ArgParser.en.rst @@ -131,6 +131,11 @@ This function call returns the new :class:`Option` instance. (0 is also number o 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 From 86a5a0baae6c5ccb52260791d37b76c26448d8cb Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Tue, 1 Sep 2026 14:46:38 +0200 Subject: [PATCH 10/14] 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. --- .../command-line/traffic_ctl.en.rst | 9 +- .../internal-libraries/ArgParser.en.rst | 6 + include/tscore/ArgParser.h | 6 + src/tscore/ArgParser.cc | 20 ++- src/tscore/unit_tests/test_ArgParser.cc | 130 ++++++++++++++++++ .../config_reload_directive_cli.test.py | 42 +++++- 6 files changed, 207 insertions(+), 6 deletions(-) diff --git a/doc/appendices/command-line/traffic_ctl.en.rst b/doc/appendices/command-line/traffic_ctl.en.rst index fbe180995b0..1064c4a4b2e 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 diff --git a/doc/developer-guide/internal-libraries/ArgParser.en.rst b/doc/developer-guide/internal-libraries/ArgParser.en.rst index c47801a98b5..2d3a8776313 100644 --- a/doc/developer-guide/internal-libraries/ArgParser.en.rst +++ b/doc/developer-guide/internal-libraries/ArgParser.en.rst @@ -127,6 +127,12 @@ This function call returns the new :class:`Option` instance. (0 is also number o 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. + 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. diff --git a/include/tscore/ArgParser.h b/include/tscore/ArgParser.h index c0a428a38c0..1b29504a83f 100644 --- a/include/tscore/ArgParser.h +++ b/include/tscore/ArgParser.h @@ -102,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 diff --git a/src/tscore/ArgParser.cc b/src/tscore/ArgParser.cc index 090c48ae0e6..65a2866bb6d 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); } }; @@ -540,8 +543,15 @@ ArgParser::Command::is_registered_option(std::string const &token) const 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 == AT_MOST_ONE_ARG_N) { // Zero or one value. A value is taken only when the following token does not name @@ -926,6 +936,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 dc10cc4e86b..d0474310813 100644 --- a/src/tscore/unit_tests/test_ArgParser.cc +++ b/src/tscore/unit_tests/test_ArgParser.cc @@ -388,3 +388,133 @@ TEST_CASE("An option taking a fixed number of arguments can be given a value sha 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 default command does not repeat the values of a global option", "[parse]") +{ + ts::ArgParser parser; + parser.add_global_usage("test_prog CMD [OPTIONS]"); + + // Mirrors traffic_layout, where "info" is the default command and --run-root is global. The + // first pass matches no command and is retried with the default inserted, so a global option + // is parsed twice and must not collect its value twice. + parser.add_option("--run-root", "", "runroot", "", 1); + parser.add_option("--tag", "", "tags", "", MORE_THAN_ZERO_ARG_N, ""); + parser.add_command("info", "show the layout").set_default(); + + const char *argv1[] = {"test_prog", "--run-root", "/tmp/rr", nullptr}; + ts::Arguments parsed = parser.parse(argv1); + REQUIRE(parsed.get("info") == true); + REQUIRE(parsed.get("run-root").size() == 1); + REQUIRE(parsed.get("run-root").value() == "/tmp/rr"); + + // An accumulating option is the case that would double, since it is the one that keeps what + // an earlier pass collected. + const char *argv2[] = {"test_prog", "--tag", "a", "b", nullptr}; + parsed = parser.parse(argv2); + REQUIRE(parsed.get("info") == true); + REQUIRE(parsed.get("tag").size() == 2); + REQUIRE(parsed.get("tag")[0] == "a"); + REQUIRE(parsed.get("tag")[1] == "b"); + + // Naming the command explicitly takes the same path once. + const char *argv3[] = {"test_prog", "info", "--run-root", "/tmp/rr", nullptr}; + parsed = parser.parse(argv3); + REQUIRE(parsed.get("run-root").size() == 1); +} + +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 index b73efcb8562..be9eceac777 100644 --- a/tests/gold_tests/jsonrpc/config_reload_directive_cli.test.py +++ b/tests/gold_tests/jsonrpc/config_reload_directive_cli.test.py @@ -5,9 +5,12 @@ 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. +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 @@ -117,3 +120,36 @@ 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 From 3652d5a60cc9fbd0529714cf22c6aeb9fb4cdee1 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Tue, 1 Sep 2026 14:47:37 +0200 Subject: [PATCH 11/14] 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. --- .../command-line/traffic_ctl.en.rst | 9 ++++++ .../internal-libraries/ArgParser.en.rst | 4 +++ src/tscore/ArgParser.cc | 5 ++++ .../records/traffic_ctl_cold_config.test.py | 28 +++++++++++++++++++ 4 files changed, 46 insertions(+) diff --git a/doc/appendices/command-line/traffic_ctl.en.rst b/doc/appendices/command-line/traffic_ctl.en.rst index 1064c4a4b2e..0e1c725be08 100644 --- a/doc/appendices/command-line/traffic_ctl.en.rst +++ b/doc/appendices/command-line/traffic_ctl.en.rst @@ -601,6 +601,15 @@ Display the current value of a configuration record. $ traffic_ctl config get -c proxy.config.diags.debug.enabled Error: at least one argument expected by get + 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. diff --git a/doc/developer-guide/internal-libraries/ArgParser.en.rst b/doc/developer-guide/internal-libraries/ArgParser.en.rst index 2d3a8776313..cdaf5cbe4f9 100644 --- a/doc/developer-guide/internal-libraries/ArgParser.en.rst +++ b/doc/developer-guide/internal-libraries/ArgParser.en.rst @@ -133,6 +133,10 @@ This function call returns the new :class:`Option` instance. (0 is also number o 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. diff --git a/src/tscore/ArgParser.cc b/src/tscore/ArgParser.cc index 65a2866bb6d..eff0c42e9ec 100644 --- a/src/tscore/ArgParser.cc +++ b/src/tscore/ArgParser.cc @@ -793,6 +793,11 @@ ArgParser::Command::append_option_data(Arguments &ret, AP_StrVec &args, int inde // 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); 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 219a9a7b766..dc0e9d0b91d 100644 --- a/tests/gold_tests/records/traffic_ctl_cold_config.test.py +++ b/tests/gold_tests/records/traffic_ctl_cold_config.test.py @@ -156,3 +156,31 @@ 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. + +# 13 +tr = Test.AddTestRun("An empty file name is reported rather than taken as the default file") +tr.Processes.Default.Command = '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 = '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 = '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') From 91bc06c5305157ecf69f0ea0dc2e5b812bb75467 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Tue, 1 Sep 2026 14:48:15 +0200 Subject: [PATCH 12/14] 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. --- doc/appendices/command-line/traffic_ctl.en.rst | 7 ++++++- tests/gold_tests/records/traffic_ctl_cold_config.test.py | 8 ++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/doc/appendices/command-line/traffic_ctl.en.rst b/doc/appendices/command-line/traffic_ctl.en.rst index 0e1c725be08..d1f108cb066 100644 --- a/doc/appendices/command-line/traffic_ctl.en.rst +++ b/doc/appendices/command-line/traffic_ctl.en.rst @@ -593,7 +593,8 @@ Display the current value of a configuration record. 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 with no records of its own and is reported as a usage error: + which leaves the command short of its own arguments. Each command reports this in terms of + what it was left without: .. code-block:: bash @@ -601,6 +602,10 @@ Display the current value of a configuration record. $ 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: 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 dc0e9d0b91d..82b1a644fb2 100644 --- a/tests/gold_tests/records/traffic_ctl_cold_config.test.py +++ b/tests/gold_tests/records/traffic_ctl_cold_config.test.py @@ -184,3 +184,11 @@ 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( + '2 argument(s) expected by set', 'set must report what it was left without, as the docs show') From 312821baba59683f93704003eff64a30deaf8ecd Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Tue, 1 Sep 2026 15:09:41 +0200 Subject: [PATCH 13/14] 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. --- src/tscore/unit_tests/test_ArgParser.cc | 33 ------------------------- 1 file changed, 33 deletions(-) diff --git a/src/tscore/unit_tests/test_ArgParser.cc b/src/tscore/unit_tests/test_ArgParser.cc index d0474310813..8803fa2eff1 100644 --- a/src/tscore/unit_tests/test_ArgParser.cc +++ b/src/tscore/unit_tests/test_ArgParser.cc @@ -470,39 +470,6 @@ TEST_CASE("A repeated option requiring at least one argument accumulates too", " REQUIRE(parsed.get("format").value() == "json"); } -TEST_CASE("A default command does not repeat the values of a global option", "[parse]") -{ - ts::ArgParser parser; - parser.add_global_usage("test_prog CMD [OPTIONS]"); - - // Mirrors traffic_layout, where "info" is the default command and --run-root is global. The - // first pass matches no command and is retried with the default inserted, so a global option - // is parsed twice and must not collect its value twice. - parser.add_option("--run-root", "", "runroot", "", 1); - parser.add_option("--tag", "", "tags", "", MORE_THAN_ZERO_ARG_N, ""); - parser.add_command("info", "show the layout").set_default(); - - const char *argv1[] = {"test_prog", "--run-root", "/tmp/rr", nullptr}; - ts::Arguments parsed = parser.parse(argv1); - REQUIRE(parsed.get("info") == true); - REQUIRE(parsed.get("run-root").size() == 1); - REQUIRE(parsed.get("run-root").value() == "/tmp/rr"); - - // An accumulating option is the case that would double, since it is the one that keeps what - // an earlier pass collected. - const char *argv2[] = {"test_prog", "--tag", "a", "b", nullptr}; - parsed = parser.parse(argv2); - REQUIRE(parsed.get("info") == true); - REQUIRE(parsed.get("tag").size() == 2); - REQUIRE(parsed.get("tag")[0] == "a"); - REQUIRE(parsed.get("tag")[1] == "b"); - - // Naming the command explicitly takes the same path once. - const char *argv3[] = {"test_prog", "info", "--run-root", "/tmp/rr", nullptr}; - parsed = parser.parse(argv3); - REQUIRE(parsed.get("run-root").size() == 1); -} - TEST_CASE("A repeated option taking a fixed number of arguments keeps the last value", "[parse]") { ts::ArgParser parser; From 0b6292635b7a1644e4c83c4b65a77fce4553add3 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Tue, 1 Sep 2026 15:55:12 +0200 Subject: [PATCH 14/14] Make the empty --cold value runs runnable under autest autest reads the first character of every argument it splits, so an empty argument written straight into Command raises IndexError before the process starts, which failed the whole file and skipped the runs after it. Pass those three through "sh -c" so the empty argument is produced by the shell instead. ContainsExpression is a regular expression, so "argument(s)" asked for "arguments" and never matched the message it was quoting. --- .../records/traffic_ctl_cold_config.test.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) 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 82b1a644fb2..f605a4deac0 100644 --- a/tests/gold_tests/records/traffic_ctl_cold_config.test.py +++ b/tests/gold_tests/records/traffic_ctl_cold_config.test.py @@ -160,10 +160,13 @@ # 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 = 'traffic_ctl config get -c "" proxy.config.diags.debug.tags' +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( @@ -171,7 +174,7 @@ # 14 tr = Test.AddTestRun("An empty file name is reported before anything is written") -tr.Processes.Default.Command = 'traffic_ctl config set -c "" proxy.config.cache.limits.http.max_alts 9' +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( @@ -179,7 +182,7 @@ # 15 tr = Test.AddTestRun("The long spelling of an empty file name is reported as written") -tr.Processes.Default.Command = 'traffic_ctl config get --cold "" proxy.config.diags.debug.tags' +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( @@ -191,4 +194,4 @@ tr.Processes.Default.ReturnCode = 64 # EX_USAGE - command line usage error tr.Processes.Default.Env = ts.Env tr.Processes.Default.Streams.All = Testers.ContainsExpression( - '2 argument(s) expected by set', 'set must report what it was left without, as the docs show') + r'2 argument\(s\) expected by set', 'set must report what it was left without, as the docs show')