From f8a1ebf4e0988757860c94ff0ccb05a0a488a1de Mon Sep 17 00:00:00 2001 From: Andrea Barbadoro Date: Fri, 15 Dec 2023 16:39:05 +0100 Subject: [PATCH 1/6] config/config_store: handle aliases for property names before this commit, finding the name in the alias table would not throw the std::invalid_argument exception. but would not set the property. --- src/v/config/config_store.h | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/v/config/config_store.h b/src/v/config/config_store.h index 2d6d522cda8e1..b5b3c6db98936 100644 --- a/src/v/config/config_store.h +++ b/src/v/config/config_store.h @@ -70,16 +70,20 @@ class config_store { for (auto const& node : root_node) { auto name = node.first.as(); - auto found = _properties.find(name); - if (found == _properties.end()) { - found = _aliases.find(name); - if (found == _aliases.end()) { - if (!ignore_missing.contains(name)) { - throw std::invalid_argument( - fmt::format("Unknown property {}", name)); - } + auto found_opt = std::optional{_properties.find(name)}; + if (found_opt == _properties.end()) { + found_opt = _aliases.find(name); + if (found_opt == _aliases.end()) { + found_opt.reset(); + } + } + if (!found_opt) { + if (!ignore_missing.contains(name)) { + throw std::invalid_argument( + fmt::format("Unknown property {}", name)); } } else { + auto& found = found_opt.value(); bool ok = false; try { auto validation_err = found->second->validate(node.second); From f0e8f8ac57b006b070821a6edfcebd4fb45ac551 Mon Sep 17 00:00:00 2001 From: Andrea Barbadoro Date: Fri, 15 Dec 2023 16:45:44 +0100 Subject: [PATCH 2/6] config/config_store: intermediary commit to remove an indentation level this is a non-functional change --- src/v/config/config_store.h | 52 ++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/src/v/config/config_store.h b/src/v/config/config_store.h index b5b3c6db98936..8485e87277496 100644 --- a/src/v/config/config_store.h +++ b/src/v/config/config_store.h @@ -82,34 +82,34 @@ class config_store { throw std::invalid_argument( fmt::format("Unknown property {}", name)); } - } else { - auto& found = found_opt.value(); - bool ok = false; - try { - auto validation_err = found->second->validate(node.second); - if (validation_err.has_value()) { - errors[name] = fmt::format( - "Validation error: {}", - validation_err.value().error_message()); - } - - found->second->set_value(node.second); - ok = true; - } catch (YAML::InvalidNode const& e) { - errors[name] = fmt::format("Invalid syntax: {}", e); - } catch (YAML::ParserException const& e) { - errors[name] = fmt::format("Invalid syntax: {}", e); - } catch (YAML::BadConversion const& e) { - errors[name] = fmt::format("Invalid value: {}", e); + continue; + } + auto& found = found_opt.value(); + bool ok = false; + try { + auto validation_err = found->second->validate(node.second); + if (validation_err.has_value()) { + errors[name] = fmt::format( + "Validation error: {}", + validation_err.value().error_message()); } - // A validation error is fatal if the property was required, - // e.g. if someone entered a non-integer node_id, or an invalid - // internal RPC address. - if (!ok && found->second->is_required()) { - throw std::invalid_argument(fmt::format( - "Property {} is required and has invalid value", name)); - } + found->second->set_value(node.second); + ok = true; + } catch (YAML::InvalidNode const& e) { + errors[name] = fmt::format("Invalid syntax: {}", e); + } catch (YAML::ParserException const& e) { + errors[name] = fmt::format("Invalid syntax: {}", e); + } catch (YAML::BadConversion const& e) { + errors[name] = fmt::format("Invalid value: {}", e); + } + + // A validation error is fatal if the property was required, + // e.g. if someone entered a non-integer node_id, or an invalid + // internal RPC address. + if (!ok && found->second->is_required()) { + throw std::invalid_argument(fmt::format( + "Property {} is required and has invalid value", name)); } } From 35ed70687f7d45e6425dddc956251c1a9a31d8c2 Mon Sep 17 00:00:00 2001 From: Andrea Barbadoro Date: Fri, 15 Dec 2023 17:22:15 +0100 Subject: [PATCH 3/6] config/config_store: optional to ptr non functional change, the optional was just used inside a code block --- src/v/config/config_store.h | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/src/v/config/config_store.h b/src/v/config/config_store.h index 8485e87277496..b8d7c4f97620c 100644 --- a/src/v/config/config_store.h +++ b/src/v/config/config_store.h @@ -70,31 +70,35 @@ class config_store { for (auto const& node : root_node) { auto name = node.first.as(); - auto found_opt = std::optional{_properties.find(name)}; - if (found_opt == _properties.end()) { - found_opt = _aliases.find(name); - if (found_opt == _aliases.end()) { - found_opt.reset(); + auto* prop = [&]() -> base_property* { + auto found = _properties.find(name); + if (found != _properties.end()) { + return found->second; } - } - if (!found_opt) { + found = _aliases.find(name); + if (found != _aliases.end()) { + return found->second; + } + + return nullptr; + }(); + + if (prop == nullptr) { if (!ignore_missing.contains(name)) { throw std::invalid_argument( fmt::format("Unknown property {}", name)); } continue; } - auto& found = found_opt.value(); bool ok = false; try { - auto validation_err = found->second->validate(node.second); + auto validation_err = prop->validate(node.second); if (validation_err.has_value()) { errors[name] = fmt::format( "Validation error: {}", validation_err.value().error_message()); } - - found->second->set_value(node.second); + prop->set_value(node.second); ok = true; } catch (YAML::InvalidNode const& e) { errors[name] = fmt::format("Invalid syntax: {}", e); @@ -107,7 +111,7 @@ class config_store { // A validation error is fatal if the property was required, // e.g. if someone entered a non-integer node_id, or an invalid // internal RPC address. - if (!ok && found->second->is_required()) { + if (!ok && prop->is_required()) { throw std::invalid_argument(fmt::format( "Property {} is required and has invalid value", name)); } From bebf4aaf0cee370f72a6da2beb625ab52f723605 Mon Sep 17 00:00:00 2001 From: Andrea Barbadoro Date: Mon, 2 Oct 2023 23:26:29 +0200 Subject: [PATCH 4/6] config/config_store property_aliases() property_names_and_aliases() property_aliases() mirrors property_names() and both get merged in property_names_and_aliases(), to be used instead of property_names when filtering/scanning over user input --- src/v/config/config_store.h | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/v/config/config_store.h b/src/v/config/config_store.h index b8d7c4f97620c..254e23e696c8a 100644 --- a/src/v/config/config_store.h +++ b/src/v/config/config_store.h @@ -194,6 +194,21 @@ class config_store { return result; } + std::set property_aliases() const { + std::set result; + for (const auto& i : _aliases) { + result.insert(i.first); + } + + return result; + } + + std::set property_names_and_aliases() const { + auto all = property_names(); + all.merge(property_aliases()); + return all; + } + friend std::ostream& operator<<(std::ostream& o, const config::config_store& c) { o << "{ "; From b025e3478b05c96c827e58d2b28a62edbbd5923a Mon Sep 17 00:00:00 2001 From: Andrea Barbadoro Date: Fri, 15 Dec 2023 17:59:40 +0100 Subject: [PATCH 5/6] config: use property_names_and_aliases() this allows to include prop aliases when validating external input --- src/v/cluster/config_manager.cc | 2 +- src/v/config/configuration.cc | 4 ++-- src/v/config/node_config.cc | 5 +++-- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/v/cluster/config_manager.cc b/src/v/cluster/config_manager.cc index d0b23804b0f0d..e83d380431b1a 100644 --- a/src/v/cluster/config_manager.cc +++ b/src/v/cluster/config_manager.cc @@ -413,7 +413,7 @@ config_manager::preload(YAML::Node const& legacy_config) { // to set something in redpanda.yaml and it's not working. if (legacy_config["redpanda"]) { const auto nag_properties - = config::shard_local_cfg().property_names(); + = config::shard_local_cfg().property_names_and_aliases(); for (auto const& node : legacy_config["redpanda"]) { auto name = node.first.as(); if (nag_properties.contains(name)) { diff --git a/src/v/config/configuration.cc b/src/v/config/configuration.cc index 04fe1e8a1b580..ed1d25a43ccde 100644 --- a/src/v/config/configuration.cc +++ b/src/v/config/configuration.cc @@ -2867,9 +2867,9 @@ configuration::error_map_t configuration::load(const YAML::Node& root_node) { throw std::invalid_argument("'redpanda' root is required"); } - const auto& ignore = node().property_names(); + auto ignore = node().property_names_and_aliases(); - return config_store::read_yaml(root_node["redpanda"], ignore); + return config_store::read_yaml(root_node["redpanda"], std::move(ignore)); } configuration& shard_local_cfg() { diff --git a/src/v/config/node_config.cc b/src/v/config/node_config.cc index 68918c3889e84..8aac0e3dac669 100644 --- a/src/v/config/node_config.cc +++ b/src/v/config/node_config.cc @@ -245,9 +245,10 @@ node_config::error_map_t node_config::load(const YAML::Node& root_node) { throw std::invalid_argument("'redpanda' root is required"); } - const auto& ignore = shard_local_cfg().property_names(); + auto ignore = shard_local_cfg().property_names_and_aliases(); - auto errors = config_store::read_yaml(root_node["redpanda"], ignore); + auto errors = config_store::read_yaml( + root_node["redpanda"], std::move(ignore)); validate_multi_node_property_config(errors); return errors; } From b76f908ca2a93d71f2c3c9839baac32b67939bc0 Mon Sep 17 00:00:00 2001 From: Andrea Barbadoro Date: Mon, 18 Dec 2023 19:25:23 +0100 Subject: [PATCH 6/6] config/config_store_test: unit testing for aliases in yaml source --- src/v/config/tests/config_store_test.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/v/config/tests/config_store_test.cc b/src/v/config/tests/config_store_test.cc index d8d3a858109ab..9a91d5867f4f7 100644 --- a/src/v/config/tests/config_store_test.cc +++ b/src/v/config/tests/config_store_test.cc @@ -136,7 +136,8 @@ YAML::Node valid_configuration() { " - two\n" " - three\n" "nullable_int: 111\n" - "secret_string: actual_secret\n"); + "secret_string: actual_secret\n" + "aliased_bool_legacy: false\n"); } } // namespace @@ -214,6 +215,7 @@ SEASTAR_THREAD_TEST_CASE(read_valid_configuration) { BOOST_TEST(cfg.strings().at(2) == "three"); BOOST_TEST(cfg.nullable_int() == std::make_optional(111)); BOOST_TEST(cfg.secret_string() == "actual_secret"); + BOOST_TEST(cfg.aliased_bool() == false); }; SEASTAR_THREAD_TEST_CASE(update_property_value) { @@ -525,4 +527,6 @@ SEASTAR_THREAD_TEST_CASE(property_aliasing) { auto property_names = cfg.property_names(); BOOST_TEST(property_names.contains("aliased_bool") == true); BOOST_TEST(property_names.contains("aliased_bool_legacy") == false); + + BOOST_TEST(cfg.property_names_and_aliases().contains("aliased_bool")); }