[2280] Improved efficiency of pool search.

Also added checks for overlapping pools within a subnet.

src/lib/dhcpsrv/subnet.cc

-// 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 <dhcp/option_space.h>
 #include <dhcpsrv/addr_utilities.h>
 #include <dhcpsrv/subnet.h>
-
+#include <algorithm>
 #include <sstream>
 
 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 {
@@ -235,20 +262,34 @@ const PoolPtr Subnet::getPool(Lease::Type type, const isc::asiolink::IOAddress&
     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<int>(prefix_len_)
                       << " to which it is being added");
+
+        }
+    }
+
+    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<int>(prefix_len_)
+                  << " to which it is being added");
     }
 
-    /// @todo: Check that pools do not overlap
+    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<uint32_t>& t1,
                  const Triplet<uint32_t>& t2,

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

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) {