Traffic ctl plugin list format - #13626
Conversation
The JSON encoders are yaml-cpp emitters, and yaml-cpp spells null as `~`, so any RPC payload holding a null value was rejected by every JSON parser. `hostdb status` hit this on every freshly started server and still exited 0, so callers saw success and then failed to parse. Initialise the two accumulator nodes as sequences so an empty result is `[]` rather than null, which also satisfies the published schema, and set LowerNull on the four JSON emitters so any remaining null is spelled `null`. Both spellings parse back as null in YAML, so the server's ability to accept YAML input is unaffected.
The tilde fix set LowerNull at the four emitters on the RPC path, but SSLMultiCertMarshaller::to_json and StorageMarshaller::to_json build their own emitters and were missed. Neither can emit a null today, so nothing is broken, but both are one null away from the same bug. Collapse the DoubleQuoted/Flow/LowerNull idiom into ts::Yaml::configure_json_emitter() so a new emitter cannot silently omit part of it. There is now exactly one place in the tree that puts an emitter into JSON mode.
Include tsutil/YamlCfg.h directly in CtrlPrinters.cc rather than relying on it arriving through the codec headers. Correct the helper comment, which claimed the output was valid JSON for every node type but null. That holds only for the node shapes these callers build; a tag, anchor or alias still emits YAML that JSON does not accept. Say so, and use the canonical yaml-cpp, YAML and JSON spellings in the comments and the architecture doc.
There was a problem hiding this comment.
🟡 Changes recommended
The moved table printer now uses snprintf() without an explicit header include (build fragility), and the new AuTest helper hardcodes python3 instead of using the harness interpreter.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes traffic_ctl plugin list so it honors -f json by routing the successful response through the configured printer (matching the behavior of other traffic_ctl commands), and adds AuTest coverage to ensure JSON output is parseable and stable.
Changes:
- Add a
PluginListPrinterand use it fortraffic_ctl plugin listso JSON format works on the success path. - Centralize “JSON-compatible yaml-cpp emitter” configuration via
ts::Yaml::configure_json_emitter()(incl.nullemission). - Add gold tests and test utilities to validate both text output presence and JSON parsing/structure for plugin list output (including empty plugin lists).
File summaries
| File | Description |
|---|---|
| tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py | Adds JSON result-data matcher helper and plugin command wrapper; supports injecting plugin.config lines into the ATS process. |
| tests/gold_tests/traffic_ctl/traffic_ctl_plugin_output.test.py | New AuTest covering plugin list output in JSON and basic text mode smoke validation. |
| tests/gold_tests/traffic_ctl/traffic_ctl_plugin_empty.test.py | New AuTest ensuring empty plugin list emits plugins: [] and JSON parses. |
| src/traffic_ctl/CtrlPrinters.h | Declares new PluginListPrinter. |
| src/traffic_ctl/CtrlPrinters.cc | Implements PluginListPrinter table rendering and switches JSON emission to shared emitter configuration. |
| src/traffic_ctl/CtrlCommands.cc | Selects PluginListPrinter for the plugin list subcommand and routes output through _printer. |
| src/mgmt/rpc/handlers/plugins/Plugins.cc | Ensures empty plugin lists are emitted as an empty sequence (not null). |
| src/mgmt/rpc/handlers/hostdb/HostDB.cc | Ensures empty hostdb partition lists are emitted as an empty sequence (not null). |
| src/config/storage.cc | Routes JSON emission through ts::Yaml::configure_json_emitter(). |
| src/config/ssl_multicert.cc | Routes JSON emission through ts::Yaml::configure_json_emitter(). |
| include/tsutil/YamlCfg.h | Introduces ts::Yaml::configure_json_emitter() to centralize JSON-ish yaml-cpp emitter configuration. |
| include/shared/rpc/yaml_codecs.h | Uses ts::Yaml::configure_json_emitter() for request encoding output. |
| include/mgmt/rpc/jsonrpc/json/YAMLCodec.h | Uses ts::Yaml::configure_json_emitter() and updates related documentation/comments. |
| doc/developer-guide/jsonrpc/jsonrpc-architecture.en.rst | Documents null emission behavior (null vs ~) for JSON compatibility. |
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
plugin_list() formatted its table straight to std::cout and only consulted the printer on the error path, so --format was silently ignored on success and the output could not be consumed as JSON. Move the table into a PluginListPrinter so the command follows the same path as every other one: the base class dispatches on format, emitting the JSON-RPC envelope for -f json and delegating to the printer for text. Text output is unchanged.
Assert the payload through -f json rather than the text table. Column widths are a presentation detail, so pinning them down byte for byte only makes the test brittle against cosmetic changes. Text mode gets a smoke check that it still renders a table. The parse is the point. A gold file would have matched unparseable output indefinitely, which is how the ignored --format flag shipped green in the first place. The empty plugin.config case gets its own test because a populated config never reaches it, and because TrafficCtl hardcodes its ATS process name, so two instances cannot share one file. Adds the plugin() command builder the DSL was missing, along with a plugin_config parameter and a structural json assertion helper.
fae05ff to
e8dfbee
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The change correctly restores format dispatch for plugin list without altering text output and adds targeted AuTests that validate real JSON parsing and payload structure.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
traffic_ctl: honor
-f jsonforplugin listTL;DR
traffic_ctl plugin list -f jsonprinted 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:both of these printed the same thing:
Column 1, char 0 — no JSON at all, not a malformed payload.
The
load_orderandenabled: falsecolumns above only exist because of therecent
plugin.yamlmigration, which makes the payload worth consumingprogrammatically in a way the old single-line
plugin.confignever was.This moves the table into a
PluginListPrinterso the command follows the samepath as every other one, and adds the autests that were impossible to write
before.
Text output is unchanged, byte for byte.
This branch is built on top of
#13609
(traffic_ctl: emit JSON null instead of YAML tilde), so GitHub shows five
commits and fourteen files. Only the top two are mine:
Add autests for traffic_ctl plugin list outputtraffic_ctl: honor -f json for plugin listtraffic_ctl: address review on the JSON emitter helpertraffic_ctl: route JSON emitters through one helpertraffic_ctl: emit JSON null instead of YAML tildeEverything under
src/config/,src/mgmt/,include/, anddoc/developer-guide/jsonrpc/in this diff belongs to #13609 — including theone-line
YAML::NodeType::Sequencechange insrc/mgmt/rpc/handlers/plugins/Plugins.cc. This PR's own changes are limitedto
src/traffic_ctl/andtests/gold_tests/traffic_ctl/. Reviewing thosetwo directories covers it.
Note this is not the
plugin.yamlmigration — that is already on master andis untouched here. #13609 is about the JSON emitter writing
~where JSONrequires
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.pycannot assert a successful parse until#13609 lands its
LowerNullemitter change together with theYAML::NodeType::Sequencefix inget_plugin_list. The alternative was tocopy 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_printerexactly once, in the errorbranch. Past that it decoded the response and hand-rolled a table into
std::cout:Format-agnostic by construction — every format produced byte-identical output,
whether the source was
plugin.yamlor the legacyplugin.config.-f rpcappeared to work, which made this easy to miss: the wire trace isemitted by the transport layer through
_printer->write_debug(), independentlyof whatever the command itself prints.
Every other command hands the response to the printer and lets
BasePrinter::write_output(JSONRPCResponse const &)branch onis_json_format()— emitting the envelope for JSON, delegating to the derivedwrite_output(YAML::Node const &)for text.plugin listwas the only onebypassing it.
The fix
Three files, and the table code moves verbatim:
CtrlPrinters.h— newPluginListPrinter, alongside the fifteen printersalready there.
CtrlPrinters.cc— the table loop, unchanged, asPluginListPrinter::write_output(YAML::Node const &).<iomanip>moves herewith it.
CtrlCommands.cc— pick the printer per subcommand;plugin_list()reduces to build, invoke, hand off.
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-virtualBasePrinter::write_output(JSONRPCResponse const &)the new code calls.Exit codes are unaffected, including on error. Worth noting for reviewers that
BasePrinter::write_outputsetsApp_Exit_Status_Code = CTRL_EX_ERRORonly intext mode — with
-f jsonit emitsfullMsgand returns before theis_error()block, so an RPC error still exitsCTRL_EX_OK. That ispre-existing and global to
traffic_ctl, unchanged here, and orthogonal tothis PR.
The
--formatflag is documented as a global option with no per-commandcarve-out, so this brings the code in line with documented behavior rather than
adding a new capability.
Why text mode keeps the table
HostDBStatusPrinterandServerStatusPrinterboth just callwrite_output_json(result["data"])in text mode — they have no human formatat all, so bare
traffic_ctl server statusalready prints JSON.plugin listis 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 jsonis 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
--formatflag and the~shipped green.traffic_ctl_plugin_output.test.py— populatedplugin.config. Asserts thetext table byte for byte, that
-f jsonparses, and thatresult.data.sourceis correct.traffic_ctl_plugin_empty.test.py— emptyplugin.config. Assertspluginsis[], which is the assertion that needs traffic_ctl: emit JSON null instead of YAML tilde #13609 underneath it.Separate file because a populated config never reaches this case, and
because
TrafficCtlhardcodes its ATS process name, so two instances cannotshare one file.
Both use
plugin.configrather thanplugin.yaml: the autest ATS extensionregisters
plugin.configas a Disk file but has noplugin.yamlequivalent,so the DSL cannot write one. This leaves the
load_ordercolumn and thedisabledstatus uncovered — both are reachable only throughplugin.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 aplugin_configparameter mirroring the existing
records_yaml, and two assertion helpers:validate_json_parses()— pipes throughjson.loadand asserts exit 0.validate_json_data_contains()— same comparison as the existingvalidate_json_contains, but descends intoresult.datafirst. The existinghelper only reaches top-level keys, and with
-f jsonthose are justjsonrpc,resultandid, 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 existingvalidate_json_containsinterpolates them straight into a double-quoted shell word, so a value
containing an apostrophe raises
SyntaxError, one containing$is silentlyshell-expanded before the comparison, and
$(...)executes. Not fixed here tokeep 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.loadreproducesExpecting value: line 1 column 1 (char 0)and exits 1.Out of scope
-f yamlis not a formattraffic_ctlsupports._Fmt_str_to_enumholdsonly
jsonandrpc,FormatFlagshas no YAML member, and--formatdocuments
{json|rpc}.parse_print_optslooks the string up and silentlykeeps
NOT_SETon a miss, so-f yaml— and any other unknown value — isignored 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::Nodehas already lost the type, soenabledarrives as"true"andindexas"1". Pre-existing and global totraffic_ctl; theexisting
validate_json_contains(initialized_done='true')assertion dependson it.
Known coverage gaps. The
load_ordercolumn and thedisabledstatus areuntested:
plugin.confighardcodesload_order = -1for every entry, so thewide header and the
--fallback are unreachable, and the autest ATS extensionregisters no
plugin.yamlDisk file for the DSL to write. The error path isalso untested — the
is_error()early-out this PR removes has no autestexercising it. Both are worth follow-ups.
admin_plugin_get_listis undocumented. It appears nowhere injsonrpc-api.en.rst, while its siblingadmin_plugin_send_basic_msgisreferenced from the
plugin msgentry. Left alone here; a doc-only follow-up.Why it matters now
#13609 fixes
traffic_ctlemitting YAML's~where JSON requiresnull, andtwo commands hit it:
hostdb statuson an empty HostDB, andplugin listwithno plugins loaded. Its
plugin listhalf could not be asserted, because-f jsonproduced no JSON to parse — that is the gap this PR closes, which iswhy it sits on top rather than beside.
The
plugin.yamlmigration is the other reason this matters now.plugin listexists to introspect a format that carries per-entry state —
load_order,enabled— and the whole point of a-f jsonon that command is lettingtooling read it. A flag that silently returns a fixed-width table instead
defeats that.