[3768] Extended commentary in the allocation engine to cover recent changes.

src/bin/dhcp4/tests/dora_unittest.cc

@@ -173,11 +173,64 @@ public:
         IfaceMgr::instance().openSockets4();
     }
 
+    /// @brief Test that server doesn't allocate the lease for the client
+    /// which has the same address or client identifier as another client.
+    ///
+    /// This test checks the server's behavior in the following scenario:
+    /// - Client A identifies itself to the server using the hardware address
+    ///   and client identifier or only one of those.
+    /// - Client A performs the 4-way exchange and obtains a lease from the server.
+    /// - Client B uses the same HW address or client identifier as the client A.
+    /// - Client B uses HW address and client identifier if the client A is using
+    ///   only the one of them. Client B uses one of the HW address or client
+    ///   identifier if the client A is using both.
+    /// - Client B sends the DHCPDISCOVER to the server.
+    ///   The server determines that there is a lease for the client A using the
+    ///   same HW address as the client B. Server discards the client's message and
+    ///   doesn't offer the lease for the client B to prevent allocation of the
+    ///   lease without a unique identifier.
+    /// - The client sends the DHCPREQUEST and the server sends the DHCPNAK for the
+    ///   same reason.
+    ///
+    /// The specific test cases using this test must make sure that one of the
+    /// provided parameters is an empty string. This simulates the situation where
+    /// one of the clients has only one of the identifiers and the other one has
+    /// two.
+    ///
+    /// @param hwaddr_a HW address of client A.
+    /// @param clientid_a Client id of client A.
+    /// @param hwaddr_b HW address of client B.
+    /// @param clientid_b Client id of client B.
     void oneAllocationOverlapTest(const std::string& hwaddr_a,
                                   const std::string& clientid_a,
                                   const std::string& hwaddr_b,
                                   const std::string& clientid_b);
 
+    /// @brief Test that server can allocate the lease for the client having
+    /// the same HW Address or client id as another client.
+    ///
+    /// This test checks the server behavior in the following situation:
+    /// - Client A identifies itself to the server using client identifier
+    ///   and the hardware address and requests allocation of the new lease.
+    /// - Server allocates the lease to the client.
+    /// - Client B has a different hardware address or client identifier than
+    ///   the client A, but the other identifier is equal to the corresponding
+    ///   identifier of the client A.
+    /// - Client B sends DHCPDISCOVER.
+    /// - Server should determine that the client B is not client A, because
+    ///   it is using a different hadrware address or client identifier, even
+    ///   As a consequence, the server should offer a different address to the
+    ///   client B.
+    /// - The client B performs the 4-way exchange again, and the server
+    ///   allocates a new address to the client, which should be different
+    ///   than the address used by the client A.
+    /// - Client B is in the renewing state and it successfully renews its
+    ///   address.
+    ///
+    /// @param hwaddr_a HW address of client A.
+    /// @param clientid_a Client id of client A.
+    /// @param hwaddr_b HW address of client B.
+    /// @param clientid_b Client id of client B.
     void twoAllocationsOverlapTest(const std::string& hwaddr_a,
                                    const std::string& clientid_a,
                                    const std::string& hwaddr_b,
@@ -467,10 +520,12 @@ DORATest::twoAllocationsOverlapTest(const std::string& hwaddr_a,
     Pkt4Ptr resp_a = client_a.getContext().response_;
     // Make sure that the server has responded with DHCPACK.
     ASSERT_EQ(DHCPACK, static_cast<int>(resp_a->getType()));
+
+    // Make sure that the lease has been recorded by the server.
     Lease4Ptr lease_a = LeaseMgrFactory::instance().getLease4(client_a.config_.lease_.addr_);
     ASSERT_TRUE(lease_a);
 
-    // Use the same client identifier by client B.
+    // Create client B.
     Dhcp4Client client_b(client_a.getServer(), Dhcp4Client::SELECTING);
     client_b.setHWAddress(hwaddr_b);
     client_b.includeClientId(clientid_b);
@@ -483,6 +538,7 @@ DORATest::twoAllocationsOverlapTest(const std::string& hwaddr_a,
     // was obtained by the client A.
     ASSERT_NE(resp_b->getYiaddr(), client_a.config_.lease_.addr_);
 
+    // Make sure that the client A lease hasn't been modified.
     lease_a = LeaseMgrFactory::instance().getLease4(client_a.config_.lease_.addr_);
     ASSERT_TRUE(lease_a);
 
@@ -494,13 +550,15 @@ DORATest::twoAllocationsOverlapTest(const std::string& hwaddr_a,
     ASSERT_TRUE(client_b.getContext().response_);
     resp_b = client_b.getContext().response_;
     // Make sure that the server has responded with DHCPACK.
-
     ASSERT_EQ(DHCPACK, static_cast<int>(resp_b->getType()));
     // Again, make sure the assigned addresses are different.
     ASSERT_NE(client_b.config_.lease_.addr_, client_a.config_.lease_.addr_);
 
+    // Make sure that the client A still has a lease.
     lease_a = LeaseMgrFactory::instance().getLease4(client_a.config_.lease_.addr_);
     ASSERT_TRUE(lease_a);
+
+    // Make sure that the client B has a lease.
     Lease4Ptr lease_b = LeaseMgrFactory::instance().getLease4(client_b.config_.lease_.addr_);
     ASSERT_TRUE(lease_b);
 
@@ -536,11 +594,18 @@ DORATest::oneAllocationOverlapTest(const std::string& hwaddr_a,
     Dhcp4Client client_b(client_a.getServer(), Dhcp4Client::SELECTING);
     client_b.setHWAddress(hwaddr_b);
     client_b.includeClientId(clientid_b);
-    // Send DHCPDISCOVER and expect the response.
+    // Client A and Client B have one common identifier (HW address
+    // or client identifier) and one of them is missing the other
+    // identifier. The allocation engine can't offer an address for
+    // the client which has the same identifier as the other client and
+    // which doesn't have any other (unique) identifier. It should
+    // discard the DHCPDISCOVER.
     ASSERT_NO_THROW(client_b.doDiscover());
     Pkt4Ptr resp_b = client_b.getContext().response_;
     ASSERT_FALSE(resp_b);
 
+    // Now repeat the same test but send the DHCPREQUEST. This time the
+    // server should send the DHCPNAK.
     client_b.config_.lease_.addr_ = IOAddress::IPV4_ZERO_ADDRESS();
     client_b.setState(Dhcp4Client::INIT_REBOOT);
     ASSERT_NO_THROW(client_b.doRequest());
@@ -574,16 +639,34 @@ TEST_F(DORATest, twoAllocationsOverlap1) {
                               "02:02:03:03:04:04", "12:34");
 }
 
+// This test is similar to twoAllocationsOverlap1, but the
+// clients differ by client identifier.
 TEST_F(DORATest, twoAllocationsOverlap2) {
     twoAllocationsOverlapTest("01:02:03:04:05:06", "12:34",
                               "01:02:03:04:05:06", "22:34");
 }
 
+// This test checks the server behavior in the following situation:
+// - Client A identifies itself to the server using the hardware address
+//   and client identifier.
+// - Client A performs the 4-way exchange and obtains a lease from the server.
+// - Client B uses the same HW address as the client A, but it doesn't use
+//   the client identifier.
+// - Client B sends the DHCPDISCOVER to the server.
+//   The server determines that there is a lease for the client A using the
+//   same HW address as the client B. Server discards the client's message and
+//   doesn't offer the lease for the client B to prevent allocation of the
+//   lease without a unique identifier.
+// - The client sends the DHCPREQUEST and the server sends the DHCPNAK for the
+//   same reason.
 TEST_F(DORATest, oneAllocationOverlap1) {
     oneAllocationOverlapTest("01:02:03:04:05:06", "12:34",
                              "01:02:03:04:05:06", "");
 }
 
+// This test is similar to oneAllocationOverlap2 but this time the client A
+// uses no client identifier, and the client B uses the HW address and the
+// client identifier. The server behaves as previously.
 TEST_F(DORATest, oneAllocationOverlap2) {
     oneAllocationOverlapTest("01:02:03:04:05:06", "",
                              "01:02:03:04:05:06", "12:34");

src/lib/dhcpsrv/alloc_engine.cc

@@ -1351,21 +1351,39 @@ AllocEngine::discoverLease4(AllocEngine::ClientContext4& ctx) {
     // Obtain the sole instance of the LeaseMgr.
     LeaseMgr& lease_mgr = LeaseMgrFactory::instance();
 
-    // Check if the client has any lease already. This information is needed
-    // to either return this lease to the client or to return it as an old
-    // (existing) lease if a different one is offered.
+    // The server should hand out existing lease to the client, so we have to check
+    // if there is one. First, try to use the client's HW address.
     Lease4Ptr client_lease = lease_mgr.getLease4(*ctx.hwaddr_, ctx.subnet_->getID());
+    // If there is no lease for this HW address or the lease doesn't seem to be ours,
+    // we will have to use the client identifier. Note that in some situations two
+    // clients may use the same HW address so even if we find the lease for the HW
+    // address it doesn't mean it is ours, because client identifier may not match.
     if (ctx.clientid_ && ((!client_lease) || (client_lease && !ctx.myLease(*client_lease)))) {
+        // Check if the lease is in conflict with the lease that we want to allocate.
+        // If the lease is in conflict because of using overlapping HW address or
+        // client identifier, we can't allocate the lease for this client.
         if (client_lease && ctx.isInConflict(*client_lease)) {
             return (Lease4Ptr());
         }
+        // There is no lease or the lease we found is not conflicting with the lease
+        // which we have found for the HW address, so there is still a chance that
+        // we will allocate the lease. Check if there is a lease using the client
+        // identifier.
         client_lease = lease_mgr.getLease4(*ctx.clientid_, ctx.subnet_->getID());
     }
 
+    // Check if the lease we have found belongs to us.
     if (client_lease && !ctx.myLease(*client_lease)) {
+        // If the lease doesn't belong to us, check if we can add new lease for
+        // the client which message we're processing, or its identifiers are
+        // in conflict with this lease.
         if (ctx.isInConflict(*client_lease)) {
             return (Lease4Ptr());
         }
+        // If there is no conflict we can proceed and try to find the appropriate
+        // lease but we don't use the one we found, because it is assigned to
+        // someone else. Reset the pointer to indicate that we're not
+        // renewing this lease.
         client_lease.reset();
     }
 
@@ -1452,15 +1470,39 @@ AllocEngine::requestLease4(AllocEngine::ClientContext4& ctx) {
     // Obtain the sole instance of the LeaseMgr.
     LeaseMgr& lease_mgr = LeaseMgrFactory::instance();
 
-    // Check if the client has any lease already. This information is needed
-    // to either return this lease to the client or to return it as an old
-    // (existing) lease if a different one is offered.
+    // The server should hand out existing lease to the client, so we have to check
+    // if there is one. First, try to use the client's HW address.
     Lease4Ptr client_lease = lease_mgr.getLease4(*ctx.hwaddr_, ctx.subnet_->getID());
+    // If there is no lease for this HW address or the lease doesn't seem to be ours,
+    // we will have to use the client identifier. Note that in some situations two
+    // clients may use the same HW address so even if we find the lease for the HW
+    // address it doesn't mean it is ours, because client identifier may not match.
     if (ctx.clientid_ && ((!client_lease) || (client_lease && !ctx.myLease(*client_lease)))) {
+        // Check if the lease is in conflict with the lease that we want to allocate.
+        // If the lease is in conflict because of using overlapping HW address or
+        // client identifier, we can't allocate the lease for this client.
+        if (client_lease && ctx.isInConflict(*client_lease)) {
+            return (Lease4Ptr());
+        }
+        // There is no lease or the lease we found is not conflicting with the lease
+        // which we have found for the HW address, so there is still a chance that
+        // we will allocate the lease. Check if there is a lease using the client
+        // identifier.
         client_lease = lease_mgr.getLease4(*ctx.clientid_, ctx.subnet_->getID());
     }
 
+    // Check if the lease we have found belongs to us.
     if (client_lease && !ctx.myLease(*client_lease)) {
+        // If the lease doesn't belong to us, check if we can add new lease for
+        // the client which message we're processing, or its identifiers are
+        // in conflict with this lease.
+        if (ctx.isInConflict(*client_lease)) {
+            return (Lease4Ptr());
+        }
+        // If there is no conflict we can proceed and try to find the appropriate
+        // lease but we don't use the one we found, because it is assigned to
+        // someone else. Reset the pointer to indicate that we're not
+        // renewing this lease.
         client_lease.reset();
     }
 

src/lib/dhcpsrv/alloc_engine.h

@@ -765,6 +765,32 @@ public:
         /// the context has been created, false otherwise.
         bool myLease(const Lease4& lease) const;
 
+        /// @brief Check if the lease conflicts with the context.
+        ///
+        /// This method is used in cases when there is a lease in the lease database
+        /// for the client and the server receives the message from another client
+        /// which may be potentially using the same client identifier or HW address.
+        ///
+        /// Currently the allocation engine allows for allocating a lease for the
+        /// client which is using the same HW address or client identifier as the
+        /// client which already has a lease in the database. However, the value of
+        /// one of the identifiers must be unique. In other words, two clients may
+        /// have the same client identifier but different HW addresses and the
+        /// leases can be allocated for both (there is no conflict).
+        ///
+        /// The other possible case is that one of the clients uses both HW address
+        /// and client identifier, and another client is using only one of those
+        /// and its value is the same as for the former client. The allocation
+        /// engine treats it as a conflict because there is no unique value in the
+        /// lease database by which the server could distinguish the clients.
+        /// Hence, it doesn't allocate the lease from the context in conflict.
+        ///
+        /// This method detects the conflict described above.
+        ///
+        /// @param lease Existing lease to be checked.
+        ///
+        /// @return true if the lease is in conflict with the context, false
+        /// otherwise.
         bool isInConflict(const Lease4& lease) const;
 
     };

src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc

@@ -761,7 +761,9 @@ TEST_F(AllocEngine4Test, reservedAddressExistingLease) {
 
     // Create a lease for the client with a different address than the reserved
     // one.
-    Lease4Ptr lease(new Lease4(IOAddress("192.0.2.101"), hwaddr_, 0, 0,
+    Lease4Ptr lease(new Lease4(IOAddress("192.0.2.101"), hwaddr_,
+                               &clientid_->getClientId()[0],
+                               clientid_->getClientId().size(),
                                100, 30, 60, time(NULL), subnet_->getID(),
                                false, false, ""));
     LeaseMgrFactory::instance().addLease(lease);
@@ -1036,7 +1038,9 @@ TEST_F(AllocEngine4Test, reservedAddressExistingLeaseNoHint) {
     CfgMgr::instance().commit();
 
     // Create a lease for a different address than reserved.
-    Lease4Ptr lease(new Lease4(IOAddress("192.0.2.101"), hwaddr_, 0, 0,
+    Lease4Ptr lease(new Lease4(IOAddress("192.0.2.101"), hwaddr_,
+                               &clientid_->getClientId()[0],
+                               clientid_->getClientId().size(),
                                100, 30, 60, time(NULL), subnet_->getID(),
                                false, false, ""));
     LeaseMgrFactory::instance().addLease(lease);
@@ -1148,7 +1152,9 @@ TEST_F(AllocEngine4Test, reservedAddressConflictResolution) {
     CfgMgr::instance().commit();
 
     // Create a lease for Client A.
-    Lease4Ptr lease(new Lease4(IOAddress("192.0.2.101"), hwaddr_, 0, 0,
+    Lease4Ptr lease(new Lease4(IOAddress("192.0.2.101"), hwaddr_,
+                               &clientid_->getClientId()[0],
+                               clientid_->getClientId().size(),
                                100, 30, 60, time(NULL), subnet_->getID(),
                                false, false, ""));
     LeaseMgrFactory::instance().addLease(lease);