[master] Merge branch 'trac3988' (lease{4,6}_recover hooks)

# Conflicts:
#	src/lib/dhcpsrv/alloc_engine.cc
#	src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc

src/bin/dhcp4/dhcp4_hooks.dox

@@ -274,4 +274,23 @@ packet processing. Hook points that are not specific to packet processing
   skip flag is only set when the lease has been actually reclaimed in the
   database by the callout.
 
+@subsection dhcpv4HooksLease4Recover lease4_recover
+
+- @b Arguments:
+  - name: @b lease4, type: isc::dhcp::Lease4Ptr, direction: <b>in</b>
+
+- @b Description: this callout is executed for each declined lease that
+  has expired (was put aside for the duration of decline-probation-period)
+  and is being recovered. The lease has already been stripped
+  from any client identifying information when it was put into declined
+  state. In principle the callouts can modify the lease in this hook,
+  but it makes little sense. There's no useful data in the lease, except
+  the IPv4 address (which must not be modified).
+
+- <b>Next step status</b>: if the callout sets the next step action to SKIP,
+  the server will skip the lease recovery. In other words, it will keep
+  the lease as is. This is not recommended in general, as the declined
+  expired leases will remain in the database and their recovery will
+  be attempted during the next reclaim cycle.
+
 */

src/bin/dhcp6/dhcp6_hooks.dox

@@ -288,4 +288,23 @@ packet processing. Hook points that are not specific to packet processing
   skip flag is only set when the lease has been actually reclaimed in the
   database by the callout.
 
+@subsection dhcpv6HooksLease6Recover lease6_recover
+
+- @b Arguments:
+  - name: @b lease6, type: isc::dhcp::Lease6Ptr, direction: <b>in</b>
+
+- @b Description: this callout is executed for each declined lease that
+  has expired (was put aside for the duration of decline-probation-period)
+  and is being recovered. The lease has already been stripped
+  from any client identifying information when it was put into declined
+  state. In principle the callouts can modify the lease in this hook,
+  but it makes little sense. There's no useful data in the lease, except
+  the IPv6 address (which must not be modified).
+
+- <b>Next step status</b>: if the callout sets the next step action to SKIP,
+  the server will skip the lease recovery. In other words, it will keep
+  the lease as is. This is not recommended in general, as the declined
+  expired leases will remain in the database and their recovery will
+  be attempted during the next reclaim cycle.
+
 */

src/lib/dhcpsrv/alloc_engine.cc

@@ -27,6 +27,7 @@
 #include <dhcpsrv/ncr_generator.h>
 #include <hooks/callout_handle.h>
 #include <hooks/hooks_manager.h>
+#include <dhcpsrv/callout_handle_store.h>
 #include <stats/stats_mgr.h>
 #include <util/stopwatch.h>
 #include <hooks/server_hooks.h>
@@ -54,20 +55,24 @@ struct AllocEngineHooks {
     int hook_index_lease4_select_; ///< index for "lease4_receive" hook point
     int hook_index_lease4_renew_;  ///< index for "lease4_renew" hook point
     int hook_index_lease4_expire_; ///< index for "lease4_expire" hook point
+    int hook_index_lease4_recover_;///< index for "lease4_recover" hook point
     int hook_index_lease6_select_; ///< index for "lease6_receive" hook point
     int hook_index_lease6_renew_;  ///< index for "lease6_renew" hook point
     int hook_index_lease6_rebind_; ///< index for "lease6_rebind" hook point
     int hook_index_lease6_expire_; ///< index for "lease6_expire" hook point
+    int hook_index_lease6_recover_;///< index for "lease6_recover" hook point
 
     /// Constructor that registers hook points for AllocationEngine
     AllocEngineHooks() {
         hook_index_lease4_select_ = HooksManager::registerHook("lease4_select");
         hook_index_lease4_renew_  = HooksManager::registerHook("lease4_renew");
         hook_index_lease4_expire_ = HooksManager::registerHook("lease4_expire");
+        hook_index_lease4_recover_= HooksManager::registerHook("lease4_recover");
         hook_index_lease6_select_ = HooksManager::registerHook("lease6_select");
         hook_index_lease6_renew_  = HooksManager::registerHook("lease6_renew");
         hook_index_lease6_rebind_ = HooksManager::registerHook("lease6_rebind");
         hook_index_lease6_expire_ = HooksManager::registerHook("lease6_expire");
+        hook_index_lease6_recover_= HooksManager::registerHook("lease6_recover");
     }
 };
 
@@ -1650,7 +1655,7 @@ AllocEngine::reclaimExpiredLease(const Lease6Ptr& lease,
             // Do extra steps required for declined lease reclaimation:
             // - bump decline-related stats
             // - log separate message
-            reclaimDeclined(lease);
+            remove_tmp = reclaimDeclined(lease);
         }
 
         if (reclaim_mode != DB_RECLAIM_LEAVE_UNCHANGED) {
@@ -1743,7 +1748,7 @@ AllocEngine::reclaimExpiredLease(const Lease4Ptr& lease,
             // Do extra steps required for declined lease reclaimation:
             // - bump decline-related stats
             // - log separate message
-            reclaimDeclined(lease);
+            remove_tmp = reclaimDeclined(lease);
         }
 
         if (reclaim_mode != DB_RECLAIM_LEAVE_UNCHANGED) {
@@ -1794,11 +1799,39 @@ AllocEngine::deleteExpiredReclaimedLeases4(const uint32_t secs) {
         .arg(deleted_leases);
 }
 
-void
+bool
 AllocEngine::reclaimDeclined(const Lease4Ptr& lease) {
 
     if (!lease || (lease->state_ != Lease::STATE_DECLINED) ) {
-        return;
+        return (true);
+    }
+
+    if (HooksManager::getHooksManager().calloutsPresent(Hooks.hook_index_lease4_recover_)) {
+
+        // Let's use a static callout handle. It will be initialized the first
+        // time lease4_recover is called and will keep to that value.
+        static CalloutHandlePtr callout_handle;
+        if (!callout_handle) {
+            callout_handle = HooksManager::createCalloutHandle();
+        }
+
+        // Delete all previous arguments
+        callout_handle->deleteAllArguments();
+
+        // Pass necessary arguments
+        callout_handle->setArgument("lease4", lease);
+
+        // Call the callouts
+        HooksManager::callCallouts(Hooks.hook_index_lease4_recover_, *callout_handle);
+
+        // Callouts decided to skip the action. This means that the lease is not
+        // assigned, so the client will get NoAddrAvail as a result. The lease
+        // won't be inserted into the database.
+        if (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_SKIP) {
+            LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_HOOKS, DHCPSRV_HOOK_LEASE4_RECOVER_SKIP)
+                .arg(lease->addr_.toText());
+            return (false);
+        }
     }
 
     LOG_INFO(alloc_engine_logger, ALLOC_ENGINE_V4_DECLINED_RECOVERED)
@@ -1821,15 +1854,42 @@ AllocEngine::reclaimDeclined(const Lease4Ptr& lease) {
 
     // Note that we do not touch assigned-addresses counters. Those are
     // modified in whatever code calls this method.
-
-    /// @todo: call lease4_decline_recycle hook here.
+    return (true);
 }
 
-void
+bool
 AllocEngine::reclaimDeclined(const Lease6Ptr& lease) {
 
     if (!lease || (lease->state_ != Lease::STATE_DECLINED) ) {
-        return;
+        return (true);
+    }
+
+    if (HooksManager::getHooksManager().calloutsPresent(Hooks.hook_index_lease6_recover_)) {
+
+        // Let's use a static callout handle. It will be initialized the first
+        // time lease6_recover is called and will keep to that value.
+        static CalloutHandlePtr callout_handle;
+        if (!callout_handle) {
+            callout_handle = HooksManager::createCalloutHandle();
+        }
+
+        // Delete all previous arguments
+        callout_handle->deleteAllArguments();
+
+        // Pass necessary arguments
+        callout_handle->setArgument("lease6", lease);
+
+        // Call the callouts
+        HooksManager::callCallouts(Hooks.hook_index_lease6_recover_, *callout_handle);
+
+        // Callouts decided to skip the action. This means that the lease is not
+        // assigned, so the client will get NoAddrAvail as a result. The lease
+        // won't be inserted into the database.
+        if (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_SKIP) {
+            LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_HOOKS, DHCPSRV_HOOK_LEASE6_RECOVER_SKIP)
+                .arg(lease->addr_.toText());
+            return (false);
+        }
     }
 
     LOG_INFO(alloc_engine_logger, ALLOC_ENGINE_V6_DECLINED_RECOVERED)
@@ -1853,7 +1913,7 @@ AllocEngine::reclaimDeclined(const Lease6Ptr& lease) {
     // Note that we do not touch assigned-addresses counters. Those are
     // modified in whatever code calls this method.
 
-    /// @todo: call lease6_decline_recycle hook here.
+    return (true);
 }
 
 

src/lib/dhcpsrv/alloc_engine.h

@@ -881,20 +881,24 @@ private:
     /// These are the additional steps required when recoving a declined lease:
     /// - bump decline recovered stat
     /// - log lease recovery
-    /// - call hook (@todo)
+    /// - call lease4_recover hook
     ///
     /// @param lease Lease to be reclaimed from Declined state
-    void reclaimDeclined(const Lease4Ptr& lease);
+    /// @return true if it's ok to remove the lease (false = hooks status says
+    ///         to keep it)
+    bool reclaimDeclined(const Lease4Ptr& lease);
 
     /// @brief Conducts steps necessary for reclaiming declined IPv6 lease.
     ///
     /// These are the additional steps required when recoving a declined lease:
     /// - bump decline recovered stat
     /// - log lease recovery
-    /// - call hook (@todo)
+    /// - call lease6_recover hook
     ///
     /// @param lease Lease to be reclaimed from Declined state
-    void reclaimDeclined(const Lease6Ptr& lease);
+    /// @return true if it's ok to remove the lease (false = hooks status says
+    ///         to keep it)
+    bool reclaimDeclined(const Lease6Ptr& lease);
 
 public:
 

src/lib/dhcpsrv/dhcpsrv_messages.mes

@@ -189,6 +189,14 @@ have been experienced.  Any such errors should have preceding entries in the
 log with details.  No further attempts to communicate with kea-dhcp-ddns will
 be made without intervention.
 
+% DHCPSRV_HOOK_LEASE4_RECOVER_SKIP DHCPv4 lease %1 was not recoverd from the declined state because a callout set the skip status.
+This debug message is printed when a callout installed on lease4_recover
+hook point set the next step status to SKIP. For this particular hook point, this
+indicates that the server should not recover the lease from declined state.
+The server will leave the lease as it is, in the declined state. The
+server will attempt to recover it the next time decline recovery procedure
+takes place.
+
 % DHCPSRV_HOOK_LEASE4_RENEW_SKIP DHCPv4 lease was not renewed because a callout set the skip flag.
 This debug message is printed when a callout installed on lease4_renew
 hook point set the skip flag. For this particular hook point, the setting
@@ -209,6 +217,14 @@ extend the lifetime for a lease. If the client requested renewal of multiple
 leases (by sending multiple IA options), the server will skip the renewal
 of the one in question and will proceed with other renewals as usual.
 
+% DHCPSRV_HOOK_LEASE6_RECOVER_SKIP DHCPv6 lease %1 was not recovered from declined state because a callout set the skip status.
+This debug message is printed when a callout installed on lease6_recover
+hook point set the next step status to SKIP. For this particular hook point, this
+indicates that the server should not recover the lease from declined state.
+The server will leave the lease as it is, in the declined state. The
+server will attempt to recover it the next time decline recovery procedure
+takes place.
+
 % DHCPSRV_HOOK_LEASE6_SELECT_SKIP Lease6 (non-temporary) creation was skipped, because of callout skip flag.
 This debug message is printed when a callout installed on lease6_select
 hook point sets the skip flag. It means that the server was told that

src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc

@@ -1104,6 +1104,8 @@ public:
     AllocEnginePtr engine_;
 };
 
+
+
 /// @brief Specialization of the @c ExpirationAllocEngineTest class to test
 /// reclamation of the IPv6 leases.
 class ExpirationAllocEngine6Test : public ExpirationAllocEngineTest<Lease6Ptr> {
@@ -1115,6 +1117,14 @@ public:
     /// stores them in the lease manager.
     ExpirationAllocEngine6Test();
 
+    /// @brief Virtual destructor.
+    ///
+    /// Clears up static fields that may be modified by hooks.
+    virtual ~ExpirationAllocEngine6Test() {
+        callout_lease_.reset();
+        callout_name_ = string("");
+    }
+
     /// @brief Creates collection of leases for a test.
     ///
     /// It is called internally at the construction time.
@@ -1195,12 +1205,62 @@ public:
     /// it the second time.
     void testReclaimReusedLeases(const uint16_t msg_type, const bool use_reclaimed);
 
+    /// @brief Callout for lease6_recover
+    ///
+    /// This callout stores passed parameter into static fields.
+    ///
+    /// @param callout_handle will be provided by hooks framework
+    /// @return always 0
+    static int lease6RecoverCallout(CalloutHandle& callout_handle) {
+        callout_name_ = "lease6_recover";
+
+        callout_handle.getArgument("lease6", callout_lease_);
+
+        return (0);
+    }
+
+    /// @brief Callout for lease6_recover that sets status to SKIP
+    ///
+    /// This callout stores passed parameter into static fields.
+    ///
+    /// @param callout_handle will be provided by hooks framework
+    /// @return always 0
+    static int lease6RecoverSkipCallout(CalloutHandle& callout_handle) {
+        // Set the next step status to SKIP
+        callout_handle.setStatus(CalloutHandle::NEXT_STEP_SKIP);
+
+        return (lease6RecoverCallout(callout_handle));
+    }
+
+    /// @brief Test install a hook callout, recovers declined leases
+    ///
+    /// This test: declines, then expires half of the leases, then
+    /// installs a callout on lease6_recover hook, then reclaims
+    /// expired leases and checks that:
+    /// - the callout was indeed called
+    /// - the parameter (lease6) was indeed passed as expected
+    /// - checks that the leases are removed (skip=false) or
+    /// - checks that the leases are still there (skip=true)
+    /// @param skip should the callout set the next step status to skip?
+    void
+    testReclaimDeclinedHook(bool skip);
+
+    /// The following parameters will be written by a callout
+    static std::string callout_name_; ///< Stores callout name
+    static Lease6Ptr callout_lease_;  ///< Stores callout parameter
 };
 
+std::string ExpirationAllocEngine6Test::callout_name_;
+Lease6Ptr ExpirationAllocEngine6Test::callout_lease_;
+
 ExpirationAllocEngine6Test::ExpirationAllocEngine6Test()
     : ExpirationAllocEngineTest<Lease6Ptr>("type=memfile universe=6 persist=false") {
     createLeases();
     callout_argument_name = "lease6";
+
+    // Let's clear any garbage previous test may have left in static fields.
+    callout_name_ = string("");
+    callout_lease_.reset();
 }
 
 void
@@ -1397,6 +1457,44 @@ ExpirationAllocEngine6Test::testReclaimReusedLeases(const uint16_t msg_type,
 
 }
 
+void
+ExpirationAllocEngine6Test::testReclaimDeclinedHook(bool skip) {
+    for (unsigned int i = 0; i < TEST_LEASES_NUM; ++i) {
+
+        // Mark leases with even indexes as expired.
+        if (evenLeaseIndex(i)) {
+
+            // Mark lease as declined with 100 seconds of probation-period
+            // (i.e. lease is supposed to be off limits for 100 seconds)
+            decline(i, 100);
+
+            // The higher the index, the more expired the lease.
+            expire(i, 10 + i);
+        }
+    }
+
+    EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
+                        "lease6_recover",
+                        skip ? lease6RecoverSkipCallout : lease6RecoverCallout));
+
+    // Run leases reclamation routine on all leases.
+    ASSERT_NO_THROW(reclaimExpiredLeases(0, 0, true));
+
+    // Make sure that the callout really was called. It was supposed to modify
+    // the callout_name_ and store the lease in callout_lease_
+    EXPECT_EQ("lease6_recover", callout_name_);
+    EXPECT_TRUE(callout_lease_);
+
+    // Leases with even indexes should not exist in the DB
+    if (skip) {
+        // Skip status should have prevented removing the lease.
+        EXPECT_TRUE(testLeases(&leaseExists, &evenLeaseIndex));
+    } else {
+        // The hook hasn't modified next step status. The lease should be gone.
+        EXPECT_TRUE(testLeases(&leaseDoesntExist, &evenLeaseIndex));
+    }
+};
+
 // This test verifies that the leases can be reclaimed without being removed
 // from the database. In such case, the leases' state is set to
 // "expired-reclaimed".
@@ -1534,6 +1632,18 @@ TEST_F(ExpirationAllocEngine6Test, reclaimReusedLeasesSolicit) {
 // again when the Solicit message is being processed.
 TEST_F(ExpirationAllocEngine6Test, reclaimReusedLeasesSolicitAlreadyReclaimed) {
     testReclaimReusedLeases(DHCPV6_SOLICIT, true);
+
+// This test verifies if the hooks installed on lease6_recover are called
+// when the lease expires.
+TEST_F(ExpirationAllocEngine6Test, reclaimDeclinedHook1) {
+    testReclaimDeclinedHook(false); // false = don't use skip callout
+}
+
+// This test verifies if the hooks installed on lease6_recover are called
+// when the lease expires and that the next step status set to SKIP
+// causes the recovery to not be conducted.
+TEST_F(ExpirationAllocEngine6Test, reclaimDeclinedHook2) {
+    testReclaimDeclinedHook(true); // true = use skip callout
 }
 
 // *******************************************************
@@ -1553,6 +1663,14 @@ public:
     /// stores them in the lease manager.
     ExpirationAllocEngine4Test();
 
+    /// @brief Virtual destructor.
+    ///
+    /// Clears up static fields that may be modified by hooks.
+    virtual ~ExpirationAllocEngine4Test() {
+        callout_lease_.reset();
+        callout_name_ = string("");
+    }
+
     /// @brief Creates collection of leases for a test.
     ///
     /// It is called internally at the construction time.
@@ -1646,12 +1764,62 @@ public:
     void testReclaimReusedLeases(const uint8_t msg_type, const bool client_renews,
                                  const bool use_reclaimed);
 
+    /// @brief Callout for lease4_recover
+    ///
+    /// This callout stores passed parameter into static fields.
+    ///
+    /// @param callout_handle will be provided by hooks framework
+    /// @return always 0
+    static int lease4RecoverCallout(CalloutHandle& callout_handle) {
+        callout_name_ = "lease4_recover";
+
+        callout_handle.getArgument("lease4", callout_lease_);
+
+        return (0);
+    }
+
+    /// @brief Callout for lease4_recover that sets status to SKIP
+    ///
+    /// This callout stores passed parameter into static fields.
+    ///
+    /// @param callout_handle will be provided by hooks framework
+    /// @return always 0
+    static int lease4RecoverSkipCallout(CalloutHandle& callout_handle) {
+        // Set the next step status to SKIP
+        callout_handle.setStatus(CalloutHandle::NEXT_STEP_SKIP);
+
+        return (lease4RecoverCallout(callout_handle));
+    }
+
+    /// @brief Test install a hook callout, recovers declined leases
+    ///
+    /// This test: declines, then expires half of the leases, then
+    /// installs a callout on lease4_recover hook, then reclaims
+    /// expired leases and checks that:
+    /// - the callout was indeed called
+    /// - the parameter (lease4) was indeed passed as expected
+    /// - checks that the leases are removed (skip=false) or
+    /// - checks that the leases are still there (skip=true)
+    /// @param skip should the callout set the next step status to skip?
+    void
+    testReclaimDeclinedHook(bool skip);
+
+    /// The following parameters will be written by a callout
+    static std::string callout_name_; ///< Stores callout name
+    static Lease4Ptr callout_lease_;  ///< Stores callout parameter
 };
 
+std::string ExpirationAllocEngine4Test::callout_name_;
+Lease4Ptr ExpirationAllocEngine4Test::callout_lease_;
+
 ExpirationAllocEngine4Test::ExpirationAllocEngine4Test()
     : ExpirationAllocEngineTest<Lease4Ptr>("type=memfile universe=4 persist=false") {
     createLeases();
     callout_argument_name = "lease4";
+
+    // Let's clear any garbage previous test may have left in static fields.
+    callout_name_ = string("");
+    callout_lease_.reset();
 }
 
 void
@@ -1945,6 +2113,43 @@ ExpirationAllocEngine4Test::testReclaimReusedLeases(const uint8_t msg_type,
     }
 }
 
+void
+ExpirationAllocEngine4Test::testReclaimDeclinedHook(bool skip) {
+    for (unsigned int i = 0; i < TEST_LEASES_NUM; ++i) {
+
+        // Mark leases with even indexes as expired.
+        if (evenLeaseIndex(i)) {
+
+            // Mark lease as declined with 100 seconds of probation-period
+            // (i.e. lease is supposed to be off limits for 100 seconds)
+            decline(i, 100);
+
+            // The higher the index, the more expired the lease.
+            expire(i, 10 + i);
+        }
+    }
+
+    EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
+                        "lease4_recover",
+                        skip ? lease4RecoverSkipCallout : lease4RecoverCallout));
+
+    // Run leases reclamation routine on all leases.
+    ASSERT_NO_THROW(reclaimExpiredLeases(0, 0, true));
+
+    // Make sure that the callout really was called. It was supposed to modify
+    // the callout_name_ and store the lease in callout_lease_
+    EXPECT_EQ("lease4_recover", callout_name_);
+    EXPECT_TRUE(callout_lease_);
+
+    // Leases with even indexes should not exist in the DB
+    if (skip) {
+        // Skip status should have prevented removing the lease.
+        EXPECT_TRUE(testLeases(&leaseExists, &evenLeaseIndex));
+    } else {
+        // The hook hasn't modified next step status. The lease should be gone.
+        EXPECT_TRUE(testLeases(&leaseDoesntExist, &evenLeaseIndex));
+    }
+};
 
 // This test verifies that the leases can be reclaimed without being removed
 // from the database. In such case, the leases' state is set to
@@ -2102,5 +2307,17 @@ TEST_F(ExpirationAllocEngine4Test, reclaimRenewedLeasesDiscoverAlreadyReclaimed)
     testReclaimReusedLeases(DHCPDISCOVER, false, true);
 }
 
+// This test verifies if the hooks installed on lease4_recover are called
+// when the lease expires.
+TEST_F(ExpirationAllocEngine4Test, reclaimDeclinedHook1) {
+    testReclaimDeclinedHook(false); // false = don't use skip callout
+}
+
+// This test verifies if the hooks installed on lease4_recover are called
+// when the lease expires and that the next step status set to SKIP
+// causes the recovery to not be conducted.
+TEST_F(ExpirationAllocEngine4Test, reclaimDeclinedHook2) {
+    testReclaimDeclinedHook(true); // true = use skip callout
+}
 
 }; // end of anonymous namespace

src/lib/hooks/callout_handle.h

@@ -234,7 +234,7 @@ public:
     /// This variable is interrogated by the server to see if the remaining
     /// callouts associated with the current hook should be bypassed.
     ///
-    /// @param skip New value of the "skip" flag.
+    /// @param next New value of the next step status.
     void setStatus(const CalloutNextStep next) {
         next_step_ = next;
     }