From: Marcin Siodelski Date: Tue, 9 Apr 2019 20:49:38 +0000 (+0200) Subject: [#103,!289] Addressed review comments. X-Git-Tag: Kea-1.6.0-beta~272 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=65f730194a3b3a036eae9e2bcf6cb4e87ca5806d;p=thirdparty%2Fkea.git [#103,!289] Addressed review comments. --- diff --git a/src/lib/dhcp/option_space_container.h b/src/lib/dhcp/option_space_container.h index f480db0585..5700ccdc84 100644 --- a/src/lib/dhcp/option_space_container.h +++ b/src/lib/dhcp/option_space_container.h @@ -99,8 +99,10 @@ public: /// or option definitions having the same id (typically id of 0). /// When configuration backend is in use it sets the unique ids /// from the database. In cases when the configuration backend is - /// not used, the ids default to 0. - + /// not used, the ids default to 0. Passing the id of 0 would + /// result in deleting all options or option definitions that were + /// not added via the database. + /// /// @param id Identifier of the items to be deleted. /// /// @return Number of deleted options or option definitions. diff --git a/src/lib/dhcpsrv/cb_ctl_dhcp4.cc b/src/lib/dhcpsrv/cb_ctl_dhcp4.cc index 20385bb7f6..dccdc22cfd 100644 --- a/src/lib/dhcpsrv/cb_ctl_dhcp4.cc +++ b/src/lib/dhcpsrv/cb_ctl_dhcp4.cc @@ -85,7 +85,21 @@ CBControlDHCPv4::databaseConfigApply(const BackendSelector& backend_selector, range = index.equal_range(boost::make_tuple("dhcp4_subnet", AuditEntry::ModificationType::DELETE)); for (auto entry = range.first; entry != range.second; ++entry) { - cfg->getCfgSubnets4()->del((*entry)->getObjectId()); + // If the deleted subnet belongs to a shared network and the + // shared network is not being removed, we need to detach the + // subnet from the shared network. + auto subnet = cfg->getCfgSubnets4()->getBySubnetId((*entry)->getObjectId()); + if (subnet) { + // Check if the subnet belongs to a shared network. + SharedNetwork4Ptr network; + subnet->getSharedNetwork(network); + if (network) { + // Detach the subnet from the shared network. + network->del(subnet->getID()); + } + // Actually delete the subnet from the configuration. + cfg->getCfgSubnets4()->del((*entry)->getObjectId()); + } } } catch (...) { diff --git a/src/lib/dhcpsrv/cfg_option.h b/src/lib/dhcpsrv/cfg_option.h index 9de6ae513b..22c57227dc 100644 --- a/src/lib/dhcpsrv/cfg_option.h +++ b/src/lib/dhcpsrv/cfg_option.h @@ -541,6 +541,8 @@ public: /// having the same id (typically id of 0). When configuration backend /// is in use it sets the unique ids from the database. In cases when /// the configuration backend is not used, the ids default to 0. + /// Passing the id of 0 would result in deleting all options that were + /// not added via the database. /// /// Both regular and vendor specific options are deleted with this /// method. diff --git a/src/lib/dhcpsrv/cfg_option_def.h b/src/lib/dhcpsrv/cfg_option_def.h index 80f95e70e9..3c58b77aaf 100644 --- a/src/lib/dhcpsrv/cfg_option_def.h +++ b/src/lib/dhcpsrv/cfg_option_def.h @@ -119,7 +119,9 @@ public: /// definitions having the same id (typically id of 0). When /// configuration backend is in use it sets the unique ids from the /// database. In cases when the configuration backend is not used, - /// the ids default to 0. + /// the ids default to 0. Passing the id of 0 would result in + /// deleting all option definitions that were not added via the + /// database. /// /// @param id Identifier of the option definitions to be deleted. /// diff --git a/src/lib/dhcpsrv/cfg_shared_networks.h b/src/lib/dhcpsrv/cfg_shared_networks.h index 7d7bb99306..f97b4dbd69 100644 --- a/src/lib/dhcpsrv/cfg_shared_networks.h +++ b/src/lib/dhcpsrv/cfg_shared_networks.h @@ -75,7 +75,8 @@ public: /// networks having the same id (typically id of 0). When configuration /// backend is in use it sets the unique ids from the database. /// In cases when the configuration backend is not used, the ids - /// default to 0. + /// default to 0. Passing the id of 0 would result in deleting all + /// shared networks that were not added via the database. /// /// @param id Identifier of the shared networks to be deleted. /// diff --git a/src/lib/dhcpsrv/tests/cb_ctl_dhcp_unittest.cc b/src/lib/dhcpsrv/tests/cb_ctl_dhcp_unittest.cc index 1fe5dad512..7dad1b7f35 100644 --- a/src/lib/dhcpsrv/tests/cb_ctl_dhcp_unittest.cc +++ b/src/lib/dhcpsrv/tests/cb_ctl_dhcp_unittest.cc @@ -285,6 +285,7 @@ public: // Insert subnets into the database. Subnet4Ptr subnet(new Subnet4(IOAddress("192.0.3.0"), 26, 1, 2, 3, SubnetID(1))); subnet->setModificationTime(getTimestamp("dhcp4_subnet")); + subnet->setSharedNetworkName("one"); mgr.getPool()->createUpdateSubnet4(BackendSelector::UNSPEC(), ServerSelector::ALL(), subnet); @@ -577,6 +578,14 @@ public: if (deleteConfigElement("dhcp4_subnet", 1)) { EXPECT_FALSE(found_subnet); + // If the subnet has been deleted, make sure that + // it was detached from the shared network it belonged + // to, if the shared network still exists. + auto found_network = srv_cfg->getCfgSharedNetworks4()->getByName("one"); + if (found_network) { + EXPECT_TRUE(found_network->getAllSubnets()->empty()); + } + } else { EXPECT_TRUE(found_subnet); } @@ -730,6 +739,7 @@ TEST_F(CBControlDHCPv4Test, databaseConfigApplySharedNetworkNotFetched) { // This test verifies that only a subnet is merged into the current // configuration. TEST_F(CBControlDHCPv4Test, databaseConfigApplySubnet) { + addCreateAuditEntry("dhcp4_shared_network"); addCreateAuditEntry("dhcp4_subnet"); testDatabaseConfigApply(getTimestamp(-5)); } diff --git a/src/lib/dhcpsrv/tests/cfg_option_def_unittest.cc b/src/lib/dhcpsrv/tests/cfg_option_def_unittest.cc index 5c9a9075c9..48f945aa89 100644 --- a/src/lib/dhcpsrv/tests/cfg_option_def_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_option_def_unittest.cc @@ -61,6 +61,8 @@ TEST(CfgOptionDefTest, getAllThenDelete) { option_name << "option-" << code; OptionDefinitionPtr def(new OptionDefinition(option_name.str(), code, "uint16")); + // We're setting id of 123 for all option definitions in this + // code range. def->setId(123); // Add option definition to "isc" option space. @@ -76,7 +78,8 @@ TEST(CfgOptionDefTest, getAllThenDelete) { option_name << "option-" << code; OptionDefinitionPtr def(new OptionDefinition(option_name.str(), code, "uint16")); - + // We're setting id of 234 for all option definitions in this + // code range. def->setId(234); ASSERT_NO_THROW(cfg.add(def, "abcde"));