[5443] Checkpoint: proposed code for dhcp4, todo new tests and dhcp6 port

diff --git a/src/bin/dhcp4/dhcp4_hooks.dox b/src/bin/dhcp4/dhcp4_hooks.dox
index 9c87a66c2d19c1ce4a8a4cd7a4940e58df839c27..c522453e6a83695e54743b8a721cfa3942aa1ffb 100644 (file)
--- a/src/bin/dhcp4/dhcp4_hooks.dox
+++ b/src/bin/dhcp4/dhcp4_hooks.dox
@@ -61,15 +61,15 @@ to the end of this list.
    are set yet. Callouts installed on this hook point can modify the data
     in the received buffer. The server will parse the buffer afterwards.
 
- - <b>Next step status</b>: If any callout sets the status to SKIP, the server will
+ - <b>Next step status</b>: If any callout sets the status to DROP, the server
+   will drop the packet and start processing the next one.
+   If any callout sets the status to SKIP, the server will
    skip the buffer parsing. In this case there is an expectation that
    the callout will parse the options carried in the buffer, create
    @c isc::dhcp::Option objects (or derived) and add them to the "query4"
    object using the @c isc::dhcp::Pkt4::addOption.
    Otherwise the server will find out that some mandatory options are
-   missing (e.g. DHCP Message Type) and will drop the message. If you
-   want to have the capability to drop a message, it is better to use
-   the skip flag in the "pkt4_receive" callout.
+   missing (e.g. DHCP Message Type) and will drop the message.
 
  @subsection dhcpv4HooksPkt4Receive pkt4_receive
 
@@ -87,7 +87,7 @@ to the end of this list.
    field has been already parsed and stored in other fields. Therefore,
    the modification in the data_ field has no effect.
 
- - <b>Next step status</b>: If any callout sets the status to SKIP, the server will
+ - <b>Next step status</b>: If any callout sets the status to SKIP (or DROP), the server will
    drop the packet and start processing the next one.  The reason for the drop
    will be logged if logging is set to the appropriate debug level.
 
@@ -107,7 +107,9 @@ to the end of this list.
    not be modified.
 
  - <b>Next step status</b>: If any callout installed on the "subnet4_select" hook
-   sets the next step status to SKIP, the server will not select any subnet.
+   sets the next step status to DROP, the server cancels current processing,
+   drop the packet and start processing the next one.
+   If any callout sets the status to SKIP, the server will not select any subnet.
    Packet processing will continue, but will be severely limited.
 
 @subsection dhcpv4HooksHost4Identifier host4_identifier
@@ -186,7 +188,7 @@ to the end of this list.
    be released. It doesn't make sense to modify it at this time.
 
  - <b>Next step status</b>: If any callout installed on the "lease4_release" hook
-   sets the next step action to SKIP, the server will not delete the lease.
+   sets the next step action to SKIP (or DROP), the server will not delete the lease.
    It will be kept in the database and will go through the regular expiration/reuse
    process.
 
@@ -206,8 +208,8 @@ to the end of this list.
    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.
+ - <b>Next step status</b>: If any callout installed on the "lease4_decline" hook
+   sets the next step action to SKIP (or 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,
@@ -233,7 +235,7 @@ to the end of this list.
    cannot be modified.
 
  - <b>Next step action</b>: if any callout installed on the "pkt4_send" hook
-   sets the next step action to SKIP, the server will not construct the raw
+   sets the next step action to SKIP (or DROP), the server will not construct the raw
    buffer. The expectation is that if the callout set skip flag, it is
    responsible for constructing raw form on its own. Otherwise the output
    packet will be sent with zero length.
@@ -254,7 +256,7 @@ to the end of this list.
    time, because they were already used to construct on-wire buffer. Their
    modification would have no effect.
 
- - <b>Next step status</b>: if any callout sets the next step action to SKIP,
+ - <b>Next step status</b>: if any callout sets the next step action to SKIP (or DROP),
    the server will drop this response packet. However, the original request
    packet from a client was processed, so server's state most likely has changed
    (e.g. lease was allocated). Setting this flag merely stops the change

diff --git a/src/bin/dhcp4/dhcp4_messages.mes b/src/bin/dhcp4/dhcp4_messages.mes
index 05426d29e7a0cf3f56a3a8b5846fe21479200edc..c6a7848400872e9caa12ea5795fd2c58bfe116d1 100644 (file)
--- a/src/bin/dhcp4/dhcp4_messages.mes
+++ b/src/bin/dhcp4/dhcp4_messages.mes
@@ -238,13 +238,21 @@ A "libreload" command was issued to reload the hooks libraries but for
 some reason the reload failed.  Other error messages issued from the
 hooks framework will indicate the nature of the problem.
 
-% DHCP4_HOOK_BUFFER_RCVD_SKIP received buffer from %1 to %2 over interface %3 was dropped because a callout set the skip flag
+% DHCP4_HOOK_BUFFER_RCVD_DROP received buffer from %1 to %2 over interface %3 was dropped because a callout set the drop flag
 This debug message is printed when a callout installed on buffer4_receive
-hook point set the skip flag. For this particular hook point, the
+hook point set the drop flag. For this particular hook point, the
 setting of the flag by a callout instructs the server to drop the packet.
 The arguments specify the source and destination IPv4 address as well as
 the name of the interface over which the buffer has been received.
 
+% DHCP4_HOOK_BUFFER_RCVD_SKIP received buffer from %1 to %2 over interface %3 is not parsed because a callout set the skip flag
+This debug message is printed when a callout installed on
+buffer4_receive hook point set the skip flag. For this particular hook
+point, the setting of the flag by a callout instructs the server to
+not parse the buffer because it was already parsed by the hook. The
+arguments specify the source and destination IPv4 address as well as
+the name of the interface over which the buffer has been received.
+
 % DHCP4_HOOK_BUFFER_SEND_SKIP %1: prepared response is dropped because a callout set the skip flag.
 This debug message is printed when a callout installed on buffer4_send
 hook point set the skip flag. For this particular hook point, the
@@ -277,6 +285,13 @@ of the flag instructs the server to drop the packet. This means that
 the client will not get any response, even though the server processed
 client's request and acted on it (e.g. possibly allocated a lease).
 
+% DHCP4_HOOK_SUBNET4_SELECT_DROP %1: packet was dropped, because a callout set drop flag
+This debug message is printed when a callout installed on the
+subnet4_select hook point sets the drop flag. For this particular hook
+point, the setting of the flag instructs the server to drop the receive
+packet. The argument specifies the client and transaction identification
+information.
+
 % DHCP4_HOOK_SUBNET4_SELECT_SKIP %1: no subnet was selected, because a callout set skip flag.
 This debug message is printed when a callout installed on the
 subnet4_select hook point sets the skip flag. For this particular hook

diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc
index 97ecfa5cf3520bf49d43f27aefd32b7ffadae23a..9a12a33b6ab1ccb86a1b8ead1906dd88061e6d93 100644 (file)
--- a/src/bin/dhcp4/dhcp4_srv.cc
+++ b/src/bin/dhcp4/dhcp4_srv.cc
@@ -483,7 +483,7 @@ Dhcpv4Srv::shutdown() {
 }
 
 isc::dhcp::Subnet4Ptr
-Dhcpv4Srv::selectSubnet(const Pkt4Ptr& query) const {
+Dhcpv4Srv::selectSubnet(Pkt4Ptr& query) {
 
     // DHCPv4-over-DHCPv6 is a special (and complex) case
     if (query->isDhcp4o6()) {
@@ -569,7 +569,16 @@ Dhcpv4Srv::selectSubnet(const Pkt4Ptr& query) const {
             return (Subnet4Ptr());
         }
 
-        /// @todo: Add support for DROP status
+        // Callouts decided to drop the packet. It is a superset of the
+        // skip case so no subnet will be selected. For the caller to
+        // know it has to drop the packet the query pointer is cleared.
+        if (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_DROP) {
+            LOG_DEBUG(hooks_logger, DBG_DHCP4_HOOKS,
+                      DHCP4_HOOK_SUBNET4_SELECT_DROP)
+                .arg(query->getLabel());
+            query.reset();
+            return (Subnet4Ptr());
+        }
 
         // Use whatever subnet was specified by the callout
         callout_handle->getArgument("subnet4", subnet);
@@ -596,7 +605,7 @@ Dhcpv4Srv::selectSubnet(const Pkt4Ptr& query) const {
 }
 
 isc::dhcp::Subnet4Ptr
-Dhcpv4Srv::selectSubnet4o6(const Pkt4Ptr& query) const {
+Dhcpv4Srv::selectSubnet4o6(Pkt4Ptr& query) {
 
     Subnet4Ptr subnet;
 
@@ -673,7 +682,16 @@ Dhcpv4Srv::selectSubnet4o6(const Pkt4Ptr& query) const {
             return (Subnet4Ptr());
         }
 
-        /// @todo: Add support for DROP status
+        // Callouts decided to drop the packet. It is a superset of the
+        // skip case so no subnet will be selected. For the caller to
+        // know it has to drop the packet the query pointer is cleared.
+        if (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_DROP) {
+            LOG_DEBUG(hooks_logger, DBG_DHCP4_HOOKS,
+                      DHCP4_HOOK_SUBNET4_SELECT_DROP)
+                .arg(query->getLabel());
+            query.reset();
+            return (Subnet4Ptr());
+        }
 
         // Use whatever subnet was specified by the callout
         callout_handle->getArgument("subnet4", subnet);
@@ -841,15 +859,14 @@ Dhcpv4Srv::run_one() {
             // 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) {
+            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;
             }
 
-            /// @todo: Add support for DROP status.
-
             callout_handle->getArgument("response4", rsp);
         }
 
@@ -911,6 +928,18 @@ Dhcpv4Srv::processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp) {
         HooksManager::callCallouts(Hooks.hook_index_buffer4_receive_,
                                    *callout_handle);
 
+        // Callouts decided to drop the received packet.
+        // The response (rsp) is null so the caller (run_one) will
+        // immediately return too.
+        if (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_DROP) {
+            LOG_DEBUG(hooks_logger, DBG_DHCP4_DETAIL,
+                      DHCP4_HOOK_BUFFER_RCVD_DROP)
+                .arg(query->getRemoteAddr().toText())
+                .arg(query->getLocalAddr().toText())
+                .arg(query->getIface());
+            return;
+        }
+
         // Callouts decided to skip the next processing step. The next
         // processing step would to parse the packet, so skip at this
         // stage means that callouts did the parsing already, so server
@@ -925,8 +954,6 @@ Dhcpv4Srv::processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp) {
         }
 
         callout_handle->getArgument("query4", query);
-
-        /// @todo: add support for DROP status
     }
 
     // Unpack the packet information unless the buffer4_receive callouts
@@ -1010,15 +1037,14 @@ Dhcpv4Srv::processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp) {
         // Callouts decided to skip the next processing step. The next
         // processing step would to process the packet, so skip at this
         // stage means drop.
-        if (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_SKIP) {
+        if ((callout_handle->getStatus() == CalloutHandle::NEXT_STEP_SKIP) ||
+            (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_DROP)) {
             LOG_DEBUG(hooks_logger, DBG_DHCP4_HOOKS,
                       DHCP4_HOOK_PACKET_RCVD_SKIP)
                 .arg(query->getLabel());
             return;
         }
 
-        /// @todo: Add support for DROP status
-
         callout_handle->getArgument("query4", query);
     }
 
@@ -1106,14 +1132,13 @@ Dhcpv4Srv::processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp) {
         // Callouts decided to skip the next processing step. The next
         // processing step would to send the packet, so skip at this
         // stage means "drop response".
-        if (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_SKIP) {
+        if ((callout_handle->getStatus() == CalloutHandle::NEXT_STEP_SKIP) ||
+            (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_DROP)) {
             LOG_DEBUG(hooks_logger, DBG_DHCP4_HOOKS,
                       DHCP4_HOOK_PACKET_SEND_SKIP)
                 .arg(query->getLabel());
             skip_pack = true;
         }
-
-        /// @todo: Add support for DROP status
     }
 
     if (!skip_pack) {
@@ -2318,6 +2343,11 @@ Dhcpv4Srv::processDiscover(Pkt4Ptr& discover) {
 
     Dhcpv4Exchange ex(alloc_engine_, discover, selectSubnet(discover));
 
+    // Stop here if selectSubnet decided to drop the packet
+    if (!discover) {
+        return (Pkt4Ptr());
+    }
+
     // If DHCPDISCOVER message contains the FQDN or Hostname option, server
     // may respond to the client with the appropriate FQDN or Hostname
     // option to indicate that whether it will take responsibility for
@@ -2370,6 +2400,11 @@ Dhcpv4Srv::processRequest(Pkt4Ptr& request) {
 
     Dhcpv4Exchange ex(alloc_engine_, request, selectSubnet(request));
 
+    // Stop here if selectSubnet decided to drop the packet
+    if (!request) {
+        return (Pkt4Ptr());
+    }
+
     // If DHCPREQUEST message contains the FQDN or Hostname option, server
     // should respond to the client with the appropriate FQDN or Hostname
     // option to indicate if it takes responsibility for the DNS updates.
@@ -2478,14 +2513,13 @@ Dhcpv4Srv::processRelease(Pkt4Ptr& release) {
             // Callouts decided to skip the next processing step. The next
             // processing step would to send the packet, so skip at this
             // stage means "drop response".
-            if (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_SKIP) {
+            if ((callout_handle->getStatus() == CalloutHandle::NEXT_STEP_SKIP) ||
+                (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_DROP)) {
                 skip = true;
                 LOG_DEBUG(hooks_logger, DBG_DHCP4_HOOKS,
                           DHCP4_HOOK_LEASE4_RELEASE_SKIP)
                     .arg(release->getLabel());
             }
-
-            /// @todo add support for DROP status
         }
 
         // Callout didn't indicate to skip the release process. Let's release
@@ -2619,9 +2653,10 @@ Dhcpv4Srv::declineLease(const Lease4Ptr& lease, const Pkt4Ptr& decline) {
         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) {
+        // Check if callouts decided to skip the next processing step.
+        // If any of them did, we will drop the packet.
+        if ((callout_handle->getStatus() == CalloutHandle::NEXT_STEP_SKIP) ||
+            (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;
@@ -2671,6 +2706,11 @@ Dhcpv4Srv::processInform(Pkt4Ptr& inform) {
 
     Dhcpv4Exchange ex(alloc_engine_, inform, selectSubnet(inform));
 
+    // Stop here if selectSubnet decided to drop the packet
+    if (!inform) {
+        return (Pkt4Ptr());
+    }
+
     Pkt4Ptr ack = ex.getResponse();
 
     ex.setReservedClientClasses();
@@ -2707,7 +2747,7 @@ Dhcpv4Srv::processInform(Pkt4Ptr& inform) {
 }
 
 bool
-Dhcpv4Srv::accept(const Pkt4Ptr& query) const {
+Dhcpv4Srv::accept(Pkt4Ptr& query) {
     // Check that the message type is accepted by the server. We rely on the
     // function called to log a message if needed.
     if (!acceptMessageType(query)) {
@@ -2735,7 +2775,7 @@ Dhcpv4Srv::accept(const Pkt4Ptr& query) const {
 }
 
 bool
-Dhcpv4Srv::acceptDirectRequest(const Pkt4Ptr& pkt) const {
+Dhcpv4Srv::acceptDirectRequest(Pkt4Ptr& pkt) {
     // Accept all relayed messages.
     if (pkt->isRelayed()) {
         return (true);
@@ -2762,7 +2802,12 @@ Dhcpv4Srv::acceptDirectRequest(const Pkt4Ptr& pkt) const {
         // we validate the message type prior to calling this function.
         return (false);
     }
-    return (!pkt->getLocalAddr().isV4Bcast() || selectSubnet(pkt));
+    bool result = (!pkt->getLocalAddr().isV4Bcast() || selectSubnet(pkt));
+    if (!pkt) {
+        // The packet must be dropped.
+        return (false);
+    }
+    return (result);
 }
 
 bool

diff --git a/src/bin/dhcp4/dhcp4_srv.h b/src/bin/dhcp4/dhcp4_srv.h
index e72af87c8415a8f763ff78efc7f1788c0263784e..d601ddddbe08110c26b4d3e5fd02e34f62c8e52b 100644 (file)
--- a/src/bin/dhcp4/dhcp4_srv.h
+++ b/src/bin/dhcp4/dhcp4_srv.h
@@ -348,7 +348,7 @@ protected:
     ///
     /// @return true if the message should be further processed, or false if
     /// the message should be discarded.
-    bool accept(const Pkt4Ptr& query) const;
+    bool accept(Pkt4Ptr& query);
 
     /// @brief Check if a message sent by directly connected client should be
     /// accepted or discarded.
@@ -377,7 +377,7 @@ protected:
     ///
     /// @return true if message is accepted for further processing, false
     /// otherwise.
-    bool acceptDirectRequest(const Pkt4Ptr& query) const;
+    bool acceptDirectRequest(Pkt4Ptr& query);
 
     /// @brief Check if received message type is valid for the server to
     /// process.
@@ -766,15 +766,19 @@ protected:
 
     /// @brief Selects a subnet for a given client's packet.
     ///
+    /// When the packet has to be dropped the query pointer is cleared
+    ///
     /// @param query client's message
     /// @return selected subnet (or NULL if no suitable subnet was found)
-    isc::dhcp::Subnet4Ptr selectSubnet(const Pkt4Ptr& query) const;
+    isc::dhcp::Subnet4Ptr selectSubnet(Pkt4Ptr& query);
 
     /// @brief Selects a subnet for a given client's DHCP4o6 packet.
     ///
+    /// When the packet has to be dropped the query pointer is cleared
+    ///
     /// @param query client's message
     /// @return selected subnet (or NULL if no suitable subnet was found)
-    isc::dhcp::Subnet4Ptr selectSubnet4o6(const Pkt4Ptr& query) const;
+    isc::dhcp::Subnet4Ptr selectSubnet4o6(Pkt4Ptr& query);
 
     /// indicates if shutdown is in progress. Setting it to true will
     /// initiate server shutdown procedure.

diff --git a/src/bin/dhcp4/tests/dhcp4_test_utils.cc b/src/bin/dhcp4/tests/dhcp4_test_utils.cc
index 0734a992290dd28c453a6fa833d4d3a2bf70d5e0..bd02d5f6fd42c95895582a7ab6eb93573cd6ff41 100644 (file)
--- a/src/bin/dhcp4/tests/dhcp4_test_utils.cc
+++ b/src/bin/dhcp4/tests/dhcp4_test_utils.cc
@@ -631,7 +631,7 @@ Dhcpv4SrvTest::configure(const std::string& config, NakedDhcpv4Srv& srv,
  }
 
 Dhcpv4Exchange
-Dhcpv4SrvTest::createExchange(const Pkt4Ptr& query) {
+Dhcpv4SrvTest::createExchange(Pkt4Ptr& query) {
     return (Dhcpv4Exchange(srv_.alloc_engine_, query, srv_.selectSubnet(query)));
 }
 

diff --git a/src/bin/dhcp4/tests/dhcp4_test_utils.h b/src/bin/dhcp4/tests/dhcp4_test_utils.h
index 48e571a447503fc041633e5904ad159f399b11ce..dd4c1a795b4824cb63d6a352d02c2c119ce3ba26 100644 (file)
--- a/src/bin/dhcp4/tests/dhcp4_test_utils.h
+++ b/src/bin/dhcp4/tests/dhcp4_test_utils.h
@@ -434,7 +434,7 @@ public:
                              uint8_t pkt_type, const std::string& stat_name);
 
     /// @brief Create @c Dhcpv4Exchange from client's query.
-    Dhcpv4Exchange createExchange(const Pkt4Ptr& query);
+    Dhcpv4Exchange createExchange(Pkt4Ptr& query);
 
     /// @brief Performs 4-way exchange to obtain new lease.
     ///

diff --git a/src/bin/dhcp4/tests/fqdn_unittest.cc b/src/bin/dhcp4/tests/fqdn_unittest.cc
index 264ec433b7fb46432de7b7a06911c5a303ba3bf9..2afa40955da1a870d66ad3aabec60283610ddb94 100644 (file)
--- a/src/bin/dhcp4/tests/fqdn_unittest.cc
+++ b/src/bin/dhcp4/tests/fqdn_unittest.cc
@@ -396,7 +396,7 @@ public:
 
     // Test that server generates the appropriate FQDN option in response to
     // client's FQDN option.
-    void testProcessFqdn(const Pkt4Ptr& query, const uint8_t exp_flags,
+    void testProcessFqdn(Pkt4Ptr& query, const uint8_t exp_flags,
                          const std::string& exp_domain_name,
                          Option4ClientFqdn::DomainNameType
                          exp_domain_type = Option4ClientFqdn::FULL) {
@@ -524,7 +524,7 @@ public:
     /// packet must contain the hostname option
     ///
     /// @return a pointer to the hostname option constructed by the server
-    OptionStringPtr processHostname(const Pkt4Ptr& query,
+    OptionStringPtr processHostname(Pkt4Ptr& query,
                                     bool must_have_host = true) {
         if (!getHostnameOption(query) && must_have_host) {
             ADD_FAILURE() << "Hostname option not carried in the query";