[3986] Decline methods moved to base class, Decline hooks test implemented.

src/bin/dhcp4/dhcp4_messages.mes

@@ -247,6 +247,13 @@ setting of the flag by a callout instructs the server to drop the packet.
 Server completed all the processing (e.g. may have assigned, updated
 or released leases), but the response will not be send to the client.
 
+% DHCP4_HOOK_DECLINE_SKIP Decline4 hook callouts set status to DROP, ignoring packet.
+This message indicates that the server received DHCPDECLINE message, it was verified
+to be correct and matching server's lease information. The server called hooks
+for decline4 hook point and one of the callouts set next step status to DROP.
+The server will now abort processing of the packet as if it was never
+received. The lease will continue to be assigned to this client.
+
 % DHCP4_HOOK_LEASE4_RELEASE_SKIP %1: lease was not released because a callout set the skip flag.
 This debug message is printed when a callout installed on lease4_release
 hook point set the skip flag. For this particular hook point, the

src/bin/dhcp4/dhcp4_srv.cc

@@ -80,6 +80,7 @@ struct Dhcp4Hooks {
     int hook_index_lease4_release_; ///< index for "lease4_release" hook point
     int hook_index_pkt4_send_;      ///< index for "pkt4_send" hook point
     int hook_index_buffer4_send_;   ///< index for "buffer4_send" hook point
+    int hook_index_lease4_decline_; ///< index for "lease4_decline" hook point
 
     /// Constructor that registers hook points for DHCPv4 engine
     Dhcp4Hooks() {
@@ -89,6 +90,7 @@ struct Dhcp4Hooks {
         hook_index_pkt4_send_      = HooksManager::registerHook("pkt4_send");
         hook_index_lease4_release_ = HooksManager::registerHook("lease4_release");
         hook_index_buffer4_send_   = HooksManager::registerHook("buffer4_send");
+        hook_index_lease4_decline_ = HooksManager::registerHook("lease4_decline");
     }
 };
 
@@ -1895,11 +1897,37 @@ Dhcpv4Srv::processDecline(Pkt4Ptr& decline) {
 
     // Ok, all is good. The client is reporting its own address. Let's
     // process it.
-    declineLease(lease, decline->getLabel());
+    declineLease(lease, decline);
 }
 
 void
-Dhcpv4Srv::declineLease(const Lease4Ptr& lease, const std::string& descr) {
+Dhcpv4Srv::declineLease(const Lease4Ptr& lease, const Pkt4Ptr& decline) {
+
+    // Let's check if there are hooks installed for decline4 hook point.
+    // If they are, let's pass the lease and client's packet. If the hook
+    // sets status to drop, we reject this Decline.
+    if (HooksManager::calloutsPresent(Hooks.hook_index_lease4_decline_)) {
+        CalloutHandlePtr callout_handle = getCalloutHandle(decline);
+
+        // Delete previously set arguments
+        callout_handle->deleteAllArguments();
+
+        // Pass incoming Decline and the lease to be declined.
+        callout_handle->setArgument("lease4", lease);
+        callout_handle->setArgument("query4", decline);
+
+        // Call callouts
+        HooksManager::callCallouts(Hooks.hook_index_lease4_decline_,
+                                   *callout_handle);
+
+        // Check if callouts decided to drop the packet. If any of them did,
+        // we will drop the packet.
+        if (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_DROP) {
+            LOG_DEBUG(hooks_logger, DBG_DHCP4_HOOKS, DHCP4_HOOK_DECLINE_SKIP)
+                .arg(decline->getLabel()).arg(lease->addr_.toText());
+            return;
+        }
+    }
 
     // Clean up DDNS, if needed.
     if (CfgMgr::instance().ddnsEnabled()) {
@@ -1938,7 +1966,7 @@ Dhcpv4Srv::declineLease(const Lease4Ptr& lease, const std::string& descr) {
     LeaseMgrFactory::instance().updateLease4(lease);
 
     LOG_INFO(dhcp4_logger, DHCP4_DECLINE_LEASE).arg(lease->addr_.toText())
-        .arg(descr).arg(lease->valid_lft_);
+        .arg(decline->getLabel()).arg(lease->valid_lft_);
 }
 
 Pkt4Ptr

src/bin/dhcp4/dhcp4_srv.h

@@ -546,12 +546,12 @@ private:
     /// - disassociate the client information
     /// - update lease in the database (switch to DECLINED state)
     /// - increase necessary statistics
-    /// - call appropriate hook (@todo)
+    /// - call lease4_decline hook
     ///
     /// @param lease lease to be declined
-    /// @param descr textual description of the client (will be used for logging)
+    /// @param decline client's message
     void
-    declineLease(const Lease4Ptr& lease, const std::string& descr);
+    declineLease(const Lease4Ptr& lease, const Pkt4Ptr& decline);
 
 protected:
 

src/bin/dhcp4/tests/decline_unittest.cc

@@ -34,7 +34,6 @@ using namespace isc::dhcp::test;
 using namespace isc::stats;
 
 namespace {
-
 /// @brief Set of JSON configurations used throughout the Decline tests.
 ///
 /// - Configuration 0:
@@ -63,56 +62,14 @@ const char* DECLINE_CONFIGS[] = {
     "}"
 };
 
-/// @brief Test fixture class for testing DHCPDECLINE message handling.
-///
-/// @todo This class is very similar to ReleaseTest. Maybe we could
-/// merge those two classes one day and use derived classes?
-class DeclineTest : public Dhcpv4SrvTest {
-public:
-
-    enum ExpectedResult {
-        SHOULD_PASS, // pass = accept decline, move lease to declined state.
-        SHOULD_FAIL  // fail = reject the decline
-    };
-
-    /// @brief Constructor.
-    ///
-    /// Sets up fake interfaces.
-    DeclineTest()
-        : Dhcpv4SrvTest(),
-          iface_mgr_test_config_(true) {
-        IfaceMgr::instance().openSockets4();
-    }
-
-    /// @brief Performs 4-way exchange to obtain new lease.
-    ///
-    /// This is used as a preparatory step for Decline operation.
-    ///
-    /// @param client Client to be used to obtain a lease.
-    void acquireLease(Dhcp4Client& client);
-
-    /// @brief Tests if the acquired lease is or is not declined.
-    ///
-    /// @param hw_address_1 HW Address to be used to acquire the lease.
-    /// @param client_id_1 Client id to be used to acquire the lease.
-    /// @param hw_address_2 HW Address to be used to decline the lease.
-    /// @param client_id_2 Client id to be used to decline the lease.
-    /// @param expected_result SHOULD_PASS if the lease is expected to
-    /// be successfully declined, or SHOULD_FAIL if the lease is expected
-    /// to not be declined.
-    void acquireAndDecline(const std::string& hw_address_1,
-                           const std::string& client_id_1,
-                           const std::string& hw_address_2,
-                           const std::string& client_id_2,
-                           ExpectedResult expected_result);
-
-    /// @brief Interface Manager's fake configuration control.
-    IfaceMgrTestConfig iface_mgr_test_config_;
-
 };
 
+namespace isc {
+namespace dhcp {
+namespace test {
+
 void
-DeclineTest::acquireLease(Dhcp4Client& client) {
+Dhcpv4SrvTest::acquireLease(Dhcp4Client& client) {
     // Perform 4-way exchange with the server but to not request any
     // specific address in the DHCPDISCOVER message.
     ASSERT_NO_THROW(client.doDORA());
@@ -132,11 +89,11 @@ DeclineTest::acquireLease(Dhcp4Client& client) {
 }
 
 void
-DeclineTest::acquireAndDecline(const std::string& hw_address_1,
-                               const std::string& client_id_1,
-                               const std::string& hw_address_2,
-                               const std::string& client_id_2,
-                               ExpectedResult expected_result) {
+Dhcpv4SrvTest::acquireAndDecline(const std::string& hw_address_1,
+                                 const std::string& client_id_1,
+                                 const std::string& hw_address_2,
+                                 const std::string& client_id_2,
+                                 ExpectedResult expected_result) {
 
     // Set this global statistic explicitly to zero.
     isc::stats::StatsMgr::instance().setValue("declined-addresses",
@@ -218,6 +175,33 @@ DeclineTest::acquireAndDecline(const std::string& hw_address_1,
     }
 }
 
+}; // end of isc::dhcp::test namespace
+}; // end of isc::dhcp namespace
+}; // end of isc namespace
+
+namespace {
+
+/// @brief Test fixture class for testing DHCPDECLINE message handling.
+///
+/// @todo This class is very similar to ReleaseTest. Maybe we could
+/// merge those two classes one day and use derived classes?
+class DeclineTest : public Dhcpv4SrvTest {
+public:
+
+    /// @brief Constructor.
+    ///
+    /// Sets up fake interfaces.
+    DeclineTest()
+        : Dhcpv4SrvTest(),
+          iface_mgr_test_config_(true) {
+        IfaceMgr::instance().openSockets4();
+    }
+
+    /// @brief Interface Manager's fake configuration control.
+    IfaceMgrTestConfig iface_mgr_test_config_;
+
+};
+
 // This test checks that the client can acquire and decline the lease.
 TEST_F(DeclineTest, declineNoIdentifierChange) {
     acquireAndDecline("01:02:03:04:05:06", "12:14",
@@ -310,4 +294,4 @@ TEST_F(DeclineTest, declineNonMatchingIPAddress) {
     EXPECT_EQ(Lease::STATE_DEFAULT, lease->state_);
 }
 
-} // end of anonymous namespace
+}; // end of anonymous namespace

src/bin/dhcp4/tests/dhcp4_test_utils.h

@@ -88,6 +88,11 @@ public:
 
 typedef boost::shared_ptr<PktFilterTest> PktFilterTestPtr;
 
+/// Forward definition for Dhcp4Client defined in dhcp4_client.h
+/// dhcp4_client.h includes dhcp_test_utils.h (this file), so to avoid
+/// circular dependencies, we need a forward class declaration.
+class Dhcp4Client;
+
 /// @brief "Naked" DHCPv4 server, exposes internal fields
 class NakedDhcpv4Srv: public Dhcpv4Srv {
 public:
@@ -211,6 +216,11 @@ public:
 class Dhcpv4SrvTest : public ::testing::Test {
 public:
 
+    enum ExpectedResult {
+        SHOULD_PASS, // pass = accept decline, move lease to declined state.
+        SHOULD_FAIL  // fail = reject the decline
+    };
+
     /// @brief Constructor
     ///
     /// Initializes common objects used in many tests.
@@ -408,6 +418,28 @@ public:
     /// @brief Create @c Dhcpv4Exchange from client's query.
     Dhcpv4Exchange createExchange(const Pkt4Ptr& query);
 
+    /// @brief Performs 4-way exchange to obtain new lease.
+    ///
+    /// This is used as a preparatory step for Decline operation.
+    ///
+    /// @param client Client to be used to obtain a lease.
+    void acquireLease(Dhcp4Client& client);
+
+    /// @brief Tests if the acquired lease is or is not declined.
+    ///
+    /// @param hw_address_1 HW Address to be used to acquire the lease.
+    /// @param client_id_1 Client id to be used to acquire the lease.
+    /// @param hw_address_2 HW Address to be used to decline the lease.
+    /// @param client_id_2 Client id to be used to decline the lease.
+    /// @param expected_result SHOULD_PASS if the lease is expected to
+    /// be successfully declined, or SHOULD_FAIL if the lease is expected
+    /// to not be declined.
+    void acquireAndDecline(const std::string& hw_address_1,
+                           const std::string& client_id_1,
+                           const std::string& hw_address_2,
+                           const std::string& client_id_2,
+                           ExpectedResult expected_result);
+
     /// @brief This function cleans up after the test.
     virtual void TearDown();
 

src/bin/dhcp4/tests/hooks_unittest.cc

@@ -113,6 +113,7 @@ public:
         HooksManager::preCalloutsLibraryHandle().deregisterAllCallouts("subnet4_select");
         HooksManager::preCalloutsLibraryHandle().deregisterAllCallouts("lease4_renew");
         HooksManager::preCalloutsLibraryHandle().deregisterAllCallouts("lease4_release");
+        HooksManager::preCalloutsLibraryHandle().deregisterAllCallouts("lease4_decline");
 
         delete srv_;
     }
@@ -474,6 +475,17 @@ public:
         return (0);
     }
 
+    /// Test lease4_decline callback that stores received parameters.
+    /// @param callout_handle handle passed by the hooks framework
+    /// @return always 0
+    static int
+    lease4_decline_callout(CalloutHandle& callout_handle) {
+        callback_name_ = string("lease4_decline");
+        callout_handle.getArgument("query4", callback_pkt4_);
+        callout_handle.getArgument("lease4", callback_lease4_);
+
+        return (0);
+    }
 
     /// resets buffers used to store data received by callouts
     void resetCalloutBuffers() {
@@ -1453,10 +1465,43 @@ TEST_F(HooksDhcpv4SrvTest, lease4ReleaseSkip) {
     //EXPECT_EQ(leases.size(), 1);
 }
 
+// Checks that decline4 hooks are triggered properly.
+TEST_F(HooksDhcpv4SrvTest, HooksDecline) {
+    IfaceMgrTestConfig test_config(true);
+    IfaceMgr::instance().openSockets4();
 
-
-// Checks if hooks are registered properly.
-TEST_F(Dhcpv4SrvTest, HooksDecline) {
-
+    // Install a callout
+    EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
+                        "lease4_decline", lease4_decline_callout));
+
+    acquireAndDecline("01:02:03:04:05:06", "12:14",
+                      "01:02:03:04:05:06", "12:14",
+                      SHOULD_PASS);
+
+    EXPECT_EQ("lease4_decline", callback_name_);
+
+    // Verifying DHCPDECLINE is a bit tricky, as it is created somewhere in
+    // acquireAndDecline. We'll just verify that it's really a DECLINE
+    // and that its address is equal to what we have in LeaseMgr.
+    ASSERT_TRUE(callback_pkt4_);
+    ASSERT_TRUE(callback_lease4_);
+
+    // Check that it's the proper packet that was reported.
+    EXPECT_EQ(DHCPDECLINE, callback_pkt4_->getType());
+
+    // Extract the address being declined.
+    OptionCustomPtr opt_declined_addr = boost::dynamic_pointer_cast<
+        OptionCustom>(callback_pkt4_->getOption(DHO_DHCP_REQUESTED_ADDRESS));
+    ASSERT_TRUE(opt_declined_addr);
+    IOAddress addr(opt_declined_addr->readAddress());
+
+    // And try to get a matching lease from the lease mgr
+    Lease4Ptr from_mgr = LeaseMgrFactory::instance().getLease4(addr);
+    ASSERT_TRUE(from_mgr);
+    EXPECT_EQ(Lease::STATE_DECLINED, from_mgr->state_);
+
+    // Let's now check that those 3 things (packet, lease returned and lease from
+    // the lease manager) all match.
+    EXPECT_EQ(addr, from_mgr->addr_);
+    EXPECT_EQ(addr, callback_lease4_->addr_);
 }
-