From: Marcin Siodelski Date: Tue, 4 Nov 2014 15:09:06 +0000 (+0100) Subject: [3467] Modified option data parser to support optional parameters. X-Git-Tag: kea-eng-20141219~12^2~6 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=1c0bc1f4b2fc3db9e82db2e5f63b90ad6c767c47;p=thirdparty%2Fkea.git [3467] Modified option data parser to support optional parameters. Also added some more unit tests for csv-format parameter. --- diff --git a/src/lib/dhcpsrv/dhcp_parsers.cc b/src/lib/dhcpsrv/dhcp_parsers.cc index f1a647bfc0..d5c97c6886 100644 --- a/src/lib/dhcpsrv/dhcp_parsers.cc +++ b/src/lib/dhcpsrv/dhcp_parsers.cc @@ -344,7 +344,7 @@ OptionDataParser::commit() { } OptionalValue -OptionDataParser::extractCode() const { +OptionDataParser::extractCode(ConstElementPtr parent) const { uint32_t code; try { code = uint32_values_->getParam("code"); @@ -355,21 +355,23 @@ OptionDataParser::extractCode() const { if (code == 0) { isc_throw(DhcpConfigError, "option code must not be zero " - "(" << uint32_values_->getPosition("code") << ")"); + "(" << uint32_values_->getPosition("code", parent) << ")"); } else if (address_family_ == AF_INET && code > std::numeric_limits::max()) { isc_throw(DhcpConfigError, "invalid option code '" << code << "', it must not be greater than '" << static_cast(std::numeric_limits::max()) - << "' (" << uint32_values_->getPosition("code") << ")"); + << "' (" << uint32_values_->getPosition("code", parent) + << ")"); } else if (address_family_ == AF_INET6 && code > std::numeric_limits::max()) { isc_throw(DhcpConfigError, "invalid option code '" << code << "', it must not exceed '" << std::numeric_limits::max() - << "' (" << uint32_values_->getPosition("code") << ")"); + << "' (" << uint32_values_->getPosition("code", parent) + << ")"); } @@ -377,7 +379,7 @@ OptionDataParser::extractCode() const { } OptionalValue -OptionDataParser::extractName() const { +OptionDataParser::extractName(ConstElementPtr parent) const { std::string name; try { name = string_values_->getParam("name"); @@ -388,12 +390,12 @@ OptionDataParser::extractName() const { if (name.empty()) { isc_throw(DhcpConfigError, "option name is empty (" - << string_values_->getPosition("name") << ")"); + << string_values_->getPosition("name", parent) << ")"); } else if (name.find(" ") != std::string::npos) { isc_throw(DhcpConfigError, "invalid option name '" << name << "', space character is not allowed (" - << string_values_->getPosition("name") << ")"); + << string_values_->getPosition("name", parent) << ")"); } return (OptionalValue(name, OptionalValueState(true))); @@ -435,9 +437,28 @@ OptionDataParser::extractSpace() const { return (space); } - if (!OptionSpace::validateName(space)) { - isc_throw(DhcpConfigError, "invalid option space name '" - << space << "' (" + try { + if (!OptionSpace::validateName(space)) { + isc_throw(DhcpConfigError, "invalid option space name '" + << space << "'"); + } + + if ((space == DHCP4_OPTION_SPACE) && (address_family_ == AF_INET6)) { + isc_throw(DhcpConfigError, "'" << DHCP4_OPTION_SPACE + << "' option space name is reserved for DHCPv4 server"); + + } else if ((space == DHCP6_OPTION_SPACE) && + (address_family_ == AF_INET)) { + isc_throw(DhcpConfigError, "'" << DHCP6_OPTION_SPACE + << "' option space name is reserved for DHCPv6 server"); + } + + } catch (std::exception& ex) { + // Append position of the option space parameter. Note, that in the case + // when 'space' was not specified a default value will be used and we + // should never get here. Therefore, it is ok to call getPosition for + // the space parameter here as this parameter will always be specified. + isc_throw(DhcpConfigError, ex.what() << " (" << string_values_->getPosition("space") << ")"); } @@ -446,25 +467,20 @@ OptionDataParser::extractSpace() const { OptionDefinitionPtr -OptionDataParser::findServerSpaceOptionDefinition(const std::string& option_space, - const uint32_t option_code) const { +OptionDataParser::findOptionDefinition(const std::string& option_space, + const uint32_t option_code) const { const Option::Universe u = address_family_ == AF_INET ? Option::V4 : Option::V6; - - if ((option_space == DHCP4_OPTION_SPACE) && (u == Option::V6)) { - isc_throw(DhcpConfigError, "'" << DHCP4_OPTION_SPACE - << "' option space name is reserved for DHCPv4 server"); - } else if ((option_space == DHCP6_OPTION_SPACE) && (u == Option::V4)) { - isc_throw(DhcpConfigError, "'" << DHCP6_OPTION_SPACE - << "' option space name is reserved for DHCPv6 server"); - } - OptionDefinitionPtr def; - if (((option_space == DHCP4_OPTION_SPACE) || (option_space == DHCP6_OPTION_SPACE)) && + + if (((option_space == DHCP4_OPTION_SPACE) || + (option_space == DHCP6_OPTION_SPACE)) && LibDHCP::isStandardOption(u, option_code)) { def = LibDHCP::getOptionDef(u, option_code); - } else { + } + + if (!def) { // Check if this is a vendor-option. If it is, get vendor-specific // definition. uint32_t vendor_id = CfgOption::optionSpaceToVendorId(option_space); @@ -472,63 +488,54 @@ OptionDataParser::findServerSpaceOptionDefinition(const std::string& option_spac def = LibDHCP::getVendorOptionDef(u, vendor_id, option_code); } } + + if (!def) { + // Check if this is an option specified by a user. + def = CfgMgr::instance().getStagingCfg()->getCfgOptionDef() + ->get(option_space, option_code); + } + return (def); } - void OptionDataParser::createOption(ConstElementPtr option_data) { const Option::Universe universe = address_family_ == AF_INET ? Option::V4 : Option::V6; - OptionalValue code_param = extractCode(); - OptionalValue name_param = extractName(); + OptionalValue code_param = extractCode(option_data); + OptionalValue name_param = extractName(option_data); OptionalValue csv_format_param = extractCSVFormat(); std::string data_param = extractData(); std::string space_param = extractSpace(); - // Find the Option Definition for the option by its option code. - // findOptionDefinition will throw if not found, no need to test. - // Find the definition for the option by its code. This function - // may throw so we catch exceptions to log the culprit line of the - // configuration. - OptionDefinitionPtr def; - try { - def = findServerSpaceOptionDefinition(space_param, code_param); - - } catch (const std::exception& ex) { - isc_throw(DhcpConfigError, ex.what() - << " (" << string_values_->getPosition("space") << ")"); - } - if (!def) { - // If we are not dealing with a standard option then we - // need to search for its definition among user-configured - // options. They are expected to be in the global storage - // already. - def = CfgMgr::instance().getStagingCfg()->getCfgOptionDef() - ->get(space_param, code_param); + // Try to find a corresponding option definition. + OptionDefinitionPtr def = findOptionDefinition(space_param, code_param); - // It's ok if we don't have option format if the option is - // specified as hex - if (!def && csv_format_param) { + // If there is no definition, the user must not explicitly enable the + // use of csv-format. + if (!def && csv_format_param.isSpecified() && csv_format_param) { isc_throw(DhcpConfigError, "definition for the option '" << space_param << "." << name_param << "' having code '" << code_param << "' does not exist (" - << string_values_->getPosition("name") << ")"); - } + << string_values_->getPosition("name", option_data) + << ")"); } // Transform string of hexadecimal digits into binary format. std::vector binary; std::vector data_tokens; - if (!csv_format_param.isSpecified() || csv_format_param) { + // If the definition is available and csv-format hasn't been explicitly + // disabled, we will parse the data as comma separated values. + if (def && (!csv_format_param.isSpecified() || csv_format_param)) { // If the option data is specified as a string of comma // separated values then we need to split this string into // individual values - each value will be used to initialize // one data field of an option. data_tokens = isc::util::str::tokens(data_param, ","); + } else { // Otherwise, the option data is specified as a string of // hexadecimal digits that we have to turn into binary format. @@ -543,27 +550,21 @@ OptionDataParser::createOption(ConstElementPtr option_data) { } catch (...) { isc_throw(DhcpConfigError, "option data is not a valid" << " string of hexadecimal digits: " << data_param - << " (" << string_values_->getPosition("data") << ")"); + << " (" + << string_values_->getPosition("data", option_data) + << ")"); } } OptionPtr option; if (!def) { - if (csv_format_param.isSpecified() && csv_format_param) { - isc_throw(DhcpConfigError, "the CSV option data format can be" - " used to specify values for an option that has a" - " definition. The option with code " << code_param - << " does not have a definition (" - << boolean_values_->getPosition("csv-format") << ")"); - } - // @todo We have a limited set of option definitions initalized at // the moment. In the future we want to initialize option definitions // for all options. Consequently an error will be issued if an option // definition does not exist for a particular option code. For now it is // ok to create generic option if definition does not exist. - OptionPtr option(new Option(universe, - static_cast(code_param), binary)); + OptionPtr option(new Option(universe, static_cast(code_param), + binary)); // The created option is stored in option_descriptor_ class member // until the commit stage when it is inserted into the main storage. // If an option with the same code exists in main storage already the @@ -583,13 +584,15 @@ OptionDataParser::createOption(ConstElementPtr option_data) { << name_param << "' does not match the " << "option definition: '" << space_param << "." << def->getName() << "' (" - << string_values_->getPosition("name") << ")"); + << string_values_->getPosition("name", option_data) + << ")"); } // Option definition has been found so let's use it to create // an instance of our option. try { - OptionPtr option = csv_format_param ? + OptionPtr option = + !csv_format_param.isSpecified() || csv_format_param ? def->optionFactory(universe, code_param, data_tokens) : def->optionFactory(universe, code_param, binary); OptionDescriptor desc(option, false); @@ -601,7 +604,8 @@ OptionDataParser::createOption(ConstElementPtr option_data) { << " option definition (space: " << space_param << ", code: " << code_param << "): " << ex.what() << " (" - << string_values_->getPosition("data") << ")"); + << string_values_->getPosition("data", option_data) + << ")"); } } diff --git a/src/lib/dhcpsrv/dhcp_parsers.h b/src/lib/dhcpsrv/dhcp_parsers.h index 6ea69ed9d3..7ab19ddfa8 100644 --- a/src/lib/dhcpsrv/dhcp_parsers.h +++ b/src/lib/dhcpsrv/dhcp_parsers.h @@ -110,15 +110,20 @@ public: /// element holding a particular value is located. /// /// @param name is the name of the parameter which position is desired. + /// @param parent Pointer to a data element which position should be + /// returned when position of the specified parameter is not found. /// /// @return Position of the data element or the position holding empty /// file name and two zeros if the position hasn't been specified for the /// particular value. - const data::Element::Position& getPosition(const std::string& name) const { + const data::Element::Position& + getPosition(const std::string& name, const data::ConstElementPtr parent = + data::ConstElementPtr()) const { typename std::map::const_iterator pos = positions_.find(name); if (pos == positions_.end()) { - return (data::Element::ZERO_POSITION()); + return (parent ? parent->getPosition() : + data::Element::ZERO_POSITION()); } return (pos->second); @@ -541,10 +546,11 @@ public: virtual ~OptionDataParser(){}; protected: - /// @brief Finds an option definition within the server's option space + + /// @brief Finds an option definition within an option space /// /// Given an option space and an option code, find the correpsonding - /// option defintion within the server's option defintion storage. + /// option defintion within the option defintion storage. /// /// @param option_space name of the parameter option space /// @param option_code numeric value of the parameter to find @@ -553,8 +559,8 @@ protected: /// @throw DhcpConfigError if the option space requested is not valid /// for this server. virtual OptionDefinitionPtr - findServerSpaceOptionDefinition(const std::string& option_space, - const uint32_t option_code) const; + findOptionDefinition(const std::string& option_space, + const uint32_t option_code) const; private: @@ -575,14 +581,46 @@ private: /// are invalid. void createOption(isc::data::ConstElementPtr option_data); - util::OptionalValue extractCode() const; + /// @brief Retrieves parsed option code as an optional value. + /// + /// @param parent A data element holding full option data configuration. + /// It is used here to log a position if the element holding a code + /// is not specified and its position is therefore unavailable. + /// + /// @return Option code, possibly unspecified. + /// @throw DhcpConfigError if option code is invalid. + util::OptionalValue + extractCode(data::ConstElementPtr parent) const; - util::OptionalValue extractName() const; + /// @brief Retrieves parsed option name as an optional value. + /// + /// @param parent A data element holding full option data configuration. + /// It is used here to log a position if the element holding a code + /// is not specified and its position is therefore unavailable. + /// + /// @return Option name, possibly unspecified. + /// @throw DhcpConfigError if option name is invalid. + util::OptionalValue + extractName(data::ConstElementPtr parent) const; + /// @brief Retrieves csv-format parameter as an optional value. + /// + /// @return Value of the csv-format parameter, possibly unspecified. util::OptionalValue extractCSVFormat() const; + /// @brief Retrieves option data as a string. + /// + /// @return Option data as a string. It will return empty string if + /// option data is unspecified. std::string extractData() const; + /// @brief Retrieves option space name. + /// + /// If option space name is not specified in the configuration the + /// 'dhcp4' or 'dhcp6' option space name is returned, depending on + /// the universe specified in the parser context. + /// + /// @return Option space name. std::string extractSpace() const; /// Storage for boolean values. diff --git a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc index 58bc8bfa2a..8bc032e684 100644 --- a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc +++ b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc @@ -361,9 +361,11 @@ public: /// @throw throws NotImplemented if element name isn't supported. ParserPtr createConfigParser(const std::string& config_id) { ParserPtr parser; + int family = parser_context_->universe_ == Option::V4 ? + AF_INET : AF_INET6; if (config_id.compare("option-data") == 0) { parser.reset(new OptionDataListParser(config_id, CfgOptionPtr(), - AF_INET)); + family)); } else if (config_id.compare("option-def") == 0) { parser.reset(new OptionDefListParser(config_id, @@ -402,6 +404,7 @@ public: ConstElementPtr status = parseElementSet(json); ConstElementPtr comment = parseAnswer(rcode_, status); error_text_ = comment->stringValue(); + std::cout << error_text_ << std::endl; // If error was reported, the error string should contain // position of the data element which caused failure. if (rcode_ != 0) { @@ -551,7 +554,10 @@ TEST_F(ParseConfigTest, basicOptionDataTest) { EXPECT_EQ(val, opt_ptr->toText()); } -TEST_F(ParseConfigTest, defaultCSVFormat) { +// This test checks behavior of the configuration parser for option data +// for different values of csv-format parameter and when there is an option +// definition present. +TEST_F(ParseConfigTest, csvFormatWithOptionDef) { std::string config = "{ \"option-data\": [ {" " \"name\": \"swap-server\"," @@ -561,13 +567,135 @@ TEST_F(ParseConfigTest, defaultCSVFormat) { " } ]" "}"; + // The default universe is V6. We need to change it to use dhcp4 option + // space. + parser_context_->universe_ = Option::V4; int rcode = 0; ASSERT_NO_THROW(rcode = parseConfiguration(config)); - ASSERT_TRUE(rcode == 0); + ASSERT_EQ(0, rcode); - OptionPtr opt_ptr = getOptionPtr("dhcp4", 16); + // Verify that the option data is correct. OptionCustomPtr addr_opt = boost::dynamic_pointer_cast< - OptionCustom>(opt_ptr); + OptionCustom>(getOptionPtr("dhcp4", 16)); + ASSERT_TRUE(addr_opt); + EXPECT_EQ("192.0.2.0", addr_opt->readAddress().toText()); + + // Explicitly enable csv-format. + CfgMgr::instance().clear(); + config = + "{ \"option-data\": [ {" + " \"name\": \"swap-server\"," + " \"space\": \"dhcp4\"," + " \"code\": 16," + " \"csv-format\": True," + " \"data\": \"192.0.2.0\"" + " } ]" + "}"; + ASSERT_NO_THROW(rcode = parseConfiguration(config)); + ASSERT_EQ(0, rcode); + + // Verify that the option data is correct. + addr_opt = boost::dynamic_pointer_cast< + OptionCustom>(getOptionPtr("dhcp4", 16)); + ASSERT_TRUE(addr_opt); + EXPECT_EQ("192.0.2.0", addr_opt->readAddress().toText()); + + // Explicitly disable csv-format and use hex instead. + CfgMgr::instance().clear(); + config = + "{ \"option-data\": [ {" + " \"name\": \"swap-server\"," + " \"space\": \"dhcp4\"," + " \"code\": 16," + " \"csv-format\": False," + " \"data\": \"C0000200\"" + " } ]" + "}"; + ASSERT_NO_THROW(rcode = parseConfiguration(config)); + ASSERT_EQ(0, rcode); + + // Verify that the option data is correct. + addr_opt = boost::dynamic_pointer_cast< + OptionCustom>(getOptionPtr("dhcp4", 16)); + ASSERT_TRUE(addr_opt); + EXPECT_EQ("192.0.2.0", addr_opt->readAddress().toText()); +} + +// This test checks behavior of the configuration parser for option data +// for different values of csv-format parameter and when there is no +// option definition. +TEST_F(ParseConfigTest, csvFormatNoOptionDef) { + // This option doesn't have any definition. It is ok to use such + // an option but the data should be specified in hex, not as CSV. + // Note that the parser will by default use the CSV format for the + // data but only in case there is a suitable option definition. + std::string config = + "{ \"option-data\": [ {" + " \"name\": \"foo-name\"," + " \"space\": \"dhcp6\"," + " \"code\": 25000," + " \"data\": \"1, 2, 5\"" + " } ]" + "}"; + int rcode = 0; + ASSERT_NO_THROW(rcode = parseConfiguration(config)); + EXPECT_NE(0, rcode); + + CfgMgr::instance().clear(); + // The data specified here will work both for CSV format and hex format. + // What we want to test here is that when the csv-format is enforced, the + // parser will fail because of lack of an option definition. + config = + "{ \"option-data\": [ {" + " \"name\": \"foo-name\"," + " \"space\": \"dhcp6\"," + " \"code\": 25000," + " \"csv-format\": True," + " \"data\": \"0\"" + " } ]" + "}"; + ASSERT_NO_THROW(rcode = parseConfiguration(config)); + EXPECT_NE(0, rcode); + + CfgMgr::instance().clear(); + // The same test case as above, but for the data specified in hex should + // be successful. + config = + "{ \"option-data\": [ {" + " \"name\": \"foo-name\"," + " \"space\": \"dhcp6\"," + " \"code\": 25000," + " \"csv-format\": False," + " \"data\": \"0\"" + " } ]" + "}"; + ASSERT_NO_THROW(rcode = parseConfiguration(config)); + ASSERT_EQ(0, rcode); + OptionPtr opt = getOptionPtr("dhcp6", 25000); + ASSERT_TRUE(opt); + ASSERT_EQ(1, opt->getData().size()); + EXPECT_EQ(0, opt->getData()[0]); + + CfgMgr::instance().clear(); + // When csv-format is not specified, the parser will check if the definition + // exists or not. Since there is no definition, the parser will accept the + // data in hex. + config = + "{ \"option-data\": [ {" + " \"name\": \"foo-name\"," + " \"space\": \"dhcp6\"," + " \"code\": 25000," + " \"data\": \"123456\"" + " } ]" + "}"; + ASSERT_NO_THROW(rcode = parseConfiguration(config)); + EXPECT_EQ(0, rcode); + opt = getOptionPtr("dhcp6", 25000); + ASSERT_TRUE(opt); + ASSERT_EQ(3, opt->getData().size()); + EXPECT_EQ(0x12, opt->getData()[0]); + EXPECT_EQ(0x34, opt->getData()[1]); + EXPECT_EQ(0x56, opt->getData()[2]); } }; // Anonymous namespace