[5033] Fixed defaults and enable-updates=false shortcut problems

src/bin/dhcp4/json_config_parser.cc

@@ -637,6 +637,10 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) {
             }
 
             if (config_pair.first == "dhcp-ddns") {
+		// Apply defaults if not in short cut
+		if (!!D2ClientConfigParser::isShortCutDisabled(config_pair.second)) {
+		    D2ClientConfigParser::setAllDefaults(config_pair.second);
+		}
                 D2ClientConfigParser parser;
                 D2ClientConfigPtr cfg = parser.parse(config_pair.second);
                 CfgMgr::instance().getStagingCfg()->setD2ClientConfig(cfg);

src/bin/dhcp4/simple_parser4.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2016-2017 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2016 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -71,27 +71,6 @@ const ParamsList SimpleParser4::INHERIT_GLOBAL_TO_SUBNET4 = {
     "rebind-timer",
     "valid-lifetime"
 };
-
-/// @brief This table defines default values for D2 client configuration
-///
-const SimpleDefaults SimpleParser4::D2_CLIENT_CONFIG_DEFAULTS = {
-    { "server-ip", Element::string, "127.0.0.1" },
-    { "server-port", Element::integer, "53001" },
-    // default sender-ip depends on server-ip family, so we leave default blank
-    // parser knows to use the appropriate ZERO address based on server-ip
-    { "sender-ip", Element::string, "" },
-    { "sender-port", Element::integer, "0" },
-    { "max-queue-size", Element::integer, "1024" },
-    { "ncr-protocol", Element::string, "UDP" },
-    { "ncr-format", Element::string, "JSON" },
-    { "always-include-fqdn", Element::boolean, "false" },
-    { "override-no-update", Element::boolean, "false" },
-    { "override-client-update", Element::boolean, "false" },
-    { "replace-client-name", Element::string, "NEVER" },
-    { "generated-prefix", Element::string, "myhost" },
-    { "qualifying-suffix", Element::string, "" }
-};
-
 /// @}
 
 /// ---------------------------------------------------------------------------
@@ -120,14 +99,6 @@ size_t SimpleParser4::setAllDefaults(isc::data::ElementPtr global) {
         }
     }
 
-    ConstElementPtr d2_client = global->get("dhcp-ddns");
-    /// @todo - what if it's not in global? should we add it?
-    if (d2_client) {
-        // Get the mutable form of d2 client config
-        ElementPtr mutable_d2 = boost::const_pointer_cast<Element>(d2_client);
-        cnt += SimpleParser::setDefaults(mutable_d2, D2_CLIENT_CONFIG_DEFAULTS);
-    }
-
     return (cnt);
 }
 

src/bin/dhcp4/simple_parser4.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2016-2017 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2016 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -33,7 +33,6 @@ public:
     static const isc::data::SimpleDefaults OPTION4_DEFAULTS;
     static const isc::data::SimpleDefaults GLOBAL4_DEFAULTS;
     static const isc::data::ParamsList INHERIT_GLOBAL_TO_SUBNET4;
-    static const isc::data::SimpleDefaults D2_CLIENT_CONFIG_DEFAULTS;
 };
 
 };

src/bin/dhcp6/json_config_parser.cc

@@ -911,6 +911,10 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) {
             }
 
             if (config_pair.first == "dhcp-ddns") {
+		// Apply defaults if not in short cut
+		if (!!D2ClientConfigParser::isShortCutDisabled(config_pair.second)) {
+		    D2ClientConfigParser::setAllDefaults(config_pair.second);
+		}
                 D2ClientConfigParser parser;
                 D2ClientConfigPtr cfg = parser.parse(config_pair.second);
                 CfgMgr::instance().getStagingCfg()->setD2ClientConfig(cfg);

src/bin/dhcp6/simple_parser6.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2016-2017 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2016 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -73,27 +73,6 @@ const ParamsList SimpleParser6::INHERIT_GLOBAL_TO_SUBNET6 = {
     "preferred-lifetime",
     "valid-lifetime"
 };
-
-/// @brief This table defines default values for D2 client configuration
-///
-const SimpleDefaults SimpleParser6::D2_CLIENT_CONFIG_DEFAULTS = {
-    { "server-ip", Element::string, "127.0.0.1" },
-    { "server-port", Element::integer, "53001" },
-    // default sender-ip depends on server-ip family, so we leave default blank
-    // parser knows to use the appropriate ZERO address based on server-ip
-    { "sender-ip", Element::string, "" },
-    { "sender-port", Element::integer, "0" },
-    { "max-queue-size", Element::integer, "1024" },
-    { "ncr-protocol", Element::string, "UDP" },
-    { "ncr-format", Element::string, "JSON" },
-    { "always-include-fqdn", Element::boolean, "false" },
-    { "override-no-update", Element::boolean, "false" },
-    { "override-client-update", Element::boolean, "false" },
-    { "replace-client-name", Element::string, "NEVER" },
-    { "generated-prefix", Element::string, "myhost" },
-    { "qualifying-suffix", Element::string, "" }
-};
-
 /// @}
 
 /// ---------------------------------------------------------------------------
@@ -122,14 +101,6 @@ size_t SimpleParser6::setAllDefaults(isc::data::ElementPtr global) {
         }
     }
 
-    ConstElementPtr d2_client = global->get("dhcp-ddns");
-    /// @todo - what if it's not in global? should we add it?
-    if (d2_client) {
-        // Get the mutable form of d2 client config
-        ElementPtr mutable_d2 = boost::const_pointer_cast<Element>(d2_client);
-        cnt += SimpleParser::setDefaults(mutable_d2, D2_CLIENT_CONFIG_DEFAULTS);
-    }
-
     return (cnt);
 }
 

src/bin/dhcp6/simple_parser6.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2016-2017 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2016 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -34,7 +34,6 @@ public:
     static const isc::data::SimpleDefaults OPTION6_DEFAULTS;
     static const isc::data::SimpleDefaults GLOBAL6_DEFAULTS;
     static const isc::data::ParamsList INHERIT_GLOBAL_TO_SUBNET6;
-    static const isc::data::SimpleDefaults D2_CLIENT_CONFIG_DEFAULTS;
 };
 
 };

src/lib/dhcpsrv/parsers/dhcp_parsers.cc

@@ -1257,6 +1257,16 @@ D2ClientConfigParser::parse(isc::data::ConstElementPtr client_config) {
     std::string qualifying_suffix;
     std::string current_param;
 
+    
+    if (isShortCutDisabled(client_config)) {
+      // If enable-updates is the only parameter and it is false then
+      // we're done.  This allows for an abbreviated configuration entry
+      // that only contains that flag.  Use the default D2ClientConfig
+      // constructor to a create a disabled instance.
+      new_config.reset(new D2ClientConfig());
+      return (new_config);
+    }
+
     // Get all parameters that are needed to create the D2ClientConfig.
     // We fetch all the parameters inside their own try clause so we
     // spit out an error with an accurate text position.  Use the
@@ -1388,5 +1398,47 @@ D2ClientConfigParser::parse(isc::data::ConstElementPtr client_config) {
     return(new_config);
 }
 
+bool
+D2ClientConfigParser::isShortCutDisabled(isc::data::ConstElementPtr d2_config) {
+    if (!d2_config->contains("enable-updates")) {
+        isc_throw(DhcpConfigError,
+                  "Mandatory parameter 'enable-updates' missing ("
+                  << d2_config->getPosition() << ")");
+    }
+    ConstElementPtr enable = d2_config->get("enable-updates");
+    if (enable->getType() != Element::boolean) {
+        isc_throw(DhcpConfigError,
+                  "invalid value type specified for parameter"
+                  " 'enable-updates' (" << enable->getPosition() << ")");
+    }
+    return (!enable->boolValue() && (d2_config->mapValue().size() == 1));
+}
+
+/// @brief This table defines default values for D2 client configuration
+const SimpleDefaults D2ClientConfigParser::D2_CLIENT_CONFIG_DEFAULTS = {
+    // enable-updates is unconditionally required
+    { "server-ip", Element::string, "127.0.0.1" },
+    { "server-port", Element::integer, "53001" },
+    // default sender-ip depends on server-ip family, so we leave default blank
+    // parser knows to use the appropriate ZERO address based on server-ip
+    { "sender-ip", Element::string, "" },
+    { "sender-port", Element::integer, "0" },
+    { "max-queue-size", Element::integer, "1024" },
+    { "ncr-protocol", Element::string, "UDP" },
+    { "ncr-format", Element::string, "JSON" },
+    { "always-include-fqdn", Element::boolean, "false" },
+    { "override-no-update", Element::boolean, "false" },
+    { "override-client-update", Element::boolean, "false" },
+    { "replace-client-name", Element::string, "never" },
+    { "generated-prefix", Element::string, "myhost" }
+    // qualifying-suffix has no default
+};
+
+size_t
+D2ClientConfigParser::setAllDefaults(isc::data::ConstElementPtr d2_config) {
+    ElementPtr mutable_d2 = boost::const_pointer_cast<Element>(d2_config);
+    return (SimpleParser::setDefaults(mutable_d2, D2_CLIENT_CONFIG_DEFAULTS));
+}
+
 };  // namespace dhcp
 };  // namespace isc

src/lib/dhcpsrv/parsers/dhcp_parsers.h

@@ -1053,6 +1053,30 @@ public:
     ///
     /// @return returns a pointer to newly created D2ClientConfig.
     D2ClientConfigPtr parse(isc::data::ConstElementPtr d2_client_cfg);
+
+    /// @brief Check the short cut disabled updates condition
+    ///
+    /// The condition is that the d2 client configuration is
+    /// reduced to "enable-updates": false
+    ///
+    /// @param d2_config d2 client configuration
+    /// @return true if and only if the condition matches.
+    /// @throw DhcpConfigError if enable-updates is not present or
+    /// is not a boolean
+    static bool isShortCutDisabled(isc::data::ConstElementPtr d2_config);
+
+    /// @brief Defaults for the D2 client configuration.
+    static const isc::data::SimpleDefaults D2_CLIENT_CONFIG_DEFAULTS;
+
+    /// @brief Sets all defaults for D2 client configuration.
+    ///
+    /// This method sets defaults value. It must not be called
+    /// before the short cut disabled updates condition was checked.
+    ///
+    /// @param d2_config d2 client configuration (will be const cast
+    //  to ElementPtr)
+    /// @return number of parameters inserted
+    static size_t setAllDefaults(isc::data::ConstElementPtr d2_config);
 };
 
 // Pointers to various parser objects.

src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc

@@ -302,9 +302,6 @@ public:
             return (answer);
         }
 
-        // option parsing must be done last, so save it if we hit if first
-        ParserPtr option_parser;
-
         ConfigPair config_pair;
         try {
             // Iterate over the config elements.
@@ -334,14 +331,8 @@ public:
 
                 // Create the parser based on element name.
                 ParserPtr parser(createConfigParser(config_pair.first));
-                // Options must be parsed last
-                if (config_pair.first == "option-data") {
-                    option_parser = parser;
-                } else {
-                    // Anything else  we can call build straight away.
-                    parser->build(config_pair.second);
-                    parser->commit();
-                }
+                parser->build(config_pair.second);
+                parser->commit();
             }
 
             int family = parser_context_->universe_ == Option::V4
@@ -434,8 +425,7 @@ public:
     size_t setAllDefaults(isc::data::ElementPtr global,
                           const SimpleDefaults& global_defaults,
                           const SimpleDefaults& option_defaults,
-                          const SimpleDefaults& option_def_defaults,
-                          const SimpleDefaults& d2_client_defaults) {
+                          const SimpleDefaults& option_def_defaults) {
         size_t cnt = 0;
         // Set global defaults first.
         cnt = SimpleParser::setDefaults(global, global_defaults);
@@ -455,16 +445,6 @@ public:
             }
         }
 
-        ConstElementPtr d2_client = global->get("dhcp-ddns");
-        /// @todo - what if it's not in global? should we add it?
-        if (d2_client) {
-            // Get the mutable form of d2 client config
-            ElementPtr mutable_d2 =
-                boost::const_pointer_cast<Element>(d2_client);
-            cnt += SimpleParser::setDefaults(mutable_d2, d2_client_defaults);
-        }
-
-
         return (cnt);
     }
 
@@ -521,31 +501,19 @@ public:
             { "valid-lifetime",     Element::integer, "7200" }
         };
 
-        /// @brief This table defines default values for D2 client configuration
-        const SimpleDefaults D2_CLIENT_CFG_DEFAULTS = {
-            { "server-ip", Element::string, "127.0.0.1" },
-            { "server-port", Element::integer, "53001" },
-            // default sender-ip depends on server-ip family, so we leave default blank
-            // parser knows to use the appropriate ZERO address based on server-ip
-            { "sender-ip", Element::string, "" },
-            { "sender-port", Element::integer, "0" },
-            { "max-queue-size", Element::integer, "1024" },
-            { "ncr-protocol", Element::string, "UDP" },
-            { "ncr-format", Element::string, "JSON" },
-            { "always-include-fqdn", Element::boolean, "false" },
-            { "override-no-update", Element::boolean, "false" },
-            { "override-client-update", Element::boolean, "false" },
-            { "replace-client-name", Element::string, "NEVER" },
-            { "generated-prefix", Element::string, "myhost" },
-            { "qualifying-suffix", Element::string, "" }
-        };
-
         if (v6) {
             setAllDefaults(config, GLOBAL6_DEFAULTS, OPTION6_DEFAULTS,
-                           OPTION6_DEF_DEFAULTS, D2_CLIENT_CFG_DEFAULTS);
+                           OPTION6_DEF_DEFAULTS);
         } else {
             setAllDefaults(config, GLOBAL6_DEFAULTS, OPTION4_DEFAULTS,
-                           OPTION4_DEF_DEFAULTS, D2_CLIENT_CFG_DEFAULTS);
+                           OPTION4_DEF_DEFAULTS);
+        }
+
+        /// D2 client configuration code is in this library
+        ConstElementPtr d2_client = config->get("dhcp-ddns");
+        if (d2_client &&
+            !D2ClientConfigParser::isShortCutDisabled(d2_client)) {
+            D2ClientConfigParser::setAllDefaults(d2_client);
         }
     }
 
@@ -1868,7 +1836,38 @@ TEST_F(ParseConfigTest, parserDefaultsD2Config) {
 
 /// @brief Check various invalid D2 client configurations.
 TEST_F(ParseConfigTest, invalidD2Config) {
+    std::string invalid_shortcuts[] = {
+        // Must supply at least enable-updates
+        "{ \"dhcp-ddns\" :"
+        "    {"
+        "    }"
+        "}",
+        // Enable-updates must be a boolean
+        "{ \"dhcp-ddns\" :"
+        "    {"
+        "     \"enable-updates\" : 0"
+        "    }"
+        "}",
+        // stop
+        ""
+    };
+    int i = 0;
+    while (!invalid_shortcuts[i].empty()) {
+        // Verify that the configuration string parsing throws
+        EXPECT_THROW(parseConfiguration(invalid_shortcuts[i]),
+                     DhcpConfigError);
+        i++;
+    }
+
     std::string invalid_configs[] = {
+#if simple_parser_get_position_got_fixed
+        // Must supply qualifying-suffix when updates are enabled
+        "{ \"dhcp-ddns\" :"
+        "    {"
+        "     \"enable-updates\" : true"
+        "    }"
+        "}",
+#endif
         // Invalid server ip value
         "{ \"dhcp-ddns\" :"
         "    {"
@@ -2017,7 +2016,7 @@ TEST_F(ParseConfigTest, invalidD2Config) {
     // Iterate through the invalid configuration strings, attempting to
     // parse each one.  They should fail to parse, but fail gracefully.
     D2ClientConfigPtr current_config;
-    int i = 0;
+    i = 0;
     while (!invalid_configs[i].empty()) {
         // Verify that the configuration string parses without throwing.
         int rcode = parseConfiguration(invalid_configs[i]);