From 6d6a3eed522f80ef7a691ac9f11975d396a3c454 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Wed, 2 Oct 2019 10:37:57 -0400 Subject: [PATCH] [#35,!517] Changed moveDdnsParams to modify element map instead of SrvConfig Moving the parameters needs to be done before defaults are applied to the config, so moveDdnsParams was changed to modify a mutable top level element map instead of SrvConfig contents. src/lib/dhcpsrv/parsers/simple_parser4.cc src/lib/dhcpsrv/parsers/simple_parser6.cc Change ddns-send-updates default to true. src/lib/dhcpsrv/srv_config.* SrvConfig::getConfiguredGlobal() - new method to fetch a global by name SrvConfig::moveDdnsParams() - changed to accept/modify a top-level Element map src/lib/dhcpsrv/tests/srv_config_unittest.cc updated unit tests accordingly --- src/lib/dhcpsrv/parsers/simple_parser4.cc | 8 +- src/lib/dhcpsrv/parsers/simple_parser6.cc | 6 +- src/lib/dhcpsrv/srv_config.cc | 34 ++- src/lib/dhcpsrv/srv_config.h | 31 ++- src/lib/dhcpsrv/tests/srv_config_unittest.cc | 275 +++++++++---------- 5 files changed, 194 insertions(+), 160 deletions(-) diff --git a/src/lib/dhcpsrv/parsers/simple_parser4.cc b/src/lib/dhcpsrv/parsers/simple_parser4.cc index f0ed3d2ddf..7f52460d9c 100644 --- a/src/lib/dhcpsrv/parsers/simple_parser4.cc +++ b/src/lib/dhcpsrv/parsers/simple_parser4.cc @@ -105,15 +105,15 @@ const SimpleDefaults SimpleParser4::GLOBAL4_DEFAULTS = { { "calculate-tee-times", Element::boolean, "false" }, { "t1-percent", Element::real, ".50" }, { "t2-percent", Element::real, ".875" }, - { "ddns-send-updates", Element::boolean, "false" }, + { "ddns-send-updates", Element::boolean, "true" }, { "ddns-override-no-update", Element::boolean, "false" }, { "ddns-override-client-update", Element::boolean, "false" }, { "ddns-replace-client-name", Element::string, "never" }, { "ddns-generated-prefix", Element::string, "myhost" }, // TKM should this still be true? qualifying-suffix has no default ?? - { "ddns-generated-suffix", Element::string, "" }, - { "hostname-char-set", Element::string, "" }, - { "hostname-char-replacement", Element::string, "" }, + { "ddns-qualifying-suffix", Element::string, "" }, + { "hostname-char-set", Element::string, "" }, + { "hostname-char-replacement", Element::string, "" } }; /// @brief This table defines all option definition parameters. diff --git a/src/lib/dhcpsrv/parsers/simple_parser6.cc b/src/lib/dhcpsrv/parsers/simple_parser6.cc index fcc61f5adc..a3d62e084b 100644 --- a/src/lib/dhcpsrv/parsers/simple_parser6.cc +++ b/src/lib/dhcpsrv/parsers/simple_parser6.cc @@ -100,15 +100,15 @@ const SimpleDefaults SimpleParser6::GLOBAL6_DEFAULTS = { { "calculate-tee-times", Element::boolean, "true" }, { "t1-percent", Element::real, ".50" }, { "t2-percent", Element::real, ".80" }, - { "ddns-send-updates", Element::boolean, "false" }, + { "ddns-send-updates", Element::boolean, "true" }, { "ddns-override-no-update", Element::boolean, "false" }, { "ddns-override-client-update", Element::boolean, "false" }, { "ddns-replace-client-name", Element::string, "never" }, { "ddns-generated-prefix", Element::string, "myhost" }, // TKM should this still be true? qualifying-suffix has no default ?? - { "ddns-generated-suffix", Element::string, "" }, + { "ddns-qualifying-suffix", Element::string, "" }, { "hostname-char-set", Element::string, "" }, - { "hostname-char-replacement", Element::string, "" }, + { "hostname-char-replacement", Element::string, "" } }; /// @brief This table defines all option definition parameters. diff --git a/src/lib/dhcpsrv/srv_config.cc b/src/lib/dhcpsrv/srv_config.cc index 31c7043b7b..6052aeadd5 100644 --- a/src/lib/dhcpsrv/srv_config.cc +++ b/src/lib/dhcpsrv/srv_config.cc @@ -273,6 +273,16 @@ SrvConfig::updateStatistics() { } } +isc::data::ConstElementPtr +SrvConfig::getConfiguredGlobal(std::string name) const { + isc::data::ConstElementPtr global; + if (configured_globals_->contains(name)) { + global = configured_globals_->get(name); + } + + return (global); +} + void SrvConfig::clearConfiguredGlobals() { configured_globals_ = isc::data::Element::createMap(); @@ -598,9 +608,19 @@ SrvConfig::getDdnsParams(const Subnet& subnet) const { } void -SrvConfig::moveDdnsParams(isc::data::ElementPtr d2_cfg) { - if (!d2_cfg || (d2_cfg->getType() != Element::map)) { - isc_throw(BadValue, "moveDdnsParams must be given a map element"); +SrvConfig::moveDdnsParams(isc::data::ElementPtr srv_elem) { + if (!srv_elem || (srv_elem->getType() != Element::map)) { + isc_throw(BadValue, "moveDdnsParams server config must be given a map element"); + } + + if (!srv_elem->contains("dhcp-ddns")) { + /* nothing to do */ + return; + } + + ElementPtr d2_elem = boost::const_pointer_cast(srv_elem->get("dhcp-ddns")); + if (!d2_elem || (d2_elem->getType() != Element::map)) { + isc_throw(BadValue, "moveDdnsParams dhcp-ddns is not a map"); } struct Param { @@ -619,10 +639,10 @@ SrvConfig::moveDdnsParams(isc::data::ElementPtr d2_cfg) { }; for (auto param : params) { - if (d2_cfg->contains(param.from_name)) { - if (!configured_globals_->contains(param.to_name)) { + if (d2_elem->contains(param.from_name)) { + if (!srv_elem->contains(param.to_name)) { // No global value for it already, so let's add it. - addConfiguredGlobal(param.to_name, d2_cfg->get(param.from_name)); + srv_elem->set(param.to_name, d2_elem->get(param.from_name)); LOG_INFO(dhcpsrv_logger, DHCPSRV_CFGMGR_DDNS_PARAMETER_MOVED) .arg(param.from_name).arg(param.to_name); } else { @@ -632,7 +652,7 @@ SrvConfig::moveDdnsParams(isc::data::ElementPtr d2_cfg) { } // Now remove it from d2_data, so D2ClientCfg won't complain. - d2_cfg->remove(param.from_name); + d2_elem->remove(param.from_name); } } } diff --git a/src/lib/dhcpsrv/srv_config.h b/src/lib/dhcpsrv/srv_config.h index 289a69e6ea..629a59307c 100644 --- a/src/lib/dhcpsrv/srv_config.h +++ b/src/lib/dhcpsrv/srv_config.h @@ -627,6 +627,12 @@ public: return (isc::data::ConstElementPtr(configured_globals_)); } + /// @brief Returns pointer to a given configured global parameter + /// @param name name of the parameter to fetch + /// @return Pointer to the parameter if it exists, otherwise an + /// empty pointer. + isc::data::ConstElementPtr getConfiguredGlobal(std::string name) const; + /// @brief Removes all configured global parameters. /// @note This removes the default values too so either /// @c applyDefaultsConfiguredGlobals and @c mergeGlobals, @@ -648,7 +654,30 @@ public: configured_globals_->set(name, value); } - void moveDdnsParams(isc::data::ElementPtr d2_cfg); + /// @brief Moves deprecated parameters from dhcp-ddns element to global element + /// + /// Given a server configuration element map, the following parameters are moved + /// from dhcp-ddns to top-level (i.e. global) element if they do not already + /// exist there: + /// + /// @code + /// From dhcp-ddns: To (global): + /// ------------------------------------------------------ + /// override-no-update ddns-override-no-update + /// override-client-update ddns-override-client-update + /// replace-client-name ddns-replace-client-name + /// generated-prefix ddns-generated-prefix + /// qualifying-suffix ddns-qualifying-suffix + /// hostname-char-set hostname-char-set + /// hostname-char-replacement hostname-char-replacement + /// @endcode + /// + /// Note that the whether or not the deprecated parameters are added + /// to the global element, they are always removed from the dhcp-ddns + /// element. + /// + /// @param srv_elem server top level map to alter + static void moveDdnsParams(isc::data::ElementPtr srv_elem); /// @brief Unparse a configuration object /// diff --git a/src/lib/dhcpsrv/tests/srv_config_unittest.cc b/src/lib/dhcpsrv/tests/srv_config_unittest.cc index c1d4482ac8..0dfc62a8e4 100644 --- a/src/lib/dhcpsrv/tests/srv_config_unittest.cc +++ b/src/lib/dhcpsrv/tests/srv_config_unittest.cc @@ -493,6 +493,20 @@ TEST_F(SrvConfigTest, configuredGlobals) { ADD_FAILURE() << "unexpected element found:" << global->first; } } + + // Verify that using getConfiguredGlobal() to fetch an individual + // parameters works. + ConstElementPtr global; + // We should find global "astring". + ASSERT_NO_THROW(global = conf.getConfiguredGlobal("astring")); + ASSERT_TRUE(global); + ASSERT_EQ(Element::string, global->getType()); + EXPECT_EQ("okay", global->stringValue()); + + // Not finding global "not-there" should return an empty pointer + // without throwing. + ASSERT_NO_THROW(global = conf.getConfiguredGlobal("not-there")); + ASSERT_FALSE(global); } // Verifies that the toElement method works well (tests limited to @@ -1108,8 +1122,10 @@ TEST_F(SrvConfigTest, mergeGlobals6) { } -// Verifies that parameters formerly in dhcp-ddns{} are correctly moved -// to configured globals. +// Validates SrvConfig::moveDdnsParams by ensuring that deprecated dhcp-ddns +// parameters are: +// 1. Translated to their global counterparts if they do not exist globally +// 2. Removed from the dhcp-ddns element TEST_F(SrvConfigTest, moveDdnsParamsTest) { DdnsParamsPtr params; @@ -1117,166 +1133,135 @@ TEST_F(SrvConfigTest, moveDdnsParamsTest) { struct Scenario { std::string description; - std::string d2_input; - std::string d2_output; - std::string input_globals; - std::string exp_globals; + std::string input_cfg; + std::string exp_cfg; }; std::vector scenarios { { - "scenario 1, move with no global conflicts", - // d2_input - "{\n" - " \"enable-updates\": true, \n" - " \"server-ip\" : \"192.0.2.0\",\n" - " \"server-port\" : 3432,\n" - " \"sender-ip\" : \"192.0.2.1\",\n" - " \"sender-port\" : 3433,\n" - " \"max-queue-size\" : 2048,\n" - " \"ncr-protocol\" : \"UDP\",\n" - " \"ncr-format\" : \"JSON\",\n" - " \"user-context\": { \"foo\": \"bar\" },\n" - " \"override-no-update\": true,\n" - " \"override-client-update\": false,\n" - " \"replace-client-name\": \"always\",\n" - " \"generated-prefix\": \"prefix\",\n" - " \"qualifying-suffix\": \"suffix.com.\",\n" - " \"hostname-char-set\": \"[^A-Z]\",\n" - " \"hostname-char-replacement\": \"x\"\n" - "}\n", - // d2_output - "{\n" - " \"enable-updates\": true,\n" - " \"server-ip\" : \"192.0.2.0\",\n" - " \"server-port\" : 3432,\n" - " \"sender-ip\" : \"192.0.2.1\",\n" - " \"sender-port\" : 3433,\n" - " \"max-queue-size\" : 2048,\n" - " \"ncr-protocol\" : \"UDP\",\n" - " \"ncr-format\" : \"JSON\",\n" - " \"user-context\": { \"foo\": \"bar\" }\n" - "}\n", - // input_globals - no globals to start with - "{\n" - "}\n", - // exp_globals - "{\n" - " \"ddns-override-no-update\": true,\n" - " \"ddns-override-client-update\": false,\n" - " \"ddns-replace-client-name\": \"always\",\n" - " \"ddns-generated-prefix\": \"prefix\",\n" - " \"ddns-qualifying-suffix\": \"suffix.com.\",\n" - " \"hostname-char-set\": \"[^A-Z]\",\n" - " \"hostname-char-replacement\": \"x\"\n" - "}\n" + "scenario 1, move with no global conflicts", + // input_cfg + "{\n" + " \"dhcp-ddns\": {\n" + " \"enable-updates\": true, \n" + " \"server-ip\" : \"192.0.2.0\",\n" + " \"server-port\" : 3432,\n" + " \"sender-ip\" : \"192.0.2.1\",\n" + " \"sender-port\" : 3433,\n" + " \"max-queue-size\" : 2048,\n" + " \"ncr-protocol\" : \"UDP\",\n" + " \"ncr-format\" : \"JSON\",\n" + " \"user-context\": { \"foo\": \"bar\" },\n" + " \"override-no-update\": true,\n" + " \"override-client-update\": false,\n" + " \"replace-client-name\": \"always\",\n" + " \"generated-prefix\": \"prefix\",\n" + " \"qualifying-suffix\": \"suffix.com.\",\n" + " \"hostname-char-set\": \"[^A-Z]\",\n" + " \"hostname-char-replacement\": \"x\"\n" + " }\n" + "}\n", + // exp_cfg + "{\n" + " \"dhcp-ddns\": {\n" + " \"enable-updates\": true, \n" + " \"server-ip\" : \"192.0.2.0\",\n" + " \"server-port\" : 3432,\n" + " \"sender-ip\" : \"192.0.2.1\",\n" + " \"sender-port\" : 3433,\n" + " \"max-queue-size\" : 2048,\n" + " \"ncr-protocol\" : \"UDP\",\n" + " \"ncr-format\" : \"JSON\",\n" + " \"user-context\": { \"foo\": \"bar\" }\n" + " },\n" + " \"ddns-override-no-update\": true,\n" + " \"ddns-override-client-update\": false,\n" + " \"ddns-replace-client-name\": \"always\",\n" + " \"ddns-generated-prefix\": \"prefix\",\n" + " \"ddns-qualifying-suffix\": \"suffix.com.\",\n" + " \"hostname-char-set\": \"[^A-Z]\",\n" + " \"hostname-char-replacement\": \"x\"\n" + "}\n" }, - { - "scenario 2, globals already exist for all movable params", - // d2_input - "{\n" - " \"enable-updates\": true, \n" - " \"override-no-update\": true,\n" - " \"override-client-update\": true,\n" - " \"replace-client-name\": \"always\",\n" - " \"generated-prefix\": \"prefix\",\n" - " \"qualifying-suffix\": \"suffix.com.\",\n" - " \"hostname-char-set\": \"[^A-Z]\",\n" - " \"hostname-char-replacement\": \"x\"\n" - "}\n", - // d2_output - "{\n" - " \"enable-updates\": true \n" - "}\n", - // input_globals - "{\n" - " \"ddns-override-no-update\": false,\n" - " \"ddns-override-client-update\": false,\n" - " \"ddns-replace-client-name\": \"when-present\",\n" - " \"ddns-generated-prefix\": \"org_prefix\",\n" - " \"ddns-qualifying-suffix\": \"org_suffix.com.\",\n" - " \"hostname-char-set\": \"[^a-z]\",\n" - " \"hostname-char-replacement\": \"y\"\n" - "}\n", - // exp_globals - "{\n" - " \"ddns-override-no-update\": false,\n" - " \"ddns-override-client-update\": false,\n" - " \"ddns-replace-client-name\": \"when-present\",\n" - " \"ddns-generated-prefix\": \"org_prefix\",\n" - " \"ddns-qualifying-suffix\": \"org_suffix.com.\",\n" - " \"hostname-char-set\": \"[^a-z]\",\n" - " \"hostname-char-replacement\": \"y\"\n" - "}\n" + "scenario 2, globals already exist for all movable params", + // input_cfg + "{\n" + " \"dhcp-ddns\" : {\n" + " \"enable-updates\": true, \n" + " \"override-no-update\": true,\n" + " \"override-client-update\": true,\n" + " \"replace-client-name\": \"always\",\n" + " \"generated-prefix\": \"prefix\",\n" + " \"qualifying-suffix\": \"suffix.com.\",\n" + " \"hostname-char-set\": \"[^A-Z]\",\n" + " \"hostname-char-replacement\": \"x\"\n" + " },\n" + " \"ddns-override-no-update\": false,\n" + " \"ddns-override-client-update\": false,\n" + " \"ddns-replace-client-name\": \"when-present\",\n" + " \"ddns-generated-prefix\": \"org_prefix\",\n" + " \"ddns-qualifying-suffix\": \"org_suffix.com.\",\n" + " \"hostname-char-set\": \"[^a-z]\",\n" + " \"hostname-char-replacement\": \"y\"\n" + "}\n", + // exp_cfg + "{\n" + " \"dhcp-ddns\" : {\n" + " \"enable-updates\": true\n" + " },\n" + " \"ddns-override-no-update\": false,\n" + " \"ddns-override-client-update\": false,\n" + " \"ddns-replace-client-name\": \"when-present\",\n" + " \"ddns-generated-prefix\": \"org_prefix\",\n" + " \"ddns-qualifying-suffix\": \"org_suffix.com.\",\n" + " \"hostname-char-set\": \"[^a-z]\",\n" + " \"hostname-char-replacement\": \"y\"\n" + "}\n" }, - { - "scenario 3, nothing to move", - // d2_input - "{\n" - " \"enable-updates\": true, \n" - " \"server-ip\" : \"192.0.2.0\",\n" - " \"server-port\" : 3432,\n" - " \"sender-ip\" : \"192.0.2.1\"\n" - "}\n", - // d2_output - "{\n" - " \"enable-updates\": true,\n" - " \"server-ip\" : \"192.0.2.0\",\n" - " \"server-port\" : 3432,\n" - " \"sender-ip\" : \"192.0.2.1\"\n" - "}\n", - // input_globals - "{\n" - " \"hostname-char-set\": \"[^A-Z]\",\n" - " \"hostname-char-replacement\": \"x\"\n" - "}\n", - // exp_globals - "{\n" - " \"hostname-char-set\": \"[^A-Z]\",\n" - " \"hostname-char-replacement\": \"x\"\n" - "}\n" - }, - + "scenario 3, nothing to move", + // input_cfg + "{\n" + " \"dhcp-ddns\" : {\n" + " \"enable-updates\": true, \n" + " \"server-ip\" : \"192.0.2.0\",\n" + " \"server-port\" : 3432,\n" + " \"sender-ip\" : \"192.0.2.1\"\n" + " }\n" + "}\n", + // exp_output + "{\n" + " \"dhcp-ddns\" : {\n" + " \"enable-updates\": true, \n" + " \"server-ip\" : \"192.0.2.0\",\n" + " \"server-port\" : 3432,\n" + " \"sender-ip\" : \"192.0.2.1\"\n" + " }\n" + "}\n" + } }; for (auto scenario : scenarios) { SrvConfig conf(32); - ConstElementPtr d2_input; - ConstElementPtr exp_d2_output; - ConstElementPtr input_globals; - ConstElementPtr exp_globals; - + ElementPtr input_cfg; + ConstElementPtr exp_cfg; { SCOPED_TRACE(scenario.description); - ASSERT_NO_THROW(d2_input = Element::fromJSON(scenario.d2_input)) - << "d2_input didn't parse, test is broken"; - - ASSERT_NO_THROW(exp_d2_output = Element::fromJSON(scenario.d2_output)) - << "d2_output didn't parse, test is broken"; - - ASSERT_NO_THROW(input_globals = Element::fromJSON(scenario.input_globals)) - << "input_globals didn't parse, test is broken"; - - ASSERT_NO_THROW(exp_globals = Element::fromJSON(scenario.exp_globals)) - << "exp_globals didn't parse, test is broken"; - - // Create the original set of configured globals. - for (auto input_global : input_globals->mapValue()) { - conf.addConfiguredGlobal(input_global.first, input_global.second); - } + // Parse the input cfg into a mutable Element map. + ASSERT_NO_THROW(input_cfg = boost::const_pointer_cast + (Element::fromJSON(scenario.input_cfg))) + << "input_cfg didn't parse, test is broken"; - // We need a mutable copy of d2_input to pass into the move function. - ElementPtr mutable_d2 = boost::const_pointer_cast(d2_input); - // NOw call moveDdnsParams. - ASSERT_NO_THROW(conf.moveDdnsParams(mutable_d2)); + // Parse the expected cfg into an Element map. + ASSERT_NO_THROW(exp_cfg = Element::fromJSON(scenario.exp_cfg)) + << "exp_cfg didn't parse, test is broken"; - // Make sure the content of mutable_d2 is correct. - EXPECT_TRUE(mutable_d2->equals(*exp_d2_output)); + // Now call moveDdnsParams. + ASSERT_NO_THROW(SrvConfig::moveDdnsParams(input_cfg)); - // Make sure the content of configured globals. - EXPECT_TRUE(conf.getConfiguredGlobals()->equals(*exp_globals)); + // Make sure the resultant configuration is as expected. + EXPECT_TRUE(input_cfg->equals(*exp_cfg)); } } } -- 2.47.2