[#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 26688039346edcac69bd7afd2f345fd00157e44b..8619ca9f0bfb7d2c5083d7c97847d7d905b3bd7a 100644 (file)
--- 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 <ha_service_states.h>
 #include <cc/dhcp_config_error.h>
 #include <util/file_utilities.h>
+#include <boost/make_shared.hpp>
 #include <limits>
 #include <set>
 
@@ -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<HAConfigMapper>();
+
         // 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<typename T>
 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<std::string> 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 2aed9de19eb41e3e5b78f62aaee8014fe30c8c84..8e56ea868e63997d0e7545b1d4d5719c1daa8afb 100644 (file)
--- 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<typename T>
-    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 f1d67fe1f63f367d5c6f056e684cc3379dc2911e..9eee6f02504662dd1854fb2395252bd5bd06cf55 100644 (file)
--- 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 4714d6ccadc42a3de608437de264cd772a8459c0..87de5ac264de1d8dec794dd0b327b9f99a3c88d6 100644 (file)
--- 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 157448213cacfc9a595d0917f61073aec2c13b4c..f6076a61c7d55c986c710e36042ce7ac4fcbd089 100644 (file)
--- 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 64b4ce43f28bdf89999f84e558602c26a7503c9d..866f30a2973008fd469d0b462a7a329b534a5d4b 100644 (file)
--- 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 903ce883c1fae34fe839a6164411f877a06bced8..88ec2142e6749185537dfbadaa9fa0a7517b5b93 100644 (file)
--- 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());
 }