brbzull0 opened a new pull request, #13626:
URL: https://github.com/apache/trafficserver/pull/13626
# traffic_ctl: honor `-f json` for `plugin list`
## TL;DR
`traffic_ctl plugin list -f json` printed the human-readable table and no
JSON.
The flag was parsed, a printer was constructed, the server returned a correct
payload — the command just never consulted the printer on the success path.
Given a `plugin.yaml`:
```yaml
plugins:
- path: stats_over_http.so
load_order: 10
- path: xdebug.so
params:
- --enable=x-cache
- path: header_rewrite.so
enabled: false
```
both of these printed the same thing:
```console
$ traffic_ctl plugin list
source: plugin.yaml
# plugin load_order status
1 stats_over_http.so 10 loaded
2 xdebug.so -- loaded
3 header_rewrite.so -- disabled
$ traffic_ctl plugin list -f json
source: plugin.yaml
# plugin load_order status
1 stats_over_http.so 10 loaded
2 xdebug.so -- loaded
3 header_rewrite.so -- disabled
$ traffic_ctl plugin list -f json | python3 -c 'import json,sys;
json.load(sys.stdin)'
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)
```
Column 1, char 0 — no JSON at all, not a malformed payload.
The `load_order` and `enabled: false` columns above only exist because of the
recent `plugin.yaml` migration, which makes the payload worth consuming
programmatically in a way the old single-line `plugin.config` never was.
This moves the table into a `PluginListPrinter` so the command follows the
same
path as every other one, and adds the autests that were impossible to write
before.
Text output is unchanged, byte for byte.
---
## ⚠️ Stacked on #13609 — please merge that one first
This branch is built on top of
[#13609](https://github.com/apache/trafficserver/pull/13609)
(*traffic_ctl: emit JSON null instead of YAML tilde*), so GitHub shows **five
commits and fourteen files**. Only the top two are mine:
| Commit | Belongs to |
|---|---|
| `Add autests for traffic_ctl plugin list output` | **this PR** |
| `traffic_ctl: honor -f json for plugin list` | **this PR** |
| `traffic_ctl: address review on the JSON emitter helper` | #13609 |
| `traffic_ctl: route JSON emitters through one helper` | #13609 |
| `traffic_ctl: emit JSON null instead of YAML tilde` | #13609 |
Everything under `src/config/`, `src/mgmt/`, `include/`, and
`doc/developer-guide/jsonrpc/` in this diff belongs to #13609 — including the
one-line `YAML::NodeType::Sequence` change in
`src/mgmt/rpc/handlers/plugins/Plugins.cc`. **This PR's own changes are
limited
to `src/traffic_ctl/` and `tests/gold_tests/traffic_ctl/`.** Reviewing those
two directories covers it.
Note this is *not* the `plugin.yaml` migration — that is already on master
and
is untouched here. #13609 is about the JSON emitter writing `~` where JSON
requires `null`.
**Why stacked rather than standalone.** On master the server emits `plugins:
~`
for an empty plugin list, which no JSON parser accepts. So
`traffic_ctl_plugin_empty.test.py` cannot assert a successful parse until
#13609 lands its `LowerNull` emitter change together with the
`YAML::NodeType::Sequence` fix in `get_plugin_list`. The alternative was to
copy that one-line fix into this branch, which would have duplicated an open
PR
and set up a merge conflict between the two. Stacking makes the dependency
visible instead of hiding it.
If reviewers would rather see this stand alone, the empty-plugin-list test
can
move into #13609 — that PR is what makes the output parseable, so the
assertion arguably belongs there — leaving this PR as the printer fix plus
the
populated-config test, both of which pass on master unchanged.
---
## The bug
`PluginCommand::plugin_list()` touched `_printer` exactly once, in the error
branch. Past that it decoded the response and hand-rolled a table into
`std::cout`:
```cpp
if (response.is_error()) {
_printer->write_output(response); // the only use of _printer
return;
}
auto info = response.result.as<PluginListResponse>();
std::cout << "source: " << info.source << '\n'; // hand-rolled from here
down
...
```
Format-agnostic by construction — every format produced byte-identical
output,
whether the source was `plugin.yaml` or the legacy `plugin.config`.
`-f rpc` appeared to work, which made this easy to miss: the wire trace is
emitted by the transport layer through `_printer->write_debug()`,
independently
of whatever the command itself prints.
Every other command hands the response to the printer and lets
`BasePrinter::write_output(JSONRPCResponse const &)` branch on
`is_json_format()` — emitting the envelope for JSON, delegating to the
derived
`write_output(YAML::Node const &)` for text. `plugin list` was the only one
bypassing it.
## The fix
Three files, and the table code moves verbatim:
- **`CtrlPrinters.h`** — new `PluginListPrinter`, alongside the fifteen
printers
already there.
- **`CtrlPrinters.cc`** — the table loop, unchanged, as
`PluginListPrinter::write_output(YAML::Node const &)`. `<iomanip>` moves
here
with it.
- **`CtrlCommands.cc`** — pick the printer per subcommand; `plugin_list()`
reduces to build, invoke, hand off.
```cpp
void
PluginCommand::plugin_list()
{
GetPluginListRequest request;
auto response = invoke_rpc(request);
_printer->write_output(response);
}
```
JSON then works through the base class. The explicit `response.is_error()`
early-out disappears with no behavior change: the old code passed the
response
to a `GenericPrinter`, which resolves to the same non-virtual
`BasePrinter::write_output(JSONRPCResponse const &)` the new code calls.
Exit codes are unaffected, including on error. Worth noting for reviewers
that
`BasePrinter::write_output` sets `App_Exit_Status_Code = CTRL_EX_ERROR` only
in
text mode — with `-f json` it emits `fullMsg` and returns before the
`is_error()` block, so an RPC error still exits `CTRL_EX_OK`. That is
pre-existing and global to `traffic_ctl`, unchanged here, and orthogonal to
this PR.
The `--format` flag is documented as a global option with no per-command
carve-out, so this brings the code in line with documented behavior rather
than
adding a new capability.
### Why text mode keeps the table
`HostDBStatusPrinter` and `ServerStatusPrinter` both just call
`write_output_json(result["data"])` in *text* mode — they have no human
format
at all, so bare `traffic_ctl server status` already prints JSON. `plugin
list`
is the only command in this family with a real table, and dropping it to
match
would be a user-visible regression for no gain. After this change text is for
humans and `-f json` is for machines.
## Tests
Two autests, and the assertion is a **real parse**, not a gold file. That
distinction is the whole point: a gold file would have matched the table
indefinitely, which is exactly how both the ignored `--format` flag and the
`~` shipped green.
- `traffic_ctl_plugin_output.test.py` — populated `plugin.config`. Asserts
the
text table byte for byte, that `-f json` parses, and that
`result.data.source` is correct.
- `traffic_ctl_plugin_empty.test.py` — empty `plugin.config`. Asserts
`plugins` is `[]`, which is the assertion that needs #13609 underneath it.
Separate file because a populated config never reaches this case, and
because `TrafficCtl` hardcodes its ATS process name, so two instances
cannot
share one file.
Both use `plugin.config` rather than `plugin.yaml`: the autest ATS extension
registers `plugin.config` as a Disk file but has no `plugin.yaml` equivalent,
so the DSL cannot write one. This leaves the `load_order` column and the
`disabled` status uncovered — both are reachable only through `plugin.yaml`.
Adding that registration is a reasonable follow-up; it is not needed to prove
the format dispatch works, which is what this PR changes.
The DSL had no `plugin()` builder, so this adds one, plus a `plugin_config`
parameter mirroring the existing `records_yaml`, and two assertion helpers:
- `validate_json_parses()` — pipes through `json.load` and asserts exit 0.
- `validate_json_data_contains()` — same comparison as the existing
`validate_json_contains`, but descends into `result.data` first. The
existing
helper only reaches top-level keys, and with `-f json` those are just
`jsonrpc`, `result` and `id`, so payload fields were unreachable.
The new helper keeps its inline script single quoted and passes expected
values
as one `shlex.quote`'d JSON argument. The existing `validate_json_contains`
interpolates them straight into a double-quoted shell word, so a value
containing an apostrophe raises `SyntaxError`, one containing `$` is silently
shell-expanded before the comparison, and `$(...)` executes. Not fixed here
to
keep the diff scoped, but it is a live footgun in that helper.
Verified the tests are a real regression guard rather than passing for the
wrong reason: feeding the captured pre-fix output to `json.load` reproduces
`Expecting value: line 1 column 1 (char 0)` and exits 1.
```
TOTAL: 2 passed, 0 failed, 0 skipped
```
## Out of scope
**`-f yaml` is not a format `traffic_ctl` supports.** `_Fmt_str_to_enum`
holds
only `json` and `rpc`, `FormatFlags` has no YAML member, and `--format`
documents `{json|rpc}`. `parse_print_opts` looks the string up and silently
keeps `NOT_SET` on a miss, so `-f yaml` — and any other unknown value — is
ignored on *every* command, not just this one. Rejecting unknown format
values
is a separate change with wider blast radius.
**Scalars are emitted as strings.** The JSON emitter double-quotes every
scalar and `YAML::Node` has already lost the type, so `enabled` arrives as
`"true"` and `index` as `"1"`. Pre-existing and global to `traffic_ctl`; the
existing `validate_json_contains(initialized_done='true')` assertion depends
on it.
**Known coverage gaps.** The `load_order` column and the `disabled` status
are
untested: `plugin.config` hardcodes `load_order = -1` for every entry, so the
wide header and the `--` fallback are unreachable, and the autest ATS
extension
registers no `plugin.yaml` Disk file for the DSL to write. The error path is
also untested — the `is_error()` early-out this PR removes has no autest
exercising it. Both are worth follow-ups.
**`admin_plugin_get_list` is undocumented.** It appears nowhere in
`jsonrpc-api.en.rst`, while its sibling `admin_plugin_send_basic_msg` is
referenced from the `plugin msg` entry. Left alone here; a doc-only
follow-up.
## Why it matters now
#13609 fixes `traffic_ctl` emitting YAML's `~` where JSON requires `null`,
and
two commands hit it: `hostdb status` on an empty HostDB, and `plugin list`
with
no plugins loaded. Its `plugin list` half could not be asserted, because
`-f json` produced no JSON to parse — that is the gap this PR closes, which
is
why it sits on top rather than beside.
The `plugin.yaml` migration is the other reason this matters now. `plugin
list`
exists to introspect a format that carries per-entry state — `load_order`,
`enabled` — and the whole point of a `-f json` on that command is letting
tooling read it. A flag that silently returns a fixed-width table instead
defeats that.
--
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]
