From: Thomas Markwalder Date: Wed, 20 Feb 2019 16:22:49 +0000 (-0500) Subject: [#402,!224] kea-dhcp4 now merges global values from config backend X-Git-Tag: 397-cb-implement-mysqlconfigbackenddhcpv6_base~14^2~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=16d2eaa574edb0ec7766649f12f6b1c039f0422e;p=thirdparty%2Fkea.git [#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 --- diff --git a/src/bin/dhcp4/json_config_parser.cc b/src/bin/dhcp4/json_config_parser.cc index 6d6351b860..0fe2094b74 100644 --- a/src/bin/dhcp4/json_config_parser.cc +++ b/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(); - 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. - if (handleExplicitGlobal(external_cfg, (*cb_global))) { - continue; - - } else { - // Otherwise it must be added to the implicitly configured globals - handleImplicitGlobal(external_cfg, (*cb_global)); - } + external_cfg->addConfiguredGlobal((*cb_global)->getName(), + (*cb_global)->getElementValue()); } } - -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 diff --git a/src/bin/dhcp4/json_config_parser.h b/src/bin/dhcp4/json_config_parser.h index 8202d3f0b9..aad0c2d0fb 100644 --- a/src/bin/dhcp4/json_config_parser.h +++ b/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 -/// to set explicit members of the given SrvConfig or to it's list of configured -/// (aka implicit) globals. +/// Iterates over the given collection of global parameters and adds them to the +/// given configuration's list of configured 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 diff --git a/src/bin/dhcp4/tests/config_backend_unittest.cc b/src/bin/dhcp4/tests/config_backend_unittest.cc index c9659a3e08..59078ac8f6 100644 --- a/src/bin/dhcp4/tests/config_backend_unittest.cc +++ b/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_type expected Element type of the global global parameter + /// @param exp_value expected value of the global paramter as an Element void checkConfiguredGlobal(const std::string &name, - const std::string exp_value, - const Element::types& exp_type) { + ConstElementPtr exp_value) { 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()) - << "global" << name << "is 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->getType(), found_global->getType()) + << "expected global: " << name << " has wrong type"; + 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 ) { - checkConfiguredGlobal(exp_global->getName(), exp_global->getValue(), exp_type); + void checkConfiguredGlobal(StampedValuePtr& exp_global) { + checkConfiguredGlobal(exp_global->getName(), exp_global->getElementValue()); } boost::scoped_ptr 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, DISABLED_mergeGlobals) { +TEST_F(Dhcp4CBTest, 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 calcTeeTimes(new StampedValue("calculate-tee-times", "false")); - StampedValuePtr t2Percent(new StampedValue("t2-percent", "0.75")); - StampedValuePtr renewTimer(new StampedValue("renew-timer", "500")); + StampedValuePtr declinePeriod(new StampedValue("decline-probation-period", Element::create(86400))); + StampedValuePtr calcTeeTimes(new StampedValue("calculate-tee-times", Element::create(bool(false)))); + StampedValuePtr t2Percent(new StampedValue("t2-percent", Element::create(0.75))); + 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. - EXPECT_TRUE(staging_cfg->getEchoClientId()); + // echo-client-id is set explicitly in the original config, meanwhile + // 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("rebind-timer", "800", Element::integer)); + ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal("valid-lifetime", Element::create(1000))); + 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(calcTeeTimes, Element::boolean)); - ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal(t2Percent, Element::real)); - ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal(renewTimer, Element::integer)); + ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal(serverHostname)); + ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal(calcTeeTimes)); + ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal(t2Percent)); + ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal(renewTimer)); } // This test verifies that externally configured option definitions diff --git a/src/lib/dhcpsrv/srv_config.cc b/src/lib/dhcpsrv/srv_config.cc index ac04646ae3..6794dc1209 100644 --- a/src/lib/dhcpsrv/srv_config.cc +++ b/src/lib/dhcpsrv/srv_config.cc @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include @@ -160,27 +161,70 @@ SrvConfig::equals(const SrvConfig& other) const { void SrvConfig::merge(const ConfigBase& other) { ConfigBase::merge(other); - try { const SrvConfig& other_srv_config = dynamic_cast(other); + if (CfgMgr::instance().getFamily() == AF_INET) { + merge4(other_srv_config); + } else { + // @todo merge6(); + } + } catch (const std::bad_cast&) { + isc_throw(InvalidOperation, "internal server error: must use derivation" + " of the SrvConfig as an argument of the call to" + " SrvConfig::merge()"); + } +} - /// We merge objects in order of dependency (real or theoretical). - /// @todo merge globals - /// @todo merge option defs - /// @todo merge options +void +SrvConfig::merge4(const SrvConfig& other) { + /// We merge objects in order of dependency (real or theoretical). - // Merge shared networks. - cfg_shared_networks4_->merge(*(other_srv_config.getCfgSharedNetworks4())); + /// Merge globals. + mergeGlobals4(other); - /// Merge subnets. - cfg_subnets4_->merge(getCfgSharedNetworks4(), *(other_srv_config.getCfgSubnets4())); + /// @todo merge option defs - /// @todo merge other parts of the configuration here. + /// @todo merge options - } catch (const std::bad_cast&) { - isc_throw(InvalidOperation, "internal server error: must use derivation" - " of the SrvConfig as an argument of the call to" - " SrvConfig::merge()"); + // 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); + } } } @@ -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); } } diff --git a/src/lib/dhcpsrv/srv_config.h b/src/lib/dhcpsrv/srv_config.h index c30e1304fd..54849ff272 100644 --- a/src/lib/dhcpsrv/srv_config.h +++ b/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_; diff --git a/src/lib/dhcpsrv/tests/srv_config_unittest.cc b/src/lib/dhcpsrv/tests/srv_config_unittest.cc index 9ea12d9d3d..cff1d6b23d 100644 --- a/src/lib/dhcpsrv/tests/srv_config_unittest.cc +++ b/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