Merge pull request #14748 from vitlibar/skip-access-storages-with-same-path

Skip access storages with same path while reading the main config.

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<std::shared_ptr<UsersConfigAccessStorage>>(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<UsersConfigAccessStorage>(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<DiskAccessStorage>(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<std::shared_ptr<DiskAccessStorage>>(storage))
+        {
+            if (disk_storage->isStoragePathEqual(directory_))
+            {
+                if (readonly_)
+                    disk_storage->setReadOnly(readonly_);
+                return;
+            }
+        }
+    }
     addStorage(std::make_shared<DiskAccessStorage>(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<std::shared_ptr<MemoryAccessStorage>>(storage))
+            return;
+    }
     addStorage(std::make_shared<MemoryAccessStorage>(storage_name_));
 }
 

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 <id>.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();

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:
@@ -66,7 +70,7 @@ private:
     void prepareNotifications(const UUID & id, const Entry & entry, bool remove, Notifications & notifications) const;
 
     String directory_path;
-    bool readonly;
+    std::atomic<bool> readonly;
     std::unordered_map<UUID, Entry> entries_by_id;
     std::unordered_map<std::string_view, Entry *> entries_by_name_and_type[static_cast<size_t>(EntityType::MAX)];
     boost::container::flat_set<EntityType> types_of_lists_to_write;

tests/integration/test_user_directories/configs/duplicates.xml

+<yandex>
+    <user_directories replace="replace">
+        <local_directory>
+            <path>/var/lib/clickhouse/access7/</path>
+        </local_directory>
+        <users_xml>
+            <path>/etc/clickhouse-server/users7.xml</path>
+        </users_xml>
+    </user_directories>
+
+    <users_config>/etc/clickhouse-server/users7.xml</users_config>
+    <access_control_path>/var/lib/clickhouse/access7/</access_control_path>
+</yandex>

tests/integration/test_user_directories/configs/mixed_style.xml

 <yandex>
     <user_directories replace="replace">
+        <local_directory>
+            <path>/var/lib/clickhouse/access6a/</path>
+        </local_directory>
         <memory/>
     </user_directories>
 

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]])