From: Marcin Siodelski Date: Wed, 11 Oct 2023 12:05:10 +0000 (+0200) Subject: [#3106] Validate configs between relationships X-Git-Tag: Kea-2.5.5~139 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=fa0e92fd1282f18e349ceece8529ae7cddd046f0;p=thirdparty%2Fkea.git [#3106] Validate configs between relationships --- diff --git a/src/hooks/dhcp/high_availability/ha_config_parser.cc b/src/hooks/dhcp/high_availability/ha_config_parser.cc index 2668803934..8619ca9f0b 100644 --- a/src/hooks/dhcp/high_availability/ha_config_parser.cc +++ b/src/hooks/dhcp/high_availability/ha_config_parser.cc @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -64,14 +65,17 @@ const SimpleDefaults HA_CONFIG_STATE_DEFAULTS = { namespace isc { namespace ha { -void -HAConfigParser::parse(const HAConfigMapperPtr& config_storage, - const ConstElementPtr& config) { +HAConfigMapperPtr +HAConfigParser::parse(const ConstElementPtr& config) { try { + auto config_storage = boost::make_shared(); + // This may cause different types of exceptions. We catch them here // and throw unified exception type. parseAllInternal(config_storage, config); + validateRelationships(config_storage); logConfigStatus(config_storage); + return (config_storage); } catch (const ConfigError& ex) { throw; @@ -376,13 +380,18 @@ HAConfigParser::parseOneInternal(const HAConfigMapperPtr& config_storage, auto peer_configs = rel_config->getAllServersConfig(); for (auto peer_config : peer_configs) { - config_storage->map(peer_config.first, rel_config); + try { + config_storage->map(peer_config.first, rel_config); + + } catch (...) { + isc_throw(HAConfigValidationError, "server names must be unique for different relationships"); + } } } template T HAConfigParser::getAndValidateInteger(const ConstElementPtr& config, - const std::string& parameter_name) const { + const std::string& parameter_name) { int64_t value = getInteger(config, parameter_name); if (value < 0) { isc_throw(ConfigError, "'" << parameter_name << "' must not be negative"); @@ -396,7 +405,7 @@ T HAConfigParser::getAndValidateInteger(const ConstElementPtr& config, } void -HAConfigParser::logConfigStatus(const HAConfigMapperPtr& config_storage) const { +HAConfigParser::logConfigStatus(const HAConfigMapperPtr& config_storage) { LOG_INFO(ha_logger, HA_CONFIGURATION_SUCCESSFUL); for (auto config : config_storage->getAll()) { @@ -437,5 +446,20 @@ HAConfigParser::logConfigStatus(const HAConfigMapperPtr& config_storage) const { } } +void +HAConfigParser::validateRelationships(const HAConfigMapperPtr& config_storage) { + auto configs = config_storage->getAll(); + if (configs.size() <= 1) { + return; + } + std::unordered_set server_names; + for (auto config : configs) { + // Only the hot-standby mode is supported for multiple relationships. + if (config->getHAMode() != HAConfig::HOT_STANDBY) { + isc_throw(HAConfigValidationError, "multiple HA relationships are only supported for 'hot-standby' mode"); + } + } +} + } // namespace ha } // namespace isc diff --git a/src/hooks/dhcp/high_availability/ha_config_parser.h b/src/hooks/dhcp/high_availability/ha_config_parser.h index 2aed9de19e..8e56ea868e 100644 --- a/src/hooks/dhcp/high_availability/ha_config_parser.h +++ b/src/hooks/dhcp/high_availability/ha_config_parser.h @@ -21,13 +21,10 @@ public: /// @brief Parses HA configuration. /// - /// @param [out] config_storage Pointer to the object where parsed configuration - /// is going to be stored. - /// /// @param config specified configuration. - /// @throw ConfigError when parsing fails or configuration is invalid. - void parse(const HAConfigMapperPtr& config_storage, - const data::ConstElementPtr& config); + /// @return Pointer to the object where parsed configuration is stored. + /// @throw HAConfigValidationError when parsing fails or configuration is invalid. + static HAConfigMapperPtr parse(const data::ConstElementPtr& config); private: @@ -37,8 +34,8 @@ private: /// is going to be stored. /// /// @param config Specified configuration. - void parseAllInternal(const HAConfigMapperPtr& config_storage, - const data::ConstElementPtr& config); + static void parseAllInternal(const HAConfigMapperPtr& config_storage, + const data::ConstElementPtr& config); /// @brief Parses HA configuration for a single relationship. /// @@ -49,8 +46,8 @@ private: /// /// @param config specified configuration for a relationship. /// @throw ConfigError when parsing fails or configuration is invalid. - void parseOneInternal(const HAConfigMapperPtr& config_storage, - const data::ElementPtr& config); + static void parseOneInternal(const HAConfigMapperPtr& config_storage, + const data::ElementPtr& config); /// @brief Validates and returns a value of the parameter. /// @@ -59,8 +56,8 @@ private: /// @param parameter_name parameter name to be fetched from the configuration. /// @tparam T parameter type, e.g. @c uint16_t, @c uint32_t etc. template - T getAndValidateInteger(const data::ConstElementPtr& config, - const std::string& parameter_name) const; + static T getAndValidateInteger(const data::ConstElementPtr& config, + const std::string& parameter_name); /// @brief Logs various information related to the successfully parsed /// configuration. @@ -70,7 +67,13 @@ private: /// /// One example of such information is a warning message indicating that /// sending lease updates is disabled. - void logConfigStatus(const HAConfigMapperPtr& config_storage) const; + static void logConfigStatus(const HAConfigMapperPtr& config_storage); + + /// @brief Validates dependencies between the relationships. + /// + /// @throw HAConfigValidationError when there are configuration errors + /// between the relationships. + static void validateRelationships(const HAConfigMapperPtr& config_storage); }; } // end of namespace isc::ha diff --git a/src/hooks/dhcp/high_availability/ha_impl.cc b/src/hooks/dhcp/high_availability/ha_impl.cc index f1d67fe1f6..9eee6f0250 100644 --- a/src/hooks/dhcp/high_availability/ha_impl.cc +++ b/src/hooks/dhcp/high_availability/ha_impl.cc @@ -28,13 +28,12 @@ namespace isc { namespace ha { HAImpl::HAImpl() - : config_(new HAConfigMapper()) { + : config_() { } void HAImpl::configure(const ConstElementPtr& input_config) { - HAConfigParser parser; - parser.parse(config_, input_config); + config_ = HAConfigParser::parse(input_config); } void diff --git a/src/hooks/dhcp/high_availability/tests/ha_config_unittest.cc b/src/hooks/dhcp/high_availability/tests/ha_config_unittest.cc index 4714d6ccad..87de5ac264 100644 --- a/src/hooks/dhcp/high_availability/tests/ha_config_unittest.cc +++ b/src/hooks/dhcp/high_availability/tests/ha_config_unittest.cc @@ -2038,7 +2038,7 @@ TEST_F(HAConfigTest, hubAndSpokeRepeatingThisServerName) { " ]" " }" "]", - "'this-server-name' must be unique for different relationships"); + "server names must be unique for different relationships"); } diff --git a/src/hooks/dhcp/high_availability/tests/ha_mt_unittest.cc b/src/hooks/dhcp/high_availability/tests/ha_mt_unittest.cc index 157448213c..f6076a61c7 100644 --- a/src/hooks/dhcp/high_availability/tests/ha_mt_unittest.cc +++ b/src/hooks/dhcp/high_availability/tests/ha_mt_unittest.cc @@ -187,9 +187,8 @@ TEST_F(HAMtServiceTest, multiThreadingBasics) { setDHCPMultiThreadingConfig(true, 3); // Create the HA configuration - HAConfigMapperPtr ha_config(new HAConfigMapper()); - HAConfigParser parser; - ASSERT_NO_THROW_LOG(parser.parse(ha_config, config_json)); + HAConfigMapperPtr ha_config; + ASSERT_NO_THROW_LOG(ha_config = HAConfigParser::parse(config_json)); // Instantiate the service. TestHAServicePtr service; @@ -308,9 +307,8 @@ TEST_F(HAMtServiceTest, multiThreadingTls) { setDHCPMultiThreadingConfig(true, 3); // Create the HA configuration - HAConfigMapperPtr ha_config(new HAConfigMapper()); - HAConfigParser parser; - ASSERT_NO_THROW_LOG(parser.parse(ha_config, config_json)); + HAConfigMapperPtr ha_config; + ASSERT_NO_THROW_LOG(ha_config = HAConfigParser::parse(config_json)); // Instantiate the service. TestHAServicePtr service; @@ -502,9 +500,8 @@ TEST_F(HAMtServiceTest, multiThreadingConfigStartup) { MultiThreadingMgr::instance().setMode(scenario.dhcp_mt_enabled_); // Create the HA configuration - HAConfigMapperPtr ha_config(new HAConfigMapper()); - HAConfigParser parser; - ASSERT_NO_THROW_LOG(parser.parse(ha_config, config_json)); + HAConfigMapperPtr ha_config; + ASSERT_NO_THROW_LOG(ha_config = HAConfigParser::parse(config_json)); // Instantiate the service. TestHAServicePtr service; diff --git a/src/hooks/dhcp/high_availability/tests/ha_service_unittest.cc b/src/hooks/dhcp/high_availability/tests/ha_service_unittest.cc index 64b4ce43f2..866f30a297 100644 --- a/src/hooks/dhcp/high_availability/tests/ha_service_unittest.cc +++ b/src/hooks/dhcp/high_availability/tests/ha_service_unittest.cc @@ -2735,9 +2735,8 @@ TEST_F(HAServiceTest, processHeartbeat) { "]"; // Parse the HA configuration. - HAConfigMapperPtr config_storage(new HAConfigMapper()); - HAConfigParser parser; - ASSERT_NO_THROW(parser.parse(config_storage, Element::fromJSON(config_text))); + HAConfigMapperPtr config_storage; + ASSERT_NO_THROW(config_storage = HAConfigParser::parse(Element::fromJSON(config_text))); TestHAService service(io_service_, network_state_, config_storage->get()); service.query_filter_.serveDefaultScopes(); diff --git a/src/hooks/dhcp/high_availability/tests/ha_test.cc b/src/hooks/dhcp/high_availability/tests/ha_test.cc index 903ce883c1..88ec2142e6 100644 --- a/src/hooks/dhcp/high_availability/tests/ha_test.cc +++ b/src/hooks/dhcp/high_availability/tests/ha_test.cc @@ -225,19 +225,13 @@ HATest::createValidPassiveBackupJsonConfiguration() const { HAConfigPtr HATest::createValidConfiguration(const HAConfig::HAMode& ha_mode) const { - HAConfigMapperPtr config_storage(new HAConfigMapper()); - HAConfigParser parser; - - parser.parse(config_storage, createValidJsonConfiguration(ha_mode)); + auto config_storage = HAConfigParser::parse(createValidJsonConfiguration(ha_mode)); return (config_storage->get()); } HAConfigPtr HATest::createValidPassiveBackupConfiguration() const { - HAConfigMapperPtr config_storage(new HAConfigMapper()); - HAConfigParser parser; - - parser.parse(config_storage, createValidPassiveBackupJsonConfiguration()); + auto config_storage = HAConfigParser::parse(createValidPassiveBackupJsonConfiguration()); return (config_storage->get()); }