[#2719] Do not shorten exiting leases

Augmented UTs to issue DHCPREQUESTS
Altered alloc engine to skip temp allocation when
there is an existing lease is longer than offer_lft

diff --git a/src/bin/dhcp4/tests/fqdn_unittest.cc b/src/bin/dhcp4/tests/fqdn_unittest.cc
index 89e98ca733d245f5136ebb4b7f947380bde0f4ea..46b8973d790852e52e9e2f62460f31d55ebe8e09 100644 (file)
--- a/src/bin/dhcp4/tests/fqdn_unittest.cc
+++ b/src/bin/dhcp4/tests/fqdn_unittest.cc
@@ -2771,7 +2771,7 @@ TEST_F(NameDhcpv4SrvTest, withOfferLifetime) {
     // When receiving DHCPDISCOVER, no NCRs should be generated.
     EXPECT_EQ(0, d2_mgr_.getQueueSize());
 
-    // Make sure thed lease was created with offer-lifetime, fqdn flags = false,
+    // Make sure the lease was created with offer-lifetime, fqdn flags = false,
     // and the FQDN.
     Lease4Ptr lease = LeaseMgrFactory::instance().getLease4(IOAddress("10.0.0.10"));
     ASSERT_TRUE(lease);

diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc
index 6948742790bdaa131ad39f52f282ddab686a7bcc..64ec6dce171cb84731003cb60561787d5bc269b2 100644 (file)
--- a/src/lib/dhcpsrv/alloc_engine.cc
+++ b/src/lib/dhcpsrv/alloc_engine.cc
@@ -3628,6 +3628,14 @@ AllocEngine::discoverLease4(AllocEngine::ClientContext4& ctx) {
                   ALLOC_ENGINE_V4_OFFER_EXISTING_LEASE)
             .arg(ctx.query_->getLabel());
 
+        // If offer-lifetime is shorter than the existing expiration, reset
+        // offer-lifetime to zero.  This allows us to simply return the
+        // existing lease without updating it in the lease store.
+        if ((ctx.offer_lft_) &&
+            (time(NULL) + ctx.offer_lft_ < client_lease->getExpirationTime())) {
+                ctx.offer_lft_ = 0;
+        }
+
         new_lease = renewLease4(client_lease, ctx);
     }
 

diff --git a/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc
index f375756577dd0c125e0c2bad60c08f7d0ee15c7a..337065425c0cf1490286cfcdf5eb6cba14eb9c7a 100644 (file)
--- a/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc
+++ b/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc
@@ -5711,6 +5711,81 @@ TEST_F(AllocEngine4Test, discoverOfferLft) {
     // Verify that we did not get a lease.
     lease = engine->allocateLease4(ctx2);
     ASSERT_FALSE(lease);
+
+    // Original client now does a DHCPREQUEST.  Make sure we get the
+    // previously offered lease with the proper valid lifetime.
+    AllocEngine::ClientContext4 ctx3(subnet_, clientid_, hwaddr_,
+                                     IOAddress("0.0.0.0"), true, true,
+                                     "one", false);
+    ctx3.query_.reset(new Pkt4(DHCPREQUEST, 1236));
+
+    lease = engine->allocateLease4(ctx3);
+    ASSERT_TRUE(lease);
+    EXPECT_EQ(addr, lease->addr_);
+    from_mgr = LeaseMgrFactory::instance().getLease4(lease->addr_);
+    ASSERT_TRUE(from_mgr);
+    EXPECT_EQ(subnet_->getValid(), from_mgr->valid_lft_);
+}
+
+// Verifies that when offer_lft is non-zero, that an existing lease is
+// whose remaining life is larger than offer_lft, is offered as is.
+TEST_F(AllocEngine4Test, discoverOfferLftUseExistingLease4) {
+    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, 300);
+    pool_ = Pool4Ptr(new Pool4(addr, addr)); // just a single address
+    subnet_->addPool(pool_);
+
+    // Set subnet's offer-lifetime to a non-zero, positive value.
+    uint32_t offer_lft = 100;
+    subnet_->setOfferLft(offer_lft);
+    ASSERT_EQ(offer_lft, subnet_->getOfferLft().get());
+
+    cfg_mgr.getStagingCfg()->getCfgSubnets4()->add(subnet_);
+
+    time_t now = time(NULL) - 5; // Allocated 5 seconds ago
+    Lease4Ptr lease(new Lease4(addr, hwaddr_, clientid_, 300, now,
+                               subnet_->getID(), true, true, "somehost"));
+
+    // 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 5 seconds ago, its valid lifetime is 300, its
+    // remaining lifetime is still larger than offer_lft.
+    ASSERT_FALSE(lease->expired());
+    ASSERT_EQ(300, lease->valid_lft_);
+    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(300, lease->valid_lft_);
+
+    // We are reusing the existing lease, the old 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 not been modified by LeaseMgr.
+    Lease4Ptr from_mgr = LeaseMgrFactory::instance().getLease4(lease->addr_);
+    ASSERT_TRUE(from_mgr);
+    EXPECT_TRUE(original_lease == *from_mgr);
 }
 
 // Verifies that when offer_lft is non-zero, that an expired lease can
@@ -5778,6 +5853,20 @@ TEST_F(AllocEngine4Test, discoverOfferLftReuseExpiredLease4) {
     EXPECT_EQ(offer_lft, from_mgr->valid_lft_);
     EXPECT_FALSE(from_mgr->fqdn_fwd_);
     EXPECT_FALSE(from_mgr->fqdn_rev_);
+
+    // Client now does a DHCPREQUEST.  Make sure we get the
+    // previously offered lease with the proper valid lifetime.
+    AllocEngine::ClientContext4 ctx2(subnet_, clientid_, hwaddr_,
+                                     IOAddress("0.0.0.0"), true, true,
+                                     "one", false);
+    ctx2.query_.reset(new Pkt4(DHCPREQUEST, 1235));
+
+    lease = engine->allocateLease4(ctx2);
+    ASSERT_TRUE(lease);
+    EXPECT_EQ(addr, lease->addr_);
+    from_mgr = LeaseMgrFactory::instance().getLease4(lease->addr_);
+    ASSERT_TRUE(from_mgr);
+    EXPECT_EQ(subnet_->getValid(), from_mgr->valid_lft_);
 }
 
 // Verifies that AllocEngine::getOfferLft(ctx4) returns the appropriate
@@ -5818,41 +5907,50 @@ TEST_F(AllocEngine4Test, getOfferLft4) {
         std::string desc_;                  // descriptive text for logging
         std::vector<std::string> classes_;  // class list of assigned classes
         uint32_t exp_valid_lft_;            // expected lease lifetime
+        bool exp_allocate_;                 // true if lease should be allocated
     };
 
+    bool exp_allocate = true;
+
     // Scenarios to test.
     std::vector<Scenario> scenarios = {
         {
             "BOOTP",
             { "BOOTP" },
-            Lease::INFINITY_LFT
+            Lease::INFINITY_LFT,
+            !exp_allocate,
         },
         {
             "no classes",
             {},
-            subnet_->getOfferLft()
+            subnet_->getOfferLft(),
+            exp_allocate
         },
         {
             "class unspecified",
             { "offer_lft_unspec" },
-            subnet_->getOfferLft()
+            subnet_->getOfferLft(),
+            exp_allocate
         },
         {
             "from last class",
             { "offer_lft_unspec", "offer_lft_one" },
-            offer_lft_one.get()
+            offer_lft_one.get(),
+            exp_allocate
         },
         {
             "from first class",
             { "offer_lft_two", "offer_lft_one" },
-            offer_lft_two.get()
+            offer_lft_two.get(),
+            exp_allocate
         },
         {
             // Useing class value of zero should override non-zero set at
             // subnet level, lease should have actual valid lft
             "zero from class",
             { "offer_lft_zero" },
-            subnet_->getValid()
+            subnet_->getValid(),
+            !exp_allocate
         }
     };
 
@@ -5873,6 +5971,15 @@ TEST_F(AllocEngine4Test, getOfferLft4) {
             Lease4Ptr lease = engine.allocateLease4(ctx);
             ASSERT_TRUE(lease);
             EXPECT_EQ(lease->valid_lft_, scenario.exp_valid_lft_);
+
+            Lease4Ptr from_mgr = LeaseMgrFactory::instance().getLease4(lease->addr_);
+            if (!scenario.exp_allocate_) {
+                ASSERT_FALSE(from_mgr);
+            } else {
+                ASSERT_TRUE(from_mgr);
+                EXPECT_EQ(from_mgr->valid_lft_, scenario.exp_valid_lft_);
+                ASSERT_TRUE(LeaseMgrFactory::instance().deleteLease(from_mgr));
+            }
         }
     }
 }

diff --git a/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc b/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc
index d0f3f989c01231826fe251ac99d9097735f7191b..9f157bf2dbf3d86fbb0c7b29dc8566ebfd31fe9b 100644 (file)
--- a/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc
+++ b/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc
@@ -2075,7 +2075,7 @@ TEST_F(ClientClassDefParserTest, offerLft) {
     EXPECT_EQ(99, offer_lft);
 }
 
-// Test verifies that the parser rejects bogus server-hostname value.
+// Test verifies that the parser rejects bogus offer-lifetime value.
 TEST_F(ClientClassDefParserTest, offerLftInvalid) {
     std::string cfg_text =
         "{ \n"