[master] Merge branch 'trac3986' (lease4_decline hook)

src/bin/dhcp4/dhcp4_hooks.dox

@@ -177,6 +177,29 @@ packet processing. Hook points that are not specific to packet processing
    It will be kept in the database and will go through the regular expiration/reuse
    process.
 
+@subsection dhcpv4HooksLeaseDecline lease4_decline
+
+ - @b Arguments:
+   - name: @b query4, type: isc::dhcp::Pkt4Ptr, direction: <b>in</b>
+   - name: @b lease4, type: isc::dhcp::Lease4Ptr, direction: <b>in</b>
+
+ - @b Description: this callout is executed when the server engine
+   is about to decline a lease, as a result of receiving DHCPDECLINE packet.
+   The server already sanity checked it (the packet is sane, attempts to decline
+   a lease that is valid and belongs to the client that requests its decline).
+   The "lease4" argument points to @c Lease4 object that contains the lease to
+   be released. Note this lease still contains client identifying information.
+   That data is provided for informational purposes and it doesn't make sense to
+   modify it at this time. All the information will be removed from the lease
+   before it is updated in the database.
+
+ - <b>Next step status</b>: If any callout installed on the "lease4_release" hook
+   sets the next step action to DROP, the server will not decline the lease.
+   Care should be taken when setting this status.  The lease will be kept in
+   the database as it is and the client will incorrectly assume that the server
+   marked this lease as unavailable. If the client restarts its configuration,
+   it will get the same (not declined) lease as a result.
+
 @subsection dhcpv4HooksPkt4Send pkt4_send
 
  - @b Arguments:

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");
     }
 };
 
@@ -1918,11 +1920,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()) {
@@ -1961,7 +1989,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/Makefile.am

@@ -85,6 +85,7 @@ dhcp4_unittests_SOURCES += config_parser_unittest.cc
 dhcp4_unittests_SOURCES += fqdn_unittest.cc
 dhcp4_unittests_SOURCES += marker_file.cc
 dhcp4_unittests_SOURCES += dhcp4_client.cc dhcp4_client.h
+dhcp4_unittests_SOURCES += hooks_unittest.cc
 dhcp4_unittests_SOURCES += inform_unittest.cc
 dhcp4_unittests_SOURCES += dora_unittest.cc
 dhcp4_unittests_SOURCES += release_unittest.cc

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();