[5007] Suppress DDNS updates on DHCPv6 lease renewals unless the FQDN changes

src/lib/dhcpsrv/alloc_engine.h
src/lib/dhcpsrv/alloc_engine.cc
    - AllocEngine::extendLease6()
    - AllocEngine::updateLeaseData() - logic was added to clear the context
    DNS update flags when the renewal does not alter the lease's FQDN.

src/bin/dhcp6/dhcp6_srv.h
src/bin/dhcp6/dhcp6_srv.cc
    - Dhcpv6Srv::createNameChangeRequests() - added context as second parameter,
    and modified function to return without creating NCR(s) if both update flags
    in the context are false.

src/bin/dhcp6/tests/fqdn_unittest.cc
    - TEST_F(FqdnDhcpv6SrvTest, createNameChangeRequests) - modified to verify
    combinations of context update flags

    - TEST_F(FqdnDhcpv6SrvTest, DISABLED_processTwoRequests) - renamed
    to TEST_F(FqdnDhcpv6SrvTest, processTwoRequestsDiffFqdn) and enabled.
    It had been disabled pending 3677 which has been completed.

    - TEST_F(FqdnDhcpv6SrvTest, processTwoRequestsSameFqdn)  - new test
    which verifies that client "renewing" a lease by sending a second
    request with the different FQDN generates the correct NCRs

    - TEST_F(FqdnDhcpv6SrvTest, DISABLED_processRequestRenew) - renamed
    to TEST_F(FqdnDhcpv6SrvTest, processRequestRenewDiffFqdn) and enabled.
    It had been disabled pending 3677 which has been completed.

    - TEST_F(FqdnDhcpv6SrvTest, processRequestRenewSameFqdn) - new test
    which verifies that client renewing a lease by sending a renew
    with the same FQDN does NOT generate any NCRs

src/bin/dhcp6/dhcp6_srv.cc

@@ -1196,9 +1196,15 @@ Dhcpv6Srv::processClientFqdn(const Pkt6Ptr& question, const Pkt6Ptr& answer,
 }
 
 void
-Dhcpv6Srv::createNameChangeRequests(const Pkt6Ptr& answer) {
-    // Don't create NameChangeRequests if DNS updates are disabled.
-    if (!CfgMgr::instance().ddnsEnabled()) {
+Dhcpv6Srv::createNameChangeRequests(const Pkt6Ptr& answer,
+                                    AllocEngine::ClientContext6& ctx) {
+    // Don't create NameChangeRequests if DNS updates are disabled
+    // or if no requests are actually required.  The context flags may be
+    // different than the lease flags. Primarily when existing leases
+    // are being extended without FQDN changes in which case the context
+    // flags will both be false.
+    if ((!CfgMgr::instance().ddnsEnabled()) ||
+       (!ctx.fwd_dns_update_ && !ctx.rev_dns_update_)) {
         return;
     }
 
@@ -2304,7 +2310,7 @@ Dhcpv6Srv::processSolicit(const Pkt6Ptr& solicit) {
     // Only generate name change requests if sending a Reply as a result
     // of receiving Rapid Commit option.
     if (response->getType() == DHCPV6_REPLY) {
-        createNameChangeRequests(response);
+        createNameChangeRequests(response, ctx);
     }
 
     return (response);
@@ -2331,7 +2337,7 @@ Dhcpv6Srv::processRequest(const Pkt6Ptr& request) {
     processClientFqdn(request, reply, ctx);
     assignLeases(request, reply, ctx);
     generateFqdn(reply);
-    createNameChangeRequests(reply);
+    createNameChangeRequests(reply, ctx);
 
     return (reply);
 }
@@ -2357,7 +2363,7 @@ Dhcpv6Srv::processRenew(const Pkt6Ptr& renew) {
     processClientFqdn(renew, reply, ctx);
     extendLeases(renew, reply, ctx);
     generateFqdn(reply);
-    createNameChangeRequests(reply);
+    createNameChangeRequests(reply, ctx);
 
     return (reply);
 }
@@ -2383,7 +2389,7 @@ Dhcpv6Srv::processRebind(const Pkt6Ptr& rebind) {
     processClientFqdn(rebind, reply, ctx);
     extendLeases(rebind, reply, ctx);
     generateFqdn(reply);
-    createNameChangeRequests(rebind);
+    createNameChangeRequests(reply, ctx);
 
     return (reply);
 }

src/bin/dhcp6/dhcp6_srv.h

@@ -567,8 +567,10 @@ protected:
     /// @todo Add support for multiple IAADDR options in the IA_NA.
     ///
     /// @param answer A message begins sent to the Client. If it holds the
+    /// @param ctx client context (contains subnet, duid and other parameters)
     /// Client FQDN option, this option is used to create NameChangeRequests.
-    void createNameChangeRequests(const Pkt6Ptr& answer);
+    void createNameChangeRequests(const Pkt6Ptr& answer,
+                                  AllocEngine::ClientContext6& ctx);
 
     /// @brief Attempts to extend the lifetime of IAs.
     ///

src/bin/dhcp6/tests/fqdn_unittest.cc

@@ -364,8 +364,12 @@ public:
         }
 
         if (create_ncr_check) {
+            // Context flags are normally set during lease allocation. Since that
+            // hasn't occurred we'll set them here to match the expected values.
             // Call createNameChangeRequests
-            ASSERT_NO_THROW(srv_->createNameChangeRequests(answer));
+            ctx.fwd_dns_update_ =  exp_fwd.value_;
+            ctx.rev_dns_update_ = exp_rev.value_;
+            ASSERT_NO_THROW(srv_->createNameChangeRequests(answer, ctx));
             if (exp_fwd.value_ || exp_rev.value_) {
                 // Should have created 1 NCR.
                 NameChangeRequestPtr ncr;
@@ -697,7 +701,9 @@ TEST_F(FqdnDhcpv6SrvTest, clientAAAAUpdateNotAllowed) {
 TEST_F(FqdnDhcpv6SrvTest, createNameChangeRequestsNoAnswer) {
     Pkt6Ptr answer;
 
-    EXPECT_THROW(srv_->createNameChangeRequests(answer),
+    AllocEngine::ClientContext6 ctx;
+    ctx.fwd_dns_update_ = ctx.rev_dns_update_ = true;
+    EXPECT_THROW(srv_->createNameChangeRequests(answer, ctx),
                  isc::Unexpected);
 
 }
@@ -711,7 +717,9 @@ TEST_F(FqdnDhcpv6SrvTest, createNameChangeRequestsNoDUID) {
                                                  Option6ClientFqdn::FULL);
     answer->addOption(fqdn);
 
-    EXPECT_THROW(srv_->createNameChangeRequests(answer), isc::Unexpected);
+    AllocEngine::ClientContext6 ctx;
+    ctx.fwd_dns_update_ = ctx.rev_dns_update_ = true;
+    EXPECT_THROW(srv_->createNameChangeRequests(answer, ctx), isc::Unexpected);
 
 }
 
@@ -721,7 +729,9 @@ TEST_F(FqdnDhcpv6SrvTest, createNameChangeRequestsNoFQDN) {
     // Create Reply message with Client Id and Server id.
     Pkt6Ptr answer = generateMessageWithIds(DHCPV6_REPLY);
 
-    ASSERT_NO_THROW(srv_->createNameChangeRequests(answer));
+    AllocEngine::ClientContext6 ctx;
+    ctx.fwd_dns_update_ = ctx.rev_dns_update_ = true;
+    ASSERT_NO_THROW(srv_->createNameChangeRequests(answer, ctx));
 
     // There should be no new NameChangeRequests.
     ASSERT_EQ(0, d2_mgr_.getQueueSize());
@@ -738,8 +748,9 @@ TEST_F(FqdnDhcpv6SrvTest, createNameChangeRequestsNoAddr) {
                                                  "myhost.example.com",
                                                  Option6ClientFqdn::FULL);
     answer->addOption(fqdn);
-
-    ASSERT_NO_THROW(srv_->createNameChangeRequests(answer));
+    AllocEngine::ClientContext6 ctx;
+    ctx.fwd_dns_update_ = ctx.rev_dns_update_ = true;
+    ASSERT_NO_THROW(srv_->createNameChangeRequests(answer, ctx));
 
     // We didn't add any IAs, so there should be no NameChangeRequests in the
     // queue.
@@ -747,7 +758,9 @@ TEST_F(FqdnDhcpv6SrvTest, createNameChangeRequestsNoAddr) {
 }
 
 // Test that exactly one NameChangeRequest is created as a result of processing
-// the answer message which holds 3 IAs and when FQDN is specified.
+// the answer message which holds 3 IAs and when FQDN is specified.  We also
+// verify that the context flags, fwd_dns_update_ and rev_dns_update_, gate
+// whether or not a NameChangeRequest is created.
 TEST_F(FqdnDhcpv6SrvTest, createNameChangeRequests) {
     // Create Reply message with Client Id and Server id.
     Pkt6Ptr answer = generateMessageWithIds(DHCPV6_REPLY);
@@ -766,7 +779,9 @@ TEST_F(FqdnDhcpv6SrvTest, createNameChangeRequests) {
     answer->addOption(fqdn);
 
     // Create NameChangeRequest for the first allocated address.
-    ASSERT_NO_THROW(srv_->createNameChangeRequests(answer));
+    AllocEngine::ClientContext6 ctx;
+    ctx.fwd_dns_update_ = ctx.rev_dns_update_ = true;
+    ASSERT_NO_THROW(srv_->createNameChangeRequests(answer, ctx));
     ASSERT_EQ(1, d2_mgr_.getQueueSize());
 
     // Verify that NameChangeRequest is correct.
@@ -776,6 +791,32 @@ TEST_F(FqdnDhcpv6SrvTest, createNameChangeRequests) {
                             "FAAAA3EBD29826B5C907B2C9268A6F52",
                             0, 500);
 
+    // If context flags are false we should not create the NCR.
+    ctx.fwd_dns_update_ = ctx.rev_dns_update_ = false;
+    ASSERT_NO_THROW(srv_->createNameChangeRequests(answer, ctx));
+    ASSERT_EQ(0, d2_mgr_.getQueueSize());
+
+    // If only the forward flag is true, we create the NCR.
+    ctx.fwd_dns_update_ = true;
+    ASSERT_NO_THROW(srv_->createNameChangeRequests(answer, ctx));
+    // Verify that NameChangeRequest is correct.
+    verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true,
+                            "2001:db8:1::1",
+                            "000201415AA33D1187D148275136FA30300478"
+                            "FAAAA3EBD29826B5C907B2C9268A6F52",
+                            0, 500);
+    ASSERT_EQ(0, d2_mgr_.getQueueSize());
+
+    // If only the reverse flag is true, we create the NCR.
+    ctx.fwd_dns_update_ = false;
+    ctx.rev_dns_update_ = true;
+    ASSERT_NO_THROW(srv_->createNameChangeRequests(answer, ctx));
+    // Verify that NameChangeRequest is correct.
+    verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true,
+                            "2001:db8:1::1",
+                            "000201415AA33D1187D148275136FA30300478"
+                            "FAAAA3EBD29826B5C907B2C9268A6F52",
+                            0, 500);
 }
 
 // Checks that NameChangeRequests to add entries are not
@@ -799,10 +840,11 @@ TEST_F(FqdnDhcpv6SrvTest, noAddRequestsWhenDisabled) {
     answer->addOption(fqdn);
 
     // An attempt to send a NCR would throw.
-    ASSERT_NO_THROW(srv_->createNameChangeRequests(answer));
+    AllocEngine::ClientContext6 ctx;
+    ctx.fwd_dns_update_ = ctx.rev_dns_update_ = true;
+    ASSERT_NO_THROW(srv_->createNameChangeRequests(answer, ctx));
 }
 
-
 // Test creation of the NameChangeRequest to remove both forward and reverse
 // mapping for the given lease.
 TEST_F(FqdnDhcpv6SrvTest, createRemovalNameChangeRequestFwdRev) {
@@ -918,20 +960,26 @@ TEST_F(FqdnDhcpv6SrvTest, processSolicit) {
 // a different domain-name. Server should use existing lease for the second
 // request but modify the DNS entries for the lease according to the contents
 // of the FQDN sent in the second request.
-/// @todo: Fix will be available on trac3677
-TEST_F(FqdnDhcpv6SrvTest, DISABLED_processTwoRequests) {
+TEST_F(FqdnDhcpv6SrvTest, processTwoRequestsDiffFqdn) {
     // Create a Request message with FQDN option and generate server's
     // response using processRequest function. This will result in the
     // creation of a new lease and the appropriate NameChangeRequest
     // to add both reverse and forward mapping to DNS.
     testProcessMessage(DHCPV6_REQUEST, "myhost.example.com",
                        "myhost.example.com.");
+
+    // The lease should have been recorded in the database.
+    lease_ = LeaseMgrFactory::instance().getLease6(Lease::TYPE_NA,
+                                                   IOAddress("2001:db8:1:1::dead:beef"));
+    ASSERT_TRUE(lease_);
+
     ASSERT_EQ(1, d2_mgr_.getQueueSize());
     verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true,
                             "2001:db8:1:1::dead:beef",
                             "000201415AA33D1187D148275136FA30300478"
                             "FAAAA3EBD29826B5C907B2C9268A6F52",
-                            lease_->cltt_ + lease_->valid_lft_, 4000);
+                            0, 4000);
+
 
     // Client may send another request message with a new domain-name. In this
     // case the same lease will be returned. The existing DNS entry needs to
@@ -947,7 +995,7 @@ TEST_F(FqdnDhcpv6SrvTest, DISABLED_processTwoRequests) {
                             "2001:db8:1:1::dead:beef",
                             "000201415AA33D1187D148275136FA30300478"
                             "FAAAA3EBD29826B5C907B2C9268A6F52",
-                            0, 4000);
+                            lease_->cltt_ + lease_->valid_lft_, 4000);
     verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true,
                             "2001:db8:1:1::dead:beef",
                             "000201D422AA463306223D269B6CB7AFE7AAD265FC"
@@ -956,6 +1004,38 @@ TEST_F(FqdnDhcpv6SrvTest, DISABLED_processTwoRequests) {
 
 }
 
+// Test that client may send two requests, each carrying FQDN option with
+// the same domain-name. Server should use existing lease for the second
+// request and not modify the DNS entries.
+TEST_F(FqdnDhcpv6SrvTest, processTwoRequestsSameFqdn) {
+    // Create a Request message with FQDN option and generate server's
+    // response using processRequest function. This will result in the
+    // creation of a new lease and the appropriate NameChangeRequest
+    // to add both reverse and forward mapping to DNS.
+    testProcessMessage(DHCPV6_REQUEST, "myhost.example.com",
+                       "myhost.example.com.");
+
+    // The lease should have been recorded in the database.
+    lease_ = LeaseMgrFactory::instance().getLease6(Lease::TYPE_NA,
+                                                   IOAddress("2001:db8:1:1::dead:beef"));
+    ASSERT_TRUE(lease_);
+
+    ASSERT_EQ(1, d2_mgr_.getQueueSize());
+    verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true,
+                            "2001:db8:1:1::dead:beef",
+                            "000201415AA33D1187D148275136FA30300478"
+                            "FAAAA3EBD29826B5C907B2C9268A6F52",
+                            0, 4000);
+
+
+    // Client may send another request message with a same domain-name. In this
+    // case the same lease will be returned. The existing DNS entry should be
+    // left alone, so we expect no NameChangeRequests queued..
+    testProcessMessage(DHCPV6_REQUEST, "myhost.example.com",
+                       "myhost.example.com.");
+    ASSERT_EQ(0, d2_mgr_.getQueueSize());
+}
+
 // Test that NameChangeRequest is not generated when Solicit message is sent.
 // The Solicit is here sent after a lease has been allocated for a client.
 // The Solicit conveys a different hostname which would trigger updates to
@@ -992,13 +1072,18 @@ TEST_F(FqdnDhcpv6SrvTest, processRequestSolicit) {
 // DNS entry added previously when Request was processed, another one to
 // add a new entry for the FQDN held in the Renew.
 /// @todo: Fix will be available on trac3677
-TEST_F(FqdnDhcpv6SrvTest, DISABLED_processRequestRenew) {
+TEST_F(FqdnDhcpv6SrvTest, processRequestRenewDiffFqdn) {
     // Create a Request message with FQDN option and generate server's
     // response using processRequest function. This will result in the
     // creation of a new lease and the appropriate NameChangeRequest
     // to add both reverse and forward mapping to DNS.
     testProcessMessage(DHCPV6_REQUEST, "myhost.example.com",
                        "myhost.example.com.");
+    // The lease should have been recorded in the database.
+    lease_ = LeaseMgrFactory::instance().getLease6(Lease::TYPE_NA,
+                                                   IOAddress("2001:db8:1:1::dead:beef"));
+    ASSERT_TRUE(lease_);
+
     ASSERT_EQ(1, d2_mgr_.getQueueSize());
     verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true,
                             "2001:db8:1:1::dead:beef",
@@ -1020,7 +1105,7 @@ TEST_F(FqdnDhcpv6SrvTest, DISABLED_processRequestRenew) {
                             "2001:db8:1:1::dead:beef",
                             "000201415AA33D1187D148275136FA30300478"
                             "FAAAA3EBD29826B5C907B2C9268A6F52",
-                            0, 4000);
+                            lease_->cltt_ + lease_->valid_lft_, 4000);
     verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true,
                             "2001:db8:1:1::dead:beef",
                             "000201D422AA463306223D269B6CB7AFE7AAD265FC"
@@ -1029,6 +1114,35 @@ TEST_F(FqdnDhcpv6SrvTest, DISABLED_processRequestRenew) {
 
 }
 
+// Test that client may send Request followed by the Renew, both holding
+// FQDN options, but each option holding different domain-name. The Renew
+// should result in generation of the two NameChangeRequests, one to remove
+// DNS entry added previously when Request was processed, another one to
+// add a new entry for the FQDN held in the Renew.
+TEST_F(FqdnDhcpv6SrvTest, processRequestRenewSameFqdn) {
+    // Create a Request message with FQDN option and generate server's
+    // response using processRequest function. This will result in the
+    // creation of a new lease and the appropriate NameChangeRequest
+    // to add both reverse and forward mapping to DNS.
+    testProcessMessage(DHCPV6_REQUEST, "myhost.example.com",
+                       "myhost.example.com.");
+    ASSERT_EQ(1, d2_mgr_.getQueueSize());
+    verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true,
+                            "2001:db8:1:1::dead:beef",
+                            "000201415AA33D1187D148275136FA30300478"
+                            "FAAAA3EBD29826B5C907B2C9268A6F52",
+                            0, 4000);
+
+    ASSERT_EQ(0, d2_mgr_.getQueueSize());
+
+    // Client may send Renew message with a same domain-name. In this
+    // case the same lease will be returned. No DNS updates should be
+    // required, so the NCR queue should be empty.
+    testProcessMessage(DHCPV6_RENEW, "myhost.example.com",
+                       "myhost.example.com.");
+    ASSERT_EQ(0, d2_mgr_.getQueueSize());
+}
+
 TEST_F(FqdnDhcpv6SrvTest, processRequestRelease) {
     // Create a Request message with FQDN option and generate server's
     // response using processRequest function. This will result in the

src/lib/dhcpsrv/alloc_engine.cc

@@ -410,7 +410,6 @@ AllocEngine::allocateLeases6(ClientContext6& ctx) {
                                                    *ctx.duid_,
                                                    ctx.currentIA().iaid_,
                                                    ctx.subnet_->getID());
-
         // Now do the checks:
         // Case 1. if there are no leases, and there are reservations...
         //   1.1. are the reserved addresses are used by someone else?
@@ -1382,10 +1381,19 @@ AllocEngine::extendLease6(ClientContext6& ctx, Lease6Ptr lease) {
         if (old_data->expired()) {
             reclaimExpiredLease(old_data, ctx.callout_handle_);
 
-        } else  if (!lease->hasIdenticalFqdn(*old_data)) {
-            // We're not reclaiming the lease but since the FQDN has changed
-            // we have to at least send NCR.
-            queueNCR(CHG_REMOVE, old_data);
+        } else {
+            if (!lease->hasIdenticalFqdn(*old_data)) {
+                // We're not reclaiming the lease but since the FQDN has changed
+                // we have to at least send NCR.
+                queueNCR(CHG_REMOVE, old_data);
+            } else {
+                // Callers should use the context flags to drive whether or
+                // not we do an update, not the lease flags.  Changing those
+                // would alter the flags in the database, which we want to
+                // preserve them as they were were when the lease was created.
+                ctx.fwd_dns_update_ = false;
+                ctx.rev_dns_update_ = false;
+            }
         }
         // Now that the lease has been reclaimed, we can go ahead and update it
         // in the lease database.
@@ -1403,22 +1411,41 @@ AllocEngine::extendLease6(ClientContext6& ctx, Lease6Ptr lease) {
 Lease6Collection
 AllocEngine::updateLeaseData(ClientContext6& ctx, const Lease6Collection& leases) {
     Lease6Collection updated_leases;
+    bool remove_queued = false;
     for (Lease6Collection::const_iterator lease_it = leases.begin();
          lease_it != leases.end(); ++lease_it) {
         Lease6Ptr lease(new Lease6(**lease_it));
         lease->fqdn_fwd_ = ctx.fwd_dns_update_;
         lease->fqdn_rev_ = ctx.rev_dns_update_;
         lease->hostname_ = ctx.hostname_;
-        if (!ctx.fake_allocation_ &&
-            (conditionalExtendLifetime(*lease) ||
-             (lease->fqdn_fwd_ != (*lease_it)->fqdn_fwd_) ||
-             (lease->fqdn_rev_ != (*lease_it)->fqdn_rev_) ||
-             (lease->hostname_ != (*lease_it)->hostname_))) {
-            ctx.currentIA().changed_leases_.push_back(*lease_it);
-            LeaseMgrFactory::instance().updateLease6(lease);
+        if (!ctx.fake_allocation_) {
+            bool fqdn_changed = ((lease->type_ != Lease::TYPE_PD) &&
+                                 !(lease->hasIdenticalFqdn(**lease_it)));
+
+            if (conditionalExtendLifetime(*lease) || fqdn_changed) {
+                ctx.currentIA().changed_leases_.push_back(*lease_it);
+                LeaseMgrFactory::instance().updateLease6(lease);
+
+                // If the FQDN differs, remove existing DNS entries.
+                // We only need one remove.
+                if (fqdn_changed && !remove_queued) {
+                    queueNCR(CHG_REMOVE, *lease_it);
+                    remove_queued = true;
+                }
+            }
         }
+
         updated_leases.push_back(lease);
     }
+
+    // If we didn't do a remove, then the FQDN is unchanged so this amounts
+    // to renew.  Note this assumes this method is only called when client
+    // issues a request, rather than a renew or rebind.
+    if (!remove_queued) {
+        ctx.fwd_dns_update_ = false;
+        ctx.rev_dns_update_ = false;
+    }
+
     return (updated_leases);
 }
 

src/lib/dhcpsrv/alloc_engine.h

@@ -856,6 +856,11 @@ private:
     /// - client's last transmission time (cltt), if the lease to be returned
     ///   to the client should have its lifetime extended,
     /// - FQDN data, when the client has negotiated new FQDN with the server.
+    /// - If the FQDN data has not changed, the DNS update flags in the
+    ///   context, fwd_dns_update_ and rev_dns_update_, are set false. This
+    ///   should indicate to callers that DDNS updates should not be done.
+    ///   The lease flags are left intact to indicate their state when the
+    ///   lease was assigned.
     ///
     /// @param ctx IPv6 client context (old versions of the leases that had
     ///            FQDN data changed will be stored in ctx.changed_leases_,
@@ -883,6 +888,11 @@ private:
     /// or lease6_rebind hooks (depending on the client's message specified in
     /// ctx.query). The lease will be extended in LeaseMgr, unless the hooks
     /// library will set the skip flag.
+    /// If the FQDN data has not changed, the DNS update flags in the
+    /// context, fwd_dns_update_ and rev_dns_update_, are set false. This
+    /// should indicate to callers that DDNS updates should not be done.
+    /// The lease flags are left intact to indicate their state when the
+    /// lease was assigned.
     ///
     /// @param ctx client context that passes all necessary information. See
     ///        @ref ClientContext6 for details.