From: Tomek Mrugalski Date: Tue, 12 Sep 2017 22:30:32 +0000 (+0200) Subject: [5357] Shared-networks parser for v4 extended. X-Git-Tag: trac5073a_base~9^2~12 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=b9c54a65ae0d983617e0947c5c685e295b056f00;p=thirdparty%2Fkea.git [5357] Shared-networks parser for v4 extended. --- diff --git a/src/bin/dhcp4/json_config_parser.cc b/src/bin/dhcp4/json_config_parser.cc index f81dbecbd8..8756e502a8 100644 --- a/src/bin/dhcp4/json_config_parser.cc +++ b/src/bin/dhcp4/json_config_parser.cc @@ -12,8 +12,8 @@ #include #include #include -#include #include +#include #include #include #include @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -47,7 +48,12 @@ using namespace isc::hooks; namespace { -/// @brief Parser that takes care of global DHCPv4 parameters. +/// @brief Parser that takes care of global DHCPv4 parameters and utility +/// functions that work on global level. +/// +/// This class is a collection of utility method that either handle +/// global parameters (see @ref parse), or conducts operations on +/// global level (see @ref sanityChecks and @ref copySubnets4). /// /// See @ref parse method for a list of supported parameters. class Dhcp4ConfigParser : public isc::data::SimpleParser { @@ -82,6 +88,126 @@ public: uint16_t dhcp4o6_port = getUint16(global, "dhcp4o6-port"); cfg->setDhcp4o6Port(dhcp4o6_port); } + + + /// @brief Copies subnets from shared networks to regular subnets container + /// + /// @param from pointer to shared networks container (copy from here) + /// @param dest pointer to cfg subnets4 (copy to here) + /// @throw BadValue if any pointer is missing + /// @throw DhcpConfigError if there are duplicates (or other subnet defects) + void + copySubnets4(const CfgSubnets4Ptr dest, const CfgSharedNetworks4Ptr from) { + + if (!dest || !from) { + isc_throw(BadValue, "Unable to copy subnets: at least one pointer is null"); + } + + const SharedNetwork4Collection* networks = from->getAll(); + if (!networks) { + // Nothing to copy. Technically, it should return a pointer to empty + // container, but let's handle null pointer as well. + return; + } + + // Let's go through all the networks one by one + for (auto net = networks->begin(); net != networks->end(); ++net) { + + // For each network go through all the subnets in it. + const Subnet4Collection* subnets = (*net)->getAllSubnets(); + if (!subnets) { + // Shared network without subnets it weird, but we decided to + // accept such configurations. + continue; + } + + // For each subnet, add it to a list of regular subnets. + for (auto subnet = subnets->begin(); subnet != subnets->end(); ++subnet) { + dest->add(*subnet); + } + } + } + + /// @brief Conducts global sanity checks + /// + /// This method is very simply now, but more sanity checks are expected + /// in the future. + /// + /// @param cfg - the parsed structure + /// @param global global Dhcp4 scope + /// @throw DhcpConfigError in case of issues found + void + sanityChecks(SrvConfigPtr cfg, ConstElementPtr global) { + + /// Shared network sanity checks + const SharedNetwork4Collection* networks = cfg->getCfgSharedNetworks4()->getAll(); + if (networks) { + sharedNetworksSanityChecks(*networks, global->get("shared-networks")); + } + } + + /// @brief Sanity checks for shared networks + /// + /// This method verifies if there are no issues with shared networks. + /// @param networks pointer to shared networks being checked + /// @param json shared-networks element + /// @throw DhcpConfigError if issues are encountered + void + sharedNetworksSanityChecks(const SharedNetwork4Collection& networks, + ConstElementPtr json) { + + // Used for names uniqueness checks. + std::set names; + + // Let's go through all the networks one by one + for (auto net = networks.begin(); net != networks.end(); ++net) { + string txt; + + // Let's check if all subnets have either the same interface + // or don't have the interface specified at all. + string iface = (*net)->getIface(); + + const Subnet4Collection* subnets = (*net)->getAllSubnets(); + if (subnets) { + // For each subnet, add it to a list of regular subnets. + for (auto subnet = subnets->begin(); subnet != subnets->end(); ++subnet) { + if (iface.empty()) { + iface = (*subnet)->getIface(); + continue; + } + + if ((*subnet)->getIface().empty()) { + continue; + } + + if (iface != (*subnet)->getIface()) { + isc_throw(DhcpConfigError, "Subnet " << (*subnet)->toText() + << " has specified interface " << (*subnet)->getIface() + << ", but earlier subnet in the same shared-network" + << " or the shared-network itself used " << iface); + } + + // Let's collect the subnets in case we later find out the + // subnet doesn't have a mandatory name. + txt += (*subnet)->toText() + " "; + } + } + + // Next, let's check name of the shared network. + if ((*net)->getName().empty()) { + isc_throw(DhcpConfigError, "Shared-network with subnets " + << txt << " is missing mandatory 'name' parameter"); + } + + // Is it unique? + if (names.find((*net)->getName()) != names.end()) { + isc_throw(DhcpConfigError, "A shared-network with " + "name " << (*net)->getName() << " defined twice."); + } + names.insert((*net)->getName()); + + } + } }; } // anonymous namespace @@ -189,6 +315,8 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set, parser.parse(cfg_option_def, option_defs); } + Dhcp4ConfigParser global_parser; + // Make parsers grouping. const std::map& values_map = mutable_cfg->mapValue(); @@ -291,11 +419,18 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set, } if (config_pair.first == "shared-networks") { - /// @todo We need to create instance of SharedNetworks4ListParser + + /// We need to create instance of SharedNetworks4ListParser /// and parse the list of the shared networks into the /// CfgSharedNetworks4 object. One additional step is then to /// add subnets from the CfgSharedNetworks4 into CfgSubnets4 /// as well. + SharedNetworks4ListParser parser; + CfgSharedNetworks4Ptr cfg = srv_cfg->getCfgSharedNetworks4(); + + parser.parse(cfg, config_pair.second); + + global_parser.copySubnets4(srv_cfg->getCfgSubnets4(), cfg); continue; } @@ -322,9 +457,13 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set, } // Apply global options in the staging config. - Dhcp4ConfigParser global_parser; global_parser.parse(srv_cfg, mutable_cfg); + // This method conducts final sanity checks and tweaks. In particular, + // it checks that there is no conflict between plain subnets and those + // defined as part of shared networks. + global_parser.sanityChecks(srv_cfg, mutable_cfg); + } catch (const isc::Exception& ex) { LOG_ERROR(dhcp4_logger, DHCP4_PARSER_FAIL) .arg(config_pair.first).arg(ex.what()); diff --git a/src/bin/dhcp4/tests/config_parser_unittest.cc b/src/bin/dhcp4/tests/config_parser_unittest.cc index a0f224a63c..dc2079b166 100644 --- a/src/bin/dhcp4/tests/config_parser_unittest.cc +++ b/src/bin/dhcp4/tests/config_parser_unittest.cc @@ -181,6 +181,40 @@ public: EXPECT_EQ(expected_code, rcode_); } + /// @brief Convenience method for running configuration + /// + /// This method does not throw, but signals errors using gtest macros. + /// + /// @param config text to be parsed as JSON + /// @param expected_code expected code (see cc/command_interpreter.h) + /// @param exp_error expected text error (check skipped if empty) + void configure(std::string config, int expected_code, + std::string exp_error = "") { + ConstElementPtr json; + ASSERT_NO_THROW(json = parseDHCP4(config, true)); + + ConstElementPtr status; + EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json)); + ASSERT_TRUE(status); + + int rcode; + ConstElementPtr comment = parseAnswer(rcode, status); + EXPECT_EQ(expected_code, rcode); + + string text; + ASSERT_NO_THROW(text = comment->stringValue()); + + if (expected_code != rcode) { + std::cout << "Reported status: " << text << std::endl; + } + + if ((rcode != 0)) { + if (!exp_error.empty()) { + EXPECT_EQ(exp_error, text); + } + } + } + ~Dhcp4ParserTest() { resetConfiguration(); @@ -570,6 +604,18 @@ public: return (ReturnType()); } + void + checkSubnet(const Subnet4Collection& col, std::string subnet, + uint32_t t1, uint32_t t2, uint32_t valid) { + const auto& index = col.get(); + auto subnet_it = index.find(subnet); + ASSERT_NE(subnet_it, index.cend()); + Subnet4Ptr s = *subnet_it; + + EXPECT_EQ(t1, s->getT1()); + EXPECT_EQ(t2, s->getT2()); + EXPECT_EQ(valid, s->getValid()); + } /// @brief This utility method attempts to configure using specified /// config and then returns requested pool from requested subnet @@ -1023,8 +1069,6 @@ TEST_F(Dhcp4ParserTest, reconfigureRemoveSubnet) { /// CASE 2: Configure 4 subnets, then reconfigure and remove one /// from in between (not first, not last) - - /// @todo: Uncomment subnet removal test as part of #3281. ASSERT_NO_THROW(json = parseDHCP4(config4)); EXPECT_NO_THROW(x = configureDhcp4Server(*srv_, json)); checkResult(x, 0); @@ -4929,21 +4973,10 @@ TEST_F(Dhcp4ParserTest, invalidPoolRange) { " } ] \n" "} \n"; - ConstElementPtr json; - ASSERT_NO_THROW(json = parseDHCP4(config, true)); - - ConstElementPtr status; - EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json)); - ASSERT_TRUE(status); - int rcode; - ConstElementPtr comment = parseAnswer(rcode, status); - string text; - ASSERT_NO_THROW(text = comment->stringValue()); - - EXPECT_EQ(1, rcode); string expected = "Failed to create pool defined by: " "192.0.2.1-19.2.0.200 (:6:26)"; - EXPECT_EQ(expected, text); + + configure(config, CONTROL_RESULT_ERROR, expected); } // Test verifies the error message for an outside subnet pool range @@ -4959,23 +4992,269 @@ TEST_F(Dhcp4ParserTest, outsideSubnetPool) { " } ] \n" "} \n"; - ConstElementPtr json; - ASSERT_NO_THROW(json = parseDHCP4(config, true)); - - ConstElementPtr status; - EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json)); - ASSERT_TRUE(status); - int rcode; - ConstElementPtr comment = parseAnswer(rcode, status); - string text; - ASSERT_NO_THROW(text = comment->stringValue()); - - EXPECT_EQ(1, rcode); string expected = "subnet configuration failed: " "a pool of type V4, with the following address range: " "192.0.2.1-192.0.2.100 does not match the prefix of a subnet: " "10.0.2.0/24 to which it is being added (:5:14)"; - EXPECT_EQ(expected, text); + + configure(config, CONTROL_RESULT_ERROR, expected); +} + +// Test verifies that empty shared networks are accepted. +TEST_F(Dhcp4ParserTest, sharedNetworksEmpty) { + string config = "{\n" + "\"valid-lifetime\": 4000, \n" + "\"rebind-timer\": 2000, \n" + "\"renew-timer\": 1000, \n" + "\"subnet4\": [ { \n" + " \"pools\": [ { \"pool\": \"10.0.2.1 - 10.0.2.100\" } ], \n" + " \"subnet\": \"10.0.2.0/24\" \n" + " } ],\n" + "\"shared-networks\": [ ]\n" + "} \n"; + + configure(config, CONTROL_RESULT_SUCCESS, ""); +} + +// Test verifies that if a shared network is defined, it at least has to have +// a name. +TEST_F(Dhcp4ParserTest, sharedNetworksNoName) { + string config = "{\n" + "\"valid-lifetime\": 4000, \n" + "\"rebind-timer\": 2000, \n" + "\"renew-timer\": 1000, \n" + "\"subnet4\": [ { \n" + " \"pools\": [ { \"pool\": \"10.0.2.1 - 10.0.2.100\" } ], \n" + " \"subnet\": \"10.0.2.0/24\" \n" + " } ],\n" + "\"shared-networks\": [ { } ]\n" + "} \n"; + + EXPECT_THROW(parseDHCP4(config, true), Dhcp4ParseError); } +// Test verifies that empty shared networks are accepted. +TEST_F(Dhcp4ParserTest, sharedNetworksEmptyName) { + string config = "{\n" + "\"valid-lifetime\": 4000, \n" + "\"rebind-timer\": 2000, \n" + "\"renew-timer\": 1000, \n" + "\"subnet4\": [ { \n" + " \"pools\": [ { \"pool\": \"10.0.2.1 - 10.0.2.100\" } ], \n" + " \"subnet\": \"10.0.2.0/24\" \n" + " } ],\n" + "\"shared-networks\": [ { \"name\": \"\" } ]\n" + "} \n"; + + configure(config, CONTROL_RESULT_ERROR, + "Shared-network with subnets is missing mandatory 'name' parameter"); +} + +// Test verifies that a degenerated shared-network (no subnets) is +// accepted. +TEST_F(Dhcp4ParserTest, sharedNetworksName) { + string config = "{\n" + "\"subnet4\": [ { \n" + " \"pools\": [ { \"pool\": \"10.0.2.1 - 10.0.2.100\" } ], \n" + " \"subnet\": \"10.0.2.0/24\" \n" + " } ],\n" + "\"shared-networks\": [ { \"name\": \"foo\" } ]\n" + "} \n"; + + configure(config, CONTROL_RESULT_SUCCESS, ""); + + // Now verify that the shared network was indeed configured. + CfgSharedNetworks4Ptr cfg_net = CfgMgr::instance().getStagingCfg() + ->getCfgSharedNetworks4(); + + ASSERT_TRUE(cfg_net); + const SharedNetwork4Collection* nets = cfg_net->getAll(); + ASSERT_TRUE(nets); + ASSERT_EQ(1, nets->size()); + + SharedNetwork4Ptr net = *(nets->begin()); + ASSERT_TRUE(net); + + EXPECT_EQ("foo", net->getName()); + + const Subnet4Collection * subs = net->getAllSubnets(); + ASSERT_TRUE(subs); + EXPECT_EQ(0, subs->size()); +} + +// Test verifies that a degenerated shared-network (just one subnet) is +// accepted. +TEST_F(Dhcp4ParserTest, sharedNetworks1subnet) { + string config = "{\n" + "\"valid-lifetime\": 4000, \n" + "\"rebind-timer\": 2000, \n" + "\"renew-timer\": 1000, \n" + "\"shared-networks\": [ {\n" + " \"name\": \"foo\"\n," + " \"subnet4\": [ { \n" + " \"subnet\": \"192.0.2.0/24\",\n" + " \"pools\": [ { \"pool\": \"192.0.2.1-192.0.2.10\" } ]\n" + " } ]\n" + " } ]\n" + "} \n"; + + configure(config, CONTROL_RESULT_SUCCESS, ""); + + // Now verify that the shared network was indeed configured. + CfgSharedNetworks4Ptr cfg_net = CfgMgr::instance().getStagingCfg() + ->getCfgSharedNetworks4(); + ASSERT_TRUE(cfg_net); + + // There should be exactly one shared subnet. + const SharedNetwork4Collection* nets = cfg_net->getAll(); + ASSERT_TRUE(nets); + ASSERT_EQ(1, nets->size()); + + SharedNetwork4Ptr net = *(nets->begin()); + ASSERT_TRUE(net); + EXPECT_EQ("foo", net->getName()); + + // It should have one subnet. + const Subnet4Collection * subs = net->getAllSubnets(); + ASSERT_TRUE(subs); + EXPECT_EQ(1, subs->size()); + checkSubnet(*subs, "192.0.2.0/24", 1000, 2000, 4000); + + // Now make sure the subnet was added to global list of subnets. + CfgSubnets4Ptr subnets4 = CfgMgr::instance().getStagingCfg()->getCfgSubnets4(); + ASSERT_TRUE(subnets4); + + subs = subnets4->getAll(); + ASSERT_TRUE(subs); + checkSubnet(*subs, "192.0.2.0/24", 1000, 2000, 4000); +} + +// Test verifies that a a proper shared-network (three subnets) is +// accepted. It verifies several things: +// - that more than one subnet can be added to shared subnets +// - that each subnet being part of the shared subnets is also stored in +// global subnets collection +// - that a subnet can inherit global values +// - that subnet can override global parameters +// - that overridden parameters only affect one subnet and not others +TEST_F(Dhcp4ParserTest, sharedNetworks3subnets) { + string config = "{\n" + "\"valid-lifetime\": 4000, \n" + "\"rebind-timer\": 2000, \n" + "\"renew-timer\": 1000, \n" + "\"shared-networks\": [ {\n" + " \"name\": \"foo\"\n," + " \"subnet4\": [\n" + " { \n" + " \"subnet\": \"192.0.1.0/24\",\n" + " \"pools\": [ { \"pool\": \"192.0.1.1-192.0.1.10\" } ]\n" + " },\n" + " { \n" + " \"subnet\": \"192.0.2.0/24\",\n" + " \"pools\": [ { \"pool\": \"192.0.2.1-192.0.2.10\" } ],\n" + " \"renew-timer\": 2,\n" + " \"rebind-timer\": 22,\n" + " \"valid-lifetime\": 222\n" + " },\n" + " { \n" + " \"subnet\": \"192.0.3.0/24\",\n" + " \"pools\": [ { \"pool\": \"192.0.3.1-192.0.3.10\" } ]\n" + " }\n" + " ]\n" + " } ]\n" + "} \n"; + + configure(config, CONTROL_RESULT_SUCCESS, ""); + + // Now verify that the shared network was indeed configured. + CfgSharedNetworks4Ptr cfg_net = CfgMgr::instance().getStagingCfg() + ->getCfgSharedNetworks4(); + + // There is expected one shared subnet. + ASSERT_TRUE(cfg_net); + const SharedNetwork4Collection* nets = cfg_net->getAll(); + ASSERT_TRUE(nets); + ASSERT_EQ(1, nets->size()); + + SharedNetwork4Ptr net = *(nets->begin()); + ASSERT_TRUE(net); + + EXPECT_EQ("foo", net->getName()); + + const Subnet4Collection * subs = net->getAllSubnets(); + ASSERT_TRUE(subs); + EXPECT_EQ(3, subs->size()); + checkSubnet(*subs, "192.0.1.0/24", 1000, 2000, 4000); + checkSubnet(*subs, "192.0.2.0/24", 2, 22, 222); + checkSubnet(*subs, "192.0.3.0/24", 1000, 2000, 4000); + + // Now make sure the subnet was added to global list of subnets. + CfgSubnets4Ptr subnets4 = CfgMgr::instance().getStagingCfg()->getCfgSubnets4(); + ASSERT_TRUE(subnets4); + + subs = subnets4->getAll(); + ASSERT_TRUE(subs); + checkSubnet(*subs, "192.0.1.0/24", 1000, 2000, 4000); + checkSubnet(*subs, "192.0.2.0/24", 2, 22, 222); + checkSubnet(*subs, "192.0.3.0/24", 1000, 2000, 4000); +} + +// This test checks if parameters are derived properly: +// - global to shared network +// - shared network to subnet +TEST_F(Dhcp4ParserTest, sharedNetworksDerive) { + string config = "{\n" + "\"valid-lifetime\": 4, \n" + "\"rebind-timer\": 2, \n" + "\"renew-timer\": 1, \n" + "\"shared-networks\": [ {\n" + " \"name\": \"foo\"\n," + " \"renew-timer\": 10,\n" + " \"subnet4\": [\n" + " { \n" + " \"subnet\": \"192.0.1.0/24\",\n" + " \"pools\": [ { \"pool\": \"192.0.1.1-192.0.1.10\" } ]\n" + " },\n" + " { \n" + " \"subnet\": \"192.0.2.0/24\",\n" + " \"pools\": [ { \"pool\": \"192.0.2.1-192.0.2.10\" } ],\n" + " \"renew-timer\": 100\n" + " }\n" + " ]\n" + " } ]\n" + "} \n"; + + configure(config, CONTROL_RESULT_SUCCESS, ""); + + // Now verify that the shared network was indeed configured. + CfgSharedNetworks4Ptr cfg_net = CfgMgr::instance().getStagingCfg() + ->getCfgSharedNetworks4(); + + // There is expected one shared subnet. + ASSERT_TRUE(cfg_net); + const SharedNetwork4Collection* nets = cfg_net->getAll(); + ASSERT_TRUE(nets); + ASSERT_EQ(1, nets->size()); + + SharedNetwork4Ptr net = *(nets->begin()); + ASSERT_TRUE(net); + + const Subnet4Collection * subs = net->getAllSubnets(); + ASSERT_TRUE(subs); + EXPECT_EQ(2, subs->size()); + + // For the first subnet, the renew-timer should be 10, because it was + // derived from shared-network level. Other parameters a derived + // from global scope to shared-network level and later again to + // subnet4 level. + checkSubnet(*subs, "192.0.1.0/24", 10, 2, 4); + + // For the second subnet, the renew-timer should be 100, because it + // was specified explicitly. Other parameters a derived + // from global scope to shared-network level and later again to + // subnet4 level. + checkSubnet(*subs, "192.0.2.0/24", 100, 2, 4); +} + + } diff --git a/src/lib/dhcpsrv/parsers/simple_parser4.cc b/src/lib/dhcpsrv/parsers/simple_parser4.cc index e3a033ea07..b9b810c51b 100644 --- a/src/lib/dhcpsrv/parsers/simple_parser4.cc +++ b/src/lib/dhcpsrv/parsers/simple_parser4.cc @@ -133,6 +133,17 @@ size_t SimpleParser4::setAllDefaults(isc::data::ElementPtr global) { cnt += setDefaults(mutable_cfg, IFACE4_DEFAULTS); } + // Set defaults for shared networks + ConstElementPtr shared = global->get("shared-networks"); + if (shared) { + BOOST_FOREACH(ElementPtr net, shared->listValue()) { + ConstElementPtr subs = net->get("subnet4"); + if (subs) { + cnt += setListDefaults(subs, SUBNET4_DEFAULTS); + } + } + } + return (cnt); } @@ -148,6 +159,30 @@ size_t SimpleParser4::deriveParameters(isc::data::ElementPtr global) { } } + ConstElementPtr shared = global->get("shared-networks"); + if (shared) { + BOOST_FOREACH(ElementPtr net, shared->listValue()) { + + // This level derives global parameters to shared network + // level. + + // First try to inherit the parameters from shared network, + // if defined there. + // Then try to inherit them from global. + cnt += SimpleParser::deriveParams(global, net, + INHERIT_GLOBAL_TO_SUBNET4); + + // Now we need to go thrugh all the subnets in this net. + subnets = net->get("subnet4"); + if (subnets) { + BOOST_FOREACH(ElementPtr single_subnet, subnets->listValue()) { + cnt += SimpleParser::deriveParams(net, single_subnet, + INHERIT_GLOBAL_TO_SUBNET4); + } + } + } + } + return (cnt); }