[#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

diff --git a/src/lib/dhcpsrv/parsers/simple_parser4.cc b/src/lib/dhcpsrv/parsers/simple_parser4.cc
index f0ed3d2ddf2755710831587b2072fe8650927711..7f52460d9c0ceee1bc10acd0f63b4508ac75fa72 100644 (file)
--- 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 fcc61f5adc778e80ed314489fb76746a82dddb77..a3d62e084b14b15c75b78f1cb8430e16c7a28066 100644 (file)
--- 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 31c7043b7b2a08aeb5f8bc671692488f3f78459a..6052aeadd51132dc6e84847c6fca6732d65d1c10 100644 (file)
--- 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<Element>(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 289a69e6eaef55adde13eb0fe12aba55692d597f..629a59307cd28aef67a0c9520ea810049f906108 100644 (file)
--- 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 c1d4482ac8afb622b03fd1a4648c8b6476d06c63..0dfc62a8e4c42beb6dc4d5f47c9c6b8bc4a188ad 100644 (file)
--- 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<Scenario> 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>
+                            (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<Element>(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));
         }
     }
 }