[4626] Unit-tests for classification + fixed fields in DHCPv4 implemented.

src/bin/dhcp4/dhcp4_srv.cc

@@ -1932,6 +1932,10 @@ Dhcpv4Srv::setFixedFields(Dhcpv4Exchange& ex) {
     Pkt4Ptr query = ex.getQuery();
     Pkt4Ptr response = ex.getResponse();
 
+    // Step 1: try to set values using HR.
+    /// @todo: Merge Marcin's code here.
+
+    // Step 2: if step 1 failed, try to set the values based on classes.
     const ClientClasses classes = query->getClasses();
     if (classes.empty()) {
         return;
@@ -1970,8 +1974,13 @@ Dhcpv4Srv::setFixedFields(Dhcpv4Exchange& ex) {
             response->setFile(&filename[0], filename.size());
         }
     }
-}
 
+    // Step 3: Finally, set the values based on subnet values.
+    /// @todo implement this.
+
+    /// @todo: We need to implement some kind of logic here that only
+    /// the values that are not set yet in previous steps are overwritten.
+}
 
 OptionPtr
 Dhcpv4Srv::getNetmaskOption(const Subnet4Ptr& subnet) {

src/bin/dhcp4/tests/Makefile.am

@@ -78,6 +78,7 @@ dhcp4_unittests_SOURCES += dhcp4_srv_unittest.cc
 dhcp4_unittests_SOURCES += dhcp4_test_utils.cc dhcp4_test_utils.h
 dhcp4_unittests_SOURCES += direct_client_unittest.cc
 dhcp4_unittests_SOURCES += ctrl_dhcp4_srv_unittest.cc
+dhcp4_unittests_SOURCES += classify_unittest.cc
 dhcp4_unittests_SOURCES += config_parser_unittest.cc
 dhcp4_unittests_SOURCES += fqdn_unittest.cc
 dhcp4_unittests_SOURCES += marker_file.cc

src/bin/dhcp4/tests/classify_unittest.cc

@@ -0,0 +1,290 @@
+// Copyright (C) 2016 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
+// file, You can obtain one at http://mozilla.org/MPL/2.0/.
+
+#include <config.h>
+#include <asiolink/io_address.h>
+#include <dhcp/dhcp4.h>
+#include <dhcp/tests/iface_mgr_test_config.h>
+#include <dhcp4/tests/dhcp4_client.h>
+#include <dhcp/option_int.h>
+#include <vector>
+
+using namespace isc;
+using namespace isc::asiolink;
+using namespace isc::dhcp;
+using namespace isc::dhcp::test;
+using namespace std;
+
+namespace {
+
+/// @brief Set of JSON configurations used throughout the DORA tests.
+///
+/// - Configuration 0:
+///   - Used for testing direct traffic
+///   - 1 subnet: 10.0.0.0/24
+///   - 1 pool: 10.0.0.10-10.0.0.100
+///   - Router option present: 10.0.0.200 and 10.0.0.201
+///   - Domain Name Server option present: 10.0.0.202, 10.0.0.203.
+///   - Log Servers option present: 192.0.2.200 and 192.0.2.201
+///   - Quotes Servers option present: 192.0.2.202, 192.0.2.203.
+///   - no classes
+///
+/// - Configuration 1:
+///   - The same as configuration 0, but has the following classes defined:
+///     option[93].hex == 0x0009, next-server set to 1.2.3.4
+///     option[93].hex == 0x0007, set server-hostname to deneb
+///     option[93].hex == 0x0006, set boot-file-name to pxelinux.0
+///     option[93].hex == 0x0001, set boot-file-name to ipxe.efi
+const char* CONFIGS[] = {
+// Configuration 0
+    "{ \"interfaces-config\": {"
+        "      \"interfaces\": [ \"*\" ]"
+        "},"
+        "\"valid-lifetime\": 600,"
+        "\"subnet4\": [ { "
+        "    \"subnet\": \"10.0.0.0/24\", "
+        "    \"id\": 1,"
+        "    \"pools\": [ { \"pool\": \"10.0.0.10-10.0.0.100\" } ],"
+        "    \"option-data\": [ {"
+        "        \"name\": \"routers\","
+        "        \"data\": \"10.0.0.200,10.0.0.201\""
+        "    },"
+        "    {"
+        "        \"name\": \"domain-name-servers\","
+        "        \"data\": \"10.0.0.202,10.0.0.203\""
+        "    },"
+        "    {"
+        "        \"name\": \"log-servers\","
+        "        \"data\": \"10.0.0.200,10.0.0.201\""
+        "    },"
+        "    {"
+        "        \"name\": \"cookie-servers\","
+        "        \"data\": \"10.0.0.202,10.0.0.203\""
+        "    } ]"
+        " } ]"
+    "}",
+
+    // Configuration 6
+    "{ \"interfaces-config\": {"
+        "   \"interfaces\": [ \"*\" ]"
+        "},"
+        "\"valid-lifetime\": 600,"
+        "\"client-classes\": ["
+        "{"
+        "   \"name\": \"pxe1\","
+        "   \"test\": \"option[93].hex == 0x0009\","
+        "   \"next-server\": \"1.2.3.4\""
+        "},"
+        "{"
+        "   \"name\": \"pxe2\","
+        "   \"test\": \"option[93].hex == 0x0007\","
+        "   \"server-hostname\": \"deneb\""
+        "},"
+        "{"
+        "   \"name\": \"pxe3\","
+        "   \"test\": \"option[93].hex == 0x0006\","
+        "   \"boot-file-name\": \"pxelinux.0\""
+        "},"
+        "{"
+        "   \"name\": \"pxe4\","
+        "   \"test\": \"option[93].hex == 0x0001\","
+        "   \"boot-file-name\": \"ipxe.efi\""
+        "},"
+        "],"
+        "\"subnet4\": [ { "
+        "    \"subnet\": \"10.0.0.0/24\", "
+        "    \"id\": 1,"
+        "    \"pools\": [ { \"pool\": \"10.0.0.10-10.0.0.100\" } ]"
+        " } ]"
+    "}"
+
+};
+
+/// @brief Test fixture class for testing classification.
+///
+/// For the time being it covers only fixed fields, but it's going to be
+/// expanded to cover other cases.
+class ClassifyTest : public Dhcpv4SrvTest {
+public:
+
+    /// @brief Constructor.
+    ///
+    /// Sets up fake interfaces.
+    ClassifyTest()
+        : Dhcpv4SrvTest(),
+          iface_mgr_test_config_(true) {
+        IfaceMgr::instance().openSockets4();
+    }
+
+    /// @brief Desctructor.
+    ///
+    ~ClassifyTest() {
+    }
+
+    void
+    testFixedFields(const char* config, uint8_t msgtype, const OptionPtr& extra_opt,
+                    const std::string& exp_next_server, const std::string& exp_sname,
+                    const std::string& exp_filename) {
+         Dhcp4Client client(Dhcp4Client::SELECTING);
+
+         // Configure DHCP server.
+         configure(config, *client.getServer());
+
+         if (extra_opt) {
+             client.addExtraOption(extra_opt);
+         }
+
+         switch (msgtype) {
+         case DHCPDISCOVER:
+             client.doDiscover();
+             break;
+         case DHCPREQUEST:
+             client.doDORA();
+             break;
+         case DHCPINFORM:
+             // Preconfigure the client with the IP address.
+             client.createLease(IOAddress("10.0.0.56"), 600);
+
+             client.doInform(false);
+             break;
+         }
+
+         ASSERT_TRUE(client.getContext().response_);
+         Pkt4Ptr resp = client.getContext().response_;
+
+         EXPECT_EQ(exp_next_server, resp->getSiaddr().toText());
+
+         // This is bizarre. If I use Pkt4::MAX_SNAME_LEN in the ASSERT_GE macro,
+         // the linker will complain about it being not defined.
+         const size_t max_sname = Pkt4::MAX_SNAME_LEN;
+
+         ASSERT_GE(max_sname, exp_sname.length());
+         vector<uint8_t> sname(max_sname, 0);
+         memcpy(&sname[0], &exp_sname[0], exp_sname.size());
+         EXPECT_EQ(sname, resp->getSname());
+
+         const size_t max_filename = Pkt4::MAX_FILE_LEN;
+         ASSERT_GE(max_filename, exp_filename.length());
+         vector<uint8_t> filename(max_filename, 0);
+         memcpy(&filename[0], &exp_filename[0], exp_filename.size());
+         EXPECT_EQ(filename, resp->getFile());
+    }
+
+    /// @brief Interface Manager's fake configuration control.
+    IfaceMgrTestConfig iface_mgr_test_config_;
+};
+
+
+// This test checks that an incoming DISCOVER that does not match any classes
+// will get the fixed fields empty.
+TEST_F(ClassifyTest, fixedFieldsDiscoverNoClasses) {
+    testFixedFields(CONFIGS[1], DHCPDISCOVER, OptionPtr(), "0.0.0.0", "", "");
+}
+// This test checks that an incoming REQUEST that does not match any classes
+// will get the fixed fields empty.
+TEST_F(ClassifyTest, fixedFieldsRequestNoClasses) {
+    testFixedFields(CONFIGS[1], DHCPREQUEST, OptionPtr(), "0.0.0.0", "", "");
+}
+// This test checks that an incoming INFORM that does not match any classes
+// will get the fixed fields empty.
+TEST_F(ClassifyTest, fixedFieldsInformNoClasses) {
+    testFixedFields(CONFIGS[1], DHCPINFORM, OptionPtr(), "0.0.0.0", "", "");
+}
+
+
+// This test checks that an incoming DISCOVER that does match a class that has
+// next-server specified will result in a response that has the next-server set.
+TEST_F(ClassifyTest, fixedFieldsDiscoverNextServer) {
+    OptionPtr pxe(new OptionInt<uint16_t>(Option::V4, 93, 0x0009));
+
+    testFixedFields(CONFIGS[1], DHCPDISCOVER, pxe, "1.2.3.4", "", "");
+}
+// This test checks that an incoming REQUEST that does match a class that has
+// next-server specified will result in a response that has the next-server set.
+TEST_F(ClassifyTest, fixedFieldsRequestNextServer) {
+    OptionPtr pxe(new OptionInt<uint16_t>(Option::V4, 93, 0x0009));
+
+    testFixedFields(CONFIGS[1], DHCPREQUEST, pxe, "1.2.3.4", "", "");
+}
+// This test checks that an incoming INFORM that does match a class that has
+// next-server specified will result in a response that has the next-server set.
+TEST_F(ClassifyTest, fixedFieldsInformNextServer) {
+    OptionPtr pxe(new OptionInt<uint16_t>(Option::V4, 93, 0x0009));
+
+    testFixedFields(CONFIGS[1], DHCPINFORM, pxe, "1.2.3.4", "", "");
+}
+
+
+// This test checks that an incoming DISCOVER that does match a class that has
+// server-hostname specified will result in a response that has the sname field set.
+TEST_F(ClassifyTest, fixedFieldsDiscoverHostname) {
+    OptionPtr pxe(new OptionInt<uint16_t>(Option::V4, 93, 0x0007));
+
+    testFixedFields(CONFIGS[1], DHCPDISCOVER, pxe, "0.0.0.0", "deneb", "");
+}
+// This test checks that an incoming REQUEST that does match a class that has
+// server-hostname specified will result in a response that has the sname field set.
+TEST_F(ClassifyTest, fixedFieldsRequestHostname) {
+    OptionPtr pxe(new OptionInt<uint16_t>(Option::V4, 93, 0x0007));
+
+    testFixedFields(CONFIGS[1], DHCPREQUEST, pxe, "0.0.0.0", "deneb", "");
+}
+// This test checks that an incoming INFORM that does match a class that has
+// server-hostname specified will result in a response that has the sname field set.
+TEST_F(ClassifyTest, fixedFieldsInformHostname) {
+    OptionPtr pxe(new OptionInt<uint16_t>(Option::V4, 93, 0x0007));
+
+    testFixedFields(CONFIGS[1], DHCPINFORM, pxe, "0.0.0.0", "deneb", "");
+}
+
+
+// This test checks that an incoming DISCOVER that does match a class that has
+// boot-file-name specified will result in a response that has the filename field set.
+TEST_F(ClassifyTest, fixedFieldsDiscoverFile1) {
+    OptionPtr pxe(new OptionInt<uint16_t>(Option::V4, 93, 0x0006));
+
+    testFixedFields(CONFIGS[1], DHCPDISCOVER, pxe, "0.0.0.0", "", "pxelinux.0");
+}
+// This test checks that an incoming REQUEST that does match a class that has
+// boot-file-name specified will result in a response that has the filename field set.
+TEST_F(ClassifyTest, fixedFieldsRequestFile1) {
+    OptionPtr pxe(new OptionInt<uint16_t>(Option::V4, 93, 0x0006));
+
+    testFixedFields(CONFIGS[1], DHCPREQUEST, pxe, "0.0.0.0", "", "pxelinux.0");
+}
+// This test checks that an incoming INFORM that does match a class that has
+// boot-file-name specified will result in a response that has the filename field set.
+TEST_F(ClassifyTest, fixedFieldsInformFile1) {
+    OptionPtr pxe(new OptionInt<uint16_t>(Option::V4, 93, 0x0006));
+
+    testFixedFields(CONFIGS[1], DHCPDISCOVER, pxe, "0.0.0.0", "", "pxelinux.0");
+}
+
+
+// This test checks that an incoming DISCOVER that does match a different class that has
+// boot-file-name specified will result in a response that has the filename field set.
+TEST_F(ClassifyTest, fixedFieldsDiscoverFile2) {
+    OptionPtr pxe(new OptionInt<uint16_t>(Option::V4, 93, 0x0001));
+
+    testFixedFields(CONFIGS[1], DHCPDISCOVER, pxe, "0.0.0.0", "", "ipxe.efi");
+}
+// This test checks that an incoming REQUEST that does match a different class that has
+// boot-file-name specified will result in a response that has the filename field set.
+TEST_F(ClassifyTest, fixedFieldsRequestFile2) {
+    OptionPtr pxe(new OptionInt<uint16_t>(Option::V4, 93, 0x0001));
+
+    testFixedFields(CONFIGS[1], DHCPREQUEST, pxe, "0.0.0.0", "", "ipxe.efi");
+}
+// This test checks that an incoming INFORM that does match a different class that has
+// boot-file-name specified will result in a response that has the filename field set.
+TEST_F(ClassifyTest, fixedFieldsInformFile2) {
+    OptionPtr pxe(new OptionInt<uint16_t>(Option::V4, 93, 0x0001));
+
+    testFixedFields(CONFIGS[1], DHCPINFORM, pxe, "0.0.0.0", "", "ipxe.efi");
+}
+
+
+} // end of anonymous namespace

src/lib/dhcpsrv/parsers/client_class_def_parser.cc

@@ -78,10 +78,6 @@ ClientClassDefParser::ClientClassDefParser(const std::string&,
 void
 ClientClassDefParser::build(ConstElementPtr class_def_cfg) {
 
-    IOAddress next_server("0.0.0.0");
-    std::vector<uint8_t> sname;
-    std::vector<uint8_t> filename;
-
     // Parse the elements that make up the option definition.
     BOOST_FOREACH(ConfigPair param, class_def_cfg->mapValue()) {
         std::string entry(param.first);
@@ -102,35 +98,15 @@ ClientClassDefParser::build(ConstElementPtr class_def_cfg) {
             opts_parser.reset(new OptionDataListParser(entry, options_, family));
             parser = opts_parser;
         } else if (entry == "next-server") {
-            // Let's parse the next-server field
-            try {
-                next_server = IOAddress(param.second->stringValue());
-            } catch (const IOError& ex) {
-                isc_throw(DhcpConfigError, "Invalid next-server value specified: '"
-                          << param.second << "'");
-            }
-            if (next_server.getFamily() != AF_INET) {
-                isc_throw(DhcpConfigError, "Invalid next-server value: '"
-                          << param.second << "', must be IPv4 address");
-            }
+            StringParserPtr str_parser(new StringParser(entry, string_values_));
+            parser = str_parser;
         } else if (entry == "server-hostname") {
-            string tmp = param.second->stringValue();
-            if (tmp.length() > Pkt4::MAX_SNAME_LEN) {
-                isc_throw(DhcpConfigError, "server-hostname must be at most "
-                          << Pkt4::MAX_SNAME_LEN << " bytes long, it is "
-                          << tmp.length());
-            }
-            sname = vector<uint8_t>(tmp.begin(), tmp.end());
+            StringParserPtr str_parser(new StringParser(entry, string_values_));
+            parser = str_parser;
 
         } else if (entry == "boot-file-name") {
-            string tmp = param.second->stringValue();
-            if (tmp.length() > Pkt4::MAX_FILE_LEN) {
-                isc_throw(DhcpConfigError, "boot-file-name must be at most "
-                          << Pkt4::MAX_FILE_LEN << " bytes long, it is "
-                          << tmp.length());
-            }
-            filename = vector<uint8_t>(tmp.begin(), tmp.end());
-
+            StringParserPtr str_parser(new StringParser(entry, string_values_));
+            parser = str_parser;
         } else {
             isc_throw(DhcpConfigError, "invalid parameter '" << entry
                       << "' (" << param.second->getPosition() << ")");
@@ -148,9 +124,41 @@ ClientClassDefParser::build(ConstElementPtr class_def_cfg) {
                   << class_def_cfg->getPosition() << ")");
     }
 
+    // Let's parse the next-server field
+    IOAddress next_server("0.0.0.0");
+    string next_server_txt = string_values_->getOptionalParam("next-server", "0.0.0.0");
+    try {
+        next_server = IOAddress(next_server_txt);
+    } catch (const IOError& ex) {
+        isc_throw(DhcpConfigError, "Invalid next-server value specified: '"
+                  << next_server_txt << "'");
+    }
+    if (next_server.getFamily() != AF_INET) {
+        isc_throw(DhcpConfigError, "Invalid next-server value: '"
+                  << next_server_txt << "', must be IPv4 address");
+    }
+
+    // Let's try to parse sname
+    string sname_txt = string_values_->getOptionalParam("server-hostname", "");
+    if (sname_txt.length() > Pkt4::MAX_SNAME_LEN) {
+        isc_throw(DhcpConfigError, "server-hostname must be at most "
+                  << Pkt4::MAX_SNAME_LEN << " bytes long, it is "
+                  << sname_txt.length());
+    }
+    std::vector<uint8_t> sname(sname_txt.begin(), sname_txt.end());
+
+    string file_txt = string_values_->getOptionalParam("boot-file-name", "");
+    if (file_txt.length() > Pkt4::MAX_FILE_LEN) {
+        isc_throw(DhcpConfigError, "boot-file-name must be at most "
+                  << Pkt4::MAX_FILE_LEN << " bytes long, it is "
+                  << file_txt.length());
+    }
+    std::vector<uint8_t> filename(file_txt.begin(), file_txt.end());
+
     try {
         // an OptionCollectionPtr
-        class_dictionary_->addClass(name, match_expr_, options_);
+        class_dictionary_->addClass(name, match_expr_, options_, next_server,
+                                    sname, filename);
     } catch (const std::exception& ex) {
         isc_throw(DhcpConfigError, ex.what()
                   << " (" << class_def_cfg->getPosition() << ")");