diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index 20eae0142b..a4bdfc4014 100644 --- 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(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(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(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(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 7cc946dfed..b350498fb1 100644 --- 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 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(opt); + ASSERT_TRUE(opstr); + EXPECT_EQ(scenario.exp_domain_name_, opstr->getValue()); + } + } + } +} + /// @todo: Implement proper tests for MySQL lease/host database, /// see ticket #4214.