[898-add-a-new-hook-to-support-bootp] Fixed classification issue

doc/sphinx/arm/dhcp4-srv.rst

@@ -5625,7 +5625,7 @@ Supported DHCP Standards
 The following standards are currently supported:
 
 -  *BOOTP Vendor Information Extensions*, `RFC
-   1497 <https://tools.ietf.org/html/rfc1497>`__: This requires an open
+   1497 <https://tools.ietf.org/html/rfc1497>`__: This requires the open
    source BOOTP hook to be loaded.
 
 -  *Dynamic Host Configuration Protocol*, `RFC

src/hooks/dhcp/bootp/bootp.dox

@@ -53,8 +53,8 @@ bootp_callouts.cc. It checks if the necessary parameter is passed and
 decodes the option configurations. @ref unload() free the configuration.
 
 Kea engine checks if the library has functions that match known hook
-point names. This library has one such function: @ref pkt4_receive
+point names. This library has two such functions: @ref buffer4_receive
-located in bootp_callouts.cc.
+and @ref pkt4_receive located in bootp_callouts.cc.
 
 If the receive query has to dhcp-message-type option then it is a BOOTP
 one: the BOOTP client class and a DHCPREQUEST dhcp-message-type option

src/hooks/dhcp/bootp/bootp_callouts.cc

@@ -20,11 +20,48 @@ using namespace isc::log;
 // issues related to namespaces.
 extern "C" {
 
-/// @brief This callout is called at the "pkt4_receive" hook.
+/// @brief This callout is called at the "buffer4_receive" hook.
 ///
 /// Ignore DHCP and BOOTREPLY messages.
-/// Remaining packets should be BOOTP requests so add the BOOTP
+/// Remaining packets should be BOOTP requests so add the BOOTP client class,
-/// client class and a DHCPREQUEST dhcp-message-type.
+///
+/// @param handle CalloutHandle.
+///
+/// @return 0 upon success, non-zero otherwise.
+int buffer4_receive(CalloutHandle& handle) {
+    // Get the received unpacked message.
+    Pkt4Ptr query;
+    handle.getArgument("query4", query);
+
+    try {
+        // Take a copy and unpack it.
+        Pkt4Ptr copy(new Pkt4(&query->data_[0], query->data_.size()));
+        copy->unpack();
+
+        if (copy->getType() != DHCP_NOTYPE) {
+            // DHCP query.
+            return (0);
+        }
+        if (copy->getOp() == BOOTREPLY) {
+            // BOOTP response.
+            return (0);
+        }
+        // BOOTP query.
+        query->addClass("BOOTP");
+        LOG_DEBUG(bootp_logger, DBGLVL_TRACE_BASIC, BOOTP_ADDED_CLASS)
+            .arg(query->getLabel());
+    } catch (const std::exception&) {
+        // Got an error. The query shall very likely be dropped later.
+        // Note this covers malformed DHCP packets where the message
+        // type option can't be unpacked, and RFC 951 BOOTP.
+    }
+
+    return (0);
+}
+
+/// @brief This callout is called at the "pkt4_receive" hook.
+///
+/// If in the BOOTP class add a DHCPREQUEST dhcp-message-type.
 ///
 /// @param handle CalloutHandle.
 ///
@@ -35,25 +72,14 @@ int pkt4_receive(CalloutHandle& handle) {
     handle.getArgument("query4", query);
 
     try {
-        if (query->getType() != DHCP_NOTYPE) {
+        if (!query->inClass("BOOTP")) {
-            // DHCP query.
             return (0);
         }
-        if (query->getOp() == BOOTREPLY) {
-            // BOOTP response.
-            return (0);
-        }
-        // BOOTP query.
-        query->addClass("BOOTP");
         query->setType(DHCPREQUEST);
-        LOG_DEBUG(bootp_logger, DBGLVL_TRACE_BASIC, BOOTP_PROCESSED)
+        LOG_DEBUG(bootp_logger, DBGLVL_TRACE_BASIC, BOOTP_ADDED_MSGTYPE)
             .arg(query->getLabel());
-    } catch (const std::exception& ex) {
+    } catch (const std::exception&) {
-        // Got an error (should not happen). The query shall very likely
+        // Got an error (should not happen).
-        // be dropped later.
-        LOG_ERROR(bootp_logger, BOOTP_PROCESS_ERROR)
-            .arg(query->getLabel())
-            .arg(ex.what());
     }
 
     return (0);

src/hooks/dhcp/bootp/bootp_messages.cc

@@ -1,20 +1,20 @@
-// File created from ../../../../src/hooks/dhcp/bootp/bootp_messages.mes on Mon Nov 18 2019 09:30
+// File created from ../../../../src/hooks/dhcp/bootp/bootp_messages.mes on Fri Nov 22 2019 01:05
 
 #include <cstddef>
 #include <log/message_types.h>
 #include <log/message_initializer.h>
 
+extern const isc::log::MessageID BOOTP_ADDED_CLASS = "BOOTP_ADDED_CLASS";
+extern const isc::log::MessageID BOOTP_ADDED_MSGTYPE = "BOOTP_ADDED_MSGTYPE";
 extern const isc::log::MessageID BOOTP_LOAD = "BOOTP_LOAD";
-extern const isc::log::MessageID BOOTP_PROCESSED = "BOOTP_PROCESSED";
-extern const isc::log::MessageID BOOTP_PROCESS_ERROR = "BOOTP_PROCESS_ERROR";
 extern const isc::log::MessageID BOOTP_UNLOAD = "BOOTP_UNLOAD";
 
 namespace {
 
 const char* values[] = {
+    "BOOTP_ADDED_CLASS", "added BOOTP class to a BOOTP query: %1",
+    "BOOTP_ADDED_MSGTYPE", "added DHCPREQUEST message type to a BOOTP query: %1",
     "BOOTP_LOAD", "Bootp hooks library has been loaded",
-    "BOOTP_PROCESSED", "processed BOOTP query: %1",
-    "BOOTP_PROCESS_ERROR", "%1: failed to process packet: %2",
     "BOOTP_UNLOAD", "Bootp hooks library has been unloaded",
     NULL
 };

src/hooks/dhcp/bootp/bootp_messages.h

@@ -1,13 +1,13 @@
-// File created from ../../../../src/hooks/dhcp/bootp/bootp_messages.mes on Mon Nov 18 2019 09:30
+// File created from ../../../../src/hooks/dhcp/bootp/bootp_messages.mes on Fri Nov 22 2019 01:05
 
 #ifndef BOOTP_MESSAGES_H
 #define BOOTP_MESSAGES_H
 
 #include <log/message_types.h>
 
+extern const isc::log::MessageID BOOTP_ADDED_CLASS;
+extern const isc::log::MessageID BOOTP_ADDED_MSGTYPE;
 extern const isc::log::MessageID BOOTP_LOAD;
-extern const isc::log::MessageID BOOTP_PROCESSED;
-extern const isc::log::MessageID BOOTP_PROCESS_ERROR;
 extern const isc::log::MessageID BOOTP_UNLOAD;
 
 #endif // BOOTP_MESSAGES_H

src/hooks/dhcp/bootp/bootp_messages.mes

@@ -1,16 +1,17 @@
 # Copyright (C) 2019 Internet Systems Consortium, Inc. ("ISC")
 
+% BOOTP_ADDED_CLASS added BOOTP class to a BOOTP query: %1
+This debug message is printed when the BOOTP client class was added to
+a BOOTP query.  The query client and transaction identification are
+displayed.
+
+% BOOTP_ADDED_MSGTYPE added DHCPREQUEST message type to a BOOTP query: %1
+This debug message is printed when the DHCPREQUEST message type was
+added to a BOOTP query.  The query client and transaction
+identification are displayed.
+
 % BOOTP_LOAD Bootp hooks library has been loaded
 This info message indicates that the Bootp hooks library has been loaded.
 
-% BOOTP_PROCESSED processed BOOTP query: %1
-This debug message is printed when a BOOTP query was processed.
-The query client and transaction identification are displayed.
-
-% BOOTP_PROCESS_ERROR %1: failed to process packet: %2
-This error message indicates an error during processing of a packet.
-The first argument contains the client and transaction identification
-information.  The second argument includes the details of the error.
-
 % BOOTP_UNLOAD Bootp hooks library has been unloaded
 This info message indicates that the Bootp hooks library has been unloaded.

src/hooks/dhcp/bootp/tests/bootp_unittests.cc

@@ -20,8 +20,10 @@ using namespace isc;
 using namespace isc::bootp;
 using namespace isc::dhcp;
 using namespace isc::hooks;
+using namespace isc::util;
 
 extern "C" {
+extern int buffer4_receive(CalloutHandle& handle);
 extern int pkt4_receive(CalloutHandle& handle);
 }
 
@@ -46,31 +48,46 @@ public:
         return(co_manager_);
     }
 
-    /// @brief Tests pkt4_receive callout.
+    /// @brief Tests buffer4_receive and pkt4_receive callout.
     ///
     /// @param pkt The packet to submit.
     /// @param processed True if the packet must be processed, false otherwise.
-    void pkt4_receiveCall(Pkt4Ptr& pkt, bool processed) {
+    void calloutsCall(Pkt4Ptr& pkt, bool processed) {
         // Get callout handle.
         CalloutHandle handle(getCalloutManager());
 
+        // Fill data so it becomes possible to unpack a copy of it.
+        ASSERT_NO_THROW(pkt->pack());
+        const OutputBuffer& buffer = pkt->getBuffer();
+        pkt->data_.resize(buffer.getLength());
+        memmove(&pkt->data_[0], buffer.getData(), pkt->data_.size());
+
         // Set query.
         handle.setArgument("query4", pkt);
 
         // Get type.
         uint8_t type = pkt->getType();
 
-        // Execute pkt4_receive callout.
+        // Execute buffer4_receive callout.
         int ret;
-        ASSERT_NO_THROW(ret = pkt4_receive(handle));
+        ASSERT_NO_THROW(ret = buffer4_receive(handle));
         EXPECT_EQ(0, ret);
 
         // Verify processing.
         if (processed) {
             EXPECT_TRUE(pkt->inClass("BOOTP"));
-            EXPECT_EQ(DHCPREQUEST, pkt->getType());
         } else {
             EXPECT_FALSE(pkt->inClass("BOOTP"));
+        }
+
+        // Execute pkt4_receive callout.
+        ASSERT_NO_THROW(ret = pkt4_receive(handle));
+        EXPECT_EQ(0, ret);
+
+        // Verify processing.
+        if (processed) {
+            EXPECT_EQ(DHCPREQUEST, pkt->getType());
+        } else {
             EXPECT_EQ(type, pkt->getType());
         }
     }
@@ -82,19 +99,19 @@ public:
 // Verifies that DHCPDISCOVER goes through unmodified.
 TEST_F(BootpTest, dhcpDiscover) {
     Pkt4Ptr pkt(new Pkt4(DHCPDISCOVER, 12345));
-    pkt4_receiveCall(pkt, false);
+    calloutsCall(pkt, false);
 }
 
 // Verifies that DHCPREQUEST goes through unmodified.
 TEST_F(BootpTest, dhcpRequest) {
     Pkt4Ptr pkt(new Pkt4(DHCPREQUEST, 12345));
-    pkt4_receiveCall(pkt, false);
+    calloutsCall(pkt, false);
 }
 
 // Verifies that DHCPOFFER goes through unmodified.
 TEST_F(BootpTest, dhcpOffer) {
     Pkt4Ptr pkt(new Pkt4(DHCPOFFER, 12345));
-    pkt4_receiveCall(pkt, false);
+    calloutsCall(pkt, false);
 }
 
 // Verifies that BOOTREPLY goes through unmodified.
@@ -104,7 +121,7 @@ TEST_F(BootpTest, bootReply) {
     pkt->setType(DHCP_NOTYPE);
     pkt->delOption(DHO_DHCP_MESSAGE_TYPE);
     ASSERT_EQ(BOOTREPLY, pkt->getOp());
-    pkt4_receiveCall(pkt, false);
+    calloutsCall(pkt, false);
 }
 
 // Verifies that BOOTREQUEST is recognized and processed.
@@ -114,7 +131,7 @@ TEST_F(BootpTest, bootRequest) {
     pkt->setType(DHCP_NOTYPE);
     pkt->delOption(DHO_DHCP_MESSAGE_TYPE);
     ASSERT_EQ(BOOTREQUEST, pkt->getOp());
-    pkt4_receiveCall(pkt, true);
+    calloutsCall(pkt, true);
 }
 
 } // end of anonymous namespace

src/hooks/dhcp/high_availability/tests/ha_impl_unittest.cc

@@ -210,8 +210,9 @@ TEST_F(HAImplTest, buffer4Receive) {
 
     // The client class should be assigned to the message to indicate that the
     // server1 should process this message.
-    ASSERT_EQ(1, query4->getClasses().size());
+    ASSERT_EQ(2, query4->getClasses().size());
-    EXPECT_EQ("HA_server1", *(query4->getClasses().cbegin()));
+    EXPECT_TRUE(query4->inClass("ALL"));
+    EXPECT_TRUE(query4->inClass("HA_server1"));
 
     // Check that the message has been parsed. The DHCP message type should
     // be set in this case.
@@ -298,8 +299,9 @@ TEST_F(HAImplTest, buffer6Receive) {
 
     // The client class should be assigned to the message to indicate that the
     // server1 should process this message.
-    ASSERT_EQ(1, query6->getClasses().size());
+    ASSERT_EQ(2, query6->getClasses().size());
-    EXPECT_EQ("HA_server1", *(query6->getClasses().cbegin()));
+    EXPECT_TRUE(query6->inClass("ALL"));
+    EXPECT_TRUE(query6->inClass("HA_server1"));
 
     // Check that the message has been parsed. The DHCP message type should
     // be set in this case.

src/lib/dhcp/pkt.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2014-2018 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2014-2019 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -98,6 +98,10 @@ Pkt::inClass(const std::string& client_class) {
 
 void
 Pkt::addClass(const std::string& client_class, bool required) {
+    // Always have ALL first.
+    if (classes_.empty()) {
+        classes_.insert("ALL");
+    }
     ClientClasses& classes = !required ? classes_ : required_classes_;
     if (!classes.contains(client_class)) {
         classes.insert(client_class);