From 4dc0a2a047a3a06499c320e3d908b5239e9190e5 Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Fri, 6 Sep 2024 20:56:53 +0200 Subject: [PATCH 01/13] Refactoring of the code for making a backup and restoring access entities. --- src/Access/AccessBackup.cpp | 466 +++++++++++------- src/Access/AccessBackup.h | 74 ++- src/Access/AccessControl.cpp | 4 +- src/Access/AccessControl.h | 2 +- src/Access/GrantedRoles.cpp | 10 + src/Access/GrantedRoles.h | 1 + src/Access/IAccessEntity.cpp | 24 - src/Access/IAccessEntity.h | 6 +- src/Access/IAccessStorage.cpp | 79 ++- src/Access/IAccessStorage.h | 3 +- src/Access/MultipleAccessStorage.cpp | 4 +- src/Access/MultipleAccessStorage.h | 2 +- src/Access/Quota.cpp | 7 +- src/Access/Quota.h | 4 +- src/Access/ReplicatedAccessStorage.cpp | 44 -- src/Access/ReplicatedAccessStorage.h | 4 +- src/Access/Role.cpp | 7 +- src/Access/Role.h | 4 +- src/Access/RolesOrUsersSet.cpp | 17 + src/Access/RolesOrUsersSet.h | 1 + src/Access/RowPolicy.cpp | 7 +- src/Access/RowPolicy.h | 4 +- src/Access/SettingsProfile.cpp | 7 +- src/Access/SettingsProfile.h | 4 +- src/Access/SettingsProfileElement.cpp | 12 + src/Access/SettingsProfileElement.h | 1 + src/Access/User.cpp | 7 +- src/Access/User.h | 4 +- src/Backups/BackupCoordinationStage.h | 3 + src/Backups/RestoreSettings.cpp | 1 + src/Backups/RestorerFromBackup.cpp | 64 +-- src/Backups/RestorerFromBackup.h | 3 +- src/Storages/System/StorageSystemQuotas.cpp | 4 +- src/Storages/System/StorageSystemRoles.cpp | 4 +- .../System/StorageSystemRowPolicies.cpp | 4 +- .../System/StorageSystemSettingsProfiles.cpp | 4 +- src/Storages/System/StorageSystemUsers.cpp | 4 +- 37 files changed, 535 insertions(+), 365 deletions(-) diff --git a/src/Access/AccessBackup.cpp b/src/Access/AccessBackup.cpp index e8ea21852b5..c2c37adffa8 100644 --- a/src/Access/AccessBackup.cpp +++ b/src/Access/AccessBackup.cpp @@ -30,6 +30,7 @@ namespace ErrorCodes { extern const int CANNOT_RESTORE_TABLE; extern const int ACCESS_ENTITY_ALREADY_EXISTS; + extern const int ACCESS_ENTITY_NOT_FOUND; extern const int LOGICAL_ERROR; } @@ -175,147 +176,6 @@ namespace } return res; } - - /// Checks if new entities (which we're going to restore) already exist, - /// and either skips them or throws an exception depending on the restore settings. - void checkExistingEntities(std::vector> & entities, - std::unordered_map & old_to_new_id, - const AccessControl & access_control, - RestoreAccessCreationMode creation_mode) - { - if (creation_mode == RestoreAccessCreationMode::kReplace) - return; - - auto should_skip = [&](const std::pair & id_and_entity) - { - const auto & id = id_and_entity.first; - const auto & entity = *id_and_entity.second; - auto existing_id = access_control.find(entity.getType(), entity.getName()); - if (!existing_id) - { - return false; - } - else if (creation_mode == RestoreAccessCreationMode::kCreateIfNotExists) - { - old_to_new_id[id] = *existing_id; - return true; - } - else - { - throw Exception(ErrorCodes::ACCESS_ENTITY_ALREADY_EXISTS, "Cannot restore {} because it already exists", entity.formatTypeWithName()); - } - }; - - std::erase_if(entities, should_skip); - } - - /// If new entities (which we're going to restore) depend on other entities which are not going to be restored or not present in the backup - /// then we should try to replace those dependencies with already existing entities. - void resolveDependencies(const std::unordered_map> & dependencies, - std::unordered_map & old_to_new_ids, - const AccessControl & access_control, - bool allow_unresolved_dependencies) - { - for (const auto & [id, name_and_type] : dependencies) - { - std::optional new_id; - if (allow_unresolved_dependencies) - new_id = access_control.find(name_and_type.second, name_and_type.first); - else - new_id = access_control.getID(name_and_type.second, name_and_type.first); - if (new_id) - old_to_new_ids.emplace(id, *new_id); - } - } - - /// Generates random IDs for the new entities. - void generateRandomIDs(std::vector> & entities, std::unordered_map & old_to_new_ids) - { - Poco::UUIDGenerator generator; - for (auto & [id, entity] : entities) - { - UUID new_id; - generator.createRandom().copyTo(reinterpret_cast(&new_id)); - old_to_new_ids.emplace(id, new_id); - id = new_id; - } - } - - /// Updates dependencies of the new entities using a specified map. - void replaceDependencies(std::vector> & entities, - const std::unordered_map & old_to_new_ids) - { - for (auto & entity : entities | boost::adaptors::map_values) - IAccessEntity::replaceDependencies(entity, old_to_new_ids); - } - - AccessRightsElements getRequiredAccessToRestore(const std::vector> & entities) - { - AccessRightsElements res; - for (const auto & entity : entities | boost::adaptors::map_values) - { - auto entity_type = entity->getType(); - switch (entity_type) - { - case User::TYPE: - { - const auto & user = typeid_cast(*entity); - res.emplace_back(AccessType::CREATE_USER); - auto elements = user.access.getElements(); - for (auto & element : elements) - { - if (element.is_partial_revoke) - continue; - element.grant_option = true; - res.emplace_back(element); - } - if (!user.granted_roles.isEmpty()) - res.emplace_back(AccessType::ROLE_ADMIN); - break; - } - - case Role::TYPE: - { - const auto & role = typeid_cast(*entity); - res.emplace_back(AccessType::CREATE_ROLE); - auto elements = role.access.getElements(); - for (auto & element : elements) - { - if (element.is_partial_revoke) - continue; - element.grant_option = true; - res.emplace_back(element); - } - if (!role.granted_roles.isEmpty()) - res.emplace_back(AccessType::ROLE_ADMIN); - break; - } - - case SettingsProfile::TYPE: - { - res.emplace_back(AccessType::CREATE_SETTINGS_PROFILE); - break; - } - - case RowPolicy::TYPE: - { - const auto & policy = typeid_cast(*entity); - res.emplace_back(AccessType::CREATE_ROW_POLICY, policy.getDatabase(), policy.getTableName()); - break; - } - - case Quota::TYPE: - { - res.emplace_back(AccessType::CREATE_QUOTA); - break; - } - - default: - throw Exception(ErrorCodes::LOGICAL_ERROR, "Unknown type: {}", toString(entity_type)); - } - } - return res; - } } @@ -339,61 +199,309 @@ AccessRestorerFromBackup::AccessRestorerFromBackup( const BackupPtr & backup_, const RestoreSettings & restore_settings_) : backup(backup_) , creation_mode(restore_settings_.create_access) - , allow_unresolved_dependencies(restore_settings_.allow_unresolved_access_dependencies) + , skip_unresolved_dependencies(restore_settings_.allow_unresolved_access_dependencies) { } AccessRestorerFromBackup::~AccessRestorerFromBackup() = default; -void AccessRestorerFromBackup::addDataPath(const String & data_path) + +void AccessRestorerFromBackup::addDataPath(const String & data_path_in_backup) { - if (!data_paths.emplace(data_path).second) - return; + if (loaded) + throw Exception(ErrorCodes::LOGICAL_ERROR, "Access entities already loaded"); - fs::path data_path_in_backup_fs = data_path; - Strings filenames = backup->listFiles(data_path, /*recursive*/ false); - if (filenames.empty()) - return; - - for (const String & filename : filenames) - { - if (!filename.starts_with("access") || !filename.ends_with(".txt")) - throw Exception(ErrorCodes::CANNOT_RESTORE_TABLE, "File name {} doesn't match the wildcard \"access*.txt\"", - String{data_path_in_backup_fs / filename}); - } - - ::sort(filenames.begin(), filenames.end()); - - for (const String & filename : filenames) - { - String filepath_in_backup = data_path_in_backup_fs / filename; - auto read_buffer_from_backup = backup->readFile(filepath_in_backup); - auto ab = AccessEntitiesInBackup::fromBackupEntry(std::move(read_buffer_from_backup), filepath_in_backup); - - boost::range::copy(ab.entities, std::back_inserter(entities)); - boost::range::copy(ab.dependencies, std::inserter(dependencies, dependencies.end())); - } - - for (const auto & id : entities | boost::adaptors::map_keys) - dependencies.erase(id); + if (std::find(data_paths_in_backup.begin(), data_paths_in_backup.end(), data_path_in_backup) == data_paths_in_backup.end()) + data_paths_in_backup.emplace_back(data_path_in_backup); } + +void AccessRestorerFromBackup::loadFromBackup() +{ + if (loaded) + return; + + /// Parse files "access*.txt" found in the added data paths in the backup. + for (size_t data_path_index = 0; data_path_index != data_paths_in_backup.size(); ++data_path_index) + { + const String & data_path_in_backup = data_paths_in_backup[data_path_index]; + + fs::path data_path_in_backup_fs = data_path_in_backup; + Strings filenames = backup->listFiles(data_path_in_backup_fs, /*recursive*/ false); + if (filenames.empty()) + continue; + + for (const String & filename : filenames) + { + if (!filename.starts_with("access") || !filename.ends_with(".txt")) + throw Exception(ErrorCodes::CANNOT_RESTORE_TABLE, "File name {} doesn't match the wildcard \"access*.txt\"", + String{data_path_in_backup_fs / filename}); + } + + for (const String & filename : filenames) + { + String filepath_in_backup = data_path_in_backup_fs / filename; + AccessEntitiesInBackup ab; + + try + { + auto read_buffer_from_backup = backup->readFile(filepath_in_backup); + ab = AccessEntitiesInBackup::fromBackupEntry(std::move(read_buffer_from_backup), filepath_in_backup); + } + catch (Exception & e) + { + e.addMessage("While reading access entities from {} in backup", filepath_in_backup); + throw; + } + + for (const auto & [id, entity] : ab.entities) + { + auto it = entity_infos.find(id); + if (it == entity_infos.end()) + { + it = entity_infos.emplace(id, EntityInfo{.id = id, .name = entity->getName(), .type = entity->getType()}).first; + } + EntityInfo & entity_info = it->second; + entity_info.entity = entity; + entity_info.restore = true; + entity_info.data_path_index = data_path_index; + } + + for (const auto & [id, name_and_type] : ab.dependencies) + { + auto it = entity_infos.find(id); + if (it == entity_infos.end()) + { + it = entity_infos.emplace(id, EntityInfo{.id = id, .name = name_and_type.first, .type = name_and_type.second}).first; + } + EntityInfo & entity_info = it->second; + entity_info.is_dependency = true; + } + } + } + + loaded = true; +} + + AccessRightsElements AccessRestorerFromBackup::getRequiredAccess() const { - return getRequiredAccessToRestore(entities); + if (!loaded) + throw Exception(ErrorCodes::LOGICAL_ERROR, "Access entities not loaded"); + + AccessRightsElements res; + for (const auto & [id, entity_info] : entity_infos) + { + if (!entity_info.restore) + continue; + const auto & entity = entity_info.entity; + auto entity_type = entity->getType(); + switch (entity_type) + { + case User::TYPE: + { + const auto & user = typeid_cast(*entity); + res.emplace_back(AccessType::CREATE_USER); + auto elements = user.access.getElements(); + for (auto & element : elements) + { + if (element.is_partial_revoke) + continue; + element.grant_option = true; + res.emplace_back(element); + } + if (!user.granted_roles.isEmpty()) + res.emplace_back(AccessType::ROLE_ADMIN); + break; + } + + case Role::TYPE: + { + const auto & role = typeid_cast(*entity); + res.emplace_back(AccessType::CREATE_ROLE); + auto elements = role.access.getElements(); + for (auto & element : elements) + { + if (element.is_partial_revoke) + continue; + element.grant_option = true; + res.emplace_back(element); + } + if (!role.granted_roles.isEmpty()) + res.emplace_back(AccessType::ROLE_ADMIN); + break; + } + + case SettingsProfile::TYPE: + { + res.emplace_back(AccessType::CREATE_SETTINGS_PROFILE); + break; + } + + case RowPolicy::TYPE: + { + const auto & policy = typeid_cast(*entity); + res.emplace_back(AccessType::CREATE_ROW_POLICY, policy.getDatabase(), policy.getTableName()); + break; + } + + case Quota::TYPE: + { + res.emplace_back(AccessType::CREATE_QUOTA); + break; + } + + default: + throw Exception(ErrorCodes::LOGICAL_ERROR, "Unknown type: {}", toString(entity_type)); + } + } + return res; } -std::vector> AccessRestorerFromBackup::getAccessEntities(const AccessControl & access_control) const + +void AccessRestorerFromBackup::generateRandomIDsAndResolveDependencies(const AccessControl & access_control) { - auto new_entities = entities; + if (ids_assigned) + return; + if (!loaded) + throw Exception(ErrorCodes::LOGICAL_ERROR, "Access entities not loaded"); + + /// Calculate `new_id` for each entity info. + /// Check which ones of the loaded access entities already exist. + /// Generate random UUIDs for access entities which we're going to restore if they don't exist. + for (auto & [id, entity_info] : entity_infos) + { + const String & name = entity_info.name; + auto type = entity_info.type; + + if (entity_info.restore && (creation_mode == RestoreAccessCreationMode::kReplace)) + { + entity_info.new_id = UUIDHelpers::generateV4(); + continue; + } + + if (auto existing_id = access_control.find(type, name)) + { + if (entity_info.restore && (creation_mode == RestoreAccessCreationMode::kCreate)) + { + throw Exception(ErrorCodes::ACCESS_ENTITY_ALREADY_EXISTS, "Cannot restore {} because it already exists", + AccessEntityTypeInfo::get(type).formatEntityNameWithType(name)); + } + entity_info.new_id = *existing_id; + entity_info.restore = false; + } + else + { + if (entity_info.is_dependency && !entity_info.restore && !skip_unresolved_dependencies) + { + throw Exception(ErrorCodes::ACCESS_ENTITY_NOT_FOUND, "Cannot resolve {} while restoring from backup", + AccessEntityTypeInfo::get(type).formatEntityNameWithType(name)); + } + if (entity_info.restore) + entity_info.new_id = UUIDHelpers::generateV4(); + } + } + + /// Prepare map from old UUIDs to new UUIDs. std::unordered_map old_to_new_ids; - checkExistingEntities(new_entities, old_to_new_ids, access_control, creation_mode); - resolveDependencies(dependencies, old_to_new_ids, access_control, allow_unresolved_dependencies); - generateRandomIDs(new_entities, old_to_new_ids); - replaceDependencies(new_entities, old_to_new_ids); - return new_entities; + for (const auto & [id, entity_info] : entity_infos) + { + if (entity_info.new_id) + old_to_new_ids[id] = *entity_info.new_id; + } + + /// Remap the UUIDs of dependencies in the access entities we're going to restore. + for (auto & [id, entity_info] : entity_infos) + { + if (entity_info.restore) + { + auto new_entity = entity_info.entity->clone(); + new_entity->replaceDependencies(old_to_new_ids); + entity_info.entity = new_entity; + } + + if (entity_info.restore && data_path_with_entities_to_restore.empty()) + data_path_with_entities_to_restore = data_paths_in_backup[entity_info.data_path_index]; + } + + ids_assigned = true; +} + + +std::vector> AccessRestorerFromBackup::getEntities(const String & data_path_in_backup) const +{ + if (!ids_assigned) + throw Exception(ErrorCodes::LOGICAL_ERROR, "IDs not assigned"); + + if (data_path_in_backup != data_path_with_entities_to_restore) + return {}; + + std::vector> res; + res.reserve(entity_infos.size()); + + for (const auto & [id, entity_info] : entity_infos) + { + if (entity_info.restore) + res.emplace_back(*entity_info.new_id, entity_info.entity); + } + + return res; +} + + +void restoreAccessEntitiesFromBackup( + IAccessStorage & destination_access_storage, + const std::vector> & entities, + const RestoreSettings & restore_settings) +{ + if (entities.empty()) + return; /// Nothing to restore. + + bool replace_if_exists = (restore_settings.create_access == RestoreAccessCreationMode::kReplace); + bool throw_if_exists = (restore_settings.create_access == RestoreAccessCreationMode::kCreate); + + std::unordered_set restored_ids; + std::unordered_map new_to_existing_ids; + + for (const auto & [id, entity] : entities) + { + UUID existing_id; + if (destination_access_storage.insert(id, entity, replace_if_exists, throw_if_exists, &existing_id)) + { + restored_ids.emplace(id); + } + else + { + /// Couldn't insert `entity` because there is an existing entity with the same name. + new_to_existing_ids[id] = existing_id; + } + } + + if (!new_to_existing_ids.empty()) + { + std::vector ids_to_update; + ids_to_update.reserve(restored_ids.size()); + boost::copy(restored_ids, std::inserter(ids_to_update, ids_to_update.end())); + + std::unordered_set new_ids; + boost::copy(new_to_existing_ids | boost::adaptors::map_keys, std::inserter(new_ids, new_ids.end())); + + /// If new entities restored from backup have dependencies on other entities from backup which were not restored because they existed, + /// then we should correct those dependencies. + auto update_func = [&](const AccessEntityPtr & entity) -> AccessEntityPtr + { + if (!entity->hasDependencies(new_ids)) + return entity; + auto res = entity->clone(); + res->replaceDependencies(new_to_existing_ids); + return res; + }; + + /// It's totally ok if some UUIDs from `ids_to_update` don't exist anymore, that's why we use tryUpdate() here. + destination_access_storage.tryUpdate(ids_to_update, update_func); + } } } diff --git a/src/Access/AccessBackup.h b/src/Access/AccessBackup.h index 51a1112e5d5..c41184a0b7c 100644 --- a/src/Access/AccessBackup.h +++ b/src/Access/AccessBackup.h @@ -2,7 +2,6 @@ #include #include -#include namespace DB @@ -12,6 +11,7 @@ enum class AccessEntityType : uint8_t; struct IAccessEntity; using AccessEntityPtr = std::shared_ptr; class AccessRightsElements; +class IAccessStorage; class IBackup; using BackupPtr = std::shared_ptr; class IBackupEntry; @@ -27,8 +27,14 @@ std::pair makeBackupEntryForAccess( size_t counter, const AccessControl & access_control); - /// Restores access entities from a backup. +void restoreAccessEntitiesFromBackup( + IAccessStorage & access_storage, + const std::vector> & entities, + const RestoreSettings & restore_settings); + + +/// Loads access entities from a backup and prepares them for insertion into an access storage. class AccessRestorerFromBackup { public: @@ -36,21 +42,67 @@ public: ~AccessRestorerFromBackup(); /// Adds a data path to loads access entities from. - void addDataPath(const String & data_path); + void addDataPath(const String & data_path_in_backup); + + /// Loads access entities from the backup. + void loadFromBackup(); /// Checks that the current user can do restoring. + /// Function loadFromBackup() must be called before that. AccessRightsElements getRequiredAccess() const; - /// Inserts all access entities loaded from all the paths added by addDataPath(). - std::vector> getAccessEntities(const AccessControl & access_control) const; + /// Generates random IDs for access entities we're restoring to insert them into an access storage; + /// and finds IDs of existing access entities which are used as dependencies. + void generateRandomIDsAndResolveDependencies(const AccessControl & access_control); + + /// Returns access entities prepared for insertion into an access storage and new random UUIDs generated for those access entities. + /// Both functions loadFromBackup() and generateRandomIDsAndResolveDependencies() must be called before that. + std::vector> getEntities(const String & data_path_in_backup) const; private: - BackupPtr backup; - RestoreAccessCreationMode creation_mode; - bool allow_unresolved_dependencies = false; - std::vector> entities; - std::unordered_map> dependencies; - std::unordered_set data_paths; + const BackupPtr backup; + const RestoreAccessCreationMode creation_mode; + const bool skip_unresolved_dependencies; + + /// Whether loadFromBackup() finished. + bool loaded = false; + + /// Whether generateRandomIDsAndResolveDependencies() finished. + bool ids_assigned = false; + + Strings data_paths_in_backup; + String data_path_with_entities_to_restore; + + /// Information about an access entity loaded from the backup. + struct EntityInfo + { + UUID id; + String name; + AccessEntityType type; + + AccessEntityPtr entity = nullptr; /// Can be nullptr if `restore=false`. + + /// Index in `data_paths_in_backup`. + size_t data_path_index = 0; + + /// Whether we're going to restore this entity. + /// For example, + /// in case of `RESTORE TABLE system.roles` this flag is true for all the roles loaded from the backup, and + /// in case of `RESTORE ALL` this flag is always true. + bool restore = false; + + /// Whether this entity info was added as a dependency of another entity which we're going to restore. + /// For example, if we're going to restore the following user: `CREATE USER user1 DEFAULT ROLE role1, role2 SETTINGS PROFILE profile1, profile2` + /// then `restore=true` for `user1` and `is_dependency=true` for `role1`, `role2`, `profile1`, `profile2`. + /// Flags `restore` and `is_dependency` both can be set at the same time. + bool is_dependency = false; + + /// New UUID for this entity - either randomly generated or copied from an existing entity. + /// This UUID is assigned by generateRandomIDsAndResolveDependencies(). + std::optional new_id = std::nullopt; + }; + + std::unordered_map entity_infos; }; } diff --git a/src/Access/AccessControl.cpp b/src/Access/AccessControl.cpp index ec513f0692d..8e656372637 100644 --- a/src/Access/AccessControl.cpp +++ b/src/Access/AccessControl.cpp @@ -629,9 +629,9 @@ AuthResult AccessControl::authenticate(const Credentials & credentials, const Po } } -void AccessControl::restoreFromBackup(RestorerFromBackup & restorer) +void AccessControl::restoreFromBackup(RestorerFromBackup & restorer, const String & data_path_in_backup) { - MultipleAccessStorage::restoreFromBackup(restorer); + MultipleAccessStorage::restoreFromBackup(restorer, data_path_in_backup); changes_notifier->sendNotifications(); } diff --git a/src/Access/AccessControl.h b/src/Access/AccessControl.h index 0c3bb9352f0..cc1b7b2ca0d 100644 --- a/src/Access/AccessControl.h +++ b/src/Access/AccessControl.h @@ -124,7 +124,7 @@ public: AuthResult authenticate(const Credentials & credentials, const Poco::Net::IPAddress & address, const String & forwarded_address) const; /// Makes a backup of access entities. - void restoreFromBackup(RestorerFromBackup & restorer) override; + void restoreFromBackup(RestorerFromBackup & restorer, const String & data_path_in_backup) override; void setExternalAuthenticatorsConfig(const Poco::Util::AbstractConfiguration & config); diff --git a/src/Access/GrantedRoles.cpp b/src/Access/GrantedRoles.cpp index e1a23182cc0..7f054e33c7c 100644 --- a/src/Access/GrantedRoles.cpp +++ b/src/Access/GrantedRoles.cpp @@ -176,6 +176,16 @@ std::vector GrantedRoles::findDependencies() const return res; } +bool GrantedRoles::hasDependencies(const std::unordered_set & ids) const +{ + for (const auto & role_id : roles) + { + if (ids.contains(role_id)) + return true; + } + return false; +} + void GrantedRoles::replaceDependencies(const std::unordered_map & old_to_new_ids) { std::vector new_ids; diff --git a/src/Access/GrantedRoles.h b/src/Access/GrantedRoles.h index ac252822089..adb3dc4d886 100644 --- a/src/Access/GrantedRoles.h +++ b/src/Access/GrantedRoles.h @@ -58,6 +58,7 @@ public: friend bool operator !=(const GrantedRoles & left, const GrantedRoles & right) { return !(left == right); } std::vector findDependencies() const; + bool hasDependencies(const std::unordered_set & ids) const; void replaceDependencies(const std::unordered_map & old_to_new_ids); private: diff --git a/src/Access/IAccessEntity.cpp b/src/Access/IAccessEntity.cpp index 9afa1b73597..5dc566fe456 100644 --- a/src/Access/IAccessEntity.cpp +++ b/src/Access/IAccessEntity.cpp @@ -9,28 +9,4 @@ bool IAccessEntity::equal(const IAccessEntity & other) const return (name == other.name) && (getType() == other.getType()); } -void IAccessEntity::replaceDependencies(std::shared_ptr & entity, const std::unordered_map & old_to_new_ids) -{ - if (old_to_new_ids.empty()) - return; - - bool need_replace_dependencies = false; - auto dependencies = entity->findDependencies(); - for (const auto & dependency : dependencies) - { - if (old_to_new_ids.contains(dependency)) - { - need_replace_dependencies = true; - break; - } - } - - if (!need_replace_dependencies) - return; - - auto new_entity = entity->clone(); - new_entity->replaceDependencies(old_to_new_ids); - entity = new_entity; -} - } diff --git a/src/Access/IAccessEntity.h b/src/Access/IAccessEntity.h index 2c2df7353c5..7b7eaa6a1cb 100644 --- a/src/Access/IAccessEntity.h +++ b/src/Access/IAccessEntity.h @@ -48,10 +48,10 @@ struct IAccessEntity /// Finds all dependencies. virtual std::vector findDependencies() const { return {}; } + virtual bool hasDependencies(const std::unordered_set & /* ids */) const { return false; } /// Replaces dependencies according to a specified map. - void replaceDependencies(const std::unordered_map & old_to_new_ids) { doReplaceDependencies(old_to_new_ids); } - static void replaceDependencies(std::shared_ptr & entity, const std::unordered_map & old_to_new_ids); + virtual void replaceDependencies(const std::unordered_map & /* old_to_new_ids */) {} /// Whether this access entity should be written to a backup. virtual bool isBackupAllowed() const { return false; } @@ -67,8 +67,6 @@ protected: { return std::make_shared(typeid_cast(*this)); } - - virtual void doReplaceDependencies(const std::unordered_map & /* old_to_new_ids */) {} }; using AccessEntityPtr = std::shared_ptr; diff --git a/src/Access/IAccessStorage.cpp b/src/Access/IAccessStorage.cpp index 29475461c45..e560619eacc 100644 --- a/src/Access/IAccessStorage.cpp +++ b/src/Access/IAccessStorage.cpp @@ -4,8 +4,10 @@ #include #include #include -#include +#include +#include #include +#include #include #include #include @@ -14,6 +16,7 @@ #include #include #include + #include #include #include @@ -604,61 +607,55 @@ void IAccessStorage::backup(BackupEntriesCollector & backup_entries_collector, c if (entities.empty()) return; - auto backup_entry = makeBackupEntryForAccess( + auto backup_entry_with_path = makeBackupEntryForAccess( entities, data_path_in_backup, backup_entries_collector.getAccessCounter(type), backup_entries_collector.getContext()->getAccessControl()); - backup_entries_collector.addBackupEntry(backup_entry); + if (isReplicated()) + { + auto backup_coordination = backup_entries_collector.getBackupCoordination(); + auto replication_id = getReplicationID(); + backup_coordination->addReplicatedAccessFilePath(replication_id, type, backup_entry_with_path.first); + + backup_entries_collector.addPostTask( + [backup_entry = backup_entry_with_path.second, + replication_id, + type, + &backup_entries_collector, + backup_coordination] + { + for (const String & path : backup_coordination->getReplicatedAccessFilePaths(replication_id, type)) + backup_entries_collector.addBackupEntry(path, backup_entry); + }); + } + else + { + backup_entries_collector.addBackupEntry(backup_entry_with_path); + } } -void IAccessStorage::restoreFromBackup(RestorerFromBackup & restorer) +void IAccessStorage::restoreFromBackup(RestorerFromBackup & restorer, const String & data_path_in_backup) { if (!isRestoreAllowed()) throwRestoreNotAllowed(); - if (isReplicated() && !acquireReplicatedRestore(restorer)) - return; - - auto entities = restorer.getAccessEntitiesToRestore(); - if (entities.empty()) - return; - - auto create_access = restorer.getRestoreSettings().create_access; - bool replace_if_exists = (create_access == RestoreAccessCreationMode::kReplace); - bool throw_if_exists = (create_access == RestoreAccessCreationMode::kCreate); - - restorer.addDataRestoreTask([this, entities_to_restore = std::move(entities), replace_if_exists, throw_if_exists] mutable + if (isReplicated()) { - std::unordered_map new_to_existing_ids; - for (auto & [id, entity] : entities_to_restore) - { - UUID existing_entity_id; - if (!insert(id, entity, replace_if_exists, throw_if_exists, &existing_entity_id)) - { - /// Couldn't insert `entity` because there is an existing entity with the same name. - new_to_existing_ids[id] = existing_entity_id; - } - } + auto restore_coordination = restorer.getRestoreCoordination(); + if (!restore_coordination->acquireReplicatedAccessStorage(getReplicationID())) + return; + } - if (!new_to_existing_ids.empty()) + restorer.addDataRestoreTask( + [this, &restorer, data_path_in_backup] { - /// If new entities restored from backup have dependencies on other entities from backup which were not restored because they existed, - /// then we should correct those dependencies. - auto update_func = [&](const AccessEntityPtr & entity) -> AccessEntityPtr - { - auto res = entity; - IAccessEntity::replaceDependencies(res, new_to_existing_ids); - return res; - }; - std::vector ids; - ids.reserve(entities_to_restore.size()); - boost::copy(entities_to_restore | boost::adaptors::map_keys, std::back_inserter(ids)); - tryUpdate(ids, update_func); - } - }); + auto entities = restorer.getAccessEntitiesToRestore(data_path_in_backup); + const auto & restore_settings = restorer.getRestoreSettings(); + restoreAccessEntitiesFromBackup(*this, entities, restore_settings); + }); } diff --git a/src/Access/IAccessStorage.h b/src/Access/IAccessStorage.h index a8ac75075d3..9158683e175 100644 --- a/src/Access/IAccessStorage.h +++ b/src/Access/IAccessStorage.h @@ -66,6 +66,7 @@ public: /// Returns true if this storage is replicated. virtual bool isReplicated() const { return false; } + virtual String getReplicationID() const { return ""; } /// Starts periodic reloading and updating of entities in this storage. virtual void startPeriodicReloading() {} @@ -214,7 +215,7 @@ public: /// Makes a backup of this access storage. virtual void backup(BackupEntriesCollector & backup_entries_collector, const String & data_path_in_backup, AccessEntityType type) const; - virtual void restoreFromBackup(RestorerFromBackup & restorer); + virtual void restoreFromBackup(RestorerFromBackup & restorer, const String & data_path_in_backup); protected: virtual std::optional findImpl(AccessEntityType type, const String & name) const = 0; diff --git a/src/Access/MultipleAccessStorage.cpp b/src/Access/MultipleAccessStorage.cpp index f1da8359d48..b722e8bd4c8 100644 --- a/src/Access/MultipleAccessStorage.cpp +++ b/src/Access/MultipleAccessStorage.cpp @@ -508,7 +508,7 @@ void MultipleAccessStorage::backup(BackupEntriesCollector & backup_entries_colle throwBackupNotAllowed(); } -void MultipleAccessStorage::restoreFromBackup(RestorerFromBackup & restorer) +void MultipleAccessStorage::restoreFromBackup(RestorerFromBackup & restorer, const String & data_path_in_backup) { auto storages = getStoragesInternal(); @@ -516,7 +516,7 @@ void MultipleAccessStorage::restoreFromBackup(RestorerFromBackup & restorer) { if (storage->isRestoreAllowed()) { - storage->restoreFromBackup(restorer); + storage->restoreFromBackup(restorer, data_path_in_backup); return; } } diff --git a/src/Access/MultipleAccessStorage.h b/src/Access/MultipleAccessStorage.h index 352cc7f7457..06147a45ba7 100644 --- a/src/Access/MultipleAccessStorage.h +++ b/src/Access/MultipleAccessStorage.h @@ -59,7 +59,7 @@ public: bool isBackupAllowed() const override; bool isRestoreAllowed() const override; void backup(BackupEntriesCollector & backup_entries_collector, const String & data_path_in_backup, AccessEntityType type) const override; - void restoreFromBackup(RestorerFromBackup & restorer) override; + void restoreFromBackup(RestorerFromBackup & restorer, const String & data_path_in_backup) override; bool containsStorage(std::string_view storage_type) const; protected: diff --git a/src/Access/Quota.cpp b/src/Access/Quota.cpp index ead5f77ce57..9571c6ba07f 100644 --- a/src/Access/Quota.cpp +++ b/src/Access/Quota.cpp @@ -24,7 +24,12 @@ std::vector Quota::findDependencies() const return to_roles.findDependencies(); } -void Quota::doReplaceDependencies(const std::unordered_map & old_to_new_ids) +bool Quota::hasDependencies(const std::unordered_set & ids) const +{ + return to_roles.hasDependencies(ids); +} + +void Quota::replaceDependencies(const std::unordered_map & old_to_new_ids) { to_roles.replaceDependencies(old_to_new_ids); } diff --git a/src/Access/Quota.h b/src/Access/Quota.h index 69ec2eb53a5..3f8493a005b 100644 --- a/src/Access/Quota.h +++ b/src/Access/Quota.h @@ -47,7 +47,9 @@ struct Quota : public IAccessEntity AccessEntityType getType() const override { return TYPE; } std::vector findDependencies() const override; - void doReplaceDependencies(const std::unordered_map & old_to_new_ids) override; + bool hasDependencies(const std::unordered_set & ids) const override; + void replaceDependencies(const std::unordered_map & old_to_new_ids) override; + bool isBackupAllowed() const override { return true; } }; diff --git a/src/Access/ReplicatedAccessStorage.cpp b/src/Access/ReplicatedAccessStorage.cpp index 9039a3b98b7..5f498851edb 100644 --- a/src/Access/ReplicatedAccessStorage.cpp +++ b/src/Access/ReplicatedAccessStorage.cpp @@ -4,10 +4,6 @@ #include #include #include -#include -#include -#include -#include #include #include #include @@ -684,44 +680,4 @@ AccessEntityPtr ReplicatedAccessStorage::readImpl(const UUID & id, bool throw_if return memory_storage.read(id, throw_if_not_exists); } - -void ReplicatedAccessStorage::backup(BackupEntriesCollector & backup_entries_collector, const String & data_path_in_backup, AccessEntityType type) const -{ - if (!isBackupAllowed()) - throwBackupNotAllowed(); - - auto entities = readAllWithIDs(type); - std::erase_if(entities, [](const std::pair & x) { return !x.second->isBackupAllowed(); }); - - if (entities.empty()) - return; - - auto backup_entry_with_path = makeBackupEntryForAccess( - entities, - data_path_in_backup, - backup_entries_collector.getAccessCounter(type), - backup_entries_collector.getContext()->getAccessControl()); - - auto backup_coordination = backup_entries_collector.getBackupCoordination(); - backup_coordination->addReplicatedAccessFilePath(zookeeper_path, type, backup_entry_with_path.first); - - backup_entries_collector.addPostTask( - [backup_entry = backup_entry_with_path.second, - my_zookeeper_path = zookeeper_path, - type, - &backup_entries_collector, - backup_coordination] - { - for (const String & path : backup_coordination->getReplicatedAccessFilePaths(my_zookeeper_path, type)) - backup_entries_collector.addBackupEntry(path, backup_entry); - }); -} - - -bool ReplicatedAccessStorage::acquireReplicatedRestore(RestorerFromBackup & restorer) const -{ - auto restore_coordination = restorer.getRestoreCoordination(); - return restore_coordination->acquireReplicatedAccessStorage(zookeeper_path); -} - } diff --git a/src/Access/ReplicatedAccessStorage.h b/src/Access/ReplicatedAccessStorage.h index 528dbb31c24..e46e854d414 100644 --- a/src/Access/ReplicatedAccessStorage.h +++ b/src/Access/ReplicatedAccessStorage.h @@ -26,7 +26,9 @@ public: void shutdown() override; const char * getStorageType() const override { return STORAGE_TYPE; } + bool isReplicated() const override { return true; } + String getReplicationID() const override { return zookeeper_path; } void startPeriodicReloading() override { startWatchingThread(); } void stopPeriodicReloading() override { stopWatchingThread(); } @@ -35,7 +37,6 @@ public: bool exists(const UUID & id) const override; bool isBackupAllowed() const override { return backup_allowed; } - void backup(BackupEntriesCollector & backup_entries_collector, const String & data_path_in_backup, AccessEntityType type) const override; private: String zookeeper_path; @@ -80,7 +81,6 @@ private: std::optional findImpl(AccessEntityType type, const String & name) const override; std::vector findAllImpl(AccessEntityType type) const override; AccessEntityPtr readImpl(const UUID & id, bool throw_if_not_exists) const override; - bool acquireReplicatedRestore(RestorerFromBackup & restorer) const override; mutable std::mutex mutex; MemoryAccessStorage memory_storage TSA_GUARDED_BY(mutex); diff --git a/src/Access/Role.cpp b/src/Access/Role.cpp index f6250594103..145a8539d12 100644 --- a/src/Access/Role.cpp +++ b/src/Access/Role.cpp @@ -21,7 +21,12 @@ std::vector Role::findDependencies() const return res; } -void Role::doReplaceDependencies(const std::unordered_map & old_to_new_ids) +bool Role::hasDependencies(const std::unordered_set & ids) const +{ + return granted_roles.hasDependencies(ids) || settings.hasDependencies(ids); +} + +void Role::replaceDependencies(const std::unordered_map & old_to_new_ids) { granted_roles.replaceDependencies(old_to_new_ids); settings.replaceDependencies(old_to_new_ids); diff --git a/src/Access/Role.h b/src/Access/Role.h index c7f98585a6c..d442d1c3ce2 100644 --- a/src/Access/Role.h +++ b/src/Access/Role.h @@ -21,7 +21,9 @@ struct Role : public IAccessEntity AccessEntityType getType() const override { return TYPE; } std::vector findDependencies() const override; - void doReplaceDependencies(const std::unordered_map & old_to_new_ids) override; + bool hasDependencies(const std::unordered_set & ids) const override; + void replaceDependencies(const std::unordered_map & old_to_new_ids) override; + bool isBackupAllowed() const override { return settings.isBackupAllowed(); } }; diff --git a/src/Access/RolesOrUsersSet.cpp b/src/Access/RolesOrUsersSet.cpp index c026ae42f76..8401182d35f 100644 --- a/src/Access/RolesOrUsersSet.cpp +++ b/src/Access/RolesOrUsersSet.cpp @@ -295,6 +295,23 @@ std::vector RolesOrUsersSet::findDependencies() const return res; } +bool RolesOrUsersSet::hasDependencies(const std::unordered_set & dependencies_ids) const +{ + for (const auto & id : ids) + { + if (dependencies_ids.contains(id)) + return true; + } + + for (const auto & id : except_ids) + { + if (dependencies_ids.contains(id)) + return true; + } + + return false; +} + void RolesOrUsersSet::replaceDependencies(const std::unordered_map & old_to_new_ids) { std::vector new_ids; diff --git a/src/Access/RolesOrUsersSet.h b/src/Access/RolesOrUsersSet.h index 29e499bc81b..0086e3366e5 100644 --- a/src/Access/RolesOrUsersSet.h +++ b/src/Access/RolesOrUsersSet.h @@ -64,6 +64,7 @@ struct RolesOrUsersSet friend bool operator !=(const RolesOrUsersSet & lhs, const RolesOrUsersSet & rhs) { return !(lhs == rhs); } std::vector findDependencies() const; + bool hasDependencies(const std::unordered_set & dependencies_ids) const; void replaceDependencies(const std::unordered_map & old_to_new_ids); bool all = false; diff --git a/src/Access/RowPolicy.cpp b/src/Access/RowPolicy.cpp index 8724d0f513c..951290b7847 100644 --- a/src/Access/RowPolicy.cpp +++ b/src/Access/RowPolicy.cpp @@ -63,7 +63,12 @@ std::vector RowPolicy::findDependencies() const return to_roles.findDependencies(); } -void RowPolicy::doReplaceDependencies(const std::unordered_map & old_to_new_ids) +bool RowPolicy::hasDependencies(const std::unordered_set & ids) const +{ + return to_roles.hasDependencies(ids); +} + +void RowPolicy::replaceDependencies(const std::unordered_map & old_to_new_ids) { to_roles.replaceDependencies(old_to_new_ids); } diff --git a/src/Access/RowPolicy.h b/src/Access/RowPolicy.h index 5cfe85c186a..ae1e289b310 100644 --- a/src/Access/RowPolicy.h +++ b/src/Access/RowPolicy.h @@ -50,7 +50,9 @@ struct RowPolicy : public IAccessEntity AccessEntityType getType() const override { return TYPE; } std::vector findDependencies() const override; - void doReplaceDependencies(const std::unordered_map & old_to_new_ids) override; + bool hasDependencies(const std::unordered_set & ids) const override; + void replaceDependencies(const std::unordered_map & old_to_new_ids) override; + bool isBackupAllowed() const override { return true; } /// Which roles or users should use this row policy. diff --git a/src/Access/SettingsProfile.cpp b/src/Access/SettingsProfile.cpp index 632bd97fbf5..b1c05120366 100644 --- a/src/Access/SettingsProfile.cpp +++ b/src/Access/SettingsProfile.cpp @@ -21,7 +21,12 @@ std::vector SettingsProfile::findDependencies() const return res; } -void SettingsProfile::doReplaceDependencies(const std::unordered_map & old_to_new_ids) +bool SettingsProfile::hasDependencies(const std::unordered_set & ids) const +{ + return elements.hasDependencies(ids) || to_roles.hasDependencies(ids); +} + +void SettingsProfile::replaceDependencies(const std::unordered_map & old_to_new_ids) { elements.replaceDependencies(old_to_new_ids); to_roles.replaceDependencies(old_to_new_ids); diff --git a/src/Access/SettingsProfile.h b/src/Access/SettingsProfile.h index 6bcaf6fef30..684f2177d51 100644 --- a/src/Access/SettingsProfile.h +++ b/src/Access/SettingsProfile.h @@ -22,7 +22,9 @@ struct SettingsProfile : public IAccessEntity AccessEntityType getType() const override { return TYPE; } std::vector findDependencies() const override; - void doReplaceDependencies(const std::unordered_map & old_to_new_ids) override; + bool hasDependencies(const std::unordered_set & ids) const override; + void replaceDependencies(const std::unordered_map & old_to_new_ids) override; + bool isBackupAllowed() const override { return elements.isBackupAllowed(); } }; diff --git a/src/Access/SettingsProfileElement.cpp b/src/Access/SettingsProfileElement.cpp index 9358391cb93..896d991a03f 100644 --- a/src/Access/SettingsProfileElement.cpp +++ b/src/Access/SettingsProfileElement.cpp @@ -158,6 +158,18 @@ std::vector SettingsProfileElements::findDependencies() const } +bool SettingsProfileElements::hasDependencies(const std::unordered_set & ids) const +{ + std::vector res; + for (const auto & element : *this) + { + if (element.parent_profile && ids.contains(*element.parent_profile)) + return true; + } + return false; +} + + void SettingsProfileElements::replaceDependencies(const std::unordered_map & old_to_new_ids) { for (auto & element : *this) diff --git a/src/Access/SettingsProfileElement.h b/src/Access/SettingsProfileElement.h index 7078f565295..bdd950c086a 100644 --- a/src/Access/SettingsProfileElement.h +++ b/src/Access/SettingsProfileElement.h @@ -63,6 +63,7 @@ public: std::shared_ptr toASTWithNames(const AccessControl & access_control) const; std::vector findDependencies() const; + bool hasDependencies(const std::unordered_set & ids) const; void replaceDependencies(const std::unordered_map & old_to_new_ids); void merge(const SettingsProfileElements & other); diff --git a/src/Access/User.cpp b/src/Access/User.cpp index 2052527f4ae..156bc306caa 100644 --- a/src/Access/User.cpp +++ b/src/Access/User.cpp @@ -49,7 +49,12 @@ std::vector User::findDependencies() const return res; } -void User::doReplaceDependencies(const std::unordered_map & old_to_new_ids) +bool User::hasDependencies(const std::unordered_set & ids) const +{ + return default_roles.hasDependencies(ids) || granted_roles.hasDependencies(ids) || grantees.hasDependencies(ids) || settings.hasDependencies(ids); +} + +void User::replaceDependencies(const std::unordered_map & old_to_new_ids) { default_roles.replaceDependencies(old_to_new_ids); granted_roles.replaceDependencies(old_to_new_ids); diff --git a/src/Access/User.h b/src/Access/User.h index 7f91c1e3756..746c55de156 100644 --- a/src/Access/User.h +++ b/src/Access/User.h @@ -32,7 +32,9 @@ struct User : public IAccessEntity void setName(const String & name_) override; std::vector findDependencies() const override; - void doReplaceDependencies(const std::unordered_map & old_to_new_ids) override; + bool hasDependencies(const std::unordered_set & ids) const override; + void replaceDependencies(const std::unordered_map & old_to_new_ids) override; + bool isBackupAllowed() const override { return settings.isBackupAllowed(); } }; diff --git a/src/Backups/BackupCoordinationStage.h b/src/Backups/BackupCoordinationStage.h index 0a414a59a77..9abdc019784 100644 --- a/src/Backups/BackupCoordinationStage.h +++ b/src/Backups/BackupCoordinationStage.h @@ -32,6 +32,9 @@ namespace BackupCoordinationStage /// Finding databases and tables in the backup which we're going to restore. constexpr const char * FINDING_TABLES_IN_BACKUP = "finding tables in backup"; + /// Loading system access tables and then checking if the current user has enough access to restore. + constexpr const char * CHECKING_ACCESS_RIGHTS = "checking access rights"; + /// Creating databases or finding them and checking their definitions. constexpr const char * CREATING_DATABASES = "creating databases"; diff --git a/src/Backups/RestoreSettings.cpp b/src/Backups/RestoreSettings.cpp index 8e60e8d129e..a18c3abe53b 100644 --- a/src/Backups/RestoreSettings.cpp +++ b/src/Backups/RestoreSettings.cpp @@ -187,6 +187,7 @@ RestoreSettings RestoreSettings::fromRestoreQuery(const ASTBackupQuery & query) else LIST_OF_RESTORE_SETTINGS(GET_SETTINGS_FROM_RESTORE_QUERY_HELPER) + throw Exception(ErrorCodes::CANNOT_PARSE_BACKUP_SETTINGS, "Unknown setting {}", setting.name); } } diff --git a/src/Backups/RestorerFromBackup.cpp b/src/Backups/RestorerFromBackup.cpp index 5e6beec791d..43b1fd935f4 100644 --- a/src/Backups/RestorerFromBackup.cpp +++ b/src/Backups/RestorerFromBackup.cpp @@ -136,6 +136,8 @@ void RestorerFromBackup::run(Mode mode) waitFutures(); /// Check access rights. + setStage(Stage::CHECKING_ACCESS_RIGHTS); + loadSystemAccessTables(); checkAccessForObjectsFoundInBackup(); if (mode == Mode::CHECK_ACCESS_ONLY) @@ -482,25 +484,6 @@ void RestorerFromBackup::findTableInBackupImpl(const QualifiedTableName & table_ res_table_info.partitions.emplace(); insertAtEnd(*res_table_info.partitions, *partitions); } - - /// Special handling for ACL-related system tables. - if (!restore_settings.structure_only && isSystemAccessTableName(table_name)) - { - if (!access_restorer) - access_restorer = std::make_unique(backup, restore_settings); - - try - { - /// addDataPath() will parse access*.txt files and extract access entities from them. - /// We need to do that early because we need those access entities to check access. - access_restorer->addDataPath(data_path_in_backup); - } - catch (Exception & e) - { - e.addMessage("While parsing data of {} from backup", tableNameWithTypeToString(table_name.database, table_name.table, false)); - throw; - } - } } void RestorerFromBackup::findDatabaseInBackup(const String & database_name_in_backup, const std::set & except_table_names) @@ -624,6 +607,27 @@ size_t RestorerFromBackup::getNumTables() const return table_infos.size(); } +void RestorerFromBackup::loadSystemAccessTables() +{ + if (restore_settings.structure_only) + return; + + /// Special handling for ACL-related system tables. + std::lock_guard lock{mutex}; + for (const auto & [table_name, table_info] : table_infos) + { + if (isSystemAccessTableName(table_name)) + { + if (!access_restorer) + access_restorer = std::make_unique(backup, restore_settings); + access_restorer->addDataPath(table_info.data_path_in_backup); + } + } + + if (access_restorer) + access_restorer->loadFromBackup(); +} + void RestorerFromBackup::checkAccessForObjectsFoundInBackup() const { AccessRightsElements required_access; @@ -708,6 +712,15 @@ void RestorerFromBackup::checkAccessForObjectsFoundInBackup() const context->checkAccess(required_access); } +std::vector> RestorerFromBackup::getAccessEntitiesToRestore(const String & data_path_in_backup) const +{ + std::lock_guard lock{mutex}; + if (!access_restorer) + return {}; + access_restorer->generateRandomIDsAndResolveDependencies(context->getAccessControl()); + return access_restorer->getEntities(data_path_in_backup); +} + void RestorerFromBackup::createDatabases() { Strings database_names; @@ -1066,19 +1079,6 @@ void RestorerFromBackup::runDataRestoreTasks() } } -std::vector> RestorerFromBackup::getAccessEntitiesToRestore() -{ - std::lock_guard lock{mutex}; - - if (!access_restorer || access_restored) - return {}; - - /// getAccessEntitiesToRestore() will return entities only when called first time (we don't want to restore the same entities again). - access_restored = true; - - return access_restorer->getAccessEntities(context->getAccessControl()); -} - void RestorerFromBackup::throwTableIsNotEmpty(const StorageID & storage_id) { throw Exception( diff --git a/src/Backups/RestorerFromBackup.h b/src/Backups/RestorerFromBackup.h index 6dbca5bced5..2318e75e377 100644 --- a/src/Backups/RestorerFromBackup.h +++ b/src/Backups/RestorerFromBackup.h @@ -68,7 +68,7 @@ public: void addDataRestoreTasks(DataRestoreTasks && new_tasks); /// Returns the list of access entities to restore. - std::vector> getAccessEntitiesToRestore(); + std::vector> getAccessEntitiesToRestore(const String & data_path_in_backup) const; /// Throws an exception that a specified table is already non-empty. [[noreturn]] static void throwTableIsNotEmpty(const StorageID & storage_id); @@ -101,6 +101,7 @@ private: size_t getNumDatabases() const; size_t getNumTables() const; + void loadSystemAccessTables(); void checkAccessForObjectsFoundInBackup() const; void createDatabases(); diff --git a/src/Storages/System/StorageSystemQuotas.cpp b/src/Storages/System/StorageSystemQuotas.cpp index 641bbb319d5..ddd86f8736b 100644 --- a/src/Storages/System/StorageSystemQuotas.cpp +++ b/src/Storages/System/StorageSystemQuotas.cpp @@ -150,10 +150,10 @@ void StorageSystemQuotas::backupData( } void StorageSystemQuotas::restoreDataFromBackup( - RestorerFromBackup & restorer, const String & /* data_path_in_backup */, const std::optional & /* partitions */) + RestorerFromBackup & restorer, const String & data_path_in_backup, const std::optional & /* partitions */) { auto & access_control = restorer.getContext()->getAccessControl(); - access_control.restoreFromBackup(restorer); + access_control.restoreFromBackup(restorer, data_path_in_backup); } } diff --git a/src/Storages/System/StorageSystemRoles.cpp b/src/Storages/System/StorageSystemRoles.cpp index 9bfddc25ebf..2409580bbd2 100644 --- a/src/Storages/System/StorageSystemRoles.cpp +++ b/src/Storages/System/StorageSystemRoles.cpp @@ -70,10 +70,10 @@ void StorageSystemRoles::backupData( } void StorageSystemRoles::restoreDataFromBackup( - RestorerFromBackup & restorer, const String & /* data_path_in_backup */, const std::optional & /* partitions */) + RestorerFromBackup & restorer, const String & data_path_in_backup, const std::optional & /* partitions */) { auto & access_control = restorer.getContext()->getAccessControl(); - access_control.restoreFromBackup(restorer); + access_control.restoreFromBackup(restorer, data_path_in_backup); } } diff --git a/src/Storages/System/StorageSystemRowPolicies.cpp b/src/Storages/System/StorageSystemRowPolicies.cpp index 93c5ba60a7f..759535e33b7 100644 --- a/src/Storages/System/StorageSystemRowPolicies.cpp +++ b/src/Storages/System/StorageSystemRowPolicies.cpp @@ -160,10 +160,10 @@ void StorageSystemRowPolicies::backupData( } void StorageSystemRowPolicies::restoreDataFromBackup( - RestorerFromBackup & restorer, const String & /* data_path_in_backup */, const std::optional & /* partitions */) + RestorerFromBackup & restorer, const String & data_path_in_backup, const std::optional & /* partitions */) { auto & access_control = restorer.getContext()->getAccessControl(); - access_control.restoreFromBackup(restorer); + access_control.restoreFromBackup(restorer, data_path_in_backup); } } diff --git a/src/Storages/System/StorageSystemSettingsProfiles.cpp b/src/Storages/System/StorageSystemSettingsProfiles.cpp index 795152e31f3..7aa9fae9ebc 100644 --- a/src/Storages/System/StorageSystemSettingsProfiles.cpp +++ b/src/Storages/System/StorageSystemSettingsProfiles.cpp @@ -101,10 +101,10 @@ void StorageSystemSettingsProfiles::backupData( } void StorageSystemSettingsProfiles::restoreDataFromBackup( - RestorerFromBackup & restorer, const String & /* data_path_in_backup */, const std::optional & /* partitions */) + RestorerFromBackup & restorer, const String & data_path_in_backup, const std::optional & /* partitions */) { auto & access_control = restorer.getContext()->getAccessControl(); - access_control.restoreFromBackup(restorer); + access_control.restoreFromBackup(restorer, data_path_in_backup); } } diff --git a/src/Storages/System/StorageSystemUsers.cpp b/src/Storages/System/StorageSystemUsers.cpp index ce4950f5e7b..7589afdeb3e 100644 --- a/src/Storages/System/StorageSystemUsers.cpp +++ b/src/Storages/System/StorageSystemUsers.cpp @@ -261,10 +261,10 @@ void StorageSystemUsers::backupData( } void StorageSystemUsers::restoreDataFromBackup( - RestorerFromBackup & restorer, const String & /* data_path_in_backup */, const std::optional & /* partitions */) + RestorerFromBackup & restorer, const String & data_path_in_backup, const std::optional & /* partitions */) { auto & access_control = restorer.getContext()->getAccessControl(); - access_control.restoreFromBackup(restorer); + access_control.restoreFromBackup(restorer, data_path_in_backup); } } From f33b6fd5f53db87d06b67a71a6f1f2ed131a9912 Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Wed, 18 Sep 2024 19:52:24 +0200 Subject: [PATCH 02/13] Add some logging to restoring access entities. --- src/Access/AccessBackup.cpp | 21 +++++++++++++++++++++ src/Access/AccessBackup.h | 2 ++ 2 files changed, 23 insertions(+) diff --git a/src/Access/AccessBackup.cpp b/src/Access/AccessBackup.cpp index c2c37adffa8..1ac9a0d61e5 100644 --- a/src/Access/AccessBackup.cpp +++ b/src/Access/AccessBackup.cpp @@ -200,6 +200,7 @@ AccessRestorerFromBackup::AccessRestorerFromBackup( : backup(backup_) , creation_mode(restore_settings_.create_access) , skip_unresolved_dependencies(restore_settings_.allow_unresolved_access_dependencies) + , log(getLogger("AccessRestorerFromBackup")) { } @@ -378,6 +379,7 @@ void AccessRestorerFromBackup::generateRandomIDsAndResolveDependencies(const Acc if (entity_info.restore && (creation_mode == RestoreAccessCreationMode::kReplace)) { entity_info.new_id = UUIDHelpers::generateV4(); + LOG_TRACE(log, "{}: Generated new UUID {}", AccessEntityTypeInfo::get(type).formatEntityNameWithType(name), *entity_info.new_id); continue; } @@ -388,8 +390,11 @@ void AccessRestorerFromBackup::generateRandomIDsAndResolveDependencies(const Acc throw Exception(ErrorCodes::ACCESS_ENTITY_ALREADY_EXISTS, "Cannot restore {} because it already exists", AccessEntityTypeInfo::get(type).formatEntityNameWithType(name)); } + bool was_going_to_restore = entity_info.restore; entity_info.new_id = *existing_id; entity_info.restore = false; + LOG_TRACE(log, "{}: Found with UUID {}{}", AccessEntityTypeInfo::get(type).formatEntityNameWithType(name), *existing_id, + (was_going_to_restore ? ", will not restore" : "")); } else { @@ -399,7 +404,14 @@ void AccessRestorerFromBackup::generateRandomIDsAndResolveDependencies(const Acc AccessEntityTypeInfo::get(type).formatEntityNameWithType(name)); } if (entity_info.restore) + { entity_info.new_id = UUIDHelpers::generateV4(); + LOG_TRACE(log, "{}: Generated new UUID {}", AccessEntityTypeInfo::get(type).formatEntityNameWithType(name), *entity_info.new_id); + } + else + { + LOG_TRACE(log, "{}: Not found, ignoring", AccessEntityTypeInfo::get(type).formatEntityNameWithType(name)); + } } } @@ -459,6 +471,8 @@ void restoreAccessEntitiesFromBackup( if (entities.empty()) return; /// Nothing to restore. + auto log = getLogger("AccessRestorerFromBackup"); + bool replace_if_exists = (restore_settings.create_access == RestoreAccessCreationMode::kReplace); bool throw_if_exists = (restore_settings.create_access == RestoreAccessCreationMode::kCreate); @@ -467,14 +481,20 @@ void restoreAccessEntitiesFromBackup( for (const auto & [id, entity] : entities) { + const String & name = entity->getName(); + auto type = entity->getType(); + LOG_TRACE(log, "{}: Adding with UUID {}", AccessEntityTypeInfo::get(type).formatEntityNameWithType(name), id); + UUID existing_id; if (destination_access_storage.insert(id, entity, replace_if_exists, throw_if_exists, &existing_id)) { + LOG_TRACE(log, "{}: Added successfully", AccessEntityTypeInfo::get(type).formatEntityNameWithType(name)); restored_ids.emplace(id); } else { /// Couldn't insert `entity` because there is an existing entity with the same name. + LOG_TRACE(log, "{}: Not added because already exists with UUID {}", AccessEntityTypeInfo::get(type).formatEntityNameWithType(name), existing_id); new_to_existing_ids[id] = existing_id; } } @@ -494,6 +514,7 @@ void restoreAccessEntitiesFromBackup( { if (!entity->hasDependencies(new_ids)) return entity; + LOG_TRACE(log, "{}: Updating dependencies", entity->formatTypeWithName()); auto res = entity->clone(); res->replaceDependencies(new_to_existing_ids); return res; diff --git a/src/Access/AccessBackup.h b/src/Access/AccessBackup.h index c41184a0b7c..84dcf74a26e 100644 --- a/src/Access/AccessBackup.h +++ b/src/Access/AccessBackup.h @@ -1,5 +1,6 @@ #pragma once +#include #include #include @@ -63,6 +64,7 @@ private: const BackupPtr backup; const RestoreAccessCreationMode creation_mode; const bool skip_unresolved_dependencies; + const LoggerPtr log; /// Whether loadFromBackup() finished. bool loaded = false; From 198974dd7e5feb5ae8e77ba3f068777356abc8fd Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Wed, 18 Sep 2024 18:44:27 +0200 Subject: [PATCH 03/13] Change comment for allow_unresolved_access_dependencies. --- src/Backups/RestoreSettings.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/Backups/RestoreSettings.h b/src/Backups/RestoreSettings.h index fe07a0a7208..726a33b07db 100644 --- a/src/Backups/RestoreSettings.h +++ b/src/Backups/RestoreSettings.h @@ -100,8 +100,14 @@ struct RestoreSettings /// How the RESTORE command will handle if an user (or role or profile) which it's going to restore already exists. RestoreAccessCreationMode create_access = RestoreAccessCreationMode::kCreateIfNotExists; - /// Skip dependencies of access entities which can't be resolved. - /// For example, if an user has a profile assigned and that profile is not in the backup and doesn't exist locally. + /// Ignore dependencies of access entities which can't be resolved. + /// For example: if a backup contains a profile assigned to a user: `CREATE PROFILE p1; CREATE USER u1 SETTINGS PROFILE p1` + /// and now we're restoring only user `u1` and profile `p1` doesn't exists, then + /// this flag is whether RESTORE should continue with restoring user `u1` without assigning profile `p1`. + /// Another example: if a backup contains a role granted to a user: `CREATE USER u2; CREATE ROLE r2; GRANT r2 TO u2` + /// and now we're restoring only user `u2` and role `r2` doesn't exist, then + /// this flag is whether RESTORE should continue with restoring user `u2` without that grant. + /// If this flag is false then RESTORE will throw an exception in that case. bool allow_unresolved_access_dependencies = false; /// How the RESTORE command will handle if a user-defined function which it's going to restore already exists. From ac0f5a8d529e11c1d6f36cc5bbbfd72546b12f25 Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Wed, 18 Sep 2024 18:44:47 +0200 Subject: [PATCH 04/13] Remove unresolved dependencies while restoring with "allow_unresolved_access_dependencies = true". --- src/Access/AccessBackup.cpp | 4 ++++ src/Access/GrantedRoles.cpp | 29 +++++++++++++++++++++++++++ src/Access/GrantedRoles.h | 1 + src/Access/IAccessEntity.h | 1 + src/Access/Quota.cpp | 5 +++++ src/Access/Quota.h | 1 + src/Access/Role.cpp | 6 ++++++ src/Access/Role.h | 1 + src/Access/RolesOrUsersSet.cpp | 19 ++++++++++++++++++ src/Access/RolesOrUsersSet.h | 1 + src/Access/RowPolicy.cpp | 5 +++++ src/Access/RowPolicy.h | 1 + src/Access/SettingsProfile.cpp | 6 ++++++ src/Access/SettingsProfile.h | 1 + src/Access/SettingsProfileElement.cpp | 7 +++++++ src/Access/SettingsProfileElement.h | 1 + src/Access/User.cpp | 8 ++++++++ src/Access/User.h | 1 + 18 files changed, 98 insertions(+) diff --git a/src/Access/AccessBackup.cpp b/src/Access/AccessBackup.cpp index 1ac9a0d61e5..77360923724 100644 --- a/src/Access/AccessBackup.cpp +++ b/src/Access/AccessBackup.cpp @@ -417,11 +417,14 @@ void AccessRestorerFromBackup::generateRandomIDsAndResolveDependencies(const Acc /// Prepare map from old UUIDs to new UUIDs. std::unordered_map old_to_new_ids; + std::unordered_set unresolved_ids; for (const auto & [id, entity_info] : entity_infos) { if (entity_info.new_id) old_to_new_ids[id] = *entity_info.new_id; + else + unresolved_ids.insert(id); } /// Remap the UUIDs of dependencies in the access entities we're going to restore. @@ -431,6 +434,7 @@ void AccessRestorerFromBackup::generateRandomIDsAndResolveDependencies(const Acc { auto new_entity = entity_info.entity->clone(); new_entity->replaceDependencies(old_to_new_ids); + new_entity->removeDependencies(unresolved_ids); entity_info.entity = new_entity; } diff --git a/src/Access/GrantedRoles.cpp b/src/Access/GrantedRoles.cpp index 7f054e33c7c..f1e2116dfa8 100644 --- a/src/Access/GrantedRoles.cpp +++ b/src/Access/GrantedRoles.cpp @@ -231,4 +231,33 @@ void GrantedRoles::replaceDependencies(const std::unordered_map & ol } } +void GrantedRoles::removeDependencies(const std::unordered_set & ids) +{ + bool found = false; + + for (auto it = roles.begin(); it != roles.end();) + { + if (ids.contains(*it)) + { + it = roles.erase(it); + found = true; + } + else + { + ++it; + } + } + + if (found) + { + for (auto it = roles_with_admin_option.begin(); it != roles_with_admin_option.end();) + { + if (ids.contains(*it)) + it = roles_with_admin_option.erase(it); + else + ++it; + } + } +} + } diff --git a/src/Access/GrantedRoles.h b/src/Access/GrantedRoles.h index adb3dc4d886..9a128d3589b 100644 --- a/src/Access/GrantedRoles.h +++ b/src/Access/GrantedRoles.h @@ -60,6 +60,7 @@ public: std::vector findDependencies() const; bool hasDependencies(const std::unordered_set & ids) const; void replaceDependencies(const std::unordered_map & old_to_new_ids); + void removeDependencies(const std::unordered_set & ids); private: boost::container::flat_set roles; diff --git a/src/Access/IAccessEntity.h b/src/Access/IAccessEntity.h index 7b7eaa6a1cb..932f8c71b2d 100644 --- a/src/Access/IAccessEntity.h +++ b/src/Access/IAccessEntity.h @@ -52,6 +52,7 @@ struct IAccessEntity /// Replaces dependencies according to a specified map. virtual void replaceDependencies(const std::unordered_map & /* old_to_new_ids */) {} + virtual void removeDependencies(const std::unordered_set & /* ids */) {} /// Whether this access entity should be written to a backup. virtual bool isBackupAllowed() const { return false; } diff --git a/src/Access/Quota.cpp b/src/Access/Quota.cpp index 9571c6ba07f..1b80b109a35 100644 --- a/src/Access/Quota.cpp +++ b/src/Access/Quota.cpp @@ -34,4 +34,9 @@ void Quota::replaceDependencies(const std::unordered_map & old_to_ne to_roles.replaceDependencies(old_to_new_ids); } +void Quota::removeDependencies(const std::unordered_set & ids) +{ + to_roles.removeDependencies(ids); +} + } diff --git a/src/Access/Quota.h b/src/Access/Quota.h index 3f8493a005b..9e5e787d54d 100644 --- a/src/Access/Quota.h +++ b/src/Access/Quota.h @@ -49,6 +49,7 @@ struct Quota : public IAccessEntity std::vector findDependencies() const override; bool hasDependencies(const std::unordered_set & ids) const override; void replaceDependencies(const std::unordered_map & old_to_new_ids) override; + void removeDependencies(const std::unordered_set & ids) override; bool isBackupAllowed() const override { return true; } }; diff --git a/src/Access/Role.cpp b/src/Access/Role.cpp index 145a8539d12..340918ec24a 100644 --- a/src/Access/Role.cpp +++ b/src/Access/Role.cpp @@ -32,4 +32,10 @@ void Role::replaceDependencies(const std::unordered_map & old_to_new settings.replaceDependencies(old_to_new_ids); } +void Role::removeDependencies(const std::unordered_set & ids) +{ + granted_roles.removeDependencies(ids); + settings.removeDependencies(ids); +} + } diff --git a/src/Access/Role.h b/src/Access/Role.h index d442d1c3ce2..e6d0cddf4b9 100644 --- a/src/Access/Role.h +++ b/src/Access/Role.h @@ -23,6 +23,7 @@ struct Role : public IAccessEntity std::vector findDependencies() const override; bool hasDependencies(const std::unordered_set & ids) const override; void replaceDependencies(const std::unordered_map & old_to_new_ids) override; + void removeDependencies(const std::unordered_set & ids) override; bool isBackupAllowed() const override { return settings.isBackupAllowed(); } }; diff --git a/src/Access/RolesOrUsersSet.cpp b/src/Access/RolesOrUsersSet.cpp index 8401182d35f..8aa840738f1 100644 --- a/src/Access/RolesOrUsersSet.cpp +++ b/src/Access/RolesOrUsersSet.cpp @@ -354,4 +354,23 @@ void RolesOrUsersSet::replaceDependencies(const std::unordered_map & boost::range::copy(new_ids, std::inserter(except_ids, except_ids.end())); } +void RolesOrUsersSet::removeDependencies(const std::unordered_set & dependencies_ids) +{ + for (auto it = ids.begin(); it != ids.end();) + { + if (dependencies_ids.contains(*it)) + it = ids.erase(it); + else + ++it; + } + + for (auto it = except_ids.begin(); it != except_ids.end();) + { + if (dependencies_ids.contains(*it)) + except_ids.erase(it); + else + ++it; + } +} + } diff --git a/src/Access/RolesOrUsersSet.h b/src/Access/RolesOrUsersSet.h index 0086e3366e5..f3d82c81682 100644 --- a/src/Access/RolesOrUsersSet.h +++ b/src/Access/RolesOrUsersSet.h @@ -66,6 +66,7 @@ struct RolesOrUsersSet std::vector findDependencies() const; bool hasDependencies(const std::unordered_set & dependencies_ids) const; void replaceDependencies(const std::unordered_map & old_to_new_ids); + void removeDependencies(const std::unordered_set & dependencies_ids); bool all = false; boost::container::flat_set ids; diff --git a/src/Access/RowPolicy.cpp b/src/Access/RowPolicy.cpp index 951290b7847..427110add4c 100644 --- a/src/Access/RowPolicy.cpp +++ b/src/Access/RowPolicy.cpp @@ -73,4 +73,9 @@ void RowPolicy::replaceDependencies(const std::unordered_map & old_t to_roles.replaceDependencies(old_to_new_ids); } +void RowPolicy::removeDependencies(const std::unordered_set & ids) +{ + to_roles.removeDependencies(ids); +} + } diff --git a/src/Access/RowPolicy.h b/src/Access/RowPolicy.h index ae1e289b310..4fd6abc67e4 100644 --- a/src/Access/RowPolicy.h +++ b/src/Access/RowPolicy.h @@ -52,6 +52,7 @@ struct RowPolicy : public IAccessEntity std::vector findDependencies() const override; bool hasDependencies(const std::unordered_set & ids) const override; void replaceDependencies(const std::unordered_map & old_to_new_ids) override; + void removeDependencies(const std::unordered_set & ids) override; bool isBackupAllowed() const override { return true; } diff --git a/src/Access/SettingsProfile.cpp b/src/Access/SettingsProfile.cpp index b1c05120366..8206bf83c23 100644 --- a/src/Access/SettingsProfile.cpp +++ b/src/Access/SettingsProfile.cpp @@ -32,4 +32,10 @@ void SettingsProfile::replaceDependencies(const std::unordered_map & to_roles.replaceDependencies(old_to_new_ids); } +void SettingsProfile::removeDependencies(const std::unordered_set & ids) +{ + elements.removeDependencies(ids); + to_roles.removeDependencies(ids); +} + } diff --git a/src/Access/SettingsProfile.h b/src/Access/SettingsProfile.h index 684f2177d51..92e9b3222f9 100644 --- a/src/Access/SettingsProfile.h +++ b/src/Access/SettingsProfile.h @@ -24,6 +24,7 @@ struct SettingsProfile : public IAccessEntity std::vector findDependencies() const override; bool hasDependencies(const std::unordered_set & ids) const override; void replaceDependencies(const std::unordered_map & old_to_new_ids) override; + void removeDependencies(const std::unordered_set & ids) override; bool isBackupAllowed() const override { return elements.isBackupAllowed(); } }; diff --git a/src/Access/SettingsProfileElement.cpp b/src/Access/SettingsProfileElement.cpp index 896d991a03f..17967ed0bc0 100644 --- a/src/Access/SettingsProfileElement.cpp +++ b/src/Access/SettingsProfileElement.cpp @@ -188,6 +188,13 @@ void SettingsProfileElements::replaceDependencies(const std::unordered_map & ids) +{ + std::erase_if( + *this, [&](const SettingsProfileElement & element) { return element.parent_profile && ids.contains(*element.parent_profile); }); +} + + void SettingsProfileElements::merge(const SettingsProfileElements & other) { insert(end(), other.begin(), other.end()); diff --git a/src/Access/SettingsProfileElement.h b/src/Access/SettingsProfileElement.h index bdd950c086a..000ae2784df 100644 --- a/src/Access/SettingsProfileElement.h +++ b/src/Access/SettingsProfileElement.h @@ -65,6 +65,7 @@ public: std::vector findDependencies() const; bool hasDependencies(const std::unordered_set & ids) const; void replaceDependencies(const std::unordered_map & old_to_new_ids); + void removeDependencies(const std::unordered_set & ids); void merge(const SettingsProfileElements & other); diff --git a/src/Access/User.cpp b/src/Access/User.cpp index 156bc306caa..18f5ea8e124 100644 --- a/src/Access/User.cpp +++ b/src/Access/User.cpp @@ -62,4 +62,12 @@ void User::replaceDependencies(const std::unordered_map & old_to_new settings.replaceDependencies(old_to_new_ids); } +void User::removeDependencies(const std::unordered_set & ids) +{ + default_roles.removeDependencies(ids); + granted_roles.removeDependencies(ids); + grantees.removeDependencies(ids); + settings.removeDependencies(ids); +} + } diff --git a/src/Access/User.h b/src/Access/User.h index 746c55de156..7d7ec5362bd 100644 --- a/src/Access/User.h +++ b/src/Access/User.h @@ -34,6 +34,7 @@ struct User : public IAccessEntity std::vector findDependencies() const override; bool hasDependencies(const std::unordered_set & ids) const override; void replaceDependencies(const std::unordered_map & old_to_new_ids) override; + void removeDependencies(const std::unordered_set & ids) override; bool isBackupAllowed() const override { return settings.isBackupAllowed(); } }; From 0bcc2b9708c2f4ac40cff06c3e58d9e760897155 Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Wed, 18 Sep 2024 13:12:48 +0200 Subject: [PATCH 05/13] Add UUID to UpdateFunc in IAccessStorage. --- src/Access/AccessBackup.cpp | 2 +- src/Access/DiskAccessStorage.cpp | 2 +- src/Access/IAccessStorage.h | 2 +- src/Access/LDAPAccessStorage.cpp | 4 ++-- src/Access/MemoryAccessStorage.cpp | 2 +- src/Access/MultipleAccessStorage.cpp | 2 +- src/Access/ReplicatedAccessStorage.cpp | 2 +- src/Interpreters/Access/InterpreterCreateQuotaQuery.cpp | 2 +- src/Interpreters/Access/InterpreterCreateRoleQuery.cpp | 2 +- src/Interpreters/Access/InterpreterCreateRowPolicyQuery.cpp | 2 +- .../Access/InterpreterCreateSettingsProfileQuery.cpp | 2 +- src/Interpreters/Access/InterpreterCreateUserQuery.cpp | 4 ++-- src/Interpreters/Access/InterpreterGrantQuery.cpp | 2 +- src/Interpreters/Access/InterpreterSetRoleQuery.cpp | 2 +- 14 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/Access/AccessBackup.cpp b/src/Access/AccessBackup.cpp index 77360923724..96b9eb5c91d 100644 --- a/src/Access/AccessBackup.cpp +++ b/src/Access/AccessBackup.cpp @@ -514,7 +514,7 @@ void restoreAccessEntitiesFromBackup( /// If new entities restored from backup have dependencies on other entities from backup which were not restored because they existed, /// then we should correct those dependencies. - auto update_func = [&](const AccessEntityPtr & entity) -> AccessEntityPtr + auto update_func = [&](const AccessEntityPtr & entity, const UUID &) -> AccessEntityPtr { if (!entity->hasDependencies(new_ids)) return entity; diff --git a/src/Access/DiskAccessStorage.cpp b/src/Access/DiskAccessStorage.cpp index 046c532cf5c..a9c69661725 100644 --- a/src/Access/DiskAccessStorage.cpp +++ b/src/Access/DiskAccessStorage.cpp @@ -676,7 +676,7 @@ bool DiskAccessStorage::updateNoLock(const UUID & id, const UpdateFunc & update_ if (!entry.entity) entry.entity = readAccessEntityFromDisk(id); auto old_entity = entry.entity; - auto new_entity = update_func(old_entity); + auto new_entity = update_func(old_entity, id); if (!new_entity->isTypeOf(old_entity->getType())) throwBadCast(id, new_entity->getType(), new_entity->getName(), old_entity->getType()); diff --git a/src/Access/IAccessStorage.h b/src/Access/IAccessStorage.h index 9158683e175..b1bec2ce465 100644 --- a/src/Access/IAccessStorage.h +++ b/src/Access/IAccessStorage.h @@ -181,7 +181,7 @@ public: /// Removes multiple entities from the storage. Returns the list of successfully dropped. std::vector tryRemove(const std::vector & ids); - using UpdateFunc = std::function; + using UpdateFunc = std::function; /// Updates an entity stored in the storage. Throws an exception if couldn't update. bool update(const UUID & id, const UpdateFunc & update_func, bool throw_if_not_exists = true); diff --git a/src/Access/LDAPAccessStorage.cpp b/src/Access/LDAPAccessStorage.cpp index f6435b0d899..b000415ca58 100644 --- a/src/Access/LDAPAccessStorage.cpp +++ b/src/Access/LDAPAccessStorage.cpp @@ -163,7 +163,7 @@ void LDAPAccessStorage::applyRoleChangeNoLock(bool grant, const UUID & role_id, // Update the granted roles of the relevant users. if (!user_ids.empty()) { - auto update_func = [&role_id, &grant] (const AccessEntityPtr & entity_) -> AccessEntityPtr + auto update_func = [&role_id, &grant] (const AccessEntityPtr & entity_, const UUID &) -> AccessEntityPtr { if (auto user = typeid_cast>(entity_)) { @@ -301,7 +301,7 @@ void LDAPAccessStorage::updateAssignedRolesNoLock(const UUID & id, const String if (it != external_role_hashes.end() && it->second == external_roles_hash) return; - auto update_func = [this, &external_roles, external_roles_hash] (const AccessEntityPtr & entity_) -> AccessEntityPtr + auto update_func = [this, &external_roles, external_roles_hash] (const AccessEntityPtr & entity_, const UUID &) -> AccessEntityPtr { if (auto user = typeid_cast>(entity_)) { diff --git a/src/Access/MemoryAccessStorage.cpp b/src/Access/MemoryAccessStorage.cpp index 3b5a987fc6e..e1743cdd6a4 100644 --- a/src/Access/MemoryAccessStorage.cpp +++ b/src/Access/MemoryAccessStorage.cpp @@ -204,7 +204,7 @@ bool MemoryAccessStorage::updateNoLock(const UUID & id, const UpdateFunc & updat Entry & entry = it->second; auto old_entity = entry.entity; - auto new_entity = update_func(old_entity); + auto new_entity = update_func(old_entity, id); if (!new_entity->isTypeOf(old_entity->getType())) throwBadCast(id, new_entity->getType(), new_entity->getName(), old_entity->getType()); diff --git a/src/Access/MultipleAccessStorage.cpp b/src/Access/MultipleAccessStorage.cpp index b722e8bd4c8..d51a7e00511 100644 --- a/src/Access/MultipleAccessStorage.cpp +++ b/src/Access/MultipleAccessStorage.cpp @@ -416,7 +416,7 @@ bool MultipleAccessStorage::updateImpl(const UUID & id, const UpdateFunc & updat { if (auto old_entity = storage_for_updating->tryRead(id)) { - auto new_entity = update_func(old_entity); + auto new_entity = update_func(old_entity, id); if (new_entity->getName() != old_entity->getName()) { for (const auto & storage : *storages) diff --git a/src/Access/ReplicatedAccessStorage.cpp b/src/Access/ReplicatedAccessStorage.cpp index 5f498851edb..f0cb8a5c9f0 100644 --- a/src/Access/ReplicatedAccessStorage.cpp +++ b/src/Access/ReplicatedAccessStorage.cpp @@ -355,7 +355,7 @@ bool ReplicatedAccessStorage::updateZooKeeper(const zkutil::ZooKeeperPtr & zooke } const AccessEntityPtr old_entity = deserializeAccessEntity(old_entity_definition, entity_path); - const AccessEntityPtr new_entity = update_func(old_entity); + const AccessEntityPtr new_entity = update_func(old_entity, id); if (!new_entity->isTypeOf(old_entity->getType())) throwBadCast(id, new_entity->getType(), new_entity->getName(), old_entity->getType()); diff --git a/src/Interpreters/Access/InterpreterCreateQuotaQuery.cpp b/src/Interpreters/Access/InterpreterCreateQuotaQuery.cpp index 56608644425..fe5837f31dd 100644 --- a/src/Interpreters/Access/InterpreterCreateQuotaQuery.cpp +++ b/src/Interpreters/Access/InterpreterCreateQuotaQuery.cpp @@ -111,7 +111,7 @@ BlockIO InterpreterCreateQuotaQuery::execute() if (query.alter) { - auto update_func = [&](const AccessEntityPtr & entity) -> AccessEntityPtr + auto update_func = [&](const AccessEntityPtr & entity, const UUID &) -> AccessEntityPtr { auto updated_quota = typeid_cast>(entity->clone()); updateQuotaFromQueryImpl(*updated_quota, query, {}, roles_from_query); diff --git a/src/Interpreters/Access/InterpreterCreateRoleQuery.cpp b/src/Interpreters/Access/InterpreterCreateRoleQuery.cpp index 4936bd15262..4c084309e37 100644 --- a/src/Interpreters/Access/InterpreterCreateRoleQuery.cpp +++ b/src/Interpreters/Access/InterpreterCreateRoleQuery.cpp @@ -74,7 +74,7 @@ BlockIO InterpreterCreateRoleQuery::execute() if (query.alter) { - auto update_func = [&](const AccessEntityPtr & entity) -> AccessEntityPtr + auto update_func = [&](const AccessEntityPtr & entity, const UUID &) -> AccessEntityPtr { auto updated_role = typeid_cast>(entity->clone()); updateRoleFromQueryImpl(*updated_role, query, {}, settings_from_query); diff --git a/src/Interpreters/Access/InterpreterCreateRowPolicyQuery.cpp b/src/Interpreters/Access/InterpreterCreateRowPolicyQuery.cpp index b48c3880c59..8790d38b8d9 100644 --- a/src/Interpreters/Access/InterpreterCreateRowPolicyQuery.cpp +++ b/src/Interpreters/Access/InterpreterCreateRowPolicyQuery.cpp @@ -88,7 +88,7 @@ BlockIO InterpreterCreateRowPolicyQuery::execute() Strings names = query.names->toStrings(); if (query.alter) { - auto update_func = [&](const AccessEntityPtr & entity) -> AccessEntityPtr + auto update_func = [&](const AccessEntityPtr & entity, const UUID &) -> AccessEntityPtr { auto updated_policy = typeid_cast>(entity->clone()); updateRowPolicyFromQueryImpl(*updated_policy, query, {}, roles_from_query); diff --git a/src/Interpreters/Access/InterpreterCreateSettingsProfileQuery.cpp b/src/Interpreters/Access/InterpreterCreateSettingsProfileQuery.cpp index 029deff9b22..aadc00d3e6f 100644 --- a/src/Interpreters/Access/InterpreterCreateSettingsProfileQuery.cpp +++ b/src/Interpreters/Access/InterpreterCreateSettingsProfileQuery.cpp @@ -90,7 +90,7 @@ BlockIO InterpreterCreateSettingsProfileQuery::execute() if (query.alter) { - auto update_func = [&](const AccessEntityPtr & entity) -> AccessEntityPtr + auto update_func = [&](const AccessEntityPtr & entity, const UUID &) -> AccessEntityPtr { auto updated_profile = typeid_cast>(entity->clone()); updateSettingsProfileFromQueryImpl(*updated_profile, query, {}, settings_from_query, roles_from_query); diff --git a/src/Interpreters/Access/InterpreterCreateUserQuery.cpp b/src/Interpreters/Access/InterpreterCreateUserQuery.cpp index 81600b2b6eb..adb4bc2283a 100644 --- a/src/Interpreters/Access/InterpreterCreateUserQuery.cpp +++ b/src/Interpreters/Access/InterpreterCreateUserQuery.cpp @@ -264,7 +264,7 @@ BlockIO InterpreterCreateUserQuery::execute() if (query.grantees) grantees_from_query = RolesOrUsersSet{*query.grantees, access_control}; - auto update_func = [&](const AccessEntityPtr & entity) -> AccessEntityPtr + auto update_func = [&](const AccessEntityPtr & entity, const UUID &) -> AccessEntityPtr { auto updated_user = typeid_cast>(entity->clone()); updateUserFromQueryImpl( @@ -317,7 +317,7 @@ BlockIO InterpreterCreateUserQuery::execute() if (query.grantees) { RolesOrUsersSet grantees_from_query = RolesOrUsersSet{*query.grantees, access_control}; - access_control.update(ids, [&](const AccessEntityPtr & entity) -> AccessEntityPtr + access_control.update(ids, [&](const AccessEntityPtr & entity, const UUID &) -> AccessEntityPtr { auto updated_user = typeid_cast>(entity->clone()); updated_user->grantees = grantees_from_query; diff --git a/src/Interpreters/Access/InterpreterGrantQuery.cpp b/src/Interpreters/Access/InterpreterGrantQuery.cpp index ac3b549a576..99d574dba1e 100644 --- a/src/Interpreters/Access/InterpreterGrantQuery.cpp +++ b/src/Interpreters/Access/InterpreterGrantQuery.cpp @@ -474,7 +474,7 @@ BlockIO InterpreterGrantQuery::execute() calculateCurrentGrantRightsWithIntersection(new_rights, current_user_access, elements_to_grant); /// Update roles and users listed in `grantees`. - auto update_func = [&](const AccessEntityPtr & entity) -> AccessEntityPtr + auto update_func = [&](const AccessEntityPtr & entity, const UUID &) -> AccessEntityPtr { auto clone = entity->clone(); if (query.current_grants) diff --git a/src/Interpreters/Access/InterpreterSetRoleQuery.cpp b/src/Interpreters/Access/InterpreterSetRoleQuery.cpp index 99a7a73d46c..78926c3a0c9 100644 --- a/src/Interpreters/Access/InterpreterSetRoleQuery.cpp +++ b/src/Interpreters/Access/InterpreterSetRoleQuery.cpp @@ -46,7 +46,7 @@ void InterpreterSetRoleQuery::setDefaultRole(const ASTSetRoleQuery & query) std::vector to_users = RolesOrUsersSet{*query.to_users, access_control, getContext()->getUserID()}.getMatchingIDs(access_control); RolesOrUsersSet roles_from_query{*query.roles, access_control}; - auto update_func = [&](const AccessEntityPtr & entity) -> AccessEntityPtr + auto update_func = [&](const AccessEntityPtr & entity, const UUID &) -> AccessEntityPtr { auto updated_user = typeid_cast>(entity->clone()); updateUserSetDefaultRoles(*updated_user, roles_from_query); From 6a96710c048ade3949faed671174843455ba8d4e Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Wed, 18 Sep 2024 13:12:56 +0200 Subject: [PATCH 06/13] Add new restore setting "update_access_entities_dependents" to allow updating dependents of restored access entities. --- src/Access/AccessBackup.cpp | 83 ++++++++++++++++++- src/Access/AccessBackup.h | 15 ++++ src/Access/GrantedRoles.cpp | 23 +++++ src/Access/GrantedRoles.h | 1 + src/Access/IAccessEntity.h | 1 + src/Access/IAccessStorage.cpp | 3 +- src/Access/Quota.cpp | 8 ++ src/Access/Quota.h | 1 + src/Access/Role.cpp | 9 ++ src/Access/Role.h | 1 + src/Access/RolesOrUsersSet.cpp | 18 ++++ src/Access/RolesOrUsersSet.h | 1 + src/Access/RowPolicy.cpp | 8 ++ src/Access/RowPolicy.h | 1 + src/Access/SettingsProfile.cpp | 9 ++ src/Access/SettingsProfile.h | 1 + src/Access/SettingsProfileElement.cpp | 16 ++++ src/Access/SettingsProfileElement.h | 1 + src/Access/User.cpp | 11 +++ src/Access/User.h | 1 + src/Backups/RestoreSettings.cpp | 1 + src/Backups/RestoreSettings.h | 10 +++ src/Backups/RestorerFromBackup.cpp | 9 ++ src/Backups/RestorerFromBackup.h | 1 + ..._restore_user_with_existing_role.reference | 1 + .../03231_restore_user_with_existing_role.sh | 12 +-- 26 files changed, 236 insertions(+), 10 deletions(-) diff --git a/src/Access/AccessBackup.cpp b/src/Access/AccessBackup.cpp index 96b9eb5c91d..85e99e611c6 100644 --- a/src/Access/AccessBackup.cpp +++ b/src/Access/AccessBackup.cpp @@ -200,6 +200,7 @@ AccessRestorerFromBackup::AccessRestorerFromBackup( : backup(backup_) , creation_mode(restore_settings_.create_access) , skip_unresolved_dependencies(restore_settings_.allow_unresolved_access_dependencies) + , update_dependents(restore_settings_.update_access_entities_dependents) , log(getLogger("AccessRestorerFromBackup")) { } @@ -416,21 +417,35 @@ void AccessRestorerFromBackup::generateRandomIDsAndResolveDependencies(const Acc } /// Prepare map from old UUIDs to new UUIDs. + std::unordered_set ids_to_restore; std::unordered_map old_to_new_ids; std::unordered_set unresolved_ids; for (const auto & [id, entity_info] : entity_infos) { + if (entity_info.restore) + ids_to_restore.insert(id); + if (entity_info.new_id) old_to_new_ids[id] = *entity_info.new_id; else unresolved_ids.insert(id); } + /// Calculate `is_dependent` for each entity info. + if (update_dependents) + { + for (auto & [id, entity_info] : entity_infos) + { + if (!entity_info.restore && entity_info.new_id && entity_info.entity && entity_info.entity->hasDependencies(ids_to_restore)) + entity_info.is_dependent = true; + } + } + /// Remap the UUIDs of dependencies in the access entities we're going to restore. for (auto & [id, entity_info] : entity_infos) { - if (entity_info.restore) + if (entity_info.restore || entity_info.is_dependent) { auto new_entity = entity_info.entity->clone(); new_entity->replaceDependencies(old_to_new_ids); @@ -466,10 +481,31 @@ std::vector> AccessRestorerFromBackup::getEntit return res; } +std::vector> AccessRestorerFromBackup::getDependents(const String & data_path_in_backup) const +{ + if (!ids_assigned) + throw Exception(ErrorCodes::LOGICAL_ERROR, "IDs not assigned"); + + if (data_path_in_backup != data_path_with_entities_to_restore) + return {}; + + std::vector> res; + res.reserve(entity_infos.size()); + + for (const auto & [id, entity_info] : entity_infos) + { + if (entity_info.is_dependent) + res.emplace_back(*entity_info.new_id, entity_info.entity); + } + + return res; +} + void restoreAccessEntitiesFromBackup( IAccessStorage & destination_access_storage, const std::vector> & entities, + const std::vector> & dependents, const RestoreSettings & restore_settings) { if (entities.empty()) @@ -479,9 +515,12 @@ void restoreAccessEntitiesFromBackup( bool replace_if_exists = (restore_settings.create_access == RestoreAccessCreationMode::kReplace); bool throw_if_exists = (restore_settings.create_access == RestoreAccessCreationMode::kCreate); + bool update_dependents = restore_settings.update_access_entities_dependents; std::unordered_set restored_ids; std::unordered_map new_to_existing_ids; + std::vector> more_dependents; + more_dependents.reserve(entities.size()); for (const auto & [id, entity] : entities) { @@ -500,6 +539,8 @@ void restoreAccessEntitiesFromBackup( /// Couldn't insert `entity` because there is an existing entity with the same name. LOG_TRACE(log, "{}: Not added because already exists with UUID {}", AccessEntityTypeInfo::get(type).formatEntityNameWithType(name), existing_id); new_to_existing_ids[id] = existing_id; + if (update_dependents) + more_dependents.emplace_back(existing_id, entity); } } @@ -527,6 +568,46 @@ void restoreAccessEntitiesFromBackup( /// It's totally ok if some UUIDs from `ids_to_update` don't exist anymore, that's why we use tryUpdate() here. destination_access_storage.tryUpdate(ids_to_update, update_func); } + + auto do_update_dependents = [&](const std::vector> & dependents_to_update) + { + if (dependents_to_update.empty()) + return; + + std::vector ids_to_update; + ids_to_update.reserve(dependents_to_update.size()); + std::unordered_map id_to_source; + + for (const auto & [id, source] : dependents_to_update) + { + if (!destination_access_storage.isReadOnly(id)) + { + ids_to_update.emplace_back(id); + auto new_source = source->clone(); + new_source->replaceDependencies(new_to_existing_ids); + id_to_source[id] = new_source; + } + } + + /// If new entities restored from backup have dependencies on other entities from backup which were not restored because they existed, + /// then we should correct those dependencies. + auto update_func = [&](const AccessEntityPtr & entity, const UUID & id) -> AccessEntityPtr + { + const auto & source = *id_to_source.at(id); + if (!source.hasDependencies(restored_ids)) + return entity; + LOG_TRACE(log, "{}: Updating dependent", entity->formatTypeWithName()); + auto res = entity->clone(); + res->copyDependenciesFrom(source, restored_ids); + return res; + }; + + /// It's totally ok if some UUIDs from `ids_to_update` don't exist anymore, that's why we use tryUpdate() here. + destination_access_storage.tryUpdate(ids_to_update, update_func); + }; + + do_update_dependents(dependents); + do_update_dependents(more_dependents); } } diff --git a/src/Access/AccessBackup.h b/src/Access/AccessBackup.h index 84dcf74a26e..5bceda5e8c8 100644 --- a/src/Access/AccessBackup.h +++ b/src/Access/AccessBackup.h @@ -32,6 +32,7 @@ std::pair makeBackupEntryForAccess( void restoreAccessEntitiesFromBackup( IAccessStorage & access_storage, const std::vector> & entities, + const std::vector> & dependents, const RestoreSettings & restore_settings); @@ -60,10 +61,18 @@ public: /// Both functions loadFromBackup() and generateRandomIDsAndResolveDependencies() must be called before that. std::vector> getEntities(const String & data_path_in_backup) const; + /// Returns dependents of the access entities we're going to restore. + /// Dependents are access entities which exist already and they should be updated after restoring. + /// For example, if there were a role granted to a user: `CREATE USER user1; CREATE ROLE role1; GRANT role1 TO user1`, + /// and we're restoring only role `role1` because user `user1` already exists, + /// then user `user1` should be modified after restoring role `role1` to add this grant `GRANT role1 TO user1`. + std::vector> getDependents(const String & data_path_in_backup) const; + private: const BackupPtr backup; const RestoreAccessCreationMode creation_mode; const bool skip_unresolved_dependencies; + const bool update_dependents; const LoggerPtr log; /// Whether loadFromBackup() finished. @@ -99,6 +108,12 @@ private: /// Flags `restore` and `is_dependency` both can be set at the same time. bool is_dependency = false; + /// Whether this entity info is a dependent of another entity which we're going to restore. + /// For example, if we're going to restore role `role1` and there is also the following user stored in the backup: + /// `CREATE USER user1 DEFAULT ROLE role1`, then `is_dependent=true` for `user1`. + /// This flags is set by generateRandomIDsAndResolveDependencies(). + bool is_dependent = false; + /// New UUID for this entity - either randomly generated or copied from an existing entity. /// This UUID is assigned by generateRandomIDsAndResolveDependencies(). std::optional new_id = std::nullopt; diff --git a/src/Access/GrantedRoles.cpp b/src/Access/GrantedRoles.cpp index f1e2116dfa8..76e77599492 100644 --- a/src/Access/GrantedRoles.cpp +++ b/src/Access/GrantedRoles.cpp @@ -231,6 +231,29 @@ void GrantedRoles::replaceDependencies(const std::unordered_map & ol } } +void GrantedRoles::copyDependenciesFrom(const GrantedRoles & src, const std::unordered_set & ids) +{ + bool found = false; + + for (const auto & role_id : src.roles) + { + if (ids.contains(role_id)) + { + roles.emplace(role_id); + found = true; + } + } + + if (found) + { + for (const auto & role_id : src.roles_with_admin_option) + { + if (ids.contains(role_id)) + roles_with_admin_option.emplace(role_id); + } + } +} + void GrantedRoles::removeDependencies(const std::unordered_set & ids) { bool found = false; diff --git a/src/Access/GrantedRoles.h b/src/Access/GrantedRoles.h index 9a128d3589b..d92af0f6db1 100644 --- a/src/Access/GrantedRoles.h +++ b/src/Access/GrantedRoles.h @@ -60,6 +60,7 @@ public: std::vector findDependencies() const; bool hasDependencies(const std::unordered_set & ids) const; void replaceDependencies(const std::unordered_map & old_to_new_ids); + void copyDependenciesFrom(const GrantedRoles & src, const std::unordered_set & ids); void removeDependencies(const std::unordered_set & ids); private: diff --git a/src/Access/IAccessEntity.h b/src/Access/IAccessEntity.h index 932f8c71b2d..40a58bec3eb 100644 --- a/src/Access/IAccessEntity.h +++ b/src/Access/IAccessEntity.h @@ -52,6 +52,7 @@ struct IAccessEntity /// Replaces dependencies according to a specified map. virtual void replaceDependencies(const std::unordered_map & /* old_to_new_ids */) {} + virtual void copyDependenciesFrom(const IAccessEntity & /* src */, const std::unordered_set & /* ids */) {} virtual void removeDependencies(const std::unordered_set & /* ids */) {} /// Whether this access entity should be written to a backup. diff --git a/src/Access/IAccessStorage.cpp b/src/Access/IAccessStorage.cpp index e560619eacc..c9400c6c06c 100644 --- a/src/Access/IAccessStorage.cpp +++ b/src/Access/IAccessStorage.cpp @@ -653,8 +653,9 @@ void IAccessStorage::restoreFromBackup(RestorerFromBackup & restorer, const Stri [this, &restorer, data_path_in_backup] { auto entities = restorer.getAccessEntitiesToRestore(data_path_in_backup); + auto dependents = restorer.getAccessEntitiesToRestoreDependents(data_path_in_backup); const auto & restore_settings = restorer.getRestoreSettings(); - restoreAccessEntitiesFromBackup(*this, entities, restore_settings); + restoreAccessEntitiesFromBackup(*this, entities, dependents, restore_settings); }); } diff --git a/src/Access/Quota.cpp b/src/Access/Quota.cpp index 1b80b109a35..57aa1446d8c 100644 --- a/src/Access/Quota.cpp +++ b/src/Access/Quota.cpp @@ -34,6 +34,14 @@ void Quota::replaceDependencies(const std::unordered_map & old_to_ne to_roles.replaceDependencies(old_to_new_ids); } +void Quota::copyDependenciesFrom(const IAccessEntity & src, const std::unordered_set & ids) +{ + if (getType() != src.getType()) + return; + const auto & src_quota = typeid_cast(src); + to_roles.copyDependenciesFrom(src_quota.to_roles, ids); +} + void Quota::removeDependencies(const std::unordered_set & ids) { to_roles.removeDependencies(ids); diff --git a/src/Access/Quota.h b/src/Access/Quota.h index 9e5e787d54d..3becb82461d 100644 --- a/src/Access/Quota.h +++ b/src/Access/Quota.h @@ -49,6 +49,7 @@ struct Quota : public IAccessEntity std::vector findDependencies() const override; bool hasDependencies(const std::unordered_set & ids) const override; void replaceDependencies(const std::unordered_map & old_to_new_ids) override; + void copyDependenciesFrom(const IAccessEntity & src, const std::unordered_set & ids) override; void removeDependencies(const std::unordered_set & ids) override; bool isBackupAllowed() const override { return true; } diff --git a/src/Access/Role.cpp b/src/Access/Role.cpp index 340918ec24a..d658467a8ac 100644 --- a/src/Access/Role.cpp +++ b/src/Access/Role.cpp @@ -32,6 +32,15 @@ void Role::replaceDependencies(const std::unordered_map & old_to_new settings.replaceDependencies(old_to_new_ids); } +void Role::copyDependenciesFrom(const IAccessEntity & src, const std::unordered_set & ids) +{ + if (getType() != src.getType()) + return; + const auto & src_role = typeid_cast(src); + granted_roles.copyDependenciesFrom(src_role.granted_roles, ids); + settings.copyDependenciesFrom(src_role.settings, ids); +} + void Role::removeDependencies(const std::unordered_set & ids) { granted_roles.removeDependencies(ids); diff --git a/src/Access/Role.h b/src/Access/Role.h index e6d0cddf4b9..3bce167b33b 100644 --- a/src/Access/Role.h +++ b/src/Access/Role.h @@ -23,6 +23,7 @@ struct Role : public IAccessEntity std::vector findDependencies() const override; bool hasDependencies(const std::unordered_set & ids) const override; void replaceDependencies(const std::unordered_map & old_to_new_ids) override; + void copyDependenciesFrom(const IAccessEntity & src, const std::unordered_set & ids) override; void removeDependencies(const std::unordered_set & ids) override; bool isBackupAllowed() const override { return settings.isBackupAllowed(); } diff --git a/src/Access/RolesOrUsersSet.cpp b/src/Access/RolesOrUsersSet.cpp index 8aa840738f1..3568b60e44d 100644 --- a/src/Access/RolesOrUsersSet.cpp +++ b/src/Access/RolesOrUsersSet.cpp @@ -354,6 +354,24 @@ void RolesOrUsersSet::replaceDependencies(const std::unordered_map & boost::range::copy(new_ids, std::inserter(except_ids, except_ids.end())); } +void RolesOrUsersSet::copyDependenciesFrom(const RolesOrUsersSet & src, const std::unordered_set & dependencies_ids) +{ + if (all != src.all) + return; + + for (const auto & id : src.ids) + { + if (dependencies_ids.contains(id)) + ids.emplace(id); + } + + for (const auto & id : src.except_ids) + { + if (dependencies_ids.contains(id)) + except_ids.emplace(id); + } +} + void RolesOrUsersSet::removeDependencies(const std::unordered_set & dependencies_ids) { for (auto it = ids.begin(); it != ids.end();) diff --git a/src/Access/RolesOrUsersSet.h b/src/Access/RolesOrUsersSet.h index f3d82c81682..93d94708657 100644 --- a/src/Access/RolesOrUsersSet.h +++ b/src/Access/RolesOrUsersSet.h @@ -66,6 +66,7 @@ struct RolesOrUsersSet std::vector findDependencies() const; bool hasDependencies(const std::unordered_set & dependencies_ids) const; void replaceDependencies(const std::unordered_map & old_to_new_ids); + void copyDependenciesFrom(const RolesOrUsersSet & src, const std::unordered_set & dependencies_ids); void removeDependencies(const std::unordered_set & dependencies_ids); bool all = false; diff --git a/src/Access/RowPolicy.cpp b/src/Access/RowPolicy.cpp index 427110add4c..c26c59c6ea4 100644 --- a/src/Access/RowPolicy.cpp +++ b/src/Access/RowPolicy.cpp @@ -73,6 +73,14 @@ void RowPolicy::replaceDependencies(const std::unordered_map & old_t to_roles.replaceDependencies(old_to_new_ids); } +void RowPolicy::copyDependenciesFrom(const IAccessEntity & src, const std::unordered_set & ids) +{ + if (getType() != src.getType()) + return; + const auto & src_policy = typeid_cast(src); + to_roles.copyDependenciesFrom(src_policy.to_roles, ids); +} + void RowPolicy::removeDependencies(const std::unordered_set & ids) { to_roles.removeDependencies(ids); diff --git a/src/Access/RowPolicy.h b/src/Access/RowPolicy.h index 4fd6abc67e4..16234720469 100644 --- a/src/Access/RowPolicy.h +++ b/src/Access/RowPolicy.h @@ -52,6 +52,7 @@ struct RowPolicy : public IAccessEntity std::vector findDependencies() const override; bool hasDependencies(const std::unordered_set & ids) const override; void replaceDependencies(const std::unordered_map & old_to_new_ids) override; + void copyDependenciesFrom(const IAccessEntity & src, const std::unordered_set & ids) override; void removeDependencies(const std::unordered_set & ids) override; bool isBackupAllowed() const override { return true; } diff --git a/src/Access/SettingsProfile.cpp b/src/Access/SettingsProfile.cpp index 8206bf83c23..24c2604e1ae 100644 --- a/src/Access/SettingsProfile.cpp +++ b/src/Access/SettingsProfile.cpp @@ -32,6 +32,15 @@ void SettingsProfile::replaceDependencies(const std::unordered_map & to_roles.replaceDependencies(old_to_new_ids); } +void SettingsProfile::copyDependenciesFrom(const IAccessEntity & src, const std::unordered_set & ids) +{ + if (getType() != src.getType()) + return; + const auto & src_profile = typeid_cast(src); + elements.copyDependenciesFrom(src_profile.elements, ids); + to_roles.copyDependenciesFrom(src_profile.to_roles, ids); +} + void SettingsProfile::removeDependencies(const std::unordered_set & ids) { elements.removeDependencies(ids); diff --git a/src/Access/SettingsProfile.h b/src/Access/SettingsProfile.h index 92e9b3222f9..df5c327c793 100644 --- a/src/Access/SettingsProfile.h +++ b/src/Access/SettingsProfile.h @@ -24,6 +24,7 @@ struct SettingsProfile : public IAccessEntity std::vector findDependencies() const override; bool hasDependencies(const std::unordered_set & ids) const override; void replaceDependencies(const std::unordered_map & old_to_new_ids) override; + void copyDependenciesFrom(const IAccessEntity & src, const std::unordered_set & ids) override; void removeDependencies(const std::unordered_set & ids) override; bool isBackupAllowed() const override { return elements.isBackupAllowed(); } diff --git a/src/Access/SettingsProfileElement.cpp b/src/Access/SettingsProfileElement.cpp index 17967ed0bc0..adf323bb55d 100644 --- a/src/Access/SettingsProfileElement.cpp +++ b/src/Access/SettingsProfileElement.cpp @@ -188,6 +188,22 @@ void SettingsProfileElements::replaceDependencies(const std::unordered_map & ids) +{ + SettingsProfileElements new_elements; + for (const auto & element : src) + { + if (element.parent_profile && ids.contains(*element.parent_profile)) + { + SettingsProfileElement new_element; + new_element.parent_profile = *element.parent_profile; + new_elements.emplace_back(new_element); + } + } + insert(begin(), new_elements.begin(), new_elements.end()); +} + + void SettingsProfileElements::removeDependencies(const std::unordered_set & ids) { std::erase_if( diff --git a/src/Access/SettingsProfileElement.h b/src/Access/SettingsProfileElement.h index 000ae2784df..3c816b563af 100644 --- a/src/Access/SettingsProfileElement.h +++ b/src/Access/SettingsProfileElement.h @@ -65,6 +65,7 @@ public: std::vector findDependencies() const; bool hasDependencies(const std::unordered_set & ids) const; void replaceDependencies(const std::unordered_map & old_to_new_ids); + void copyDependenciesFrom(const SettingsProfileElements & src, const std::unordered_set & ids); void removeDependencies(const std::unordered_set & ids); void merge(const SettingsProfileElements & other); diff --git a/src/Access/User.cpp b/src/Access/User.cpp index 18f5ea8e124..880864d21ee 100644 --- a/src/Access/User.cpp +++ b/src/Access/User.cpp @@ -62,6 +62,17 @@ void User::replaceDependencies(const std::unordered_map & old_to_new settings.replaceDependencies(old_to_new_ids); } +void User::copyDependenciesFrom(const IAccessEntity & src, const std::unordered_set & ids) +{ + if (getType() != src.getType()) + return; + const auto & src_user = typeid_cast(src); + default_roles.copyDependenciesFrom(src_user.default_roles, ids); + granted_roles.copyDependenciesFrom(src_user.granted_roles, ids); + grantees.copyDependenciesFrom(src_user.grantees, ids); + settings.copyDependenciesFrom(src_user.settings, ids); +} + void User::removeDependencies(const std::unordered_set & ids) { default_roles.removeDependencies(ids); diff --git a/src/Access/User.h b/src/Access/User.h index 7d7ec5362bd..fa2331f3d02 100644 --- a/src/Access/User.h +++ b/src/Access/User.h @@ -34,6 +34,7 @@ struct User : public IAccessEntity std::vector findDependencies() const override; bool hasDependencies(const std::unordered_set & ids) const override; void replaceDependencies(const std::unordered_map & old_to_new_ids) override; + void copyDependenciesFrom(const IAccessEntity & src, const std::unordered_set & ids) override; void removeDependencies(const std::unordered_set & ids) override; bool isBackupAllowed() const override { return settings.isBackupAllowed(); } diff --git a/src/Backups/RestoreSettings.cpp b/src/Backups/RestoreSettings.cpp index a18c3abe53b..7383da699e9 100644 --- a/src/Backups/RestoreSettings.cpp +++ b/src/Backups/RestoreSettings.cpp @@ -161,6 +161,7 @@ namespace M(Bool, allow_non_empty_tables) \ M(RestoreAccessCreationMode, create_access) \ M(Bool, allow_unresolved_access_dependencies) \ + M(Bool, update_access_entities_dependents) \ M(RestoreUDFCreationMode, create_function) \ M(Bool, allow_s3_native_copy) \ M(Bool, use_same_s3_credentials_for_base_backup) \ diff --git a/src/Backups/RestoreSettings.h b/src/Backups/RestoreSettings.h index 726a33b07db..1cab081b20d 100644 --- a/src/Backups/RestoreSettings.h +++ b/src/Backups/RestoreSettings.h @@ -110,6 +110,16 @@ struct RestoreSettings /// If this flag is false then RESTORE will throw an exception in that case. bool allow_unresolved_access_dependencies = false; + /// Try to update dependents of restored access entities. + /// For example: if a backup contains a profile assigned to a user: `CREATE PROFILE p1; CREATE USER u1 SETTINGS PROFILE p1` + /// and now we're restoring only profile `p1` and user `u1` already exists, then + /// this flag is whether restored profile `p1` should be assigned to user `u1` again. + /// Another example, if a backup contains a role granted to a user: `CREATE USER u2; CREATE ROLE r2; GRANT r2 TO u2` + /// and now we're restoring only role `r2` and user `u2` already exists, then + /// this flag is whether restored role `r2` should be granted to user `u2` again. + /// If this flag is false then RESTORE won't update existing access entities. + bool update_access_entities_dependents = false; + /// How the RESTORE command will handle if a user-defined function which it's going to restore already exists. RestoreUDFCreationMode create_function = RestoreUDFCreationMode::kCreateIfNotExists; diff --git a/src/Backups/RestorerFromBackup.cpp b/src/Backups/RestorerFromBackup.cpp index 43b1fd935f4..44da20a23eb 100644 --- a/src/Backups/RestorerFromBackup.cpp +++ b/src/Backups/RestorerFromBackup.cpp @@ -721,6 +721,15 @@ std::vector> RestorerFromBackup::getAccessEntit return access_restorer->getEntities(data_path_in_backup); } +std::vector> RestorerFromBackup::getAccessEntitiesToRestoreDependents(const String & data_path_in_backup) const +{ + std::lock_guard lock{mutex}; + if (!access_restorer) + return {}; + access_restorer->generateRandomIDsAndResolveDependencies(context->getAccessControl()); + return access_restorer->getDependents(data_path_in_backup); +} + void RestorerFromBackup::createDatabases() { Strings database_names; diff --git a/src/Backups/RestorerFromBackup.h b/src/Backups/RestorerFromBackup.h index 2318e75e377..dd082b555db 100644 --- a/src/Backups/RestorerFromBackup.h +++ b/src/Backups/RestorerFromBackup.h @@ -69,6 +69,7 @@ public: /// Returns the list of access entities to restore. std::vector> getAccessEntitiesToRestore(const String & data_path_in_backup) const; + std::vector> getAccessEntitiesToRestoreDependents(const String & data_path_in_backup) const; /// Throws an exception that a specified table is already non-empty. [[noreturn]] static void throwTableIsNotEmpty(const StorageID & storage_id); diff --git a/tests/queries/0_stateless/03231_restore_user_with_existing_role.reference b/tests/queries/0_stateless/03231_restore_user_with_existing_role.reference index cad1bf13574..6615c5844af 100644 --- a/tests/queries/0_stateless/03231_restore_user_with_existing_role.reference +++ b/tests/queries/0_stateless/03231_restore_user_with_existing_role.reference @@ -1,5 +1,6 @@ Everything dropped User dropped +Role dropped Nothing dropped Nothing dropped, mode=replace Nothing dropped, mode=create diff --git a/tests/queries/0_stateless/03231_restore_user_with_existing_role.sh b/tests/queries/0_stateless/03231_restore_user_with_existing_role.sh index 04f907b719d..08a70d5e71a 100755 --- a/tests/queries/0_stateless/03231_restore_user_with_existing_role.sh +++ b/tests/queries/0_stateless/03231_restore_user_with_existing_role.sh @@ -55,14 +55,10 @@ ${CLICKHOUSE_CLIENT} --query "DROP USER ${user_a}" ${CLICKHOUSE_CLIENT} --query "RESTORE ALL FROM ${backup_name} FORMAT Null" do_check -# TODO: Cannot restore a dropped role granted to an existing user. The result after RESTORE ALL below is the following: -# CREATE USER user_a DEFAULT ROLE NONE SETTINGS custom_x = 2; GRANT NONE TO user_a; CREATE ROLE role_b SETTINGS custom_x = 1 -# because `role_b` is restored but not granted to existing user `user_a`. -# -# echo "Role dropped" -# ${CLICKHOUSE_CLIENT} --query "DROP ROLE ${role_b}" -# ${CLICKHOUSE_CLIENT} --query "RESTORE ALL FROM ${backup_name} FORMAT Null" -# do_check +echo "Role dropped" +${CLICKHOUSE_CLIENT} --query "DROP ROLE ${role_b}" +${CLICKHOUSE_CLIENT} --query "RESTORE ALL FROM ${backup_name} SETTINGS update_access_entities_dependents=true FORMAT Null" +do_check echo "Nothing dropped" ${CLICKHOUSE_CLIENT} --query "RESTORE ALL FROM ${backup_name} FORMAT Null" From 11ef73f779f8cd2be0564a31dd9965098fd58049 Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Wed, 18 Sep 2024 13:12:38 +0200 Subject: [PATCH 07/13] Add more tests. --- ..._restore_user_with_existing_role.reference | 3 +++ .../03231_restore_user_with_existing_role.sh | 23 +++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/tests/queries/0_stateless/03231_restore_user_with_existing_role.reference b/tests/queries/0_stateless/03231_restore_user_with_existing_role.reference index 6615c5844af..834c2b71833 100644 --- a/tests/queries/0_stateless/03231_restore_user_with_existing_role.reference +++ b/tests/queries/0_stateless/03231_restore_user_with_existing_role.reference @@ -5,3 +5,6 @@ Nothing dropped Nothing dropped, mode=replace Nothing dropped, mode=create ACCESS_ENTITY_ALREADY_EXISTS +Everything dropped, restore system.roles, then system.users +user_a 0 +role_b 1 diff --git a/tests/queries/0_stateless/03231_restore_user_with_existing_role.sh b/tests/queries/0_stateless/03231_restore_user_with_existing_role.sh index 08a70d5e71a..7c5fb342955 100755 --- a/tests/queries/0_stateless/03231_restore_user_with_existing_role.sh +++ b/tests/queries/0_stateless/03231_restore_user_with_existing_role.sh @@ -71,3 +71,26 @@ do_check echo "Nothing dropped, mode=create" ${CLICKHOUSE_CLIENT} --query "RESTORE ALL FROM ${backup_name} SETTINGS create_access='create' FORMAT Null" 2>&1 | grep -om1 "ACCESS_ENTITY_ALREADY_EXISTS" do_check + +echo "Everything dropped, restore system.roles, then system.users" +${CLICKHOUSE_CLIENT} --query "DROP USER ${user_a}" +${CLICKHOUSE_CLIENT} --query "DROP ROLE ${role_b}" +${CLICKHOUSE_CLIENT} --query "RESTORE TABLE system.roles FROM ${backup_name} FORMAT Null" +${CLICKHOUSE_CLIENT} --query "SELECT 'user_a', count() FROM system.users WHERE name = '${user_a}'" +${CLICKHOUSE_CLIENT} --query "SELECT 'role_b', count() FROM system.roles WHERE name = '${role_b}'" +${CLICKHOUSE_CLIENT} --query "RESTORE TABLE system.users FROM ${backup_name} FORMAT Null" +do_check + +# TODO: Cannot restore system.users, then system.roles correctly. The result after the second RESTORE ALL is the following: +# CREATE USER user_a IDENTIFIED WITH no_password DEFAULT ROLE NONE SETTINGS custom_x = 2; CREATE ROLE role_b SETTINGS custom_x = 1 +# because there is no `role_b` at the time when `user_a` is restored, +# and no information about the default role and the grant at the time when `role_b` is restored. +# +# echo "Everything dropped, restore system.users, then system.roles" +# ${CLICKHOUSE_CLIENT} --query "DROP USER ${user_a}" +# ${CLICKHOUSE_CLIENT} --query "DROP ROLE ${role_b}" +# ${CLICKHOUSE_CLIENT} --query "RESTORE TABLE system.users FROM ${backup_name} SETTINGS allow_unresolved_access_dependencies=true FORMAT Null" +# ${CLICKHOUSE_CLIENT} --query "SELECT 'user_a', count() FROM system.users WHERE name = '${user_a}'" +# ${CLICKHOUSE_CLIENT} --query "SELECT 'role_b', count() FROM system.roles WHERE name = '${role_b}'" +# ${CLICKHOUSE_CLIENT} --query "RESTORE TABLE system.roles FROM ${backup_name} SETTINGS update_access_entities_dependents=true FORMAT Null" +# do_check From bd6b7fb6db32efb69b8ccd92fde9c75e2242c781 Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Wed, 18 Sep 2024 13:13:04 +0200 Subject: [PATCH 08/13] Change filenames of files with access entities inside a backup: use "access-.txt" instead of "access01.txt", "access02.txt", ... --- src/Access/AccessBackup.cpp | 5 ++--- src/Access/AccessBackup.h | 5 ++--- src/Access/IAccessStorage.cpp | 3 +-- src/Backups/BackupCoordinationRemote.cpp | 2 +- .../BackupCoordinationReplicatedAccess.cpp | 19 ++++++++++++++++--- .../BackupCoordinationReplicatedAccess.h | 8 ++++---- src/Backups/BackupEntriesCollector.cpp | 7 ------- src/Backups/BackupEntriesCollector.h | 5 ----- 8 files changed, 26 insertions(+), 28 deletions(-) diff --git a/src/Access/AccessBackup.cpp b/src/Access/AccessBackup.cpp index 85e99e611c6..16727750a20 100644 --- a/src/Access/AccessBackup.cpp +++ b/src/Access/AccessBackup.cpp @@ -179,17 +179,16 @@ namespace } -std::pair makeBackupEntryForAccess( +std::pair makeBackupEntryForAccessEntities( const std::vector> & access_entities, const String & data_path_in_backup, - size_t counter, const AccessControl & access_control) { auto dependencies = readDependenciesNamesAndTypes(findDependencies(access_entities), access_control); AccessEntitiesInBackup ab; boost::range::copy(access_entities, std::inserter(ab.entities, ab.entities.end())); ab.dependencies = std::move(dependencies); - String filename = fmt::format("access{:02}.txt", counter + 1); /// access01.txt, access02.txt, ... + String filename = fmt::format("access-{}.txt", UUIDHelpers::generateV4()); String file_path_in_backup = fs::path{data_path_in_backup} / filename; return {file_path_in_backup, ab.toBackupEntry()}; } diff --git a/src/Access/AccessBackup.h b/src/Access/AccessBackup.h index 5bceda5e8c8..437be0e6c54 100644 --- a/src/Access/AccessBackup.h +++ b/src/Access/AccessBackup.h @@ -21,11 +21,10 @@ struct RestoreSettings; enum class RestoreAccessCreationMode : uint8_t; -/// Makes a backup of access entities of a specified type. -std::pair makeBackupEntryForAccess( +/// Makes a backup entry for of a set of access entities. +std::pair makeBackupEntryForAccessEntities( const std::vector> & access_entities, const String & data_path_in_backup, - size_t counter, const AccessControl & access_control); /// Restores access entities from a backup. diff --git a/src/Access/IAccessStorage.cpp b/src/Access/IAccessStorage.cpp index c9400c6c06c..9d51ac1f9a4 100644 --- a/src/Access/IAccessStorage.cpp +++ b/src/Access/IAccessStorage.cpp @@ -607,10 +607,9 @@ void IAccessStorage::backup(BackupEntriesCollector & backup_entries_collector, c if (entities.empty()) return; - auto backup_entry_with_path = makeBackupEntryForAccess( + auto backup_entry_with_path = makeBackupEntryForAccessEntities( entities, data_path_in_backup, - backup_entries_collector.getAccessCounter(type), backup_entries_collector.getContext()->getAccessControl()); if (isReplicated()) diff --git a/src/Backups/BackupCoordinationRemote.cpp b/src/Backups/BackupCoordinationRemote.cpp index f353062f628..a60ac0c636f 100644 --- a/src/Backups/BackupCoordinationRemote.cpp +++ b/src/Backups/BackupCoordinationRemote.cpp @@ -570,7 +570,7 @@ void BackupCoordinationRemote::prepareReplicatedAccess() const if (replicated_access) return; - std::vector file_path_for_access_entities; + std::vector file_path_for_access_entities; auto holder = with_retries.createRetriesControlHolder("prepareReplicatedAccess"); holder.retries_ctl.retryLoop( [&, &zk = holder.faulty_zookeeper]() diff --git a/src/Backups/BackupCoordinationReplicatedAccess.cpp b/src/Backups/BackupCoordinationReplicatedAccess.cpp index 5a7d049920c..075ccadcc5d 100644 --- a/src/Backups/BackupCoordinationReplicatedAccess.cpp +++ b/src/Backups/BackupCoordinationReplicatedAccess.cpp @@ -1,5 +1,9 @@ #include +#include + +namespace fs = std::filesystem; + namespace DB { @@ -7,7 +11,7 @@ namespace DB BackupCoordinationReplicatedAccess::BackupCoordinationReplicatedAccess() = default; BackupCoordinationReplicatedAccess::~BackupCoordinationReplicatedAccess() = default; -void BackupCoordinationReplicatedAccess::addFilePath(FilePathForAccessEntitry && file_path_for_access_entity) +void BackupCoordinationReplicatedAccess::addFilePath(FilePathForAccessEntity && file_path_for_access_entity) { const auto & access_zk_path = file_path_for_access_entity.access_zk_path; const auto & access_entity_type = file_path_for_access_entity.access_entity_type; @@ -28,10 +32,19 @@ Strings BackupCoordinationReplicatedAccess::getFilePaths(const String & access_z return {}; const auto & file_paths = it->second; - if (file_paths.host_to_store_access != host_id) + if ((file_paths.host_to_store_access != host_id) || file_paths.file_paths.empty()) return {}; - Strings res{file_paths.file_paths.begin(), file_paths.file_paths.end()}; + /// Use the same filename for all the paths in backup. + /// Those filenames have format "access-.txt", where UUID is random. + /// It's not really necessary, however it looks better if those files have the same filename + /// for a backup of ReplicatedAccessStorage on different hosts. + Strings res; + res.reserve(file_paths.file_paths.size()); + String filename = fs::path{*file_paths.file_paths.begin()}.filename(); + for (const auto & file_path : file_paths.file_paths) + res.emplace_back(fs::path{file_path}.replace_filename(filename)); + return res; } diff --git a/src/Backups/BackupCoordinationReplicatedAccess.h b/src/Backups/BackupCoordinationReplicatedAccess.h index 23672c3799e..34a7c915588 100644 --- a/src/Backups/BackupCoordinationReplicatedAccess.h +++ b/src/Backups/BackupCoordinationReplicatedAccess.h @@ -2,7 +2,7 @@ #include #include -#include +#include namespace DB @@ -28,7 +28,7 @@ public: BackupCoordinationReplicatedAccess(); ~BackupCoordinationReplicatedAccess(); - struct FilePathForAccessEntitry + struct FilePathForAccessEntity { String access_zk_path; AccessEntityType access_entity_type; @@ -37,7 +37,7 @@ public: }; /// Adds a path to access*.txt file keeping access entities of a ReplicatedAccessStorage. - void addFilePath(FilePathForAccessEntitry && file_path_for_access_entity); + void addFilePath(FilePathForAccessEntity && file_path_for_access_entity); /// Returns all paths added by addFilePath() if `host_id` is a host chosen to store access. Strings getFilePaths(const String & access_zk_path, AccessEntityType access_entity_type, const String & host_id) const; @@ -47,7 +47,7 @@ private: struct FilePathsAndHost { - std::unordered_set file_paths; + std::set file_paths; String host_to_store_access; }; diff --git a/src/Backups/BackupEntriesCollector.cpp b/src/Backups/BackupEntriesCollector.cpp index 3ee7a05600e..2c7984a5468 100644 --- a/src/Backups/BackupEntriesCollector.cpp +++ b/src/Backups/BackupEntriesCollector.cpp @@ -903,11 +903,4 @@ void BackupEntriesCollector::runPostTasks() LOG_TRACE(log, "All post tasks successfully executed"); } -size_t BackupEntriesCollector::getAccessCounter(AccessEntityType type) -{ - std::lock_guard lock(mutex); - access_counters.resize(static_cast(AccessEntityType::MAX)); - return access_counters[static_cast(type)]++; -} - } diff --git a/src/Backups/BackupEntriesCollector.h b/src/Backups/BackupEntriesCollector.h index fda5774105e..319f189ab3b 100644 --- a/src/Backups/BackupEntriesCollector.h +++ b/src/Backups/BackupEntriesCollector.h @@ -21,7 +21,6 @@ class IBackupCoordination; class IDatabase; using DatabasePtr = std::shared_ptr; struct StorageID; -enum class AccessEntityType : uint8_t; class QueryStatus; using QueryStatusPtr = std::shared_ptr; @@ -61,9 +60,6 @@ public: /// 1) we need to join (in a backup) the data of replicated tables gathered on different hosts. void addPostTask(std::function task); - /// Returns an incremental counter used to backup access control. - size_t getAccessCounter(AccessEntityType type); - private: void calculateRootPathInBackup(); @@ -179,7 +175,6 @@ private: BackupEntries backup_entries; std::queue> post_tasks; - std::vector access_counters; ThreadPool & threadpool; std::mutex mutex; From 995b5f7d4bf727a4b88f1bb3b63426064fa51c09 Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Wed, 18 Sep 2024 13:13:13 +0200 Subject: [PATCH 09/13] Extend IAccessStorage::findAll() to enable finding all the entities in the storage. --- src/Access/IAccessStorage.cpp | 14 +++++++++++++- src/Access/IAccessStorage.h | 18 +++++++++++++++--- 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/src/Access/IAccessStorage.cpp b/src/Access/IAccessStorage.cpp index 9d51ac1f9a4..5f367693b40 100644 --- a/src/Access/IAccessStorage.cpp +++ b/src/Access/IAccessStorage.cpp @@ -16,7 +16,7 @@ #include #include #include - +#include #include #include #include @@ -74,6 +74,18 @@ std::vector IAccessStorage::find(AccessEntityType type, const Strings & na } +std::vector IAccessStorage::findAllImpl() const +{ + std::vector res; + for (auto type : collections::range(AccessEntityType::MAX)) + { + auto ids = findAllImpl(type); + res.insert(res.end(), ids.begin(), ids.end()); + } + return res; +} + + UUID IAccessStorage::getID(AccessEntityType type, const String & name) const { auto id = findImpl(type, name); diff --git a/src/Access/IAccessStorage.h b/src/Access/IAccessStorage.h index b1bec2ce465..84cbdd0a751 100644 --- a/src/Access/IAccessStorage.h +++ b/src/Access/IAccessStorage.h @@ -91,8 +91,9 @@ public: /// Returns the identifiers of all the entities of a specified type contained in the storage. std::vector findAll(AccessEntityType type) const; - template - std::vector findAll() const { return findAll(EntityClassT::TYPE); } + /// Returns the identifiers of all the entities in the storage. + template + std::vector findAll() const; /// Searches for an entity with specified type and name. Returns std::nullopt if not found. std::optional find(AccessEntityType type, const String & name) const; @@ -149,7 +150,7 @@ public: std::optional> tryReadNameWithType(const UUID & id) const; /// Reads all entities and returns them with their IDs. - template + template std::vector>> readAllWithIDs() const; std::vector> readAllWithIDs(AccessEntityType type) const; @@ -220,6 +221,7 @@ public: protected: virtual std::optional findImpl(AccessEntityType type, const String & name) const = 0; virtual std::vector findAllImpl(AccessEntityType type) const = 0; + virtual std::vector findAllImpl() const; virtual AccessEntityPtr readImpl(const UUID & id, bool throw_if_not_exists) const = 0; virtual std::optional> readNameWithTypeImpl(const UUID & id, bool throw_if_not_exists) const; virtual bool insertImpl(const UUID & id, const AccessEntityPtr & entity, bool replace_if_exists, bool throw_if_exists, UUID * conflicting_id); @@ -268,6 +270,16 @@ private: }; +template +std::vector IAccessStorage::findAll() const +{ + if constexpr (std::is_same_v) + return findAllImpl(); + else + return findAllImpl(EntityClassT::TYPE); +} + + template std::shared_ptr IAccessStorage::read(const UUID & id, bool throw_if_not_exists) const { From 7ff8bf2958908f5a88641f0a5ad86b8d97596d03 Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Wed, 18 Sep 2024 13:13:24 +0200 Subject: [PATCH 10/13] Support writing dependents of access entities to backup, added backup setting "write_access_entities_dependents". --- src/Access/AccessBackup.cpp | 196 +++++++++++------- src/Access/AccessBackup.h | 7 +- src/Access/IAccessEntity.h | 1 + src/Access/IAccessStorage.cpp | 13 +- src/Access/Quota.cpp | 6 + src/Access/Quota.h | 1 + src/Access/Role.cpp | 6 + src/Access/Role.h | 1 + src/Access/RowPolicy.cpp | 6 + src/Access/RowPolicy.h | 1 + src/Access/SettingsProfile.cpp | 5 + src/Access/SettingsProfile.h | 1 + src/Access/SettingsProfileElement.cpp | 9 + src/Access/SettingsProfileElement.h | 2 + src/Access/User.cpp | 10 + src/Access/User.h | 1 + src/Backups/BackupEntriesCollector.cpp | 17 ++ src/Backups/BackupEntriesCollector.h | 7 + src/Backups/BackupSettings.cpp | 1 + src/Backups/BackupSettings.h | 5 + ..._restore_user_with_existing_role.reference | 3 + .../03231_restore_user_with_existing_role.sh | 23 +- 22 files changed, 227 insertions(+), 95 deletions(-) diff --git a/src/Access/AccessBackup.cpp b/src/Access/AccessBackup.cpp index 16727750a20..10b5561574e 100644 --- a/src/Access/AccessBackup.cpp +++ b/src/Access/AccessBackup.cpp @@ -42,6 +42,7 @@ namespace { std::unordered_map entities; std::unordered_map> dependencies; + std::unordered_map dependents; BackupEntryPtr toBackupEntry() const { @@ -73,6 +74,24 @@ namespace } } + if (!dependents.empty()) + { + if (!dependencies.empty()) + writeText("\n", buf); + writeText("DEPENDENTS\n", buf); + for (const auto & [id, entity] : dependents) + { + writeText(id, buf); + writeChar('\t', buf); + writeText(entity->getTypeInfo().name, buf); + writeChar('\t', buf); + writeText(entity->getName(), buf); + writeChar('\n', buf); + writeText(serializeAccessEntity(*entity), buf); + writeChar('\n', buf); + } + } + return std::make_shared(buf.str()); } @@ -82,59 +101,71 @@ namespace { AccessEntitiesInBackup res; - bool dependencies_found = false; + bool reading_dependencies = false; + bool reading_dependents = false; while (!buf->eof()) { String line; readStringUntilNewlineInto(line, *buf); buf->ignore(); + if (line == "DEPENDENCIES") { - dependencies_found = true; - break; + reading_dependencies = true; + reading_dependents = false; + continue; + } + else if (line == "DEPENDENTS") + { + reading_dependents = true; + reading_dependencies = false; + continue; + } + else if (line.empty()) + { + continue; } - UUID id = parse(line.substr(0, line.find('\t'))); - line.clear(); + size_t separator1 = line.find('\t'); + size_t separator2 = line.find('\t', separator1 + 1); + if ((separator1 == String::npos) || (separator2 == String::npos)) + throw Exception(ErrorCodes::CANNOT_RESTORE_TABLE, "Separators not found in line {}", line); - String queries; - while (!buf->eof()) + UUID id = parse(line.substr(0, separator1)); + AccessEntityType type = AccessEntityTypeInfo::parseType(line.substr(separator1 + 1, separator2 - separator1 - 1)); + String name = line.substr(separator2 + 1); + + if (reading_dependencies) { - String query; - readStringUntilNewlineInto(query, *buf); - buf->ignore(); - if (query.empty()) - break; - if (!queries.empty()) - queries.append("\n"); - queries.append(query); + res.dependencies.emplace(id, std::pair{name, type}); } - - AccessEntityPtr entity = deserializeAccessEntity(queries); - res.entities.emplace(id, entity); - } - - if (dependencies_found) - { - while (!buf->eof()) + else { - String id_as_string; - readStringInto(id_as_string, *buf); - buf->ignore(); - UUID id = parse(id_as_string); + String queries; + while (!buf->eof()) + { + String query; + readStringUntilNewlineInto(query, *buf); + buf->ignore(); + if (query.empty()) + break; + if (!queries.empty()) + queries.append("\n"); + queries.append(query); + } - String type_as_string; - readStringInto(type_as_string, *buf); - buf->ignore(); - AccessEntityType type = AccessEntityTypeInfo::parseType(type_as_string); + AccessEntityPtr entity = deserializeAccessEntity(queries); - String name; - readStringInto(name, *buf); - buf->ignore(); + if (name != entity->getName()) + throw Exception(ErrorCodes::CANNOT_RESTORE_TABLE, "Unexpected name {} is specified for {}", name, entity->formatTypeWithName()); + if (type != entity->getType()) + throw Exception(ErrorCodes::CANNOT_RESTORE_TABLE, "Unexpected type {} is specified for {}", AccessEntityTypeInfo::get(type).name, entity->formatTypeWithName()); - if (!res.entities.contains(id)) - res.dependencies.emplace(id, std::pair{name, type}); + if (reading_dependents) + res.dependents.emplace(id, entity); + else + res.entities.emplace(id, entity); } } @@ -147,47 +178,58 @@ namespace } } }; - - std::vector findDependencies(const std::vector> & entities) - { - std::vector res; - for (const auto & entity : entities | boost::adaptors::map_values) - insertAtEnd(res, entity->findDependencies()); - - /// Remove duplicates in the list of dependencies (some entities can refer to other entities). - ::sort(res.begin(), res.end()); - res.erase(std::unique(res.begin(), res.end()), res.end()); - for (const auto & id : entities | boost::adaptors::map_keys) - { - auto it = std::lower_bound(res.begin(), res.end(), id); - if ((it != res.end()) && (*it == id)) - res.erase(it); - } - return res; - } - - std::unordered_map> readDependenciesNamesAndTypes(const std::vector & dependencies, const AccessControl & access_control) - { - std::unordered_map> res; - for (const auto & id : dependencies) - { - if (auto name_and_type = access_control.tryReadNameWithType(id)) - res.emplace(id, name_and_type.value()); - } - return res; - } } std::pair makeBackupEntryForAccessEntities( - const std::vector> & access_entities, - const String & data_path_in_backup, - const AccessControl & access_control) + const std::vector & entities_ids, + const std::unordered_map & all_entities, + bool write_dependents, + const String & data_path_in_backup) { - auto dependencies = readDependenciesNamesAndTypes(findDependencies(access_entities), access_control); AccessEntitiesInBackup ab; - boost::range::copy(access_entities, std::inserter(ab.entities, ab.entities.end())); - ab.dependencies = std::move(dependencies); + + std::unordered_set entities_ids_set; + for (const auto & id : entities_ids) + entities_ids_set.emplace(id); + + for (const auto & id : entities_ids) + { + auto it = all_entities.find(id); + if (it != all_entities.end()) + { + AccessEntityPtr entity = it->second; + ab.entities.emplace(id, entity); + + auto dependencies = entity->findDependencies(); + for (const auto & dependency_id : dependencies) + { + if (!entities_ids_set.contains(dependency_id)) + { + auto it_dependency = all_entities.find(dependency_id); + if (it_dependency != all_entities.end()) + { + auto dependency_entity = it_dependency->second; + ab.dependencies.emplace(dependency_id, std::make_pair(dependency_entity->getName(), dependency_entity->getType())); + } + } + } + } + } + + if (write_dependents) + { + for (const auto & [id, possible_dependent] : all_entities) + { + if (!entities_ids_set.contains(id) && possible_dependent->hasDependencies(entities_ids_set)) + { + auto dependent = possible_dependent->clone(); + dependent->clearAllExceptDependencies(); + ab.dependents.emplace(id, dependent); + } + } + } + String filename = fmt::format("access-{}.txt", UUIDHelpers::generateV4()); String file_path_in_backup = fs::path{data_path_in_backup} / filename; return {file_path_in_backup, ab.toBackupEntry()}; @@ -278,6 +320,18 @@ void AccessRestorerFromBackup::loadFromBackup() EntityInfo & entity_info = it->second; entity_info.is_dependency = true; } + + for (const auto & [id, entity] : ab.dependents) + { + auto it = entity_infos.find(id); + if (it == entity_infos.end()) + { + it = entity_infos.emplace(id, EntityInfo{.id = id, .name = entity->getName(), .type = entity->getType()}).first; + } + EntityInfo & entity_info = it->second; + if (!entity_info.restore) + entity_info.entity = entity; + } } } diff --git a/src/Access/AccessBackup.h b/src/Access/AccessBackup.h index 437be0e6c54..c0c8358e327 100644 --- a/src/Access/AccessBackup.h +++ b/src/Access/AccessBackup.h @@ -23,9 +23,10 @@ enum class RestoreAccessCreationMode : uint8_t; /// Makes a backup entry for of a set of access entities. std::pair makeBackupEntryForAccessEntities( - const std::vector> & access_entities, - const String & data_path_in_backup, - const AccessControl & access_control); + const std::vector & entities_ids, + const std::unordered_map & all_entities, + bool write_dependents, + const String & data_path_in_backup); /// Restores access entities from a backup. void restoreAccessEntitiesFromBackup( diff --git a/src/Access/IAccessEntity.h b/src/Access/IAccessEntity.h index 40a58bec3eb..09c5eb6cf10 100644 --- a/src/Access/IAccessEntity.h +++ b/src/Access/IAccessEntity.h @@ -54,6 +54,7 @@ struct IAccessEntity virtual void replaceDependencies(const std::unordered_map & /* old_to_new_ids */) {} virtual void copyDependenciesFrom(const IAccessEntity & /* src */, const std::unordered_set & /* ids */) {} virtual void removeDependencies(const std::unordered_set & /* ids */) {} + virtual void clearAllExceptDependencies() {} /// Whether this access entity should be written to a backup. virtual bool isBackupAllowed() const { return false; } diff --git a/src/Access/IAccessStorage.cpp b/src/Access/IAccessStorage.cpp index 5f367693b40..bcddfc58fbd 100644 --- a/src/Access/IAccessStorage.cpp +++ b/src/Access/IAccessStorage.cpp @@ -613,16 +613,15 @@ void IAccessStorage::backup(BackupEntriesCollector & backup_entries_collector, c if (!isBackupAllowed()) throwBackupNotAllowed(); - auto entities = readAllWithIDs(type); - std::erase_if(entities, [](const std::pair & x) { return !x.second->isBackupAllowed(); }); - - if (entities.empty()) + auto entities_ids = findAll(type); + if (entities_ids.empty()) return; auto backup_entry_with_path = makeBackupEntryForAccessEntities( - entities, - data_path_in_backup, - backup_entries_collector.getContext()->getAccessControl()); + entities_ids, + backup_entries_collector.getAllAccessEntities(), + backup_entries_collector.getBackupSettings().write_access_entities_dependents, + data_path_in_backup); if (isReplicated()) { diff --git a/src/Access/Quota.cpp b/src/Access/Quota.cpp index 57aa1446d8c..3d4bff1ccba 100644 --- a/src/Access/Quota.cpp +++ b/src/Access/Quota.cpp @@ -47,4 +47,10 @@ void Quota::removeDependencies(const std::unordered_set & ids) to_roles.removeDependencies(ids); } +void Quota::clearAllExceptDependencies() +{ + all_limits.clear(); + key_type = QuotaKeyType::NONE; +} + } diff --git a/src/Access/Quota.h b/src/Access/Quota.h index 3becb82461d..2281714eb13 100644 --- a/src/Access/Quota.h +++ b/src/Access/Quota.h @@ -51,6 +51,7 @@ struct Quota : public IAccessEntity void replaceDependencies(const std::unordered_map & old_to_new_ids) override; void copyDependenciesFrom(const IAccessEntity & src, const std::unordered_set & ids) override; void removeDependencies(const std::unordered_set & ids) override; + void clearAllExceptDependencies() override; bool isBackupAllowed() const override { return true; } }; diff --git a/src/Access/Role.cpp b/src/Access/Role.cpp index d658467a8ac..4140daa0fd9 100644 --- a/src/Access/Role.cpp +++ b/src/Access/Role.cpp @@ -47,4 +47,10 @@ void Role::removeDependencies(const std::unordered_set & ids) settings.removeDependencies(ids); } +void Role::clearAllExceptDependencies() +{ + access = {}; + settings.removeSettingsKeepProfiles(); +} + } diff --git a/src/Access/Role.h b/src/Access/Role.h index 3bce167b33b..7b2450adfbe 100644 --- a/src/Access/Role.h +++ b/src/Access/Role.h @@ -25,6 +25,7 @@ struct Role : public IAccessEntity void replaceDependencies(const std::unordered_map & old_to_new_ids) override; void copyDependenciesFrom(const IAccessEntity & src, const std::unordered_set & ids) override; void removeDependencies(const std::unordered_set & ids) override; + void clearAllExceptDependencies() override; bool isBackupAllowed() const override { return settings.isBackupAllowed(); } }; diff --git a/src/Access/RowPolicy.cpp b/src/Access/RowPolicy.cpp index c26c59c6ea4..5b9d8521d9a 100644 --- a/src/Access/RowPolicy.cpp +++ b/src/Access/RowPolicy.cpp @@ -86,4 +86,10 @@ void RowPolicy::removeDependencies(const std::unordered_set & ids) to_roles.removeDependencies(ids); } +void RowPolicy::clearAllExceptDependencies() +{ + for (auto & filter : filters) + filter = {}; +} + } diff --git a/src/Access/RowPolicy.h b/src/Access/RowPolicy.h index 16234720469..3ce96b24823 100644 --- a/src/Access/RowPolicy.h +++ b/src/Access/RowPolicy.h @@ -54,6 +54,7 @@ struct RowPolicy : public IAccessEntity void replaceDependencies(const std::unordered_map & old_to_new_ids) override; void copyDependenciesFrom(const IAccessEntity & src, const std::unordered_set & ids) override; void removeDependencies(const std::unordered_set & ids) override; + void clearAllExceptDependencies() override; bool isBackupAllowed() const override { return true; } diff --git a/src/Access/SettingsProfile.cpp b/src/Access/SettingsProfile.cpp index 24c2604e1ae..8f863b29e86 100644 --- a/src/Access/SettingsProfile.cpp +++ b/src/Access/SettingsProfile.cpp @@ -47,4 +47,9 @@ void SettingsProfile::removeDependencies(const std::unordered_set & ids) to_roles.removeDependencies(ids); } +void SettingsProfile::clearAllExceptDependencies() +{ + elements.removeSettingsKeepProfiles(); +} + } diff --git a/src/Access/SettingsProfile.h b/src/Access/SettingsProfile.h index df5c327c793..4e70a3d6727 100644 --- a/src/Access/SettingsProfile.h +++ b/src/Access/SettingsProfile.h @@ -26,6 +26,7 @@ struct SettingsProfile : public IAccessEntity void replaceDependencies(const std::unordered_map & old_to_new_ids) override; void copyDependenciesFrom(const IAccessEntity & src, const std::unordered_set & ids) override; void removeDependencies(const std::unordered_set & ids) override; + void clearAllExceptDependencies() override; bool isBackupAllowed() const override { return elements.isBackupAllowed(); } }; diff --git a/src/Access/SettingsProfileElement.cpp b/src/Access/SettingsProfileElement.cpp index adf323bb55d..96684316151 100644 --- a/src/Access/SettingsProfileElement.cpp +++ b/src/Access/SettingsProfileElement.cpp @@ -211,6 +211,15 @@ void SettingsProfileElements::removeDependencies(const std::unordered_set } +void SettingsProfileElements::removeSettingsKeepProfiles() +{ + for (auto & element : *this) + element.setting_name.clear(); + + std::erase_if(*this, [&](const SettingsProfileElement & element) { return element.setting_name.empty() && !element.parent_profile; }); +} + + void SettingsProfileElements::merge(const SettingsProfileElements & other) { insert(end(), other.begin(), other.end()); diff --git a/src/Access/SettingsProfileElement.h b/src/Access/SettingsProfileElement.h index 3c816b563af..ea42b823ce1 100644 --- a/src/Access/SettingsProfileElement.h +++ b/src/Access/SettingsProfileElement.h @@ -68,6 +68,8 @@ public: void copyDependenciesFrom(const SettingsProfileElements & src, const std::unordered_set & ids); void removeDependencies(const std::unordered_set & ids); + void removeSettingsKeepProfiles(); + void merge(const SettingsProfileElements & other); Settings toSettings() const; diff --git a/src/Access/User.cpp b/src/Access/User.cpp index 880864d21ee..887abc213f9 100644 --- a/src/Access/User.cpp +++ b/src/Access/User.cpp @@ -81,4 +81,14 @@ void User::removeDependencies(const std::unordered_set & ids) settings.removeDependencies(ids); } +void User::clearAllExceptDependencies() +{ + authentication_methods.clear(); + allowed_client_hosts = AllowedClientHosts::AnyHostTag{}; + access = {}; + settings.removeSettingsKeepProfiles(); + default_database = {}; + valid_until = 0; +} + } diff --git a/src/Access/User.h b/src/Access/User.h index fa2331f3d02..03d62bf2277 100644 --- a/src/Access/User.h +++ b/src/Access/User.h @@ -36,6 +36,7 @@ struct User : public IAccessEntity void replaceDependencies(const std::unordered_map & old_to_new_ids) override; void copyDependenciesFrom(const IAccessEntity & src, const std::unordered_set & ids) override; void removeDependencies(const std::unordered_set & ids) override; + void clearAllExceptDependencies() override; bool isBackupAllowed() const override { return settings.isBackupAllowed(); } }; diff --git a/src/Backups/BackupEntriesCollector.cpp b/src/Backups/BackupEntriesCollector.cpp index 2c7984a5468..925b9be7a98 100644 --- a/src/Backups/BackupEntriesCollector.cpp +++ b/src/Backups/BackupEntriesCollector.cpp @@ -1,4 +1,5 @@ #include +#include #include #include #include @@ -903,4 +904,20 @@ void BackupEntriesCollector::runPostTasks() LOG_TRACE(log, "All post tasks successfully executed"); } +std::unordered_map BackupEntriesCollector::getAllAccessEntities() +{ + std::lock_guard lock(mutex); + if (!all_access_entities) + { + all_access_entities.emplace(); + auto entities_with_ids = context->getAccessControl().readAllWithIDs(); + for (const auto & [id, entity] : entities_with_ids) + { + if (entity->isBackupAllowed()) + all_access_entities->emplace(id, entity); + } + } + return *all_access_entities; +} + } diff --git a/src/Backups/BackupEntriesCollector.h b/src/Backups/BackupEntriesCollector.h index 319f189ab3b..ae076a84c8b 100644 --- a/src/Backups/BackupEntriesCollector.h +++ b/src/Backups/BackupEntriesCollector.h @@ -21,6 +21,8 @@ class IBackupCoordination; class IDatabase; using DatabasePtr = std::shared_ptr; struct StorageID; +struct IAccessEntity; +using AccessEntityPtr = std::shared_ptr; class QueryStatus; using QueryStatusPtr = std::shared_ptr; @@ -48,6 +50,9 @@ public: ContextPtr getContext() const { return context; } const ZooKeeperRetriesInfo & getZooKeeperRetriesInfo() const { return global_zookeeper_retries_info; } + /// Returns all access entities which can be put into a backup. + std::unordered_map getAllAccessEntities(); + /// Adds a backup entry which will be later returned by run(). /// These function can be called by implementations of IStorage::backupData() in inherited storage classes. void addBackupEntry(const String & file_name, BackupEntryPtr backup_entry); @@ -173,6 +178,8 @@ private: std::vector> previous_databases_metadata; std::vector> previous_tables_metadata; + std::optional> all_access_entities; + BackupEntries backup_entries; std::queue> post_tasks; diff --git a/src/Backups/BackupSettings.cpp b/src/Backups/BackupSettings.cpp index e982a806b7c..9b8117c6587 100644 --- a/src/Backups/BackupSettings.cpp +++ b/src/Backups/BackupSettings.cpp @@ -37,6 +37,7 @@ namespace ErrorCodes M(Bool, check_parts) \ M(Bool, check_projection_parts) \ M(Bool, allow_backup_broken_projections) \ + M(Bool, write_access_entities_dependents) \ M(Bool, internal) \ M(String, host_id) \ M(OptionalUUID, backup_uuid) diff --git a/src/Backups/BackupSettings.h b/src/Backups/BackupSettings.h index 0abeb897db4..6482237e77f 100644 --- a/src/Backups/BackupSettings.h +++ b/src/Backups/BackupSettings.h @@ -77,6 +77,11 @@ struct BackupSettings /// Allow to create backup with broken projections. bool allow_backup_broken_projections = false; + /// Whether dependents of access entities should be written along with the access entities. + /// For example, if a role is granted to a user and we're making a backup of system.roles (but not system.users) + /// this is whether the backup will contain information to grant the role to the corresponding user again. + bool write_access_entities_dependents = false; + /// Internal, should not be specified by user. /// Whether this backup is a part of a distributed backup created by BACKUP ON CLUSTER. bool internal = false; diff --git a/tests/queries/0_stateless/03231_restore_user_with_existing_role.reference b/tests/queries/0_stateless/03231_restore_user_with_existing_role.reference index 834c2b71833..70ff4078811 100644 --- a/tests/queries/0_stateless/03231_restore_user_with_existing_role.reference +++ b/tests/queries/0_stateless/03231_restore_user_with_existing_role.reference @@ -8,3 +8,6 @@ ACCESS_ENTITY_ALREADY_EXISTS Everything dropped, restore system.roles, then system.users user_a 0 role_b 1 +Everything dropped, restore system.users, then system.roles +user_a 1 +role_b 0 diff --git a/tests/queries/0_stateless/03231_restore_user_with_existing_role.sh b/tests/queries/0_stateless/03231_restore_user_with_existing_role.sh index 7c5fb342955..4a079ef1613 100755 --- a/tests/queries/0_stateless/03231_restore_user_with_existing_role.sh +++ b/tests/queries/0_stateless/03231_restore_user_with_existing_role.sh @@ -19,7 +19,7 @@ CREATE USER ${user_a} DEFAULT ROLE ${role_b} SETTINGS custom_x=2; backup_name="Disk('backups', '${CLICKHOUSE_TEST_UNIQUE_NAME}')" -${CLICKHOUSE_CLIENT} --query "BACKUP TABLE system.users, TABLE system.roles TO ${backup_name} FORMAT Null" +${CLICKHOUSE_CLIENT} --query "BACKUP TABLE system.users, TABLE system.roles TO ${backup_name} SETTINGS write_access_entities_dependents = true FORMAT Null" ${CLICKHOUSE_CLIENT} --query "RESTORE ALL FROM ${backup_name} FORMAT Null" do_check() @@ -81,16 +81,11 @@ ${CLICKHOUSE_CLIENT} --query "SELECT 'role_b', count() FROM system.roles WHERE n ${CLICKHOUSE_CLIENT} --query "RESTORE TABLE system.users FROM ${backup_name} FORMAT Null" do_check -# TODO: Cannot restore system.users, then system.roles correctly. The result after the second RESTORE ALL is the following: -# CREATE USER user_a IDENTIFIED WITH no_password DEFAULT ROLE NONE SETTINGS custom_x = 2; CREATE ROLE role_b SETTINGS custom_x = 1 -# because there is no `role_b` at the time when `user_a` is restored, -# and no information about the default role and the grant at the time when `role_b` is restored. -# -# echo "Everything dropped, restore system.users, then system.roles" -# ${CLICKHOUSE_CLIENT} --query "DROP USER ${user_a}" -# ${CLICKHOUSE_CLIENT} --query "DROP ROLE ${role_b}" -# ${CLICKHOUSE_CLIENT} --query "RESTORE TABLE system.users FROM ${backup_name} SETTINGS allow_unresolved_access_dependencies=true FORMAT Null" -# ${CLICKHOUSE_CLIENT} --query "SELECT 'user_a', count() FROM system.users WHERE name = '${user_a}'" -# ${CLICKHOUSE_CLIENT} --query "SELECT 'role_b', count() FROM system.roles WHERE name = '${role_b}'" -# ${CLICKHOUSE_CLIENT} --query "RESTORE TABLE system.roles FROM ${backup_name} SETTINGS update_access_entities_dependents=true FORMAT Null" -# do_check +echo "Everything dropped, restore system.users, then system.roles" +${CLICKHOUSE_CLIENT} --query "DROP USER ${user_a}" +${CLICKHOUSE_CLIENT} --query "DROP ROLE ${role_b}" +${CLICKHOUSE_CLIENT} --query "RESTORE TABLE system.users FROM ${backup_name} SETTINGS allow_unresolved_access_dependencies=true FORMAT Null" +${CLICKHOUSE_CLIENT} --query "SELECT 'user_a', count() FROM system.users WHERE name = '${user_a}'" +${CLICKHOUSE_CLIENT} --query "SELECT 'role_b', count() FROM system.roles WHERE name = '${role_b}'" +${CLICKHOUSE_CLIENT} --query "RESTORE TABLE system.roles FROM ${backup_name} SETTINGS update_access_entities_dependents=true FORMAT Null" +do_check From c038239ec8a4343753ed3905152c1e14d86ce9c3 Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Wed, 18 Sep 2024 13:13:35 +0200 Subject: [PATCH 11/13] Rename restore setting "allow_unresolved_access_dependencies" -> "skip_unresolved_access_entities_dependencies". --- src/Access/AccessBackup.cpp | 2 +- src/Backups/RestoreSettings.cpp | 8 ++++++-- src/Backups/RestoreSettings.h | 2 +- .../0_stateless/03231_restore_user_with_existing_role.sh | 2 +- 4 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/Access/AccessBackup.cpp b/src/Access/AccessBackup.cpp index 10b5561574e..fe90c976f44 100644 --- a/src/Access/AccessBackup.cpp +++ b/src/Access/AccessBackup.cpp @@ -240,7 +240,7 @@ AccessRestorerFromBackup::AccessRestorerFromBackup( const BackupPtr & backup_, const RestoreSettings & restore_settings_) : backup(backup_) , creation_mode(restore_settings_.create_access) - , skip_unresolved_dependencies(restore_settings_.allow_unresolved_access_dependencies) + , skip_unresolved_dependencies(restore_settings_.skip_unresolved_access_entities_dependencies) , update_dependents(restore_settings_.update_access_entities_dependents) , log(getLogger("AccessRestorerFromBackup")) { diff --git a/src/Backups/RestoreSettings.cpp b/src/Backups/RestoreSettings.cpp index 7383da699e9..eed31a6f79a 100644 --- a/src/Backups/RestoreSettings.cpp +++ b/src/Backups/RestoreSettings.cpp @@ -160,7 +160,7 @@ namespace M(UInt64, replica_num_in_backup) \ M(Bool, allow_non_empty_tables) \ M(RestoreAccessCreationMode, create_access) \ - M(Bool, allow_unresolved_access_dependencies) \ + M(Bool, skip_unresolved_access_entities_dependencies) \ M(Bool, update_access_entities_dependents) \ M(RestoreUDFCreationMode, create_function) \ M(Bool, allow_s3_native_copy) \ @@ -189,7 +189,11 @@ RestoreSettings RestoreSettings::fromRestoreQuery(const ASTBackupQuery & query) LIST_OF_RESTORE_SETTINGS(GET_SETTINGS_FROM_RESTORE_QUERY_HELPER) - throw Exception(ErrorCodes::CANNOT_PARSE_BACKUP_SETTINGS, "Unknown setting {}", setting.name); + /// `allow_unresolved_access_dependencies` is an obsolete name. + if (setting.name == "allow_unresolved_access_dependencies") + res.skip_unresolved_access_entities_dependencies = SettingFieldBool{setting.value}.value; + else + throw Exception(ErrorCodes::CANNOT_PARSE_BACKUP_SETTINGS, "Unknown setting {}", setting.name); } } diff --git a/src/Backups/RestoreSettings.h b/src/Backups/RestoreSettings.h index 1cab081b20d..e8101cdd71c 100644 --- a/src/Backups/RestoreSettings.h +++ b/src/Backups/RestoreSettings.h @@ -108,7 +108,7 @@ struct RestoreSettings /// and now we're restoring only user `u2` and role `r2` doesn't exist, then /// this flag is whether RESTORE should continue with restoring user `u2` without that grant. /// If this flag is false then RESTORE will throw an exception in that case. - bool allow_unresolved_access_dependencies = false; + bool skip_unresolved_access_entities_dependencies = false; /// Try to update dependents of restored access entities. /// For example: if a backup contains a profile assigned to a user: `CREATE PROFILE p1; CREATE USER u1 SETTINGS PROFILE p1` diff --git a/tests/queries/0_stateless/03231_restore_user_with_existing_role.sh b/tests/queries/0_stateless/03231_restore_user_with_existing_role.sh index 4a079ef1613..8ff23b2467d 100755 --- a/tests/queries/0_stateless/03231_restore_user_with_existing_role.sh +++ b/tests/queries/0_stateless/03231_restore_user_with_existing_role.sh @@ -84,7 +84,7 @@ do_check echo "Everything dropped, restore system.users, then system.roles" ${CLICKHOUSE_CLIENT} --query "DROP USER ${user_a}" ${CLICKHOUSE_CLIENT} --query "DROP ROLE ${role_b}" -${CLICKHOUSE_CLIENT} --query "RESTORE TABLE system.users FROM ${backup_name} SETTINGS allow_unresolved_access_dependencies=true FORMAT Null" +${CLICKHOUSE_CLIENT} --query "RESTORE TABLE system.users FROM ${backup_name} SETTINGS skip_unresolved_access_entities_dependencies=true FORMAT Null" ${CLICKHOUSE_CLIENT} --query "SELECT 'user_a', count() FROM system.users WHERE name = '${user_a}'" ${CLICKHOUSE_CLIENT} --query "SELECT 'role_b', count() FROM system.roles WHERE name = '${role_b}'" ${CLICKHOUSE_CLIENT} --query "RESTORE TABLE system.roles FROM ${backup_name} SETTINGS update_access_entities_dependents=true FORMAT Null" From 7cf575c20fc020ae8e8b8b2c720b7176927dba4e Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Wed, 18 Sep 2024 13:13:44 +0200 Subject: [PATCH 12/13] Enable by default backup setting "write_access_entities_dependents", restore settings "update_access_entities_dependents" and "skip_unresolved_access_entities_dependencies". --- src/Backups/BackupSettings.h | 2 +- src/Backups/RestoreSettings.h | 4 ++-- .../0_stateless/03231_restore_user_with_existing_role.sh | 8 ++++---- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/Backups/BackupSettings.h b/src/Backups/BackupSettings.h index 6482237e77f..8c2ea21df01 100644 --- a/src/Backups/BackupSettings.h +++ b/src/Backups/BackupSettings.h @@ -80,7 +80,7 @@ struct BackupSettings /// Whether dependents of access entities should be written along with the access entities. /// For example, if a role is granted to a user and we're making a backup of system.roles (but not system.users) /// this is whether the backup will contain information to grant the role to the corresponding user again. - bool write_access_entities_dependents = false; + bool write_access_entities_dependents = true; /// Internal, should not be specified by user. /// Whether this backup is a part of a distributed backup created by BACKUP ON CLUSTER. diff --git a/src/Backups/RestoreSettings.h b/src/Backups/RestoreSettings.h index e8101cdd71c..ad027b83354 100644 --- a/src/Backups/RestoreSettings.h +++ b/src/Backups/RestoreSettings.h @@ -108,7 +108,7 @@ struct RestoreSettings /// and now we're restoring only user `u2` and role `r2` doesn't exist, then /// this flag is whether RESTORE should continue with restoring user `u2` without that grant. /// If this flag is false then RESTORE will throw an exception in that case. - bool skip_unresolved_access_entities_dependencies = false; + bool skip_unresolved_access_entities_dependencies = true; /// Try to update dependents of restored access entities. /// For example: if a backup contains a profile assigned to a user: `CREATE PROFILE p1; CREATE USER u1 SETTINGS PROFILE p1` @@ -118,7 +118,7 @@ struct RestoreSettings /// and now we're restoring only role `r2` and user `u2` already exists, then /// this flag is whether restored role `r2` should be granted to user `u2` again. /// If this flag is false then RESTORE won't update existing access entities. - bool update_access_entities_dependents = false; + bool update_access_entities_dependents = true; /// How the RESTORE command will handle if a user-defined function which it's going to restore already exists. RestoreUDFCreationMode create_function = RestoreUDFCreationMode::kCreateIfNotExists; diff --git a/tests/queries/0_stateless/03231_restore_user_with_existing_role.sh b/tests/queries/0_stateless/03231_restore_user_with_existing_role.sh index 8ff23b2467d..6aa8b22de10 100755 --- a/tests/queries/0_stateless/03231_restore_user_with_existing_role.sh +++ b/tests/queries/0_stateless/03231_restore_user_with_existing_role.sh @@ -19,7 +19,7 @@ CREATE USER ${user_a} DEFAULT ROLE ${role_b} SETTINGS custom_x=2; backup_name="Disk('backups', '${CLICKHOUSE_TEST_UNIQUE_NAME}')" -${CLICKHOUSE_CLIENT} --query "BACKUP TABLE system.users, TABLE system.roles TO ${backup_name} SETTINGS write_access_entities_dependents = true FORMAT Null" +${CLICKHOUSE_CLIENT} --query "BACKUP TABLE system.users, TABLE system.roles TO ${backup_name} FORMAT Null" ${CLICKHOUSE_CLIENT} --query "RESTORE ALL FROM ${backup_name} FORMAT Null" do_check() @@ -57,7 +57,7 @@ do_check echo "Role dropped" ${CLICKHOUSE_CLIENT} --query "DROP ROLE ${role_b}" -${CLICKHOUSE_CLIENT} --query "RESTORE ALL FROM ${backup_name} SETTINGS update_access_entities_dependents=true FORMAT Null" +${CLICKHOUSE_CLIENT} --query "RESTORE ALL FROM ${backup_name} FORMAT Null" do_check echo "Nothing dropped" @@ -84,8 +84,8 @@ do_check echo "Everything dropped, restore system.users, then system.roles" ${CLICKHOUSE_CLIENT} --query "DROP USER ${user_a}" ${CLICKHOUSE_CLIENT} --query "DROP ROLE ${role_b}" -${CLICKHOUSE_CLIENT} --query "RESTORE TABLE system.users FROM ${backup_name} SETTINGS skip_unresolved_access_entities_dependencies=true FORMAT Null" +${CLICKHOUSE_CLIENT} --query "RESTORE TABLE system.users FROM ${backup_name} FORMAT Null" ${CLICKHOUSE_CLIENT} --query "SELECT 'user_a', count() FROM system.users WHERE name = '${user_a}'" ${CLICKHOUSE_CLIENT} --query "SELECT 'role_b', count() FROM system.roles WHERE name = '${role_b}'" -${CLICKHOUSE_CLIENT} --query "RESTORE TABLE system.roles FROM ${backup_name} SETTINGS update_access_entities_dependents=true FORMAT Null" +${CLICKHOUSE_CLIENT} --query "RESTORE TABLE system.roles FROM ${backup_name} FORMAT Null" do_check From eb5c7b1ced3955be0524fac02dc727922fa8d05a Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Thu, 19 Sep 2024 01:24:07 +0200 Subject: [PATCH 13/13] Add one more test. --- ...thout_access_entities_dependents.reference | 7 ++++ ...ckup_without_access_entities_dependents.sh | 30 ++++++++++++++++++ ...kup_without_access_entities_dependents.zip | Bin 0 -> 1996 bytes 3 files changed, 37 insertions(+) create mode 100644 tests/queries/0_stateless/03231_old_backup_without_access_entities_dependents.reference create mode 100755 tests/queries/0_stateless/03231_old_backup_without_access_entities_dependents.sh create mode 100644 tests/queries/0_stateless/backups/old_backup_without_access_entities_dependents.zip diff --git a/tests/queries/0_stateless/03231_old_backup_without_access_entities_dependents.reference b/tests/queries/0_stateless/03231_old_backup_without_access_entities_dependents.reference new file mode 100644 index 00000000000..8268df7a65b --- /dev/null +++ b/tests/queries/0_stateless/03231_old_backup_without_access_entities_dependents.reference @@ -0,0 +1,7 @@ +CREATE USER user_03231 IDENTIFIED WITH no_password DEFAULT ROLE role_a_03231 SETTINGS custom_x = \'x\' +GRANT role_a_03231 TO user_03231 +CREATE ROLE role_a_03231 +GRANT INSERT ON *.* TO role_a_03231 +GRANT role_b_03231 TO role_a_03231 +CREATE ROLE role_b_03231 +GRANT SELECT ON *.* TO role_b_03231 diff --git a/tests/queries/0_stateless/03231_old_backup_without_access_entities_dependents.sh b/tests/queries/0_stateless/03231_old_backup_without_access_entities_dependents.sh new file mode 100755 index 00000000000..28d66288f0c --- /dev/null +++ b/tests/queries/0_stateless/03231_old_backup_without_access_entities_dependents.sh @@ -0,0 +1,30 @@ +#!/usr/bin/env bash +# Tags: no-parallel + +# Disabled parallel since we drop and restore fixed users and roles. + +CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +# shellcheck source=../shell_config.sh +. "$CURDIR"/../shell_config.sh + +# In this test we restore from "/tests/queries/0_stateless/backups/old_backup_without_access_entities_dependents.zip" +backup_name="$($CURDIR/helpers/install_predefined_backup.sh old_backup_without_access_entities_dependents.zip)" + +${CLICKHOUSE_CLIENT} -m --query " +DROP USER IF EXISTS user_03231; +DROP ROLE IF EXISTS role_a_03231, role_b_03231; +" + +${CLICKHOUSE_CLIENT} --query "RESTORE ALL FROM Disk('backups', '${backup_name}') FORMAT Null" + +${CLICKHOUSE_CLIENT} --query "SHOW CREATE USER user_03231" +${CLICKHOUSE_CLIENT} --query "SHOW GRANTS FOR user_03231" +${CLICKHOUSE_CLIENT} --query "SHOW CREATE ROLE role_a_03231" +${CLICKHOUSE_CLIENT} --query "SHOW GRANTS FOR role_a_03231" +${CLICKHOUSE_CLIENT} --query "SHOW CREATE ROLE role_b_03231" +${CLICKHOUSE_CLIENT} --query "SHOW GRANTS FOR role_b_03231" + +${CLICKHOUSE_CLIENT} -m --query " +DROP USER user_03231; +DROP ROLE role_a_03231, role_b_03231; +" diff --git a/tests/queries/0_stateless/backups/old_backup_without_access_entities_dependents.zip b/tests/queries/0_stateless/backups/old_backup_without_access_entities_dependents.zip new file mode 100644 index 0000000000000000000000000000000000000000..a20e66b2f6b8294fcd81323c467e36064caa7f20 GIT binary patch literal 1996 zcmWIWW@Zs#VB+9ln8|G%sVi%5r_IQ~u#|~`ft`VYK`$vWIlHuA>ZNSI!v;JI4^|g6 z>XqG_xhRfxXD74hYqM!0jLVlx_jB?J{7yeQ&CXuva$fYY-F9{#7xwMW?GLPJGc2C> zeYRoXDXnal1|`WU|-a-8u*W=7b&WRG9#3{SqT;(Tl(wkE}7?V7c(ge#dg zvHTXd{1%#ha;L0MzUTExHPeM}|9vB8CK+zCsE?!7iA8rJ6W2)xHv82QTOI|^%)Q=! zsh&?<>fP+N-xj$By(`0|-se@Z;|??QkSHxqEkX|orE>Mu^Eg5Rq8>RUrcN--K5QV; zlKwk<&)KsI9XcnwIAnePOyXz?ej{!r6+LPBHoY?EPoEzm9de* zz3jtNo=>WrZ#7LSzeMfortHd+RU5m`1xw6c>cQGyyr^b#`)*!)vl~p_o1(i}_e9+~ zeEdSqw#y}wZv3oypD#Bqu#zj_vs0~ApZjuZ(xH!$m;4M~h8%aA{ZpZ*tVnbc>lKfb z)3LnoY-)MmpB0^_xKr(Qep~F+>0jnMS^aa0J6_nRA93!1ba&~W>3uKk6s6j?U)a)b zpNBi>IDtWun_7}cAXN$=D?>?{u#|S@_(m=*pej8emc*hO9L9RZg*nB1MWsB2p&W%J zp9(`Y*6HYZp1rEA>!+&~R`9j>`Y-` zbx|x}YXn)q$RxsyyXXUY0}K{4f+%P~2-Acse?zngFuZUK0A@O9xI*l~rUjJW5n762 zTHs-fTPG;hBXrJ1(+MjLP<@DA9>7dk(0CTBe&jrX&*P|WSq`i^z#fPC0+t_9-GZJY z5pKDTRX?$A0VQb&07b$hW|RbuJ2*i}9HEVq1xXuPWWZ7+dXk6fU(jfdMK?SHz`_w` h40^&w7&8NlG4Ny_;LXYgQpyd4dw~J=6R4Ph0RUw8O-KL$ literal 0 HcmV?d00001