From 09cfa792bffe8ffa9106c915334ef539577fb62d Mon Sep 17 00:00:00 2001 From: Tomek Mrugalski Date: Mon, 30 Dec 2013 18:38:37 +0100 Subject: [PATCH] [3234] Subnet-id is now set properly in b10-dhcp{4,6} servers --- ChangeLog | 6 ++ src/bin/dhcp4/config_parser.cc | 4 ++ src/bin/dhcp4/tests/config_parser_unittest.cc | 64 +++++++++++++++++++ src/bin/dhcp6/config_parser.cc | 4 ++ src/bin/dhcp6/tests/config_parser_unittest.cc | 57 +++++++++++++++++ src/lib/dhcpsrv/subnet.cc | 3 + src/lib/dhcpsrv/subnet.h | 20 +++++- 7 files changed, 156 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index b12cf47a8b..f9e71d5a11 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +7XX. [bug] tomek + 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) + 720. [func] tmark Added the initial implementation of the class, NameAddTransaction, to b10-dhcp-ddns. This class provides a state machine which implements diff --git a/src/bin/dhcp4/config_parser.cc b/src/bin/dhcp4/config_parser.cc index 777cce5d2b..0e45fdeb9c 100644 --- a/src/bin/dhcp4/config_parser.cc +++ b/src/bin/dhcp4/config_parser.cc @@ -433,6 +433,10 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) { LOG_DEBUG(dhcp4_logger, DBG_DHCP4_COMMAND, DHCP4_CONFIG_START).arg(config_set->str()); + // Before starting any subnet operations, let's reset the subnet-id counter, + // so newly recreated configuration starts with first subnet-id equal 1. + Subnet::resetSubnetID(); + // Some of the values specified in the configuration depend on // other values. Typically, the values in the subnet4 structure // depend on the global values. Also, option values configuration diff --git a/src/bin/dhcp4/tests/config_parser_unittest.cc b/src/bin/dhcp4/tests/config_parser_unittest.cc index 1d9ccb7fa0..e7a034df7f 100644 --- a/src/bin/dhcp4/tests/config_parser_unittest.cc +++ b/src/bin/dhcp4/tests/config_parser_unittest.cc @@ -410,8 +410,72 @@ TEST_F(Dhcp4ParserTest, subnetGlobalDefaults) { EXPECT_EQ(1000, subnet->getT1()); EXPECT_EQ(2000, subnet->getT2()); EXPECT_EQ(4000, subnet->getValid()); + + // Check that subnet-id is 1 + EXPECT_EQ(1, subnet->getID()); } +// Goal of this test is to verify that multiple subnets get unique +// subnet-ids. Also, test checks that it's possible to do reconfiguration +// multiple times. +TEST_F(Dhcp4ParserTest, multipleSubnets) { + ConstElementPtr x; + string config = "{ \"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 }"; + + ElementPtr json = Element::fromJSON(config); + + EXPECT_NO_THROW(x = configureDhcp4Server(*srv_, json)); + ASSERT_TRUE(x); + comment_ = parseAnswer(rcode_, x); + ASSERT_EQ(0, rcode_); + + int cnt = 0; // Number of reconfigurations + + do { + ElementPtr json = Element::fromJSON(config); + + 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 + + // 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()); + EXPECT_EQ(4, subnets->at(3)->getID()); + + // Repeat reconfiguration process 10 times and check that the subnet-id + // is set to the same value. Technically, just two iterations would be + // sufficient, but it's nice to have a test that exercises reconfiguration + // a bit. + } while (++cnt < 10); +} + + // Checks if the next-server defined as global parameter is taken into // consideration. TEST_F(Dhcp4ParserTest, nextServerGlobal) { diff --git a/src/bin/dhcp6/config_parser.cc b/src/bin/dhcp6/config_parser.cc index 1bb3da0421..fb0e962510 100644 --- a/src/bin/dhcp6/config_parser.cc +++ b/src/bin/dhcp6/config_parser.cc @@ -646,6 +646,10 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) { LOG_DEBUG(dhcp6_logger, DBG_DHCP6_COMMAND, DHCP6_CONFIG_START).arg(config_set->str()); + // Before starting any subnet operations, let's reset the subnet-id counter, + // so newly recreated configuration starts with first subnet-id equal 1. + Subnet::resetSubnetID(); + // Some of the values specified in the configuration depend on // other values. Typically, the values in the subnet6 structure // depend on the global values. Also, option values configuration diff --git a/src/bin/dhcp6/tests/config_parser_unittest.cc b/src/bin/dhcp6/tests/config_parser_unittest.cc index 242017236f..b613095d28 100644 --- a/src/bin/dhcp6/tests/config_parser_unittest.cc +++ b/src/bin/dhcp6/tests/config_parser_unittest.cc @@ -436,6 +436,63 @@ TEST_F(Dhcp6ParserTest, subnetGlobalDefaults) { EXPECT_EQ(2000, subnet->getT2()); EXPECT_EQ(3000, subnet->getPreferred()); EXPECT_EQ(4000, subnet->getValid()); + + // Check that subnet-id is 1 + EXPECT_EQ(1, subnet->getID()); +} + +// Goal of this test is to verify that multiple subnets get unique +// subnet-ids. Also, test checks that it's possible to do reconfiguration +// multiple times. +TEST_F(Dhcp6ParserTest, multipleSubnets) { + ConstElementPtr x; + string config = "{ \"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 }"; + + int cnt = 0; // Number of reconfigurations + + do { + ElementPtr json = Element::fromJSON(config); + + 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 + + // 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()); + EXPECT_EQ(4, subnets->at(3)->getID()); + + // Repeat reconfiguration process 10 times and check that the subnet-id + // is set to the same value. Technically, just two iterations would be + // sufficient, but it's nice to have a test that exercises reconfiguration + // a bit. + } while (++cnt < 10); } // This test checks if it is possible to override global values diff --git a/src/lib/dhcpsrv/subnet.cc b/src/lib/dhcpsrv/subnet.cc index d861afe25f..a07442e5a6 100644 --- a/src/lib/dhcpsrv/subnet.cc +++ b/src/lib/dhcpsrv/subnet.cc @@ -24,6 +24,9 @@ using namespace isc::asiolink; namespace isc { namespace dhcp { +// This is an initial value of subnet-id. See comments in subnet.h for details. +SubnetID Subnet::static_id_ = 1; + Subnet::Subnet(const isc::asiolink::IOAddress& prefix, uint8_t len, const Triplet& t1, const Triplet& t2, diff --git a/src/lib/dhcpsrv/subnet.h b/src/lib/dhcpsrv/subnet.h index 31dc9477af..ec8c397246 100644 --- a/src/lib/dhcpsrv/subnet.h +++ b/src/lib/dhcpsrv/subnet.h @@ -366,6 +366,15 @@ public: /// @return textual representation virtual std::string toText() const; + /// @brief Resets subnet-id counter to its initial value (1) + /// + /// This should be called during reconfiguration, before any new + /// subnet objects are created. It will ensure that the subnet_id will + /// be consistent between reconfigures. + static void resetSubnetID() { + static_id_ = 1; + } + protected: /// @brief Returns all pools (non-const variant) /// @@ -390,12 +399,19 @@ protected: /// derive from this class. virtual ~Subnet() { }; + /// @brief keeps the subnet-id value + /// + /// It is inreased every time a new Subnet object is created. + /// It is reset (@ref resetSubnetId) every time reconfiguration occurs. + /// + /// Static value initialized in subnet.cc. + static SubnetID static_id_; + /// @brief returns the next unique Subnet-ID /// /// @return the next unique Subnet-ID static SubnetID getNextID() { - static SubnetID id = 0; - return (id++); + return (static_id_++); } /// @brief Checks if used pool type is valid