Argparser variable arg option parsing fix/improvement.

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::
 

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

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);

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 + "'");
       }

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");
+}

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