From c416c968f81f54c9e69d92ff7ea4ced4d681c1d9 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Thu, 28 Mar 2019 11:03:07 -0400 Subject: [PATCH 1/9] [#413,!288] - kea-dhcp6 now uses globals from config back end src/bin/dhcp6/tests/config_backend_unittest.cc TEST_F(Dhcp6CBTest, mergeGlobals) - enabled test src/lib/dhcpsrv/srv_config.* SrvConfig::merge(ConfigBase& other) - invoke merge6() SrvConfig::mergeGlobals4() renamed to mergeGlobals() SrvConfig::merge6(SrvConfig& other) - new method src/lib/dhcpsrv/tests/srv_config_unittest.cc TEST_F(SrvConfigTest, mergeGlobals6) - new test --- .../dhcp6/tests/config_backend_unittest.cc | 2 +- src/lib/dhcpsrv/srv_config.cc | 43 ++++++++++-- src/lib/dhcpsrv/srv_config.h | 30 +++++++- src/lib/dhcpsrv/tests/srv_config_unittest.cc | 70 ++++++++++++++++++- 4 files changed, 134 insertions(+), 11 deletions(-) diff --git a/src/bin/dhcp6/tests/config_backend_unittest.cc b/src/bin/dhcp6/tests/config_backend_unittest.cc index fe995168fe..0d4d50ffb6 100644 --- a/src/bin/dhcp6/tests/config_backend_unittest.cc +++ b/src/bin/dhcp6/tests/config_backend_unittest.cc @@ -162,7 +162,7 @@ public: // This test verifies that externally configured globals are // merged correctly into staging configuration. -TEST_F(Dhcp6CBTest, DISABLED_mergeGlobals) { +TEST_F(Dhcp6CBTest, mergeGlobals) { string base_config = "{ \n" " \"interfaces-config\": { \n" diff --git a/src/lib/dhcpsrv/srv_config.cc b/src/lib/dhcpsrv/srv_config.cc index c7b42b87be..0aea3b208d 100644 --- a/src/lib/dhcpsrv/srv_config.cc +++ b/src/lib/dhcpsrv/srv_config.cc @@ -164,7 +164,7 @@ SrvConfig::merge(ConfigBase& other) { if (CfgMgr::instance().getFamily() == AF_INET) { merge4(other_srv_config); } else { - /// @todo merge6(); + merge6(other_srv_config); } } catch (const std::bad_cast&) { isc_throw(InvalidOperation, "internal server error: must use derivation" @@ -178,7 +178,7 @@ SrvConfig::merge4(SrvConfig& other) { // We merge objects in order of dependency (real or theoretical). // Merge globals. - mergeGlobals4(other); + mergeGlobals(other); // Merge option defs. We need to do this next so we // pass these into subsequent merges so option instances @@ -186,28 +186,56 @@ SrvConfig::merge4(SrvConfig& other) { // definitions. cfg_option_def_->merge((*other.getCfgOptionDef())); - // Merge options. + // Merge options. cfg_option_->merge(cfg_option_def_, (*other.getCfgOption())); // Merge shared networks. cfg_shared_networks4_->merge(cfg_option_def_, *(other.getCfgSharedNetworks4())); // Merge subnets. - cfg_subnets4_->merge(cfg_option_def_, getCfgSharedNetworks4(), + cfg_subnets4_->merge(cfg_option_def_, getCfgSharedNetworks4(), *(other.getCfgSubnets4())); /// @todo merge other parts of the configuration here. } void -SrvConfig::mergeGlobals4(SrvConfig& other) { +SrvConfig::merge6(SrvConfig& other) { + // We merge objects in order of dependency (real or theoretical). + + // Merge globals. + mergeGlobals(other); + +#if 0 + // 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. + cfg_option_->merge(cfg_option_def_, (*other.getCfgOption())); + + // Merge shared networks. + cfg_shared_networks6_->merge(cfg_option_def_, *(other.getCfgSharedNetworks6())); + + // Merge subnets. + cfg_subnets6_->merge(cfg_option_def_, getCfgSharedNetworks4(), + *(other.getCfgSubnets6())); +#endif + + /// @todo merge other parts of the configuration here. +} + +void +SrvConfig::mergeGlobals(SrvConfig& other) { // Iterate over the "other" globals, adding/overwriting them into // this config's list of globals. for (auto other_global : other.getConfiguredGlobals()->mapValue()) { addConfiguredGlobal(other_global.first, other_global.second); } - // A handful of values are stored as members in SrvConfig. So we'll + // A handful of values are stored as members in SrvConfig. So we'll // iterate over the merged globals, setting approprate members. for (auto merged_global : getConfiguredGlobals()->mapValue()) { std::string name = merged_global.first; @@ -217,6 +245,8 @@ SrvConfig::mergeGlobals4(SrvConfig& other) { setDeclinePeriod(element->intValue()); } else if (name == "echo-client-id") { + // echo-client-id is v4 only, but we'll let upstream + // worry about that. setEchoClientId(element->boolValue()); } else if (name == "dhcp4o6port") { @@ -232,6 +262,7 @@ SrvConfig::mergeGlobals4(SrvConfig& other) { } } + void SrvConfig::removeStatistics() { diff --git a/src/lib/dhcpsrv/srv_config.h b/src/lib/dhcpsrv/srv_config.h index 12abefdd37..71636c8270 100644 --- a/src/lib/dhcpsrv/srv_config.h +++ b/src/lib/dhcpsrv/srv_config.h @@ -652,8 +652,34 @@ private: /// into this configuration. void merge4(SrvConfig& other); + /// @brief Merges the DHCPv6 configuration specified as a parameter into + /// this configuration. + /// + /// The general rule is that the configuration data from the @c other + /// object replaces configuration data held in this object instance. + /// The data that do not overlap between the two objects is simply + /// inserted into this configuration. + /// + /// @warning The call to @c merge may modify the data in the @c other + /// object. 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. + /// + /// The @c other parameter must be a @c SrvConfig or its derivation. + /// + /// Currently, the following parts of the v6 configuration are merged: + /// - globals + /// - shared-networks + /// - subnets + /// + /// @todo Add support for merging other configuration elements. + /// + /// @param other An object holding the configuration to be merged + /// into this configuration. + void merge6(SrvConfig& other); - /// @brief Merges the DHCPv4 globals specified in the given configuration + /// @brief Merges the globals specified in the given configuration /// into this configuration. /// /// Configurable global values may be specified either via JSON @@ -675,7 +701,7 @@ private: /// /// @param other An object holding the configuration to be merged /// into this configuration. - void mergeGlobals4(SrvConfig& other); + void mergeGlobals(SrvConfig& other); /// @brief Sequence number identifying the configuration. uint32_t sequence_; diff --git a/src/lib/dhcpsrv/tests/srv_config_unittest.cc b/src/lib/dhcpsrv/tests/srv_config_unittest.cc index 89a3dcdd23..b1a0a09f28 100644 --- a/src/lib/dhcpsrv/tests/srv_config_unittest.cc +++ b/src/lib/dhcpsrv/tests/srv_config_unittest.cc @@ -985,7 +985,7 @@ TEST_F(SrvConfigTest, mergeGlobals4) { // Let's create the "existing" config we will merge into. SrvConfig cfg_to; - // Set some explicit values. + // Set some explicit values. cfg_to.setDeclinePeriod(100); cfg_to.setEchoClientId(false); cfg_to.setDhcp4o6Port(777); @@ -1025,7 +1025,73 @@ TEST_F(SrvConfigTest, mergeGlobals4) { // server-tag port should be the "from" configured value. EXPECT_EQ("use_this_server", cfg_to.getServerTag().get()); - // Next we check the explicitly "configured" globals. + // Next we check the explicitly "configured" globals. + // The list should be all of the "to" + "from", with the + // latter overwriting the former. + std::string exp_globals = + "{ \n" + " \"decline-probation-period\": 300, \n" + " \"dhcp4o6port\": 999, \n" + " \"server-tag\": \"use_this_server\" \n" + "} \n"; + + ConstElementPtr expected_globals; + ASSERT_NO_THROW(expected_globals = Element::fromJSON(exp_globals)) + << "exp_globals didn't parse, test is broken"; + + EXPECT_TRUE(isEquivalent(expected_globals, cfg_to.getConfiguredGlobals())); + +} + +// This test verifies that globals from one SrvConfig +// can be merged into another. It verifies that values +// in the from-config override those in to-config which +// override those in GLOBAL4_DEFAULTS. +TEST_F(SrvConfigTest, mergeGlobals6) { + // Set the family we're working with. + CfgMgr::instance().setFamily(AF_INET6); + + // Let's create the "existing" config we will merge into. + SrvConfig cfg_to; + + // Set some explicit values. + cfg_to.setDeclinePeriod(100); + cfg_to.setEchoClientId(false); + cfg_to.setDhcp4o6Port(777); + cfg_to.setServerTag("not_this_server"); + + // Add some configured globals + cfg_to.addConfiguredGlobal("decline-probation-period", Element::create(300)); + cfg_to.addConfiguredGlobal("dhcp4o6port", Element::create(888)); + + // Now we'll create the config we'll merge from. + SrvConfig cfg_from; + + // Set some explicit values. None of these should be preserved. + cfg_from.setDeclinePeriod(200); + cfg_from.setEchoClientId(true); + cfg_from.setDhcp4o6Port(888); + cfg_from.setServerTag("nor_this_server"); + + // Add some configured globals: + cfg_to.addConfiguredGlobal("dhcp4o6port", Element::create(999)); + cfg_to.addConfiguredGlobal("server-tag", Element::create("use_this_server")); + + // Now let's merge. + ASSERT_NO_THROW(cfg_to.merge(cfg_from)); + + // Make sure the explicit values are set correctly. + + // decline-probation-period should be the "to" configured value. + EXPECT_EQ(300, cfg_to.getDeclinePeriod()); + + // dhcp4o6port should be the "from" configured value. + EXPECT_EQ(999, cfg_to.getDhcp4o6Port()); + + // server-tag port should be the "from" configured value. + EXPECT_EQ("use_this_server", cfg_to.getServerTag().get()); + + // Next we check the explicitly "configured" globals. // The list should be all of the "to" + "from", with the // latter overwriting the former. std::string exp_globals = -- GitLab From cee633f070eaa0f8fbc24423081d340e79626f0b Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Thu, 28 Mar 2019 13:11:17 -0400 Subject: [PATCH 2/9] [#413,!288] Added in new file to Makefile.am Missed adding it before. Minor mods to make the globals test work, and others to compile. src/bin/dhcp6/tests/Makefile.am src/bin/dhcp6/tests/config_backend_unittest.cc --- src/bin/dhcp6/tests/Makefile.am | 1 + .../dhcp6/tests/config_backend_unittest.cc | 26 ++++++------------- 2 files changed, 9 insertions(+), 18 deletions(-) diff --git a/src/bin/dhcp6/tests/Makefile.am b/src/bin/dhcp6/tests/Makefile.am index 81e5078edd..15ad57b9de 100644 --- a/src/bin/dhcp6/tests/Makefile.am +++ b/src/bin/dhcp6/tests/Makefile.am @@ -83,6 +83,7 @@ TESTS += dhcp6_unittests # this order. dhcp6_unittests_SOURCES = classify_unittests.cc dhcp6_unittests_SOURCES += config_parser_unittest.cc +dhcp6_unittests_SOURCES += config_backend_unittest.cc dhcp6_unittests_SOURCES += confirm_unittest.cc dhcp6_unittests_SOURCES += ctrl_dhcp6_srv_unittest.cc dhcp6_unittests_SOURCES += d2_unittest.cc d2_unittest.h diff --git a/src/bin/dhcp6/tests/config_backend_unittest.cc b/src/bin/dhcp6/tests/config_backend_unittest.cc index 0d4d50ffb6..893f2384fd 100644 --- a/src/bin/dhcp6/tests/config_backend_unittest.cc +++ b/src/bin/dhcp6/tests/config_backend_unittest.cc @@ -10,6 +10,7 @@ #include #include +#include #include #include #include @@ -168,11 +169,10 @@ TEST_F(Dhcp6CBTest, mergeGlobals) { " \"interfaces-config\": { \n" " \"interfaces\": [\"*\" ] \n" " }, \n" - " \"echo-client-id\": false, \n" " \"decline-probation-period\": 7000, \n" " \"valid-lifetime\": 1000, \n" " \"rebind-timer\": 800, \n" - " \"server-hostname\": \"overwrite.me.com\", \n" + " \"server-tag\": \"first-server\", \n" " \"config-control\": { \n" " \"config-databases\": [ { \n" " \"type\": \"memfile\", \n" @@ -188,18 +188,14 @@ TEST_F(Dhcp6CBTest, mergeGlobals) { extractConfig(base_config); // Make some globals: - StampedValuePtr server_hostname(new StampedValue("server-hostname", "isc.example.org")); + StampedValuePtr server_tag(new StampedValue("server-tag", "second-server")); StampedValuePtr decline_period(new StampedValue("decline-probation-period", Element::create(86400))); - StampedValuePtr calc_tee_times(new StampedValue("calculate-tee-times", Element::create(bool(false)))); - StampedValuePtr t2_percent(new StampedValue("t2-percent", Element::create(0.75))); StampedValuePtr renew_timer(new StampedValue("renew-timer", Element::create(500))); // Let's add all of the globals to the second backend. This will verify // we find them there. - db2_->createUpdateGlobalParameter6(ServerSelector::ALL(), server_hostname); + db2_->createUpdateGlobalParameter6(ServerSelector::ALL(), server_tag); db2_->createUpdateGlobalParameter6(ServerSelector::ALL(), decline_period); - db2_->createUpdateGlobalParameter6(ServerSelector::ALL(), calc_tee_times); - db2_->createUpdateGlobalParameter6(ServerSelector::ALL(), t2_percent); db2_->createUpdateGlobalParameter6(ServerSelector::ALL(), renew_timer); // Should parse and merge without error. @@ -209,10 +205,6 @@ TEST_F(Dhcp6CBTest, mergeGlobals) { // CfgMgr::instance().commit() hasn't been called) SrvConfigPtr staging_cfg = CfgMgr::instance().getStagingCfg(); - // echo-client-id is set explicitly in the original config, meanwhile - // the backend config does not set it, so the explicit value wins. - EXPECT_FALSE(staging_cfg->getEchoClientId()); - // decline-probation-period is an explicit member that should come // from the backend. EXPECT_EQ(86400, staging_cfg->getDeclinePeriod()); @@ -224,9 +216,7 @@ TEST_F(Dhcp6CBTest, mergeGlobals) { Element::create(800))); // Verify that the implicit globals from the backend are there. - ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal(staging_cfg, server_hostname)); - ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal(staging_cfg, calc_tee_times)); - ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal(staging_cfg, t2_percent)); + ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal(staging_cfg, server_tag)); ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal(staging_cfg, renew_timer)); } @@ -315,7 +305,7 @@ TEST_F(Dhcp6CBTest, DISABLED_mergeOptions) { " \"name\": \"dhcp-message\", \n" " \"data\": \"0A0B0C0D\", \n" " \"csv-format\": false \n" - " },{ \n" + " },{ \n" " \"name\": \"host-name\", \n" " \"data\": \"old.example.com\", \n" " \"csv-format\": true \n" @@ -372,7 +362,7 @@ TEST_F(Dhcp6CBTest, DISABLED_mergeOptions) { ASSERT_TRUE(found_opt.option_); EXPECT_EQ("0x0A0B0C0D", found_opt.option_->toHexString()); - // host-name should come from the first back end, + // host-name should come from the first back end, // (overwriting the original). found_opt = options->get("dhcp6", DHO_HOST_NAME); ASSERT_TRUE(found_opt.option_); @@ -425,7 +415,7 @@ TEST_F(Dhcp6CBTest, DISABLED_mergeSharedNetworks) { // CfgMgr::instance().commit() hasn't been called) SrvConfigPtr staging_cfg = CfgMgr::instance().getStagingCfg(); - CfgSharedNetworks4Ptr networks = staging_cfg->getCfgSharedNetworks4(); + CfgSharedNetworks6Ptr networks = staging_cfg->getCfgSharedNetworks6(); SharedNetwork6Ptr staged_network; // SharedNetwork One should have been added from db1 config -- GitLab From e0192a912868f139bf4a2b0e8431c0f182ca6446 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Thu, 28 Mar 2019 13:27:55 -0400 Subject: [PATCH 3/9] [#413,!288] kea-dhcp6 now uses option defs from config backends src/bin/dhcp6/tests/config_backend_unittest.cc set the CfgMgr family to AF_INET6 TEST_F(Dhcp6CBTest, mergeOptionDefs) - enabled test src/lib/dhcpsrv/srv_config.cc SrvConfig::merge6(SrvConfig& other) - now merges option defs --- src/bin/dhcp6/tests/config_backend_unittest.cc | 3 ++- src/lib/dhcpsrv/srv_config.cc | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/bin/dhcp6/tests/config_backend_unittest.cc b/src/bin/dhcp6/tests/config_backend_unittest.cc index 893f2384fd..a7add189c3 100644 --- a/src/bin/dhcp6/tests/config_backend_unittest.cc +++ b/src/bin/dhcp6/tests/config_backend_unittest.cc @@ -98,6 +98,7 @@ public: // Create fresh context. resetConfiguration(); + CfgMgr::instance().setFamily(AF_INET6); } /// Destructor @@ -222,7 +223,7 @@ TEST_F(Dhcp6CBTest, mergeGlobals) { // This test verifies that externally configured option definitions // merged correctly into staging configuration. -TEST_F(Dhcp6CBTest, DISABLED_mergeOptionDefs) { +TEST_F(Dhcp6CBTest, mergeOptionDefs) { string base_config = "{ \n" " \"option-def\": [ { \n" diff --git a/src/lib/dhcpsrv/srv_config.cc b/src/lib/dhcpsrv/srv_config.cc index 0aea3b208d..05a67bc3ee 100644 --- a/src/lib/dhcpsrv/srv_config.cc +++ b/src/lib/dhcpsrv/srv_config.cc @@ -206,13 +206,13 @@ SrvConfig::merge6(SrvConfig& other) { // Merge globals. mergeGlobals(other); -#if 0 // 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())); +#if 0 // Merge options. cfg_option_->merge(cfg_option_def_, (*other.getCfgOption())); -- GitLab From 72011040d1a49191b7c935a99dd459cb0463e8da Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Fri, 29 Mar 2019 10:41:45 -0400 Subject: [PATCH 4/9] [#413,!288] kea-dhcp6 now uses options from config backends src/bin/dhcp6/tests/config_backend_unittest.cc TEST_F(Dhcp6CBTest, mergeOptions) - enabled and revamped. src/lib/dhcpsrv/tests/cfg_option_unittest.cc TEST_F(CfgOptionTest, createDescriptorOptionValid) - added test of a standard V6 option src/lib/dhcpsrv/srv_config.cc SrvConfig::merge6(SrvConfig& other) - now merges options --- .../dhcp6/tests/config_backend_unittest.cc | 59 ++++++++----------- src/lib/dhcpsrv/srv_config.cc | 2 +- src/lib/dhcpsrv/tests/cfg_option_unittest.cc | 23 ++++++-- 3 files changed, 43 insertions(+), 41 deletions(-) diff --git a/src/bin/dhcp6/tests/config_backend_unittest.cc b/src/bin/dhcp6/tests/config_backend_unittest.cc index a7add189c3..799de93b4f 100644 --- a/src/bin/dhcp6/tests/config_backend_unittest.cc +++ b/src/bin/dhcp6/tests/config_backend_unittest.cc @@ -10,6 +10,7 @@ #include #include +#include #include #include #include @@ -299,17 +300,15 @@ TEST_F(Dhcp6CBTest, mergeOptionDefs) { // This test verifies that externally configured options // merged correctly into staging configuration. -TEST_F(Dhcp6CBTest, DISABLED_mergeOptions) { +TEST_F(Dhcp6CBTest, mergeOptions) { string base_config = "{ \n" " \"option-data\": [ { \n" - " \"name\": \"dhcp-message\", \n" - " \"data\": \"0A0B0C0D\", \n" - " \"csv-format\": false \n" + " \"name\": \"solmax-rt\", \n" + " \"data\": \"500\" \n" " },{ \n" - " \"name\": \"host-name\", \n" - " \"data\": \"old.example.com\", \n" - " \"csv-format\": true \n" + " \"name\": \"bootfile-url\", \n" + " \"data\": \"orig-boot-file\" \n" " } \n" " ], \n" " \"config-control\": { \n" @@ -324,55 +323,43 @@ TEST_F(Dhcp6CBTest, DISABLED_mergeOptions) { " } \n" "} \n"; - extractConfig(base_config); OptionDescriptorPtr opt; - - // Add host-name to the first backend. - opt.reset(new OptionDescriptor( - createOption(Option::V6, DHO_HOST_NAME, - true, false, "new.example.com"))); - opt->space_name_ = DHCP6_OPTION_SPACE; - db1_->createUpdateOption6(ServerSelector::ALL(), opt); - - // Add boot-file-name to the first backend. + // Add solmax-rt to the first backend. opt.reset(new OptionDescriptor( - createOption(Option::V6, DHO_BOOT_FILE_NAME, - true, false, "my-boot-file"))); + createOption(Option::V6, D6O_BOOTFILE_URL, + true, false, "updated-boot-file"))); opt->space_name_ = DHCP6_OPTION_SPACE; db1_->createUpdateOption6(ServerSelector::ALL(), opt); - // Add boot-file-name to the second backend. + // Add solmax-rt to the second backend. opt.reset(new OptionDescriptor( - createOption(Option::V6, DHO_BOOT_FILE_NAME, - true, false, "your-boot-file"))); + createOption(Option::V6, D6O_SOL_MAX_RT, + false, true, 700))); opt->space_name_ = DHCP6_OPTION_SPACE; db2_->createUpdateOption6(ServerSelector::ALL(), opt); // Should parse and merge without error. ASSERT_NO_FATAL_FAILURE(configure(base_config, CONTROL_RESULT_SUCCESS, "")); - // Verify the composite staging is correct. + // Now let's verify that composite staging options are correct. SrvConfigPtr staging_cfg = CfgMgr::instance().getStagingCfg(); - - // 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("dhcp6", DHO_DHCP_MESSAGE); - ASSERT_TRUE(found_opt.option_); - EXPECT_EQ("0x0A0B0C0D", found_opt.option_->toHexString()); - - // host-name should come from the first back end, + // bootfile-url should come from the first config back end. // (overwriting the original). - found_opt = options->get("dhcp6", DHO_HOST_NAME); + OptionDescriptor found_opt = options->get("dhcp6", D6O_BOOTFILE_URL); ASSERT_TRUE(found_opt.option_); - EXPECT_EQ("new.example.com", found_opt.option_->toString()); + OptionStringPtr opstr = boost::dynamic_pointer_cast(found_opt.option_); + ASSERT_TRUE(opstr); + EXPECT_EQ("updated-boot-file", opstr->getValue()); - // booth-file-name should come from the first back end. - found_opt = options->get("dhcp6", DHO_BOOT_FILE_NAME); + // sol-maxt-rt should come from the original config + found_opt = options->get("dhcp6", D6O_SOL_MAX_RT); ASSERT_TRUE(found_opt.option_); - EXPECT_EQ("my-boot-file", found_opt.option_->toString()); + OptionUint32Ptr opint = boost::dynamic_pointer_cast(found_opt.option_); + ASSERT_TRUE(opint); + EXPECT_EQ(500, opint->getValue()); } // This test verifies that externally configured shared-networks are diff --git a/src/lib/dhcpsrv/srv_config.cc b/src/lib/dhcpsrv/srv_config.cc index 05a67bc3ee..e42ae4c78a 100644 --- a/src/lib/dhcpsrv/srv_config.cc +++ b/src/lib/dhcpsrv/srv_config.cc @@ -212,10 +212,10 @@ SrvConfig::merge6(SrvConfig& other) { // definitions. cfg_option_def_->merge((*other.getCfgOptionDef())); -#if 0 // Merge options. cfg_option_->merge(cfg_option_def_, (*other.getCfgOption())); +#if 0 // Merge shared networks. cfg_shared_networks6_->merge(cfg_option_def_, *(other.getCfgSharedNetworks6())); diff --git a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc index cc77262c4b..52660d6faa 100644 --- a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include @@ -547,10 +548,10 @@ TEST_F(CfgOptionTest, createDescriptorOptionValid) { 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. + // We'll try a standard V4 option first. std::string space = "dhcp4"; - std::string value("example.org"); - OptionPtr option(new Option(Option::V4, DHO_HOST_NAME)); + std::string value = "v4.example.com"; + OptionPtr option(new Option(Option::V6, DHO_HOST_NAME)); option->setData(value.begin(), value.end()); OptionDescriptorPtr desc(new OptionDescriptor(option, false)); @@ -559,7 +560,21 @@ TEST_F(CfgOptionTest, createDescriptorOptionValid) { ASSERT_TRUE(updated); OptionStringPtr opstr = boost::dynamic_pointer_cast(desc->option_); ASSERT_TRUE(opstr); - EXPECT_EQ("example.org", opstr->getValue()); + EXPECT_EQ("v4.example.com", opstr->getValue()); + + // Next we'll try a standard V6 option. + space = "dhcp6"; + std::vector fqdn = + { 2, 'v', '6', 7, 'e', 'x', 'a', 'm', 'p', 'l', 'e', 3, 'c', 'o', 'm', 0 }; + option.reset(new Option(Option::V6, D6O_AFTR_NAME)); + option->setData(fqdn.begin(), fqdn.end()); + desc.reset(new OptionDescriptor(option, false)); + + ASSERT_NO_THROW(updated = CfgOption::createDescriptorOption(defs, space, *desc)); + ASSERT_TRUE(updated); + OptionCustomPtr opcustom = boost::dynamic_pointer_cast(desc->option_); + ASSERT_TRUE(opcustom); + EXPECT_EQ("v6.example.com.", opcustom->readFqdn()); // Next we'll try a vendor option with a formatted value space = "vendor-4491"; -- GitLab From ff367e273ed8763b354db272c5955a78203d865e Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Fri, 29 Mar 2019 15:17:57 -0400 Subject: [PATCH 5/9] [#413,!288] kea-dhcp6 now uses shared-networks config backends src/bin/dhcp6/tests/config_backend_unittest.cc TEST_F(Dhcp6CBTest, mergeSharedNetworks) - enabled test src/lib/dhcpsrv/tests/cfg_shared_networks6_unittest.cc TEST(CfgSharedNetworks6Test, mergeNetworks) - new test src/lib/dhcpsrv/cfg_shared_networks.* CfgSharedNetworks4::merge() CfgSharedNetworks4::getAll() - moved to CfgSharedNetworks template class --- .../dhcp6/tests/config_backend_unittest.cc | 2 +- src/lib/dhcpsrv/cfg_shared_networks.cc | 51 -------- src/lib/dhcpsrv/cfg_shared_networks.h | 99 +++++++++----- src/lib/dhcpsrv/srv_config.cc | 45 +++---- .../tests/cfg_shared_networks4_unittest.cc | 13 +- .../tests/cfg_shared_networks6_unittest.cc | 122 ++++++++++++++++++ 6 files changed, 215 insertions(+), 117 deletions(-) diff --git a/src/bin/dhcp6/tests/config_backend_unittest.cc b/src/bin/dhcp6/tests/config_backend_unittest.cc index 799de93b4f..b7108e522e 100644 --- a/src/bin/dhcp6/tests/config_backend_unittest.cc +++ b/src/bin/dhcp6/tests/config_backend_unittest.cc @@ -364,7 +364,7 @@ TEST_F(Dhcp6CBTest, mergeOptions) { // This test verifies that externally configured shared-networks are // merged correctly into staging configuration. -TEST_F(Dhcp6CBTest, DISABLED_mergeSharedNetworks) { +TEST_F(Dhcp6CBTest, mergeSharedNetworks) { string base_config = "{ \n" " \"interfaces-config\": { \n" diff --git a/src/lib/dhcpsrv/cfg_shared_networks.cc b/src/lib/dhcpsrv/cfg_shared_networks.cc index 7f5c48041f..75d7059e51 100644 --- a/src/lib/dhcpsrv/cfg_shared_networks.cc +++ b/src/lib/dhcpsrv/cfg_shared_networks.cc @@ -19,56 +19,5 @@ CfgSharedNetworks4::hasNetworkWithServerId(const IOAddress& server_id) const { return (network_it != index.cend()); } - - -void -CfgSharedNetworks4::merge(CfgOptionDefPtr cfg_def, CfgSharedNetworks4& other) { - auto& index = networks_.get(); - - // Iterate over the subnets to be merged. They will replace the existing - // subnets with the same id. All new subnets will be inserted into this - // configuration. - auto other_networks = other.getAll(); - for (auto other_network = other_networks->begin(); - other_network != other_networks->end(); ++other_network) { - - // In theory we should drop subnet assignments from "other". The - // idea being those that come from the CB should not have subnets_ - // populated. We will quietly throw them away, just in case. - (*other_network)->delAll(); - - // Check if the other network exists in this config. - auto existing_network = index.find((*other_network)->getName()); - if (existing_network != index.end()) { - - // Somehow the same instance is in both, skip it. - if (*existing_network == *other_network) { - continue; - } - - // Network exists, which means we're updating it. - // First we need to move its subnets to the new - // version of the network. - const Subnet4Collection* subnets = (*existing_network)->getAllSubnets(); - - Subnet4Collection copy_subnets(*subnets); - for (auto subnet = copy_subnets.cbegin(); subnet != copy_subnets.cend(); ++subnet) { - (*existing_network)->del((*subnet)->getID()); - (*other_network)->add(*subnet); - } - - // Now we discard the existing copy of the network. - 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); - } - -} - } // end of namespace isc::dhcp } // end of namespace isc diff --git a/src/lib/dhcpsrv/cfg_shared_networks.h b/src/lib/dhcpsrv/cfg_shared_networks.h index f97b4dbd69..8072306a24 100644 --- a/src/lib/dhcpsrv/cfg_shared_networks.h +++ b/src/lib/dhcpsrv/cfg_shared_networks.h @@ -33,6 +33,10 @@ namespace dhcp { template class CfgSharedNetworks : public data::CfgToElement { public: + /// @brief Returns pointer to all configured shared networks. + const SharedNetworkCollection* getAll() const { + return (&networks_); + } /// @brief Adds new shared network to the configuration. /// @@ -126,30 +130,6 @@ public: return (list); } -protected: - - /// @brief Multi index container holding shared networks. - SharedNetworkCollection networks_; -}; - -/// @brief Represents configuration of IPv4 shared networks. -class CfgSharedNetworks4 : public CfgSharedNetworks { -public: - - /// @brief Returns pointer to all configured shared networks. - const SharedNetwork4Collection* getAll() const { - return (&networks_); - } - - /// @brief Checks if specified server identifier has been specified for - /// any network. - /// - /// @param server_id Server identifier. - /// - /// @return true if there is a network with a specified server identifier. - bool hasNetworkWithServerId(const asiolink::IOAddress& server_id) const; - /// @brief Merges specified shared network configuration into this /// configuration. /// @@ -179,7 +159,70 @@ public: /// when creating option instances. /// @param other the shared network configuration to be merged into this /// configuration. - void merge(CfgOptionDefPtr cfg_def, CfgSharedNetworks4& other); + void merge(CfgOptionDefPtr cfg_def, CfgSharedNetworks& other) { + auto& index = networks_.template get(); + + // Iterate over the subnets to be merged. They will replace the existing + // subnets with the same id. All new subnets will be inserted into this + // configuration. + auto other_networks = other.getAll(); + for (auto other_network = other_networks->begin(); + other_network != other_networks->end(); ++other_network) { + + // In theory we should drop subnet assignments from "other". The + // idea being those that come from the CB should not have subnets_ + // populated. We will quietly throw them away, just in case. + (*other_network)->delAll(); + + // Check if the other network exists in this config. + auto existing_network = index.find((*other_network)->getName()); + if (existing_network != index.end()) { + + // Somehow the same instance is in both, skip it. + if (*existing_network == *other_network) { + continue; + } + + // Network exists, which means we're updating it. + // First we need to move its subnets to the new + // version of the network. + const auto subnets = (*existing_network)->getAllSubnets(); + + auto copy_subnets(*subnets); + for (auto subnet = copy_subnets.cbegin(); subnet != copy_subnets.cend(); ++subnet) { + (*existing_network)->del((*subnet)->getID()); + (*other_network)->add(*subnet); + } + + // Now we discard the existing copy of the network. + 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); + } + } + +protected: + + /// @brief Multi index container holding shared networks. + SharedNetworkCollection networks_; +}; + +/// @brief Represents configuration of IPv4 shared networks. +class CfgSharedNetworks4 : public CfgSharedNetworks { +public: + /// @brief Checks if specified server identifier has been specified for + /// any network. + /// + /// @param server_id Server identifier. + /// + /// @return true if there is a network with a specified server identifier. + bool hasNetworkWithServerId(const asiolink::IOAddress& server_id) const; }; /// @brief Pointer to the configuration of IPv4 shared networks. @@ -188,12 +231,6 @@ typedef boost::shared_ptr CfgSharedNetworks4Ptr; /// @brief Represents configuration of IPv6 shared networks. class CfgSharedNetworks6 : public CfgSharedNetworks { -public: - - /// @brief Returns pointer to all configured shared networks. - const SharedNetwork6Collection* getAll() const { - return (&networks_); - } }; /// @brief Pointer to the configuration of IPv6 shared networks. diff --git a/src/lib/dhcpsrv/srv_config.cc b/src/lib/dhcpsrv/srv_config.cc index e42ae4c78a..37b8a12bdb 100644 --- a/src/lib/dhcpsrv/srv_config.cc +++ b/src/lib/dhcpsrv/srv_config.cc @@ -161,6 +161,21 @@ SrvConfig::merge(ConfigBase& other) { ConfigBase::merge(other); try { SrvConfig& other_srv_config = dynamic_cast(other); + // We merge objects in order of dependency (real or theoretical). + // First we merge the common stuff. + + // Merge globals. + mergeGlobals(other_srv_config); + + // 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_srv_config.getCfgOptionDef())); + + // Merge options. + cfg_option_->merge(cfg_option_def_, (*other_srv_config.getCfgOption())); + if (CfgMgr::instance().getFamily() == AF_INET) { merge4(other_srv_config); } else { @@ -175,20 +190,6 @@ SrvConfig::merge(ConfigBase& other) { void SrvConfig::merge4(SrvConfig& other) { - // We merge objects in order of dependency (real or theoretical). - - // Merge globals. - mergeGlobals(other); - - // 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. - cfg_option_->merge(cfg_option_def_, (*other.getCfgOption())); - // Merge shared networks. cfg_shared_networks4_->merge(cfg_option_def_, *(other.getCfgSharedNetworks4())); @@ -201,24 +202,10 @@ SrvConfig::merge4(SrvConfig& other) { void SrvConfig::merge6(SrvConfig& other) { - // We merge objects in order of dependency (real or theoretical). - - // Merge globals. - mergeGlobals(other); - - // 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. - cfg_option_->merge(cfg_option_def_, (*other.getCfgOption())); - -#if 0 // Merge shared networks. cfg_shared_networks6_->merge(cfg_option_def_, *(other.getCfgSharedNetworks6())); +#if 0 // Merge subnets. cfg_subnets6_->merge(cfg_option_def_, getCfgSharedNetworks4(), *(other.getCfgSubnets6())); diff --git a/src/lib/dhcpsrv/tests/cfg_shared_networks4_unittest.cc b/src/lib/dhcpsrv/tests/cfg_shared_networks4_unittest.cc index 1332fe2199..8e9ea86a82 100644 --- a/src/lib/dhcpsrv/tests/cfg_shared_networks4_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_shared_networks4_unittest.cc @@ -18,6 +18,11 @@ using namespace asiolink; namespace { +/// Attempts to verify an expected network within a collection of networks +/// @param networks set of networks in which to look +/// @param name name of the expected network +/// @param exp_valid expected valid lifetime of the network +/// @param exp_subnets list of subnet IDs the network is expected to own void checkMergedNetwork(const CfgSharedNetworks4& networks, const std::string& name, const Triplet& exp_valid, const std::vector& exp_subnets) { @@ -288,9 +293,6 @@ TEST(CfgSharedNetworks4Test, mergeNetworks) { 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. @@ -318,8 +320,9 @@ TEST(CfgSharedNetworks4Test, mergeNetworks) { // 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()); + OptionStringPtr opstr = boost::dynamic_pointer_cast(desc.option_); + ASSERT_TRUE(opstr); + EXPECT_EQ("Yay!", opstr->getValue()); // No changes to network2. ASSERT_NO_FATAL_FAILURE(checkMergedNetwork(cfg_to, "network2", Triplet(200), diff --git a/src/lib/dhcpsrv/tests/cfg_shared_networks6_unittest.cc b/src/lib/dhcpsrv/tests/cfg_shared_networks6_unittest.cc index c561c93f02..5add7d9859 100644 --- a/src/lib/dhcpsrv/tests/cfg_shared_networks6_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_shared_networks6_unittest.cc @@ -6,6 +6,7 @@ #include #include +#include #include #include #include @@ -17,6 +18,25 @@ using namespace asiolink; namespace { +/// Attempts to verify an expected network within a collection of networks +/// @param networks set of networks in which to look +/// @param name name of the expected network +/// @param exp_valid expected valid lifetime of the network +/// @param exp_subnets list of subnet IDs the network is expected to own +void checkMergedNetwork(const CfgSharedNetworks6& networks, const std::string& name, + const Triplet& exp_valid, + const std::vector& exp_subnets) { + auto network = networks.getByName(name); + ASSERT_TRUE(network) << "expected network: " << name << " not found"; + ASSERT_EQ(exp_valid, network->getValid()) << " network valid lifetime wrong"; + const Subnet6Collection* subnets = network->getAllSubnets(); + ASSERT_EQ(exp_subnets.size(), subnets->size()) << " wrong number of subnets"; + for (auto exp_id : exp_subnets) { + ASSERT_TRUE(network->getSubnet(exp_id)) + << " did not find expected subnet: " << exp_id; + } +} + // This test verifies that shared networks can be added to the configruation // and retrieved by name. TEST(CfgSharedNetworks6Test, getByName) { @@ -208,4 +228,106 @@ TEST(CfgSharedNetworks6Test, unparse) { test::runToElementTest(expected, cfg); } +// This test verifies that shared-network configurations are properly merged. +TEST(CfgSharedNetworks6Test, 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"); + + Subnet6Ptr subnet1(new Subnet6(IOAddress("2001:1::"), + 64, 60, 80, 100, 200, SubnetID(1))); + Subnet6Ptr subnet2(new Subnet6(IOAddress("2001:2::"), + 64, 60, 80, 100, 200, SubnetID(2))); + Subnet6Ptr subnet3(new Subnet6(IOAddress("2001:3::"), + 64, 60, 80, 100, 200, SubnetID(3))); + Subnet6Ptr subnet4(new Subnet6(IOAddress("2001:4::"), + 64, 60, 80, 100, 200, SubnetID(4))); + + // Create network1 and add two subnets to it + SharedNetwork6Ptr network1(new SharedNetwork6("network1")); + network1->setValid(Triplet(100)); + ASSERT_NO_THROW(network1->add(subnet1)); + ASSERT_NO_THROW(network1->add(subnet2)); + + // Create network2 with no subnets. + SharedNetwork6Ptr network2(new SharedNetwork6("network2")); + network2->setValid(Triplet(200)); + + // Create network3 with one subnet. + SharedNetwork6Ptr network3(new SharedNetwork6("network3")); + network3->setValid(Triplet(300)); + ASSERT_NO_THROW(network3->add(subnet3)); + + // Create our "existing" configured networks. + // Add all three networks to the existing config. + CfgSharedNetworks6 cfg_to; + ASSERT_NO_THROW(cfg_to.add(network1)); + ASSERT_NO_THROW(cfg_to.add(network2)); + ASSERT_NO_THROW(cfg_to.add(network3)); + + // Merge in an "empty" config. Should have the original config, still intact. + CfgSharedNetworks6 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())); + + ASSERT_NO_FATAL_FAILURE(checkMergedNetwork(cfg_to, "network3", Triplet(300), + std::vector{SubnetID(3)})); + + // Create network1b, this is an "update" of network1 + // We'll double the valid time and add subnet4 to it + SharedNetwork6Ptr network1b(new SharedNetwork6("network1")); + network1b->setValid(Triplet(200)); + + // Now let's a add generic option 1 to network1b. + std::string value("Yay!"); + OptionPtr option(new Option(Option::V6, 1)); + option->setData(value.begin(), value.end()); + ASSERT_NO_THROW(network1b->getCfgOption()->add(option, false, "isc")); + ASSERT_NO_THROW(network1b->add(subnet4)); + + // Network2 we will not touch. + + // Create network3b, this is an "update" of network3. + // We'll double it's valid time, but leave off the subnet. + SharedNetwork6Ptr network3b(new SharedNetwork6("network3")); + network3b->setValid(Triplet(600)); + + // Create our "existing" configured networks. + ASSERT_NO_THROW(cfg_from.add(network1b)); + ASSERT_NO_THROW(cfg_from.add(network3b)); + + ASSERT_NO_THROW(cfg_to.merge(cfg_def, cfg_from)); + + // Should still have 3 networks. + + // Network1 should have doubled its valid lifetime but still only have + // the orignal two subnets. Merge should discard assocations on CB + // subnets and preserve the associations from existing config. + ASSERT_EQ(3, cfg_to.getAll()->size()); + 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_); + OptionStringPtr opstr = boost::dynamic_pointer_cast(desc.option_); + ASSERT_TRUE(opstr); + EXPECT_EQ("Yay!", opstr->getValue()); + + // No changes to network2. + ASSERT_NO_FATAL_FAILURE(checkMergedNetwork(cfg_to, "network2", Triplet(200), + std::vector())); + + // Network1 should have doubled its valid lifetime and still subnet3. + ASSERT_NO_FATAL_FAILURE(checkMergedNetwork(cfg_to, "network3", Triplet(600), + std::vector{SubnetID(3)})); +} + } // end of anonymous namespace -- GitLab From 96e2832176f384c2c450cbb5df52081fc7562e23 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Mon, 1 Apr 2019 08:36:22 -0400 Subject: [PATCH 6/9] [#413,!288] kea-dhcp6 now uses subnets from config backends src/bin/dhcp6/tests/config_backend_unittest.cc TEST_F(Dhcp6CBTest, mergeSubnets) - updated and enabled src/lib/dhcpsrv/cfg_subnets6.* CfgSubnets6::merge() - new method src/lib/dhcpsrv/srv_config.cc SrvConfig::merge6() - now merges subnets src/lib/dhcpsrv/tests/cfg_subnets4_unittest.cc minor cleanup src/lib/dhcpsrv/tests/cfg_subnets6_unittest.cc checkMergedSubnet() - new function TEST(CfgSubnets6Test, mergeSubnets) - new test --- .../dhcp6/tests/config_backend_unittest.cc | 8 +- src/lib/dhcpsrv/cfg_subnets6.cc | 73 +++++++ src/lib/dhcpsrv/cfg_subnets6.h | 45 ++++ src/lib/dhcpsrv/srv_config.cc | 4 +- .../dhcpsrv/tests/cfg_subnets4_unittest.cc | 45 ++-- .../dhcpsrv/tests/cfg_subnets6_unittest.cc | 202 ++++++++++++++++++ 6 files changed, 351 insertions(+), 26 deletions(-) diff --git a/src/bin/dhcp6/tests/config_backend_unittest.cc b/src/bin/dhcp6/tests/config_backend_unittest.cc index b7108e522e..00a8da44cf 100644 --- a/src/bin/dhcp6/tests/config_backend_unittest.cc +++ b/src/bin/dhcp6/tests/config_backend_unittest.cc @@ -424,7 +424,7 @@ TEST_F(Dhcp6CBTest, mergeSharedNetworks) { // This test verifies that externally configured subnets are // merged correctly into staging configuration. -TEST_F(Dhcp6CBTest, DISABLED_mergeSubnets) { +TEST_F(Dhcp6CBTest, mergeSubnets) { string base_config = "{ \n" " \"interfaces-config\": { \n" @@ -444,15 +444,15 @@ TEST_F(Dhcp6CBTest, DISABLED_mergeSubnets) { " \"subnet6\": [ \n" " { \n" " \"id\": 2,\n" - " \"subnet\": \"192.0.3.0/24\" \n" + " \"subnet\": \"2001:2::/64\" \n" " } ]\n" "} \n"; extractConfig(base_config); // Make a few subnets - Subnet6Ptr subnet1(new Subnet6(IOAddress("192.0.2.0"), 26, 1, 2, 3, SubnetID(1))); - Subnet6Ptr subnet3(new Subnet6(IOAddress("192.0.4.0"), 26, 1, 2, 3, SubnetID(3))); + Subnet6Ptr subnet1(new Subnet6(IOAddress("2001:1::"), 64, 1, 2, 100, 100, SubnetID(1))); + Subnet6Ptr subnet3(new Subnet6(IOAddress("2001:3::"), 64, 1, 2, 100, 100, SubnetID(3))); // Add subnet1 to db1 and subnet3 to db2 db1_->createUpdateSubnet6(ServerSelector::ALL(), subnet1); diff --git a/src/lib/dhcpsrv/cfg_subnets6.cc b/src/lib/dhcpsrv/cfg_subnets6.cc index b087ef4f59..3167e3b7b4 100644 --- a/src/lib/dhcpsrv/cfg_subnets6.cc +++ b/src/lib/dhcpsrv/cfg_subnets6.cc @@ -63,6 +63,79 @@ CfgSubnets6::del(const SubnetID& subnet_id) { .arg(subnet->toText()); } +void +CfgSubnets6::merge(CfgOptionDefPtr cfg_def, CfgSharedNetworks6Ptr networks, + CfgSubnets6& other) { + auto& index = subnets_.get(); + + // Iterate over the subnets to be merged. They will replace the existing + // subnets with the same id. All new subnets will be inserted into the + // configuration into which we're merging. + auto other_subnets = other.getAll(); + for (auto other_subnet = other_subnets->begin(); + other_subnet != other_subnets->end(); + ++other_subnet) { + + // Check if there is a subnet with the same ID. + auto subnet_it = index.find((*other_subnet)->getID()); + if (subnet_it != index.end()) { + + // Subnet found. + auto existing_subnet = *subnet_it; + + // If the existing subnet and other subnet + // are the same instance skip it. + if (existing_subnet == *other_subnet) { + continue; + } + + // We're going to replace the existing subnet with the other + // version. If it belongs to a shared network, we need + // remove it from that network. + SharedNetwork6Ptr network; + existing_subnet->getSharedNetwork(network); + if (network) { + network->del(existing_subnet->getID()); + } + + // Now we remove the existing subnet. + index.erase(subnet_it); + } + + // Create the subnet's options based on the given definitions. + (*other_subnet)->getCfgOption()->createOptions(cfg_def); + + // Create the options for pool based on the given definitions. + for (auto pool : (*other_subnet)->getPoolsWritable(Lease::TYPE_NA)) { + pool->getCfgOption()->createOptions(cfg_def); + } + + for (auto pool : (*other_subnet)->getPoolsWritable(Lease::TYPE_PD)) { + pool->getCfgOption()->createOptions(cfg_def); + } + + // Add the "other" subnet to the our collection of subnets. + subnets_.push_back(*other_subnet); + + // If it belongs to a shared network, find the network and + // add the subnet to it + std::string network_name = (*other_subnet)->getSharedNetworkName(); + if (!network_name.empty()) { + SharedNetwork6Ptr network = networks->getByName(network_name); + if (network) { + network->add(*other_subnet); + } else { + // This implies the shared-network collection we were given + // is out of sync with the subnets we were given. + isc_throw(InvalidOperation, "Cannot assign subnet ID of " + << (*other_subnet)->getID() + << " to shared network: " << network_name + << ", network does not exist"); + } + } + } +} + ConstSubnet6Ptr CfgSubnets6::getBySubnetId(const SubnetID& subnet_id) const { const auto& index = subnets_.get(); diff --git a/src/lib/dhcpsrv/cfg_subnets6.h b/src/lib/dhcpsrv/cfg_subnets6.h index ce3e1dd9f4..caf98e4ac3 100644 --- a/src/lib/dhcpsrv/cfg_subnets6.h +++ b/src/lib/dhcpsrv/cfg_subnets6.h @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -55,6 +56,50 @@ public: /// @throw isc::BadValue if such subnet doesn't exist. void del(const SubnetID& subnet_id); + /// @brief Merges specified subnet configuration into this configuration. + /// + /// This method merges subnets from the @c other configuration into this + /// configuration. The general rule is that existing subnets are replaced + /// by the subnets from @c other. If there is no corresponding subnet in + /// this configuration the subnet from @c other configuration is inserted. + /// + /// The complexity of the merge process stems from the associations between + /// the subnets and shared networks. It is assumed that subnets in @c other + /// are the authority on their shared network assignments. It is also + /// assumed that @ networks is the list of shared networks that should be + /// used in making assignments. The general concept is that the overarching + /// merge process will first merge shared networks and then pass that list + /// of networks into this method. Subnets from @c other are then merged + /// into this configuration as follows: + /// + /// For each subnet in @c other: + /// + /// - If a subnet of the same ID already exists in this configuration: + /// -# 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. + /// + /// @warning The merge operation affects 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 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(CfgOptionDefPtr cfg_def, CfgSharedNetworks6Ptr networks, + CfgSubnets6& other); + /// @brief Returns pointer to the collection of all IPv6 subnets. /// /// This is used in a hook (subnet6_select), where the hook is able diff --git a/src/lib/dhcpsrv/srv_config.cc b/src/lib/dhcpsrv/srv_config.cc index 37b8a12bdb..a4d3b59a61 100644 --- a/src/lib/dhcpsrv/srv_config.cc +++ b/src/lib/dhcpsrv/srv_config.cc @@ -205,11 +205,9 @@ SrvConfig::merge6(SrvConfig& other) { // Merge shared networks. cfg_shared_networks6_->merge(cfg_option_def_, *(other.getCfgSharedNetworks6())); -#if 0 // Merge subnets. - cfg_subnets6_->merge(cfg_option_def_, getCfgSharedNetworks4(), + cfg_subnets6_->merge(cfg_option_def_, getCfgSharedNetworks6(), *(other.getCfgSubnets6())); -#endif /// @todo merge other parts of the configuration here. } diff --git a/src/lib/dhcpsrv/tests/cfg_subnets4_unittest.cc b/src/lib/dhcpsrv/tests/cfg_subnets4_unittest.cc index e21b9bb33a..600adce08b 100644 --- a/src/lib/dhcpsrv/tests/cfg_subnets4_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_subnets4_unittest.cc @@ -34,9 +34,16 @@ using namespace isc::test; namespace { +/// @brief Verifies that a set of subnets contains a given a subnet +/// +/// @param cfg_subnets set of sunbets in which to look +/// @param prefix prefix of the target subnet +/// @param exp_subnet_id expected id of the target subnet +/// @param exp_valid expected valid lifetime of the subnet +/// @param exp_network pointer to the subnet's shared-network (if one) void checkMergedSubnet(CfgSubnets4& cfg_subnets, - const SubnetID exp_subnet_id, const std::string& prefix, + const SubnetID exp_subnet_id, int exp_valid, SharedNetwork4Ptr exp_network) { // Look for the network by prefix. @@ -190,14 +197,14 @@ TEST(CfgSubnets4Test, mergeSubnets) { 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)); - ASSERT_NO_FATAL_FAILURE(checkMergedSubnet(cfg_to, SubnetID(2), - "192.0.2.0/26", 100, shared_network2)); - ASSERT_NO_FATAL_FAILURE(checkMergedSubnet(cfg_to, SubnetID(3), - "192.0.3.0/26", 100, no_network)); - ASSERT_NO_FATAL_FAILURE(checkMergedSubnet(cfg_to, SubnetID(4), - "192.0.4.0/26", 100, shared_network2)); + ASSERT_NO_FATAL_FAILURE(checkMergedSubnet(cfg_to, "192.0.1.0/26", + SubnetID(1), 100, shared_network1)); + ASSERT_NO_FATAL_FAILURE(checkMergedSubnet(cfg_to, "192.0.2.0/26", + SubnetID(2), 100, shared_network2)); + ASSERT_NO_FATAL_FAILURE(checkMergedSubnet(cfg_to, "192.0.3.0/26", + SubnetID(3), 100, no_network)); + ASSERT_NO_FATAL_FAILURE(checkMergedSubnet(cfg_to, "192.0.4.0/26", + SubnetID(4), 100, shared_network2)); // Fill cfg_from configuration with subnets. // subnet 1b updates subnet 1 but leaves it in network 1 @@ -259,8 +266,8 @@ TEST(CfgSubnets4Test, mergeSubnets) { 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)); + ASSERT_NO_FATAL_FAILURE(checkMergedSubnet(cfg_to, "192.0.1.0/26", + SubnetID(1), 400, shared_network1)); // Let's verify that our option is there and populated correctly. auto subnet = cfg_to.getByPrefix("192.0.1.0/26"); @@ -271,12 +278,12 @@ TEST(CfgSubnets4Test, mergeSubnets) { 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), - "192.0.2.0/26", 100, shared_network2)); + ASSERT_NO_FATAL_FAILURE(checkMergedSubnet(cfg_to, "192.0.2.0/26", + SubnetID(2), 100, shared_network2)); // 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)); + ASSERT_NO_FATAL_FAILURE(checkMergedSubnet(cfg_to, "192.0.3.0/26", + SubnetID(3), 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); @@ -286,12 +293,12 @@ TEST(CfgSubnets4Test, mergeSubnets) { EXPECT_EQ("Team!", opstr->getValue()); // subnet4 should be replaced by subnet4b and moved to network1. - ASSERT_NO_FATAL_FAILURE(checkMergedSubnet(cfg_to, SubnetID(4), - "192.0.4.0/26", 500, shared_network1)); + ASSERT_NO_FATAL_FAILURE(checkMergedSubnet(cfg_to, "192.0.4.0/26", + SubnetID(4), 500, shared_network1)); // subnet5 should have been added to configuration. - ASSERT_NO_FATAL_FAILURE(checkMergedSubnet(cfg_to, SubnetID(5), - "192.0.5.0/26", 300, shared_network2)); + ASSERT_NO_FATAL_FAILURE(checkMergedSubnet(cfg_to, "192.0.5.0/26", + SubnetID(5), 300, shared_network2)); // Let's verify that both pools have the proper options. subnet = cfg_to.getByPrefix("192.0.5.0/26"); diff --git a/src/lib/dhcpsrv/tests/cfg_subnets6_unittest.cc b/src/lib/dhcpsrv/tests/cfg_subnets6_unittest.cc index c411abf1d0..9d631039da 100644 --- a/src/lib/dhcpsrv/tests/cfg_subnets6_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_subnets6_unittest.cc @@ -8,6 +8,8 @@ #include #include #include +#include +#include #include #include #include @@ -32,6 +34,39 @@ generateInterfaceId(const std::string& text) { return OptionPtr(new Option(Option::V6, D6O_INTERFACE_ID, buffer)); } +/// @brief Verifies that a set of subnets contains a given a subnet +/// +/// @param cfg_subnets set of sunbets in which to look +/// @param exp_subnet_id expected id of the target subnet +/// @param prefix prefix of the target subnet +/// @param exp_valid expected valid lifetime of the subnet +/// @param exp_network pointer to the subnet's shared-network (if one) +void checkMergedSubnet(CfgSubnets6& cfg_subnets, + const std::string& prefix, + const SubnetID exp_subnet_id, + int exp_valid, + SharedNetwork6Ptr exp_network) { + // 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()) << "subnetID:" + << subnet->getID() << ", subnet valid time is wrong"; + + SharedNetwork6Ptr shared_network; + subnet->getSharedNetwork(shared_network); + if (exp_network) { + ASSERT_TRUE(shared_network) + << " expected network: " << exp_network->getName() << " not found"; + ASSERT_TRUE(shared_network == exp_network) << " networks do no match"; + } else { + ASSERT_FALSE(shared_network) << " unexpected network assignment: " + << shared_network->getName(); + } +} + // This test verifies that specific subnet can be retrieved by specifying // subnet identifier or subnet prefix. TEST(CfgSubnets6Test, getSpecificSubnet) { @@ -634,4 +669,171 @@ TEST(CfgSubnets6Test, getSubnet) { EXPECT_EQ(Subnet6Ptr(), cfg.getSubnet(400)); // no such subnet } +// This test verifies that subnets configuration is properly merged. +TEST(CfgSubnets6Test, 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"); + + Subnet6Ptr subnet1(new Subnet6(IOAddress("2001:1::"), + 64, 1, 2, 100, 100, SubnetID(1))); + Subnet6Ptr subnet2(new Subnet6(IOAddress("2001:2::"), + 64, 1, 2, 100, 100, SubnetID(2))); + Subnet6Ptr subnet3(new Subnet6(IOAddress("2001:3::"), + 64, 1, 2, 100, 100, SubnetID(3))); + Subnet6Ptr subnet4(new Subnet6(IOAddress("2001:4::"), + 64, 1, 2, 100, 100, SubnetID(4))); + + // Create the "existing" list of shared networks + CfgSharedNetworks6Ptr networks(new CfgSharedNetworks6()); + SharedNetwork6Ptr shared_network1(new SharedNetwork6("shared-network1")); + networks->add(shared_network1); + SharedNetwork6Ptr shared_network2(new SharedNetwork6("shared-network2")); + networks->add(shared_network2); + + // Empty network pointer. + SharedNetwork6Ptr no_network; + + // Add Subnets 1, 2 and 4 to shared networks. + ASSERT_NO_THROW(shared_network1->add(subnet1)); + ASSERT_NO_THROW(shared_network2->add(subnet2)); + ASSERT_NO_THROW(shared_network2->add(subnet4)); + + // Create our "existing" configured subnets. + CfgSubnets6 cfg_to; + ASSERT_NO_THROW(cfg_to.add(subnet1)); + ASSERT_NO_THROW(cfg_to.add(subnet2)); + 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. + CfgSubnets6 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, "2001:1::/64", + SubnetID(1), 100, shared_network1)); + ASSERT_NO_FATAL_FAILURE(checkMergedSubnet(cfg_to, "2001:2::/64", + SubnetID(2), 100, shared_network2)); + ASSERT_NO_FATAL_FAILURE(checkMergedSubnet(cfg_to, "2001:3::/64", + SubnetID(3), 100, no_network)); + ASSERT_NO_FATAL_FAILURE(checkMergedSubnet(cfg_to, "2001:4::/64", + SubnetID(4), 100, shared_network2)); + + // Fill cfg_from configuration with subnets. + // subnet 1b updates subnet 1 but leaves it in network 1 + Subnet6Ptr subnet1b(new Subnet6(IOAddress("2001:1::"), + 64, 2, 3, 100, 400, SubnetID(1))); + subnet1b->setSharedNetworkName("shared-network1"); + + // Add generic option 1 to subnet 1b. + std::string value("Yay!"); + OptionPtr option(new Option(Option::V6, 1)); + option->setData(value.begin(), value.end()); + ASSERT_NO_THROW(subnet1b->getCfgOption()->add(option, false, "isc")); + + // subnet 3b updates subnet 3 and removes it from network 2 + Subnet6Ptr subnet3b(new Subnet6(IOAddress("2001:3::"), + 64, 3, 4, 100, 500, SubnetID(3))); + + // Now Add generic option 1 to subnet 3b. + value = "Team!"; + option.reset(new Option(Option::V6, 1)); + option->setData(value.begin(), value.end()); + ASSERT_NO_THROW(subnet3b->getCfgOption()->add(option, false, "isc")); + + // subnet 4b updates subnet 4 and moves it from network2 to network 1 + Subnet6Ptr subnet4b(new Subnet6(IOAddress("2001:4::"), + 64, 3, 4, 100, 500, SubnetID(4))); + subnet4b->setSharedNetworkName("shared-network1"); + + // subnet 5 is new and belongs to network 2 + // Has two pools both with an option 1 + Subnet6Ptr subnet5(new Subnet6(IOAddress("2001:5::"), + 64, 1, 2, 100, 300, SubnetID(5))); + subnet5->setSharedNetworkName("shared-network2"); + + // Add pool 1 + Pool6Ptr pool(new Pool6(Lease::TYPE_NA, IOAddress("2001:5::10"), IOAddress("2001:5::20"))); + value = "POOLS"; + option.reset(new Option(Option::V6, 1)); + option->setData(value.begin(), value.end()); + ASSERT_NO_THROW(pool->getCfgOption()->add(option, false, "isc")); + subnet5->addPool(pool); + + // Add pool 2 + pool.reset(new Pool6(Lease::TYPE_PD, IOAddress("2001:6::"), 64)); + value ="RULE!"; + option.reset(new Option(Option::V6, 1)); + option->setData(value.begin(), value.end()); + ASSERT_NO_THROW(pool->getCfgOption()->add(option, false, "isc")); + subnet5->addPool(pool); + + // Add subnets to the merge from config. + ASSERT_NO_THROW(cfg_from.add(subnet1b)); + ASSERT_NO_THROW(cfg_from.add(subnet3b)); + ASSERT_NO_THROW(cfg_from.add(subnet4b)); + ASSERT_NO_THROW(cfg_from.add(subnet5)); + + // Merge again. + 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, "2001:1::/64", + SubnetID(1), 400, shared_network1)); + + // Let's verify that our option is there and populated correctly. + auto subnet = cfg_to.getByPrefix("2001:1::/64"); + auto desc = subnet->getCfgOption()->get("isc", 1); + ASSERT_TRUE(desc.option_); + 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, "2001:2::/64", + SubnetID(2), 100, shared_network2)); + + // subnet3 should be replaced by subnet3b and no longer assigned to a network. + ASSERT_NO_FATAL_FAILURE(checkMergedSubnet(cfg_to, "2001:3::/64", + SubnetID(3), 500, no_network)); + // Let's verify that our option is there and populated correctly. + subnet = cfg_to.getByPrefix("2001:3::/64"); + desc = subnet->getCfgOption()->get("isc", 1); + ASSERT_TRUE(desc.option_); + 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, "2001:4::/64", + SubnetID(4), 500, shared_network1)); + + // subnet5 should have been added to configuration. + ASSERT_NO_FATAL_FAILURE(checkMergedSubnet(cfg_to, "2001:5::/64", + SubnetID(5), 300, shared_network2)); + + // Let's verify that both pools have the proper options. + subnet = cfg_to.getByPrefix("2001:5::/64"); + const PoolPtr merged_pool = subnet->getPool(Lease::TYPE_NA, IOAddress("2001:5::10")); + ASSERT_TRUE(merged_pool); + desc = merged_pool->getCfgOption()->get("isc", 1); + opstr = boost::dynamic_pointer_cast(desc.option_); + ASSERT_TRUE(opstr); + EXPECT_EQ("POOLS", opstr->getValue()); + + const PoolPtr merged_pool2 = subnet->getPool(Lease::TYPE_PD, IOAddress("2001:1::")); + ASSERT_TRUE(merged_pool2); + desc = merged_pool2->getCfgOption()->get("isc", 1); + opstr = boost::dynamic_pointer_cast(desc.option_); + ASSERT_TRUE(opstr); + EXPECT_EQ("RULE!", opstr->getValue()); +} + } // end of anonymous namespace -- GitLab From 73c147b8cef562ac3878770755c7fa1d4ebb3bcd Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Mon, 1 Apr 2019 08:44:48 -0400 Subject: [PATCH 7/9] [#413,!288] Added ChangeLog entry --- ChangeLog | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ChangeLog b/ChangeLog index 690ef6625c..bc8f589a1b 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,8 @@ +1554. [func] tmark + kea-dhcp6 now uses globals, option definitions, options, + share-networks, and subnets from configuration back ends. + (Gitlab #413,!288, git ff367e273ed8763b354db272c5955a78203d865e) + 1553. [func] marcin DHCPv4 server automatically fetches incremental configuration updates from the configuration backends. -- GitLab From cf958bf3295feefbb567039d2829cb0428ee2b5b Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Wed, 10 Apr 2019 09:31:12 -0400 Subject: [PATCH 8/9] [#413,!288] Addressed review comments. --- src/lib/dhcpsrv/srv_config.h | 42 ++----------------- .../tests/cfg_shared_networks4_unittest.cc | 6 ++- .../tests/cfg_shared_networks6_unittest.cc | 6 ++- src/lib/dhcpsrv/tests/srv_config_unittest.cc | 1 - 4 files changed, 12 insertions(+), 43 deletions(-) diff --git a/src/lib/dhcpsrv/srv_config.h b/src/lib/dhcpsrv/srv_config.h index 71636c8270..61c1aec044 100644 --- a/src/lib/dhcpsrv/srv_config.h +++ b/src/lib/dhcpsrv/srv_config.h @@ -628,25 +628,8 @@ private: /// @brief Merges the DHCPv4 configuration specified as a parameter into /// this configuration. /// - /// The general rule is that the configuration data from the @c other - /// object replaces configuration data held in this object instance. - /// The data that do not overlap between the two objects is simply - /// inserted into this configuration. - /// - /// @warning The call to @c merge may modify the data in the @c other - /// object. 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. - /// - /// The @c other parameter must be a @c SrvConfig or its derivation. - /// - /// Currently, the following parts of the v4 configuration are merged: - /// - globals - /// - shared-networks - /// - subnets - /// - /// @todo Add support for merging other configuration elements. + /// This is called by @c merge() to handle v4 specifics, such as + /// networks and subnets. /// /// @param other An object holding the configuration to be merged /// into this configuration. @@ -655,25 +638,8 @@ private: /// @brief Merges the DHCPv6 configuration specified as a parameter into /// this configuration. /// - /// The general rule is that the configuration data from the @c other - /// object replaces configuration data held in this object instance. - /// The data that do not overlap between the two objects is simply - /// inserted into this configuration. - /// - /// @warning The call to @c merge may modify the data in the @c other - /// object. 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. - /// - /// The @c other parameter must be a @c SrvConfig or its derivation. - /// - /// Currently, the following parts of the v6 configuration are merged: - /// - globals - /// - shared-networks - /// - subnets - /// - /// @todo Add support for merging other configuration elements. + /// This is called by @c merge() to handle v4 specifics, such as + /// networks and subnets. /// /// @param other An object holding the configuration to be merged /// into this configuration. diff --git a/src/lib/dhcpsrv/tests/cfg_shared_networks4_unittest.cc b/src/lib/dhcpsrv/tests/cfg_shared_networks4_unittest.cc index 8e9ea86a82..cd4e7ca468 100644 --- a/src/lib/dhcpsrv/tests/cfg_shared_networks4_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_shared_networks4_unittest.cc @@ -18,9 +18,11 @@ using namespace asiolink; namespace { -/// Attempts to verify an expected network within a collection of networks +/// @brief Attempts to verify an expected network within a collection +/// of networks +/// /// @param networks set of networks in which to look -/// @param name name of the expected network +/// @param name name of the expected network /// @param exp_valid expected valid lifetime of the network /// @param exp_subnets list of subnet IDs the network is expected to own void checkMergedNetwork(const CfgSharedNetworks4& networks, const std::string& name, diff --git a/src/lib/dhcpsrv/tests/cfg_shared_networks6_unittest.cc b/src/lib/dhcpsrv/tests/cfg_shared_networks6_unittest.cc index 5add7d9859..44f4b72c26 100644 --- a/src/lib/dhcpsrv/tests/cfg_shared_networks6_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_shared_networks6_unittest.cc @@ -18,9 +18,11 @@ using namespace asiolink; namespace { -/// Attempts to verify an expected network within a collection of networks +/// @brief Attempts to verify an expected network within a collection +/// of networks +/// /// @param networks set of networks in which to look -/// @param name name of the expected network +/// @param name name of the expected network /// @param exp_valid expected valid lifetime of the network /// @param exp_subnets list of subnet IDs the network is expected to own void checkMergedNetwork(const CfgSharedNetworks6& networks, const std::string& name, diff --git a/src/lib/dhcpsrv/tests/srv_config_unittest.cc b/src/lib/dhcpsrv/tests/srv_config_unittest.cc index b1a0a09f28..0e81acfa9b 100644 --- a/src/lib/dhcpsrv/tests/srv_config_unittest.cc +++ b/src/lib/dhcpsrv/tests/srv_config_unittest.cc @@ -1056,7 +1056,6 @@ TEST_F(SrvConfigTest, mergeGlobals6) { // Set some explicit values. cfg_to.setDeclinePeriod(100); - cfg_to.setEchoClientId(false); cfg_to.setDhcp4o6Port(777); cfg_to.setServerTag("not_this_server"); -- GitLab From 96eecd565ad684c9dbb091716a244320194bb10c Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Wed, 10 Apr 2019 10:30:14 -0400 Subject: [PATCH 9/9] [#413,!288] More review comments. --- src/lib/dhcpsrv/srv_config.h | 8 ++++++-- src/lib/dhcpsrv/tests/srv_config_unittest.cc | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/lib/dhcpsrv/srv_config.h b/src/lib/dhcpsrv/srv_config.h index 61c1aec044..3d957ea723 100644 --- a/src/lib/dhcpsrv/srv_config.h +++ b/src/lib/dhcpsrv/srv_config.h @@ -507,10 +507,14 @@ public: /// The @c other parameter must be a @c SrvConfig or its derivation. /// /// This method calls either @c merge4 or @c merge6 based on - /// @c CfgMgr::family_. /// /// Currently, the following parts of the configuration are merged: - /// - IPv4 subnets + /// - globals + /// - option definitions + /// - options + /// - via @c merge4 or @c merge6 depending on @c CfgMgr::family_: + /// - shared networks + /// - subnets /// /// @todo Add support for merging other configuration elements. /// diff --git a/src/lib/dhcpsrv/tests/srv_config_unittest.cc b/src/lib/dhcpsrv/tests/srv_config_unittest.cc index 0e81acfa9b..cdafcd5a41 100644 --- a/src/lib/dhcpsrv/tests/srv_config_unittest.cc +++ b/src/lib/dhcpsrv/tests/srv_config_unittest.cc @@ -1046,7 +1046,7 @@ TEST_F(SrvConfigTest, mergeGlobals4) { // This test verifies that globals from one SrvConfig // can be merged into another. It verifies that values // in the from-config override those in to-config which -// override those in GLOBAL4_DEFAULTS. +// override those in GLOBAL6_DEFAULTS. TEST_F(SrvConfigTest, mergeGlobals6) { // Set the family we're working with. CfgMgr::instance().setFamily(AF_INET6); -- GitLab