From: Marcin Siodelski Date: Mon, 22 Jan 2018 16:50:05 +0000 (+0100) Subject: [5457] Packet can be parked after leases4_committed callout. X-Git-Tag: ha_checkpoints12~3^2~16 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=0ff431f984e97323315a48627ab1b1a6de6ab304;p=thirdparty%2Fkea.git [5457] Packet can be parked after leases4_committed callout. --- diff --git a/src/bin/dhcp4/dhcp4_messages.mes b/src/bin/dhcp4/dhcp4_messages.mes index 6557c97098..127aecb0da 100644 --- a/src/bin/dhcp4/dhcp4_messages.mes +++ b/src/bin/dhcp4/dhcp4_messages.mes @@ -272,6 +272,10 @@ This debug message is printed when a callout installed on lease4_release hook point set the next step status to SKIP. For this particular hook point, the value set by a callout instructs the server to not release a lease. +% DHCP4_HOOK_LEASES4_COMMITTED_DROP %1: packet is dropped, because a callout set the next step tp DROP +This debug message is printed when a callout installed on the leases4_committed +hook point sets the next step to DROP. + % DHCP4_HOOK_PACKET_RCVD_SKIP %1: packet is dropped, because a callout set the next step to SKIP This debug message is printed when a callout installed on the pkt4_receive hook point sets the next step to SKIP. For this particular hook point, the diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index f40fe7536b..e06bf8d37e 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -835,72 +835,12 @@ Dhcpv4Srv::run_one() { return; } - try { - // Now all fields and options are constructed into output wire buffer. - // Option objects modification does not make sense anymore. Hooks - // can only manipulate wire buffer at this stage. - // Let's execute all callouts registered for buffer4_send - if (HooksManager::calloutsPresent(Hooks.hook_index_buffer4_send_)) { - CalloutHandlePtr callout_handle = getCalloutHandle(query); - - // Delete previously set arguments - callout_handle->deleteAllArguments(); - - // Enable copying options from the packet within hook library. - ScopedEnableOptionsCopy resp4_options_copy(rsp); - - // Pass incoming packet as argument - callout_handle->setArgument("response4", rsp); - - // Call callouts - HooksManager::callCallouts(Hooks.hook_index_buffer4_send_, - *callout_handle); - - // Callouts decided to skip the next processing step. The next - // processing step would to parse the packet, so skip at this - // stage means drop. - if ((callout_handle->getStatus() == CalloutHandle::NEXT_STEP_SKIP) || - (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_DROP)) { - LOG_DEBUG(hooks_logger, DBG_DHCP4_HOOKS, - DHCP4_HOOK_BUFFER_SEND_SKIP) - .arg(rsp->getLabel()); - return; - } - - callout_handle->getArgument("response4", rsp); - } - - LOG_DEBUG(packet4_logger, DBG_DHCP4_BASIC, DHCP4_PACKET_SEND) - .arg(rsp->getLabel()) - .arg(rsp->getName()) - .arg(static_cast(rsp->getType())) - .arg(rsp->getLocalAddr().isV4Zero() ? "*" : rsp->getLocalAddr().toText()) - .arg(rsp->getLocalPort()) - .arg(rsp->getRemoteAddr()) - .arg(rsp->getRemotePort()) - .arg(rsp->getIface().empty() ? "to be determined from routing" : - rsp->getIface()); - - LOG_DEBUG(packet4_logger, DBG_DHCP4_DETAIL_DATA, - DHCP4_RESPONSE_DATA) - .arg(rsp->getLabel()) - .arg(rsp->getName()) - .arg(static_cast(rsp->getType())) - .arg(rsp->toText()); - sendPacket(rsp); - - // Update statistics accordingly for sent packet. - processStatsSent(rsp); - - } catch (const std::exception& e) { - LOG_ERROR(packet4_logger, DHCP4_PACKET_SEND_FAIL) - .arg(rsp->getLabel()) - .arg(e.what()); - } + CalloutHandlePtr callout_handle = getCalloutHandle(query); + processPacketBufferSend(callout_handle, rsp); } void -Dhcpv4Srv::processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp) { +Dhcpv4Srv::processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp, bool allow_packet_park) { // Log reception of the packet. We need to increase it early, as any // failures in unpacking will cause the packet to be dropped. We // will increase type specific statistic further down the road. @@ -1129,25 +1069,21 @@ Dhcpv4Srv::processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp) { } } callout_handle->setArgument("deleted_leases4", deleted_leases); - callout_handle->setArgument("io_service", getIOService()); // Call all installed callouts HooksManager::callCallouts(Hooks.hook_index_leases4_committed_, *callout_handle); + if (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_DROP) { + LOG_DEBUG(hooks_logger, DBG_DHCP4_HOOKS, + DHCP4_HOOK_LEASES4_COMMITTED_DROP) + .arg(query->getLabel()); + rsp.reset(); - switch (callout_handle->getStatus()) { - // If next step is set to "skip" we simply terminate here, because the - // next step would be to send the packet. - case CalloutHandle::NEXT_STEP_SKIP: - return; - - case CalloutHandle::NEXT_STEP_PARK: + } else if ((callout_handle->getStatus() == CalloutHandle::NEXT_STEP_PARK) + && allow_packet_park) { packet_park = true; - break; - default: - ; } } @@ -1155,22 +1091,34 @@ Dhcpv4Srv::processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp) { return; } + // PARKING SPOT after leases4_committed hook point. + CalloutHandlePtr callout_handle = getCalloutHandle(query); if (packet_park) { // Park the packet. The function we bind here will be executed when the hook // library unparks the packet. HooksManager::park("leases4_committed", query, - std::bind(&Dhcpv4Srv::leases4CommittedContinue, this, - getCalloutHandle(query), query, rsp)); + [this, callout_handle, query, rsp]() mutable { + processPacketPktSend(callout_handle, query, rsp); + processPacketBufferSend(callout_handle, rsp); + }); + + // If we have parked the packet, let's reset the pointer to the + // response to indicate to the caller that it should return, as + // the packet processing will continue via the callback. + rsp.reset(); } else { - // Packet is not to be parked, so let's continue processing. - leases4CommittedContinue(getCalloutHandle(query), query, rsp); + processPacketPktSend(callout_handle, query, rsp); } } void -Dhcpv4Srv::leases4CommittedContinue(const CalloutHandlePtr& callout_handle, - Pkt4Ptr& query, Pkt4Ptr& rsp) { +Dhcpv4Srv::processPacketPktSend(hooks::CalloutHandlePtr& callout_handle, + Pkt4Ptr& query, Pkt4Ptr& rsp) { + if (!rsp) { + return; + } + // Specifies if server should do the packing bool skip_pack = false; @@ -1222,6 +1170,76 @@ Dhcpv4Srv::leases4CommittedContinue(const CalloutHandlePtr& callout_handle, } } +void +Dhcpv4Srv::processPacketBufferSend(CalloutHandlePtr& callout_handle, + Pkt4Ptr& rsp) { + if (!rsp) { + return; + } + + try { + // Now all fields and options are constructed into output wire buffer. + // Option objects modification does not make sense anymore. Hooks + // can only manipulate wire buffer at this stage. + // Let's execute all callouts registered for buffer4_send + if (HooksManager::calloutsPresent(Hooks.hook_index_buffer4_send_)) { + + // Delete previously set arguments + callout_handle->deleteAllArguments(); + + // Enable copying options from the packet within hook library. + ScopedEnableOptionsCopy resp4_options_copy(rsp); + + // Pass incoming packet as argument + callout_handle->setArgument("response4", rsp); + + // Call callouts + HooksManager::callCallouts(Hooks.hook_index_buffer4_send_, + *callout_handle); + + // Callouts decided to skip the next processing step. The next + // processing step would to parse the packet, so skip at this + // stage means drop. + if ((callout_handle->getStatus() == CalloutHandle::NEXT_STEP_SKIP) || + (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_DROP)) { + LOG_DEBUG(hooks_logger, DBG_DHCP4_HOOKS, + DHCP4_HOOK_BUFFER_SEND_SKIP) + .arg(rsp->getLabel()); + return; + } + + callout_handle->getArgument("response4", rsp); + } + + LOG_DEBUG(packet4_logger, DBG_DHCP4_BASIC, DHCP4_PACKET_SEND) + .arg(rsp->getLabel()) + .arg(rsp->getName()) + .arg(static_cast(rsp->getType())) + .arg(rsp->getLocalAddr().isV4Zero() ? "*" : rsp->getLocalAddr().toText()) + .arg(rsp->getLocalPort()) + .arg(rsp->getRemoteAddr()) + .arg(rsp->getRemotePort()) + .arg(rsp->getIface().empty() ? "to be determined from routing" : + rsp->getIface()); + + LOG_DEBUG(packet4_logger, DBG_DHCP4_DETAIL_DATA, + DHCP4_RESPONSE_DATA) + .arg(rsp->getLabel()) + .arg(rsp->getName()) + .arg(static_cast(rsp->getType())) + .arg(rsp->toText()); + sendPacket(rsp); + + // Update statistics accordingly for sent packet. + processStatsSent(rsp); + + } catch (const std::exception& e) { + LOG_ERROR(packet4_logger, DHCP4_PACKET_SEND_FAIL) + .arg(rsp->getLabel()) + .arg(e.what()); + } +} + string Dhcpv4Srv::srvidToString(const OptionPtr& srvid) { if (!srvid) { diff --git a/src/bin/dhcp4/dhcp4_srv.h b/src/bin/dhcp4/dhcp4_srv.h index dd42547237..fbcda55016 100644 --- a/src/bin/dhcp4/dhcp4_srv.h +++ b/src/bin/dhcp4/dhcp4_srv.h @@ -26,6 +26,7 @@ #include +#include #include #include @@ -260,18 +261,11 @@ public: /// /// @param query A pointer to the packet to be processed. /// @param rsp A pointer to the response - void processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp); + /// @param allow_packet_park Indicates if parking a packet is allowed. + void processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp, + bool allow_packet_park = true); - /// @brief Continues DHCPv4 packet processing after leases4_committed hook - /// point. - /// - /// @param callout_handle Callout handle to be used. - /// @param query A pointer to the packet to be processed. - /// @param rsp A pointer to the response - void leases4CommittedContinue(const hooks::CalloutHandlePtr& callout_handle, - Pkt4Ptr& query, Pkt4Ptr& rsp); - /// @brief Instructs the server to shut down. void shutdown(); @@ -837,6 +831,21 @@ protected: /// @param query Pointer to the client message. void deferredUnpack(Pkt4Ptr& query); + /// @brief Executes pkt4_send callout. + /// + /// @param callout_handle pointer to the callout handle. + /// @param query Pointer to a query. + /// @param rsp Pointer to a response. + void processPacketPktSend(hooks::CalloutHandlePtr& callout_handle, + Pkt4Ptr& query, Pkt4Ptr& rsp); + + /// @brief Executes buffer4_send callout and sends the response. + /// + /// @param callout_handle pointer to the callout handle. + /// @param pkt Pointer to a response. + void processPacketBufferSend(hooks::CalloutHandlePtr& callout_handle, + Pkt4Ptr& rsp); + /// @brief Allocation Engine. /// Pointer to the allocation engine that we are currently using /// It must be a pointer, because we will support changing engines diff --git a/src/bin/dhcp4/dhcp4to6_ipc.cc b/src/bin/dhcp4/dhcp4to6_ipc.cc index e1222a1bb8..ce957ef062 100644 --- a/src/bin/dhcp4/dhcp4to6_ipc.cc +++ b/src/bin/dhcp4/dhcp4to6_ipc.cc @@ -103,7 +103,7 @@ void Dhcp4to6Ipc::handler() { // From Dhcpv4Srv::run_one() processing and after Pkt4Ptr rsp; - ControlledDhcpv4Srv::getInstance()->processPacket(query, rsp); + ControlledDhcpv4Srv::getInstance()->processPacket(query, rsp, false); if (!rsp) { return; diff --git a/src/bin/dhcp4/tests/dhcp4_client.cc b/src/bin/dhcp4/tests/dhcp4_client.cc index c117ba4320..0340a01cea 100644 --- a/src/bin/dhcp4/tests/dhcp4_client.cc +++ b/src/bin/dhcp4/tests/dhcp4_client.cc @@ -406,6 +406,15 @@ Dhcp4Client::doRequest() { } } +void +Dhcp4Client::receiveResponse() { + context_.response_ = receiveOneMsg(); + // If the server has responded, store the configuration received. + if (context_.response_) { + applyConfiguration(); + } +} + void Dhcp4Client::includeClientId(const std::string& clientid) { if (clientid.empty()) { @@ -519,7 +528,14 @@ Dhcp4Client::sendMsg(const Pkt4Ptr& msg) { msg_copy->setLocalAddr(dest_addr_); msg_copy->setIface(iface_name_); srv_->fakeReceive(msg_copy); - srv_->run(); + + try { + // Invoke run_one instead of run, because we want to avoid triggering + // IO service. + srv_->run_one(); + } catch (...) { + // Suppress errors, as the DHCPv4 server does. + } } void diff --git a/src/bin/dhcp4/tests/dhcp4_client.h b/src/bin/dhcp4/tests/dhcp4_client.h index a30b7d821f..b10e5535a0 100644 --- a/src/bin/dhcp4/tests/dhcp4_client.h +++ b/src/bin/dhcp4/tests/dhcp4_client.h @@ -206,6 +206,15 @@ public: /// DHCPACK message is applied and can be obtained from the @c config_. void doRequest(); + /// @brief Receives a response from the server. + /// + /// This method is useful to receive response from the server after + /// parking a packet. In this case, the packet is not received as a + /// result of initial exchange, e.g. @c doRequest. The test can call + /// this method to complete the transaction when it expects that the + /// packet has been unparked. + void receiveResponse(); + /// @brief Generates a hardware address used by the client. /// /// It assigns random values to the bytes of the hardware address. diff --git a/src/bin/dhcp4/tests/hooks_unittest.cc b/src/bin/dhcp4/tests/hooks_unittest.cc index b1813cdd63..a52e6cdaf8 100644 --- a/src/bin/dhcp4/tests/hooks_unittest.cc +++ b/src/bin/dhcp4/tests/hooks_unittest.cc @@ -658,8 +658,7 @@ public: static void leases4_committed_unpark(ParkingLotHandlePtr parking_lot, Pkt4Ptr query) { - std::cout << "unparking" << std::endl; - std::cout << parking_lot->unpark(query) << std::endl; + parking_lot->unpark(query); } /// Test callback which asks the server to park the packet. @@ -1926,19 +1925,26 @@ TEST_F(HooksDhcpv4SrvTest, leases4CommittedRequest) { EXPECT_FALSE(callback_deleted_lease4_); } -TEST_F(HooksDhcpv4SrvTest, leases4CommittedParkRequest) { +// This test verifies that it is possible to park a packet as a result of +// the leases4_committed callouts. +TEST_F(HooksDhcpv4SrvTest, leases4CommittedParkRequests) { IfaceMgrTestConfig test_config(true); IfaceMgr::instance().openSockets4(); + // This callout uses provided IO service object to post a function + // that unparks the packet. The packet is pared and can be unparked + // by simply calling IOService::poll. ASSERT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout( "leases4_committed", leases4_committed_park_callout)); - Dhcp4Client client(Dhcp4Client::SELECTING); - client.setIfaceName("eth1"); - ASSERT_NO_THROW(client.doDORA(boost::shared_ptr(new IOAddress("192.0.2.100")))); + // Create first client and perform DORA. + Dhcp4Client client1(Dhcp4Client::SELECTING); + client1.setIfaceName("eth1"); + ASSERT_NO_THROW(client1.doDORA(boost::shared_ptr(new IOAddress("192.0.2.100")))); - // Make sure that we received a response - ASSERT_FALSE(client.getContext().response_); + // We should be offerred an address but the DHCPACK should not arrive + // at this point, because the packet is parked. + ASSERT_FALSE(client1.getContext().response_); // Check that the callback called is indeed the one we installed EXPECT_EQ("leases4_committed", callback_name_); @@ -1953,7 +1959,7 @@ TEST_F(HooksDhcpv4SrvTest, leases4CommittedParkRequest) { sort(expected_argument_names.begin(), expected_argument_names.end()); EXPECT_TRUE(callback_argument_names_ == expected_argument_names); - // Newly allocated lease should be returned. + // Newly allocated lease should be passed to the callout. ASSERT_TRUE(callback_lease4_); EXPECT_EQ("192.0.2.100", callback_lease4_->addr_.toText()); @@ -1963,7 +1969,39 @@ TEST_F(HooksDhcpv4SrvTest, leases4CommittedParkRequest) { // Pkt passed to a callout must be configured to copy retrieved options. EXPECT_TRUE(callback_qry_options_copy_); -// client.getServer()->getIOService()->run_one(); + // Reset all indicators because we'll be now creating a second client. + resetCalloutBuffers(); + + // Create the second client to test that it may communicate with the + // server while the previous packet is parked. + Dhcp4Client client2(client1.getServer(), Dhcp4Client::SELECTING); + client2.setIfaceName("eth1"); + ASSERT_NO_THROW(client2.doDORA(boost::shared_ptr(new IOAddress("192.0.2.101")))); + + // The DHCPOFFER should have been returned but not DHCPACK, as this + // packet got parked too. + ASSERT_FALSE(client2.getContext().response_); + + // Check that the callback called is indeed the one we installed. + EXPECT_EQ("leases4_committed", callback_name_); + + // There should be now two actions schedulede on our IO service + // by the invoked callouts. They unpark both DHCPACK messages. + ASSERT_NO_THROW(client1.getServer()->getIOService()->poll()); + + // Receive and check the first response. + ASSERT_NO_THROW(client1.receiveResponse()); + ASSERT_TRUE(client1.getContext().response_); + Pkt4Ptr rsp = client1.getContext().response_; + EXPECT_EQ(DHCPACK, rsp->getType()); + EXPECT_EQ("192.0.2.100", rsp->getYiaddr().toText()); + + // Receive and check the second response. + ASSERT_NO_THROW(client2.receiveResponse()); + ASSERT_TRUE(client2.getContext().response_); + rsp = client2.getContext().response_; + EXPECT_EQ(DHCPACK, rsp->getType()); + EXPECT_EQ("192.0.2.101", rsp->getYiaddr().toText()); } // This test verifies that valid RELEASE triggers lease4_release callouts