[3677] Fix for two unit-tests disabled in #3563

src/bin/dhcp6/dhcp6_messages.mes

@@ -91,8 +91,7 @@ to perform the DNS Update, which removes RRs from the DNS.
 This debug message is logged when FQDN mapping for a particular lease has
 been changed by the recent Request message. This mapping will be changed in DNS.
 
-% DHCP6_DDNS_LEASE_RENEW_FQDN_CHANGE FQDN for the renewed lease: %1 has changed
-New  values: hostname = %2, reverse mapping = %3, forward mapping = %4
+% DHCP6_DDNS_LEASE_RENEW_FQDN_CHANGE FQDN for the renewed lease: %1 has changed new  values: hostname = %2, reverse mapping = %3, forward mapping = %4
 This debug message is logged when FQDN mapping for a particular lease has been
 changed by the recent Renew message. This mapping will be changed in DNS.
 

src/bin/dhcp6/dhcp6_srv.cc

@@ -1340,24 +1340,25 @@ Dhcpv6Srv::assignIA_NA(const Subnet6Ptr& subnet, const DuidPtr& duid,
         // code is considered a success.
 
         Lease6Ptr old_lease;
-        if (!ctx.old_leases_.empty()) {
-            old_lease = *ctx.old_leases_.begin();
+        if (!ctx.changed_leases_.empty()) {
+            old_lease = *ctx.changed_leases_.begin();
         }
         // Allocation engine may have returned an existing lease. If so, we
         // have to check that the FQDN settings we provided are the same
         // that were set. If they aren't, we will have to remove existing
         // DNS records and update the lease with the new settings.
-        if (!fake_allocation && old_lease &&
-            !lease->hasIdenticalFqdn(*old_lease)) {
-            LOG_DEBUG(dhcp6_logger, DBG_DHCP6_DETAIL,
-                      DHCP6_DDNS_LEASE_ASSIGN_FQDN_CHANGE)
-                .arg(old_lease->toText())
-                .arg(hostname)
-                .arg(do_rev ? "true" : "false")
-                .arg(do_fwd ? "true" : "false");
+        if (!fake_allocation && old_lease) {
+            conditionalNCRRemoval(old_lease, lease, hostname, do_fwd, do_rev);
+        }
+        old_lease.reset();
 
-            // Schedule removal of the existing lease.
-            createRemovalNameChangeRequest(old_lease);
+        // We need to repeat that check for leases that used to be used, but
+        // are no longer valid.
+        if (!ctx.old_leases_.empty()) {
+            old_lease = *ctx.old_leases_.begin();
+        }
+        if (!fake_allocation && old_lease) {
+            conditionalNCRRemoval(old_lease, lease, hostname, do_fwd, do_rev);
         }
 
     } else {
@@ -1376,6 +1377,26 @@ Dhcpv6Srv::assignIA_NA(const Subnet6Ptr& subnet, const DuidPtr& duid,
     return (ia_rsp);
 }
 
+void
+Dhcpv6Srv::conditionalNCRRemoval(Lease6Ptr& old_lease, Lease6Ptr& new_lease,
+                                 const std::string& hostname, bool do_fwd,
+                                 bool do_rev) {
+    if (!old_lease) {
+        return;
+    }
+
+    if (!new_lease->hasIdenticalFqdn(*old_lease)) {
+        LOG_DEBUG(dhcp6_logger, DBG_DHCP6_DETAIL, DHCP6_DDNS_LEASE_ASSIGN_FQDN_CHANGE)
+            .arg(old_lease->toText())
+            .arg(hostname)
+            .arg(do_rev ? "true" : "false")
+            .arg(do_fwd ? "true" : "false");
+
+        // Schedule removal of the existing lease.
+        createRemovalNameChangeRequest(old_lease);
+    }
+}
+
 OptionPtr
 Dhcpv6Srv::assignIA_PD(const Subnet6Ptr& subnet, const DuidPtr& duid,
                        const Pkt6Ptr& query, boost::shared_ptr<Option6IA> ia) {

src/bin/dhcp6/dhcp6_srv.h

@@ -667,6 +667,21 @@ private:
     /// as a programmatic error.
     void generateFqdn(const Pkt6Ptr& answer);
 
+
+    /// @brief Triggers removal Name Change Request if FQDN data changes in leases
+    ///
+    /// If there are any differences (different fwd or rev flags, or different
+    /// hostname) a DNS update for removing entry will be generated.
+    ///
+    /// @param old_lease old version of the lease
+    /// @param new_lease new version of the lease (may be NULL)
+    /// @param hostname specifies hostname (for printing purposes)
+    /// @param do_fwd specifies if reverse updates are enabled (for printing purposes)
+    /// @param do_rev specifies if reverse updates are enabled (for printing purposes)
+    void conditionalNCRRemoval(Lease6Ptr& old_lease, Lease6Ptr& new_lease,
+                               const std::string& hostname,
+                               bool do_fwd, bool do_rev);
+
     /// @brief Allocation Engine.
     /// Pointer to the allocation engine that we are currently using
     /// It must be a pointer, because we will support changing engines

src/bin/dhcp6/tests/fqdn_unittest.cc

@@ -414,7 +414,7 @@ public:
             ASSERT_NO_THROW(reply = srv_->processRequest(req));
 
         } else if (msg_type == DHCPV6_RENEW) {
-            ASSERT_NO_THROW(reply = srv_->processRequest(req));
+            ASSERT_NO_THROW(reply = srv_->processRenew(req));
 
         } else if (msg_type == DHCPV6_RELEASE) {
             // For Release no lease will be acquired so we have to leave

src/lib/dhcpsrv/alloc_engine.cc

@@ -386,9 +386,7 @@ AllocEngine::allocateLeases6(ClientContext6& ctx) {
 
             if (!leases.empty()) {
                 // Return old leases so the server can see what has changed.
-                return (updateFqdnData(leases, ctx.fwd_dns_update_,
-                                       ctx.rev_dns_update_,
-                                       ctx.hostname_, ctx.fake_allocation_));
+                return (updateFqdnData(ctx, leases));
             }
 
             // If leases are empty at this stage, it means that we used to have
@@ -603,6 +601,7 @@ AllocEngine::allocateUnreservedLeases6(ClientContext6& ctx) {
         if (ctx.type_ == Lease::TYPE_PD) {
             Pool6Ptr pool = boost::dynamic_pointer_cast<Pool6>(
                 ctx.subnet_->getPool(ctx.type_, candidate, false));
+            /// @todo: verify that the pool is non-null
             prefix_len = pool->getLength();
         }
 
@@ -1590,22 +1589,19 @@ AllocEngine::updateLease4Information(const Lease4Ptr& lease,
 }
 
 Lease6Collection
-AllocEngine::updateFqdnData(const Lease6Collection& leases,
-                            const bool fwd_dns_update,
-                            const bool rev_dns_update,
-                            const std::string& hostname,
-                            const bool fake_allocation) {
+AllocEngine::updateFqdnData(ClientContext6& ctx, const Lease6Collection& leases) {
     Lease6Collection updated_leases;
     for (Lease6Collection::const_iterator lease_it = leases.begin();
          lease_it != leases.end(); ++lease_it) {
         Lease6Ptr lease(new Lease6(**lease_it));
-        lease->fqdn_fwd_ = fwd_dns_update;
-        lease->fqdn_rev_ = rev_dns_update;
-        lease->hostname_ = hostname;
-        if (!fake_allocation &&
+        lease->fqdn_fwd_ = ctx.fwd_dns_update_;
+        lease->fqdn_rev_ = ctx.rev_dns_update_;
+        lease->hostname_ = ctx.hostname_;
+        if (!ctx.fake_allocation_ &&
             ((lease->fqdn_fwd_ != (*lease_it)->fqdn_fwd_) ||
              (lease->fqdn_rev_ != (*lease_it)->fqdn_rev_) ||
              (lease->hostname_ != (*lease_it)->hostname_))) {
+            ctx.changed_leases_.push_back(*lease_it);
             LeaseMgrFactory::instance().updateLease6(lease);
         }
         updated_leases.push_back(lease);

src/lib/dhcpsrv/alloc_engine.h

@@ -383,6 +383,13 @@ protected:
         /// and give a new one to this client.
         Lease6Collection old_leases_;
 
+        /// @brief A pointer to any leases that have changed FQDN information.
+        ///
+        /// This list may contain old versions of the leases that are still
+        /// valid. In particular, it will contain a lease if the client's
+        /// FQDN has changed.
+        Lease6Collection changed_leases_;
+
         /// @brief A pointer to the object identifying host reservations.
         ///
         /// May be NULL if there are no reservations.
@@ -908,10 +915,11 @@ private:
     ///        responsibility for the reverse DNS Update for this lease
     ///        (if true).
     /// @ref ClientContext6::hostname_ A fully qualified domain-name of the client.
-    /// @ref ClientContext6::callout_handle_ a callout handle (used in hooks). A lease callouts
-    ///        will be executed if this parameter is passed.
-    /// @ref ClientContext6::fake_allocation_ is this real i.e. REQUEST (false) or just picking
-    ///        an address for SOLICIT that is not really allocated (true)
+    /// @ref ClientContext6::callout_handle_ a callout handle (used in hooks). A
+    ///        lease callouts will be executed if this parameter is passed.
+    /// @ref ClientContext6::fake_allocation_ is this real i.e. REQUEST (false)
+    ///        or just picking an address for SOLICIT that is not really
+    ///        allocated (true)
     ///
     /// @return refreshed lease
     /// @throw BadValue if trying to recycle lease that is still valid
@@ -921,25 +929,17 @@ private:
 
     /// @brief Updates FQDN data for a collection of leases.
     ///
+    /// @param ctx IPv6 client context (old versions of the leases that had
+    ///            FQDN data changed will be stored in ctx.changed_leases_,
+    ///            ctx.fwd_dns_update, ctx.rev_dns_update, ctx.hostname_
+    ///            and ctx.fake_allocation_ will be used.
     /// @param leases Collection of leases for which FQDN data should be
     /// updated.
-    /// @param fwd_dns_update Boolean value which indicates whether forward FQDN
-    /// update was performed for each lease (true) or not (false).
-    /// @param rev_dns_update Boolean value which indicates whether reverse FQDN
-    /// update was performed for each lease (true) or not (false).
-    /// @param hostname Client hostname associated with a lease.
-    /// @param fake_allocation Boolean value which indicates that it is a real
-    /// lease allocation, e.g. Request message is processed (false), or address
-    /// is just being picked as a result of processing Solicit (true). In the
-    /// latter case, the FQDN data should not be updated in the lease database.
     ///
     /// @return Collection of leases with updated FQDN data. Note that returned
     /// collection holds updated FQDN data even for fake allocation.
-    Lease6Collection updateFqdnData(const Lease6Collection& leases,
-                                    const bool fwd_dns_update,
-                                    const bool rev_dns_update,
-                                    const std::string& hostname,
-                                    const bool fake_allocation);
+    Lease6Collection updateFqdnData(ClientContext6& ctx,
+                                    const Lease6Collection& leases);
 
     /// @brief Utility function that removes all leases with a specified address
     /// @param container A collection of Lease6 pointers