From: Marcin Siodelski Date: Wed, 21 Aug 2013 10:15:21 +0000 (+0200) Subject: [3083] Allocation engine returns a copy of the previous lease instance. X-Git-Tag: bind10-1.2.0beta1-release~242^2~5 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=bf6d6bcddd460b7fa2c77314bccb4e14ab856609;p=thirdparty%2Fkea.git [3083] Allocation engine returns a copy of the previous lease instance. --- diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index 531fff9666..ac5e61626a 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -332,6 +332,9 @@ AllocEngine::allocateAddress4(const SubnetPtr& subnet, const isc::hooks::CalloutHandlePtr& callout_handle, Lease4Ptr& old_lease) { + // The NULL pointer indicates that the old lease didn't exist. It may + // be later set to non NULL value if existing lease is found in the + // database. old_lease.reset(); try { @@ -352,6 +355,7 @@ AllocEngine::allocateAddress4(const SubnetPtr& subnet, // Check if there's existing lease for that subnet/clientid/hwaddr combination. Lease4Ptr existing = LeaseMgrFactory::instance().getLease4(*hwaddr, subnet->getID()); if (existing) { + // Save the old lease, before renewal. old_lease.reset(new Lease4(*existing)); // We have a lease already. This is a returning client, probably after // its reboot. @@ -367,6 +371,7 @@ AllocEngine::allocateAddress4(const SubnetPtr& subnet, if (clientid) { existing = LeaseMgrFactory::instance().getLease4(*clientid, subnet->getID()); if (existing) { + // Save the old lease before renewal. old_lease.reset(new Lease4(*existing)); // we have a lease already. This is a returning client, probably after // its reboot. @@ -398,6 +403,7 @@ AllocEngine::allocateAddress4(const SubnetPtr& subnet, } } else { if (existing->expired()) { + // Save the old lease, before reusing it. old_lease.reset(new Lease4(*existing)); return (reuseExpiredLease(existing, subnet, clientid, hwaddr, callout_handle, fake_allocation)); @@ -444,6 +450,8 @@ AllocEngine::allocateAddress4(const SubnetPtr& subnet, // allocation attempts. } else { if (existing->expired()) { + // Save old lease before reusing it. + old_lease.reset(new Lease4(*existing)); return (reuseExpiredLease(existing, subnet, clientid, hwaddr, callout_handle, fake_allocation)); } diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h index 9a919dce49..fd966dc11e 100644 --- a/src/lib/dhcpsrv/alloc_engine.h +++ b/src/lib/dhcpsrv/alloc_engine.h @@ -182,11 +182,33 @@ protected: /// we give up (0 means unlimited) AllocEngine(AllocType engine_type, unsigned int attempts); - /// @brief Allocates an IPv4 lease + /// @brief Returns IPv4 lease. /// - /// This method uses currently selected allocator to pick an address from - /// specified subnet, creates a lease for that address and then inserts - /// it into LeaseMgr (if this allocation is not fake). + /// This method finds the appropriate lease for the client using the + /// following algorithm: + /// - If lease exists for the combination of the HW address, client id and + /// subnet, try to renew a lease and return it. + /// - If lease exists for the combination of the client id and subnet, try + /// to renew the lease and return it. + /// - If client supplied an address hint and this address is available, + /// allocate the new lease with this address. + /// - If client supplied an address hint and the lease for this address + /// exists in the database, return this lease if it is expired. + /// - Pick new address from the pool and try to allocate it for the client, + /// if expired lease exists for the picked address, try to reuse this lease. + /// + /// When a server should do DNS updates, it is required that allocation + /// returns the information how the lease was obtained by the allocation + /// engine. In particular, the DHCP server should be able to check whether + /// existing lease was returned, or new lease was allocated. When existing + /// lease was returned, server should check whether the FQDN has changed + /// between the allocation of the old and new lease. If so, server should + /// perform appropriate DNS update. If not, server may choose to not + /// perform the update. The information about the old lease is returned via + /// @c old_lease parameter. If NULL value is returned, it is an indication + /// that new lease was allocated for the client. If non-NULL value is + /// returned, it is an indication that allocation engine reused/renewed an + /// existing lease. /// /// @param subnet subnet the allocation should come from /// @param clientid Client identifier @@ -337,7 +359,8 @@ private: /// an address for DISCOVER that is not really allocated (true) /// @return refreshed lease /// @throw BadValue if trying to recycle lease that is still valid - Lease4Ptr reuseExpiredLease(Lease4Ptr& expired, const SubnetPtr& subnet, + Lease4Ptr reuseExpiredLease(Lease4Ptr& expired, + const SubnetPtr& subnet, const ClientIdPtr& clientid, const HWAddrPtr& hwaddr, const isc::hooks::CalloutHandlePtr& callout_handle, diff --git a/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc index 6c6abdd3dc..622e5643be 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc @@ -616,6 +616,8 @@ TEST_F(AllocEngine4Test, simpleAlloc4) { IOAddress("0.0.0.0"), false, CalloutHandlePtr(), old_lease_); + // The new lease has been allocated, so the old lease should not exist. + EXPECT_FALSE(old_lease_); // Check that we got a lease ASSERT_TRUE(lease); @@ -642,6 +644,9 @@ TEST_F(AllocEngine4Test, fakeAlloc4) { CalloutHandlePtr(), old_lease_); + // The new lease has been allocated, so the old lease should not exist. + EXPECT_FALSE(old_lease_); + // Check that we got a lease ASSERT_TRUE(lease); @@ -665,10 +670,12 @@ TEST_F(AllocEngine4Test, allocWithValidHint4) { IOAddress("192.0.2.105"), false, CalloutHandlePtr(), old_lease_); - // Check that we got a lease ASSERT_TRUE(lease); + // We have allocated the new lease, so the old lease should not exist. + EXPECT_FALSE(old_lease_); + // We should get what we asked for EXPECT_EQ(lease->addr_.toText(), "192.0.2.105"); @@ -706,6 +713,10 @@ TEST_F(AllocEngine4Test, allocWithUsedHint4) { IOAddress("192.0.2.106"), false, CalloutHandlePtr(), old_lease_); + + // New lease has been allocated, so the old lease should not exist. + EXPECT_FALSE(old_lease_); + // Check that we got a lease ASSERT_TRUE(lease); @@ -744,6 +755,9 @@ TEST_F(AllocEngine4Test, allocBogusHint4) { // Check that we got a lease ASSERT_TRUE(lease); + // We have allocated a new lease, so the old lease should not exist. + EXPECT_FALSE(old_lease_); + // We should NOT get what we asked for, because it is used already EXPECT_TRUE(lease->addr_.toText() != "10.1.1.1"); @@ -777,6 +791,7 @@ TEST_F(AllocEngine4Test, allocateAddress4Nulls) { CalloutHandlePtr(), old_lease_); EXPECT_FALSE(lease); + EXPECT_FALSE(old_lease_); // Allocations without client-id are allowed clientid_ = ClientIdPtr(); @@ -786,6 +801,8 @@ TEST_F(AllocEngine4Test, allocateAddress4Nulls) { old_lease_); // Check that we got a lease ASSERT_TRUE(lease); + // New lease has been allocated, so the old lease should not exist. + EXPECT_FALSE(old_lease_); // Do all checks on the lease checkLease4(lease); @@ -893,6 +910,9 @@ TEST_F(AllocEngine4Test, smallPool4) { // Check that we got that single lease ASSERT_TRUE(lease); + // We have allocated new lease, so the old lease should not exist. + EXPECT_FALSE(old_lease_); + EXPECT_EQ("192.0.2.17", lease->addr_.toText()); // Do all checks on the lease @@ -940,6 +960,7 @@ TEST_F(AllocEngine4Test, outOfAddresses4) { CalloutHandlePtr(), old_lease_); EXPECT_FALSE(lease2); + EXPECT_FALSE(old_lease_); } // This test checks if an expired lease can be reused in DISCOVER (fake allocation) @@ -962,21 +983,33 @@ TEST_F(AllocEngine4Test, discoverReuseExpiredLease4) { uint8_t hwaddr2[] = { 0, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe}; uint8_t clientid2[] = { 8, 7, 6, 5, 4, 3, 2, 1 }; time_t now = time(NULL) - 500; // Allocated 500 seconds ago - Lease4Ptr lease(new Lease4(addr, clientid2, sizeof(clientid2), hwaddr2, sizeof(hwaddr2), + Lease4Ptr lease(new Lease4(addr, clientid2, sizeof(clientid2), + hwaddr2, sizeof(hwaddr2), 495, 100, 200, now, subnet_->getID())); + // Copy the lease, so as it can be compared with the old lease returned + // by the allocation engine. + Lease4 original_lease(*lease); + // Lease was assigned 500 seconds ago, but its valid lifetime is 495, so it // is expired already ASSERT_TRUE(lease->expired()); ASSERT_TRUE(LeaseMgrFactory::instance().addLease(lease)); // CASE 1: Asking for any address - lease = engine->allocateAddress4(subnet_, clientid_, hwaddr_, IOAddress("0.0.0.0"), + lease = engine->allocateAddress4(subnet_, clientid_, hwaddr_, + IOAddress("0.0.0.0"), true, CalloutHandlePtr(), old_lease_); // Check that we got that single lease ASSERT_TRUE(lease); EXPECT_EQ(addr.toText(), lease->addr_.toText()); + // We are reusing expired lease, the old (expired) instance should be + // returned. The returned instance should be the same as the original + // lease. + ASSERT_TRUE(old_lease_); + EXPECT_TRUE(original_lease == *old_lease_); + // Do all checks on the lease (if subnet-id, preferred/valid times are ok etc.) checkLease4(lease); @@ -987,6 +1020,11 @@ TEST_F(AllocEngine4Test, discoverReuseExpiredLease4) { // Check that we got that single lease ASSERT_TRUE(lease); EXPECT_EQ(addr.toText(), lease->addr_.toText()); + + // We are updating expired lease. The copy of the old lease should be + // returned and it should be equal to the original lease. + ASSERT_TRUE(old_lease_); + EXPECT_TRUE(*old_lease_ == original_lease); } // This test checks if an expired lease can be reused in REQUEST (actual allocation) @@ -1001,8 +1039,13 @@ TEST_F(AllocEngine4Test, requestReuseExpiredLease4) { uint8_t hwaddr2[] = { 0, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe}; uint8_t clientid2[] = { 8, 7, 6, 5, 4, 3, 2, 1 }; time_t now = time(NULL) - 500; // Allocated 500 seconds ago - Lease4Ptr lease(new Lease4(addr, clientid2, sizeof(clientid2), hwaddr2, sizeof(hwaddr2), - 495, 100, 200, now, subnet_->getID())); + Lease4Ptr lease(new Lease4(addr, clientid2, sizeof(clientid2), hwaddr2, + sizeof(hwaddr2), 495, 100, 200, now, + subnet_->getID())); + // Make a copy of the lease, so as we can comapre that with the old lease + // instance returned by the allocation engine. + Lease4 original_lease(*lease); + // Lease was assigned 500 seconds ago, but its valid lifetime is 495, so it // is expired already ASSERT_TRUE(lease->expired()); @@ -1024,6 +1067,11 @@ TEST_F(AllocEngine4Test, requestReuseExpiredLease4) { // Now check that the lease in LeaseMgr has the same parameters detailCompareLease(lease, from_mgr); + + // The allocation engine should return a copy of the old lease. This + // lease should be equal to the original lease. + ASSERT_TRUE(old_lease_); + EXPECT_TRUE(*old_lease_ == original_lease); } /// @todo write renewLease6