From cd39b7dd1c64a006122adbec1cb732028e4d8037 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Tue, 8 Sep 2020 15:10:10 -0400 Subject: [PATCH] [#1409] Clear DNS fields when reusing expired v4 leases Clearing DNS fields after we do the remove ensures that we'll do an add if the client is needs new DNS entries , while avoiding duplicate DNS removes. src/bin/dhcp4/tests/dhcp4_client.cc Dhcp4Client::includeHostname() - reset hostname if given an empty parameter src/bin/dhcp4/tests/fqdn_unittest.cc TEST_F(NameDhcpv4SrvTest, processReuseExpired) - new test src/lib/dhcpsrv/alloc_engine.cc AllocEngine::reclaimExpiredLease(Lease4Ptr...) - always clear the DNS fields after the call to queue and CHG_REMOVE AllocEngine::allocateOrReuseLease4() - clear DNS fields in the old lease so we don't trigger redundant removes --- src/bin/dhcp4/tests/dhcp4_client.cc | 7 +- src/bin/dhcp4/tests/fqdn_unittest.cc | 174 ++++++++++++++++++++++++++- src/lib/dhcpsrv/alloc_engine.cc | 10 ++ 3 files changed, 184 insertions(+), 7 deletions(-) diff --git a/src/bin/dhcp4/tests/dhcp4_client.cc b/src/bin/dhcp4/tests/dhcp4_client.cc index a23c915973..56813aae5f 100644 --- a/src/bin/dhcp4/tests/dhcp4_client.cc +++ b/src/bin/dhcp4/tests/dhcp4_client.cc @@ -449,10 +449,13 @@ Dhcp4Client::includeFQDN(const uint8_t flags, const std::string& fqdn_name, void Dhcp4Client::includeHostname(const std::string& name) { - hostname_.reset(new OptionString(Option::V4, DHO_HOST_NAME, name)); + if (name.empty()) { + hostname_.reset(); + } else { + hostname_.reset(new OptionString(Option::V4, DHO_HOST_NAME, name)); + } } - HWAddrPtr Dhcp4Client::generateHWAddr(const uint8_t htype) const { if (htype != HTYPE_ETHER) { diff --git a/src/bin/dhcp4/tests/fqdn_unittest.cc b/src/bin/dhcp4/tests/fqdn_unittest.cc index 2853ec213c..70c08a2304 100644 --- a/src/bin/dhcp4/tests/fqdn_unittest.cc +++ b/src/bin/dhcp4/tests/fqdn_unittest.cc @@ -249,6 +249,23 @@ const char* CONFIGS[] = { " \"ddns-send-updates\": true\n" " }\n" "]\n" + "}", + // 9 + // Simple config with DDNS updates enabled. Note pool is one address + // large to ensure we get a specific address back. + "{ \"interfaces-config\": {" + " \"interfaces\": [ \"*\" ]" + "}," + "\"valid-lifetime\": 3000," + "\"subnet4\": [ { " + " \"subnet\": \"192.0.2.0/24\", " + " \"id\": 1," + " \"pools\": [ { \"pool\": \"192.0.2.10-192.0.2.10\" } ]" + " }]," + "\"dhcp-ddns\": {" + "\"enable-updates\": true," + "\"qualifying-suffix\": \"fake-suffix.isc.org.\"" + "}" "}" }; @@ -258,7 +275,7 @@ public: D2ClientMgr& d2_mgr_; /// @brief Pointer to the DHCP server instance. - NakedDhcpv4Srv* srv_; + boost::shared_ptr srv_; /// @brief Interface Manager's fake configuration control. IfaceMgrTestConfig iface_mgr_test_config_; @@ -285,17 +302,15 @@ public: NameDhcpv4SrvTest() : Dhcpv4SrvTest(), d2_mgr_(CfgMgr::instance().getD2ClientMgr()), - srv_(NULL), iface_mgr_test_config_(true) { - srv_ = new NakedDhcpv4Srv(0); + srv_ = boost::shared_ptr(new NakedDhcpv4Srv(0)); IfaceMgr::instance().openSockets4(); // Config DDNS to be enabled, all controls off enableD2(); } virtual ~NameDhcpv4SrvTest() { - delete srv_; // CfgMgr singleton doesn't get wiped between tests, so we'll // disable D2 explicitly between tests. disableD2(); @@ -586,7 +601,7 @@ public: // Create the configuration and configure the server char config_buf[1024]; sprintf(config_buf, config_template, mode); - ASSERT_NO_THROW(configure(config_buf, srv_)) << "configuration failed"; + ASSERT_NO_THROW(configure(config_buf, *srv_)) << "configuration failed"; // Build our client packet Pkt4Ptr query; @@ -2253,4 +2268,153 @@ TEST_F(NameDhcpv4SrvTest, ddnsScopeTest) { time(NULL), 7200, true); } +// Verifies that when reusing an expired lesae, whether or not it is given to its +// original owner or not, appropriate DNS updates are done if needed. +TEST_F(NameDhcpv4SrvTest, processReuseExpired) { + IfaceMgrTestConfig test_config(true); + IfaceMgr::instance().openSockets4(); + + // Configure with a one address pool with DDNS enabled. + configure(CONFIGS[9], *srv_); + + struct Scenario { + std::string label_; + std::string client_id1_; + std::string dhcid1_; + std::string hostname1_; + + std::string client_id2_; + std::string dhcid2_; + std::string hostname2_; + bool expect_remove_; + bool expect_add_; + }; + + std::string cid1 = "11:11:11:11"; + std::string dhcid1 = "0001013904F56A5BD4E926EB6BC36C825CEA1159FF2AFBE28E4391E67CC040F6A35785"; + std::string cid2 = "22:22:22:22"; + std::string dhcid2 = "000101459343356AC37A73A372ECE989F9C4397E7FBBD6658239EA4B3B77B6B904A46F"; + bool remove = true; + bool add = true; + + std::vector scenarios = { + { + "same client, hostname added", + cid1, dhcid1, "", + cid1, dhcid1, "one.example.com.", + !remove, add + }, + { + "same client, same host", + cid1, dhcid1, "one.example.com.", + cid1, dhcid1, "one.example.com.", + remove, add + }, + { + "same client, hostname removed", + cid1, dhcid1, "one.example.com.", + cid1, dhcid1, "", + remove, !add + }, + { + "different client, hostname added", + cid1, dhcid1, "", + cid2, dhcid2, "two.example.com.", + !remove, add + }, + { + "different client, different host", + cid1, dhcid1, "one.example.com.", + cid2, dhcid2, "two.example.com.", + remove, add + }, + { + "different client, hostname removed", + cid1, dhcid1, "one.example.com.", + cid2, dhcid2, "", + remove, !add + } + }; + + for (auto scenario : scenarios) { + SCOPED_TRACE(scenario.label_); + { + // Create the original leasing client. + boost::shared_ptr client1(new Dhcp4Client(srv_, Dhcp4Client::SELECTING)); + client1->setIfaceName("eth1"); + client1->setIfaceIndex(ETH1_INDEX); + client1->includeClientId(scenario.client_id1_); + if (!scenario.hostname1_.empty()) { + ASSERT_NO_THROW(client1->includeHostname(scenario.hostname1_)); + } + + ASSERT_NO_THROW(client1->doDORA()); + Pkt4Ptr resp = client1->getContext().response_; + ASSERT_TRUE(resp); + ASSERT_EQ(DHCPACK, static_cast(resp->getType())); + + // Verify that there is one NameChangeRequest generated. + if (scenario.hostname1_.empty()) { + ASSERT_EQ(0, d2_mgr_.getQueueSize()); + } else { + ASSERT_EQ(1, d2_mgr_.getQueueSize()); + verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, + true, true, + resp->getYiaddr().toText(), scenario.hostname1_, + scenario.dhcid1_, + time(NULL), subnet_->getValid(), true); + } + + // Expire the lease + Lease4Ptr lease = LeaseMgrFactory::instance().getLease4(resp->getYiaddr()); + ASSERT_TRUE(lease); + lease->cltt_ = 0; + ASSERT_NO_THROW(LeaseMgrFactory::instance().updateLease4(lease)); + lease = LeaseMgrFactory::instance().getLease4(resp->getYiaddr()); + ASSERT_TRUE(lease->expired()); + + // Create the requesting/returning client. + boost::shared_ptr client2; + if (scenario.client_id1_ == scenario.client_id2_) { + client2 = client1; + } else { + client2.reset(new Dhcp4Client(srv_, Dhcp4Client::SELECTING)); + client2->setIfaceName("eth1"); + client2->setIfaceIndex(ETH1_INDEX); + client2->includeClientId(scenario.client_id2_); + } + + ASSERT_NO_THROW(client2->includeHostname(scenario.hostname2_)); + + ASSERT_NO_THROW(client2->doDORA()); + resp = client2->getContext().response_; + ASSERT_TRUE(resp); + ASSERT_EQ(DHCPACK, static_cast(resp->getType())); + + // Verify that there is one NameChangeRequest generated. + size_t expected_count = (scenario.expect_remove_ ? 1 : 0) + + (scenario.expect_add_ ? 1 : 0); + + ASSERT_EQ(expected_count, d2_mgr_.getQueueSize()); + if (scenario.expect_remove_) { + verifyNameChangeRequest(isc::dhcp_ddns::CHG_REMOVE, + true, true, + resp->getYiaddr().toText(), scenario.hostname1_, + scenario.dhcid1_, + time(NULL), subnet_->getValid(), true); + } + + if (scenario.expect_add_) { + verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, + true, true, + resp->getYiaddr().toText(), scenario.hostname2_, + scenario.dhcid2_, + time(NULL), subnet_->getValid(), true); + } + + ASSERT_NO_THROW(LeaseMgrFactory::instance().deleteLease(lease)); + } + } +} + } // end of anonymous namespace diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index 84c025a028..274bba7f65 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -2665,6 +2665,10 @@ AllocEngine::reclaimExpiredLease(const Lease4Ptr& lease, // This will return immediately if the DNS wasn't updated // when the lease was created. queueNCR(CHG_REMOVE, lease); + // Clear DNS fields so we avoid redundant removes. + lease->hostname_.clear(); + lease->fqdn_fwd_ = false; + lease->fqdn_rev_ = false; // Let's check if the lease that just expired is in DECLINED state. // If it is, we need to perform a couple extra steps. @@ -3961,6 +3965,12 @@ AllocEngine::allocateOrReuseLease4(const IOAddress& candidate, ClientContext4& c if (exist_lease) { if (exist_lease->expired()) { ctx.old_lease_ = Lease4Ptr(new Lease4(*exist_lease)); + // reuseExpiredLease4() will reclaim the use which will + // queue an NCR remove it needed. Clear the DNS fields in + // the old lease to avoid a redundant remove in server logic. + ctx.old_lease_->hostname_.clear(); + ctx.old_lease_->fqdn_fwd_ = false; + ctx.old_lease_->fqdn_rev_ = false; return (reuseExpiredLease4(exist_lease, ctx, callout_status)); } else {