[3467] Modified option data parser to support optional parameters.

Also added some more unit tests for csv-format parameter.

src/lib/dhcpsrv/dhcp_parsers.cc

@@ -344,7 +344,7 @@ OptionDataParser::commit() {
 }
 
 OptionalValue<uint32_t>
-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<uint8_t>::max()) {
         isc_throw(DhcpConfigError, "invalid option code '" << code
                 << "', it must not be greater than '"
                   << static_cast<int>(std::numeric_limits<uint8_t>::max())
-                  << "' (" << uint32_values_->getPosition("code") << ")");
+                  << "' (" << uint32_values_->getPosition("code", parent)
+                  << ")");
 
     } else if (address_family_ == AF_INET6 &&
                code > std::numeric_limits<uint16_t>::max()) {
         isc_throw(DhcpConfigError, "invalid option code '" << code
                 << "', it must not exceed '"
                   << std::numeric_limits<uint16_t>::max()
-                  << "' (" << uint32_values_->getPosition("code") << ")");
+                  << "' (" << uint32_values_->getPosition("code", parent)
+                  << ")");
 
     }
 
@@ -377,7 +379,7 @@ OptionDataParser::extractCode() const {
 }
 
 OptionalValue<std::string>
-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<std::string>(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<uint32_t> code_param =  extractCode();
-    OptionalValue<std::string> name_param = extractName();
+    OptionalValue<uint32_t> code_param =  extractCode(option_data);
+    OptionalValue<std::string> name_param = extractName(option_data);
     OptionalValue<bool> 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<uint8_t> binary;
     std::vector<std::string> 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<uint16_t>(code_param), binary));
+        OptionPtr option(new Option(universe, static_cast<uint16_t>(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)
+                      << ")");
         }
     }
 

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<std::string, data::Element::Position>::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<uint32_t> 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<uint32_t>
+    extractCode(data::ConstElementPtr parent) const;
 
-    util::OptionalValue<std::string> 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<std::string>
+    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<bool> 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.

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