From: Tomek Mrugalski Date: Wed, 8 Jan 2014 14:27:44 +0000 (+0100) Subject: [3234] Changes after review: X-Git-Tag: bind10-1.2.0beta1-release~139^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=31e416087685a6dadc3047fdbb0927bbf60095aa;p=thirdparty%2Fkea.git [3234] Changes after review: - ChangeLog updated - Current reconfiguration limitations described in User's Guide - Dhcp{4,6}ParserTest.reconfigureRemoveSubnet added - Subnet::getNextID -> generateNextID - Various comments improved --- diff --git a/ChangeLog b/ChangeLog index f9e71d5a11..c27d1aee76 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,5 @@ 7XX. [bug] tomek - b10-dhcp4,b10-dhcp6: Both servers used to unnecessarily increase + b10-dhcp4, b10-dhcp6: Both servers used to unnecessarily increase subnet-id values after reconfiguration. The subnet-ids are now reset to 1 every time a server is reconfigured. (Trac #3234, git abcd) diff --git a/doc/guide/bind10-guide.xml b/doc/guide/bind10-guide.xml index d915e279cc..13d017f94d 100644 --- a/doc/guide/bind10-guide.xml +++ b/doc/guide/bind10-guide.xml @@ -4476,6 +4476,21 @@ Dhcp4/subnet4 [] list (default) development and should be treated as not implemented yet, rather than actual limitations. + + + On-line configuration has some limitations. Adding new subnets or + modifying existing ones work, as is removing the last subnet from + the list. However, removing non-last (e.g. removing subnet 1,2 or 3 if + there are 4 subnets configured) will cause issues. The problem is + caused by simplistic subnet-id assignment. The subnets are always + numbered, starting from 1. That subnet-id is then used in leases + that are stored in the lease database. Removing non-last subnet will + cause the configuration information to mismatch data in the lease + database. It is possible to manually update subnet-id fields in + MySQL database, but it is awkward and error prone process. A better + reconfiguration support is planned. + + On startup, the DHCPv4 server does not get the full configuration from @@ -5389,6 +5404,22 @@ should include options from the isc option space: yet, rather than actual limitations. + + + On-line configuration has some limitations. Adding new subnets or + modifying existing ones work, as is removing the last subnet from + the list. However, removing non-last (e.g. removing subnet 1,2 or 3 if + there are 4 subnets configured) will cause issues. The problem is + caused by simplistic subnet-id assignment. The subnets are always + numbered, starting from 1. That subnet-id is then used in leases + that are stored in the lease database. Removing non-last subnet will + cause the configuration information to mismatch data in the lease + database. It is possible to manually update subnet-id fields in + MySQL database, but it is awkward and error prone process. A better + reconfiguration support is planned. + + + On startup, the DHCPv6 server does not get the full configuration from diff --git a/src/bin/dhcp4/tests/config_parser_unittest.cc b/src/bin/dhcp4/tests/config_parser_unittest.cc index e7a034df7f..433a612916 100644 --- a/src/bin/dhcp4/tests/config_parser_unittest.cc +++ b/src/bin/dhcp4/tests/config_parser_unittest.cc @@ -475,6 +475,129 @@ TEST_F(Dhcp4ParserTest, multipleSubnets) { } while (++cnt < 10); } +// Goal of this test is to verify that a previously configured subnet can be +// deleted in subsequent reconfiguration. +TEST_F(Dhcp4ParserTest, reconfigureRemoveSubnet) { + ConstElementPtr x; + + // All four subnets + string config4 = "{ \"interfaces\": [ \"*\" ]," + "\"rebind-timer\": 2000, " + "\"renew-timer\": 1000, " + "\"subnet4\": [ { " + " \"pool\": [ \"192.0.2.1 - 192.0.2.100\" ]," + " \"subnet\": \"192.0.2.0/24\" " + " }," + " {" + " \"pool\": [ \"192.0.3.101 - 192.0.3.150\" ]," + " \"subnet\": \"192.0.3.0/24\" " + " }," + " {" + " \"pool\": [ \"192.0.4.101 - 192.0.4.150\" ]," + " \"subnet\": \"192.0.4.0/24\" " + " }," + " {" + " \"pool\": [ \"192.0.5.101 - 192.0.5.150\" ]," + " \"subnet\": \"192.0.5.0/24\" " + " } ]," + "\"valid-lifetime\": 4000 }"; + + // Three subnets (the last one removed) + string config_first3 = "{ \"interfaces\": [ \"*\" ]," + "\"rebind-timer\": 2000, " + "\"renew-timer\": 1000, " + "\"subnet4\": [ { " + " \"pool\": [ \"192.0.2.1 - 192.0.2.100\" ]," + " \"subnet\": \"192.0.2.0/24\" " + " }," + " {" + " \"pool\": [ \"192.0.3.101 - 192.0.3.150\" ]," + " \"subnet\": \"192.0.3.0/24\" " + " }," + " {" + " \"pool\": [ \"192.0.4.101 - 192.0.4.150\" ]," + " \"subnet\": \"192.0.4.0/24\" " + " } ]," + "\"valid-lifetime\": 4000 }"; + + // Second subnet removed + string config_second_removed = "{ \"interfaces\": [ \"*\" ]," + "\"rebind-timer\": 2000, " + "\"renew-timer\": 1000, " + "\"subnet4\": [ { " + " \"pool\": [ \"192.0.2.1 - 192.0.2.100\" ]," + " \"subnet\": \"192.0.2.0/24\" " + " }," + " {" + " \"pool\": [ \"192.0.4.101 - 192.0.4.150\" ]," + " \"subnet\": \"192.0.4.0/24\" " + " }," + " {" + " \"pool\": [ \"192.0.5.101 - 192.0.5.150\" ]," + " \"subnet\": \"192.0.5.0/24\" " + " } ]," + "\"valid-lifetime\": 4000 }"; + + // CASE 1: Configure 4 subnets, then reconfigure and remove the + // last one. + + ElementPtr json = Element::fromJSON(config4); + EXPECT_NO_THROW(x = configureDhcp4Server(*srv_, json)); + ASSERT_TRUE(x); + comment_ = parseAnswer(rcode_, x); + ASSERT_EQ(0, rcode_); + + const Subnet4Collection* subnets = CfgMgr::instance().getSubnets4(); + ASSERT_TRUE(subnets); + ASSERT_EQ(4, subnets->size()); // We expect 4 subnets + + // Do the reconfiguration (the last subnet is removed) + json = Element::fromJSON(config_first3); + EXPECT_NO_THROW(x = configureDhcp4Server(*srv_, json)); + ASSERT_TRUE(x); + comment_ = parseAnswer(rcode_, x); + ASSERT_EQ(0, rcode_); + + subnets = CfgMgr::instance().getSubnets4(); + ASSERT_TRUE(subnets); + ASSERT_EQ(3, subnets->size()); // We expect 3 subnets now (4th is removed) + + // Check subnet-ids of each subnet (it should be monotonously increasing) + EXPECT_EQ(1, subnets->at(0)->getID()); + EXPECT_EQ(2, subnets->at(1)->getID()); + EXPECT_EQ(3, subnets->at(2)->getID()); + + /// CASE 2: Configure 4 subnets, then reconfigure and remove one + /// from in between (not first, not last) + +#if 0 + /// @todo: Uncomment subnet removal test as part of #3281. + json = Element::fromJSON(config4); + EXPECT_NO_THROW(x = configureDhcp4Server(*srv_, json)); + ASSERT_TRUE(x); + comment_ = parseAnswer(rcode_, x); + ASSERT_EQ(0, rcode_); + + // Do reconfiguration + json = Element::fromJSON(config_second_removed); + EXPECT_NO_THROW(x = configureDhcp4Server(*srv_, json)); + ASSERT_TRUE(x); + comment_ = parseAnswer(rcode_, x); + ASSERT_EQ(0, rcode_); + + subnets = CfgMgr::instance().getSubnets4(); + ASSERT_TRUE(subnets); + ASSERT_EQ(3, subnets->size()); // We expect 4 subnets + + EXPECT_EQ(1, subnets->at(0)->getID()); + // The second subnet (with subnet-id = 2) is no longer there + EXPECT_EQ(3, subnets->at(1)->getID()); + EXPECT_EQ(4, subnets->at(2)->getID()); +#endif + +} + +/// @todo: implement subnet removal test as part of #3281. // Checks if the next-server defined as global parameter is taken into // consideration. diff --git a/src/bin/dhcp6/tests/config_parser_unittest.cc b/src/bin/dhcp6/tests/config_parser_unittest.cc index b613095d28..84dd5b7e0c 100644 --- a/src/bin/dhcp6/tests/config_parser_unittest.cc +++ b/src/bin/dhcp6/tests/config_parser_unittest.cc @@ -495,6 +495,131 @@ TEST_F(Dhcp6ParserTest, multipleSubnets) { } while (++cnt < 10); } +// Goal of this test is to verify that a previously configured subnet can be +// deleted in subsequent reconfiguration. +TEST_F(Dhcp6ParserTest, reconfigureRemoveSubnet) { + ConstElementPtr x; + + // All four subnets + string config4 = "{ \"interfaces\": [ \"*\" ]," + "\"preferred-lifetime\": 3000," + "\"rebind-timer\": 2000, " + "\"renew-timer\": 1000, " + "\"subnet6\": [ { " + " \"pool\": [ \"2001:db8:1::/80\" ]," + " \"subnet\": \"2001:db8:1::/64\" " + " }," + " {" + " \"pool\": [ \"2001:db8:2::/80\" ]," + " \"subnet\": \"2001:db8:2::/64\" " + " }," + " {" + " \"pool\": [ \"2001:db8:3::/80\" ]," + " \"subnet\": \"2001:db8:3::/64\" " + " }," + " {" + " \"pool\": [ \"2001:db8:4::/80\" ]," + " \"subnet\": \"2001:db8:4::/64\" " + " } ]," + "\"valid-lifetime\": 4000 }"; + + // Three subnets (the last one removed) + string config_first3 = "{ \"interfaces\": [ \"*\" ]," + "\"preferred-lifetime\": 3000," + "\"rebind-timer\": 2000, " + "\"renew-timer\": 1000, " + "\"subnet6\": [ { " + " \"pool\": [ \"2001:db8:1::/80\" ]," + " \"subnet\": \"2001:db8:1::/64\" " + " }," + " {" + " \"pool\": [ \"2001:db8:2::/80\" ]," + " \"subnet\": \"2001:db8:2::/64\" " + " }," + " {" + " \"pool\": [ \"2001:db8:3::/80\" ]," + " \"subnet\": \"2001:db8:3::/64\" " + " } ]," + "\"valid-lifetime\": 4000 }"; + + // Second subnet removed + string config_second_removed = "{ \"interfaces\": [ \"*\" ]," + "\"preferred-lifetime\": 3000," + "\"rebind-timer\": 2000, " + "\"renew-timer\": 1000, " + "\"subnet6\": [ { " + " \"pool\": [ \"2001:db8:1::/80\" ]," + " \"subnet\": \"2001:db8:1::/64\" " + " }," + " {" + " \"pool\": [ \"2001:db8:3::/80\" ]," + " \"subnet\": \"2001:db8:3::/64\" " + " }," + " {" + " \"pool\": [ \"2001:db8:4::/80\" ]," + " \"subnet\": \"2001:db8:4::/64\" " + " } ]," + "\"valid-lifetime\": 4000 }"; + + // CASE 1: Configure 4 subnets, then reconfigure and remove the + // last one. + + ElementPtr json = Element::fromJSON(config4); + EXPECT_NO_THROW(x = configureDhcp6Server(srv_, json)); + ASSERT_TRUE(x); + comment_ = parseAnswer(rcode_, x); + ASSERT_EQ(0, rcode_); + + const Subnet6Collection* subnets = CfgMgr::instance().getSubnets6(); + ASSERT_TRUE(subnets); + ASSERT_EQ(4, subnets->size()); // We expect 4 subnets + + // Do the reconfiguration (the last subnet is removed) + json = Element::fromJSON(config_first3); + EXPECT_NO_THROW(x = configureDhcp6Server(srv_, json)); + ASSERT_TRUE(x); + comment_ = parseAnswer(rcode_, x); + ASSERT_EQ(0, rcode_); + + subnets = CfgMgr::instance().getSubnets6(); + ASSERT_TRUE(subnets); + ASSERT_EQ(3, subnets->size()); // We expect 3 subnets now (4th is removed) + + EXPECT_EQ(1, subnets->at(0)->getID()); + EXPECT_EQ(2, subnets->at(1)->getID()); + EXPECT_EQ(3, subnets->at(2)->getID()); + + /// CASE 2: Configure 4 subnets, then reconfigure and remove one + /// from in between (not first, not last) + +#if 0 + /// @todo: Uncomment subnet removal test as part of #3281. + json = Element::fromJSON(config4); + EXPECT_NO_THROW(x = configureDhcp6Server(srv_, json)); + ASSERT_TRUE(x); + comment_ = parseAnswer(rcode_, x); + ASSERT_EQ(0, rcode_); + + // Do reconfiguration + json = Element::fromJSON(config_second_removed); + EXPECT_NO_THROW(x = configureDhcp6Server(srv_, json)); + ASSERT_TRUE(x); + comment_ = parseAnswer(rcode_, x); + ASSERT_EQ(0, rcode_); + + subnets = CfgMgr::instance().getSubnets6(); + ASSERT_TRUE(subnets); + ASSERT_EQ(3, subnets->size()); // We expect 4 subnets + + EXPECT_EQ(1, subnets->at(0)->getID()); + // The second subnet (with subnet-id = 2) is no longer there + EXPECT_EQ(3, subnets->at(1)->getID()); + EXPECT_EQ(4, subnets->at(2)->getID()); +#endif +} + + + // This test checks if it is possible to override global values // on a per subnet basis. TEST_F(Dhcp6ParserTest, subnetLocal) { diff --git a/src/lib/dhcpsrv/subnet.cc b/src/lib/dhcpsrv/subnet.cc index a07442e5a6..56ed105fcb 100644 --- a/src/lib/dhcpsrv/subnet.cc +++ b/src/lib/dhcpsrv/subnet.cc @@ -31,7 +31,7 @@ Subnet::Subnet(const isc::asiolink::IOAddress& prefix, uint8_t len, const Triplet& t1, const Triplet& t2, const Triplet& valid_lifetime) - :id_(getNextID()), prefix_(prefix), prefix_len_(len), t1_(t1), + :id_(generateNextID()), prefix_(prefix), prefix_len_(len), t1_(t1), t2_(t2), valid_(valid_lifetime), last_allocated_ia_(lastAddrInPrefix(prefix, len)), last_allocated_ta_(lastAddrInPrefix(prefix, len)), diff --git a/src/lib/dhcpsrv/subnet.h b/src/lib/dhcpsrv/subnet.h index ec8c397246..ecac6c3af8 100644 --- a/src/lib/dhcpsrv/subnet.h +++ b/src/lib/dhcpsrv/subnet.h @@ -387,7 +387,12 @@ protected: /// @brief Protected constructor // /// By making the constructor protected, we make sure that noone will - /// ever instantiate that class. Pool4 and Pool6 should be used instead. + /// ever instantiate that class. Subnet4 and Subnet6 should be used instead. + /// + /// This constructor assigns a new subnet-id (see @ref generateNextID). + /// This subnet-id has unique value that is strictly monotonously increasing + /// for each subnet, until it is explicitly reset back to 1 during + /// reconfiguration process. Subnet(const isc::asiolink::IOAddress& prefix, uint8_t len, const Triplet& t1, const Triplet& t2, @@ -409,8 +414,13 @@ protected: /// @brief returns the next unique Subnet-ID /// + /// This method generates and returns the next unique subnet-id. + /// It is a strictly monotonously increasing value (1,2,3,...) for + /// each new Subnet object created. It can be explicitly reset + /// back to 1 during reconfiguration (@ref resetSubnetID). + /// /// @return the next unique Subnet-ID - static SubnetID getNextID() { + static SubnetID generateNextID() { return (static_id_++); } @@ -511,6 +521,8 @@ public: /// @brief Constructor with all parameters /// + /// This constructor calls Subnet::Subnet, where subnet-id is generated. + /// /// @param prefix Subnet4 prefix /// @param length prefix length /// @param t1 renewal timer (in seconds) @@ -575,6 +587,8 @@ public: /// @brief Constructor with all parameters /// + /// This constructor calls Subnet::Subnet, where subnet-id is generated. + /// /// @param prefix Subnet6 prefix /// @param length prefix length /// @param t1 renewal timer (in seconds)