[#1672] class fixed field order now matches class option order in kea-dhc4

src/bin/dhcp4/dhcp4_srv.cc
    Dhcpv4Srv::setFixedFields - modified to use the value for a field
    from the first class in query's list of classes that specifies
    the field.

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
    TEST_F(Dhcpv4SrvTest, fixedFieldsInClassOrder) - new unit test

diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc
index 20eae0142b9f0a74b972df6b3cba311aa35e8002..a4bdfc4014dcb86e064a6c37310b0e0a2fe82b6d 100644 (file)
--- a/src/bin/dhcp4/dhcp4_srv.cc
+++ b/src/bin/dhcp4/dhcp4_srv.cc
@@ -1678,10 +1678,12 @@ Dhcpv4Srv::buildCfgOptionList(Dhcpv4Exchange& ex) {
             // Skip it
             continue;
         }
+
         if (ccdef->getCfgOption()->empty()) {
             // Skip classes which don't configure options
             continue;
         }
+
         co_list.push_back(ccdef->getCfgOption());
     }
 
@@ -2916,8 +2918,13 @@ Dhcpv4Srv::setFixedFields(Dhcpv4Exchange& ex) {
 
         // Now we need to iterate over the classes assigned to the
         // query packet and find corresponding class definitions for it.
+        // We want the first value found for each field.
+        IOAddress next_server("0.0.0.0");
+        string sname;
+        string filename;
+        size_t found_cnt = 0;
         for (ClientClasses::const_iterator name = classes.cbegin();
-             name != classes.cend(); ++name) {
+             name != classes.cend() && found_cnt < 3; ++name) {
 
             ClientClassDefPtr cl = dict->findClass(*name);
             if (!cl) {
@@ -2928,31 +2935,40 @@ Dhcpv4Srv::setFixedFields(Dhcpv4Exchange& ex) {
                 continue;
             }
 
-            IOAddress next_server = cl->getNextServer();
-            if (!next_server.isV4Zero()) {
-                response->setSiaddr(next_server);
+            if (next_server == IOAddress::IPV4_ZERO_ADDRESS()) {
+                next_server = cl->getNextServer();
+                if (!next_server.isV4Zero()) {
+                    response->setSiaddr(next_server);
+                    found_cnt++;
+                }
             }
 
-            const string& sname = cl->getSname();
-            if (!sname.empty()) {
-                // Converting string to (const uint8_t*, size_t len) format is
-                // tricky. reinterpret_cast is not the most elegant solution,
-                // but it does avoid us making unnecessary copy. We will convert
-                // sname and file fields in Pkt4 to string one day and life
-                // will be easier.
-                response->setSname(reinterpret_cast<const uint8_t*>(sname.c_str()),
-                                   sname.size());
+            if (sname.empty()) {
+                sname = cl->getSname();
+                if (!sname.empty()) {
+                    // Converting string to (const uint8_t*, size_t len) format is
+                    // tricky. reinterpret_cast is not the most elegant solution,
+                    // but it does avoid us making unnecessary copy. We will convert
+                    // sname and file fields in Pkt4 to string one day and life
+                    // will be easier.
+                    response->setSname(reinterpret_cast<const uint8_t*>(sname.c_str()),
+                                       sname.size());
+                    found_cnt++;
+                }
             }
 
-            const string& filename = cl->getFilename();
-            if (!filename.empty()) {
-                // Converting string to (const uint8_t*, size_t len) format is
-                // tricky. reinterpret_cast is not the most elegant solution,
-                // but it does avoid us making unnecessary copy. We will convert
-                // sname and file fields in Pkt4 to string one day and life
-                // will be easier.
-                response->setFile(reinterpret_cast<const uint8_t*>(filename.c_str()),
-                                  filename.size());
+            if (filename.empty()) {
+                filename = cl->getFilename();
+                if (!filename.empty()) {
+                    // Converting string to (const uint8_t*, size_t len) format is
+                    // tricky. reinterpret_cast is not the most elegant solution,
+                    // but it does avoid us making unnecessary copy. We will convert
+                    // sname and file fields in Pkt4 to string one day and life
+                    // will be easier.
+                    response->setFile(reinterpret_cast<const uint8_t*>(filename.c_str()),
+                                     filename.size());
+                    found_cnt++;
+                }
             }
         }
     }

diff --git a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
index 7cc946dfed712c5e5e90bb5faf522ca61320f13b..b350498fb175a0649a65132b66e6fa934468b849 100644 (file)
--- a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
+++ b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
@@ -136,6 +136,15 @@ const char* CONFIGS[] = {
     "\"valid-lifetime\": 4000 }",
 };
 
+// Convenience function for comparing option buffer to an expected string value
+// @param exp_string expected string value
+// @param buffer OptionBuffer whose contents are to be tested
+void checkStringInBuffer( const std::string& exp_string, const OptionBuffer& buffer) {
+    std::string buffer_string(buffer.begin(), buffer.end());
+    EXPECT_EQ(exp_string, std::string(buffer_string.c_str()));
+}
+
+
 // This test verifies that the destination address of the response
 // message is set to giaddr, when giaddr is set to non-zero address
 // in the received message.
@@ -4389,6 +4398,157 @@ TEST_F(Dhcpv4SrvTest, userContext) {
     EXPECT_EQ("{ \"value\": 42 }", pools[0]->getContext()->str());
 }
 
+// Verify that fixed fields are set from classes in the same order
+// as class options.
+TEST_F(Dhcpv4SrvTest, fixedFieldsInClassOrder) {
+    IfaceMgrTestConfig test_config(true);
+    IfaceMgr::instance().openSockets4();
+
+    NakedDhcpv4Srv srv(0);
+
+    std::string config = R"(
+    {
+        "interfaces-config": { "interfaces": [ "*" ] },
+        "client-classes": [
+        {
+            "name":"one",
+            "server-hostname": "server_one",
+            "next-server": "192.0.2.111",
+            "boot-file-name":"one.boot",
+            "option-data": [
+            {
+                "name": "domain-name",
+                "data": "one.example.com"
+            }]
+        },
+        {
+            "name":"two",
+            "server-hostname": "server_two",
+            "next-server":"192.0.2.222",
+            "boot-file-name":"two.boot",
+            "option-data": [
+            {
+                "name": "domain-name",
+                "data": "two.example.com"
+            }]
+        },
+        {
+            "name":"next-server-only",
+            "next-server":"192.0.2.100"
+        },
+        {
+            "name":"server-hostname-only",
+            "server-hostname": "server_only"
+        },
+        {
+            "name":"bootfile-only",
+            "boot-file-name": "only.boot"
+        }],
+
+        "subnet4": [
+        {
+            "subnet": "192.0.2.0/24",
+            "pools": [ { "pool": "192.0.2.1 - 192.0.2.100" } ],
+            "reservations": [
+            {
+                "hw-address": "08:00:27:25:d3:01",
+                "client-classes": [ "one", "two" ]
+            },
+            {
+                "hw-address": "08:00:27:25:d3:02",
+                "client-classes": [ "two", "one" ]
+            },
+            {
+                "hw-address": "08:00:27:25:d3:03",
+                "client-classes": [ "server-hostname-only", "bootfile-only", "next-server-only" ]
+            }]
+        }]
+    }
+    )";
+
+    ConstElementPtr json;
+    ASSERT_NO_THROW_LOG(json = parseDHCP4(config));
+    ConstElementPtr status;
+
+    // Configure the server and make sure the config is accepted
+    EXPECT_NO_THROW(status = configureDhcp4Server(srv, json));
+    ASSERT_TRUE(status);
+    comment_ = config::parseAnswer(rcode_, status);
+    ASSERT_EQ(0, rcode_);
+
+    CfgMgr::instance().commit();
+
+    struct Scenario {
+        std::string hw_str_;
+        std::string exp_classes_;
+        std::string exp_server_hostname_;
+        std::string exp_next_server_;
+        std::string exp_bootfile_;
+        std::string exp_domain_name_;
+    };
+
+    const std::vector<Scenario> scenarios = {
+       {
+        "08:00:27:25:d3:01",
+        "ALL, one, two, KNOWN",
+        "server_one",
+        "192.0.2.111",
+        "one.boot",
+        "one.example.com"
+       },
+       {
+        "08:00:27:25:d3:02",
+        "ALL, two, one, KNOWN",
+        "server_two",
+        "192.0.2.222",
+        "two.boot",
+        "two.example.com"
+       },
+       {
+        "08:00:27:25:d3:03",
+        "ALL, server-hostname-only, bootfile-only, next-server-only, KNOWN",
+        "server_only",
+        "192.0.2.100",
+        "only.boot",
+        ""
+       }
+    };
+
+    for (auto scenario : scenarios) {
+        SCOPED_TRACE(scenario.hw_str_); {
+            // Build a DISCOVER
+            Pkt4Ptr query(new Pkt4(DHCPDISCOVER, 1234));
+            query->setRemoteAddr(IOAddress("192.0.2.1"));
+            query->setIface("eth1");
+
+            HWAddrPtr hw_addr(new HWAddr(HWAddr::fromText(scenario.hw_str_, 10)));
+            query->setHWAddr(hw_addr);
+
+            // Process it.
+            Pkt4Ptr response = srv.processDiscover(query);
+
+            // Make sure class list is as expected.
+            ASSERT_EQ(scenario.exp_classes_, query->getClasses().toText());
+
+            // Now check the fixed fields.
+            checkStringInBuffer(scenario.exp_server_hostname_, response->getSname());
+            EXPECT_EQ(scenario.exp_next_server_, response->getSiaddr().toText());
+            checkStringInBuffer(scenario.exp_bootfile_, response->getFile());
+
+            // Check domain name option.
+            OptionPtr opt = response->getOption(DHO_DOMAIN_NAME);
+            if (scenario.exp_domain_name_.empty()) {
+                ASSERT_FALSE(opt);
+            } else {
+                ASSERT_TRUE(opt);
+                OptionStringPtr opstr = boost::dynamic_pointer_cast<OptionString>(opt);
+                ASSERT_TRUE(opstr);
+                EXPECT_EQ(scenario.exp_domain_name_,  opstr->getValue());
+            }
+        }
+    }
+}
+
 /// @todo: Implement proper tests for MySQL lease/host database,
 ///        see ticket #4214.