From c1310841d9a14f95d31beb2361289c455b7e53df Mon Sep 17 00:00:00 2001 From: Alexander Tokmakov Date: Thu, 28 Oct 2021 19:19:41 +0300 Subject: [PATCH] fix ambiguity when extracting auxiliary zk name --- src/Interpreters/Context.cpp | 3 ++ src/Storages/StorageReplicatedMergeTree.cpp | 41 ++++++++++++------- .../test.py | 10 +++++ 3 files changed, 39 insertions(+), 15 deletions(-) diff --git a/src/Interpreters/Context.cpp b/src/Interpreters/Context.cpp index 69f9518a912..9490c43203b 100644 --- a/src/Interpreters/Context.cpp +++ b/src/Interpreters/Context.cpp @@ -1971,6 +1971,9 @@ zkutil::ZooKeeperPtr Context::getAuxiliaryZooKeeper(const String & name) const auto zookeeper = shared->auxiliary_zookeepers.find(name); if (zookeeper == shared->auxiliary_zookeepers.end()) { + if (name.find(':') != std::string::npos || name.find('/') != std::string::npos) + throw Exception(ErrorCodes::BAD_ARGUMENTS, "Invalid auxiliary ZooKeeper name {}: ':' and '/' are not allowed", name); + const auto & config = shared->auxiliary_zookeepers_config ? *shared->auxiliary_zookeepers_config : getConfigRef(); if (!config.has("auxiliary_zookeepers." + name)) throw Exception( diff --git a/src/Storages/StorageReplicatedMergeTree.cpp b/src/Storages/StorageReplicatedMergeTree.cpp index c8ec9b91211..ff2676cd3e9 100644 --- a/src/Storages/StorageReplicatedMergeTree.cpp +++ b/src/Storages/StorageReplicatedMergeTree.cpp @@ -192,43 +192,54 @@ zkutil::ZooKeeperPtr StorageReplicatedMergeTree::getZooKeeper() const return res; } -static std::string normalizeZooKeeperPath(std::string zookeeper_path) +static std::string normalizeZooKeeperPath(std::string zookeeper_path, bool check_starts_with_slash, Poco::Logger * log = nullptr) { if (!zookeeper_path.empty() && zookeeper_path.back() == '/') zookeeper_path.resize(zookeeper_path.size() - 1); /// If zookeeper chroot prefix is used, path should start with '/', because chroot concatenates without it. if (!zookeeper_path.empty() && zookeeper_path.front() != '/') + { + /// Do not allow this for new tables, print warning for tables created in old versions + if (check_starts_with_slash) + throw Exception(ErrorCodes::BAD_ARGUMENTS, "ZooKeeper path must starts with '/', got '{}'", zookeeper_path); + if (log) + LOG_WARNING(log, "ZooKeeper path ('{}') does not start with '/'. It will not be supported in future releases"); zookeeper_path = "/" + zookeeper_path; + } return zookeeper_path; } static String extractZooKeeperName(const String & path) { + static constexpr auto default_zookeeper_name = "default"; if (path.empty()) - throw Exception("ZooKeeper path should not be empty", ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT); - auto pos = path.find(':'); - if (pos != String::npos) + throw Exception("ZooKeeper path should not be empty", ErrorCodes::BAD_ARGUMENTS); + if (path[0] == '/') + return default_zookeeper_name; + auto pos = path.find(":/"); + if (pos != String::npos && pos < path.find('/')) { auto zookeeper_name = path.substr(0, pos); if (zookeeper_name.empty()) - throw Exception("Zookeeper path should start with '/' or ':/'", ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT); + throw Exception("Zookeeper path should start with '/' or ':/'", ErrorCodes::BAD_ARGUMENTS); return zookeeper_name; } - static constexpr auto default_zookeeper_name = "default"; return default_zookeeper_name; } -static String extractZooKeeperPath(const String & path) +static String extractZooKeeperPath(const String & path, bool check_starts_with_slash, Poco::Logger * log = nullptr) { if (path.empty()) - throw Exception("ZooKeeper path should not be empty", ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT); - auto pos = path.find(':'); - if (pos != String::npos) + throw Exception("ZooKeeper path should not be empty", ErrorCodes::BAD_ARGUMENTS); + if (path[0] == '/') + return path; + auto pos = path.find(":/"); + if (pos != String::npos && pos < path.find('/')) { - return normalizeZooKeeperPath(path.substr(pos + 1, String::npos)); + return normalizeZooKeeperPath(path.substr(pos + 1, String::npos), check_starts_with_slash, log); } - return normalizeZooKeeperPath(path); + return normalizeZooKeeperPath(path, check_starts_with_slash, log); } static MergeTreePartInfo makeDummyDropRangeForMovePartitionOrAttachPartitionFrom(const String & partition_id) @@ -275,7 +286,7 @@ StorageReplicatedMergeTree::StorageReplicatedMergeTree( attach, [this] (const std::string & name) { enqueuePartForCheck(name); }) , zookeeper_name(extractZooKeeperName(zookeeper_path_)) - , zookeeper_path(extractZooKeeperPath(zookeeper_path_)) + , zookeeper_path(extractZooKeeperPath(zookeeper_path_, /* check_starts_with_slash */ !attach, log)) , replica_name(replica_name_) , replica_path(fs::path(zookeeper_path) / "replicas" / replica_name_) , reader(*this) @@ -5425,7 +5436,7 @@ void StorageReplicatedMergeTree::fetchPartition( info.table_id.uuid = UUIDHelpers::Nil; auto expand_from = query_context->getMacros()->expand(from_, info); String auxiliary_zookeeper_name = extractZooKeeperName(expand_from); - String from = extractZooKeeperPath(expand_from); + String from = extractZooKeeperPath(expand_from, /* check_starts_with_slash */ true); if (from.empty()) throw Exception("ZooKeeper path should not be empty", ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT); @@ -6490,7 +6501,7 @@ void StorageReplicatedMergeTree::movePartitionToShard( if (!move_part) throw Exception("MOVE PARTITION TO SHARD is not supported, use MOVE PART instead", ErrorCodes::NOT_IMPLEMENTED); - if (normalizeZooKeeperPath(zookeeper_path) == normalizeZooKeeperPath(to)) + if (normalizeZooKeeperPath(zookeeper_path, /* check_starts_with_slash */ true) == normalizeZooKeeperPath(to, /* check_starts_with_slash */ true)) throw Exception("Source and destination are the same", ErrorCodes::BAD_ARGUMENTS); auto zookeeper = getZooKeeper(); diff --git a/tests/integration/test_replicated_merge_tree_with_auxiliary_zookeepers/test.py b/tests/integration/test_replicated_merge_tree_with_auxiliary_zookeepers/test.py index a9dcce1b9d4..4644790ff94 100644 --- a/tests/integration/test_replicated_merge_tree_with_auxiliary_zookeepers/test.py +++ b/tests/integration/test_replicated_merge_tree_with_auxiliary_zookeepers/test.py @@ -101,3 +101,13 @@ def test_drop_replicated_merge_tree_with_auxiliary_zookeeper(started_cluster): assert zk.exists('/clickhouse/tables/test/test_auxiliary_zookeeper') drop_table([node1, node2], "test_auxiliary_zookeeper") assert zk.exists('/clickhouse/tables/test/test_auxiliary_zookeeper') is None + +def test_path_ambiguity(started_cluster): + drop_table([node1, node2], "test_path_ambiguity1") + drop_table([node1, node2], "test_path_ambiguity2") + node1.query("create table test_path_ambiguity1 (n int) engine=ReplicatedMergeTree('/test:bad:/path', '1') order by n") + assert "Invalid auxiliary ZooKeeper name" in node1.query_and_get_error("create table test_path_ambiguity2 (n int) engine=ReplicatedMergeTree('test:bad:/path', '1') order by n") + assert "ZooKeeper path must starts with '/'" in node1.query_and_get_error("create table test_path_ambiguity2 (n int) engine=ReplicatedMergeTree('test/bad:/path', '1') order by n") + node1.query("create table test_path_ambiguity2 (n int) engine=ReplicatedMergeTree('zookeeper2:/bad:/path', '1') order by n") + drop_table([node1, node2], "test_path_ambiguity1") + drop_table([node1, node2], "test_path_ambiguity2")