[3035] Implemented function which generates NameChangeRequests for a lease.

src/bin/dhcp4/dhcp4_srv.cc

@@ -847,17 +847,66 @@ Dhcpv4Srv::processHostnameOption(const Pkt4Ptr&, Pkt4Ptr&) {
 
 void
 Dhcpv4Srv::createNameChangeRequests(const Lease4Ptr& lease,
-                                    const Lease4Ptr&) {
+                                    const Lease4Ptr& old_lease) {
     if (!lease) {
         isc_throw(isc::Unexpected,
                   "NULL lease specified when creating NameChangeRequest");
     }
 
-    D2Dhcid dhcid = computeDhcid(lease);
-    NameChangeRequest ncr(isc::dhcp_ddns::CHG_ADD,
-                          lease->fqdn_fwd_, lease->fqdn_rev_, lease->hostname_,
-                          lease->addr_.toText(), dhcid, 0, lease->valid_lft_);
-    name_change_reqs_.push(ncr);
+    // If old lease is not NULL, it is an indication that the lease has
+    // just been renewed. In such case we may need to generate the
+    // additional NameChangeRequest to remove an existing entry before
+    // we create a NameChangeRequest to add the entry for an updated lease.
+    // We may also decide not to generate any requests at all. This is when
+    // we discover that nothing has changed in the client's FQDN data.
+    if (old_lease) {
+        if (!lease->matches(*old_lease)) {
+            isc_throw(isc::Unexpected,
+                      "there is no match between the current instance of the"
+                      " lease: " << lease->toText() << ", and the previous"
+                      " instance: " << lease->toText());
+        } else {
+            // There will be a NameChangeRequest generated to remove existing
+            // DNS entries if the following conditions are met:
+            // - The hostname is set for the existing lease, we can't generate
+            //   removal request for non-existent hostname.
+            // - A server has performed reverse, forward or both updates.
+            // - FQDN data between the new and old lease do not match.
+            if ((!old_lease->hostname_.empty() &&
+                (old_lease->fqdn_fwd_ || old_lease->fqdn_rev_)) &&
+                ((lease->hostname_ != old_lease->hostname_) ||
+                 (lease->fqdn_fwd_ != old_lease->fqdn_fwd_) ||
+                 (lease->fqdn_rev_ != old_lease->fqdn_rev_))) {
+                D2Dhcid dhcid = computeDhcid(old_lease);
+                NameChangeRequest ncr(isc::dhcp_ddns::CHG_REMOVE,
+                                      old_lease->fqdn_fwd_,
+                                      old_lease->fqdn_rev_,
+                                      old_lease->hostname_,
+                                      old_lease->addr_.toText(),
+                                      dhcid, 0, old_lease->valid_lft_);
+                name_change_reqs_.push(ncr);
+
+            // If FQDN data from both leases match, there is no need to update.
+            } else if ((lease->hostname_ == old_lease->hostname_) &&
+                       (lease->fqdn_fwd_ == old_lease->fqdn_fwd_) &&
+                       (lease->fqdn_rev_ == old_lease->fqdn_rev_)) {
+                return;
+            }
+
+        }
+    }
+
+    // We may need to generate the NameChangeRequest for the new lease. But,
+    // this is only when hostname is set and if forward or reverse update has
+    // been requested.
+    if (!lease->hostname_.empty() && (lease->fqdn_fwd_ || lease->fqdn_rev_)) {
+        D2Dhcid dhcid = computeDhcid(lease);
+        NameChangeRequest ncr(isc::dhcp_ddns::CHG_ADD,
+                              lease->fqdn_fwd_, lease->fqdn_rev_,
+                              lease->hostname_, lease->addr_.toText(),
+                              dhcid, 0, lease->valid_lft_);
+        name_change_reqs_.push(ncr);
+    }
 }
 
 void

src/bin/dhcp4/tests/fqdn_unittest.cc

@@ -148,6 +148,7 @@ public:
     void verifyNameChangeRequest(const isc::dhcp_ddns::NameChangeType type,
                                  const bool reverse, const bool forward,
                                  const std::string& addr,
+                                 const std::string& fqdn,
                                  const std::string& dhcid,
                                  const uint16_t expires,
                                  const uint16_t len) {
@@ -156,6 +157,7 @@ public:
         EXPECT_EQ(forward, ncr.isForwardChange());
         EXPECT_EQ(reverse, ncr.isReverseChange());
         EXPECT_EQ(addr, ncr.getIpAddress());
+        EXPECT_EQ(fqdn, ncr.getFqdn());
         EXPECT_EQ(dhcid, ncr.getDhcid().toStr());
         EXPECT_EQ(expires, ncr.getLeaseExpiresOn());
         EXPECT_EQ(len, ncr.getLeaseLength());
@@ -281,7 +283,8 @@ TEST_F(FqdnDhcpv4SrvTest, noUpdate) {
                                         "myhost.example.com.",
                                         Option4ClientFqdn::FULL,
                                         true);
-    testProcessFqdn(query, Option4ClientFqdn::FLAG_E | Option4ClientFqdn::FLAG_N,
+    testProcessFqdn(query, Option4ClientFqdn::FLAG_E |
+                    Option4ClientFqdn::FLAG_N,
                     "myhost.example.com.");
 }
 
@@ -311,8 +314,9 @@ TEST_F(FqdnDhcpv4SrvTest, createNameChangeRequestsNewLease) {
     ASSERT_EQ(1, srv_->name_change_reqs_.size());
 
     verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true,
-                            "192.0.2.3", "00010132E91AA355CFBB753C0F049"
-                            "7A5A940436965B68B6D438D98E680BF10B09F3BCF",
+                            "192.0.2.3", "myhost.example.com.",
+                            "00010132E91AA355CFBB753C0F0497A5A940436965"
+                            "B68B6D438D98E680BF10B09F3BCF",
                             0, 100);
 }
 
@@ -329,5 +333,70 @@ TEST_F(FqdnDhcpv4SrvTest, createNameChangeRequestsRenewNoChange) {
     EXPECT_TRUE(srv_->name_change_reqs_.empty());
 }
 
+// Test that no NameChangeRequest is generated when forward and reverse
+// DNS update flags are not set in the lease.
+TEST_F(FqdnDhcpv4SrvTest, createNameChangeRequestsNoUpdate) {
+    Lease4Ptr lease1 = createLease(IOAddress("192.0.2.3"),
+                                   "lease1.example.com.",
+                                   true, true);
+    Lease4Ptr lease2 = createLease(IOAddress("192.0.2.3"),
+                                   "lease2.example.com.",
+                                   false, false);
+    ASSERT_NO_THROW(srv_->createNameChangeRequests(lease2, lease1));
+    EXPECT_EQ(1, srv_->name_change_reqs_.size());
+
+    verifyNameChangeRequest(isc::dhcp_ddns::CHG_REMOVE, true, true,
+                            "192.0.2.3", "lease1.example.com.",
+                            "0001013A5B311F5B9FB10DDF8E53689B874F25D"
+                            "62CC147C2FF237A64C90E5A597C9B7A",
+                            0, 100);
+
+    lease2->hostname_ = "";
+    lease2->fqdn_rev_ = true;
+    lease2->fqdn_fwd_ = true;
+    ASSERT_NO_THROW(srv_->createNameChangeRequests(lease2, lease1));
+    EXPECT_EQ(1, srv_->name_change_reqs_.size());
+
+}
+
+// Test that two NameChangeRequests are generated when the lease is being
+// renewed and the new lease has updated FQDN data.
+TEST_F(FqdnDhcpv4SrvTest, createNameChangeRequestsRenew) {
+    Lease4Ptr lease1 = createLease(IOAddress("192.0.2.3"),
+                                   "lease1.example.com.",
+                                   true, true);
+    Lease4Ptr lease2 = createLease(IOAddress("192.0.2.3"),
+                                   "lease2.example.com.",
+                                   true, true);
+    ASSERT_NO_THROW(srv_->createNameChangeRequests(lease2, lease1));
+    ASSERT_EQ(2, srv_->name_change_reqs_.size());
+
+    verifyNameChangeRequest(isc::dhcp_ddns::CHG_REMOVE, true, true,
+                            "192.0.2.3", "lease1.example.com.",
+                            "0001013A5B311F5B9FB10DDF8E53689B874F25D"
+                            "62CC147C2FF237A64C90E5A597C9B7A",
+                            0, 100);
+
+    verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true,
+                            "192.0.2.3", "lease2.example.com.",
+                            "000101F906D2BB752E1B2EECC5FF2BF434C0B2D"
+                            "D6D7F7BD873F4F280165DB8C9DBA7CB",
+                            0, 100);
+
+}
+
+// This test verifies that exception is thrown when leases passed to the
+// createNameChangeRequests function do not match, i.e. they comprise
+// different IP addresses, client ids etc.
+TEST_F(FqdnDhcpv4SrvTest, createNameChangeRequestsLeaseMismatch) {
+    Lease4Ptr lease1 = createLease(IOAddress("192.0.2.3"),
+                                   "lease1.example.com.",
+                                   true, true);
+    Lease4Ptr lease2 = createLease(IOAddress("192.0.2.4"),
+                                   "lease2.example.com.",
+                                   true, true);
+    EXPECT_THROW(srv_->createNameChangeRequests(lease2, lease1),
+                 isc::Unexpected);
+}
 
 } // end of anonymous namespace