From: Thomas Markwalder Date: Wed, 6 Mar 2019 18:26:01 +0000 (-0500) Subject: [#401,!254] kea-dhcp4 now merges in options from config backend X-Git-Tag: Kea-1.6.0-beta~362^2~9 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e0481e0548ae8dc639564d62be54e7b40af5447c;p=thirdparty%2Fkea.git [#401,!254] kea-dhcp4 now merges in options from config backend src/lib/dhcpsrv/cfg_option.* CfgOption::merge() - new function which merges a given set of option descriports into the existing set CfgOption::createDescriptorOption - new function that uses an option definition factory to create a descriptor's option instance src/lib/dhcpsrv/tests/cfg_option_unittest.cc TEST_F(CfgOptionTest, validMerge) TEST_F(CfgOptionTest, invalidMerge) - new tests --- diff --git a/src/lib/dhcpsrv/cfg_option.cc b/src/lib/dhcpsrv/cfg_option.cc index c7b2d06d5a..dd28e37c77 100644 --- a/src/lib/dhcpsrv/cfg_option.cc +++ b/src/lib/dhcpsrv/cfg_option.cc @@ -11,6 +11,8 @@ #include #include #include +#include +#include #include #include #include @@ -82,17 +84,87 @@ CfgOption::getVendorIdsSpaceNames() const { } void -CfgOption::merge(CfgOption& other) { +CfgOption::merge(CfgOptionDefPtr cfg_def, CfgOption& other) { // First we merge our options into other. // This adds my opitions that are not // in other, to other (i.e we skip over - // duplicates). + // duplicates). mergeTo(other); + // Iterate over all the options in all the spaces and + // validate them against the definitions. + for (auto space : other.getOptionSpaceNames()) { + for (auto opt_desc : *(other.getAll(space))) { + createDescriptorOption(cfg_def, space, opt_desc); + } + } + // Next we copy "other" on top of ourself. other.copyTo(*this); } +void +CfgOption::createDescriptorOption(CfgOptionDefPtr cfg_def, const std::string& space, + OptionDescriptor& opt_desc) { + if (!opt_desc.option_) { + isc_throw(BadValue, + "validateCreateOption: descriptor has no option instance"); + } + + Option::Universe universe = opt_desc.option_->getUniverse(); + uint16_t code = opt_desc.option_->getType(); + + // Find the option's defintion, if it has one. + // First, check for a standard definition. + OptionDefinitionPtr def = LibDHCP::getOptionDef(space, code); + + // If there is no standard definition but the option is vendor specific, + // we should search the definition within the vendor option space. + if (!def && (space != DHCP4_OPTION_SPACE) && (space != DHCP6_OPTION_SPACE)) { + uint32_t vendor_id = LibDHCP::optionSpaceToVendorId(space); + if (vendor_id > 0) { + def = LibDHCP::getVendorOptionDef(universe, vendor_id, code); + } + } + + // Still haven't found the definition, so look for custom + // definition in the given set of configured definitions + if (!def) { + def = cfg_def->get(space, code); + } + + std::string& formatted_value = opt_desc.formatted_value_; + if (!def) { + if (!formatted_value.empty()) { + isc_throw(InvalidOperation, "option: " << space << "." << code + << " has a formatted value: '" << formatted_value + << "' but no option definition"); + } + + // If there's no definition and no formatted string, we'll + // settle for the generic option already in the descriptor. + return; + } + + try { + + // Definition found. Let's replace the generic option in + // the descriptor with one created based on definition's factory. + if (formatted_value.empty()) { + // No formatted value, use data stored in the generic option. + opt_desc.option_ = def->optionFactory(universe, code, opt_desc.option_->getData()); + } else { + // Spit the value specified in comma separated values format. + std::vector split_vec; + boost::split(split_vec, formatted_value, boost::is_any_of(",")); + opt_desc.option_ = def->optionFactory(universe, code, split_vec); + } + } catch (const std::exception& ex) { + isc_throw(InvalidOperation, "could not create option: " << space << "." << code + << " from data specified, reason: " << ex.what()); + } +} + void CfgOption::mergeTo(CfgOption& other) const { // Merge non-vendor options. diff --git a/src/lib/dhcpsrv/cfg_option.h b/src/lib/dhcpsrv/cfg_option.h index 0fd0c101c8..6253f94b6e 100644 --- a/src/lib/dhcpsrv/cfg_option.h +++ b/src/lib/dhcpsrv/cfg_option.h @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -335,9 +336,12 @@ public: /// @brief Merges another option configuration into this one. /// /// This method calls @c mergeTo() to add this configuration's - /// options into @c other (skipping any duplicates). It then calls - /// @c copyTo() to overwrite this configurations' options with - /// the merged set in @c other. + /// options into @c other (skipping any duplicates). Next it calls + /// @c createDescriptorOption() for each option descriptor in the + /// merged set. This (re)-creates each descriptor's option based on + /// the merged set of opt definitioins. Finally, it calls + /// @c copyTo() to overwrite this configuration's options with + /// the merged set in @c other. /// /// @warning The merge operation will affect the @c other configuration. /// Therefore, the caller must not rely on the data held in the @c other @@ -346,7 +350,49 @@ public: /// merged configuration. /// /// @param option configurations to merge with. - void merge(CfgOption& other); + void merge(CfgOptionDefPtr cfg_def, CfgOption& other); + + /// @brief Creates an option descriptor's option based on a set of option defs + /// + /// This function's primary use is to create definition specific options for + /// option descriptors fetched from a configuration backend, as part of a + /// configuration merge. + /// + /// Given an OptionDescriptor whose option_ member contains a generic option + /// (i.e has a code and/or data), this function will attempt to find a matching + /// definition and then use that definition's factory to create an option + /// instance specific to that definition. It will then replace the descriptor's + /// generic option with the specific option. + /// + /// Three sources of definitions are searched, in the following order: + /// + /// 1. Standard option definitions (@c LIBDHCP::getOptionDef)) + /// 2. Vendor option definitions (@c LIBDHCP::getVendorOptionDef)) + /// 3. User specified definitions passed in via cfg_def parameter. + /// + /// The code will use the first matching definition found. It then applies + /// the following rules: + /// + /// -# If no definition is found but the descriptor conveys a non-empty + /// formatted value, throw an error. + /// -# If not definition is found and there is no formatted value, return + /// This leaves intact the generic option in the descriptor. + /// -# If a definition is found and there is no formatted value, pass the + /// descriptor's generic option's data into the definition's factory. Replace + /// the descriptor's option with the newly created option. + /// -# If a definition is found and there is a formatted value, split + /// the value into vector of values and pass that into the definition's + /// factory. Replace the descriptor's option with the newly created option. + /// + /// @param cfg_def the user specified definitions to use + /// @param space the option space name of the option + /// @param opt_desc OptionDescriptor describing the option. + /// + /// @throw InvalidOperation if the descriptor conveys a formatted value and + /// there is no definition matching the option code in the given space, or + /// if the definition factory invocation fails. + static void createDescriptorOption(CfgOptionDefPtr cfg_def, const std::string& space, + OptionDescriptor& opt_desc); /// @brief Merges this configuration to another configuration. /// diff --git a/src/lib/dhcpsrv/srv_config.cc b/src/lib/dhcpsrv/srv_config.cc index 6cd4d574b9..c8c2024e5b 100644 --- a/src/lib/dhcpsrv/srv_config.cc +++ b/src/lib/dhcpsrv/srv_config.cc @@ -185,10 +185,9 @@ SrvConfig::merge4(SrvConfig& other) { // Merge option defs cfg_option_def_->merge((*other.getCfgOptionDef())); - // Merge options - // @todo should we sanity check and make sure - // that there are option defs for merged options? - cfg_option_->merge((*other.getCfgOption())); + // Merge options. Note that we pass in the merged definitions + // so we can validate options against them. + cfg_option_->merge(cfg_option_def_, (*other.getCfgOption())); // Merge shared networks. cfg_shared_networks4_->merge(*(other.getCfgSharedNetworks4())); diff --git a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc index 35d5f5c756..a9bc396fe3 100644 --- a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc @@ -333,81 +333,134 @@ TEST_F(CfgOptionTest, copy) { // can be merged into an existing configuration, with any // duplicates in other overwriting those in the existing // configuration. -TEST_F(CfgOptionTest, merge) { +TEST_F(CfgOptionTest, validMerge) { CfgOption this_cfg; CfgOption other_cfg; - // Create collection of options in option space dhcp6, with option codes - // from the range of 100 to 109 and holding one byte of data equal to 0xFF. - for (uint16_t code = 100; code < 110; ++code) { - OptionPtr option(new Option(Option::V6, code, OptionBuffer(1, 0xFF))); - ASSERT_NO_THROW(this_cfg.add(option, false, DHCP6_OPTION_SPACE)); - } + // We need to create a dictionary of defintions pass into option merge. + CfgOptionDefPtr defs(new CfgOptionDef()); + defs->add((OptionDefinitionPtr(new OptionDefinition("one", 1, "uint8"))), "isc"); + defs->add((OptionDefinitionPtr(new OptionDefinition("two", 2, "uint8"))), "isc"); + defs->add((OptionDefinitionPtr(new OptionDefinition("four", 4, "uint8"))), "isc"); + defs->add((OptionDefinitionPtr(new OptionDefinition("three", 3, "uint8"))), "fluff"); + defs->add((OptionDefinitionPtr(new OptionDefinition("four", 4, "uint8"))), "fluff"); - // Create collection of options in vendor space 123, with option codes - // from the range of 100 to 109 and holding one byte of data equal to 0xFF. - for (uint16_t code = 100; code < 110; code += 2) { - OptionPtr option(new Option(Option::V6, code, OptionBuffer(1, 0xFF))); - ASSERT_NO_THROW(this_cfg.add(option, false, "vendor-123")); - } + // Create our existing config, that gets merged into. + OptionPtr option(new Option(Option::V4, 1, OptionBuffer(1, 0x01))); + ASSERT_NO_THROW(this_cfg.add(option, false, "isc")); - // Create destination configuration (configuration that we merge the - // other configuration to). + // Add option 3 to "fluff" + option.reset(new Option(Option::V4, 3, OptionBuffer(1, 0x03))); + ASSERT_NO_THROW(this_cfg.add(option, false, "fluff")); - // Create collection of options having even option codes in the range of - // 100 to 108. - for (uint16_t code = 100; code < 110; code += 2) { - OptionPtr option(new Option(Option::V6, code, OptionBuffer(1, 0x01))); - ASSERT_NO_THROW(other_cfg.add(option, false, DHCP6_OPTION_SPACE)); - } + // Add option 4 to "fluff". + option.reset(new Option(Option::V4, 4, OptionBuffer(1, 0x04))); + ASSERT_NO_THROW(this_cfg.add(option, false, "fluff")); - // Create collection of options having odd option codes in the range of - // 101 to 109. - for (uint16_t code = 101; code < 110; code += 2) { - OptionPtr option(new Option(Option::V6, code, OptionBuffer(1, 0x01))); - ASSERT_NO_THROW(other_cfg.add(option, false, "vendor-123")); - } + // Create our other config that will be merged from. + // Add Option 1 to "isc", this should "overwrite" the original. + option.reset(new Option(Option::V4, 1, OptionBuffer(1, 0x10))); + ASSERT_NO_THROW(other_cfg.add(option, false, "isc")); + + // Add option 2 to "isc". + option.reset(new Option(Option::V4, 2, OptionBuffer(1, 0x20))); + ASSERT_NO_THROW(other_cfg.add(option, false, "isc")); + + // Add option 4 to "isc". + option.reset(new Option(Option::V4, 4, OptionBuffer(1, 0x40))); + ASSERT_NO_THROW(other_cfg.add(option, false, "isc")); // Merge source configuration to the destination configuration. The options // in the destination should be preserved. The options from the source // configuration should be added. - ASSERT_NO_THROW(this_cfg.merge(other_cfg)); - + try { + this_cfg.merge(defs, other_cfg); + } catch(const std::exception& ex) { + GTEST_FAIL () << "Unexpected exception:" << ex.what(); + } + +// ASSERT_NO_THROW(this_cfg.merge(defs, other_cfg)); + + // isc:1 should come from "other" config. + OptionDescriptor desc = this_cfg.get("isc", 1); + ASSERT_TRUE(desc.option_); + ASSERT_EQ(1, desc.option_->getData().size()); + EXPECT_EQ(0x10, desc.option_->getData()[0]); + + // isc:2 should come from "other" config. + desc = this_cfg.get("isc", 2); + ASSERT_TRUE(desc.option_); + ASSERT_EQ(1, desc.option_->getData().size()); + EXPECT_EQ(0x20, desc.option_->getData()[0]); + + // isc:4 should come from "other" config. + desc = this_cfg.get("isc", 4); + ASSERT_TRUE(desc.option_); + ASSERT_EQ(1, desc.option_->getData().size()); + EXPECT_EQ(0x40, desc.option_->getData()[0]); + + // fluff:3 should come from "this" config. + desc = this_cfg.get("fluff", 3); + ASSERT_TRUE(desc.option_); + ASSERT_EQ(1, desc.option_->getData().size()); + EXPECT_EQ(0x03, desc.option_->getData()[0]); + + // fluff:4 should come from "this" config. + desc = this_cfg.get("fluff", 4); + ASSERT_TRUE(desc.option_); + ASSERT_EQ(1, desc.option_->getData().size()); + EXPECT_EQ(0x4, desc.option_->getData()[0]); +} - // Validate the options in the dhcp6 option space in the destination. - for (uint16_t code = 100; code < 110; ++code) { - OptionDescriptor desc = this_cfg.get(DHCP6_OPTION_SPACE, code); - ASSERT_TRUE(desc.option_); - ASSERT_EQ(1, desc.option_->getData().size()); - // The options with even option codes should hold one byte of data - // equal to 0x1. These are the ones that we have initially added to - // the destination configuration. The other options should hold the - // values of 0xFF which indicates that they have been merged from the - // source configuration. - if ((code % 2) == 0) { - EXPECT_EQ(0x01, desc.option_->getData()[0]); - } else { - EXPECT_EQ(0xFF, desc.option_->getData()[0]); - } - } +TEST_F(CfgOptionTest, invalidMerge) { + CfgOption this_cfg; + CfgOption other_cfg; - // Validate the options in the vendor space. - for (uint16_t code = 100; code < 110; ++code) { - OptionDescriptor desc = this_cfg.get(123, code); - ASSERT_TRUE(desc.option_); - ASSERT_EQ(1, desc.option_->getData().size()); - // This time, the options with even option codes should hold a byte - // of data equal to 0xFF. The other options should hold the byte of - // data equal to 0x01. - if ((code % 2) == 0) { - EXPECT_EQ(0xFF, desc.option_->getData()[0]); - } else { - EXPECT_EQ(0x01, desc.option_->getData()[0]); - } + // Create an empty dictionary of defintions pass into option merge. + CfgOptionDefPtr defs(new CfgOptionDef()); + + // Create our other config that will be merged from. + // Add an option without a formatted value. + OptionPtr option(new Option(Option::V4, 1, OptionBuffer(1, 0x01))); + ASSERT_NO_THROW(other_cfg.add(option, false, "isc")); + + // Add an option with a formatted value. + option.reset(new Option(Option::V4, 2)); + OptionDescriptor desc(option, false, "one,two,three"); + ASSERT_NO_THROW(other_cfg.add(desc, "isc")); + + // When we attempt to merge, it should fail, recognizing that + // option two has no definition. + try { + this_cfg.merge(defs, other_cfg); + GTEST_FAIL() << "merge should have thrown"; + } catch (const InvalidOperation& ex) { + std::string exp_msg = "option: isc.2 has a formatted value: " + "'one,two,three' but no option definition"; + EXPECT_EQ(std::string(exp_msg), std::string(ex.what())); + } catch (const std::exception& ex) { + GTEST_FAIL() << "wrong exception thrown:" << ex.what(); + } + + // Now let's add an option definition that will force data truncated + // error for option one. + defs->add((OptionDefinitionPtr(new OptionDefinition("one", 1, "uint16"))), "isc"); + + // When we attempt to merge, it should fail because option one's data + // is not valid per its definition. + try { + this_cfg.merge(defs, other_cfg); + GTEST_FAIL() << "merge should have thrown"; + } catch (const InvalidOperation& ex) { + std::string exp_msg = "could not create option: isc.1" + " from data specified, reason:" + " Option 1 truncated"; + EXPECT_EQ(std::string(exp_msg), std::string(ex.what())); + } catch (const std::exception& ex) { + GTEST_FAIL() << "wrong exception thrown:" << ex.what(); } } - // This test verifies that encapsulated options are added as sub-options // to the top level options on request. TEST_F(CfgOptionTest, encapsulate) {