From: Thomas Markwalder Date: Thu, 26 Apr 2018 17:20:39 +0000 (-0400) Subject: [5535] RelayInfo parsing handles both ip-address and ip-addresses X-Git-Tag: trac5549_base~1^2~5 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=0e5e017d4bf56ccc623723908e4b43e10a9ce63c;p=thirdparty%2Fkea.git [5535] RelayInfo parsing handles both ip-address and ip-addresses src/lib/dhcpsrv/parsers/dhcp_parsers.* RelayInfoParser::parse() - reworked to support either ip-address or ip-addresses RelayInfoParser::addAddress() - new parser helper method src/lib/dhcpsrv/parsers/shared_network_parser.cc SharedNetwork4Parser::parse() SharedNetwork6Parser::parse() - both now parse "relay" element (was missing) src/lib/dhcpsrv/tests/shared_network_parser_unittest.cc Modified to support testing "relay" element parsing Added new tests: TEST_F(SharedNetwork4ParserTest, relayInfoTests) TEST_F(SharedNetwork6ParserTest, relayInfoTests) --- diff --git a/src/lib/dhcpsrv/parsers/dhcp_parsers.cc b/src/lib/dhcpsrv/parsers/dhcp_parsers.cc index 961a5334a5..168137f115 100644 --- a/src/lib/dhcpsrv/parsers/dhcp_parsers.cc +++ b/src/lib/dhcpsrv/parsers/dhcp_parsers.cc @@ -233,24 +233,79 @@ RelayInfoParser::RelayInfoParser(const Option::Universe& family) }; void -RelayInfoParser::parse(const isc::dhcp::Network::RelayInfoPtr& cfg, - ConstElementPtr relay_info) { - // There is only one parameter which is mandatory - IOAddress ip = getAddress(relay_info, "ip-address"); +RelayInfoParser::parse(const isc::dhcp::Network::RelayInfoPtr& relay_info, + ConstElementPtr relay_elem) { + + if (relay_elem->getType() != Element::map) { + isc_throw(DhcpConfigError, "relay must be a map"); + } + + ConstElementPtr address = relay_elem->get("ip-address"); + ConstElementPtr addresses = relay_elem->get("ip-addresses"); + + if (address && addresses) { + isc_throw(DhcpConfigError, + "specify either ip-address or ip-addresses, not both"); + } + + if (!address && !addresses) { + isc_throw(DhcpConfigError, "ip-addresses is required"); + } + + // Create our resultant RelayInfo structure + *relay_info = isc::dhcp::Network::RelayInfo(); + + if (address) { + // log a deprec debug message ? + addAddress("ip-address", getString(relay_elem, "ip-address"), + relay_elem, relay_info); + return; + } + + if (addresses->getType() != Element::list) { + isc_throw(DhcpConfigError, "ip-addresses must be a list " + << " (" << getPosition("ip-addresses", relay_elem) << ")"); + } + + BOOST_FOREACH(ConstElementPtr address_element, addresses->listValue()) { + addAddress("ip-addresses", address_element->stringValue(), + relay_elem, relay_info); + } +} + +void +RelayInfoParser::addAddress(const std::string& name, + const std::string& address_str, + ConstElementPtr relay_elem, + const isc::dhcp::Network::RelayInfoPtr& relay_info) { + boost::scoped_ptr ip; + try { + ip.reset(new isc::asiolink::IOAddress(address_str)); + } catch (const std::exception& ex) { + isc_throw(DhcpConfigError, "address " << address_str + << " is not a valid: " + << (family_ == Option::V4 ? "IPv4" : "IPv6") + << "address" + << " (" << getPosition(name, relay_elem) << ")"); + } // Check if the address family matches. - if ((ip.isV4() && family_ != Option::V4) || - (ip.isV6() && family_ != Option::V6) ) { - isc_throw(DhcpConfigError, "ip-address field " << ip.toText() - << " does not have IP address of expected family type: " + if ((ip->isV4() && family_ != Option::V4) || + (ip->isV6() && family_ != Option::V6) ) { + isc_throw(DhcpConfigError, "address " << address_str + << " is not a: " << (family_ == Option::V4 ? "IPv4" : "IPv6") - << " (" << getPosition("ip-address", relay_info) << ")"); + << "address" + << " (" << getPosition(name, relay_elem) << ")"); } - // Ok, we're done with parsing. Let's store the result in the structure - // we were given as configuration storage. - *cfg = isc::dhcp::Network::RelayInfo(); - cfg->addAddress(ip); + try { + relay_info->addAddress(*ip); + } catch (const std::exception& ex) { + isc_throw(DhcpConfigError, "cannot add address: " << address_str + << "to relay info: " << ex.what() + << " (" << getPosition(name, relay_elem) << ")"); + } } //****************************** PoolParser ******************************** @@ -698,7 +753,7 @@ Subnet4ConfigParser::initSubnet(data::ConstElementPtr params, try { std::string hr_mode = getString(params, "reservation-mode"); subnet4->setHostReservationMode(hrModeFromText(hr_mode)); - } catch (const BadValue& ex) { + } catch (const BadValue& ex) { isc_throw(DhcpConfigError, "Failed to process specified value " " of reservation-mode parameter: " << ex.what() << "(" << getPosition("reservation-mode", params) << ")"); @@ -879,7 +934,7 @@ PdPoolParser::parse(PoolStoragePtr pools, ConstElementPtr pd_pool_) { OptionDataListParser opts_parser(AF_INET6); opts_parser.parse(options_, option_data); } - + ConstElementPtr user_context = pd_pool_->get("user-context"); if (user_context) { user_context_ = user_context; @@ -921,7 +976,7 @@ PdPoolParser::parse(PoolStoragePtr pools, ConstElementPtr pd_pool_) { pool_->allowClientClass(cclass); } } - + if (class_list) { const std::vector& classes = class_list->listValue(); for (auto cclass = classes.cbegin(); @@ -1096,7 +1151,7 @@ Subnet6ConfigParser::initSubnet(data::ConstElementPtr params, try { std::string hr_mode = getString(params, "reservation-mode"); subnet6->setHostReservationMode(hrModeFromText(hr_mode)); - } catch (const BadValue& ex) { + } catch (const BadValue& ex) { isc_throw(DhcpConfigError, "Failed to process specified value " " of reservation-mode parameter: " << ex.what() << "(" << getPosition("reservation-mode", params) << ")"); @@ -1213,9 +1268,9 @@ D2ClientConfigParser::parse(isc::data::ConstElementPtr client_config) { std::string sender_ip_str = getString(client_config, "sender-ip"); - uint32_t sender_port = getUint32(client_config, "sender-port"); + uint32_t sender_port = getUint32(client_config, "sender-port"); - uint32_t max_queue_size = getUint32(client_config, "max-queue-size"); + uint32_t max_queue_size = getUint32(client_config, "max-queue-size"); dhcp_ddns::NameChangeProtocol ncr_protocol = getProtocol(client_config, "ncr-protocol"); @@ -1244,7 +1299,7 @@ D2ClientConfigParser::parse(isc::data::ConstElementPtr client_config) { if (client_config->contains("qualifying-suffix")) { qualifying_suffix = getString(client_config, "qualifying-suffix"); found_qualifying_suffix = true; - } + } IOAddress sender_ip(0); if (sender_ip_str.empty()) { diff --git a/src/lib/dhcpsrv/parsers/dhcp_parsers.h b/src/lib/dhcpsrv/parsers/dhcp_parsers.h index abf1e57a02..b323eb4b87 100644 --- a/src/lib/dhcpsrv/parsers/dhcp_parsers.h +++ b/src/lib/dhcpsrv/parsers/dhcp_parsers.h @@ -397,12 +397,32 @@ public: /// /// The elements currently supported are: /// -# ip-address - /// - /// @param cfg configuration will be stored here - /// @param relay_info JSON structure holding relay parameters to parse - void parse(const isc::dhcp::Network::RelayInfoPtr& cfg, - isc::data::ConstElementPtr relay_info); - + /// -# ip-addresses + /// + /// Note that ip-address and ip-addresses are mutually exclusive, with + /// former being deprecated. The use of ip-address will cause an debug + /// log to be emitted, reminded users to switch. + /// + /// @param relay_info configuration will be stored here + /// @param relay_elem Element tree containing the relay and its members + /// @throw isc::dhcp::DhcpConfigError if both or neither of ip-address + /// and ip-addresses are specified. + void parse(const isc::dhcp::Network::RelayInfoPtr& relay_info, + isc::data::ConstElementPtr relay_elem); + + /// @brief Attempts to add an IP address to list of relay addresses + /// + /// @param name name of the element supplying the address string, (either + /// "ip-address" or "ip-addresses") + /// @param address string form of the IP address to add + /// @param relay_elem parent relay element (needed for position info) + /// @param relay_info RelayInfo to which the address should be added + /// @throw isc::dhcp::DhcpConfigError if the address string is not a valid + /// IP address, is an address of the wrong family, or is already in the + /// relay address list + void addAddress(const std::string& name, const std::string& address_str, + isc::data::ConstElementPtr relay_elem, + const isc::dhcp::Network::RelayInfoPtr& relay_info); private: /// Protocol family (IPv4 or IPv6) diff --git a/src/lib/dhcpsrv/parsers/shared_network_parser.cc b/src/lib/dhcpsrv/parsers/shared_network_parser.cc index ad395683b2..693b0f271a 100644 --- a/src/lib/dhcpsrv/parsers/shared_network_parser.cc +++ b/src/lib/dhcpsrv/parsers/shared_network_parser.cc @@ -89,6 +89,15 @@ SharedNetwork4Parser::parse(const data::ConstElementPtr& shared_network_data) { } } + if (shared_network_data->contains("relay")) { + auto relay_parms = shared_network_data->get("relay"); + if (relay_parms) { + RelayInfoParser parser(Option::V4); + Network::RelayInfoPtr relay_info(new Network::RelayInfo()); + parser.parse(relay_info, relay_parms); + shared_network->setRelayInfo(*relay_info); + } + } } catch (const DhcpConfigError&) { // Position was already added throw; @@ -164,6 +173,15 @@ SharedNetwork6Parser::parse(const data::ConstElementPtr& shared_network_data) { } } + if (shared_network_data->contains("relay")) { + auto relay_parms = shared_network_data->get("relay"); + if (relay_parms) { + RelayInfoParser parser(Option::V6); + Network::RelayInfoPtr relay_info(new Network::RelayInfo()); + parser.parse(relay_info, relay_parms); + shared_network->setRelayInfo(*relay_info); + } + } } catch (const std::exception& ex) { isc_throw(DhcpConfigError, ex.what() << " (" << shared_network_data->getPosition() << ")"); diff --git a/src/lib/dhcpsrv/tests/shared_network_parser_unittest.cc b/src/lib/dhcpsrv/tests/shared_network_parser_unittest.cc index d54ffe8b63..7fbe2da39f 100644 --- a/src/lib/dhcpsrv/tests/shared_network_parser_unittest.cc +++ b/src/lib/dhcpsrv/tests/shared_network_parser_unittest.cc @@ -21,15 +21,97 @@ using namespace isc::data; using namespace isc::dhcp; namespace { +class SharedNetworkParserTest : public ::testing::Test { +public: + + /// @brief Structure for describing a single relay test scenario + struct RelayTest { + /// @brief Description of the test scenario, used for logging + std::string description_; + /// @brief JSON configuration body of the "relay" element + std::string json_content_; + /// @brief indicates if parsing should pass or fail + bool should_parse_; + /// @brief list of addresses expected after parsing + IOAddressList addresses_; + }; + + /// @brief virtual destructor + virtual ~SharedNetworkParserTest(){}; + + /// @brief Fetch valid shared network configuration JSON text + virtual std::string getWorkingConfig() const = 0; + ElementPtr makeTestConfig(const std::string& name, const std::string& json_content) { + // Create working config element tree + ElementPtr config = Element::fromJSON(getWorkingConfig()); + + // Create test element contents + ElementPtr content = Element::fromJSON(json_content); + + // Add the test element to working config + config->set(name, content); + return (config); + } + + /// @brief Executes a single "relay" parsing scenario + /// + /// Each test pass consists of the following steps: + /// -# Attempt to parse the given JSON text + /// -# If parsing is expected to fail and it does return otherwise fatal fail + /// -# If parsing is expected to succeed, fatal fail if it does not + /// -# Verify the network's relay address list matches the expected list + /// in size and content. + /// + /// @param test RelayTest which describes the test to conduct + void relayTest(const RelayTest& test) { + ElementPtr test_config; + ASSERT_NO_THROW(test_config = + makeTestConfig("relay", test.json_content_)); + + // Init our ref to a place holder + Network4 dummy; + Network& network = dummy; + + // If parsing should fail, call parse expecting a throw. + if (!test.should_parse_) { + ASSERT_THROW(network = parseIntoNetwork(test_config), DhcpConfigError); + // No throw so test outcome is correct, nothing else to do. + return; + } + + // Should parse without error, let's see if it does. + ASSERT_NO_THROW(network = parseIntoNetwork(test_config)); + + // It parsed, are the number of entries correct? + ASSERT_EQ(test.addresses_.size(), network.getRelayAddresses().size()); + + // Are the expected addresses in the list? + for (auto exp_address = test.addresses_.begin(); exp_address != test.addresses_.end(); + ++exp_address) { + EXPECT_TRUE(network.hasRelayAddress(*exp_address)) + << " expected address: " << (*exp_address).toText() << " not found" ; + } + } + + /// @brief Attempts to parse the given configuration into a shared network + /// + /// Virtual function used by relayTest() to parse a test configuration. + /// Implementation should not catch parsing exceptions. + /// + /// @param test_config JSON configuration text to parse + /// @return A reference to the Network created if parsing is successful + virtual Network& parseIntoNetwork(ConstElementPtr test_config) = 0; +}; + /// @brief Test fixture class for SharedNetwork4Parser class. -class SharedNetwork4ParserTest : public ::testing::Test { +class SharedNetwork4ParserTest : public SharedNetworkParserTest { public: /// @brief Creates valid shared network configuration. /// /// @return Valid shared network configuration. - std::string getWorkingConfig() const { + virtual std::string getWorkingConfig() const { std::string config = "{" " \"user-context\": { \"comment\": \"example\" }," " \"name\": \"bird\"," @@ -88,6 +170,16 @@ public: return (config); } + + virtual Network& parseIntoNetwork(ConstElementPtr test_config) { + // Parse configuration. + SharedNetwork4Parser parser; + network_ = parser.parse(test_config); + return (*network_); + } + +private: + SharedNetwork4Ptr network_; }; // This test verifies that shared network parser for IPv4 works properly @@ -152,8 +244,6 @@ TEST_F(SharedNetwork4ParserTest, missingName) { ASSERT_THROW(network = parser.parse(config_element), DhcpConfigError); } -// This test verifies that it's possible to specify client-class -// and match-client-id on shared-network level. TEST_F(SharedNetwork4ParserTest, clientClassMatchClientId) { std::string config = getWorkingConfig(); ElementPtr config_element = Element::fromJSON(config); @@ -172,14 +262,96 @@ TEST_F(SharedNetwork4ParserTest, clientClassMatchClientId) { EXPECT_FALSE(network->getMatchClientId()); } +// This test verifies that parsing of the "relay" element. +// It checks both valid and invalid scenarios. +TEST_F(SharedNetwork4ParserTest, relayInfoTests) { + + // Create the vector of test scenarios. + std::vector tests = { + { + "valid ip-address #1", + "{ \"ip-address\": \"192.168.2.1\" }", + true, + { asiolink::IOAddress("192.168.2.1") } + }, + { + "invalid ip-address #1", + "{ \"ip-address\": \"not an address\" }", + false, + { } + }, + { + "invalid ip-address #2", + "{ \"ip-address\": \"2001:db8::1\" }", + false, + { } + }, + { + "valid ip-addresses #1", + "{ \"ip-addresses\": [ ] }", + true, + {} + }, + { + "valid ip-addresses #2", + "{ \"ip-addresses\": [ \"192.168.2.1\" ] }", + true, + { asiolink::IOAddress("192.168.2.1") } + }, + { + "valid ip-addresses #3", + "{ \"ip-addresses\": [ \"192.168.2.1\", \"192.168.2.2\" ] }", + true, + { asiolink::IOAddress("192.168.2.1"), asiolink::IOAddress("192.168.2.2") } + }, + { + "invalid ip-addresses #1", + "{ \"ip-addresses\": [ \"not an address\" ] }", + false, + { } + }, + { + "invalid ip-addresses #2", + "{ \"ip-addresses\": [ \"2001:db8::1\" ] }", + false, + { } + }, + { + "invalid both ip-address and ip-addresses", + "{" + " \"ip-address\": \"192.168.2.1\", " + " \"ip-addresses\": [ \"192.168.2.1\", \"192.168.2.2\" ]" + " }", + false, + { } + }, + { + "invalid neither ip-address nor ip-addresses", + "{}", + false, + { } + } + }; + + // Iterate over the test scenarios, verifying each prescribed + // outcome. + for (auto test = tests.begin(); test != tests.end(); ++test) { + { + SCOPED_TRACE((*test).description_); + relayTest(*test); + } + } +} + + /// @brief Test fixture class for SharedNetwork6Parser class. -class SharedNetwork6ParserTest : public ::testing::Test { +class SharedNetwork6ParserTest : public SharedNetworkParserTest { public: /// @brief Creates valid shared network configuration. /// /// @return Valid shared network configuration. - std::string getWorkingConfig() const { + virtual std::string getWorkingConfig() const { std::string config = "{" " \"name\": \"bird\"," " \"interface\": \"eth1\"," @@ -228,6 +400,16 @@ public: return (config); } + + virtual Network& parseIntoNetwork(ConstElementPtr test_config) { + // Parse configuration. + SharedNetwork6Parser parser; + network_ = parser.parse(test_config); + return (*network_); + } + +private: + SharedNetwork6Ptr network_; }; // This test verifies that shared network parser for IPv4 works properly @@ -346,4 +528,85 @@ TEST_F(SharedNetwork6ParserTest, badEvalClientClasses) { EXPECT_THROW(network = parser.parse(config_element), DhcpConfigError); } +// This test verifies that v6 parsing of the "relay" element. +// It checks both valid and invalid scenarios. +TEST_F(SharedNetwork6ParserTest, relayInfoTests) { + + // Create the vector of test scenarios. + std::vector tests = { + { + "valid ip-address #1", + "{ \"ip-address\": \"2001:db8::1\" }", + true, + { asiolink::IOAddress("2001:db8::1") } + }, + { + "invalid ip-address #1", + "{ \"ip-address\": \"not an address\" }", + false, + { } + }, + { + "invalid ip-address #2", + "{ \"ip-address\": \"192.168.2.1\" }", + false, + { } + }, + { + "valid ip-addresses #1", + "{ \"ip-addresses\": [ ] }", + true, + {} + }, + { + "valid ip-addresses #2", + "{ \"ip-addresses\": [ \"2001:db8::1\" ] }", + true, + { asiolink::IOAddress("2001:db8::1") } + }, + { + "valid ip-addresses #3", + "{ \"ip-addresses\": [ \"2001:db8::1\", \"2001:db8::2\" ] }", + true, + { asiolink::IOAddress("2001:db8::1"), asiolink::IOAddress("2001:db8::2") } + }, + { + "invalid ip-addresses #1", + "{ \"ip-addresses\": [ \"not an address\" ] }", + false, + { } + }, + { + "invalid ip-addresses #2", + "{ \"ip-addresses\": [ \"192.168.1.1\" ] }", + false, + { } + }, + { + "invalid both ip-address and ip-addresses", + "{" + " \"ip-address\": \"2001:db8::1\", " + " \"ip-addresses\": [ \"2001:db8::1\", \"2001:db8::2\" ]" + " }", + false, + { } + }, + { + "invalid neither ip-address nor ip-addresses", + "{}", + false, + { } + } + }; + + // Iterate over the test scenarios, verifying each prescribed + // outcome. + for (auto test = tests.begin(); test != tests.end(); ++test) { + { + SCOPED_TRACE((*test).description_); + relayTest(*test); + } + } +} + } // end of anonymous namespace