From: Tomek Mrugalski Date: Wed, 24 Aug 2016 16:51:28 +0000 (+0200) Subject: [4626] Unit-tests for classification + fixed fields in DHCPv4 implemented. X-Git-Tag: trac4631_base~1^2~4 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8209c92960dbf88bdb262edfd14a1fd0fb15750f;p=thirdparty%2Fkea.git [4626] Unit-tests for classification + fixed fields in DHCPv4 implemented. --- diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index 5808f0f0e5..8dae9d5f37 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/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) { diff --git a/src/bin/dhcp4/tests/Makefile.am b/src/bin/dhcp4/tests/Makefile.am index 9f658e14fc..f0b5babb3d 100644 --- a/src/bin/dhcp4/tests/Makefile.am +++ b/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 diff --git a/src/bin/dhcp4/tests/classify_unittest.cc b/src/bin/dhcp4/tests/classify_unittest.cc new file mode 100644 index 0000000000..fcea893146 --- /dev/null +++ b/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 +#include +#include +#include +#include +#include +#include + +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 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 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(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(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(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(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(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(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(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(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(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(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(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(Option::V4, 93, 0x0001)); + + testFixedFields(CONFIGS[1], DHCPINFORM, pxe, "0.0.0.0", "", "ipxe.efi"); +} + + +} // end of anonymous namespace diff --git a/src/lib/dhcpsrv/parsers/client_class_def_parser.cc b/src/lib/dhcpsrv/parsers/client_class_def_parser.cc index b7e40e9690..218457ce85 100644 --- a/src/lib/dhcpsrv/parsers/client_class_def_parser.cc +++ b/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 sname; - std::vector 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(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(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 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 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() << ")");