[3035] Create NameChangeRequests for incoming Request and Release.

Also, use Ciaddr instead of Yiaddr to get the released lease from the
lease database.

src/bin/dhcp4/dhcp4_messages.mes

@@ -70,6 +70,11 @@ This message is printed when DHCPv4 server disables an interface from being
 used to receive DHCPv4 traffic. Sockets on this interface will not be opened
 by the Interface Manager until interface is enabled.
 
+% DHCP4_DHCID_COMPUTE_ERROR failed to compute the DHCID for lease: %1, reason: %2
+This error message is logged when the attempt to compute DHCID for a specified
+lease has failed. The lease details and reason for failure is logged in the
+message.
+
 % DHCP4_HOOK_BUFFER_RCVD_SKIP received DHCPv4 buffer was dropped because a callout set the skip flag.
 This debug message is printed when a callout installed on buffer4_receive
 hook point set the skip flag. For this particular hook point, the

src/bin/dhcp4/dhcp4_srv.cc

@@ -872,19 +872,11 @@ Dhcpv4Srv::createNameChangeRequests(const Lease4Ptr& lease,
             //   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_) ||
+            if  ((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_);
-                queueNameChangeRequest(ncr);
+                 (lease->fqdn_rev_ != old_lease->fqdn_rev_)) {
+                queueNameChangeRequest(isc::dhcp_ddns::CHG_REMOVE,
+                                       old_lease);
 
             // If FQDN data from both leases match, there is no need to update.
             } else if ((lease->hostname_ == old_lease->hostname_) &&
@@ -896,23 +888,37 @@ Dhcpv4Srv::createNameChangeRequests(const Lease4Ptr& lease,
         }
     }
 
-    // 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_);
-        queueNameChangeRequest(ncr);
-    }
+    // 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.
+    queueNameChangeRequest(isc::dhcp_ddns::CHG_ADD, lease);
 }
 
 void
 Dhcpv4Srv::
-queueNameChangeRequest(const isc::dhcp_ddns::NameChangeRequest& ncr) {
-    if (ncr.getChangeType() == isc::dhcp_ddns::CHG_ADD) {
+queueNameChangeRequest(const isc::dhcp_ddns::NameChangeType chg_type,
+                       const Lease4Ptr& lease) {
+    // The hostname must not be empty, and at least one type of update
+    // should be requested.
+    if (!lease || lease->hostname_.empty() ||
+        (!lease->fqdn_rev_ && !lease->fqdn_fwd_)) {
+        return;
+    }
+    D2Dhcid dhcid;
+    try {
+        dhcid  = computeDhcid(lease);
+
+    } catch (const DhcidComputeError& ex) {
+        LOG_ERROR(dhcp4_logger, DHCP4_DHCID_COMPUTE_ERROR)
+            .arg(lease->toText())
+            .arg(ex.what());
+        return;
+
+    }
+    NameChangeRequest ncr(chg_type, lease->fqdn_fwd_, lease->fqdn_rev_,
+                          lease->hostname_, lease->addr_.toText(),
+                          dhcid, 0, lease->valid_lft_);
+    if (chg_type == isc::dhcp_ddns::CHG_ADD) {
         LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL_DATA,
                   DHCP4_QUEUE_ADDITION_NCR)
             .arg(ncr.toText());
@@ -926,6 +932,7 @@ queueNameChangeRequest(const isc::dhcp_ddns::NameChangeRequest& ncr) {
     name_change_reqs_.push(ncr);
 }
 
+
 void
 Dhcpv4Srv::assignLease(const Pkt4Ptr& question, Pkt4Ptr& answer) {
 
@@ -975,6 +982,17 @@ Dhcpv4Srv::assignLease(const Pkt4Ptr& question, Pkt4Ptr& answer) {
 
     CalloutHandlePtr callout_handle = getCalloutHandle(question);
 
+    std::string hostname;
+    bool fqdn_fwd = false;
+    bool fqdn_rev = false;
+    Option4ClientFqdnPtr fqdn = boost::dynamic_pointer_cast<
+        Option4ClientFqdn>(answer->getOption(DHO_FQDN));
+    if (fqdn) {
+        hostname = fqdn->getDomainName();
+        fqdn_fwd = fqdn->getFlag(Option4ClientFqdn::FLAG_S);
+        fqdn_rev = !fqdn->getFlag(Option4ClientFqdn::FLAG_N);
+    }
+
     // Use allocation engine to pick a lease for this client. Allocation engine
     // will try to honour the hint, but it is just a hint - some other address
     // may be used instead. If fake_allocation is set to false, the lease will
@@ -982,7 +1000,8 @@ Dhcpv4Srv::assignLease(const Pkt4Ptr& question, Pkt4Ptr& answer) {
     // @todo pass the actual FQDN data.
     Lease4Ptr old_lease;
     Lease4Ptr lease = alloc_engine_->allocateAddress4(subnet, client_id, hwaddr,
-                                                      hint, false, false, "",
+                                                      hint, fqdn_fwd, fqdn_rev,
+                                                      hostname,
                                                       fake_allocation,
                                                       callout_handle,
                                                       old_lease);
@@ -1044,6 +1063,9 @@ Dhcpv4Srv::assignLease(const Pkt4Ptr& question, Pkt4Ptr& answer) {
 
         answer->setType(DHCPNAK);
         answer->setYiaddr(IOAddress("0.0.0.0"));
+
+        answer->delOption(DHO_FQDN);
+        answer->delOption(DHO_HOST_NAME);
     }
 }
 
@@ -1123,6 +1145,12 @@ Dhcpv4Srv::processDiscover(Pkt4Ptr& discover) {
     appendDefaultOptions(offer, DHCPOFFER);
     appendRequestedOptions(discover, offer);
 
+    // If DISCOVER message contains the FQDN or Hostname option, server
+    // may respond to the client with the appropriate FQDN or Hostname
+    // option to indicate that whether it will take responsibility for
+    // updating DNS when the client sends REQUEST message.
+    processClientName(discover, offer);
+
     assignLease(discover, offer);
 
     // There are a few basic options that we always want to
@@ -1146,6 +1174,12 @@ Dhcpv4Srv::processRequest(Pkt4Ptr& request) {
     appendDefaultOptions(ack, DHCPACK);
     appendRequestedOptions(request, ack);
 
+    // If REQUEST message contains the FQDN or Hostname option, server
+    // should respond to the client with the appropriate FQDN or Hostname
+    // option to indicate if it takes responsibility for the DNS updates.
+    // This is performed by the function below.
+    processClientName(request, ack);
+
     // Note that we treat REQUEST message uniformly, regardless if this is a
     // first request (requesting for new address), renewing existing address
     // or even rebinding.
@@ -1174,12 +1208,12 @@ Dhcpv4Srv::processRelease(Pkt4Ptr& release) {
 
     try {
         // Do we have a lease for that particular address?
-        Lease4Ptr lease = LeaseMgrFactory::instance().getLease4(release->getYiaddr());
+        Lease4Ptr lease = LeaseMgrFactory::instance().getLease4(release->getCiaddr());
 
         if (!lease) {
             // No such lease - bogus release
             LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_RELEASE_FAIL_NO_LEASE)
-                .arg(release->getYiaddr().toText())
+                .arg(release->getCiaddr().toText())
                 .arg(release->getHWAddr()->toText())
                 .arg(client_id ? client_id->toText() : "(no client-id)");
             return;
@@ -1190,7 +1224,7 @@ Dhcpv4Srv::processRelease(Pkt4Ptr& release) {
         if (lease->hwaddr_ != release->getHWAddr()->hwaddr_) {
             // @todo: Print hwaddr from lease as part of ticket #2589
             LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_RELEASE_FAIL_WRONG_HWADDR)
-                .arg(release->getYiaddr().toText())
+                .arg(release->getCiaddr().toText())
                 .arg(client_id ? client_id->toText() : "(no client-id)")
                 .arg(release->getHWAddr()->toText());
             return;
@@ -1200,7 +1234,7 @@ Dhcpv4Srv::processRelease(Pkt4Ptr& release) {
         // the client sent us.
         if (lease->client_id_ && client_id && *lease->client_id_ != *client_id) {
             LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_RELEASE_FAIL_WRONG_CLIENT_ID)
-                .arg(release->getYiaddr().toText())
+                .arg(release->getCiaddr().toText())
                 .arg(client_id->toText())
                 .arg(lease->client_id_->toText());
             return;
@@ -1241,11 +1275,15 @@ Dhcpv4Srv::processRelease(Pkt4Ptr& release) {
             bool success = LeaseMgrFactory::instance().deleteLease(lease->addr_);
 
             if (success) {
-                // Release successful - we're done here
+                // Release successful
                 LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_RELEASE)
                     .arg(lease->addr_.toText())
                     .arg(client_id ? client_id->toText() : "(no client-id)")
                     .arg(release->getHWAddr()->toText());
+
+                // Remove existing DNS entries for the lease, if any.
+                queueNameChangeRequest(isc::dhcp_ddns::CHG_REMOVE, lease);
+
             } else {
                 // Release failed -
                 LOG_ERROR(dhcp4_logger, DHCP4_RELEASE_FAIL)

src/bin/dhcp4/dhcp4_srv.h

@@ -329,15 +329,18 @@ protected:
     void createNameChangeRequests(const Lease4Ptr& lease,
                                   const Lease4Ptr& old_lease);
 
-    /// @brief Adds the NameChangeRequest to the queue for processing.
+    /// @brief Creates the NameChangeRequest and adds to the queue for
+    /// processing.
     ///
     /// This function adds the @c isc::dhcp_ddns::NameChangeRequest to the
     /// queue and emits the debug message which indicates whether the request
     /// being added is to remove DNS entry or add a new entry. This function
     /// is exception free.
     ///
+    /// @param chg_type A type of the NameChangeRequest (ADD or REMOVE).
     /// @param ncr An isc::dhcp_ddns::NameChangeRequest object being added.
-    void queueNameChangeRequest(const isc::dhcp_ddns::NameChangeRequest& ncr);
+    void queueNameChangeRequest(const isc::dhcp_ddns::NameChangeType chg_type,
+                                const Lease4Ptr& lease);
 
     /// @brief Attempts to renew received addresses
     ///

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

@@ -860,7 +860,7 @@ TEST_F(Dhcpv4SrvTest, ReleaseBasic) {
     // Generate client-id also duid_
     Pkt4Ptr rel = Pkt4Ptr(new Pkt4(DHCPRELEASE, 1234));
     rel->setRemoteAddr(addr);
-    rel->setYiaddr(addr);
+    rel->setCiaddr(addr);
     rel->addOption(clientid);
     rel->addOption(srv->getServerID());
     rel->setHWAddr(hw);
@@ -925,7 +925,7 @@ TEST_F(Dhcpv4SrvTest, ReleaseReject) {
     // Generate client-id also duid_
     Pkt4Ptr rel = Pkt4Ptr(new Pkt4(DHCPRELEASE, 1234));
     rel->setRemoteAddr(addr);
-    rel->setYiaddr(addr);
+    rel->setCiaddr(addr);
     rel->addOption(clientid);
     rel->addOption(srv->getServerID());
     rel->setHWAddr(bogus_hw);
@@ -2284,7 +2284,7 @@ TEST_F(HooksDhcpv4SrvTest, lease4ReleaseSimple) {
     // Generate client-id also duid_
     Pkt4Ptr rel = Pkt4Ptr(new Pkt4(DHCPRELEASE, 1234));
     rel->setRemoteAddr(addr);
-    rel->setYiaddr(addr);
+    rel->setCiaddr(addr);
     rel->addOption(clientid);
     rel->addOption(srv_->getServerID());
     rel->setHWAddr(hw);

src/bin/dhcp4/tests/fqdn_unittest.cc

@@ -33,7 +33,6 @@ public:
     FqdnDhcpv4SrvTest() : Dhcpv4SrvTest() {
         srv_ = new NakedDhcpv4Srv(0);
     }
-
     virtual ~FqdnDhcpv4SrvTest() {
         delete srv_;
     }
@@ -110,6 +109,7 @@ public:
         return (pkt);
     }
 
+
     // Test that server generates the appropriate FQDN option in response to
     // client's FQDN option.
     void testProcessFqdn(const Pkt4Ptr& query, const uint8_t exp_flags,
@@ -144,6 +144,37 @@ public:
 
     }
 
+    // Test that the client message holding an FQDN is processed and the
+    // NameChangeRequests are generated.
+    void testProcessMessageWithFqdn(const uint8_t msg_type,
+                            const std::string& hostname) {
+        Pkt4Ptr req = generatePktWithFqdn(msg_type, Option4ClientFqdn::FLAG_S |
+                                          Option4ClientFqdn::FLAG_E, hostname,
+                                          Option4ClientFqdn::FULL, true);
+        Pkt4Ptr reply;
+        if (msg_type == DHCPDISCOVER) {
+            ASSERT_NO_THROW(reply = srv_->processDiscover(req));
+
+        } else if (msg_type == DHCPREQUEST) {
+            ASSERT_NO_THROW(reply = srv_->processRequest(req));
+
+        } else if (msg_type == DHCPRELEASE) {
+            ASSERT_NO_THROW(srv_->processRelease(req));
+            return;
+
+        } else {
+            return;
+        }
+
+        if (msg_type == DHCPDISCOVER) {
+            checkResponse(reply, DHCPOFFER, 1234);
+
+        } else {
+            checkResponse(reply, DHCPACK, 1234);
+        }
+
+    }
+
     // Verify that NameChangeRequest holds valid values.
     void verifyNameChangeRequest(const isc::dhcp_ddns::NameChangeType type,
                                  const bool reverse, const bool forward,
@@ -399,4 +430,108 @@ TEST_F(FqdnDhcpv4SrvTest, createNameChangeRequestsLeaseMismatch) {
                  isc::Unexpected);
 }
 
+// Test that the OFFER message generated as a result of the DISCOVER message
+// processing will not result in generation of the NameChangeRequests.
+TEST_F(FqdnDhcpv4SrvTest, processDiscover) {
+    Pkt4Ptr req = generatePktWithFqdn(DHCPDISCOVER, Option4ClientFqdn::FLAG_S |
+                                      Option4ClientFqdn::FLAG_E,
+                                      "myhost.example.com.",
+                                      Option4ClientFqdn::FULL, true);
+
+    Pkt4Ptr reply;
+    ASSERT_NO_THROW(reply = srv_->processDiscover(req));
+
+    checkResponse(reply, DHCPOFFER, 1234);
+
+    EXPECT_TRUE(srv_->name_change_reqs_.empty());
+}
+
+// Test that client may send two requests, each carrying FQDN option with
+// 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.
+TEST_F(FqdnDhcpv4SrvTest, processTwoRequests) {
+    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, srv_->name_change_reqs_.size());
+    verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true,
+                            reply->getYiaddr().toText(), "myhost.example.com.",
+                            "00010132E91AA355CFBB753C0F0497A5A940436"
+                            "965B68B6D438D98E680BF10B09F3BCF",
+                            0, subnet_->getValid());
+
+    // Create another Request message but with a different FQDN. Server
+    // should generate two NameChangeRequests: one to remove existing entry,
+    // another one to add new entry with updated domain-name.
+    Pkt4Ptr req2 = generatePktWithFqdn(DHCPREQUEST, Option4ClientFqdn::FLAG_S |
+                                       Option4ClientFqdn::FLAG_E,
+                                       "otherhost.example.com.",
+                                       Option4ClientFqdn::FULL, true);
+
+    ASSERT_NO_THROW(reply = srv_->processRequest(req2));
+
+    checkResponse(reply, DHCPACK, 1234);
+
+    // There should be two NameChangeRequests. Verify that they are valid.
+    ASSERT_EQ(2, srv_->name_change_reqs_.size());
+    verifyNameChangeRequest(isc::dhcp_ddns::CHG_REMOVE, true, true,
+                            reply->getYiaddr().toText(),
+                            "myhost.example.com.",
+                            "00010132E91AA355CFBB753C0F0497A5A940436"
+                            "965B68B6D438D98E680BF10B09F3BCF",
+                            0, subnet_->getValid());
+
+    verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true,
+                            reply->getYiaddr().toText(),
+                            "otherhost.example.com.",
+                            "000101A5AEEA7498BD5AD9D3BF600E49FF39A7E3"
+                            "AFDCE8C3D0E53F35CC584DD63C89CA",
+                            0, subnet_->getValid());
+}
+
+// Test that when the Release message is sent for the previously acquired
+// lease, then server genenerates a NameChangeRequest to remove the entries
+// corresponding to the lease being released.
+TEST_F(FqdnDhcpv4SrvTest, processRequestRelease) {
+    Pkt4Ptr req = generatePktWithFqdn(DHCPREQUEST, Option4ClientFqdn::FLAG_S |
+                                      Option4ClientFqdn::FLAG_E,
+                                      "myhost.example.com.",
+                                      Option4ClientFqdn::FULL, true);
+
+    Pkt4Ptr reply;
+    ASSERT_NO_THROW(reply = srv_->processRequest(req));
+
+    checkResponse(reply, DHCPACK, 1234);
+
+    // Verify that there is one NameChangeRequest generated.
+    ASSERT_EQ(1, srv_->name_change_reqs_.size());
+    verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true,
+                            reply->getYiaddr().toText(), "myhost.example.com.",
+                            "00010132E91AA355CFBB753C0F0497A5A940436"
+                            "965B68B6D438D98E680BF10B09F3BCF",
+                            0, subnet_->getValid());
+
+    // Create a Release message.
+    Pkt4Ptr rel = Pkt4Ptr(new Pkt4(DHCPRELEASE, 1234));
+    rel->setCiaddr(reply->getYiaddr());
+    rel->setRemoteAddr(IOAddress("192.0.2.3"));
+    rel->addOption(generateClientId());
+    rel->addOption(srv_->getServerID());
+
+    ASSERT_NO_THROW(srv_->processRelease(rel));
+
+    // The lease has been removed, so there should be a NameChangeRequest to
+    // remove corresponding DNS entries.
+    ASSERT_EQ(1, srv_->name_change_reqs_.size());
+}
+
 } // end of anonymous namespace