From e2c2a679ef6a932d0372ca0a6f019bdca64c19e8 Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Fri, 11 Sep 2020 19:54:22 +0300 Subject: [PATCH] Skip access storages with same path while reading the main config. --- src/Access/AccessControlManager.cpp | 30 +++++++++++++++++- src/Access/DiskAccessStorage.cpp | 31 +++++++++++++------ src/Access/DiskAccessStorage.h | 6 +++- .../configs/duplicates.xml | 13 ++++++++ .../configs/mixed_style.xml | 3 ++ .../integration/test_user_directories/test.py | 16 ++++++---- 6 files changed, 81 insertions(+), 18 deletions(-) create mode 100644 tests/integration/test_user_directories/configs/duplicates.xml diff --git a/src/Access/AccessControlManager.cpp b/src/Access/AccessControlManager.cpp index 1fa26c8535..4113786721 100644 --- a/src/Access/AccessControlManager.cpp +++ b/src/Access/AccessControlManager.cpp @@ -181,6 +181,15 @@ void AccessControlManager::addUsersConfigStorage( const String & preprocessed_dir_, const zkutil::GetZooKeeper & get_zookeeper_function_) { + auto storages = getStoragesPtr(); + for (const auto & storage : *storages) + { + if (auto users_config_storage = typeid_cast>(storage)) + { + if (users_config_storage->getStoragePath() == users_config_path_) + return; + } + } auto check_setting_name_function = [this](const std::string_view & setting_name) { checkSettingNameIsAllowed(setting_name); }; auto new_storage = std::make_shared(storage_name_, check_setting_name_function); new_storage->load(users_config_path_, include_from_path_, preprocessed_dir_, get_zookeeper_function_); @@ -210,17 +219,36 @@ void AccessControlManager::startPeriodicReloadingUsersConfigs() void AccessControlManager::addDiskStorage(const String & directory_, bool readonly_) { - addStorage(std::make_shared(directory_, readonly_)); + addDiskStorage(DiskAccessStorage::STORAGE_TYPE, directory_, readonly_); } void AccessControlManager::addDiskStorage(const String & storage_name_, const String & directory_, bool readonly_) { + auto storages = getStoragesPtr(); + for (const auto & storage : *storages) + { + if (auto disk_storage = typeid_cast>(storage)) + { + if (disk_storage->isStoragePathEqual(directory_)) + { + if (readonly_) + disk_storage->setReadOnly(readonly_); + return; + } + } + } addStorage(std::make_shared(storage_name_, directory_, readonly_)); } void AccessControlManager::addMemoryStorage(const String & storage_name_) { + auto storages = getStoragesPtr(); + for (const auto & storage : *storages) + { + if (auto memory_storage = typeid_cast>(storage)) + return; + } addStorage(std::make_shared(storage_name_)); } diff --git a/src/Access/DiskAccessStorage.cpp b/src/Access/DiskAccessStorage.cpp index fc80859885..2fcb9480e6 100644 --- a/src/Access/DiskAccessStorage.cpp +++ b/src/Access/DiskAccessStorage.cpp @@ -218,6 +218,16 @@ namespace } + /// Converts a path to an absolute path and append it with a separator. + String makeDirectoryPathCanonical(const String & directory_path) + { + auto canonical_directory_path = std::filesystem::weakly_canonical(directory_path); + if (canonical_directory_path.has_filename()) + canonical_directory_path += std::filesystem::path::preferred_separator; + return canonical_directory_path; + } + + /// Calculates the path to a file named .sql for saving an access entity. String getEntityFilePath(const String & directory_path, const UUID & id) { @@ -298,22 +308,17 @@ DiskAccessStorage::DiskAccessStorage(const String & directory_path_, bool readon { } - DiskAccessStorage::DiskAccessStorage(const String & storage_name_, const String & directory_path_, bool readonly_) : IAccessStorage(storage_name_) { - auto canonical_directory_path = std::filesystem::weakly_canonical(directory_path_); - if (canonical_directory_path.has_filename()) - canonical_directory_path += std::filesystem::path::preferred_separator; + directory_path = makeDirectoryPathCanonical(directory_path_); + readonly = readonly_; std::error_code create_dir_error_code; - std::filesystem::create_directories(canonical_directory_path, create_dir_error_code); + std::filesystem::create_directories(directory_path, create_dir_error_code); - if (!std::filesystem::exists(canonical_directory_path) || !std::filesystem::is_directory(canonical_directory_path) || create_dir_error_code) - throw Exception("Couldn't create directory " + canonical_directory_path.string() + " reason: '" + create_dir_error_code.message() + "'", ErrorCodes::DIRECTORY_DOESNT_EXIST); - - directory_path = canonical_directory_path; - readonly = readonly_; + if (!std::filesystem::exists(directory_path) || !std::filesystem::is_directory(directory_path) || create_dir_error_code) + throw Exception("Couldn't create directory " + directory_path + " reason: '" + create_dir_error_code.message() + "'", ErrorCodes::DIRECTORY_DOESNT_EXIST); bool should_rebuild_lists = std::filesystem::exists(getNeedRebuildListsMarkFilePath(directory_path)); if (!should_rebuild_lists) @@ -337,6 +342,12 @@ DiskAccessStorage::~DiskAccessStorage() } +bool DiskAccessStorage::isStoragePathEqual(const String & directory_path_) const +{ + return getStoragePath() == makeDirectoryPathCanonical(directory_path_); +} + + void DiskAccessStorage::clear() { entries_by_id.clear(); diff --git a/src/Access/DiskAccessStorage.h b/src/Access/DiskAccessStorage.h index 11eb1c3b1a..f136b046ac 100644 --- a/src/Access/DiskAccessStorage.h +++ b/src/Access/DiskAccessStorage.h @@ -18,7 +18,11 @@ public: ~DiskAccessStorage() override; const char * getStorageType() const override { return STORAGE_TYPE; } + String getStoragePath() const override { return directory_path; } + bool isStoragePathEqual(const String & directory_path_) const; + + void setReadOnly(bool readonly_) { readonly = readonly_; } bool isStorageReadOnly() const override { return readonly; } private: @@ -67,7 +71,7 @@ private: void prepareNotifications(const UUID & id, const Entry & entry, bool remove, Notifications & notifications) const; String directory_path; - bool readonly; + std::atomic readonly; std::unordered_map entries_by_id; std::unordered_map entries_by_name_and_type[static_cast(EntityType::MAX)]; boost::container::flat_set types_of_lists_to_write; diff --git a/tests/integration/test_user_directories/configs/duplicates.xml b/tests/integration/test_user_directories/configs/duplicates.xml new file mode 100644 index 0000000000..69bb06a112 --- /dev/null +++ b/tests/integration/test_user_directories/configs/duplicates.xml @@ -0,0 +1,13 @@ + + + + /var/lib/clickhouse/access7/ + + + /etc/clickhouse-server/users7.xml + + + + /etc/clickhouse-server/users7.xml + /var/lib/clickhouse/access7/ + diff --git a/tests/integration/test_user_directories/configs/mixed_style.xml b/tests/integration/test_user_directories/configs/mixed_style.xml index d6ddecf6f5..f668140521 100644 --- a/tests/integration/test_user_directories/configs/mixed_style.xml +++ b/tests/integration/test_user_directories/configs/mixed_style.xml @@ -1,5 +1,8 @@ + + /var/lib/clickhouse/access6a/ + diff --git a/tests/integration/test_user_directories/test.py b/tests/integration/test_user_directories/test.py index 218330cb1a..7174550206 100644 --- a/tests/integration/test_user_directories/test.py +++ b/tests/integration/test_user_directories/test.py @@ -12,11 +12,8 @@ def started_cluster(): try: cluster.start() - node.exec_in_container("cp /etc/clickhouse-server/users.xml /etc/clickhouse-server/users2.xml") - node.exec_in_container("cp /etc/clickhouse-server/users.xml /etc/clickhouse-server/users3.xml") - node.exec_in_container("cp /etc/clickhouse-server/users.xml /etc/clickhouse-server/users4.xml") - node.exec_in_container("cp /etc/clickhouse-server/users.xml /etc/clickhouse-server/users5.xml") - node.exec_in_container("cp /etc/clickhouse-server/users.xml /etc/clickhouse-server/users6.xml") + for i in range(2, 8): + node.exec_in_container("cp /etc/clickhouse-server/users.xml /etc/clickhouse-server/users{}.xml".format(i)) yield cluster @@ -56,4 +53,11 @@ def test_mixed_style(): node.restart_clickhouse() assert node.query("SELECT * FROM system.user_directories") == TSV([["users.xml", "users.xml", "/etc/clickhouse-server/users6.xml", 1, 1], ["local directory", "local directory", "/var/lib/clickhouse/access6/", 0, 2], - ["memory", "memory", "", 0, 3]]) + ["local directory", "local directory", "/var/lib/clickhouse/access6a/", 0, 3], + ["memory", "memory", "", 0, 4]]) + +def test_duplicates(): + node.copy_file_to_container(os.path.join(SCRIPT_DIR, "configs/duplicates.xml"), '/etc/clickhouse-server/config.d/z.xml') + node.restart_clickhouse() + assert node.query("SELECT * FROM system.user_directories") == TSV([["users.xml", "users.xml", "/etc/clickhouse-server/users7.xml", 1, 1], + ["local directory", "local directory", "/var/lib/clickhouse/access7/", 0, 2]]) -- GitLab