[2317] Option data parser is dependent on option definitions parser.

src/bin/dhcp4/config_parser.cc

@@ -91,6 +91,9 @@ StringStorage string_defaults;
 /// @brief Global storage for options that will be used as defaults.
 OptionStorage option_defaults;
 
+/// @brief Global storage for option definitions.
+OptionDefStorage option_def_defaults;
+
 /// @brief a dummy configuration parser
 ///
 /// It is a debugging parser. It does not configure anything,
@@ -769,12 +772,22 @@ private:
             isc_throw(DhcpConfigError, "'dhcp6' option space name is reserved"
                       << " for DHCPv6 server");
         } else {
-            def = CfgMgr::instance().getOptionDef(option_space, option_code);
+            // 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.
+            OptionDefContainerPtr defs = option_def_defaults.getItems(option_space);
+            assert(defs);
+            const OptionDefContainerTypeIndex& idx = defs->get<1>();
+            OptionDefContainerTypeRange range = idx.equal_range(option_code);
+            if (std::distance(range.first, range.second) > 0) {
+                def = *range.first;
+            }
             if (!def) {
                 isc_throw(DhcpConfigError, "definition for the option '"
                           << option_space << "." << option_name
                           << "' having code '" <<  option_code
-                          << " does not exist");
+                          << "' does not exist");
             }
 
         }
@@ -834,9 +847,9 @@ private:
             // definition of option value makes sense.
             if (def->getName() != option_name) {
                 isc_throw(DhcpConfigError, "specified option name '"
-                          << option_name << " does not match the in "
+                          << option_name << " does not match the "
-                          << "option definition: '" << def->getName()
+                          << "option definition: '" << option_space
-                          << "'");
+                          << "." << def->getName() << "'");
             }
 
             // Option definition has been found so let's use it to create
@@ -849,8 +862,9 @@ private:
                 option_descriptor_.option = option;
                 option_descriptor_.persistent = false;
             } catch (const isc::Exception& ex) {
-                isc_throw(DhcpConfigError, "Parser error: option data does not match"
+                isc_throw(DhcpConfigError, "option data does not match"
-                          << " option definition (code " << option_code << "): "
+                          << " option definition (space: " << option_space
+                          << ", code: " << option_code << "): "
                           << ex.what());
             }
         }
@@ -1033,6 +1047,7 @@ public:
     void commit() {
         // @todo validate option space name once 2313 is merged.
         if (storage_ && option_definition_) {
+            std::cout << "Adding item" << std::endl;
             storage_->addItem(option_definition_, option_space_name_);
         }
     }
@@ -1146,6 +1161,10 @@ public:
     /// that define option definitions.
     /// @throw DhcpConfigError if configuration parsing fails.
     void build(ConstElementPtr option_def_list) {
+        // Clear existing items in the global storage.
+        // We are going to replace all of them.
+        option_def_defaults.clearItems();
+
         if (!option_def_list) {
             isc_throw(DhcpConfigError, "parser error: a pointer to a list of"
                       << " option definitions is NULL");
@@ -1154,7 +1173,7 @@ public:
         BOOST_FOREACH(ConstElementPtr option_def, option_def_list->listValue()) {
             boost::shared_ptr<OptionDefParser>
                 parser(new OptionDefParser("single-option-def"));
-            parser->setStorage(&option_defs_local_);
+            parser->setStorage(&option_def_defaults);
             parser->build(option_def);
             parser->commit();
         }
@@ -1170,10 +1189,10 @@ public:
         // We need to move option definitions from the temporary
         // storage to the global storage.
         BOOST_FOREACH(std::string space_name,
-                      option_defs_local_.getOptionSpaceNames()) {
+                      option_def_defaults.getOptionSpaceNames()) {
 
             BOOST_FOREACH(OptionDefinitionPtr def,
-                          *option_defs_local_.getItems(space_name)) {
+                          *option_def_defaults.getItems(space_name)) {
                 assert(def);
                 cfg_mgr.addOptionDef(def, space_name);
             }
@@ -1193,7 +1212,7 @@ private:
 
     /// Temporary storage for option definitions. It holds option
     /// definitions before \ref commit is called.
-    OptionDefStorage option_defs_local_;
+    //    OptionDefStorage option_defs_local_;
 };
 
 /// @brief this class parses a single subnet
@@ -1357,15 +1376,23 @@ private:
             subnet_->addPool4(*it);
         }
 
-        Subnet::OptionContainerPtr options = subnet_->getOptionDescriptors("dhcp4");
-        const Subnet::OptionContainerTypeIndex& idx = options->get<1>();
-
         // We have to get all option space names for options we are
         // configuring and iterate over them to add options that belong
         // to them to the subnet.
+
+        // We are going to move configured options to the Subnet object.
+        // Configured options reside in the container where options
+        // are grouped by space names. Thus we need to get all space names
+        // and iterate over all options that belong to them.
         BOOST_FOREACH(std::string option_space, options_.getOptionSpaceNames()) {
-            BOOST_FOREACH(Subnet::OptionDescriptor desc, *options_.getItems(option_space)) {
+            // Get all options within a particular option space.
+            BOOST_FOREACH(Subnet::OptionDescriptor desc,
+                          *options_.getItems(option_space)) {
+                // In theory, option should be non-NULL.
                 assert(desc.option);
+                // We want to check whether an option with the particular
+                // option code has been already added. If so, we want
+                // to issue a warning.
                 Subnet::OptionDescriptor existing_desc =
                     subnet_->getOptionDescriptor("option_space",
                                                  desc.option->getType());
@@ -1373,6 +1400,7 @@ private:
                     LOG_WARN(dhcp4_logger, DHCP4_CONFIG_OPTION_DUPLICATE)
                         .arg(desc.option->getType()).arg(addr.toText());
                 }
+                // In any case, we add the option to the subnet.
                 subnet_->addOption(desc.option, false, option_space);
             }
         }
@@ -1381,22 +1409,23 @@ private:
         // they have been configured in the global scope. If they have been
         // configured in the subnet scope we don't add global option because
         // the one configured in the subnet scope always takes precedence.
-        BOOST_FOREACH(Subnet::OptionDescriptor desc, option_defaults) {
+        BOOST_FOREACH(std::string option_space,
-            // Get all options specified locally in the subnet and having
+                      option_defaults.getOptionSpaceNames()) {
-            // code equal to global option's code.
+            // Get all global options for the particular option space.
-            Subnet::OptionContainerPtr options = subnet_->getOptionDescriptors("dhcp4");
+            BOOST_FOREACH(Subnet::OptionDescriptor desc,
-            const Subnet::OptionContainerTypeIndex& idx = options->get<1>();
+                          *option_defaults.getItems(option_space)) {
-            Subnet::OptionContainerTypeRange range = idx.equal_range(desc.option->getType());
+                assert(desc.option);
-            // @todo: In the future we will be searching for options using either
+                // Check if the particular option has been already added.
-            // an option code or namespace. Currently we have only the option
+                // This would mean that it has been configured in the
-            // code available so if there is at least one option found with the
+                // subnet scope. Since option values configured in the
-            // specific code we don't add the globally configured option.
+                // subnet scope take precedence over globally configured
-            // @todo with this code the first globally configured option
+                // values we don't add option from the global storage
-            // with the given code will be added to a subnet. We may
+                // if there is one already.
-            // want to issue a warning about dropping the configuration of
+                Subnet::OptionDescriptor existing_desc =
-            // a global option if one already exsists.
+                    subnet_->getOptionDescriptor(option_space, desc.option->getType());
-            if (std::distance(range.first, range.second) == 0) {
+                if (!existing_desc.option) {
-                subnet_->addOption(desc.option, false, "dhcp4");
+                    subnet_->addOption(desc.option, false, option_space);
+                }
             }
         }
     }
@@ -1603,6 +1632,8 @@ configureDhcp4Server(Dhcpv4Srv& , ConstElementPtr config_set) {
     // that must process data when the independent data is already
     // processed (dependent_parsers).
     ParserCollection independent_parsers;
+    ParserPtr subnet_parser;
+    ParserPtr option_parser;
     ParserCollection dependent_parsers;
 
     // The subnet parsers implement data inheritance by directly
@@ -1615,6 +1646,7 @@ configureDhcp4Server(Dhcpv4Srv& , ConstElementPtr config_set) {
     Uint32Storage uint32_local(uint32_defaults);
     StringStorage string_local(string_defaults);
     OptionStorage option_local(option_defaults);
+    OptionDefStorage option_def_local(option_def_defaults);
 
     // answer will hold the result.
     ConstElementPtr answer;
@@ -1624,11 +1656,15 @@ configureDhcp4Server(Dhcpv4Srv& , ConstElementPtr config_set) {
 
     try {
 
-        // Iterate over all independent parsers first (all but subnet4)
+        const std::map<std::string, ConstElementPtr>& values_map =
-        // and try to parse the data.
+            config_set->mapValue();
-        BOOST_FOREACH(ConfigPair config_pair, config_set->mapValue()) {
+        BOOST_FOREACH(ConfigPair config_pair, values_map) {
-            if (config_pair.first != "subnet4") {
             ParserPtr parser(createGlobalDhcp4ConfigParser(config_pair.first));
+            if (config_pair.first == "subnet4") {
+                subnet_parser = parser;
+            } else if (config_pair.first == "option-data") {
+                option_parser = parser;
+            } else {
                 independent_parsers.push_back(parser);
                 parser->build(config_pair.second);
                 // The commit operation here may modify the global storage
@@ -1638,13 +1674,18 @@ configureDhcp4Server(Dhcpv4Srv& , ConstElementPtr config_set) {
             }
         }
 
-        // Process dependent configuration data.
+        std::map<std::string, ConstElementPtr>::const_iterator option_config =
-        BOOST_FOREACH(ConfigPair config_pair, config_set->mapValue()) {
+            values_map.find("option-data");
-            if (config_pair.first == "subnet4") {
+        if (option_config != values_map.end()) {
-                ParserPtr parser(createGlobalDhcp4ConfigParser(config_pair.first));
+            option_parser->build(option_config->second);
-                dependent_parsers.push_back(parser);
+            option_parser->commit();
-                parser->build(config_pair.second);
         }
+
+        std::map<std::string, ConstElementPtr>::const_iterator subnet_config =
+            values_map.find("subnet4");
+        if (subnet_config != values_map.end()) {
+            subnet_parser->build(subnet_config->second);
+            subnet_parser->commit();
         }
 
     } catch (const isc::Exception& ex) {
@@ -1692,6 +1733,7 @@ configureDhcp4Server(Dhcpv4Srv& , ConstElementPtr config_set) {
         std::swap(uint32_defaults, uint32_local);
         std::swap(string_defaults, string_local);
         std::swap(option_defaults, option_local);
+        std::swap(option_def_defaults, option_def_local);
         return (answer);
     }
 

src/bin/dhcp4/tests/config_parser_unittest.cc

@@ -67,6 +67,7 @@ public:
     void checkResult(ConstElementPtr status, int expected_code) {
         ASSERT_TRUE(status);
         comment_ = parseAnswer(rcode_, status);
+        std::cout << comment_->str() << std::endl;
         EXPECT_EQ(expected_code, rcode_);
     }
 
@@ -872,6 +873,74 @@ TEST_F(Dhcp4ParserTest, optionDataDefaults) {
     testOption(*range.first, 23, foo2_expected, sizeof(foo2_expected));
 }
 
+/// The goal of this test is to verify that two options having the same
+/// option code can be added to different option spaces.
+TEST_F(Dhcp4ParserTest, optionDataTwoSpaces) {
+
+    // This configuration string is to configure two options
+    // sharing the code 56 and having different definitions
+    // and belonging to the different option spaces.
+    // The option definition must be provided for the
+    // option that belongs to the 'isc' option space.
+    // The definition is not required for the option that
+    // belongs to the 'dhcp4' option space as it is the
+    // standard option.
+    string config = "{ \"interface\": [ \"all\" ],"
+        "\"rebind-timer\": 2000,"
+        "\"renew-timer\": 1000,"
+        "\"option-data\": [ {"
+        "    \"name\": \"dhcp-message\","
+        "    \"space\": \"dhcp4\","
+        "    \"code\": 56,"
+        "    \"data\": \"AB CDEF0105\","
+        "    \"csv-format\": False"
+        " },"
+        " {"
+        "    \"name\": \"foo\","
+        "    \"space\": \"isc\","
+        "    \"code\": 56,"
+        "    \"data\": \"1234\","
+        "    \"csv-format\": True"
+        " } ],"
+        "\"option-def\": [ {"
+        "    \"name\": \"foo\","
+        "    \"code\": 56,"
+        "    \"type\": \"uint32\","
+        "    \"array\": False,"
+        "    \"record-types\": \"\","
+        "    \"space\": \"isc\""
+        " } ],"
+        "\"subnet4\": [ { "
+        "    \"pool\": [ \"192.0.2.1 - 192.0.2.100\" ],"
+        "    \"subnet\": \"192.0.2.0/24\""
+        " } ]"
+        "}";
+
+    ConstElementPtr status;
+   
+    ElementPtr json = Element::fromJSON(config);
+
+    EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json));
+    ASSERT_TRUE(status);
+    checkResult(status, 0);
+
+    // Options should be now availabe for the subnet.
+    Subnet4Ptr subnet = CfgMgr::instance().getSubnet4(IOAddress("192.0.2.200"));
+    ASSERT_TRUE(subnet);
+    // Try to get the option from the space dhcp4.
+    Subnet::OptionDescriptor desc1 = subnet->getOptionDescriptor("dhcp4", 56);
+    ASSERT_TRUE(desc1.option);
+    EXPECT_EQ(56, desc1.option->getType());
+    // Try to get the option from the space isc.
+    Subnet::OptionDescriptor desc2 = subnet->getOptionDescriptor("isc", 56);
+    ASSERT_TRUE(desc2.option);
+    EXPECT_EQ(56, desc1.option->getType());
+    // Try to get the non-existing option from the non-existing
+    // option space and  expect that option is not returned.
+    Subnet::OptionDescriptor desc3 = subnet->getOptionDescriptor("non-existing", 56);
+    ASSERT_FALSE(desc3.option);
+}
+
 // Goal of this test is to verify options configuration
 // for a single subnet. In particular this test checks
 // that local options configuration overrides global