From: Marcin Siodelski Date: Thu, 19 Mar 2015 18:43:22 +0000 (+0100) Subject: [3768] Extended commentary in the allocation engine to cover recent changes. X-Git-Tag: kea-0.9.1~10^2~3 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=568a6beed1b34b512d7348dd1abac3e60734e232;p=thirdparty%2Fkea.git [3768] Extended commentary in the allocation engine to cover recent changes. --- diff --git a/src/bin/dhcp4/tests/dora_unittest.cc b/src/bin/dhcp4/tests/dora_unittest.cc index 79c7314566..29735c9044 100644 --- a/src/bin/dhcp4/tests/dora_unittest.cc +++ b/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(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(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"); diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index f3a55217ad..33c9ba1cd8 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/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(); } diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h index 440b19bdc7..9da85c311a 100644 --- a/src/lib/dhcpsrv/alloc_engine.h +++ b/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; }; diff --git a/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc index 99c3b1a252..7935aa65dc 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc +++ b/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);