From e83c4f59c08b7cc8c599fb983e0d787b248499f5 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Fri, 21 Mar 2025 15:23:25 -0400 Subject: [PATCH] [#3555] Handle conflicts when offer-liftime > 0 /src/bin/dhcp4/tests/dora_unittest.cc DORATest::reservationsWithConflicts() - added parameter for offer-lifetime TEST_F(DORATest, reservationsWithConflictsOfferLft) TEST_F(DORATest, reservationsWithConflictsOfferLftMultiThreading) - new tests /src/lib/dhcpsrv/alloc_engine.* deleteAssignedLease() - new convenience function AllocEngine::requestLease4() - added logic to handle conflicting lease when offer-lifetime > 0 AllocEngine::getOfferLft() - added flag to disable DISCOVER only check /src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc TEST_F(AllocEngine4Test, existingLeasePlusTemporary) - new test --- src/bin/dhcp4/tests/dora_unittest.cc | 22 ++++++- src/lib/dhcpsrv/alloc_engine.cc | 63 ++++++++++++------- src/lib/dhcpsrv/alloc_engine.h | 8 ++- .../dhcpsrv/tests/alloc_engine4_unittest.cc | 56 +++++++++++++++++ 4 files changed, 122 insertions(+), 27 deletions(-) diff --git a/src/bin/dhcp4/tests/dora_unittest.cc b/src/bin/dhcp4/tests/dora_unittest.cc index beddd0f371..9353c4b491 100644 --- a/src/bin/dhcp4/tests/dora_unittest.cc +++ b/src/bin/dhcp4/tests/dora_unittest.cc @@ -927,7 +927,9 @@ public: /// allocating the address reserved for the Client B. /// 21. Client B uses 4-way exchange to obtain a new lease. /// 22. The server finally allocates a reserved address to the Client B. - void reservationsWithConflicts(); + /// @param offer_lifetime value to use as the global value of offer-lietime + /// throughout the test. + void reservationsWithConflicts(int offer_lifetime = 0); /// @brief This test verifies that the allocation engine ignores /// reservations when reservations flags are set to "disabled". @@ -2198,10 +2200,13 @@ TEST_F(DORATest, messageFieldsReservationsMultiThreading) { } void -DORATest::reservationsWithConflicts() { +DORATest::reservationsWithConflicts(int offer_lifetime /* = 0 */) { + ElementPtr offer_lft(Element::create(offer_lifetime)); Dhcp4Client client(srv_, Dhcp4Client::SELECTING); // Configure DHCP server. configure(DORA_CONFIGS[0], *client.getServer()); + CfgMgr::instance().getCurrentCfg()->addConfiguredGlobal("offer-lifetime", offer_lft); + // Client A performs 4-way exchange and obtains a lease from the // dynamic pool. ASSERT_NO_THROW(client.doDORA(boost::shared_ptr< @@ -2223,6 +2228,7 @@ DORATest::reservationsWithConflicts() { SUBNET_ID_UNUSED, IOAddress("10.0.0.9"))); CfgMgr::instance().getStagingCfg()->getCfgHosts()->add(host); CfgMgr::instance().commit(); + CfgMgr::instance().getCurrentCfg()->addConfiguredGlobal("offer-lifetime", offer_lft); // Let's transition the client to Renewing state. client.setState(Dhcp4Client::RENEWING); @@ -2260,6 +2266,7 @@ DORATest::reservationsWithConflicts() { // By reconfiguring the server, we remove the existing reservations. configure(DORA_CONFIGS[0]); + CfgMgr::instance().getCurrentCfg()->addConfiguredGlobal("offer-lifetime", offer_lft); // Try to renew the existing lease again. ASSERT_NO_THROW(client.doRequest()); @@ -2303,6 +2310,7 @@ DORATest::reservationsWithConflicts() { SUBNET_ID_UNUSED, in_pool_addr)); CfgMgr::instance().getStagingCfg()->getCfgHosts()->add(host); CfgMgr::instance().commit(); + CfgMgr::instance().getCurrentCfg()->addConfiguredGlobal("offer-lifetime", offer_lft); // Client B performs a DHCPDISCOVER. clientB.setState(Dhcp4Client::SELECTING); @@ -2414,6 +2422,16 @@ TEST_F(DORATest, reservationsWithConflictsMultiThreading) { reservationsWithConflicts(); } +TEST_F(DORATest, reservationsWithConflictsOfferLft) { + Dhcpv4SrvMTTestGuard guard(*this, false); + reservationsWithConflicts(120); +} + +TEST_F(DORATest, reservationsWithConflictsOfferLftMultiThreading) { + Dhcpv4SrvMTTestGuard guard(*this, true); + reservationsWithConflicts(120); +} + void DORATest::reservationModeDisabled() { // Client has a reservation. diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index 6248f46634..4d4acb2deb 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -4013,6 +4013,28 @@ AllocEngine::discoverLease4(AllocEngine::ClientContext4& ctx) { return (new_lease); } +void deleteAssignedLease(Lease4Ptr lease) { + if (LeaseMgrFactory::instance().deleteLease(lease) && + (lease->state_ != Lease4::STATE_RELEASED)) { + // Need to decrease statistic for assigned addresses. + StatsMgr::instance().addValue(StatsMgr::generateName("subnet", lease->subnet_id_, + "assigned-addresses"), + static_cast(-1)); + + auto const& subnet = CfgMgr::instance().getCurrentCfg()->getCfgSubnets4() + ->getBySubnetId(lease->subnet_id_); + if (subnet) { + auto const& pool = subnet->getPool(Lease::TYPE_V4, lease->addr_, false); + if (pool) { + StatsMgr::instance().addValue(StatsMgr::generateName("subnet", subnet->getID(), + StatsMgr::generateName("pool", pool->getID(), + "assigned-addresses")), + static_cast(-1)); + } + } + } +} + Lease4Ptr AllocEngine::requestLease4(AllocEngine::ClientContext4& ctx) { // Find an existing lease for this client. This function will return null @@ -4113,6 +4135,21 @@ AllocEngine::requestLease4(AllocEngine::ClientContext4& ctx) { ctx.unknown_requested_addr_ = true; return (Lease4Ptr()); } + + // During a prior discover the allocation engine deemed that the + // client's current lease (client_lease) should no longer be used and + // so offered a different lease (existing) that was temporarily + // allocated because offer-lifetime is greater than zero. We need to + // delete to unusable lease and renew the temporary lease. + if (((client_lease && existing) && (client_lease->addr_ != existing->addr_) && + (existing->addr_ == ctx.requested_address_) && + (existing->belongsToClient(ctx.hwaddr_, ctx.subnet_->getMatchClientId() ? + ctx.clientid_ : ClientIdPtr()))) + && getOfferLft(ctx, false)) { + auto conflicted_lease = client_lease; + client_lease = existing;; + deleteAssignedLease(conflicted_lease); + } } // We have gone through all the checks, so we can now allocate the address @@ -4161,7 +4198,6 @@ AllocEngine::requestLease4(AllocEngine::ClientContext4& ctx) { CalloutHandle::CalloutNextStep callout_status = CalloutHandle::NEXT_STEP_CONTINUE; new_lease = allocateOrReuseLease4(ctx.requested_address_, ctx, callout_status); - } else { LOG_DEBUG(alloc_engine_logger, ALLOC_ENGINE_DBG_TRACE, @@ -4184,26 +4220,7 @@ AllocEngine::requestLease4(AllocEngine::ClientContext4& ctx) { ALLOC_ENGINE_V4_REQUEST_REMOVE_LEASE) .arg(ctx.query_->getLabel()) .arg(client_lease->addr_.toText()); - - if (LeaseMgrFactory::instance().deleteLease(client_lease) && (client_lease->state_ != Lease4::STATE_RELEASED)) { - // Need to decrease statistic for assigned addresses. - StatsMgr::instance().addValue( - StatsMgr::generateName("subnet", client_lease->subnet_id_, - "assigned-addresses"), - static_cast(-1)); - - auto const& subnet = CfgMgr::instance().getCurrentCfg()->getCfgSubnets4()->getBySubnetId(client_lease->subnet_id_); - if (subnet) { - auto const& pool = subnet->getPool(Lease::TYPE_V4, client_lease->addr_, false); - if (pool) { - StatsMgr::instance().addValue( - StatsMgr::generateName("subnet", subnet->getID(), - StatsMgr::generateName("pool", pool->getID(), - "assigned-addresses")), - static_cast(-1)); - } - } - } + deleteAssignedLease(client_lease); } // Return the allocated lease or NULL pointer if allocation was @@ -4212,9 +4229,9 @@ AllocEngine::requestLease4(AllocEngine::ClientContext4& ctx) { } uint32_t -AllocEngine::getOfferLft(const ClientContext4& ctx) { +AllocEngine::getOfferLft(const ClientContext4& ctx, bool only_on_discover /* = true */) { // Not a DISCOVER or it's BOOTP, punt. - if ((!ctx.fake_allocation_) || (ctx.query_->inClass("BOOTP"))) { + if (only_on_discover && ((!ctx.fake_allocation_) || (ctx.query_->inClass("BOOTP")))) { return (0); } diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h index 79574c970b..3d93b008b6 100644 --- a/src/lib/dhcpsrv/alloc_engine.h +++ b/src/lib/dhcpsrv/alloc_engine.h @@ -1548,9 +1548,13 @@ public: /// to the query. /// /// @param ctx Client context holding various information about the client. + /// @param only_on_discover when true (default) function will return + /// zero if the context is not for DISCOVER. When false it will search + /// scopes for offer-lifetime regardless of the context query type. + /// /// @return unsigned integer value of the offer lifetime to use. - static uint32_t getOfferLft(const ClientContext4& ctx); - + static uint32_t getOfferLft(const ClientContext4& ctx, + bool only_on_discover = true); private: /// @brief Offers the lease. diff --git a/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc index a89b322f71..c63f13ca3f 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc @@ -2125,6 +2125,62 @@ TEST_F(AllocEngine4Test, requestOtherClientLease) { EXPECT_EQ("192.0.2.101", new_lease->addr_.toText()); } +// Allocation engine should remove a pre-existing lease +// and renew the temporary lease when offer-lifetime > 0. +// This can happen when a client gets a lease which is +// later rendered invalid by the creation of host reservation, +// either for the lease address or for the client with +// a different address. +TEST_F(AllocEngine4Test, existingLeasePlusTemporary) { + // Create the first lease. + Lease4Ptr lease(new Lease4(IOAddress("192.0.2.101"), hwaddr_, + &clientid_->getClientId()[0], + clientid_->getClientId().size(), + 100, time(NULL), subnet_->getID(), + false, false, "")); + + // Create the second lease. + Lease4Ptr lease2(new Lease4(IOAddress("192.0.2.102"), hwaddr_, + &clientid_->getClientId()[0], + clientid_->getClientId().size(), + 100, time(NULL), subnet_->getID(), + false, false, "")); + // Add leases for both clients to the Lease Manager. + LeaseMgrFactory::instance().addLease(lease); + LeaseMgrFactory::instance().addLease(lease2); + + // First when temporary allocations are off. + + // Client requests the second lease. + AllocEngine engine(0); + AllocEngine::ClientContext4 ctx(subnet_, clientid_, hwaddr_, IOAddress("192.0.2.102"), + false, false, "", false); + ctx.query_.reset(new Pkt4(DHCPREQUEST, 1234)); + Lease4Ptr new_lease = engine.allocateLease4(ctx); + + ASSERT_FALSE(new_lease); + + // Both leases should still be there. + ASSERT_TRUE(LeaseMgrFactory::instance().getLease4(lease->addr_)); + ASSERT_TRUE(LeaseMgrFactory::instance().getLease4(lease2->addr_)); + + // Now enable temporary allocations by setting offer lifetime > 0. + subnet_->setOfferLft(120); + + // Client requests the second lease. + AllocEngine::ClientContext4 ctx2(subnet_, clientid_, hwaddr_, IOAddress("192.0.2.102"), + false, false, "", false); + ctx2.query_.reset(new Pkt4(DHCPREQUEST, 1234)); + new_lease = engine.allocateLease4(ctx2); + + // Allocation should renew it and remove the original lease. + ASSERT_TRUE(new_lease); + EXPECT_EQ("192.0.2.102", new_lease->addr_.toText()); + + // Orignal lease should be gone. + ASSERT_FALSE(LeaseMgrFactory::instance().getLease4(lease->addr_)); +} + // This test checks the behavior of the allocation engine in the following // scenario: // - Client has no lease in the database. -- 2.47.3