There are two distinct defects in ArgParser's handling of options, both reproducible on master.
Bug 1: a variable-argument option consumes the options that follow it
Options declared with MORE_THAN_ZERO_ARG_N / MORE_THAN_ONE_ARG_N collect every
remaining token as a value, including tokens that name another option of the same
command. Those options are then never parsed.
$ traffic_ctl config reload -D ip_allow.id=foo -t mytok
Error: '-t' looks like a flag, not a directive. Place -D as the last option on the command line.
$ traffic_ctl config reload -D ip_allow.id=foo -m
Error: '-m' looks like a flag, not a directive. Place -D as the last option on the command line.
(The error text above comes from a guard added in #13110; without it the flag would
be silently swallowed.)
Reversing the order works, which is what makes this a bug rather than a design
constraint:
$ traffic_ctl config reload -m -D ip_allow.id=foo # fine
Only options registered on the same command are affected. Global options are
already gone by then, because Command::parse() runs the top-level
append_option_data() pass before recursing into subcommands, so
traffic_ctl config reload -D ip_allow.id=foo -f rpc parses correctly.
Root cause
handle_args() in src/tscore/ArgParser.cc:
for (unsigned j = index + 1; j < args.size(); j++) {
ret.append_arg(name, args[j]);
}
args.erase(args.begin() + index, args.end());
Nothing stops the collection at an option-like token, and the erase truncates to
args.end(), so append_option_data() continues iterating over a vector whose
remaining tokens have been removed.
For comparison, Python's argparse stops consuming nargs='*' / nargs='+'
values at the next token that matches a known option.
There is also no support for -- as an end-of-options marker, so there is no way
to pass a value that legitimately begins with -.
Bug 2: --option=value truncates values containing =
The name is taken up to the first = but the value from the last one:
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);
So any value with an embedded = is silently cut down to whatever follows the
final =:
$ traffic_ctl config reload --directive=ip_allow.id=foo
Error: Invalid directive format 'foo'. Expected: config_key.directive_key=value
--directive values are key=value pairs by definition, so the = form is
unusable for the option that needs it most. The fix is to use find_first_of('=')
for both.
Not a parser bug: variable-argument options versus positional arguments
For completeness, since it looks related and is a real usability problem, but needs
a different fix.
$ traffic_ctl config get -c proxy.config.diags.debug.enabled
Error: at least one argument expected by get
$ traffic_ctl config set -c proxy.config.diags.debug.enabled 1
Error: 2 argument(s) expected by set
Here --cold, -c is MORE_THAN_ZERO_ARG_N and the record name is genuinely
ambiguous — it could be the filename for -c or the positional for get. Python's
argparse resolves it the same way (option wins, then the positional is reported
missing), so stopping collection at option-like tokens does not help. The = form
already works today and is the correct spelling:
$ traffic_ctl config get --cold=records.yaml proxy.config.diags.debug.enabled
$ traffic_ctl config set --cold=records.yaml proxy.config.diags.debug.enabled 1
What is wrong here is the advertised usage: traffic_ctl config get --help shows
traffic_ctl config get [OPTIONS] RECORD [RECORD ...], an order that cannot work
for -c. Worth addressing separately, either by giving --cold an explicit
"zero or one argument" semantic or by fixing the usage strings and documentation.
Affected declarations
Variable-argument options, the ones that can lose following options, all in
src/traffic_ctl/traffic_ctl.cc:
--cold, -c on config get and config set
--data, -d and --directive, -D on config reload
--params, -p on rpc invoke
Variable-argument commands (config describe, config get, config match,
host up/down, metric get/describe/match, plugin msg,
storage offline, rpc file, ...) are not directly affected, because options are
stripped before a command's positional arguments are collected.
There are two distinct defects in
ArgParser's handling of options, both reproducible onmaster.Bug 1: a variable-argument option consumes the options that follow it
Options declared with
MORE_THAN_ZERO_ARG_N/MORE_THAN_ONE_ARG_Ncollect everyremaining token as a value, including tokens that name another option of the same
command. Those options are then never parsed.
(The error text above comes from a guard added in #13110; without it the flag would
be silently swallowed.)
Reversing the order works, which is what makes this a bug rather than a design
constraint:
$ traffic_ctl config reload -m -D ip_allow.id=foo # fineOnly options registered on the same command are affected. Global options are
already gone by then, because
Command::parse()runs the top-levelappend_option_data()pass before recursing into subcommands, sotraffic_ctl config reload -D ip_allow.id=foo -f rpcparses correctly.Root cause
handle_args()insrc/tscore/ArgParser.cc:Nothing stops the collection at an option-like token, and the erase truncates to
args.end(), soappend_option_data()continues iterating over a vector whoseremaining tokens have been removed.
For comparison, Python's
argparsestops consumingnargs='*'/nargs='+'values at the next token that matches a known option.
There is also no support for
--as an end-of-options marker, so there is no wayto pass a value that legitimately begins with
-.Bug 2:
--option=valuetruncates values containing=The name is taken up to the first
=but the value from the last one:So any value with an embedded
=is silently cut down to whatever follows thefinal
=:--directivevalues arekey=valuepairs by definition, so the=form isunusable for the option that needs it most. The fix is to use
find_first_of('=')for both.
Not a parser bug: variable-argument options versus positional arguments
For completeness, since it looks related and is a real usability problem, but needs
a different fix.
Here
--cold, -cisMORE_THAN_ZERO_ARG_Nand the record name is genuinelyambiguous — it could be the filename for
-cor the positional forget. Python'sargparseresolves it the same way (option wins, then the positional is reportedmissing), so stopping collection at option-like tokens does not help. The
=formalready works today and is the correct spelling:
What is wrong here is the advertised usage:
traffic_ctl config get --helpshowstraffic_ctl config get [OPTIONS] RECORD [RECORD ...], an order that cannot workfor
-c. Worth addressing separately, either by giving--coldan explicit"zero or one argument" semantic or by fixing the usage strings and documentation.
Affected declarations
Variable-argument options, the ones that can lose following options, all in
src/traffic_ctl/traffic_ctl.cc:--cold, -conconfig getandconfig set--data, -dand--directive, -Donconfig reload--params, -ponrpc invokeVariable-argument commands (
config describe,config get,config match,host up/down,metric get/describe/match,plugin msg,storage offline,rpc file, ...) are not directly affected, because options arestripped before a command's positional arguments are collected.