From d55e6585ab6f7263754ef9fd499136df2848118b Mon Sep 17 00:00:00 2001 From: Tomek Mrugalski Date: Tue, 6 Oct 2015 23:51:33 +0200 Subject: [PATCH] [3986] Decline methods moved to base class, Decline hooks test implemented. --- src/bin/dhcp4/dhcp4_messages.mes | 7 ++ src/bin/dhcp4/dhcp4_srv.cc | 34 ++++++++- src/bin/dhcp4/dhcp4_srv.h | 6 +- src/bin/dhcp4/tests/decline_unittest.cc | 92 ++++++++++--------------- src/bin/dhcp4/tests/dhcp4_test_utils.h | 32 +++++++++ src/bin/dhcp4/tests/hooks_unittest.cc | 55 +++++++++++++-- 6 files changed, 161 insertions(+), 65 deletions(-) diff --git a/src/bin/dhcp4/dhcp4_messages.mes b/src/bin/dhcp4/dhcp4_messages.mes index ad1036dc3..a734bf545 100644 --- a/src/bin/dhcp4/dhcp4_messages.mes +++ b/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 diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index de3d8d967..cbe7ff3f5 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/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 diff --git a/src/bin/dhcp4/dhcp4_srv.h b/src/bin/dhcp4/dhcp4_srv.h index 142ee4452..fcc5faaf2 100644 --- a/src/bin/dhcp4/dhcp4_srv.h +++ b/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: diff --git a/src/bin/dhcp4/tests/decline_unittest.cc b/src/bin/dhcp4/tests/decline_unittest.cc index db1f72554..15194aec5 100644 --- a/src/bin/dhcp4/tests/decline_unittest.cc +++ b/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 diff --git a/src/bin/dhcp4/tests/dhcp4_test_utils.h b/src/bin/dhcp4/tests/dhcp4_test_utils.h index f8fbcbc8d..13008b9f3 100644 --- a/src/bin/dhcp4/tests/dhcp4_test_utils.h +++ b/src/bin/dhcp4/tests/dhcp4_test_utils.h @@ -88,6 +88,11 @@ public: typedef boost::shared_ptr 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(); diff --git a/src/bin/dhcp4/tests/hooks_unittest.cc b/src/bin/dhcp4/tests/hooks_unittest.cc index c55f75338..9622d4ea1 100644 --- a/src/bin/dhcp4/tests/hooks_unittest.cc +++ b/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_); } - -- GitLab