From 44d863b33a65a1f3a8b4aceb3b162459077b7bea Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Tue, 11 Oct 2016 23:57:54 +0200 Subject: [PATCH] [2280] Improved efficiency of pool search. Also added checks for overlapping pools within a subnet. --- src/lib/dhcpsrv/subnet.cc | 162 +++++++++++++++++++++-- src/lib/dhcpsrv/subnet.h | 27 +++- src/lib/dhcpsrv/tests/subnet_unittest.cc | 155 +++++++++++++++++++--- 3 files changed, 309 insertions(+), 35 deletions(-) diff --git a/src/lib/dhcpsrv/subnet.cc b/src/lib/dhcpsrv/subnet.cc index c95daff3f..b9cec1ef1 100644 --- a/src/lib/dhcpsrv/subnet.cc +++ b/src/lib/dhcpsrv/subnet.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2012-2015 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2012-2016 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -10,10 +10,37 @@ #include #include #include - +#include #include using namespace isc::asiolink; +using namespace isc::dhcp; + +namespace { + +/// @brief Function used in calls to std::upper_bound to check +/// if the specified prefix is lower than the first address a pool. +/// +/// @return true if prefix is lower than the first address in the pool. +bool +prefixLessThanFirstAddress(const IOAddress& prefix, const PoolPtr& pool) { + return (prefix < pool->getFirstAddress()); +} + +/// @brief Function used in calls to std::sort to compare first +/// prefixes of the two pools. +/// +/// @param pool1 First pool. +/// @param pool2 Second pool. +/// +/// @return true if first prefix of the first pool is smaller than +/// the first address of the second pool. +bool +comparePoolFirstAddress(const PoolPtr& pool1, const PoolPtr& pool2) { + return (pool1->getFirstAddress() < pool2->getFirstAddress()); +}; + +} namespace isc { namespace dhcp { @@ -228,27 +255,41 @@ PoolCollection& Subnet::getPoolsWritable(Lease::Type type) { } const PoolPtr Subnet::getPool(Lease::Type type, const isc::asiolink::IOAddress& hint, - bool anypool /* true */) const { + bool anypool /* true */) const { // check if the type is valid (and throw if it isn't) checkType(type); const PoolCollection& pools = getPools(type); PoolPtr candidate; - for (PoolCollection::const_iterator pool = pools.begin(); - pool != pools.end(); ++pool) { - // if we won't find anything better, then let's just use the first pool - if (anypool && !candidate) { - candidate = *pool; + if (!pools.empty()) { + // Pools are sorted by their first prefixes. For example: 2001::, + // 2001::db8::, 3000:: etc. If our hint is 2001:db8:5:: we want to + // find the pool with the longest matching prefix, so: 2001:db8::, + // rather than 2001::. upper_bound returns the first pool with a prefix + // that is greater than 2001:db8:5::, i.e. 3000::. To find the longest + // matching prefix we use decrement operator to go back by one item. + // If returned iterator points to begin it means that prefixes in all + // pools are greater than out prefix, and thus there is no match. + PoolCollection::const_iterator ub = + std::upper_bound(pools.begin(), pools.end(), hint, + prefixLessThanFirstAddress); + + if (ub != pools.begin()) { + --ub; + if ((*ub)->inRange(hint)) { + candidate = *ub; + } } - // if the client provided a pool and there's a pool that hint is valid - // in, then let's use that pool - if ((*pool)->inRange(hint)) { - return (*pool); + // If we don't find anything better, then let's just use the first pool + if (!candidate && anypool) { + candidate = *pools.begin(); } } + + // Return a pool or NULL if no match found. return (candidate); } @@ -271,13 +312,40 @@ Subnet::addPool(const PoolPtr& pool) { << " the prefix of a subnet: " << prefix_ << "/" << static_cast(prefix_len_) << " to which it is being added"); + } } - /// @todo: Check that pools do not overlap + bool overlaps = false; + if (pool->getType() == Lease::TYPE_V4) { + overlaps = poolOverlaps(Lease::TYPE_V4, pool); + + } else { + overlaps = + poolOverlaps(Lease::TYPE_NA, pool) || + poolOverlaps(Lease::TYPE_PD, pool) || + poolOverlaps(Lease::TYPE_TA, pool); + } + + if (overlaps) { + isc_throw(BadValue,"a pool of type " + << Lease::typeToText(pool->getType()) + << ", with the following address range: " + << pool->getFirstAddress() << "-" + << pool->getLastAddress() << " overlaps with " + "an existing pool in the subnet: " + << prefix_ << "/" << static_cast(prefix_len_) + << " to which it is being added"); + } + + PoolCollection& pools_writable = getPoolsWritable(pool->getType()); // Add the pool to the appropriate pools collection - getPoolsWritable(pool->getType()).push_back(pool); + pools_writable.push_back(pool); + + // Sort pools by first address. + std::sort(pools_writable.begin(), pools_writable.end(), + comparePoolFirstAddress); } void @@ -315,6 +383,72 @@ Subnet::inPool(Lease::Type type, const isc::asiolink::IOAddress& addr) const { return (false); } +bool +Subnet::poolOverlaps(const Lease::Type& pool_type, const PoolPtr& pool) const { + const PoolCollection& pools = getPools(pool_type); + + // If no pools, we don't overlap. Nothing to do. + if (pools.empty()) { + return (false); + } + + // We're going to insert a new pool, likely between two existing pools. + // So we're going to end up with the following case: + // |<---- pool1 ---->| |<-------- pool2 ------>| |<-- pool3 -->| + // F1 L1 F2 L2 F3 L3 + // where pool1 and pool3 are existing pools, pool2 is a pool being + // inserted and "F"/"L" mark first and last address in the pools + // respectively. So the following conditions must be fulfilled: + // F2 > L1 and L2 < F3. Obviously, for any pool: F < L. + + // Search for pool3. We use F2 and upper_bound to find the F3 (upper_bound + // returns first pool in the sorted container which first address is + // greater than F2). prefixLessThanPoolAddress with the first argument + // set to "true" is the custom comparison function for upper_bound, which + // compares F2 with the first addresses of the existing pools. + PoolCollection::const_iterator pool3_it = + std::upper_bound(pools.begin(), pools.end(), pool->getFirstAddress(), + prefixLessThanFirstAddress); + + // upper_bound returns a first pool which first address is greater than the + // address F2. However, it is also possible that there is a pool which first + // address is equal to F2. Such pool is also in conflict with a new pool. + // If the returned value is pools.begin() it means that all pools have greater + // first address than F2, thus none of the pools can have first address equal + // to F2. Otherwise, we'd need to check them for equality. + if (pool3_it != pools.begin()) { + // Go back one pool and check if addresses are equal. + PoolPtr pool3 = *(pool3_it - 1); + if (pool3->getFirstAddress() == pool->getFirstAddress()) { + return (true); + } + } + + // If returned value is unequal pools.end() it means that there is a pool3, + // with F3 > F2. + if (pool3_it != pools.end()) { + // Let's store the pointer to this pool. + PoolPtr pool3 = *pool3_it; + // F3 must be greater than L2, otherwise pools will overlap. + if (pool3->getFirstAddress() <= pool->getLastAddress()) { + return (true); + } + } + + // If L2 is ok, we now have to find the pool1. This pool should be + // right before the pool3 if there is any pool before pool3. + if (pool3_it != pools.begin()) { + PoolPtr pool1 = *(pool3_it - 1); + // F2 must be greater than L1. + if (pool->getFirstAddress() <= pool1->getLastAddress()) { + return (true); + } + } + + return (false); +} + + Subnet6::Subnet6(const isc::asiolink::IOAddress& prefix, uint8_t length, const Triplet& t1, const Triplet& t2, diff --git a/src/lib/dhcpsrv/subnet.h b/src/lib/dhcpsrv/subnet.h index 13abdd14b..d1d6d4840 100644 --- a/src/lib/dhcpsrv/subnet.h +++ b/src/lib/dhcpsrv/subnet.h @@ -173,14 +173,21 @@ public: /// requesting router because the requesting router may use the /// delegated prefixes in different networks (using different subnets). /// + /// A DHCPv4 pool being added must not overlap with any existing DHCPv4 + /// pool. A DHCPv6 pool being added must not overlap with any existing + /// DHCPv6 pool. + /// + /// Pools held within a subnet are sorted by first pool address/prefix + /// from the lowest to the highest. + /// /// @param pool pool to be added /// - /// @throw isc::BadValue if the pool type is invalid or the pool + /// @throw isc::BadValue if the pool type is invalid, the pool /// is not an IA_PD pool and the address range of this pool does not - /// match the subnet prefix. + /// match the subnet prefix, or the pool overlaps with an existing pool + /// within the subnet. void addPool(const PoolPtr& pool); - /// @brief Deletes all pools of specified type /// /// This method is used for testing purposes only @@ -189,6 +196,10 @@ public: /// @brief Returns a pool that specified address belongs to /// + /// This method uses binary search to retrieve the pool. Thus, the number + /// of comparisons performed by this method is logarithmic in the number + /// of pools belonging to a subnet. + /// /// If there is no pool that the address belongs to (hint is invalid), other /// pool of specified type will be returned. /// @@ -413,6 +424,16 @@ protected: /// @return sum of possible leases uint64_t sumPoolCapacity(const PoolCollection& pools) const; + /// @brief Checks if the specified pool overlaps with an existing pool. + /// + /// @param pool_type Pool type. + /// @param pool Pointer to a pool for which the method should check if + /// it overlaps with any existing pool within this subnet. + /// + /// @return true if pool overlaps with an existing pool of a specified + /// type. + bool poolOverlaps(const Lease::Type& pool_type, const PoolPtr& pool) const; + /// @brief subnet-id /// /// Subnet-id is a unique value that can be used to find or identify diff --git a/src/lib/dhcpsrv/tests/subnet_unittest.cc b/src/lib/dhcpsrv/tests/subnet_unittest.cc index b87c71b5c..46947062e 100644 --- a/src/lib/dhcpsrv/tests/subnet_unittest.cc +++ b/src/lib/dhcpsrv/tests/subnet_unittest.cc @@ -124,7 +124,8 @@ TEST(Subnet4Test, matchClientId) { EXPECT_TRUE(subnet.getMatchClientId()); } -TEST(Subnet4Test, Pool4InSubnet4) { +// Checks that it is possible to add and retrieve multiple pools. +TEST(Subnet4Test, pool4InSubnet4) { Subnet4Ptr subnet(new Subnet4(IOAddress("192.1.2.0"), 24, 1, 2, 3)); @@ -132,15 +133,16 @@ TEST(Subnet4Test, Pool4InSubnet4) { PoolPtr pool2(new Pool4(IOAddress("192.1.2.128"), 26)); PoolPtr pool3(new Pool4(IOAddress("192.1.2.192"), 30)); - EXPECT_NO_THROW(subnet->addPool(pool1)); + // Add pools in reverse order to make sure that they get ordered by + // first address. + EXPECT_NO_THROW(subnet->addPool(pool3)); // If there's only one pool, get that pool PoolPtr mypool = subnet->getAnyPool(Lease::TYPE_V4); - EXPECT_EQ(mypool, pool1); - + EXPECT_EQ(mypool, pool3); EXPECT_NO_THROW(subnet->addPool(pool2)); - EXPECT_NO_THROW(subnet->addPool(pool3)); + EXPECT_NO_THROW(subnet->addPool(pool1)); // If there are more than one pool and we didn't provide hint, we // should get the first pool @@ -149,11 +151,30 @@ TEST(Subnet4Test, Pool4InSubnet4) { EXPECT_EQ(mypool, pool1); // If we provide a hint, we should get a pool that this hint belongs to - EXPECT_NO_THROW(mypool = subnet->getPool(Lease::TYPE_V4, + ASSERT_NO_THROW(mypool = subnet->getPool(Lease::TYPE_V4, IOAddress("192.1.2.195"))); - EXPECT_EQ(mypool, pool3); + ASSERT_NO_THROW(mypool = subnet->getPool(Lease::TYPE_V4, + IOAddress("192.1.2.129"))); + EXPECT_EQ(mypool, pool2); + + ASSERT_NO_THROW(mypool = subnet->getPool(Lease::TYPE_V4, + IOAddress("192.1.2.64"))); + EXPECT_EQ(mypool, pool1); + + // Specify addresses which don't belong to any existing pools. The + // third parameter prevents it from returning "any" available + // pool if a good match is not found. + ASSERT_NO_THROW(mypool = subnet->getPool(Lease::TYPE_V4, + IOAddress("192.1.2.200"), + false)); + EXPECT_FALSE(mypool); + + ASSERT_NO_THROW(mypool = subnet->getPool(Lease::TYPE_V4, + IOAddress("192.1.1.254"), + false)); + EXPECT_FALSE(mypool); } // Check if it's possible to get specified number of possible leases for @@ -182,22 +203,69 @@ TEST(Subnet4Test, getCapacity) { EXPECT_EQ(196, subnet->getPoolCapacity(Lease::TYPE_V4)); } -TEST(Subnet4Test, Subnet4_Pool4_checks) { +// Checks that it is not allowed to add invalid pools. +TEST(Subnet4Test, pool4Checks) { Subnet4Ptr subnet(new Subnet4(IOAddress("192.0.2.0"), 8, 1, 2, 3)); // this one is in subnet - Pool4Ptr pool1(new Pool4(IOAddress("192.255.0.0"), 16)); + Pool4Ptr pool1(new Pool4(IOAddress("192.254.0.0"), 16)); subnet->addPool(pool1); // this one is larger than the subnet! Pool4Ptr pool2(new Pool4(IOAddress("193.0.0.0"), 24)); - EXPECT_THROW(subnet->addPool(pool2), BadValue); + ASSERT_THROW(subnet->addPool(pool2), BadValue); // this one is totally out of blue Pool4Ptr pool3(new Pool4(IOAddress("1.2.3.4"), 16)); - EXPECT_THROW(subnet->addPool(pool3), BadValue); + ASSERT_THROW(subnet->addPool(pool3), BadValue); + + // This pool should be added just fine. + Pool4Ptr pool4(new Pool4(IOAddress("192.0.2.10"), + IOAddress("192.0.2.20"))); + ASSERT_NO_THROW(subnet->addPool(pool4)); + + // This one overlaps with the previous pool. + Pool4Ptr pool5(new Pool4(IOAddress("192.0.2.1"), + IOAddress("192.0.2.15"))); + ASSERT_THROW(subnet->addPool(pool5), BadValue); + + // This one also overlaps. + Pool4Ptr pool6(new Pool4(IOAddress("192.0.2.20"), + IOAddress("192.0.2.30"))); + ASSERT_THROW(subnet->addPool(pool6), BadValue); + + // This one "surrounds" the other pool. + Pool4Ptr pool7(new Pool4(IOAddress("192.0.2.8"), + IOAddress("192.0.2.23"))); + ASSERT_THROW(subnet->addPool(pool7), BadValue); + + // This one does not overlap. + Pool4Ptr pool8(new Pool4(IOAddress("192.0.2.30"), + IOAddress("192.0.2.40"))); + ASSERT_NO_THROW(subnet->addPool(pool8)); + + // This one has a lower bound in the pool of 192.0.2.10-20. + Pool4Ptr pool9(new Pool4(IOAddress("192.0.2.18"), + IOAddress("192.0.2.30"))); + ASSERT_THROW(subnet->addPool(pool9), BadValue); + + // This one has an upper bound in the pool of 192.0.2.30-40. + Pool4Ptr pool10(new Pool4(IOAddress("192.0.2.25"), + IOAddress("192.0.2.32"))); + ASSERT_THROW(subnet->addPool(pool10), BadValue); + + // Add a pool with a single address. + Pool4Ptr pool11(new Pool4(IOAddress("192.255.0.50"), + IOAddress("192.255.0.50"))); + ASSERT_NO_THROW(subnet->addPool(pool11)); + + // Now we're going to add the same pool again. This is an interesting + // case because we're checking if the code is properly using upper_bound + // function, which returns a pool that has an address greater than the + // specified one. + ASSERT_THROW(subnet->addPool(pool11), BadValue); } // Tests whether Subnet4 object is able to store and process properly @@ -721,27 +789,78 @@ TEST(Subnet6Test, clientClassesMultiple) { EXPECT_TRUE(subnet->clientSupported(bar_class)); } -TEST(Subnet6Test, Subnet6_Pool6_checks) { +// Checks that it is not allowed to add invalid pools. +TEST(Subnet6Test, pool6Checks) { Subnet6Ptr subnet(new Subnet6(IOAddress("2001:db8:1::"), 56, 1, 2, 3, 4)); // this one is in subnet Pool6Ptr pool1(new Pool6(Lease::TYPE_NA, IOAddress("2001:db8:1:1::"), 64)); - subnet->addPool(pool1); + ASSERT_NO_THROW(subnet->addPool(pool1)); // this one is larger than the subnet! Pool6Ptr pool2(new Pool6(Lease::TYPE_NA, IOAddress("2001:db8::"), 48)); - EXPECT_THROW(subnet->addPool(pool2), BadValue); - + ASSERT_THROW(subnet->addPool(pool2), BadValue); // this one is totally out of blue Pool6Ptr pool3(new Pool6(Lease::TYPE_NA, IOAddress("3000::"), 16)); - EXPECT_THROW(subnet->addPool(pool3), BadValue); - + ASSERT_THROW(subnet->addPool(pool3), BadValue); Pool6Ptr pool4(new Pool6(Lease::TYPE_NA, IOAddress("4001:db8:1::"), 80)); - EXPECT_THROW(subnet->addPool(pool4), BadValue); + ASSERT_THROW(subnet->addPool(pool4), BadValue); + + // This pool should be added just fine. + Pool6Ptr pool5(new Pool6(Lease::TYPE_NA, IOAddress("2001:db8:1:2::100"), + IOAddress("2001:db8:1:2::200"))); + ASSERT_NO_THROW(subnet->addPool(pool5)); + + // This pool overlaps with a previously added pool. + Pool6Ptr pool6(new Pool6(Lease::TYPE_NA, IOAddress("2001:db8:1:2::1"), + IOAddress("2001:db8:1:2::150"))); + ASSERT_THROW(subnet->addPool(pool6), BadValue); + + // This pool also overlaps + Pool6Ptr pool7(new Pool6(Lease::TYPE_NA, IOAddress("2001:db8:1:2::150"), + IOAddress("2001:db8:1:2::300"))); + ASSERT_THROW(subnet->addPool(pool7), BadValue); + + // This one "surrounds" the other pool. + Pool6Ptr pool8(new Pool6(Lease::TYPE_NA, IOAddress("2001:db8:1:2::50"), + IOAddress("2001:db8:1:2::250"))); + ASSERT_THROW(subnet->addPool(pool8), BadValue); + + // This one does not overlap. + Pool6Ptr pool9(new Pool6(Lease::TYPE_NA, IOAddress("2001:db8:1:2::300"), + IOAddress("2001:db8:1:2::400"))); + ASSERT_NO_THROW(subnet->addPool(pool9)); + + // This one has a lower bound in the pool of 2001:db8:1::100-200. + Pool6Ptr pool10(new Pool6(Lease::TYPE_NA, IOAddress("2001:db8:1:2::200"), + IOAddress("2001:db8:1:2::225"))); + ASSERT_THROW(subnet->addPool(pool10), BadValue); + + // This one has an upper bound in the pool of 2001:db8:1::300-400. + Pool6Ptr pool11(new Pool6(Lease::TYPE_NA, IOAddress("2001:db8:1:2::250"), + IOAddress("2001:db8:1:2::300"))); + ASSERT_THROW(subnet->addPool(pool11), BadValue); + + // Add a pool with a single address. + Pool6Ptr pool12(new Pool6(Lease::TYPE_NA, IOAddress("2001:db8:1:3::250"), + IOAddress("2001:db8:1:3::250"))); + ASSERT_NO_THROW(subnet->addPool(pool12)); + + // Now we're going to add the same pool again. This is an interesting + // case because we're checking if the code is properly using upper_bound + // function, which returns a pool that has an address greater than the + // specified one. + ASSERT_THROW(subnet->addPool(pool12), BadValue); + + // Prefix pool overlaps with the pool1. We can't hand out addresses and + // prefixes from the same range. + Pool6Ptr pool13(new Pool6(Lease::TYPE_PD, IOAddress("2001:db8:1:1:2::"), + 80, 96)); + ASSERT_THROW(subnet->addPool(pool13), BadValue); } TEST(Subnet6Test, addOptions) { -- GitLab