From: Marcin Siodelski Date: Thu, 12 Mar 2015 19:14:41 +0000 (+0100) Subject: [3688] Avoid multiple calls to selectSubnet by the DHCPv4 server. X-Git-Tag: trac3764_base~12^2~5 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=5ed8d73666ebf312cadf03b515cda66ae75454b1;p=thirdparty%2Fkea.git [3688] Avoid multiple calls to selectSubnet by the DHCPv4 server. --- diff --git a/src/bin/dhcp4/dhcp4_messages.mes b/src/bin/dhcp4/dhcp4_messages.mes index 1237c59da0..e9528ae798 100644 --- a/src/bin/dhcp4/dhcp4_messages.mes +++ b/src/bin/dhcp4/dhcp4_messages.mes @@ -31,10 +31,6 @@ to establish a session with the Kea control channel. This debug message informs that incoming packet has been assigned to specified class or classes. This is a normal behavior and indicates successful operation. -% DHCP4_CLASS_PROCESSING_FAILED client class specific processing failed -This debug message means that the server processing that is unique for each -client class has reported a failure. The response packet will not be sent. - % DHCP4_CLIENT_NAME_PROC_FAIL failed to process the fqdn or hostname sent by a client: %1 This debug message is issued when the DHCP server was unable to process the FQDN or Hostname option sent by a client. This is likely because the client's @@ -78,6 +74,10 @@ change is committed by the administrator. A debug message indicating that the DHCPv4 server has received an updated configuration from the Kea configuration system. +% DHCP4_DISCOVER_CLASS_PROCESSING_FAILED client class specific processing failed for DHCPDISCOVER +This debug message means that the server processing that is unique for each +client class has reported a failure. The response packet will not be sent. + % DHCP4_DDNS_REQUEST_SEND_FAILED failed sending a request to kea-dhcp-ddns, error: %1, ncr: %2 This error message indicates that DHCP4 server attempted to send a DDNS update request to the DHCP-DDNS server. This is most likely a configuration or @@ -148,6 +148,10 @@ point, the setting of the flag instructs the server not to choose a subnet, an action that severely limits further processing; the server will be only able to offer global options - no addresses will be assigned. +% DHCP4_INFORM_CLASS_PROCESSING_FAILED client class specific processing failed for DHCPINFORM +This debug message means that the server processing that is unique for each +client class has reported a failure. The response packet will not be sent. + % DHCP4_INIT_FAIL failed to initialize Kea server: %1 The server has failed to initialize. This may be because the configuration was not successful, or it encountered any other critical error on startup. @@ -349,6 +353,10 @@ a different hardware address. One possible reason for using different hardware address is that a cloned virtual machine was not updated and both clones use the same client-id. +% DHCP4_REQUEST_CLASS_PROCESSING_FAILED client class specific processing failed for DHCPREQUEST +This debug message means that the server processing that is unique for each +client class has reported a failure. The response packet will not be sent. + % DHCP4_RESPONSE_DATA responding with packet type %1, data is <%2> A debug message listing the data returned to the client. diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index c3bfb8d595..95a26066e3 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -85,13 +85,21 @@ namespace isc { namespace dhcp { Dhcpv4Exchange::Dhcpv4Exchange(const AllocEnginePtr& alloc_engine, - const Pkt4Ptr& query) + const Pkt4Ptr& query, + const Subnet4Ptr& subnet) : alloc_engine_(alloc_engine), query_(query), resp_(), context_(new AllocEngine::ClientContext4()) { + + if (!alloc_engine_ || !query_) { + isc_throw(BadValue, "alloc_engine and query values must not" + " be NULL when creating an instance of the" + " Dhcpv4Exchange"); + } + // Create response message. initResponse(); // Select subnet for the query message. - selectSubnet(); + context_->subnet_ = subnet; // Hardware address. context_->hwaddr_ = query->getHWAddr(); // Client Identifier @@ -122,61 +130,6 @@ Dhcpv4Exchange::initResponse() { } } -void -Dhcpv4Exchange::selectSubnet() { - context_->subnet_ = selectSubnet(query_); -} - -Subnet4Ptr -Dhcpv4Exchange::selectSubnet(const Pkt4Ptr& query) { - - Subnet4Ptr subnet; - - SubnetSelector selector; - selector.ciaddr_ = query->getCiaddr(); - selector.giaddr_ = query->getGiaddr(); - selector.local_address_ = query->getLocalAddr(); - selector.remote_address_ = query->getRemoteAddr(); - selector.client_classes_ = query->classes_; - selector.iface_name_ = query->getIface(); - - CfgMgr& cfgmgr = CfgMgr::instance(); - subnet = cfgmgr.getCurrentCfg()->getCfgSubnets4()->selectSubnet(selector); - - // Let's execute all callouts registered for subnet4_select - if (HooksManager::calloutsPresent(Hooks.hook_index_subnet4_select_)) { - CalloutHandlePtr callout_handle = getCalloutHandle(query); - - // We're reusing callout_handle from previous calls - callout_handle->deleteAllArguments(); - - // Set new arguments - callout_handle->setArgument("query4", query); - callout_handle->setArgument("subnet4", subnet); - callout_handle->setArgument("subnet4collection", - cfgmgr.getCurrentCfg()-> - getCfgSubnets4()->getAll()); - - // Call user (and server-side) callouts - HooksManager::callCallouts(Hooks.hook_index_subnet4_select_, - *callout_handle); - - // Callouts decided to skip this step. This means that no subnet - // will be selected. Packet processing will continue, but it will - // be severely limited (i.e. only global options will be assigned) - if (callout_handle->getSkip()) { - LOG_DEBUG(dhcp4_logger, DBG_DHCP4_HOOKS, - DHCP4_HOOK_SUBNET4_SELECT_SKIP); - return (Subnet4Ptr()); - } - - // Use whatever subnet was specified by the callout - callout_handle->getArgument("subnet4", subnet); - } - - return (subnet); -} - const std::string Dhcpv4Srv::VENDOR_CLASS_PREFIX("VENDOR_CLASS_"); Dhcpv4Srv::Dhcpv4Srv(uint16_t port, const bool use_bcast, @@ -232,8 +185,53 @@ Dhcpv4Srv::shutdown() { } isc::dhcp::Subnet4Ptr -Dhcpv4Srv::selectSubnet(const Pkt4Ptr& question) { - return (Dhcpv4Exchange::selectSubnet(question)); +Dhcpv4Srv::selectSubnet(const Pkt4Ptr& query, const bool run_hooks) const { + + Subnet4Ptr subnet; + + SubnetSelector selector; + selector.ciaddr_ = query->getCiaddr(); + selector.giaddr_ = query->getGiaddr(); + selector.local_address_ = query->getLocalAddr(); + selector.remote_address_ = query->getRemoteAddr(); + selector.client_classes_ = query->classes_; + selector.iface_name_ = query->getIface(); + + CfgMgr& cfgmgr = CfgMgr::instance(); + subnet = cfgmgr.getCurrentCfg()->getCfgSubnets4()->selectSubnet(selector); + + // Let's execute all callouts registered for subnet4_select + if (run_hooks && HooksManager::calloutsPresent(hook_index_subnet4_select_)) { + CalloutHandlePtr callout_handle = getCalloutHandle(query); + + // We're reusing callout_handle from previous calls + callout_handle->deleteAllArguments(); + + // Set new arguments + callout_handle->setArgument("query4", query); + callout_handle->setArgument("subnet4", subnet); + callout_handle->setArgument("subnet4collection", + cfgmgr.getCurrentCfg()-> + getCfgSubnets4()->getAll()); + + // Call user (and server-side) callouts + HooksManager::callCallouts(hook_index_subnet4_select_, + *callout_handle); + + // Callouts decided to skip this step. This means that no subnet + // will be selected. Packet processing will continue, but it will + // be severely limited (i.e. only global options will be assigned) + if (callout_handle->getSkip()) { + LOG_DEBUG(dhcp4_logger, DBG_DHCP4_HOOKS, + DHCP4_HOOK_SUBNET4_SELECT_SKIP); + return (Subnet4Ptr()); + } + + // Use whatever subnet was specified by the callout + callout_handle->getArgument("subnet4", subnet); + } + + return (subnet); } Pkt4Ptr @@ -457,17 +455,6 @@ Dhcpv4Srv::run() { continue; } - // Let's do class specific processing. This is done before - // pkt4_send. - // - /// @todo: decide whether we want to add a new hook point for - /// doing class specific processing. - if (!classSpecificProcessing(query, rsp)) { - /// @todo add more verbosity here - LOG_DEBUG(dhcp4_logger, DBG_DHCP4_BASIC, DHCP4_CLASS_PROCESSING_FAILED); - - continue; - } // Specifies if server should do the packing bool skip_pack = false; @@ -1468,9 +1455,9 @@ Dhcpv4Srv::getNetmaskOption(const Subnet4Ptr& subnet) { Pkt4Ptr Dhcpv4Srv::processDiscover(Pkt4Ptr& discover) { - Dhcpv4Exchange ex(alloc_engine_, discover); + sanityCheck(discover, FORBIDDEN); - sanityCheck(ex, FORBIDDEN); + Dhcpv4Exchange ex(alloc_engine_, discover, selectSubnet(discover)); copyDefaultFields(ex); appendDefaultOptions(ex); @@ -1509,15 +1496,22 @@ Dhcpv4Srv::processDiscover(Pkt4Ptr& discover) { appendServerID(ex); + /// @todo: decide whether we want to add a new hook point for + /// doing class specific processing. + if (!classSpecificProcessing(ex)) { + /// @todo add more verbosity here + LOG_DEBUG(dhcp4_logger, DBG_DHCP4_BASIC, DHCP4_DISCOVER_CLASS_PROCESSING_FAILED); + } + return (ex.getResponse()); } Pkt4Ptr Dhcpv4Srv::processRequest(Pkt4Ptr& request) { - Dhcpv4Exchange ex(alloc_engine_, request); - /// @todo Uncomment this (see ticket #3116) - /// sanityCheck(ex, MANDATORY); + /// sanityCheck(request, MANDATORY); + + Dhcpv4Exchange ex(alloc_engine_, request, selectSubnet(request)); copyDefaultFields(ex); appendDefaultOptions(ex); @@ -1554,6 +1548,13 @@ Dhcpv4Srv::processRequest(Pkt4Ptr& request) { appendServerID(ex); + /// @todo: decide whether we want to add a new hook point for + /// doing class specific processing. + if (!classSpecificProcessing(ex)) { + /// @todo add more verbosity here + LOG_DEBUG(dhcp4_logger, DBG_DHCP4_BASIC, DHCP4_REQUEST_CLASS_PROCESSING_FAILED); + } + return (ex.getResponse()); } @@ -1675,10 +1676,10 @@ Dhcpv4Srv::processDecline(Pkt4Ptr&) { Pkt4Ptr Dhcpv4Srv::processInform(Pkt4Ptr& inform) { - Dhcpv4Exchange ex(alloc_engine_, inform); - // DHCPINFORM MUST not include server identifier. - sanityCheck(ex, FORBIDDEN); + sanityCheck(inform, FORBIDDEN); + + Dhcpv4Exchange ex(alloc_engine_, inform, selectSubnet(inform)); Pkt4Ptr ack = ex.getResponse(); @@ -1701,6 +1702,14 @@ Dhcpv4Srv::processInform(Pkt4Ptr& inform) { // The DHCPACK must contain server id. appendServerID(ex); + + /// @todo: decide whether we want to add a new hook point for + /// doing class specific processing. + if (!classSpecificProcessing(ex)) { + /// @todo add more verbosity here + LOG_DEBUG(dhcp4_logger, DBG_DHCP4_BASIC, DHCP4_INFORM_CLASS_PROCESSING_FAILED); + } + return (ex.getResponse()); } @@ -1800,7 +1809,7 @@ Dhcpv4Srv::acceptDirectRequest(const Pkt4Ptr& pkt) const { return (false); } return ((pkt->getLocalAddr() != IOAddress::IPV4_BCAST_ADDRESS() - || Dhcpv4Exchange::selectSubnet(pkt))); + || selectSubnet(pkt, false))); } bool @@ -1899,14 +1908,14 @@ Dhcpv4Srv::acceptServerId(const Pkt4Ptr& query) const { } void -Dhcpv4Srv::sanityCheck(const Dhcpv4Exchange& ex, RequirementLevel serverid) { - OptionPtr server_id = ex.getQuery()->getOption(DHO_DHCP_SERVER_IDENTIFIER); +Dhcpv4Srv::sanityCheck(const Pkt4Ptr& query, RequirementLevel serverid) { + OptionPtr server_id = query->getOption(DHO_DHCP_SERVER_IDENTIFIER); switch (serverid) { case FORBIDDEN: if (server_id) { isc_throw(RFCViolation, "Server-id option was not expected, but " << "received in " - << serverReceivedPacketName(ex.getQuery()->getType())); + << serverReceivedPacketName(query->getType())); } break; @@ -1914,7 +1923,7 @@ Dhcpv4Srv::sanityCheck(const Dhcpv4Exchange& ex, RequirementLevel serverid) { if (!server_id) { isc_throw(RFCViolation, "Server-id option was expected, but not " " received in message " - << serverReceivedPacketName(ex.getQuery()->getType())); + << serverReceivedPacketName(query->getType())); } break; @@ -1924,19 +1933,19 @@ Dhcpv4Srv::sanityCheck(const Dhcpv4Exchange& ex, RequirementLevel serverid) { } // If there is HWAddress set and it is non-empty, then we're good - if (ex.getQuery()->getHWAddr() && !ex.getQuery()->getHWAddr()->hwaddr_.empty()) { + if (query->getHWAddr() && !query->getHWAddr()->hwaddr_.empty()) { return; } // There has to be something to uniquely identify the client: // either non-zero MAC address or client-id option present (or both) - OptionPtr client_id = ex.getQuery()->getOption(DHO_DHCP_CLIENT_IDENTIFIER); + OptionPtr client_id = query->getOption(DHO_DHCP_CLIENT_IDENTIFIER); // If there's no client-id (or a useless one is provided, i.e. 0 length) if (!client_id || client_id->len() == client_id->getHeaderLen()) { isc_throw(RFCViolation, "Missing or useless client-id and no HW address " " provided in message " - << serverReceivedPacketName(ex.getQuery()->getType())); + << serverReceivedPacketName(query->getType())); } } @@ -2086,10 +2095,15 @@ void Dhcpv4Srv::classifyPacket(const Pkt4Ptr& pkt) { } } -bool Dhcpv4Srv::classSpecificProcessing(const Pkt4Ptr& query, const Pkt4Ptr& rsp) { +bool +Dhcpv4Srv::classSpecificProcessing(const Dhcpv4Exchange& ex) { - Subnet4Ptr subnet = Dhcpv4Exchange::selectSubnet(query); - if (!subnet) { + Subnet4Ptr subnet = ex.getContext()->subnet_; + Pkt4Ptr query = ex.getQuery(); + Pkt4Ptr rsp = ex.getResponse(); + + // If any of those is missing, there is nothing to do. + if (!subnet || !query || !rsp) { return (true); } diff --git a/src/bin/dhcp4/dhcp4_srv.h b/src/bin/dhcp4/dhcp4_srv.h index 24d33199bd..78cc83b12b 100644 --- a/src/bin/dhcp4/dhcp4_srv.h +++ b/src/bin/dhcp4/dhcp4_srv.h @@ -75,7 +75,9 @@ public: /// @param alloc_engine Pointer to the instance of the Allocation Engine /// used by the server. /// @param query Pointer to the client message. - Dhcpv4Exchange(const AllocEnginePtr& alloc_engine, const Pkt4Ptr& query); + /// @param subnet Pointer to the subnet to which the client belongs. + Dhcpv4Exchange(const AllocEnginePtr& alloc_engine, const Pkt4Ptr& query, + const Subnet4Ptr& subnet); /// @brief Initializes the instance of the response message. /// @@ -85,25 +87,6 @@ public: /// response is not initialized. void initResponse(); - /// @brief Selects the subnet for the message processing. - /// - /// The pointer to the selected subnet is stored in the @c ClientContext4 - /// structure. - void selectSubnet(); - - /// @brief Selects the subnet for the message processing. - /// - /// @todo This variant of the @c selectSubnet method is static and public so - /// as it may be invoked by the @c Dhcpv4Srv object. This is temporary solution - /// and the function will go away once the server code fully supports the use - /// of this class and it obtains the subnet from the context returned by the - /// @c getContext method. - /// - /// @param query Pointer to the client's message. - /// @return Pointer to the selected subnet or NULL if no suitable subnet - /// has been found. - static Subnet4Ptr selectSubnet(const Pkt4Ptr& query); - /// @brief Returns the pointer to the query from the client. Pkt4Ptr getQuery() const { return (query_); @@ -372,10 +355,10 @@ protected: /// Checks if mandatory option is really there, that forbidden option /// is not there, and that client-id or server-id appears only once. /// - /// @param ex DHCPv4 exchange holding the client's message to be checked. + /// @param query Pointer to the client's message. /// @param serverid expectation regarding server-id option /// @throw RFCViolation if any issues are detected - static void sanityCheck(const Dhcpv4Exchange& ex, RequirementLevel serverid); + static void sanityCheck(const Pkt4Ptr& query, RequirementLevel serverid); /// @brief Processes incoming DISCOVER and returns response. /// @@ -709,9 +692,19 @@ protected: /// @brief Selects a subnet for a given client's packet. /// - /// @param question client's message + /// The @c run_hooks parameters controls whether the method should run + /// installed hooks for subnet selection. Disabling it is useful in + /// cases when the server should sanity check the client's packet before + /// the actual processing. If the sanity check fails, the packet can + /// be discarded. + /// + /// @param query client's message + /// @param run_hooks A boolean value which specifies if the method should + /// run installed hooks after selecting the subnet (if true). The default + /// value is true. /// @return selected subnet (or NULL if no suitable subnet was found) - static isc::dhcp::Subnet4Ptr selectSubnet(const Pkt4Ptr& question); + isc::dhcp::Subnet4Ptr selectSubnet(const Pkt4Ptr& query, + const bool run_hooks = true) const; /// indicates if shutdown is in progress. Setting it to true will /// initiate server shutdown procedure. @@ -753,12 +746,15 @@ protected: /// @brief Performs packet processing specific to a class /// - /// This processing is a likely candidate to be pushed into hooks. + /// If the selected subnet, query or response in the @c ex object is NULL + /// this method returns immediately and returns true. /// - /// @param query incoming client's packet - /// @param rsp server's response + /// @note This processing is a likely candidate to be pushed into hooks. + /// + /// @param ex The exchange holding both the client's message and the + /// server's response. /// @return true if successful, false otherwise (will prevent sending response) - bool classSpecificProcessing(const Pkt4Ptr& query, const Pkt4Ptr& rsp); + bool classSpecificProcessing(const Dhcpv4Exchange& ex); /// @brief Allocation Engine. /// Pointer to the allocation engine that we are currently using diff --git a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc index d6a1fa10bf..65e0e7ef15 100644 --- a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc +++ b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc @@ -960,28 +960,25 @@ TEST_F(Dhcpv4SrvTest, sanityCheck) { pkt->setHWAddr(generateHWAddr(6)); // Server-id is optional for information-request, so - EXPECT_NO_THROW(NakedDhcpv4Srv::sanityCheck(createExchange(pkt), - Dhcpv4Srv::OPTIONAL)); + EXPECT_NO_THROW(NakedDhcpv4Srv::sanityCheck(pkt, Dhcpv4Srv::OPTIONAL)); // Empty packet, no server-id - EXPECT_THROW(NakedDhcpv4Srv::sanityCheck(createExchange(pkt), Dhcpv4Srv::MANDATORY), + EXPECT_THROW(NakedDhcpv4Srv::sanityCheck(pkt, Dhcpv4Srv::MANDATORY), RFCViolation); pkt->addOption(srv->getServerID()); // Server-id is mandatory and present = no exception - EXPECT_NO_THROW(NakedDhcpv4Srv::sanityCheck(createExchange(pkt), - Dhcpv4Srv::MANDATORY)); + EXPECT_NO_THROW(NakedDhcpv4Srv::sanityCheck(pkt, Dhcpv4Srv::MANDATORY)); // Server-id is forbidden, but present => exception - EXPECT_THROW(NakedDhcpv4Srv::sanityCheck(createExchange(pkt), Dhcpv4Srv::FORBIDDEN), + EXPECT_THROW(NakedDhcpv4Srv::sanityCheck(pkt, Dhcpv4Srv::FORBIDDEN), RFCViolation); // There's no client-id and no HWADDR. Server needs something to // identify the client pkt->setHWAddr(generateHWAddr(0)); - EXPECT_THROW(NakedDhcpv4Srv::sanityCheck(createExchange(pkt), - Dhcpv4Srv::MANDATORY), + EXPECT_THROW(NakedDhcpv4Srv::sanityCheck(pkt, Dhcpv4Srv::MANDATORY), RFCViolation); } diff --git a/src/bin/dhcp4/tests/dhcp4_test_utils.cc b/src/bin/dhcp4/tests/dhcp4_test_utils.cc index 058f32ae7c..9a82bd46e7 100644 --- a/src/bin/dhcp4/tests/dhcp4_test_utils.cc +++ b/src/bin/dhcp4/tests/dhcp4_test_utils.cc @@ -589,7 +589,7 @@ Dhcpv4SrvTest::configure(const std::string& config, NakedDhcpv4Srv& srv, Dhcpv4Exchange Dhcpv4SrvTest::createExchange(const Pkt4Ptr& query) { - return (Dhcpv4Exchange(srv_.alloc_engine_, query)); + return (Dhcpv4Exchange(srv_.alloc_engine_, query, srv_.selectSubnet(query))); }