[master] Merge branch 'trac3436'

Fixed Conflicts:
	src/bin/d2/tests/Makefile.am

doc/examples/ddns/sample1.json

@@ -0,0 +1,112 @@
+# This is an example configuration file for D2, Kea's DHCP-DDNS processor.
+# It supports updating two Forward DNS zones "four.example.com" and
+# "six.example.com"; and one Reverse DNS zone, "2.0.192.in-addr.arpa."
+
+{
+# ------------------ DHCP-DDNS ---------------------
+#
+"DhcpDdns":
+{
+
+# --------------  Global Parameters ----------------
+#
+#   D2 will listen for update requests for Kea DHCP servers at 172.16.1.10
+#   on port 53001.  Maximum time to we will  wait for a DNS server to
+#   respond to us is 1000 ms.
+
+    "ip_address": "172.16.1.10",
+    "port": 53001,
+    "dns_server_timeout" : 1000,
+
+#
+# ----------------- Forward DDNS  ------------------
+#
+#   1. Zone - "four.example.com.
+#      It uses TSIG, key name is "d2.md5.key"
+#      It is served by one DNS server which listens for DDNS requests at
+#      172.16.1.1 on the default port 53 (standard  DNS port)
+#
+#   2. Zone - "six.example.com."
+#      It does not use TSIG.
+#      It is server by one DNS server at "2001:db8:1::10" on port 7802
+
+    "forward_ddns":
+    {
+        "ddns_domains":
+        [
+#           DdnsDomain for zone "four.example.com."
+            {
+                "name": "four.example.com.",
+                "key_name": "d2.md5.key",
+                "dns_servers":
+                [
+                    {
+                        "ip_address": "172.16.1.1"
+                    }
+                ]
+            },
+
+#           DdnsDomain for zone "six.example.com."
+            {
+                "name": "six.example.com.",
+                "dns_servers":
+                [
+                    {
+                        "ip_address": "2001:db8:1::10",
+                        "port": 7802
+                    }
+                ]
+            }
+        ]
+    },
+
+#
+# ----------------- Reverse DDNS  ------------------
+#
+# We will update Reverse DNS for one zone "2.0.192.in-addr-arpa". It
+# uses TSIG with key "d2.sha1.key" and is served by two DNS servers:
+# one listening at "172.16.1.1" on 53001 and the other at "192.168.2.10".
+#
+    "reverse_ddns":
+    {
+        "ddns_domains":
+        [
+            {
+                "name": "2.0.192.in-addr.arpa.",
+                "key_name": "d2.sha1.key",
+                "dns_servers":
+                [
+                    {
+                        "ip_address": "172.16.1.1",
+                        "port": 53001
+                    },
+                    {
+                        "ip_address": "192.168.2.10"
+                    }
+                ]
+            }
+        ]
+    },
+
+#
+# ------------------ TSIG keys ---------------------
+#
+#   Each key has a name, an algorithm (HMAC-MD5, HMAC-SHA1, HMAC-SHA224...)
+#   and a base-64 encoded shared secret.
+#
+    "tsig_keys":
+    [
+        {
+            "name": "d2.md5.key",
+            "algorithm": "HMAC-MD5",
+            "secret": "LSWXnfkKZjdPJI5QxlpnfQ=="
+        },
+        {
+            "name": "d2.sha1.key",
+            "algorithm": "HMAC-SHA1",
+            "secret": "hRrp29wzUv3uzSNRLlY68w=="
+        }
+    ]
+}
+
+}

doc/examples/ddns/template.json

@@ -0,0 +1,105 @@
+# This file may be used a template for constructing DHCP-DDNS JSON
+# configuration.
+#
+# Default values that may be omitted are '#' commented out.
+
+# If in a file by itself, it must start with a left-curly-bracket.
+{
+
+"DhcpDdns" :
+{
+#
+# --------------  Global Parameters ----------------
+#
+#    All of the global parameters have default values as shown.  If these
+#    are satisfactory you may omit them.
+#
+#    "ip_address" : "127.0.0.1",
+#    "port" : 53001,
+#    "dns_server_timeout" : 100,
+#    "ncr_protocol" : "UDP"
+#    "ncr_format" : "JSON"
+
+#
+# ----------------- Forward DDNS  ------------------
+#
+    "forward_ddns" :
+    {
+        "ddns_domains" :
+        [
+            {
+                "name" : "<zone name 1>",
+#                "key_name" : "<key name>",
+                "dns_servers" :
+                [
+                    {
+                        "ip_address" : "<ip address>"
+#                       ,"port" : 53
+                    }
+#                   ,
+#                   {
+#                       next DNS server for this DdnsDomain
+#                   }
+#                   :
+                ]
+            }
+#           ,
+#           {
+#                next Forward DdnsDomain
+#           }
+#           :
+        ]
+    },
+
+#
+# ----------------- Reverse DDNS  ------------------
+#
+    "reverse_ddns" :
+    {
+        "ddns_domains" :
+        [
+            {
+                "name" : "<reverse zone name 1>",
+#                "key_name" : "<key name>",
+                "dns_servers" :
+                [
+                    {
+                        "ip_address" : "<ip address>"
+#                        ,"port" : 53
+                    }
+#                   ,
+#                   {
+#                       next DNS server for this DdnsDomain
+#                   }
+#                   :
+                ]
+            }
+#           ,
+#           {
+#                next Reverse DdnsDomain
+#           }
+#           :
+        ]
+    },
+#
+# ------------------ TSIG keys ---------------------
+#
+    "tsig_keys" :
+    [
+        {
+            "name" : "<key name>",
+            "algorithm" : "<algorithm name>",
+#            Valid values for algorithm are:    HMAC-MD5, HMAC-SHA1,
+#                                               HMAC-SHA224, HMAC-SHA256,
+#                                               HMAC-SHA384, HMAC-SHA512
+            "secret" : "<shared secret value>"
+        }
+#       ,
+#        {
+#                next TSIG Key
+#        }
+    ]
+}
+
+# If in a file by itself, it must end with an right-curly-bracket.
+}

src/bin/d2/d2_cfg_mgr.cc

@@ -202,31 +202,94 @@ D2CfgMgr::buildParams(isc::data::ConstElementPtr params_config) {
     // This populate the context scalar stores with all of the parameters.
     DCfgMgrBase::buildParams(params_config);
 
-    // Fetch the parameters from the context to create the D2Params.
+    // Fetch and validate the parameters from the context to create D2Params.
+    // We validate them here rather than just relying on D2Param constructor
+    // so we can spit out config text position info with errors.
+
+    // Fetch and validate ip_address.
     D2CfgContextPtr context = getD2CfgContext();
     isc::dhcp::StringStoragePtr strings = context->getStringStorage();
-    asiolink::IOAddress ip_address(strings->
-                                   getOptionalParam("ip_address",
-                                                    D2Params::DFT_IP_ADDRESS));
+    asiolink::IOAddress ip_address(D2Params::DFT_IP_ADDRESS);
+
+    std::string ip_address_str = strings->getOptionalParam("ip_address",
+                                                            D2Params::
+                                                            DFT_IP_ADDRESS);
+    try {
+        ip_address = asiolink::IOAddress(ip_address_str);
+    } catch (const std::exception& ex) {
+        isc_throw(D2CfgError, "IP address invalid : \""
+                  << ip_address_str << "\" ("
+                  << strings->getPosition("ip_address") << ")");
+    }
+
+    if ((ip_address.toText() == "0.0.0.0") || (ip_address.toText() == "::")) {
+        isc_throw(D2CfgError, "IP address cannot be \"" << ip_address << "\" ("
+                   << strings->getPosition("ip_address") << ")");
+    }
 
+    // Fetch and validate port.
     isc::dhcp::Uint32StoragePtr ints = context->getUint32Storage();
     uint32_t port = ints->getOptionalParam("port", D2Params::DFT_PORT);
 
+    if (port == 0) {
+        isc_throw(D2CfgError, "port cannot be 0 ("
+                  << ints->getPosition("port") << ")");
+    }
+
+    // Fetch and validate dns_server_timeout.
     uint32_t dns_server_timeout
         = ints->getOptionalParam("dns_server_timeout",
                                  D2Params::DFT_DNS_SERVER_TIMEOUT);
 
-    dhcp_ddns::NameChangeProtocol ncr_protocol
-        = dhcp_ddns::stringToNcrProtocol(strings->
-                                         getOptionalParam("ncr_protocol",
-                                                          D2Params::
-                                                          DFT_NCR_PROTOCOL));
-    dhcp_ddns::NameChangeFormat ncr_format
-        = dhcp_ddns::stringToNcrFormat(strings->
+    if (dns_server_timeout < 1) {
+        isc_throw(D2CfgError, "DNS server timeout must be larger than 0 ("
+                  << ints->getPosition("dns_server_timeout") << ")");
+    }
+
+    // Fetch and validate ncr_protocol.
+    dhcp_ddns::NameChangeProtocol ncr_protocol;
+    try {
+        ncr_protocol = dhcp_ddns::
+                       stringToNcrProtocol(strings->
+                                           getOptionalParam("ncr_protocol",
+                                                            D2Params::
+                                                            DFT_NCR_PROTOCOL));
+    } catch (const std::exception& ex) {
+        isc_throw(D2CfgError, "ncr_protocol : "
+                  << ex.what() << " ("
+                  << strings->getPosition("ncr_protocol") << ")");
+    }
+
+    if (ncr_protocol != dhcp_ddns::NCR_UDP) {
+        isc_throw(D2CfgError, "ncr_protocol : "
+                  << dhcp_ddns::ncrProtocolToString(ncr_protocol)
+                  << " is not yet supported ("
+                  << strings->getPosition("ncr_protocol") << ")");
+    }
+
+    // Fetch and validate ncr_format.
+    dhcp_ddns::NameChangeFormat ncr_format;
+    try {
+        ncr_format = dhcp_ddns::
+                     stringToNcrFormat(strings->
                                        getOptionalParam("ncr_format",
-                                                          D2Params::
-                                                          DFT_NCR_FORMAT));
-    // Attempt to create the new client config.
+                                                        D2Params::
+                                                        DFT_NCR_FORMAT));
+    } catch (const std::exception& ex) {
+        isc_throw(D2CfgError, "ncr_format : "
+                  << ex.what() << " ("
+                  << strings->getPosition("ncr_format") << ")");
+    }
+
+    if (ncr_format != dhcp_ddns::FMT_JSON) {
+        isc_throw(D2CfgError, "NCR Format:"
+                  << dhcp_ddns::ncrFormatToString(ncr_format)
+                  << " is not yet supported ("
+                  << strings->getPosition("ncr_format") << ")");
+    }
+
+    // Attempt to create the new client config. This ought to fly as
+    // we already validated everything.
     D2ParamsPtr params(new D2Params(ip_address, port, dns_server_timeout,
                                     ncr_protocol, ncr_format));
 
@@ -234,7 +297,8 @@ D2CfgMgr::buildParams(isc::data::ConstElementPtr params_config) {
 }
 
 isc::dhcp::ParserPtr
-D2CfgMgr::createConfigParser(const std::string& config_id) {
+D2CfgMgr::createConfigParser(const std::string& config_id,
+                             const isc::data::Element::Position& pos) {
     // Get D2 specific context.
     D2CfgContextPtr context = getD2CfgContext();
 
@@ -262,8 +326,8 @@ D2CfgMgr::createConfigParser(const std::string& config_id) {
                                                context->getKeys()));
     } else {
         isc_throw(NotImplemented,
-                  "parser error: D2CfgMgr parameter not supported: "
-                  << config_id);
+                  "parser error: D2CfgMgr parameter not supported : "
+                  " (" << config_id << pos << ")");
     }
 
     return (parser);

src/bin/d2/d2_cfg_mgr.h

@@ -245,7 +245,18 @@ protected:
     /// parameter's id and then invoking its build method passing in the
     /// parameter's configuration value.
     ///
+    /// It then fetches the parameters, validating their values and if
+    /// valid instantiates a D2Params instance.  Invalid values result in
+    /// a throw.
+    ///
     /// @param params_config set of scalar configuration elements to parse
+    ///
+    /// @throw D2CfgError if any of the following are true:
+    /// -# ip_address is 0.0.0.0 or ::
+    /// -# port is 0
+    /// -# dns_server_timeout is < 1
+    /// -# ncr_protocol is invalid, currently only NCR_UDP is supported
+    /// -# ncr_format is invalid, currently only FMT_JSON is supported
     virtual void buildParams(isc::data::ConstElementPtr params_config);
 
     /// @brief Given an element_id returns an instance of the appropriate
@@ -264,11 +275,15 @@ protected:
     ///
     /// @param element_id is the string name of the element as it will appear
     /// in the configuration set.
+    /// @param pos position within the configuration text (or file) of element
+    /// to be parsed.  This is passed for error messaging.
     ///
     /// @return returns a ParserPtr to the parser instance.
     /// @throw throws DCfgMgrBaseError if an error occurs.
     virtual isc::dhcp::ParserPtr
-    createConfigParser(const std::string& element_id);
+    createConfigParser(const std::string& element_id,
+                       const isc::data::Element::Position& pos =
+                       isc::data::Element::Position());
 
     /// @brief Creates an new, blank D2CfgContext context
     ///

src/bin/d2/d2_config.cc

@@ -348,7 +348,9 @@ TSIGKeyInfoParser::build(isc::data::ConstElementPtr key_config) {
     // 3. Invoke the parser's commit method to store the element's parsed
     // data to the parser's local storage.
     BOOST_FOREACH (config_pair, key_config->mapValue()) {
-        isc::dhcp::ParserPtr parser(createConfigParser(config_pair.first));
+        isc::dhcp::ParserPtr parser(createConfigParser(config_pair.first,
+                                                       config_pair.second->
+                                                       getPosition()));
         parser->build(config_pair.second);
         parser->commit();
     }
@@ -356,43 +358,66 @@ TSIGKeyInfoParser::build(isc::data::ConstElementPtr key_config) {
     std::string name;
     std::string algorithm;
     std::string secret;
+    std::map<std::string, isc::data::Element::Position> pos;
 
-    // Fetch the key configuration's parsed scalar values from parser's
-    // local storage.
-    local_scalars_.getParam("name", name);
-    local_scalars_.getParam("algorithm", algorithm);
-    local_scalars_.getParam("secret", secret);
+    // Fetch the key's parsed scalar values from parser's local storage.
+    // All are required, if any are missing we'll throw.
+    try {
+        pos["name"] = local_scalars_.getParam("name", name);
+        pos["algorithm"] = local_scalars_.getParam("algorithm", algorithm);
+        pos["secret"] = local_scalars_.getParam("secret", secret);
+    } catch (const std::exception& ex) {
+        isc_throw(D2CfgError, "TSIG Key incomplete : " << ex.what()
+                  << " (" << key_config->getPosition() << ")");
+    }
 
     // Name cannot be blank.
     if (name.empty()) {
-        isc_throw(D2CfgError, "TSIG Key Info must specify name");
+        isc_throw(D2CfgError, "TSIG key must specify name (" << pos["name"] << ")");
     }
 
-    // Algorithm cannot be blank.
-    if (algorithm.empty()) {
-        isc_throw(D2CfgError, "TSIG Key Info must specify algorithm");
+    // Currently, the premise is that key storage is always empty prior to
+    // parsing so we are always adding keys never replacing them. Duplicates
+    // are not allowed and should be flagged as a configuration error.
+    if (keys_->find(name) != keys_->end()) {
+        isc_throw(D2CfgError, "Duplicate TSIG key name specified : " << name
+                              << " (" << pos["name"] << ")");
+    }
+
+    // Algorithm must be valid.
+    try {
+        TSIGKeyInfo::stringToAlgorithmName(algorithm);
+    } catch (const std::exception& ex) {
+        isc_throw(D2CfgError, "TSIG key : " << ex.what() << " (" << pos["algorithm"] << ")");
     }
 
     // Secret cannot be blank.
+    // Cryptolink lib doesn't offer any way to validate these. As long as it
+    // isn't blank we'll accept it.  If the content is bad, the call to in
+    // TSIGKeyInfo::remakeKey() made in the TSIGKeyInfo ctor will throw.
+    // We'll deal with that below.
     if (secret.empty()) {
-        isc_throw(D2CfgError, "TSIG Key Info must specify secret");
+        isc_throw(D2CfgError, "TSIG key must specify secret (" << pos["secret"] << ")");
     }
 
-    // Currently, the premise is that key storage is always empty prior to
-    // parsing so we are always adding keys never replacing them. Duplicates
-    // are not allowed and should be flagged as a configuration error.
-    if (keys_->find(name) != keys_->end()) {
-        isc_throw(D2CfgError, "Duplicate TSIG key specified:" << name);
-    }
+    // Everything should be valid, so create the key instance.
+    // It is possible for the asiodns::dns::TSIGKey create to fail such as
+    // with an invalid secret content.
+    TSIGKeyInfoPtr key_info;
+    try {
+        key_info.reset(new TSIGKeyInfo(name, algorithm, secret));
+    } catch (const std::exception& ex) {
+        isc_throw(D2CfgError, ex.what() << " (" << key_config->getPosition() << ")");
 
-    TSIGKeyInfoPtr key_info(new TSIGKeyInfo(name, algorithm, secret));
+    }
 
     // Add the new TSIGKeyInfo to the key storage.
     (*keys_)[name]=key_info;
 }
 
 isc::dhcp::ParserPtr
-TSIGKeyInfoParser::createConfigParser(const std::string& config_id) {
+TSIGKeyInfoParser::createConfigParser(const std::string& config_id,
+                                      const isc::data::Element::Position& pos) {
     DhcpConfigParser* parser = NULL;
     // Based on the configuration id of the element, create the appropriate
     // parser. Scalars are set to use the parser's local scalar storage.
@@ -404,7 +429,7 @@ TSIGKeyInfoParser::createConfigParser(const std::string& config_id) {
     } else {
         isc_throw(NotImplemented,
                   "parser error: TSIGKeyInfo parameter not supported: "
-                  << config_id);
+                  << config_id << " (" << pos << ")");
     }
 
     // Return the new parser instance.
@@ -487,7 +512,9 @@ DnsServerInfoParser::build(isc::data::ConstElementPtr server_config) {
     // 3. Invoke the parser's commit method to store the element's parsed
     // data to the parser's local storage.
     BOOST_FOREACH (config_pair, server_config->mapValue()) {
-        isc::dhcp::ParserPtr parser(createConfigParser(config_pair.first));
+        isc::dhcp::ParserPtr parser(createConfigParser(config_pair.first,
+                                                       config_pair.second->
+                                                       getPosition()));
         parser->build(config_pair.second);
         parser->commit();
     }
@@ -495,26 +522,48 @@ DnsServerInfoParser::build(isc::data::ConstElementPtr server_config) {
     std::string hostname;
     std::string ip_address;
     uint32_t port = DnsServerInfo::STANDARD_DNS_PORT;
+    std::map<std::string, isc::data::Element::Position> pos;
 
     // Fetch the server configuration's parsed scalar values from parser's
-    // local storage.
-    local_scalars_.getParam("hostname", hostname, DCfgContextBase::OPTIONAL);
-    local_scalars_.getParam("ip_address", ip_address,
-                            DCfgContextBase::OPTIONAL);
-    local_scalars_.getParam("port", port, DCfgContextBase::OPTIONAL);
+    // local storage.  They're all optional, so no try-catch here.
+    pos["hostname"] = local_scalars_.getParam("hostname", hostname,
+                                              DCfgContextBase::OPTIONAL);
+    pos["ip_address"] = local_scalars_.getParam("ip_address", ip_address,
+                                                DCfgContextBase::OPTIONAL);
+    pos["port"] =  local_scalars_.getParam("port", port,
+                                           DCfgContextBase::OPTIONAL);
 
     // The configuration must specify one or the other.
     if (hostname.empty() == ip_address.empty()) {
         isc_throw(D2CfgError, "Dns Server must specify one or the other"
-                  " of hostname and IP address");
+                  " of hostname or IP address"
+                  << " (" << server_config->getPosition() << ")");
+    }
+
+    // Port cannot be zero.
+    if (port == 0) {
+        isc_throw(D2CfgError, "Dns Server : port cannot be 0"
+                  << " (" << pos["port"] << ")");
     }
 
     DnsServerInfoPtr serverInfo;
     if (!hostname.empty()) {
-        // When  hostname is specified, create a valid, blank IOAddress and
-        // then create the DnsServerInfo.
-        isc::asiolink::IOAddress io_addr(DnsServerInfo::EMPTY_IP_STR);
-        serverInfo.reset(new DnsServerInfo(hostname, io_addr, port));
+        /// @todo when resolvable hostname is supported we create the entry
+        /// as follows:
+        ///
+        /// @code
+        /// // When  hostname is specified, create a valid, blank IOAddress
+        /// // and then create the DnsServerInfo.
+        /// isc::asiolink::IOAddress io_addr(DnsServerInfo::EMPTY_IP_STR);
+        /// serverInfo.reset(new DnsServerInfo(hostname, io_addr, port));
+        ///
+        /// @endcode
+        ///
+        /// Resolution will be done prior to connection during transaction
+        /// processing.
+        /// Until then we'll throw unsupported.
+        isc_throw(D2CfgError, "Dns Server : hostname is not yet supported"
+                  << " (" << pos["hostname"] << ")");
     } else {
         try {
             // Create an IOAddress from the IP address string given and then
@@ -522,7 +571,8 @@ DnsServerInfoParser::build(isc::data::ConstElementPtr server_config) {
             isc::asiolink::IOAddress io_addr(ip_address);
             serverInfo.reset(new DnsServerInfo(hostname, io_addr, port));
         } catch (const isc::asiolink::IOError& ex) {
-            isc_throw(D2CfgError, "Invalid IP address:" << ip_address);
+            isc_throw(D2CfgError, "Dns Server : invalid IP address : "
+                      << ip_address << " (" << pos["ip_address"] << ")");
         }
     }
 
@@ -531,7 +581,9 @@ DnsServerInfoParser::build(isc::data::ConstElementPtr server_config) {
 }
 
 isc::dhcp::ParserPtr
-DnsServerInfoParser::createConfigParser(const std::string& config_id) {
+DnsServerInfoParser::createConfigParser(const std::string& config_id,
+                                        const isc::data::Element::
+                                        Position& pos) {
     DhcpConfigParser* parser = NULL;
     // Based on the configuration id of the element, create the appropriate
     // parser. Scalars are set to use the parser's local scalar storage.
@@ -545,7 +597,7 @@ DnsServerInfoParser::createConfigParser(const std::string& config_id) {
     } else {
         isc_throw(NotImplemented,
                   "parser error: DnsServerInfo parameter not supported: "
-                  << config_id);
+                  << config_id << " (" << pos << ")");
     }
 
     // Return the new parser instance.
@@ -591,7 +643,8 @@ build(isc::data::ConstElementPtr server_list){
 
     // Domains must have at least one server.
     if (parsers_.size() == 0) {
-        isc_throw (D2CfgError, "Server List must contain at least one server");
+        isc_throw (D2CfgError, "Server List must contain at least one server"
+                   << " (" << server_list->getPosition() << ")");
     }
 }
 
@@ -630,7 +683,9 @@ DdnsDomainParser::build(isc::data::ConstElementPtr domain_config) {
     // data to the parser's local storage.
     isc::dhcp::ConfigPair config_pair;
     BOOST_FOREACH(config_pair, domain_config->mapValue()) {
-        isc::dhcp::ParserPtr parser(createConfigParser(config_pair.first));
+        isc::dhcp::ParserPtr parser(createConfigParser(config_pair.first,
+                                                       config_pair.second->
+                                                       getPosition()));
         parser->build(config_pair.second);
         parser->commit();
     }
@@ -638,13 +693,24 @@ DdnsDomainParser::build(isc::data::ConstElementPtr domain_config) {
     // Now construct the domain.
     std::string name;
     std::string key_name;
+    std::map<std::string, isc::data::Element::Position> pos;
+
 
-    // Domain name is not optional. The get will throw if its not there.
-    local_scalars_.getParam("name", name);
+    // Fetch the parsed scalar values from parser's local storage.
+    // Any required that are missing will throw.
+    try {
+        pos["name"] = local_scalars_.getParam("name", name);
+        pos["key_name"] = local_scalars_.getParam("key_name", key_name,
+                                                  DCfgContextBase::OPTIONAL);
+    } catch (const std::exception& ex) {
+        isc_throw(D2CfgError, "DdnsDomain incomplete : " << ex.what()
+                  << " (" << domain_config->getPosition() << ")");
+    }
 
     // Blank domain names are not allowed.
     if (name.empty()) {
-        isc_throw(D2CfgError, "Domain name cannot be blank");
+        isc_throw(D2CfgError, "DndsDomain : name cannot be blank ("
+                   << pos["name"] << ")");
     }
 
     // Currently, the premise is that domain storage is always empty
@@ -652,13 +718,13 @@ DdnsDomainParser::build(isc::data::ConstElementPtr domain_config) {
     // Duplicates are not allowed and should be flagged as a configuration
     // error.
     if (domains_->find(name) != domains_->end()) {
-        isc_throw(D2CfgError, "Duplicate domain specified:" << name);
+        isc_throw(D2CfgError, "Duplicate domain specified:" << name
+                  << " (" << pos["name"] << ")");
     }
 
     // Key name is optional. If it is not blank, then find the key in the
     /// list of defined keys.
     TSIGKeyInfoPtr tsig_key_info;
-    local_scalars_.getParam("key_name", key_name, DCfgContextBase::OPTIONAL);
     if (!key_name.empty()) {
         if (keys_) {
             TSIGKeyInfoMap::iterator kit = keys_->find(key_name);
@@ -668,8 +734,9 @@ DdnsDomainParser::build(isc::data::ConstElementPtr domain_config) {
         }
 
         if (!tsig_key_info) {
-            isc_throw(D2CfgError, "DdnsDomain " << name <<
-                     " specifies an undefined key: " << key_name);
+            isc_throw(D2CfgError, "DdnsDomain : " << name
+                      << " specifies an undefined key: " << key_name
+                      << " (" << pos["key_name"] << ")");
         }
     }
 
@@ -681,7 +748,8 @@ DdnsDomainParser::build(isc::data::ConstElementPtr domain_config) {
 }
 
 isc::dhcp::ParserPtr
-DdnsDomainParser::createConfigParser(const std::string& config_id) {
+DdnsDomainParser::createConfigParser(const std::string& config_id,
+                                     const isc::data::Element::Position& pos) {
     DhcpConfigParser* parser = NULL;
     // Based on the configuration id of the element, create the appropriate
     // parser. Scalars are set to use the parser's local scalar storage.
@@ -697,7 +765,7 @@ DdnsDomainParser::createConfigParser(const std::string& config_id) {
     } else {
        isc_throw(NotImplemented,
                 "parser error: DdnsDomain parameter not supported: "
-                << config_id);
+                << config_id << " (" << pos << ")");
     }
 
     // Return the new domain parser instance.
@@ -774,7 +842,9 @@ DdnsDomainListMgrParser::build(isc::data::ConstElementPtr domain_config) {
     // data to the parser's local storage.
     isc::dhcp::ConfigPair config_pair;
     BOOST_FOREACH(config_pair, domain_config->mapValue()) {
-        isc::dhcp::ParserPtr parser(createConfigParser(config_pair.first));
+        isc::dhcp::ParserPtr parser(createConfigParser(config_pair.first,
+                                                       config_pair.second->
+                                                       getPosition()));
         parser->build(config_pair.second);
         parser->commit();
     }
@@ -784,7 +854,9 @@ DdnsDomainListMgrParser::build(isc::data::ConstElementPtr domain_config) {
 }
 
 isc::dhcp::ParserPtr
-DdnsDomainListMgrParser::createConfigParser(const std::string& config_id) {
+DdnsDomainListMgrParser::createConfigParser(const std::string& config_id,
+                                            const isc::data::Element::
+                                            Position& pos) {
     DhcpConfigParser* parser = NULL;
     if (config_id == "ddns_domains") {
        // Domain list parser is given our local domain storage. It will pass
@@ -793,7 +865,8 @@ DdnsDomainListMgrParser::createConfigParser(const std::string& config_id) {
        parser = new DdnsDomainListParser(config_id, local_domains_, keys_);
     } else {
        isc_throw(NotImplemented, "parser error: "
-                 "DdnsDomainListMgr parameter not supported: " << config_id);
+                 "DdnsDomainListMgr parameter not supported: " << config_id
+                 << " (" << pos << ")");
     }
 
     // Return the new domain parser instance.

src/bin/d2/d2_config.h

@@ -754,10 +754,17 @@ public:
     ///
     /// @param config_id is the "item_name" for a specific member element of
     /// the "tsig_key" specification.
+    /// @param pos position within the configuration text (or file) of element
+    /// to be parsed.  This is passed for error messaging.
     ///
     /// @return returns a pointer to newly created parser.
-    virtual isc::dhcp::ParserPtr createConfigParser(const std::string&
-                                                    config_id);
+    ///
+    /// @throw D2CfgError if configuration contains an unknown parameter
+    virtual isc::dhcp::ParserPtr
+    createConfigParser(const std::string& config_id,
+                       const isc::data::Element::Position& pos =
+                       isc::data::Element::ZERO_POSITION());
+
     /// @brief Commits the TSIGKeyInfo configuration
     /// Currently this method is a NOP, as the key instance is created and
     /// then added to a local list of keys in build().
@@ -857,8 +864,15 @@ public:
     /// Parses a configuration for the elements needed to instantiate a
     /// DnsServerInfo, validates those entries, creates a DnsServerInfo instance
     /// then attempts to add to a list of  servers.
+    /// @param pos position within the configuration text (or file) of element
+    /// to be parsed.  This is passed for error messaging.
     ///
     /// @param server_config is the "dns_server" configuration to parse
+    ///
+    /// @throw D2CfgError if:
+    /// -# hostname is not blank, hostname is not yet supported
+    /// -# ip_address is invalid
+    /// -# port is 0
     virtual void build(isc::data::ConstElementPtr server_config);
 
     /// @brief Creates a parser for the given "dns_server" member element id.
@@ -870,10 +884,16 @@ public:
     ///
     /// @param config_id is the "item_name" for a specific member element of
     /// the "dns_server" specification.
+    /// @param pos position within the configuration text (or file) of element
+    /// to be parsed.  This is passed for error messaging.
     ///
     /// @return returns a pointer to newly created parser.
-    virtual isc::dhcp::ParserPtr createConfigParser(const std::string&
-                                                    config_id);
+    ///
+    /// @throw D2CfgError if configuration contains an unknown parameter
+    virtual isc::dhcp::ParserPtr
+    createConfigParser(const std::string& config_id,
+                       const isc::data::Element::Position& =
+                       isc::data::Element::ZERO_POSITION());
 
     /// @brief Commits the configured DnsServerInfo
     /// Currently this method is a NOP, as the server instance is created and
@@ -985,10 +1005,16 @@ public:
     ///
     /// @param config_id is the "item_name" for a specific member element of
     /// the "ddns_domain" specification.
+    /// @param pos position within the configuration text (or file) of element
+    /// to be parsed.  This is passed for error messaging.
     ///
     /// @return returns a pointer to newly created parser.
-    virtual isc::dhcp::ParserPtr createConfigParser(const std::string&
-                                                    config_id);
+    ///
+    /// @throw D2CfgError if configuration contains an unknown parameter
+    virtual isc::dhcp::ParserPtr
+    createConfigParser(const std::string& config_id,
+                       const isc::data::Element::Position& pos =
+                       isc::data::Element::ZERO_POSITION());
 
     /// @brief Commits the configured DdnsDomain
     /// Currently this method is a NOP, as the domain instance is created and
@@ -1119,10 +1145,16 @@ public:
     ///
     /// @param config_id is the "item_name" for a specific member element of
     /// the manager specification.
+    /// @param pos position within the configuration text (or file) of element
+    /// to be parsed.  This is passed for error messaging.
     ///
     /// @return returns a pointer to newly created parser.
-    virtual isc::dhcp::ParserPtr createConfigParser(const std::string&
-                                                    config_id);
+    ///
+    /// @throw D2CfgError if configuration contains an unknown parameter
+    virtual isc::dhcp::ParserPtr
+    createConfigParser(const std::string& config_id,
+                       const isc::data::Element::Position& pos =
+                       isc::data::Element::ZERO_POSITION());
 
     /// @brief Commits the configured DdsnDomainListMgr
     /// Currently this method is a NOP, as the manager instance is created

src/bin/d2/d2_messages.mes

@@ -32,7 +32,7 @@ new configuration. It is output during server startup, and when an updated
 configuration is committed by the administrator.  Additional information
 may be provided.
 
-% DCTL_CONFIG_FILE_LOAD_FAIL %1 configuration could not be loaded from file: %2
+% DCTL_CONFIG_FILE_LOAD_FAIL %1 reason: %2
 This fatal error message indicates that the application attempted to load its
 initial configuration from file and has failed. The service will exit.
 
@@ -72,22 +72,11 @@ application and will exit.
 A warning message is issued when an attempt is made to shut down the
 application when it is not running.
 
-% DCTL_ORDER_ERROR configuration contains more elements than the parsing order
-An error message which indicates that configuration being parsed includes
-element ids not specified the configuration manager's parse order list. This
-is a programmatic error.
-
-% DCTL_ORDER_NO_ELEMENT element: %1 is in the parsing order but is missing from the configuration
-An error message output during a configuration update.  The program is
-expecting an item but has not found it in the new configuration.  This may
-mean that the BIND 10 configuration database is corrupt.
-
-% DCTL_PARSER_FAIL configuration parsing failed for configuration element: %1, reason: %2
-On receipt of message containing details to a change of its configuration,
-the server failed to create a parser to decode the contents of the named
-configuration element, or the creation succeeded but the parsing actions
-and committal of changes failed.  The reason for the failure is given in
-the message.
+% DCTL_PARSER_FAIL : %1
+On receipt of a new configuration, the server failed to create a parser to
+decode the contents of the named configuration element, or the creation
+succeeded but the parsing actions and committal of changes failed.
+The reason for the failure is given in the message.
 
 % DCTL_PROCESS_FAILED %1 application execution failed: %2
 The controller has encountered a fatal error while running the

src/bin/d2/d_cfg_mgr.cc

@@ -52,43 +52,51 @@ DCfgContextBase::DCfgContextBase(const DCfgContextBase& rhs):
         string_values_(new StringStorage(*(rhs.string_values_))) {
 }
 
-void
+const data::Element::Position&
 DCfgContextBase::getParam(const std::string& name, bool& value, bool optional) {
     try {
         value = boolean_values_->getParam(name);
+        return (boolean_values_->getPosition(name));
     } catch (DhcpConfigError& ex) {
         // If the parameter is not optional, re-throw the exception.
         if (!optional) {
             throw;
         }
     }
-}
 
+    return (data::Element::ZERO_POSITION());
+}
 
-void
+const data::Element::Position&
 DCfgContextBase::getParam(const std::string& name, uint32_t& value,
                           bool optional) {
     try {
         value = uint32_values_->getParam(name);
+        return (uint32_values_->getPosition(name));
     } catch (DhcpConfigError& ex) {
         // If the parameter is not optional, re-throw the exception.
         if (!optional) {
             throw;
         }
     }
+
+    return (data::Element::ZERO_POSITION());
 }
 
-void
+const data::Element::Position&
 DCfgContextBase::getParam(const std::string& name, std::string& value,
                           bool optional) {
     try {
         value = string_values_->getParam(name);
+        return (string_values_->getPosition(name));
     } catch (DhcpConfigError& ex) {
         // If the parameter is not optional, re-throw the exception.
         if (!optional) {
             throw;
         }
     }
+
+    return (data::Element::ZERO_POSITION());
 }
 
 DCfgContextBase::~DCfgContextBase() {
@@ -189,37 +197,43 @@ DCfgMgrBase::parseConfig(isc::data::ConstElementPtr config_set) {
             // thrown.  Note, that elements tagged as "optional" from the user
             // perspective must still have default or empty entries in the
             // configuration set to be parsed.
-            int parsed_count = 0;
             std::map<std::string, ConstElementPtr>::const_iterator it;
             BOOST_FOREACH(element_id, parse_order_) {
                 it = objects_map.find(element_id);
                 if (it != objects_map.end()) {
-                    ++parsed_count;
                     buildAndCommit(element_id, it->second);
+                    // We parsed it, take it out of the list.
+                    objects_map.erase(it);
                 }
                 else {
-                    LOG_ERROR(dctl_logger, DCTL_ORDER_NO_ELEMENT)
-                              .arg(element_id);
-                    isc_throw(DCfgMgrBaseError, "Element:" << element_id <<
-                              " is listed in the parse order but is not "
-                              " present in the configuration");
+                    isc_throw(DCfgMgrBaseError,
+                               "Element required by parsing order is missing: "
+                               << element_id << " ("
+                               << config_set->getPosition() << ")");
                 }
             }
 
             // NOTE: When using ordered parsing, the parse order list MUST
             // include every possible element id that the value_map may contain.
-            // Entries in the map that are not in the parse order, would not be
+            // Entries in the map that are not in the parse order, will not be
             // parsed. For now we will flag this as a programmatic error.  One
             // could attempt to adjust for this, by identifying such entries
             // and parsing them either first or last but which would be correct?
-            // Better to hold the engineer accountable.  So, if we parsed none
-            // or we parsed fewer than are in the map; then either the parse i
-            // order is incomplete OR the map has unsupported values.
-            if (!parsed_count ||
-                (parsed_count && ((parsed_count + 1) < objects_map.size()))) {
-                LOG_ERROR(dctl_logger, DCTL_ORDER_ERROR);
+            // Better to hold the engineer accountable.  So, if there are any
+            // left in the objects_map then they were not in the parse order.
+            if (!objects_map.empty()) {
+                std::ostringstream stream;
+                bool add_comma = false;
+                ConfigPair config_pair;
+                BOOST_FOREACH(config_pair, objects_map) {
+                    stream << ( add_comma ? ", " : "") << config_pair.first
+                           << " (" << config_pair.second->getPosition() << ")";
+                    add_comma = true;
+                }
+
                 isc_throw(DCfgMgrBaseError,
-                        "Configuration contains elements not in parse order");
+                        "Configuration contains elements not in parse order: "
+                        << stream.str());
             }
         } else {
             // Order doesn't matter so iterate over the value map directly.
@@ -235,10 +249,9 @@ DCfgMgrBase::parseConfig(isc::data::ConstElementPtr config_set) {
         LOG_INFO(dctl_logger, DCTL_CONFIG_COMPLETE).arg("");
         answer = isc::config::createAnswer(0, "Configuration committed.");
 
-    } catch (const isc::Exception& ex) {
-        LOG_ERROR(dctl_logger, DCTL_PARSER_FAIL).arg(element_id).arg(ex.what());
-        answer = isc::config::createAnswer(1,
-                     string("Configuration parsing failed: ") + ex.what());
+    } catch (const std::exception& ex) {
+        LOG_ERROR(dctl_logger, DCTL_PARSER_FAIL).arg(ex.what());
+        answer = isc::config::createAnswer(1, ex.what());
 
         // An error occurred, so make sure that we restore original context.
         context_ = original_context;
@@ -253,7 +266,8 @@ DCfgMgrBase::buildParams(isc::data::ConstElementPtr params_config) {
     // Loop through scalars parsing them and committing them to storage.
     BOOST_FOREACH(dhcp::ConfigPair param, params_config->mapValue()) {
         // Call derivation's method to create the proper parser.
-        dhcp::ParserPtr parser(createConfigParser(param.first));
+        dhcp::ParserPtr parser(createConfigParser(param.first,
+                                                  param.second->getPosition()));
         parser->build(param.second);
         parser->commit();
     }
@@ -263,28 +277,21 @@ void DCfgMgrBase::buildAndCommit(std::string& element_id,
                                  isc::data::ConstElementPtr value) {
     // Call derivation's implementation to create the appropriate parser
     // based on the element id.
-    ParserPtr parser = createConfigParser(element_id);
+    ParserPtr parser = createConfigParser(element_id, value->getPosition());
     if (!parser) {
         isc_throw(DCfgMgrBaseError, "Could not create parser");
     }
 
-    try {
-        // Invoke the parser's build method passing in the value. This will
-        // "convert" the Element form of value into the actual data item(s)
-        // and store them in parser's local storage.
-        parser->build(value);
-
-        // Invoke the parser's commit method. This "writes" the the data
-        // item(s) stored locally by the parser into the context.  (Note that
-        // parsers are free to do more than update the context, but that is an
-        // nothing something we are concerned with here.)
-        parser->commit();
-    } catch (const isc::Exception& ex) {
-        isc_throw(DCfgMgrBaseError,
-                  "Could not build and commit: " << ex.what());
-    } catch (...) {
-        isc_throw(DCfgMgrBaseError, "Non-ISC exception occurred");
-    }
+    // Invoke the parser's build method passing in the value. This will
+    // "convert" the Element form of value into the actual data item(s)
+    // and store them in parser's local storage.
+    parser->build(value);
+
+    // Invoke the parser's commit method. This "writes" the the data
+    // item(s) stored locally by the parser into the context.  (Note that
+    // parsers are free to do more than update the context, but that is an
+    // nothing something we are concerned with here.)
+    parser->commit();
 }
 
 }; // end of isc::dhcp namespace

src/bin/d2/d_cfg_mgr.h

@@ -80,9 +80,14 @@ public:
     /// will not throw if the parameter is not found in the context. The
     /// contents of the output parameter, value, will not be altered.
     /// It defaults to false if not specified.
+    ///
+    /// @return The parameter's element's position information if found,
+    /// otherwise it returns isc::data::Element::ZERO_POSITION().
+    ///
     /// @throw throws DhcpConfigError if the context does not contain the
     /// parameter and optional is false.
-    void getParam(const std::string& name, bool& value, bool optional=false);
+    const data::Element::Position&
+    getParam(const std::string& name, bool& value, bool optional=false);
 
     /// @brief Fetches the value for a given uint32_t configuration parameter
     /// from the context.
@@ -93,9 +98,14 @@ public:
     /// @param optional if true, the parameter is optional and the method
     /// will not throw if the parameter is not found in the context. The
     /// contents of the output parameter, value, will not be altered.
+    ///
+    /// @return The parameter's element's position information if found,
+    /// otherwise it returns isc::data::Element::ZERO_POSITION().
+    ///
     /// @throw throws DhcpConfigError if the context does not contain the
     /// parameter and optional is false.
-    void getParam(const std::string& name, uint32_t& value,
+    const data::Element::Position&
+    getParam(const std::string& name, uint32_t& value,
                  bool optional=false);
 
     /// @brief Fetches the value for a given string configuration parameter
@@ -107,9 +117,14 @@ public:
     /// @param optional if true, the parameter is optional and the method
     /// will not throw if the parameter is not found in the context. The
     /// contents of the output parameter, value, will not be altered.
+    ///
+    /// @return The parameter's element's position information if found,
+    /// otherwise it returns isc::data::Element::ZERO_POSITION().
+    ///
     /// @throw throws DhcpConfigError if the context does not contain the
     /// parameter and optional is false.
-    void getParam(const std::string& name, std::string& value,
+    const data::Element::Position&
+    getParam(const std::string& name, std::string& value,
                   bool optional=false);
 
     /// @brief Fetches the Boolean Storage. Typically used for passing
@@ -316,11 +331,15 @@ protected:
     ///
     /// @param element_id is the string name of the element as it will appear
     /// in the configuration set.
+    /// @param pos position within the configuration text (or file) of element
+    /// to be parsed.  This is passed for error messaging.
     ///
     /// @return returns a ParserPtr to the parser instance.
     /// @throw throws DCfgMgrBaseError if an error occurs.
     virtual isc::dhcp::ParserPtr
-    createConfigParser(const std::string& element_id) = 0;
+    createConfigParser(const std::string& element_id,
+                       const isc::data::Element::Position& pos
+                       = isc::data::Element::Position()) = 0;
 
     /// @brief Abstract factory which creates a context instance.
     ///

src/bin/d2/tests/Makefile.am

@@ -8,6 +8,7 @@ endif
 noinst_SCRIPTS = d2_process_tests.sh
 
 EXTRA_DIST  = d2_process_tests.sh.in
+EXTRA_DIST += testdata/d2_cfg_tests.json
 
 # test using command-line arguments, so use check-local target instead of TESTS
 check-local:
@@ -110,6 +111,7 @@ d2_unittests_LDADD += $(top_builddir)/src/lib/config/libkea-cfgclient.la
 d2_unittests_LDADD += $(top_builddir)/src/lib/dhcp_ddns/libkea-dhcp_ddns.la
 d2_unittests_LDADD += $(top_builddir)/src/lib/dhcp/libkea-dhcp++.la
 d2_unittests_LDADD += $(top_builddir)/src/lib/dhcpsrv/libkea-dhcpsrv.la
+d2_unittests_LDADD += $(top_builddir)/src/lib/dhcpsrv/testutils/libdhcpsrvtest.la
 d2_unittests_LDADD += $(top_builddir)/src/lib/dns/libkea-dns++.la
 d2_unittests_LDADD += $(top_builddir)/src/lib/util/libkea-util.la
 d2_unittests_LDADD += $(top_builddir)/src/lib/hooks/libkea-hooks.la

src/bin/d2/tests/d2_cfg_mgr_unittests.cc

@@ -18,6 +18,7 @@
 #include <d_test_stubs.h>
 #include <test_data_files_config.h>
 #include <util/encode/base64.h>
+#include <dhcpsrv/testutils/config_result_check.h>
 
 #include <boost/foreach.hpp>
 #include <gtest/gtest.h>
@@ -28,10 +29,26 @@ using namespace isc::d2;
 
 namespace {
 
+/// @brief Function to create full path to the spec file
+///
+/// The full path is dependent upon the value of D2_SRC_DIR which
+/// whose value is generated from test_data_files_config.h.in
+///
+/// @param name file name to which the path should be prepended
 std::string specfile(const std::string& name) {
     return (std::string(D2_SRC_DIR) + "/" + name);
 }
 
+/// @brief Function to create full path to test data file
+///
+/// The full path is dependent upon the value of D2_TEST_DATA_DIR which
+/// whose value is generated from test_data_files_config.h.in
+///
+/// @param name file name to which the path should be prepended
+std::string testDataFile(const std::string& name) {
+    return (std::string(D2_TEST_DATA_DIR) + "/" + name);
+}
+
 /// @brief Test fixture class for testing D2CfgMgr class.
 /// It maintains an member instance of D2CfgMgr and provides methods for
 /// converting JSON strings to configuration element sets, checking parse
@@ -82,6 +99,11 @@ public:
 
         return (config.str());
     }
+    /// @brief Enumeration to select between expected configuration outcomes
+    enum RunConfigMode {
+       SHOULD_PASS,
+       SHOULD_FAIL
+    };
 
     /// @brief Parses a configuration string and tests against a given outcome
     ///
@@ -93,15 +115,16 @@ public:
     /// the D2Params pointer with the newly parsed instance.
     ///
     /// @param config_str the JSON configuration text to parse
-    /// @param should_fail boolean indicator if the parsing should fail or not.
-    /// It defaults to false.
-    void runConfig(std::string config_str, bool should_fail=false) {
+    /// @param mode indicator if the parsing should fail or not.  It defaults
+    /// defaults to SHOULD_PASS.
+    ///
+    void runConfig(std::string config_str, RunConfigMode mode=SHOULD_PASS) {
         // We assume the config string is valid JSON.
         ASSERT_TRUE(fromJSON(config_str));
 
         // Parse the configuration and verify we got the expected outcome.
         answer_ = cfg_mgr_->parseConfig(config_set_);
-        ASSERT_TRUE(checkAnswer(should_fail));
+        ASSERT_TRUE(checkAnswer(mode == SHOULD_FAIL));
 
         // Verify that the D2 context can be retrieved and is not null.
         D2CfgContextPtr context;
@@ -112,6 +135,50 @@ public:
         ASSERT_TRUE(d2_params_);
     }
 
+    /// @brief Check parse result against expected outcome and position info
+    ///
+    /// This method analyzes the given parsing result against an expected outcome
+    /// of SHOULD_PASS or SHOULD_FAIL.  If it is expected to fail, the comment
+    /// contained within the result is searched for Element::Position information
+    /// which should contain the given file name.  It does not attempt to verify
+    /// the numerical values for line number and col.
+    ///
+    /// @param answer Element set containing an integer result code and string
+    /// comment.
+    /// @param mode indicator if the parsing should fail or not.
+    /// @param file_name name of the file containing the configuration text
+    /// parsed. It defaults to "<string>" which is the value present if the
+    /// configuration text did not originate from a file. (i.e. one did not use
+    /// isc::data::Element::fromJSONFile() to read the JSON text).
+    void
+    checkAnswerWithError(isc::data::ConstElementPtr answer,
+                         RunConfigMode mode, std::string file_name="<string>") {
+        int rcode = 0;
+        isc::data::ConstElementPtr comment;
+        comment = isc::config::parseAnswer(rcode, answer);
+
+        if (mode == SHOULD_PASS) {
+            if (rcode == 0) {
+                return;
+            }
+
+            FAIL() << "Parsing was expected to pass but failed : " << rcode
+                   << " comment: " << *comment;
+        }
+
+        if (rcode == 0) {
+            FAIL() << "Parsing was expected to fail but passed : "
+                   << " comment: " << *comment;
+        }
+
+        // Parsing was expected to fail, test for position info.
+        if (isc::dhcp::test::errorContainsPosition(answer, file_name)) {
+            return;
+        }
+
+        FAIL() << "Parsing failed as expected but lacks position : " << *comment;
+    }
+
     /// @brief Pointer the D2Params most recently parsed.
     D2ParamsPtr d2_params_;
 };
@@ -397,6 +464,44 @@ TEST_F(D2CfgMgrTest, defaultValues) {
               d2_params_->getNcrFormat());
 }
 
+/// @brief Tests the unsupported scalar parameters and objects are detected.
+TEST_F(D2CfgMgrTest, unsupportedTopLevelItems) {
+    // Check that an unsupported top level parameter fails.
+    std::string config =
+            "{"
+            " \"ip_address\": \"127.0.0.1\", "
+            " \"port\": 777 , "
+            " \"dns_server_timeout\": 333 , "
+            " \"ncr_protocol\": \"UDP\" , "
+            " \"ncr_format\": \"JSON\", "
+            "\"tsig_keys\": [], "
+            "\"forward_ddns\" : {}, "
+            "\"reverse_ddns\" : {}, "
+            "\"bogus_param\" : true "
+            "}";
+
+    runConfig(config, SHOULD_FAIL);
+
+    // Check that unsupported top level objects fails.  For
+    // D2 these fail as they are not in the parse order.
+    config =
+            "{"
+            " \"ip_address\": \"127.0.0.1\", "
+            " \"port\": 777 , "
+            " \"dns_server_timeout\": 333 , "
+            " \"ncr_protocol\": \"UDP\" , "
+            " \"ncr_format\": \"JSON\", "
+            "\"tsig_keys\": [], "
+            "\"bogus_object_one\" : {}, "
+            "\"forward_ddns\" : {}, "
+            "\"reverse_ddns\" : {}, "
+            "\"bogus_object_two\" : {} "
+            "}";
+
+    runConfig(config, SHOULD_FAIL);
+}
+
+
 /// @brief Tests the enforcement of data validation when parsing D2Params.
 /// It verifies that:
 /// -# ip_address cannot be "0.0.0.0"
@@ -409,27 +514,31 @@ TEST_F(D2CfgMgrTest, invalidEntry) {
     // Cannot use IPv4 ANY address
     std::string config = makeParamsConfigString ("0.0.0.0", 777, 333,
                                            "UDP", "JSON");
-    runConfig(config, 1);
+    runConfig(config, SHOULD_FAIL);
 
     // Cannot use IPv6 ANY address
     config = makeParamsConfigString ("::", 777, 333, "UDP", "JSON");
-    runConfig(config, 1);
+    runConfig(config, SHOULD_FAIL);
 
     // Cannot use port  0
     config = makeParamsConfigString ("127.0.0.1", 0, 333, "UDP", "JSON");
-    runConfig(config, 1);
+    runConfig(config, SHOULD_FAIL);
 
     // Cannot use dns server timeout of 0
     config = makeParamsConfigString ("127.0.0.1", 777, 0, "UDP", "JSON");
-    runConfig(config, 1);
+    runConfig(config, SHOULD_FAIL);
 
     // Invalid protocol
     config = makeParamsConfigString ("127.0.0.1", 777, 333, "BOGUS", "JSON");
-    runConfig(config, 1);
+    runConfig(config, SHOULD_FAIL);
+
+    // Unsupported protocol
+    config = makeParamsConfigString ("127.0.0.1", 777, 333, "TCP", "JSON");
+    runConfig(config, SHOULD_FAIL);
 
     // Invalid format
     config = makeParamsConfigString ("127.0.0.1", 777, 333, "UDP", "BOGUS");
-    runConfig(config, 1);
+    runConfig(config, SHOULD_FAIL);
 }
 
 /// @brief Tests the enforcement of data validation when parsing TSIGKeyInfos.
@@ -724,30 +833,31 @@ TEST_F(DnsServerInfoTest, invalidEntry) {
 /// 2. A DnsServerInfo entry is correctly made, when given ip address and port.
 /// 3. A DnsServerInfo entry is correctly made, when given only an ip address.
 TEST_F(DnsServerInfoTest, validEntry) {
-    // Valid entries for dynamic host
-    std::string config = "{ \"hostname\": \"pegasus.tmark\" }";
-    ASSERT_TRUE(fromJSON(config));
+    /// @todo When resolvable hostname is supported you'll need this test.
+    /// // Valid entries for dynamic host
+    /// std::string config = "{ \"hostname\": \"pegasus.tmark\" }";
+    /// ASSERT_TRUE(fromJSON(config));
 
-    // Verify that it builds and commits without throwing.
-    ASSERT_NO_THROW(parser_->build(config_set_));
-    ASSERT_NO_THROW(parser_->commit());
+    /// // Verify that it builds and commits without throwing.
+    /// ASSERT_NO_THROW(parser_->build(config_set_));
+    /// ASSERT_NO_THROW(parser_->commit());
 
-    // Verify the correct number of servers are present
-    int count =  servers_->size();
-    EXPECT_EQ(1, count);
+    /// //Verify the correct number of servers are present
+    /// int count =  servers_->size();
+    /// EXPECT_EQ(1, count);
 
-    // Verify the server exists and has the correct values.
-    DnsServerInfoPtr server = (*servers_)[0];
-    EXPECT_TRUE(checkServer(server, "pegasus.tmark",
-                            DnsServerInfo::EMPTY_IP_STR,
-                            DnsServerInfo::STANDARD_DNS_PORT));
+    /// Verify the server exists and has the correct values.
+    /// DnsServerInfoPtr server = (*servers_)[0];
+    /// EXPECT_TRUE(checkServer(server, "pegasus.tmark",
+    ///                         DnsServerInfo::EMPTY_IP_STR,
+    ///                         DnsServerInfo::STANDARD_DNS_PORT));
 
-    // Start over for a new test.
-    reset();
+    /// // Start over for a new test.
+    /// reset();
 
     // Valid entries for static ip
-    config = " { \"ip_address\": \"127.0.0.1\" , "
-             "  \"port\": 100 }";
+    std::string config = " { \"ip_address\": \"127.0.0.1\" , "
+                         "  \"port\": 100 }";
     ASSERT_TRUE(fromJSON(config));
 
     // Verify that it builds and commits without throwing.
@@ -755,11 +865,11 @@ TEST_F(DnsServerInfoTest, validEntry) {
     ASSERT_NO_THROW(parser_->commit());
 
     // Verify the correct number of servers are present
-    count =  servers_->size();
+    int count =  servers_->size();
     EXPECT_EQ(1, count);
 
     // Verify the server exists and has the correct values.
-    server = (*servers_)[0];
+    DnsServerInfoPtr server = (*servers_)[0];
     EXPECT_TRUE(checkServer(server, "", "127.0.0.1", 100));
 
     // Start over for a new test.
@@ -787,9 +897,9 @@ TEST_F(DnsServerInfoTest, validEntry) {
 /// entries is detected.
 TEST_F(ConfigParseTest, invalidServerList) {
     // Construct a list of servers with an invalid server entry.
-    std::string config = "[ { \"hostname\": \"one.tmark\" }, "
-                        "{ \"hostname\": \"\" }, "
-                        "{ \"hostname\": \"three.tmark\" } ]";
+    std::string config = "[ { \"ip_address\": \"127.0.0.1\" }, "
+                        "{ \"ip_address\": \"\" }, "
+                        "{ \"ip_address\": \"127.0.0.2\" } ]";
     ASSERT_TRUE(fromJSON(config));
 
     // Create the server storage and list parser.
@@ -805,9 +915,9 @@ TEST_F(ConfigParseTest, invalidServerList) {
 /// a valid configuration.
 TEST_F(ConfigParseTest, validServerList) {
     // Create a valid list of servers.
-    std::string config = "[ { \"hostname\": \"one.tmark\" }, "
-                        "{ \"hostname\": \"two.tmark\" }, "
-                        "{ \"hostname\": \"three.tmark\" } ]";
+    std::string config = "[ { \"ip_address\": \"127.0.0.1\" }, "
+                        "{ \"ip_address\": \"127.0.0.2\" }, "
+                        "{ \"ip_address\": \"127.0.0.3\" } ]";
     ASSERT_TRUE(fromJSON(config));
 
     // Create the server storage and list parser.
@@ -825,17 +935,17 @@ TEST_F(ConfigParseTest, validServerList) {
 
     // Verify the first server exists and has the correct values.
     DnsServerInfoPtr server = (*servers)[0];
-    EXPECT_TRUE(checkServer(server, "one.tmark", DnsServerInfo::EMPTY_IP_STR,
+    EXPECT_TRUE(checkServer(server, "", "127.0.0.1",
                             DnsServerInfo::STANDARD_DNS_PORT));
 
     // Verify the second server exists and has the correct values.
     server = (*servers)[1];
-    EXPECT_TRUE(checkServer(server, "two.tmark", DnsServerInfo::EMPTY_IP_STR,
+    EXPECT_TRUE(checkServer(server, "", "127.0.0.2",
                             DnsServerInfo::STANDARD_DNS_PORT));
 
     // Verify the third server exists and has the correct values.
     server = (*servers)[2];
-    EXPECT_TRUE(checkServer(server, "three.tmark", DnsServerInfo::EMPTY_IP_STR,
+    EXPECT_TRUE(checkServer(server, "", "127.0.0.3",
                             DnsServerInfo::STANDARD_DNS_PORT));
 }
 
@@ -864,7 +974,7 @@ TEST_F(DdnsDomainTest, invalidDdnsDomainEntry) {
     ASSERT_TRUE(fromJSON(config));
 
     // Verify that the domain configuration builds fails.
-    EXPECT_THROW(parser_->build(config_set_), isc::dhcp::DhcpConfigError);
+    EXPECT_THROW(parser_->build(config_set_), D2CfgError);
 
     // Create a domain configuration with an empty server list.
     config = "{ \"name\": \"tmark.org\" , "
@@ -1155,17 +1265,17 @@ TEST_F(D2CfgMgrTest, fullConfig) {
                         "{ \"name\": \"tmark.org\" , "
                         "  \"key_name\": \"d2_key.tmark.org\" , "
                         "  \"dns_servers\" : [ "
-                        "  { \"hostname\": \"one.tmark\" } , "
-                        "  { \"hostname\": \"two.tmark\" } , "
-                        "  { \"hostname\": \"three.tmark\"} "
+                        "  { \"ip_address\": \"127.0.0.1\" } , "
+                        "  { \"ip_address\": \"127.0.0.2\" } , "
+                        "  { \"ip_address\": \"127.0.0.3\"} "
                         "  ] } "
                         ", "
                         "{ \"name\": \"billcat.net\" , "
                         "  \"key_name\": \"d2_key.billcat.net\" , "
                         "  \"dns_servers\" : [ "
-                        "  { \"hostname\": \"four.billcat\" } , "
-                        "  { \"hostname\": \"five.billcat\" } , "
-                        "  { \"hostname\": \"six.billcat\" } "
+                        "  { \"ip_address\": \"127.0.0.4\" } , "
+                        "  { \"ip_address\": \"127.0.0.5\" } , "
+                        "  { \"ip_address\": \"127.0.0.6\" } "
                         "  ] } "
                         "] },"
                         "\"reverse_ddns\" : {"
@@ -1173,17 +1283,17 @@ TEST_F(D2CfgMgrTest, fullConfig) {
                         "{ \"name\": \" 0.168.192.in.addr.arpa.\" , "
                         "  \"key_name\": \"d2_key.tmark.org\" , "
                         "  \"dns_servers\" : [ "
-                        "  { \"hostname\": \"one.rev\" } , "
-                        "  { \"hostname\": \"two.rev\" } , "
-                        "  { \"hostname\": \"three.rev\" } "
+                        "  { \"ip_address\": \"127.0.1.1\" } , "
+                        "  { \"ip_address\": \"127.0.2.1\" } , "
+                        "  { \"ip_address\": \"127.0.3.1\" } "
                         "  ] } "
                         ", "
                         "{ \"name\": \" 0.247.106.in.addr.arpa.\" , "
                         "  \"key_name\": \"d2_key.billcat.net\" , "
                         "  \"dns_servers\" : [ "
-                        "  { \"hostname\": \"four.rev\" }, "
-                        "  { \"hostname\": \"five.rev\" } , "
-                        "  { \"hostname\": \"six.rev\" } "
+                        "  { \"ip_address\": \"127.0.4.1\" }, "
+                        "  { \"ip_address\": \"127.0.5.1\" } , "
+                        "  { \"ip_address\": \"127.0.6.1\" } "
                         "  ] } "
                         "] } }";
 
@@ -1292,7 +1402,7 @@ TEST_F(D2CfgMgrTest, forwardMatch) {
                         ", "
                         "{ \"name\": \"*\" , "
                         "  \"dns_servers\" : [ "
-                        "  { \"hostname\": \"global.net\" } "
+                        "  { \"ip_address\": \"127.0.0.3\" } "
                         "  ] } "
                         "] }, "
                         "\"reverse_ddns\" : {} "
@@ -1514,4 +1624,99 @@ TEST_F(D2CfgMgrTest, matchReverse) {
     ASSERT_THROW(cfg_mgr_->matchReverse("", match), D2CfgError);
 }
 
+/// @brief Tests D2 config parsing against a wide range of config permutations.
+/// It iterates over all of the test configurations described in given file.
+/// The file content is JSON specialized to this test. The format of the file
+/// is:
+///
+/// @code
+/// # The file must open with a list. It's name is arbitrary.
+///
+/// { "test_list" :
+/// [
+///
+/// #    Test one starts here:
+///      {
+///
+/// #    Each test has:
+/// #      1. description - optional text description
+/// #      2. should_fail - bool indicator if parsing is expected to file
+/// #         (defaults to false)
+/// #       3. data - configuration text to parse
+/// #
+///      "description" : "<text describing test>",
+///      "should_fail" : <true|false> ,
+///      "data" :
+///          {
+/// #        configuration elements here
+///          "bool_val" : false,
+///          "some_map" :  {}
+/// #         :
+///          }
+///      }
+///
+/// #    Next test would start here
+///      ,
+///      {
+///      }
+///
+/// ]}
+///
+/// @endcode
+///
+/// (The file supports comments per Element::fromJSONFile())
+///
+TEST_F(D2CfgMgrTest, configPermutations) {
+    std::string test_file = testDataFile("d2_cfg_tests.json");
+    isc::data::ConstElementPtr tests;
+
+    // Read contents of the file and parse it as JSON. Note it must contain
+    // all valid JSON, we aren't testing JSON parsing.
+    try {
+        tests = isc::data::Element::fromJSONFile(test_file, true);
+    } catch (const std::exception& ex) {
+        FAIL() << "ERROR parsing file : " << test_file << " : " << ex.what();
+    }
+
+    // Read in each test For each test, read:
+    //  1. description - optional text description
+    //  2. should_fail - bool indicator if parsing is expected to file (defaults
+    //     to false
+    //  3. data - configuration text to parse
+    //
+    // Next attempt to parse the configuration by passing it into
+    // D2CfgMgr::parseConfig().  Then check the parsing outcome against the
+    // expected outcome as given by should_fail.
+    isc::data::ConstElementPtr test;
+    BOOST_FOREACH(test, tests->get("test_list")->listValue()) {
+
+        // Grab the description.
+        std::string description = "<no desc>";
+        isc::data::ConstElementPtr elem = test->get("description");
+        if (elem) {
+            elem->getValue(description);
+        }
+
+        // Grab the outcome flag, should_fail, defaults to false if it's
+        // not specified.
+        bool should_fail = false;
+        elem = test->get("should_fail");
+        if (elem)  {
+            elem->getValue(should_fail);
+        }
+
+        // Grab the test's configuration data.
+        isc::data::ConstElementPtr data = test->get("data");
+        ASSERT_TRUE(data) << "No data for test: "
+                          << " : " << test->getPosition();
+
+        // Attempt to parse the configuration. We verify that we get the expected
+        // outcome, and if it was supposed to fail if the explanation contains
+        // position information.
+        checkAnswerWithError(cfg_mgr_->parseConfig(data),
+                             (should_fail ? SHOULD_FAIL : SHOULD_PASS),
+                             test_file);
+    }
+}
+
 } // end of anonymous namespace

src/bin/d2/tests/d2_process_unittests.cc

@@ -47,7 +47,7 @@ const char* bad_ip_d2_config = "{ "
                         "{ \"name\": \"tmark.org\" , "
                         "  \"key_name\": \"d2_key.tmark.org\" , "
                         "  \"dns_servers\" : [ "
-                        "  { \"hostname\": \"one.tmark\" } "
+                        "  { \"ip_address\": \"127.0.0.101\" } "
                         "] } ] }, "
                         "\"reverse_ddns\" : {"
                         "\"ddns_domains\": [ "

src/bin/d2/tests/d_cfg_mgr_unittests.cc

@@ -53,7 +53,8 @@ public:
 
     /// @brief Dummy implementation as this method is abstract.
     virtual isc::dhcp::ParserPtr
-    createConfigParser(const std::string& /* element_id */) {
+    createConfigParser(const std::string& /* element_id */,
+                       const isc::data::Element::Position& /* pos */) {
         return (isc::dhcp::ParserPtr());
     }
 };
@@ -433,4 +434,50 @@ TEST_F(DStubCfgMgrTest, rollBackTest) {
     EXPECT_TRUE(object);
 }
 
+// Tests that configuration element position is returned by getParam variants.
+TEST_F(DStubCfgMgrTest, paramPosition) {
+    // Create a configuration with one of each scalar types.  We end them
+    // with line feeds so we can test position value.
+    string config = "{ \"bool_test\": true , \n"
+                    "  \"uint32_test\": 77 , \n"
+                    "  \"string_test\": \"hmmm chewy\" }";
+    ASSERT_TRUE(fromJSON(config));
+
+    // Verify that the configuration parses without error.
+    answer_ = cfg_mgr_->parseConfig(config_set_);
+    ASSERT_TRUE(checkAnswer(0));
+    DStubContextPtr context = getStubContext();
+    ASSERT_TRUE(context);
+
+    // Verify that the boolean parameter was parsed correctly by retrieving
+    // its value from the context.
+    bool actual_bool = false;
+    isc::data::Element::Position pos;
+    EXPECT_NO_THROW(pos = context->getParam("bool_test", actual_bool));
+    EXPECT_EQ(true, actual_bool);
+    EXPECT_EQ(1, pos.line_);
+
+    // Verify that the uint32 parameter was parsed correctly by retrieving
+    // its value from the context.
+    uint32_t actual_uint32 = 0;
+    EXPECT_NO_THROW(pos = context->getParam("uint32_test", actual_uint32));
+    EXPECT_EQ(77, actual_uint32);
+    EXPECT_EQ(2, pos.line_);
+
+    // Verify that the string parameter was parsed correctly by retrieving
+    // its value from the context.
+    std::string actual_string = "";
+    EXPECT_NO_THROW(pos = context->getParam("string_test", actual_string));
+    EXPECT_EQ("hmmm chewy", actual_string);
+    EXPECT_EQ(3, pos.line_);
+
+    // Verify that an optional parameter that is not defined, returns the
+    // zero position.
+    pos = isc::data::Element::ZERO_POSITION();
+    EXPECT_NO_THROW(pos = context->getParam("bogus_value",
+                                            actual_string, true));
+    EXPECT_EQ(pos.file_, isc::data::Element::ZERO_POSITION().file_);
+}
+
+
 } // end of anonymous namespace

src/bin/d2/tests/d_test_stubs.cc

@@ -34,7 +34,7 @@ const char* valid_d2_config = "{ "
                         "{ \"name\": \"tmark.org.\" , "
                         "  \"key_name\": \"d2_key.tmark.org\" , "
                         "  \"dns_servers\" : [ "
-                        "  { \"hostname\": \"one.tmark\" } "
+                        "  { \"ip_address\": \"127.0.0.101\" } "
                         "] } ] }, "
                         "\"reverse_ddns\" : {"
                         "\"ddns_domains\": [ "
@@ -396,7 +396,8 @@ DStubCfgMgr::createNewContext() {
 }
 
 isc::dhcp::ParserPtr
-DStubCfgMgr::createConfigParser(const std::string& element_id) {
+DStubCfgMgr::createConfigParser(const std::string& element_id,
+                                const isc::data::Element::Position& pos) {
     isc::dhcp::ParserPtr parser;
     DStubContextPtr context
         = boost::dynamic_pointer_cast<DStubContext>(getContext());
@@ -416,7 +417,8 @@ DStubCfgMgr::createConfigParser(const std::string& element_id) {
         // to "succeed" without specifically supporting them.
         if (SimFailure::shouldFailOn(SimFailure::ftElementUnknown)) {
             isc_throw(DCfgMgrBaseError,
-                      "Configuration parameter not supported: " << element_id);
+                      "Configuration parameter not supported: " << element_id
+                      << pos);
         }
 
         // Going to assume anything else is an object element.

src/bin/d2/tests/d_test_stubs.h

@@ -691,11 +691,15 @@ public:
     ///
     /// @param element_id is the string name of the element as it will appear
     /// in the configuration set.
+    /// @param pos position within the configuration text (or file) of element
+    /// to be parsed.  This is passed for error messaging.
     ///
     /// @return returns a ParserPtr to the parser instance.
     /// @throw throws DCfgMgrBaseError if SimFailure is ftElementUnknown.
     virtual isc::dhcp::ParserPtr
-    createConfigParser(const std::string& element_id);
+    createConfigParser(const std::string& element_id,
+                       const isc::data::Element::Position& pos
+                       = isc::data::Element::Position());
 
     /// @brief A list for remembering the element ids in the order they were
     /// parsed.

src/bin/d2/tests/test_data_files_config.h.in

@@ -15,3 +15,4 @@
 /// @brief Path to D2 source dir so tests against the dhcp-ddns.spec file
 /// can find it reliably.
 #define D2_SRC_DIR "@abs_top_srcdir@/src/bin/d2"
+#define D2_TEST_DATA_DIR "@abs_top_srcdir@/src/bin/d2/tests/testdata"