[3329] Dhcpv4 now sends NameChangeRequests to D2ClientMgr

Dhcpv4Srv now sends NameChangeRequests to D2ClientMgr rather than posting
them to an internal queue.  At this point, b10-dhcp4 should generate requests
and ship them to b10-dhcp-ddns when configured appropriately.

src/bin/dhcp4/dhcp4_srv.cc

@@ -420,9 +420,6 @@ Dhcpv4Srv::run() {
             LOG_ERROR(dhcp4_logger, DHCP4_PACKET_SEND_FAIL)
                 .arg(e.what());
         }
-
-        // Send NameChangeRequests to the b10-dhcp_ddns module.
-        sendNameChangeRequests();
     }
 
     return (true);
@@ -881,26 +878,23 @@ queueNameChangeRequest(const isc::dhcp_ddns::NameChangeType chg_type,
             .arg(ex.what());
         return;
     }
+
     // Create NameChangeRequest
-    NameChangeRequest ncr(chg_type, lease->fqdn_fwd_, lease->fqdn_rev_,
-                          lease->hostname_, lease->addr_.toText(),
-                          dhcid, lease->cltt_ + lease->valid_lft_,
-                          lease->valid_lft_);
-    // And queue it.
+    NameChangeRequestPtr ncr(new NameChangeRequest(chg_type, lease->fqdn_fwd_,
+                                                   lease->fqdn_rev_,
+                                                   lease->hostname_,
+                                                   lease->addr_.toText(),
+                                                   dhcid,
+                                                   (lease->cltt_ +
+                                                    lease->valid_lft_),
+                                                   lease->valid_lft_));
+
     LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL_DATA, DHCP4_QUEUE_NCR)
         .arg(chg_type == CHG_ADD ? "add" : "remove")
-        .arg(ncr.toText());
-    name_change_reqs_.push(ncr);
-}
+        .arg(ncr->toText());
 
-void
-Dhcpv4Srv::sendNameChangeRequests() {
-    while (!name_change_reqs_.empty()) {
-        /// @todo Once next NameChangeRequest is picked from the queue
-        /// we should send it to the b10-dhcp_ddns module. Currently we
-        /// just drop it.
-        name_change_reqs_.pop();
-    }
+    // And pass it to the the manager.
+    CfgMgr::instance().getD2ClientMgr().sendRequest(ncr);
 }
 
 void

src/bin/dhcp4/dhcp4_srv.h

@@ -480,10 +480,10 @@ protected:
     /// @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.
+    /// This creates the @c isc::dhcp_ddns::NameChangeRequest; emits a
+    /// the debug message which indicates whether the request being added is
+    /// to remove DNS entry or add a new entry; and then sends the request
+    /// to the D2ClientMgr for transmission to b10-dhcp-ddns.
     ///
     /// @param chg_type A type of the NameChangeRequest (ADD or REMOVE).
     /// @param lease A lease for which the NameChangeRequest is created and
@@ -491,17 +491,6 @@ protected:
     void queueNameChangeRequest(const isc::dhcp_ddns::NameChangeType chg_type,
                                 const Lease4Ptr& lease);
 
-    /// @brief Sends all outstanding NameChangeRequests to b10-dhcp-ddns module.
-    ///
-    /// The purpose of this function is to pick all outstanding
-    /// NameChangeRequests from the FIFO queue and send them to b10-dhcp-ddns
-    /// module.
-    ///
-    /// @todo Currently this function simply removes all requests from the
-    /// queue but doesn't send them anywhere. In the future, the
-    /// NameChangeSender will be used to deliver requests to the other module.
-    void sendNameChangeRequests();
-
     /// @brief Attempts to renew received addresses
     ///
     /// Attempts to renew existing lease. This typically includes finding a lease that
@@ -711,12 +700,6 @@ 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/d2_unittest.cc

@@ -320,6 +320,36 @@ TEST_F(Dhcp4SrvD2Test, forceUDPSendFailure) {
     EXPECT_EQ(2, mgr.getQueueSize());
 }
 
+// Tests error handling of D2ClientMgr::sendRequest() failure
+// by attempting to queue maximum number of messages.
+TEST_F(Dhcp4SrvD2Test, queueMaxError) {
+    // Configure it enabled and start it.
+    dhcp::D2ClientMgr& mgr = CfgMgr::instance().getD2ClientMgr();
+    ASSERT_NO_FATAL_FAILURE(configureD2(true));
+    ASSERT_TRUE(mgr.ddnsEnabled());
+    ASSERT_NO_THROW(srv_.startD2());
+    ASSERT_TRUE(mgr.amSending());
+
+    // Attempt to queue more then the maximum allowed.
+    int max_msgs = mgr.getQueueMaxSize();
+    for (int i = 0; i < max_msgs + 1; i++) {
+        dhcp_ddns::NameChangeRequestPtr ncr = buildTestNcr(i + 1);
+        ASSERT_NO_THROW(mgr.sendRequest(ncr));
+    }
+
+    // Stopping sender will coπplete the first message so there
+    // should be max less one.
+    EXPECT_EQ(max_msgs - 1, mgr.getQueueSize());
+
+    // Verify the error handler was invoked.
+    EXPECT_EQ(1, srv_.error_count_);
+
+    // Verify that updates are disabled and we are no longer sending.
+    ASSERT_FALSE(mgr.ddnsEnabled());
+    ASSERT_FALSE(mgr.amSending());
+}
+
+
 } // namespace test
 } // namespace dhcp
 } // namespace isc

src/bin/dhcp4/tests/dhcp4_test_utils.h

@@ -195,7 +195,6 @@ public:
     using Dhcpv4Srv::sanityCheck;
     using Dhcpv4Srv::srvidToString;
     using Dhcpv4Srv::unpackOptions;
-    using Dhcpv4Srv::name_change_reqs_;
     using Dhcpv4Srv::classifyPacket;
     using Dhcpv4Srv::accept;
     using Dhcpv4Srv::acceptMessageType;

src/bin/dhcp4/tests/fqdn_unittest.cc

@@ -34,13 +34,17 @@ namespace {
 
 class NameDhcpv4SrvTest : public Dhcpv4SrvTest {
 public:
+    // Reference to D2ClientMgr singleton
+    D2ClientMgr& d2_mgr_;
+
     // Bit Constants for turning on and off DDNS configuration options.
     static const uint16_t ALWAYS_INCLUDE_FQDN = 1;
     static const uint16_t OVERRIDE_NO_UPDATE = 2;
     static const uint16_t OVERRIDE_CLIENT_UPDATE = 4;
     static const uint16_t REPLACE_CLIENT_NAME = 8;
 
-    NameDhcpv4SrvTest() : Dhcpv4SrvTest() {
+    NameDhcpv4SrvTest() : Dhcpv4SrvTest(),
+        d2_mgr_(CfgMgr::instance().getD2ClientMgr()) {
         srv_ = new NakedDhcpv4Srv(0);
         // Config DDNS to be enabled, all controls off
         enableD2();
@@ -48,6 +52,9 @@ public:
 
     virtual ~NameDhcpv4SrvTest() {
         delete srv_;
+        // CfgMgr singleton doesn't get wiped between tests, so  we'll
+        // disable D2 explictly between tests.
+        disableD2();
     }
 
     /// @brief Sets the server's DDNS configuration to ddns updates disabled.
@@ -69,15 +76,15 @@ public:
         D2ClientConfigPtr cfg;
 
         ASSERT_NO_THROW(cfg.reset(new D2ClientConfig(true,
-                                  isc::asiolink::IOAddress("192.0.2.1"), 477,
+                                  isc::asiolink::IOAddress("127.0.0.1"), 53001,
                                   dhcp_ddns::NCR_UDP, dhcp_ddns::FMT_JSON,
                                   (mask & ALWAYS_INCLUDE_FQDN),
                                   (mask & OVERRIDE_NO_UPDATE),
                                   (mask & OVERRIDE_CLIENT_UPDATE),
                                   (mask & REPLACE_CLIENT_NAME),
                                   "myhost", "example.com")));
-
-        CfgMgr::instance().setD2ClientConfig(cfg);
+        ASSERT_NO_THROW(CfgMgr::instance().setD2ClientConfig(cfg));
+        ASSERT_NO_THROW(srv_->startD2());
     }
 
     // Create a lease to be used by various tests.
@@ -317,16 +324,19 @@ public:
                                  const time_t cltt,
                                  const uint16_t len,
                                  const bool not_strict_expire_check = false) {
-        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(fqdn, ncr.getFqdn());
+        NameChangeRequestPtr ncr;
+        ASSERT_NO_THROW(ncr = d2_mgr_.peekAt(0));
+        ASSERT_TRUE(ncr);
+
+        EXPECT_EQ(type, ncr->getChangeType());
+        EXPECT_EQ(forward, ncr->isForwardChange());
+        EXPECT_EQ(reverse, ncr->isReverseChange());
+        EXPECT_EQ(addr, ncr->getIpAddress());
+        EXPECT_EQ(fqdn, ncr->getFqdn());
         // Compare dhcid if it is not empty. In some cases, the DHCID is
         // not known in advance and can't be compared.
         if (!dhcid.empty()) {
-            EXPECT_EQ(dhcid, ncr.getDhcid().toStr());
+            EXPECT_EQ(dhcid, ncr->getDhcid().toStr());
         }
         // In some cases, the test doesn't have access to the last transmission
         // time for the particular client. In such cases, the test can use the
@@ -334,13 +344,15 @@ public:
         // for equality but rather check that the lease expiration time is not
         // greater than the current time + lease lifetime.
         if (not_strict_expire_check) {
-            EXPECT_GE(cltt + len, ncr.getLeaseExpiresOn());
+            EXPECT_GE(cltt + len, ncr->getLeaseExpiresOn());
         } else {
-            EXPECT_EQ(cltt + len, ncr.getLeaseExpiresOn());
+            EXPECT_EQ(cltt + len, ncr->getLeaseExpiresOn());
         }
-        EXPECT_EQ(len, ncr.getLeaseLength());
-        EXPECT_EQ(isc::dhcp_ddns::ST_NEW, ncr.getStatus());
-        srv_->name_change_reqs_.pop();
+        EXPECT_EQ(len, ncr->getLeaseLength());
+        EXPECT_EQ(isc::dhcp_ddns::ST_NEW, ncr->getStatus());
+
+        // Process the message off the queue
+        ASSERT_NO_THROW(d2_mgr_.runReadyIO());
     }
 
 
@@ -373,20 +385,23 @@ public:
         checkResponse(reply, DHCPACK, 1234);
         checkFqdnFlags(reply, response_flags);
 
-        // There should be an NCR only if response S flag is 1.
-        /// @todo This logic will need to change if forward and reverse
-        /// updates are ever controlled independently.
-        if ((response_flags & Option4ClientFqdn::FLAG_S) == 0) {
-            ASSERT_EQ(0, srv_->name_change_reqs_.size());
-        } else {
-            // Verify that there is one NameChangeRequest generated as expected.
-            ASSERT_EQ(1, srv_->name_change_reqs_.size());
-            verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true,
-                                    reply->getYiaddr().toText(),
-                                    "myhost.example.com.",
-                                    "", // empty DHCID means don't check it
-                                    time(NULL) + subnet_->getValid(),
-                                    subnet_->getValid(), true);
+        // NCRs cannot be sent to the d2_mgr unless updates are enabled.
+        if (d2_mgr_.ddnsEnabled()) {
+            // There should be an NCR only if response S flag is 1.
+            /// @todo This logic will need to change if forward and reverse
+            /// updates are ever controlled independently.
+            if ((response_flags & Option4ClientFqdn::FLAG_S) == 0) {
+                ASSERT_EQ(0, d2_mgr_.getQueueSize());
+            } else {
+                // Verify that there is one NameChangeRequest as expected.
+                ASSERT_EQ(1, d2_mgr_.getQueueSize());
+                verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true,
+                                        reply->getYiaddr().toText(),
+                                        "myhost.example.com.",
+                                        "", // empty DHCID means don't check it
+                                        time(NULL) + subnet_->getValid(),
+                                        subnet_->getValid(), true);
+            }
         }
     }
 
@@ -639,7 +654,7 @@ TEST_F(NameDhcpv4SrvTest, createNameChangeRequestsNewLease) {
     Lease4Ptr old_lease;
 
     ASSERT_NO_THROW(srv_->createNameChangeRequests(lease, old_lease));
-    ASSERT_EQ(1, srv_->name_change_reqs_.size());
+    ASSERT_EQ(1, d2_mgr_.getQueueSize());
 
     verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true,
                             "192.0.2.3", "myhost.example.com.",
@@ -658,7 +673,7 @@ TEST_F(NameDhcpv4SrvTest, createNameChangeRequestsRenewNoChange) {
     old_lease->valid_lft_ += 100;
 
     ASSERT_NO_THROW(srv_->createNameChangeRequests(lease, old_lease));
-    EXPECT_TRUE(srv_->name_change_reqs_.empty());
+    ASSERT_EQ(0, d2_mgr_.getQueueSize());
 }
 
 // Test that no NameChangeRequest is generated when forward and reverse
@@ -671,7 +686,7 @@ TEST_F(NameDhcpv4SrvTest, createNameChangeRequestsNoUpdate) {
                                    "lease2.example.com.",
                                    false, false);
     ASSERT_NO_THROW(srv_->createNameChangeRequests(lease2, lease1));
-    EXPECT_EQ(1, srv_->name_change_reqs_.size());
+    EXPECT_EQ(1, d2_mgr_.getQueueSize());
 
     verifyNameChangeRequest(isc::dhcp_ddns::CHG_REMOVE, true, true,
                             "192.0.2.3", "lease1.example.com.",
@@ -683,7 +698,7 @@ TEST_F(NameDhcpv4SrvTest, createNameChangeRequestsNoUpdate) {
     lease2->fqdn_rev_ = true;
     lease2->fqdn_fwd_ = true;
     ASSERT_NO_THROW(srv_->createNameChangeRequests(lease2, lease1));
-    EXPECT_EQ(1, srv_->name_change_reqs_.size());
+    EXPECT_EQ(1, d2_mgr_.getQueueSize());
 
 }
 
@@ -697,7 +712,7 @@ TEST_F(NameDhcpv4SrvTest, createNameChangeRequestsRenew) {
                                    "lease2.example.com.",
                                    true, true);
     ASSERT_NO_THROW(srv_->createNameChangeRequests(lease2, lease1));
-    ASSERT_EQ(2, srv_->name_change_reqs_.size());
+    ASSERT_EQ(2, d2_mgr_.getQueueSize());
 
     verifyNameChangeRequest(isc::dhcp_ddns::CHG_REMOVE, true, true,
                             "192.0.2.3", "lease1.example.com.",
@@ -742,7 +757,7 @@ TEST_F(NameDhcpv4SrvTest, processDiscover) {
     ASSERT_NO_THROW(reply = srv_->processDiscover(req));
     checkResponse(reply, DHCPOFFER, 1234);
 
-    EXPECT_TRUE(srv_->name_change_reqs_.empty());
+    EXPECT_EQ(0, d2_mgr_.getQueueSize());
 }
 
 // Test that server generates client's hostname from the IP address assigned
@@ -761,7 +776,7 @@ TEST_F(NameDhcpv4SrvTest, processRequestFqdnEmptyDomainName) {
     checkResponse(reply, DHCPACK, 1234);
 
     // Verify that there is one NameChangeRequest generated.
-    ASSERT_EQ(1, srv_->name_change_reqs_.size());
+    ASSERT_EQ(1, d2_mgr_.getQueueSize());
 
     // The hostname is generated from the IP address acquired (yiaddr).
     std::string hostname = generatedNameFromAddress(reply->getYiaddr());
@@ -818,7 +833,7 @@ TEST_F(NameDhcpv4SrvTest, processRequestEmptyHostname) {
     checkResponse(reply, DHCPACK, 1234);
 
     // Verify that there is one NameChangeRequest generated.
-    ASSERT_EQ(1, srv_->name_change_reqs_.size());
+    ASSERT_EQ(1, d2_mgr_.getQueueSize());
 
     // The hostname is generated from the IP address acquired (yiaddr).
     std::string hostname = generatedNameFromAddress(reply->getYiaddr());
@@ -848,7 +863,7 @@ TEST_F(NameDhcpv4SrvTest, processTwoRequestsFqdn) {
     checkResponse(reply, DHCPACK, 1234);
 
     // Verify that there is one NameChangeRequest generated.
-    ASSERT_EQ(1, srv_->name_change_reqs_.size());
+    ASSERT_EQ(1, d2_mgr_.getQueueSize());
     verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true,
                             reply->getYiaddr().toText(), "myhost.example.com.",
                             "00010132E91AA355CFBB753C0F0497A5A940436"
@@ -868,7 +883,7 @@ TEST_F(NameDhcpv4SrvTest, processTwoRequestsFqdn) {
     checkResponse(reply, DHCPACK, 1234);
 
     // There should be two NameChangeRequests. Verify that they are valid.
-    ASSERT_EQ(2, srv_->name_change_reqs_.size());
+    ASSERT_EQ(2, d2_mgr_.getQueueSize());
     verifyNameChangeRequest(isc::dhcp_ddns::CHG_REMOVE, true, true,
                             reply->getYiaddr().toText(),
                             "myhost.example.com.",
@@ -905,7 +920,7 @@ TEST_F(NameDhcpv4SrvTest, processTwoRequestsHostname) {
     checkResponse(reply, DHCPACK, 1234);
 
     // Verify that there is one NameChangeRequest generated.
-    ASSERT_EQ(1, srv_->name_change_reqs_.size());
+    ASSERT_EQ(1, d2_mgr_.getQueueSize());
     verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true,
                             reply->getYiaddr().toText(), "myhost.example.com.",
                             "00010132E91AA355CFBB753C0F0497A5A940436"
@@ -926,7 +941,7 @@ TEST_F(NameDhcpv4SrvTest, processTwoRequestsHostname) {
     checkResponse(reply, DHCPACK, 1234);
 
     // There should be two NameChangeRequests. Verify that they are valid.
-    ASSERT_EQ(2, srv_->name_change_reqs_.size());
+    ASSERT_EQ(2, d2_mgr_.getQueueSize());
     verifyNameChangeRequest(isc::dhcp_ddns::CHG_REMOVE, true, true,
                             reply->getYiaddr().toText(),
                             "myhost.example.com.",
@@ -962,7 +977,7 @@ TEST_F(NameDhcpv4SrvTest, processRequestRelease) {
     checkResponse(reply, DHCPACK, 1234);
 
     // Verify that there is one NameChangeRequest generated for lease.
-    ASSERT_EQ(1, srv_->name_change_reqs_.size());
+    ASSERT_EQ(1, d2_mgr_.getQueueSize());
     verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true,
                             reply->getYiaddr().toText(), "myhost.example.com.",
                             "00010132E91AA355CFBB753C0F0497A5A940436"
@@ -979,7 +994,7 @@ TEST_F(NameDhcpv4SrvTest, processRequestRelease) {
 
     // The lease has been removed, so there should be a NameChangeRequest to
     // remove corresponding DNS entries.
-    ASSERT_EQ(1, srv_->name_change_reqs_.size());
+    ASSERT_EQ(1, d2_mgr_.getQueueSize());
     verifyNameChangeRequest(isc::dhcp_ddns::CHG_REMOVE, true, true,
                             reply->getYiaddr().toText(), "myhost.example.com.",
                             "00010132E91AA355CFBB753C0F0497A5A940436"
@@ -990,6 +1005,9 @@ TEST_F(NameDhcpv4SrvTest, processRequestRelease) {
 // Test that when the Release message is sent for a previously acquired lease
 // and DDNS updates are disabled that server does NOT generate a
 // NameChangeRequest to remove entries corresponding to the released lease.
+// Queue size is not available when updates are not enabled, however,
+// attempting to send a NCR when updates disabled will result in a throw.
+// If no throws are experienced then no attempt was made to send a NCR.
 TEST_F(NameDhcpv4SrvTest, processRequestReleaseUpdatesDisabled) {
     // Create fake interfaces and open fake sockets.
     IfaceMgrTestConfig test_config(true);
@@ -1008,10 +1026,6 @@ TEST_F(NameDhcpv4SrvTest, processRequestReleaseUpdatesDisabled) {
     ASSERT_NO_THROW(reply = srv_->processRequest(req));
     checkResponse(reply, DHCPACK, 1234);
 
-    // With DDNS updates disabled, there should be not be a NameChangeRequest
-    // for the add.
-    ASSERT_EQ(0, srv_->name_change_reqs_.size());
-
     // Create and process the Release message.
     Pkt4Ptr rel = Pkt4Ptr(new Pkt4(DHCPRELEASE, 1234));
     rel->setCiaddr(reply->getYiaddr());
@@ -1019,10 +1033,6 @@ TEST_F(NameDhcpv4SrvTest, processRequestReleaseUpdatesDisabled) {
     rel->addOption(generateClientId());
     rel->addOption(srv_->getServerID());
     ASSERT_NO_THROW(srv_->processRelease(rel));
-
-    // With DDNS updates disabled, there should be not be a NameChangeRequest
-    // for the remove.
-    ASSERT_EQ(0, srv_->name_change_reqs_.size());
 }