[5007] Addressed review comments

This is solution #2:

src/lib/dhcpsrv/alloc_engine.h
src/lib/dhcpsrv/alloc_engine.cc
    AllocEngine::extendLease6() - replaced logic to set the context flags with
    simply adding the original lease to the changed_leases_ list.

    AllocEngine::updateLeaseData() - removed logic to set the context flags.

src/bin/dhcp6/dhcp6_srv.cc
    Dhcpv6Srv::createNameChangeRequests() - replaced the context flag check
    with logic which looks for candidate IA addresses in the ctx.changed_leases_
    list.  If found and the FDQN doman name has not changed, we move on to the
    next candidate.

src/bin/dhcp6/tests/fqdn_unittest.cc
    TEST_F(FqdnDhcpv6SrvTest, createNameChangeRequestsNoAddr) - removed
    testing of context flag permutations

src/bin/dhcp6/dhcp6_srv.cc

@@ -1198,13 +1198,8 @@ Dhcpv6Srv::processClientFqdn(const Pkt6Ptr& question, const Pkt6Ptr& answer,
 void
 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_)) {
+    // Don't create NameChangeRequests if DNS updates are disabled.
+    if (!CfgMgr::instance().ddnsEnabled()) {
         return;
     }
 
@@ -1274,6 +1269,25 @@ Dhcpv6Srv::createNameChangeRequests(const Pkt6Ptr& answer,
         if (!iaaddr) {
             continue;
         }
+
+        // see if the lease for iaadr is in changed_leases, and if so
+        // if the FQDN is different, if not continue
+        bool extended_only = false;
+        for (Lease6Collection::const_iterator l = ctx.currentIA().changed_leases_.begin();
+             l != ctx.currentIA().changed_leases_.end(); ++l) {
+            if ((*l)->addr_ == iaaddr->getAddress()) {
+                if ((*l)->hostname_ == opt_fqdn->getDomainName()) {
+                    extended_only = true;
+                    break;
+                }
+            }
+        }
+
+        if (extended_only) {
+            continue;
+        }
+
+
         // Create new NameChangeRequest. Use the domain name from the FQDN.
         // This is an FQDN included in the response to the client, so it
         // holds a fully qualified domain-name already (not partial).

src/bin/dhcp6/tests/fqdn_unittest.cc

@@ -758,9 +758,7 @@ 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.  We also
-// verify that the context flags, fwd_dns_update_ and rev_dns_update_, gate
-// whether or not a NameChangeRequest is created.
+// the answer message which holds 3 IAs and when FQDN is specified.
 TEST_F(FqdnDhcpv6SrvTest, createNameChangeRequests) {
     // Create Reply message with Client Id and Server id.
     Pkt6Ptr answer = generateMessageWithIds(DHCPV6_REPLY);
@@ -790,33 +788,6 @@ TEST_F(FqdnDhcpv6SrvTest, createNameChangeRequests) {
                             "000201415AA33D1187D148275136FA30300478"
                             "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

src/lib/dhcpsrv/alloc_engine.cc

@@ -1386,15 +1386,9 @@ AllocEngine::extendLease6(ClientContext6& ctx, Lease6Ptr lease) {
                 // 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.
         LeaseMgrFactory::instance().updateLease6(lease);
@@ -1406,8 +1400,13 @@ AllocEngine::extendLease6(ClientContext6& ctx, Lease6Ptr lease) {
         // fields of returned Lease6Ptr, the actual updateLease6() is no-op.
         *lease = *old_data;
     }
+
+    // Add the old lease to the changed lease list. This allows the server
+    // to make decisions regarding DNS updates.
+    ctx.currentIA().changed_leases_.push_back(old_data);
 }
 
+
 Lease6Collection
 AllocEngine::updateLeaseData(ClientContext6& ctx, const Lease6Collection& leases) {
     Lease6Collection updated_leases;
@@ -1438,14 +1437,6 @@ AllocEngine::updateLeaseData(ClientContext6& ctx, const Lease6Collection& leases
         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,11 +856,6 @@ 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_,
@@ -887,12 +882,9 @@ private:
     /// This method attempts to extend the lease. It will call the lease6_renew
     /// 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.
+    /// library will set the skip flag.  The old lease is added to the
+    /// the context's changed_leases_ list which allows the server to make
+    /// decisions regarding DNS updates.
     ///
     /// @param ctx client context that passes all necessary information. See
     ///        @ref ClientContext6 for details.