[#402,!224] kea-dhcp4 now merges global values from config backend

src/bin/dhcp4/json_config_parser.*
    Moved global merge logic into SrvConfig

src/lib/dhcpsrv/srv_config.*
    SrvConfig::merge(const ConfigBase& other) - now calls protocol
    specific merge methods

    SrvConfig::merge4() - new method for v4 merges
    SrvConfig::mergeGlobals4() - new method for merging v4 globals

src/lib/dhcpsrv/tests/srv_config_unittest.cc
    TEST_F(SrvConfigTest, mergeGlobals4) - new test

src/bin/dhcp4/json_config_parser.cc

@@ -740,50 +740,17 @@ bool databaseConfigConnect(const SrvConfigPtr& srv_cfg) {
 
 
 void addGlobalsToConfig(SrvConfigPtr external_cfg, data::StampedValueCollection& cb_globals) {
-
     const auto& index = cb_globals.get<StampedValueNameIndexTag>();
-
     for (auto cb_global = index.begin(); cb_global != index.end(); ++cb_global) {
 
         if ((*cb_global)->amNull()) {
             continue;
         }
 
-        // If the global is an explicit member of SrvConfig handle it that way.
+        external_cfg->addConfiguredGlobal((*cb_global)->getName(), 
-        if (handleExplicitGlobal(external_cfg, (*cb_global))) {
+                                          (*cb_global)->getElementValue());
-            continue;
-
-        } else {
-            // Otherwise it must be added to the implicitly configured globals
-            handleImplicitGlobal(external_cfg, (*cb_global));
-        }
     }
 }
 
-
-bool handleExplicitGlobal(SrvConfigPtr external_cfg, const data::StampedValuePtr& cb_global) {
-    bool was_handled = true;
-    try {
-        const std::string& name = cb_global->getName();
-        if (name == "decline-probation-period") {
-            external_cfg->setDeclinePeriod(cb_global->getIntegerValue());
-        }
-        else if (name == "echo-client-id") {
-            external_cfg->setEchoClientId(cb_global->getValue() == "true" ? true : false);
-        } else {
-            was_handled = false;
-        }
-    } catch(const std::exception& ex) {
-       isc_throw (BadValue, "Invalid value:" << cb_global->getValue()
-                             << " explict global:" << cb_global->getName());
-    }
-
-    return (was_handled);
-}
-
-void handleImplicitGlobal(SrvConfigPtr external_cfg, const data::StampedValuePtr& cb_global) {
-    external_cfg->addConfiguredGlobal(cb_global->getName(), cb_global->getElementValue());
-}
-
 }; // end of isc::dhcp namespace
 }; // end of isc namespace

src/bin/dhcp4/json_config_parser.h

@@ -92,46 +92,15 @@ databaseConfigConnect(const SrvConfigPtr& srv_cfg);
 
 /// @brief Adds globals fetched from config backend(s) to a SrvConfig instance
 ///
-/// Iterates over the given collection of global parameters and either uses them
+/// Iterates over the given collection of global parameters and adds them to the
-/// to set explicit members of the given SrvConfig or to it's list of configured
+/// given configuration's list of configured globals.
-/// (aka implicit) globals.
 ///
 /// @param external_cfg SrvConfig instance to update
 /// @param cb_globals collection of global parameters supplied by configuration
 /// backend
-///
-/// @throw DhcpConfigError if any of the globals is not recognized as a supported
-/// value.
 void addGlobalsToConfig(SrvConfigPtr external_cfg,
                         data::StampedValueCollection& cb_globals);
 
-/// @brief Sets the appropriate member of SrvConfig from a config backend
-/// global value
-///
-/// If the given global maps to a global parameter stored explicitly as member
-/// of SrvConfig, then it's value is used to set said member.
-///
-/// @param external_cfg SrvConfig instance to update
-/// @param cb_global global parameter supplied by configuration backend
-///
-/// @return True if the global mapped to an explicit member of SrvConfig,
-/// false otherwise
-///
-/// @throw BadValue if the global's value is not the expected data type
-bool handleExplicitGlobal(SrvConfigPtr external_cfg,
-                          const data::StampedValuePtr& cb_global);
-
-/// @brief Adds a config backend global value to a SrvConfig's list of
-/// configured globals
-///
-/// The given global is converted to an Element of the appropriate type and
-/// added to the SrvConfig's list of configured globals.
-///
-/// @param external_cfg SrvConfig instance to update
-/// @param cb_global global parameter supplied by configuration backend
-void handleImplicitGlobal(SrvConfigPtr external_cfg,
-                          const data::StampedValuePtr& cb_global);
-
 }; // end of isc::dhcp namespace
 }; // end of isc namespace
 

src/bin/dhcp4/tests/config_backend_unittest.cc

@@ -152,48 +152,30 @@ public:
     /// @brief Tests that a given global is in the staged configured globals
     ///
     /// @param name name of the global parameter
-    /// @param exp_value expected string value of the global parameter
+    /// @param exp_value expected value of the global paramter as an Element
-    /// @param exp_type expected Element type of the global global parameter
     void checkConfiguredGlobal(const std::string &name,
-                               const std::string exp_value,
+                               ConstElementPtr exp_value) {
-                               const Element::types& exp_type) {
         SrvConfigPtr staging_cfg = CfgMgr::instance().getStagingCfg();
         ConstElementPtr globals = staging_cfg->getConfiguredGlobals();
         ConstElementPtr found_global = globals->get(name);
         ASSERT_TRUE(found_global) << "expected global: "
                     << name << " not found";
 
-        ASSERT_EQ(exp_type, found_global->getType())
+        ASSERT_EQ(exp_value->getType(), found_global->getType())
-                  << "global" << name << "is wrong type";
+                  << "expected global: " << name << " has wrong type";
-
-        switch (exp_type) {
-        case Element::integer:
-        case Element::real:
-        case Element::boolean:
-            ASSERT_EQ(exp_value, found_global->str())
-                      << "expected global: " << name << " has wrong value";
-            break;
-        case Element::string:
-            // We do it this way to avoid the embedded quotes
-            ASSERT_EQ(exp_value, found_global->stringValue())
-                      << "expected global: " << name << " has wrong value";
-            break;
-        default:
-            ADD_FAILURE() << "unsupported global type, test broken?";
-            return;
-        }
 
+        ASSERT_EQ(*exp_value, *found_global)
+                  << "expected global: " << name << " has wrong value";
     }
 
     /// @brief Tests that a given global is in the staged configured globals
     ///
     /// @param exp_global StampedValue representing the global value to verify
-    /// @param exp_type expected Element type of the global global parameter  -
     ///
     /// @todo At the point in time StampedVlaue carries type, exp_type should be
     /// replaced with exp_global->getType()
-    void checkConfiguredGlobal(StampedValuePtr& exp_global, const Element::types& exp_type ) {
+    void checkConfiguredGlobal(StampedValuePtr& exp_global) {
-        checkConfiguredGlobal(exp_global->getName(), exp_global->getValue(), exp_type);
+        checkConfiguredGlobal(exp_global->getName(), exp_global->getElementValue());
     }
 
     boost::scoped_ptr<Dhcpv4Srv> srv_;  ///< DHCP4 server under test
@@ -209,14 +191,14 @@ public:
 
 // This test verifies that externally configured globals are
 // merged correctly into staging configuration.
-// @todo enable test when SrvConfig can merge globals.
+TEST_F(Dhcp4CBTest, mergeGlobals) {
-TEST_F(Dhcp4CBTest, DISABLED_mergeGlobals) {
     string base_config =
         "{ \n"
         "    \"interfaces-config\": { \n"
         "        \"interfaces\": [\"*\" ] \n"
         "    }, \n"
-        "    \"echo-client-id\": true, \n"
+        "    \"echo-client-id\": false, \n"
+        "    \"decline-probation-period\": 7000, \n"
         "    \"valid-lifetime\": 1000, \n"
         "    \"rebind-timer\": 800, \n"
         "    \"server-hostname\": \"overwrite.me.com\", \n"
@@ -235,12 +217,11 @@ TEST_F(Dhcp4CBTest, DISABLED_mergeGlobals) {
     extractConfig(base_config);
 
     // Make some globals:
-    // @todo StampedValue is going to be extended to hold type.
     StampedValuePtr serverHostname(new StampedValue("server-hostname", "isc.example.org"));
-    StampedValuePtr declinePeriod(new StampedValue("decline-probation-period", "86400"));
+    StampedValuePtr declinePeriod(new StampedValue("decline-probation-period", Element::create(86400)));
-    StampedValuePtr calcTeeTimes(new StampedValue("calculate-tee-times", "false"));
+    StampedValuePtr calcTeeTimes(new StampedValue("calculate-tee-times", Element::create(bool(false))));
-    StampedValuePtr t2Percent(new StampedValue("t2-percent", "0.75"));
+    StampedValuePtr t2Percent(new StampedValue("t2-percent", Element::create(0.75)));
-    StampedValuePtr renewTimer(new StampedValue("renew-timer", "500"));
+    StampedValuePtr renewTimer(new StampedValue("renew-timer", Element::create(500)));
 
     // Let's add all of the globals to the second backend.  This will verify
     // we find them there.
@@ -257,22 +238,23 @@ TEST_F(Dhcp4CBTest, DISABLED_mergeGlobals) {
     // CfgMgr::instance().commit() hasn't been called)
     SrvConfigPtr staging_cfg = CfgMgr::instance().getStagingCfg();
 
-    // echo-client-id is an explicit member that should come from JSON.
+    // echo-client-id is set explicitly in the original config, meanwhile
-    EXPECT_TRUE(staging_cfg->getEchoClientId());
+    // the backend config does not set it, so the explicit value wins.
+    EXPECT_FALSE(staging_cfg->getEchoClientId());
 
-    // decline-probation-periodis an explicit member that should come
+    // decline-probation-period is an explicit member that should come
     // from the backend.
     EXPECT_EQ(86400, staging_cfg->getDeclinePeriod());
 
     // Verify that the implicit globals from JSON are there.
-    ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal("valid-lifetime", "1000", Element::integer));
+    ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal("valid-lifetime", Element::create(1000)));
-    ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal("rebind-timer", "800", Element::integer));
+    ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal("rebind-timer", Element::create(800)));
 
     // Verify that the implicit globals from the backend are there.
-    ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal(serverHostname, Element::string));
+    ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal(serverHostname));
-    ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal(calcTeeTimes, Element::boolean));
+    ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal(calcTeeTimes));
-    ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal(t2Percent, Element::real));
+    ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal(t2Percent));
-    ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal(renewTimer, Element::integer));
+    ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal(renewTimer));
 }
 
 // This test verifies that externally configured option definitions

src/lib/dhcpsrv/srv_config.cc

@@ -7,6 +7,7 @@
 #include <config.h>
 #include <exceptions/exceptions.h>
 #include <dhcpsrv/cfgmgr.h>
+#include <dhcpsrv/parsers/simple_parser4.h>
 #include <dhcpsrv/srv_config.h>
 #include <dhcpsrv/lease_mgr_factory.h>
 #include <dhcpsrv/cfg_hosts_util.h>
@@ -160,23 +161,13 @@ SrvConfig::equals(const SrvConfig& other) const {
 void
 SrvConfig::merge(const ConfigBase& other) {
     ConfigBase::merge(other);
-
     try {
         const SrvConfig& other_srv_config = dynamic_cast<const SrvConfig&>(other);
-
+        if (CfgMgr::instance().getFamily() == AF_INET) {
-        /// We merge objects in order of dependency (real or theoretical).
+            merge4(other_srv_config);
-        /// @todo merge globals
+        } else {
-        /// @todo merge option defs
+            // @todo merge6();
-        /// @todo merge options
+        }
-
-        // Merge shared networks.
-        cfg_shared_networks4_->merge(*(other_srv_config.getCfgSharedNetworks4()));
-
-        /// Merge subnets.
-        cfg_subnets4_->merge(getCfgSharedNetworks4(), *(other_srv_config.getCfgSubnets4()));
-
-        /// @todo merge other parts of the configuration here.
-
     } catch (const std::bad_cast&) {
         isc_throw(InvalidOperation, "internal server error: must use derivation"
                   " of the SrvConfig as an argument of the call to"
@@ -184,6 +175,59 @@ SrvConfig::merge(const ConfigBase& other) {
     }
 }
 
+void
+SrvConfig::merge4(const SrvConfig& other) {
+    /// We merge objects in order of dependency (real or theoretical).
+
+    /// Merge globals.
+    mergeGlobals4(other);
+
+    /// @todo merge option defs
+
+    /// @todo merge options
+
+    // Merge shared networks.
+    cfg_shared_networks4_->merge(*(other.getCfgSharedNetworks4()));
+
+    /// Merge subnets.
+    cfg_subnets4_->merge(getCfgSharedNetworks4(), *(other.getCfgSubnets4()));
+
+    /// @todo merge other parts of the configuration here.
+}
+
+void
+SrvConfig::mergeGlobals4(const SrvConfig& other) {
+    // Iterate over the "other" globals, adding/overwriting them into
+    // this config's list of globals.
+    for (auto other_global : other.getConfiguredGlobals()->mapValue()) {
+        addConfiguredGlobal(other_global.first, other_global.second);
+    }
+
+    // A handful of values are stored as members in SrvConfig. So we'll 
+    // iterate over the merged globals, setting approprate members.
+    for (auto merged_global : getConfiguredGlobals()->mapValue()) {
+        std::string name = merged_global.first;
+        ConstElementPtr element = merged_global.second;
+        try {
+            if (name == "decline-probation-period") {
+                setDeclinePeriod(element->intValue());
+            }
+            else if (name == "echo-client-id") {
+                setEchoClientId(element->boolValue());
+            }
+            else if (name == "dhcp4o6port") {
+                setDhcp4o6Port(element->intValue());
+            }
+            else if (name == "server-tag") {
+                setServerTag(element->stringValue());
+            }
+        } catch(const std::exception& ex) {
+            isc_throw (BadValue, "Invalid value:" << element->str()
+                       << " explict global:" << name);
+        }
+    }
+}
+
 void
 SrvConfig::removeStatistics() {
 
@@ -218,6 +262,7 @@ SrvConfig::extractConfiguredGlobals(isc::data::ConstElementPtr config) {
     for (auto value = values.begin(); value != values.end(); ++value) {
         if (value->second->getType() != Element::list &&
             value->second->getType() != Element::map) {
+                std::cout << "adding config'd global: " << value->first << std::endl;
                 addConfiguredGlobal(value->first, value->second);
         }
     }

src/lib/dhcpsrv/srv_config.h

@@ -506,6 +506,9 @@ public:
     ///
     /// The @c other parameter must be a @c SrvConfig or its derivation.
     ///
+    /// This method calls either @c merge4 or @c merge6 based on
+    /// @c CfgMgr::family_.
+    ///
     /// Currently, the following parts of the configuration are merged:
     /// - IPv4 subnets
     ///
@@ -577,7 +580,7 @@ public:
     ///
     /// See @ref setDhcp4o6Port for brief discussion.
     /// @return value of DHCP4o6 IPC port
-    uint16_t getDhcp4o6Port() {
+    uint16_t getDhcp4o6Port() const {
         return (dhcp4o6_port_);
     }
 
@@ -634,6 +637,49 @@ public:
 
 private:
 
+    /// @brief Merges the DHCPv4 configuration specified as a parameter into
+    /// this configuration.
+    ///
+    /// The general rule is that the configuration data from the @c other
+    /// object replaces configuration data held in this object instance.
+    /// The data that do not overlap between the two objects is simply
+    /// inserted into this configuration.
+    ///
+    /// @warning The call to @c merge may modify the data in the @c other
+    /// object. Therefore, the caller must not rely on the data held
+    /// in the @c other object after the call to @c merge. Also, the
+    /// data held in @c other must not be modified after the call to
+    /// @c merge because it may affect the merged configuration.
+    ///
+    /// The @c other parameter must be a @c SrvConfig or its derivation.
+    ///
+    /// Currently, the following parts of the v4 configuration are merged:
+    /// - globals
+    /// - shared-networks
+    /// - subnets
+    ///
+    /// @todo Add support for merging other configuration elements.
+    ///
+    /// @param other An object holding the configuration to be merged
+    /// into this configuration.
+    void merge4(const SrvConfig& other);
+
+
+    /// @brief Merges the DHCPv4 globals specified in the given configuration
+    /// into this configuration.
+    ///
+    /// This function iterates over the given config's explicitly
+    /// configured globals and either adds them to or updates the value
+    /// of this config's configured globals.
+    /// 
+    /// It then iterates over the merged list, setting all of the
+    /// corresponding configuration member values (e.g. c@ 
+    /// SrvConfig::echo_client_id_, @c SrvConfig::server_tag_).
+    /// 
+    /// @param other An object holding the configuration to be merged
+    /// into this configuration.
+    void mergeGlobals4(const SrvConfig& other);
+
     /// @brief Sequence number identifying the configuration.
     uint32_t sequence_;
 

src/lib/dhcpsrv/tests/srv_config_unittest.cc

@@ -990,4 +990,72 @@ TEST_F(SrvConfigTest, mergeBadCast) {
     ASSERT_THROW(srv_config.merge(non_srv_config), isc::InvalidOperation);
 }
 
+// This test verifies that globals from one SrvConfig
+// can be merged into another. It verifies that values
+// in the from-config override those in to-config which
+// override those in GLOBAL4_DEFAULTS.
+TEST_F(SrvConfigTest, mergeGlobals4) {
+    // Set the family we're working with.
+    CfgMgr::instance().setFamily(AF_INET);
+
+    // Let's create the "existing" config we will merge into.
+    SrvConfig cfg_to;
+
+    // Set some explicit values. 
+    cfg_to.setDeclinePeriod(100);
+    cfg_to.setEchoClientId(false);
+    cfg_to.setDhcp4o6Port(777);
+    cfg_to.setServerTag("not_this_server");
+
+    // Add some configured globals
+    cfg_to.addConfiguredGlobal("decline-probation-period", Element::create(300));
+    cfg_to.addConfiguredGlobal("dhcp4o6port", Element::create(888));
+
+    // Now we'll create the config we'll merge from.
+    SrvConfig cfg_from;
+
+    // Set some explicit values. None of these should be preserved.
+    cfg_from.setDeclinePeriod(200);
+    cfg_from.setEchoClientId(true);
+    cfg_from.setDhcp4o6Port(888);
+    cfg_from.setServerTag("nor_this_server");
+
+    // Add some configured globals:
+    cfg_to.addConfiguredGlobal("dhcp4o6port", Element::create(999));
+    cfg_to.addConfiguredGlobal("server-tag", Element::create("use_this_server"));
+
+    // Now let's merge.
+    ASSERT_NO_THROW(cfg_to.merge(cfg_from));
+
+    // Make sure the explicit values are set correctly.
+
+    // decline-probation-period should be the "to" configured value.
+    EXPECT_EQ(300, cfg_to.getDeclinePeriod());
+
+    // echo-client-id should be the preserved "to" member value.
+    EXPECT_EQ(false, cfg_to.getEchoClientId());
+
+    //  dhcp4o6port should be the "from" configured value.
+    EXPECT_EQ(999, cfg_to.getDhcp4o6Port());
+
+    //  server-tag port should be the "from" configured value.
+    EXPECT_EQ("use_this_server", cfg_to.getServerTag());
+
+    // Next we check the explicitly "configured" globals. 
+    // The list should be all of the "to" + "from", with the
+    // latter overwriting the former.
+    std::string exp_globals =
+        "{ \n"
+        "   \"decline-probation-period\": 300,  \n"
+        "   \"dhcp4o6port\": 999,  \n"
+        "   \"server-tag\": \"use_this_server\"  \n"
+        "} \n";
+
+    ConstElementPtr expected_globals;
+    ASSERT_NO_THROW(expected_globals = Element::fromJSON(exp_globals))
+                    << "exp_globals didn't parse, test is broken";
+
+    EXPECT_TRUE(expected_globals->equals(*(cfg_to.getConfiguredGlobals())));
+}
+
 } // end of anonymous namespace