From: Marcin Siodelski Date: Tue, 4 Nov 2014 16:14:57 +0000 (+0100) Subject: [3467] Option code, name and space are now optional. X-Git-Tag: kea-eng-20141219~12^2~5 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=65ceb1cac5a20dcf018296597484cc7b86adb5a9;p=thirdparty%2Fkea.git [3467] Option code, name and space are now optional. --- diff --git a/src/lib/dhcp/option6_addrlst.h b/src/lib/dhcp/option6_addrlst.h index 8327201e32..fe45bb49d3 100644 --- a/src/lib/dhcp/option6_addrlst.h +++ b/src/lib/dhcp/option6_addrlst.h @@ -1,4 +1,4 @@ -// Copyright (C) 2011-2012 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2011-2012, 2014 Internet Systems Consortium, Inc. ("ISC") // // Permission to use, copy, modify, and/or distribute this software for any // purpose with or without fee is hereby granted, provided that the above @@ -17,7 +17,7 @@ #include #include - +#include #include namespace isc { @@ -94,6 +94,9 @@ protected: AddressContainer addrs_; }; +/// @brief Pointer to the @c Option6AddrLst object. +typedef boost::shared_ptr Option6AddrLstPtr; + } // isc::dhcp namespace } // isc namespace diff --git a/src/lib/dhcpsrv/cfg_option_def.cc b/src/lib/dhcpsrv/cfg_option_def.cc index 130a9ee102..4421b642e6 100644 --- a/src/lib/dhcpsrv/cfg_option_def.cc +++ b/src/lib/dhcpsrv/cfg_option_def.cc @@ -138,7 +138,27 @@ CfgOptionDef::get(const std::string& option_space, return (OptionDefinitionPtr()); } - +OptionDefinitionPtr +CfgOptionDef::get(const std::string& option_space, + const std::string& option_name) const { + // Get the pointer to collection of the option definitions that belong + // to the particular option space. + OptionDefContainerPtr defs = getAll(option_space); + // If there are any option definitions for this option space, get the + // one that has the specified option name. + if (defs && !defs->empty()) { + const OptionDefContainerNameIndex& idx = defs->get<2>(); + const OptionDefContainerNameRange& range = idx.equal_range(option_name); + // If there is more than one definition matching the option name, + // return the first one. In fact, it shouldn't happen that we have + // more than one because we check for duplicates when we add them. + if (std::distance(range.first, range.second) > 0) { + return (*range.first); + } + } + // Nothing found. Return NULL pointer. + return (OptionDefinitionPtr()); +} } // end of namespace isc::dhcp } // end of namespace isc diff --git a/src/lib/dhcpsrv/cfg_option_def.h b/src/lib/dhcpsrv/cfg_option_def.h index 09932acfd2..81850e8738 100644 --- a/src/lib/dhcpsrv/cfg_option_def.h +++ b/src/lib/dhcpsrv/cfg_option_def.h @@ -110,6 +110,16 @@ public: OptionDefinitionPtr get(const std::string& option_space, const uint16_t option_code) const; + /// @brief Return option definition for the particular option space and name. + /// + /// @param option_space pption space. + /// @param option_name option name. + /// + /// @return An option definition or NULL pointer if option definition + /// has not been found. + OptionDefinitionPtr get(const std::string& option_space, + const std::string& option_name) const; + private: /// @brief A collection of option definitions. diff --git a/src/lib/dhcpsrv/dhcp_parsers.cc b/src/lib/dhcpsrv/dhcp_parsers.cc index d5c97c6886..a62cfb76ea 100644 --- a/src/lib/dhcpsrv/dhcp_parsers.cc +++ b/src/lib/dhcpsrv/dhcp_parsers.cc @@ -388,11 +388,7 @@ OptionDataParser::extractName(ConstElementPtr parent) const { return (OptionalValue(name)); } - if (name.empty()) { - isc_throw(DhcpConfigError, "option name is empty (" - << string_values_->getPosition("name", parent) << ")"); - - } else if (name.find(" ") != std::string::npos) { + if (name.find(" ") != std::string::npos) { isc_throw(DhcpConfigError, "invalid option name '" << name << "', space character is not allowed (" << string_values_->getPosition("name", parent) << ")"); @@ -465,18 +461,17 @@ OptionDataParser::extractSpace() const { return (space); } - +template OptionDefinitionPtr OptionDataParser::findOptionDefinition(const std::string& option_space, - const uint32_t option_code) const { + const SearchKey& search_key) const { const Option::Universe u = address_family_ == AF_INET ? Option::V4 : Option::V6; OptionDefinitionPtr def; - if (((option_space == DHCP4_OPTION_SPACE) || - (option_space == DHCP6_OPTION_SPACE)) && - LibDHCP::isStandardOption(u, option_code)) { - def = LibDHCP::getOptionDef(u, option_code); + if ((option_space == DHCP4_OPTION_SPACE) || + (option_space == DHCP6_OPTION_SPACE)) { + def = LibDHCP::getOptionDef(u, search_key); } @@ -485,14 +480,14 @@ OptionDataParser::findOptionDefinition(const std::string& option_space, // definition. uint32_t vendor_id = CfgOption::optionSpaceToVendorId(option_space); if (vendor_id) { - def = LibDHCP::getVendorOptionDef(u, vendor_id, option_code); + def = LibDHCP::getVendorOptionDef(u, vendor_id, search_key); } } if (!def) { // Check if this is an option specified by a user. def = CfgMgr::instance().getStagingCfg()->getCfgOptionDef() - ->get(option_space, option_code); + ->get(option_space, search_key); } return (def); @@ -509,18 +504,40 @@ OptionDataParser::createOption(ConstElementPtr option_data) { std::string data_param = extractData(); std::string space_param = extractSpace(); - // Try to find a corresponding option definition. - OptionDefinitionPtr def = findOptionDefinition(space_param, code_param); + // Require that option code or option name is specified. + if (!code_param.isSpecified() && !name_param.isSpecified()) { + isc_throw(DhcpConfigError, "option data configuration requires one of" + " 'code' or 'name' parameters to be specified" + << " (" << option_data->getPosition() << ")"); + } + + // Try to find a corresponding option definition using option code or + // option name. + OptionDefinitionPtr def = code_param.isSpecified() ? + findOptionDefinition(space_param, code_param) : + findOptionDefinition(space_param, name_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) { + if (!def) { + // If explicitly requested that the CSV format is to be used, + // the option definition is a must. + if (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", option_data) << ")"); + + // If there is no option definition and the option code is not specified + // we have no means to find the option code. + } else if (name_param.isSpecified() && !code_param.isSpecified()) { + isc_throw(DhcpConfigError, "definition for the option '" + << space_param << "." << name_param + << " does not exist (" + << string_values_->getPosition("name", option_data)); + } } // Transform string of hexadecimal digits into binary format. @@ -574,12 +591,8 @@ OptionDataParser::createOption(ConstElementPtr option_data) { } else { - // Option name should match the definition. The option name - // may seem to be redundant but in the future we may want - // to reference options and definitions using their names - // and/or option codes so keeping the option name in the - // definition of option value makes sense. - if (def->getName() != name_param.get()) { + // Option name is specified it should match the name in the definition. + if (name_param.isSpecified() && (def->getName() != name_param.get())) { isc_throw(DhcpConfigError, "specified option name '" << name_param << "' does not match the " << "option definition: '" << space_param @@ -593,8 +606,8 @@ OptionDataParser::createOption(ConstElementPtr option_data) { try { OptionPtr option = !csv_format_param.isSpecified() || csv_format_param ? - def->optionFactory(universe, code_param, data_tokens) : - def->optionFactory(universe, code_param, binary); + def->optionFactory(universe, def->getCode(), data_tokens) : + def->optionFactory(universe, def->getCode(), binary); OptionDescriptor desc(option, false); option_descriptor_.option_ = option; option_descriptor_.persistent_ = false; @@ -602,7 +615,7 @@ OptionDataParser::createOption(ConstElementPtr option_data) { } catch (const isc::Exception& ex) { isc_throw(DhcpConfigError, "option data does not match" << " option definition (space: " << space_param - << ", code: " << code_param << "): " + << ", code: " << def->getCode() << "): " << ex.what() << " (" << string_values_->getPosition("data", option_data) << ")"); diff --git a/src/lib/dhcpsrv/dhcp_parsers.h b/src/lib/dhcpsrv/dhcp_parsers.h index 7ab19ddfa8..f83fbfb72c 100644 --- a/src/lib/dhcpsrv/dhcp_parsers.h +++ b/src/lib/dhcpsrv/dhcp_parsers.h @@ -544,8 +544,7 @@ public: /// @brief virtual destructor to ensure orderly destruction of derivations. virtual ~OptionDataParser(){}; - -protected: +private: /// @brief Finds an option definition within an option space /// @@ -553,16 +552,18 @@ protected: /// 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 + /// @param search_key an option code or name to be used to lookup the + /// option definition. + /// @tparam A numeric type for searching using an option code or the + /// string for searching using the option name. + /// /// @return OptionDefintionPtr of the option defintion or an /// empty OptionDefinitionPtr if not found. /// @throw DhcpConfigError if the option space requested is not valid /// for this server. - virtual OptionDefinitionPtr - findOptionDefinition(const std::string& option_space, - const uint32_t option_code) const; - -private: + template + OptionDefinitionPtr findOptionDefinition(const std::string& option_space, + const SearchKey& search_key) const; /// @brief Create option instance. /// diff --git a/src/lib/dhcpsrv/tests/cfg_option_def_unittest.cc b/src/lib/dhcpsrv/tests/cfg_option_def_unittest.cc index c9dc9f7ee3..458065d5d4 100644 --- a/src/lib/dhcpsrv/tests/cfg_option_def_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_option_def_unittest.cc @@ -122,7 +122,7 @@ TEST(CfgOptionDefTest, getAll) { } // This test verifies that single option definition is correctly -// returned with getOptionDef function. +// returned with get function. TEST(CfgOptionDefTest, get) { CfgOptionDef cfg; // Create a set of option definitions with codes between 100 and 109. @@ -160,6 +160,12 @@ TEST(CfgOptionDefTest, get) { option_name << "option-" << code; EXPECT_EQ(option_name.str(), def->getName()); EXPECT_EQ(code, def->getCode()); + + // Try to get the same option definition using an option name as + // a key. + def = cfg.get("isc", option_name.str()); + ASSERT_TRUE(def); + EXPECT_EQ(code, def->getCode()); } // Check that the option codes are valid. @@ -172,7 +178,12 @@ TEST(CfgOptionDefTest, get) { std::ostringstream option_name; option_name << "option-other-" << code; EXPECT_EQ(option_name.str(), def->getName()); + EXPECT_EQ(code, def->getCode()); + // Try to get the same option definition using an option name as + // a key. + def = cfg.get("abcde", option_name.str()); + ASSERT_TRUE(def); EXPECT_EQ(code, def->getCode()); } diff --git a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc index 8bc032e684..fb64728555 100644 --- a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc +++ b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -404,7 +405,6 @@ 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) { @@ -557,7 +557,7 @@ TEST_F(ParseConfigTest, basicOptionDataTest) { // 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) { +TEST_F(ParseConfigTest, optionDataCSVFormatWithOptionDef) { std::string config = "{ \"option-data\": [ {" " \"name\": \"swap-server\"," @@ -624,7 +624,7 @@ TEST_F(ParseConfigTest, csvFormatWithOptionDef) { // 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) { +TEST_F(ParseConfigTest, optionDataCSVFormatNoOptionDef) { // 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 @@ -698,6 +698,85 @@ TEST_F(ParseConfigTest, csvFormatNoOptionDef) { EXPECT_EQ(0x56, opt->getData()[2]); } +// This test verifies that the option name is not mandatory, if the option +// code has been specified. +TEST_F(ParseConfigTest, optionDataNoName) { + std::string config = + "{ \"option-data\": [ {" + " \"space\": \"dhcp6\"," + " \"code\": 23," + " \"csv-format\": True," + " \"data\": \"2001:db8:1::1\"" + " } ]" + "}"; + int rcode = 0; + ASSERT_NO_THROW(rcode = parseConfiguration(config)); + EXPECT_EQ(0, rcode); + Option6AddrLstPtr opt = boost::dynamic_pointer_cast< + Option6AddrLst>(getOptionPtr("dhcp6", 23)); + ASSERT_TRUE(opt); + ASSERT_EQ(1, opt->getAddresses().size()); + EXPECT_EQ( "2001:db8:1::1", opt->getAddresses()[0].toText()); +} + +// This test verifies that the option code is not mandatory, if the option +// name has been specified. +TEST_F(ParseConfigTest, optionDataNoCode) { + std::string config = + "{ \"option-data\": [ {" + " \"space\": \"dhcp6\"," + " \"name\": \"dns-servers\"," + " \"csv-format\": True," + " \"data\": \"2001:db8:1::1\"" + " } ]" + "}"; + int rcode = 0; + ASSERT_NO_THROW(rcode = parseConfiguration(config)); + EXPECT_EQ(0, rcode); + Option6AddrLstPtr opt = boost::dynamic_pointer_cast< + Option6AddrLst>(getOptionPtr("dhcp6", 23)); + ASSERT_TRUE(opt); + ASSERT_EQ(1, opt->getAddresses().size()); + EXPECT_EQ( "2001:db8:1::1", opt->getAddresses()[0].toText()); +} + +// This test verifies that the option data configuration with a minimal +// set of parameters works as expected. +TEST_F(ParseConfigTest, optionDataMinimal) { + std::string config = + "{ \"option-data\": [ {" + " \"name\": \"dns-servers\"," + " \"data\": \"2001:db8:1::10\"" + " } ]" + "}"; + int rcode = 0; + ASSERT_NO_THROW(rcode = parseConfiguration(config)); + EXPECT_EQ(0, rcode); + Option6AddrLstPtr opt = boost::dynamic_pointer_cast< + Option6AddrLst>(getOptionPtr("dhcp6", 23)); + ASSERT_TRUE(opt); + ASSERT_EQ(1, opt->getAddresses().size()); + EXPECT_EQ( "2001:db8:1::10", opt->getAddresses()[0].toText()); + + CfgMgr::instance().clear(); + // This time using an option code. + config = + "{ \"option-data\": [ {" + " \"code\": 23," + " \"data\": \"2001:db8:1::20\"" + " } ]" + "}"; + rcode = 0; + ASSERT_NO_THROW(rcode = parseConfiguration(config)); + EXPECT_EQ(0, rcode); + opt = boost::dynamic_pointer_cast(getOptionPtr("dhcp6", + 23)); + ASSERT_TRUE(opt); + ASSERT_EQ(1, opt->getAddresses().size()); + EXPECT_EQ( "2001:db8:1::20", opt->getAddresses()[0].toText()); +} + + }; // Anonymous namespace /// These tests check basic operation of the HooksLibrariesParser.