Merge pull request #55949 from azat/connections_credentials-fix

Fix overrides via connections_credentials in case of root directives exists

programs/benchmark/Benchmark.cpp

@@ -4,6 +4,7 @@
 #include <iostream>
 #include <fstream>
 #include <iomanip>
+#include <optional>
 #include <random>
 #include <string_view>
 #include <pcg_random.hpp>
@@ -28,10 +29,12 @@
 #include <Interpreters/Context.h>
 #include <Client/Connection.h>
 #include <Common/InterruptListener.h>
-#include <Common/Config/configReadClient.h>
+#include <Common/Config/ConfigProcessor.h>
+#include <Common/Config/getClientConfigPath.h>
 #include <Common/TerminalSize.h>
 #include <Common/StudentTTest.h>
 #include <Common/CurrentMetrics.h>
+#include <Common/ErrorCodes.h>
 #include <filesystem>
 
 
@@ -156,7 +159,17 @@ public:
         if (home_path_cstr)
             home_path = home_path_cstr;
 
-        configReadClient(config(), home_path);
+        std::optional<std::string> config_path;
+        if (config().has("config-file"))
+            config_path.emplace(config().getString("config-file"));
+        else
+            config_path = getClientConfigPath(home_path);
+        if (config_path.has_value())
+        {
+            ConfigProcessor config_processor(*config_path);
+            auto loaded_config = config_processor.loadConfig();
+            config().add(loaded_config.configuration);
+        }
     }
 
     int main(const std::vector<std::string> &) override

programs/client/Client.cpp

@@ -25,7 +25,8 @@
 #include <Common/Exception.h>
 #include <Common/formatReadable.h>
 #include <Common/TerminalSize.h>
-#include <Common/Config/configReadClient.h>
+#include <Common/Config/ConfigProcessor.h>
+#include <Common/Config/getClientConfigPath.h>
 
 #include <Core/QueryProcessingStage.h>
 #include <Columns/ColumnString.h>
@@ -131,69 +132,64 @@ void Client::showWarnings()
     }
 }
 
-void Client::parseConnectionsCredentials()
+void Client::parseConnectionsCredentials(Poco::Util::AbstractConfiguration & config, const std::string & connection_name)
 {
-    /// It is not possible to correctly handle multiple --host --port options.
-    if (hosts_and_ports.size() >= 2)
-        return;
-
-    std::optional<String> host;
+    std::optional<String> default_connection_name;
     if (hosts_and_ports.empty())
     {
-        if (config().has("host"))
-            host = config().getString("host");
+        if (config.has("host"))
+            default_connection_name = config.getString("host");
     }
     else
     {
-        host = hosts_and_ports.front().host;
+        default_connection_name = hosts_and_ports.front().host;
     }
 
     String connection;
-    if (config().has("connection"))
-        connection = config().getString("connection");
+    if (!connection_name.empty())
+        connection = connection_name;
     else
-        connection = host.value_or("localhost");
+        connection = default_connection_name.value_or("localhost");
 
     Strings keys;
-    config().keys("connections_credentials", keys);
+    config.keys("connections_credentials", keys);
     bool connection_found = false;
     for (const auto & key : keys)
     {
         const String & prefix = "connections_credentials." + key;
 
-        const String & connection_name = config().getString(prefix + ".name", "");
-        if (connection_name != connection)
+        const String & name = config.getString(prefix + ".name", "");
+        if (name != connection)
             continue;
         connection_found = true;
 
         String connection_hostname;
-        if (config().has(prefix + ".hostname"))
-            connection_hostname = config().getString(prefix + ".hostname");
+        if (config.has(prefix + ".hostname"))
+            connection_hostname = config.getString(prefix + ".hostname");
         else
-            connection_hostname = connection_name;
+            connection_hostname = name;
 
-        if (hosts_and_ports.empty())
-            config().setString("host", connection_hostname);
-        if (config().has(prefix + ".port") && hosts_and_ports.empty())
-            config().setInt("port", config().getInt(prefix + ".port"));
-        if (config().has(prefix + ".secure") && !config().has("secure"))
-            config().setBool("secure", config().getBool(prefix + ".secure"));
-        if (config().has(prefix + ".user") && !config().has("user"))
-            config().setString("user", config().getString(prefix + ".user"));
-        if (config().has(prefix + ".password") && !config().has("password"))
-            config().setString("password", config().getString(prefix + ".password"));
-        if (config().has(prefix + ".database") && !config().has("database"))
-            config().setString("database", config().getString(prefix + ".database"));
-        if (config().has(prefix + ".history_file") && !config().has("history_file"))
+        config.setString("host", connection_hostname);
+        if (config.has(prefix + ".port"))
+            config.setInt("port", config.getInt(prefix + ".port"));
+        if (config.has(prefix + ".secure"))
+            config.setBool("secure", config.getBool(prefix + ".secure"));
+        if (config.has(prefix + ".user"))
+            config.setString("user", config.getString(prefix + ".user"));
+        if (config.has(prefix + ".password"))
+            config.setString("password", config.getString(prefix + ".password"));
+        if (config.has(prefix + ".database"))
+            config.setString("database", config.getString(prefix + ".database"));
+        if (config.has(prefix + ".history_file"))
         {
-            String history_file = config().getString(prefix + ".history_file");
+            String history_file = config.getString(prefix + ".history_file");
             if (history_file.starts_with("~") && !home_path.empty())
                 history_file = home_path + "/" + history_file.substr(1);
-            config().setString("history_file", history_file);
+            config.setString("history_file", history_file);
         }
     }
 
-    if (config().has("connection") && !connection_found)
+    if (!connection_name.empty() && !connection_found)
         throw Exception(ErrorCodes::NO_ELEMENTS_IN_CONFIG, "No such connection '{}' in connections_credentials", connection);
 }
 
@@ -263,7 +259,20 @@ void Client::initialize(Poco::Util::Application & self)
     if (home_path_cstr)
         home_path = home_path_cstr;
 
-    configReadClient(config(), home_path);
+    std::optional<std::string> config_path;
+    if (config().has("config-file"))
+        config_path.emplace(config().getString("config-file"));
+    else
+        config_path = getClientConfigPath(home_path);
+    if (config_path.has_value())
+    {
+        ConfigProcessor config_processor(*config_path);
+        auto loaded_config = config_processor.loadConfig();
+        parseConnectionsCredentials(*loaded_config.configuration, config().getString("connection", ""));
+        config().add(loaded_config.configuration);
+    }
+    else if (config().has("connection"))
+        throw Exception(ErrorCodes::BAD_ARGUMENTS, "--connection was specified, but config does not exists");
 
     /** getenv is thread-safe in Linux glibc and in all sane libc implementations.
       * But the standard does not guarantee that subsequent calls will not rewrite the value by returned pointer.
@@ -286,8 +295,6 @@ void Client::initialize(Poco::Util::Application & self)
     if (env_password && !config().has("password"))
         config().setString("password", env_password);
 
-    parseConnectionsCredentials();
-
     // global_context->setApplicationType(Context::ApplicationType::CLIENT);
     global_context->setQueryParameters(query_parameters);
 
@@ -1454,7 +1461,9 @@ int mainEntryClickHouseClient(int argc, char ** argv)
     try
     {
         DB::Client client;
+        // Initialize command line options
         client.init(argc, argv);
+        /// Initialize config file
         return client.run();
     }
     catch (const DB::Exception & e)

programs/client/Client.h

@@ -47,7 +47,7 @@ protected:
 private:
     void printChangedSettings() const;
     void showWarnings();
-    void parseConnectionsCredentials();
+    void parseConnectionsCredentials(Poco::Util::AbstractConfiguration & config, const std::string & connection_name);
     std::vector<String> loadWarningMessages();
 };
 }

src/Common/Config/CMakeLists.txt

@@ -1,7 +1,7 @@
 set (SRCS
     AbstractConfigurationComparison.cpp
     ConfigProcessor.cpp
-    configReadClient.cpp
+    getClientConfigPath.cpp
     ConfigReloader.cpp
     YAMLParser.cpp
     ConfigHelper.cpp

src/Common/Config/configReadClient.cpp

@@ -1,61 +0,0 @@
-#include "configReadClient.h"
-
-#include <Poco/Util/LayeredConfiguration.h>
-#include "ConfigProcessor.h"
-#include <filesystem>
-#include <base/types.h>
-
-
-namespace fs = std::filesystem;
-
-namespace DB
-{
-
-bool configReadClient(Poco::Util::LayeredConfiguration & config, const std::string & home_path)
-{
-    std::string config_path;
-
-    bool found = false;
-    if (config.has("config-file"))
-    {
-        found = true;
-        config_path = config.getString("config-file");
-    }
-    else
-    {
-        std::vector<std::string> names;
-        names.emplace_back("./clickhouse-client");
-        if (!home_path.empty())
-            names.emplace_back(home_path + "/.clickhouse-client/config");
-        names.emplace_back("/etc/clickhouse-client/config");
-
-        for (const auto & name : names)
-        {
-            for (const auto & extension : {".xml", ".yaml", ".yml"})
-            {
-                config_path = name + extension;
-
-                std::error_code ec;
-                if (fs::exists(config_path, ec))
-                {
-                    found = true;
-                    break;
-                }
-            }
-            if (found)
-                break;
-        }
-    }
-
-    if (found)
-    {
-        ConfigProcessor config_processor(config_path);
-        auto loaded_config = config_processor.loadConfig();
-        config.add(loaded_config.configuration);
-        return true;
-    }
-
-    return false;
-}
-
-}

src/Common/Config/configReadClient.h

@@ -1,10 +0,0 @@
-#pragma once
-#include <string>
-
-namespace Poco { class Logger; namespace Util { class LayeredConfiguration; } }
-
-namespace DB
-{
-/// Read configuration files related to clickhouse-client like applications. Returns true if any configuration files were read.
-bool configReadClient(Poco::Util::LayeredConfiguration & config, const std::string & home_path);
-}

src/Common/Config/getClientConfigPath.cpp

@@ -0,0 +1,46 @@
+#include <Common/Config/getClientConfigPath.h>
+
+#include <filesystem>
+#include <vector>
+
+
+namespace fs = std::filesystem;
+
+namespace DB
+{
+
+std::optional<std::string> getClientConfigPath(const std::string & home_path)
+{
+    std::string config_path;
+    bool found = false;
+
+    std::vector<std::string> names;
+    names.emplace_back("./clickhouse-client");
+    if (!home_path.empty())
+        names.emplace_back(home_path + "/.clickhouse-client/config");
+    names.emplace_back("/etc/clickhouse-client/config");
+
+    for (const auto & name : names)
+    {
+        for (const auto & extension : {".xml", ".yaml", ".yml"})
+        {
+            config_path = name + extension;
+
+            std::error_code ec;
+            if (fs::exists(config_path, ec))
+            {
+                found = true;
+                break;
+            }
+        }
+        if (found)
+            break;
+    }
+
+    if (found)
+        return config_path;
+
+    return std::nullopt;
+}
+
+}

src/Common/Config/getClientConfigPath.h

@@ -0,0 +1,12 @@
+#pragma once
+
+#include <string>
+#include <optional>
+
+namespace DB
+{
+
+/// Return path to existing configuration file.
+std::optional<std::string> getClientConfigPath(const std::string & home_path);
+
+}

tests/queries/0_stateless/02550_client_connections_credentials.reference

@@ -20,3 +20,8 @@ default: Authentication failed: password is incorrect, or there is no user with
 default
 history_file
 Cannot create file: /no/such/dir/.history
+root overrides
+foo: Authentication failed: password is incorrect, or there is no user with such name.
+default
+default
+foo: Authentication failed: password is incorrect, or there is no user with such name.

tests/queries/0_stateless/02550_client_connections_credentials.sh

@@ -70,6 +70,33 @@ cat > $CONFIG <<EOL
 </clickhouse>
 EOL
 
+CONFIG_ROOT_OVERRIDES=$CLICKHOUSE_TMP/client_user_pass.xml
+cat > $CONFIG_ROOT_OVERRIDES <<EOL
+<clickhouse>
+    <host>$TEST_HOST</host>
+    <port>$TEST_PORT</port>
+    <database>$TEST_DATABASE</database>
+    <user>foo</user>
+    <password>pass</password>
+
+    <connections_credentials>
+        <connection>
+            <name>incorrect_auth</name>
+            <hostname>$TEST_HOST</hostname>
+            <database>system</database>
+        </connection>
+
+        <connection>
+            <name>default</name>
+            <user>default</user>
+            <password></password>
+            <hostname>$TEST_HOST</hostname>
+            <database>system</database>
+        </connection>
+    </connections_credentials>
+</clickhouse>
+EOL
+
 echo 'connection'
 $CLICKHOUSE_CLIENT --config $CONFIG --connection no_such_connection -q 'select 1' |& grep -F -o "No such connection 'no_such_connection' in connections_credentials"
 echo 'hostname'
@@ -99,4 +126,13 @@ $CLICKHOUSE_CLIENT --config $CONFIG --connection test_password --password "" -q
 echo 'history_file'
 $CLICKHOUSE_CLIENT --progress off --interactive --config $CONFIG --connection test_history_file -q 'select 1' </dev/null |& grep -F -o 'Cannot create file: /no/such/dir/.history'
 
+# Just in case
+unset CLICKHOUSE_USER
+unset CLICKHOUSE_PASSWORD
+echo 'root overrides'
+$CLICKHOUSE_CLIENT --config $CONFIG_ROOT_OVERRIDES --connection incorrect_auth -q 'select currentUser()' |& grep -F -o 'foo: Authentication failed: password is incorrect, or there is no user with such name.'
+$CLICKHOUSE_CLIENT --config $CONFIG_ROOT_OVERRIDES --connection incorrect_auth --user "default" --password "" -q 'select currentUser()'
+$CLICKHOUSE_CLIENT --config $CONFIG_ROOT_OVERRIDES --connection default -q 'select currentUser()'
+$CLICKHOUSE_CLIENT --config $CONFIG_ROOT_OVERRIDES --connection default --user foo -q 'select currentUser()' |& grep -F -o 'foo: Authentication failed: password is incorrect, or there is no user with such name.'
+
 rm -f "${CONFIG:?}"