[#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

@@ -2855,8 +2855,12 @@ Dhcpv4Srv::assignLease(Dhcpv4Exchange& ex) {
         }
 
         // IP Address Lease time (type 51)
-        OptionPtr opt(new OptionUint32(Option::V4, DHO_DHCP_LEASE_TIME,
+        // If we're not allocating on discover  then we just sent the lifetime on the lease.
-                                       lease->valid_lft_));
+        // 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;
 

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<uint32_t> 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.
+    // Get the context appropriate lifetime.
-    uint32_t valid_lft = getValidLft(ctx);
+    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.
+    // Valid lifetime has changed.
-    if (lease->valid_lft_ < lease->current_valid_lft_) {
+    if (lease->valid_lft_ != lease->current_valid_lft_) {
         changed = true;
     }
 

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.

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, discoverOfferLft) {
-TEST_F(AllocEngine4Test, DISABLED_discoverOfferLft) {
     boost::scoped_ptr<AllocEngine> 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<AllocEngine> 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

src/lib/dhcpsrv/tests/alloc_engine_hooks_unittest.cc

@@ -551,6 +551,7 @@ TEST_F(HookAllocEngine4Test, lease4_select) {
     vector<string> 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);