diff --git a/doc/developer-guide/jsonrpc/jsonrpc-architecture.en.rst b/doc/developer-guide/jsonrpc/jsonrpc-architecture.en.rst index 5f860545606..cea4b8d336b 100644 --- a/doc/developer-guide/jsonrpc/jsonrpc-architecture.en.rst +++ b/doc/developer-guide/jsonrpc/jsonrpc-architecture.en.rst @@ -74,6 +74,9 @@ Our JSONRPC protocol implementation uses lib yamlcpp for parsing incoming and o this allows the server to accept either JSON or YAML format messages which then will be parsed by the protocol implementation. This seems handy for user that want to feed |TS| with existing yaml configuration without the need to translate yaml into json. +The server emits null values as the literal ``null``, not as YAML's ``~``. JSON parsers reject ``~``. YAML resolves ``~`` and +``null`` to the same value. Clients that read the response as YAML see no change, and the server still accepts YAML input. + .. note:: :program:`traffic_ctl` have an option to read files from disc and push them into |TS| through the RPC server. Files should be a diff --git a/include/mgmt/rpc/jsonrpc/json/YAMLCodec.h b/include/mgmt/rpc/jsonrpc/json/YAMLCodec.h index cc29dfe2dd9..a41078add35 100644 --- a/include/mgmt/rpc/jsonrpc/json/YAMLCodec.h +++ b/include/mgmt/rpc/jsonrpc/json/YAMLCodec.h @@ -21,16 +21,18 @@ #pragma once #include + +#include "tsutil/YamlCfg.h" #include "mgmt/rpc/jsonrpc/error/RPCError.h" #include "mgmt/rpc/jsonrpc/Defs.h" namespace rpc::json_codecs { /// -/// @note The overall design is to make this classes @c yamlcpp_json_decoder and @c yamlcpp_json_encoder plugables into the Json Rpc -/// encode/decode logic. yamlcpp does not give us all the behavior we need, such as the way it handles the null values. Json needs -/// to use literal null and yamlcpp uses ~. If this becomes a problem, then we may need to change the codec implementation, we just -/// follow the api and it should work with minimum changes. +/// @note The design keeps @c yamlcpp_json_decoder and @c yamlcpp_json_encoder replaceable in the JSONRPC encode/decode logic. +/// yaml-cpp emits null as @c ~ by default, which JSON parsers reject. Every emitter here calls +/// @c ts::Yaml::configure_json_emitter, which writes the literal @c null instead. YAML resolves @c ~ and @c null to the same +/// value, so the server still accepts YAML input. /// /// @@ -251,8 +253,8 @@ class yamlcpp_json_encoder if (!resp.id.empty()) { json << YAML::Key << "id" << YAML::Value << resp.id; } - // else: We do not insert null as it will break the json, we need literal null and not ~ (as per yaml) - // json << YAML::Null; + // else: the field is omitted rather than set to null. Emitting it would be valid JSON now that LowerNull is set, but the + // omission is deliberate, see the id note in mgmt/rpc/schema/jsonrpc_response_schema.json. json << YAML::EndMap; } @@ -268,7 +270,7 @@ class yamlcpp_json_encoder encode(const specs::RPCResponseInfo &resp) { YAML::Emitter json; - json << YAML::DoubleQuoted << YAML::Flow; + ts::Yaml::configure_json_emitter(json); encode(resp, json); return json.c_str(); @@ -284,7 +286,7 @@ class yamlcpp_json_encoder encode(const specs::RPCResponse &response) { YAML::Emitter json; - json << YAML::DoubleQuoted << YAML::Flow; + ts::Yaml::configure_json_emitter(json); { if (response.is_batch()) { json << YAML::BeginSeq; diff --git a/include/shared/rpc/yaml_codecs.h b/include/shared/rpc/yaml_codecs.h index d719973f312..d05c5bd74aa 100644 --- a/include/shared/rpc/yaml_codecs.h +++ b/include/shared/rpc/yaml_codecs.h @@ -22,6 +22,8 @@ #include #include +#include "tsutil/YamlCfg.h" + #include "shared/rpc/RPCRequests.h" /// JSONRPC 2.0 Client API request/response codecs only. If you need to define your own specific codecs they should then be defined @@ -202,7 +204,7 @@ class yamlcpp_json_emitter encode(shared::rpc::JSONRPCRequest const &req) { YAML::Emitter json; - json << YAML::DoubleQuoted << YAML::Flow; + ts::Yaml::configure_json_emitter(json); json << YAML::BeginMap; if (!req.id.empty()) { diff --git a/include/tsutil/YamlCfg.h b/include/tsutil/YamlCfg.h index 09686eba54a..367b95b3fd0 100644 --- a/include/tsutil/YamlCfg.h +++ b/include/tsutil/YamlCfg.h @@ -39,6 +39,31 @@ namespace Yaml constexpr std::string_view YAML_BOOL_TAG_URI{"tag:yaml.org,2002:bool"}; constexpr std::string_view YAML_NULL_TAG_URI{"tag:yaml.org,2002:null"}; + // Put an emitter into JSON output mode. + // + // yaml-cpp has no JSON output mode. The nearest equivalent is flow style with every scalar double quoted. For the + // node shapes the callers here emit -- maps, sequences, scalars and nulls, carrying no tags, anchors or aliases -- + // that parses as JSON except for null: yaml-cpp writes `~`, which JSON parsers reject. LowerNull writes the + // literal `null` instead. YAML resolves `~` and `null` to the same value, so the output still reads as YAML. + // + // Parses as JSON is the whole guarantee. It is not type-faithful JSON: `DoubleQuoted` quotes every scalar, so + // numbers and booleans arrive as strings -- `"12"` rather than `12`, `"true"` rather than `true`. A consumer + // validating against a schema that declares `integer` or `boolean` will reject that, and no manipulator here + // changes it. Preserving scalar types needs a real JSON serializer, not a yaml-cpp emitter. + // + // This is not a general YAML to JSON converter. A node that carries a tag, an anchor or an alias still emits YAML + // syntax that JSON does not accept. + // + // Every emitter whose output reaches a JSON consumer must go through here. Setting only some of the manipulators + // gives output that looks like JSON and parses correctly until some node is null. + // + inline void + configure_json_emitter(YAML::Emitter &emitter) + { + emitter.SetNullFormat(YAML::LowerNull); + emitter << YAML::DoubleQuoted << YAML::Flow; + } + // A class that is a wrapper for a YAML::Node that corresponds to a map in a YAML input file. // It's purpose is to make sure all keys in the map are processed. // diff --git a/src/config/ssl_multicert.cc b/src/config/ssl_multicert.cc index c64780113ef..7b661baced2 100644 --- a/src/config/ssl_multicert.cc +++ b/src/config/ssl_multicert.cc @@ -34,6 +34,7 @@ #include "swoc/swoc_file.h" #include "swoc/TextView.h" #include "tsutil/ts_diag_levels.h" +#include "tsutil/YamlCfg.h" namespace { @@ -360,7 +361,7 @@ std::string SSLMultiCertMarshaller::to_json(SSLMultiCertConfig const &config) { YAML::Emitter json; - json << YAML::DoubleQuoted << YAML::Flow; + ts::Yaml::configure_json_emitter(json); json << YAML::BeginMap; json << YAML::Key << KEY_SSL_MULTICERT << YAML::Value << YAML::BeginSeq; diff --git a/src/config/storage.cc b/src/config/storage.cc index 42b2273f61f..aae5c21ef27 100644 --- a/src/config/storage.cc +++ b/src/config/storage.cc @@ -37,6 +37,7 @@ #include "swoc/swoc_file.h" #include "tscore/ParseRules.h" #include "tsutil/ts_diag_levels.h" +#include "tsutil/YamlCfg.h" namespace { @@ -828,7 +829,7 @@ std::string StorageMarshaller::to_json(StorageConfig const &config) { YAML::Emitter out; - out << YAML::DoubleQuoted << YAML::Flow; + ts::Yaml::configure_json_emitter(out); out << YAML::BeginMap; out << YAML::Key << KEY_CACHE << YAML::Value << YAML::BeginMap; diff --git a/src/mgmt/rpc/handlers/hostdb/HostDB.cc b/src/mgmt/rpc/handlers/hostdb/HostDB.cc index 9759a1cd19e..f1e386b2fb1 100644 --- a/src/mgmt/rpc/handlers/hostdb/HostDB.cc +++ b/src/mgmt/rpc/handlers/hostdb/HostDB.cc @@ -82,7 +82,7 @@ template <> struct convert { static Node encode(const HostDBCache *const hostDB, std::string_view hostname) { - Node partitions; + Node partitions{YAML::NodeType::Sequence}; for (size_t i = 0; i < hostDB->refcountcache->partition_count(); i++) { auto &partition = hostDB->refcountcache->get_partition(i); std::vector partition_entries; diff --git a/src/mgmt/rpc/handlers/plugins/Plugins.cc b/src/mgmt/rpc/handlers/plugins/Plugins.cc index 58e43f26ebd..7edfaa68c49 100644 --- a/src/mgmt/rpc/handlers/plugins/Plugins.cc +++ b/src/mgmt/rpc/handlers/plugins/Plugins.cc @@ -100,7 +100,7 @@ get_plugin_list(std::string_view const & /* id ATS_UNUSED */, YAML::Node const & data["source"] = summary.source; - YAML::Node plugins; + YAML::Node plugins{YAML::NodeType::Sequence}; for (const auto &e : summary.entries) { YAML::Node plugin; diff --git a/src/traffic_ctl/CtrlPrinters.cc b/src/traffic_ctl/CtrlPrinters.cc index 22b934179df..613cd6aa0c2 100644 --- a/src/traffic_ctl/CtrlPrinters.cc +++ b/src/traffic_ctl/CtrlPrinters.cc @@ -24,6 +24,7 @@ #include #include "tsutil/ts_bw_format.h" +#include "tsutil/YamlCfg.h" #include "CtrlPrinters.h" #include "jsonrpc/ctrl_yaml_codecs.h" @@ -102,7 +103,7 @@ void BasePrinter::write_output_json(YAML::Node const &node) const { YAML::Emitter out; - out << YAML::DoubleQuoted << YAML::Flow; + ts::Yaml::configure_json_emitter(out); out << node; std::cout << out.c_str() << '\n'; } 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 new file mode 100644 index 00000000000..00a050f7791 --- /dev/null +++ b/tests/gold_tests/traffic_ctl/traffic_ctl_json_null.test.py @@ -0,0 +1,94 @@ +# 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 = ''' +traffic_ctl JSON output must be parseable JSON, including when a node is null. + +yaml-cpp emits null as `~`, which is valid YAML but rejected by every JSON +parser. Emitters that produce JSON must set YAML::LowerNull, and container +nodes that may stay empty must be constructed as sequences so they emit `[]` +rather than null. + +The trigger for both regressions is the *empty* case, so this test +deliberately runs against a freshly started server with an empty HostDB and no +plugins loaded. A test that populates either one first would pass against the +bug. +''' + +Test.ContinueOnFail = True + +records_yaml = ''' + exec_thread: + autoconfig: + enabled: 0 + limit: 4 + ''' + +traffic_ctl = Make_traffic_ctl(Test, records_yaml) + +###### +# hostdb status -- `partitions` is empty on a fresh server. +# +# Flagless output goes through BasePrinter::write_output_json (the client +# printer). Before the fix this emitted `"partitions": ~`. +traffic_ctl.hostdb().status().validate_is_valid_json() + +# ... and it must be an empty array, not null. hostdb_status_schema.json +# declares partitions as "type": "array". +traffic_ctl.hostdb().status().validate_json_contains(partitions=[]) + +# -f json goes through the full envelope. Same emitter, different entry point. +traffic_ctl.hostdb().status().as_json().validate_is_valid_json() + +# The server-side encoder (yamlcpp_json_encoder) is a third, independent +# emitter. rpc invoke exercises it directly. +# +# The params are required: get_hostdb_status without them fails with "invalid +# node; this may result from using a map iterator as a sequence iterator", and +# an error envelope is valid JSON no matter what the emitter does -- the +# assertion would pass against the bug. +traffic_ctl.rpc().invoke(handler="get_hostdb_status", params='"hostname: \\"\\""').validate_is_valid_json() + +###### +# 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() +# +# 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 +# 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 +# default ever load a plugin, this assertion fails rather than going quiet. +traffic_ctl.rpc().invoke( + handler="admin_plugin_get_list").validate_result_with_text('{"data": {"source": "plugin.config", "plugins": []}}') + +###### +# Commands that were already valid JSON -- guard against the shared emitter +# change regressing them. +traffic_ctl.server().status().validate_is_valid_json() +traffic_ctl.rpc().invoke(handler="show_registered_handlers").validate_is_valid_json() 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 29275c9ffd9..86e7e329daa 100644 --- a/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py +++ b/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py @@ -15,10 +15,15 @@ # limitations under the License. import atexit +import json import os +import shlex import shutil import tempfile +import autest.testers as Testers +from autest.testers import All + _gold_tmpdir = None @@ -61,6 +66,95 @@ def MakeGoldFileWithText(content, dir, test_number, add_new_line=True): return gold_filepath +def _read_stdout(path): + """Read a captured stream file as strict UTF-8, as `(text, error)`. + + JSON has to be UTF-8, so output that does not decode is not valid JSON + either and the callers report it as a failure. Replacing the bad bytes + instead would hide exactly that: U+FFFD is a legal character inside a + JSON string, so undecodable output would go on to parse cleanly and pass + a check whose whole job is to reject output that is not JSON. + + No command wrapped here reaches that branch today. yaml-cpp substitutes + U+FFFD itself when writing a double quoted scalar, so traffic_ctl cannot + put undecodable bytes on stdout on any of these paths. Decoding strictly + states the requirement rather than resting on that staying true. + + The failure comes back as a value rather than an exception because autest + treats an exception from a tester callback as fatal, setting KillOnFailure + and abandoning the rest of the test run. On failure the text is still + rendered, lossily, so the caller can show what arrived. + """ + with open(path, 'rb') as stream: + raw = stream.read() + try: + return raw.decode('utf-8'), None + except UnicodeDecodeError as ex: + return raw.decode('utf-8', errors='replace'), str(ex) + + +def _check_is_valid_json(path): + """Tester callback: the captured output must parse as JSON.""" + desc = "Check that the output parses as JSON" + raw, decode_error = _read_stdout(path) + if decode_error: + return (False, desc, f"Output is not valid UTF-8, so it is not JSON: {decode_error}\nOutput was:\n{raw}") + try: + json.loads(raw) + except ValueError as ex: + return (False, desc, f"Output is not JSON: {ex}\nOutput was:\n{raw}") + return (True, desc, "Output parses as JSON") + + +def _check_json_fields(path, expected): + """Tester callback: every expected field must match its value in the parsed output. + + The expectation is compared to the parsed value with `==`, so write it as + the Python value the JSON parses to: `[]` for a JSON array, `'[]'` only + for the JSON string `"[]"`. Comparing `str()` of both sides instead would + equate those two and let a field regress from an array to a string + without failing, which is the class of bug these tests exist to catch. + + Scalars and sequences therefore read differently. The emitters these + tests cover set `YAML::DoubleQuoted`, which encodes every *scalar* as a + JSON string: `get_server_status` sends `"is_draining": "false"`, matched + by `is_draining='false'`. A genuine JSON boolean would parse to Python + `True` or `False` and needs `is_draining=False`. A sequence is not a + scalar and is untouched by `DoubleQuoted`, so `hostdb status` sends a + real `"partitions": []`, matched by `partitions=[]`. + + `validate_result_with_text` does take JSON-spelled text, so the two + helpers are not interchangeable. + + A key the output does not carry is reported as missing rather than + compared, so a misspelled field name fails instead of quietly matching an + expectation of `None`. Asserting that a field is present and null is + therefore written `field=None`, which only passes when the key is there. + """ + desc = "Check that the JSON output contains the expected fields" + raw, decode_error = _read_stdout(path) + if decode_error: + return (False, desc, f"Output is not valid UTF-8, so it is not JSON: {decode_error}\nOutput was:\n{raw}") + try: + doc = json.loads(raw) + except ValueError as ex: + return (False, desc, f"Output is not JSON: {ex}\nOutput was:\n{raw}") + if not isinstance(doc, dict): + return (False, desc, f"Output is a JSON {type(doc).__name__}, not an object, so it has no fields\nOutput was:\n{raw}") + + failed = [] + for key, want in expected.items(): + if key not in doc: + failed.append(f"{key} is missing (expected {want!r})") + continue + actual = doc[key] + if actual != want: + failed.append(f"{key} = {actual!r} (expected {want!r})") + if failed: + return (False, desc, "FAIL: " + "; ".join(failed) + f"\nOutput was:\n{raw}") + return (True, desc, "All expected fields matched") + + class Common(): """ Handy class to map common traffic_ctl test options. @@ -115,13 +209,8 @@ def validate_contains_all(self, *strings): "Set proxy.config.diags.debug.enabled" ) """ - import sys - # Testers and All are injected by autest into the test file's globals - caller_globals = sys._getframe(1).f_globals - _Testers = caller_globals['Testers'] - _All = caller_globals['All'] - testers = [_Testers.IncludesExpression(s, f"should contain: {s}") for s in strings] - self._tr.Processes.Default.Streams.stdout = _All(*testers) + testers = [Testers.IncludesExpression(s, f"should contain: {s}") for s in strings] + self._tr.Processes.Default.Streams.stdout = All(*testers) self._finish() return self @@ -142,26 +231,42 @@ 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. - Prints detailed error on failure: "FAIL: field_name = actual_value (expected expected_value)" - stream.all.txt will contain the actual output with the failed fields. + Every mismatch is reported as "field_name = actual_value (expected expected_value)", + 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 + pipeline is the parser's, so a non-zero traffic_ctl exit would never reach the + ReturnCode check. Example: traffic_ctl.server().status().validate_json_contains( initialized_done='true', is_draining='false' ) """ - import json - checks_str = ', '.join(f"'{k}': '{v}'" for k, v in field_checks.items()) - self._cmd = ( - f'{self._cmd} | python3 -c "' - f"import sys, json; " - f"d = json.load(sys.stdin); " - f"c = {{{checks_str}}}; " - f"failed = [(k, v, str(d.get(k))) for k, v in c.items() if str(d.get(k)) != v]; " - f"[print(f'FAIL: {{k}} = {{actual}} (expected {{expected}})', file=sys.stderr) " - f"for k, expected, actual in failed]; " - f"exit(0 if not failed else 1)" - f'"') + self._tr.Processes.Default.Streams.stdout = Testers.Lambda( + lambda info, tester: _check_json_fields(tester.GetContent(info), field_checks)) + self._finish() + return self + + def validate_is_valid_json(self): + """ + Validate that stdout parses as JSON. Performs no field checks. + + Use this as a regression guard on any command documented to emit JSON. + A gold file cannot do this job: yaml-cpp spells null as `~`, which a + gold file matches happily but no JSON parser accepts. + + The check runs in the autest process against the captured stdout file, so + traffic_ctl stays the only process in the test run and the exit status the + harness compares against ReturnCode is still traffic_ctl's own. The raw + output is reported on failure. + + Example: + traffic_ctl.hostdb().status().validate_is_valid_json() + """ + self._tr.Processes.Default.Streams.stdout = Testers.Lambda( + lambda info, tester: _check_is_valid_json(tester.GetContent(info))) self._finish() return self @@ -467,6 +572,53 @@ def invoke(self, handler: str, params={}): return self +class HostDB(Common): + """ + Handy class to map traffic_ctl hostdb options. + """ + + def __init__(self, dir, tr, tn): + super().__init__(tr) + self._cmd = "traffic_ctl hostdb " + self._dir = dir + self._tn = tn + + def status(self, hostname: str = ""): + """Get HostDB info (traffic_ctl hostdb status [HOSTNAME]) + + The hostname is shell quoted. It is omitted entirely when empty, since + passing an empty argument is not the same as passing none. + """ + arg = f' {shlex.quote(hostname)}' if hostname else '' + self._cmd = f'{self._cmd} status{arg} ' + return self + + def as_json(self): + self._cmd = f'{self._cmd} -f json' + return self + + +class Plugin(Common): + """ + Handy class to map traffic_ctl plugin options. + """ + + def __init__(self, dir, tr, tn): + super().__init__(tr) + self._cmd = "traffic_ctl plugin " + self._dir = dir + self._tn = tn + + def list(self): + """Show globally loaded plugins and their status (traffic_ctl plugin list)""" + self._cmd = f'{self._cmd} list ' + return self + + def as_json(self): + self._cmd = f'{self._cmd} -f json' + return self + + ''' Handy wrapper around traffic_ctl, ATS and the autest output validation mechanism. @@ -534,6 +686,14 @@ def rpc(self): self.add_test() return RPC(self._Test.TestDirectory, self._tests[self.__get_index()], self._testNumber) + def hostdb(self): + self.add_test() + return HostDB(self._Test.TestDirectory, self._tests[self.__get_index()], self._testNumber) + + def plugin(self): + self.add_test() + 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)