[3688] Avoid multiple calls to selectSubnet by the DHCPv4 server.

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.
 

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) {
+Dhcpv4Srv::selectSubnet(const Pkt4Ptr& query, const bool run_hooks) const {
-    return (Dhcpv4Exchange::selectSubnet(question));
+
+    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) {
+Dhcpv4Srv::sanityCheck(const Pkt4Ptr& query, RequirementLevel serverid) {
-    OptionPtr server_id = ex.getQuery()->getOption(DHO_DHCP_SERVER_IDENTIFIER);
+    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);
+    Subnet4Ptr subnet = ex.getContext()->subnet_;
-    if (!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);
     }
 

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
+    /// @note This processing is a likely candidate to be pushed into hooks.
-    /// @param rsp server's response
+    ///
+    /// @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

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),
+    EXPECT_NO_THROW(NakedDhcpv4Srv::sanityCheck(pkt, Dhcpv4Srv::OPTIONAL));
-                                                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),
+    EXPECT_NO_THROW(NakedDhcpv4Srv::sanityCheck(pkt, Dhcpv4Srv::MANDATORY));
-                                                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),
+    EXPECT_THROW(NakedDhcpv4Srv::sanityCheck(pkt, Dhcpv4Srv::MANDATORY),
-                                             Dhcpv4Srv::MANDATORY),
                  RFCViolation);
 }
 

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