[master] kea-dhcp4 DDNS logic now correctly handles renews with name replacement

    Merge branch 'trac5369'

src/bin/dhcp4/dhcp4_srv.cc

@@ -1667,6 +1667,11 @@ Dhcpv4Srv::createNameChangeRequests(const Lease4Ptr& lease,
                   "NULL lease specified when creating NameChangeRequest");
 
     } else if (!old_lease || !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);
+        }
+
         // We may need to generate the NameChangeRequest for the new lease. It
         // will be generated only if hostname is set and if forward or reverse
         // update has been requested.

src/bin/dhcp4/tests/fqdn_unittest.cc

@@ -900,6 +900,17 @@ TEST_F(NameDhcpv4SrvTest, processRequestFqdnEmptyDomainName) {
                             "", // empty DHCID forces that it is not checked
                             time(NULL) + subnet_->getValid(),
                             subnet_->getValid(), true);
+
+    req = generatePktWithFqdn(DHCPREQUEST, Option4ClientFqdn::FLAG_S |
+                              Option4ClientFqdn::FLAG_E,
+                              "", Option4ClientFqdn::PARTIAL, true);
+
+    ASSERT_NO_THROW(reply = srv_->processRequest(req));
+
+    checkResponse(reply, DHCPACK, 1234);
+
+    // Verify that there are no NameChangeRequests generated.
+    ASSERT_EQ(0, d2_mgr_.getQueueSize());
 }
 
 // Test that server generates client's hostname from the IP address assigned
@@ -1071,6 +1082,88 @@ TEST_F(NameDhcpv4SrvTest, processTwoRequestsHostname) {
                             time(NULL), subnet_->getValid(), true);
 }
 
+// Test that client may send two requests, each carrying the same FQDN option.
+// Server should renew  existing lease for the second request without generating
+// any NCRs.
+TEST_F(NameDhcpv4SrvTest, processRequestRenewFqdn) {
+    IfaceMgrTestConfig test_config(true);
+    IfaceMgr::instance().openSockets4();
+
+    Pkt4Ptr req1 = generatePktWithFqdn(DHCPREQUEST, Option4ClientFqdn::FLAG_S |
+                                       Option4ClientFqdn::FLAG_E,
+                                       "myhost.example.com.",
+                                       Option4ClientFqdn::FULL, true);
+
+    Pkt4Ptr reply;
+    ASSERT_NO_THROW(reply = srv_->processRequest(req1));
+
+    checkResponse(reply, DHCPACK, 1234);
+
+    // Verify that there is one NameChangeRequest generated.
+    ASSERT_EQ(1, d2_mgr_.getQueueSize());
+    verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true,
+                            reply->getYiaddr().toText(), "myhost.example.com.",
+                            "00010132E91AA355CFBB753C0F0497A5A940436"
+                            "965B68B6D438D98E680BF10B09F3BCF",
+                            time(NULL), subnet_->getValid(), true);
+
+    // Create another Request message with the same FQDN. Server
+    // should generate no NameChangeRequests.
+    Pkt4Ptr req2 = generatePktWithFqdn(DHCPREQUEST, Option4ClientFqdn::FLAG_S |
+                                       Option4ClientFqdn::FLAG_E,
+                                       "myhost.example.com.",
+                                       Option4ClientFqdn::FULL, true);
+
+    ASSERT_NO_THROW(reply = srv_->processRequest(req2));
+
+    checkResponse(reply, DHCPACK, 1234);
+
+    // There should be no NameChangeRequests.
+    ASSERT_EQ(0, d2_mgr_.getQueueSize());
+}
+
+// Test that client may send two requests, each carrying the same hostname
+// option.  Server should renew  existing lease for the second request without
+// generating any NCRs.
+TEST_F(NameDhcpv4SrvTest, processRequestRenewHostname) {
+    IfaceMgrTestConfig test_config(true);
+    IfaceMgr::instance().openSockets4();
+
+    Pkt4Ptr req1 = generatePktWithHostname(DHCPREQUEST, "myhost.example.com.");
+
+    // Set interface for the incoming packet. The server requires it to
+    // generate client id.
+    req1->setIface("eth1");
+
+    Pkt4Ptr reply;
+    ASSERT_NO_THROW(reply = srv_->processRequest(req1));
+
+    checkResponse(reply, DHCPACK, 1234);
+
+    // Verify that there is one NameChangeRequest generated.
+    ASSERT_EQ(1, d2_mgr_.getQueueSize());
+    verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true,
+                            reply->getYiaddr().toText(), "myhost.example.com.",
+                            "00010132E91AA355CFBB753C0F0497A5A940436"
+                            "965B68B6D438D98E680BF10B09F3BCF",
+                            time(NULL), subnet_->getValid(), true);
+
+    // Create another Request message with the same Hostname. Server
+    // should generate no NameChangeRequests.
+    Pkt4Ptr req2 = generatePktWithHostname(DHCPREQUEST, "myhost.example.com.");
+
+    // Set interface for the incoming packet. The server requires it to
+    // generate client id.
+    req2->setIface("eth1");
+
+    ASSERT_NO_THROW(reply = srv_->processRequest(req2));
+
+    checkResponse(reply, DHCPACK, 1234);
+
+    // There should be no NameChangeRequests.
+    ASSERT_EQ(0, d2_mgr_.getQueueSize());
+}
+
 // Test that when a release message is sent for a previously acquired lease,
 // DDNS updates are enabled that the server generates a NameChangeRequest
 // to remove entries corresponding to the released lease.

src/lib/dhcpsrv/alloc_engine.cc

@@ -2995,10 +2995,6 @@ AllocEngine::renewLease4(const Lease4Ptr& lease,
         if (ctx.old_lease_->expired()) {
             reclaimExpiredLease(ctx.old_lease_, ctx.callout_handle_);
 
-        } else if (!lease->hasIdenticalFqdn(*ctx.old_lease_)) {
-            // The lease is not expired but the FQDN information has
-            // changed. So, we have to remove the previous DNS entry.
-            queueNCR(CHG_REMOVE, ctx.old_lease_);
         }
 
         lease->state_ = Lease::STATE_DEFAULT;