brbzull0 commented on code in PR #13570:
URL: https://github.com/apache/trafficserver/pull/13570#discussion_r3914230940
##########
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;
+ }
Review Comment:
Confirmed, and fixed in f1f3bbd6da.
`--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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
