diff --git a/components/percona_telemetry/data_provider.cc b/components/percona_telemetry/data_provider.cc index 5b064554dc2b..88c62c6e01b7 100644 --- a/components/percona_telemetry/data_provider.cc +++ b/components/percona_telemetry/data_provider.cc @@ -23,6 +23,20 @@ namespace { inline const char *b2s(bool val) { return val ? "1" : "0"; } +/* + mysql.session user is mostly enough, but it lacks the following privileges: + + 1. REPLICATION SLAVE + 2. REPLICATION CLIENT + 3. SELECT on mysql.component + 4. SELECT on performance_schema.replication_group_members + + These privileges are added at server startup in setup_percona_telemetry() + if Percona telemetry is enabled. +*/ +constexpr const char default_command_user_name[] = "mysql.session"; +constexpr const char default_command_host_name[] = "localhost"; + namespace JSONKey { const char *pillar_version = "pillar_version"; const char *db_instance_id = "db_instance_id"; @@ -120,25 +134,29 @@ bool DataProvider::do_query(const std::string &query, QueryResult *result, } result->clear(); + /* command_factory_service_.init() allocates memory for mysql_h + We need to call close() always. + Even if init() fails, becaues it doesn't allocate anything, calling close() + is safe, because internally it checks if provided pointer is valid + */ + std::shared_ptr mysql_h_close_guard( + &mysql_h, [&srv = command_factory_service_](MYSQL_H *ptr) { + srv.close(*ptr); + }); + mysql_service_status_t sstatus = command_factory_service_.init(&mysql_h); + if (!sstatus) sstatus |= command_options_service_.set(mysql_h, MYSQL_COMMAND_PROTOCOL, nullptr); if (!sstatus) - sstatus |= - command_options_service_.set(mysql_h, MYSQL_COMMAND_USER_NAME, "root"); + sstatus |= command_options_service_.set(mysql_h, MYSQL_COMMAND_USER_NAME, + default_command_user_name); if (!sstatus) - sstatus |= - command_options_service_.set(mysql_h, MYSQL_COMMAND_HOST_NAME, nullptr); + sstatus |= command_options_service_.set(mysql_h, MYSQL_COMMAND_HOST_NAME, + default_command_host_name); if (!sstatus) sstatus |= command_factory_service_.connect(mysql_h); - // starting from this point, if the above succeeded we need to close mysql_h. - std::shared_ptr mysql_h_close_guard( - mysql_h, - [&srv = command_factory_service_, do_close = !sstatus](void *ptr) { - if (do_close && ptr) srv.close(static_cast(ptr)); - }); - // if any of the above failed, just exit if (sstatus) { goto err; @@ -212,7 +230,7 @@ bool DataProvider::collect_db_instance_id_info(rapidjson::Document *document) { so the SQL query failed. It will recover next time. 2. Some other reason that caused selecting server_id to fail. */ if (id.length() == 0) { - logger_.warning( + logger_.info( "Collecting db_instance_id failed. It may be caused by server still " "initializing."); return true; @@ -346,7 +364,7 @@ bool DataProvider::collect_se_usage_info(rapidjson::Document *document) { QueryResult result; if (do_query("SELECT DISTINCT ENGINE FROM information_schema.tables WHERE " "table_schema NOT IN('mysql', 'information_schema', " - "'performance_schema', 'sys');", + "'performance_schema', 'sys')", &result)) { return true; } diff --git a/mysql-test/suite/component_percona_telemetry/r/no_user.result b/mysql-test/suite/component_percona_telemetry/r/no_user.result new file mode 100644 index 000000000000..80c3f9461413 --- /dev/null +++ b/mysql-test/suite/component_percona_telemetry/r/no_user.result @@ -0,0 +1,31 @@ +SELECT * FROM information_schema.table_privileges WHERE grantee = "'mysql.session'@'localhost'" ORDER BY table_schema, table_name; +GRANTEE TABLE_CATALOG TABLE_SCHEMA TABLE_NAME PRIVILEGE_TYPE IS_GRANTABLE +'mysql.session'@'localhost' def mysql component SELECT NO +'mysql.session'@'localhost' def mysql user SELECT NO +'mysql.session'@'localhost' def performance_schema replication_group_members SELECT NO +SHOW GRANTS FOR 'mysql.session'@'localhost'; +Grants for mysql.session@localhost +GRANT SHUTDOWN, SUPER, REPLICATION SLAVE, REPLICATION CLIENT ON *.* TO `mysql.session`@`localhost` +GRANT AUDIT_ABORT_EXEMPT,AUTHENTICATION_POLICY_ADMIN,BACKUP_ADMIN,CLONE_ADMIN,CONNECTION_ADMIN,FIREWALL_EXEMPT,PERSIST_RO_VARIABLES_ADMIN,SESSION_VARIABLES_ADMIN,SYSTEM_USER,SYSTEM_VARIABLES_ADMIN ON *.* TO `mysql.session`@`localhost` +GRANT SELECT ON `performance_schema`.* TO `mysql.session`@`localhost` +GRANT SELECT ON `mysql`.`component` TO `mysql.session`@`localhost` +GRANT SELECT ON `mysql`.`user` TO `mysql.session`@`localhost` +GRANT SELECT ON `performance_schema`.`replication_group_members` TO `mysql.session`@`localhost` +# restart:--percona_telemetry.grace_interval=30 --percona_telemetry.scrape_interval=30 --percona_telemetry.telemetry_root_dir= +RENAME USER 'root'@'localhost' to 'root.tmp'@'localhost'; +Warnings: +Warning 4005 User 'root'@'localhost' is referenced as a definer account in a stored routine. +Warning 4005 User 'root'@'localhost' is referenced as a definer account in a trigger. +'root' user used by component's 1st verison does not exist. Telemetry dir should contain 1 file. +1 +RENAME USER 'root.tmp'@'localhost' to 'root'@'localhost'; +Warnings: +Warning 4005 User 'root'@'localhost' is referenced as a definer account in a stored routine. +Warning 4005 User 'root'@'localhost' is referenced as a definer account in a trigger. +# restart:--percona_telemetry.grace_interval=30 --percona_telemetry.scrape_interval=30 --percona_telemetry.telemetry_root_dir= +RENAME USER 'mysql.session'@'localhost' to 'mysql.session.tmp'@'localhost'; +include/assert.inc [No orphaned sessions expected in processlist] +'mysql.session' user used by component does not exist. Telemetry dir should still contain 1 file. +1 +RENAME USER 'mysql.session.tmp'@'localhost' to 'mysql.session'@'localhost'; +# restart:--percona_telemetry.grace_interval=30 --percona_telemetry.scrape_interval=30 --percona_telemetry.telemetry_root_dir= diff --git a/mysql-test/suite/component_percona_telemetry/t/no_user.test b/mysql-test/suite/component_percona_telemetry/t/no_user.test new file mode 100644 index 000000000000..2b51f2ccb6a0 --- /dev/null +++ b/mysql-test/suite/component_percona_telemetry/t/no_user.test @@ -0,0 +1,70 @@ +# Test that lack of the user used by Percona Telemetry Component +# doesn't cause hangs during server restart and no orphaned sessions are created. + +--source include/have_percona_telemetry.inc + +--let $telemetry_root_dir = $MYSQL_TMP_DIR/telemetry_dir +--let $grace_interval = 30 +--let $scrape_interval = 30 + +--mkdir $telemetry_root_dir + +# Record mysql.session user privileges +SELECT * FROM information_schema.table_privileges WHERE grantee = "'mysql.session'@'localhost'" ORDER BY table_schema, table_name; +SHOW GRANTS FOR 'mysql.session'@'localhost'; + +# restart the server with custom telemetry file path and timeouts +--let $restart_parameters = "restart:--percona_telemetry.grace_interval=$grace_interval --percona_telemetry.scrape_interval=$scrape_interval --percona_telemetry.telemetry_root_dir=$telemetry_root_dir" +--replace_regex /telemetry_root_dir=.*telemetry_dir/telemetry_root_dir=/ +--source include/restart_mysqld.inc + +# Rename 'root' user (1st version of Percona Telemetry Component used 'root' user) +# 1st version will not collect any data and will not create telemetry file and the restart will hang. +# Fixed version will work properly as it doesn't use 'root' user. +RENAME USER 'root'@'localhost' to 'root.tmp'@'localhost'; + +# sleep more than grace_interval and check that telemetry file was created +--let $timeout = `select $grace_interval + 10` +--sleep $timeout + +--echo 'root' user used by component's 1st verison does not exist. Telemetry dir should contain 1 file. +--exec ls -1 $telemetry_root_dir | wc -l + +# +# It should be possible to restart the server. +# +RENAME USER 'root.tmp'@'localhost' to 'root'@'localhost'; +--let $restart_parameters = "restart:--percona_telemetry.grace_interval=$grace_interval --percona_telemetry.scrape_interval=$scrape_interval --percona_telemetry.telemetry_root_dir=$telemetry_root_dir" +--replace_regex /telemetry_root_dir=.*telemetry_dir/telemetry_root_dir=/ +--source include/restart_mysqld.inc + + +# +# Now rename the user used by component +# +RENAME USER 'mysql.session'@'localhost' to 'mysql.session.tmp'@'localhost'; + +# Wait a few cycles and ensure that SHOW PROCESSLIST does not contain rows related to orphaned sessions. +--let $timeout = `select $grace_interval + 3 * $scrape_interval` +--sleep $timeout + +--let $assert_text = No orphaned sessions expected in processlist +--let $assert_cond = [SELECT COUNT(*) as Result FROM performance_schema.processlist WHERE user = "mysql.session";, Result, 1] = 0 +--source include/assert.inc + +# Check that no new telemetry file was created +--echo 'mysql.session' user used by component does not exist. Telemetry dir should still contain 1 file. +--exec ls -1 $telemetry_root_dir | wc -l + + +# +# It should be still possible to restart the server. +# +RENAME USER 'mysql.session.tmp'@'localhost' to 'mysql.session'@'localhost'; +--let $restart_parameters = "restart:--percona_telemetry.grace_interval=$grace_interval --percona_telemetry.scrape_interval=$scrape_interval --percona_telemetry.telemetry_root_dir=$telemetry_root_dir" +--replace_regex /telemetry_root_dir=.*telemetry_dir/telemetry_root_dir=/ +--source include/restart_mysqld.inc + +# cleanup +--force-rmdir $telemetry_root_dir + diff --git a/sql/dd/impl/upgrade/server.cc b/sql/dd/impl/upgrade/server.cc index 4577aff79a09..d7cf8ab5a03b 100644 --- a/sql/dd/impl/upgrade/server.cc +++ b/sql/dd/impl/upgrade/server.cc @@ -959,6 +959,15 @@ bool upgrade_system_schemas(THD *thd) { /* 1. We INSERT INTO, because prepared statements do not support INSTALL COMPONENT 2. We use stored procedure to be able to do conditional action. + 3. Percona Telemetry Component uses mysql.session user. For the component to + be fully functional, mysql.session user lacks the following privileges: + 1. REPLICATION SLAVE + 2. REPLICATION CLIENT + 3. SELECT on mysql.component + 4. SELECT on performance_schema.replication_group_members + GRANT does not work yet as ACL is not initialized yet. Use UPDATES. + For 3 and 4 we could set Select_priv = 'Y' in mysql.user table for + mysql.session user, but let's allow only minimal required privileges. */ static const char *percona_telemetry_install[] = { "USE mysql;\n", @@ -977,6 +986,15 @@ static const char *percona_telemetry_install[] = { "PREPARE stmt FROM @str;\n", "EXECUTE stmt;\n", "DROP PREPARE stmt;\n", + "UPDATE mysql.user SET Repl_slave_priv = 'Y' WHERE User = 'mysql.session' " + "AND Host = 'localhost';\n", + "UPDATE mysql.user SET Repl_client_priv = 'Y' WHERE User = 'mysql.session' " + "AND Host = 'localhost';\n", + "INSERT IGNORE INTO mysql.tables_priv VALUES ('localhost', 'mysql', " + "'mysql.session', 'component', 'root@localhost', NOW(), 'Select', '');\n", + "INSERT IGNORE INTO mysql.tables_priv VALUES ('localhost', " + "'performance_schema', 'mysql.session', 'replication_group_members', " + "'root@localhost', NOW(), 'Select', '');\n", NULL}; static const char *percona_telemetry_uninstall[] = { diff --git a/sql/server_component/mysql_command_backend.cc b/sql/server_component/mysql_command_backend.cc index b962b1c34210..d33872eb799e 100644 --- a/sql/server_component/mysql_command_backend.cc +++ b/sql/server_component/mysql_command_backend.cc @@ -75,6 +75,21 @@ mysql_state_machine_status cssm_begin_connect(mysql_async_connect *ctx) { my_h_service h_command_consumer_srv = nullptr; MYSQL_SESSION mysql_session = nullptr; + + /* We need to handle the failure in this function. + Setting mcs_extn->session_svc right after session open is not enough + to handle user lookup errors (and following errors as well) + because in case of this function returns error, mysql->extension is + cleaned up immediately by the caller. The caller does not take care of + session_svc, because it is not aware of this structure. + */ + std::shared_ptr mysql_session_close_guard( + &mysql_session, [mcs_extn](MYSQL_SESSION *mysql_session_ptr) { + if (*mysql_session_ptr == nullptr) return; + mcs_extn->session_svc = nullptr; + srv_session_close(*mysql_session_ptr); + }); + if (mcs_extn->mcs_thd == nullptr || mcs_extn->session_svc == nullptr) { /* Avoid possibility of nested txn in the current thd. @@ -239,6 +254,7 @@ mysql_state_machine_status cssm_begin_connect(mysql_async_connect *ctx) { } mysql->client_flag = 0; /* For handshake */ mysql->server_status = SERVER_STATUS_AUTOCOMMIT; + mysql_session = nullptr; // disable delete quard return STATE_MACHINE_DONE; } diff --git a/unittest/gunit/components/percona_telemetry/data_provider-t.cc b/unittest/gunit/components/percona_telemetry/data_provider-t.cc index c6e64987157f..7f41826d8f57 100644 --- a/unittest/gunit/components/percona_telemetry/data_provider-t.cc +++ b/unittest/gunit/components/percona_telemetry/data_provider-t.cc @@ -287,7 +287,7 @@ TEST_F(DataProviderTest, collect_se_usage_info_test) { const std::string query( std::string("SELECT DISTINCT ENGINE FROM information_schema.tables WHERE " "table_schema NOT IN('mysql', 'information_schema', " - "'performance_schema', 'sys');")); + "'performance_schema', 'sys')")); const std::string expected_json_key("se_engines_in_use"); collect_array_info_common(query, expected_json_key, &MockDataProvider::collect_se_usage_info);