[3436] D2 DnsServerInfo configuration errors now include position info.

Modified TSIGKeyInfoParser::build to validate parameters and use position
in error messages.

Parameter "hostname" now throws an "not supported yet" error.  This
can be undone if/when its implmemented.

Port must now be non-zero.

src/bin/d2/d2_config.cc

@@ -348,7 +348,8 @@ 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();
     }
@@ -415,7 +416,8 @@ TSIGKeyInfoParser::build(isc::data::ConstElementPtr key_config) {
 }
 
 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.
@@ -427,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.
@@ -510,7 +512,8 @@ 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();
     }
@@ -518,26 +521,48 @@ DnsServerInfoParser::build(isc::data::ConstElementPtr server_config) {
     std::string hostname;
     std::string ip_address;
     uint32_t port = DnsServerInfo::STANDARD_DNS_PORT;
+    std::vector<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);
+    pos.push_back(local_scalars_.getParam("hostname", hostname,
+                                          DCfgContextBase::OPTIONAL));
+    pos.push_back(local_scalars_.getParam("ip_address", ip_address,
+                                          DCfgContextBase::OPTIONAL));
+    pos.push_back(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[2]);
     }
 
     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[0]);
     } else {
         try {
             // Create an IOAddress from the IP address string given and then
@@ -545,7 +570,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[1]);
         }
     }
 
@@ -554,7 +580,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.
@@ -568,7 +596,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.

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

src/bin/d2/tests/d2_cfg_mgr_unittests.cc

@@ -773,30 +773,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.
@@ -804,11 +805,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.
@@ -836,9 +837,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.
@@ -854,9 +855,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.
@@ -874,17 +875,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));
 }
 
@@ -1204,17 +1205,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\" : {"
@@ -1222,17 +1223,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\" } "
                         "  ] } "
                         "] } }";
 
@@ -1341,7 +1342,7 @@ TEST_F(D2CfgMgrTest, forwardMatch) {
                         ", "
                         "{ \"name\": \"*\" , "
                         "  \"dns_servers\" : [ "
-                        "  { \"hostname\": \"global.net\" } "
+                        "  { \"ip_address\": \"127.0.0.3\" } "
                         "  ] } "
                         "] }, "
                         "\"reverse_ddns\" : {} "

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_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\": [ "