From: Thomas Markwalder Date: Mon, 4 Mar 2019 15:54:53 +0000 (-0500) Subject: [#401,!254] kea-dhcp4 now merges CB options into current config X-Git-Tag: Kea-1.6.0-beta~362^2~10 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=1a663d09e49eb40d2f5132d8af2cda810d660390;p=thirdparty%2Fkea.git [#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 --- 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); - - // 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); + OptionDescriptorPtr opt; + + // 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) {