diff --git a/ChangeLog b/ChangeLog index 651d84d496..139679e696 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +2303. [bug] tmark + Modified both kea-dhcp4 and kea-dhcp6 to avoid + generating DDNS update requests when leases are + being reused due to lease caching. + (Gitlab #3257) + Kea 2.7.4 (development) released on October 30, 2024 2302. [func] tmark diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index d63e03243b..8942c15768 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -2893,7 +2893,8 @@ Dhcpv4Srv::createNameChangeRequests(const Lease4Ptr& lease, return; } - if (!old_lease || ddns_params.getUpdateOnRenew() || !lease->hasIdenticalFqdn(*old_lease)) { + if ((!lease->reuseable_valid_lft_) && + (!old_lease || ddns_params.getUpdateOnRenew() || !lease->hasIdenticalFqdn(*old_lease))) { if (old_lease) { // Queue's up a remove of the old lease's DNS (if needed) queueNCR(CHG_REMOVE, old_lease); diff --git a/src/bin/dhcp4/tests/dora_unittest.cc b/src/bin/dhcp4/tests/dora_unittest.cc index 3e747efe9c..e4754782bd 100644 --- a/src/bin/dhcp4/tests/dora_unittest.cc +++ b/src/bin/dhcp4/tests/dora_unittest.cc @@ -633,6 +633,11 @@ const char* DORA_CONFIGS[] = { "interfaces-config": { "interfaces": [ "*" ] }, + "dhcp-ddns": { + "enable-updates": true + }, + "ddns-send-updates": true, + "ddns-update-on-renew": true, "subnet4": [ { "id": 1, @@ -3049,10 +3054,18 @@ DORATest::leaseCaching() { // Configure a DHCP client. Dhcp4Client client; client.includeClientId("11:22"); - + // Include the Client FQDN option. + ASSERT_NO_THROW(client.includeFQDN((Option4ClientFqdn::FLAG_S + | Option4ClientFqdn::FLAG_E), + "first.example.com.", + Option4ClientFqdn::FULL)); // Configure a DHCP server. configure(DORA_CONFIGS[18], *client.getServer()); + ASSERT_NO_THROW(client.getServer()->startD2()); + auto& d2_mgr = CfgMgr::instance().getD2ClientMgr(); + ASSERT_EQ(0, d2_mgr.getQueueSize()); + // Obtain a lease from the server using the 4-way exchange. ASSERT_NO_THROW(client.doDORA()); @@ -3078,6 +3091,12 @@ DORATest::leaseCaching() { // There should only be the default statistic point in the other subnet. checkStat("subnet[2].v4-lease-reuses", 1, 0); + // There should be a single NCR. + ASSERT_EQ(1, d2_mgr.getQueueSize()); + // Clear the NCR queue. + ASSERT_NO_THROW(d2_mgr.runReadyIO()); + ASSERT_EQ(0, d2_mgr.getQueueSize()); + // Assume the client enters init-reboot and does a request. client.setState(Dhcp4Client::INIT_REBOOT); ASSERT_NO_THROW(client.doRequest()); @@ -3104,6 +3123,9 @@ DORATest::leaseCaching() { // Statistics for the other subnet should not be affected. checkStat("subnet[2].v4-lease-reuses", 1, 0); + // There should be no NCRs queued. + ASSERT_EQ(0, d2_mgr.getQueueSize()); + // Let's say the client suddenly decides to do a full DORA. ASSERT_NO_THROW(client.doDORA()); @@ -3129,6 +3151,9 @@ DORATest::leaseCaching() { // Statistics for the other subnet should not be affected. checkStat("subnet[2].v4-lease-reuses", 1, 0); + // There should be no NCRs queued. + ASSERT_EQ(0, d2_mgr.getQueueSize()); + // Try to request a different address than the client has. The server // should respond with DHCPNAK. client.config_.lease_.addr_ = IOAddress("10.0.0.30"); @@ -3153,6 +3178,9 @@ DORATest::leaseCaching() { // Statistics for the other subnet should certainly not be affected. checkStat("subnet[2].v4-lease-reuses", 1, 0); + + // There should be no NCRs queued. + ASSERT_EQ(0, d2_mgr.getQueueSize()); } TEST_F(DORATest, leaseCaching) { diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index 346ed3f085..fffae2da83 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -2439,13 +2439,17 @@ Dhcpv6Srv::createNameChangeRequests(const Pkt6Ptr& answer, // Compute DHCID from Client Identifier and FQDN. isc::dhcp_ddns::D2Dhcid dhcid(*duid, buf_vec); - // Get all IAs from the answer. For each IA, holding an address we will - // create a corresponding NameChangeRequest. - for (auto const& answer_ia : answer->getOptions(D6O_IA_NA)) { + // Iterate over the IAContexts (there should be one per client IA processed). + // For the first address in each IA_NA, create the appropriate NCR(s). + for (auto& ia_ctx : ctx.getIAContexts()) { + if ((ia_ctx.type_ != Lease::TYPE_NA) || !ia_ctx.ia_rsp_) { + continue; + } + /// @todo IA_NA may contain multiple addresses. We should process - /// each address individually. Currently we get only one. - Option6IAAddrPtr iaaddr = boost::static_pointer_cast< - Option6IAAddr>(answer_ia.second->getOption(D6O_IAADDR)); + /// each address individually. Currently we only process the first one. + Option6IAAddrPtr iaaddr = boost::static_pointer_cast + (ia_ctx.ia_rsp_->getOption(D6O_IAADDR)); // We need an address to create a name-to-address mapping. // If address is missing for any reason, go to the next IA. @@ -2453,19 +2457,33 @@ Dhcpv6Srv::createNameChangeRequests(const Pkt6Ptr& answer, continue; } + bool extended_only = false; + // If the lease is being reused (i.e. lease caching in effect) skip it. + IOAddress ia_address = iaaddr->getAddress(); + for (auto const& l : ia_ctx.reused_leases_) { + if (l->addr_ == ia_address) { + extended_only = true; + break; + } + } + + if (extended_only) { + continue; + } + // If the lease for iaaddr is in the list of changed leases, we need // to determine if the changes included changes to the FQDN. If so // then we may need to do a CHG_REMOVE. - bool extended_only = false; - for (auto const& l : ctx.currentIA().changed_leases_) { + for (auto const& l : ia_ctx.changed_leases_) { - if (l->addr_ == iaaddr->getAddress()) { + if (l->addr_ == ia_address) { // The address is the same so this must be renewal. If we're not // always updating on renew, then we only renew if DNS info has // changed. - if (!ctx.getDdnsParams()->getUpdateOnRenew() && + if (l->reuseable_valid_lft_ || + (!ctx.getDdnsParams()->getUpdateOnRenew() && (l->hostname_ == opt_fqdn->getDomainName() && - l->fqdn_fwd_ == do_fwd && l->fqdn_rev_ == do_rev)) { + l->fqdn_fwd_ == do_fwd && l->fqdn_rev_ == do_rev))) { extended_only = true; } else { // Queue a CHG_REMOVE of the old data. @@ -2630,6 +2648,7 @@ Dhcpv6Srv::assignIA_NA(const Pkt6Ptr& query, // Do not use OptionDefinition to create option's instance so // as we can initialize IAID using a constructor. Option6IAPtr ia_rsp(new Option6IA(D6O_IA_NA, ia->getIAID())); + ctx.currentIA().ia_rsp_ = ia_rsp; if (lease) { // We have a lease! Let's wrap its content into IA_NA option @@ -2648,6 +2667,8 @@ Dhcpv6Srv::assignIA_NA(const Pkt6Ptr& query, } else { lease->valid_lft_ = lease->reuseable_valid_lft_; lease->preferred_lft_ = lease->reuseable_preferred_lft_; + std::cout << __LINE__ << " flagged as resuseable: " << lease->addr_.toText() << std::endl; + ctx.currentIA().reused_leases_.push_back(lease); LOG_INFO(lease6_logger, DHCP6_LEASE_REUSE) .arg(query->getLabel()) .arg(lease->addr_.toText()) @@ -2751,6 +2772,7 @@ Dhcpv6Srv::assignIA_PD(const Pkt6Ptr& query, ctx.currentIA().addHint(hint, 0); } ctx.currentIA().type_ = Lease::TYPE_PD; + ctx.currentIA().ia_rsp_ = ia_rsp; // Use allocation engine to pick a lease for this client. Allocation engine // will try to honor the hint, but it is just a hint - some other address diff --git a/src/bin/dhcp6/tests/fqdn_unittest.cc b/src/bin/dhcp6/tests/fqdn_unittest.cc index d75ed06176..dfc977ba70 100644 --- a/src/bin/dhcp6/tests/fqdn_unittest.cc +++ b/src/bin/dhcp6/tests/fqdn_unittest.cc @@ -255,12 +255,17 @@ public: /// /// @param iaid IAID /// @param pkt A DHCPv6 message to which the IA option should be added. - void addIA(const uint32_t iaid, const IOAddress& addr, Pkt6Ptr& pkt) { + /// @param cxt allocation engine context to which IA option should be + /// added. + void addIA(const uint32_t iaid, const IOAddress& addr, Pkt6Ptr& pkt, + AllocEngine::ClientContext6& ctx) { Option6IAPtr opt_ia = generateIA(D6O_IA_NA, iaid, 1500, 3000); Option6IAAddrPtr opt_iaaddr(new Option6IAAddr(D6O_IAADDR, addr, 300, 500)); opt_ia->addOption(opt_iaaddr); pkt->addOption(opt_ia); + ctx.createIAContext(); + ctx.currentIA().ia_rsp_ = opt_ia; } /// @brief Adds IA option to the message. @@ -270,10 +275,15 @@ public: /// @param iaid IAID /// @param status_code Status code /// @param pkt A DHCPv6 message to which the option should be added. - void addIA(const uint32_t iaid, const uint16_t status_code, Pkt6Ptr& pkt) { + /// @param cxt allocation engine context to which IA option should be + /// added. + void addIA(const uint32_t iaid, const uint16_t status_code, Pkt6Ptr& pkt, + AllocEngine::ClientContext6& ctx) { Option6IAPtr opt_ia = generateIA(D6O_IA_NA, iaid, 1500, 3000); addStatusCode(status_code, "", opt_ia); pkt->addOption(opt_ia); + ctx.createIAContext(); + ctx.currentIA().ia_rsp_ = opt_ia; } /// @brief Creates status code with the specified code and message. @@ -347,13 +357,13 @@ public: ? DHCPV6_ADVERTISE : DHCPV6_REPLY); - // Create three IAs, each having different address. - addIA(1234, IOAddress("2001:db8:1::1"), answer); - AllocEngine::ClientContext6 ctx; // Set the selected subnet so ddns params get returned correctly. ctx.subnet_ = subnet_; + // Create three IAs, each having different address. + addIA(1234, IOAddress("2001:db8:1::1"), answer, ctx); + ASSERT_NO_THROW(srv_->processClientFqdn(question, answer, ctx)); Option6ClientFqdnPtr answ_fqdn = boost::dynamic_pointer_cast< Option6ClientFqdn>(answer->getOption(D6O_CLIENT_FQDN)); @@ -823,10 +833,15 @@ TEST_F(FqdnDhcpv6SrvTest, createNameChangeRequests) { // Create Reply message with Client Id and Server id. Pkt6Ptr answer = generateMessageWithIds(DHCPV6_REPLY); + // Create NameChangeRequest for the first allocated address. + AllocEngine::ClientContext6 ctx; + ctx.subnet_ = subnet_; + ctx.fwd_dns_update_ = ctx.rev_dns_update_ = true; + // Create three IAs, each having different address. - addIA(1234, IOAddress("2001:db8:1::1"), answer); - addIA(2345, IOAddress("2001:db8:1::2"), answer); - addIA(3456, IOAddress("2001:db8:1::3"), answer); + addIA(1234, IOAddress("2001:db8:1::1"), answer, ctx); + addIA(2345, IOAddress("2001:db8:1::2"), answer, ctx); + addIA(3456, IOAddress("2001:db8:1::3"), answer, ctx); // Use domain name in upper case. It should be converted to lower-case // before DHCID is calculated. So, we should get the same result as if @@ -836,10 +851,7 @@ TEST_F(FqdnDhcpv6SrvTest, createNameChangeRequests) { Option6ClientFqdn::FULL); answer->addOption(fqdn); - // Create NameChangeRequest for the first allocated address. - AllocEngine::ClientContext6 ctx; - ctx.subnet_ = subnet_; - ctx.fwd_dns_update_ = ctx.rev_dns_update_ = true; + ASSERT_NO_THROW(srv_->createNameChangeRequests(answer, ctx)); ASSERT_EQ(1, d2_mgr_.getQueueSize()); @@ -857,8 +869,14 @@ TEST_F(FqdnDhcpv6SrvTest, noConflictResolution) { // Create Reply message with Client Id and Server id. Pkt6Ptr answer = generateMessageWithIds(DHCPV6_REPLY); + // Create NameChangeRequest for the first allocated address. + AllocEngine::ClientContext6 ctx; + subnet_->setDdnsConflictResolutionMode("no-check-with-dhcid"); + ctx.subnet_ = subnet_; + ctx.fwd_dns_update_ = ctx.rev_dns_update_ = true; + // Create three IAs, each having different address. - addIA(1234, IOAddress("2001:db8:1::1"), answer); + addIA(1234, IOAddress("2001:db8:1::1"), answer, ctx); // Use domain name in upper case. It should be converted to lower-case // before DHCID is calculated. So, we should get the same result as if @@ -868,11 +886,6 @@ TEST_F(FqdnDhcpv6SrvTest, noConflictResolution) { Option6ClientFqdn::FULL); answer->addOption(fqdn); - // Create NameChangeRequest for the first allocated address. - AllocEngine::ClientContext6 ctx; - subnet_->setDdnsConflictResolutionMode("no-check-with-dhcid"); - ctx.subnet_ = subnet_; - ctx.fwd_dns_update_ = ctx.rev_dns_update_ = true; ASSERT_NO_THROW(srv_->createNameChangeRequests(answer, ctx)); ASSERT_EQ(1, d2_mgr_.getQueueSize()); @@ -893,8 +906,13 @@ TEST_F(FqdnDhcpv6SrvTest, noAddRequestsWhenDisabled) { // Create Reply message with Client Id and Server id. Pkt6Ptr answer = generateMessageWithIds(DHCPV6_REPLY); + // An attempt to send a NCR would throw. + AllocEngine::ClientContext6 ctx; + ctx.subnet_ = subnet_; + ctx.fwd_dns_update_ = ctx.rev_dns_update_ = true; + // Create three IAs, each having different address. - addIA(1234, IOAddress("2001:db8:1::1"), answer); + addIA(1234, IOAddress("2001:db8:1::1"), answer, ctx); // Use domain name in upper case. It should be converted to lower-case // before DHCID is calculated. So, we should get the same result as if @@ -904,10 +922,6 @@ TEST_F(FqdnDhcpv6SrvTest, noAddRequestsWhenDisabled) { Option6ClientFqdn::FULL); answer->addOption(fqdn); - // An attempt to send a NCR would throw. - AllocEngine::ClientContext6 ctx; - ctx.subnet_ = subnet_; - ctx.fwd_dns_update_ = ctx.rev_dns_update_ = true; ASSERT_NO_THROW(srv_->createNameChangeRequests(answer, ctx)); } @@ -2153,8 +2167,15 @@ TEST_F(FqdnDhcpv6SrvTest, ddnsTtlPercent) { // Create Reply message with Client Id and Server id. Pkt6Ptr answer = generateMessageWithIds(DHCPV6_REPLY); + // Create NameChangeRequest for the first allocated address. + AllocEngine::ClientContext6 ctx; + subnet_->setDdnsConflictResolutionMode("no-check-with-dhcid"); + subnet_->setDdnsTtlPercent(Optional(0.10)); + ctx.subnet_ = subnet_; + ctx.fwd_dns_update_ = ctx.rev_dns_update_ = true; + // Create an IA. - addIA(1234, IOAddress("2001:db8:1::1"), answer); + addIA(1234, IOAddress("2001:db8:1::1"), answer, ctx); // Use domain name in upper case. It should be converted to lower-case // before DHCID is calculated. So, we should get the same result as if @@ -2164,12 +2185,6 @@ TEST_F(FqdnDhcpv6SrvTest, ddnsTtlPercent) { Option6ClientFqdn::FULL); answer->addOption(fqdn); - // Create NameChangeRequest for the first allocated address. - AllocEngine::ClientContext6 ctx; - subnet_->setDdnsConflictResolutionMode("no-check-with-dhcid"); - subnet_->setDdnsTtlPercent(Optional(0.10)); - ctx.subnet_ = subnet_; - ctx.fwd_dns_update_ = ctx.rev_dns_update_ = true; ASSERT_NO_THROW(srv_->createNameChangeRequests(answer, ctx)); ASSERT_EQ(1, d2_mgr_.getQueueSize()); diff --git a/src/bin/dhcp6/tests/sarr_unittest.cc b/src/bin/dhcp6/tests/sarr_unittest.cc index f5e81e41eb..19938af2c1 100644 --- a/src/bin/dhcp6/tests/sarr_unittest.cc +++ b/src/bin/dhcp6/tests/sarr_unittest.cc @@ -313,6 +313,11 @@ const char* CONFIGS[] = { "interfaces-config": { "interfaces": [ "*" ] }, + "dhcp-ddns": { + "enable-updates": true + }, + "ddns-send-updates": true, + "ddns-update-on-renew": true, "subnet6": [ { "id": 1, @@ -1367,6 +1372,16 @@ SARRTest::leaseCaching() { ASSERT_NO_THROW(client.requestAddress(1234, asiolink::IOAddress("2001:db8::10"))); ASSERT_NO_THROW(client.requestPrefix(5678, 32, asiolink::IOAddress("2001:db8:1::"))); + // Include FQDN to trigger generation of name change requests. + ASSERT_NO_THROW(client.useFQDN(Option6ClientFqdn::FLAG_S, + "client-name.example.org", + Option6ClientFqdn::FULL)); + + // Start D2 client mgr and verify the NCR queue is empty. + ASSERT_NO_THROW(client.getServer()->startD2()); + auto& d2_mgr = CfgMgr::instance().getD2ClientMgr(); + ASSERT_EQ(0, d2_mgr.getQueueSize()); + // Perform 4-way exchange. ASSERT_NO_THROW(client.doSARR()); @@ -1393,6 +1408,12 @@ SARRTest::leaseCaching() { checkStat("subnet[1].v6-ia-pd-lease-reuses", 1, 0); checkStat("subnet[2].v6-ia-pd-lease-reuses", 1, 0); + // There should be a single NCR. + ASSERT_EQ(1, d2_mgr.getQueueSize()); + // Clear the NCR queue. + ASSERT_NO_THROW(d2_mgr.runReadyIO()); + ASSERT_EQ(0, d2_mgr.getQueueSize()); + // Request the same prefix with a different length. The server should // return an existing lease. client.clearRequestedIAs(); @@ -1415,6 +1436,9 @@ SARRTest::leaseCaching() { checkStat("subnet[1].v6-ia-pd-lease-reuses", 2, 1); checkStat("subnet[2].v6-ia-pd-lease-reuses", 1, 0); + // There should be no NCRs queued. + ASSERT_EQ(0, d2_mgr.getQueueSize()); + // Try to request another prefix. The client should still get the existing // lease. client.clearRequestedIAs(); @@ -1436,6 +1460,9 @@ SARRTest::leaseCaching() { checkStat("v6-ia-pd-lease-reuses", 3, 2); checkStat("subnet[1].v6-ia-pd-lease-reuses", 3, 2); checkStat("subnet[2].v6-ia-pd-lease-reuses", 1, 0); + + // There should be no NCRs queued. + ASSERT_EQ(0, d2_mgr.getQueueSize()); } TEST_F(SARRTest, leaseCaching) { diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index da504bded0..36be5ee81c 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -224,7 +224,7 @@ AllocEngine::ClientContext6::ClientContext6(const ConstSubnet6Ptr& subnet, AllocEngine::ClientContext6::IAContext::IAContext() : iaid_(0), type_(Lease::TYPE_NA), hints_(), old_leases_(), - changed_leases_(), new_resources_(), ia_rsp_() { + changed_leases_(),reused_leases_(), new_resources_(), ia_rsp_() { } void @@ -2371,6 +2371,10 @@ AllocEngine::extendLease6(ClientContext6& ctx, Lease6Ptr lease) { lease->pool_id_ = pool->getID(); } LeaseMgrFactory::instance().updateLease6(lease); + } else { + // Server looks at changed_leases_ (i.e. old_data) when deciding + // on DNS updates etc. + old_data->reuseable_valid_lft_ = lease->reuseable_valid_lft_; } if (update_stats) { @@ -2465,9 +2469,13 @@ AllocEngine::updateLeaseData(ClientContext6& ctx, const Lease6Collection& leases if (!fqdn_changed) { setLeaseReusable(lease, current_preferred_lft, ctx); } + if (lease->reuseable_valid_lft_ == 0) { ctx.currentIA().changed_leases_.push_back(lease_it); LeaseMgrFactory::instance().updateLease6(lease); + } else { + // Server needs to know about resused leases to avoid DNS updates. + ctx.currentIA().reused_leases_.push_back(lease_it); } if (update_stats) { diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h index 84c3f1e1f0..79574c970b 100644 --- a/src/lib/dhcpsrv/alloc_engine.h +++ b/src/lib/dhcpsrv/alloc_engine.h @@ -322,6 +322,9 @@ public: /// FQDN has changed. Lease6Collection changed_leases_; + /// @brief Set of leases marked for reuse by lease caching + Lease6Collection reused_leases_; + /// @brief Holds addresses and prefixes allocated for this IA. /// /// This collection is used to update at most once new leases. @@ -424,6 +427,10 @@ public: return (ias_.back()); } + std::vector& getIAContexts() { + return (ias_); + } + /// @brief Creates new IA context. /// /// This method should be invoked prior to processing a next IA included