From d74021c60bf6347132dfa7e29a194efdddf568e8 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Fri, 3 Mar 2023 14:55:03 -0500 Subject: [PATCH] [#2719] kea-dhcp4 offer-lft is now functional kea-dhcp4 supports offer-lft for global, shared-network, and subnets. Not yet supported in classes. src/bin/dhcp4/dhcp4_srv.cc Dhcpv4Srv::assignLease() - adjust outbound lease time option Dhcpv4Srv::postAllocateNameUpdate() - allow reuse check on allocated discover src/lib/dhcpsrv/alloc_engine.* AllocEngine::ClientContext4 - add offer_lft_ member AllocEngine::discoverLease4() -set contexts offer_lft_ AllocEngine::getOfferLft() - new, get context appropriate value for offer_lft AllocEngine::createLease4() - use offer_lft on DISCOVER, pass offer_lft into hook, set lease fqdn flags to false AllocEngine::reuseExpiredLease4() - reclaim on discover allocation AllocEngine::updateLease4Information() - use offer_lft if appropriate src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc TEST_F(AllocEngine4Test, discoverOfferLft) - enabled TEST_F(AllocEngine4Test, discoverOfferLftReuseExpiredLease4) - new test --- src/bin/dhcp4/dhcp4_srv.cc | 11 ++- src/lib/dhcpsrv/alloc_engine.cc | 86 +++++++++++++++---- src/lib/dhcpsrv/alloc_engine.h | 25 +++++- .../dhcpsrv/tests/alloc_engine4_unittest.cc | 72 +++++++++++++++- .../tests/alloc_engine_hooks_unittest.cc | 1 + 5 files changed, 171 insertions(+), 24 deletions(-) diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index 2f8e0724b3..8431c2750d 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -2855,8 +2855,12 @@ Dhcpv4Srv::assignLease(Dhcpv4Exchange& ex) { } // IP Address Lease time (type 51) - OptionPtr opt(new OptionUint32(Option::V4, DHO_DHCP_LEASE_TIME, - lease->valid_lft_)); + // If we're not allocating on discover then we just sent the lifetime on the lease. + // Otherwise (i.e. offer_lft > 0), the lease's lifetime has been set to offer_lft but + // we want to send the client the proper valid lifetime so we have to fetch it. + auto send_lft = (ctx->offer_lft_ ? AllocEngine::getValidLft(*ctx) : lease->valid_lft_); + OptionPtr opt(new OptionUint32(Option::V4, DHO_DHCP_LEASE_TIME, send_lft)); + resp->addOption(opt); // Subnet mask (type 1) @@ -2964,7 +2968,8 @@ Dhcpv4Srv::postAllocateNameUpdate(const AllocEngine::ClientContext4Ptr& ctx, con // any potential exceptions (e.g. invalid lease database backend // implementation) and log an error. try { - if (!ctx->fake_allocation_) { + /// TKM - do this on committed-discover + if (!ctx->fake_allocation_ || (ctx->offer_lft_ > 0)) { // The lease can't be reused. lease->reuseable_valid_lft_ = 0; diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index 48b8aebdbf..8d01430f25 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -3323,10 +3323,11 @@ AllocEngine::ClientContext4::ClientContext4() subnet_(), clientid_(), hwaddr_(), requested_address_(IOAddress::IPV4_ZERO_ADDRESS()), fwd_dns_update_(false), rev_dns_update_(false), - hostname_(""), callout_handle_(), fake_allocation_(false), + hostname_(""), callout_handle_(), fake_allocation_(false), offer_lft_(0), old_lease_(), new_lease_(), hosts_(), conflicting_lease_(), query_(), host_identifiers_(), unknown_requested_addr_(false), ddns_params_() { + } AllocEngine::ClientContext4::ClientContext4(const Subnet4Ptr& subnet, @@ -3336,13 +3337,14 @@ AllocEngine::ClientContext4::ClientContext4(const Subnet4Ptr& subnet, const bool fwd_dns_update, const bool rev_dns_update, const std::string& hostname, - const bool fake_allocation) + const bool fake_allocation, + const uint32_t offer_lft) : early_global_reservations_lookup_(false), subnet_(subnet), clientid_(clientid), hwaddr_(hwaddr), requested_address_(requested_addr), fwd_dns_update_(fwd_dns_update), rev_dns_update_(rev_dns_update), hostname_(hostname), callout_handle_(), - fake_allocation_(fake_allocation), old_lease_(), new_lease_(), + fake_allocation_(fake_allocation), offer_lft_(offer_lft), old_lease_(), new_lease_(), hosts_(), host_identifiers_(), unknown_requested_addr_(false), ddns_params_(new DdnsParams()) { @@ -3424,7 +3426,6 @@ AllocEngine::allocateLease4(ClientContext4& ctx) { if (ctx.fake_allocation_) { return (discoverLease4(ctx)); - } else { ctx.new_lease_ = requestLease4(ctx); } @@ -3560,6 +3561,9 @@ AllocEngine::discoverLease4(AllocEngine::ClientContext4& ctx) { Lease4Ptr client_lease; findClientLease(ctx, client_lease); + // Fetch offer_lft to see if we're allocating on DISCOVER. + ctx.offer_lft_ = getOfferLft(ctx); + // new_lease will hold the pointer to the lease that we will offer to the // caller. Lease4Ptr new_lease; @@ -3857,8 +3861,47 @@ AllocEngine::requestLease4(AllocEngine::ClientContext4& ctx) { } uint32_t -AllocEngine::getValidLft(const ClientContext4& ctx) { +AllocEngine::getOfferLft(const ClientContext4& ctx) { + // Not a DISCOVER or it's BOOTP, punt. + if ((!ctx.fake_allocation_) || (ctx.query_->inClass("BOOTP"))) { + return (0); + } + + util::Optional offer_lft; + +#if 0 + /// @todo TKM need this when we add offer-lst to client class. + // If specified in one of our classes use it. + // We use the first one we find. + const ClientClasses classes = ctx.query_->getClasses(); + if (!classes.empty()) { + // Let's get class definitions + const ClientClassDictionaryPtr& dict = + CfgMgr::instance().getCurrentCfg()->getClientClassDictionary(); + + // Iterate over the assigned class definitions. + for (ClientClasses::const_iterator name = classes.cbegin(); + name != classes.cend(); ++name) { + ClientClassDefPtr cl = dict->findClass(*name); + if (cl && (!cl->getOfferLft().unspecified())) { + offer_lft = cl->getOfferLft(); + break; + } + } + } +#endif + + // If no classes specified it, get it from the subnet. + if (offer_lft.unspecified()) { + offer_lft = ctx.subnet_->getOfferLft(); + } + return (offer_lft.unspecified() ? 0 : offer_lft.get()); +} + + +uint32_t +AllocEngine::getValidLft(const ClientContext4& ctx) { // If it's BOOTP, use infinite valid lifetime. if (ctx.query_->inClass("BOOTP")) { return (Lease::INFINITY_LFT); @@ -3919,8 +3962,8 @@ AllocEngine::createLease4(const ClientContext4& ctx, const IOAddress& addr, isc_throw(BadValue, "Can't create a lease without a subnet"); } - // Get the context appropriate valid lifetime. - uint32_t valid_lft = getValidLft(ctx); + // Get the context appropriate lifetime. + uint32_t valid_lft = (ctx.offer_lft_ ? ctx.offer_lft_ : getValidLft(ctx)); time_t now = time(NULL); @@ -3967,6 +4010,9 @@ AllocEngine::createLease4(const ClientContext4& ctx, const IOAddress& addr, // Is this solicit (fake = true) or request (fake = false) ctx.callout_handle_->setArgument("fake_allocation", ctx.fake_allocation_); + // Are we allocating on DISCOVER? (i.e. offer_lft > 0). + ctx.callout_handle_->setArgument("offer_lft", ctx.offer_lft_); + // Pass the intended lease as well ctx.callout_handle_->setArgument("lease4", lease); @@ -3988,7 +4034,15 @@ AllocEngine::createLease4(const ClientContext4& ctx, const IOAddress& addr, ctx.callout_handle_->getArgument("lease4", lease); } - if (!ctx.fake_allocation_) { + if (ctx.fake_allocation_ && ctx.offer_lft_) { + // Turn them off before we persist, so we'll see it as different when + // we extend it in the REQUEST. This should cause us to do DDNS (if + // it's enabled). + lease->fqdn_fwd_ = false; + lease->fqdn_rev_ = false; + } + + if (!ctx.fake_allocation_ || ctx.offer_lft_) { // That is a real (REQUEST) allocation bool status = LeaseMgrFactory::instance().addLease(lease); if (status) { @@ -4015,7 +4069,6 @@ AllocEngine::createLease4(const ClientContext4& ctx, const IOAddress& addr, } } else { // That is only fake (DISCOVER) allocation - // It is for OFFER only. We should not insert the lease and callers // have already verified the lease does not exist in the database. return (lease); @@ -4043,7 +4096,7 @@ AllocEngine::renewLease4(const Lease4Ptr& lease, setLeaseReusable(lease, ctx); } - if (!ctx.fake_allocation_) { + if (!ctx.fake_allocation_ || ctx.offer_lft_) { // If the lease is expired we have to reclaim it before // re-assigning it to the client. The lease reclamation // involves execution of hooks and DNS update. @@ -4103,7 +4156,7 @@ AllocEngine::renewLease4(const Lease4Ptr& lease, /// DROP status does not make sense here. } - if (!ctx.fake_allocation_ && !skip && (lease->reuseable_valid_lft_ == 0)) { + if ((!ctx.fake_allocation_ || ctx.offer_lft_) && !skip && (lease->reuseable_valid_lft_ == 0)) { // for REQUEST we do update the lease LeaseMgrFactory::instance().updateLease4(lease); @@ -4142,7 +4195,7 @@ AllocEngine::reuseExpiredLease4(Lease4Ptr& expired, isc_throw(BadValue, "null subnet specified for the reuseExpiredLease"); } - if (!ctx.fake_allocation_) { + if (!ctx.fake_allocation_ || ctx.offer_lft_) { // The expired lease needs to be reclaimed before it can be reused. // This includes declined leases for which probation period has // elapsed. @@ -4184,6 +4237,7 @@ AllocEngine::reuseExpiredLease4(Lease4Ptr& expired, // Is this solicit (fake = true) or request (fake = false) ctx.callout_handle_->setArgument("fake_allocation", ctx.fake_allocation_); + ctx.callout_handle_->setArgument("offer_lft", ctx.offer_lft_); // The lease that will be assigned to a client ctx.callout_handle_->setArgument("lease4", expired); @@ -4209,7 +4263,7 @@ AllocEngine::reuseExpiredLease4(Lease4Ptr& expired, ctx.callout_handle_->getArgument("lease4", expired); } - if (!ctx.fake_allocation_) { + if (!ctx.fake_allocation_ || ctx.offer_lft_) { // for REQUEST we do update the lease LeaseMgrFactory::instance().updateLease4(expired); @@ -4514,10 +4568,10 @@ AllocEngine::updateLease4Information(const Lease4Ptr& lease, lease->cltt_ = time(NULL); // Get the context appropriate valid lifetime. - lease->valid_lft_ = getValidLft(ctx); + lease->valid_lft_ = (ctx.offer_lft_ ? ctx.offer_lft_ : getValidLft(ctx)); - // Reduced valid lifetime is a significant change. - if (lease->valid_lft_ < lease->current_valid_lft_) { + // Valid lifetime has changed. + if (lease->valid_lft_ != lease->current_valid_lft_) { changed = true; } diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h index dcc594d6e9..8b493c0d25 100644 --- a/src/lib/dhcpsrv/alloc_engine.h +++ b/src/lib/dhcpsrv/alloc_engine.h @@ -1266,6 +1266,10 @@ public: /// update existing lease. bool fake_allocation_; + /// @brief If not zero, then we will allocate on DISCOVER for this + /// amount of time. + uint32_t offer_lft_; + /// @brief A pointer to an old lease that the client had before update. Lease4Ptr old_lease_; @@ -1355,12 +1359,14 @@ public: /// @param fake_allocation Is this real i.e. REQUEST (false) /// or just picking an address for DISCOVER that is not really /// allocated (true) + /// @param offer_lft When not zero, leases ARE allocated on DISCOVER and use + /// this value as lease lifetime. ClientContext4(const Subnet4Ptr& subnet, const ClientIdPtr& clientid, const HWAddrPtr& hwaddr, const asiolink::IOAddress& requested_addr, const bool fwd_dns_update, const bool rev_dns_update, - const std::string& hostname, const bool fake_allocation); - + const std::string& hostname, const bool fake_allocation, + const uint32_t offer_lft = 0); private: /// @brief Contains a pointer to the DDNS parameters for selected /// subnet. Set by the first call to getDdnsParams() made when @@ -1523,6 +1529,21 @@ public: /// @return unsigned integer value of the valid lifetime to use. static uint32_t getValidLft(const ClientContext4& ctx); + /// @brief Returns the offer lifetime based on the v4 context + /// + /// If the client query is a BOOTP query or something other than + /// DHCPDISCOVER, return 0. + /// + /// @todo Classes not supported yet. + /// Otherwise, the value will be selected from the first + /// class matched to the query which defines it or from the subnet + /// if none do. Classes are searched in the order they are assigned + /// to the query. + /// + /// @param ctx Client context holding various information about the client. + /// @return unsigned integer value of the offer lifetime to use. + static uint32_t getOfferLft(const ClientContext4& ctx); + 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 645b238925..a09e6873a6 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc @@ -5655,8 +5655,7 @@ TEST_F(PgSqlAllocEngine4Test, bootpDelete) { // Verifies that offer_lft is non-zero, that an offered lease is stored // in the lease database. -// @todo Currently fails, enable when offer-lft is functional. -TEST_F(AllocEngine4Test, DISABLED_discoverOfferLft) { +TEST_F(AllocEngine4Test, discoverOfferLft) { boost::scoped_ptr engine; ASSERT_NO_THROW(engine.reset(new AllocEngine(0))); ASSERT_TRUE(engine); @@ -5710,10 +5709,77 @@ TEST_F(AllocEngine4Test, DISABLED_discoverOfferLft) { ctx2.query_.reset(new Pkt4(DHCPDISCOVER, 1235)); // Verify that we did not get a lease. - lease = engine->allocateLease4(ctx1); + lease = engine->allocateLease4(ctx2); ASSERT_FALSE(lease); } +// Verifies that when offer_lft is non-zero, that an expired lease can +// be reclaimed an offered correctly. +TEST_F(AllocEngine4Test, discoverOfferLftReuseExpiredLease4) { + boost::scoped_ptr engine; + ASSERT_NO_THROW(engine.reset(new AllocEngine(0))); + ASSERT_TRUE(engine); + + IOAddress addr("192.0.2.15"); + CfgMgr& cfg_mgr = CfgMgr::instance(); + // Get rid of the default test configuration. + cfg_mgr.clear(); + + // Create configuration similar to other tests, but with a single address pool + subnet_ = Subnet4::create(IOAddress("192.0.2.0"), 24, 1, 2, 3); + pool_ = Pool4Ptr(new Pool4(addr, addr)); // just a single address + subnet_->addPool(pool_); + + // Set subnet's offer-lft to a non-zero, positive value. + uint32_t offer_lft = (subnet_->getValid() / 3); + subnet_->setOfferLft(offer_lft); + ASSERT_EQ(offer_lft, subnet_->getOfferLft().get()); + + cfg_mgr.getStagingCfg()->getCfgSubnets4()->add(subnet_); + + // Just a different hw/client-id for the second client + uint8_t hwaddr2_data[] = { 0, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe }; + HWAddrPtr hwaddr2(new HWAddr(hwaddr2_data, sizeof(hwaddr2_data), HTYPE_ETHER)); + uint8_t clientid2[] = { 8, 7, 6, 5, 4, 3, 2, 1 }; + time_t now = time(NULL) - 500; // Allocated 500 seconds ago + Lease4Ptr lease(new Lease4(addr, hwaddr2, clientid2, sizeof(clientid2), + 495, now, subnet_->getID())); + // Copy the lease, so as it can be compared with the old lease returned + // by the allocation engine. + Lease4 original_lease(*lease); + + // Lease was assigned 500 seconds ago, but its valid lifetime is 495, so it + // is expired already + ASSERT_TRUE(lease->expired()); + ASSERT_TRUE(LeaseMgrFactory::instance().addLease(lease)); + + // CASE 1: Asking for any address + AllocEngine::ClientContext4 ctx1(subnet_, clientid_, hwaddr_, + IOAddress("0.0.0.0"), false, false, + "", true); + ctx1.query_.reset(new Pkt4(DHCPDISCOVER, 1234)); + lease = engine->allocateLease4(ctx1); + // Check that we got that single lease + ASSERT_TRUE(lease); + EXPECT_EQ(addr, lease->addr_); + EXPECT_EQ(offer_lft, lease->valid_lft_); + EXPECT_FALSE(lease->fqdn_fwd_); + EXPECT_FALSE(lease->fqdn_rev_); + + // We are reusing expired lease, the old (expired) instance should be + // returned. The returned instance should be the same as the original + // lease. + ASSERT_TRUE(ctx1.old_lease_); + EXPECT_TRUE(original_lease == *ctx1.old_lease_); + + // Check that the lease has been stored by LeaseMgr. + Lease4Ptr from_mgr = LeaseMgrFactory::instance().getLease4(lease->addr_); + ASSERT_TRUE(from_mgr); + EXPECT_EQ(offer_lft, from_mgr->valid_lft_); + EXPECT_FALSE(from_mgr->fqdn_fwd_); + EXPECT_FALSE(from_mgr->fqdn_rev_); +} + } // namespace test } // namespace dhcp } // namespace isc diff --git a/src/lib/dhcpsrv/tests/alloc_engine_hooks_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine_hooks_unittest.cc index 11e5c0a3d1..1585dd0bcb 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine_hooks_unittest.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine_hooks_unittest.cc @@ -551,6 +551,7 @@ TEST_F(HookAllocEngine4Test, lease4_select) { vector expected_argument_names; expected_argument_names.push_back("fake_allocation"); expected_argument_names.push_back("lease4"); + expected_argument_names.push_back("offer_lft"); expected_argument_names.push_back("query4"); expected_argument_names.push_back("subnet4"); EXPECT_TRUE(callback_argument_names_ == expected_argument_names); -- 2.47.2