[#2860] Fix in delegated prefix permutation

The random device wasn't properly initialized for the prefix permuations.
As a result, the same set of prefixes have been handed out by the random
and FLQ allocators. This issue has been addressed and the proper unit tests
have been added.

diff --git a/src/lib/dhcpsrv/ip_range_permutation.cc b/src/lib/dhcpsrv/ip_range_permutation.cc
index 3ffbc9c61c047d2507266cffc8e0f740f98112c1..183ce966839a88eb5eee4bff090523413735c609 100644 (file)
--- a/src/lib/dhcpsrv/ip_range_permutation.cc
+++ b/src/lib/dhcpsrv/ip_range_permutation.cc
@@ -26,6 +26,8 @@ IPRangePermutation::IPRangePermutation(const PrefixRange& range)
     : range_start_(range.start_), step_(static_cast<uint64_t>(1) << (128 - range.delegated_length_)),
       cursor_(prefixesInRange(range.prefix_length_, range.delegated_length_) - 1),
       initial_cursor_(cursor_), state_(), done_(false), generator_() {
+    std::random_device rd;
+    generator_.seed(rd());
 }
 
 IOAddress

diff --git a/src/lib/dhcpsrv/tests/ip_range_permutation_unittest.cc b/src/lib/dhcpsrv/tests/ip_range_permutation_unittest.cc
index 9285f063bad38b474737678e77b67bcbfc2ebe51..544c63bc775bf7b305f8d097778f6ef3236c49c7 100644 (file)
--- a/src/lib/dhcpsrv/tests/ip_range_permutation_unittest.cc
+++ b/src/lib/dhcpsrv/tests/ip_range_permutation_unittest.cc
@@ -10,6 +10,7 @@
 #include <gtest/gtest.h>
 
 #include <set>
+#include <vector>
 
 using namespace isc;
 using namespace isc::asiolink;
@@ -31,110 +32,171 @@ TEST(IPRangePermutationTest, constructor) {
 }
 
 // This test verifies that a permutation of IPv4 address range can
-// be generated.
+// be generated and each time a different permutation is generated.
 TEST(IPRangePermutationTest, ipv4) {
     // Create address range with 91 addresses.
     AddressRange range(IOAddress("192.0.2.10"), IOAddress("192.0.2.100"));
-    IPRangePermutation perm(range);
-
-    // This set will record unique IP addresses generated.
-    std::set<IOAddress> addrs;
-    bool done = false;
 
-    // Call the next() function 95 times. The first 91 calls should return non-zero
-    // IP addresses.
-    for (auto i = 0; i < 95; ++i) {
-        auto next = perm.next(done);
-        if (!next.isV4Zero()) {
-            // Make sure the returned address is within the range.
-            EXPECT_LE(range.start_, next);
-            EXPECT_LE(next, range.end_);
+    std::vector<std::vector<IOAddress>> iterations;
+    for (auto i = 0; i < 2; ++i) {
+        IPRangePermutation perm(range);
+        // This set will record unique IP addresses generated.
+        std::set<IOAddress> addrs;
+        // This vector will record the addresses assignment order.
+        std::vector<IOAddress> ordered_addrs;
+        bool done = false;
+        // Call the next() function 95 times. The first 91 calls should return non-zero
+        // IP addresses.
+        for (auto i = 0; i < 95; ++i) {
+            auto next = perm.next(done);
+            if (!next.isV4Zero()) {
+                // Make sure the returned address is within the range.
+                EXPECT_LE(range.start_, next);
+                EXPECT_LE(next, range.end_);
+            }
+            // If we went over all addresses in the range, the flags indicating that
+            // the permutation is exhausted should be set to true.
+            if (i >= 90) {
+                EXPECT_TRUE(done);
+                EXPECT_TRUE(perm.exhausted());
+            } else {
+                // We're not done yet, so these flag should still be false.
+                EXPECT_FALSE(done);
+                EXPECT_FALSE(perm.exhausted());
+            }
+            // Insert the address returned to the set and vector.
+            addrs.insert(next);
+            ordered_addrs.push_back(next);
         }
-        // If we went over all addresses in the range, the flags indicating that
-        // the permutation is exhausted should be set to true.
-        if (i >= 90) {
-            EXPECT_TRUE(done);
-            EXPECT_TRUE(perm.exhausted());
-        } else {
-            // We're not done yet, so these flag should still be false.
-            EXPECT_FALSE(done);
-            EXPECT_FALSE(perm.exhausted());
+
+        // We should have recorded 92 unique addresses, including the zero address.
+        EXPECT_EQ(92, addrs.size());
+        EXPECT_TRUE(addrs.begin()->isV4Zero());
+
+        iterations.push_back(ordered_addrs);
+    }
+
+    int overlaps = 0;
+    for (auto i = 0; i < iterations[0].size(); ++i) {
+        if (iterations[0][i] == iterations[1][i]) {
+            ++overlaps;
         }
-        // Insert the address returned to the set.
-        addrs.insert(next);
     }
 
-    // We should have recorded 92 unique addresses, including the zero address.
-    EXPECT_EQ(92, addrs.size());
-    EXPECT_TRUE(addrs.begin()->isV4Zero());
+    EXPECT_LE(overlaps, iterations[0].size()/5)
+        << "The number of overlapping random address between the test two iterations"
+        << " is greater than 20% of all allocated addresses in each iteration."
+        << " It means that the permutation mechanism does not sufficiently randomize"
+        << " addresses. Perhaps the randomization device is not properly initialized?";
 }
 
 // This test verifies that a permutation of IPv6 address range can
-// be generated.
+// be generated and each time a different permutation is generated.
 TEST(IPRangePermutationTest, ipv6) {
     AddressRange range(IOAddress("2001:db8:1::1:fea0"),
                        IOAddress("2001:db8:1::2:abcd"));
-    IPRangePermutation perm(range);
 
-    std::set<IOAddress> addrs;
-    bool done = false;
-    for (auto i = 0; i < 44335; ++i) {
-        auto next = perm.next(done);
-        if (!next.isV6Zero()) {
-            // Make sure that the address is within the range.
-            EXPECT_LE(range.start_, next);
-            EXPECT_LE(next, range.end_);
+    std::vector<std::vector<IOAddress>> iterations;
+    for (auto i = 0; i < 2; ++i) {
+        IPRangePermutation perm(range);
+        std::set<IOAddress> addrs;
+        std::vector<IOAddress> ordered_addrs;
+        bool done = false;
+        for (auto i = 0; i < 44335; ++i) {
+            auto next = perm.next(done);
+            if (!next.isV6Zero()) {
+                // Make sure that the address is within the range.
+                EXPECT_LE(range.start_, next);
+                EXPECT_LE(next, range.end_);
+            }
+            // If we went over all addresses in the range, the flags indicating that
+            // the permutation is exhausted should be set to true.
+            if (i >= 44333) {
+                EXPECT_TRUE(done);
+                EXPECT_TRUE(perm.exhausted());
+            } else {
+                // We're not done yet, so these flag should still be false.
+                EXPECT_FALSE(done);
+                EXPECT_FALSE(perm.exhausted());
+            }
+            // Insert the address returned to the set and vector.
+            addrs.insert(next);
+            ordered_addrs.push_back(next);
         }
-        // If we went over all addresses in the range, the flags indicating that
-        // the permutation is exhausted should be set to true.
-        if (i >= 44333) {
-            EXPECT_TRUE(done);
-            EXPECT_TRUE(perm.exhausted());
-        } else {
-            // We're not done yet, so these flag should still be false.
-            EXPECT_FALSE(done);
-            EXPECT_FALSE(perm.exhausted());
+        // We should have recorded 44335 unique addresses, including the zero address.
+        EXPECT_EQ(44335, addrs.size());
+        EXPECT_TRUE(addrs.begin()->isV6Zero());
+
+        iterations.push_back(ordered_addrs);
+    }
+
+    int overlaps = 0;
+    for (auto i = 0; i < iterations[0].size(); ++i) {
+        if (iterations[0][i] == iterations[1][i]) {
+            ++overlaps;
         }
-        // Insert the address returned to the set.
-        addrs.insert(next);
     }
-    // We should have recorded 44335 unique addresses, including the zero address.
-    EXPECT_EQ(44335, addrs.size());
-    EXPECT_TRUE(addrs.begin()->isV6Zero());
+
+    EXPECT_LE(overlaps, iterations[0].size()/5)
+        << "The number of overlapping random address between the test two iterations"
+        << " is greater than 20% of all allocated addresses in each iteration."
+        << " It means that the permutation mechanism does not sufficiently randomize"
+        << " addresses. Perhaps the randomization device is not properly initialized?";
 }
 
 // This test verifies that a permutation of delegated prefixes can be
-// generated.
+// generated and each time a different permutation is generated.
 TEST(IPRangePermutationTest, pd) {
     PrefixRange range(IOAddress("3000::"), 112, 120);
-    IPRangePermutation perm(range);
 
-    std::set<IOAddress> addrs;
-    bool done = false;
-    for (auto i = 0; i < 257; ++i) {
-        auto next = perm.next(done);
-        if (!next.isV6Zero()) {
-            // Make sure the prefix is within the range.
-            EXPECT_LE(range.start_, next);
-            EXPECT_LE(next, range.end_);
+    std::vector<std::vector<IOAddress>> iterations;
+    for (auto i = 0; i < 2; ++i) {
+        IPRangePermutation perm(range);
+        std::set<IOAddress> addrs;
+        std::vector<IOAddress> ordered_addrs;
+        bool done = false;
+        for (auto i = 0; i < 257; ++i) {
+            auto next = perm.next(done);
+            if (!next.isV6Zero()) {
+                // Make sure the prefix is within the range.
+                EXPECT_LE(range.start_, next);
+                EXPECT_LE(next, range.end_);
+            }
+            // If we went over all delegated prefixes in the range, the flags indicating
+            // that the permutation is exhausted should be set to true.
+            if (i >= 255) {
+                EXPECT_TRUE(done);
+                EXPECT_TRUE(perm.exhausted());
+            } else {
+                // We're not done yet, so these flag should still be false.
+                EXPECT_FALSE(done);
+                EXPECT_FALSE(perm.exhausted());
+            }
+            // Insert the prefix returned to the set and vector.
+            addrs.insert(next);
+            ordered_addrs.push_back(next);
         }
-        // If we went over all delegated prefixes in the range, the flags indicating
-        // that the permutation is exhausted should be set to true.
-        if (i >= 255) {
-            EXPECT_TRUE(done);
-            EXPECT_TRUE(perm.exhausted());
-        } else {
-            // We're not done yet, so these flag should still be false.
-            EXPECT_FALSE(done);
-            EXPECT_FALSE(perm.exhausted());
+        // We should have recorded 257 unique addresses, including the zero address.
+        EXPECT_EQ(257, addrs.size());
+        EXPECT_TRUE(addrs.begin()->isV6Zero());
+
+        iterations.push_back(ordered_addrs);
+    }
+
+    ASSERT_EQ(2, iterations.size());
+
+    int overlaps = 0;
+    for (auto i = 0; i < iterations[0].size(); ++i) {
+        if (iterations[0][i] == iterations[1][i]) {
+            ++overlaps;
         }
-        // Insert the prefix returned to the set.
-        addrs.insert(next);
     }
 
-    // We should have recorded 257 unique addresses, including the zero address.
-    EXPECT_EQ(257, addrs.size());
-    EXPECT_TRUE(addrs.begin()->isV6Zero());
+    EXPECT_LE(overlaps, iterations[0].size()/5)
+        << "The number of overlapping random prefixes between the test two iterations"
+        << " is greater than 20% of all allocated addresses in each iteration."
+        << " It means that the permutation mechanism does not sufficiently randomize"
+        << " prefixes. Perhaps the randomization device is not properly initialized?";
 }
 
 // This test verifies that a permutation of delegated prefixes is