Argparser variable arg option parsing fix/improvement. - #13570
Conversation
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: apache#13569
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.
|
[approve ci debian] |
|
[approve ci] |
There was a problem hiding this comment.
Pull request overview
This PR fixes two long-standing ts::ArgParser option-parsing defects that impacted traffic_ctl config reload (and other variable-argument options): variable-argument options no longer swallow subsequent options on the same command, -- now terminates option recognition so dash-prefixed values remain expressible, and --option=value parsing no longer truncates values containing embedded =.
Changes:
- Update
ArgParservariable-argument handling to stop consuming at the next registered option (and honor--), and fix--option=valuevalue extraction to split on the first=. - Remove the
-D“must be last” workaround behavior intraffic_ctl, and make empty-D/-dinvocations explicit errors instead of silently degrading to full reloads. - Add unit + gold tests covering the corrected parsing behaviors and update
traffic_ctldocumentation accordingly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/tscore/ArgParser.cc |
Adjust variable-arg option collection logic, add registered-option detection, and fix --opt=... value splitting. |
include/tscore/ArgParser.h |
Declare new Command helpers used for the updated parsing behavior. |
src/tscore/unit_tests/test_ArgParser.cc |
Add unit coverage for variable-arg stop-at-next-option, -- handling, and embedded-= values. |
src/traffic_ctl/CtrlCommands.cc |
Remove old -D ordering guard; error out on empty -d / -D to prevent silent full reload behavior. |
tests/gold_tests/jsonrpc/config_reload_directive_cli.test.py |
Add end-to-end CLI parsing coverage for traffic_ctl config reload -D / -d interactions and --. |
doc/appendices/command-line/traffic_ctl.en.rst |
Update operator guidance for -D/-d, including -- usage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
[approve ci centos] |
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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/tscore/ArgParser.cc:716
append_option_data()indexesargs[i][0]/args[i][1]when checking for--option=value. If an argument is an empty string or a single "-" (possible for positional args / values), this is out-of-bounds and undefined behavior. Consider switching to a prefix check that doesn’t index into the string.
if (args[i][0] == '-' && args[i][1] == '-' && args[i].find('=') != std::string::npos) {
|
@brbzull0 does this need to be back ported to 10.2.x. If so, please mark the project. |
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.
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 '-'.
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.
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.
c21e8c8 to
68e5708
Compare
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.
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.
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.
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.
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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
src/traffic_ctl/CtrlCommands.cc:601
- Similarly,
-D ""yields a single empty directive token, which bypasses the size()==0 check and then gets dropped byif (dir.empty()) continue;, leaving the request effectively unscoped. If the intent is "-D must specify at least one directive", the check should treat an all-empty value list as an error too (consistent with--directive=being rejected).
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;
| // 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; | ||
| } |
There was a problem hiding this comment.
Confirmed, and fixed in f1f3bbd.
--data is registered MORE_THAN_ZERO_ARG_N, and the variable-arg branch of handle_args collects an empty token as a value — it is not --, and is_registered_option("") is false — so -d "" yields size() == 1 and passes the size() == 0 guard. The parse loop then skips the empty token, leaving configs empty, and config_reload() reloads every handler. Same for -D "" on the other line you flagged.
I went further than the minimum and refused an empty value even when a real one is present, rather than just skipping it. -d "$IP_ALLOW_YAML" -d "$SNI_YAML" with SNI_YAML unset would otherwise reload ip_allow and quietly leave sni out — the same silent-degradation failure mode, one size smaller. The two cases also report differently now, because "requires content: @file, @- or a YAML string" is misleading when the operator did pass a value and it was empty from an unset variable:
$ traffic_ctl config reload -d
Error: --data (-d) requires content: @file, @- or a YAML string
$ traffic_ctl config reload -d ""
Error: --data (-d) received an empty value, so its config would be left out
The fix is in CtrlCommands.cc rather than in handle_args, because that function is shared with command positionals (ArgParser.cc:866), so rejecting empty tokens there would also change config get "", host down "" and plugin msg TAG "" — and plugin msg documents its DATA argument as optional.
Three autest cases added, for -d "", -D "", and an empty token next to a real one. Reverting only the CtrlCommands.cc change makes all three fail and leaves the ten existing runs in that file passing, so they detect the bug rather than passing vacuously.
One note for anyone adding a case like these: autest's own isShellCommand indexes arg[0] on every argument and raises IndexError on an empty one, so a test that passes an empty argument needs ForceUseShell = True.
An option can be present and still carry no content. `-d ""`, or `-d $VAR` with VAR unset, yields an empty token that passed the size() check and was then skipped by the parse loop, so a scoped reload silently became a full reload of every handler. An empty value is now refused even alongside a real one, so a reload never covers fewer configs than were asked for, and it is reported separately from the bare option so the operator is told which of the two mistakes they made.
There was a problem hiding this comment.
🟢 Approval recommended
The parser behavior changes are well-scoped and are backed by comprehensive unit and gold-test coverage that exercises the corrected edge cases.
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
[approve ci clang-analyzer] |
|
[approve ci autest 3] |
cmcfarlen
left a comment
There was a problem hiding this comment.
Read through the parser changes against the existing traffic_ctl call sites. The semantics hold up: the find_last_of to find_first_of fix, the accumulate-on-repeat guard keyed off the new Arguments::has() (with append() still overwriting for the non-accumulating arities, so fixed arity keeps last-one-wins), the check_map counting that makes the two --cold spellings share one budget, and is_variable_arg_num() replacing the sentinel comparisons. I looked for a remaining direct sentinel comparison that would treat AT_MOST_ONE_ARG_N as a literal count of 4294967293 and did not find one.
I also checked the pre-existing bare --cold runs in traffic_ctl_cold_config.test.py (the ones at lines 42, 50 and 66 that write the option last). Under AT_MOST_ONE_ARG_N the option is at the end of the line, so no value is taken and it falls back to TS_RECORD_YAML, exactly as under MORE_THAN_ZERO_ARG_N. And the new config set -c FILE record value run writes a fresh new_records3.yaml whose content matches the cold_test5.gold that the existing new_records.yaml and new_records2.yaml runs already assert.
Approving on the strength of that and the restarted AuTest job. Four notes, none of them blocking.
The parse() guard is untested, and the reason is a real latent defect. The ret = Arguments{} reset on the default command retry has no regression test, because set_default() leaked state into the rest of the binary. The root cause is worth its own issue: default_command, parser_program_name and usage_return_code are file scope globals at ArgParser.cc:36-41 with external linkage — not static, not in an anonymous namespace — so they are exported symbols of libtscore, and default_command is never cleared. Making them members of ArgParser, or at minimum giving them internal linkage plus a reset hook for tests, would fix the symbol leak and make this retry path testable. Commit 312821b already diagnoses it accurately; it just deserves a follow-up rather than staying an untestable corner.
The empty value policy is split across two layers. The parser now rejects -c "" for at most one arity, while -D "" and -d "" parse fine and are caught by has_empty_value() in CtrlCommands.cc. Two layers enforcing the same rule for two different arities. Having handle_args reject an empty value token uniformly would centralize it, though it would affect other consumers, so this may well be deliberate.
Small dead branch. Once the new guard returns on any empty directive value, the loop below can no longer see one, so its if (dir.empty()) { continue; } is unreachable.
The -- rule is more uniform than the docs make it sound, and that is worth writing down rather than changing. It is one rule: option recognition goes off for the remainder of that option's value collection. For -D that reads as "the rest of the line" only because it keeps collecting to the end; for -c it escapes exactly one token; for a fixed arity it lasts until the values are filled. The unit tests pin all three, including the nice case where get -c -- -weird-name.yaml proxy.config.x yields both cold=-weird-name.yaml and get=[proxy.config.x]. Stating the general rule in the ArgParser developer guide would keep the three arities from reading as three separate behaviours.
One thing worth a comment in the code rather than a change: is_registered_option() consults only this command's options, which is correct because append_option_data() sweeps the whole remaining vector for the parent's options before recursing into subcommands. It does mean a variable arity option declared on a command that has subcommands would still swallow the subcommand name. Every variable arity option in traffic_ctl today is on a leaf command, reload and invoke, so nothing reaches it, but the next person to add one will want to know.
The test coverage here is the part I would call out as exemplary: both orderings, both spellings, mixing the two, repetition, and --, plus autests for the operator-visible behaviour. That is what made this reviewable.
LGTM, assuming the restarted AuTest comes back green.
* 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: apache#13569 * Drop the -D placement workaround from traffic_ctl The guard rejecting directive values that start with '-' existed only because variable-argument parsing swallowed any option written after -D. That no longer happens, so the guard can only fire for a value the caller passed deliberately, and its advice to place -D last is now wrong. A malformed value is reported by the directive format check instead. Require values for both -D and -d. Supplying either with no values built a request identical to a plain reload, silently widening a scoped reload to every handler. Also document that -D may appear anywhere among the options and can be combined with -d, which the previous note said was impossible. * Add an at-most-one-argument arity to ArgParser An option whose value is optional had to be declared as taking zero or more values, the only variable arity available, so it also consumed the positional arguments of its own command. That is why traffic_ctl rejected "config get -c FILE RECORD" with an error naming get, and why --cold only worked written last or as --cold=FILE. Add AT_MOST_ONE_ARG_N, the equivalent of nargs='?' in Python argparse which this parser imitates, and declare --cold with it. The count check for the --option=value form now asks is_variable_arg_num() rather than comparing against the sentinels, so a third sentinel is not mistaken for a literal argument count. * Correct the variable-arg comment in handle_args The comment claimed the command's positional arguments were left in place, but collection only stops at a token naming another option, so positional tokens are still taken as values. Say so, and point at AT_MOST_ONE_ARG_N for an option whose value is optional. * Stop fixed-arity options consuming later options An option expecting a fixed number of values took whatever token followed it, so "traffic_ctl server debug enable -t -a" set the debug tags to the literal "-a" and wrote that to the running configuration. Apply the rule the variable-arity path already follows: a token naming another option of the same command is not a value, so the missing value is reported, and "--" still passes a value that starts with '-'. * Reject a repeated at-most-one-argument option is_variable_arg_num() exempts AT_MOST_ONE_ARG_N from the count check for the --option=value form, so --cold=a --cold=b silently kept the first and dropped the second, where the fixed arity equivalent is a usage error. * Correct the bare -c example in the traffic_ctl docs A bare -c before a record takes the record as the file name, which leaves config get with no records and exits with a usage error rather than reading a file named for the record. * Count both --cold spellings against the at-most-one limit The repetition check only counted the --option=value form, so "-c a -c b" silently kept the last file name, and mixing the two spellings put two values in an option that permits one. Fixed arity options keep their existing last-one-wins behaviour, which is a separate concern. * Say what "--" does to the options that follow it Option recognition stays off for the rest of a variable-length value list, so every later token becomes a value and any option written afterwards is swallowed. Neither the guide nor the traffic_ctl page said so, which made "--" look safe to use before other options. * Accumulate a repeated variable argument option Each occurrence reset the entry rather than adding to it, so "-D a -D b" kept only b and "-d f1 -d f2" reloaded only f2, silently dropping inline content the documentation says is merged. The --option=value form has always accumulated, so the two spellings of one option disagreed. A fixed arity option keeps its last-one-wins behaviour. The default command retry starts from a clean Arguments, since a global option is otherwise parsed twice and would collect its values twice. * Reject an empty value for an at-most-one-argument option An empty token was taken as the value, and traffic_ctl reads an empty --cold file name as a request for the default file, so a set whose file name came from an unset variable wrote to the live records.yaml and exited zero. The --option=value spelling already refuses an empty value, so the two spellings disagreed. The error names the option as it was written, matching that spelling. * Report the set symptom in the bare -c documentation A bare -c before a record takes the record as the file name, and the docs quoted only the error config get produces. config set is left short of its own arguments and reports a different one, so an operator who hit that did not find their message. * Drop the default command test that leaked into other tests set_default() writes a file scope default_command that nothing clears, so the test left every later parse in the same binary inserting "info" into its arguments. The mutex group and option dependency tests share that binary and failed on four platforms, while the file passed on its own. The retry path the test covered keeps its guard in parse(); it cannot be exercised without changing global state for the rest of the run. (cherry picked from commit d0fb283)
|
Cherry-picked to the 10.2.x branch as 028568e for the 10.2.1 release. |
ArgParser options taking a variable number of values collected every remaining token, so
an option written after one was swallowed and never parsed:
config reload -D ip_allow.id=foo -t mytoklost the token, while the reverse order worked. Collection nowstops at a token naming another declared option of the same command, and
--ends optionrecognition so a dash-prefixed value stays expressible.
Also,
--option=valuetook the name up to the first=but the value from the last, so--directive=ip_allow.id=fooarrived asfoo.The second commit drops the
-D-must-be-last workaround in traffic_ctl, makes an empty-D/-dan error rather than a silent full reload, and corrects the docs.The third commit adds the arity that was missing entirely,
AT_MOST_ONE_ARG_N, theequivalent of
nargs='?'in the argparse this parser imitates. An option whose value isoptional had to be declared as taking zero or more values, so it also ate the positional
arguments of its own command:
config get -c FILE RECORDfailed with an error namingget, and--coldonly worked written last or as--cold=FILE. That has been thebehaviour since 10.0.
--coldis now declared with the new arity, and the count check forthe
--option=valueform asksis_variable_arg_num()instead of comparing against thesentinels.
Fixes: #13569