From 2bbeba1845bf109e0ec6e0d32e182eea6b3a76a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Jaramago=20Fern=C3=A1ndez?= Date: Fri, 25 Nov 2022 17:03:20 +0100 Subject: [PATCH 1/7] Fix forwarding of 'SERVER_SESSION_STATE_CHANGED' status to clients #4023 Right now server session state isn't tracked for backend connections, so we don't include this information in the generated OK packet. Because of this, we should never report of this status change to clients. --- lib/MySQL_Protocol.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/MySQL_Protocol.cpp b/lib/MySQL_Protocol.cpp index 27caba7ca..436aba5c6 100644 --- a/lib/MySQL_Protocol.cpp +++ b/lib/MySQL_Protocol.cpp @@ -533,6 +533,8 @@ bool MySQL_Protocol::generate_pkt_OK(bool send, void **ptr, unsigned int *len, u internal_status |= SERVER_STATUS_NO_BACKSLASH_ESCAPES; } } + // Always remove 'SERVER_SESSION_STATE_CHANGED', since we don't track this information right now + internal_status &= ~SERVER_SESSION_STATE_CHANGED; memcpy(_ptr+l, &internal_status, sizeof(uint16_t)); l+=sizeof(uint16_t); memcpy(_ptr+l, &warnings, sizeof(uint16_t)); l+=sizeof(uint16_t); if (msg && strlen(msg)) { From a5e8ab045cbe12ade6f2984b113d83ada76530c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Jaramago=20Fern=C3=A1ndez?= Date: Fri, 25 Nov 2022 17:13:01 +0100 Subject: [PATCH 2/7] Refactor 'memcpy' into actual capability flags used during 'initial_handshake' #4023 --- lib/MySQL_Protocol.cpp | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/lib/MySQL_Protocol.cpp b/lib/MySQL_Protocol.cpp index 436aba5c6..b9550801d 100644 --- a/lib/MySQL_Protocol.cpp +++ b/lib/MySQL_Protocol.cpp @@ -1251,17 +1251,24 @@ bool MySQL_Protocol::generate_pkt_initial_handshake(bool send, void **ptr, unsig uint8_t uint8_charset = ci->nr & 255; memcpy(_ptr+l,&uint8_charset, sizeof(uint8_charset)); l+=sizeof(uint8_charset); memcpy(_ptr+l,&server_status, sizeof(server_status)); l+=sizeof(server_status); + uint32_t extended_capabilities = CLIENT_MULTI_RESULTS | CLIENT_MULTI_STATEMENTS | CLIENT_PS_MULTI_RESULTS | + CLIENT_PLUGIN_AUTH | CLIENT_SESSION_TRACKING | CLIENT_REMEMBER_OPTIONS; // we conditionally reply the client specifying in 'server_capabilities' that // 'CLIENT_DEPRECATE_EOF' is available if explicitly enabled by 'mysql-enable_client_deprecate_eof' // variable. This is the first step of ensuring that client connections doesn't // enable 'CLIENT_DEPRECATE_EOF' unless explicitly stated by 'mysql-enable_client_deprecate_eof'. // Second step occurs during client handshake response (process_pkt_handshake_response). if (deprecate_eof_active && mysql_thread___enable_client_deprecate_eof) { - memcpy(_ptr+l,"\x8f\x81\x15",3); l+=3; - } - else { - memcpy(_ptr+l,"\x8f\x80\x15",3); l+=3; + extended_capabilities |= CLIENT_DEPRECATE_EOF; } + // Copy the 'capability_flags_2' + uint16_t upper_word = static_cast(extended_capabilities >> 16); + memcpy(_ptr+l, static_cast(&upper_word), sizeof(upper_word)); l += sizeof(upper_word); + // Copy the 'auth_plugin_data_len'. Hardcoded due to 'CLIENT_PLUGIN_AUTH' always enabled and reported + // as 'mysql_native_password'. + uint8_t auth_plugin_data_len = 21; + memcpy(_ptr+l, &auth_plugin_data_len, sizeof(auth_plugin_data_len)); l += sizeof(auth_plugin_data_len); + for (i=0;i<10; i++) { _ptr[l]=0x00; l++; } //filler //create_random_string(mypkt->data+l,12,(struct my_rnd_struct *)&rand_st); l+=12; //#ifdef MARIADB_BASE_VERSION From dcfbb0de62198af6e6ab83831551fd46180d4077 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Jaramago=20Fern=C3=A1ndez?= Date: Fri, 25 Nov 2022 17:15:56 +0100 Subject: [PATCH 3/7] Add test checking capabilities reported by ProxySQL in 'Greeting message' #4023 --- .../tests/test_greeting_capabilities-t.cpp | 134 ++++++++++++++++++ 1 file changed, 134 insertions(+) create mode 100644 test/tap/tests/test_greeting_capabilities-t.cpp diff --git a/test/tap/tests/test_greeting_capabilities-t.cpp b/test/tap/tests/test_greeting_capabilities-t.cpp new file mode 100644 index 000000000..51fe72714 --- /dev/null +++ b/test/tap/tests/test_greeting_capabilities-t.cpp @@ -0,0 +1,134 @@ +/** + * @file test_greeting_capabilities-t.cpp + * @brief Checks that ProxySQL sends the correct capabilities during handshake. + * @details Thist test should also check conditional capabilities enabled by config variables. E.g: + * 'CLIENT_DEPRECATE_EOF' when enabled through 'mysql-enable_client_deprecate_eof'. + */ + +#include +#include +#include +#include +#include + +#include "mysql.h" + +#include "tap.h" +#include "command_line.h" +#include "utils.h" + +using std::pair; +using std::string; +using std::vector; + +// By default the following capabilities should be present +std::vector def_capabilities { + CLIENT_MULTI_RESULTS, + CLIENT_MULTI_STATEMENTS, + CLIENT_PS_MULTI_RESULTS, + CLIENT_PLUGIN_AUTH, + CLIENT_SESSION_TRACKING, + CLIENT_REMEMBER_OPTIONS +}; + +pair check_server_capabilities( + MYSQL* proxy, const vector& exp_conn_caps, bool present +) { + bool caps_match = true; + uint64_t exp_caps = 0; + + for (const uint64_t cap : def_capabilities) { + caps_match = proxy->server_capabilities & cap; + exp_caps |= cap; + + if (caps_match == false) { + diag("Missing expected DEFAULT capability: %ld", cap); + } + } + + for (const uint64_t exp_cap : exp_conn_caps) { + if (present) { + caps_match = proxy->server_capabilities & exp_cap; + exp_caps |= exp_cap; + } else { + caps_match = !(proxy->server_capabilities & exp_cap); + exp_caps &= ~exp_cap; + } + + if (caps_match == false) { + diag("Missing expected CONDITIONAL capability: %ld", exp_cap); + } + } + + return { caps_match, exp_caps }; +} + +int test_proxy_capabilites(const CommandLine& cl, MYSQL* admin) { + MYSQL_QUERY(admin, "SET mysql-enable_client_deprecate_eof=0"); + MYSQL_QUERY(admin, "LOAD MYSQL VARIABLES TO RUNTIME"); + + MYSQL* proxy = mysql_init(NULL); + + if (!mysql_real_connect(proxy, cl.host, cl.username, cl.password, NULL, cl.port, NULL, 0)) { + fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxy)); + return EXIT_FAILURE; + } + + pair caps_res { check_server_capabilities(proxy, { CLIENT_DEPRECATE_EOF }, false) }; + uint64_t ext_caps = (proxy->server_capabilities >> 16) << 16; + + mysql_close(proxy); + + ok( + caps_res.first, "ProxySQL greeting should return the expected capabilities - Exp: '%ld', Act: '%ld'", + caps_res.second, ext_caps + ); + + MYSQL_QUERY(admin, "SET mysql-enable_client_deprecate_eof=1"); + MYSQL_QUERY(admin, "LOAD MYSQL VARIABLES TO RUNTIME"); + + proxy = mysql_init(NULL); + proxy->options.client_flag |= CLIENT_DEPRECATE_EOF; + + if (!mysql_real_connect(proxy, cl.host, cl.username, cl.password, NULL, cl.port, NULL, 0)) { + fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxy)); + return EXIT_FAILURE; + } + + caps_res = check_server_capabilities(proxy, { CLIENT_DEPRECATE_EOF }, true); + ext_caps = (proxy->server_capabilities >> 16) << 16; + + ok( + caps_res.first, "ProxySQL greeting should return the expected capabilities - Exp: '%ld', Act: '%ld'", + caps_res.second, ext_caps + ); + + mysql_close(proxy); + + return EXIT_SUCCESS; +} + +int main(int argc, char** argv) { + CommandLine cl; + + // TODO: Harcoded for now, this is an initial version of the test. + plan(2); + + if (cl.getEnv()) { + diag("Failed to get the required environmental variables."); + return EXIT_FAILURE; + } + + MYSQL* admin = mysql_init(NULL); + + if (!mysql_real_connect(admin, cl.host, cl.admin_username, cl.admin_password, NULL, cl.admin_port, NULL, 0)) { + fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(admin)); + return EXIT_FAILURE; + } + + test_proxy_capabilites(cl, admin); + + mysql_close(admin); + + return exit_status(); +} From dde861797cff226a7491be7eaf5aaf6d5e6290b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Jaramago=20Fern=C3=A1ndez?= Date: Fri, 25 Nov 2022 17:18:06 +0100 Subject: [PATCH 4/7] Add test for checking 'server_status' reported by ProxySQL #4023 --- test/tap/tests/test_server_sess_status-t.cpp | 150 +++++++++++++++++++ 1 file changed, 150 insertions(+) create mode 100644 test/tap/tests/test_server_sess_status-t.cpp diff --git a/test/tap/tests/test_server_sess_status-t.cpp b/test/tap/tests/test_server_sess_status-t.cpp new file mode 100644 index 000000000..187036e80 --- /dev/null +++ b/test/tap/tests/test_server_sess_status-t.cpp @@ -0,0 +1,150 @@ +/** + * @file test_server_sess_status-t.cpp + * @brief Test checking that ProxySQL 'server_status' value is properly updated for different operations. + * @details Test should also check that unsupported status like 'SERVER_SESSION_STATE_CHANGED' are never + * reported by ProxySQL. + */ + +#include +#include +#include +#include +#include + +#include "mysql.h" + +#include "tap.h" +#include "command_line.h" +#include "utils.h" + +using std::pair; +using std::string; + +int get_user_def_hg(MYSQL* admin, const string& user) { + const string sel_q { "SELECT default_hostgroup FROM mysql_users WHERE username='" + user + "'" }; + if (mysql_query(admin, sel_q.c_str())) { + fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(admin)); \ + return -1; + } + + MYSQL_RES* myres = mysql_store_result(admin); + MYSQL_ROW myrow = mysql_fetch_row(myres); + + if (myrow && myrow[0]) { + int def_hg = std::atoi(myrow[0]); + mysql_free_result(myres); + + return def_hg; + } else { + const string err_msg { "Unexpected empty result received for query: " + sel_q }; + fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, err_msg.c_str()); + return -1; + } +} + +pair get_def_srv_host_port(MYSQL* admin, int hg) { + const string sel_q { "SELECT hostname,port FROM mysql_servers WHERE hostgroup_id=" + std::to_string(hg) }; + int myrc = mysql_query(admin, sel_q.c_str()); + + if (myrc) { + fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(admin)); + return { "", -1 }; + } else { + MYSQL_RES* myres = mysql_store_result(admin); + MYSQL_ROW myrow = mysql_fetch_row(myres); + + if (myrow && myrow[0] && myrow[1]) { + string host { myrow[0] }; + int port { std::atoi(myrow[1]) }; + mysql_free_result(myres); + + return { host, port }; + } else { + const string err_msg { "Unexpected empty result received for query: '" + sel_q + "'"}; + fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, err_msg.c_str()); + return { "", -1 }; + } + } +} + +pair get_def_srv_host(MYSQL* admin, const string user) { + // Get the server from the default hostgroup + int def_hg = get_user_def_hg(admin, user); + if (def_hg == -1) { + return { "", -1 }; + } + + return get_def_srv_host_port(admin, def_hg); +} + +int main(int argc, char** argv) { + CommandLine cl; + + // TODO: Harcoded for now, this is an initial version of the test. + plan(4); + + if (cl.getEnv()) { + diag("Failed to get the required environmental variables."); + return EXIT_FAILURE; + } + + MYSQL* proxy = mysql_init(NULL); + MYSQL* mysql = mysql_init(NULL); + MYSQL* admin = mysql_init(NULL); + + if (!mysql_real_connect(proxy, cl.host, cl.username, cl.password, NULL, cl.port, NULL, 0)) { + fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxy)); + return EXIT_FAILURE; + } + + if (!mysql_real_connect(admin, cl.host, cl.admin_username, cl.admin_password, NULL, cl.admin_port, NULL, 0)) { + fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(admin)); + return EXIT_FAILURE; + } + + const pair srv_host { get_def_srv_host(admin, cl.username) }; + if (srv_host.first.empty()) { + diag("Failed to obtain the target server hostname/port. Aborting further testing"); + goto cleanup; + } + + { + if (!mysql_real_connect(mysql, srv_host.first.c_str(), cl.username, cl.password, NULL, srv_host.second, NULL, 0)) { + fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxy)); + goto cleanup; + } + + bool exp_mysql_srv_st = mysql->server_status == 2; + ok(exp_mysql_srv_st, "MySQL init server status should match expected '%d'", mysql->server_status); + + mysql_query(mysql, "SET SESSION session_track_transaction_info=\"CHARACTERISTICS\""); + mysql_query(mysql, "START TRANSACTION"); + + exp_mysql_srv_st = + mysql->server_status & SERVER_STATUS_AUTOCOMMIT && + mysql->server_status & SERVER_STATUS_IN_TRANS && + mysql->server_status & SERVER_SESSION_STATE_CHANGED; + + ok(exp_mysql_srv_st, "MySQL new server status should match expected '%d'", mysql->server_status); + + // TODO-FIXME: Expected to fail due to ProxySQL bug + bool exp_proxy_srv_st = proxy->server_status == 2; + ok(exp_proxy_srv_st, "ProxySQL init server status should match expected '%d'", proxy->server_status); + + mysql_query(proxy, "SET SESSION session_track_transaction_info=\"CHARACTERISTICS\""); + mysql_query(proxy, "START TRANSACTION"); + + exp_proxy_srv_st = + mysql->server_status & SERVER_STATUS_AUTOCOMMIT && + mysql->server_status & SERVER_STATUS_IN_TRANS; + + ok(exp_proxy_srv_st, "ProxySQL new server status should match expected '%d'", proxy->server_status); + } + +cleanup: + mysql_close(proxy); + mysql_close(mysql); + mysql_close(admin); + + return exit_status(); +} From fc5ebf297cdd30b2c62e3bb2a7cf122b9bfedb00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Jaramago=20Fern=C3=A1ndez?= Date: Mon, 12 Dec 2022 17:00:14 +0100 Subject: [PATCH 5/7] Improve 'test_server_sess_status-t' logic - Test now uses the explicit expected flags for 'server_status' checks. - TODO have been left for when 'AUTOCOMMIT' issue for handshake response is fixed. --- test/tap/tests/test_server_sess_status-t.cpp | 40 +++++++++++++------- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/test/tap/tests/test_server_sess_status-t.cpp b/test/tap/tests/test_server_sess_status-t.cpp index 187036e80..b2643604b 100644 --- a/test/tap/tests/test_server_sess_status-t.cpp +++ b/test/tap/tests/test_server_sess_status-t.cpp @@ -114,31 +114,43 @@ int main(int argc, char** argv) { goto cleanup; } - bool exp_mysql_srv_st = mysql->server_status == 2; - ok(exp_mysql_srv_st, "MySQL init server status should match expected '%d'", mysql->server_status); + int exp_mysql_srv_st = SERVER_STATUS_AUTOCOMMIT; + + ok( + exp_mysql_srv_st == SERVER_STATUS_AUTOCOMMIT, + "MySQL init server status should match expected - exp: '%d', act:'%d'", + exp_mysql_srv_st, mysql->server_status + ); mysql_query(mysql, "SET SESSION session_track_transaction_info=\"CHARACTERISTICS\""); mysql_query(mysql, "START TRANSACTION"); - exp_mysql_srv_st = - mysql->server_status & SERVER_STATUS_AUTOCOMMIT && - mysql->server_status & SERVER_STATUS_IN_TRANS && - mysql->server_status & SERVER_SESSION_STATE_CHANGED; + exp_mysql_srv_st = SERVER_STATUS_AUTOCOMMIT | SERVER_STATUS_IN_TRANS | SERVER_SESSION_STATE_CHANGED; - ok(exp_mysql_srv_st, "MySQL new server status should match expected '%d'", mysql->server_status); + ok( + exp_mysql_srv_st == mysql->server_status, + "MySQL new server status should match expected - exp: '%d', act:'%d'", + exp_mysql_srv_st, mysql->server_status + ); - // TODO-FIXME: Expected to fail due to ProxySQL bug - bool exp_proxy_srv_st = proxy->server_status == 2; - ok(exp_proxy_srv_st, "ProxySQL init server status should match expected '%d'", proxy->server_status); + // TODO-FIXME: We are setting here '0' as expecting to see 'SERVER_STATUS_AUTOCOMMIT' to be false. + // This is a bug that should be addressed, and this test revisited. + ok( + proxy->server_status == 0, + "ProxySQL init server status should match expected - exp: '%d', act:'%d'", + 0, proxy->server_status + ); mysql_query(proxy, "SET SESSION session_track_transaction_info=\"CHARACTERISTICS\""); mysql_query(proxy, "START TRANSACTION"); - exp_proxy_srv_st = - mysql->server_status & SERVER_STATUS_AUTOCOMMIT && - mysql->server_status & SERVER_STATUS_IN_TRANS; + uint32_t exp_proxy_srv_st = SERVER_STATUS_AUTOCOMMIT | SERVER_STATUS_IN_TRANS; - ok(exp_proxy_srv_st, "ProxySQL new server status should match expected '%d'", proxy->server_status); + ok( + exp_proxy_srv_st == proxy->server_status, + "ProxySQL new server status should match expected - exp: '%d', act:'%d'", + exp_proxy_srv_st, proxy->server_status + ); } cleanup: From 6bffdc19f30fb637caf1d9b2d266762d01fc83e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20S=C3=A1nchez=20Parra?= Date: Mon, 12 Dec 2022 18:27:19 +0100 Subject: [PATCH 6/7] Fix server status comparison in 'test_server_sess_status-t' --- test/tap/tests/test_server_sess_status-t.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/tap/tests/test_server_sess_status-t.cpp b/test/tap/tests/test_server_sess_status-t.cpp index b2643604b..076481547 100644 --- a/test/tap/tests/test_server_sess_status-t.cpp +++ b/test/tap/tests/test_server_sess_status-t.cpp @@ -117,7 +117,7 @@ int main(int argc, char** argv) { int exp_mysql_srv_st = SERVER_STATUS_AUTOCOMMIT; ok( - exp_mysql_srv_st == SERVER_STATUS_AUTOCOMMIT, + exp_mysql_srv_st == mysql->server_status, "MySQL init server status should match expected - exp: '%d', act:'%d'", exp_mysql_srv_st, mysql->server_status ); From 4e1ba8da319eac6502b9517d04a9728a3df40476 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Jaramago=20Fern=C3=A1ndez?= Date: Tue, 13 Dec 2022 14:34:44 +0100 Subject: [PATCH 7/7] Remove 'SERVER_SESSION_STATE_CHANGED' only when not GTID related #4023 --- lib/MySQL_Protocol.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/MySQL_Protocol.cpp b/lib/MySQL_Protocol.cpp index faab94097..642a2979e 100644 --- a/lib/MySQL_Protocol.cpp +++ b/lib/MySQL_Protocol.cpp @@ -533,8 +533,10 @@ bool MySQL_Protocol::generate_pkt_OK(bool send, void **ptr, unsigned int *len, u internal_status |= SERVER_STATUS_NO_BACKSLASH_ESCAPES; } } - // Always remove 'SERVER_SESSION_STATE_CHANGED', since we don't track this information right now - internal_status &= ~SERVER_SESSION_STATE_CHANGED; + if (gtid_len == 0) { + // Remove 'SERVER_SESSION_STATE_CHANGED', since we don't track this info unless GTID related + internal_status &= ~SERVER_SESSION_STATE_CHANGED; + } memcpy(_ptr+l, &internal_status, sizeof(uint16_t)); l+=sizeof(uint16_t); memcpy(_ptr+l, &warnings, sizeof(uint16_t)); l+=sizeof(uint16_t); if (msg && strlen(msg)) {