From: Thomas Markwalder Date: Thu, 26 Jun 2014 14:17:21 +0000 (-0400) Subject: [3436] D2 DnsServerInfo configuration errors now include position info. X-Git-Tag: trac3473_base~5^2~6 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=80997c1225c0cd669b890699be24bece088651c6;p=thirdparty%2Fkea.git [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. --- diff --git a/src/bin/d2/d2_config.cc b/src/bin/d2/d2_config.cc index 7256092b05..8a971c0eba 100644 --- a/src/bin/d2/d2_config.cc +++ b/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 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. diff --git a/src/bin/d2/d2_config.h b/src/bin/d2/d2_config.h index 28e67bd925..1b96f86d67 100644 --- a/src/bin/d2/d2_config.h +++ b/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 diff --git a/src/bin/d2/tests/d2_cfg_mgr_unittests.cc b/src/bin/d2/tests/d2_cfg_mgr_unittests.cc index a1cbcc9424..a2d0ada98a 100644 --- a/src/bin/d2/tests/d2_cfg_mgr_unittests.cc +++ b/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\" : {} " diff --git a/src/bin/d2/tests/d2_process_unittests.cc b/src/bin/d2/tests/d2_process_unittests.cc index 721159caf7..8f7b188c8e 100644 --- a/src/bin/d2/tests/d2_process_unittests.cc +++ b/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\": [ " diff --git a/src/bin/d2/tests/d_test_stubs.cc b/src/bin/d2/tests/d_test_stubs.cc index 17df507c26..ee7afc5933 100644 --- a/src/bin/d2/tests/d_test_stubs.cc +++ b/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\": [ "