From 394317b91754f2b87ba503bc500a0692413954ba Mon Sep 17 00:00:00 2001 From: unashi Date: Mon, 26 Aug 2024 18:12:51 +0800 Subject: [PATCH 01/49] [feature] Add 'check grant' to see if (current user has been granted certain access rights elements && the elements exist). --- .../Access/InterpreterCheckGrantQuery.cpp | 114 +++++++++ .../Access/InterpreterCheckGrantQuery.h | 26 ++ src/Interpreters/InterpreterFactory.cpp | 5 + src/Interpreters/registerInterpreters.cpp | 2 + src/Parsers/Access/ASTCheckGrantQuery.cpp | 30 +++ src/Parsers/Access/ASTCheckGrantQuery.h | 27 ++ src/Parsers/Access/ParserCheckGrantQuery.cpp | 232 ++++++++++++++++++ src/Parsers/Access/ParserCheckGrantQuery.h | 17 ++ src/Parsers/CommonParsers.h | 1 + src/Parsers/ParserQuery.cpp | 3 + 10 files changed, 457 insertions(+) create mode 100644 src/Interpreters/Access/InterpreterCheckGrantQuery.cpp create mode 100644 src/Interpreters/Access/InterpreterCheckGrantQuery.h create mode 100644 src/Parsers/Access/ASTCheckGrantQuery.cpp create mode 100644 src/Parsers/Access/ASTCheckGrantQuery.h create mode 100644 src/Parsers/Access/ParserCheckGrantQuery.cpp create mode 100644 src/Parsers/Access/ParserCheckGrantQuery.h diff --git a/src/Interpreters/Access/InterpreterCheckGrantQuery.cpp b/src/Interpreters/Access/InterpreterCheckGrantQuery.cpp new file mode 100644 index 00000000000..7a01380e343 --- /dev/null +++ b/src/Interpreters/Access/InterpreterCheckGrantQuery.cpp @@ -0,0 +1,114 @@ +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include "Databases/IDatabase.h" +#include "Storages/IStorage.h" + +namespace DB +{ + +namespace +{ + /// Extracts access rights elements which are going to check grant + void collectAccessRightsElementsToGrantOrRevoke( + const ASTCheckGrantQuery & query, + AccessRightsElements & elements_to_check_grant) + { + elements_to_check_grant.clear(); + /// GRANT + elements_to_check_grant = query.access_rights_elements; + } +} + + +BlockIO InterpreterCheckGrantQuery::execute() +{ + auto & query = query_ptr->as(); + query.access_rights_elements.eraseNonGrantable(); + + auto current_user_access = getContext()->getAccess(); + + /// Collect access rights elements which will be checked. + AccessRightsElements elements_to_check_grant; + collectAccessRightsElementsToGrantOrRevoke(query, elements_to_check_grant); + + + /// Replacing empty database with the default. This step must be done before replication to avoid privilege escalation. + String current_database = getContext()->getCurrentDatabase(); + elements_to_check_grant.replaceEmptyDatabase(current_database); + query.access_rights_elements.replaceEmptyDatabase(current_database); + auto *logger = &::Poco::Logger::get("CheckGrantQuery"); + + /// Check If Table/Columns exist. + for (const auto & elem : elements_to_check_grant) + { + try + { + DatabasePtr database; + database = DatabaseCatalog::instance().getDatabase(elem.database); + if (!database->isTableExist(elem.table, getContext())) + { + /// Table not found. + return executeQuery("SELECT 0 AS CHECK_GRANT", getContext(), QueryFlags{.internal = true}).second; + } + auto table = database->getTable(elem.table, getContext()); + + auto column_name_with_sizes = table->getColumnSizes(); + for (const auto & elem_col : elem.columns) + { + bool founded = false; + for (const auto & col_in_table : column_name_with_sizes) + { + if (col_in_table.first == elem_col) + { + founded = true; + break; + } + } + if (!founded) + { + /// Column not found. + return executeQuery("SELECT 0 AS CHECK_GRANT", getContext(), QueryFlags{.internal = true}).second; + } + } + } + catch (...) + { + tryLogCurrentException(logger); + return executeQuery("SELECT 0 AS CHECK_GRANT", getContext(), QueryFlags{.internal = true}).second; + } + } + bool user_is_granted = current_user_access->isGranted(elements_to_check_grant); + if (!user_is_granted) + { + return executeQuery("SELECT 0 AS CHECK_GRANT", getContext(), QueryFlags{.internal = true}).second; + } + + return executeQuery("SELECT 1 AS CHECK_GRANT", getContext(), QueryFlags{.internal = true}).second; +} + +void registerInterpreterCheckGrantQuery(InterpreterFactory & factory) +{ + auto create_fn = [] (const InterpreterFactory::Arguments & args) + { + return std::make_unique(args.query, args.context); + }; + factory.registerInterpreter("InterpreterCheckGrantQuery", create_fn); +} + +} diff --git a/src/Interpreters/Access/InterpreterCheckGrantQuery.h b/src/Interpreters/Access/InterpreterCheckGrantQuery.h new file mode 100644 index 00000000000..e79d6925c93 --- /dev/null +++ b/src/Interpreters/Access/InterpreterCheckGrantQuery.h @@ -0,0 +1,26 @@ +#pragma once + +#include +#include +#include + + +namespace DB +{ + +class ASTCheckGrantQuery; +struct User; +struct Role; + +class InterpreterCheckGrantQuery : public IInterpreter, WithMutableContext +{ +public: + InterpreterCheckGrantQuery(const ASTPtr & query_ptr_, ContextMutablePtr context_) : WithMutableContext(context_), query_ptr(query_ptr_) {} + + BlockIO execute() override; + +private: + ASTPtr query_ptr; +}; + +} diff --git a/src/Interpreters/InterpreterFactory.cpp b/src/Interpreters/InterpreterFactory.cpp index cfc95124895..7265f45232e 100644 --- a/src/Interpreters/InterpreterFactory.cpp +++ b/src/Interpreters/InterpreterFactory.cpp @@ -60,6 +60,7 @@ #include #include #include +#include "Parsers/Access/ASTCheckGrantQuery.h" #include @@ -304,6 +305,10 @@ InterpreterFactory::InterpreterPtr InterpreterFactory::get(ASTPtr & query, Conte { interpreter_name = "InterpreterShowGrantsQuery"; } + else if (query->as()) + { + interpreter_name = "InterpreterCheckGrantQuery"; + } else if (query->as()) { interpreter_name = "InterpreterShowAccessEntitiesQuery"; diff --git a/src/Interpreters/registerInterpreters.cpp b/src/Interpreters/registerInterpreters.cpp index 481d0597a85..7d8ed17a11f 100644 --- a/src/Interpreters/registerInterpreters.cpp +++ b/src/Interpreters/registerInterpreters.cpp @@ -45,6 +45,7 @@ void registerInterpreterDropNamedCollectionQuery(InterpreterFactory & factory); void registerInterpreterGrantQuery(InterpreterFactory & factory); void registerInterpreterShowCreateAccessEntityQuery(InterpreterFactory & factory); void registerInterpreterShowGrantsQuery(InterpreterFactory & factory); +void registerInterpreterCheckGrantQuery(InterpreterFactory & factory); void registerInterpreterShowAccessEntitiesQuery(InterpreterFactory & factory); void registerInterpreterShowAccessQuery(InterpreterFactory & factory); void registerInterpreterShowPrivilegesQuery(InterpreterFactory & factory); @@ -104,6 +105,7 @@ void registerInterpreters() registerInterpreterGrantQuery(factory); registerInterpreterShowCreateAccessEntityQuery(factory); registerInterpreterShowGrantsQuery(factory); + registerInterpreterCheckGrantQuery(factory); registerInterpreterShowAccessEntitiesQuery(factory); registerInterpreterShowAccessQuery(factory); registerInterpreterShowPrivilegesQuery(factory); diff --git a/src/Parsers/Access/ASTCheckGrantQuery.cpp b/src/Parsers/Access/ASTCheckGrantQuery.cpp new file mode 100644 index 00000000000..364e73920ab --- /dev/null +++ b/src/Parsers/Access/ASTCheckGrantQuery.cpp @@ -0,0 +1,30 @@ +#include +#include +#include "Common/logger_useful.h" +#include +#include + + +namespace DB +{ + +String ASTCheckGrantQuery::getID(char) const +{ + return "CheckGrantQuery"; +} + + +ASTPtr ASTCheckGrantQuery::clone() const +{ + auto res = std::make_shared(*this); + + return res; +} + + +void ASTCheckGrantQuery::replaceEmptyDatabase(const String & current_database) +{ + access_rights_elements.replaceEmptyDatabase(current_database); +} + +} diff --git a/src/Parsers/Access/ASTCheckGrantQuery.h b/src/Parsers/Access/ASTCheckGrantQuery.h new file mode 100644 index 00000000000..7e363d587ce --- /dev/null +++ b/src/Parsers/Access/ASTCheckGrantQuery.h @@ -0,0 +1,27 @@ +#pragma once + +#include +#include +#include +#include "Parsers/ASTQueryParameter.h" + + +namespace DB +{ +class ASTRolesOrUsersSet; + + +/** Parses queries like + * CHECK GRANT access_type[(column_name [,...])] [,...] ON {db.table|db.*|*.*|table|*} + */ +class ASTCheckGrantQuery : public IAST +{ +public: + AccessRightsElements access_rights_elements; + + String getID(char) const override; + ASTPtr clone() const override; + void replaceEmptyDatabase(const String & current_database); + QueryKind getQueryKind() const override { return QueryKind::Check; } +}; +} diff --git a/src/Parsers/Access/ParserCheckGrantQuery.cpp b/src/Parsers/Access/ParserCheckGrantQuery.cpp new file mode 100644 index 00000000000..c1daa435ad2 --- /dev/null +++ b/src/Parsers/Access/ParserCheckGrantQuery.cpp @@ -0,0 +1,232 @@ +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + + +namespace DB +{ +namespace ErrorCodes +{ + extern const int INVALID_GRANT; + extern const int SYNTAX_ERROR; +} + +namespace +{ + bool parseAccessFlags(IParser::Pos & pos, Expected & expected, AccessFlags & access_flags) + { + static constexpr auto is_one_of_access_type_words = [](IParser::Pos & pos_) + { + if (pos_->type != TokenType::BareWord) + return false; + std::string_view word{pos_->begin, pos_->size()}; + return !(boost::iequals(word, toStringView(Keyword::ON)) || boost::iequals(word, toStringView(Keyword::TO)) || boost::iequals(word, toStringView(Keyword::FROM))); + }; + + expected.add(pos, "access type"); + + return IParserBase::wrapParseImpl(pos, [&] + { + if (!is_one_of_access_type_words(pos)) + return false; + + String str; + do + { + if (!str.empty()) + str += " "; + str += std::string_view(pos->begin, pos->size()); + ++pos; + } + while (is_one_of_access_type_words(pos)); + + try + { + access_flags = AccessFlags{str}; + } + catch (...) + { + return false; + } + + return true; + }); + } + + + bool parseColumnNames(IParser::Pos & pos, Expected & expected, Strings & columns) + { + return IParserBase::wrapParseImpl(pos, [&] + { + if (!ParserToken{TokenType::OpeningRoundBracket}.ignore(pos, expected)) + return false; + + ASTPtr ast; + if (!ParserList{std::make_unique(), std::make_unique(TokenType::Comma), false}.parse(pos, ast, expected)) + return false; + + Strings res_columns; + for (const auto & child : ast->children) + res_columns.emplace_back(getIdentifierName(child)); + + if (!ParserToken{TokenType::ClosingRoundBracket}.ignore(pos, expected)) + return false; + + columns = std::move(res_columns); + return true; + }); + } + + bool parseAccessFlagsWithColumns(IParser::Pos & pos, Expected & expected, + std::vector> & access_and_columns) + { + std::vector> res; + + auto parse_access_and_columns = [&] + { + AccessFlags access_flags; + if (!parseAccessFlags(pos, expected, access_flags)) + return false; + + Strings columns; + parseColumnNames(pos, expected, columns); + res.emplace_back(access_flags, std::move(columns)); + return true; + }; + + if (!ParserList::parseUtil(pos, expected, parse_access_and_columns, false)) + return false; + + access_and_columns = std::move(res); + return true; + } + + + bool parseElementsWithoutOptions(IParser::Pos & pos, Expected & expected, AccessRightsElements & elements) + { + return IParserBase::wrapParseImpl(pos, [&] + { + AccessRightsElements res_elements; + + auto parse_around_on = [&] + { + std::vector> access_and_columns; + if (!parseAccessFlagsWithColumns(pos, expected, access_and_columns)) + return false; + + String database_name, table_name, parameter; + bool any_database = false, any_table = false, any_parameter = false; + + size_t is_global_with_parameter = 0; + for (const auto & elem : access_and_columns) + { + if (elem.first.isGlobalWithParameter()) + ++is_global_with_parameter; + } + + if (!ParserKeyword{Keyword::ON}.ignore(pos, expected)) + return false; + + if (is_global_with_parameter && is_global_with_parameter == access_and_columns.size()) + { + ASTPtr parameter_ast; + if (ParserToken{TokenType::Asterisk}.ignore(pos, expected)) + { + any_parameter = true; + } + else if (ParserIdentifier{}.parse(pos, parameter_ast, expected)) + { + any_parameter = false; + parameter = getIdentifierName(parameter_ast); + } + else + return false; + + any_database = any_table = true; + } + else if (!parseDatabaseAndTableNameOrAsterisks(pos, expected, database_name, any_database, table_name, any_table)) + { + return false; + } + + for (auto & [access_flags, columns] : access_and_columns) + { + AccessRightsElement element; + element.access_flags = access_flags; + element.any_column = columns.empty(); + element.columns = std::move(columns); + element.any_database = any_database; + element.database = database_name; + element.any_table = any_table; + element.any_parameter = any_parameter; + element.table = table_name; + element.parameter = parameter; + res_elements.emplace_back(std::move(element)); + } + + return true; + }; + + if (!ParserList::parseUtil(pos, expected, parse_around_on, false)) + return false; + + elements = std::move(res_elements); + return true; + }); + } + + void throwIfNotGrantable(AccessRightsElements & elements) + { + std::erase_if(elements, [](AccessRightsElement & element) + { + if (element.empty()) + return true; + auto old_flags = element.access_flags; + element.eraseNonGrantable(); + if (!element.empty()) + return false; + + if (!element.any_column) + throw Exception(ErrorCodes::INVALID_GRANT, "{} cannot check grant on the column level", old_flags.toString()); + else if (!element.any_table) + throw Exception(ErrorCodes::INVALID_GRANT, "{} cannot check grant on the table level", old_flags.toString()); + else if (!element.any_database) + throw Exception(ErrorCodes::INVALID_GRANT, "{} cannot check grant on the database level", old_flags.toString()); + else if (!element.any_parameter) + throw Exception(ErrorCodes::INVALID_GRANT, "{} cannot check grant on the global with parameter level", old_flags.toString()); + else + throw Exception(ErrorCodes::INVALID_GRANT, "{} cannot check grant", old_flags.toString()); + }); + } +} + + +bool ParserCheckGrantQuery::parseImpl(Pos & pos, ASTPtr & node, Expected & expected) +{ + if (!ParserKeyword{Keyword::CHECK_GRANT}.ignore(pos, expected)) + return false; + + + AccessRightsElements elements; + + if (!parseElementsWithoutOptions(pos, expected, elements) ) + return false; + + throwIfNotGrantable(elements); + + auto query = std::make_shared(); + node = query; + + query->access_rights_elements = std::move(elements); + + return true; +} +} diff --git a/src/Parsers/Access/ParserCheckGrantQuery.h b/src/Parsers/Access/ParserCheckGrantQuery.h new file mode 100644 index 00000000000..d58ea6002a8 --- /dev/null +++ b/src/Parsers/Access/ParserCheckGrantQuery.h @@ -0,0 +1,17 @@ +#pragma once + +#include + + +namespace DB +{ +/** Parses queries like + * CHECK GRANT access_type[(column_name [,...])] [,...] ON {db.table|db.*|*.*|table|*} + */ +class ParserCheckGrantQuery : public IParserBase +{ +protected: + const char * getName() const override { return "CHECK GRANT"; } + bool parseImpl(Pos & pos, ASTPtr & node, Expected & expected) override; +}; +} diff --git a/src/Parsers/CommonParsers.h b/src/Parsers/CommonParsers.h index 00be5aa6a05..fc5c4494884 100644 --- a/src/Parsers/CommonParsers.h +++ b/src/Parsers/CommonParsers.h @@ -79,6 +79,7 @@ namespace DB MR_MACROS(CHARACTER, "CHARACTER") \ MR_MACROS(CHECK_ALL_TABLES, "CHECK ALL TABLES") \ MR_MACROS(CHECK_TABLE, "CHECK TABLE") \ + MR_MACROS(CHECK_GRANT, "CHECK GRANT")\ MR_MACROS(CHECK, "CHECK") \ MR_MACROS(CLEANUP, "CLEANUP") \ MR_MACROS(CLEAR_COLUMN, "CLEAR COLUMN") \ diff --git a/src/Parsers/ParserQuery.cpp b/src/Parsers/ParserQuery.cpp index 22ddc25019f..92a83fe99f1 100644 --- a/src/Parsers/ParserQuery.cpp +++ b/src/Parsers/ParserQuery.cpp @@ -28,6 +28,7 @@ #include #include #include +#include "Parsers/Access/ParserCheckGrantQuery.h" namespace DB @@ -56,6 +57,7 @@ bool ParserQuery::parseImpl(Pos & pos, ASTPtr & node, Expected & expected) ParserDropAccessEntityQuery drop_access_entity_p; ParserMoveAccessEntityQuery move_access_entity_p; ParserGrantQuery grant_p; + ParserCheckGrantQuery check_grant_p; ParserSetRoleQuery set_role_p; ParserExternalDDLQuery external_ddl_p; ParserTransactionControl transaction_control_p; @@ -82,6 +84,7 @@ bool ParserQuery::parseImpl(Pos & pos, ASTPtr & node, Expected & expected) || drop_access_entity_p.parse(pos, node, expected) || move_access_entity_p.parse(pos, node, expected) || grant_p.parse(pos, node, expected) + || check_grant_p.parse(pos, node, expected) || external_ddl_p.parse(pos, node, expected) || transaction_control_p.parse(pos, node, expected) || delete_p.parse(pos, node, expected); From bae1aef42e52827e6e7aad62ed80631ef7cdd730 Mon Sep 17 00:00:00 2001 From: unashi Date: Mon, 26 Aug 2024 18:13:40 +0800 Subject: [PATCH 02/49] [test] Add test case --- .../integration/test_check_grant/__init__.py | 0 .../configs/remote_servers.xml | 17 +++ .../test_check_grant/configs/users.xml | 121 ++++++++++++++++++ tests/integration/test_check_grant/test.py | 60 +++++++++ 4 files changed, 198 insertions(+) create mode 100644 tests/integration/test_check_grant/__init__.py create mode 100644 tests/integration/test_check_grant/configs/remote_servers.xml create mode 100644 tests/integration/test_check_grant/configs/users.xml create mode 100644 tests/integration/test_check_grant/test.py diff --git a/tests/integration/test_check_grant/__init__.py b/tests/integration/test_check_grant/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/integration/test_check_grant/configs/remote_servers.xml b/tests/integration/test_check_grant/configs/remote_servers.xml new file mode 100644 index 00000000000..ea4769f55e1 --- /dev/null +++ b/tests/integration/test_check_grant/configs/remote_servers.xml @@ -0,0 +1,17 @@ + + + + + true + + node1 + 9000 + + + node2 + 9000 + + + + + diff --git a/tests/integration/test_check_grant/configs/users.xml b/tests/integration/test_check_grant/configs/users.xml new file mode 100644 index 00000000000..4a56df660df --- /dev/null +++ b/tests/integration/test_check_grant/configs/users.xml @@ -0,0 +1,121 @@ + + + + + + + + + + + + 1 + + + + + + + + + + + + + ::/0 + + + + default + + + default + + + 1 + + + 1 + 1 + 1 + + + + + + + + + + + + + 3600 + + + 0 + 0 + 0 + 0 + 0 + + + + diff --git a/tests/integration/test_check_grant/test.py b/tests/integration/test_check_grant/test.py new file mode 100644 index 00000000000..e22d5dcaa5f --- /dev/null +++ b/tests/integration/test_check_grant/test.py @@ -0,0 +1,60 @@ +#!/usr/bin/env python3 + +import logging +import pytest +from helpers.cluster import ClickHouseCluster + +@pytest.fixture(scope="module") +def cluster(): + try: + cluster = ClickHouseCluster(__file__) + cluster.add_instance( + "node1", + user_configs=[ + "configs/users.xml" + ] + ) + + cluster.add_instance( + "node2", + user_configs=[ + "configs/users.xml", + ] + ) + logging.info("Starting cluster...") + cluster.start() + logging.info("Cluster started") + + yield cluster + finally: + cluster.shutdown() + + +def test_check_grant(cluster): + node1 = cluster.instances["node1"] + + node1.query("DROP user IF EXISTS tuser") + node1.query("CREATE USER tuser") + node1.query("GRANT SELECT ON tb TO tuser") + # Has been granted but not table not exists + res, _ = node1.query("CHECK GRANT SELECT ON tb", user = "tuser") + assert res == "0" + + node1.query("CREATE TABLE tb (`content` UInt64) ENGINE = MergeTree ORDER BY content") + node1.query("INSERT INTO tb VALUES (1)") + # Has been granted and table exists + res, _ = node1.query("CHECK GRANT SELECT ON tb", user = "tuser") + assert res == "1" + + node1.query("REVOKE SELECT ON tb FROM tuser") + # Has not been granted but table exists + res, _ = node1.query("CHECK GRANT SELECT ON tb", user = "tuser") + assert res == "0" + + # Role + node1.query("CREATE ROLE trole") + node1.query("GRANT SELECT ON tb TO trole") + node1.query("GRANT trole TO tuser") + node1.query("SET ROLE trole", user = "tuser") + res, _ = node1.query("CHECK GRANT SELECT ON tb", user = "tuser") + assert res == "1" From c29008528fe5546101c093fbc4caaef0381f8e3d Mon Sep 17 00:00:00 2001 From: unashi Date: Mon, 26 Aug 2024 20:33:09 +0800 Subject: [PATCH 03/49] [fix] Add formatImpl in AST --- src/Parsers/Access/ASTCheckGrantQuery.cpp | 98 ++++++++++++++++++++++- src/Parsers/Access/ASTCheckGrantQuery.h | 2 +- 2 files changed, 98 insertions(+), 2 deletions(-) diff --git a/src/Parsers/Access/ASTCheckGrantQuery.cpp b/src/Parsers/Access/ASTCheckGrantQuery.cpp index 364e73920ab..0f23fcce48a 100644 --- a/src/Parsers/Access/ASTCheckGrantQuery.cpp +++ b/src/Parsers/Access/ASTCheckGrantQuery.cpp @@ -1,6 +1,5 @@ #include #include -#include "Common/logger_useful.h" #include #include @@ -8,6 +7,92 @@ namespace DB { +namespace +{ + void formatColumnNames(const Strings & columns, const IAST::FormatSettings & settings) + { + settings.ostr << "("; + bool need_comma = false; + for (const auto & column : columns) + { + if (std::exchange(need_comma, true)) + settings.ostr << ", "; + settings.ostr << backQuoteIfNeed(column); + } + settings.ostr << ")"; + } + + + void formatONClause(const AccessRightsElement & element, const IAST::FormatSettings & settings) + { + settings.ostr << (settings.hilite ? IAST::hilite_keyword : "") << "ON " << (settings.hilite ? IAST::hilite_none : ""); + if (element.isGlobalWithParameter()) + { + if (element.any_parameter) + settings.ostr << "*"; + else + settings.ostr << backQuoteIfNeed(element.parameter); + } + else if (element.any_database) + { + settings.ostr << "*.*"; + } + else + { + if (!element.database.empty()) + settings.ostr << backQuoteIfNeed(element.database) << "."; + if (element.any_table) + settings.ostr << "*"; + else + settings.ostr << backQuoteIfNeed(element.table); + } + } + + + void formatElementsWithoutOptions(const AccessRightsElements & elements, const IAST::FormatSettings & settings) + { + bool no_output = true; + for (size_t i = 0; i != elements.size(); ++i) + { + const auto & element = elements[i]; + auto keywords = element.access_flags.toKeywords(); + if (keywords.empty() || (!element.any_column && element.columns.empty())) + continue; + + for (const auto & keyword : keywords) + { + if (!std::exchange(no_output, false)) + settings.ostr << ", "; + + settings.ostr << (settings.hilite ? IAST::hilite_keyword : "") << keyword << (settings.hilite ? IAST::hilite_none : ""); + if (!element.any_column) + formatColumnNames(element.columns, settings); + } + + bool next_element_on_same_db_and_table = false; + if (i != elements.size() - 1) + { + const auto & next_element = elements[i + 1]; + if (element.sameDatabaseAndTableAndParameter(next_element)) + { + next_element_on_same_db_and_table = true; + } + } + + if (!next_element_on_same_db_and_table) + { + settings.ostr << " "; + formatONClause(element, settings); + } + } + + if (no_output) + settings.ostr << (settings.hilite ? IAST::hilite_keyword : "") << "USAGE ON " << (settings.hilite ? IAST::hilite_none : "") << "*.*"; + } + +} + + String ASTCheckGrantQuery::getID(char) const { return "CheckGrantQuery"; @@ -22,6 +107,17 @@ ASTPtr ASTCheckGrantQuery::clone() const } +void ASTCheckGrantQuery::formatImpl(const FormatSettings & settings, FormatState &, FormatStateStacked) const +{ + settings.ostr << (settings.hilite ? IAST::hilite_keyword : "") << "CHECK GRANT" + << (settings.hilite ? IAST::hilite_none : ""); + + settings.ostr << " "; + + formatElementsWithoutOptions(access_rights_elements, settings); +} + + void ASTCheckGrantQuery::replaceEmptyDatabase(const String & current_database) { access_rights_elements.replaceEmptyDatabase(current_database); diff --git a/src/Parsers/Access/ASTCheckGrantQuery.h b/src/Parsers/Access/ASTCheckGrantQuery.h index 7e363d587ce..567ffdb289a 100644 --- a/src/Parsers/Access/ASTCheckGrantQuery.h +++ b/src/Parsers/Access/ASTCheckGrantQuery.h @@ -3,7 +3,6 @@ #include #include #include -#include "Parsers/ASTQueryParameter.h" namespace DB @@ -21,6 +20,7 @@ public: String getID(char) const override; ASTPtr clone() const override; + void formatImpl(const FormatSettings & settings, FormatState &, FormatStateStacked) const override; void replaceEmptyDatabase(const String & current_database); QueryKind getQueryKind() const override { return QueryKind::Check; } }; From 75dc880c2eb924b4c6aafe2973e771c2e34dfe65 Mon Sep 17 00:00:00 2001 From: unashi Date: Mon, 26 Aug 2024 20:40:04 +0800 Subject: [PATCH 04/49] [doc] Add doc --- .../sql-reference/statements/check-grant.md | 43 +++++++++++++++++++ .../sql-reference/statements/check-grant.mdx | 10 +++++ 2 files changed, 53 insertions(+) create mode 100644 docs/en/sql-reference/statements/check-grant.md create mode 100644 docs/zh/sql-reference/statements/check-grant.mdx diff --git a/docs/en/sql-reference/statements/check-grant.md b/docs/en/sql-reference/statements/check-grant.md new file mode 100644 index 00000000000..a1971eb59d1 --- /dev/null +++ b/docs/en/sql-reference/statements/check-grant.md @@ -0,0 +1,43 @@ +--- +slug: /en/sql-reference/statements/check-grant +sidebar_position: 56 +sidebar_label: CHECK GRANT +title: "CHECK GRANT Statement" +--- + +The `CHECK GRANT` query is used to check whether the current user/role has been granted a specific privilege, and whether the corresponding table/column exists in the memory. + +## Syntax + +The basic syntax of the query is as follows: + +```sql +CHECK GRANT privilege[(column_name [,...])] [,...] ON {db.table|db.*|*.*|table|*} +``` + +- `privilege` — Type of privilege. + +## Examples + +If the user used to be granted the privilege, or the role (which is granted with the privilege), and the db.table(column) exists on this node, the response`check_grant` will be `1`. Otherwise, the response `check_grant` will be `0`. + +If `table_1.col1` exists and current user is granted by privilege `SELECT`/`SELECT(con)` or role(with privilege), the response is `1`. +```sql +CHECK GRANT SELECT(col1) ON table_1; +``` + +```text +┌─CHECK_GRANT─┐ +│ 1 │ +└─────────────┘ +``` +If `table_2.col2` doesn't exists, or current user is not granted by privilege `SELECT`/`SELECT(con)` or role(with privilege), the response is `0`. +```sql +CHECK GRANT SELECT(col2) ON table_2; +``` + +```text +┌─CHECK_GRANT─┐ +│ 0 │ +└─────────────┘ +``` diff --git a/docs/zh/sql-reference/statements/check-grant.mdx b/docs/zh/sql-reference/statements/check-grant.mdx new file mode 100644 index 00000000000..60c95699a5e --- /dev/null +++ b/docs/zh/sql-reference/statements/check-grant.mdx @@ -0,0 +1,10 @@ +--- +slug: /zh/sql-reference/statements/check-grant +sidebar_position: 56 +sidebar_label: CHECK +title: "CHECK GRANT Statement" +--- + +import Content from '@site/docs/en/sql-reference/statements/check-grant.md'; + + From a7d693573bbc470d66ea3897ef274d27596830ee Mon Sep 17 00:00:00 2001 From: unashi Date: Mon, 26 Aug 2024 20:46:04 +0800 Subject: [PATCH 05/49] [fix] Fix include format --- src/Interpreters/InterpreterFactory.cpp | 2 +- src/Parsers/ParserQuery.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Interpreters/InterpreterFactory.cpp b/src/Interpreters/InterpreterFactory.cpp index 7265f45232e..a2b990d09d3 100644 --- a/src/Interpreters/InterpreterFactory.cpp +++ b/src/Interpreters/InterpreterFactory.cpp @@ -40,6 +40,7 @@ #include #include #include +#include #include #include #include @@ -60,7 +61,6 @@ #include #include #include -#include "Parsers/Access/ASTCheckGrantQuery.h" #include diff --git a/src/Parsers/ParserQuery.cpp b/src/Parsers/ParserQuery.cpp index 92a83fe99f1..e939cdd07ba 100644 --- a/src/Parsers/ParserQuery.cpp +++ b/src/Parsers/ParserQuery.cpp @@ -26,9 +26,9 @@ #include #include #include +#include #include #include -#include "Parsers/Access/ParserCheckGrantQuery.h" namespace DB From 26cab169aa95156d3ddd2d0dd9777519e433fa19 Mon Sep 17 00:00:00 2001 From: unashi Date: Tue, 27 Aug 2024 10:25:07 +0800 Subject: [PATCH 06/49] [fix] Fix style check --- src/Parsers/Access/ParserCheckGrantQuery.cpp | 1 - tests/integration/test_check_grant/test.py | 24 +++++++++----------- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/src/Parsers/Access/ParserCheckGrantQuery.cpp b/src/Parsers/Access/ParserCheckGrantQuery.cpp index c1daa435ad2..4be3d2a0d9f 100644 --- a/src/Parsers/Access/ParserCheckGrantQuery.cpp +++ b/src/Parsers/Access/ParserCheckGrantQuery.cpp @@ -16,7 +16,6 @@ namespace DB namespace ErrorCodes { extern const int INVALID_GRANT; - extern const int SYNTAX_ERROR; } namespace diff --git a/tests/integration/test_check_grant/test.py b/tests/integration/test_check_grant/test.py index e22d5dcaa5f..c20dfe86b16 100644 --- a/tests/integration/test_check_grant/test.py +++ b/tests/integration/test_check_grant/test.py @@ -4,22 +4,18 @@ import logging import pytest from helpers.cluster import ClickHouseCluster + @pytest.fixture(scope="module") def cluster(): try: cluster = ClickHouseCluster(__file__) - cluster.add_instance( - "node1", - user_configs=[ - "configs/users.xml" - ] - ) + cluster.add_instance("node1", user_configs=["configs/users.xml"]) cluster.add_instance( "node2", user_configs=[ "configs/users.xml", - ] + ], ) logging.info("Starting cluster...") cluster.start() @@ -37,24 +33,26 @@ def test_check_grant(cluster): node1.query("CREATE USER tuser") node1.query("GRANT SELECT ON tb TO tuser") # Has been granted but not table not exists - res, _ = node1.query("CHECK GRANT SELECT ON tb", user = "tuser") + res, _ = node1.query("CHECK GRANT SELECT ON tb", user="tuser") assert res == "0" - node1.query("CREATE TABLE tb (`content` UInt64) ENGINE = MergeTree ORDER BY content") + node1.query( + "CREATE TABLE tb (`content` UInt64) ENGINE = MergeTree ORDER BY content" + ) node1.query("INSERT INTO tb VALUES (1)") # Has been granted and table exists - res, _ = node1.query("CHECK GRANT SELECT ON tb", user = "tuser") + res, _ = node1.query("CHECK GRANT SELECT ON tb", user="tuser") assert res == "1" node1.query("REVOKE SELECT ON tb FROM tuser") # Has not been granted but table exists - res, _ = node1.query("CHECK GRANT SELECT ON tb", user = "tuser") + res, _ = node1.query("CHECK GRANT SELECT ON tb", user="tuser") assert res == "0" # Role node1.query("CREATE ROLE trole") node1.query("GRANT SELECT ON tb TO trole") node1.query("GRANT trole TO tuser") - node1.query("SET ROLE trole", user = "tuser") - res, _ = node1.query("CHECK GRANT SELECT ON tb", user = "tuser") + node1.query("SET ROLE trole", user="tuser") + res, _ = node1.query("CHECK GRANT SELECT ON tb", user="tuser") assert res == "1" From a5d977bfc13bcfbb0576b036b8de9db9cc5b59db Mon Sep 17 00:00:00 2001 From: unashi Date: Tue, 27 Aug 2024 12:32:48 +0800 Subject: [PATCH 07/49] [Fix] Fix code style --- src/Parsers/Access/ParserCheckGrantQuery.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Parsers/Access/ParserCheckGrantQuery.cpp b/src/Parsers/Access/ParserCheckGrantQuery.cpp index 4be3d2a0d9f..c7433b7d4df 100644 --- a/src/Parsers/Access/ParserCheckGrantQuery.cpp +++ b/src/Parsers/Access/ParserCheckGrantQuery.cpp @@ -216,7 +216,7 @@ bool ParserCheckGrantQuery::parseImpl(Pos & pos, ASTPtr & node, Expected & expec AccessRightsElements elements; - if (!parseElementsWithoutOptions(pos, expected, elements) ) + if (!parseElementsWithoutOptions(pos, expected, elements)) return false; throwIfNotGrantable(elements); From 639560bd2d6093210a15a0c238e0f3fcf1043e3a Mon Sep 17 00:00:00 2001 From: unashi Date: Sun, 8 Sep 2024 16:09:01 +0800 Subject: [PATCH 08/49] [Update] Use stateless test instead of integration test --- .../0_stateless/03234_check_grant.reference | 4 ++++ .../queries/0_stateless/03234_check_grant.sh | 20 +++++++++++++++++++ 2 files changed, 24 insertions(+) create mode 100644 tests/queries/0_stateless/03234_check_grant.reference create mode 100755 tests/queries/0_stateless/03234_check_grant.sh diff --git a/tests/queries/0_stateless/03234_check_grant.reference b/tests/queries/0_stateless/03234_check_grant.reference new file mode 100644 index 00000000000..5565ed6787f --- /dev/null +++ b/tests/queries/0_stateless/03234_check_grant.reference @@ -0,0 +1,4 @@ +0 +1 +0 +1 diff --git a/tests/queries/0_stateless/03234_check_grant.sh b/tests/queries/0_stateless/03234_check_grant.sh new file mode 100755 index 00000000000..922f8fa96ac --- /dev/null +++ b/tests/queries/0_stateless/03234_check_grant.sh @@ -0,0 +1,20 @@ +#!/usr/bin/env bash +# Tags: no-parallel + +${CLICKHOUSE_CLIENT} --query "DROP USER IF EXISTS user_03234; DROP TABLE IF EXISTS tb;CREATE USER user_03234; GRANT SELECT ON tb TO user_03234;" + +# Has been granted but not table not exists +${CLICKHOUSE_CURL} -sS "${CLICKHOUSE_URL}&user=user_03234" --data-binary "CHECK GRANT SELECT ON tb" + +${CLICKHOUSE_CLIENT} --query "CREATE TABLE tb (\`content\` UInt64) ENGINE = MergeTree ORDER BY content; INSERT INTO tb VALUES (1);" +# Has been granted and table exists +${CLICKHOUSE_CURL} -sS "${CLICKHOUSE_URL}&user=user_03234" --data-binary "CHECK GRANT SELECT ON tb" + +${CLICKHOUSE_CLIENT} --query "REVOKE SELECT ON tb FROM user_03234;" +# Has not been granted but table exists +${CLICKHOUSE_CURL} -sS "${CLICKHOUSE_URL}&user=user_03234" --data-binary "CHECK GRANT SELECT ON tb" + +# Role +${CLICKHOUSE_CLIENT} --query "DROP ROLE IF EXISTS role_03234;CREATE ROLE role_03234;GRANT SELECT ON tb TO role_03234;GRANT role_03234 TO user_03234" +${CLICKHOUSE_CURL} -sS "${CLICKHOUSE_URL}&user=user_03234" --data-binary "SET ROLE role_03234" +${CLICKHOUSE_CURL} -sS "${CLICKHOUSE_URL}&user=user_03234" --data-binary "CHECK GRANT SELECT ON tb" From 636a5ccff54fbc53b29e66dafda49166ae848c72 Mon Sep 17 00:00:00 2001 From: unashi Date: Sun, 8 Sep 2024 16:11:30 +0800 Subject: [PATCH 09/49] [rm] Remove integration test --- .../integration/test_check_grant/__init__.py | 0 .../configs/remote_servers.xml | 17 --- .../test_check_grant/configs/users.xml | 121 ------------------ tests/integration/test_check_grant/test.py | 58 --------- 4 files changed, 196 deletions(-) delete mode 100644 tests/integration/test_check_grant/__init__.py delete mode 100644 tests/integration/test_check_grant/configs/remote_servers.xml delete mode 100644 tests/integration/test_check_grant/configs/users.xml delete mode 100644 tests/integration/test_check_grant/test.py diff --git a/tests/integration/test_check_grant/__init__.py b/tests/integration/test_check_grant/__init__.py deleted file mode 100644 index e69de29bb2d..00000000000 diff --git a/tests/integration/test_check_grant/configs/remote_servers.xml b/tests/integration/test_check_grant/configs/remote_servers.xml deleted file mode 100644 index ea4769f55e1..00000000000 --- a/tests/integration/test_check_grant/configs/remote_servers.xml +++ /dev/null @@ -1,17 +0,0 @@ - - - - - true - - node1 - 9000 - - - node2 - 9000 - - - - - diff --git a/tests/integration/test_check_grant/configs/users.xml b/tests/integration/test_check_grant/configs/users.xml deleted file mode 100644 index 4a56df660df..00000000000 --- a/tests/integration/test_check_grant/configs/users.xml +++ /dev/null @@ -1,121 +0,0 @@ - - - - - - - - - - - - 1 - - - - - - - - - - - - - ::/0 - - - - default - - - default - - - 1 - - - 1 - 1 - 1 - - - - - - - - - - - - - 3600 - - - 0 - 0 - 0 - 0 - 0 - - - - diff --git a/tests/integration/test_check_grant/test.py b/tests/integration/test_check_grant/test.py deleted file mode 100644 index c20dfe86b16..00000000000 --- a/tests/integration/test_check_grant/test.py +++ /dev/null @@ -1,58 +0,0 @@ -#!/usr/bin/env python3 - -import logging -import pytest -from helpers.cluster import ClickHouseCluster - - -@pytest.fixture(scope="module") -def cluster(): - try: - cluster = ClickHouseCluster(__file__) - cluster.add_instance("node1", user_configs=["configs/users.xml"]) - - cluster.add_instance( - "node2", - user_configs=[ - "configs/users.xml", - ], - ) - logging.info("Starting cluster...") - cluster.start() - logging.info("Cluster started") - - yield cluster - finally: - cluster.shutdown() - - -def test_check_grant(cluster): - node1 = cluster.instances["node1"] - - node1.query("DROP user IF EXISTS tuser") - node1.query("CREATE USER tuser") - node1.query("GRANT SELECT ON tb TO tuser") - # Has been granted but not table not exists - res, _ = node1.query("CHECK GRANT SELECT ON tb", user="tuser") - assert res == "0" - - node1.query( - "CREATE TABLE tb (`content` UInt64) ENGINE = MergeTree ORDER BY content" - ) - node1.query("INSERT INTO tb VALUES (1)") - # Has been granted and table exists - res, _ = node1.query("CHECK GRANT SELECT ON tb", user="tuser") - assert res == "1" - - node1.query("REVOKE SELECT ON tb FROM tuser") - # Has not been granted but table exists - res, _ = node1.query("CHECK GRANT SELECT ON tb", user="tuser") - assert res == "0" - - # Role - node1.query("CREATE ROLE trole") - node1.query("GRANT SELECT ON tb TO trole") - node1.query("GRANT trole TO tuser") - node1.query("SET ROLE trole", user="tuser") - res, _ = node1.query("CHECK GRANT SELECT ON tb", user="tuser") - assert res == "1" From 61f2a2d78b7597b45f29b907f44006872a1f171b Mon Sep 17 00:00:00 2001 From: unashi Date: Sun, 8 Sep 2024 17:19:36 +0800 Subject: [PATCH 10/49] [Fix] Add source env build in stateless test --- tests/queries/0_stateless/03234_check_grant.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/queries/0_stateless/03234_check_grant.sh b/tests/queries/0_stateless/03234_check_grant.sh index 922f8fa96ac..29b33e64f2e 100755 --- a/tests/queries/0_stateless/03234_check_grant.sh +++ b/tests/queries/0_stateless/03234_check_grant.sh @@ -1,5 +1,8 @@ #!/usr/bin/env bash # Tags: no-parallel +CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +# shellcheck source=../shell_config.sh +. "$CURDIR"/../shell_config.sh ${CLICKHOUSE_CLIENT} --query "DROP USER IF EXISTS user_03234; DROP TABLE IF EXISTS tb;CREATE USER user_03234; GRANT SELECT ON tb TO user_03234;" From 9e446361bd9f073506a6e271eb23bc7ddd0acf29 Mon Sep 17 00:00:00 2001 From: unashi Date: Tue, 1 Oct 2024 15:36:03 +0800 Subject: [PATCH 11/49] [Update] Remove check for existance of tables/columns, just check grant; Update the way to return result. --- .../Access/InterpreterCheckGrantQuery.cpp | 55 ++++--------------- 1 file changed, 10 insertions(+), 45 deletions(-) diff --git a/src/Interpreters/Access/InterpreterCheckGrantQuery.cpp b/src/Interpreters/Access/InterpreterCheckGrantQuery.cpp index 7a01380e343..3a94251b66f 100644 --- a/src/Interpreters/Access/InterpreterCheckGrantQuery.cpp +++ b/src/Interpreters/Access/InterpreterCheckGrantQuery.cpp @@ -16,7 +16,9 @@ #include #include #include -#include "Databases/IDatabase.h" +#include +#include +#include #include "Storages/IStorage.h" namespace DB @@ -52,54 +54,17 @@ BlockIO InterpreterCheckGrantQuery::execute() String current_database = getContext()->getCurrentDatabase(); elements_to_check_grant.replaceEmptyDatabase(current_database); query.access_rights_elements.replaceEmptyDatabase(current_database); - auto *logger = &::Poco::Logger::get("CheckGrantQuery"); - - /// Check If Table/Columns exist. - for (const auto & elem : elements_to_check_grant) - { - try - { - DatabasePtr database; - database = DatabaseCatalog::instance().getDatabase(elem.database); - if (!database->isTableExist(elem.table, getContext())) - { - /// Table not found. - return executeQuery("SELECT 0 AS CHECK_GRANT", getContext(), QueryFlags{.internal = true}).second; - } - auto table = database->getTable(elem.table, getContext()); - - auto column_name_with_sizes = table->getColumnSizes(); - for (const auto & elem_col : elem.columns) - { - bool founded = false; - for (const auto & col_in_table : column_name_with_sizes) - { - if (col_in_table.first == elem_col) - { - founded = true; - break; - } - } - if (!founded) - { - /// Column not found. - return executeQuery("SELECT 0 AS CHECK_GRANT", getContext(), QueryFlags{.internal = true}).second; - } - } - } - catch (...) - { - tryLogCurrentException(logger); - return executeQuery("SELECT 0 AS CHECK_GRANT", getContext(), QueryFlags{.internal = true}).second; - } - } bool user_is_granted = current_user_access->isGranted(elements_to_check_grant); if (!user_is_granted) { - return executeQuery("SELECT 0 AS CHECK_GRANT", getContext(), QueryFlags{.internal = true}).second; + BlockIO res; + res.pipeline = QueryPipeline(std::make_shared(Block{{ColumnUInt8::create(1, 0), std::make_shared(), "result"}})); + return res; } - - return executeQuery("SELECT 1 AS CHECK_GRANT", getContext(), QueryFlags{.internal = true}).second; + BlockIO res; + res.pipeline = QueryPipeline( + std::make_shared(Block{{ColumnUInt8::create(1, 1), std::make_shared(), "result"}})); + return res; } void registerInterpreterCheckGrantQuery(InterpreterFactory & factory) From f41c60ddd77cc3ff7160cd9db581af4020587b19 Mon Sep 17 00:00:00 2001 From: unashi Date: Tue, 1 Oct 2024 15:37:39 +0800 Subject: [PATCH 12/49] [Update] Add wildcard check for check grant --- src/Parsers/Access/ASTCheckGrantQuery.cpp | 10 ++--- src/Parsers/Access/ParserCheckGrantQuery.cpp | 42 +++++++++----------- 2 files changed, 23 insertions(+), 29 deletions(-) diff --git a/src/Parsers/Access/ASTCheckGrantQuery.cpp b/src/Parsers/Access/ASTCheckGrantQuery.cpp index 0f23fcce48a..5a549f3daec 100644 --- a/src/Parsers/Access/ASTCheckGrantQuery.cpp +++ b/src/Parsers/Access/ASTCheckGrantQuery.cpp @@ -28,12 +28,12 @@ namespace settings.ostr << (settings.hilite ? IAST::hilite_keyword : "") << "ON " << (settings.hilite ? IAST::hilite_none : ""); if (element.isGlobalWithParameter()) { - if (element.any_parameter) + if (element.anyParameter()) settings.ostr << "*"; else settings.ostr << backQuoteIfNeed(element.parameter); } - else if (element.any_database) + else if (element.anyDatabase()) { settings.ostr << "*.*"; } @@ -41,7 +41,7 @@ namespace { if (!element.database.empty()) settings.ostr << backQuoteIfNeed(element.database) << "."; - if (element.any_table) + if (element.anyDatabase()) settings.ostr << "*"; else settings.ostr << backQuoteIfNeed(element.table); @@ -56,7 +56,7 @@ namespace { const auto & element = elements[i]; auto keywords = element.access_flags.toKeywords(); - if (keywords.empty() || (!element.any_column && element.columns.empty())) + if (keywords.empty() || (!element.anyColumn() && element.columns.empty())) continue; for (const auto & keyword : keywords) @@ -65,7 +65,7 @@ namespace settings.ostr << ", "; settings.ostr << (settings.hilite ? IAST::hilite_keyword : "") << keyword << (settings.hilite ? IAST::hilite_none : ""); - if (!element.any_column) + if (!element.anyColumn()) formatColumnNames(element.columns, settings); } diff --git a/src/Parsers/Access/ParserCheckGrantQuery.cpp b/src/Parsers/Access/ParserCheckGrantQuery.cpp index c7433b7d4df..fe53377be59 100644 --- a/src/Parsers/Access/ParserCheckGrantQuery.cpp +++ b/src/Parsers/Access/ParserCheckGrantQuery.cpp @@ -109,7 +109,7 @@ namespace } - bool parseElementsWithoutOptions(IParser::Pos & pos, Expected & expected, AccessRightsElements & elements) + bool parseElements(IParser::Pos & pos, Expected & expected, AccessRightsElements & elements) { return IParserBase::wrapParseImpl(pos, [&] { @@ -122,7 +122,6 @@ namespace return false; String database_name, table_name, parameter; - bool any_database = false, any_table = false, any_parameter = false; size_t is_global_with_parameter = 0; for (const auto & elem : access_and_columns) @@ -134,40 +133,35 @@ namespace if (!ParserKeyword{Keyword::ON}.ignore(pos, expected)) return false; + bool wildcard = false; + bool default_database = false; if (is_global_with_parameter && is_global_with_parameter == access_and_columns.size()) { ASTPtr parameter_ast; - if (ParserToken{TokenType::Asterisk}.ignore(pos, expected)) + if (!ParserToken{TokenType::Asterisk}.ignore(pos, expected)) { - any_parameter = true; + if (ParserIdentifier{}.parse(pos, parameter_ast, expected)) + parameter = getIdentifierName(parameter_ast); + else + return false; } - else if (ParserIdentifier{}.parse(pos, parameter_ast, expected)) - { - any_parameter = false; - parameter = getIdentifierName(parameter_ast); - } - else - return false; - any_database = any_table = true; + if (ParserToken{TokenType::Asterisk}.ignore(pos, expected)) + wildcard = true; } - else if (!parseDatabaseAndTableNameOrAsterisks(pos, expected, database_name, any_database, table_name, any_table)) - { + else if (!parseDatabaseAndTableNameOrAsterisks(pos, expected, database_name, table_name, wildcard, default_database)) return false; - } for (auto & [access_flags, columns] : access_and_columns) { AccessRightsElement element; element.access_flags = access_flags; - element.any_column = columns.empty(); element.columns = std::move(columns); - element.any_database = any_database; element.database = database_name; - element.any_table = any_table; - element.any_parameter = any_parameter; element.table = table_name; element.parameter = parameter; + element.wildcard = wildcard; + element.default_database = default_database; res_elements.emplace_back(std::move(element)); } @@ -193,13 +187,13 @@ namespace if (!element.empty()) return false; - if (!element.any_column) + if (!element.anyColumn()) throw Exception(ErrorCodes::INVALID_GRANT, "{} cannot check grant on the column level", old_flags.toString()); - else if (!element.any_table) + else if (!element.anyTable()) throw Exception(ErrorCodes::INVALID_GRANT, "{} cannot check grant on the table level", old_flags.toString()); - else if (!element.any_database) + else if (!element.anyDatabase()) throw Exception(ErrorCodes::INVALID_GRANT, "{} cannot check grant on the database level", old_flags.toString()); - else if (!element.any_parameter) + else if (!element.anyParameter()) throw Exception(ErrorCodes::INVALID_GRANT, "{} cannot check grant on the global with parameter level", old_flags.toString()); else throw Exception(ErrorCodes::INVALID_GRANT, "{} cannot check grant", old_flags.toString()); @@ -216,7 +210,7 @@ bool ParserCheckGrantQuery::parseImpl(Pos & pos, ASTPtr & node, Expected & expec AccessRightsElements elements; - if (!parseElementsWithoutOptions(pos, expected, elements)) + if (!parseElements(pos, expected, elements)) return false; throwIfNotGrantable(elements); From de6f2b0f9188e09326422830046c15fece76d7f7 Mon Sep 17 00:00:00 2001 From: unashi Date: Tue, 1 Oct 2024 15:38:19 +0800 Subject: [PATCH 13/49] [Test] Add test case for check grant --- .../0_stateless/03234_check_grant.reference | 3 +- .../queries/0_stateless/03234_check_grant.sh | 28 +++++++++++++------ 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/tests/queries/0_stateless/03234_check_grant.reference b/tests/queries/0_stateless/03234_check_grant.reference index 5565ed6787f..f204dab8412 100644 --- a/tests/queries/0_stateless/03234_check_grant.reference +++ b/tests/queries/0_stateless/03234_check_grant.reference @@ -1,4 +1,5 @@ -0 +1 1 0 1 +1 diff --git a/tests/queries/0_stateless/03234_check_grant.sh b/tests/queries/0_stateless/03234_check_grant.sh index 29b33e64f2e..3e050d5ce6e 100755 --- a/tests/queries/0_stateless/03234_check_grant.sh +++ b/tests/queries/0_stateless/03234_check_grant.sh @@ -3,21 +3,31 @@ CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) # shellcheck source=../shell_config.sh . "$CURDIR"/../shell_config.sh +db=${CLICKHOUSE_DATABASE} -${CLICKHOUSE_CLIENT} --query "DROP USER IF EXISTS user_03234; DROP TABLE IF EXISTS tb;CREATE USER user_03234; GRANT SELECT ON tb TO user_03234;" +${CLICKHOUSE_CLIENT} --query "DROP USER IF EXISTS user_03234; DROP TABLE IF EXISTS ${db}.tb;CREATE USER user_03234; GRANT SELECT ON ${db}.tb TO user_03234;" -# Has been granted but not table not exists -${CLICKHOUSE_CURL} -sS "${CLICKHOUSE_URL}&user=user_03234" --data-binary "CHECK GRANT SELECT ON tb" +# Has been granted but not table not exists +# expected to 1 +${CLICKHOUSE_CURL} -sS "${CLICKHOUSE_URL}&user=user_03234" --data-binary "CHECK GRANT SELECT ON ${db}.tb" -${CLICKHOUSE_CLIENT} --query "CREATE TABLE tb (\`content\` UInt64) ENGINE = MergeTree ORDER BY content; INSERT INTO tb VALUES (1);" +${CLICKHOUSE_CLIENT} --query "CREATE TABLE ${db}.tb (\`content\` UInt64) ENGINE = MergeTree ORDER BY content; INSERT INTO ${db}.tb VALUES (1);" # Has been granted and table exists -${CLICKHOUSE_CURL} -sS "${CLICKHOUSE_URL}&user=user_03234" --data-binary "CHECK GRANT SELECT ON tb" +# expected to 1 +${CLICKHOUSE_CURL} -sS "${CLICKHOUSE_URL}&user=user_03234" --data-binary "CHECK GRANT SELECT ON ${db}.tb" -${CLICKHOUSE_CLIENT} --query "REVOKE SELECT ON tb FROM user_03234;" +${CLICKHOUSE_CLIENT} --query "REVOKE SELECT ON ${db}.tb FROM user_03234;" # Has not been granted but table exists -${CLICKHOUSE_CURL} -sS "${CLICKHOUSE_URL}&user=user_03234" --data-binary "CHECK GRANT SELECT ON tb" +# expected to 0 +${CLICKHOUSE_CURL} -sS "${CLICKHOUSE_URL}&user=user_03234" --data-binary "CHECK GRANT SELECT ON ${db}.tb" # Role -${CLICKHOUSE_CLIENT} --query "DROP ROLE IF EXISTS role_03234;CREATE ROLE role_03234;GRANT SELECT ON tb TO role_03234;GRANT role_03234 TO user_03234" +# expected to 1 +${CLICKHOUSE_CLIENT} --query "DROP ROLE IF EXISTS role_03234;CREATE ROLE role_03234;GRANT SELECT ON ${db}.tb TO role_03234;GRANT role_03234 TO user_03234" ${CLICKHOUSE_CURL} -sS "${CLICKHOUSE_URL}&user=user_03234" --data-binary "SET ROLE role_03234" -${CLICKHOUSE_CURL} -sS "${CLICKHOUSE_URL}&user=user_03234" --data-binary "CHECK GRANT SELECT ON tb" +${CLICKHOUSE_CURL} -sS "${CLICKHOUSE_URL}&user=user_03234" --data-binary "CHECK GRANT SELECT ON ${db}.tb" + +# wildcard +# expected to 1 +${CLICKHOUSE_CLIENT} --query "GRANT SELECT ON ${db}.tbb* TO user_03234;" +${CLICKHOUSE_CURL} -sS "${CLICKHOUSE_URL}&user=user_03234" --data-binary "CHECK GRANT SELECT ON ${db}.tbb1" From e5229770eeb581e6f159ae7a0ce30a9ff7f4cc8f Mon Sep 17 00:00:00 2001 From: unashi Date: Wed, 2 Oct 2024 01:19:33 +0800 Subject: [PATCH 14/49] [Perf] Perf the codes for check grant --- .../Access/InterpreterCheckGrantQuery.cpp | 26 ++----------------- 1 file changed, 2 insertions(+), 24 deletions(-) diff --git a/src/Interpreters/Access/InterpreterCheckGrantQuery.cpp b/src/Interpreters/Access/InterpreterCheckGrantQuery.cpp index 3a94251b66f..3410d201e17 100644 --- a/src/Interpreters/Access/InterpreterCheckGrantQuery.cpp +++ b/src/Interpreters/Access/InterpreterCheckGrantQuery.cpp @@ -24,20 +24,6 @@ namespace DB { -namespace -{ - /// Extracts access rights elements which are going to check grant - void collectAccessRightsElementsToGrantOrRevoke( - const ASTCheckGrantQuery & query, - AccessRightsElements & elements_to_check_grant) - { - elements_to_check_grant.clear(); - /// GRANT - elements_to_check_grant = query.access_rights_elements; - } -} - - BlockIO InterpreterCheckGrantQuery::execute() { auto & query = query_ptr->as(); @@ -46,24 +32,16 @@ BlockIO InterpreterCheckGrantQuery::execute() auto current_user_access = getContext()->getAccess(); /// Collect access rights elements which will be checked. - AccessRightsElements elements_to_check_grant; - collectAccessRightsElementsToGrantOrRevoke(query, elements_to_check_grant); - + AccessRightsElements & elements_to_check_grant = query.access_rights_elements; /// Replacing empty database with the default. This step must be done before replication to avoid privilege escalation. String current_database = getContext()->getCurrentDatabase(); elements_to_check_grant.replaceEmptyDatabase(current_database); query.access_rights_elements.replaceEmptyDatabase(current_database); bool user_is_granted = current_user_access->isGranted(elements_to_check_grant); - if (!user_is_granted) - { - BlockIO res; - res.pipeline = QueryPipeline(std::make_shared(Block{{ColumnUInt8::create(1, 0), std::make_shared(), "result"}})); - return res; - } BlockIO res; res.pipeline = QueryPipeline( - std::make_shared(Block{{ColumnUInt8::create(1, 1), std::make_shared(), "result"}})); + std::make_shared(Block{{ColumnUInt8::create(1, user_is_granted), std::make_shared(), "result"}})); return res; } From 1f7be09d70046628792c5a1a9abb1d6176d04e59 Mon Sep 17 00:00:00 2001 From: unashi Date: Wed, 2 Oct 2024 08:11:17 +0800 Subject: [PATCH 15/49] [Doc/Test] Update doc about check grant wildcard; Add more case in stateless-test case about check grant wildcard --- .../sql-reference/statements/check-grant.md | 21 +++++++++++-------- .../0_stateless/03234_check_grant.reference | 1 + .../queries/0_stateless/03234_check_grant.sh | 4 +++- 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/docs/en/sql-reference/statements/check-grant.md b/docs/en/sql-reference/statements/check-grant.md index a1971eb59d1..7dc78b66bf7 100644 --- a/docs/en/sql-reference/statements/check-grant.md +++ b/docs/en/sql-reference/statements/check-grant.md @@ -5,21 +5,21 @@ sidebar_label: CHECK GRANT title: "CHECK GRANT Statement" --- -The `CHECK GRANT` query is used to check whether the current user/role has been granted a specific privilege, and whether the corresponding table/column exists in the memory. +The `CHECK GRANT` query is used to check whether the current user/role has been granted a specific privilege. ## Syntax The basic syntax of the query is as follows: ```sql -CHECK GRANT privilege[(column_name [,...])] [,...] ON {db.table|db.*|*.*|table|*} +CHECK GRANT privilege[(column_name [,...])] [,...] ON {db.table[*]|db[*].*|*.*|table[*]|*} ``` - `privilege` — Type of privilege. ## Examples -If the user used to be granted the privilege, or the role (which is granted with the privilege), and the db.table(column) exists on this node, the response`check_grant` will be `1`. Otherwise, the response `check_grant` will be `0`. +If the user used to be granted the privilege, the response`check_grant` will be `1`. Otherwise, the response `check_grant` will be `0`. If `table_1.col1` exists and current user is granted by privilege `SELECT`/`SELECT(con)` or role(with privilege), the response is `1`. ```sql @@ -27,9 +27,9 @@ CHECK GRANT SELECT(col1) ON table_1; ``` ```text -┌─CHECK_GRANT─┐ -│ 1 │ -└─────────────┘ +┌─result─┐ +│ 1 │ +└────────┘ ``` If `table_2.col2` doesn't exists, or current user is not granted by privilege `SELECT`/`SELECT(con)` or role(with privilege), the response is `0`. ```sql @@ -37,7 +37,10 @@ CHECK GRANT SELECT(col2) ON table_2; ``` ```text -┌─CHECK_GRANT─┐ -│ 0 │ -└─────────────┘ +┌─result─┐ +│ 0 │ +└────────┘ ``` + +## Wildcard +Specifying privileges you can use asterisk (`*`) instead of a table or a database name. Please check [WILDCARD GRANTS](../../sql-reference/statements/grant.md#wildcard-grants) for wildcard rules. diff --git a/tests/queries/0_stateless/03234_check_grant.reference b/tests/queries/0_stateless/03234_check_grant.reference index f204dab8412..23dfd352361 100644 --- a/tests/queries/0_stateless/03234_check_grant.reference +++ b/tests/queries/0_stateless/03234_check_grant.reference @@ -3,3 +3,4 @@ 0 1 1 +1 diff --git a/tests/queries/0_stateless/03234_check_grant.sh b/tests/queries/0_stateless/03234_check_grant.sh index 3e050d5ce6e..d697cdc7c90 100755 --- a/tests/queries/0_stateless/03234_check_grant.sh +++ b/tests/queries/0_stateless/03234_check_grant.sh @@ -28,6 +28,8 @@ ${CLICKHOUSE_CURL} -sS "${CLICKHOUSE_URL}&user=user_03234" --data-binary "SET RO ${CLICKHOUSE_CURL} -sS "${CLICKHOUSE_URL}&user=user_03234" --data-binary "CHECK GRANT SELECT ON ${db}.tb" # wildcard -# expected to 1 ${CLICKHOUSE_CLIENT} --query "GRANT SELECT ON ${db}.tbb* TO user_03234;" +# expected to 1 ${CLICKHOUSE_CURL} -sS "${CLICKHOUSE_URL}&user=user_03234" --data-binary "CHECK GRANT SELECT ON ${db}.tbb1" +# expected to 1 +${CLICKHOUSE_CURL} -sS "${CLICKHOUSE_URL}&user=user_03234" --data-binary "CHECK GRANT SELECT ON ${db}.tbb2*" From 57ceeb4a63535b184512ef0bc92d3332dafe6414 Mon Sep 17 00:00:00 2001 From: Aleksei Filatov Date: Tue, 12 Nov 2024 20:38:23 +0000 Subject: [PATCH 16/49] Change acquiring zero-copy shared lock during moving part --- src/Common/FailPoint.cpp | 2 + src/Storages/MergeTree/IMergeTreeDataPart.cpp | 13 +- src/Storages/MergeTree/IMergeTreeDataPart.h | 3 + src/Storages/MergeTree/MergeTreeData.cpp | 60 +++++---- .../MergeTree/MergeTreePartsMover.cpp | 42 ++++++- src/Storages/MergeTree/MergeTreePartsMover.h | 3 + src/Storages/StorageReplicatedMergeTree.cpp | 6 + .../configs/config.d/s3.xml | 6 +- .../test_s3_zero_copy_replication/test.py | 119 +++++++++++++++++- 9 files changed, 215 insertions(+), 39 deletions(-) diff --git a/src/Common/FailPoint.cpp b/src/Common/FailPoint.cpp index 0898bdded83..49e5dee60dd 100644 --- a/src/Common/FailPoint.cpp +++ b/src/Common/FailPoint.cpp @@ -76,6 +76,8 @@ static struct InitFiu PAUSEABLE(stop_moving_part_before_swap_with_active) \ REGULAR(slowdown_index_analysis) \ REGULAR(replicated_merge_tree_all_replicas_stale) \ + REGULAR(zero_copy_lock_zk_fail_before_op) \ + REGULAR(zero_copy_lock_zk_fail_after_op) \ namespace FailPoints diff --git a/src/Storages/MergeTree/IMergeTreeDataPart.cpp b/src/Storages/MergeTree/IMergeTreeDataPart.cpp index 51c445945e6..af08e10b916 100644 --- a/src/Storages/MergeTree/IMergeTreeDataPart.cpp +++ b/src/Storages/MergeTree/IMergeTreeDataPart.cpp @@ -544,6 +544,14 @@ SerializationPtr IMergeTreeDataPart::tryGetSerialization(const String & column_n return it == serializations.end() ? nullptr : it->second; } +bool IMergeTreeDataPart::isMovingPart() const +{ + fs::path part_directory_path = getDataPartStorage().getRelativePath(); + if (part_directory_path.filename().empty()) + part_directory_path = part_directory_path.parent_path(); + return part_directory_path.parent_path().filename() == "moving"; +} + void IMergeTreeDataPart::removeIfNeeded() { assert(assertHasValidVersionMetadata()); @@ -568,10 +576,7 @@ void IMergeTreeDataPart::removeIfNeeded() throw Exception(ErrorCodes::LOGICAL_ERROR, "relative_path {} of part {} is invalid or not set", getDataPartStorage().getPartDirectory(), name); - fs::path part_directory_path = getDataPartStorage().getRelativePath(); - if (part_directory_path.filename().empty()) - part_directory_path = part_directory_path.parent_path(); - bool is_moving_part = part_directory_path.parent_path().filename() == "moving"; + bool is_moving_part = isMovingPart(); if (!startsWith(file_name, "tmp") && !endsWith(file_name, ".tmp_proj") && !is_moving_part) { LOG_ERROR( diff --git a/src/Storages/MergeTree/IMergeTreeDataPart.h b/src/Storages/MergeTree/IMergeTreeDataPart.h index 55f1265318c..e844ea3640e 100644 --- a/src/Storages/MergeTree/IMergeTreeDataPart.h +++ b/src/Storages/MergeTree/IMergeTreeDataPart.h @@ -434,6 +434,9 @@ public: bool isProjectionPart() const { return parent_part != nullptr; } + /// Check if the part is in the `/moving` directory + bool isMovingPart() const; + const IMergeTreeDataPart * getParentPart() const { return parent_part; } String getParentPartName() const { return parent_part_name; } diff --git a/src/Storages/MergeTree/MergeTreeData.cpp b/src/Storages/MergeTree/MergeTreeData.cpp index b2f35d0a309..8806c96ac9b 100644 --- a/src/Storages/MergeTree/MergeTreeData.cpp +++ b/src/Storages/MergeTree/MergeTreeData.cpp @@ -8257,33 +8257,49 @@ MovePartsOutcome MergeTreeData::moveParts(const CurrentlyMovingPartsTaggerPtr & /// replica will actually move the part from disk to some /// zero-copy storage other replicas will just fetch /// metainformation. - if (auto lock = tryCreateZeroCopyExclusiveLock(moving_part.part->name, disk); lock) - { - if (lock->isLocked()) - { - cloned_part = parts_mover.clonePart(moving_part, read_settings, write_settings); - parts_mover.swapClonedPart(cloned_part); - break; - } - if (wait_for_move_if_zero_copy) - { - LOG_DEBUG(log, "Other replica is working on move of {}, will wait until lock disappear", moving_part.part->name); - /// Wait and checks not only for timeout but also for shutdown and so on. - while (!waitZeroCopyLockToDisappear(*lock, 3000)) - { - LOG_DEBUG(log, "Waiting until some replica will move {} and zero copy lock disappear", moving_part.part->name); - } - } - else - break; - } - else + auto lock = tryCreateZeroCopyExclusiveLock(moving_part.part->name, disk); + if (!lock) { /// Move will be retried but with backoff. - LOG_DEBUG(log, "Move of part {} postponed, because zero copy mode enabled and someone other moving this part right now", moving_part.part->name); + LOG_DEBUG( + log, + "Move of part {} postponed, because zero copy mode enabled and zero-copy lock was not acquired", + moving_part.part->name); result = MovePartsOutcome::MoveWasPostponedBecauseOfZeroCopy; break; } + + if (lock->isLocked()) + { + cloned_part = parts_mover.clonePart(moving_part, read_settings, write_settings); + /// Cloning part can take a long time. + /// Recheck if the lock (and keeper session expirity) is OK + if (lock->isLocked()) + { + parts_mover.swapClonedPart(cloned_part); + break; /// Successfully moved + } + else + { + LOG_DEBUG( + log, + "Move of part {} postponed, because zero copy mode enabled and zero-copy lock was lost during cloning the part", + moving_part.part->name); + result = MovePartsOutcome::MoveWasPostponedBecauseOfZeroCopy; + break; + } + } + if (wait_for_move_if_zero_copy) + { + LOG_DEBUG(log, "Other replica is working on move of {}, will wait until lock disappear", moving_part.part->name); + /// Wait and checks not only for timeout but also for shutdown and so on. + while (!waitZeroCopyLockToDisappear(*lock, 3000)) + { + LOG_DEBUG(log, "Waiting until some replica will move {} and zero copy lock disappear", moving_part.part->name); + } + } + else + break; } } else /// Ordinary move as it should be diff --git a/src/Storages/MergeTree/MergeTreePartsMover.cpp b/src/Storages/MergeTree/MergeTreePartsMover.cpp index e9c9f2b4b06..c2a1636513e 100644 --- a/src/Storages/MergeTree/MergeTreePartsMover.cpp +++ b/src/Storages/MergeTree/MergeTreePartsMover.cpp @@ -259,6 +259,7 @@ MergeTreePartsMover::TemporaryClonedPart MergeTreePartsMover::clonePart(const Me disk->createDirectories(path_to_clone); + /// TODO: Make it possible to fetch only zero-copy part without fallback to fetching a full-copy one auto zero_copy_part = data->tryToFetchIfShared(*part, disk, fs::path(path_to_clone) / part->name); if (zero_copy_part) @@ -301,6 +302,28 @@ MergeTreePartsMover::TemporaryClonedPart MergeTreePartsMover::clonePart(const Me return cloned_part; } +void MergeTreePartsMover::renameClonedPart(IMergeTreeDataPart & part) const +try +{ + part.is_temp = false; + /// Mark it DeleteOnDestroy to ensure deleting in destructor + /// if something goes wrong before swapping + part.setState(MergeTreeDataPartState::DeleteOnDestroy); + /// Don't remove new directory but throw an error because it may contain part which is currently in use. + part.renameTo(part.name, /* remove_new_dir_if_exists */ false); +} +catch (...) +{ + /// Check if part was renamed or not + /// `renameTo()` does not provide strong exception guarantee in case of an exception + if (part.isMovingPart()) + { + /// Restore its temporary state + part.is_temp = true; + part.setState(MergeTreeDataPartState::Temporary); + } + throw; +} void MergeTreePartsMover::swapClonedPart(TemporaryClonedPart & cloned_part) const { @@ -327,12 +350,23 @@ void MergeTreePartsMover::swapClonedPart(TemporaryClonedPart & cloned_part) cons return; } - cloned_part.part->is_temp = false; + /// It is safe to acquire zero-copy lock for the temporary part here + /// because no one can fetch it until it is *swapped*. + /// + /// Set ASK_KEEPER to try to unlock it in destructor if something goes wrong before *renaming* + /// If unlocking is failed we will not get a stucked part in moving directory + /// because it will be renamed to delete_tmp_ beforehand and cleaned up later. + /// Worst outcomes: trash in object storage and/or orphaned shared zero-copy lock. It is acceptable. + /// See DataPartStorageOnDiskBase::remove(). + cloned_part.part->remove_tmp_policy = IMergeTreeDataPart::BlobsRemovalPolicyForTemporaryParts::ASK_KEEPER; + data->lockSharedData(*cloned_part.part, /* replace_existing_lock = */ true); - /// Don't remove new directory but throw an error because it may contain part which is currently in use. - cloned_part.part->renameTo(active_part->name, false); + renameClonedPart(*cloned_part.part); - /// TODO what happen if server goes down here? + /// If server goes down here we will get two copy of the part with the same name on different disks. + /// And on the next ClickHouse startup during loading parts the first copy (in the order of defining disks + /// in the storage policy) will be loaded as Active, the second one will be loaded as Outdated and removed as duplicate. + /// See MergeTreeData::loadDataParts(). data->swapActivePart(cloned_part.part, part_lock); LOG_TRACE(log, "Part {} was moved to {}", cloned_part.part->name, cloned_part.part->getDataPartStorage().getFullPath()); diff --git a/src/Storages/MergeTree/MergeTreePartsMover.h b/src/Storages/MergeTree/MergeTreePartsMover.h index 3cf270946d8..7a6583008c0 100644 --- a/src/Storages/MergeTree/MergeTreePartsMover.h +++ b/src/Storages/MergeTree/MergeTreePartsMover.h @@ -75,6 +75,9 @@ public: /// merge or mutation. void swapClonedPart(TemporaryClonedPart & cloned_part) const; + /// Rename cloned part from `moving/` directory to the actual part storage + void renameClonedPart(IMergeTreeDataPart & part) const; + /// Can stop background moves and moves from queries ActionBlocker moves_blocker; diff --git a/src/Storages/StorageReplicatedMergeTree.cpp b/src/Storages/StorageReplicatedMergeTree.cpp index 793fd02c656..c0c637b6d77 100644 --- a/src/Storages/StorageReplicatedMergeTree.cpp +++ b/src/Storages/StorageReplicatedMergeTree.cpp @@ -216,6 +216,8 @@ namespace FailPoints extern const char replicated_queue_fail_next_entry[]; extern const char replicated_queue_unfail_entries[]; extern const char finish_set_quorum_failed_parts[]; + extern const char zero_copy_lock_zk_fail_before_op[]; + extern const char zero_copy_lock_zk_fail_after_op[]; } namespace ErrorCodes @@ -10483,6 +10485,10 @@ void StorageReplicatedMergeTree::createZeroCopyLockNode( Coordination::Requests ops; Coordination::Responses responses; getZeroCopyLockNodeCreateOps(zookeeper, zookeeper_node, ops, mode, replace_existing_lock, path_to_set_hardlinked_files, hardlinked_files); + + fiu_do_on(FailPoints::zero_copy_lock_zk_fail_before_op, { zookeeper->forceFailureBeforeOperation(); }); + fiu_do_on(FailPoints::zero_copy_lock_zk_fail_after_op, { zookeeper->forceFailureAfterOperation(); }); + auto error = zookeeper->tryMulti(ops, responses); if (error == Coordination::Error::ZOK) { diff --git a/tests/integration/test_s3_zero_copy_replication/configs/config.d/s3.xml b/tests/integration/test_s3_zero_copy_replication/configs/config.d/s3.xml index 8df9e8e8c26..3a2bec7f314 100644 --- a/tests/integration/test_s3_zero_copy_replication/configs/config.d/s3.xml +++ b/tests/integration/test_s3_zero_copy_replication/configs/config.d/s3.xml @@ -7,21 +7,21 @@ http://minio1:9001/root/data/ minio minio123 - true + false s3 http://minio1:9001/root/data/ minio minio123 - true + false s3 http://minio1:9001/root/data2/ minio minio123 - true + false diff --git a/tests/integration/test_s3_zero_copy_replication/test.py b/tests/integration/test_s3_zero_copy_replication/test.py index e723a4ffc57..72593c9a2ff 100644 --- a/tests/integration/test_s3_zero_copy_replication/test.py +++ b/tests/integration/test_s3_zero_copy_replication/test.py @@ -1,10 +1,13 @@ import datetime import logging +import threading import time import pytest from helpers.cluster import ClickHouseCluster +from helpers.network import PartitionManager + logging.getLogger().setLevel(logging.INFO) logging.getLogger().addHandler(logging.StreamHandler()) @@ -77,15 +80,15 @@ def wait_for_large_objects_count(cluster, expected, size=100, timeout=30): assert get_large_objects_count(cluster, size=size) == expected -def wait_for_active_parts(node, num_expected_parts, table_name, timeout=30): +def wait_for_active_parts(node, num_expected_parts, table_name, timeout=30, disk_name = None): deadline = time.monotonic() + timeout num_parts = 0 while time.monotonic() < deadline: - num_parts_str = node.query( - "select count() from system.parts where table = '{}' and active".format( - table_name - ) - ) + query = f"select count() from system.parts where table = '{table_name}' and active" + if disk_name: + query += f" and disk_name='{disk_name}'" + + num_parts_str = node.query(query) num_parts = int(num_parts_str.strip()) if num_parts == num_expected_parts: return @@ -95,6 +98,22 @@ def wait_for_active_parts(node, num_expected_parts, table_name, timeout=30): assert num_parts == num_expected_parts +@pytest.fixture(scope="function") +def test_name(request): + return request.node.name + + +@pytest.fixture(scope="function") +def test_table(test_name): + normalized = ( + test_name.replace("[", "_") + .replace("]", "_") + .replace(" ", "_") + .replace("-", "_") + ) + return "table_" + normalized + + # Result of `get_large_objects_count` can be changed in other tests, so run this case at the beginning @pytest.mark.order(0) @pytest.mark.parametrize("policy", ["s3"]) @@ -668,3 +687,91 @@ def test_s3_zero_copy_keeps_data_after_mutation(started_cluster): time.sleep(10) check_objects_not_exisis(cluster, objectsY) + + +@pytest.mark.parametrize("failpoint", ["zero_copy_lock_zk_fail_before_op", "zero_copy_lock_zk_fail_after_op"]) +def test_move_shared_lock_fail_once(started_cluster, test_table, failpoint): + node1 = cluster.instances["node1"] + node2 = cluster.instances["node2"] + + node1.query( + f""" + CREATE TABLE {test_table} ON CLUSTER test_cluster (num UInt64, date DateTime) + ENGINE=ReplicatedMergeTree('/clickhouse/tables/{test_table}', '{{replica}}') + ORDER BY date PARTITION BY date + SETTINGS storage_policy='hybrid' + """ + ) + + date = '2024-10-23' + + node2.query(f"SYSTEM STOP FETCHES {test_table}") + node1.query(f"INSERT INTO {test_table} VALUES (1, '{date}')") + + # Try to move and get fail on acquring zero-copy shared lock + node1.query(f"SYSTEM ENABLE FAILPOINT {failpoint}") + node1.query_and_get_error(f"ALTER TABLE {test_table} MOVE PARTITION '{date}' TO VOLUME 'external'") + + # After fail the part must remain on the source disk + assert node1.query(f"SELECT disk_name FROM system.parts WHERE table='{test_table}' GROUP BY disk_name") == "default\n" + + # Try another attempt after zk connection is restored + # It should not failed due to leftovers of previous attempt (temporary cloned files) + node1.query(f"SYSTEM DISABLE FAILPOINT {failpoint}") + node1.query(f"ALTER TABLE {test_table} MOVE PARTITION '{date}' TO VOLUME 'external'") + + assert node1.query(f"SELECT disk_name FROM system.parts WHERE table='{test_table}' GROUP BY disk_name") == "s31\n" + + # Sanity check + node2.query(f"SYSTEM START FETCHES {test_table}") + wait_for_active_parts(node2, 1, test_table, disk_name='s31') + assert node2.query(f"SELECT sum(num) FROM {test_table}") == "1\n" + + node1.query(f"DROP TABLE IF EXISTS {test_table} SYNC") + node2.query(f"DROP TABLE IF EXISTS {test_table} SYNC") + + +def test_move_shared_lock_fail_keeper_unavailable(started_cluster, test_table): + node1 = cluster.instances["node1"] + node2 = cluster.instances["node2"] + + node1.query( + f""" + CREATE TABLE {test_table} ON CLUSTER test_cluster (num UInt64, date DateTime) + ENGINE=ReplicatedMergeTree('/clickhouse/tables/{test_table}', '{{replica}}') + ORDER BY date PARTITION BY date + SETTINGS storage_policy='hybrid' + """ + ) + + date = '2024-10-23' + node2.query(f"SYSTEM STOP FETCHES {test_table}") + + node1.query(f"INSERT INTO {test_table} VALUES (1, '{date}')") + # Pause moving after part cloning, but before swapping + node1.query("SYSTEM ENABLE FAILPOINT stop_moving_part_before_swap_with_active") + + def move(node): + node.query_and_get_error(f"ALTER TABLE {test_table} MOVE PARTITION '{date}' TO VOLUME 'external'") + + # Start moving + t1 = threading.Thread(target=move, args=[node1]) + t1.start() + + with PartitionManager() as pm: + pm.drop_instance_zk_connections(node1) + # Continue moving and try to swap + node1.query("SYSTEM DISABLE FAILPOINT stop_moving_part_before_swap_with_active") + t1.join() + + # Previous MOVE was failed, try another one after zk connection is restored + # It should not failed due to leftovers of previous attempt (temporary cloned files) + node1.query_with_retry(f"ALTER TABLE {test_table} MOVE PARTITION '{date}' TO VOLUME 'external'") + + # Sanity check + node2.query(f"SYSTEM START FETCHES {test_table}") + wait_for_active_parts(node2, 1, test_table, disk_name='s31') + assert node2.query(f"SELECT sum(num) FROM {test_table}") == "1\n" + + node1.query(f"DROP TABLE IF EXISTS {test_table} SYNC") + node2.query(f"DROP TABLE IF EXISTS {test_table} SYNC") From 2aa5447f090a55f3dd0b818a2adbaf874a2ba3bc Mon Sep 17 00:00:00 2001 From: Aleksei Filatov Date: Wed, 13 Nov 2024 11:02:30 +0000 Subject: [PATCH 17/49] Fix style for tests --- .../test_s3_zero_copy_replication/test.py | 52 +++++++++++++------ 1 file changed, 36 insertions(+), 16 deletions(-) diff --git a/tests/integration/test_s3_zero_copy_replication/test.py b/tests/integration/test_s3_zero_copy_replication/test.py index 72593c9a2ff..4996bf8d79d 100644 --- a/tests/integration/test_s3_zero_copy_replication/test.py +++ b/tests/integration/test_s3_zero_copy_replication/test.py @@ -689,10 +689,12 @@ def test_s3_zero_copy_keeps_data_after_mutation(started_cluster): check_objects_not_exisis(cluster, objectsY) -@pytest.mark.parametrize("failpoint", ["zero_copy_lock_zk_fail_before_op", "zero_copy_lock_zk_fail_after_op"]) +@pytest.mark.parametrize( + "failpoint", ["zero_copy_lock_zk_fail_before_op", "zero_copy_lock_zk_fail_after_op"] +) def test_move_shared_lock_fail_once(started_cluster, test_table, failpoint): node1 = cluster.instances["node1"] - node2 = cluster.instances["node2"] + node2 = cluster.instances["node2"] node1.query( f""" @@ -703,28 +705,42 @@ def test_move_shared_lock_fail_once(started_cluster, test_table, failpoint): """ ) - date = '2024-10-23' + date = "2024-10-23" - node2.query(f"SYSTEM STOP FETCHES {test_table}") + node2.query(f"SYSTEM STOP FETCHES {test_table}") node1.query(f"INSERT INTO {test_table} VALUES (1, '{date}')") # Try to move and get fail on acquring zero-copy shared lock node1.query(f"SYSTEM ENABLE FAILPOINT {failpoint}") - node1.query_and_get_error(f"ALTER TABLE {test_table} MOVE PARTITION '{date}' TO VOLUME 'external'") + node1.query_and_get_error( + f"ALTER TABLE {test_table} MOVE PARTITION '{date}' TO VOLUME 'external'" + ) # After fail the part must remain on the source disk - assert node1.query(f"SELECT disk_name FROM system.parts WHERE table='{test_table}' GROUP BY disk_name") == "default\n" + assert ( + node1.query( + f"SELECT disk_name FROM system.parts WHERE table='{test_table}' GROUP BY disk_name" + ) + == "default\n" + ) # Try another attempt after zk connection is restored # It should not failed due to leftovers of previous attempt (temporary cloned files) node1.query(f"SYSTEM DISABLE FAILPOINT {failpoint}") - node1.query(f"ALTER TABLE {test_table} MOVE PARTITION '{date}' TO VOLUME 'external'") + node1.query( + f"ALTER TABLE {test_table} MOVE PARTITION '{date}' TO VOLUME 'external'" + ) - assert node1.query(f"SELECT disk_name FROM system.parts WHERE table='{test_table}' GROUP BY disk_name") == "s31\n" + assert ( + node1.query( + f"SELECT disk_name FROM system.parts WHERE table='{test_table}' GROUP BY disk_name" + ) + == "s31\n" + ) # Sanity check node2.query(f"SYSTEM START FETCHES {test_table}") - wait_for_active_parts(node2, 1, test_table, disk_name='s31') + wait_for_active_parts(node2, 1, test_table, disk_name="s31") assert node2.query(f"SELECT sum(num) FROM {test_table}") == "1\n" node1.query(f"DROP TABLE IF EXISTS {test_table} SYNC") @@ -733,7 +749,7 @@ def test_move_shared_lock_fail_once(started_cluster, test_table, failpoint): def test_move_shared_lock_fail_keeper_unavailable(started_cluster, test_table): node1 = cluster.instances["node1"] - node2 = cluster.instances["node2"] + node2 = cluster.instances["node2"] node1.query( f""" @@ -744,15 +760,17 @@ def test_move_shared_lock_fail_keeper_unavailable(started_cluster, test_table): """ ) - date = '2024-10-23' + date = "2024-10-23" node2.query(f"SYSTEM STOP FETCHES {test_table}") node1.query(f"INSERT INTO {test_table} VALUES (1, '{date}')") # Pause moving after part cloning, but before swapping - node1.query("SYSTEM ENABLE FAILPOINT stop_moving_part_before_swap_with_active") + node1.query("SYSTEM ENABLE FAILPOINT stop_moving_part_before_swap_with_active") def move(node): - node.query_and_get_error(f"ALTER TABLE {test_table} MOVE PARTITION '{date}' TO VOLUME 'external'") + node.query_and_get_error( + f"ALTER TABLE {test_table} MOVE PARTITION '{date}' TO VOLUME 'external'" + ) # Start moving t1 = threading.Thread(target=move, args=[node1]) @@ -763,14 +781,16 @@ def test_move_shared_lock_fail_keeper_unavailable(started_cluster, test_table): # Continue moving and try to swap node1.query("SYSTEM DISABLE FAILPOINT stop_moving_part_before_swap_with_active") t1.join() - + # Previous MOVE was failed, try another one after zk connection is restored # It should not failed due to leftovers of previous attempt (temporary cloned files) - node1.query_with_retry(f"ALTER TABLE {test_table} MOVE PARTITION '{date}' TO VOLUME 'external'") + node1.query_with_retry( + f"ALTER TABLE {test_table} MOVE PARTITION '{date}' TO VOLUME 'external'" + ) # Sanity check node2.query(f"SYSTEM START FETCHES {test_table}") - wait_for_active_parts(node2, 1, test_table, disk_name='s31') + wait_for_active_parts(node2, 1, test_table, disk_name="s31") assert node2.query(f"SELECT sum(num) FROM {test_table}") == "1\n" node1.query(f"DROP TABLE IF EXISTS {test_table} SYNC") From 41415197ab88d72470571701a591b8499aaa68a1 Mon Sep 17 00:00:00 2001 From: Vladimir Cherkasov Date: Wed, 13 Nov 2024 12:28:46 +0100 Subject: [PATCH 18/49] stylecheck --- src/Storages/MergeTree/MergeTreePartsMover.cpp | 2 +- tests/integration/test_s3_zero_copy_replication/test.py | 9 ++++++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/Storages/MergeTree/MergeTreePartsMover.cpp b/src/Storages/MergeTree/MergeTreePartsMover.cpp index c2a1636513e..82eafd47cb8 100644 --- a/src/Storages/MergeTree/MergeTreePartsMover.cpp +++ b/src/Storages/MergeTree/MergeTreePartsMover.cpp @@ -354,7 +354,7 @@ void MergeTreePartsMover::swapClonedPart(TemporaryClonedPart & cloned_part) cons /// because no one can fetch it until it is *swapped*. /// /// Set ASK_KEEPER to try to unlock it in destructor if something goes wrong before *renaming* - /// If unlocking is failed we will not get a stucked part in moving directory + /// If unlocking is failed we will not get a stuck part in moving directory /// because it will be renamed to delete_tmp_ beforehand and cleaned up later. /// Worst outcomes: trash in object storage and/or orphaned shared zero-copy lock. It is acceptable. /// See DataPartStorageOnDiskBase::remove(). diff --git a/tests/integration/test_s3_zero_copy_replication/test.py b/tests/integration/test_s3_zero_copy_replication/test.py index 4996bf8d79d..bb0c93d8410 100644 --- a/tests/integration/test_s3_zero_copy_replication/test.py +++ b/tests/integration/test_s3_zero_copy_replication/test.py @@ -8,7 +8,6 @@ import pytest from helpers.cluster import ClickHouseCluster from helpers.network import PartitionManager - logging.getLogger().setLevel(logging.INFO) logging.getLogger().addHandler(logging.StreamHandler()) @@ -80,11 +79,15 @@ def wait_for_large_objects_count(cluster, expected, size=100, timeout=30): assert get_large_objects_count(cluster, size=size) == expected -def wait_for_active_parts(node, num_expected_parts, table_name, timeout=30, disk_name = None): +def wait_for_active_parts( + node, num_expected_parts, table_name, timeout=30, disk_name = None +): deadline = time.monotonic() + timeout num_parts = 0 while time.monotonic() < deadline: - query = f"select count() from system.parts where table = '{table_name}' and active" + query = ( + f"select count() from system.parts where table = '{table_name}' and active" + ) if disk_name: query += f" and disk_name='{disk_name}'" From eb384d489c7e2b1845b182d5871fba6b0b311381 Mon Sep 17 00:00:00 2001 From: Aleksei Filatov Date: Wed, 13 Nov 2024 11:51:42 +0000 Subject: [PATCH 19/49] Fix style for tests 2 --- tests/integration/test_s3_zero_copy_replication/test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/test_s3_zero_copy_replication/test.py b/tests/integration/test_s3_zero_copy_replication/test.py index bb0c93d8410..c7d03d4301d 100644 --- a/tests/integration/test_s3_zero_copy_replication/test.py +++ b/tests/integration/test_s3_zero_copy_replication/test.py @@ -80,7 +80,7 @@ def wait_for_large_objects_count(cluster, expected, size=100, timeout=30): def wait_for_active_parts( - node, num_expected_parts, table_name, timeout=30, disk_name = None + node, num_expected_parts, table_name, timeout=30, disk_name=None ): deadline = time.monotonic() + timeout num_parts = 0 From a41cadb15051abe4a28494b2f58fafd556026d04 Mon Sep 17 00:00:00 2001 From: Yarik Briukhovetskyi <114298166+yariks5s@users.noreply.github.com> Date: Wed, 13 Nov 2024 17:37:38 +0100 Subject: [PATCH 20/49] init --- src/Storages/ObjectStorage/StorageObjectStorage.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/Storages/ObjectStorage/StorageObjectStorage.cpp b/src/Storages/ObjectStorage/StorageObjectStorage.cpp index fd2fe0400bb..a246e714db2 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorage.cpp +++ b/src/Storages/ObjectStorage/StorageObjectStorage.cpp @@ -47,6 +47,7 @@ String StorageObjectStorage::getPathSample(StorageInMemoryMetadata metadata, Con auto query_settings = configuration->getQuerySettings(context); /// We don't want to throw an exception if there are no files with specified path. query_settings.throw_on_zero_files_match = false; + query_settings.ignore_non_existent_file = true; bool local_distributed_processing = distributed_processing; if (context->getSettingsRef()[Setting::use_hive_partitioning]) @@ -64,6 +65,9 @@ String StorageObjectStorage::getPathSample(StorageInMemoryMetadata metadata, Con {} // file_progress_callback ); + if (dynamic_cast(file_iterator.get())) + return ""; + if (auto file = file_iterator->next(0)) return file->getPath(); return ""; From 9c22f3d89fb7d72de8e5545d716b6a6852ae53ea Mon Sep 17 00:00:00 2001 From: Yarik Briukhovetskyi <114298166+yariks5s@users.noreply.github.com> Date: Wed, 13 Nov 2024 17:54:59 +0100 Subject: [PATCH 21/49] Review update. --- src/Storages/ObjectStorage/StorageObjectStorage.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Storages/ObjectStorage/StorageObjectStorage.cpp b/src/Storages/ObjectStorage/StorageObjectStorage.cpp index a246e714db2..5e9ce8dce28 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorage.cpp +++ b/src/Storages/ObjectStorage/StorageObjectStorage.cpp @@ -65,8 +65,8 @@ String StorageObjectStorage::getPathSample(StorageInMemoryMetadata metadata, Con {} // file_progress_callback ); - if (dynamic_cast(file_iterator.get())) - return ""; + if (!configuration->isArchive() && !configuration->isPathWithGlobs() && !local_distributed_processing) + return configuration->getPath(); if (auto file = file_iterator->next(0)) return file->getPath(); From 3145aeda846e5febcba501cec83d038fd081e283 Mon Sep 17 00:00:00 2001 From: Anton Ivashkin Date: Wed, 13 Nov 2024 22:24:37 +0100 Subject: [PATCH 22/49] Cluster autodiscovery with auxiliary keeper test --- .../test_cluster_discovery/config/config.xml | 5 - .../config/config_discovery_path.xml | 9 ++ ...config_discovery_path_auxiliary_keeper.xml | 9 ++ .../config/config_keepers.xml | 24 ++++ .../config/config_observer.xml | 1 - .../config/config_shard1.xml | 1 - .../config/config_shard3.xml | 1 - .../test_cluster_discovery/config/macros0.xml | 6 + .../test_cluster_discovery/config/macros1.xml | 6 + .../test_cluster_discovery/config/macros2.xml | 6 + .../test_cluster_discovery/config/macros3.xml | 6 + .../test_cluster_discovery/config/macros4.xml | 6 + .../config/macros_o.xml | 6 + .../test_cluster_discovery/test.py | 2 +- .../test_auxiliary_keeper.py | 121 ++++++++++++++++++ 15 files changed, 200 insertions(+), 9 deletions(-) create mode 100644 tests/integration/test_cluster_discovery/config/config_discovery_path.xml create mode 100644 tests/integration/test_cluster_discovery/config/config_discovery_path_auxiliary_keeper.xml create mode 100644 tests/integration/test_cluster_discovery/config/config_keepers.xml create mode 100644 tests/integration/test_cluster_discovery/config/macros0.xml create mode 100644 tests/integration/test_cluster_discovery/config/macros1.xml create mode 100644 tests/integration/test_cluster_discovery/config/macros2.xml create mode 100644 tests/integration/test_cluster_discovery/config/macros3.xml create mode 100644 tests/integration/test_cluster_discovery/config/macros4.xml create mode 100644 tests/integration/test_cluster_discovery/config/macros_o.xml create mode 100644 tests/integration/test_cluster_discovery/test_auxiliary_keeper.py diff --git a/tests/integration/test_cluster_discovery/config/config.xml b/tests/integration/test_cluster_discovery/config/config.xml index a63ca3e5438..ed2d3b27259 100644 --- a/tests/integration/test_cluster_discovery/config/config.xml +++ b/tests/integration/test_cluster_discovery/config/config.xml @@ -1,11 +1,6 @@ 1 - - - /clickhouse/discovery/test_auto_cluster - - diff --git a/tests/integration/test_cluster_discovery/config/config_discovery_path.xml b/tests/integration/test_cluster_discovery/config/config_discovery_path.xml new file mode 100644 index 00000000000..8eaecb813ab --- /dev/null +++ b/tests/integration/test_cluster_discovery/config/config_discovery_path.xml @@ -0,0 +1,9 @@ + + + + + /clickhouse/discovery/test_auto_cluster + + + + diff --git a/tests/integration/test_cluster_discovery/config/config_discovery_path_auxiliary_keeper.xml b/tests/integration/test_cluster_discovery/config/config_discovery_path_auxiliary_keeper.xml new file mode 100644 index 00000000000..3e3835dfcab --- /dev/null +++ b/tests/integration/test_cluster_discovery/config/config_discovery_path_auxiliary_keeper.xml @@ -0,0 +1,9 @@ + + + + + zookeeper2:/clickhouse/discovery/test_auto_cluster + + + + diff --git a/tests/integration/test_cluster_discovery/config/config_keepers.xml b/tests/integration/test_cluster_discovery/config/config_keepers.xml new file mode 100644 index 00000000000..4e976578223 --- /dev/null +++ b/tests/integration/test_cluster_discovery/config/config_keepers.xml @@ -0,0 +1,24 @@ + + + + zoo1 + 2181 + + + zoo2 + 2181 + + + zoo3 + 2181 + + + + + + zoo1 + 2181 + + + + diff --git a/tests/integration/test_cluster_discovery/config/config_observer.xml b/tests/integration/test_cluster_discovery/config/config_observer.xml index 84d9539169e..c100c9f5348 100644 --- a/tests/integration/test_cluster_discovery/config/config_observer.xml +++ b/tests/integration/test_cluster_discovery/config/config_observer.xml @@ -3,7 +3,6 @@ - /clickhouse/discovery/test_auto_cluster diff --git a/tests/integration/test_cluster_discovery/config/config_shard1.xml b/tests/integration/test_cluster_discovery/config/config_shard1.xml index 06a77a37263..89590bed607 100644 --- a/tests/integration/test_cluster_discovery/config/config_shard1.xml +++ b/tests/integration/test_cluster_discovery/config/config_shard1.xml @@ -3,7 +3,6 @@ - /clickhouse/discovery/test_auto_cluster 1 diff --git a/tests/integration/test_cluster_discovery/config/config_shard3.xml b/tests/integration/test_cluster_discovery/config/config_shard3.xml index 2c706f7c268..9badd0a6132 100644 --- a/tests/integration/test_cluster_discovery/config/config_shard3.xml +++ b/tests/integration/test_cluster_discovery/config/config_shard3.xml @@ -3,7 +3,6 @@ - /clickhouse/discovery/test_auto_cluster 3 diff --git a/tests/integration/test_cluster_discovery/config/macros0.xml b/tests/integration/test_cluster_discovery/config/macros0.xml new file mode 100644 index 00000000000..a4cabff3005 --- /dev/null +++ b/tests/integration/test_cluster_discovery/config/macros0.xml @@ -0,0 +1,6 @@ + + + shard0 + replica0 + + diff --git a/tests/integration/test_cluster_discovery/config/macros1.xml b/tests/integration/test_cluster_discovery/config/macros1.xml new file mode 100644 index 00000000000..bae1ce11925 --- /dev/null +++ b/tests/integration/test_cluster_discovery/config/macros1.xml @@ -0,0 +1,6 @@ + + + shard1 + replica1 + + diff --git a/tests/integration/test_cluster_discovery/config/macros2.xml b/tests/integration/test_cluster_discovery/config/macros2.xml new file mode 100644 index 00000000000..989555d8225 --- /dev/null +++ b/tests/integration/test_cluster_discovery/config/macros2.xml @@ -0,0 +1,6 @@ + + + shard2 + replica2 + + diff --git a/tests/integration/test_cluster_discovery/config/macros3.xml b/tests/integration/test_cluster_discovery/config/macros3.xml new file mode 100644 index 00000000000..e0d5ea23696 --- /dev/null +++ b/tests/integration/test_cluster_discovery/config/macros3.xml @@ -0,0 +1,6 @@ + + + shard3 + replica3 + + diff --git a/tests/integration/test_cluster_discovery/config/macros4.xml b/tests/integration/test_cluster_discovery/config/macros4.xml new file mode 100644 index 00000000000..38b4c43d1b9 --- /dev/null +++ b/tests/integration/test_cluster_discovery/config/macros4.xml @@ -0,0 +1,6 @@ + + + shard4 + replica4 + + diff --git a/tests/integration/test_cluster_discovery/config/macros_o.xml b/tests/integration/test_cluster_discovery/config/macros_o.xml new file mode 100644 index 00000000000..4ac6241b0a6 --- /dev/null +++ b/tests/integration/test_cluster_discovery/config/macros_o.xml @@ -0,0 +1,6 @@ + + + shard_o + replica_o + + diff --git a/tests/integration/test_cluster_discovery/test.py b/tests/integration/test_cluster_discovery/test.py index b47c8d06269..59317d2be48 100644 --- a/tests/integration/test_cluster_discovery/test.py +++ b/tests/integration/test_cluster_discovery/test.py @@ -20,7 +20,7 @@ shard_configs = { nodes = { node_name: cluster.add_instance( node_name, - main_configs=[shard_config], + main_configs=[shard_config, "config/config_discovery_path.xml"], stay_alive=True, with_zookeeper=True, ) diff --git a/tests/integration/test_cluster_discovery/test_auxiliary_keeper.py b/tests/integration/test_cluster_discovery/test_auxiliary_keeper.py new file mode 100644 index 00000000000..cfc94a05b58 --- /dev/null +++ b/tests/integration/test_cluster_discovery/test_auxiliary_keeper.py @@ -0,0 +1,121 @@ +import functools + +import pytest + +from helpers.cluster import ClickHouseCluster + +from .common import check_on_cluster + +cluster = ClickHouseCluster(__file__) + +shard_configs = { + "node0": ["config/config.xml", "config/macros0.xml"], + "node1": ["config/config_shard1.xml", "config/macros1.xml"], + "node2": ["config/config.xml", "config/macros2.xml"], + "node3": ["config/config_shard3.xml", "config/macros3.xml"], + "node4": ["config/config.xml", "config/macros4.xml"], + "node_observer": ["config/config_observer.xml", "config/macros_o.xml"], +} + +nodes = { + node_name: cluster.add_instance( + node_name, + main_configs=shard_config + ["config/config_discovery_path_auxiliary_keeper.xml", "config/config_keepers.xml"], + stay_alive=True, + with_zookeeper=True, + #use_keeper=False, + ) + for node_name, shard_config in shard_configs.items() +} + + +@pytest.fixture(scope="module") +def start_cluster(): + try: + cluster.start() + yield cluster + finally: + cluster.shutdown() + + +def test_cluster_discovery_with_auxiliary_keeper_startup_and_stop(start_cluster): + """ + Start cluster, check nodes count in system.clusters, + then stop/start some nodes and check that it (dis)appeared in cluster. + """ + + check_nodes_count = functools.partial( + check_on_cluster, what="count()", msg="Wrong nodes count in cluster" + ) + check_shard_num = functools.partial( + check_on_cluster, + what="count(DISTINCT shard_num)", + msg="Wrong shard_num count in cluster", + ) + + total_shards = 3 + total_nodes = 5 + + check_nodes_count( + [nodes["node0"], nodes["node2"], nodes["node_observer"]], total_nodes + ) + check_shard_num( + [nodes["node0"], nodes["node2"], nodes["node_observer"]], total_shards + ) + + # test ON CLUSTER query + nodes["node0"].query( + "CREATE TABLE tbl ON CLUSTER 'test_auto_cluster' (x UInt64) ENGINE = ReplicatedMergeTree('zookeeper2:/clickhouse/{shard}/tbl', '{replica}') ORDER BY x" + ) + nodes["node0"].query("INSERT INTO tbl VALUES (1)") + nodes["node1"].query("INSERT INTO tbl VALUES (2)") + + assert ( + int( + nodes["node_observer"] + .query( + "SELECT sum(x) FROM clusterAllReplicas(test_auto_cluster, default.tbl)" + ) + .strip() + ) + == 3 + ) + + # Query SYSTEM DROP DNS CACHE may reload cluster configuration + # check that it does not affect cluster discovery + nodes["node1"].query("SYSTEM DROP DNS CACHE") + nodes["node0"].query("SYSTEM DROP DNS CACHE") + + check_shard_num( + [nodes["node0"], nodes["node2"], nodes["node_observer"]], total_shards + ) + + nodes["node1"].stop_clickhouse(kill=True) + check_nodes_count( + [nodes["node0"], nodes["node2"], nodes["node_observer"]], total_nodes - 1 + ) + + # node1 was the only node in shard '1' + check_shard_num( + [nodes["node0"], nodes["node2"], nodes["node_observer"]], total_shards - 1 + ) + + nodes["node3"].stop_clickhouse() + check_nodes_count( + [nodes["node0"], nodes["node2"], nodes["node_observer"]], total_nodes - 2 + ) + + nodes["node1"].start_clickhouse() + check_nodes_count( + [nodes["node0"], nodes["node2"], nodes["node_observer"]], total_nodes - 1 + ) + + nodes["node3"].start_clickhouse() + check_nodes_count( + [nodes["node0"], nodes["node2"], nodes["node_observer"]], total_nodes + ) + + # regular cluster is not affected + check_nodes_count( + [nodes["node1"], nodes["node2"]], 2, cluster_name="two_shards", retries=1 + ) From 57ddde47ea9185e47da3893e22ea8438d1469d1e Mon Sep 17 00:00:00 2001 From: Anton Ivashkin Date: Thu, 14 Nov 2024 11:16:48 +0100 Subject: [PATCH 23/49] Use auxiliary keeper for cluster discovery --- src/Interpreters/ClusterDiscovery.cpp | 11 +++++++---- src/Interpreters/ClusterDiscovery.h | 3 +++ 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/Interpreters/ClusterDiscovery.cpp b/src/Interpreters/ClusterDiscovery.cpp index 6f9c375c2f5..ab2ec886e76 100644 --- a/src/Interpreters/ClusterDiscovery.cpp +++ b/src/Interpreters/ClusterDiscovery.cpp @@ -129,9 +129,11 @@ ClusterDiscovery::ClusterDiscovery( if (!config.has(cluster_config_prefix)) continue; - String zk_root = config.getString(cluster_config_prefix + ".path"); - if (zk_root.empty()) + String zk_name_and_root = config.getString(cluster_config_prefix + ".path"); + if (zk_name_and_root.empty()) throw Exception(ErrorCodes::NO_ELEMENTS_IN_CONFIG, "ZooKeeper path for cluster '{}' is empty", key); + String zk_root = zkutil::extractZooKeeperPath(zk_name_and_root, true); + String zk_name = zkutil::extractZooKeeperName(zk_name_and_root); const auto & password = config.getString(cluster_config_prefix + ".password", ""); const auto & cluster_secret = config.getString(cluster_config_prefix + ".secret", ""); @@ -142,6 +144,7 @@ ClusterDiscovery::ClusterDiscovery( key, ClusterInfo( /* name_= */ key, + /* zk_name_= */ zk_name, /* zk_root_= */ zk_root, /* host_name= */ config.getString(cluster_config_prefix + ".my_hostname", getFQDNOrHostName()), /* username= */ config.getString(cluster_config_prefix + ".user", context->getUserName()), @@ -288,7 +291,7 @@ bool ClusterDiscovery::updateCluster(ClusterInfo & cluster_info) { LOG_DEBUG(log, "Updating cluster '{}'", cluster_info.name); - auto zk = context->getZooKeeper(); + auto zk = context->getDefaultOrAuxiliaryZooKeeper(cluster_info.zk_name); int start_version; Strings node_uuids = getNodeNames(zk, cluster_info.zk_root, cluster_info.name, &start_version, false); @@ -381,9 +384,9 @@ void ClusterDiscovery::initialUpdate() throw Exception(ErrorCodes::KEEPER_EXCEPTION, "Failpoint cluster_discovery_faults is triggered"); }); - auto zk = context->getZooKeeper(); for (auto & [_, info] : clusters_info) { + auto zk = context->getDefaultOrAuxiliaryZooKeeper(info.zk_name); registerInZk(zk, info); if (!updateCluster(info)) { diff --git a/src/Interpreters/ClusterDiscovery.h b/src/Interpreters/ClusterDiscovery.h index 756ed3d8d9b..b253473ce3e 100644 --- a/src/Interpreters/ClusterDiscovery.h +++ b/src/Interpreters/ClusterDiscovery.h @@ -67,6 +67,7 @@ private: struct ClusterInfo { const String name; + const String zk_name; const String zk_root; NodesInfo nodes_info; @@ -88,6 +89,7 @@ private: String cluster_secret; ClusterInfo(const String & name_, + const String & zk_name_, const String & zk_root_, const String & host_name, const String & username_, @@ -99,6 +101,7 @@ private: bool observer_mode, bool invisible) : name(name_) + , zk_name(zk_name_) , zk_root(zk_root_) , current_node(host_name + ":" + toString(port), secure, shard_id) , current_node_is_observer(observer_mode) From c2543d03166a85a4b6542b578007f69bc77b34c5 Mon Sep 17 00:00:00 2001 From: avogar Date: Fri, 15 Nov 2024 14:20:05 +0000 Subject: [PATCH 24/49] Allow only SELECT queries in EXPLAIN AST used inside subquery --- src/Parsers/ExpressionElementParsers.cpp | 3 +++ .../03273_select_from_explain_ast_non_select.reference | 0 .../0_stateless/03273_select_from_explain_ast_non_select.sql | 4 ++++ 3 files changed, 7 insertions(+) create mode 100644 tests/queries/0_stateless/03273_select_from_explain_ast_non_select.reference create mode 100644 tests/queries/0_stateless/03273_select_from_explain_ast_non_select.sql diff --git a/src/Parsers/ExpressionElementParsers.cpp b/src/Parsers/ExpressionElementParsers.cpp index ad062d27a37..ceb5789260f 100644 --- a/src/Parsers/ExpressionElementParsers.cpp +++ b/src/Parsers/ExpressionElementParsers.cpp @@ -140,6 +140,9 @@ bool ParserSubquery::parseImpl(Pos & pos, ASTPtr & node, Expected & expected) const ASTPtr & explained_ast = explain_query.getExplainedQuery(); if (explained_ast) { + if (!explained_ast->as()) + throw Exception(ErrorCodes::BAD_ARGUMENTS, "EXPLAIN inside subquery supports only SELECT queries"); + auto view_explain = makeASTFunction("viewExplain", std::make_shared(kind_str), std::make_shared(settings_str), diff --git a/tests/queries/0_stateless/03273_select_from_explain_ast_non_select.reference b/tests/queries/0_stateless/03273_select_from_explain_ast_non_select.reference new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/queries/0_stateless/03273_select_from_explain_ast_non_select.sql b/tests/queries/0_stateless/03273_select_from_explain_ast_non_select.sql new file mode 100644 index 00000000000..8a624142923 --- /dev/null +++ b/tests/queries/0_stateless/03273_select_from_explain_ast_non_select.sql @@ -0,0 +1,4 @@ +SELECT * FROM ( EXPLAIN AST CREATE TABLE test ENGINE=Memory ); -- {clientError BAD_ARGUMENTS} +SELECT * FROM ( EXPLAIN AST CREATE MATERIALIZED VIEW mv (data String) AS SELECT data FROM table ); -- {clientError BAD_ARGUMENTS} +SELECT * FROM ( EXPLAIN AST INSERT INTO TABLE test VALUES); -- {clientError BAD_ARGUMENTS} +SELECT * FROM ( EXPLAIN AST ALTER TABLE test MODIFY COLUMN x UInt32 ); -- {clientError BAD_ARGUMENTS} From 2d48406a82a37fd7e076e6a6728a54e5fb60ef64 Mon Sep 17 00:00:00 2001 From: Anton Ivashkin Date: Mon, 18 Nov 2024 11:21:32 +0100 Subject: [PATCH 25/49] Fix test style --- .../test_cluster_discovery/test_auxiliary_keeper.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/integration/test_cluster_discovery/test_auxiliary_keeper.py b/tests/integration/test_cluster_discovery/test_auxiliary_keeper.py index cfc94a05b58..31f32de8b4f 100644 --- a/tests/integration/test_cluster_discovery/test_auxiliary_keeper.py +++ b/tests/integration/test_cluster_discovery/test_auxiliary_keeper.py @@ -20,10 +20,13 @@ shard_configs = { nodes = { node_name: cluster.add_instance( node_name, - main_configs=shard_config + ["config/config_discovery_path_auxiliary_keeper.xml", "config/config_keepers.xml"], + main_configs=shard_config + + [ + "config/config_discovery_path_auxiliary_keeper.xml", + "config/config_keepers.xml", + ], stay_alive=True, with_zookeeper=True, - #use_keeper=False, ) for node_name, shard_config in shard_configs.items() } From 05f638ea5228be0a4f332783916116eda4094124 Mon Sep 17 00:00:00 2001 From: avogar Date: Mon, 18 Nov 2024 11:44:54 +0000 Subject: [PATCH 26/49] Update test --- ...03273_select_from_explain_ast_non_select.reference | 11 +++++++++++ .../03273_select_from_explain_ast_non_select.sql | 1 + 2 files changed, 12 insertions(+) diff --git a/tests/queries/0_stateless/03273_select_from_explain_ast_non_select.reference b/tests/queries/0_stateless/03273_select_from_explain_ast_non_select.reference index e69de29bb2d..83830f7b448 100644 --- a/tests/queries/0_stateless/03273_select_from_explain_ast_non_select.reference +++ b/tests/queries/0_stateless/03273_select_from_explain_ast_non_select.reference @@ -0,0 +1,11 @@ +SelectWithUnionQuery (children 1) + ExpressionList (children 1) + SelectQuery (children 2) + ExpressionList (children 1) + Asterisk + TablesInSelectQuery (children 1) + TablesInSelectQueryElement (children 1) + TableExpression (children 1) + Function numbers (children 1) + ExpressionList (children 1) + Literal UInt64_10 diff --git a/tests/queries/0_stateless/03273_select_from_explain_ast_non_select.sql b/tests/queries/0_stateless/03273_select_from_explain_ast_non_select.sql index 8a624142923..7298186414b 100644 --- a/tests/queries/0_stateless/03273_select_from_explain_ast_non_select.sql +++ b/tests/queries/0_stateless/03273_select_from_explain_ast_non_select.sql @@ -1,3 +1,4 @@ +SELECT * FROM ( EXPLAIN AST SELECT * FROM numbers(10) ); SELECT * FROM ( EXPLAIN AST CREATE TABLE test ENGINE=Memory ); -- {clientError BAD_ARGUMENTS} SELECT * FROM ( EXPLAIN AST CREATE MATERIALIZED VIEW mv (data String) AS SELECT data FROM table ); -- {clientError BAD_ARGUMENTS} SELECT * FROM ( EXPLAIN AST INSERT INTO TABLE test VALUES); -- {clientError BAD_ARGUMENTS} From fb4e1feb16c4de9082c2f6eb066a79477bdbdef5 Mon Sep 17 00:00:00 2001 From: Anton Ivashkin Date: Mon, 18 Nov 2024 14:38:46 +0100 Subject: [PATCH 27/49] Add cleanup after test --- tests/integration/test_cluster_discovery/test.py | 5 +++++ .../test_cluster_discovery/test_auxiliary_keeper.py | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/tests/integration/test_cluster_discovery/test.py b/tests/integration/test_cluster_discovery/test.py index 59317d2be48..4c447ac06a2 100644 --- a/tests/integration/test_cluster_discovery/test.py +++ b/tests/integration/test_cluster_discovery/test.py @@ -119,3 +119,8 @@ def test_cluster_discovery_startup_and_stop(start_cluster): check_nodes_count( [nodes["node1"], nodes["node2"]], 2, cluster_name="two_shards", retries=1 ) + + # cleanup + nodes["node0"].query( + "DROP TABLE tbl ON CLUSTER 'test_auto_cluster' SYNC" + ) diff --git a/tests/integration/test_cluster_discovery/test_auxiliary_keeper.py b/tests/integration/test_cluster_discovery/test_auxiliary_keeper.py index 31f32de8b4f..fe2e885b3c9 100644 --- a/tests/integration/test_cluster_discovery/test_auxiliary_keeper.py +++ b/tests/integration/test_cluster_discovery/test_auxiliary_keeper.py @@ -122,3 +122,8 @@ def test_cluster_discovery_with_auxiliary_keeper_startup_and_stop(start_cluster) check_nodes_count( [nodes["node1"], nodes["node2"]], 2, cluster_name="two_shards", retries=1 ) + + # cleanup + nodes["node0"].query( + "DROP TABLE tbl ON CLUSTER 'test_auto_cluster' SYNC" + ) From 9917dc66d4d83e7c7a8dfb96dc236790ace9ed72 Mon Sep 17 00:00:00 2001 From: Anton Ivashkin Date: Mon, 18 Nov 2024 16:18:36 +0100 Subject: [PATCH 28/49] Antother style fix --- tests/integration/test_cluster_discovery/test.py | 4 +--- .../test_cluster_discovery/test_auxiliary_keeper.py | 4 +--- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/tests/integration/test_cluster_discovery/test.py b/tests/integration/test_cluster_discovery/test.py index 4c447ac06a2..7eb1000b23a 100644 --- a/tests/integration/test_cluster_discovery/test.py +++ b/tests/integration/test_cluster_discovery/test.py @@ -121,6 +121,4 @@ def test_cluster_discovery_startup_and_stop(start_cluster): ) # cleanup - nodes["node0"].query( - "DROP TABLE tbl ON CLUSTER 'test_auto_cluster' SYNC" - ) + nodes["node0"].query("DROP TABLE tbl ON CLUSTER 'test_auto_cluster' SYNC") diff --git a/tests/integration/test_cluster_discovery/test_auxiliary_keeper.py b/tests/integration/test_cluster_discovery/test_auxiliary_keeper.py index fe2e885b3c9..d35a22ab09a 100644 --- a/tests/integration/test_cluster_discovery/test_auxiliary_keeper.py +++ b/tests/integration/test_cluster_discovery/test_auxiliary_keeper.py @@ -124,6 +124,4 @@ def test_cluster_discovery_with_auxiliary_keeper_startup_and_stop(start_cluster) ) # cleanup - nodes["node0"].query( - "DROP TABLE tbl ON CLUSTER 'test_auto_cluster' SYNC" - ) + nodes["node0"].query("DROP TABLE tbl ON CLUSTER 'test_auto_cluster' SYNC") From ff7e1333c2bcfc54676f9d88465678f19287b1ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Wed, 6 Nov 2024 13:27:11 +0100 Subject: [PATCH 29/49] Fix incorrect setting order (cherry picked from commit 3f22f01e8abe27040c2bab5bbb5f41a2f16f5cc8) --- src/Databases/DatabasesCommon.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Databases/DatabasesCommon.cpp b/src/Databases/DatabasesCommon.cpp index 23d199cd160..7bb4d23ef92 100644 --- a/src/Databases/DatabasesCommon.cpp +++ b/src/Databases/DatabasesCommon.cpp @@ -55,8 +55,8 @@ void validateCreateQuery(const ASTCreateQuery & query, ContextPtr context) serialized_query.data() + serialized_query.size(), "after altering table ", 0, - context->getSettingsRef()[Setting::max_parser_backtracks], - context->getSettingsRef()[Setting::max_parser_depth]); + context->getSettingsRef()[Setting::max_parser_depth], + context->getSettingsRef()[Setting::max_parser_backtracks]); const auto & new_query = new_query_raw->as(); /// If there are no columns, then there is nothing much we can do if (!new_query.columns_list || !new_query.columns_list->columns) From 86eb3d64258b8adfc971b619faeaadeffc6257cf Mon Sep 17 00:00:00 2001 From: Andrey Zvonov Date: Fri, 4 Oct 2024 12:05:59 +0000 Subject: [PATCH 30/49] Revive #42537 Co-authored-by: Enmk fix memory access --- docs/ru/operations/settings/settings.md | 6 + programs/client/Client.cpp | 2 +- src/Access/ContextAccess.cpp | 8 +- src/Access/ContextAccessParams.cpp | 15 + src/Access/ContextAccessParams.h | 2 + src/Access/LDAPAccessStorage.cpp | 5 +- src/Client/ClientBase.cpp | 2 + src/Client/Connection.cpp | 17 +- src/Client/Connection.h | 1 + src/Client/HedgedConnections.cpp | 8 +- src/Client/HedgedConnections.h | 3 +- src/Client/IConnections.h | 3 +- src/Client/IServerConnection.h | 1 + src/Client/LocalConnection.cpp | 1 + src/Client/LocalConnection.h | 1 + src/Client/MultiplexedConnections.cpp | 7 +- src/Client/MultiplexedConnections.h | 3 +- src/Client/Suggest.cpp | 2 +- src/Core/ProtocolDefines.h | 5 +- src/Core/Settings.cpp | 3 + src/Core/SettingsChangesHistory.cpp | 1 + src/Interpreters/Context.cpp | 18 +- src/Interpreters/Context.h | 5 +- src/Interpreters/Session.cpp | 42 +- src/Interpreters/Session.h | 9 +- src/QueryPipeline/RemoteInserter.cpp | 3 +- src/QueryPipeline/RemoteQueryExecutor.cpp | 27 +- src/Server/TCPHandler.cpp | 27 +- .../test_ldap_role_mapping/__init__.py | 0 .../clickhouse/config.d/ldap_servers.xml | 11 + .../configs/clickhouse/config.d/logger.xml | 15 + .../clickhouse/config.d/remote_servers.xml | 46 ++ .../configs/ldap/export.ldif | 64 +++ .../test_ldap_role_mapping/test.py | 411 ++++++++++++++++++ 34 files changed, 733 insertions(+), 41 deletions(-) create mode 100644 tests/integration/test_ldap_role_mapping/__init__.py create mode 100644 tests/integration/test_ldap_role_mapping/configs/clickhouse/config.d/ldap_servers.xml create mode 100644 tests/integration/test_ldap_role_mapping/configs/clickhouse/config.d/logger.xml create mode 100644 tests/integration/test_ldap_role_mapping/configs/clickhouse/config.d/remote_servers.xml create mode 100644 tests/integration/test_ldap_role_mapping/configs/ldap/export.ldif create mode 100644 tests/integration/test_ldap_role_mapping/test.py diff --git a/docs/ru/operations/settings/settings.md b/docs/ru/operations/settings/settings.md index bbe1f071381..222acc7aa4c 100644 --- a/docs/ru/operations/settings/settings.md +++ b/docs/ru/operations/settings/settings.md @@ -4215,3 +4215,9 @@ SELECT toFloat64('1.7091'), toFloat64('1.5008753E7') SETTINGS precise_float_pars │ 1.7091 │ 15008753 │ └─────────────────────┴──────────────────────────┘ ``` + +## push_external_roles_in_interserver_queries + +Позволяет передавать роли пользователя от инициатора запроса другим нодам при выполнении запроса. + +Значение по умолчанию: `true`. diff --git a/programs/client/Client.cpp b/programs/client/Client.cpp index 05e1e61be7b..f78071b1278 100644 --- a/programs/client/Client.cpp +++ b/programs/client/Client.cpp @@ -220,7 +220,7 @@ std::vector Client::loadWarningMessages() "" /* query_id */, QueryProcessingStage::Complete, &client_context->getSettingsRef(), - &client_context->getClientInfo(), false, {}); + &client_context->getClientInfo(), false, {}, {}); while (true) { Packet packet = connection->receivePacket(); diff --git a/src/Access/ContextAccess.cpp b/src/Access/ContextAccess.cpp index 06e89d78339..fd7803f1d27 100644 --- a/src/Access/ContextAccess.cpp +++ b/src/Access/ContextAccess.cpp @@ -366,6 +366,13 @@ void ContextAccess::setUser(const UserPtr & user_) const current_roles_with_admin_option = user->granted_roles.findGrantedWithAdminOption(*params.current_roles); } + if (params.external_roles && !params.external_roles->empty()) + { + current_roles.insert(current_roles.end(), params.external_roles->begin(), params.external_roles->end()); + auto new_granted_with_admin_option = user->granted_roles.findGrantedWithAdminOption(*params.external_roles); + current_roles_with_admin_option.insert(current_roles_with_admin_option.end(), new_granted_with_admin_option.begin(), new_granted_with_admin_option.end()); + } + subscription_for_roles_changes.reset(); enabled_roles = access_control->getEnabledRoles(current_roles, current_roles_with_admin_option); subscription_for_roles_changes = enabled_roles->subscribeForChanges([weak_ptr = weak_from_this()](const std::shared_ptr & roles_info_) @@ -516,7 +523,6 @@ std::optional ContextAccess::getQuotaUsage() const return getQuota()->getUsage(); } - SettingsChanges ContextAccess::getDefaultSettings() const { std::lock_guard lock{mutex}; diff --git a/src/Access/ContextAccessParams.cpp b/src/Access/ContextAccessParams.cpp index f5a405c7bc1..4d86940b842 100644 --- a/src/Access/ContextAccessParams.cpp +++ b/src/Access/ContextAccessParams.cpp @@ -18,6 +18,7 @@ ContextAccessParams::ContextAccessParams( bool full_access_, bool use_default_roles_, const std::shared_ptr> & current_roles_, + const std::shared_ptr> & external_roles_, const Settings & settings_, const String & current_database_, const ClientInfo & client_info_) @@ -25,6 +26,7 @@ ContextAccessParams::ContextAccessParams( , full_access(full_access_) , use_default_roles(use_default_roles_) , current_roles(current_roles_) + , external_roles(external_roles_) , readonly(settings_[Setting::readonly]) , allow_ddl(settings_[Setting::allow_ddl]) , allow_introspection(settings_[Setting::allow_introspection_functions]) @@ -59,6 +61,17 @@ String ContextAccessParams::toString() const } out << "]"; } + if (external_roles && !external_roles->empty()) + { + out << separator() << "external_roles = ["; + for (size_t i = 0; i != external_roles->size(); ++i) + { + if (i) + out << ", "; + out << (*external_roles)[i]; + } + out << "]"; + } if (readonly) out << separator() << "readonly = " << readonly; if (allow_ddl) @@ -107,6 +120,7 @@ bool operator ==(const ContextAccessParams & left, const ContextAccessParams & r CONTEXT_ACCESS_PARAMS_EQUALS(full_access) CONTEXT_ACCESS_PARAMS_EQUALS(use_default_roles) CONTEXT_ACCESS_PARAMS_EQUALS(current_roles) + CONTEXT_ACCESS_PARAMS_EQUALS(external_roles) CONTEXT_ACCESS_PARAMS_EQUALS(readonly) CONTEXT_ACCESS_PARAMS_EQUALS(allow_ddl) CONTEXT_ACCESS_PARAMS_EQUALS(allow_introspection) @@ -157,6 +171,7 @@ bool operator <(const ContextAccessParams & left, const ContextAccessParams & ri CONTEXT_ACCESS_PARAMS_LESS(full_access) CONTEXT_ACCESS_PARAMS_LESS(use_default_roles) CONTEXT_ACCESS_PARAMS_LESS(current_roles) + CONTEXT_ACCESS_PARAMS_LESS(external_roles) CONTEXT_ACCESS_PARAMS_LESS(readonly) CONTEXT_ACCESS_PARAMS_LESS(allow_ddl) CONTEXT_ACCESS_PARAMS_LESS(allow_introspection) diff --git a/src/Access/ContextAccessParams.h b/src/Access/ContextAccessParams.h index 07503a3af6d..82592d630dd 100644 --- a/src/Access/ContextAccessParams.h +++ b/src/Access/ContextAccessParams.h @@ -19,6 +19,7 @@ public: bool full_access_, bool use_default_roles_, const std::shared_ptr> & current_roles_, + const std::shared_ptr> & external_roles_, const Settings & settings_, const String & current_database_, const ClientInfo & client_info_); @@ -31,6 +32,7 @@ public: const bool use_default_roles; const std::shared_ptr> current_roles; + const std::shared_ptr> external_roles; const UInt64 readonly; const bool allow_ddl; diff --git a/src/Access/LDAPAccessStorage.cpp b/src/Access/LDAPAccessStorage.cpp index 2636a3cffbb..c1bf8b60495 100644 --- a/src/Access/LDAPAccessStorage.cpp +++ b/src/Access/LDAPAccessStorage.cpp @@ -26,7 +26,6 @@ namespace ErrorCodes extern const int BAD_ARGUMENTS; } - LDAPAccessStorage::LDAPAccessStorage(const String & storage_name_, AccessControl & access_control_, const Poco::Util::AbstractConfiguration & config, const String & prefix) : IAccessStorage(storage_name_), access_control(access_control_), memory_storage(storage_name_, access_control.getChangesNotifier(), false) { @@ -320,6 +319,10 @@ std::set LDAPAccessStorage::mapExternalRolesNoLock(const LDAPClient::Sea { std::set role_names; + // If this node can't access LDAP server (or has not privileges to fetch roles) and gets empty list of external roles + if (external_roles.size() == 0) + return role_names; + if (external_roles.size() != role_search_params.size()) throw Exception(ErrorCodes::BAD_ARGUMENTS, "Unable to map external roles"); diff --git a/src/Client/ClientBase.cpp b/src/Client/ClientBase.cpp index 5a6f05ac98e..c0f5744a4d5 100644 --- a/src/Client/ClientBase.cpp +++ b/src/Client/ClientBase.cpp @@ -1121,6 +1121,7 @@ void ClientBase::processOrdinaryQuery(const String & query_to_execute, ASTPtr pa &client_context->getSettingsRef(), &client_context->getClientInfo(), true, + {}, [&](const Progress & progress) { onProgress(progress); }); if (send_external_tables) @@ -1624,6 +1625,7 @@ void ClientBase::processInsertQuery(const String & query_to_execute, ASTPtr pars &client_context->getSettingsRef(), &client_context->getClientInfo(), true, + {}, [&](const Progress & progress) { onProgress(progress); }); if (send_external_tables) diff --git a/src/Client/Connection.cpp b/src/Client/Connection.cpp index 8301eda9334..5587d793ec2 100644 --- a/src/Client/Connection.cpp +++ b/src/Client/Connection.cpp @@ -14,6 +14,7 @@ #include #include #include +#include "Common/logger_useful.h" #include #include #include @@ -22,8 +23,8 @@ #include #include #include -#include #include +#include #include #include #include @@ -752,6 +753,7 @@ void Connection::sendQuery( const Settings * settings, const ClientInfo * client_info, bool with_pending_data, + const std::vector & external_roles, std::function) { OpenTelemetry::SpanHolder span("Connection::sendQuery()", OpenTelemetry::SpanKind::CLIENT); @@ -824,6 +826,18 @@ void Connection::sendQuery( else writeStringBinary("" /* empty string is a marker of the end of settings */, *out); + String external_roles_str; + if (server_revision >= DBMS_MIN_PROTOCOL_VERSION_WITH_INTERSERVER_EXTERNALLY_GRANTED_ROLES) + { + WriteBufferFromString buffer(external_roles_str); + writeVectorBinary(external_roles, buffer); + buffer.finalize(); + + LOG_DEBUG(log_wrapper.get(), "Sending external_roles with query: [{}] ({})", fmt::join(external_roles, ", "), external_roles.size()); + + writeStringBinary(external_roles_str, *out); + } + /// Interserver secret if (server_revision >= DBMS_MIN_REVISION_WITH_INTERSERVER_SECRET) { @@ -844,6 +858,7 @@ void Connection::sendQuery( data += query; data += query_id; data += client_info->initial_user; + data += external_roles_str; /// TODO: add source/target host/ip-address std::string hash = encodeSHA256(data); diff --git a/src/Client/Connection.h b/src/Client/Connection.h index 5ecafe59faf..ad43a710575 100644 --- a/src/Client/Connection.h +++ b/src/Client/Connection.h @@ -108,6 +108,7 @@ public: const Settings * settings/* = nullptr */, const ClientInfo * client_info/* = nullptr */, bool with_pending_data/* = false */, + const std::vector & external_roles, std::function process_progress_callback) override; void sendCancel() override; diff --git a/src/Client/HedgedConnections.cpp b/src/Client/HedgedConnections.cpp index c30ca0eb9d4..6c2a1a88ee4 100644 --- a/src/Client/HedgedConnections.cpp +++ b/src/Client/HedgedConnections.cpp @@ -161,7 +161,8 @@ void HedgedConnections::sendQuery( const String & query_id, UInt64 stage, ClientInfo & client_info, - bool with_pending_data) + bool with_pending_data, + const std::vector & external_roles) { std::lock_guard lock(cancel_mutex); @@ -188,7 +189,7 @@ void HedgedConnections::sendQuery( hedged_connections_factory.skipReplicasWithTwoLevelAggregationIncompatibility(); } - auto send_query = [this, timeouts, query, query_id, stage, client_info, with_pending_data](ReplicaState & replica) + auto send_query = [this, timeouts, query, query_id, stage, client_info, with_pending_data, external_roles](ReplicaState & replica) { Settings modified_settings = settings; @@ -218,7 +219,8 @@ void HedgedConnections::sendQuery( modified_settings.set("allow_experimental_analyzer", static_cast(modified_settings[Setting::allow_experimental_analyzer])); replica.connection->sendQuery( - timeouts, query, /* query_parameters */ {}, query_id, stage, &modified_settings, &client_info, with_pending_data, {}); + timeouts, query, /* query_parameters */ {}, query_id, stage, &modified_settings, &client_info, with_pending_data, external_roles, {}); + replica.change_replica_timeout.setRelative(timeouts.receive_data_timeout); replica.packet_receiver->setTimeout(hedged_connections_factory.getConnectionTimeouts().receive_timeout); }; diff --git a/src/Client/HedgedConnections.h b/src/Client/HedgedConnections.h index 7f538804e5a..e64f17658d8 100644 --- a/src/Client/HedgedConnections.h +++ b/src/Client/HedgedConnections.h @@ -90,7 +90,8 @@ public: const String & query_id, UInt64 stage, ClientInfo & client_info, - bool with_pending_data) override; + bool with_pending_data, + const std::vector & external_roles) override; void sendReadTaskResponse(const String &) override { diff --git a/src/Client/IConnections.h b/src/Client/IConnections.h index 09211de53b0..a521fdd8b00 100644 --- a/src/Client/IConnections.h +++ b/src/Client/IConnections.h @@ -23,7 +23,8 @@ public: const String & query_id, UInt64 stage, ClientInfo & client_info, - bool with_pending_data) = 0; + bool with_pending_data, + const std::vector & external_roles) = 0; virtual void sendReadTaskResponse(const String &) = 0; virtual void sendMergeTreeReadTaskResponse(const ParallelReadResponse & response) = 0; diff --git a/src/Client/IServerConnection.h b/src/Client/IServerConnection.h index 98cb820f6ad..332481d2701 100644 --- a/src/Client/IServerConnection.h +++ b/src/Client/IServerConnection.h @@ -100,6 +100,7 @@ public: const Settings * settings, const ClientInfo * client_info, bool with_pending_data, + const std::vector & external_roles, std::function process_progress_callback) = 0; virtual void sendCancel() = 0; diff --git a/src/Client/LocalConnection.cpp b/src/Client/LocalConnection.cpp index 6e703a59530..bb36d0bbf39 100644 --- a/src/Client/LocalConnection.cpp +++ b/src/Client/LocalConnection.cpp @@ -106,6 +106,7 @@ void LocalConnection::sendQuery( const Settings *, const ClientInfo * client_info, bool, + const std::vector & /*external_roles*/, std::function process_progress_callback) { /// Last query may not have been finished or cancelled due to exception on client side. diff --git a/src/Client/LocalConnection.h b/src/Client/LocalConnection.h index a70ed6ffa7e..c605b37b075 100644 --- a/src/Client/LocalConnection.h +++ b/src/Client/LocalConnection.h @@ -114,6 +114,7 @@ public: const Settings * settings/* = nullptr */, const ClientInfo * client_info/* = nullptr */, bool with_pending_data/* = false */, + const std::vector & external_roles, std::function process_progress_callback) override; void sendCancel() override; diff --git a/src/Client/MultiplexedConnections.cpp b/src/Client/MultiplexedConnections.cpp index 1cc6ec537c8..c17a31b2d1c 100644 --- a/src/Client/MultiplexedConnections.cpp +++ b/src/Client/MultiplexedConnections.cpp @@ -128,7 +128,8 @@ void MultiplexedConnections::sendQuery( const String & query_id, UInt64 stage, ClientInfo & client_info, - bool with_pending_data) + bool with_pending_data, + const std::vector & external_roles) { std::lock_guard lock(cancel_mutex); @@ -181,14 +182,14 @@ void MultiplexedConnections::sendQuery( modified_settings[Setting::parallel_replica_offset] = i; replica_states[i].connection->sendQuery( - timeouts, query, /* query_parameters */ {}, query_id, stage, &modified_settings, &client_info, with_pending_data, {}); + timeouts, query, /* query_parameters */ {}, query_id, stage, &modified_settings, &client_info, with_pending_data, external_roles, {}); } } else { /// Use single replica. replica_states[0].connection->sendQuery( - timeouts, query, /* query_parameters */ {}, query_id, stage, &modified_settings, &client_info, with_pending_data, {}); + timeouts, query, /* query_parameters */ {}, query_id, stage, &modified_settings, &client_info, with_pending_data, external_roles, {}); } sent_query = true; diff --git a/src/Client/MultiplexedConnections.h b/src/Client/MultiplexedConnections.h index dec32e52d4f..4b308dca02e 100644 --- a/src/Client/MultiplexedConnections.h +++ b/src/Client/MultiplexedConnections.h @@ -36,7 +36,8 @@ public: const String & query_id, UInt64 stage, ClientInfo & client_info, - bool with_pending_data) override; + bool with_pending_data, + const std::vector & external_roles) override; void sendReadTaskResponse(const String &) override; void sendMergeTreeReadTaskResponse(const ParallelReadResponse & response) override; diff --git a/src/Client/Suggest.cpp b/src/Client/Suggest.cpp index 4f68ceecc37..e8f5409a009 100644 --- a/src/Client/Suggest.cpp +++ b/src/Client/Suggest.cpp @@ -163,7 +163,7 @@ void Suggest::load(IServerConnection & connection, void Suggest::fetch(IServerConnection & connection, const ConnectionTimeouts & timeouts, const std::string & query, const ClientInfo & client_info) { connection.sendQuery( - timeouts, query, {} /* query_parameters */, "" /* query_id */, QueryProcessingStage::Complete, nullptr, &client_info, false, {}); + timeouts, query, {} /* query_parameters */, "" /* query_id */, QueryProcessingStage::Complete, nullptr, &client_info, false, {} /* external_roles*/, {}); while (true) { diff --git a/src/Core/ProtocolDefines.h b/src/Core/ProtocolDefines.h index b68eff0aa5a..cd89ca013ee 100644 --- a/src/Core/ProtocolDefines.h +++ b/src/Core/ProtocolDefines.h @@ -90,6 +90,9 @@ static constexpr auto DBMS_MIN_PROTOCOL_VERSION_WITH_CHUNKED_PACKETS = 54470; static constexpr auto DBMS_MIN_REVISION_WITH_VERSIONED_PARALLEL_REPLICAS_PROTOCOL = 54471; +/// Push externally granted roles to other nodes +static constexpr auto DBMS_MIN_PROTOCOL_VERSION_WITH_INTERSERVER_EXTERNALLY_GRANTED_ROLES = 54472; + /// Version of ClickHouse TCP protocol. /// /// Should be incremented manually on protocol changes. @@ -97,6 +100,6 @@ static constexpr auto DBMS_MIN_REVISION_WITH_VERSIONED_PARALLEL_REPLICAS_PROTOCO /// NOTE: DBMS_TCP_PROTOCOL_VERSION has nothing common with VERSION_REVISION, /// later is just a number for server version (one number instead of commit SHA) /// for simplicity (sometimes it may be more convenient in some use cases). -static constexpr auto DBMS_TCP_PROTOCOL_VERSION = 54471; +static constexpr auto DBMS_TCP_PROTOCOL_VERSION = 54472; } diff --git a/src/Core/Settings.cpp b/src/Core/Settings.cpp index 0a34d168413..c73d1861ca5 100644 --- a/src/Core/Settings.cpp +++ b/src/Core/Settings.cpp @@ -5727,6 +5727,9 @@ If enabled, MongoDB tables will return an error when a MongoDB query cannot be b Allow writing simple SELECT queries without the leading SELECT keyword, which makes it simple for calculator-style usage, e.g. `1 + 2` becomes a valid query. In `clickhouse-local` it is enabled by default and can be explicitly disabled. +)", 0) \ + DECLARE(Bool, push_external_roles_in_interserver_queries, true, R"( +Enable pushing user roles from originator to other nodes while performing a query. )", 0) \ \ \ diff --git a/src/Core/SettingsChangesHistory.cpp b/src/Core/SettingsChangesHistory.cpp index 5944de54142..34a85eb5faf 100644 --- a/src/Core/SettingsChangesHistory.cpp +++ b/src/Core/SettingsChangesHistory.cpp @@ -86,6 +86,7 @@ static std::initializer_listusers_config; } -void Context::setUser(const UUID & user_id_) +void Context::setUser(const UUID & user_id_, const std::vector & external_roles_) { /// Prepare lists of user's profiles, constraints, settings, roles. /// NOTE: AccessControl::read() and other AccessControl's functions may require some IO work, @@ -1508,7 +1508,6 @@ void Context::setUser(const UUID & user_id_) const auto & database = user->default_database; /// Apply user's profiles, constraints, settings, roles. - std::lock_guard lock(mutex); setUserIDWithLock(user_id_, lock); @@ -1518,6 +1517,7 @@ void Context::setUser(const UUID & user_id_) setCurrentProfilesWithLock(*enabled_profiles, /* check_constraints= */ false, lock); setCurrentRolesWithLock(default_roles, lock); + setExternalRolesWithLock(external_roles_, lock); /// It's optional to specify the DEFAULT DATABASE in the user's definition. if (!database.empty()) @@ -1561,6 +1561,18 @@ void Context::setCurrentRolesWithLock(const std::vector & new_current_role need_recalculate_access = true; } +void Context::setExternalRolesWithLock(const std::vector & new_external_roles, const std::lock_guard &) +{ + if (external_roles && !external_roles->empty()) + { + if (current_roles) + current_roles->insert(current_roles->end(), new_external_roles.begin(), new_external_roles.end()); + else + current_roles = std::make_shared>(new_external_roles); + } + need_recalculate_access = true; +} + void Context::setCurrentRolesImpl(const std::vector & new_current_roles, bool throw_if_not_granted, bool skip_if_not_granted, const std::shared_ptr & user) { if (skip_if_not_granted) @@ -1675,7 +1687,7 @@ std::shared_ptr Context::getAccess() const bool full_access = !user_id; return ContextAccessParams{ - user_id, full_access, /* use_default_roles= */ false, current_roles, *settings, current_database, client_info}; + user_id, full_access, /* use_default_roles= */ false, current_roles, external_roles, *settings, current_database, client_info}; }; /// Check if the current access rights are still valid, otherwise get parameters for recalculating access rights. diff --git a/src/Interpreters/Context.h b/src/Interpreters/Context.h index e8ccc31f597..327ac0af5fd 100644 --- a/src/Interpreters/Context.h +++ b/src/Interpreters/Context.h @@ -289,6 +289,7 @@ protected: std::optional user_id; std::shared_ptr> current_roles; + std::shared_ptr> external_roles; std::shared_ptr settings_constraints_and_current_profiles; mutable std::shared_ptr access; mutable bool need_recalculate_access = true; @@ -634,7 +635,7 @@ public: /// Sets the current user assuming that he/she is already authenticated. /// WARNING: This function doesn't check password! - void setUser(const UUID & user_id_); + void setUser(const UUID & user_id_, const std::vector & external_roles_ = {}); UserPtr getUser() const; std::optional getUserID() const; @@ -1398,6 +1399,8 @@ private: void setCurrentRolesWithLock(const std::vector & new_current_roles, const std::lock_guard & lock); + void setExternalRolesWithLock(const std::vector & new_external_roles, const std::lock_guard & lock); + void setSettingWithLock(std::string_view name, const String & value, const std::lock_guard & lock); void setSettingWithLock(std::string_view name, const Field & value, const std::lock_guard & lock); diff --git a/src/Interpreters/Session.cpp b/src/Interpreters/Session.cpp index bc6555af595..60a5b0a850f 100644 --- a/src/Interpreters/Session.cpp +++ b/src/Interpreters/Session.cpp @@ -6,6 +6,8 @@ #include #include #include +#include +#include #include #include #include @@ -25,12 +27,12 @@ #include #include - namespace DB { namespace Setting { extern const SettingsUInt64 max_sessions_for_user; + extern const SettingsBool push_external_roles_in_interserver_queries; } namespace ErrorCodes @@ -288,7 +290,7 @@ void Session::shutdownNamedSessions() Session::Session(const ContextPtr & global_context_, ClientInfo::Interface interface_, bool is_secure, const std::string & certificate) : auth_id(UUIDHelpers::generateV4()), global_context(global_context_), - log(getLogger(String{magic_enum::enum_name(interface_)} + "-Session")) + log(getLogger(String{magic_enum::enum_name(interface_)} + "-Session-" + toString(auth_id))) { prepared_client_info.emplace(); prepared_client_info->interface = interface_; @@ -342,12 +344,12 @@ std::unordered_set Session::getAuthenticationTypesOrLogInFai } } -void Session::authenticate(const String & user_name, const String & password, const Poco::Net::SocketAddress & address) +void Session::authenticate(const String & user_name, const String & password, const Poco::Net::SocketAddress & address, const Strings & external_roles_) { - authenticate(BasicCredentials{user_name, password}, address); + authenticate(BasicCredentials{user_name, password}, address, external_roles_); } -void Session::authenticate(const Credentials & credentials_, const Poco::Net::SocketAddress & address_) +void Session::authenticate(const Credentials & credentials_, const Poco::Net::SocketAddress & address_, const Strings & external_roles_) { if (session_context) throw Exception(ErrorCodes::LOGICAL_ERROR, "If there is a session context it must be created after authentication"); @@ -359,8 +361,8 @@ void Session::authenticate(const Credentials & credentials_, const Poco::Net::So if ((address == Poco::Net::SocketAddress{}) && (prepared_client_info->interface == ClientInfo::Interface::LOCAL)) address = Poco::Net::SocketAddress{"127.0.0.1", 0}; - LOG_DEBUG(log, "{} Authenticating user '{}' from {}", - toString(auth_id), credentials_.getUserName(), address.toString()); + LOG_DEBUG(log, "Authenticating user '{}' from {}", + credentials_.getUserName(), address.toString()); try { @@ -370,6 +372,14 @@ void Session::authenticate(const Credentials & credentials_, const Poco::Net::So settings_from_auth_server = auth_result.settings; LOG_DEBUG(log, "{} Authenticated with global context as user {}", toString(auth_id), toString(*user_id)); + + if (!external_roles_.empty() && global_context->getSettingsRef()[Setting::push_external_roles_in_interserver_queries]) + { + external_roles = global_context->getAccessControl().find(external_roles_); + + LOG_DEBUG(log, "User {} has external_roles applied: [{}] ({})", + toString(*user_id), fmt::join(external_roles_, ", "), external_roles_.size()); + } } catch (const Exception & e) { @@ -394,7 +404,7 @@ void Session::checkIfUserIsStillValid() void Session::onAuthenticationFailure(const std::optional & user_name, const Poco::Net::SocketAddress & address_, const Exception & e) { - LOG_DEBUG(log, "{} Authentication failed with error: {}", toString(auth_id), e.what()); + LOG_DEBUG(log, "Authentication failed with error: {}", e.what()); if (auto session_log = getSessionLog()) { /// Add source address to the log @@ -520,8 +530,8 @@ ContextMutablePtr Session::makeSessionContext() if (session_tracker_handle) throw Exception(ErrorCodes::LOGICAL_ERROR, "Session tracker handle was created before making session"); - LOG_DEBUG(log, "{} Creating session context with user_id: {}", - toString(auth_id), toString(*user_id)); + LOG_DEBUG(log, "Creating session context with user_id: {}", + toString(*user_id)); /// Make a new session context. ContextMutablePtr new_session_context; new_session_context = Context::createCopy(global_context); @@ -532,7 +542,7 @@ ContextMutablePtr Session::makeSessionContext() prepared_client_info.reset(); /// Set user information for the new context: current profiles, roles, access rights. - new_session_context->setUser(*user_id); + new_session_context->setUser(*user_id, external_roles); /// Session context is ready. session_context = new_session_context; @@ -563,8 +573,8 @@ ContextMutablePtr Session::makeSessionContext(const String & session_name_, std: if (session_tracker_handle) throw Exception(ErrorCodes::LOGICAL_ERROR, "Session tracker handle was created before making session"); - LOG_DEBUG(log, "{} Creating named session context with name: {}, user_id: {}", - toString(auth_id), session_name_, toString(*user_id)); + LOG_DEBUG(log, "Creating named session context with name: {}, user_id: {}", + session_name_, toString(*user_id)); /// Make a new session context OR /// if the `session_id` and `user_id` were used before then just get a previously created session context. @@ -587,7 +597,7 @@ ContextMutablePtr Session::makeSessionContext(const String & session_name_, std: /// Set user information for the new context: current profiles, roles, access rights. if (!access->tryGetUser()) { - new_session_context->setUser(*user_id); + new_session_context->setUser(*user_id, external_roles); max_sessions_for_user = new_session_context->getSettingsRef()[Setting::max_sessions_for_user]; } else @@ -639,7 +649,7 @@ ContextMutablePtr Session::makeQueryContextImpl(const ClientInfo * client_info_t throw Exception(ErrorCodes::LOGICAL_ERROR, "Query context must be created after authentication"); /// We can create a query context either from a session context or from a global context. - bool from_session_context = static_cast(session_context); + const bool from_session_context = static_cast(session_context); /// Create a new query context. ContextMutablePtr query_context = Context::createCopy(from_session_context ? session_context : global_context); @@ -679,7 +689,7 @@ ContextMutablePtr Session::makeQueryContextImpl(const ClientInfo * client_info_t /// Set user information for the new context: current profiles, roles, access rights. if (user_id && !query_context->getAccess()->tryGetUser()) - query_context->setUser(*user_id); + query_context->setUser(*user_id, external_roles); /// Query context is ready. query_context_created = true; diff --git a/src/Interpreters/Session.h b/src/Interpreters/Session.h index 0a20dd896a9..d2957964925 100644 --- a/src/Interpreters/Session.h +++ b/src/Interpreters/Session.h @@ -10,6 +10,7 @@ #include #include #include +#include namespace Poco::Net { class SocketAddress; } @@ -50,8 +51,11 @@ public: /// Sets the current user, checks the credentials and that the specified address is allowed to connect from. /// The function throws an exception if there is no such user or password is wrong. - void authenticate(const String & user_name, const String & password, const Poco::Net::SocketAddress & address); - void authenticate(const Credentials & credentials_, const Poco::Net::SocketAddress & address_); + void authenticate(const String & user_name, const String & password, const Poco::Net::SocketAddress & address, const Strings & external_roles_ = {}); + + /// `external_roles_` names of the additional roles (over what is granted via local access control mechanisms) that would be granted to user during this session. + /// Role is not granted if it can't be found by name via AccessControl (i.e. doesn't exist on this instance). + void authenticate(const Credentials & credentials_, const Poco::Net::SocketAddress & address_, const Strings & external_roles_ = {}); // Verifies whether the user's validity extends beyond the current time. // Throws an exception if the user's validity has expired. @@ -112,6 +116,7 @@ private: mutable UserPtr user; std::optional user_id; + std::vector external_roles; AuthenticationData user_authenticated_with; ContextMutablePtr session_context; diff --git a/src/QueryPipeline/RemoteInserter.cpp b/src/QueryPipeline/RemoteInserter.cpp index b958924f008..d0c42a068af 100644 --- a/src/QueryPipeline/RemoteInserter.cpp +++ b/src/QueryPipeline/RemoteInserter.cpp @@ -56,8 +56,9 @@ RemoteInserter::RemoteInserter( /** Send query and receive "header", that describes table structure. * Header is needed to know, what structure is required for blocks to be passed to 'write' method. */ + /// TODO (vnemkov): figure out should we pass additional roles in this case or not. connection.sendQuery( - timeouts, query, /* query_parameters */ {}, "", QueryProcessingStage::Complete, &settings, &modified_client_info, false, {}); + timeouts, query, /* query_parameters */ {}, "", QueryProcessingStage::Complete, &settings, &modified_client_info, false, /* external_roles */ {}, {}); while (true) { diff --git a/src/QueryPipeline/RemoteQueryExecutor.cpp b/src/QueryPipeline/RemoteQueryExecutor.cpp index 5faae03bc8f..41ab87e1a18 100644 --- a/src/QueryPipeline/RemoteQueryExecutor.cpp +++ b/src/QueryPipeline/RemoteQueryExecutor.cpp @@ -22,8 +22,12 @@ #include #include #include -#include #include +#include + +#include +#include +#include namespace ProfileEvents { @@ -43,6 +47,7 @@ namespace Setting extern const SettingsBool skip_unavailable_shards; extern const SettingsOverflowMode timeout_overflow_mode; extern const SettingsBool use_hedged_requests; + extern const SettingsBool push_external_roles_in_interserver_queries; } namespace ErrorCodes @@ -398,7 +403,25 @@ void RemoteQueryExecutor::sendQueryUnlocked(ClientInfo::QueryKind query_kind, As if (!duplicated_part_uuids.empty()) connections->sendIgnoredPartUUIDs(duplicated_part_uuids); - connections->sendQuery(timeouts, query, query_id, stage, modified_client_info, true); + // Collect all roles granted on this node and pass those to the remote node + std::vector local_granted_roles; + if (context->getSettingsRef()[Setting::push_external_roles_in_interserver_queries] && !modified_client_info.initial_user.empty()) + { + auto user = context->getAccessControl().read(modified_client_info.initial_user, true); + boost::container::flat_set granted_roles; + if (user) + { + const auto & access_control = context->getAccessControl(); + for (const auto & e : user->granted_roles.getElements()) + { + auto names = access_control.readNames(e.ids); + granted_roles.insert(names.begin(), names.end()); + } + } + local_granted_roles.insert(local_granted_roles.end(), granted_roles.begin(), granted_roles.end()); + } + + connections->sendQuery(timeouts, query, query_id, stage, modified_client_info, true, local_granted_roles); established = false; sent_query = true; diff --git a/src/Server/TCPHandler.cpp b/src/Server/TCPHandler.cpp index d23449aced1..01f6af348c5 100644 --- a/src/Server/TCPHandler.cpp +++ b/src/Server/TCPHandler.cpp @@ -63,12 +63,17 @@ #include #include +#include + #include "TCPHandler.h" #include #include +#include +#include + using namespace std::literals; using namespace DB; @@ -1960,6 +1965,13 @@ void TCPHandler::processQuery(std::optional & state) Settings passed_settings; passed_settings.read(*in, settings_format); + std::string received_extra_roles; + // TODO: check if having `is_interserver_mode` doesn't break interoperability with the CH-client. + if (client_tcp_protocol_version >= DBMS_MIN_PROTOCOL_VERSION_WITH_INTERSERVER_EXTERNALLY_GRANTED_ROLES) + { + readStringBinary(received_extra_roles, *in); + } + /// Interserver secret. std::string received_hash; if (client_tcp_protocol_version >= DBMS_MIN_REVISION_WITH_INTERSERVER_SECRET) @@ -2019,6 +2031,7 @@ void TCPHandler::processQuery(std::optional & state) data += state->query; data += state->query_id; data += client_info.initial_user; + data += received_extra_roles; std::string calculated_hash = encodeSHA256(data); assert(calculated_hash.size() == 32); @@ -2039,13 +2052,25 @@ void TCPHandler::processQuery(std::optional & state) } else { + // In a cluster, query originator may have an access to the external auth provider (like LDAP server), + // that grants specific roles to the user. We want these roles to be granted to the user on other nodes of cluster when + // query is executed. + Strings external_roles; + if (!received_extra_roles.empty()) + { + ReadBufferFromString buffer(received_extra_roles); + + readVectorBinary(external_roles, buffer); + LOG_DEBUG(log, "Parsed extra roles [{}]", fmt::join(external_roles, ", ")); + } + LOG_DEBUG(log, "User (initial, interserver mode): {} (client: {})", client_info.initial_user, getClientAddress(client_info).toString()); /// In case of inter-server mode authorization is done with the /// initial address of the client, not the real address from which /// the query was come, since the real address is the address of /// the initiator server, while we are interested in client's /// address. - session->authenticate(AlwaysAllowCredentials{client_info.initial_user}, client_info.initial_address); + session->authenticate(AlwaysAllowCredentials{client_info.initial_user}, client_info.initial_address, external_roles); } is_interserver_authenticated = true; diff --git a/tests/integration/test_ldap_role_mapping/__init__.py b/tests/integration/test_ldap_role_mapping/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/integration/test_ldap_role_mapping/configs/clickhouse/config.d/ldap_servers.xml b/tests/integration/test_ldap_role_mapping/configs/clickhouse/config.d/ldap_servers.xml new file mode 100644 index 00000000000..abdcdc86b35 --- /dev/null +++ b/tests/integration/test_ldap_role_mapping/configs/clickhouse/config.d/ldap_servers.xml @@ -0,0 +1,11 @@ + + + + openldap + 389 + cn={user_name},ou=users,dc=company,dc=com + no + never + + + diff --git a/tests/integration/test_ldap_role_mapping/configs/clickhouse/config.d/logger.xml b/tests/integration/test_ldap_role_mapping/configs/clickhouse/config.d/logger.xml new file mode 100644 index 00000000000..a12ffca5a2e --- /dev/null +++ b/tests/integration/test_ldap_role_mapping/configs/clickhouse/config.d/logger.xml @@ -0,0 +1,15 @@ + + + debug + /var/log/clickhouse-server/log.log + /var/log/clickhouse-server/log.err.log + 1000M + 10 + /var/log/clickhouse-server/stderr.log + /var/log/clickhouse-server/stdout.log + + /var/log/clickhouse-server/clickhouse-library-bridge.log + /var/log/clickhouse-server/clickhouse-library-bridge.err.log + trace + + diff --git a/tests/integration/test_ldap_role_mapping/configs/clickhouse/config.d/remote_servers.xml b/tests/integration/test_ldap_role_mapping/configs/clickhouse/config.d/remote_servers.xml new file mode 100644 index 00000000000..f1e4d95dd53 --- /dev/null +++ b/tests/integration/test_ldap_role_mapping/configs/clickhouse/config.d/remote_servers.xml @@ -0,0 +1,46 @@ + + + + + + instance1 + 9000 + + + + + instance2 + 9000 + + + + + instance3 + 9000 + + + + + + qwerty123 + + + instance1 + 9000 + + + + + instance2 + 9000 + + + + + instance3 + 9000 + + + + + \ No newline at end of file diff --git a/tests/integration/test_ldap_role_mapping/configs/ldap/export.ldif b/tests/integration/test_ldap_role_mapping/configs/ldap/export.ldif new file mode 100644 index 00000000000..82bb1bf89ab --- /dev/null +++ b/tests/integration/test_ldap_role_mapping/configs/ldap/export.ldif @@ -0,0 +1,64 @@ +# LDIF Export for dc=company,dc=com +# Server: openldap (openldap) +# Search Scope: sub +# Search Filter: (objectClass=*) +# Total Entries: 7 +# +# Generated by phpLDAPadmin (http://phpldapadmin.sourceforge.net) on May 22, 2020 5:51 pm +# Version: 1.2.5 + +# Entry 1: dc=company,dc=com +#dn: dc=company,dc=com +#dc: company +#o: company +#objectclass: top +#objectclass: dcObject +#objectclass: organization + +# Entry 2: cn=admin,dc=company,dc=com +#dn: cn=admin,dc=company,dc=com +#cn: admin +#description: LDAP administrator +#objectclass: simpleSecurityObject +#objectclass: organizationalRole +#userpassword: {SSHA}eUEupkQCTvq9SkrxfWGSe5rX+orrjVbF + +# Entry 3: ou=groups,dc=company,dc=com +dn: ou=groups,dc=company,dc=com +objectclass: organizationalUnit +objectclass: top +ou: groups + +# Entry 4: cn=admin,ou=groups,dc=company,dc=com +dn: cn=admin,ou=groups,dc=company,dc=com +cn: admin +gidnumber: 500 +objectclass: posixGroup +objectclass: top + +# Entry 5: cn=users,ou=groups,dc=company,dc=com +dn: cn=users,ou=groups,dc=company,dc=com +cn: users +gidnumber: 501 +objectclass: posixGroup +objectclass: top + +# Entry 6: ou=users,dc=company,dc=com +dn: ou=users,dc=company,dc=com +objectclass: organizationalUnit +objectclass: top +ou: users + +# Entry 7: cn=myuser,ou=users,dc=company,dc=com +# dn: cn=myuser,ou=users,dc=company,dc=com +# cn: myuser +# gidnumber: 501 +# givenname: John +# homedirectory: /home/users/myuser +# objectclass: inetOrgPerson +# objectclass: posixAccount +# objectclass: top +# sn: User +# uid: myuser +# uidnumber: 1101 +# userpassword: myuser diff --git a/tests/integration/test_ldap_role_mapping/test.py b/tests/integration/test_ldap_role_mapping/test.py new file mode 100644 index 00000000000..607223faff3 --- /dev/null +++ b/tests/integration/test_ldap_role_mapping/test.py @@ -0,0 +1,411 @@ +import time +from os import getuid + +import pytest + +from helpers.cluster import ClickHouseCluster + +cluster = ClickHouseCluster(__file__) + + +cluster.add_instance( + "instance1", + main_configs=[ + "configs/clickhouse/config.d/logger.xml", + "configs/clickhouse/config.d/ldap_servers.xml", + "configs/clickhouse/config.d/remote_servers.xml", + ], + macros={"shard": 1, "replica": "instance1"}, + stay_alive=True, + with_ldap=True, + with_zookeeper=True, +) +cluster.add_instance( + "instance2", + main_configs=[ + "configs/clickhouse/config.d/logger.xml", + "configs/clickhouse/config.d/ldap_servers.xml", + "configs/clickhouse/config.d/remote_servers.xml", + ], + macros={"shard": 1, "replica": "instance2"}, + stay_alive=True, + with_ldap=True, + with_zookeeper=True, +) +cluster.add_instance( + "instance3", + main_configs=[ + "configs/clickhouse/config.d/logger.xml", + "configs/clickhouse/config.d/ldap_servers.xml", + "configs/clickhouse/config.d/remote_servers.xml", + ], + macros={"shard": 1, "replica": "instance3"}, + stay_alive=True, + with_ldap=True, + with_zookeeper=True, +) + +instances = [ + cluster.instances["instance1"], + cluster.instances["instance2"], + cluster.instances["instance3"], +] + + +ldap_server = { + "host": "openldap", + "port": "389", + "enable_tls": "no", + "bind_dn": "cn={user_name},ou=users,dc=company,dc=com", +} + +# Fixtures + + +@pytest.fixture(scope="module") +def started_cluster(): + try: + cluster.start() + yield cluster + + finally: + cluster.shutdown() + + +# Helpers + + +def create_table(node, on_cluster, name=None): + if name is None: + name = f"tbl_{getuid()}" + + node.query(f"DROP TABLE IF EXISTS {name} ON CLUSTER {on_cluster} SYNC") + + node.query( + f"CREATE TABLE {name} ON CLUSTER {on_cluster} (d Date, a String, b UInt8, x String, y Int8) " + f"ENGINE = ReplicatedMergeTree('/clickhouse/tables/{{shard}}/{name}', '{{replica}}') " + "PARTITION BY y ORDER BY (d, b)" + ) + + return name + + +def create_distributed_table(node, on_cluster, over, name=None): + if name is None: + name = f"dis_tbl_{getuid()}" + + node.query(f"DROP TABLE IF EXISTS {name} ON CLUSTER {on_cluster} SYNC") + + node.query( + f"CREATE TABLE {name} ON CLUSTER {on_cluster} AS {over} " + f"ENGINE = Distributed({on_cluster}, default, {over}, rand())" + ) + + return name + + +def drop_table(node, name, on_cluster): + node.query(f"DROP TABLE IF EXISTS {name} ON CLUSTER {on_cluster} SYNC") + + +def grant_select(cluster, privilege, role_or_user, node): + """Grant select privilege on a table on a given cluster + to a role or a user. + """ + node.query(f"GRANT ON CLUSTER {cluster} {privilege} TO {role_or_user}") + + +def revoke_select(cluster, privilege, role_or_user, node): + node.query(f"REVOKE ON CLUSTER {cluster} {privilege} FROM {role_or_user}") + + +def fix_ldap_permissions_command(): + ldif = ( + "dn: olcDatabase={1}mdb,cn=config\n" + "changetype: modify\n" + "delete: olcAccess\n" + "-\n" + "add: olcAccess\n" + 'olcAccess: to attrs=userPassword,shadowLastChange by self write by dn=\\"cn=admin,dc=company,dc=com\\" write by anonymous auth by * none\n' + 'olcAccess: to * by self write by dn=\\"cn=admin,dc=company,dc=com\\" read by users read by * none' + ) + + return f'echo -e "{ldif}" | ldapmodify -Y EXTERNAL -Q -H ldapi:///' + + +def add_user_to_ldap_command( + cn, + userpassword, + givenname=None, + homedirectory=None, + sn=None, + uid=None, + uidnumber=None, +): + if uid is None: + uid = cn + if givenname is None: + givenname = "John" + if homedirectory is None: + homedirectory = f"/home/{cn}" + if sn is None: + sn = "User" + if uidnumber is None: + uidnumber = 2000 + + user = { + "dn": f"cn={cn},ou=users,dc=company,dc=com", + "cn": cn, + "gidnumber": 501, + "givenname": givenname, + "homedirectory": homedirectory, + "objectclass": ["inetOrgPerson", "posixAccount", "top"], + "sn": sn, + "uid": uid, + "uidnumber": uidnumber, + "userpassword": userpassword, + "_server": cluster.ldap_host, + } + + lines = [] + + for key, value in list(user.items()): + if key.startswith("_"): + continue + elif key == "objectclass": + for cls in value: + lines.append(f"objectclass: {cls}") + else: + lines.append(f"{key}: {value}") + + ldif = "\n".join(lines) + + return f'echo -e "{ldif}" | ldapadd -x -H ldap://localhost -D "cn=admin,dc=company,dc=com" -w admin' + + +def add_rbac_user(user, node): + username = user.get("username", None) or user["cn"] + password = user.get("password", None) or user["userpassword"] + node.query( + f"CREATE USER OR REPLACE {username} IDENTIFIED WITH PLAINTEXT_PASSWORD BY '{password}'" + ) + + +def add_rbac_role(role, node): + node.query(f"DROP ROLE IF EXISTS {role}") + node.query(f"CREATE ROLE OR REPLACE {role}") + + +def outline_test_select_using_mapped_role(cluster, role_name, role_mapped, user): + # default cluster node + node = instances[0] + + query_settings = {"user": user["username"], "password": user["password"]} + + # create base table on cluster + src_table = create_table(node=node, on_cluster=cluster) + + # create distristibuted table over base table on cluster + dist_table = create_distributed_table(on_cluster=cluster, over=src_table, node=node) + + # check that grants for the user + for instance in instances: + for _ in range(10): + r = instance.query(f"SHOW GRANTS", settings=query_settings) + if role_mapped: + assert role_name in r + else: + time.sleep(1) + + # no privilege on source table + for instance in instances: + assert "Not enough privileges" in instance.query_and_get_error( + f"SELECT * FROM {src_table}", settings=query_settings + ) + + # with privilege on source table + grant_select( + cluster=cluster, + privilege=f"SELECT ON {src_table}", + role_or_user=role_name, + node=node, + ) + + # user should be able to read from the source table + for instance in instances: + if role_mapped: + instance.query(f"SELECT * FROM {src_table}", settings=query_settings) + else: + instance.query_and_get_error( + f"SELECT * FROM {src_table}", settings=query_settings + ) == 241 + + revoke_select( + cluster=cluster, + privilege=f"SELECT ON {src_table}", + role_or_user=role_name, + node=node, + ) + + # privilege only on distributed table + grant_select( + cluster=cluster, + privilege=f"SELECT ON {dist_table}", + role_or_user=role_name, + node=node, + ) + + # user should still not be able to read from distributed table + for instance in instances: + instance.query_and_get_error( + f"SELECT * FROM {dist_table}", settings=query_settings + ) == 241 + + revoke_select( + cluster=cluster, + privilege=f"SELECT ON {dist_table}", + role_or_user=role_name, + node=node, + ) + + # privilege only on source but not on distributed table + grant_select( + cluster=cluster, + privilege=f"SELECT ON {src_table}", + role_or_user=role_name, + node=node, + ) + + # user should still not be able to read from distributed table + for instance in instances: + instance.query_and_get_error( + f"SELECT * FROM {dist_table}", settings=query_settings + ) == 241 + + revoke_select( + cluster=cluster, + privilege=f"SELECT ON {src_table}", + role_or_user=role_name, + node=node, + ) + + # privilege on source and distributed + grant_select( + cluster=cluster, + privilege=f"SELECT ON {src_table}", + role_or_user=role_name, + node=node, + ) + + grant_select( + cluster=cluster, + privilege=f"SELECT ON {dist_table}", + role_or_user=role_name, + node=node, + ) + + # user should be able to read from the distributed table + for instance in instances: + if role_mapped: + instance.query(f"SELECT * FROM {dist_table}", settings=query_settings) + else: + instance.query_and_get_error( + f"SELECT * FROM {dist_table}", settings=query_settings + ) == 241 + + revoke_select( + cluster=cluster, + privilege=f"SELECT ON {src_table}", + role_or_user=role_name, + node=node, + ) + + revoke_select( + cluster=cluster, + privilege=f"SELECT ON {dist_table}", + role_or_user=role_name, + node=node, + ) + + +def execute_tests(role_name, role_mapped, ldap_user, local_user): + for cluster_type in ["with_secret", "without_secret"]: + for user in [ldap_user, local_user]: + if role_mapped and user["type"] == "local user": + for instance in instances: + instance.query(f"GRANT {role_name} TO {local_user['username']}") + + outline_test_select_using_mapped_role( + cluster=f"sharded_cluster_{cluster_type}", + role_name=role_name, + role_mapped=role_mapped, + user=user, + ) + + +def test_using_authenticated_users(started_cluster): + role_name = f"role_{getuid()}" + + ldap_user = { + "type": "ldap authenticated user", + "cn": "myuser", + "username": "myuser", + "userpassword": "myuser", + "password": "myuser", + "server": "openldap", + "uidnumber": 1101, + } + + local_user = { + "type": "local user", + "username": "local_user2", + "password": "local_user2", + } + + # fix_ldap_permissions + cluster.exec_in_container( + cluster.ldap_id, ["bash", "-c", fix_ldap_permissions_command()] + ) + + # add LDAP user + cluster.exec_in_container( + cluster.ldap_id, + [ + "bash", + "-c", + add_user_to_ldap_command( + cn=ldap_user["cn"], + userpassword=ldap_user["userpassword"], + uidnumber=ldap_user["uidnumber"], + ), + ], + ) + + # cluster.exec_in_container(cluster.ldap_id, ["ldapsearch -x -LLL uid=*"]) + # add local RBAC user + for instance in instances: + add_rbac_user(user=local_user, node=instance) + + # add RBAC role on cluster that user will use + for instance in instances: + add_rbac_role(role_name, instance) + + # create LDAP-auth user and grant role + for instance in instances: + instance.query( + f"CREATE USER OR REPLACE {ldap_user['username']} IDENTIFIED WITH LDAP SERVER '{ldap_user['server']}'" + ) + + for instance in instances: + instance.query(f"GRANT {role_name} TO {ldap_user['username']}") + + # grant role to local RBAC user + for instance in instances: + instance.query(f"GRANT {role_name} TO {local_user['username']}") + + execute_tests( + role_name=role_name, + role_mapped=role_name, + ldap_user=ldap_user, + local_user=local_user, + ) From c5b5b5841b52a79c6617a5abc4e171c1e79f5767 Mon Sep 17 00:00:00 2001 From: Andrey Zvonov Date: Sat, 9 Nov 2024 10:58:13 +0000 Subject: [PATCH 31/49] fix typo --- src/Interpreters/Context.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Interpreters/Context.cpp b/src/Interpreters/Context.cpp index 8ee1e9edb2a..953fcfb253c 100644 --- a/src/Interpreters/Context.cpp +++ b/src/Interpreters/Context.cpp @@ -1563,7 +1563,7 @@ void Context::setCurrentRolesWithLock(const std::vector & new_current_role void Context::setExternalRolesWithLock(const std::vector & new_external_roles, const std::lock_guard &) { - if (external_roles && !external_roles->empty()) + if (!new_external_roles.empty()) { if (current_roles) current_roles->insert(current_roles->end(), new_external_roles.begin(), new_external_roles.end()); From 9fadfb98b4afec54150524ecb591d28971d9578f Mon Sep 17 00:00:00 2001 From: Andrey Zvonov Date: Sat, 9 Nov 2024 22:44:59 +0000 Subject: [PATCH 32/49] add a test that works --- .../configs/ldap_with_role_mapping.xml | 1 + .../configs/remote_servers.xml | 18 + .../test_ldap_external_user_directory/test.py | 80 +++- .../test_ldap_role_mapping/__init__.py | 0 .../clickhouse/config.d/ldap_servers.xml | 11 - .../configs/clickhouse/config.d/logger.xml | 15 - .../clickhouse/config.d/remote_servers.xml | 46 -- .../configs/ldap/export.ldif | 64 --- .../test_ldap_role_mapping/test.py | 411 ------------------ 9 files changed, 82 insertions(+), 564 deletions(-) create mode 100644 tests/integration/test_ldap_external_user_directory/configs/remote_servers.xml delete mode 100644 tests/integration/test_ldap_role_mapping/__init__.py delete mode 100644 tests/integration/test_ldap_role_mapping/configs/clickhouse/config.d/ldap_servers.xml delete mode 100644 tests/integration/test_ldap_role_mapping/configs/clickhouse/config.d/logger.xml delete mode 100644 tests/integration/test_ldap_role_mapping/configs/clickhouse/config.d/remote_servers.xml delete mode 100644 tests/integration/test_ldap_role_mapping/configs/ldap/export.ldif delete mode 100644 tests/integration/test_ldap_role_mapping/test.py diff --git a/tests/integration/test_ldap_external_user_directory/configs/ldap_with_role_mapping.xml b/tests/integration/test_ldap_external_user_directory/configs/ldap_with_role_mapping.xml index 28b8f7d0030..a5783a87a10 100644 --- a/tests/integration/test_ldap_external_user_directory/configs/ldap_with_role_mapping.xml +++ b/tests/integration/test_ldap_external_user_directory/configs/ldap_with_role_mapping.xml @@ -5,6 +5,7 @@ 1389 cn={user_name},ou=users,dc=example,dc=org no + never diff --git a/tests/integration/test_ldap_external_user_directory/configs/remote_servers.xml b/tests/integration/test_ldap_external_user_directory/configs/remote_servers.xml new file mode 100644 index 00000000000..cf1bdf9dcb1 --- /dev/null +++ b/tests/integration/test_ldap_external_user_directory/configs/remote_servers.xml @@ -0,0 +1,18 @@ + + + + + + instance1 + 9000 + + + + + instance2 + 9000 + + + + + \ No newline at end of file diff --git a/tests/integration/test_ldap_external_user_directory/test.py b/tests/integration/test_ldap_external_user_directory/test.py index 6c25c0ac789..732b064a0aa 100644 --- a/tests/integration/test_ldap_external_user_directory/test.py +++ b/tests/integration/test_ldap_external_user_directory/test.py @@ -9,8 +9,22 @@ LDAP_ADMIN_BIND_DN = "cn=admin,dc=example,dc=org" LDAP_ADMIN_PASSWORD = "clickhouse" cluster = ClickHouseCluster(__file__) -instance = cluster.add_instance( - "instance", main_configs=["configs/ldap_with_role_mapping.xml"], with_ldap=True + +instance1 = cluster.add_instance( + "instance1", + main_configs=["configs/ldap_with_role_mapping.xml", "configs/remote_servers.xml"], + macros={"shard": 1, "replica": "instance1"}, + stay_alive=True, + with_ldap=True, + with_zookeeper=True, +) + +instance2 = cluster.add_instance( + "instance2", + main_configs=["configs/remote_servers.xml"], + macros={"shard": 1, "replica": "instance2"}, + stay_alive=True, + with_zookeeper=True, ) @@ -74,59 +88,91 @@ def delete_ldap_group(ldap_cluster, group_cn): def test_authentication_pass(): - assert instance.query( + assert instance1.query( "SELECT currentUser()", user="janedoe", password="qwerty" ) == TSV([["janedoe"]]) def test_authentication_fail(): # User doesn't exist. - assert "doesnotexist: Authentication failed" in instance.query_and_get_error( + assert "doesnotexist: Authentication failed" in instance1.query_and_get_error( "SELECT currentUser()", user="doesnotexist" ) # Wrong password. - assert "janedoe: Authentication failed" in instance.query_and_get_error( + assert "janedoe: Authentication failed" in instance1.query_and_get_error( "SELECT currentUser()", user="janedoe", password="123" ) def test_role_mapping(ldap_cluster): - instance.query("DROP ROLE IF EXISTS role_1") - instance.query("DROP ROLE IF EXISTS role_2") - instance.query("DROP ROLE IF EXISTS role_3") - instance.query("CREATE ROLE role_1") - instance.query("CREATE ROLE role_2") + instance1.query("DROP ROLE IF EXISTS role_1") + instance1.query("DROP ROLE IF EXISTS role_2") + instance1.query("DROP ROLE IF EXISTS role_3") + instance1.query("CREATE ROLE role_1") + instance1.query("CREATE ROLE role_2") add_ldap_group(ldap_cluster, group_cn="clickhouse-role_1", member_cn="johndoe") add_ldap_group(ldap_cluster, group_cn="clickhouse-role_2", member_cn="johndoe") - assert instance.query( + assert instance1.query( "select currentUser()", user="johndoe", password="qwertz" ) == TSV([["johndoe"]]) - assert instance.query( + assert instance1.query( "select role_name from system.current_roles ORDER BY role_name", user="johndoe", password="qwertz", ) == TSV([["role_1"], ["role_2"]]) - instance.query("CREATE ROLE role_3") + instance1.query("CREATE ROLE role_3") add_ldap_group(ldap_cluster, group_cn="clickhouse-role_3", member_cn="johndoe") # Check that non-existing role in ClickHouse is ignored during role update # See https://github.com/ClickHouse/ClickHouse/issues/54318 add_ldap_group(ldap_cluster, group_cn="clickhouse-role_4", member_cn="johndoe") - assert instance.query( + assert instance1.query( "select role_name from system.current_roles ORDER BY role_name", user="johndoe", password="qwertz", ) == TSV([["role_1"], ["role_2"], ["role_3"]]) - instance.query("DROP ROLE role_1") - instance.query("DROP ROLE role_2") - instance.query("DROP ROLE role_3") + instance1.query("DROP ROLE role_1") + instance1.query("DROP ROLE role_2") + instance1.query("DROP ROLE role_3") delete_ldap_group(ldap_cluster, group_cn="clickhouse-role_1") delete_ldap_group(ldap_cluster, group_cn="clickhouse-role_2") delete_ldap_group(ldap_cluster, group_cn="clickhouse-role_3") delete_ldap_group(ldap_cluster, group_cn="clickhouse-role_4") + + +def test_push_role_to_other_nodes(ldap_cluster): + instance1.query("DROP TABLE IF EXISTS distributed_table SYNC") + instance1.query("DROP TABLE IF EXISTS local_table SYNC") + instance2.query("DROP TABLE IF EXISTS local_table SYNC") + instance1.query("DROP ROLE IF EXISTS role_read") + + instance1.query("CREATE ROLE role_read") + instance1.query("GRANT SELECT ON *.* TO role_read") + + add_ldap_group(ldap_cluster, group_cn="clickhouse-role_read", member_cn="johndoe") + + assert instance1.query( + "select currentUser()", user="johndoe", password="qwertz" + ) == TSV([["johndoe"]]) + + instance1.query("CREATE TABLE IF NOT EXISTS local_table (id UInt32) ENGINE = MergeTree() ORDER BY id") + instance2.query("CREATE TABLE IF NOT EXISTS local_table (id UInt32) ENGINE = MergeTree() ORDER BY id") + + instance2.query("INSERT INTO local_table VALUES (1), (2), (3)") + + instance1.query("CREATE TABLE IF NOT EXISTS distributed_table AS local_table ENGINE = Distributed(test_ldap_cluster, default, local_table)") + + result = instance1.query("SELECT sum(id) FROM distributed_table", user="johndoe", password="qwertz") + assert result.strip() == '6' + + instance1.query("DROP TABLE IF EXISTS distributed_table SYNC") + instance1.query("DROP TABLE IF EXISTS local_table SYNC") + instance2.query("DROP TABLE IF EXISTS local_table SYNC") + + instance2.query("DROP ROLE IF EXISTS role_read") diff --git a/tests/integration/test_ldap_role_mapping/__init__.py b/tests/integration/test_ldap_role_mapping/__init__.py deleted file mode 100644 index e69de29bb2d..00000000000 diff --git a/tests/integration/test_ldap_role_mapping/configs/clickhouse/config.d/ldap_servers.xml b/tests/integration/test_ldap_role_mapping/configs/clickhouse/config.d/ldap_servers.xml deleted file mode 100644 index abdcdc86b35..00000000000 --- a/tests/integration/test_ldap_role_mapping/configs/clickhouse/config.d/ldap_servers.xml +++ /dev/null @@ -1,11 +0,0 @@ - - - - openldap - 389 - cn={user_name},ou=users,dc=company,dc=com - no - never - - - diff --git a/tests/integration/test_ldap_role_mapping/configs/clickhouse/config.d/logger.xml b/tests/integration/test_ldap_role_mapping/configs/clickhouse/config.d/logger.xml deleted file mode 100644 index a12ffca5a2e..00000000000 --- a/tests/integration/test_ldap_role_mapping/configs/clickhouse/config.d/logger.xml +++ /dev/null @@ -1,15 +0,0 @@ - - - debug - /var/log/clickhouse-server/log.log - /var/log/clickhouse-server/log.err.log - 1000M - 10 - /var/log/clickhouse-server/stderr.log - /var/log/clickhouse-server/stdout.log - - /var/log/clickhouse-server/clickhouse-library-bridge.log - /var/log/clickhouse-server/clickhouse-library-bridge.err.log - trace - - diff --git a/tests/integration/test_ldap_role_mapping/configs/clickhouse/config.d/remote_servers.xml b/tests/integration/test_ldap_role_mapping/configs/clickhouse/config.d/remote_servers.xml deleted file mode 100644 index f1e4d95dd53..00000000000 --- a/tests/integration/test_ldap_role_mapping/configs/clickhouse/config.d/remote_servers.xml +++ /dev/null @@ -1,46 +0,0 @@ - - - - - - instance1 - 9000 - - - - - instance2 - 9000 - - - - - instance3 - 9000 - - - - - - qwerty123 - - - instance1 - 9000 - - - - - instance2 - 9000 - - - - - instance3 - 9000 - - - - - \ No newline at end of file diff --git a/tests/integration/test_ldap_role_mapping/configs/ldap/export.ldif b/tests/integration/test_ldap_role_mapping/configs/ldap/export.ldif deleted file mode 100644 index 82bb1bf89ab..00000000000 --- a/tests/integration/test_ldap_role_mapping/configs/ldap/export.ldif +++ /dev/null @@ -1,64 +0,0 @@ -# LDIF Export for dc=company,dc=com -# Server: openldap (openldap) -# Search Scope: sub -# Search Filter: (objectClass=*) -# Total Entries: 7 -# -# Generated by phpLDAPadmin (http://phpldapadmin.sourceforge.net) on May 22, 2020 5:51 pm -# Version: 1.2.5 - -# Entry 1: dc=company,dc=com -#dn: dc=company,dc=com -#dc: company -#o: company -#objectclass: top -#objectclass: dcObject -#objectclass: organization - -# Entry 2: cn=admin,dc=company,dc=com -#dn: cn=admin,dc=company,dc=com -#cn: admin -#description: LDAP administrator -#objectclass: simpleSecurityObject -#objectclass: organizationalRole -#userpassword: {SSHA}eUEupkQCTvq9SkrxfWGSe5rX+orrjVbF - -# Entry 3: ou=groups,dc=company,dc=com -dn: ou=groups,dc=company,dc=com -objectclass: organizationalUnit -objectclass: top -ou: groups - -# Entry 4: cn=admin,ou=groups,dc=company,dc=com -dn: cn=admin,ou=groups,dc=company,dc=com -cn: admin -gidnumber: 500 -objectclass: posixGroup -objectclass: top - -# Entry 5: cn=users,ou=groups,dc=company,dc=com -dn: cn=users,ou=groups,dc=company,dc=com -cn: users -gidnumber: 501 -objectclass: posixGroup -objectclass: top - -# Entry 6: ou=users,dc=company,dc=com -dn: ou=users,dc=company,dc=com -objectclass: organizationalUnit -objectclass: top -ou: users - -# Entry 7: cn=myuser,ou=users,dc=company,dc=com -# dn: cn=myuser,ou=users,dc=company,dc=com -# cn: myuser -# gidnumber: 501 -# givenname: John -# homedirectory: /home/users/myuser -# objectclass: inetOrgPerson -# objectclass: posixAccount -# objectclass: top -# sn: User -# uid: myuser -# uidnumber: 1101 -# userpassword: myuser diff --git a/tests/integration/test_ldap_role_mapping/test.py b/tests/integration/test_ldap_role_mapping/test.py deleted file mode 100644 index 607223faff3..00000000000 --- a/tests/integration/test_ldap_role_mapping/test.py +++ /dev/null @@ -1,411 +0,0 @@ -import time -from os import getuid - -import pytest - -from helpers.cluster import ClickHouseCluster - -cluster = ClickHouseCluster(__file__) - - -cluster.add_instance( - "instance1", - main_configs=[ - "configs/clickhouse/config.d/logger.xml", - "configs/clickhouse/config.d/ldap_servers.xml", - "configs/clickhouse/config.d/remote_servers.xml", - ], - macros={"shard": 1, "replica": "instance1"}, - stay_alive=True, - with_ldap=True, - with_zookeeper=True, -) -cluster.add_instance( - "instance2", - main_configs=[ - "configs/clickhouse/config.d/logger.xml", - "configs/clickhouse/config.d/ldap_servers.xml", - "configs/clickhouse/config.d/remote_servers.xml", - ], - macros={"shard": 1, "replica": "instance2"}, - stay_alive=True, - with_ldap=True, - with_zookeeper=True, -) -cluster.add_instance( - "instance3", - main_configs=[ - "configs/clickhouse/config.d/logger.xml", - "configs/clickhouse/config.d/ldap_servers.xml", - "configs/clickhouse/config.d/remote_servers.xml", - ], - macros={"shard": 1, "replica": "instance3"}, - stay_alive=True, - with_ldap=True, - with_zookeeper=True, -) - -instances = [ - cluster.instances["instance1"], - cluster.instances["instance2"], - cluster.instances["instance3"], -] - - -ldap_server = { - "host": "openldap", - "port": "389", - "enable_tls": "no", - "bind_dn": "cn={user_name},ou=users,dc=company,dc=com", -} - -# Fixtures - - -@pytest.fixture(scope="module") -def started_cluster(): - try: - cluster.start() - yield cluster - - finally: - cluster.shutdown() - - -# Helpers - - -def create_table(node, on_cluster, name=None): - if name is None: - name = f"tbl_{getuid()}" - - node.query(f"DROP TABLE IF EXISTS {name} ON CLUSTER {on_cluster} SYNC") - - node.query( - f"CREATE TABLE {name} ON CLUSTER {on_cluster} (d Date, a String, b UInt8, x String, y Int8) " - f"ENGINE = ReplicatedMergeTree('/clickhouse/tables/{{shard}}/{name}', '{{replica}}') " - "PARTITION BY y ORDER BY (d, b)" - ) - - return name - - -def create_distributed_table(node, on_cluster, over, name=None): - if name is None: - name = f"dis_tbl_{getuid()}" - - node.query(f"DROP TABLE IF EXISTS {name} ON CLUSTER {on_cluster} SYNC") - - node.query( - f"CREATE TABLE {name} ON CLUSTER {on_cluster} AS {over} " - f"ENGINE = Distributed({on_cluster}, default, {over}, rand())" - ) - - return name - - -def drop_table(node, name, on_cluster): - node.query(f"DROP TABLE IF EXISTS {name} ON CLUSTER {on_cluster} SYNC") - - -def grant_select(cluster, privilege, role_or_user, node): - """Grant select privilege on a table on a given cluster - to a role or a user. - """ - node.query(f"GRANT ON CLUSTER {cluster} {privilege} TO {role_or_user}") - - -def revoke_select(cluster, privilege, role_or_user, node): - node.query(f"REVOKE ON CLUSTER {cluster} {privilege} FROM {role_or_user}") - - -def fix_ldap_permissions_command(): - ldif = ( - "dn: olcDatabase={1}mdb,cn=config\n" - "changetype: modify\n" - "delete: olcAccess\n" - "-\n" - "add: olcAccess\n" - 'olcAccess: to attrs=userPassword,shadowLastChange by self write by dn=\\"cn=admin,dc=company,dc=com\\" write by anonymous auth by * none\n' - 'olcAccess: to * by self write by dn=\\"cn=admin,dc=company,dc=com\\" read by users read by * none' - ) - - return f'echo -e "{ldif}" | ldapmodify -Y EXTERNAL -Q -H ldapi:///' - - -def add_user_to_ldap_command( - cn, - userpassword, - givenname=None, - homedirectory=None, - sn=None, - uid=None, - uidnumber=None, -): - if uid is None: - uid = cn - if givenname is None: - givenname = "John" - if homedirectory is None: - homedirectory = f"/home/{cn}" - if sn is None: - sn = "User" - if uidnumber is None: - uidnumber = 2000 - - user = { - "dn": f"cn={cn},ou=users,dc=company,dc=com", - "cn": cn, - "gidnumber": 501, - "givenname": givenname, - "homedirectory": homedirectory, - "objectclass": ["inetOrgPerson", "posixAccount", "top"], - "sn": sn, - "uid": uid, - "uidnumber": uidnumber, - "userpassword": userpassword, - "_server": cluster.ldap_host, - } - - lines = [] - - for key, value in list(user.items()): - if key.startswith("_"): - continue - elif key == "objectclass": - for cls in value: - lines.append(f"objectclass: {cls}") - else: - lines.append(f"{key}: {value}") - - ldif = "\n".join(lines) - - return f'echo -e "{ldif}" | ldapadd -x -H ldap://localhost -D "cn=admin,dc=company,dc=com" -w admin' - - -def add_rbac_user(user, node): - username = user.get("username", None) or user["cn"] - password = user.get("password", None) or user["userpassword"] - node.query( - f"CREATE USER OR REPLACE {username} IDENTIFIED WITH PLAINTEXT_PASSWORD BY '{password}'" - ) - - -def add_rbac_role(role, node): - node.query(f"DROP ROLE IF EXISTS {role}") - node.query(f"CREATE ROLE OR REPLACE {role}") - - -def outline_test_select_using_mapped_role(cluster, role_name, role_mapped, user): - # default cluster node - node = instances[0] - - query_settings = {"user": user["username"], "password": user["password"]} - - # create base table on cluster - src_table = create_table(node=node, on_cluster=cluster) - - # create distristibuted table over base table on cluster - dist_table = create_distributed_table(on_cluster=cluster, over=src_table, node=node) - - # check that grants for the user - for instance in instances: - for _ in range(10): - r = instance.query(f"SHOW GRANTS", settings=query_settings) - if role_mapped: - assert role_name in r - else: - time.sleep(1) - - # no privilege on source table - for instance in instances: - assert "Not enough privileges" in instance.query_and_get_error( - f"SELECT * FROM {src_table}", settings=query_settings - ) - - # with privilege on source table - grant_select( - cluster=cluster, - privilege=f"SELECT ON {src_table}", - role_or_user=role_name, - node=node, - ) - - # user should be able to read from the source table - for instance in instances: - if role_mapped: - instance.query(f"SELECT * FROM {src_table}", settings=query_settings) - else: - instance.query_and_get_error( - f"SELECT * FROM {src_table}", settings=query_settings - ) == 241 - - revoke_select( - cluster=cluster, - privilege=f"SELECT ON {src_table}", - role_or_user=role_name, - node=node, - ) - - # privilege only on distributed table - grant_select( - cluster=cluster, - privilege=f"SELECT ON {dist_table}", - role_or_user=role_name, - node=node, - ) - - # user should still not be able to read from distributed table - for instance in instances: - instance.query_and_get_error( - f"SELECT * FROM {dist_table}", settings=query_settings - ) == 241 - - revoke_select( - cluster=cluster, - privilege=f"SELECT ON {dist_table}", - role_or_user=role_name, - node=node, - ) - - # privilege only on source but not on distributed table - grant_select( - cluster=cluster, - privilege=f"SELECT ON {src_table}", - role_or_user=role_name, - node=node, - ) - - # user should still not be able to read from distributed table - for instance in instances: - instance.query_and_get_error( - f"SELECT * FROM {dist_table}", settings=query_settings - ) == 241 - - revoke_select( - cluster=cluster, - privilege=f"SELECT ON {src_table}", - role_or_user=role_name, - node=node, - ) - - # privilege on source and distributed - grant_select( - cluster=cluster, - privilege=f"SELECT ON {src_table}", - role_or_user=role_name, - node=node, - ) - - grant_select( - cluster=cluster, - privilege=f"SELECT ON {dist_table}", - role_or_user=role_name, - node=node, - ) - - # user should be able to read from the distributed table - for instance in instances: - if role_mapped: - instance.query(f"SELECT * FROM {dist_table}", settings=query_settings) - else: - instance.query_and_get_error( - f"SELECT * FROM {dist_table}", settings=query_settings - ) == 241 - - revoke_select( - cluster=cluster, - privilege=f"SELECT ON {src_table}", - role_or_user=role_name, - node=node, - ) - - revoke_select( - cluster=cluster, - privilege=f"SELECT ON {dist_table}", - role_or_user=role_name, - node=node, - ) - - -def execute_tests(role_name, role_mapped, ldap_user, local_user): - for cluster_type in ["with_secret", "without_secret"]: - for user in [ldap_user, local_user]: - if role_mapped and user["type"] == "local user": - for instance in instances: - instance.query(f"GRANT {role_name} TO {local_user['username']}") - - outline_test_select_using_mapped_role( - cluster=f"sharded_cluster_{cluster_type}", - role_name=role_name, - role_mapped=role_mapped, - user=user, - ) - - -def test_using_authenticated_users(started_cluster): - role_name = f"role_{getuid()}" - - ldap_user = { - "type": "ldap authenticated user", - "cn": "myuser", - "username": "myuser", - "userpassword": "myuser", - "password": "myuser", - "server": "openldap", - "uidnumber": 1101, - } - - local_user = { - "type": "local user", - "username": "local_user2", - "password": "local_user2", - } - - # fix_ldap_permissions - cluster.exec_in_container( - cluster.ldap_id, ["bash", "-c", fix_ldap_permissions_command()] - ) - - # add LDAP user - cluster.exec_in_container( - cluster.ldap_id, - [ - "bash", - "-c", - add_user_to_ldap_command( - cn=ldap_user["cn"], - userpassword=ldap_user["userpassword"], - uidnumber=ldap_user["uidnumber"], - ), - ], - ) - - # cluster.exec_in_container(cluster.ldap_id, ["ldapsearch -x -LLL uid=*"]) - # add local RBAC user - for instance in instances: - add_rbac_user(user=local_user, node=instance) - - # add RBAC role on cluster that user will use - for instance in instances: - add_rbac_role(role_name, instance) - - # create LDAP-auth user and grant role - for instance in instances: - instance.query( - f"CREATE USER OR REPLACE {ldap_user['username']} IDENTIFIED WITH LDAP SERVER '{ldap_user['server']}'" - ) - - for instance in instances: - instance.query(f"GRANT {role_name} TO {ldap_user['username']}") - - # grant role to local RBAC user - for instance in instances: - instance.query(f"GRANT {role_name} TO {local_user['username']}") - - execute_tests( - role_name=role_name, - role_mapped=role_name, - ldap_user=ldap_user, - local_user=local_user, - ) From 94e2a9cc43a516753bf4ebe46f5a845dfc16ca34 Mon Sep 17 00:00:00 2001 From: Andrey Zvonov Date: Sat, 9 Nov 2024 22:49:12 +0000 Subject: [PATCH 33/49] fix style --- .../configs/ldap_with_role_mapping.xml | 1 - tests/integration/test_ldap_external_user_directory/test.py | 3 --- 2 files changed, 4 deletions(-) diff --git a/tests/integration/test_ldap_external_user_directory/configs/ldap_with_role_mapping.xml b/tests/integration/test_ldap_external_user_directory/configs/ldap_with_role_mapping.xml index a5783a87a10..28b8f7d0030 100644 --- a/tests/integration/test_ldap_external_user_directory/configs/ldap_with_role_mapping.xml +++ b/tests/integration/test_ldap_external_user_directory/configs/ldap_with_role_mapping.xml @@ -5,7 +5,6 @@ 1389 cn={user_name},ou=users,dc=example,dc=org no - never diff --git a/tests/integration/test_ldap_external_user_directory/test.py b/tests/integration/test_ldap_external_user_directory/test.py index 732b064a0aa..e5d38baeaa3 100644 --- a/tests/integration/test_ldap_external_user_directory/test.py +++ b/tests/integration/test_ldap_external_user_directory/test.py @@ -163,9 +163,7 @@ def test_push_role_to_other_nodes(ldap_cluster): instance1.query("CREATE TABLE IF NOT EXISTS local_table (id UInt32) ENGINE = MergeTree() ORDER BY id") instance2.query("CREATE TABLE IF NOT EXISTS local_table (id UInt32) ENGINE = MergeTree() ORDER BY id") - instance2.query("INSERT INTO local_table VALUES (1), (2), (3)") - instance1.query("CREATE TABLE IF NOT EXISTS distributed_table AS local_table ENGINE = Distributed(test_ldap_cluster, default, local_table)") result = instance1.query("SELECT sum(id) FROM distributed_table", user="johndoe", password="qwertz") @@ -174,5 +172,4 @@ def test_push_role_to_other_nodes(ldap_cluster): instance1.query("DROP TABLE IF EXISTS distributed_table SYNC") instance1.query("DROP TABLE IF EXISTS local_table SYNC") instance2.query("DROP TABLE IF EXISTS local_table SYNC") - instance2.query("DROP ROLE IF EXISTS role_read") From c4fc7a1bac02659da2a22dd76ea79492f1a3acdd Mon Sep 17 00:00:00 2001 From: Andrey Zvonov Date: Sat, 9 Nov 2024 23:03:55 +0000 Subject: [PATCH 34/49] fix style (2) --- .../test_ldap_external_user_directory/test.py | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/tests/integration/test_ldap_external_user_directory/test.py b/tests/integration/test_ldap_external_user_directory/test.py index e5d38baeaa3..eaaf8a33de3 100644 --- a/tests/integration/test_ldap_external_user_directory/test.py +++ b/tests/integration/test_ldap_external_user_directory/test.py @@ -161,13 +161,21 @@ def test_push_role_to_other_nodes(ldap_cluster): "select currentUser()", user="johndoe", password="qwertz" ) == TSV([["johndoe"]]) - instance1.query("CREATE TABLE IF NOT EXISTS local_table (id UInt32) ENGINE = MergeTree() ORDER BY id") - instance2.query("CREATE TABLE IF NOT EXISTS local_table (id UInt32) ENGINE = MergeTree() ORDER BY id") + instance1.query( + "CREATE TABLE IF NOT EXISTS local_table (id UInt32) ENGINE = MergeTree() ORDER BY id" + ) + instance2.query( + "CREATE TABLE IF NOT EXISTS local_table (id UInt32) ENGINE = MergeTree() ORDER BY id" + ) instance2.query("INSERT INTO local_table VALUES (1), (2), (3)") - instance1.query("CREATE TABLE IF NOT EXISTS distributed_table AS local_table ENGINE = Distributed(test_ldap_cluster, default, local_table)") + instance1.query( + "CREATE TABLE IF NOT EXISTS distributed_table AS local_table ENGINE = Distributed(test_ldap_cluster, default, local_table)" + ) - result = instance1.query("SELECT sum(id) FROM distributed_table", user="johndoe", password="qwertz") - assert result.strip() == '6' + result = instance1.query( + "SELECT sum(id) FROM distributed_table", user="johndoe", password="qwertz" + ) + assert result.strip() == "6" instance1.query("DROP TABLE IF EXISTS distributed_table SYNC") instance1.query("DROP TABLE IF EXISTS local_table SYNC") From 6e58cfc5b8ddb92a7cc25c0f3c6982fec66b695b Mon Sep 17 00:00:00 2001 From: Andrey Zvonov Date: Sun, 10 Nov 2024 11:08:17 +0000 Subject: [PATCH 35/49] fix build, fix test --- src/Access/LDAPAccessStorage.cpp | 2 +- tests/integration/test_ldap_external_user_directory/test.py | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Access/LDAPAccessStorage.cpp b/src/Access/LDAPAccessStorage.cpp index c1bf8b60495..97b7b1ac52e 100644 --- a/src/Access/LDAPAccessStorage.cpp +++ b/src/Access/LDAPAccessStorage.cpp @@ -320,7 +320,7 @@ std::set LDAPAccessStorage::mapExternalRolesNoLock(const LDAPClient::Sea std::set role_names; // If this node can't access LDAP server (or has not privileges to fetch roles) and gets empty list of external roles - if (external_roles.size() == 0) + if (external_roles.empty()) return role_names; if (external_roles.size() != role_search_params.size()) diff --git a/tests/integration/test_ldap_external_user_directory/test.py b/tests/integration/test_ldap_external_user_directory/test.py index eaaf8a33de3..ce16d7ad286 100644 --- a/tests/integration/test_ldap_external_user_directory/test.py +++ b/tests/integration/test_ldap_external_user_directory/test.py @@ -181,3 +181,5 @@ def test_push_role_to_other_nodes(ldap_cluster): instance1.query("DROP TABLE IF EXISTS local_table SYNC") instance2.query("DROP TABLE IF EXISTS local_table SYNC") instance2.query("DROP ROLE IF EXISTS role_read") + + delete_ldap_group(ldap_cluster, group_cn="clickhouse-role_read") From e785bb908ec0a1f70fe653cf20b05c30fad1788c Mon Sep 17 00:00:00 2001 From: Andrey Zvonov Date: Mon, 18 Nov 2024 23:17:10 +0000 Subject: [PATCH 36/49] fix after review --- src/Client/Connection.cpp | 6 ++++-- src/Interpreters/Context.cpp | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/Client/Connection.cpp b/src/Client/Connection.cpp index 5587d793ec2..ace3c2fe9af 100644 --- a/src/Client/Connection.cpp +++ b/src/Client/Connection.cpp @@ -833,7 +833,7 @@ void Connection::sendQuery( writeVectorBinary(external_roles, buffer); buffer.finalize(); - LOG_DEBUG(log_wrapper.get(), "Sending external_roles with query: [{}] ({})", fmt::join(external_roles, ", "), external_roles.size()); + LOG_TRACE(log_wrapper.get(), "Sending external_roles with query: [{}] ({})", fmt::join(external_roles, ", "), external_roles.size()); writeStringBinary(external_roles_str, *out); } @@ -858,7 +858,9 @@ void Connection::sendQuery( data += query; data += query_id; data += client_info->initial_user; - data += external_roles_str; + // Also for backwards compatibility + if (server_revision >= DBMS_MIN_PROTOCOL_VERSION_WITH_INTERSERVER_EXTERNALLY_GRANTED_ROLES) + data += external_roles_str; /// TODO: add source/target host/ip-address std::string hash = encodeSHA256(data); diff --git a/src/Interpreters/Context.cpp b/src/Interpreters/Context.cpp index 953fcfb253c..ce788842e3e 100644 --- a/src/Interpreters/Context.cpp +++ b/src/Interpreters/Context.cpp @@ -1569,8 +1569,8 @@ void Context::setExternalRolesWithLock(const std::vector & new_external_ro current_roles->insert(current_roles->end(), new_external_roles.begin(), new_external_roles.end()); else current_roles = std::make_shared>(new_external_roles); + need_recalculate_access = true; } - need_recalculate_access = true; } void Context::setCurrentRolesImpl(const std::vector & new_current_roles, bool throw_if_not_granted, bool skip_if_not_granted, const std::shared_ptr & user) From 6e1de8b8a4a14ae709bd1c8cf4290f5d24cb469e Mon Sep 17 00:00:00 2001 From: Kseniia Sumarokova <54203879+kssenii@users.noreply.github.com> Date: Tue, 19 Nov 2024 11:23:46 +0100 Subject: [PATCH 37/49] Update test.py --- tests/integration/test_disk_over_web_server/test.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/integration/test_disk_over_web_server/test.py b/tests/integration/test_disk_over_web_server/test.py index 1aa1ff08e0d..427bc98a28f 100644 --- a/tests/integration/test_disk_over_web_server/test.py +++ b/tests/integration/test_disk_over_web_server/test.py @@ -112,11 +112,12 @@ def test_usage(cluster, node_name): for i in range(3): node2.query( """ + DROP TABLE IF EXISTS test{}; CREATE TABLE test{} UUID '{}' (id Int32) ENGINE = MergeTree() ORDER BY id SETTINGS storage_policy = 'web'; """.format( - i, uuids[i] + i, i, uuids[i] ) ) From 042f0acdb92d5c27093dd102e0ee6762c1ab3468 Mon Sep 17 00:00:00 2001 From: "Mikhail f. Shiryaev" Date: Tue, 19 Nov 2024 13:28:39 +0100 Subject: [PATCH 38/49] Check the keeper docker works for the simplest case --- docker/keeper/Dockerfile | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docker/keeper/Dockerfile b/docker/keeper/Dockerfile index 118b688f8e8..c3a52de877a 100644 --- a/docker/keeper/Dockerfile +++ b/docker/keeper/Dockerfile @@ -86,7 +86,8 @@ RUN arch=${TARGETARCH:-amd64} \ ARG DEFAULT_CONFIG_DIR="/etc/clickhouse-keeper" ARG DEFAULT_DATA_DIR="/var/lib/clickhouse-keeper" ARG DEFAULT_LOG_DIR="/var/log/clickhouse-keeper" -RUN mkdir -p "${DEFAULT_DATA_DIR}" "${DEFAULT_LOG_DIR}" "${DEFAULT_CONFIG_DIR}" \ +RUN clickhouse-keeper --version \ + && mkdir -p "${DEFAULT_DATA_DIR}" "${DEFAULT_LOG_DIR}" "${DEFAULT_CONFIG_DIR}" \ && chown clickhouse:clickhouse "${DEFAULT_DATA_DIR}" \ && chown root:clickhouse "${DEFAULT_LOG_DIR}" \ && chmod ugo+Xrw -R "${DEFAULT_DATA_DIR}" "${DEFAULT_LOG_DIR}" "${DEFAULT_CONFIG_DIR}" From a1fb0ad7067ffdbcf983fe44898c0731d4bfcc2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Tue, 19 Nov 2024 13:11:11 +0000 Subject: [PATCH 39/49] Make sure parens are always matching --- src/Parsers/ASTAlterQuery.cpp | 9 ++++++--- .../integration/test_unambiguous_alter_commands/test.py | 7 +++++++ 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/Parsers/ASTAlterQuery.cpp b/src/Parsers/ASTAlterQuery.cpp index 58eeb7c4cbf..93b5c231428 100644 --- a/src/Parsers/ASTAlterQuery.cpp +++ b/src/Parsers/ASTAlterQuery.cpp @@ -70,8 +70,14 @@ ASTPtr ASTAlterCommand::clone() const void ASTAlterCommand::formatImpl(const FormatSettings & settings, FormatState & state, FormatStateStacked frame) const { + scope_guard closing_bracket_guard; if (format_alter_commands_with_parentheses) + { settings.ostr << "("; + closing_bracket_guard = make_scope_guard([&settings](){ + settings.ostr << ")"; + }); + } if (type == ASTAlterCommand::ADD_COLUMN) { @@ -498,9 +504,6 @@ void ASTAlterCommand::formatImpl(const FormatSettings & settings, FormatState & } else throw Exception(ErrorCodes::UNEXPECTED_AST_STRUCTURE, "Unexpected type of ALTER"); - - if (format_alter_commands_with_parentheses) - settings.ostr << ")"; } void ASTAlterCommand::forEachPointerToChild(std::function f) diff --git a/tests/integration/test_unambiguous_alter_commands/test.py b/tests/integration/test_unambiguous_alter_commands/test.py index 5bf9e32c589..27a9c5b75ad 100644 --- a/tests/integration/test_unambiguous_alter_commands/test.py +++ b/tests/integration/test_unambiguous_alter_commands/test.py @@ -43,3 +43,10 @@ ALTER TABLE a\\n (DROP COLUMN b),\\n (DROP COLUMN c) """ result = node.query(INPUT) assert result == EXPECTED_OUTPUT + + +def test_move_partition_to_table_command(): + INPUT = "SELECT formatQuery('ALTER TABLE a MOVE PARTITION tuple() TO TABLE b')" + EXPECTED_OUTPUT = "ALTER TABLE a\\n (MOVE PARTITION tuple() TO TABLE b)" + result = node.query(INPUT) + assert result == EXPECTED_OUTPUT From c6f901adaacc751143c9eceff1ba081a17fe6b40 Mon Sep 17 00:00:00 2001 From: "Mikhail f. Shiryaev" Date: Tue, 19 Nov 2024 14:13:46 +0100 Subject: [PATCH 40/49] Add docker/keeper to the digests --- tests/ci/ci_definitions.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/ci/ci_definitions.py b/tests/ci/ci_definitions.py index fb3e55fdbe3..c7b2d737b8a 100644 --- a/tests/ci/ci_definitions.py +++ b/tests/ci/ci_definitions.py @@ -570,6 +570,7 @@ class CommonJobConfigs: "tests/ci/docker_server.py", "tests/ci/docker_images_helper.py", "./docker/server", + "./docker/keeper", ] ), runner_type=Runners.STYLE_CHECKER, From 144fd4082b4351315ced997b8855462a309261d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Tue, 19 Nov 2024 13:29:28 +0000 Subject: [PATCH 41/49] Fix compile error --- src/Parsers/ASTAlterQuery.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Parsers/ASTAlterQuery.cpp b/src/Parsers/ASTAlterQuery.cpp index 93b5c231428..7347ddacba0 100644 --- a/src/Parsers/ASTAlterQuery.cpp +++ b/src/Parsers/ASTAlterQuery.cpp @@ -74,9 +74,9 @@ void ASTAlterCommand::formatImpl(const FormatSettings & settings, FormatState & if (format_alter_commands_with_parentheses) { settings.ostr << "("; - closing_bracket_guard = make_scope_guard([&settings](){ + closing_bracket_guard = make_scope_guard(std::function([&settings](){ settings.ostr << ")"; - }); + })); } if (type == ASTAlterCommand::ADD_COLUMN) From 1243f2fb54fe78e5369d89e3e2f93713cf7e102d Mon Sep 17 00:00:00 2001 From: "Mikhail f. Shiryaev" Date: Tue, 19 Nov 2024 14:33:37 +0100 Subject: [PATCH 42/49] Add forgotten patch for the `ln` command --- docker/keeper/Dockerfile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docker/keeper/Dockerfile b/docker/keeper/Dockerfile index c3a52de877a..0d75c0bd225 100644 --- a/docker/keeper/Dockerfile +++ b/docker/keeper/Dockerfile @@ -31,8 +31,8 @@ COPY entrypoint.sh /entrypoint.sh ARG TARGETARCH RUN arch=${TARGETARCH:-amd64} \ && case $arch in \ - amd64) mkdir -p /lib64 && ln -sf /lib/ld-2.31.so /lib64/ld-linux-x86-64.so.2 ;; \ - arm64) ln -sf /lib/ld-2.31.so /lib/ld-linux-aarch64.so.1 ;; \ + amd64) mkdir -p /lib64 && ln -sf /lib/ld-2.35.so /lib64/ld-linux-x86-64.so.2 ;; \ + arm64) ln -sf /lib/ld-2.35.so /lib/ld-linux-aarch64.so.1 ;; \ esac # lts / testing / prestable / etc From 7f493c81d01eb577dbf6fa532032b8aca8dd4565 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Tue, 19 Nov 2024 14:10:55 +0000 Subject: [PATCH 43/49] Fix test --- tests/integration/test_unambiguous_alter_commands/test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/test_unambiguous_alter_commands/test.py b/tests/integration/test_unambiguous_alter_commands/test.py index 27a9c5b75ad..a0449047de9 100644 --- a/tests/integration/test_unambiguous_alter_commands/test.py +++ b/tests/integration/test_unambiguous_alter_commands/test.py @@ -47,6 +47,6 @@ ALTER TABLE a\\n (DROP COLUMN b),\\n (DROP COLUMN c) def test_move_partition_to_table_command(): INPUT = "SELECT formatQuery('ALTER TABLE a MOVE PARTITION tuple() TO TABLE b')" - EXPECTED_OUTPUT = "ALTER TABLE a\\n (MOVE PARTITION tuple() TO TABLE b)" + EXPECTED_OUTPUT = "ALTER TABLE a\\n (MOVE PARTITION tuple() TO TABLE b)\n" result = node.query(INPUT) assert result == EXPECTED_OUTPUT From 7eac602350522e7f755aeee0801f96fe74141ff8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Tue, 19 Nov 2024 14:57:08 +0000 Subject: [PATCH 44/49] Try to fix style --- src/Parsers/ASTAlterQuery.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/Parsers/ASTAlterQuery.cpp b/src/Parsers/ASTAlterQuery.cpp index 7347ddacba0..8bcb6fdc2fc 100644 --- a/src/Parsers/ASTAlterQuery.cpp +++ b/src/Parsers/ASTAlterQuery.cpp @@ -74,9 +74,7 @@ void ASTAlterCommand::formatImpl(const FormatSettings & settings, FormatState & if (format_alter_commands_with_parentheses) { settings.ostr << "("; - closing_bracket_guard = make_scope_guard(std::function([&settings](){ - settings.ostr << ")"; - })); + closing_bracket_guard = make_scope_guard(std::function([&settings]() { settings.ostr << ")"; })); } if (type == ASTAlterCommand::ADD_COLUMN) From 529721923f52d6d9c86b3008fe4168b38d2495e3 Mon Sep 17 00:00:00 2001 From: Max Kainov Date: Tue, 19 Nov 2024 17:14:55 +0100 Subject: [PATCH 45/49] CI: Enable fuzzer job in Nightly workflow --- .github/workflows/nightly.yml | 34 ++++++++++++++++++++++++++++++++-- tests/ci/ci_config.py | 6 ++++++ tests/ci/ci_definitions.py | 1 + 3 files changed, 39 insertions(+), 2 deletions(-) diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml index 1cea94e7500..739958b6f90 100644 --- a/.github/workflows/nightly.yml +++ b/.github/workflows/nightly.yml @@ -27,7 +27,7 @@ jobs: id: runconfig run: | echo "::group::configure CI run" - python3 "$GITHUB_WORKSPACE/tests/ci/ci.py" --configure --skip-jobs --outfile ${{ runner.temp }}/ci_run_data.json + python3 "$GITHUB_WORKSPACE/tests/ci/ci.py" --configure --workflow NightlyBuilds --outfile ${{ runner.temp }}/ci_run_data.json echo "::endgroup::" echo "::group::CI run configure results" @@ -44,9 +44,39 @@ jobs: with: data: "${{ needs.RunConfig.outputs.data }}" set_latest: true + + Builds_1: + needs: [RunConfig] + if: ${{ !failure() && !cancelled() && contains(fromJson(needs.RunConfig.outputs.data).stages_data.stages_to_do, 'Builds_1') }} + uses: ./.github/workflows/reusable_build_stage.yml + with: + stage: Builds_1 + data: ${{ needs.RunConfig.outputs.data }} + Tests_1: + needs: [RunConfig, Builds_1] + if: ${{ !failure() && !cancelled() && contains(fromJson(needs.RunConfig.outputs.data).stages_data.stages_to_do, 'Tests_1') }} + uses: ./.github/workflows/reusable_test_stage.yml + with: + stage: Tests_1 + data: ${{ needs.RunConfig.outputs.data }} + Builds_2: + needs: [RunConfig, Builds_1] + if: ${{ !failure() && !cancelled() && contains(fromJson(needs.RunConfig.outputs.data).stages_data.stages_to_do, 'Builds_2') }} + uses: ./.github/workflows/reusable_build_stage.yml + with: + stage: Builds_2 + data: ${{ needs.RunConfig.outputs.data }} + Tests_2: + needs: [RunConfig, Builds_1, Tests_1] + if: ${{ !failure() && !cancelled() && contains(fromJson(needs.RunConfig.outputs.data).stages_data.stages_to_do, 'Tests_2') }} + uses: ./.github/workflows/reusable_test_stage.yml + with: + stage: Tests_2 + data: ${{ needs.RunConfig.outputs.data }} + CheckWorkflow: if: ${{ !cancelled() }} - needs: [RunConfig, BuildDockers] + needs: [RunConfig, BuildDockers, Tests_2] runs-on: [self-hosted, style-checker-aarch64] steps: - name: Check out repository code diff --git a/tests/ci/ci_config.py b/tests/ci/ci_config.py index 67cdbbdcf6d..9135d50337b 100644 --- a/tests/ci/ci_config.py +++ b/tests/ci/ci_config.py @@ -46,6 +46,12 @@ class CI: JobNames.JEPSEN_KEEPER, JobNames.JEPSEN_SERVER, ] + ), + WorkFlowNames.NIGHTLY: LabelConfig( + run_jobs=[ + BuildNames.FUZZERS, + JobNames.LIBFUZZER_TEST, + ] ) } # type: Dict[str, LabelConfig] diff --git a/tests/ci/ci_definitions.py b/tests/ci/ci_definitions.py index fb3e55fdbe3..aefed29dceb 100644 --- a/tests/ci/ci_definitions.py +++ b/tests/ci/ci_definitions.py @@ -92,6 +92,7 @@ class WorkFlowNames(metaclass=WithIter): JEPSEN = "JepsenWorkflow" CreateRelease = "CreateRelease" + NIGHTLY = "NightlyBuilds" class BuildNames(metaclass=WithIter): From 5bdb1dc8aabd3cd52183799c0797998aa793582b Mon Sep 17 00:00:00 2001 From: robot-clickhouse Date: Tue, 19 Nov 2024 16:36:42 +0000 Subject: [PATCH 46/49] Automatic style fix --- tests/ci/ci_config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ci/ci_config.py b/tests/ci/ci_config.py index 9135d50337b..4bd240e241c 100644 --- a/tests/ci/ci_config.py +++ b/tests/ci/ci_config.py @@ -52,7 +52,7 @@ class CI: BuildNames.FUZZERS, JobNames.LIBFUZZER_TEST, ] - ) + ), } # type: Dict[str, LabelConfig] TAG_CONFIGS = { From d35e230ef6a48d381d3aa328def07fc01ac692c9 Mon Sep 17 00:00:00 2001 From: Pablo Marcos Date: Tue, 19 Nov 2024 16:47:46 +0000 Subject: [PATCH 47/49] Add jwt-cpp submodule --- .gitmodules | 3 +++ contrib/CMakeLists.txt | 4 +++- contrib/jwt-cpp | 1 + contrib/jwt-cpp-cmake/CMakeLists.txt | 18 ++++++++++++++++++ 4 files changed, 25 insertions(+), 1 deletion(-) create mode 160000 contrib/jwt-cpp create mode 100644 contrib/jwt-cpp-cmake/CMakeLists.txt diff --git a/.gitmodules b/.gitmodules index a3b6450032a..03e4a541eec 100644 --- a/.gitmodules +++ b/.gitmodules @@ -1,6 +1,9 @@ # Please do not use 'branch = ...' tags with submodule entries. Such tags make updating submodules a # little bit more convenient but they do *not* specify the tracked submodule branch. Thus, they are # more confusing than useful. +[submodule "contrib/jwt-cpp"] + path = contrib/jwt-cpp + url = https://github.com/Thalhammer/jwt-cpp [submodule "contrib/zstd"] path = contrib/zstd url = https://github.com/facebook/zstd diff --git a/contrib/CMakeLists.txt b/contrib/CMakeLists.txt index fa0f95245f2..f9c57dc7fa1 100644 --- a/contrib/CMakeLists.txt +++ b/contrib/CMakeLists.txt @@ -217,7 +217,9 @@ add_contrib (libssh-cmake libssh) add_contrib (prometheus-protobufs-cmake prometheus-protobufs prometheus-protobufs-gogo) -add_contrib(numactl-cmake numactl) +add_contrib (numactl-cmake numactl) + +add_contrib (jwt-cpp-cmake jwt-cpp) # Put all targets defined here and in subdirectories under "contrib/" folders in GUI-based IDEs. # Some of third-party projects may override CMAKE_FOLDER or FOLDER property of their targets, so they would not appear diff --git a/contrib/jwt-cpp b/contrib/jwt-cpp new file mode 160000 index 00000000000..a6927cb8140 --- /dev/null +++ b/contrib/jwt-cpp @@ -0,0 +1 @@ +Subproject commit a6927cb8140858c34e05d1a954626b9849fbcdfc diff --git a/contrib/jwt-cpp-cmake/CMakeLists.txt b/contrib/jwt-cpp-cmake/CMakeLists.txt new file mode 100644 index 00000000000..05400f1a954 --- /dev/null +++ b/contrib/jwt-cpp-cmake/CMakeLists.txt @@ -0,0 +1,18 @@ +option (ENABLE_JWT_CPP "Enable jwt-cpp library" ${ENABLE_LIBRARIES}) + +if (NOT ENABLE_JWT_CPP) + message(STATUS "Not using jwt-cpp") + return() +endif() + +if(ENABLE_JWT_CPP) + if(NOT TARGET OpenSSL::Crypto) + message (${RECONFIGURE_MESSAGE_LEVEL} "Can't use jwt-cpp without OpenSSL") + endif() +endif() + +set (JWT_CPP_INCLUDE_DIR "${ClickHouse_SOURCE_DIR}/contrib/jwt-cpp/include") + +add_library (_jwt-cpp INTERFACE) +target_include_directories(_jwt-cpp SYSTEM BEFORE INTERFACE ${JWT_CPP_INCLUDE_DIR}) +add_library(ch_contrib::jwt-cpp ALIAS _jwt-cpp) From 4563e796887121bf6ef6d9f64a3e973cb67becac Mon Sep 17 00:00:00 2001 From: Pablo Marcos Date: Tue, 19 Nov 2024 17:18:41 +0000 Subject: [PATCH 48/49] Enable jwt-cpp only for ClickHouse Cloud --- contrib/jwt-cpp-cmake/CMakeLists.txt | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/contrib/jwt-cpp-cmake/CMakeLists.txt b/contrib/jwt-cpp-cmake/CMakeLists.txt index 05400f1a954..4cb8716bc68 100644 --- a/contrib/jwt-cpp-cmake/CMakeLists.txt +++ b/contrib/jwt-cpp-cmake/CMakeLists.txt @@ -1,4 +1,9 @@ -option (ENABLE_JWT_CPP "Enable jwt-cpp library" ${ENABLE_LIBRARIES}) +set(ENABLE_JWT_CPP_DEFAULT OFF) +if(ENABLE_LIBRARIES AND CLICKHOUSE_CLOUD) + set(ENABLE_JWT_CPP_DEFAULT ON) +endif() + +option(ENABLE_JWT_CPP "Enable jwt-cpp library" ${ENABLE_JWT_CPP_DEFAULT}) if (NOT ENABLE_JWT_CPP) message(STATUS "Not using jwt-cpp") From 6446c11a7b4f1dc9ec74097aca4c0a5358b63d2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Tue, 19 Nov 2024 21:56:16 +0100 Subject: [PATCH 49/49] Fix list-licenses.sh with OSX --- utils/list-licenses/list-licenses.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/utils/list-licenses/list-licenses.sh b/utils/list-licenses/list-licenses.sh index c06d61a9c43..101b7c9b18a 100755 --- a/utils/list-licenses/list-licenses.sh +++ b/utils/list-licenses/list-licenses.sh @@ -13,8 +13,8 @@ fi ROOT_PATH="$(git rev-parse --show-toplevel)" LIBS_PATH="${ROOT_PATH}/contrib" -mapfile -t libs < <(echo "${ROOT_PATH}/base/poco"; find "${LIBS_PATH}" -maxdepth 1 -type d -not -name '*-cmake' -not -name 'rust_vendor' | LC_ALL=C sort) -for LIB in "${libs[@]}" +libs=$(echo "${ROOT_PATH}/base/poco"; (find "${LIBS_PATH}" -maxdepth 1 -type d -not -name '*-cmake' -not -name 'rust_vendor' | LC_ALL=C sort) ) +for LIB in ${libs} do LIB_NAME=$(basename "$LIB") @@ -97,4 +97,4 @@ do done # Special care for Rust -find "${LIBS_PATH}/rust_vendor/" -name 'Cargo.toml' | xargs grep 'license = ' | (grep -v -P 'MIT|Apache|MPL' && echo "Fatal error: unrecognized licenses in the Rust code" >&2 && exit 1 || true) +find "${LIBS_PATH}/rust_vendor/" -name 'Cargo.toml' | xargs ${GREP_CMD} 'license = ' | (${GREP_CMD} -v -P 'MIT|Apache|MPL' && echo "Fatal error: unrecognized licenses in the Rust code" >&2 && exit 1 || true)