diff --git a/src/traffic_ctl/CtrlCommands.cc b/src/traffic_ctl/CtrlCommands.cc index 8b880830672..97d5292d43a 100644 --- a/src/traffic_ctl/CtrlCommands.cc +++ b/src/traffic_ctl/CtrlCommands.cc @@ -26,7 +26,6 @@ #include #include #include -#include #include #include #include @@ -1004,12 +1003,17 @@ HostDBCommand::status_get() //------------------------------------------------------------------------------------------------------------------------------------ PluginCommand::PluginCommand(ts::Arguments *args) : CtrlCommand(args) { + BasePrinter::Options printOpts{parse_print_opts(args)}; + if (get_parsed_arguments()->get(MSG_STR)) { _invoked_func = [&]() { plugin_msg(); }; + _printer = std::make_unique(printOpts); } else if (get_parsed_arguments()->get(LIST_STR)) { _invoked_func = [&]() { plugin_list(); }; + _printer = std::make_unique(printOpts); + } else { + _printer = std::make_unique(printOpts); } - _printer = std::make_unique(parse_print_opts(args)); } void @@ -1033,44 +1037,7 @@ PluginCommand::plugin_list() GetPluginListRequest request; auto response = invoke_rpc(request); - if (response.is_error()) { - _printer->write_output(response); - return; - } - - auto info = response.result.as(); - - std::cout << "source: " << info.source << '\n'; - - bool has_load_order = false; - for (const auto &p : info.plugins) { - if (p.load_order >= 0) { - has_load_order = true; - break; - } - } - - if (has_load_order) { - std::cout << " # plugin load_order status\n"; - } else { - std::cout << " # plugin status\n"; - } - - for (const auto &p : info.plugins) { - std::cout << " " << std::right << std::setw(2) << p.index << " " << std::left << std::setw(30) << p.path; - - if (has_load_order) { - char order_buf[12]; - if (p.load_order >= 0) { - snprintf(order_buf, sizeof(order_buf), "%d", p.load_order); - } else { - snprintf(order_buf, sizeof(order_buf), "--"); - } - std::cout << " " << std::left << std::setw(11) << order_buf; - } - - std::cout << " " << p.status << '\n'; - } + _printer->write_output(response); } //------------------------------------------------------------------------------------------------------------------------------------ DirectRPCCommand::DirectRPCCommand(ts::Arguments *args) : CtrlCommand(args) diff --git a/src/traffic_ctl/CtrlPrinters.cc b/src/traffic_ctl/CtrlPrinters.cc index c0243c6ee24..4bf9a44b4f3 100644 --- a/src/traffic_ctl/CtrlPrinters.cc +++ b/src/traffic_ctl/CtrlPrinters.cc @@ -18,6 +18,8 @@ limitations under the License. */ +#include +#include #include #include #include @@ -700,3 +702,41 @@ ServerStatusPrinter::write_output(YAML::Node const &result) write_output_json(result["data"] ? result["data"] : result); } //------------------------------------------------------------------------------------------------------------------------------------- +void +PluginListPrinter::write_output(YAML::Node const &result) +{ + auto info = result.as(); + + std::cout << "source: " << info.source << '\n'; + + bool has_load_order = false; + for (const auto &p : info.plugins) { + if (p.load_order >= 0) { + has_load_order = true; + break; + } + } + + if (has_load_order) { + std::cout << " # plugin load_order status\n"; + } else { + std::cout << " # plugin status\n"; + } + + for (const auto &p : info.plugins) { + std::cout << " " << std::right << std::setw(2) << p.index << " " << std::left << std::setw(30) << p.path; + + if (has_load_order) { + char order_buf[12]; + if (p.load_order >= 0) { + snprintf(order_buf, sizeof(order_buf), "%d", p.load_order); + } else { + snprintf(order_buf, sizeof(order_buf), "--"); + } + std::cout << " " << std::left << std::setw(11) << order_buf; + } + + std::cout << " " << p.status << '\n'; + } +} +//------------------------------------------------------------------------------------------------------------------------------------- diff --git a/src/traffic_ctl/CtrlPrinters.h b/src/traffic_ctl/CtrlPrinters.h index dcae30837f8..684b6f80f6a 100644 --- a/src/traffic_ctl/CtrlPrinters.h +++ b/src/traffic_ctl/CtrlPrinters.h @@ -333,6 +333,14 @@ class ServerStatusPrinter : public BasePrinter ServerStatusPrinter(BasePrinter::Options opt) : BasePrinter(opt) {} }; //------------------------------------------------------------------------------------------------------------------------------------ +class PluginListPrinter : public BasePrinter +{ + void write_output(YAML::Node const &result) override; + +public: + PluginListPrinter(BasePrinter::Options opt) : BasePrinter(opt) {} +}; +//------------------------------------------------------------------------------------------------------------------------------------ /// In case a derived class needs to call derived class functions. Ugly but works. /// Note: CRTP may worth a try. diff --git a/tests/gold_tests/traffic_ctl/traffic_ctl_json_null.test.py b/tests/gold_tests/traffic_ctl/traffic_ctl_json_null.test.py index 00a050f7791..134e7e1d652 100644 --- a/tests/gold_tests/traffic_ctl/traffic_ctl_json_null.test.py +++ b/tests/gold_tests/traffic_ctl/traffic_ctl_json_null.test.py @@ -72,14 +72,13 @@ ###### # plugin list -- `plugins` is empty when plugin.config loads nothing. # -# plugin list ignores the format flag today and prints a human table, so only -# the RPC path is assertable. Once plugin list honours -f json, add: -# traffic_ctl.plugin().list().as_json().validate_is_valid_json() +# This is the RPC path. The traffic_ctl -f json path is asserted in +# traffic_ctl_plugin_empty.test.py, which needs its own ATS because TrafficCtl +# hardcodes the process name. # # Assert the shape rather than mere parseability: `plugins` has to be `[]`, -# matching what the hostdb case above asserts for `partitions`. The field sits -# at result.data.plugins, which validate_json_contains cannot reach, so this -# compares the whole result, as the connection tracker cases in +# matching what the hostdb case above asserts for `partitions`. This compares +# the whole result, as the connection tracker cases in # traffic_ctl_server_output.test.py do. Before the fix the field emitted `~`, # which fails this comparison as surely as it fails a JSON parser. Nothing # forces plugin.config to be empty here, and nothing needs to: should a diff --git a/tests/gold_tests/traffic_ctl/traffic_ctl_plugin_empty.test.py b/tests/gold_tests/traffic_ctl/traffic_ctl_plugin_empty.test.py new file mode 100644 index 00000000000..5fd436a0196 --- /dev/null +++ b/tests/gold_tests/traffic_ctl/traffic_ctl_plugin_empty.test.py @@ -0,0 +1,45 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import sys + +# To include util classes +sys.path.insert(0, f'{Test.TestDirectory}') + +from traffic_ctl_test_utils import Make_traffic_ctl + +Test.Summary = 'Test traffic_ctl plugin list with no plugins loaded.' + +Test.ContinueOnFail = True + +records_yaml = ''' + exec_thread: + autoconfig: + enabled: 0 + limit: 4 + ''' + +# No plugin_config, so the default empty plugin.config is used. This is the +# case a populated-config test never reaches, and the one that produced the +# unparseable `plugins: ~` before the json emitter was fixed. +traffic_ctl = Make_traffic_ctl(Test, records_yaml) + +# An empty list, not null. A client iterating this field should not have to +# special case a server with nothing loaded. Asserting the whole structure is +# what makes [] distinguishable from null here. +traffic_ctl.plugin().list().as_json().validate_json_contains(result={'data': {'source': 'plugin.config', 'plugins': []}}) + +traffic_ctl.plugin().list().validate_contains_all('source: plugin.config') diff --git a/tests/gold_tests/traffic_ctl/traffic_ctl_plugin_output.test.py b/tests/gold_tests/traffic_ctl/traffic_ctl_plugin_output.test.py new file mode 100644 index 00000000000..f258dfeebe6 --- /dev/null +++ b/tests/gold_tests/traffic_ctl/traffic_ctl_plugin_output.test.py @@ -0,0 +1,75 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import sys + +# To include util classes +sys.path.insert(0, f'{Test.TestDirectory}') + +from traffic_ctl_test_utils import Make_traffic_ctl + +Test.Summary = 'Test traffic_ctl plugin list output in text and json format.' + +Test.ContinueOnFail = True + +Test.SkipUnless(Condition.PluginExists('xdebug.so'), Condition.PluginExists('stats_over_http.so')) + +records_yaml = ''' + exec_thread: + autoconfig: + enabled: 0 + limit: 4 + ''' + +# xdebug logs an error and does nothing when no feature is enabled. The +# reported path is argv[0] only, so the argument does not reach the table. +plugin_config = ['xdebug.so --enable=x-cache', 'stats_over_http.so'] + +traffic_ctl = Make_traffic_ctl(Test, records_yaml, plugin_config=plugin_config) + +# The payload is asserted through json, not through the table. Column widths +# are a presentation detail, so pinning them down only makes the test brittle +# against cosmetic changes. This checks the whole structure, so a renamed, +# added or dropped key fails too. Json mode emits the whole jsonrpc envelope, +# so the payload sits under result.data; naming result asserts it carries +# nothing else. +traffic_ctl.plugin().list().as_json().validate_json_contains( + result={ + 'data': + { + 'source': 'plugin.config', + 'plugins': + [ + { + 'path': 'xdebug.so', + 'enabled': 'true', + 'status': 'loaded', + 'index': '1' + }, + { + 'path': 'stats_over_http.so', + 'enabled': 'true', + 'status': 'loaded', + 'index': '2' + }, + ], + }, + }) + +# Text mode only needs to still render a table. This is a smoke check that the +# formatter survived being moved into PluginListPrinter; it deliberately does +# not assert the column layout. +traffic_ctl.plugin().list().validate_contains_all('source: plugin.config', 'xdebug.so', 'stats_over_http.so', 'loaded') diff --git a/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py b/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py index 86e7e329daa..8406285ff81 100644 --- a/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py +++ b/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py @@ -106,6 +106,24 @@ def _check_is_valid_json(path): return (True, desc, "Output parses as JSON") +def _render_json_value(value): + """Render a value for a failure message, key-sorted when it is a structure. + + A nested expectation compared as one `repr()` puts both structures on a + single line, in source order on one side and emission order on the other, + which is unreadable for anything larger than a scalar. Serialising both + sides the same way, key-sorted, makes the differing key findable. + + `default` catches a value json cannot serialise and non-string dict keys + raise regardless, so the call is guarded: autest treats an exception from + a tester callback as fatal, see the note in `_read_stdout`. + """ + try: + return json.dumps(value, sort_keys=True, default=repr) + except (TypeError, ValueError): + return repr(value) + + def _check_json_fields(path, expected): """Tester callback: every expected field must match its value in the parsed output. @@ -145,13 +163,15 @@ def _check_json_fields(path, expected): failed = [] for key, want in expected.items(): if key not in doc: - failed.append(f"{key} is missing (expected {want!r})") + failed.append(f"{key} is missing\n expected: {_render_json_value(want)}") continue actual = doc[key] if actual != want: - failed.append(f"{key} = {actual!r} (expected {want!r})") + failed.append( + f"{key} does not match\n actual : {_render_json_value(actual)}\n" + f" expected: {_render_json_value(want)}") if failed: - return (False, desc, "FAIL: " + "; ".join(failed) + f"\nOutput was:\n{raw}") + return (False, desc, "FAIL:\n" + "\n".join(failed) + f"\nOutput was:\n{raw}") return (True, desc, "All expected fields matched") @@ -230,9 +250,10 @@ def validate_result_with_text(self, text: str): def validate_json_contains(self, **field_checks): """ - Validate JSON output contains specific field:value pairs. Only checks specified fields. - Every mismatch is reported as "field_name = actual_value (expected expected_value)", - followed by the raw output. + Validate JSON output contains specific field:value pairs. Only checks specified fields, + each compared whole, so naming a field whose value is a structure asserts that subtree + exactly. Every mismatch is reported with the actual and expected values on their own + lines, key-sorted, followed by the raw output. The check runs in the autest process against the captured stdout file. Piping traffic_ctl into a JSON parser instead would hide failures: the exit status of a shell @@ -646,7 +667,7 @@ class TrafficCtl(Config, Server): Every time a config() is called, a new test is created. """ - def __init__(self, test, records_yaml=None, retcode=0): + def __init__(self, test, records_yaml=None, retcode=0, plugin_config=None): self._testNumber = 0 self._current_test_number = self._testNumber self._retcode = retcode @@ -654,6 +675,9 @@ def __init__(self, test, records_yaml=None, retcode=0): self._ts = self._Test.MakeATSProcess(f"ts_{self._testNumber}") if records_yaml != None: self._ts.Disk.records_config.update(records_yaml) + if plugin_config != None: + for line in plugin_config: + self._ts.Disk.plugin_config.AddLine(line) self._tests = [] def __get_index(self): @@ -695,6 +719,6 @@ def plugin(self): return Plugin(self._Test.TestDirectory, self._tests[self.__get_index()], self._testNumber) -def Make_traffic_ctl(test, records_yaml=None, retcode=0): - tctl = TrafficCtl(test, records_yaml, retcode) +def Make_traffic_ctl(test, records_yaml=None, retcode=0, plugin_config=None): + tctl = TrafficCtl(test, records_yaml, retcode, plugin_config) return tctl