diff --git a/src/mgmt/rpc/CMakeLists.txt b/src/mgmt/rpc/CMakeLists.txt index b3a844f792f..8bd738c48c0 100644 --- a/src/mgmt/rpc/CMakeLists.txt +++ b/src/mgmt/rpc/CMakeLists.txt @@ -72,6 +72,12 @@ if(BUILD_TESTING) test_jsonrpcserver Catch2::Catch2WithMain ts::jsonrpc_server ts::inkevent libswoc::libswoc configmanager ) add_catch2_test(NAME test_jsonrpcserver COMMAND test_jsonrpcserver) + + add_executable(test_record_yaml handlers/common/unit_tests/test_record_yaml.cc) + target_link_libraries( + test_record_yaml PRIVATE Catch2::Catch2WithMain ts::rpcpublichandlers ts::tscore yaml-cpp::yaml-cpp + ) + add_catch2_test(NAME test_record_yaml COMMAND test_record_yaml) endif() clang_tidy_check(jsonrpc_protocol) diff --git a/src/mgmt/rpc/handlers/common/convert.h b/src/mgmt/rpc/handlers/common/convert.h index 4105d495856..6d541398534 100644 --- a/src/mgmt/rpc/handlers/common/convert.h +++ b/src/mgmt/rpc/handlers/common/convert.h @@ -139,26 +139,41 @@ template <> struct convert { node[constants_rec::OVERRIDABLE] = (it == ts::Overridable_Txn_Vars.end()) ? "false" : "true"; } + // access_type lives in config_meta, which shares storage with stat_meta + // via a union; only inspect it when the record is actually a CONFIG + // record. Records registered with @c RECA_NO_ACCESS opt out of having + // their value exposed, so withhold the value fields while still emitting + // the type label so callers can tell which records exist. + const bool no_access = REC_TYPE_IS_CONFIG(record.rec_type) && record.config_meta.access_type == RECA_NO_ACCESS; + switch (record.data_type) { case RECD_INT: - node[constants_rec::DATA_TYPE] = "INT"; - node[constants_rec::CURRENT_VALUE] = record.data.rec_int; - node[constants_rec::DEFAULT_VALUE] = record.data_default.rec_int; + node[constants_rec::DATA_TYPE] = "INT"; + if (!no_access) { + node[constants_rec::CURRENT_VALUE] = record.data.rec_int; + node[constants_rec::DEFAULT_VALUE] = record.data_default.rec_int; + } break; case RECD_FLOAT: - node[constants_rec::DATA_TYPE] = "FLOAT"; - node[constants_rec::CURRENT_VALUE] = record.data.rec_float; - node[constants_rec::DEFAULT_VALUE] = record.data_default.rec_float; + node[constants_rec::DATA_TYPE] = "FLOAT"; + if (!no_access) { + node[constants_rec::CURRENT_VALUE] = record.data.rec_float; + node[constants_rec::DEFAULT_VALUE] = record.data_default.rec_float; + } break; case RECD_STRING: - node[constants_rec::DATA_TYPE] = "STRING"; - node[constants_rec::CURRENT_VALUE] = record.data.rec_string ? record.data.rec_string : "null"; - node[constants_rec::DEFAULT_VALUE] = record.data_default.rec_string ? record.data_default.rec_string : "null"; + node[constants_rec::DATA_TYPE] = "STRING"; + if (!no_access) { + node[constants_rec::CURRENT_VALUE] = record.data.rec_string ? record.data.rec_string : "null"; + node[constants_rec::DEFAULT_VALUE] = record.data_default.rec_string ? record.data_default.rec_string : "null"; + } break; case RECD_COUNTER: - node[constants_rec::DATA_TYPE] = "COUNTER"; - node[constants_rec::CURRENT_VALUE] = record.data.rec_counter; - node[constants_rec::DEFAULT_VALUE] = record.data_default.rec_counter; + node[constants_rec::DATA_TYPE] = "COUNTER"; + if (!no_access) { + node[constants_rec::CURRENT_VALUE] = record.data.rec_counter; + node[constants_rec::DEFAULT_VALUE] = record.data_default.rec_counter; + } break; default: // this is an error, internal we should flag it diff --git a/src/mgmt/rpc/handlers/common/unit_tests/test_record_yaml.cc b/src/mgmt/rpc/handlers/common/unit_tests/test_record_yaml.cc new file mode 100644 index 00000000000..2034a0bd3a4 --- /dev/null +++ b/src/mgmt/rpc/handlers/common/unit_tests/test_record_yaml.cc @@ -0,0 +1,146 @@ +/** + @section license License + + 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. +*/ + +#include + +#include +#include +#include + +#include "../convert.h" + +namespace +{ +// +// Build minimal RecRecord fixtures aimed at the YAML encoder under test in +// place. RecRecord embeds a RecMutex (with an atomic) and is therefore not +// copyable, so the helpers operate on an output reference and only populate +// the fields the encoder actually reads. The lock and the unused half of the +// stat_meta / config_meta union stay zero-initialized. +// +void +fill_string_config(RecRecord &r, const char *name, const char *current, const char *def, RecAccessT access) +{ + r.rec_type = RECT_CONFIG; + r.data_type = RECD_STRING; + r.name = name; + r.data.rec_string = const_cast(current); + r.data_default.rec_string = const_cast(def); + r.config_meta.access_type = access; +} + +void +fill_int_config(RecRecord &r, const char *name, RecInt current, RecInt def, RecAccessT access) +{ + r.rec_type = RECT_CONFIG; + r.data_type = RECD_INT; + r.name = name; + r.data.rec_int = current; + r.data_default.rec_int = def; + r.config_meta.access_type = access; +} + +void +fill_int_stat(RecRecord &r, const char *name, RecInt current) +{ + r.rec_type = RECT_PROCESS; // STAT category + r.data_type = RECD_INT; + r.name = name; + r.data.rec_int = current; + r.data_default.rec_int = 0; + + // stat_meta is the active union member after value-initialization of + // RecRecord; explicitly re-establish that to keep the contract clear + // and to give the encoder a well-defined object to read. + r.stat_meta = RecStatMeta{}; + + // Plant a RECA_NO_ACCESS bit pattern at the storage location that the + // overlaid RecConfigMeta would expose as access_type, without making + // config_meta the active union member. A hypothetical encoder that + // reads record.config_meta.access_type without a REC_TYPE_IS_CONFIG + // guard would observe RECA_NO_ACCESS and incorrectly suppress the + // value fields below; the well-defined encoder path only reads + // stat_meta for STAT records. + static_assert(sizeof(RecStatMeta) >= offsetof(RecConfigMeta, access_type) + sizeof(RecAccessT), + "RecStatMeta must fully overlap RecConfigMeta::access_type"); + const RecAccessT bad_access = RECA_NO_ACCESS; + std::memcpy(reinterpret_cast(&r.stat_meta) + offsetof(RecConfigMeta, access_type), &bad_access, sizeof(bad_access)); +} + +YAML::Node +encode_record_node(const RecRecord &record) +{ + // The encoder wraps the actual record fields in a {"record": ...} envelope. + return YAML::convert::encode(record)[constants_rec::REC]; +} +} // namespace + +TEST_CASE("Record YAML encoder exposes values for default-access config records", "[mgmt][rpc][record_yaml]") +{ + RecRecord rec{}; + fill_string_config(rec, "proxy.config.example.normal", "current", "default", RECA_NULL); + const YAML::Node node = encode_record_node(rec); + + REQUIRE(node[constants_rec::DATA_TYPE].as() == "STRING"); + REQUIRE(node[constants_rec::CURRENT_VALUE].as() == "current"); + REQUIRE(node[constants_rec::DEFAULT_VALUE].as() == "default"); +} + +TEST_CASE("Record YAML encoder withholds values for RECA_NO_ACCESS string records", "[mgmt][rpc][record_yaml][no_access]") +{ + RecRecord rec{}; + fill_string_config(rec, "proxy.config.example.secret", "supersecret", "secret-default", RECA_NO_ACCESS); + const YAML::Node node = encode_record_node(rec); + + // Type label and metadata are still expected so callers can enumerate the + // record's existence and tier. + REQUIRE(node[constants_rec::DATA_TYPE].as() == "STRING"); + REQUIRE(node[constants_rec::NAME].as() == "proxy.config.example.secret"); + REQUIRE(node[constants_rec::CONFIG_META][constants_rec::ACCESS_TYPE].as() == RECA_NO_ACCESS); + + // The protected value fields must not appear in the encoded output. + REQUIRE_FALSE(node[constants_rec::CURRENT_VALUE]); + REQUIRE_FALSE(node[constants_rec::DEFAULT_VALUE]); +} + +TEST_CASE("Record YAML encoder withholds values for RECA_NO_ACCESS int records", "[mgmt][rpc][record_yaml][no_access]") +{ + RecRecord rec{}; + fill_int_config(rec, "proxy.config.example.token", 42, 0, RECA_NO_ACCESS); + const YAML::Node node = encode_record_node(rec); + + REQUIRE(node[constants_rec::DATA_TYPE].as() == "INT"); + REQUIRE_FALSE(node[constants_rec::CURRENT_VALUE]); + REQUIRE_FALSE(node[constants_rec::DEFAULT_VALUE]); +} + +TEST_CASE("Record YAML encoder ignores access_type on STAT records", "[mgmt][rpc][record_yaml][union_safety]") +{ + // STAT records do not carry config_meta and must not be filtered by an + // access_type read out of the wrong half of the union. This guards the + // REC_TYPE_IS_CONFIG check that fences the new no-access logic. + RecRecord rec{}; + fill_int_stat(rec, "proxy.process.example.counter", 7); + const YAML::Node node = encode_record_node(rec); + + REQUIRE(node[constants_rec::DATA_TYPE].as() == "INT"); + REQUIRE(node[constants_rec::CURRENT_VALUE].as() == 7); + REQUIRE(node[constants_rec::DEFAULT_VALUE].as() == 0); +}