From: Tomek Mrugalski Date: Thu, 25 Aug 2016 14:44:55 +0000 (+0200) Subject: [4626] Changes after review X-Git-Tag: trac4631_base~1^2~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ada652bab5133d89db54e135797aa17f6c10d56b;p=thirdparty%2Fkea.git [4626] Changes after review - Dhcp4Srv::vendorClassSpecificProcessing removed - User's Guide updated - disabled obsolete test (will need to be rewritten to take advantage of new classification code) - classes definitions now use strings for server-name and filename - unused configuration removed from unit-tests - textFixedFields is now documented - Unit-tests now have short descriptions --- diff --git a/doc/guide/dhcp4-srv.xml b/doc/guide/dhcp4-srv.xml index f4189cfef4..681481fed9 100644 --- a/doc/guide/dhcp4-srv.xml +++ b/doc/guide/dhcp4-srv.xml @@ -1745,12 +1745,11 @@ It is merely echoed by the server - For clients that belong to the VENDOR_CLASS_docsis3.0 class, the siaddr - field is set to the value of next-server (if specified in a subnet). If - there is a boot-file-name option specified, its value is also set in the - file field in the DHCPv4 packet. For eRouter1.0 class, the siaddr is - always set to 0.0.0.0. That capability is expected to be moved to - an external hook library that will be dedicated to cable modems. + Note: Kea 1.0 and earlier versions performed special actions for + clients that were in VENDOR_CLASS_docsis3.0. This is no longer the + case in Kea 1.1 and newer. In those newer versions the old behavior + can be achieved by defining VENDOR_CLASS_docsis3.0 and setting + its next-server and boot-file-name values appropriately. diff --git a/src/bin/dhcp4/dhcp4_messages.mes b/src/bin/dhcp4/dhcp4_messages.mes index 708a9256da..a8aa19fa30 100644 --- a/src/bin/dhcp4/dhcp4_messages.mes +++ b/src/bin/dhcp4/dhcp4_messages.mes @@ -233,11 +233,6 @@ information. The second and third argument contains the packet name and type respectively. The fourth argument contains detailed packet information. -% DHCP4_DISCOVER_CLASS_PROCESSING_FAILED %1: client class specific processing failed for DHCPDISCOVER -This debug message means that the server processing that is unique for each -client class has reported a failure. The response packet will not be sent. -The argument holds the client and transaction identification information. - % DHCP4_DYNAMIC_RECONFIGURATION initiate server reconfiguration using file: %1, after receiving SIGHUP signal This is the info message logged when the DHCPv4 server starts reconfiguration as a result of receiving SIGHUP signal. @@ -314,12 +309,6 @@ will be only able to offer global options - no addresses will be assigned. The argument specifies the client and transaction identification information. -% DHCP4_INFORM_CLASS_PROCESSING_FAILED %1: client class specific processing failed for DHCPINFORM -This debug message means that the server processing that is unique for each -client class has reported a failure. The response packet will not be sent. -The argument specifies the client and the transaction identification -information. - % DHCP4_INFORM_DIRECT_REPLY %1: DHCPACK in reply to the DHCPINFORM will be sent directly to %2 over %3 This debug message is issued when the DHCPACK will be sent directly to the client, rather than via a relay. The first argument contains the client @@ -615,12 +604,6 @@ and the lease. The first argument includes the client and the transaction identification information. The second argument specifies the leased address. -% DHCP4_REQUEST_CLASS_PROCESSING_FAILED %1: client class specific processing failed for DHCPREQUEST -This debug message means that the server processing that is unique for each -client class has reported a failure. The response packet will not be sent. -The argument contains the client and transaction identification -information. - % DHCP4_RESPONSE_DATA %1: responding with packet %2 (type %3), packet details: %4 A debug message including the detailed data about the packet being sent to the client. The first argument contains the client and the transaction diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index 8dae9d5f37..18d3923794 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -1964,14 +1964,26 @@ Dhcpv4Srv::setFixedFields(Dhcpv4Exchange& ex) { response->setSiaddr(next_server); } - const vector& sname = cl->second->getSname(); + const string& sname = cl->second->getSname(); if (!sname.empty()) { - response->setSname(&sname[0], sname.size()); + // Converting string to (const uint8_t*, size_t len) format is + // tricky. reineterpret_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 live + // will be easier. + response->setSname(reinterpret_cast(sname.c_str()), + sname.size()); } - const vector& filename = cl->second->getFilename(); + const string& filename = cl->second->getFilename(); if (!filename.empty()) { - response->setFile(&filename[0], filename.size()); + // Converting string to (const uint8_t*, size_t len) format is + // tricky. reineterpret_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 live + // will be easier. + response->setFile(reinterpret_cast(filename.c_str()), + filename.size()); } } @@ -2021,8 +2033,8 @@ Dhcpv4Srv::processDiscover(Pkt4Ptr& discover) { // them we append them for him. appendBasicOptions(ex); - // See if the class mandates setting any fixed fields (siaddr, sname, - // filename). + // Set fixed fields (siaddr, sname, filename) if defined in + // the reservation, class or subnet specific configuration. setFixedFields(ex); } else { @@ -2037,14 +2049,6 @@ Dhcpv4Srv::processDiscover(Pkt4Ptr& discover) { appendServerID(ex); - /// @todo: decide whether we want to add a new hook point for - /// doing class specific processing. - if (!vendorClassSpecificProcessing(ex)) { - /// @todo add more verbosity here - LOG_DEBUG(options4_logger, DBG_DHCP4_DETAIL, DHCP4_DISCOVER_CLASS_PROCESSING_FAILED) - .arg(discover->getLabel()); - } - return (ex.getResponse()); } @@ -2081,8 +2085,8 @@ Dhcpv4Srv::processRequest(Pkt4Ptr& request) { // them we append them for him. appendBasicOptions(ex); - // See if the class mandates setting any fixed fields (siaddr, sname, - // filename). + // Set fixed fields (siaddr, sname, filename) if defined in + // the reservation, class or subnet specific configuration. setFixedFields(ex); } @@ -2092,14 +2096,6 @@ Dhcpv4Srv::processRequest(Pkt4Ptr& request) { appendServerID(ex); - /// @todo: decide whether we want to add a new hook point for - /// doing class specific processing. - if (!vendorClassSpecificProcessing(ex)) { - /// @todo add more verbosity here - LOG_DEBUG(options4_logger, DBG_DHCP4_DETAIL, DHCP4_REQUEST_CLASS_PROCESSING_FAILED) - .arg(ex.getQuery()->getLabel()); - } - return (ex.getResponse()); } @@ -2369,8 +2365,8 @@ Dhcpv4Srv::processInform(Pkt4Ptr& inform) { appendBasicOptions(ex); adjustIfaceData(ex); - // See if the class mandates setting any fixed fields (siaddr, sname, - // filename). + // Set fixed fields (siaddr, sname, filename) if defined in + // the reservation, class or subnet specific configuration. setFixedFields(ex); // There are cases for the DHCPINFORM that the server receives it via @@ -2391,14 +2387,6 @@ Dhcpv4Srv::processInform(Pkt4Ptr& inform) { // The DHCPACK must contain server id. appendServerID(ex); - /// @todo: decide whether we want to add a new hook point for - /// doing class specific processing. - if (!vendorClassSpecificProcessing(ex)) { - LOG_DEBUG(options4_logger, DBG_DHCP4_DETAIL, - DHCP4_INFORM_CLASS_PROCESSING_FAILED) - .arg(inform->getLabel()); - } - return (ex.getResponse()); } @@ -2622,17 +2610,8 @@ void Dhcpv4Srv::classifyByVendor(const Pkt4Ptr& pkt, std::string& classes) { // is indeed a modem, John B. suggested to check whether chaddr field // quals subscriber-id option that was inserted by the relay (CMTS). // This kind of logic will appear here soon. - if (vendor_class->getValue().find(DOCSIS3_CLASS_MODEM) != std::string::npos) { - pkt->addClass(VENDOR_CLASS_PREFIX + DOCSIS3_CLASS_MODEM); - classes += string(VENDOR_CLASS_PREFIX + DOCSIS3_CLASS_MODEM) + " "; - } else - if (vendor_class->getValue().find(DOCSIS3_CLASS_EROUTER) != std::string::npos) { - pkt->addClass(VENDOR_CLASS_PREFIX + DOCSIS3_CLASS_EROUTER); - classes += string(VENDOR_CLASS_PREFIX + DOCSIS3_CLASS_EROUTER) + " "; - } else { - pkt->addClass(VENDOR_CLASS_PREFIX + vendor_class->getValue()); - classes += VENDOR_CLASS_PREFIX + vendor_class->getValue(); - } + pkt->addClass(VENDOR_CLASS_PREFIX + vendor_class->getValue()); + classes += VENDOR_CLASS_PREFIX + vendor_class->getValue(); } void Dhcpv4Srv::classifyPacket(const Pkt4Ptr& pkt) { @@ -2687,52 +2666,6 @@ void Dhcpv4Srv::classifyPacket(const Pkt4Ptr& pkt) { } } -bool -Dhcpv4Srv::vendorClassSpecificProcessing(const Dhcpv4Exchange& ex) { - - Subnet4Ptr subnet = ex.getContext()->subnet_; - Pkt4Ptr query = ex.getQuery(); - Pkt4Ptr rsp = ex.getResponse(); - - // If any of those is missing, there is nothing to do. - if (!subnet || !query || !rsp) { - return (true); - } - - if (query->inClass(VENDOR_CLASS_PREFIX + DOCSIS3_CLASS_MODEM)) { - - // Set next-server. This is TFTP server address. Cable modems will - // download their configuration from that server. - rsp->setSiaddr(subnet->getSiaddr()); - - // Now try to set up file field in DHCPv4 packet. We will just copy - // content of the boot-file option, which contains the same information. - const CfgOptionList& co_list = ex.getCfgOptionList(); - for (CfgOptionList::const_iterator copts = co_list.begin(); - copts != co_list.end(); ++copts) { - OptionDescriptor desc = (*copts)->get("dhcp4", DHO_BOOT_FILE_NAME); - - if (desc.option_) { - boost::shared_ptr boot = - boost::dynamic_pointer_cast(desc.option_); - if (boot) { - std::string filename = boot->getValue(); - rsp->setFile((const uint8_t*)filename.c_str(), filename.size()); - break; - } - } - } - } - - if (query->inClass(VENDOR_CLASS_PREFIX + DOCSIS3_CLASS_EROUTER)) { - - // Do not set TFTP server address for eRouter devices. - rsp->setSiaddr(IOAddress::IPV4_ZERO_ADDRESS()); - } - - return (true); -} - void Dhcpv4Srv::startD2() { D2ClientMgr& d2_mgr = CfgMgr::instance().getD2ClientMgr(); diff --git a/src/bin/dhcp4/dhcp4_srv.h b/src/bin/dhcp4/dhcp4_srv.h index 813e66635c..b6646b4dec 100644 --- a/src/bin/dhcp4/dhcp4_srv.h +++ b/src/bin/dhcp4/dhcp4_srv.h @@ -517,8 +517,10 @@ protected: /// /// If the incoming packets belongs to a class and that class defines /// next-server, server-hostname or boot-file-name, we need to set the - /// siaddr, sname or filename fields in the outgoing packet. This - /// is what this method does. + /// siaddr, sname or filename fields in the outgoing packet. Also, those + /// values can be defined for subnet or in reservations. The values + /// defined in reservation takes precedence over class values, which + /// in turn take precedence over subnet values. /// /// @param ex DHCPv4 exchange holding the client's message and the server's /// response to be adjusted. @@ -763,18 +765,6 @@ protected: /// @param pkt packet to be classified void classifyPacket(const Pkt4Ptr& pkt); - /// @brief Performs packet processing specific to a vendor class - /// - /// If the selected subnet, query or response in the @c ex object is NULL - /// this method returns immediately and returns true. - /// - /// @note This processing is a likely candidate to be pushed into hooks. - /// - /// @param ex The exchange holding both the client's message and the - /// server's response. - /// @return true if successful, false otherwise (will prevent sending response) - bool vendorClassSpecificProcessing(const Dhcpv4Exchange& ex); - /// @brief Allocation Engine. /// Pointer to the allocation engine that we are currently using /// It must be a pointer, because we will support changing engines diff --git a/src/bin/dhcp4/tests/classify_unittest.cc b/src/bin/dhcp4/tests/classify_unittest.cc index fcea893146..c3862f3090 100644 --- a/src/bin/dhcp4/tests/classify_unittest.cc +++ b/src/bin/dhcp4/tests/classify_unittest.cc @@ -20,54 +20,19 @@ using namespace std; namespace { -/// @brief Set of JSON configurations used throughout the DORA tests. +/// @brief Set of JSON configurations used throughout the classify 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: +/// - 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 + // Configuration 0 "{ \"interfaces-config\": {" " \"interfaces\": [ \"*\" ]" "}," @@ -124,6 +89,19 @@ public: ~ClassifyTest() { } + /// @brief Does client exchanges and checks if fixed fields have expected values. + /// + /// Depending on the value of msgtype (allowed types: DHCPDISCOVER, DHCPREQUEST or + /// DHCPINFORM), this method sets up the server, then conducts specified exchange + /// and then checks if the response contains expected values of next-server, sname + /// and filename fields. + /// + /// @param config server configuration to be used + /// @param msgtype DHCPDISCOVER, DHCPREQUEST or DHCPINFORM + /// @param extra_opt option to include in client messages (optional) + /// @param exp_next_server expected value of the next-server field + /// @param exp_sname expected value of the sname field + /// @param exp_filename expected value of the filename field void testFixedFields(const char* config, uint8_t msgtype, const OptionPtr& extra_opt, const std::string& exp_next_server, const std::string& exp_sname, @@ -181,17 +159,17 @@ public: // 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", "", ""); + testFixedFields(CONFIGS[0], 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", "", ""); + testFixedFields(CONFIGS[0], 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", "", ""); + testFixedFields(CONFIGS[0], DHCPINFORM, OptionPtr(), "0.0.0.0", "", ""); } @@ -200,21 +178,21 @@ TEST_F(ClassifyTest, fixedFieldsInformNoClasses) { TEST_F(ClassifyTest, fixedFieldsDiscoverNextServer) { OptionPtr pxe(new OptionInt(Option::V4, 93, 0x0009)); - testFixedFields(CONFIGS[1], DHCPDISCOVER, pxe, "1.2.3.4", "", ""); + testFixedFields(CONFIGS[0], 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", "", ""); + testFixedFields(CONFIGS[0], 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", "", ""); + testFixedFields(CONFIGS[0], DHCPINFORM, pxe, "1.2.3.4", "", ""); } @@ -223,21 +201,21 @@ TEST_F(ClassifyTest, fixedFieldsInformNextServer) { TEST_F(ClassifyTest, fixedFieldsDiscoverHostname) { OptionPtr pxe(new OptionInt(Option::V4, 93, 0x0007)); - testFixedFields(CONFIGS[1], DHCPDISCOVER, pxe, "0.0.0.0", "deneb", ""); + testFixedFields(CONFIGS[0], 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", ""); + testFixedFields(CONFIGS[0], 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", ""); + testFixedFields(CONFIGS[0], DHCPINFORM, pxe, "0.0.0.0", "deneb", ""); } @@ -246,21 +224,21 @@ TEST_F(ClassifyTest, fixedFieldsInformHostname) { TEST_F(ClassifyTest, fixedFieldsDiscoverFile1) { OptionPtr pxe(new OptionInt(Option::V4, 93, 0x0006)); - testFixedFields(CONFIGS[1], DHCPDISCOVER, pxe, "0.0.0.0", "", "pxelinux.0"); + testFixedFields(CONFIGS[0], 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"); + testFixedFields(CONFIGS[0], 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"); + testFixedFields(CONFIGS[0], DHCPDISCOVER, pxe, "0.0.0.0", "", "pxelinux.0"); } @@ -269,21 +247,21 @@ TEST_F(ClassifyTest, fixedFieldsInformFile1) { TEST_F(ClassifyTest, fixedFieldsDiscoverFile2) { OptionPtr pxe(new OptionInt(Option::V4, 93, 0x0001)); - testFixedFields(CONFIGS[1], DHCPDISCOVER, pxe, "0.0.0.0", "", "ipxe.efi"); + testFixedFields(CONFIGS[0], 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"); + testFixedFields(CONFIGS[0], 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"); + testFixedFields(CONFIGS[0], DHCPINFORM, pxe, "0.0.0.0", "", "ipxe.efi"); } diff --git a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc index b8e5575019..af3c8667b0 100644 --- a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc +++ b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc @@ -1486,8 +1486,13 @@ TEST_F(Dhcpv4SrvTest, vendorOptionsDocsisDefinitions) { ASSERT_EQ(0, rcode_); } -// Checks if DOCSIS client packets are classified properly -TEST_F(Dhcpv4SrvTest, docsisClientClassification) { +/// Checks if DOCSIS client packets are classified properly +/// +/// @todo: With the change in #4626 the vendorClassSpecificProcessing +/// code was removed and replaced with generic classification. One day +/// we should rewrite this test to use classes. It would check that the +/// classification system can be used for docsis packets. +TEST_F(Dhcpv4SrvTest, DISABLED_docsisClientClassification) { NakedDhcpv4Srv srv(0); diff --git a/src/lib/dhcpsrv/client_class_def.cc b/src/lib/dhcpsrv/client_class_def.cc index bc9cd48641..010457eaed 100644 --- a/src/lib/dhcpsrv/client_class_def.cc +++ b/src/lib/dhcpsrv/client_class_def.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2015 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2015-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 @@ -16,7 +16,7 @@ ClientClassDef::ClientClassDef(const std::string& name, const ExpressionPtr& match_expr, const CfgOptionPtr& cfg_option) : name_(name), match_expr_(match_expr), cfg_option_(cfg_option), - next_server_(asiolink::IOAddress("0.0.0.0")) { + next_server_(asiolink::IOAddress::IPV4_ZERO_ADDRESS()) { // Name can't be blank if (name_.empty()) { @@ -34,7 +34,8 @@ ClientClassDef::ClientClassDef(const std::string& name, ClientClassDef::ClientClassDef(const ClientClassDef& rhs) : name_(rhs.name_), match_expr_(ExpressionPtr()), - cfg_option_(new CfgOption()), next_server_(asiolink::IOAddress("0.0.0.0")) { + cfg_option_(new CfgOption()), + next_server_(asiolink::IOAddress::IPV4_ZERO_ADDRESS()) { if (rhs.match_expr_) { match_expr_.reset(new Expression()); @@ -124,8 +125,8 @@ ClientClassDictionary::addClass(const std::string& name, const ExpressionPtr& match_expr, const CfgOptionPtr& cfg_option, asiolink::IOAddress next_server, - const std::vector& sname, - const std::vector& filename) { + const std::string& sname, + const std::string& filename) { ClientClassDefPtr cclass(new ClientClassDef(name, match_expr, cfg_option)); cclass->setNextServer(next_server); cclass->setSname(sname); diff --git a/src/lib/dhcpsrv/client_class_def.h b/src/lib/dhcpsrv/client_class_def.h index 36d1237807..82084aded1 100644 --- a/src/lib/dhcpsrv/client_class_def.h +++ b/src/lib/dhcpsrv/client_class_def.h @@ -1,4 +1,4 @@ -// Copyright (C) 2015 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2015-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 @@ -108,7 +108,7 @@ public: /// @brief returns next-server value /// @return next-server value - asiolink::IOAddress getNextServer() const { + const asiolink::IOAddress& getNextServer() const { return (next_server_); } @@ -122,26 +122,26 @@ public: /// @brief sets the server-name value /// /// @param sname the value to be set - void setSname(const std::vector& sname) { + void setSname(const std::string& sname) { sname_ = sname; } /// @brief sets the boot-file-name value /// /// @param filename the value to be set - void setFilename(const std::vector& filename) { + void setFilename(const std::string& filename) { filename_ = filename; } /// @brief returns server-hostname value /// @return the vector that contains server-hostname (may be empty if not defined) - const std::vector& getSname() const { + const std::string& getSname() const { return (sname_); } /// @brief returns boot-file-name value /// @return the vector that contains boot-file-name (may be empty if not defined) - const std::vector& getFilename() const { + const std::string& getFilename() const { return (filename_); } @@ -165,13 +165,13 @@ private: /// If set by the server-hostname parameter, this value will be /// set in the sname field of the DHCPv4 packet. /// This can be up to 64 octets long. - std::vector sname_; + std::string sname_; /// @brief boot-file-name /// If set by the boot-file-name parameter, this value will be /// set in the file field of the DHCPv4 packet. /// This can be up to 128 octets long. - std::vector filename_; + std::string filename_; }; @@ -214,8 +214,8 @@ public: void addClass(const std::string& name, const ExpressionPtr& match_expr, const CfgOptionPtr& options, asiolink::IOAddress next_server = asiolink::IOAddress("0.0.0.0"), - const std::vector& sname = std::vector(), - const std::vector& filename = std::vector()); + const std::string& sname = std::string(), + const std::string& filename = std::string()); /// @brief Adds a new class to the list /// diff --git a/src/lib/dhcpsrv/parsers/client_class_def_parser.cc b/src/lib/dhcpsrv/parsers/client_class_def_parser.cc index 218457ce85..39b298f7cf 100644 --- a/src/lib/dhcpsrv/parsers/client_class_def_parser.cc +++ b/src/lib/dhcpsrv/parsers/client_class_def_parser.cc @@ -119,43 +119,42 @@ ClientClassDefParser::build(ConstElementPtr class_def_cfg) { std::string name; try { name = string_values_->getParam("name"); - } catch (const std::exception& ex) { - isc_throw(DhcpConfigError, ex.what() << " (" - << 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 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); + } - // 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()); + if (next_server.getFamily() != AF_INET) { + isc_throw(DhcpConfigError, "Invalid next-server value: '" + << next_server_txt << "', must be IPv4 address"); + } - 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()); + if (next_server.isV4Bcast()) { + isc_throw(DhcpConfigError, "Invalid next-server value: '" + << next_server_txt << "', must not be a broadcast"); + } + + // Let's try to parse sname + string sname = string_values_->getOptionalParam("server-hostname", ""); + if (sname.length() >= Pkt4::MAX_SNAME_LEN) { + isc_throw(DhcpConfigError, "server-hostname must be at most " + << Pkt4::MAX_SNAME_LEN - 1 << " bytes long, it is " + << sname.length()); + } + + string filename = string_values_->getOptionalParam("boot-file-name", ""); + if (filename.length() > Pkt4::MAX_FILE_LEN) { + isc_throw(DhcpConfigError, "boot-file-name must be at most " + << Pkt4::MAX_FILE_LEN - 1 << " bytes long, it is " + << filename.length()); + } - try { // an OptionCollectionPtr class_dictionary_->addClass(name, match_expr_, options_, next_server, sname, filename); diff --git a/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc b/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc index f61d0074ba..5949cdbb43 100644 --- a/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc +++ b/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2015 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2015-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 @@ -585,6 +585,8 @@ TEST_F(ClientClassDefListParserTest, invalidClass) { DhcpConfigError); } +// Test verifies that without any class specified, the fixed fields have their +// default, empty value. TEST_F(ClientClassDefParserTest, noFixedFields) { std::string cfg_text = @@ -610,7 +612,8 @@ TEST_F(ClientClassDefParserTest, noFixedFields) { EXPECT_EQ(0, cclass->getFilename().size()); } - +// Test verifies that it is possible to define next-server field and it +// is actually set in the class properly. TEST_F(ClientClassDefParserTest, nextServer) { std::string cfg_text = @@ -637,6 +640,7 @@ TEST_F(ClientClassDefParserTest, nextServer) { EXPECT_EQ(0, cclass->getFilename().size()); } +// Test verifies that the parser rejects bogus next-server value. TEST_F(ClientClassDefParserTest, nextServerBogus) { std::string bogus_v6 = @@ -666,6 +670,8 @@ TEST_F(ClientClassDefParserTest, nextServerBogus) { EXPECT_THROW(parseClientClassDef(bogus_junk, Option::V4), DhcpConfigError); } +// Test verifies that it is possible to define server-hostname field and it +// is actually set in the class properly. TEST_F(ClientClassDefParserTest, serverName) { std::string cfg_text = @@ -687,12 +693,12 @@ TEST_F(ClientClassDefParserTest, serverName) { ASSERT_TRUE(cclass); // And it should not have any fixed fields set - std::string exp_txt("hal9000"); - std::vector exp_sname(exp_txt.begin(), exp_txt.end()); + std::string exp_sname("hal9000"); EXPECT_EQ(exp_sname, cclass->getSname()); } +// Test verifies that the parser rejects bogus server-hostname value. TEST_F(ClientClassDefParserTest, serverNameInvalid) { std::string cfg_too_long = @@ -712,6 +718,8 @@ TEST_F(ClientClassDefParserTest, serverNameInvalid) { } +// Test verifies that it is possible to define boot-file-name field and it +// is actually set in the class properly. TEST_F(ClientClassDefParserTest, filename) { std::string cfg_text = @@ -733,12 +741,11 @@ TEST_F(ClientClassDefParserTest, filename) { ASSERT_TRUE(cclass); // And it should not have any fixed fields set - std::string exp_txt("ipxe.efi"); - std::vector exp_filename(exp_txt.begin(), exp_txt.end()); - + std::string exp_filename("ipxe.efi"); EXPECT_EQ(exp_filename, cclass->getFilename()); } +// Test verifies that the parser rejects bogus boot-file-name value. TEST_F(ClientClassDefParserTest, filenameBogus) { // boot-file-name is allowed up to 128 bytes, this one is 129. diff --git a/src/lib/dhcpsrv/tests/client_class_def_unittest.cc b/src/lib/dhcpsrv/tests/client_class_def_unittest.cc index 04523c2152..734b9551dd 100644 --- a/src/lib/dhcpsrv/tests/client_class_def_unittest.cc +++ b/src/lib/dhcpsrv/tests/client_class_def_unittest.cc @@ -316,7 +316,7 @@ TEST(ClientClassDef, fixedFieldsDefaults) { ASSERT_NO_THROW(cclass.reset(new ClientClassDef(name, expr))); // Let's checks that it doesn't return any nonsense - vector empty; + string empty; ASSERT_EQ(IOAddress("0.0.0.0"), cclass->getNextServer()); EXPECT_EQ(empty, cclass->getSname()); EXPECT_EQ(empty, cclass->getFilename()); @@ -338,11 +338,8 @@ TEST(ClientClassDef, fixedFieldsBasics) { ASSERT_NO_THROW(cclass.reset(new ClientClassDef(name, expr))); - string sname_txt = "This is a very long string that can be a server name"; - vector sname(sname_txt.begin(), sname_txt.end()); - - string filename_txt = "this-is-a-slightly-longish-name-of-a-file.txt"; - vector filename(sname_txt.begin(), sname_txt.end()); + string sname = "This is a very long string that can be a server name"; + string filename = "this-is-a-slightly-longish-name-of-a-file.txt"; cclass->setNextServer(IOAddress("1.2.3.4")); cclass->setSname(sname);