[3709] allocateLease4 now uses ClientContext4

src/bin/dhcp4/dhcp4_srv.cc

@@ -1077,13 +1077,11 @@ Dhcpv4Srv::assignLease(const Pkt4Ptr& question, Pkt4Ptr& answer) {
     // may be used instead. If fake_allocation is set to false, the lease will
     // be inserted into the LeaseMgr as well.
     /// @todo pass the actual FQDN data.
-    Lease4Ptr old_lease;
-    Lease4Ptr lease = alloc_engine_->allocateLease4(subnet, client_id, hwaddr,
-                                                    hint, fqdn_fwd, fqdn_rev,
-                                                    hostname,
-                                                    fake_allocation,
-                                                    callout_handle,
-                                                    old_lease);
+    AllocEngine::ClientContext4 ctx(subnet, client_id, hwaddr, hint, fqdn_fwd,
+                                    fqdn_rev, hostname, fake_allocation);
+    ctx.callout_handle_ = callout_handle;
+
+    Lease4Ptr lease = alloc_engine_->allocateLease4(ctx);
 
     if (lease) {
         // We have a lease! Let's set it in the packet and send it back to
@@ -1172,7 +1170,7 @@ Dhcpv4Srv::assignLease(const Pkt4Ptr& question, Pkt4Ptr& answer) {
         // real allocation.
         if (!fake_allocation && CfgMgr::instance().ddnsEnabled()) {
             try {
-                createNameChangeRequests(lease, old_lease);
+                createNameChangeRequests(lease, ctx.old_lease_);
             } catch (const Exception& ex) {
                 LOG_ERROR(dhcp4_logger, DHCP4_NCR_CREATION_FAILED)
                     .arg(ex.what());

src/lib/dhcpsrv/alloc_engine.cc

@@ -801,54 +801,27 @@ AllocEngine::removeNonreservedLeases6(ClientContext6& ctx,
 }
 
 Lease4Ptr
-AllocEngine::allocateLease4(const SubnetPtr& subnet, const ClientIdPtr& clientid,
-                            const HWAddrPtr& hwaddr, const IOAddress& hint,
-                            const bool fwd_dns_update, const bool rev_dns_update,
-                            const std::string& hostname, bool fake_allocation,
-                            const isc::hooks::CalloutHandlePtr& callout_handle,
-                            Lease4Ptr& old_lease) {
+AllocEngine::allocateLease4(ClientContext4& ctx) {
 
     // The NULL pointer indicates that the old lease didn't exist. It may
     // be later set to non NULL value if existing lease is found in the
     // database.
-    old_lease.reset();
+    ctx.old_lease_.reset();
 
     try {
 
         // Set allocator.
         AllocatorPtr allocator = getAllocator(Lease::TYPE_V4);
 
-        if (!subnet) {
+        if (!ctx.subnet_) {
             isc_throw(BadValue, "Can't allocate IPv4 address without subnet");
         }
 
-        if (!hwaddr) {
+        if (!ctx.hwaddr_) {
             isc_throw(BadValue, "HWAddr must be defined");
         }
 
-        /// @todo The context for lease allocation should really be created
-        /// by the DHCPv4 server and passed to this function. The reason for
-        /// this is that the server should retrieve the Host object for the
-        /// client because the Host object contains the data not only useful
-        /// for the address allocation but also hostname and DHCP options
-        /// for the client. The Host object should be passed in the context.
-        /// Making this change would require a change to the allocateLease4
-        /// API which would in turn require lots of changes in unit tests.
-        /// The ticket introducing a context and host reservation in the
-        /// allocation engine is complex enough by itself to warrant that
-        /// the API change is done with a separate ticket (#3709).
-        ClientContext4 ctx;
-        ctx.subnet_ = subnet;
-        ctx.clientid_ = clientid;
-        ctx.hwaddr_ = hwaddr;
-        ctx.requested_address_ = hint;
-        ctx.fwd_dns_update_ = fwd_dns_update;
-        ctx.rev_dns_update_ = rev_dns_update;
-        ctx.hostname_ = hostname;
-        ctx.fake_allocation_ = fake_allocation;
-        ctx.callout_handle_ = callout_handle;
-        ctx.old_lease_ = old_lease;
-        ctx.host_ = HostMgr::instance().get4(subnet->getID(), hwaddr);
+        ctx.host_ = HostMgr::instance().get4(ctx.subnet_->getID(), ctx.hwaddr_);
 
         // If there is a reservation for this client we want to allocate the
         // reserved address to the client, rather than any other address.
@@ -876,9 +849,9 @@ AllocEngine::allocateLease4(const SubnetPtr& subnet, const ClientIdPtr& clientid
         // Check if the client has any leases in the lease database, using HW
         // address or client identifier.
         LeaseMgr& lease_mgr = LeaseMgrFactory::instance();
-        Lease4Ptr existing = lease_mgr.getLease4(*hwaddr, ctx.subnet_->getID());
-        if (!existing && clientid) {
-            existing = lease_mgr.getLease4(*clientid, ctx.subnet_->getID());
+        Lease4Ptr existing = lease_mgr.getLease4(*ctx.hwaddr_, ctx.subnet_->getID());
+        if (!existing && ctx.clientid_) {
+            existing = lease_mgr.getLease4(*ctx.clientid_, ctx.subnet_->getID());
         }
 
         // If client has a lease there are two choices. The server may need
@@ -896,7 +869,6 @@ AllocEngine::allocateLease4(const SubnetPtr& subnet, const ClientIdPtr& clientid
             // send DHCPNAK to indicate that the client should try to
             // start over the allocation process.
             if (ctx.interrupt_processing_) {
-                old_lease = ctx.old_lease_;
                 return (Lease4Ptr());
 
             // If we tried to reallocate the reserved lease we return
@@ -904,7 +876,6 @@ AllocEngine::allocateLease4(const SubnetPtr& subnet, const ClientIdPtr& clientid
             // We also return when allocation passed, no matter if this
             // was a reserved address or not.
             } else  if (ctx.host_ || existing) {
-                old_lease = ctx.old_lease_;
                 return (existing);
             }
         }
@@ -913,7 +884,7 @@ AllocEngine::allocateLease4(const SubnetPtr& subnet, const ClientIdPtr& clientid
         // proceed with a new allocation. We will try to allocate a
         // reserved address or an address from a dynamic pool if there is
         // no reservation.
-        if (ctx.host_ || subnet->inPool(Lease::TYPE_V4, ctx.requested_address_)) {
+        if (ctx.host_ || ctx.subnet_->inPool(Lease::TYPE_V4, ctx.requested_address_)) {
             // If a client is requesting specific IP address, but the
             // reservation was made for a different address the server returns
             // NAK to the client. By returning NULL lease here we indicate to
@@ -955,11 +926,13 @@ AllocEngine::allocateLease4(const SubnetPtr& subnet, const ClientIdPtr& clientid
                 if (!existing) {
                     // The candidate address is currently unused. Let's create a
                     // lease for it.
-                    Lease4Ptr lease = createLease4(subnet, clientid, hwaddr,
-                                                   candidate, fwd_dns_update,
-                                                   rev_dns_update,
-                                                   hostname, callout_handle,
-                                                   fake_allocation);
+                    Lease4Ptr lease = createLease4(ctx.subnet_, ctx.clientid_,
+                                                   ctx.hwaddr_, candidate,
+                                                   ctx.fwd_dns_update_,
+                                                   ctx.rev_dns_update_,
+                                                   ctx.hostname_,
+                                                   ctx.callout_handle_,
+                                                   ctx.fake_allocation_);
 
                     // If we have allocated the lease let's return it. Also,
                     // always return when tried to allocate reserved address,
@@ -977,7 +950,7 @@ AllocEngine::allocateLease4(const SubnetPtr& subnet, const ClientIdPtr& clientid
                 } else {
                     if (existing->expired()) {
                         // Save the old lease, before reusing it.
-                        old_lease.reset(new Lease4(*existing));
+                        ctx.old_lease_.reset(new Lease4(*existing));
                         return (reuseExpiredLease(existing, ctx));
 
                         // The existing lease is not expired (is in use by some
@@ -1001,24 +974,19 @@ AllocEngine::allocateLease4(const SubnetPtr& subnet, const ClientIdPtr& clientid
         // - we find a free address
         // - we find an address for which the lease has expired
         // - we exhaust the number of tries
-        //
-        /// @todo: We used to use hardcoded number of attempts (100). Now we dynamically
-        /// calculate the number of possible leases in all pools in this subnet and
-        /// try that number of times at most. It would be useful to that value if
-        /// attempts_, specified by the user could override that value (and keep
-        /// dynamic if they're set to 0).
-        uint64_t i = subnet->getPoolCapacity(Lease::TYPE_V4);
+        uint64_t i = ctx.subnet_->getPoolCapacity(Lease::TYPE_V4);
         do {
             // Decrease the number of remaining attempts here so as we guarantee
             // that it is decreased when the code below uses "continue".
             --i;
-            IOAddress candidate = allocator->pickAddress(subnet, clientid,
+            IOAddress candidate = allocator->pickAddress(ctx.subnet_,
+                                                         ctx.clientid_,
                                                          ctx.requested_address_);
 
             // Check if this address is reserved. There is no need to check for
             // whom it is reserved, because if it has been reserved for us we would
             // have already allocated a lease.
-            if (HostMgr::instance().get4(subnet->getID(), candidate)) {
+            if (HostMgr::instance().get4(ctx.subnet_->getID(), candidate)) {
                 // Don't allocate a reserved address.
                 continue;
             }
@@ -1027,10 +995,10 @@ AllocEngine::allocateLease4(const SubnetPtr& subnet, const ClientIdPtr& clientid
             if (!existing) {
                 // there's no existing lease for selected candidate, so it is
                 // free. Let's allocate it.
-                Lease4Ptr lease = createLease4(subnet, clientid, hwaddr,
-                                               candidate, fwd_dns_update,
-                                               rev_dns_update, hostname,
-                                               callout_handle, fake_allocation);
+                Lease4Ptr lease = createLease4(ctx.subnet_, ctx.clientid_, ctx.hwaddr_,
+                                               candidate, ctx.fwd_dns_update_,
+                                               ctx.rev_dns_update_, ctx.hostname_,
+                                               ctx.callout_handle_, ctx.fake_allocation_);
                 if (lease) {
                     return (lease);
                 }
@@ -1041,7 +1009,7 @@ AllocEngine::allocateLease4(const SubnetPtr& subnet, const ClientIdPtr& clientid
             } else {
                 if (existing->expired()) {
                     // Save old lease before reusing it.
-                    old_lease.reset(new Lease4(*existing));
+                    ctx.old_lease_.reset(new Lease4(*existing));
                     return (reuseExpiredLease(existing, ctx));
                 }
             }

src/lib/dhcpsrv/alloc_engine.h

@@ -225,6 +225,40 @@ protected:
     /// information to the allocation engine methods is that adding
     /// new information doesn't modify the API of the allocation engine.
     struct ClientContext4 {
+        /// @brief Default constructor.
+        ClientContext4()
+            : subnet_(), clientid_(), hwaddr_(), requested_address_("0.0.0.0"),
+              fwd_dns_update_(false), rev_dns_update_(false),
+              hostname_(""), callout_handle_(), fake_allocation_(false),
+              old_lease_(), host_(), interrupt_processing_(false) {
+        }
+
+        /// @brief Constructor with parameters
+        /// @param subnet subnet the allocation should come from (mandatory)
+        /// @param clientid Client identifier (optional)
+        /// @param hwaddr Client's hardware address info (mandatory)
+        /// @param hint A hint that the client provided (may be 0.0.0.0)
+        /// @param fwd_dns_update Indicates whether forward DNS
+        ///      update will be performed for the client (true) or not (false).
+        /// @param rev_dns_update Indicates whether reverse DNS
+        ///      update will be performed for the client (true) or not (false).
+        /// @param hostname A string carrying hostname to be used for DNS updates.
+        /// @param fake_allocation Is this real i.e. REQUEST (false)
+        ///      or just picking an address for DISCOVER that is not really
+        ///      allocated (true)
+        ClientContext4(const SubnetPtr& 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)
+            : 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_(), host_(),
+            interrupt_processing_(false) {
+        }
+
         /// @brief Subnet selected for the client by the server.
         SubnetPtr subnet_;
 
@@ -283,14 +317,6 @@ protected:
         /// @c AllocEngine::renewLease4 sets this flag so as the
         /// upstream methods return the NULL lease pointer to the server.
         bool interrupt_processing_;
-
-        /// @brief Default constructor.
-        ClientContext4()
-            : subnet_(), clientid_(), hwaddr_(), requested_address_("0.0.0.0"),
-              fwd_dns_update_(false), rev_dns_update_(false),
-              hostname_(""), callout_handle_(), fake_allocation_(false),
-              old_lease_(), host_(), interrupt_processing_(false) {
-        }
     };
 
     /// @brief Defines a single hint (an address + prefix-length).
@@ -552,35 +578,32 @@ protected:
     /// returned, it is an indication that allocation engine reused/renewed an
     /// existing lease.
     ///
-    /// @todo Replace parameters with a single parameter of a
-    /// @c ClientContext4 type.
-    ///
-    /// @param subnet subnet the allocation should come from
-    /// @param clientid Client identifier
-    /// @param hwaddr Client's hardware address info
-    /// @param hint A hint that the client provided
-    /// @param fwd_dns_update Indicates whether forward DNS update will be
-    ///        performed for the client (true) or not (false).
-    /// @param rev_dns_update Indicates whether reverse DNS update will be
-    ///        performed for the client (true) or not (false).
-    /// @param hostname A string carrying hostname to be used for DNS updates.
-    /// @param fake_allocation Is this real i.e. REQUEST (false) or just picking
-    ///        an address for DISCOVER that is not really allocated (true)
-    /// @param callout_handle A callout handle (used in hooks). A lease callouts
-    ///        will be executed if this parameter is passed.
-    /// @param [out] old_lease Holds the pointer to a previous instance of a
-    ///        lease. The NULL pointer indicates that lease didn't exist prior
-    ///        to calling this function (e.g. new lease has been allocated).
+    /// @param ctx client context that passes all necessary information. See
+    ///        @ref ClientContext4 for details.
+    ///
+    /// The following fields of @ref ClientContext4 are used:
+    ///
+    /// @ref ClientContext4::subnet_ subnet the allocation should come from
+    /// @ref ClientContext4::clientid_ Client identifier
+    /// @ref ClientContext4::hwaddr_ Client's hardware address info
+    /// @ref ClientContext4::hint_ A hint that the client provided
+    /// @ref ClientContext4::fwd_dns_update_ Indicates whether forward DNS
+    ///      update will be performed for the client (true) or not (false).
+    /// @ref ClientContext4::rev_dns_update_ Indicates whether reverse DNS
+    ///      update will be performed for the client (true) or not (false).
+    /// @ref ClientContext4::hostname_ A string carrying hostname to be used for
+    ///      DNS updates.
+    /// @ref ClientContext4::fake_allocation_ Is this real i.e. REQUEST (false)
+    ///      or just picking an address for DISCOVER that is not really
+    ///      allocated (true)
+    /// @ref ClientContext4::callout_handle_ A callout handle (used in hooks).
+    ///      A lease callouts will be executed if this parameter is passed.
+    /// @ref ClientContext4::old_lease_ [out] Holds the pointer to a previous
+    ///      instance of a lease. The NULL pointer indicates that lease didn't
+    ///      exist prior to calling this function (e.g. new lease has been allocated).
     ///
     /// @return Allocated IPv4 lease (or NULL if allocation failed).
-    Lease4Ptr
-    allocateLease4(const SubnetPtr& subnet, const ClientIdPtr& clientid,
-                   const HWAddrPtr& hwaddr,
-                   const isc::asiolink::IOAddress& hint,
-                   const bool fwd_dns_update, const bool rev_dns_update,
-                   const std::string& hostname, bool fake_allocation,
-                   const isc::hooks::CalloutHandlePtr& callout_handle,
-                   Lease4Ptr& old_lease);
+    Lease4Ptr allocateLease4(ClientContext4& ctx);
 
     /// @brief Renews an DHCPv4 lease.
     ///

src/lib/dhcpsrv/tests/alloc_engine_hooks_unittest.cc

@@ -383,11 +383,13 @@ TEST_F(HookAllocEngine4Test, lease4_select) {
 
     CalloutHandlePtr callout_handle = HooksManager::createCalloutHandle();
 
-    Lease4Ptr lease = engine->allocateLease4(subnet_, clientid_, hwaddr_,
-                                               IOAddress("0.0.0.0"),
-                                               false, false, "",
-                                               false, callout_handle,
-                                               old_lease_);
+    AllocEngine::ClientContext4 ctx(subnet_, clientid_, hwaddr_,
+                                    IOAddress("0.0.0.0"),
+                                    false, false, "", false);
+    ctx.callout_handle_ = callout_handle;
+
+    Lease4Ptr lease = engine->allocateLease4(ctx);
+
     // Check that we got a lease
     ASSERT_TRUE(lease);
 
@@ -449,12 +451,14 @@ TEST_F(HookAllocEngine4Test, change_lease4_select) {
     // but in tests we need to create it on our own.
     CalloutHandlePtr callout_handle = HooksManager::createCalloutHandle();
 
+
+    AllocEngine::ClientContext4 ctx(subnet_, clientid_, hwaddr_, IOAddress("0.0.0.0"),
+                                    false, true, "somehost.example.com.", false);
+    ctx.callout_handle_ = callout_handle;
+
     // Call allocateLease4. Callouts should be triggered here.
-    Lease4Ptr lease = engine->allocateLease4(subnet_, clientid_, hwaddr_,
-                                             IOAddress("0.0.0.0"),
-                                             false, false, "",
-                                             false, callout_handle,
-                                             old_lease_);
+    Lease4Ptr lease = engine->allocateLease4(ctx);
+
     // Check that we got a lease
     ASSERT_TRUE(lease);
 

src/lib/dhcpsrv/tests/alloc_engine_utils.h

@@ -379,7 +379,6 @@ public:
     Subnet4Ptr subnet_;         ///< Subnet4 (used in tests)
     Pool4Ptr pool_;             ///< Pool belonging to subnet_
     LeaseMgrFactory factory_;   ///< Pointer to LeaseMgr factory
-    Lease4Ptr old_lease_;       ///< Holds previous instance of the lease.
     AllocEngine::ClientContext4 ctx_; ///< Context information passed to various
                                      ///< allocation engine functions.
 };