From: Thomas Markwalder Date: Thu, 14 Mar 2019 17:54:41 +0000 (-0400) Subject: [#401,!254] Options are now created when merging shared-network4s X-Git-Tag: Kea-1.6.0-beta~362^2~6 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=d55b3c695e74d2353d0a5930ab138f3680657a1d;p=thirdparty%2Fkea.git [#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. --- 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,16 +91,34 @@ 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))) { + // 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); } } - // Next we copy "other" on top of ourself. - other.copyTo(*this); + // Copy the updated descriptors over our own. + revamped.copyTo(*this); } void @@ -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()));