[5033] - migrated D2ClientConfigParser to SimpleParser, kea-dhcp4 uses it

src/bin/dhcp4/json_config_parser.cc
    createGlobalDhcp4ConfigParser()
    - added clause to invoke new D2ClientConfigParser to
    set staging config
    - added clause to apply staged D2ClientConfig (formerly done
    by parser commit

src/bin/dhcp4/parser_context.h
src/bin/dhcp4/parser_context.cc
    aded PARSER_DHCP_DDNS context

src/bin/dhcp4/simple_parser4.h
src/bin/dhcp4/simple_parser4.cc
    defined SimpleParser4::D2_CLIENT_CONFIG_DEFAULTS
    SimpleParser4::setAllDefaults() - now sets defaults
    for D2ClientConfig

src/bin/dhcp4/tests/d2_unittest.cc
src/bin/dhcp4/tests/fqdn_unittest.cc
    Updated replace-name-mode values (true/false no longer supported)

src/lib/dhcpsrv/parsers/dhcp_parsers.h
src/lib/dhcpsrv/parsers/dhcp_parsers.cc
    D2ClientConfig now derives from SimpleParser

src/lib/dhcpsrv/srv_config.h
src/lib/dhcpsrv/srv_config.cc
    Added a D2ClientConfigPtr member to SrvConfig.

src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc
    Updated tests to set D2 client config defaults

doc/guide/dhcp4-srv.xml
    Updated, replace-client-name no longer accepts booleans

doc/guide/dhcp4-srv.xml

@@ -2174,9 +2174,10 @@ It is merely echoed by the server
     <note>
     Note that formerly, this parameter was a boolean and permitted only values
     of <command>true</command> and <command>false</command>.  Boolean values
-    will still be accepted but may eventually be deprecated. A value of
-    <command>true</command> equates to <command>when-present</command>,
-    <command>false</command> equates to <command>never</command>.
+    have been deprecated and are no longer accepted.  If you are currently using
+    booleans, you must replace them with the desired mode name. A value of
+    <command>true</command> maps to <command>"when-present"</command>, while
+    <command>false</command> maps to <command>"never"</command>.
     </note>
 
       For example, To instruct kea-dhcp4 to always generate the FQDN for a

src/bin/dhcp4/json_config_parser.cc

@@ -436,8 +436,7 @@ DhcpConfigParser* createGlobalDhcp4ConfigParser(const std::string& config_id,
         parser = new HooksLibrariesParser(config_id);
     } else if (config_id.compare("echo-client-id") == 0) {
         parser = new BooleanParser(config_id, globalContext()->boolean_values_);
-    } else if (config_id.compare("dhcp-ddns") == 0) {
-        parser = new D2ClientConfigParser(config_id);
+    // dhcp-ddns has been converted to SimpleParser.
     } else if (config_id.compare("match-client-id") == 0) {
         parser = new BooleanParser(config_id, globalContext()->boolean_values_);
     } else if (config_id.compare("control-socket") == 0) {
@@ -637,6 +636,13 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) {
                 continue;
             }
 
+            if (config_pair.first == "dhcp-ddns") {
+                D2ClientConfigParser parser;
+                D2ClientConfigPtr cfg = parser.parse(config_pair.second);
+                CfgMgr::instance().getStagingCfg()->setD2ClientConfig(cfg);
+                continue;
+            }
+
             ParserPtr parser(createGlobalDhcp4ConfigParser(config_pair.first,
                                                            config_pair.second));
             LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_PARSER_CREATED)
@@ -735,6 +741,13 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) {
             if (hooks_parser) {
                 hooks_parser->commit();
             }
+
+            {
+                // Used to be done by parser commit
+                D2ClientConfigPtr cfg;
+                cfg = CfgMgr::instance().getStagingCfg()->getD2ClientConfig();
+                CfgMgr::instance().setD2ClientConfig(cfg);
+            }
         }
         catch (const isc::Exception& ex) {
             LOG_ERROR(dhcp4_logger, DHCP4_PARSER_COMMIT_FAIL).arg(ex.what());

src/bin/dhcp4/parser_context.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2016 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2017 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
@@ -159,6 +159,8 @@ Parser4Context::contextName()
         return ("loggers");
     case OUTPUT_OPTIONS:
         return ("output-options");
+    case DHCP_DDNS:
+        return ("dhcp-ddns");
     default:
         return ("__unknown__");
     }

src/bin/dhcp4/parser_context.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2015-2016 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2015-2017 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
@@ -80,7 +80,10 @@ public:
         PARSER_OPTION_DATA,
 
         /// This will parse the input as hooks-library.
-        PARSER_HOOKS_LIBRARY
+        PARSER_HOOKS_LIBRARY,
+
+        /// This will parse the input as dhcp-ddns.
+        PARSER_DHCP_DDNS
     } ParserType;
 
     /// @brief Default constructor.
@@ -184,9 +187,6 @@ public:
         ///< Used while parsing content of Dhcp4.
         DHCP4,
 
-        // not yet DHCP6,
-        // not yet DHCP_DDNS,
-
         ///< Used while parsing content of Logging
         LOGGING,
 
@@ -241,7 +241,10 @@ public:
         LOGGERS,
 
         /// Used while parsing Logging/loggers/output_options structures.
-        OUTPUT_OPTIONS
+        OUTPUT_OPTIONS,
+
+        /// Used while parsing Dhcp4/dhcp-ddns
+        DHCP_DDNS
     } ParserContext;
 
     /// @brief File name

src/bin/dhcp4/simple_parser4.cc

@@ -71,6 +71,27 @@ 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, "" }
+};
+
 /// @}
 
 /// ---------------------------------------------------------------------------
@@ -99,6 +120,22 @@ 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) {
+        // Because "dhcp-ddns" is a MapElement and global->get()
+        // returns a ConstElementPtr, then we get a map we can't
+        // change.  So go thru gyrations to create a non-const
+        // map, update it with default values and then replace
+        // the one in global with the new one. Ick.
+        std::map<std::string, ConstElementPtr> d2_map;
+        d2_client->getValue(d2_map);
+        ElementPtr new_map(new MapElement());
+        new_map->setValue(d2_map);
+        cnt += SimpleParser::setDefaults(new_map, D2_CLIENT_CONFIG_DEFAULTS);
+        global->set("dhcp-ddns", new_map);
+    }
+
     return (cnt);
 }
 

src/bin/dhcp4/simple_parser4.h

@@ -33,6 +33,7 @@ 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/dhcp4/tests/d2_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2014-2015 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2014-2017 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
@@ -112,7 +112,7 @@ Dhcp4SrvD2Test::configureD2(bool enable_d2, const bool exp_result,
         "     \"allow-client-update\" : true, "
         "     \"override-no-update\" : true, "
         "     \"override-client-update\" : true, "
-        "     \"replace-client-name\" : true, "
+        "     \"replace-client-name\" : \"when-present\", "
         "     \"generated-prefix\" : \"test.prefix\", "
         "     \"qualifying-suffix\" : \"test.suffix.\" },"
         "\"valid-lifetime\": 4000 }";

src/bin/dhcp4/tests/fqdn_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2013-2016 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2017 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
@@ -1551,8 +1551,6 @@ TEST_F(NameDhcpv4SrvTest, emptyFqdn) {
 // the supported modes.
 TEST_F(NameDhcpv4SrvTest, replaceClientNameModeTest) {
 
-    // We pass mode labels in with enclosing quotes so we can also test
-    // unquoted boolean literals true/false
     testReplaceClientNameMode("\"never\"",
                               CLIENT_NAME_NOT_PRESENT, NAME_NOT_REPLACED);
     testReplaceClientNameMode("\"never\"",
@@ -1572,18 +1570,6 @@ TEST_F(NameDhcpv4SrvTest, replaceClientNameModeTest) {
                               CLIENT_NAME_NOT_PRESENT, NAME_REPLACED);
     testReplaceClientNameMode("\"when-not-present\"",
                               CLIENT_NAME_PRESENT, NAME_NOT_REPLACED);
-
-    // Verify that boolean false produces the same result as RCM_NEVER
-    testReplaceClientNameMode("false",
-                              CLIENT_NAME_NOT_PRESENT, NAME_NOT_REPLACED);
-    testReplaceClientNameMode("false",
-                              CLIENT_NAME_PRESENT, NAME_NOT_REPLACED);
-
-    // Verify that boolean true produces the same result as RCM_WHEN_PRESENT
-    testReplaceClientNameMode("true",
-                              CLIENT_NAME_NOT_PRESENT, NAME_NOT_REPLACED);
-    testReplaceClientNameMode("true",
-                              CLIENT_NAME_PRESENT, NAME_REPLACED);
 }
 
 } // end of anonymous namespace

src/lib/dhcpsrv/cfgmgr.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2012-2015 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012-2017 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
@@ -82,7 +82,17 @@ CfgMgr::isDuplicate(const Subnet6& subnet) const {
 
 void
 CfgMgr::setD2ClientConfig(D2ClientConfigPtr& new_config) {
+    ensureCurrentAllocated();
+    // Note that D2ClientMgr::setD2Config() actually attempts to apply the
+    // configuration by stopping its sender and opening a new one and so
+    // forth per the new configuration.
     d2_client_mgr_.setD2ClientConfig(new_config);
+
+    // Manager will throw if the set fails, if it succeeds
+    // we'll update our SrvConfig, configuration_, with the D2ClientConfig
+    // used. This is largely bookkeeping in case we ever want to compare
+    // configuration_ to another SrvConfig.
+    configuration_->setD2ClientConfig(new_config);
 }
 
 bool

src/lib/dhcpsrv/cfgmgr.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2012-2015 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012-2017 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
@@ -148,6 +148,11 @@ public:
 
     /// @brief Updates the DHCP-DDNS client configuration to the given value.
     ///
+    /// Passes the new configuration to the D2ClientMgr instance,
+    /// d2_client_mgr_, which will attempt to apply the new configuration
+    /// by shutting down its sender and opening a new connection per the new
+    /// configuration (see @c D2ClientMgr::setD2ClientConfig()).
+    ///
     /// @param new_config pointer to the new client configuration.
     ///
     /// @throw Underlying method(s) will throw D2ClientError if given an empty

src/lib/dhcpsrv/parsers/dhcp_parsers.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2013-2016 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2017 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
@@ -1237,119 +1237,85 @@ SubnetConfigParser::getOptionalParam(const std::string& name) {
 }
 
 //**************************** D2ClientConfigParser **********************
-D2ClientConfigParser::D2ClientConfigParser(const std::string& entry_name)
-    : entry_name_(entry_name), boolean_values_(new BooleanStorage()),
-      uint32_values_(new Uint32Storage()), string_values_(new StringStorage()),
-      local_client_config_() {
-}
-
-D2ClientConfigParser::~D2ClientConfigParser() {
-}
-
-void
-D2ClientConfigParser::build(isc::data::ConstElementPtr client_config) {
-    BOOST_FOREACH(ConfigPair param, client_config->mapValue()) {
-        ParserPtr parser;
-        try {
-            parser = createConfigParser(param.first);
-        } catch (std::exception& ex) {
-            // Catch exception in case the configuration contains the
-            // unsupported parameter. In this case, we will need to
-            // append the position of this element.
-            isc_throw(DhcpConfigError, ex.what() << " ("
-                      << param.second->getPosition() << ")");
-        }
-
-        parser->build(param.second);
-        parser->commit();
-    }
 
-    /// @todo Create configuration from the configuration parameters. Because
-    /// the validation of the D2 configuration is atomic, there is no way to
-    /// tell which parameter is invalid. Therefore, we catch all exceptions
-    /// and append the line number of the parent element. In the future we
-    /// may should extend D2ClientConfig code so as it returns the name of
-    /// the invalid parameter.
+D2ClientConfigPtr
+D2ClientConfigParser::parse(isc::data::ConstElementPtr client_config) {
+    D2ClientConfigPtr new_config;
+    bool enable_updates;
+    IOAddress server_ip(0);
+    uint32_t server_port;
+    IOAddress sender_ip(0);
+    uint32_t sender_port;
+    uint32_t max_queue_size;
+    dhcp_ddns::NameChangeProtocol ncr_protocol;
+    dhcp_ddns::NameChangeFormat ncr_format;
+    bool always_include_fqdn;
+    bool override_no_update;
+    bool override_client_update;
+    D2ClientConfig::ReplaceClientNameMode replace_client_name_mode;
+    std::string generated_prefix;
+    std::string qualifying_suffix;
+    std::string current_param;
+
+    // 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
+    // local, current_param, to keep track of the parameter involved.
     try {
-        bool enable_updates = boolean_values_->getParam("enable-updates");
+        current_param = "enable-updates";
+        enable_updates = getBoolean(client_config, current_param);
         if (!enable_updates && (client_config->mapValue().size() == 1)) {
             // 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.
-            local_client_config_.reset(new D2ClientConfig());
+            new_config.reset(new D2ClientConfig());
+            return (new_config);
+        }
 
-            return;
+        current_param = "server-ip";
+        server_ip = IOAddress(getString(client_config, (current_param)));
+
+        current_param = "server-port";
+        server_port = getInteger(client_config, current_param);
+
+        current_param = "sender-ip";
+        std::string sender_ip_str(getString(client_config, current_param));
+        if (sender_ip_str.empty()) {
+            // The default sender IP depends on the server IP family
+            sender_ip = (server_ip.isV4() ? IOAddress::IPV4_ZERO_ADDRESS() :
+                                            IOAddress::IPV6_ZERO_ADDRESS());
+        }
+        else {
+            sender_ip = IOAddress(sender_ip_str);
         }
 
-        // Get all parameters that are needed to create the D2ClientConfig.
-
-        // The qualifying suffix is mandatory when updates are enabled
-        std::string qualifying_suffix =
-            string_values_->getParam("qualifying-suffix");
-
-        IOAddress server_ip =
-            IOAddress(string_values_->getOptionalParam("server-ip",
-                                                       D2ClientConfig::
-                                                       DFT_SERVER_IP));
-
-        uint32_t server_port =
-            uint32_values_->getOptionalParam("server-port",
-                                             D2ClientConfig::DFT_SERVER_PORT);
-
-        // The default sender IP depends on the server IP family
-        asiolink::IOAddress
-            sender_ip(string_values_->
-                      getOptionalParam("sender-ip",
-                                       (server_ip.isV4() ?
-                                        D2ClientConfig::DFT_V4_SENDER_IP :
-                                        D2ClientConfig::DFT_V6_SENDER_IP)));
-
-        uint32_t sender_port =
-            uint32_values_->getOptionalParam("sender-port",
-                                             D2ClientConfig::
-                                             DFT_SENDER_PORT);
-        uint32_t max_queue_size
-            = uint32_values_->getOptionalParam("max-queue-size",
-                                               D2ClientConfig::
-                                               DFT_MAX_QUEUE_SIZE);
-
-        dhcp_ddns::NameChangeProtocol ncr_protocol =
-            dhcp_ddns::stringToNcrProtocol(string_values_->
-                                           getOptionalParam("ncr-protocol",
-                                                            D2ClientConfig::
-                                                            DFT_NCR_PROTOCOL));
-        dhcp_ddns::NameChangeFormat ncr_format
-            = dhcp_ddns::stringToNcrFormat(string_values_->
-                                           getOptionalParam("ncr-format",
-                                                            D2ClientConfig::
-                                                            DFT_NCR_FORMAT));
-        std::string generated_prefix =
-            string_values_->getOptionalParam("generated-prefix",
-                                             D2ClientConfig::
-                                             DFT_GENERATED_PREFIX);
-        bool always_include_fqdn =
-            boolean_values_->getOptionalParam("always-include-fqdn",
-                                                D2ClientConfig::
-                                                DFT_ALWAYS_INCLUDE_FQDN);
-
-        bool override_no_update =
-            boolean_values_->getOptionalParam("override-no-update",
-                                              D2ClientConfig::
-                                              DFT_OVERRIDE_NO_UPDATE);
-
-        bool override_client_update =
-            boolean_values_->getOptionalParam("override-client-update",
-                                              D2ClientConfig::
-                                              DFT_OVERRIDE_CLIENT_UPDATE);
-
-        // Formerly, replace-client-name was boolean, so for now we'll support boolean
-        // values by mapping them to the appropriate mode
-        D2ClientConfig::ReplaceClientNameMode replace_client_name_mode;
-        std::string mode_str;
-        mode_str  = string_values_->getOptionalParam("replace-client-name",
-                                                     D2ClientConfig::
-                                                     DFT_REPLACE_CLIENT_NAME_MODE);
+        current_param = "sender-port";
+        sender_port = getInteger(client_config, current_param);
+
+        current_param = "max-queue-size";
+        max_queue_size = getInteger(client_config, current_param);
+
+        current_param = "ncr-protocol";
+        ncr_protocol = dhcp_ddns::stringToNcrProtocol(getString(client_config,
+                                                                current_param));
+        current_param = "ncr-format";
+        ncr_format = dhcp_ddns::stringToNcrFormat(getString(client_config,
+                                                            current_param));
+
+        current_param = "always-include-fqdn";
+        always_include_fqdn = getBoolean(client_config, current_param);
+
+        current_param = "override-no-update";
+        override_no_update = getBoolean(client_config, current_param);
+
+        current_param = "override-client-update";
+        override_client_update = getBoolean(client_config, current_param);
+
+        // Formerly, replace-client-name was boolean, so for now we'll support
+        // boolean values by mapping them to the appropriate mode
+        current_param = "replace-client-name";
+        std::string mode_str = getString(client_config, current_param);
         if (boost::iequals(mode_str, "false")) {
             // @todo add a debug log
             replace_client_name_mode = D2ClientConfig::RCM_NEVER;
@@ -1362,8 +1328,52 @@ D2ClientConfigParser::build(isc::data::ConstElementPtr client_config) {
                                        stringToReplaceClientNameMode(mode_str);
         }
 
+        current_param = "generated-prefix";
+        generated_prefix = getString(client_config, current_param);
+
+        current_param = "qualifying-suffix";
+        qualifying_suffix = getString(client_config, current_param);
+    } catch (const std::exception& ex) {
+        isc_throw(D2ClientError, "D2ClientConfig error: " << ex.what()
+                   << " " << getPosition(current_param, client_config));
+    }
+
+    // Now we check for logical errors. This repeats what is done in
+    // D2ClientConfig::validate(), but doing it here permits us to
+    // emit meaningful parameter position info in the error.
+    if (ncr_format != dhcp_ddns::FMT_JSON) {
+        isc_throw(D2ClientError, "D2ClientConfig error: NCR Format: "
+                    << dhcp_ddns::ncrFormatToString(ncr_format)
+                    << " is not supported."
+                    << " " << getPosition("ncr-format", client_config));
+    }
+
+    if (ncr_protocol != dhcp_ddns::NCR_UDP) {
+        isc_throw(D2ClientError, "D2ClientConfig error: NCR Protocol: "
+                  << dhcp_ddns::ncrProtocolToString(ncr_protocol)
+                  << " is not supported."
+                  << " " << getPosition("ncr-protocol", client_config));
+    }
+
+    if (sender_ip.getFamily() != server_ip.getFamily()) {
+        isc_throw(D2ClientError, "D2ClientConfig error: address family mismatch: "
+                  << "server-ip: " << server_ip.toText()
+                  << " is: " << (server_ip.isV4() ? "IPv4" : "IPv6")
+                  << " while sender-ip: "  << sender_ip.toText()
+                  << " is: " << (sender_ip.isV4() ? "IPv4" : "IPv6")
+                  << " " << getPosition("sender-ip", client_config));
+    }
+
+    if (server_ip == sender_ip && server_port == sender_port) {
+        isc_throw(D2ClientError, "D2ClientConfig error: server and sender cannot"
+                  " share the exact same IP address/port: "
+                  << server_ip.toText() << "/" << server_port
+                  << " " << getPosition("sender-ip", client_config));
+    }
+
+    try {
         // Attempt to create the new client config.
-        local_client_config_.reset(new D2ClientConfig(enable_updates,
+        new_config.reset(new D2ClientConfig(enable_updates,
                                                       server_ip,
                                                       server_port,
                                                       sender_ip,
@@ -1382,49 +1392,8 @@ D2ClientConfigParser::build(isc::data::ConstElementPtr client_config) {
         isc_throw(DhcpConfigError, ex.what() << " ("
                   << client_config->getPosition() << ")");
     }
-}
-
-isc::dhcp::ParserPtr
-D2ClientConfigParser::createConfigParser(const std::string& config_id) {
-    DhcpConfigParser* parser = NULL;
-    if ((config_id.compare("server-port") == 0) ||
-        (config_id.compare("sender-port") == 0) ||
-        (config_id.compare("max-queue-size") == 0)) {
-        parser = new Uint32Parser(config_id, uint32_values_);
-    } else if ((config_id.compare("server-ip") == 0) ||
-        (config_id.compare("ncr-protocol") == 0) ||
-        (config_id.compare("ncr-format") == 0) ||
-        (config_id.compare("generated-prefix") == 0) ||
-        (config_id.compare("sender-ip") == 0) ||
-        (config_id.compare("qualifying-suffix") == 0) ||
-        (config_id.compare("replace-client-name") == 0)) {
-        parser = new StringParser(config_id, string_values_);
-    } else if ((config_id.compare("enable-updates") == 0) ||
-        (config_id.compare("always-include-fqdn") == 0) ||
-        (config_id.compare("allow-client-update") == 0) ||
-        (config_id.compare("override-no-update") == 0) ||
-        (config_id.compare("override-client-update") == 0)) {
-        parser = new BooleanParser(config_id, boolean_values_);
-    } else {
-        isc_throw(NotImplemented,
-            "parser error: D2ClientConfig parameter not supported: "
-            << config_id);
-    }
-
-    return (isc::dhcp::ParserPtr(parser));
-}
 
-void
-D2ClientConfigParser::commit() {
-    // @todo if local_client_config_ is empty then shutdown the listener...
-    // @todo Should this also attempt to start a listener?
-    // In keeping with Interface, Subnet, and Hooks parsers, then this
-    // should initialize the listener.  Failure to init it, should cause
-    // rollback.  This gets sticky, because who owns the listener instance?
-    // Does CfgMgr maintain it or does the server class?  If the latter
-    // how do we get that value here?
-    // I'm thinkikng D2ClientConfig could contain the listener instance
-    CfgMgr::instance().setD2ClientConfig(local_client_config_);
+    return(new_config);
 }
 
 };  // namespace dhcp

src/lib/dhcpsrv/parsers/dhcp_parsers.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2013-2016 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2017 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
@@ -1026,29 +1026,18 @@ protected:
 /// This class parses the configuration element "dhcp-ddns" common to the
 /// spec files for both dhcp4 and dhcp6. It creates an instance of a
 /// D2ClientConfig.
-class D2ClientConfigParser : public  isc::dhcp::DhcpConfigParser {
+class D2ClientConfigParser : public  isc::data::SimpleParser {
 public:
     /// @brief Constructor
     ///
-    /// @param entry_name is an arbitrary label assigned to this configuration
-    /// definition.
-    D2ClientConfigParser(const std::string& entry_name);
+    D2ClientConfigParser(){};
 
     /// @brief Destructor
-    virtual ~D2ClientConfigParser();
+    virtual ~D2ClientConfigParser(){};
 
-    /// @brief Performs the parsing of the given dhcp-ddns element.
-    ///
-    /// The results of the parsing are retained internally for use during
-    /// commit.
-    /// @todo This parser supplies hard-coded default values for all
-    /// optional parameters.  This should be changed once a new plan
-    /// for configuration is determined.
+    /// @brief Parses a given dhcp-ddns element into D2ClientConfig.
     ///
     /// @param client_config is the "dhcp-ddns" configuration to parse
-    virtual void build(isc::data::ConstElementPtr client_config);
-
-    /// @brief Creates a parser for the given "dhcp-ddns" member element id.
     ///
     /// The elements currently supported are (see isc::dhcp::D2ClientConfig
     /// for details on each):
@@ -1066,33 +1055,8 @@ public:
     /// -# replace-client-name
     /// -# generated-prefix
     ///
-    /// @param config_id is the "item_name" for a specific member element of
-    /// the "dns_server" specification.
-    ///
-    /// @return returns a pointer to newly created parser.
-    virtual isc::dhcp::ParserPtr createConfigParser(const std::string&
-                                                    config_id);
-
-    /// @brief Instantiates a D2ClientConfig from internal data values
-    /// passes to CfgMgr singleton.
-    virtual void commit();
-
-private:
-    /// @brief Arbitrary label assigned to this parser instance.
-    /// Primarily used for diagnostics.
-    std::string entry_name_;
-
-    /// Storage for subnet-specific boolean values.
-    BooleanStoragePtr boolean_values_;
-
-    /// Storage for subnet-specific integer values.
-    Uint32StoragePtr uint32_values_;
-
-    /// Storage for subnet-specific string values.
-    StringStoragePtr string_values_;
-
-    /// @brief Pointer to temporary local instance created during build.
-    D2ClientConfigPtr local_client_config_ ;
+    /// @return returns a pointer to newly created D2ClientConfig.
+    D2ClientConfigPtr parse(isc::data::ConstElementPtr d2_client_cfg);
 };
 
 // Pointers to various parser objects.

src/lib/dhcpsrv/srv_config.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2014-2016 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2014-2017 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
@@ -29,7 +29,8 @@ SrvConfig::SrvConfig()
       cfg_host_operations4_(CfgHostOperations::createConfig4()),
       cfg_host_operations6_(CfgHostOperations::createConfig6()),
       class_dictionary_(new ClientClassDictionary()),
-      decline_timer_(0), dhcp4o6_port_(0) {
+      decline_timer_(0), dhcp4o6_port_(0),
+      d2_client_config_(new D2ClientConfig()) {
 }
 
 SrvConfig::SrvConfig(const uint32_t sequence)
@@ -42,7 +43,8 @@ SrvConfig::SrvConfig(const uint32_t sequence)
       cfg_host_operations4_(CfgHostOperations::createConfig4()),
       cfg_host_operations6_(CfgHostOperations::createConfig6()),
       class_dictionary_(new ClientClassDictionary()),
-      decline_timer_(0), dhcp4o6_port_(0) {
+      decline_timer_(0), dhcp4o6_port_(0),
+      d2_client_config_(new D2ClientConfig()) {
 }
 
 std::string
@@ -70,7 +72,7 @@ SrvConfig::getConfigSummary(const uint32_t selection) const {
     }
 
     if ((selection & CFGSEL_DDNS) == CFGSEL_DDNS) {
-        bool ddns_enabled = CfgMgr::instance().ddnsEnabled();
+        bool ddns_enabled = getD2ClientConfig()->getEnableUpdates();
         s << "DDNS: " << (ddns_enabled ? "enabled" : "disabled") << "; ";
     }
 
@@ -106,7 +108,8 @@ SrvConfig::copy(SrvConfig& new_config) const {
     cfg_option_->copyTo(*new_config.cfg_option_);
     // Replace the client class dictionary
     new_config.class_dictionary_.reset(new ClientClassDictionary(*class_dictionary_));
-
+    // Replace the D2 client configuration
+    new_config.setD2ClientConfig(getD2ClientConfig());
 }
 
 void
@@ -151,7 +154,8 @@ SrvConfig::equals(const SrvConfig& other) const {
     return ((*cfg_iface_ == *other.cfg_iface_) &&
             (*cfg_option_def_ == *other.cfg_option_def_) &&
             (*cfg_option_ == *other.cfg_option_) &&
-            (*class_dictionary_ == *other.class_dictionary_));
+            (*class_dictionary_ == *other.class_dictionary_) &&
+            (*d2_client_config_ == *other.d2_client_config_));
 }
 
 void

src/lib/dhcpsrv/srv_config.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2014-2016 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2014-2017 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
@@ -20,6 +20,7 @@
 #include <dhcpsrv/cfg_subnets6.h>
 #include <dhcpsrv/cfg_mac_source.h>
 #include <dhcpsrv/client_class_def.h>
+#include <dhcpsrv/d2_client_cfg.h>
 #include <dhcpsrv/logging_info.h>
 #include <cc/data.h>
 #include <boost/shared_ptr.hpp>
@@ -481,12 +482,28 @@ public:
 
     /// @brief Returns DHCP4o6 IPC port
     ///
-    /// See @ref setDhcp4o6Port or brief discussion.                         
+    /// See @ref setDhcp4o6Port or brief discussion.
     /// @return value of DHCP4o6 IPC port
     uint32_t getDhcp4o6Port() {
         return (dhcp4o6_port_);
     }
 
+    /// @brief Returns pointer to the D2 client configuration
+    D2ClientConfigPtr getD2ClientConfig() {
+        return (d2_client_config_);
+    }
+
+    /// @brief Returns pointer to const D2 client configuration
+    const D2ClientConfigPtr getD2ClientConfig() const {
+        return (d2_client_config_);
+    }
+
+    /// @brief Sets the D2 client configuration
+    /// @param dictionary pointer to the new D2 client configuration
+    void setD2ClientConfig(const D2ClientConfigPtr& d2_client_config) {
+        d2_client_config_ = d2_client_config;
+    }
+
 private:
 
     /// @brief Sequence number identifying the configuration.
@@ -570,6 +587,8 @@ private:
     /// DHCPv4-over-DHCPv6 uses a UDP socket for interserver communication,
     /// this socket is bound and connected to this port and port + 1
     uint32_t dhcp4o6_port_;
+
+    D2ClientConfigPtr d2_client_config_;
 };
 
 /// @name Pointers to the @c SrvConfig object.

src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc

@@ -314,17 +314,18 @@ public:
 
                 // These are the simple parsers. No need to go through
                 // the ParserPtr hooplas with them.
-                if ( (config_pair.first == "option-data") ||
-                     (config_pair.first == "option-def")) {
+                if ((config_pair.first == "option-data") ||
+                    (config_pair.first == "option-def") ||
+                    (config_pair.first == "dhcp-ddns")) {
                     continue;
                 }
 
                 // We also don't care about the default values that may be been
                 // inserted
-                if ( (config_pair.first == "preferred-lifetime") ||
-                     (config_pair.first == "valid-lifetime") ||
-                     (config_pair.first == "renew-timer") ||
-                     (config_pair.first == "rebind-timer")) {
+                if ((config_pair.first == "preferred-lifetime") ||
+                    (config_pair.first == "valid-lifetime") ||
+                    (config_pair.first == "renew-timer") ||
+                    (config_pair.first == "rebind-timer")) {
                     continue;
                 }
 
@@ -366,6 +367,16 @@ public:
                 option_list_parser.parse(cfg_option, option_config->second);
             }
 
+            // The dhcp-ddns parser is the next one to be run.
+            std::map<std::string, ConstElementPtr>::const_iterator
+                                d2_client_config = values_map.find("dhcp-ddns");
+            if (d2_client_config != values_map.end()) {
+                // Used to be done by parser commit
+                D2ClientConfigParser parser;
+                D2ClientConfigPtr cfg = parser.parse(d2_client_config->second);
+                CfgMgr::instance().setD2ClientConfig(cfg);
+            }
+
             // Everything was fine. Configuration is successful.
             answer = isc::config::createAnswer(0, "Configuration committed.");
         } catch (const isc::Exception& ex) {
@@ -401,8 +412,6 @@ public:
             parser.reset(new HooksLibrariesParser(config_id));
             hooks_libraries_parser_ =
                 boost::dynamic_pointer_cast<HooksLibrariesParser>(parser);
-        } else if (config_id.compare("dhcp-ddns") == 0) {
-            parser.reset(new D2ClientConfigParser(config_id));
         } else {
             isc_throw(NotImplemented,
                 "Parser error: configuration parameter not supported: "
@@ -425,7 +434,8 @@ public:
     size_t setAllDefaults(isc::data::ElementPtr global,
                           const SimpleDefaults& global_defaults,
                           const SimpleDefaults& option_defaults,
-                          const SimpleDefaults& option_def_defaults) {
+                          const SimpleDefaults& option_def_defaults,
+                          const SimpleDefaults& d2_client_defaults) {
         size_t cnt = 0;
         // Set global defaults first.
         cnt = SimpleParser::setDefaults(global, global_defaults);
@@ -445,6 +455,23 @@ public:
             }
         }
 
+        ConstElementPtr d2_client = global->get("dhcp-ddns");
+        /// @todo - what if it's not in global? should we add it?
+        if (d2_client) {
+            // Because "dhcp-ddns" is a MapElement and global->get()
+            // returns a ConstElementPtr, then we get a map we can't
+            // change.  So go thru gyrations to create a non-const
+            // map, update it with default values and then replace
+            // the one in global with the new one. Ick.
+            std::map<std::string, ConstElementPtr> d2_map;
+            d2_client->getValue(d2_map);
+            ElementPtr new_map(new MapElement());
+            new_map->setValue(d2_map);
+            cnt += SimpleParser::setDefaults(new_map, d2_client_defaults);
+            global->set("dhcp-ddns", new_map);
+        }
+
+
         return (cnt);
     }
 
@@ -456,6 +483,10 @@ public:
     /// src/bin/dhcp{4,6}, the most straightforward way is to simply copy the
     /// minimum code here. Hence this method.
     ///
+    /// @todo - TKM, I think this is fairly hideous and we should figure out a
+    /// a way to not have to replicate in this fashion.  It may be minimum code
+    /// now, but it won't be fairly soon.
+    ///
     /// @param config configuration structure to be filled with default values
     /// @param v6 true = DHCPv6, false = DHCPv4
     void setAllDefaults(ElementPtr config, bool v6) {
@@ -497,12 +528,31 @@ 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);
+                           OPTION6_DEF_DEFAULTS, D2_CLIENT_CFG_DEFAULTS);
         } else {
             setAllDefaults(config, GLOBAL6_DEFAULTS, OPTION4_DEFAULTS,
-                           OPTION4_DEF_DEFAULTS);
+                           OPTION4_DEF_DEFAULTS, D2_CLIENT_CFG_DEFAULTS);
         }
     }
 
@@ -529,6 +579,7 @@ public:
             // If error was reported, the error string should contain
             // position of the data element which caused failure.
             if (rcode_ != 0) {
+                std::cout << "Error text:" << error_text_ << std::endl;
                 EXPECT_TRUE(errorContainsPosition(status, "<string>"));
             }
         }
@@ -801,7 +852,6 @@ TEST_F(ParseConfigTest, escapedOptionDataTest) {
         "    \"data\": \"\\\\SMSBoot\\\\x64\\\\wdsnbp.com\""
         " } ]"
         "}";
-    std::cout << config << std::endl;
 
     // Verify that the configuration string parses.
     int rcode = parseConfiguration(config);
@@ -1826,17 +1876,6 @@ TEST_F(ParseConfigTest, parserDefaultsD2Config) {
 /// @brief Check various invalid D2 client configurations.
 TEST_F(ParseConfigTest, invalidD2Config) {
     std::string invalid_configs[] = {
-        // Must supply at least enable-updates
-        "{ \"dhcp-ddns\" :"
-        "    {"
-        "    }"
-        "}",
-        // Must supply qualifying-suffix when updates are enabled
-        "{ \"dhcp-ddns\" :"
-        "    {"
-        "     \"enable-updates\" : true"
-        "    }"
-        "}",
         // Invalid server ip value
         "{ \"dhcp-ddns\" :"
         "    {"

src/lib/dhcpsrv/tests/srv_config_unittest.cc

@@ -141,8 +141,7 @@ SrvConfigTest::addSubnet6(const unsigned int index) {
 
 void
 SrvConfigTest::enableDDNS(const bool enable) {
-    // D2 configuration should always be non-NULL.
-    CfgMgr::instance().getD2ClientConfig()->enableUpdates(enable);
+    conf_.getD2ClientConfig()->enableUpdates(enable);
 }
 
 // Check that by default there are no logging entries

src/lib/dhcpsrv/testutils/config_result_check.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2014-2015 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2014-2017 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
@@ -67,12 +67,6 @@ bool errorContainsPosition(ConstElementPtr error_element,
                     ++i;
                 }
 
-                // Make sure that there has been at least one digit and that the
-                // position is followed by the paren.
-                if ((i == 0) || (split_pos[2][i] != ')')) {
-                    return (false);
-                }
-
                 // All checks passed.
                 return (true);
             }