Skip to content
GitLab
Commit 661e1f13 authored by Tomek Mrugalski's avatar Tomek Mrugalski 🛰
Browse files

[master] Merge branch 'trac3234' (DHCP subnet-id increase fix) 

Conflicts:
	ChangeLog
parents 2fad937d 31e41608
Hide whitespace changes
Inline Side-by-side
ChangeLog
View file @ 661e1f13
730. [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 31e416087685a6dadc3047fdbb0927bbf60095aa)
729. [bug] marcin
b10-dhcp4 discards DHCPv4 messages carrying server identifiers
which don't match server identifiers used by the server.
......
doc/guide/bind10-guide.xml
View file @ 661e1f13
......@@ -4471,6 +4471,21 @@ Dhcp4/subnet4 [] list (default)
development and should be treated as <quote>not implemented
yet</quote>, rather than actual limitations.</para>
<itemizedlist>
<listitem> <!-- see tickets #3234, #3281 -->
<para>
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.
</para>
</listitem>
<listitem>
<para>
On startup, the DHCPv4 server does not get the full configuration from
......@@ -5384,6 +5399,22 @@ should include options from the isc option space:
yet</quote>, rather than actual limitations.</para>
<itemizedlist>
<listitem> <!-- see tickets #3234, #3281 -->
<para>
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.
</para>
</listitem>
<listitem>
<para>
On startup, the DHCPv6 server does not get the full configuration from
......
src/bin/dhcp4/config_parser.cc
View file @ 661e1f13
......@@ -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
......
src/bin/dhcp4/tests/config_parser_unittest.cc
View file @ 661e1f13
......@@ -410,8 +410,195 @@ 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);
}
// 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.
TEST_F(Dhcp4ParserTest, nextServerGlobal) {
......
src/bin/dhcp6/config_parser.cc
View file @ 661e1f13
......@@ -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
......
src/bin/dhcp6/tests/config_parser_unittest.cc
View file @ 661e1f13
......@@ -436,8 +436,190 @@ 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);
}
// 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) {
......
src/lib/dhcpsrv/subnet.cc
View file @ 661e1f13
......@@ -24,11 +24,14 @@ 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<uint32_t>& t1,
const Triplet<uint32_t>& t2,
const Triplet<uint32_t>& 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)),
......
src/lib/dhcpsrv/subnet.h
View file @ 661e1f13
......@@ -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)
///
......@@ -378,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<uint32_t>& t1,
const Triplet<uint32_t>& t2,
......@@ -390,12 +404,24 @@ 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
///
/// 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 id = 0;
return (id++);
static SubnetID generateNextID() {
return (static_id_++);
}
/// @brief Checks if used pool type is valid
......@@ -495,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)
......@@ -559,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)
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment