[3035] Generate NameChangeRequest when new lease is acquired.

src/bin/dhcp4/dhcp4_srv.cc

@@ -570,43 +570,48 @@ Dhcpv4Srv::srvidToString(const OptionPtr& srvid) {
 }
 
 isc::dhcp_ddns::D2Dhcid
-Dhcpv4Srv::computeDhcid(const Pkt4Ptr& query, const Pkt4Ptr& answer) {
-    std::vector<uint8_t> dhcid_data(1);
-    OptionPtr client_id = answer->getOption(DHO_DHCP_CLIENT_IDENTIFIER);
-    if (client_id) {
-        dhcid_data.push_back(1);
-        dhcid_data.insert(dhcid_data.end(), client_id->getData().begin(),
-                          client_id->getData().end());
-    } else {
-        HWAddrPtr hwaddr = query->getHWAddr();
-        dhcid_data.push_back(0);
-        dhcid_data.push_back(hwaddr->htype_);
-        dhcid_data.insert(dhcid_data.end(), hwaddr->hwaddr_.begin(),
-                          hwaddr->hwaddr_.end());
+Dhcpv4Srv::computeDhcid(const Lease4Ptr& lease) {
+    if (!lease) {
+        isc_throw(DhcidComputeError, "a pointer to the lease must be not"
+                  " NULL to compute DHCID");
+
+    } else if (lease->hostname_.empty()) {
+        isc_throw(DhcidComputeError, "unable to compute the DHCID for the"
+                  " lease which has empty hostname set");
+
     }
 
-    std::string domain_name;
-    Option4ClientFqdnPtr fqdn = boost::dynamic_pointer_cast<Option4ClientFqdn>
-        (answer->getOption(DHO_FQDN));
-    if (fqdn) {
-        domain_name = fqdn->getDomainName();
+    // In order to compute DHCID the client's hostname must be encoded in
+    // canonical wire format. It is unlikely that the FQDN is malformed
+    // because it is validated by the classes which encapsulate options
+    // carrying client FQDN. However, if the client name was carried in the
+    // Hostname option it is more likely as it carries the hostname as a
+    // regular string.
+    std::vector<uint8_t> fqdn_wire;
+    try {
+        OptionDataTypeUtil::writeFqdn(lease->hostname_, fqdn_wire, true);
 
-    } else {
-        OptionCustomPtr hostname = boost::dynamic_pointer_cast<OptionCustom>
-            (answer->getOption(DHO_HOST_NAME));
-        if (hostname) {
-            domain_name = hostname->readString();
-        }
+    } catch (const Exception& ex) {
+        isc_throw(DhcidComputeError, "unable to compute DHCID because the"
+                  " hostname: " << lease->hostname_ << " is invalid");
 
     }
+
+    // Prefer client id to HW address to compute DHCID. If Client Id is
+    // NULL, use HW address.
     try {
-        OptionDataTypeUtil::writeFqdn(domain_name, dhcid_data, true);
+        if (lease->client_id_) {
+            return (D2Dhcid(lease->client_id_->getClientId(), fqdn_wire));
+
+        } else {
+            HWAddrPtr hwaddr(new HWAddr(lease->hwaddr_, HTYPE_ETHER));
+            return (D2Dhcid(hwaddr, fqdn_wire));
+        }
     } catch (const Exception& ex) {
-        ;
-    }
+        isc_throw(DhcidComputeError, "unable to compute DHCID: "
+                  << ex.what());
 
-    D2Dhcid dhcid(dhcid_data);
-    return (dhcid);
+    }
 
 }
 
@@ -841,6 +846,21 @@ Dhcpv4Srv::processHostnameOption(const Pkt4Ptr&, Pkt4Ptr&) {
 }
 
 void
+Dhcpv4Srv::createNameChangeRequests(const Lease4Ptr& lease,
+                                    const Lease4Ptr&) {
+    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);
+}
+
+void
 Dhcpv4Srv::assignLease(const Pkt4Ptr& question, Pkt4Ptr& answer) {
 
     // We need to select a subnet the client is connected in.

src/bin/dhcp4/dhcp4_srv.h

@@ -27,10 +27,18 @@
 #include <boost/noncopyable.hpp>
 
 #include <iostream>
+#include <queue>
 
 namespace isc {
 namespace dhcp {
 
+/// @brief Exception thrown when DHCID computation failed.
+class DhcidComputeError : public isc::Exception {
+public:
+    DhcidComputeError(const char* file, size_t line, const char* what) :
+        isc::Exception(file, line, what) { };
+};
+
 /// @brief DHCPv4 server service.
 ///
 /// This singleton class represents DHCPv4 server. It contains all
@@ -303,6 +311,24 @@ private:
     void processHostnameOption(const Pkt4Ptr& query, Pkt4Ptr& answer);
 
 protected:
+
+    /// @brief Creates NameChangeRequests which correspond to the lease
+    /// which has been acquired.
+    ///
+    /// If this function is called whe an existing lease is renewed, it
+    /// may generate NameChangeRequest to remove existing DNS entries which
+    /// correspond to the old lease instance. This function may cease to
+    /// generate NameChangeRequests if the notion of the client's FQDN hasn't
+    /// changed between an old and new lease.
+    ///
+    /// @param lease A pointer to the new lease which has been acquired.
+    /// @param old_lease A pointer to the instance of the old lease which has
+    /// been replaced by the new lease passed in the first argument. The NULL
+    /// value indicates that the new lease has been allocated, rather than
+    /// lease being renewed.
+    void createNameChangeRequests(const Lease4Ptr& lease,
+                                  const Lease4Ptr& old_lease);
+
     /// @brief Attempts to renew received addresses
     ///
     /// Attempts to renew existing lease. This typically includes finding a lease that
@@ -377,15 +403,11 @@ protected:
     /// @return string representation
     static std::string srvidToString(const OptionPtr& opt);
 
-    /// @brief Computes DHCID using options stored in the response message
-    /// to a client.
+    /// @brief Computes DHCID from a lease.
     ///
-    /// @param query An object encapsulating client's message to the server.
-    /// @param answer An object encapsulating response message being sent to
-    /// a client.
+    /// @param lease A pointer to the structure describing a lease.
     /// @return An object encapsulating DHCID to be used for DNS updates.
-    static isc::dhcp_ddns::D2Dhcid computeDhcid(const Pkt4Ptr& query,
-                                                const Pkt4Ptr& answer);
+    static isc::dhcp_ddns::D2Dhcid computeDhcid(const Lease4Ptr& lease);
 
     /// @brief Selects a subnet for a given client's packet.
     ///
@@ -433,6 +455,12 @@ private:
     int hook_index_pkt4_receive_;
     int hook_index_subnet4_select_;
     int hook_index_pkt4_send_;
+
+protected:
+
+    /// Holds a list of @c isc::dhcp_ddns::NameChangeRequest objects which
+    /// are waiting for sending  to b10-dhcp-ddns module.
+    std::queue<isc::dhcp_ddns::NameChangeRequest> name_change_reqs_;
 };
 
 }; // namespace isc::dhcp

src/bin/dhcp4/tests/dhcp4_test_utils.h

@@ -127,6 +127,8 @@ public:
     std::list<Pkt4Ptr> fake_sent_;
 
     using Dhcpv4Srv::adjustRemoteAddr;
+    using Dhcpv4Srv::computeDhcid;
+    using Dhcpv4Srv::createNameChangeRequests;
     using Dhcpv4Srv::processDiscover;
     using Dhcpv4Srv::processRequest;
     using Dhcpv4Srv::processRelease;
@@ -139,6 +141,7 @@ public:
     using Dhcpv4Srv::writeServerID;
     using Dhcpv4Srv::sanityCheck;
     using Dhcpv4Srv::srvidToString;
+    using Dhcpv4Srv::name_change_reqs_;
 };
 
 /// @brief Dummy Packet Filtering class.

src/bin/dhcp4/tests/fqdn_unittest.cc

@@ -16,6 +16,7 @@
 #include <asiolink/io_address.h>
 #include <dhcp/option4_client_fqdn.h>
 #include <dhcp4/tests/dhcp4_test_utils.h>
+#include <dhcp_ddns/ncr_msg.h>
 
 #include <gtest/gtest.h>
 #include <boost/scoped_ptr.hpp>
@@ -23,6 +24,7 @@
 using namespace isc;
 using namespace isc::asiolink;
 using namespace isc::dhcp;
+using namespace isc::dhcp_ddns;
 using namespace isc::test;
 
 namespace {
@@ -36,6 +38,24 @@ public:
         delete srv_;
     }
 
+    // Create a lease to be used by various tests.
+    Lease4Ptr createLease(const isc::asiolink::IOAddress& addr,
+                          const std::string& hostname,
+                          const bool fqdn_fwd,
+                          const bool fqdn_rev) {
+        const uint8_t hwaddr[] = { 0, 1, 2, 3, 4, 5, 6 };
+        Lease4Ptr lease(new Lease4(addr, hwaddr, sizeof(hwaddr),
+                                   &generateClientId()->getData()[0],
+                                   generateClientId()->getData().size(),
+                                   100, 50, 75, time(NULL), subnet_->getID()));
+        // @todo Set this through the Lease4 constructor.
+        lease->hostname_ = hostname;
+        lease->fqdn_fwd_ = fqdn_fwd;
+        lease->fqdn_rev_ = fqdn_rev;
+
+        return (lease);
+    }
+
     // Create an instance of the DHCPv4 Client FQDN Option.
     Option4ClientFqdnPtr
     createClientFqdn(const uint8_t flags,
@@ -59,7 +79,8 @@ public:
                                 const std::string& fqdn_domain_name,
                                 const Option4ClientFqdn::DomainNameType
                                 fqdn_type,
-                                const bool include_prl) {
+                                const bool include_prl,
+                                const bool include_clientid = true) {
         Pkt4Ptr pkt = Pkt4Ptr(new Pkt4(msg_type, 1234));
         pkt->setRemoteAddr(IOAddress("192.0.2.3"));
         // For DISCOVER we don't include server id, because client broadcasts
@@ -67,8 +88,10 @@ public:
         if (msg_type != DHCPDISCOVER) {
             pkt->addOption(srv_->getServerID());
         }
-        // Client id is required.
-        pkt->addOption(generateClientId());
+
+        if (include_clientid) {
+            pkt->addOption(generateClientId());
+        }
 
         // Create Client FQDN Option with the specified flags and
         // domain-name.
@@ -121,11 +144,87 @@ public:
 
     }
 
-private:
+    // Verify that NameChangeRequest holds valid values.
+    void verifyNameChangeRequest(const isc::dhcp_ddns::NameChangeType type,
+                                 const bool reverse, const bool forward,
+                                 const std::string& addr,
+                                 const std::string& dhcid,
+                                 const uint16_t expires,
+                                 const uint16_t len) {
+        NameChangeRequest ncr = srv_->name_change_reqs_.front();
+        EXPECT_EQ(type, ncr.getChangeType());
+        EXPECT_EQ(forward, ncr.isForwardChange());
+        EXPECT_EQ(reverse, ncr.isReverseChange());
+        EXPECT_EQ(addr, ncr.getIpAddress());
+        EXPECT_EQ(dhcid, ncr.getDhcid().toStr());
+        EXPECT_EQ(expires, ncr.getLeaseExpiresOn());
+        EXPECT_EQ(len, ncr.getLeaseLength());
+        EXPECT_EQ(isc::dhcp_ddns::ST_NEW, ncr.getStatus());
+        srv_->name_change_reqs_.pop();
+    }
+
     NakedDhcpv4Srv* srv_;
 
 };
 
+// Test that the exception is thrown if lease pointer specified as the argument
+// of computeDhcid function is NULL.
+TEST_F(FqdnDhcpv4SrvTest, dhcidNullLease) {
+    Lease4Ptr lease;
+    EXPECT_THROW(srv_->computeDhcid(lease), isc::dhcp::DhcidComputeError);
+
+}
+
+// Test that the appropriate exception is thrown if the lease object used
+// to compute DHCID comprises wrong hostname.
+TEST_F(FqdnDhcpv4SrvTest, dhcidWrongHostname) {
+    // First, make sure that the lease with the correct hostname is accepted.
+    Lease4Ptr lease = createLease(IOAddress("192.0.2.3"),
+                                  "myhost.example.com.", true, true);
+    ASSERT_NO_THROW(srv_->computeDhcid(lease));
+
+    // Now, use the wrong hostname. It should result in the exception.
+    lease->hostname_ = "myhost...example.com.";
+    EXPECT_THROW(srv_->computeDhcid(lease), isc::dhcp::DhcidComputeError);
+
+    // Also, empty hostname is wrong.
+    lease->hostname_ = "";
+    EXPECT_THROW(srv_->computeDhcid(lease), isc::dhcp::DhcidComputeError);
+}
+
+// Test that the DHCID is computed correctly, when the lease holds
+// correct hostname and non-NULL client id.
+TEST_F(FqdnDhcpv4SrvTest, dhcidComputeFromClientId) {
+    Lease4Ptr lease = createLease(IOAddress("192.0.2.3"),
+                                  "myhost.example.com.",
+                                  true, true);
+    isc::dhcp_ddns::D2Dhcid dhcid;
+    ASSERT_NO_THROW(dhcid = srv_->computeDhcid(lease));
+
+    // Make sure that the computed DHCID is valid.
+    std::string dhcid_ref = "00010132E91AA355CFBB753C0F0497A5A9404"
+        "36965B68B6D438D98E680BF10B09F3BCF";
+    EXPECT_EQ(dhcid_ref, dhcid.toStr());
+}
+
+// Test that the DHCID is computed correctly, when the lease holds correct
+// hostname and NULL client id.
+TEST_F(FqdnDhcpv4SrvTest, dhcidComputeFromHWAddr) {
+    Lease4Ptr lease = createLease(IOAddress("192.0.2.3"),
+                                  "myhost.example.com.",
+                                  true, true);
+    lease->client_id_.reset();
+
+    isc::dhcp_ddns::D2Dhcid dhcid;
+    ASSERT_NO_THROW(dhcid = srv_->computeDhcid(lease));
+
+    // Make sure that the computed DHCID is valid.
+    std::string dhcid_ref = "0000012247F6DC4423C3E8627434A9D6868609"
+        "D88948F78018B215EDCAA30C0C135035";
+    EXPECT_EQ(dhcid_ref, dhcid.toStr());
+}
+
+
 // Test that server confirms to perform the forward and reverse DNS update,
 // when client asks for it.
 TEST_F(FqdnDhcpv4SrvTest, serverUpdateForward) {
@@ -201,6 +300,34 @@ TEST_F(FqdnDhcpv4SrvTest, clientUpdateNotAllowed) {
 
 }
 
+// Test that exactly one NameChangeRequest is generated when the new lease
+// has been acquired (old lease is NULL).
+TEST_F(FqdnDhcpv4SrvTest, createNameChangeRequestsNewLease) {
+    Lease4Ptr lease = createLease(IOAddress("192.0.2.3"), "myhost.example.com.",
+                                  true, true);
+    Lease4Ptr old_lease;
+
+    ASSERT_NO_THROW(srv_->createNameChangeRequests(lease, old_lease));
+    ASSERT_EQ(1, srv_->name_change_reqs_.size());
+
+    verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true,
+                            "192.0.2.3", "00010132E91AA355CFBB753C0F049"
+                            "7A5A940436965B68B6D438D98E680BF10B09F3BCF",
+                            0, 100);
+}
+
+// Test that no NameChangeRequest is generated when a lease is renewed and
+// the FQDN data hasn't changed.
+TEST_F(FqdnDhcpv4SrvTest, createNameChangeRequestsRenewNoChange) {
+    Lease4Ptr lease = createLease(IOAddress("192.0.2.3"), "myhost.example.com.",
+                                  true, true);
+    Lease4Ptr old_lease = createLease(IOAddress("192.0.2.3"),
+                                      "myhost.example.com.", true, true);
+    old_lease->valid_lft_ += 100;
+
+    ASSERT_NO_THROW(srv_->createNameChangeRequests(lease, old_lease));
+    EXPECT_TRUE(srv_->name_change_reqs_.empty());
+}
 
 
 } // end of anonymous namespace