[#1409] Correct DNS handling when reusing expired leases

src/bin/dhcp6/dhcp6_srv.cc
    Dhcpv6Srv::createNameChangeRequests() - added logic to queue
    CHG_REMOVES for changed leases when new lease has no FQDN

src/lib/dhcpsrv/alloc_engine.cc
    AllocEngine::allocateUnreservedLeases6() - return the old lease
    as a changed lease, when reusing an expired lease

    AllocEngine::reclaimExpiredLease() - moved queue CHG_REMOVE to
    only occur when the DB is flagged for change.

    AllocEngine::reclaimLeaseInDatabase() - removed logic to
    clear lease FQDN data

src/lib/dhcpsrv/tests/alloc_engine6_unittest.cc
    TEST_F(AllocEngine6Test, requestReuseExpiredLease6) - verifies
    changed_leases instead of old_leases

diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc
index 95629d6fb66f109c32c2ead1d370f80d09aaadd8..ceb2a35a4aa75cf7500d4bb744e953822d45a11c 100644 (file)
--- a/src/bin/dhcp6/dhcp6_srv.cc
+++ b/src/bin/dhcp6/dhcp6_srv.cc
@@ -1847,7 +1847,15 @@ Dhcpv6Srv::createNameChangeRequests(const Pkt6Ptr& answer,
     // FQDN option will be NULL. This is valid state, so we simply return.
     Option6ClientFqdnPtr opt_fqdn = boost::dynamic_pointer_cast<
         Option6ClientFqdn>(answer->getOption(D6O_CLIENT_FQDN));
+
     if (!opt_fqdn) {
+        // This lease has no FQDN, remove DNS entries for any changed leases
+        // that have it.
+        for (Lease6Collection::const_iterator l = ctx.currentIA().changed_leases_.begin();
+                l != ctx.currentIA().changed_leases_.end(); ++l) {
+            queueNCR(CHG_REMOVE, *l);
+        }
+
         return;
     }
 

diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc
index 84c025a028181a5dc499f91a61ae2a8a9379e855..0bf792b165f755e39a07879f96b629841d438099 100644 (file)
--- a/src/lib/dhcpsrv/alloc_engine.cc
+++ b/src/lib/dhcpsrv/alloc_engine.cc
@@ -941,9 +941,10 @@ AllocEngine::allocateUnreservedLeases6(ClientContext6& ctx) {
             if (!host) {
 
                 // Copy an existing, expired lease so as it can be returned
-                // to the caller.
+                // to the caller as changed so caller can deal with DNS as
+                // needed.
                 Lease6Ptr old_lease(new Lease6(*lease));
-                ctx.currentIA().old_leases_.push_back(old_lease);
+                ctx.currentIA().changed_leases_.push_back(old_lease);
 
                 /// We found a lease and it is expired, so we can reuse it
                 lease = reuseExpiredLease(lease, ctx, pool->getLength(),
@@ -2530,7 +2531,6 @@ void
 AllocEngine::reclaimExpiredLease(const Lease6Ptr& lease,
                                  const DbReclaimMode& reclaim_mode,
                                  const CalloutHandlePtr& callout_handle) {
-
     LOG_DEBUG(alloc_engine_logger, ALLOC_ENGINE_DBG_TRACE,
               ALLOC_ENGINE_V6_LEASE_RECLAIM)
         .arg(Pkt6::makeLabel(lease->duid_, lease->hwaddr_))
@@ -2564,12 +2564,6 @@ AllocEngine::reclaimExpiredLease(const Lease6Ptr& lease,
     /// Not sure if we need to support every possible status everywhere.
 
     if (!skipped) {
-
-        // Generate removal name change request for D2, if required.
-        // This will return immediately if the DNS wasn't updated
-        // when the lease was created.
-        queueNCR(CHG_REMOVE, lease);
-
         // Let's check if the lease that just expired is in DECLINED state.
         // If it is, we need to perform a couple extra steps.
         bool remove_lease = (reclaim_mode == DB_RECLAIM_REMOVE);
@@ -2586,6 +2580,15 @@ AllocEngine::reclaimExpiredLease(const Lease6Ptr& lease,
         }
 
         if (reclaim_mode != DB_RECLAIM_LEAVE_UNCHANGED) {
+            // Generate removal name change request for D2, if required.
+            // We're doing this here because the lease either being updated
+            // or removed.  If the lease is being reused, higher level logic
+            // will decide what to do for DNS and we won't be in this block.
+            queueNCR(CHG_REMOVE, lease);
+            lease->hostname_.clear();
+            lease->fqdn_fwd_ = false;
+            lease->fqdn_rev_ = false;
+
             // Reclaim the lease - depending on the configuration, set the
             // expired-reclaimed state or simply remove it.
             LeaseMgr& lease_mgr = LeaseMgrFactory::instance();
@@ -2628,7 +2631,6 @@ void
 AllocEngine::reclaimExpiredLease(const Lease4Ptr& lease,
                                  const DbReclaimMode& reclaim_mode,
                                  const CalloutHandlePtr& callout_handle) {
-
     LOG_DEBUG(alloc_engine_logger, ALLOC_ENGINE_DBG_TRACE,
               ALLOC_ENGINE_V4_LEASE_RECLAIM)
         .arg(Pkt4::makeLabel(lease->hwaddr_, lease->client_id_))
@@ -2660,12 +2662,6 @@ AllocEngine::reclaimExpiredLease(const Lease4Ptr& lease,
     /// Not sure if we need to support every possible status everywhere.
 
     if (!skipped) {
-
-        // Generate removal name change request for D2, if required.
-        // This will return immediately if the DNS wasn't updated
-        // when the lease was created.
-        queueNCR(CHG_REMOVE, lease);
-
         // Let's check if the lease that just expired is in DECLINED state.
         // If it is, we need to perform a couple extra steps.
         bool remove_lease = (reclaim_mode == DB_RECLAIM_REMOVE);
@@ -2682,6 +2678,15 @@ AllocEngine::reclaimExpiredLease(const Lease4Ptr& lease,
         }
 
         if (reclaim_mode != DB_RECLAIM_LEAVE_UNCHANGED) {
+            // Generate removal name change request for D2, if required.
+            // We're doing this here because the lease either being updated
+            // or removed.  If the lease is being reused, higher level logic
+            // will decide what to do for DNS and we won't be in this block.
+            queueNCR(CHG_REMOVE, lease);
+            lease->hostname_.clear();
+            lease->fqdn_fwd_ = false;
+            lease->fqdn_rev_ = false;
+
             // Reclaim the lease - depending on the configuration, set the
             // expired-reclaimed state or simply remove it.
             LeaseMgr& lease_mgr = LeaseMgrFactory::instance();
@@ -2853,14 +2858,8 @@ void AllocEngine::reclaimLeaseInDatabase(const LeasePtrType& lease,
     if (remove_lease) {
         lease_mgr.deleteLease(lease);
     } else if (lease_update_fun) {
-        // Clear FQDN information as we have already sent the
-        // name change request to remove the DNS record.
-        lease->hostname_.clear();
-        lease->fqdn_fwd_ = false;
-        lease->fqdn_rev_ = false;
         lease->state_ = Lease::STATE_EXPIRED_RECLAIMED;
         lease_update_fun(lease);
-
     } else {
         return;
     }

diff --git a/src/lib/dhcpsrv/tests/alloc_engine6_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine6_unittest.cc
index 34c316a50fef44f39f4ee2e18fa36089887d0022..cf1481915635d3c51289540dddfa8f7e8ec6f0ce 100644 (file)
--- a/src/lib/dhcpsrv/tests/alloc_engine6_unittest.cc
+++ b/src/lib/dhcpsrv/tests/alloc_engine6_unittest.cc
@@ -1203,14 +1203,15 @@ TEST_F(AllocEngine6Test, requestReuseExpiredLease6) {
     EXPECT_FALSE(lease->fqdn_fwd_);
     EXPECT_FALSE(lease->fqdn_rev_);
 
-    // Check that the old lease has been returned.
-    Lease6Ptr old_lease = expectOneLease(ctx.currentIA().old_leases_);
+    // Check that the changed lease has been returned.
+    Lease6Ptr changed_lease = expectOneLease(ctx.currentIA().changed_leases_);
+    ASSERT_TRUE(changed_lease);
     // It should at least have the same IPv6 address.
-    EXPECT_EQ(lease->addr_, old_lease->addr_);
+    EXPECT_EQ(lease->addr_, changed_lease->addr_);
     // Check that it carries not updated FQDN data.
-    EXPECT_EQ("myhost.example.com.", old_lease->hostname_);
-    EXPECT_TRUE(old_lease->fqdn_fwd_);
-    EXPECT_TRUE(old_lease->fqdn_rev_);
+    EXPECT_EQ("myhost.example.com.", changed_lease->hostname_);
+    EXPECT_TRUE(changed_lease->fqdn_fwd_);
+    EXPECT_TRUE(changed_lease->fqdn_rev_);
 
     // Check that the lease is indeed updated in LeaseMgr
     Lease6Ptr from_mgr = LeaseMgrFactory::instance().getLease6(Lease::TYPE_NA,