[#3257] Suppress NCRs when reusing leases

/src/bin/dhcp4/dhcp4_srv.cc
    Dhcpv4Srv::createNameChangeRequests() - modified to check for
    reused lease

/src/bin/dhcp4/tests/dora_unittest.cc
    DORATest::leaseCaching() - modified to verify suppression of NCRs

/src/bin/dhcp6/dhcp6_srv.cc
    Dhcpv6Srv::createNameChangeRequests() - modified to iteralte over
    IA contexts rather than IA options in response and to check for
    reused leases

/src/bin/dhcp6/tests/fqdn_unittest.cc
    Updated tests to populate IAContexts

/src/bin/dhcp6/tests/sarr_unittest.cc
    SARRTest::leaseCaching()- modified to verify suppression of NCRs

/src/lib/dhcpsrv/alloc_engine.*
    AllocEngine::ClientContext6::IAContext - added reused_leases_ container
    AllocEngine::ClientContext6::getIAContexts() - new function

ChangeLog

@@ -1,3 +1,9 @@
+2303.	[bug]		tmark
+	Modified both kea-dhcp4 and kea-dhcp6 to avoid
+	generating DDNS update requests when leases are
+	being reused due to lease caching.
+	(Gitlab #3257)
+	
 Kea 2.7.4 (development) released on October 30, 2024
 
 2302.	[func]		tmark

src/bin/dhcp4/dhcp4_srv.cc

@@ -2893,7 +2893,8 @@ Dhcpv4Srv::createNameChangeRequests(const Lease4Ptr& lease,
         return;
     }
 
-    if (!old_lease || ddns_params.getUpdateOnRenew() || !lease->hasIdenticalFqdn(*old_lease)) {
+    if ((!lease->reuseable_valid_lft_) &&
+        (!old_lease || ddns_params.getUpdateOnRenew() || !lease->hasIdenticalFqdn(*old_lease))) {
         if (old_lease) {
             // Queue's up a remove of the old lease's DNS (if needed)
             queueNCR(CHG_REMOVE, old_lease);

src/bin/dhcp4/tests/dora_unittest.cc

@@ -633,6 +633,11 @@ const char* DORA_CONFIGS[] = {
         "interfaces-config": {
             "interfaces": [ "*" ]
         },
+        "dhcp-ddns": {
+            "enable-updates": true
+        },
+        "ddns-send-updates": true,
+        "ddns-update-on-renew": true,
         "subnet4": [
             {
                 "id": 1,
@@ -3049,10 +3054,18 @@ DORATest::leaseCaching() {
     // Configure a DHCP client.
     Dhcp4Client client;
     client.includeClientId("11:22");
-
+    // Include the Client FQDN option.
+    ASSERT_NO_THROW(client.includeFQDN((Option4ClientFqdn::FLAG_S
+                                       | Option4ClientFqdn::FLAG_E),
+                                       "first.example.com.",
+                                       Option4ClientFqdn::FULL));
     // Configure a DHCP server.
     configure(DORA_CONFIGS[18], *client.getServer());
 
+    ASSERT_NO_THROW(client.getServer()->startD2());
+    auto& d2_mgr = CfgMgr::instance().getD2ClientMgr();
+    ASSERT_EQ(0, d2_mgr.getQueueSize());
+
     // Obtain a lease from the server using the 4-way exchange.
     ASSERT_NO_THROW(client.doDORA());
 
@@ -3078,6 +3091,12 @@ DORATest::leaseCaching() {
     // There should only be the default statistic point in the other subnet.
     checkStat("subnet[2].v4-lease-reuses", 1, 0);
 
+    // There should be a single NCR.
+    ASSERT_EQ(1, d2_mgr.getQueueSize());
+    // Clear the NCR queue.
+    ASSERT_NO_THROW(d2_mgr.runReadyIO());
+    ASSERT_EQ(0, d2_mgr.getQueueSize());
+
     // Assume the client enters init-reboot and does a request.
     client.setState(Dhcp4Client::INIT_REBOOT);
     ASSERT_NO_THROW(client.doRequest());
@@ -3104,6 +3123,9 @@ DORATest::leaseCaching() {
     // Statistics for the other subnet should not be affected.
     checkStat("subnet[2].v4-lease-reuses", 1, 0);
 
+    // There should be no NCRs queued.
+    ASSERT_EQ(0, d2_mgr.getQueueSize());
+
     // Let's say the client suddenly decides to do a full DORA.
     ASSERT_NO_THROW(client.doDORA());
 
@@ -3129,6 +3151,9 @@ DORATest::leaseCaching() {
     // Statistics for the other subnet should not be affected.
     checkStat("subnet[2].v4-lease-reuses", 1, 0);
 
+    // There should be no NCRs queued.
+    ASSERT_EQ(0, d2_mgr.getQueueSize());
+
     // Try to request a different address than the client has. The server
     // should respond with DHCPNAK.
     client.config_.lease_.addr_ = IOAddress("10.0.0.30");
@@ -3153,6 +3178,9 @@ DORATest::leaseCaching() {
 
     // Statistics for the other subnet should certainly not be affected.
     checkStat("subnet[2].v4-lease-reuses", 1, 0);
+
+    // There should be no NCRs queued.
+    ASSERT_EQ(0, d2_mgr.getQueueSize());
 }
 
 TEST_F(DORATest, leaseCaching) {

src/bin/dhcp6/dhcp6_srv.cc

@@ -2439,13 +2439,17 @@ Dhcpv6Srv::createNameChangeRequests(const Pkt6Ptr& answer,
     // Compute DHCID from Client Identifier and FQDN.
     isc::dhcp_ddns::D2Dhcid dhcid(*duid, buf_vec);
 
-    // Get all IAs from the answer. For each IA, holding an address we will
+    // Iterate over the IAContexts (there should be one per client IA processed).
-    // create a corresponding NameChangeRequest.
+    // For the first address in each IA_NA, create the appropriate NCR(s).
-    for (auto const& answer_ia : answer->getOptions(D6O_IA_NA)) {
+    for (auto& ia_ctx : ctx.getIAContexts()) {
+        if ((ia_ctx.type_ != Lease::TYPE_NA) || !ia_ctx.ia_rsp_) {
+            continue;
+        }
+
         /// @todo IA_NA may contain multiple addresses. We should process
-        /// each address individually. Currently we get only one.
+        /// each address individually. Currently we only process the first one.
-        Option6IAAddrPtr iaaddr = boost::static_pointer_cast<
+        Option6IAAddrPtr iaaddr = boost::static_pointer_cast
-            Option6IAAddr>(answer_ia.second->getOption(D6O_IAADDR));
+                                  <Option6IAAddr>(ia_ctx.ia_rsp_->getOption(D6O_IAADDR));
 
         // We need an address to create a name-to-address mapping.
         // If address is missing for any reason, go to the next IA.
@@ -2453,19 +2457,33 @@ Dhcpv6Srv::createNameChangeRequests(const Pkt6Ptr& answer,
             continue;
         }
 
+        bool extended_only = false;
+        // If the lease is being reused (i.e. lease caching in effect) skip it.
+        IOAddress ia_address = iaaddr->getAddress();
+        for (auto const& l : ia_ctx.reused_leases_) {
+            if (l->addr_ == ia_address) {
+                extended_only = true;
+                break;
+            }
+        }
+
+        if (extended_only) {
+            continue;
+        }
+
         // If the lease for iaaddr is in the list of changed leases, we need
         // to determine if the changes included changes to the FQDN. If so
         // then we may need to do a CHG_REMOVE.
-        bool extended_only = false;
+        for (auto const& l : ia_ctx.changed_leases_) {
-        for (auto const& l : ctx.currentIA().changed_leases_) {
 
-            if (l->addr_ == iaaddr->getAddress()) {
+            if (l->addr_ == ia_address) {
                 // The address is the same so this must be renewal. If we're not
                 // always updating on renew, then we only renew if DNS info has
                 // changed.
-                if (!ctx.getDdnsParams()->getUpdateOnRenew() &&
+                if (l->reuseable_valid_lft_ ||
+                    (!ctx.getDdnsParams()->getUpdateOnRenew() &&
                     (l->hostname_ == opt_fqdn->getDomainName() &&
-                     l->fqdn_fwd_ == do_fwd && l->fqdn_rev_ == do_rev)) {
+                     l->fqdn_fwd_ == do_fwd && l->fqdn_rev_ == do_rev))) {
                     extended_only = true;
                 } else {
                     // Queue a CHG_REMOVE of the old data.
@@ -2630,6 +2648,7 @@ Dhcpv6Srv::assignIA_NA(const Pkt6Ptr& query,
     // Do not use OptionDefinition to create option's instance so
     // as we can initialize IAID using a constructor.
     Option6IAPtr ia_rsp(new Option6IA(D6O_IA_NA, ia->getIAID()));
+    ctx.currentIA().ia_rsp_ = ia_rsp;
 
     if (lease) {
         // We have a lease! Let's wrap its content into IA_NA option
@@ -2648,6 +2667,8 @@ Dhcpv6Srv::assignIA_NA(const Pkt6Ptr& query,
         } else {
             lease->valid_lft_ = lease->reuseable_valid_lft_;
             lease->preferred_lft_ = lease->reuseable_preferred_lft_;
+            std::cout << __LINE__ << " flagged as resuseable: " << lease->addr_.toText() << std::endl;
+            ctx.currentIA().reused_leases_.push_back(lease);
             LOG_INFO(lease6_logger, DHCP6_LEASE_REUSE)
                 .arg(query->getLabel())
                 .arg(lease->addr_.toText())
@@ -2751,6 +2772,7 @@ Dhcpv6Srv::assignIA_PD(const Pkt6Ptr& query,
         ctx.currentIA().addHint(hint, 0);
     }
     ctx.currentIA().type_ = Lease::TYPE_PD;
+    ctx.currentIA().ia_rsp_ = ia_rsp;
 
     // Use allocation engine to pick a lease for this client. Allocation engine
     // will try to honor the hint, but it is just a hint - some other address

src/bin/dhcp6/tests/fqdn_unittest.cc

@@ -255,12 +255,17 @@ public:
     ///
     /// @param iaid IAID
     /// @param pkt A DHCPv6 message to which the IA option should be added.
-    void addIA(const uint32_t iaid, const IOAddress& addr, Pkt6Ptr& pkt) {
+    /// @param cxt allocation engine context to which IA option should be 
+    /// added. 
+    void addIA(const uint32_t iaid, const IOAddress& addr, Pkt6Ptr& pkt,
+               AllocEngine::ClientContext6& ctx) {
         Option6IAPtr opt_ia = generateIA(D6O_IA_NA, iaid, 1500, 3000);
         Option6IAAddrPtr opt_iaaddr(new Option6IAAddr(D6O_IAADDR, addr,
                                                       300, 500));
         opt_ia->addOption(opt_iaaddr);
         pkt->addOption(opt_ia);
+        ctx.createIAContext();
+        ctx.currentIA().ia_rsp_ = opt_ia;
     }
 
     /// @brief Adds IA option to the message.
@@ -270,10 +275,15 @@ public:
     /// @param iaid IAID
     /// @param status_code Status code
     /// @param pkt A DHCPv6 message to which the option should be added.
-    void addIA(const uint32_t iaid, const uint16_t status_code, Pkt6Ptr& pkt) {
+    /// @param cxt allocation engine context to which IA option should be 
+    /// added. 
+    void addIA(const uint32_t iaid, const uint16_t status_code, Pkt6Ptr& pkt,
+               AllocEngine::ClientContext6& ctx) {
         Option6IAPtr opt_ia = generateIA(D6O_IA_NA, iaid, 1500, 3000);
         addStatusCode(status_code, "", opt_ia);
         pkt->addOption(opt_ia);
+        ctx.createIAContext();
+        ctx.currentIA().ia_rsp_ = opt_ia;
     }
 
     /// @brief Creates status code with the specified code and message.
@@ -347,13 +357,13 @@ public:
                                                 ? DHCPV6_ADVERTISE :
                                                 DHCPV6_REPLY);
 
-        // Create three IAs, each having different address.
-        addIA(1234, IOAddress("2001:db8:1::1"), answer);
-
         AllocEngine::ClientContext6 ctx;
         // Set the selected subnet so ddns params get returned correctly.
         ctx.subnet_ = subnet_;
 
+        // Create three IAs, each having different address.
+        addIA(1234, IOAddress("2001:db8:1::1"), answer, ctx);
+
         ASSERT_NO_THROW(srv_->processClientFqdn(question, answer, ctx));
         Option6ClientFqdnPtr answ_fqdn = boost::dynamic_pointer_cast<
             Option6ClientFqdn>(answer->getOption(D6O_CLIENT_FQDN));
@@ -823,10 +833,15 @@ TEST_F(FqdnDhcpv6SrvTest, createNameChangeRequests) {
     // Create Reply message with Client Id and Server id.
     Pkt6Ptr answer = generateMessageWithIds(DHCPV6_REPLY);
 
+    // Create NameChangeRequest for the first allocated address.
+    AllocEngine::ClientContext6 ctx;
+    ctx.subnet_ = subnet_;
+    ctx.fwd_dns_update_ = ctx.rev_dns_update_ = true;
+
     // Create three IAs, each having different address.
-    addIA(1234, IOAddress("2001:db8:1::1"), answer);
+    addIA(1234, IOAddress("2001:db8:1::1"), answer, ctx);
-    addIA(2345, IOAddress("2001:db8:1::2"), answer);
+    addIA(2345, IOAddress("2001:db8:1::2"), answer, ctx);
-    addIA(3456, IOAddress("2001:db8:1::3"), answer);
+    addIA(3456, IOAddress("2001:db8:1::3"), answer, ctx);
 
     // Use domain name in upper case. It should be converted to lower-case
     // before DHCID is calculated. So, we should get the same result as if
@@ -836,10 +851,7 @@ TEST_F(FqdnDhcpv6SrvTest, createNameChangeRequests) {
                                                  Option6ClientFqdn::FULL);
     answer->addOption(fqdn);
 
-    // Create NameChangeRequest for the first allocated address.
+
-    AllocEngine::ClientContext6 ctx;
-    ctx.subnet_ = subnet_;
-    ctx.fwd_dns_update_ = ctx.rev_dns_update_ = true;
     ASSERT_NO_THROW(srv_->createNameChangeRequests(answer, ctx));
     ASSERT_EQ(1, d2_mgr_.getQueueSize());
 
@@ -857,8 +869,14 @@ TEST_F(FqdnDhcpv6SrvTest, noConflictResolution) {
     // Create Reply message with Client Id and Server id.
     Pkt6Ptr answer = generateMessageWithIds(DHCPV6_REPLY);
 
+    // Create NameChangeRequest for the first allocated address.
+    AllocEngine::ClientContext6 ctx;
+    subnet_->setDdnsConflictResolutionMode("no-check-with-dhcid");
+    ctx.subnet_ = subnet_;
+    ctx.fwd_dns_update_ = ctx.rev_dns_update_ = true;
+
     // Create three IAs, each having different address.
-    addIA(1234, IOAddress("2001:db8:1::1"), answer);
+    addIA(1234, IOAddress("2001:db8:1::1"), answer, ctx);
 
     // Use domain name in upper case. It should be converted to lower-case
     // before DHCID is calculated. So, we should get the same result as if
@@ -868,11 +886,6 @@ TEST_F(FqdnDhcpv6SrvTest, noConflictResolution) {
                                                  Option6ClientFqdn::FULL);
     answer->addOption(fqdn);
 
-    // Create NameChangeRequest for the first allocated address.
-    AllocEngine::ClientContext6 ctx;
-    subnet_->setDdnsConflictResolutionMode("no-check-with-dhcid");
-    ctx.subnet_ = subnet_;
-    ctx.fwd_dns_update_ = ctx.rev_dns_update_ = true;
     ASSERT_NO_THROW(srv_->createNameChangeRequests(answer, ctx));
     ASSERT_EQ(1, d2_mgr_.getQueueSize());
 
@@ -893,8 +906,13 @@ TEST_F(FqdnDhcpv6SrvTest, noAddRequestsWhenDisabled) {
     // Create Reply message with Client Id and Server id.
     Pkt6Ptr answer = generateMessageWithIds(DHCPV6_REPLY);
 
+    // An attempt to send a NCR would throw.
+    AllocEngine::ClientContext6 ctx;
+    ctx.subnet_ = subnet_;
+    ctx.fwd_dns_update_ = ctx.rev_dns_update_ = true;
+
     // Create three IAs, each having different address.
-    addIA(1234, IOAddress("2001:db8:1::1"), answer);
+    addIA(1234, IOAddress("2001:db8:1::1"), answer, ctx);
 
     // Use domain name in upper case. It should be converted to lower-case
     // before DHCID is calculated. So, we should get the same result as if
@@ -904,10 +922,6 @@ TEST_F(FqdnDhcpv6SrvTest, noAddRequestsWhenDisabled) {
                                                  Option6ClientFqdn::FULL);
     answer->addOption(fqdn);
 
-    // An attempt to send a NCR would throw.
-    AllocEngine::ClientContext6 ctx;
-    ctx.subnet_ = subnet_;
-    ctx.fwd_dns_update_ = ctx.rev_dns_update_ = true;
     ASSERT_NO_THROW(srv_->createNameChangeRequests(answer, ctx));
 }
 
@@ -2153,8 +2167,15 @@ TEST_F(FqdnDhcpv6SrvTest, ddnsTtlPercent) {
     // Create Reply message with Client Id and Server id.
     Pkt6Ptr answer = generateMessageWithIds(DHCPV6_REPLY);
 
+    // Create NameChangeRequest for the first allocated address.
+    AllocEngine::ClientContext6 ctx;
+    subnet_->setDdnsConflictResolutionMode("no-check-with-dhcid");
+    subnet_->setDdnsTtlPercent(Optional<double>(0.10));
+    ctx.subnet_ = subnet_;
+    ctx.fwd_dns_update_ = ctx.rev_dns_update_ = true;
+
     // Create an IA.
-    addIA(1234, IOAddress("2001:db8:1::1"), answer);
+    addIA(1234, IOAddress("2001:db8:1::1"), answer, ctx);
 
     // Use domain name in upper case. It should be converted to lower-case
     // before DHCID is calculated. So, we should get the same result as if
@@ -2164,12 +2185,6 @@ TEST_F(FqdnDhcpv6SrvTest, ddnsTtlPercent) {
                                                  Option6ClientFqdn::FULL);
     answer->addOption(fqdn);
 
-    // Create NameChangeRequest for the first allocated address.
-    AllocEngine::ClientContext6 ctx;
-    subnet_->setDdnsConflictResolutionMode("no-check-with-dhcid");
-    subnet_->setDdnsTtlPercent(Optional<double>(0.10));
-    ctx.subnet_ = subnet_;
-    ctx.fwd_dns_update_ = ctx.rev_dns_update_ = true;
     ASSERT_NO_THROW(srv_->createNameChangeRequests(answer, ctx));
     ASSERT_EQ(1, d2_mgr_.getQueueSize());
 

src/bin/dhcp6/tests/sarr_unittest.cc

@@ -313,6 +313,11 @@ const char* CONFIGS[] = {
         "interfaces-config": {
             "interfaces": [ "*" ]
         },
+        "dhcp-ddns": {
+            "enable-updates": true
+        },
+        "ddns-send-updates": true,
+        "ddns-update-on-renew": true,
         "subnet6": [
             {
                 "id": 1,
@@ -1367,6 +1372,16 @@ SARRTest::leaseCaching() {
     ASSERT_NO_THROW(client.requestAddress(1234, asiolink::IOAddress("2001:db8::10")));
     ASSERT_NO_THROW(client.requestPrefix(5678, 32, asiolink::IOAddress("2001:db8:1::")));
 
+    // Include FQDN to trigger generation of name change requests.
+    ASSERT_NO_THROW(client.useFQDN(Option6ClientFqdn::FLAG_S,
+                                   "client-name.example.org",
+                                   Option6ClientFqdn::FULL));
+
+    // Start D2 client mgr and verify the NCR queue is empty.
+    ASSERT_NO_THROW(client.getServer()->startD2());
+    auto& d2_mgr = CfgMgr::instance().getD2ClientMgr();
+    ASSERT_EQ(0, d2_mgr.getQueueSize());
+
     // Perform 4-way exchange.
     ASSERT_NO_THROW(client.doSARR());
 
@@ -1393,6 +1408,12 @@ SARRTest::leaseCaching() {
     checkStat("subnet[1].v6-ia-pd-lease-reuses", 1, 0);
     checkStat("subnet[2].v6-ia-pd-lease-reuses", 1, 0);
 
+    // There should be a single NCR.
+    ASSERT_EQ(1, d2_mgr.getQueueSize());
+    // Clear the NCR queue.
+    ASSERT_NO_THROW(d2_mgr.runReadyIO());
+    ASSERT_EQ(0, d2_mgr.getQueueSize());
+
     // Request the same prefix with a different length. The server should
     // return an existing lease.
     client.clearRequestedIAs();
@@ -1415,6 +1436,9 @@ SARRTest::leaseCaching() {
     checkStat("subnet[1].v6-ia-pd-lease-reuses", 2, 1);
     checkStat("subnet[2].v6-ia-pd-lease-reuses", 1, 0);
 
+    // There should be no NCRs queued.
+    ASSERT_EQ(0, d2_mgr.getQueueSize());
+
     // Try to request another prefix. The client should still get the existing
     // lease.
     client.clearRequestedIAs();
@@ -1436,6 +1460,9 @@ SARRTest::leaseCaching() {
     checkStat("v6-ia-pd-lease-reuses", 3, 2);
     checkStat("subnet[1].v6-ia-pd-lease-reuses", 3, 2);
     checkStat("subnet[2].v6-ia-pd-lease-reuses", 1, 0);
+
+    // There should be no NCRs queued.
+    ASSERT_EQ(0, d2_mgr.getQueueSize());
 }
 
 TEST_F(SARRTest, leaseCaching) {

src/lib/dhcpsrv/alloc_engine.cc

@@ -224,7 +224,7 @@ AllocEngine::ClientContext6::ClientContext6(const ConstSubnet6Ptr& subnet,
 
 AllocEngine::ClientContext6::IAContext::IAContext()
     : iaid_(0), type_(Lease::TYPE_NA), hints_(), old_leases_(),
-      changed_leases_(), new_resources_(), ia_rsp_() {
+      changed_leases_(),reused_leases_(), new_resources_(), ia_rsp_() {
 }
 
 void
@@ -2371,6 +2371,10 @@ AllocEngine::extendLease6(ClientContext6& ctx, Lease6Ptr lease) {
                 lease->pool_id_ = pool->getID();
             }
             LeaseMgrFactory::instance().updateLease6(lease);
+        } else {
+           // Server looks at changed_leases_ (i.e. old_data) when deciding
+           // on DNS updates etc.
+           old_data->reuseable_valid_lft_ = lease->reuseable_valid_lft_;
         }
 
         if (update_stats) {
@@ -2465,9 +2469,13 @@ AllocEngine::updateLeaseData(ClientContext6& ctx, const Lease6Collection& leases
             if (!fqdn_changed) {
                 setLeaseReusable(lease, current_preferred_lft, ctx);
             }
+
             if (lease->reuseable_valid_lft_ == 0) {
                 ctx.currentIA().changed_leases_.push_back(lease_it);
                 LeaseMgrFactory::instance().updateLease6(lease);
+            } else {
+                // Server needs to know about resused leases to avoid DNS updates.
+                ctx.currentIA().reused_leases_.push_back(lease_it);
             }
 
             if (update_stats) {

src/lib/dhcpsrv/alloc_engine.h

@@ -322,6 +322,9 @@ public:
             /// FQDN has changed.
             Lease6Collection changed_leases_;
 
+            /// @brief Set of leases marked for reuse by lease caching
+            Lease6Collection reused_leases_;
+
             /// @brief Holds addresses and prefixes allocated for this IA.
             ///
             /// This collection is used to update at most once new leases.
@@ -424,6 +427,10 @@ public:
             return (ias_.back());
         }
 
+        std::vector<IAContext>& getIAContexts() {
+            return (ias_);
+        }
+
         /// @brief Creates new IA context.
         ///
         /// This method should be invoked prior to processing a next IA included