From: Thomas Markwalder Date: Wed, 4 Nov 2020 19:05:58 +0000 (-0500) Subject: [#1419] Backported #1409 to v1_8_0 X-Git-Tag: Kea-1.8.1~5 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=cd7fd97179c1683070762e52c106f055c7ada2f3;p=thirdparty%2Fkea.git [#1419] Backported #1409 to v1_8_0 modified: ChangeLog src/bin/dhcp4/tests/dhcp4_client.cc src/bin/dhcp4/tests/fqdn_unittest.cc src/lib/dhcpsrv/alloc_engine.cc --- diff --git a/ChangeLog b/ChangeLog index 73f6319aad..ce5f2b557e 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,12 @@ +1799. [bug] tmark + kea-dhcp4 now correctly updates DNS when a client + returns for lease after the lease has expired. Prior + to this, the server would remove the entries but then + fail to add them unless the hostname (or FQDN) changed. + This change also eliminates redundant DNS removes when + expired leases are reclaimed and given to different clients. + (Gitlab #1419,#1409) + 1798. [func] razvan,tmark Removed "Multithreading is experimental" warning log message from kea-dhcp4 and kea-dhcp6 servers. 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..66792a0c9d 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::make_shared(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 lease, 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 c63e9093a9..134df34930 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -2671,6 +2671,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. @@ -3972,6 +3976,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 {