From: Thomas Markwalder Date: Fri, 4 Sep 2020 17:42:23 +0000 (-0400) Subject: [#1409] Correct DNS handling when reusing expired leases X-Git-Tag: Kea-1.9.0~167 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=d6b1297831c4758da3db22673c8019ce2bf69795;p=thirdparty%2Fkea.git [#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 95629d6fb6..ceb2a35a4a 100644 --- 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 84c025a028..0bf792b165 100644 --- 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 34c316a50f..cf14819156 100644 --- 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,