BIND 10 trac3329, updated. 6c21d3743e3e779bede1ebd6e4906697d3b0076a [3329] Dhcpv4 now sends NameChangeRequests to D2ClientMgr
BIND 10 source code commits
bind10-changes at lists.isc.org
Fri Feb 14 14:33:39 UTC 2014
The branch, trac3329 has been updated
via 6c21d3743e3e779bede1ebd6e4906697d3b0076a (commit)
from c03cc8c38810d3316faf205d8dda1d9bbbaab795 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit 6c21d3743e3e779bede1ebd6e4906697d3b0076a
Author: Thomas Markwalder <tmark at isc.org>
Date: Fri Feb 14 09:30:58 2014 -0500
[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.
-----------------------------------------------------------------------
Summary of changes:
src/bin/dhcp4/dhcp4_srv.cc | 32 ++++-----
src/bin/dhcp4/dhcp4_srv.h | 25 ++-----
src/bin/dhcp4/tests/d2_unittest.cc | 30 +++++++++
src/bin/dhcp4/tests/dhcp4_test_utils.h | 1 -
src/bin/dhcp4/tests/fqdn_unittest.cc | 114 +++++++++++++++++---------------
5 files changed, 109 insertions(+), 93 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc
index ace3cd8..0608b93 100644
--- a/src/bin/dhcp4/dhcp4_srv.cc
+++ b/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
diff --git a/src/bin/dhcp4/dhcp4_srv.h b/src/bin/dhcp4/dhcp4_srv.h
index ebb2f51..b08df2c 100644
--- a/src/bin/dhcp4/dhcp4_srv.h
+++ b/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
diff --git a/src/bin/dhcp4/tests/d2_unittest.cc b/src/bin/dhcp4/tests/d2_unittest.cc
index d8b4ee8..f657b71 100644
--- a/src/bin/dhcp4/tests/d2_unittest.cc
+++ b/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
diff --git a/src/bin/dhcp4/tests/dhcp4_test_utils.h b/src/bin/dhcp4/tests/dhcp4_test_utils.h
index 5090c7d..e8b0204 100644
--- a/src/bin/dhcp4/tests/dhcp4_test_utils.h
+++ b/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;
diff --git a/src/bin/dhcp4/tests/fqdn_unittest.cc b/src/bin/dhcp4/tests/fqdn_unittest.cc
index f1cb7b0..4215e92 100644
--- a/src/bin/dhcp4/tests/fqdn_unittest.cc
+++ b/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());
}
More information about the bind10-changes
mailing list