From f0e6cabb47d3d38d7416bd0c623f5e86cff73306 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Mon, 4 Mar 2019 10:54:53 -0500 Subject: [PATCH 01/13] [#401,!254] kea-dhcp4 now merges CB options into current config Basic merging works, need to add validation against definitions. src/bin/dhcp4/tests/config_backend_unittest.cc TEST_F(Dhcp4CBTest, mergeOptions) - enabled and updated src/lib/dhcpsrv/cfg_option.* CfgOption::merge(CfgOption& other) - new method, conformant to others, that merges/updates a config option from another src/lib/dhcpsrv/srv_config.cc SrvConfig::merge4(SrvConfig& other) - modified to merge configured options src/lib/dhcpsrv/tests/cfg_option_unittest.cc TEST_F(CfgOptionTest, merge) - new test --- .../dhcp4/tests/config_backend_unittest.cc | 61 ++++++++----- src/lib/dhcpsrv/cfg_option.cc | 12 +++ src/lib/dhcpsrv/cfg_option.h | 18 +++- src/lib/dhcpsrv/srv_config.cc | 15 ++-- src/lib/dhcpsrv/tests/cfg_option_unittest.cc | 85 ++++++++++++++++++- 5 files changed, 161 insertions(+), 30 deletions(-) diff --git a/src/bin/dhcp4/tests/config_backend_unittest.cc b/src/bin/dhcp4/tests/config_backend_unittest.cc index dde99847c6..26d8abbb74 100644 --- a/src/bin/dhcp4/tests/config_backend_unittest.cc +++ b/src/bin/dhcp4/tests/config_backend_unittest.cc @@ -335,15 +335,19 @@ TEST_F(Dhcp4CBTest, mergeOptionDefs) { // This test verifies that externally configured options // merged correctly into staging configuration. -// @todo enable test when SrvConfig can merge options. -TEST_F(Dhcp4CBTest, DISABLED_mergeOptions) { +TEST_F(Dhcp4CBTest, mergeOptions) { string base_config = "{ \n" - " \"option-data\": [ {" - " \"name\": \"dhcp-message\"," - " \"data\": \"0A0B0C0D\"," - " \"csv-format\": false" - " } ]," + " \"option-data\": [ { \n" + " \"name\": \"dhcp-message\", \n" + " \"data\": \"0A0B0C0D\", \n" + " \"csv-format\": false \n" + " },{ \n" + " \"name\": \"host-name\", \n" + " \"data\": \"old.example.com\", \n" + " \"csv-format\": true \n" + " } \n" + " ], \n" " \"config-control\": { \n" " \"config-databases\": [ { \n" " \"type\": \"memfile\", \n" @@ -358,19 +362,28 @@ TEST_F(Dhcp4CBTest, DISABLED_mergeOptions) { extractConfig(base_config); - // Create option two and add it to first backend. - OptionDescriptorPtr opt_two(new OptionDescriptor( - createOption(Option::V4, DHO_BOOT_FILE_NAME, - true, false, "my-boot-file"))); - opt_two->space_name_ = DHCP4_OPTION_SPACE; - db1_->createUpdateOption4(ServerSelector::ALL(), opt_two); + OptionDescriptorPtr opt; - // Create option three and add it to second backend. - OptionDescriptorPtr opt_three(new OptionDescriptor( - createOption(Option::V4, DHO_BOOT_FILE_NAME, - true, false, "your-boot-file"))); - opt_three->space_name_ = DHCP4_OPTION_SPACE; - db2_->createUpdateOption4(ServerSelector::ALL(), opt_three); + // Add host-name to the first backend. + opt.reset(new OptionDescriptor( + createOption(Option::V4, DHO_HOST_NAME, + true, false, "new.example.com"))); + opt->space_name_ = DHCP4_OPTION_SPACE; + db1_->createUpdateOption4(ServerSelector::ALL(), opt); + + // Add boot-file-name to the first backend. + opt.reset(new OptionDescriptor( + createOption(Option::V4, DHO_BOOT_FILE_NAME, + true, false, "my-boot-file"))); + opt->space_name_ = DHCP4_OPTION_SPACE; + db1_->createUpdateOption4(ServerSelector::ALL(), opt); + + // Add boot-file-name to the second backend. + opt.reset(new OptionDescriptor( + createOption(Option::V4, DHO_BOOT_FILE_NAME, + true, false, "your-boot-file"))); + opt->space_name_ = DHCP4_OPTION_SPACE; + db2_->createUpdateOption4(ServerSelector::ALL(), opt); // Should parse and merge without error. ASSERT_NO_FATAL_FAILURE(configure(base_config, CONTROL_RESULT_SUCCESS, "")); @@ -381,13 +394,21 @@ TEST_F(Dhcp4CBTest, DISABLED_mergeOptions) { // Option definition from JSON should be there. CfgOptionPtr options = staging_cfg->getCfgOption(); + // dhcp-message should come from the original config. OptionDescriptor found_opt = options->get("dhcp4", DHO_DHCP_MESSAGE); ASSERT_TRUE(found_opt.option_); EXPECT_EQ("0x0A0B0C0D", found_opt.option_->toHexString()); + // host-name should come from the first back end, + // (overwriting the original). + found_opt = options->get("dhcp4", DHO_HOST_NAME); + ASSERT_TRUE(found_opt.option_); + EXPECT_EQ("new.example.com", found_opt.option_->toString()); + + // booth-file-name should come from the first back end. found_opt = options->get("dhcp4", DHO_BOOT_FILE_NAME); ASSERT_TRUE(found_opt.option_); - EXPECT_EQ("my-boot-file", found_opt.formatted_value_); + EXPECT_EQ("my-boot-file", found_opt.option_->toString()); } // This test verifies that externally configured shared-networks are diff --git a/src/lib/dhcpsrv/cfg_option.cc b/src/lib/dhcpsrv/cfg_option.cc index 71a4d5a66d..c7b2d06d5a 100644 --- a/src/lib/dhcpsrv/cfg_option.cc +++ b/src/lib/dhcpsrv/cfg_option.cc @@ -81,6 +81,18 @@ CfgOption::getVendorIdsSpaceNames() const { return (names); } +void +CfgOption::merge(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). + mergeTo(other); + + // Next we copy "other" on top of ourself. + other.copyTo(*this); +} + 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 600321f7a8..0fd0c101c8 100644 --- a/src/lib/dhcpsrv/cfg_option.h +++ b/src/lib/dhcpsrv/cfg_option.h @@ -332,6 +332,22 @@ public: /// @throw isc::BadValue if the option space is invalid. void add(const OptionDescriptor& desc, const std::string& option_space); + /// @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. + /// + /// @warning The merge operation will affect the @c other configuration. + /// Therefore, the caller must not rely on the data held in the @c other + /// object after the call to @c merge. Also, the data held in @c other must + /// not be modified after the call to @c merge because it may affect the + /// merged configuration. + /// + /// @param option configurations to merge with. + void merge(CfgOption& other); + /// @brief Merges this configuration to another configuration. /// /// This method iterates over the configuration items held in this @@ -339,8 +355,6 @@ public: /// as a parameter. If an item exists in the destination it is not /// copied. /// - /// @note: this method is not longer used so should become private. - /// /// @param [out] other Configuration object to merge to. void mergeTo(CfgOption& other) const; diff --git a/src/lib/dhcpsrv/srv_config.cc b/src/lib/dhcpsrv/srv_config.cc index f335fc5360..6cd4d574b9 100644 --- a/src/lib/dhcpsrv/srv_config.cc +++ b/src/lib/dhcpsrv/srv_config.cc @@ -177,23 +177,26 @@ SrvConfig::merge(ConfigBase& other) { void SrvConfig::merge4(SrvConfig& other) { - /// We merge objects in order of dependency (real or theoretical). + // We merge objects in order of dependency (real or theoretical). - /// Merge globals. + // Merge globals. mergeGlobals4(other); - /// Merge option defs + // Merge option defs cfg_option_def_->merge((*other.getCfgOptionDef())); - /// @todo merge options + // Merge options + // @todo should we sanity check and make sure + // that there are option defs for merged options? + cfg_option_->merge((*other.getCfgOption())); // Merge shared networks. cfg_shared_networks4_->merge(*(other.getCfgSharedNetworks4())); - /// Merge subnets. + // Merge subnets. cfg_subnets4_->merge(getCfgSharedNetworks4(), *(other.getCfgSubnets4())); - /// @todo merge other parts of the configuration here. + // @todo merge other parts of the configuration here. } void diff --git a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc index 7439f70c3a..35d5f5c756 100644 --- a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc @@ -213,8 +213,8 @@ TEST_F(CfgOptionTest, add) { EXPECT_TRUE(options->empty()); } -// This test verifies that two option configurations can be merged. -TEST_F(CfgOptionTest, merge) { +// This test verifies that one configuration can be merged into another. +TEST_F(CfgOptionTest, mergeTo) { CfgOption cfg_src; CfgOption cfg_dst; @@ -327,6 +327,87 @@ TEST_F(CfgOptionTest, copy) { EXPECT_EQ(10, container->size()); } +// This test verifies option definitions from one configuration +// can be used to update definitions in another configuration. +// In other words, definitions from an "other" configuration +// can be merged into an existing configuration, with any +// duplicates in other overwriting those in the existing +// configuration. +TEST_F(CfgOptionTest, merge) { + 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)); + } + + // 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 destination configuration (configuration that we merge the + // other configuration to). + + // 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)); + } + + // 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")); + } + + // 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)); + + + // 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]); + } + } + + // 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]); + } + } +} + + // This test verifies that encapsulated options are added as sub-options // to the top level options on request. TEST_F(CfgOptionTest, encapsulate) { From 3d838cbcb71de7342e326d1df7e206a1a1387eca Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Wed, 6 Mar 2019 13:26:01 -0500 Subject: [PATCH 02/13] [#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 --- src/lib/dhcpsrv/cfg_option.cc | 76 ++++++++- src/lib/dhcpsrv/cfg_option.h | 54 +++++- src/lib/dhcpsrv/srv_config.cc | 7 +- src/lib/dhcpsrv/tests/cfg_option_unittest.cc | 171 ++++++++++++------- 4 files changed, 239 insertions(+), 69 deletions(-) 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,80 +333,133 @@ 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)); - - - // 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]); - } + try { + this_cfg.merge(defs, other_cfg); + } catch(const std::exception& ex) { + GTEST_FAIL () << "Unexpected exception:" << ex.what(); } - // 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]); - } - } +// 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]); } +TEST_F(CfgOptionTest, invalidMerge) { + CfgOption this_cfg; + CfgOption other_cfg; + + // 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. From af7fbbe37fef3d77ea815da58a07d70c7c137fef Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Fri, 8 Mar 2019 16:39:19 -0500 Subject: [PATCH 03/13] [#401,!24] Addressed most review comments This commit addresse the more trivial review comments, it does not include option creation for networks,subnets, or pools. Will rebase and address remaining comments. --- src/lib/dhcpsrv/cfg_option.cc | 5 +- src/lib/dhcpsrv/srv_config.cc | 2 +- src/lib/dhcpsrv/tests/cfg_option_unittest.cc | 98 ++++++++++++++++---- 3 files changed, 86 insertions(+), 19 deletions(-) diff --git a/src/lib/dhcpsrv/cfg_option.cc b/src/lib/dhcpsrv/cfg_option.cc index dd28e37c77..feb385f35f 100644 --- a/src/lib/dhcpsrv/cfg_option.cc +++ b/src/lib/dhcpsrv/cfg_option.cc @@ -86,7 +86,7 @@ CfgOption::getVendorIdsSpaceNames() const { void CfgOption::merge(CfgOptionDefPtr cfg_def, CfgOption& other) { // First we merge our options into other. - // This adds my opitions that are not + // This adds my options that are not // in other, to other (i.e we skip over // duplicates). mergeTo(other); @@ -148,6 +148,9 @@ CfgOption::createDescriptorOption(CfgOptionDefPtr cfg_def, const std::string& sp try { + std::cout << "def:" << def->getName() << ", code:" << def->getCode() + << ", type: " << def->getType() << std::endl; + // Definition found. Let's replace the generic option in // the descriptor with one created based on definition's factory. if (formatted_value.empty()) { diff --git a/src/lib/dhcpsrv/srv_config.cc b/src/lib/dhcpsrv/srv_config.cc index c8c2024e5b..cda2708d84 100644 --- a/src/lib/dhcpsrv/srv_config.cc +++ b/src/lib/dhcpsrv/srv_config.cc @@ -195,7 +195,7 @@ SrvConfig::merge4(SrvConfig& other) { // Merge subnets. cfg_subnets4_->merge(getCfgSharedNetworks4(), *(other.getCfgSubnets4())); - // @todo merge other parts of the configuration here. + /// @todo merge other parts of the configuration here. } void diff --git a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc index a9bc396fe3..0127894dfa 100644 --- a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc @@ -8,7 +8,10 @@ #include #include #include +#include #include +#include +#include #include #include #include @@ -327,12 +330,12 @@ TEST_F(CfgOptionTest, copy) { EXPECT_EQ(10, container->size()); } -// This test verifies option definitions from one configuration -// can be used to update definitions in another configuration. -// In other words, definitions from an "other" configuration +// This test verifies that DHCP options from one configuration +// can be used to update options in another configuration. +// In other words, options from an "other" configuration // can be merged into an existing configuration, with any // duplicates in other overwriting those in the existing -// configuration. +// configuration. TEST_F(CfgOptionTest, validMerge) { CfgOption this_cfg; CfgOption other_cfg; @@ -358,7 +361,7 @@ TEST_F(CfgOptionTest, validMerge) { ASSERT_NO_THROW(this_cfg.add(option, false, "fluff")); // Create our other config that will be merged from. - // Add Option 1 to "isc", this should "overwrite" the original. + // 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")); @@ -366,20 +369,14 @@ TEST_F(CfgOptionTest, validMerge) { option.reset(new Option(Option::V4, 2, OptionBuffer(1, 0x20))); ASSERT_NO_THROW(other_cfg.add(option, false, "isc")); - // Add option 4 to "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. - 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)); + ASSERT_NO_THROW(this_cfg.merge(defs, other_cfg)); // isc:1 should come from "other" config. OptionDescriptor desc = this_cfg.get("isc", 1); @@ -412,7 +409,10 @@ TEST_F(CfgOptionTest, validMerge) { EXPECT_EQ(0x4, desc.option_->getData()[0]); } -TEST_F(CfgOptionTest, invalidMerge) { +// This test verifies that attempting to merge options +// which are by incompatible with "known" option definitions +// are detected. +TEST_F(CfgOptionTest, mergeInvalid) { CfgOption this_cfg; CfgOption other_cfg; @@ -430,7 +430,7 @@ TEST_F(CfgOptionTest, invalidMerge) { ASSERT_NO_THROW(other_cfg.add(desc, "isc")); // When we attempt to merge, it should fail, recognizing that - // option two has no definition. + // option 2, which has a formatted value, has no definition. try { this_cfg.merge(defs, other_cfg); GTEST_FAIL() << "merge should have thrown"; @@ -443,10 +443,10 @@ TEST_F(CfgOptionTest, invalidMerge) { } // Now let's add an option definition that will force data truncated - // error for option one. + // error for option 1. defs->add((OptionDefinitionPtr(new OptionDefinition("one", 1, "uint16"))), "isc"); - // When we attempt to merge, it should fail because option one's data + // When we attempt to merge, it should fail because option 1's data // is not valid per its definition. try { this_cfg.merge(defs, other_cfg); @@ -461,6 +461,70 @@ TEST_F(CfgOptionTest, invalidMerge) { } } +// This test verifies the all of the valid option cases +// in createDescriptorOption(): +// 1. standard option +// 2. vendor option +// 3. user-defined, unformatted option +// 4. user-defined, formatted option +// 5. undefined, unformatted option +TEST_F(CfgOptionTest, createDescriptorOptionValid) { + // First we'll create our "known" user definitions + CfgOptionDefPtr defs(new CfgOptionDef()); + defs->add((OptionDefinitionPtr(new OptionDefinition("one", 1, "uint8"))), "isc"); + defs->add((OptionDefinitionPtr(new OptionDefinition("two", 2, "uint8", true))), "isc"); + + // We'll try a standard option first. + std::string space = "dhcp4"; + std::string value("example.org"); + OptionPtr option(new Option(Option::V4, DHO_HOST_NAME)); + option->setData(value.begin(), value.end()); + OptionDescriptorPtr desc(new OptionDescriptor(option, false)); + + ASSERT_NO_THROW(CfgOption::createDescriptorOption(defs, space, *desc)); + OptionStringPtr opstr = boost::dynamic_pointer_cast(desc->option_); + ASSERT_TRUE(opstr); + EXPECT_EQ("type=012, len=011: \"example.org\" (string)", opstr->toText()); + + // Next we'll try a vendor option with a formatted value + space = "vendor-4491"; + value = "192.0.2.1, 192.0.2.2"; + option.reset(new Option(Option::V4, 2)); + desc.reset(new OptionDescriptor(option, false, value)); + + ASSERT_NO_THROW(CfgOption::createDescriptorOption(defs, space, *desc)); + std::cout << "option:" << desc->option_->toText() << std::endl; + Option4AddrLstPtr opaddrs = boost::dynamic_pointer_cast(desc->option_); + ASSERT_TRUE(opaddrs); + EXPECT_EQ("type=002, len=008: 192.0.2.1 192.0.2.2", opaddrs->toText()); + + // Now, a user defined uint8 option + space = "isc"; + option.reset(new Option(Option::V4, 1, OptionBuffer(1, 0x77))); + desc.reset(new OptionDescriptor(option, false)); + + ASSERT_NO_THROW(CfgOption::createDescriptorOption(defs, space, *desc)); + OptionUint8Ptr opint = boost::dynamic_pointer_cast(desc->option_); + ASSERT_TRUE(opint); + EXPECT_EQ("type=001, len=001: 119 (uint8)", opint->toText()); + + // Now, a user defined array of strings + option.reset(new Option(Option::V4, 2)); + desc.reset(new OptionDescriptor(option, false, "1,2,3")); + + ASSERT_NO_THROW(CfgOption::createDescriptorOption(defs, space, *desc)); + OptionUint8ArrayPtr oparray = boost::dynamic_pointer_cast(desc->option_); + ASSERT_TRUE(oparray); + EXPECT_EQ("type=002, len=003: 1(uint8) 2(uint8) 3(uint8)", oparray->toText()); + + // Finally, a generic, undefined option + option.reset(new Option(Option::V4, 2, OptionBuffer(1, 0x77))); + desc.reset(new OptionDescriptor(option, false)); + + ASSERT_NO_THROW(CfgOption::createDescriptorOption(defs, space, *desc)); + EXPECT_EQ("type=002, len=001: 119(uint8)", desc->option_->toText()); +} + // This test verifies that encapsulated options are added as sub-options // to the top level options on request. TEST_F(CfgOptionTest, encapsulate) { From 1a663d09e49eb40d2f5132d8af2cda810d660390 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Mon, 4 Mar 2019 10:54:53 -0500 Subject: [PATCH 04/13] [#401,!254] kea-dhcp4 now merges CB options into current config Basic merging works, need to add validation against definitions. src/bin/dhcp4/tests/config_backend_unittest.cc TEST_F(Dhcp4CBTest, mergeOptions) - enabled and updated src/lib/dhcpsrv/cfg_option.* CfgOption::merge(CfgOption& other) - new method, conformant to others, that merges/updates a config option from another src/lib/dhcpsrv/srv_config.cc SrvConfig::merge4(SrvConfig& other) - modified to merge configured options src/lib/dhcpsrv/tests/cfg_option_unittest.cc TEST_F(CfgOptionTest, merge) - new test --- .../dhcp4/tests/config_backend_unittest.cc | 61 ++++++++----- src/lib/dhcpsrv/cfg_option.cc | 12 +++ src/lib/dhcpsrv/cfg_option.h | 18 +++- src/lib/dhcpsrv/srv_config.cc | 15 ++-- src/lib/dhcpsrv/tests/cfg_option_unittest.cc | 85 ++++++++++++++++++- 5 files changed, 161 insertions(+), 30 deletions(-) diff --git a/src/bin/dhcp4/tests/config_backend_unittest.cc b/src/bin/dhcp4/tests/config_backend_unittest.cc index dde99847c6..26d8abbb74 100644 --- a/src/bin/dhcp4/tests/config_backend_unittest.cc +++ b/src/bin/dhcp4/tests/config_backend_unittest.cc @@ -335,15 +335,19 @@ TEST_F(Dhcp4CBTest, mergeOptionDefs) { // This test verifies that externally configured options // merged correctly into staging configuration. -// @todo enable test when SrvConfig can merge options. -TEST_F(Dhcp4CBTest, DISABLED_mergeOptions) { +TEST_F(Dhcp4CBTest, mergeOptions) { string base_config = "{ \n" - " \"option-data\": [ {" - " \"name\": \"dhcp-message\"," - " \"data\": \"0A0B0C0D\"," - " \"csv-format\": false" - " } ]," + " \"option-data\": [ { \n" + " \"name\": \"dhcp-message\", \n" + " \"data\": \"0A0B0C0D\", \n" + " \"csv-format\": false \n" + " },{ \n" + " \"name\": \"host-name\", \n" + " \"data\": \"old.example.com\", \n" + " \"csv-format\": true \n" + " } \n" + " ], \n" " \"config-control\": { \n" " \"config-databases\": [ { \n" " \"type\": \"memfile\", \n" @@ -358,19 +362,28 @@ TEST_F(Dhcp4CBTest, DISABLED_mergeOptions) { extractConfig(base_config); - // Create option two and add it to first backend. - OptionDescriptorPtr opt_two(new OptionDescriptor( - createOption(Option::V4, DHO_BOOT_FILE_NAME, - true, false, "my-boot-file"))); - opt_two->space_name_ = DHCP4_OPTION_SPACE; - db1_->createUpdateOption4(ServerSelector::ALL(), opt_two); + OptionDescriptorPtr opt; - // Create option three and add it to second backend. - OptionDescriptorPtr opt_three(new OptionDescriptor( - createOption(Option::V4, DHO_BOOT_FILE_NAME, - true, false, "your-boot-file"))); - opt_three->space_name_ = DHCP4_OPTION_SPACE; - db2_->createUpdateOption4(ServerSelector::ALL(), opt_three); + // Add host-name to the first backend. + opt.reset(new OptionDescriptor( + createOption(Option::V4, DHO_HOST_NAME, + true, false, "new.example.com"))); + opt->space_name_ = DHCP4_OPTION_SPACE; + db1_->createUpdateOption4(ServerSelector::ALL(), opt); + + // Add boot-file-name to the first backend. + opt.reset(new OptionDescriptor( + createOption(Option::V4, DHO_BOOT_FILE_NAME, + true, false, "my-boot-file"))); + opt->space_name_ = DHCP4_OPTION_SPACE; + db1_->createUpdateOption4(ServerSelector::ALL(), opt); + + // Add boot-file-name to the second backend. + opt.reset(new OptionDescriptor( + createOption(Option::V4, DHO_BOOT_FILE_NAME, + true, false, "your-boot-file"))); + opt->space_name_ = DHCP4_OPTION_SPACE; + db2_->createUpdateOption4(ServerSelector::ALL(), opt); // Should parse and merge without error. ASSERT_NO_FATAL_FAILURE(configure(base_config, CONTROL_RESULT_SUCCESS, "")); @@ -381,13 +394,21 @@ TEST_F(Dhcp4CBTest, DISABLED_mergeOptions) { // Option definition from JSON should be there. CfgOptionPtr options = staging_cfg->getCfgOption(); + // dhcp-message should come from the original config. OptionDescriptor found_opt = options->get("dhcp4", DHO_DHCP_MESSAGE); ASSERT_TRUE(found_opt.option_); EXPECT_EQ("0x0A0B0C0D", found_opt.option_->toHexString()); + // host-name should come from the first back end, + // (overwriting the original). + found_opt = options->get("dhcp4", DHO_HOST_NAME); + ASSERT_TRUE(found_opt.option_); + EXPECT_EQ("new.example.com", found_opt.option_->toString()); + + // booth-file-name should come from the first back end. found_opt = options->get("dhcp4", DHO_BOOT_FILE_NAME); ASSERT_TRUE(found_opt.option_); - EXPECT_EQ("my-boot-file", found_opt.formatted_value_); + EXPECT_EQ("my-boot-file", found_opt.option_->toString()); } // This test verifies that externally configured shared-networks are diff --git a/src/lib/dhcpsrv/cfg_option.cc b/src/lib/dhcpsrv/cfg_option.cc index 71a4d5a66d..c7b2d06d5a 100644 --- a/src/lib/dhcpsrv/cfg_option.cc +++ b/src/lib/dhcpsrv/cfg_option.cc @@ -81,6 +81,18 @@ CfgOption::getVendorIdsSpaceNames() const { return (names); } +void +CfgOption::merge(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). + mergeTo(other); + + // Next we copy "other" on top of ourself. + other.copyTo(*this); +} + 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 600321f7a8..0fd0c101c8 100644 --- a/src/lib/dhcpsrv/cfg_option.h +++ b/src/lib/dhcpsrv/cfg_option.h @@ -332,6 +332,22 @@ public: /// @throw isc::BadValue if the option space is invalid. void add(const OptionDescriptor& desc, const std::string& option_space); + /// @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. + /// + /// @warning The merge operation will affect the @c other configuration. + /// Therefore, the caller must not rely on the data held in the @c other + /// object after the call to @c merge. Also, the data held in @c other must + /// not be modified after the call to @c merge because it may affect the + /// merged configuration. + /// + /// @param option configurations to merge with. + void merge(CfgOption& other); + /// @brief Merges this configuration to another configuration. /// /// This method iterates over the configuration items held in this @@ -339,8 +355,6 @@ public: /// as a parameter. If an item exists in the destination it is not /// copied. /// - /// @note: this method is not longer used so should become private. - /// /// @param [out] other Configuration object to merge to. void mergeTo(CfgOption& other) const; diff --git a/src/lib/dhcpsrv/srv_config.cc b/src/lib/dhcpsrv/srv_config.cc index f335fc5360..6cd4d574b9 100644 --- a/src/lib/dhcpsrv/srv_config.cc +++ b/src/lib/dhcpsrv/srv_config.cc @@ -177,23 +177,26 @@ SrvConfig::merge(ConfigBase& other) { void SrvConfig::merge4(SrvConfig& other) { - /// We merge objects in order of dependency (real or theoretical). + // We merge objects in order of dependency (real or theoretical). - /// Merge globals. + // Merge globals. mergeGlobals4(other); - /// Merge option defs + // Merge option defs cfg_option_def_->merge((*other.getCfgOptionDef())); - /// @todo merge options + // Merge options + // @todo should we sanity check and make sure + // that there are option defs for merged options? + cfg_option_->merge((*other.getCfgOption())); // Merge shared networks. cfg_shared_networks4_->merge(*(other.getCfgSharedNetworks4())); - /// Merge subnets. + // Merge subnets. cfg_subnets4_->merge(getCfgSharedNetworks4(), *(other.getCfgSubnets4())); - /// @todo merge other parts of the configuration here. + // @todo merge other parts of the configuration here. } void diff --git a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc index 7439f70c3a..35d5f5c756 100644 --- a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc @@ -213,8 +213,8 @@ TEST_F(CfgOptionTest, add) { EXPECT_TRUE(options->empty()); } -// This test verifies that two option configurations can be merged. -TEST_F(CfgOptionTest, merge) { +// This test verifies that one configuration can be merged into another. +TEST_F(CfgOptionTest, mergeTo) { CfgOption cfg_src; CfgOption cfg_dst; @@ -327,6 +327,87 @@ TEST_F(CfgOptionTest, copy) { EXPECT_EQ(10, container->size()); } +// This test verifies option definitions from one configuration +// can be used to update definitions in another configuration. +// In other words, definitions from an "other" configuration +// can be merged into an existing configuration, with any +// duplicates in other overwriting those in the existing +// configuration. +TEST_F(CfgOptionTest, merge) { + 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)); + } + + // 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 destination configuration (configuration that we merge the + // other configuration to). + + // 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)); + } + + // 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")); + } + + // 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)); + + + // 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]); + } + } + + // 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]); + } + } +} + + // This test verifies that encapsulated options are added as sub-options // to the top level options on request. TEST_F(CfgOptionTest, encapsulate) { From e0481e0548ae8dc639564d62be54e7b40af5447c Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Wed, 6 Mar 2019 13:26:01 -0500 Subject: [PATCH 05/13] [#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 --- src/lib/dhcpsrv/cfg_option.cc | 76 ++++++++- src/lib/dhcpsrv/cfg_option.h | 54 +++++- src/lib/dhcpsrv/srv_config.cc | 7 +- src/lib/dhcpsrv/tests/cfg_option_unittest.cc | 171 ++++++++++++------- 4 files changed, 239 insertions(+), 69 deletions(-) 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,80 +333,133 @@ 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)); - - - // 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]); - } + try { + this_cfg.merge(defs, other_cfg); + } catch(const std::exception& ex) { + GTEST_FAIL () << "Unexpected exception:" << ex.what(); } - // 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]); - } - } +// 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]); } +TEST_F(CfgOptionTest, invalidMerge) { + CfgOption this_cfg; + CfgOption other_cfg; + + // 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. From 65f8e39cd91dda54dbabef857b06f66f43e28c70 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Fri, 8 Mar 2019 16:39:19 -0500 Subject: [PATCH 06/13] [#401,!24] Addressed most review comments This commit addresse the more trivial review comments, it does not include option creation for networks,subnets, or pools. Will rebase and address remaining comments. --- src/lib/dhcpsrv/cfg_option.cc | 5 +- src/lib/dhcpsrv/srv_config.cc | 2 +- src/lib/dhcpsrv/tests/cfg_option_unittest.cc | 98 ++++++++++++++++---- 3 files changed, 86 insertions(+), 19 deletions(-) diff --git a/src/lib/dhcpsrv/cfg_option.cc b/src/lib/dhcpsrv/cfg_option.cc index dd28e37c77..feb385f35f 100644 --- a/src/lib/dhcpsrv/cfg_option.cc +++ b/src/lib/dhcpsrv/cfg_option.cc @@ -86,7 +86,7 @@ CfgOption::getVendorIdsSpaceNames() const { void CfgOption::merge(CfgOptionDefPtr cfg_def, CfgOption& other) { // First we merge our options into other. - // This adds my opitions that are not + // This adds my options that are not // in other, to other (i.e we skip over // duplicates). mergeTo(other); @@ -148,6 +148,9 @@ CfgOption::createDescriptorOption(CfgOptionDefPtr cfg_def, const std::string& sp try { + std::cout << "def:" << def->getName() << ", code:" << def->getCode() + << ", type: " << def->getType() << std::endl; + // Definition found. Let's replace the generic option in // the descriptor with one created based on definition's factory. if (formatted_value.empty()) { diff --git a/src/lib/dhcpsrv/srv_config.cc b/src/lib/dhcpsrv/srv_config.cc index c8c2024e5b..cda2708d84 100644 --- a/src/lib/dhcpsrv/srv_config.cc +++ b/src/lib/dhcpsrv/srv_config.cc @@ -195,7 +195,7 @@ SrvConfig::merge4(SrvConfig& other) { // Merge subnets. cfg_subnets4_->merge(getCfgSharedNetworks4(), *(other.getCfgSubnets4())); - // @todo merge other parts of the configuration here. + /// @todo merge other parts of the configuration here. } void diff --git a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc index a9bc396fe3..0127894dfa 100644 --- a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc @@ -8,7 +8,10 @@ #include #include #include +#include #include +#include +#include #include #include #include @@ -327,12 +330,12 @@ TEST_F(CfgOptionTest, copy) { EXPECT_EQ(10, container->size()); } -// This test verifies option definitions from one configuration -// can be used to update definitions in another configuration. -// In other words, definitions from an "other" configuration +// This test verifies that DHCP options from one configuration +// can be used to update options in another configuration. +// In other words, options from an "other" configuration // can be merged into an existing configuration, with any // duplicates in other overwriting those in the existing -// configuration. +// configuration. TEST_F(CfgOptionTest, validMerge) { CfgOption this_cfg; CfgOption other_cfg; @@ -358,7 +361,7 @@ TEST_F(CfgOptionTest, validMerge) { ASSERT_NO_THROW(this_cfg.add(option, false, "fluff")); // Create our other config that will be merged from. - // Add Option 1 to "isc", this should "overwrite" the original. + // 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")); @@ -366,20 +369,14 @@ TEST_F(CfgOptionTest, validMerge) { option.reset(new Option(Option::V4, 2, OptionBuffer(1, 0x20))); ASSERT_NO_THROW(other_cfg.add(option, false, "isc")); - // Add option 4 to "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. - 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)); + ASSERT_NO_THROW(this_cfg.merge(defs, other_cfg)); // isc:1 should come from "other" config. OptionDescriptor desc = this_cfg.get("isc", 1); @@ -412,7 +409,10 @@ TEST_F(CfgOptionTest, validMerge) { EXPECT_EQ(0x4, desc.option_->getData()[0]); } -TEST_F(CfgOptionTest, invalidMerge) { +// This test verifies that attempting to merge options +// which are by incompatible with "known" option definitions +// are detected. +TEST_F(CfgOptionTest, mergeInvalid) { CfgOption this_cfg; CfgOption other_cfg; @@ -430,7 +430,7 @@ TEST_F(CfgOptionTest, invalidMerge) { ASSERT_NO_THROW(other_cfg.add(desc, "isc")); // When we attempt to merge, it should fail, recognizing that - // option two has no definition. + // option 2, which has a formatted value, has no definition. try { this_cfg.merge(defs, other_cfg); GTEST_FAIL() << "merge should have thrown"; @@ -443,10 +443,10 @@ TEST_F(CfgOptionTest, invalidMerge) { } // Now let's add an option definition that will force data truncated - // error for option one. + // error for option 1. defs->add((OptionDefinitionPtr(new OptionDefinition("one", 1, "uint16"))), "isc"); - // When we attempt to merge, it should fail because option one's data + // When we attempt to merge, it should fail because option 1's data // is not valid per its definition. try { this_cfg.merge(defs, other_cfg); @@ -461,6 +461,70 @@ TEST_F(CfgOptionTest, invalidMerge) { } } +// This test verifies the all of the valid option cases +// in createDescriptorOption(): +// 1. standard option +// 2. vendor option +// 3. user-defined, unformatted option +// 4. user-defined, formatted option +// 5. undefined, unformatted option +TEST_F(CfgOptionTest, createDescriptorOptionValid) { + // First we'll create our "known" user definitions + CfgOptionDefPtr defs(new CfgOptionDef()); + defs->add((OptionDefinitionPtr(new OptionDefinition("one", 1, "uint8"))), "isc"); + defs->add((OptionDefinitionPtr(new OptionDefinition("two", 2, "uint8", true))), "isc"); + + // We'll try a standard option first. + std::string space = "dhcp4"; + std::string value("example.org"); + OptionPtr option(new Option(Option::V4, DHO_HOST_NAME)); + option->setData(value.begin(), value.end()); + OptionDescriptorPtr desc(new OptionDescriptor(option, false)); + + ASSERT_NO_THROW(CfgOption::createDescriptorOption(defs, space, *desc)); + OptionStringPtr opstr = boost::dynamic_pointer_cast(desc->option_); + ASSERT_TRUE(opstr); + EXPECT_EQ("type=012, len=011: \"example.org\" (string)", opstr->toText()); + + // Next we'll try a vendor option with a formatted value + space = "vendor-4491"; + value = "192.0.2.1, 192.0.2.2"; + option.reset(new Option(Option::V4, 2)); + desc.reset(new OptionDescriptor(option, false, value)); + + ASSERT_NO_THROW(CfgOption::createDescriptorOption(defs, space, *desc)); + std::cout << "option:" << desc->option_->toText() << std::endl; + Option4AddrLstPtr opaddrs = boost::dynamic_pointer_cast(desc->option_); + ASSERT_TRUE(opaddrs); + EXPECT_EQ("type=002, len=008: 192.0.2.1 192.0.2.2", opaddrs->toText()); + + // Now, a user defined uint8 option + space = "isc"; + option.reset(new Option(Option::V4, 1, OptionBuffer(1, 0x77))); + desc.reset(new OptionDescriptor(option, false)); + + ASSERT_NO_THROW(CfgOption::createDescriptorOption(defs, space, *desc)); + OptionUint8Ptr opint = boost::dynamic_pointer_cast(desc->option_); + ASSERT_TRUE(opint); + EXPECT_EQ("type=001, len=001: 119 (uint8)", opint->toText()); + + // Now, a user defined array of strings + option.reset(new Option(Option::V4, 2)); + desc.reset(new OptionDescriptor(option, false, "1,2,3")); + + ASSERT_NO_THROW(CfgOption::createDescriptorOption(defs, space, *desc)); + OptionUint8ArrayPtr oparray = boost::dynamic_pointer_cast(desc->option_); + ASSERT_TRUE(oparray); + EXPECT_EQ("type=002, len=003: 1(uint8) 2(uint8) 3(uint8)", oparray->toText()); + + // Finally, a generic, undefined option + option.reset(new Option(Option::V4, 2, OptionBuffer(1, 0x77))); + desc.reset(new OptionDescriptor(option, false)); + + ASSERT_NO_THROW(CfgOption::createDescriptorOption(defs, space, *desc)); + EXPECT_EQ("type=002, len=001: 119(uint8)", desc->option_->toText()); +} + // This test verifies that encapsulated options are added as sub-options // to the top level options on request. TEST_F(CfgOptionTest, encapsulate) { From d55b3c695e74d2353d0a5930ab138f3680657a1d Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Thu, 14 Mar 2019 13:54:41 -0400 Subject: [PATCH 07/13] [#401,!254] Options are now created when merging shared-network4s src/lib/dhcpsrv/cfg_option.* CfgOption::createOptions(CfgOptionDefPtr cfg_def) - new function which creates options for all of a cfg's descriptors CfgOption::merge() - calls createOptions() src/lib/dhcpsrv/cfg_shared_networks.* CfgSharedNetworks4::merge() - added cfg_def parameter and call to populate the "other" networks' options src/lib/dhcpsrv/srv_config.cc SrvConfig::merge4() - passes merged option definitions into shared network merge. --- src/lib/dhcpsrv/cfg_option.cc | 41 +++++++++++++------ src/lib/dhcpsrv/cfg_option.h | 4 ++ src/lib/dhcpsrv/cfg_shared_networks.cc | 7 +++- src/lib/dhcpsrv/cfg_shared_networks.h | 5 ++- src/lib/dhcpsrv/srv_config.cc | 10 +++-- src/lib/dhcpsrv/tests/cfg_option_unittest.cc | 25 +++++------ .../tests/cfg_shared_networks4_unittest.cc | 26 ++++++++++-- 7 files changed, 84 insertions(+), 34 deletions(-) diff --git a/src/lib/dhcpsrv/cfg_option.cc b/src/lib/dhcpsrv/cfg_option.cc index feb385f35f..e1268322c2 100644 --- a/src/lib/dhcpsrv/cfg_option.cc +++ b/src/lib/dhcpsrv/cfg_option.cc @@ -91,18 +91,36 @@ CfgOption::merge(CfgOptionDefPtr cfg_def, CfgOption& other) { // 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); - } - } + // Create option instances based on the given definitions. + other.createOptions(cfg_def); // Next we copy "other" on top of ourself. other.copyTo(*this); } +void +CfgOption::createOptions(CfgOptionDefPtr cfg_def) { + // Iterate over all the option descriptors in + // all the spaces and instantiate the options + // based on the given definitions. + + // Descriptors can only be fetched as copies of + // what is in the container. Since we don't + // currently have a replace mechanism, we'll + // create a revamped set of descriptors and then + // copy them on top of ourself. + CfgOption revamped; + for (auto space : getOptionSpaceNames()) { + for (auto opt_desc : *(getAll(space))) { + createDescriptorOption(cfg_def, space, opt_desc); + revamped.add(opt_desc, space); + } + } + + // Copy the updated descriptors over our own. + revamped.copyTo(*this); +} + void CfgOption::createDescriptorOption(CfgOptionDefPtr cfg_def, const std::string& space, OptionDescriptor& opt_desc) { @@ -111,6 +129,7 @@ CfgOption::createDescriptorOption(CfgOptionDefPtr cfg_def, const std::string& sp "validateCreateOption: descriptor has no option instance"); } + Option::Universe universe = opt_desc.option_->getUniverse(); uint16_t code = opt_desc.option_->getType(); @@ -141,16 +160,12 @@ CfgOption::createDescriptorOption(CfgOptionDefPtr cfg_def, const std::string& sp << "' but no option definition"); } - // If there's no definition and no formatted string, we'll + // If there's no definition and no formatted string, we'll // settle for the generic option already in the descriptor. return; } try { - - std::cout << "def:" << def->getName() << ", code:" << def->getCode() - << ", type: " << def->getType() << std::endl; - // Definition found. Let's replace the generic option in // the descriptor with one created based on definition's factory. if (formatted_value.empty()) { @@ -164,7 +179,7 @@ CfgOption::createDescriptorOption(CfgOptionDefPtr cfg_def, const std::string& sp } } catch (const std::exception& ex) { isc_throw(InvalidOperation, "could not create option: " << space << "." << code - << " from data specified, reason: " << ex.what()); + << " from data specified, reason: " << ex.what()); } } diff --git a/src/lib/dhcpsrv/cfg_option.h b/src/lib/dhcpsrv/cfg_option.h index 6253f94b6e..b5eb66b855 100644 --- a/src/lib/dhcpsrv/cfg_option.h +++ b/src/lib/dhcpsrv/cfg_option.h @@ -349,9 +349,13 @@ public: /// not be modified after the call to @c merge because it may affect the /// merged configuration. /// + /// @param cfg_def set of of user-defined option definitions to use + /// when creating option instances. /// @param option configurations to merge with. void merge(CfgOptionDefPtr cfg_def, CfgOption& other); + void createOptions(CfgOptionDefPtr cfg_def); + /// @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 diff --git a/src/lib/dhcpsrv/cfg_shared_networks.cc b/src/lib/dhcpsrv/cfg_shared_networks.cc index f0f02d3cc8..7f5c48041f 100644 --- a/src/lib/dhcpsrv/cfg_shared_networks.cc +++ b/src/lib/dhcpsrv/cfg_shared_networks.cc @@ -19,8 +19,10 @@ CfgSharedNetworks4::hasNetworkWithServerId(const IOAddress& server_id) const { return (network_it != index.cend()); } + + void -CfgSharedNetworks4::merge(CfgSharedNetworks4& other) { +CfgSharedNetworks4::merge(CfgOptionDefPtr cfg_def, CfgSharedNetworks4& other) { auto& index = networks_.get(); // Iterate over the subnets to be merged. They will replace the existing @@ -59,6 +61,9 @@ CfgSharedNetworks4::merge(CfgSharedNetworks4& other) { index.erase(existing_network); } + // Create the network's options based on the given definitions. + (*other_network)->getCfgOption()->createOptions(cfg_def); + // Add the new/updated nework. networks_.push_back(*other_network); } diff --git a/src/lib/dhcpsrv/cfg_shared_networks.h b/src/lib/dhcpsrv/cfg_shared_networks.h index 24a219d0ce..bd7516413f 100644 --- a/src/lib/dhcpsrv/cfg_shared_networks.h +++ b/src/lib/dhcpsrv/cfg_shared_networks.h @@ -140,6 +140,7 @@ public: /// configuration: /// - All of its associated subnets are moved to the "other" network. /// - The existing network is removed from this configuration. + /// - The "other" network's option instances are created. /// - The "other" network is added to this configuration. /// /// @warning The merge operation may affect the @c other configuration. @@ -148,9 +149,11 @@ public: /// not be modified after the call to @c merge because it may affect the /// merged configuration. /// + /// @param cfg_def set of of user-defined option definitions to use + /// when creating option instances. /// @param other the shared network configuration to be merged into this /// configuration. - void merge(CfgSharedNetworks4& other); + void merge(CfgOptionDefPtr cfg_def, CfgSharedNetworks4& other); }; /// @brief Pointer to the configuration of IPv4 shared networks. diff --git a/src/lib/dhcpsrv/srv_config.cc b/src/lib/dhcpsrv/srv_config.cc index cda2708d84..7a3bf88501 100644 --- a/src/lib/dhcpsrv/srv_config.cc +++ b/src/lib/dhcpsrv/srv_config.cc @@ -182,15 +182,17 @@ SrvConfig::merge4(SrvConfig& other) { // Merge globals. mergeGlobals4(other); - // Merge option defs + // Merge option defs. We need to do this next so we + // pass these into subsequent merges so option instances + // at each level can be created based on the merged + // definitions. cfg_option_def_->merge((*other.getCfgOptionDef())); - // Merge options. Note that we pass in the merged definitions - // so we can validate options against them. + // Merge options. cfg_option_->merge(cfg_option_def_, (*other.getCfgOption())); // Merge shared networks. - cfg_shared_networks4_->merge(*(other.getCfgSharedNetworks4())); + cfg_shared_networks4_->merge(cfg_option_def_, *(other.getCfgSharedNetworks4())); // Merge subnets. cfg_subnets4_->merge(getCfgSharedNetworks4(), *(other.getCfgSubnets4())); diff --git a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc index 0127894dfa..afe7260661 100644 --- a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2014-2018 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2014-2019 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -350,27 +350,33 @@ TEST_F(CfgOptionTest, validMerge) { // Create our existing config, that gets merged into. OptionPtr option(new Option(Option::V4, 1, OptionBuffer(1, 0x01))); + EXPECT_EQ("type=001, len=001: 01", option->toText()); ASSERT_NO_THROW(this_cfg.add(option, false, "isc")); // Add option 3 to "fluff" option.reset(new Option(Option::V4, 3, OptionBuffer(1, 0x03))); + EXPECT_EQ("type=003, len=001: 03", option->toText()); ASSERT_NO_THROW(this_cfg.add(option, false, "fluff")); // Add option 4 to "fluff". option.reset(new Option(Option::V4, 4, OptionBuffer(1, 0x04))); + EXPECT_EQ("type=004, len=001: 04", option->toText()); ASSERT_NO_THROW(this_cfg.add(option, false, "fluff")); // 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))); + EXPECT_EQ("type=001, len=001: 10", option->toText()); ASSERT_NO_THROW(other_cfg.add(option, false, "isc")); // Add option 2 to "isc". option.reset(new Option(Option::V4, 2, OptionBuffer(1, 0x20))); + EXPECT_EQ("type=002, len=001: 20", option->toText()); ASSERT_NO_THROW(other_cfg.add(option, false, "isc")); // Add option 4 to "isc". option.reset(new Option(Option::V4, 4, OptionBuffer(1, 0x40))); + EXPECT_EQ("type=004, len=001: 40", option->toText()); ASSERT_NO_THROW(other_cfg.add(option, false, "isc")); // Merge source configuration to the destination configuration. The options @@ -381,32 +387,27 @@ TEST_F(CfgOptionTest, validMerge) { // 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]); + EXPECT_EQ("type=001, len=001: 16 (uint8)", desc.option_->toText()); // 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]); + EXPECT_EQ("type=002, len=001: 32 (uint8)", desc.option_->toText()); // 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]); + EXPECT_EQ("type=004, len=001: 64 (uint8)", desc.option_->toText()); // 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]); + EXPECT_EQ("type=003, len=001: 3 (uint8)", desc.option_->toText()); // 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]); + EXPECT_EQ("type=004, len=001: 4 (uint8)", desc.option_->toText()); } // This test verifies that attempting to merge options @@ -508,7 +509,7 @@ TEST_F(CfgOptionTest, createDescriptorOptionValid) { ASSERT_TRUE(opint); EXPECT_EQ("type=001, len=001: 119 (uint8)", opint->toText()); - // Now, a user defined array of strings + // Now, a user defined array of ints from a formatted value option.reset(new Option(Option::V4, 2)); desc.reset(new OptionDescriptor(option, false, "1,2,3")); diff --git a/src/lib/dhcpsrv/tests/cfg_shared_networks4_unittest.cc b/src/lib/dhcpsrv/tests/cfg_shared_networks4_unittest.cc index c8e80d7bec..76991a709c 100644 --- a/src/lib/dhcpsrv/tests/cfg_shared_networks4_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_shared_networks4_unittest.cc @@ -6,6 +6,7 @@ #include #include +#include #include #include #include @@ -168,6 +169,11 @@ TEST(CfgSharedNetworks4Test, unparse) { // This test verifies that shared-network configurations are properly merged. TEST(CfgSharedNetworks4Test, mergeNetworks) { + // Create custom options dictionary for testing merge. We're keeping it + // simple because they are more rigorous tests elsewhere. + CfgOptionDefPtr cfg_def(new CfgOptionDef()); + cfg_def->add((OptionDefinitionPtr(new OptionDefinition("one", 1, "string"))), "isc"); + Subnet4Ptr subnet1(new Subnet4(IOAddress("192.0.1.0"), 26, 1, 2, 100, SubnetID(1))); Subnet4Ptr subnet2(new Subnet4(IOAddress("192.0.2.0"), @@ -201,12 +207,11 @@ TEST(CfgSharedNetworks4Test, mergeNetworks) { // Merge in an "empty" config. Should have the original config, still intact. CfgSharedNetworks4 cfg_from; - ASSERT_NO_THROW(cfg_to.merge(cfg_from)); + ASSERT_NO_THROW(cfg_to.merge(cfg_def, cfg_from)); ASSERT_EQ(3, cfg_to.getAll()->size()); ASSERT_NO_FATAL_FAILURE(checkMergedNetwork(cfg_to, "network1", Triplet(100), std::vector{SubnetID(1), SubnetID(2)})); - ASSERT_NO_FATAL_FAILURE(checkMergedNetwork(cfg_to, "network2", Triplet(200), std::vector())); @@ -217,6 +222,15 @@ TEST(CfgSharedNetworks4Test, mergeNetworks) { // We'll double the valid time and add subnet4 to it SharedNetwork4Ptr network1b(new SharedNetwork4("network1")); network1b->setValid(Triplet(200)); + + // Now let's a add generic option 1 to network1b. + std::string value("Yay!"); + OptionPtr option(new Option(Option::V4, 1)); + option->setData(value.begin(), value.end()); + ASSERT_NO_THROW(network1b->getCfgOption()->add(option, false, "isc")); + // Verify that our option is a generic option. + EXPECT_EQ("type=001, len=004: 59:61:79:21", option->toText()); + ASSERT_NO_THROW(network1b->add(subnet4)); // Network2 we will not touch. @@ -230,7 +244,7 @@ TEST(CfgSharedNetworks4Test, mergeNetworks) { ASSERT_NO_THROW(cfg_from.add(network1b)); ASSERT_NO_THROW(cfg_from.add(network3b)); - ASSERT_NO_THROW(cfg_to.merge(cfg_from)); + ASSERT_NO_THROW(cfg_to.merge(cfg_def, cfg_from)); // Should still have 3 networks. @@ -241,6 +255,12 @@ TEST(CfgSharedNetworks4Test, mergeNetworks) { ASSERT_NO_FATAL_FAILURE(checkMergedNetwork(cfg_to, "network1", Triplet(200), std::vector{SubnetID(1), SubnetID(2)})); + // Make sure we have option 1 and that it has been replaced with a string option. + auto network = cfg_to.getByName("network1"); + auto desc = network->getCfgOption()->get("isc", 1); + ASSERT_TRUE(desc.option_); + EXPECT_EQ("type=001, len=004: \"Yay!\" (string)", desc.option_->toText()); + // No changes to network2. ASSERT_NO_FATAL_FAILURE(checkMergedNetwork(cfg_to, "network2", Triplet(200), std::vector())); From 5406df22fc2807a98372aa585771900232c6deed Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Fri, 15 Mar 2019 08:26:35 -0400 Subject: [PATCH 08/13] [#401,!254] Options are now created when merging subnets src/lib/dhcpsrv/cfg_subnets4.* CfgSubnets4::merge() - added cfg_def, now creates options for each subnet and their pools. src/lib/dhcpsrv/srv_config.cc SrvConfig::merge4() - passes merged option defs into subnets merge. src/lib/dhcpsrv/subnet.h Subnet::getPoolsWritable() - changed to public. --- src/lib/dhcpsrv/cfg_subnets4.cc | 8 +- src/lib/dhcpsrv/cfg_subnets4.h | 7 +- src/lib/dhcpsrv/srv_config.cc | 3 +- src/lib/dhcpsrv/subnet.h | 3 +- .../dhcpsrv/tests/cfg_subnets4_unittest.cc | 78 +++++++++++++++++-- 5 files changed, 87 insertions(+), 12 deletions(-) diff --git a/src/lib/dhcpsrv/cfg_subnets4.cc b/src/lib/dhcpsrv/cfg_subnets4.cc index 99e64893db..fd86f6dd76 100644 --- a/src/lib/dhcpsrv/cfg_subnets4.cc +++ b/src/lib/dhcpsrv/cfg_subnets4.cc @@ -56,7 +56,7 @@ CfgSubnets4::del(const ConstSubnet4Ptr& subnet) { } void -CfgSubnets4::merge(CfgSharedNetworks4Ptr networks, +CfgSubnets4::merge(CfgOptionDefPtr cfg_def, CfgSharedNetworks4Ptr networks, CfgSubnets4& other) { auto& index = subnets_.get(); @@ -94,6 +94,12 @@ CfgSubnets4::merge(CfgSharedNetworks4Ptr networks, index.erase(subnet_it); } + // Create the subnet's options based on the given definitions. + (*other_subnet)->getCfgOption()->createOptions(cfg_def); + for (auto pool : (*other_subnet)->getPoolsWritable(Lease::TYPE_V4)) { + pool->getCfgOption()->createOptions(cfg_def); + } + // Add the "other" subnet to the our collection of subnets. subnets_.push_back(*other_subnet); diff --git a/src/lib/dhcpsrv/cfg_subnets4.h b/src/lib/dhcpsrv/cfg_subnets4.h index 79fa3664d3..27890f6a28 100644 --- a/src/lib/dhcpsrv/cfg_subnets4.h +++ b/src/lib/dhcpsrv/cfg_subnets4.h @@ -70,6 +70,8 @@ public: /// -# If it belongs to a shared network, remove it from that network /// -# Remove the subnet from this configuration and discard it /// + /// - Create the subnet's option instance, as well as any options + /// that belong to any of the subnet's pools. /// - Add the subnet from @c other to this configuration. /// - If that subnet is associated to shared network, find that network /// in @ networks and add that subnet to it. @@ -80,12 +82,15 @@ public: /// not be modified after the call to @c merge because it may affect the /// merged configuration. /// + /// @param cfg_def set of of user-defined option definitions to use + /// when creating option instances. /// @param networks collection of shared networks that to which assignments /// should be added. In other words, the list of shared networks that belong /// to the same SrvConfig instance we are merging into. /// @param other the subnet configuration to be merged into this /// configuration. - void merge(CfgSharedNetworks4Ptr networks, CfgSubnets4& other); + void merge(CfgOptionDefPtr cfg_def, CfgSharedNetworks4Ptr networks, + CfgSubnets4& other); /// @brief Returns pointer to the collection of all IPv4 subnets. /// diff --git a/src/lib/dhcpsrv/srv_config.cc b/src/lib/dhcpsrv/srv_config.cc index 7a3bf88501..07137f3feb 100644 --- a/src/lib/dhcpsrv/srv_config.cc +++ b/src/lib/dhcpsrv/srv_config.cc @@ -195,7 +195,8 @@ SrvConfig::merge4(SrvConfig& other) { cfg_shared_networks4_->merge(cfg_option_def_, *(other.getCfgSharedNetworks4())); // Merge subnets. - cfg_subnets4_->merge(getCfgSharedNetworks4(), *(other.getCfgSubnets4())); + cfg_subnets4_->merge(cfg_option_def_, getCfgSharedNetworks4(), + *(other.getCfgSubnets4())); /// @todo merge other parts of the configuration here. } diff --git a/src/lib/dhcpsrv/subnet.h b/src/lib/dhcpsrv/subnet.h index 31d6680ca7..c2c16ff28e 100644 --- a/src/lib/dhcpsrv/subnet.h +++ b/src/lib/dhcpsrv/subnet.h @@ -289,7 +289,6 @@ public: shared_network_name_ = shared_network_name; } -protected: /// @brief Returns all pools (non-const variant) /// /// The reference is only valid as long as the object that returned it. @@ -298,6 +297,8 @@ protected: /// @return a collection of all pools PoolCollection& getPoolsWritable(Lease::Type type); +protected: + /// @brief Protected constructor // /// By making the constructor protected, we make sure that no one will diff --git a/src/lib/dhcpsrv/tests/cfg_subnets4_unittest.cc b/src/lib/dhcpsrv/tests/cfg_subnets4_unittest.cc index c5206458ae..3921dd4851 100644 --- a/src/lib/dhcpsrv/tests/cfg_subnets4_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_subnets4_unittest.cc @@ -38,11 +38,11 @@ void checkMergedSubnet(CfgSubnets4& cfg_subnets, const std::string& prefix, int exp_valid, SharedNetwork4Ptr exp_network) { - - // The subnet1 should be replaced by subnet4 but the shared network - // should not be affected. + // Look for the network by prefix. auto subnet = cfg_subnets.getByPrefix(prefix); ASSERT_TRUE(subnet) << "subnet: " << prefix << " not found"; + + // Make sure we have the one we expect. ASSERT_EQ(exp_subnet_id, subnet->getID()) << "subnet ID is wrong"; ASSERT_EQ(exp_valid, subnet->getValid()) << "subnet valid time is wrong"; @@ -139,6 +139,11 @@ TEST(CfgSubnets4Test, deleteSubnet) { // This test verifies that subnets configuration is properly merged. TEST(CfgSubnets4Test, mergeSubnets) { + // Create custom options dictionary for testing merge. We're keeping it + // simple because they are more rigorous tests elsewhere. + CfgOptionDefPtr cfg_def(new CfgOptionDef()); + cfg_def->add((OptionDefinitionPtr(new OptionDefinition("one", 1, "string"))), "isc"); + Subnet4Ptr subnet1(new Subnet4(IOAddress("192.0.1.0"), 26, 1, 2, 100, SubnetID(1))); Subnet4Ptr subnet2(new Subnet4(IOAddress("192.0.2.0"), @@ -148,7 +153,6 @@ TEST(CfgSubnets4Test, mergeSubnets) { Subnet4Ptr subnet4(new Subnet4(IOAddress("192.0.4.0"), 26, 1, 2, 100, SubnetID(4))); - // Create the "existing" list of shared networks CfgSharedNetworks4Ptr networks(new CfgSharedNetworks4()); SharedNetwork4Ptr shared_network1(new SharedNetwork4("shared-network1")); @@ -171,16 +175,14 @@ TEST(CfgSubnets4Test, mergeSubnets) { ASSERT_NO_THROW(cfg_to.add(subnet3)); ASSERT_NO_THROW(cfg_to.add(subnet4)); - // Merge in an "empty" config. Should have the original config, // still intact. CfgSubnets4 cfg_from; - ASSERT_NO_THROW(cfg_to.merge(networks, cfg_from)); + ASSERT_NO_THROW(cfg_to.merge(cfg_def, networks, cfg_from)); // We should have all four subnets, with no changes. ASSERT_EQ(4, cfg_to.getAll()->size()); - // Should be no changes to the configuration. ASSERT_NO_FATAL_FAILURE(checkMergedSubnet(cfg_to, SubnetID(1), "192.0.1.0/26", 100, shared_network1)); @@ -197,20 +199,55 @@ TEST(CfgSubnets4Test, mergeSubnets) { 26, 2, 3, 400, SubnetID(1))); subnet1b->setSharedNetworkName("shared-network1"); + // Add generic option 1 to subnet 1b. + std::string value("Yay!"); + OptionPtr option(new Option(Option::V4, 1)); + option->setData(value.begin(), value.end()); + ASSERT_NO_THROW(subnet1b->getCfgOption()->add(option, false, "isc")); + // Verify that our option is a generic option. + EXPECT_EQ("type=001, len=004: 59:61:79:21", option->toText()); + // subnet 3b updates subnet 3 and removes it from network 2 Subnet4Ptr subnet3b(new Subnet4(IOAddress("192.0.3.0"), 26, 3, 4, 500, SubnetID(3))); + // Now Add generic option 1 to subnet 3b. + value = "Team!"; + option.reset(new Option(Option::V4, 1)); + option->setData(value.begin(), value.end()); + ASSERT_NO_THROW(subnet3b->getCfgOption()->add(option, false, "isc")); + // Verify that our option is a generic option. + EXPECT_EQ("type=001, len=005: 54:65:61:6d:21", option->toText()); + // subnet 4b updates subnet 4 and moves it from network2 to network 1 Subnet4Ptr subnet4b(new Subnet4(IOAddress("192.0.4.0"), 26, 3, 4, 500, SubnetID(4))); subnet4b->setSharedNetworkName("shared-network1"); // subnet 5 is new and belongs to network 2 + // Has two pools both with an option 1 Subnet4Ptr subnet5(new Subnet4(IOAddress("192.0.5.0"), 26, 1, 2, 300, SubnetID(5))); subnet5->setSharedNetworkName("shared-network2"); + // Add pool 1 + Pool4Ptr pool(new Pool4(IOAddress("192.0.5.10"), IOAddress("192.0.5.20"))); + value = "POOLS"; + option.reset(new Option(Option::V4, 1)); + option->setData(value.begin(), value.end()); + ASSERT_NO_THROW(pool->getCfgOption()->add(option, false, "isc")); + EXPECT_EQ(option->toText(), "type=001, len=005: 50:4f:4f:4c:53"); + subnet5->addPool(pool); + + // Add pool 2 + pool.reset(new Pool4(IOAddress("192.0.5.30"), IOAddress("192.0.5.40"))); + value ="RULE!"; + option.reset(new Option(Option::V4, 1)); + option->setData(value.begin(), value.end()); + ASSERT_NO_THROW(pool->getCfgOption()->add(option, false, "isc")); + EXPECT_EQ(option->toText(), "type=001, len=005: 52:55:4c:45:21"); + subnet5->addPool(pool); + // Add subnets to the merge from config. ASSERT_NO_THROW(cfg_from.add(subnet1b)); ASSERT_NO_THROW(cfg_from.add(subnet3b)); @@ -218,13 +255,19 @@ TEST(CfgSubnets4Test, mergeSubnets) { ASSERT_NO_THROW(cfg_from.add(subnet5)); // Merge again. - ASSERT_NO_THROW(cfg_to.merge(networks, cfg_from)); + ASSERT_NO_THROW(cfg_to.merge(cfg_def, networks, cfg_from)); ASSERT_EQ(5, cfg_to.getAll()->size()); // The subnet1 should be replaced by subnet1b. ASSERT_NO_FATAL_FAILURE(checkMergedSubnet(cfg_to, SubnetID(1), "192.0.1.0/26", 400, shared_network1)); + // Let's verify that our option is there and populated correctly. + auto subnet = cfg_to.getByPrefix("192.0.1.0/26"); + auto desc = subnet->getCfgOption()->get("isc", 1); + ASSERT_TRUE(desc.option_); + EXPECT_EQ(desc.option_->toText(), "type=001, len=004: \"Yay!\" (string)"); + // The subnet2 should not be affected because it was not present. ASSERT_NO_FATAL_FAILURE(checkMergedSubnet(cfg_to, SubnetID(2), "192.0.2.0/26", 100, shared_network2)); @@ -232,6 +275,11 @@ TEST(CfgSubnets4Test, mergeSubnets) { // subnet3 should be replaced by subnet3b and no longer assigned to a network. ASSERT_NO_FATAL_FAILURE(checkMergedSubnet(cfg_to, SubnetID(3), "192.0.3.0/26", 500, no_network)); + // Let's verify that our option is there and populated correctly. + subnet = cfg_to.getByPrefix("192.0.3.0/26"); + desc = subnet->getCfgOption()->get("isc", 1); + ASSERT_TRUE(desc.option_); + EXPECT_EQ(desc.option_->toText(), "type=001, len=005: \"Team!\" (string)"); // subnet4 should be replaced by subnet4b and moved to network1. ASSERT_NO_FATAL_FAILURE(checkMergedSubnet(cfg_to, SubnetID(4), @@ -240,6 +288,20 @@ TEST(CfgSubnets4Test, mergeSubnets) { // subnet5 should have been added to configuration. ASSERT_NO_FATAL_FAILURE(checkMergedSubnet(cfg_to, SubnetID(5), "192.0.5.0/26", 300, shared_network2)); + + // Let's verify that both pools have the proper options. + subnet = cfg_to.getByPrefix("192.0.5.0/26"); + const PoolPtr merged_pool = subnet->getPool(Lease::TYPE_V4, IOAddress("192.0.5.10")); + ASSERT_TRUE(merged_pool); + desc = merged_pool->getCfgOption()->get("isc", 1); + ASSERT_TRUE(desc.option_); + EXPECT_EQ(desc.option_->toText(), "type=001, len=005: \"POOLS\" (string)"); + + const PoolPtr merged_pool2 = subnet->getPool(Lease::TYPE_V4, IOAddress("192.0.5.30")); + ASSERT_TRUE(merged_pool2); + desc = merged_pool2->getCfgOption()->get("isc", 1); + ASSERT_TRUE(desc.option_); + EXPECT_EQ(desc.option_->toText(), "type=001, len=005: \"RULE!\" (string)"); } // This test verifies that it is possible to retrieve a subnet using an From b45f1ab92f69f6e5270b10a3b8d59c51185d8a9b Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Fri, 15 Mar 2019 10:34:38 -0400 Subject: [PATCH 09/13] [#401,!254] Implemented CfgOption::replace src/lib/dhcpsrv/cfg_option.* CfgOption::replace() - new method to update an OptionDescriptor CfgOption::createDescriptorOption() - new returns a bool indicating whether or not the option_ instance was replaced --- src/lib/dhcpsrv/cfg_option.cc | 50 +++++++++----- src/lib/dhcpsrv/cfg_option.h | 16 ++++- src/lib/dhcpsrv/tests/cfg_option_unittest.cc | 69 ++++++++++++++++++-- 3 files changed, 111 insertions(+), 24 deletions(-) diff --git a/src/lib/dhcpsrv/cfg_option.cc b/src/lib/dhcpsrv/cfg_option.cc index e1268322c2..54d4f8f456 100644 --- a/src/lib/dhcpsrv/cfg_option.cc +++ b/src/lib/dhcpsrv/cfg_option.cc @@ -68,6 +68,31 @@ CfgOption::add(const OptionDescriptor& desc, const std::string& option_space) { } } +void +CfgOption::replace(const OptionDescriptor& desc, const std::string& option_space) { + if (!desc.option_) { + isc_throw(isc::BadValue, "option being replaced must not be NULL"); + } + + // Check for presence of options. + OptionContainerPtr options = getAll(option_space); + if (!options) { + isc_throw(isc::BadValue, "option space" << option_space << " does not exist"); + } + + // Find the option we want to replace. + OptionContainerTypeIndex& idx = options->get<1>(); + OptionContainerTypeIndex::const_iterator od_itr = idx.find(desc.option_->getType()); + if (od_itr == idx.end()) { + isc_throw(isc::BadValue, "cannot replace option: " + << option_space << ":" << desc.option_->getType() + << ", it does not exist"); + } + + idx.replace(od_itr, desc); +} + + std::list CfgOption::getVendorIdsSpaceNames() const { std::list ids = getVendorIds(); @@ -103,25 +128,17 @@ CfgOption::createOptions(CfgOptionDefPtr cfg_def) { // Iterate over all the option descriptors in // all the spaces and instantiate the options // based on the given definitions. - - // Descriptors can only be fetched as copies of - // what is in the container. Since we don't - // currently have a replace mechanism, we'll - // create a revamped set of descriptors and then - // copy them on top of ourself. - CfgOption revamped; for (auto space : getOptionSpaceNames()) { for (auto opt_desc : *(getAll(space))) { - createDescriptorOption(cfg_def, space, opt_desc); - revamped.add(opt_desc, space); + if (createDescriptorOption(cfg_def, space, opt_desc)) { + // Option was recreated, let's replace the descriptor. + replace(opt_desc,space); + } } } - - // Copy the updated descriptors over our own. - revamped.copyTo(*this); } -void +bool CfgOption::createDescriptorOption(CfgOptionDefPtr cfg_def, const std::string& space, OptionDescriptor& opt_desc) { if (!opt_desc.option_) { @@ -129,7 +146,6 @@ CfgOption::createDescriptorOption(CfgOptionDefPtr cfg_def, const std::string& sp "validateCreateOption: descriptor has no option instance"); } - Option::Universe universe = opt_desc.option_->getUniverse(); uint16_t code = opt_desc.option_->getType(); @@ -162,7 +178,8 @@ CfgOption::createDescriptorOption(CfgOptionDefPtr cfg_def, const std::string& sp // If there's no definition and no formatted string, we'll // settle for the generic option already in the descriptor. - return; + // Indicate no-change by returning false. + return (false); } try { @@ -181,6 +198,9 @@ CfgOption::createDescriptorOption(CfgOptionDefPtr cfg_def, const std::string& sp isc_throw(InvalidOperation, "could not create option: " << space << "." << code << " from data specified, reason: " << ex.what()); } + + // Indicate we replaced the definition. + return(true); } void diff --git a/src/lib/dhcpsrv/cfg_option.h b/src/lib/dhcpsrv/cfg_option.h index b5eb66b855..f6b2ee80cc 100644 --- a/src/lib/dhcpsrv/cfg_option.h +++ b/src/lib/dhcpsrv/cfg_option.h @@ -333,13 +333,15 @@ public: /// @throw isc::BadValue if the option space is invalid. void add(const OptionDescriptor& desc, const std::string& option_space); + void replace(const OptionDescriptor& desc, const std::string& option_space); + /// @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). 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 + /// the merged set of opt definitions. Finally, it calls /// @c copyTo() to overwrite this configuration's options with /// the merged set in @c other. /// @@ -354,6 +356,15 @@ public: /// @param option configurations to merge with. void merge(CfgOptionDefPtr cfg_def, CfgOption& other); + /// @brief Re-create the option in each descriptor based on given definitions + /// + /// Invokes @c createDescriptorOption() on each option descriptor in + /// each option space, passing in the the given dictionary of option + /// definitions. If the descriptor's option is re-created, then the + /// descriptor is updated by calling @c replace(). + /// + /// @param cfg_def set of of user-defined option definitions to use + /// when creating option instances. void createOptions(CfgOptionDefPtr cfg_def); /// @brief Creates an option descriptor's option based on a set of option defs @@ -392,10 +403,11 @@ public: /// @param space the option space name of the option /// @param opt_desc OptionDescriptor describing the option. /// + /// @return True if the descriptor's option instance was replaced. /// @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, + static bool createDescriptorOption(CfgOptionDefPtr cfg_def, const std::string& space, OptionDescriptor& opt_desc); /// @brief Merges this configuration to another configuration. diff --git a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc index afe7260661..d53ddb35b2 100644 --- a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc @@ -216,6 +216,55 @@ TEST_F(CfgOptionTest, add) { EXPECT_TRUE(options->empty()); } +// This test verifies that options can be replaced with udpated content. +TEST_F(CfgOptionTest, replace) { + CfgOption cfg; + + // Let's add some options to the config to the config. + OptionStringPtr option(new OptionString(Option::V6, 1, "one")); + ASSERT_NO_THROW(cfg.add(option, false, "isc")); + + option.reset(new OptionString(Option::V6, 2, "two")); + ASSERT_NO_THROW(cfg.add(option, false, "isc")); + + option.reset(new OptionString(Option::V6, 3, "three")); + ASSERT_NO_THROW(cfg.add(option, false, "isc")); + + // Now let's make sure we can find them and they are as expected. + OptionDescriptor desc = cfg.get("isc", 1); + ASSERT_TRUE(desc.option_); + ASSERT_EQ(desc.option_->toText(),"type=00001, len=00003: \"one\" (string)"); + + desc = cfg.get("isc", 2); + ASSERT_TRUE(desc.option_); + ASSERT_EQ(desc.option_->toText(),"type=00002, len=00003: \"two\" (string)"); + + desc = cfg.get("isc", 3); + ASSERT_TRUE(desc.option_); + ASSERT_EQ(desc.option_->toText(),"type=00003, len=00005: \"three\" (string)"); + + // Now let's replace one and three. + desc.option_.reset(new OptionString(Option::V6, 1, "new one")); + ASSERT_NO_THROW(cfg.replace(desc, "isc")); + + desc.option_.reset(new OptionString(Option::V6, 3, "new three")); + ASSERT_NO_THROW(cfg.replace(desc, "isc")); + + // Now let's make sure we can find them again and they are as expected. + desc = cfg.get("isc", 1); + ASSERT_TRUE(desc.option_); + ASSERT_EQ(desc.option_->toText(),"type=00001, len=00007: \"new one\" (string)"); + + desc = cfg.get("isc", 2); + ASSERT_TRUE(desc.option_); + ASSERT_EQ(desc.option_->toText(),"type=00002, len=00003: \"two\" (string)"); + + desc = cfg.get("isc", 3); + ASSERT_TRUE(desc.option_); + ASSERT_EQ(desc.option_->toText(),"type=00003, len=00009: \"new three\" (string)"); +} + + // This test verifies that one configuration can be merged into another. TEST_F(CfgOptionTest, mergeTo) { CfgOption cfg_src; @@ -482,7 +531,9 @@ TEST_F(CfgOptionTest, createDescriptorOptionValid) { option->setData(value.begin(), value.end()); OptionDescriptorPtr desc(new OptionDescriptor(option, false)); - ASSERT_NO_THROW(CfgOption::createDescriptorOption(defs, space, *desc)); + bool updated = false; + ASSERT_NO_THROW(updated = CfgOption::createDescriptorOption(defs, space, *desc)); + ASSERT_TRUE(updated); OptionStringPtr opstr = boost::dynamic_pointer_cast(desc->option_); ASSERT_TRUE(opstr); EXPECT_EQ("type=012, len=011: \"example.org\" (string)", opstr->toText()); @@ -493,7 +544,8 @@ TEST_F(CfgOptionTest, createDescriptorOptionValid) { option.reset(new Option(Option::V4, 2)); desc.reset(new OptionDescriptor(option, false, value)); - ASSERT_NO_THROW(CfgOption::createDescriptorOption(defs, space, *desc)); + ASSERT_NO_THROW(updated = CfgOption::createDescriptorOption(defs, space, *desc)); + ASSERT_TRUE(updated); std::cout << "option:" << desc->option_->toText() << std::endl; Option4AddrLstPtr opaddrs = boost::dynamic_pointer_cast(desc->option_); ASSERT_TRUE(opaddrs); @@ -504,7 +556,8 @@ TEST_F(CfgOptionTest, createDescriptorOptionValid) { option.reset(new Option(Option::V4, 1, OptionBuffer(1, 0x77))); desc.reset(new OptionDescriptor(option, false)); - ASSERT_NO_THROW(CfgOption::createDescriptorOption(defs, space, *desc)); + ASSERT_NO_THROW(updated = CfgOption::createDescriptorOption(defs, space, *desc)); + ASSERT_TRUE(updated); OptionUint8Ptr opint = boost::dynamic_pointer_cast(desc->option_); ASSERT_TRUE(opint); EXPECT_EQ("type=001, len=001: 119 (uint8)", opint->toText()); @@ -513,17 +566,19 @@ TEST_F(CfgOptionTest, createDescriptorOptionValid) { option.reset(new Option(Option::V4, 2)); desc.reset(new OptionDescriptor(option, false, "1,2,3")); - ASSERT_NO_THROW(CfgOption::createDescriptorOption(defs, space, *desc)); + ASSERT_NO_THROW(updated = CfgOption::createDescriptorOption(defs, space, *desc)); + ASSERT_TRUE(updated); OptionUint8ArrayPtr oparray = boost::dynamic_pointer_cast(desc->option_); ASSERT_TRUE(oparray); EXPECT_EQ("type=002, len=003: 1(uint8) 2(uint8) 3(uint8)", oparray->toText()); // Finally, a generic, undefined option - option.reset(new Option(Option::V4, 2, OptionBuffer(1, 0x77))); + option.reset(new Option(Option::V4, 199, OptionBuffer(1, 0x77))); desc.reset(new OptionDescriptor(option, false)); - ASSERT_NO_THROW(CfgOption::createDescriptorOption(defs, space, *desc)); - EXPECT_EQ("type=002, len=001: 119(uint8)", desc->option_->toText()); + ASSERT_NO_THROW(updated = CfgOption::createDescriptorOption(defs, space, *desc)); + ASSERT_FALSE(updated); + EXPECT_EQ("type=199, len=001: 77", desc->option_->toText()); } // This test verifies that encapsulated options are added as sub-options From df83514ac07f43cd45e414e8740b945692b84e50 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Mon, 18 Mar 2019 12:51:04 +0100 Subject: [PATCH 10/13] [#401,!254] Use three slashes between todo in the SrvConfig. --- src/lib/dhcpsrv/srv_config.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lib/dhcpsrv/srv_config.cc b/src/lib/dhcpsrv/srv_config.cc index 07137f3feb..63aa046919 100644 --- a/src/lib/dhcpsrv/srv_config.cc +++ b/src/lib/dhcpsrv/srv_config.cc @@ -166,7 +166,7 @@ SrvConfig::merge(ConfigBase& other) { if (CfgMgr::instance().getFamily() == AF_INET) { merge4(other_srv_config); } else { - // @todo merge6(); + /// @todo merge6(); } } catch (const std::bad_cast&) { isc_throw(InvalidOperation, "internal server error: must use derivation" @@ -450,7 +450,7 @@ SrvConfig::toElement() const { } // Set client-classes ConstElementPtr client_classes = class_dictionary_->toElement(); - // @todo accept empty list + /// @todo accept empty list if (!client_classes->empty()) { dhcp->set("client-classes", client_classes); } From 8873fce8a2485e1889c26ce6b18a5c6a41cb7bc6 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Mon, 18 Mar 2019 11:30:05 -0400 Subject: [PATCH 11/13] [#401,!254] Addressed more review comments. --- src/lib/dhcpsrv/cfg_option.h | 13 +++++++++++++ src/lib/dhcpsrv/tests/cfg_option_unittest.cc | 1 - 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/lib/dhcpsrv/cfg_option.h b/src/lib/dhcpsrv/cfg_option.h index f6b2ee80cc..060c940ea0 100644 --- a/src/lib/dhcpsrv/cfg_option.h +++ b/src/lib/dhcpsrv/cfg_option.h @@ -333,6 +333,19 @@ public: /// @throw isc::BadValue if the option space is invalid. void add(const OptionDescriptor& desc, const std::string& option_space); + /// @brief Replaces the instance of an option within this collection + /// + /// This method locates the option within the given space and replaces + /// it with a copy of the given descriptor. This effectively updates + /// the contents without altering the container indexing. + /// + /// @param desc Option descriptor holding option instance and other + /// parameters pertaining to the option. + /// @param option_space Option space name. + /// + /// @throw isc::BadValue if the descriptor's option instance is null, + /// if space is invalid, or if the option does not already exist + /// in the given space. void replace(const OptionDescriptor& desc, const std::string& option_space); /// @brief Merges another option configuration into this one. diff --git a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc index d53ddb35b2..b820305bc7 100644 --- a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc @@ -546,7 +546,6 @@ TEST_F(CfgOptionTest, createDescriptorOptionValid) { ASSERT_NO_THROW(updated = CfgOption::createDescriptorOption(defs, space, *desc)); ASSERT_TRUE(updated); - std::cout << "option:" << desc->option_->toText() << std::endl; Option4AddrLstPtr opaddrs = boost::dynamic_pointer_cast(desc->option_); ASSERT_TRUE(opaddrs); EXPECT_EQ("type=002, len=008: 192.0.2.1 192.0.2.2", opaddrs->toText()); From 6193cc7ae1200f47a08d9e66700d330f31570140 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Mon, 18 Mar 2019 17:31:58 +0100 Subject: [PATCH 12/13] [#401,!254] Added space in the error message. --- src/lib/dhcpsrv/cfg_option.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/lib/dhcpsrv/cfg_option.cc b/src/lib/dhcpsrv/cfg_option.cc index 54d4f8f456..85a8d7d84c 100644 --- a/src/lib/dhcpsrv/cfg_option.cc +++ b/src/lib/dhcpsrv/cfg_option.cc @@ -77,7 +77,8 @@ CfgOption::replace(const OptionDescriptor& desc, const std::string& option_space // Check for presence of options. OptionContainerPtr options = getAll(option_space); if (!options) { - isc_throw(isc::BadValue, "option space" << option_space << " does not exist"); + isc_throw(isc::BadValue, "option space " << option_space + << " does not exist"); } // Find the option we want to replace. From 3c8a5b9be22d7a552a9b22dfb1ce96dd86b937eb Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Mon, 18 Mar 2019 14:31:42 -0400 Subject: [PATCH 13/13] [#401,!254] Further review work. Removed use of Option::toText from tests: src/lib/dhcpsrv/tests/cfg_option_unittest.cc TEST_F(CfgOptionTest, replace) TEST_F(CfgOptionTest, mergeTo) TEST_F(CfgOptionTest, createDescriptorOptionValid) src/lib/dhcpsrv/tests/cfg_subnets4_unittest.cc TEST(CfgSubnets4Test, mergeSubnets) --- src/lib/dhcpsrv/tests/cfg_option_unittest.cc | 65 ++++++++++++------- .../dhcpsrv/tests/cfg_subnets4_unittest.cc | 25 +++---- 2 files changed, 55 insertions(+), 35 deletions(-) diff --git a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc index b820305bc7..21c6a0b995 100644 --- a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc @@ -23,6 +23,7 @@ #include using namespace isc; +using namespace isc::asiolink; using namespace isc::dhcp; namespace { @@ -233,15 +234,21 @@ TEST_F(CfgOptionTest, replace) { // Now let's make sure we can find them and they are as expected. OptionDescriptor desc = cfg.get("isc", 1); ASSERT_TRUE(desc.option_); - ASSERT_EQ(desc.option_->toText(),"type=00001, len=00003: \"one\" (string)"); + OptionStringPtr opstr = boost::dynamic_pointer_cast(desc.option_); + ASSERT_TRUE(opstr); + EXPECT_EQ("one", opstr->getValue()); desc = cfg.get("isc", 2); ASSERT_TRUE(desc.option_); - ASSERT_EQ(desc.option_->toText(),"type=00002, len=00003: \"two\" (string)"); + opstr = boost::dynamic_pointer_cast(desc.option_); + ASSERT_TRUE(opstr); + EXPECT_EQ("two", opstr->getValue()); desc = cfg.get("isc", 3); ASSERT_TRUE(desc.option_); - ASSERT_EQ(desc.option_->toText(),"type=00003, len=00005: \"three\" (string)"); + opstr = boost::dynamic_pointer_cast(desc.option_); + ASSERT_TRUE(opstr); + EXPECT_EQ("three", opstr->getValue()); // Now let's replace one and three. desc.option_.reset(new OptionString(Option::V6, 1, "new one")); @@ -253,18 +260,23 @@ TEST_F(CfgOptionTest, replace) { // Now let's make sure we can find them again and they are as expected. desc = cfg.get("isc", 1); ASSERT_TRUE(desc.option_); - ASSERT_EQ(desc.option_->toText(),"type=00001, len=00007: \"new one\" (string)"); + opstr = boost::dynamic_pointer_cast(desc.option_); + ASSERT_TRUE(opstr); + EXPECT_EQ("new one", opstr->getValue()); desc = cfg.get("isc", 2); ASSERT_TRUE(desc.option_); - ASSERT_EQ(desc.option_->toText(),"type=00002, len=00003: \"two\" (string)"); + opstr = boost::dynamic_pointer_cast(desc.option_); + ASSERT_TRUE(opstr); + EXPECT_EQ("two", opstr->getValue()); desc = cfg.get("isc", 3); ASSERT_TRUE(desc.option_); - ASSERT_EQ(desc.option_->toText(),"type=00003, len=00009: \"new three\" (string)"); + opstr = boost::dynamic_pointer_cast(desc.option_); + ASSERT_TRUE(opstr); + EXPECT_EQ("new three", opstr->getValue()); } - // This test verifies that one configuration can be merged into another. TEST_F(CfgOptionTest, mergeTo) { CfgOption cfg_src; @@ -399,33 +411,27 @@ TEST_F(CfgOptionTest, validMerge) { // Create our existing config, that gets merged into. OptionPtr option(new Option(Option::V4, 1, OptionBuffer(1, 0x01))); - EXPECT_EQ("type=001, len=001: 01", option->toText()); ASSERT_NO_THROW(this_cfg.add(option, false, "isc")); // Add option 3 to "fluff" option.reset(new Option(Option::V4, 3, OptionBuffer(1, 0x03))); - EXPECT_EQ("type=003, len=001: 03", option->toText()); ASSERT_NO_THROW(this_cfg.add(option, false, "fluff")); // Add option 4 to "fluff". option.reset(new Option(Option::V4, 4, OptionBuffer(1, 0x04))); - EXPECT_EQ("type=004, len=001: 04", option->toText()); ASSERT_NO_THROW(this_cfg.add(option, false, "fluff")); // 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))); - EXPECT_EQ("type=001, len=001: 10", option->toText()); ASSERT_NO_THROW(other_cfg.add(option, false, "isc")); // Add option 2 to "isc". option.reset(new Option(Option::V4, 2, OptionBuffer(1, 0x20))); - EXPECT_EQ("type=002, len=001: 20", option->toText()); ASSERT_NO_THROW(other_cfg.add(option, false, "isc")); // Add option 4 to "isc". option.reset(new Option(Option::V4, 4, OptionBuffer(1, 0x40))); - EXPECT_EQ("type=004, len=001: 40", option->toText()); ASSERT_NO_THROW(other_cfg.add(option, false, "isc")); // Merge source configuration to the destination configuration. The options @@ -436,27 +442,37 @@ TEST_F(CfgOptionTest, validMerge) { // isc:1 should come from "other" config. OptionDescriptor desc = this_cfg.get("isc", 1); ASSERT_TRUE(desc.option_); - EXPECT_EQ("type=001, len=001: 16 (uint8)", desc.option_->toText()); + OptionUint8Ptr opint = boost::dynamic_pointer_cast(desc.option_); + ASSERT_TRUE(opint); + EXPECT_EQ(16, opint->getValue()); // isc:2 should come from "other" config. desc = this_cfg.get("isc", 2); ASSERT_TRUE(desc.option_); - EXPECT_EQ("type=002, len=001: 32 (uint8)", desc.option_->toText()); + opint = boost::dynamic_pointer_cast(desc.option_); + ASSERT_TRUE(opint); + EXPECT_EQ(32, opint->getValue()); // isc:4 should come from "other" config. desc = this_cfg.get("isc", 4); ASSERT_TRUE(desc.option_); - EXPECT_EQ("type=004, len=001: 64 (uint8)", desc.option_->toText()); + opint = boost::dynamic_pointer_cast(desc.option_); + ASSERT_TRUE(opint); + EXPECT_EQ(64, opint->getValue()); // fluff:3 should come from "this" config. desc = this_cfg.get("fluff", 3); ASSERT_TRUE(desc.option_); - EXPECT_EQ("type=003, len=001: 3 (uint8)", desc.option_->toText()); + opint = boost::dynamic_pointer_cast(desc.option_); + ASSERT_TRUE(opint); + EXPECT_EQ(3, opint->getValue()); // fluff:4 should come from "this" config. desc = this_cfg.get("fluff", 4); ASSERT_TRUE(desc.option_); - EXPECT_EQ("type=004, len=001: 4 (uint8)", desc.option_->toText()); + opint = boost::dynamic_pointer_cast(desc.option_); + ASSERT_TRUE(opint); + EXPECT_EQ(4, opint->getValue()); } // This test verifies that attempting to merge options @@ -536,7 +552,7 @@ TEST_F(CfgOptionTest, createDescriptorOptionValid) { ASSERT_TRUE(updated); OptionStringPtr opstr = boost::dynamic_pointer_cast(desc->option_); ASSERT_TRUE(opstr); - EXPECT_EQ("type=012, len=011: \"example.org\" (string)", opstr->toText()); + EXPECT_EQ("example.org", opstr->getValue()); // Next we'll try a vendor option with a formatted value space = "vendor-4491"; @@ -548,7 +564,8 @@ TEST_F(CfgOptionTest, createDescriptorOptionValid) { ASSERT_TRUE(updated); Option4AddrLstPtr opaddrs = boost::dynamic_pointer_cast(desc->option_); ASSERT_TRUE(opaddrs); - EXPECT_EQ("type=002, len=008: 192.0.2.1 192.0.2.2", opaddrs->toText()); + Option4AddrLst::AddressContainer exp_addresses = { IOAddress("192.0.2.1"), IOAddress("192.0.2.2") }; + EXPECT_EQ(exp_addresses, opaddrs->getAddresses()); // Now, a user defined uint8 option space = "isc"; @@ -559,7 +576,7 @@ TEST_F(CfgOptionTest, createDescriptorOptionValid) { ASSERT_TRUE(updated); OptionUint8Ptr opint = boost::dynamic_pointer_cast(desc->option_); ASSERT_TRUE(opint); - EXPECT_EQ("type=001, len=001: 119 (uint8)", opint->toText()); + EXPECT_EQ(119, opint->getValue()); // Now, a user defined array of ints from a formatted value option.reset(new Option(Option::V4, 2)); @@ -569,7 +586,8 @@ TEST_F(CfgOptionTest, createDescriptorOptionValid) { ASSERT_TRUE(updated); OptionUint8ArrayPtr oparray = boost::dynamic_pointer_cast(desc->option_); ASSERT_TRUE(oparray); - EXPECT_EQ("type=002, len=003: 1(uint8) 2(uint8) 3(uint8)", oparray->toText()); + std::vector exp_ints = { 1, 2, 3 }; + EXPECT_EQ(exp_ints, oparray->getValues()); // Finally, a generic, undefined option option.reset(new Option(Option::V4, 199, OptionBuffer(1, 0x77))); @@ -577,7 +595,8 @@ TEST_F(CfgOptionTest, createDescriptorOptionValid) { ASSERT_NO_THROW(updated = CfgOption::createDescriptorOption(defs, space, *desc)); ASSERT_FALSE(updated); - EXPECT_EQ("type=199, len=001: 77", desc->option_->toText()); + ASSERT_EQ(1, desc->option_->getData().size()); + EXPECT_EQ(0x77, desc->option_->getData()[0]); } // This test verifies that encapsulated options are added as sub-options diff --git a/src/lib/dhcpsrv/tests/cfg_subnets4_unittest.cc b/src/lib/dhcpsrv/tests/cfg_subnets4_unittest.cc index 3921dd4851..f883e0a9a0 100644 --- a/src/lib/dhcpsrv/tests/cfg_subnets4_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_subnets4_unittest.cc @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -204,8 +205,6 @@ TEST(CfgSubnets4Test, mergeSubnets) { OptionPtr option(new Option(Option::V4, 1)); option->setData(value.begin(), value.end()); ASSERT_NO_THROW(subnet1b->getCfgOption()->add(option, false, "isc")); - // Verify that our option is a generic option. - EXPECT_EQ("type=001, len=004: 59:61:79:21", option->toText()); // subnet 3b updates subnet 3 and removes it from network 2 Subnet4Ptr subnet3b(new Subnet4(IOAddress("192.0.3.0"), @@ -216,8 +215,6 @@ TEST(CfgSubnets4Test, mergeSubnets) { option.reset(new Option(Option::V4, 1)); option->setData(value.begin(), value.end()); ASSERT_NO_THROW(subnet3b->getCfgOption()->add(option, false, "isc")); - // Verify that our option is a generic option. - EXPECT_EQ("type=001, len=005: 54:65:61:6d:21", option->toText()); // subnet 4b updates subnet 4 and moves it from network2 to network 1 Subnet4Ptr subnet4b(new Subnet4(IOAddress("192.0.4.0"), @@ -236,7 +233,6 @@ TEST(CfgSubnets4Test, mergeSubnets) { option.reset(new Option(Option::V4, 1)); option->setData(value.begin(), value.end()); ASSERT_NO_THROW(pool->getCfgOption()->add(option, false, "isc")); - EXPECT_EQ(option->toText(), "type=001, len=005: 50:4f:4f:4c:53"); subnet5->addPool(pool); // Add pool 2 @@ -245,7 +241,6 @@ TEST(CfgSubnets4Test, mergeSubnets) { option.reset(new Option(Option::V4, 1)); option->setData(value.begin(), value.end()); ASSERT_NO_THROW(pool->getCfgOption()->add(option, false, "isc")); - EXPECT_EQ(option->toText(), "type=001, len=005: 52:55:4c:45:21"); subnet5->addPool(pool); // Add subnets to the merge from config. @@ -266,7 +261,9 @@ TEST(CfgSubnets4Test, mergeSubnets) { auto subnet = cfg_to.getByPrefix("192.0.1.0/26"); auto desc = subnet->getCfgOption()->get("isc", 1); ASSERT_TRUE(desc.option_); - EXPECT_EQ(desc.option_->toText(), "type=001, len=004: \"Yay!\" (string)"); + OptionStringPtr opstr = boost::dynamic_pointer_cast(desc.option_); + ASSERT_TRUE(opstr); + EXPECT_EQ("Yay!", opstr->getValue()); // The subnet2 should not be affected because it was not present. ASSERT_NO_FATAL_FAILURE(checkMergedSubnet(cfg_to, SubnetID(2), @@ -279,7 +276,9 @@ TEST(CfgSubnets4Test, mergeSubnets) { subnet = cfg_to.getByPrefix("192.0.3.0/26"); desc = subnet->getCfgOption()->get("isc", 1); ASSERT_TRUE(desc.option_); - EXPECT_EQ(desc.option_->toText(), "type=001, len=005: \"Team!\" (string)"); + opstr = boost::dynamic_pointer_cast(desc.option_); + ASSERT_TRUE(opstr); + EXPECT_EQ("Team!", opstr->getValue()); // subnet4 should be replaced by subnet4b and moved to network1. ASSERT_NO_FATAL_FAILURE(checkMergedSubnet(cfg_to, SubnetID(4), @@ -294,14 +293,16 @@ TEST(CfgSubnets4Test, mergeSubnets) { const PoolPtr merged_pool = subnet->getPool(Lease::TYPE_V4, IOAddress("192.0.5.10")); ASSERT_TRUE(merged_pool); desc = merged_pool->getCfgOption()->get("isc", 1); - ASSERT_TRUE(desc.option_); - EXPECT_EQ(desc.option_->toText(), "type=001, len=005: \"POOLS\" (string)"); + opstr = boost::dynamic_pointer_cast(desc.option_); + ASSERT_TRUE(opstr); + EXPECT_EQ("POOLS", opstr->getValue()); const PoolPtr merged_pool2 = subnet->getPool(Lease::TYPE_V4, IOAddress("192.0.5.30")); ASSERT_TRUE(merged_pool2); desc = merged_pool2->getCfgOption()->get("isc", 1); - ASSERT_TRUE(desc.option_); - EXPECT_EQ(desc.option_->toText(), "type=001, len=005: \"RULE!\" (string)"); + opstr = boost::dynamic_pointer_cast(desc.option_); + ASSERT_TRUE(opstr); + EXPECT_EQ("RULE!", opstr->getValue()); } // This test verifies that it is possible to retrieve a subnet using an