From 72bc8cc9645f7280440c0a45d786a26e30c1f797 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Tue, 25 Jun 2019 18:50:18 +0200 Subject: [PATCH] [#642,!373] Addressed review comments. - Don't allow for deleting logical server 'all'. - Additional tests to make sure that other servers aren't affected by deletion. - Added note that getAll() doesn't return logical server all. --- src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc | 2 +- src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.h | 5 +++++ src/hooks/dhcp/mysql_cb/mysql_cb_dhcp6.cc | 2 +- src/hooks/dhcp/mysql_cb/mysql_cb_dhcp6.h | 5 +++++ src/hooks/dhcp/mysql_cb/mysql_cb_impl.cc | 11 +++++++++-- src/hooks/dhcp/mysql_cb/mysql_cb_impl.h | 4 +++- .../dhcp/mysql_cb/tests/mysql_cb_dhcp4_unittest.cc | 6 ++++++ .../dhcp/mysql_cb/tests/mysql_cb_dhcp6_unittest.cc | 6 ++++++ src/lib/dhcpsrv/config_backend_dhcp4.h | 3 +++ src/lib/dhcpsrv/config_backend_dhcp6.h | 3 +++ src/lib/dhcpsrv/config_backend_pool_dhcp4.h | 3 +++ src/lib/dhcpsrv/config_backend_pool_dhcp6.h | 3 +++ 12 files changed, 48 insertions(+), 5 deletions(-) diff --git a/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc b/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc index b318ff6495..37ba0d069b 100644 --- a/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc +++ b/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc @@ -2846,7 +2846,7 @@ MySqlConfigBackendDHCPv4::deleteServer4(const ServerTag& server_tag) { .arg(server_tag.get()); uint64_t result = impl_->deleteServer(MySqlConfigBackendDHCPv4Impl::CREATE_AUDIT_REVISION, MySqlConfigBackendDHCPv4Impl::DELETE_SERVER4, - server_tag.get()); + server_tag); LOG_DEBUG(mysql_cb_logger, DBGLVL_TRACE_BASIC, MYSQL_CB_DELETE_SERVER4_RESULT) .arg(result); return (result); diff --git a/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.h b/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.h index 1262cc24fe..afda51a52b 100644 --- a/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.h +++ b/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.h @@ -227,6 +227,9 @@ public: /// @brief Retrieves all servers. /// + /// This method returns the list of servers excluding the logical server + /// 'all'. + /// /// @return Collection of servers from the backend. virtual db::ServerCollection getAllServers4() const; @@ -480,6 +483,8 @@ public: /// /// @param server_tag Tag of the server to be deleted. /// @return Number of deleted servers. + /// @throw isc::InvalidOperation when trying to delete the logical + /// server 'all'. virtual uint64_t deleteServer4(const data::ServerTag& server_tag); diff --git a/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp6.cc b/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp6.cc index efd7ed18cc..67ffe7006f 100644 --- a/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp6.cc +++ b/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp6.cc @@ -3233,7 +3233,7 @@ MySqlConfigBackendDHCPv6::deleteServer6(const ServerTag& server_tag) { .arg(server_tag.get()); uint64_t result = impl_->deleteServer(MySqlConfigBackendDHCPv6Impl::CREATE_AUDIT_REVISION, MySqlConfigBackendDHCPv6Impl::DELETE_SERVER6, - server_tag.get()); + server_tag); LOG_DEBUG(mysql_cb_logger, DBGLVL_TRACE_BASIC, MYSQL_CB_DELETE_SERVER6_RESULT) .arg(result); return (result); diff --git a/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp6.h b/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp6.h index 4b246f549d..1e8983f6e3 100644 --- a/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp6.h +++ b/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp6.h @@ -227,6 +227,9 @@ public: /// @brief Retrieves all servers. /// + /// This method returns the list of servers excluding the logical server + /// 'all'. + /// /// @return Collection of servers from the backend. virtual db::ServerCollection getAllServers6() const; @@ -513,6 +516,8 @@ public: /// /// @param server_tag Tag of the server to be deleted. /// @return Number of deleted servers. + /// @throw isc::InvalidOperation when trying to delete the logical + /// server 'all'. virtual uint64_t deleteServer6(const data::ServerTag& server_tag); diff --git a/src/hooks/dhcp/mysql_cb/mysql_cb_impl.cc b/src/hooks/dhcp/mysql_cb/mysql_cb_impl.cc index b5a9e5d863..30301fc0df 100644 --- a/src/hooks/dhcp/mysql_cb/mysql_cb_impl.cc +++ b/src/hooks/dhcp/mysql_cb/mysql_cb_impl.cc @@ -964,7 +964,14 @@ MySqlConfigBackendImpl::createUpdateServer(const int& create_audit_revision, uint64_t MySqlConfigBackendImpl::deleteServer(const int& create_audit_revision, const int& delete_index, - const std::string& server_tag) { + const ServerTag& server_tag) { + + // It is not allowed to delete 'all' logical server. + if (server_tag.amAll()) { + isc_throw(InvalidOperation, "'all' is a name reserved for the server tag which" + " associates the configuration elements with all servers connecting" + " to the database and can't be deleted"); + } MySqlTransaction transaction(conn_); @@ -976,7 +983,7 @@ MySqlConfigBackendImpl::deleteServer(const int& create_audit_revision, // Specify which server should be deleted. MySqlBindingCollection in_bindings = { - MySqlBinding::createString(server_tag) + MySqlBinding::createString(server_tag.get()) }; // Attempt to delete the server. diff --git a/src/hooks/dhcp/mysql_cb/mysql_cb_impl.h b/src/hooks/dhcp/mysql_cb/mysql_cb_impl.h index 4bbf2cb383..f910af865b 100644 --- a/src/hooks/dhcp/mysql_cb/mysql_cb_impl.h +++ b/src/hooks/dhcp/mysql_cb/mysql_cb_impl.h @@ -655,8 +655,10 @@ public: /// @param create_index Index of the DELETE query to be executed. /// @param server_tag Tag of the server to be deleted. /// @return Number of deleted servers. + /// @throw isc::InvalidOperation when trying to delete the logical + /// server 'all'. uint64_t deleteServer(const int& create_audit_revision, const int& index, - const std::string& server_tag); + const data::ServerTag& server_tag); /// @brief Attempts to delete all servers. /// diff --git a/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp4_unittest.cc b/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp4_unittest.cc index a4ce59f41b..5c2d874664 100644 --- a/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp4_unittest.cc +++ b/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp4_unittest.cc @@ -506,6 +506,12 @@ TEST_F(MySqlConfigBackendDHCPv4Test, createUpdateDeleteServer) { EXPECT_NO_THROW(servers_deleted = cbptr_->deleteServer4(ServerTag("server2"))); EXPECT_EQ(0, servers_deleted); + // Make sure that the server1 wasn't deleted. + EXPECT_NO_THROW(returned_server = cbptr_->getServer4(ServerTag("server1"))); + EXPECT_TRUE(returned_server); + + // Deleting logical server 'all' is not allowed. + EXPECT_THROW(cbptr_->deleteServer4(ServerTag()), isc::InvalidOperation); // Delete the existing server. EXPECT_NO_THROW(servers_deleted = cbptr_->deleteServer4(ServerTag("server1"))); diff --git a/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp6_unittest.cc b/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp6_unittest.cc index 2003190e80..f6636112c2 100644 --- a/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp6_unittest.cc +++ b/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp6_unittest.cc @@ -547,6 +547,12 @@ TEST_F(MySqlConfigBackendDHCPv6Test, createUpdateDeleteServer) { EXPECT_NO_THROW(servers_deleted = cbptr_->deleteServer6(ServerTag("server2"))); EXPECT_EQ(0, servers_deleted); + // Make sure that the server1 wasn't deleted. + EXPECT_NO_THROW(returned_server = cbptr_->getServer6(ServerTag("server1"))); + EXPECT_TRUE(returned_server); + + // Deleting logical server 'all' is not allowed. + EXPECT_THROW(cbptr_->deleteServer6(ServerTag()), isc::InvalidOperation); // Delete the existing server. EXPECT_NO_THROW(servers_deleted = cbptr_->deleteServer6(ServerTag("server1"))); diff --git a/src/lib/dhcpsrv/config_backend_dhcp4.h b/src/lib/dhcpsrv/config_backend_dhcp4.h index dcf96c6dd3..c155398868 100644 --- a/src/lib/dhcpsrv/config_backend_dhcp4.h +++ b/src/lib/dhcpsrv/config_backend_dhcp4.h @@ -231,6 +231,9 @@ public: /// @brief Retrieves all servers. /// + /// This method returns the list of servers excluding the logical server + /// 'all'. + /// /// @return Collection of servers from the backend. virtual db::ServerCollection getAllServers4() const = 0; diff --git a/src/lib/dhcpsrv/config_backend_dhcp6.h b/src/lib/dhcpsrv/config_backend_dhcp6.h index 72f1c63041..3a0e3c411a 100644 --- a/src/lib/dhcpsrv/config_backend_dhcp6.h +++ b/src/lib/dhcpsrv/config_backend_dhcp6.h @@ -232,6 +232,9 @@ public: /// @brief Retrieves all servers. /// + /// This method returns the list of servers excluding the logical server + /// 'all'. + /// /// @return Collection of servers from the backend. virtual db::ServerCollection getAllServers6() const = 0; diff --git a/src/lib/dhcpsrv/config_backend_pool_dhcp4.h b/src/lib/dhcpsrv/config_backend_pool_dhcp4.h index 973cee42b9..56497be43d 100644 --- a/src/lib/dhcpsrv/config_backend_pool_dhcp4.h +++ b/src/lib/dhcpsrv/config_backend_pool_dhcp4.h @@ -235,6 +235,9 @@ public: /// @brief Retrieves all servers from the particular backend. /// + /// This method returns the list of servers excluding the logical server + /// 'all'. + /// /// @param backend_selector Backend selector. /// @return Collection of servers from the backend. virtual db::ServerCollection diff --git a/src/lib/dhcpsrv/config_backend_pool_dhcp6.h b/src/lib/dhcpsrv/config_backend_pool_dhcp6.h index 3cf5b6bb40..c79bb44abd 100644 --- a/src/lib/dhcpsrv/config_backend_pool_dhcp6.h +++ b/src/lib/dhcpsrv/config_backend_pool_dhcp6.h @@ -234,6 +234,9 @@ public: /// @brief Retrieves all servers from the particular backend. /// + /// This method returns the list of servers excluding the logical server + /// 'all'. + /// /// @param backend_selector Backend selector. /// @return Collection of servers from the backend. virtual db::ServerCollection -- GitLab