From 2ad8b77f110fcd13e00766f82bf83772804f1348 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Sun, 17 Jun 2018 22:17:46 +0300 Subject: [PATCH 1/8] Fix current command tracking in MariaDBClient The debug assertion introduced by commit 3d1c2b421a fails when a COM_CHANGE_USER was executed. This was caused by the fact that the authentication data was being interpreted as a command when it should've been ignored. Added a debug assertion into the reauthentication code to make sure the current command remains the same. --- .../protocol/MySQL/mariadbclient/mysql_client.cc | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc b/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc index 8aaa0d1ea..a7ce5c300 100644 --- a/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc +++ b/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc @@ -1012,9 +1012,17 @@ gw_read_normal_data(DCB *dcb, GWBUF *read_buffer, int nbytes_read) return 0; } - // Update the current command, required by KILL command processing + /** + * Update the current command, required by KILL command processing. + * If a COM_CHANGE_USER is in progress, this must not be done as the client + * is sending authentication data that does not have the command byte. + */ MySQLProtocol *proto = (MySQLProtocol*)dcb->protocol; - proto->current_command = (mxs_mysql_cmd_t)mxs_mysql_get_command(read_buffer); + + if (!proto->changing_user) + { + proto->current_command = (mxs_mysql_cmd_t)mxs_mysql_get_command(read_buffer); + } set_qc_mode(session, &read_buffer); @@ -1696,6 +1704,7 @@ static int route_by_statement(MXS_SESSION* session, uint64_t capabilities, GWBUF } else if (proto->changing_user) { + ss_dassert(proto->current_command == MXS_COM_CHANGE_USER); proto->changing_user = false; bool ok = reauthenticate_client(session, packetbuf); gwbuf_free(packetbuf); From 24bb8b0b2c8aca4f7f7c5d48a392544d87b4eb7d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Sun, 17 Jun 2018 22:07:43 +0300 Subject: [PATCH 2/8] Clean up mxs548_short_session_change_user The test unnecessarily did an unconditional drop of the user resulting in errors. Also prevented stopping of MaxScale if it wasn't started in the first place. --- maxscale-system-test/mxs548_short_session_change_user.cpp | 2 +- maxscale-system-test/testconnections.cpp | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/maxscale-system-test/mxs548_short_session_change_user.cpp b/maxscale-system-test/mxs548_short_session_change_user.cpp index d2b452beb..208575fcc 100644 --- a/maxscale-system-test/mxs548_short_session_change_user.cpp +++ b/maxscale-system-test/mxs548_short_session_change_user.cpp @@ -86,7 +86,7 @@ int main(int argc, char *argv[]) Test->repl->sync_slaves(); Test->tprintf("Creating user 'user' \n"); - execute_query(Test->maxscales->conn_rwsplit[0], (char *) "DROP USER user@'%%'"); + execute_query(Test->maxscales->conn_rwsplit[0], (char *) "DROP USER IF EXISTS user@'%%'"); execute_query(Test->maxscales->conn_rwsplit[0], (char *) "CREATE USER user@'%%' IDENTIFIED BY 'pass2'"); execute_query(Test->maxscales->conn_rwsplit[0], (char *) "GRANT SELECT ON test.* TO user@'%%'"); execute_query(Test->maxscales->conn_rwsplit[0], (char *) "DROP TABLE IF EXISTS test.t1"); diff --git a/maxscale-system-test/testconnections.cpp b/maxscale-system-test/testconnections.cpp index 1c9e75f26..c5c4ea280 100644 --- a/maxscale-system-test/testconnections.cpp +++ b/maxscale-system-test/testconnections.cpp @@ -641,7 +641,10 @@ void TestConnections::init_maxscale(int m) { const char * template_name = get_template_name(test_name); - stop_maxscale(m); + if (maxscale::start) + { + stop_maxscale(m); + } process_template(m, template_name, maxscales->access_homedir[m]); maxscales->ssh_node_f(m, true, From 6df0804e70cef745ec4cf34aeb98a2efd522dc44 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 18 Jun 2018 10:04:15 +0300 Subject: [PATCH 3/8] Add missing tls-passphrase documentation The tls-passphrase was not documented in MaxCtrl. Also regenerated the documentation. --- Documentation/Reference/MaxCtrl.md | 10 ++++++++-- maxctrl/lib/core.js | 4 ++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/Documentation/Reference/MaxCtrl.md b/Documentation/Reference/MaxCtrl.md index 4b602e466..f457d8b5a 100644 --- a/Documentation/Reference/MaxCtrl.md +++ b/Documentation/Reference/MaxCtrl.md @@ -59,8 +59,9 @@ HTTPS/TLS Options: [boolean] [default: true] Options: - --version Show version number [boolean] - --help Show help [boolean] + --version Show version number [boolean] + --tls-passphrase Password for the TLS private key [string] + --help Show help [boolean] ``` ## list @@ -675,6 +676,11 @@ Usage: api Commands: get [path] Get raw JSON +API options: + --sum Calculate sum of API result. Only works for arrays of numbers e.g. `api + get --sum servers data[].attributes.statistics.connections`. + [boolean] [default: false] + ``` ### api get diff --git a/maxctrl/lib/core.js b/maxctrl/lib/core.js index 8d7dd6c40..9c0a5a472 100644 --- a/maxctrl/lib/core.js +++ b/maxctrl/lib/core.js @@ -74,6 +74,10 @@ program describe: 'Path to TLS public certificate', type: 'string' }) + .option('tls-passphrase', { + describe: 'Password for the TLS private key', + type: 'string' + }) .option('tls-ca-cert', { describe: 'Path to TLS CA certificate', type: 'string' From 45f39e7fdd7b4281d221c443b801da01a00af881 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 18 Jun 2018 12:57:54 +0300 Subject: [PATCH 4/8] Wait for monitor in mxs1719 Replaced hard-coded sleep with dynamic waiting. --- maxscale-system-test/mxs1719.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/maxscale-system-test/mxs1719.cpp b/maxscale-system-test/mxs1719.cpp index 9d60ca768..efb634ea3 100644 --- a/maxscale-system-test/mxs1719.cpp +++ b/maxscale-system-test/mxs1719.cpp @@ -75,8 +75,7 @@ int main(int argc, char* argv[]) { if (test.maxscales->start() == 0) { - // Give the monitor a few seconds to monitor the servers - sleep(5); + test.maxscales->wait_for_monitor(); if (test.maxscales->connect_rwsplit() == 0) { From 8eaa265168f5529ad16aa965f381a69f8db87cec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 18 Jun 2018 20:42:23 +0300 Subject: [PATCH 5/8] MXS-1931: Remove use of gw_MySQL_get_next_packet The function implemented redundant functionality and replacement with modutil_get_next_MySQL_packet was planned. When faced with a packet header spread over multiple buffers, the packet length calculation would read past the buffer end. This is fixed by taking modutil_get_next_MySQL_packet into use. Identical behavior to the old function is achieved by calling gwbuf_make_contiguous for each packet to store them in a contiguous area of memory. This should be either removed and only done when RCAP_TYPE_CONTIGUOUS_INPUT is requested or be made an innate feature of statement based routing. --- include/maxscale/protocol/mysql.h | 2 - .../MySQL/mariadbclient/mysql_client.cc | 42 ++++----- server/modules/protocol/MySQL/mysql_common.cc | 94 ------------------- 3 files changed, 18 insertions(+), 120 deletions(-) diff --git a/include/maxscale/protocol/mysql.h b/include/maxscale/protocol/mysql.h index 88f0b0bc7..dd8369b00 100644 --- a/include/maxscale/protocol/mysql.h +++ b/include/maxscale/protocol/mysql.h @@ -464,8 +464,6 @@ int mysql_send_custom_error(DCB *dcb, int sequence, int affected_rows, const cha int mysql_send_standard_error(DCB *dcb, int sequence, int errnum, const char *msg); int mysql_send_auth_error(DCB *dcb, int sequence, int affected_rows, const char* msg); -GWBUF* gw_MySQL_get_next_packet(GWBUF** p_readbuf); -GWBUF* gw_MySQL_get_packets(GWBUF** p_readbuf, int* npackets); void protocol_add_srv_command(MySQLProtocol* p, mxs_mysql_cmd_t cmd); void protocol_remove_srv_command(MySQLProtocol* p); bool protocol_waits_response(MySQLProtocol* p); diff --git a/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc b/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc index a7ce5c300..22db11462 100644 --- a/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc +++ b/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc @@ -1576,6 +1576,17 @@ static bool reauthenticate_client(MXS_SESSION* session, GWBUF* packetbuf) return rval; } +// Helper function for debug assertions +static bool only_one_packet(GWBUF* buffer) +{ + ss_dassert(buffer); + uint8_t header[4] = {}; + gwbuf_copy_data(buffer, 0, MYSQL_HEADER_LEN, header); + size_t packet_len = gw_mysql_get_byte3(header); + size_t buffer_len = gwbuf_length(buffer); + return packet_len + MYSQL_HEADER_LEN == buffer_len; +} + /** * Detect if buffer includes partial mysql packet or multiple packets. * Store partial packet to dcb_readqueue. Send complete packets one by one @@ -1597,15 +1608,15 @@ static int route_by_statement(MXS_SESSION* session, uint64_t capabilities, GWBUF GWBUF* packetbuf; do { - /** - * Collect incoming bytes to a buffer until complete packet has - * arrived and then return the buffer. - */ - // TODO: This should be replaced with modutil_get_next_MySQL_packet. - packetbuf = gw_MySQL_get_next_packet(p_readbuf); + // Process client request one packet at a time + packetbuf = modutil_get_next_MySQL_packet(p_readbuf); + + // TODO: Do this only when RCAP_TYPE_CONTIGUOUS_INPUT is requested + packetbuf = gwbuf_make_contiguous(packetbuf); if (packetbuf != NULL) { + ss_dassert(only_one_packet(packetbuf)); CHK_GWBUF(packetbuf); MySQLProtocol* proto = (MySQLProtocol*)session->client_dcb->protocol; @@ -1619,24 +1630,7 @@ static int route_by_statement(MXS_SESSION* session, uint64_t capabilities, GWBUF if (rcap_type_required(capabilities, RCAP_TYPE_CONTIGUOUS_INPUT)) { - if (!GWBUF_IS_CONTIGUOUS(packetbuf)) - { - // TODO: As long as gw_MySQL_get_next_packet is used above, the buffer - // TODO: will be contiguous. That function should be replaced with - // TODO: modutil_get_next_MySQL_packet. - GWBUF* tmp = gwbuf_make_contiguous(packetbuf); - if (tmp) - { - packetbuf = tmp; - } - else - { - // TODO: A memory allocation failure. We should close the dcb - // TODO: and terminate the session. - rc = 0; - goto return_rc; - } - } + ss_dassert(GWBUF_IS_CONTIGUOUS(packetbuf)); if (rcap_type_required(capabilities, RCAP_TYPE_TRANSACTION_TRACKING)) { diff --git a/server/modules/protocol/MySQL/mysql_common.cc b/server/modules/protocol/MySQL/mysql_common.cc index 45609763c..d1b396504 100644 --- a/server/modules/protocol/MySQL/mysql_common.cc +++ b/server/modules/protocol/MySQL/mysql_common.cc @@ -473,100 +473,6 @@ int mysql_send_auth_error(DCB *dcb, return sizeof(mysql_packet_header) + mysql_payload_size; } - -/** - * Buffer contains at least one of the following: - * complete [complete] [partial] mysql packet - * - * @param p_readbuf Address of read buffer pointer - * - * @return pointer to gwbuf containing a complete packet or - * NULL if no complete packet was found. - */ -GWBUF* gw_MySQL_get_next_packet(GWBUF** p_readbuf) -{ - GWBUF* packetbuf; - GWBUF* readbuf; - size_t buflen; - size_t packetlen; - size_t totalbuflen; - uint8_t* data; - size_t nbytes_copied = 0; - uint8_t* target; - - readbuf = *p_readbuf; - - if (readbuf == NULL) - { - packetbuf = NULL; - goto return_packetbuf; - } - CHK_GWBUF(readbuf); - - if (GWBUF_EMPTY(readbuf)) - { - packetbuf = NULL; - goto return_packetbuf; - } - totalbuflen = gwbuf_length(readbuf); - data = (uint8_t *)GWBUF_DATA((readbuf)); - packetlen = MYSQL_GET_PAYLOAD_LEN(data) + 4; - - /** packet is incomplete */ - if (packetlen > totalbuflen) - { - packetbuf = NULL; - goto return_packetbuf; - } - - packetbuf = gwbuf_alloc(packetlen); - target = GWBUF_DATA(packetbuf); - packetbuf->gwbuf_type = readbuf->gwbuf_type; /*< Copy the type too */ - /** - * Copy first MySQL packet to packetbuf and leave posible other - * packets to read buffer. - */ - while (nbytes_copied < packetlen && totalbuflen > 0) - { - uint8_t* src = GWBUF_DATA((*p_readbuf)); - size_t bytestocopy; - - buflen = GWBUF_LENGTH((*p_readbuf)); - bytestocopy = buflen < (packetlen - nbytes_copied) ? buflen : packetlen - nbytes_copied; - - memcpy(target + nbytes_copied, src, bytestocopy); - *p_readbuf = gwbuf_consume((*p_readbuf), bytestocopy); - totalbuflen = gwbuf_length((*p_readbuf)); - nbytes_copied += bytestocopy; - } - ss_dassert(buflen == 0 || nbytes_copied == packetlen); - -return_packetbuf: - return packetbuf; -} - -/** - * Move from buffer pointed to by <*p_readbuf>. - * Appears to be unused 11 May 2016 (Martin) - */ -GWBUF* gw_MySQL_get_packets(GWBUF** p_srcbuf, - int* npackets) -{ - GWBUF* packetbuf; - GWBUF* targetbuf = NULL; - - while (*npackets > 0 && (packetbuf = gw_MySQL_get_next_packet(p_srcbuf)) != NULL) - { - targetbuf = gwbuf_append(targetbuf, packetbuf); - *npackets -= 1; - } - ss_dassert(*npackets < 128); - ss_dassert(*npackets >= 0); - - return targetbuf; -} - - static server_command_t* server_command_init(server_command_t* srvcmd, mxs_mysql_cmd_t cmd) { From bf4673d3f78c11fb89dbf6bc0783c7b3e69eee58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Fri, 15 Jun 2018 12:37:36 +0300 Subject: [PATCH 6/8] MXS-872: Add role test case The test case checks whether roles work via MaxScale. --- maxscale-system-test/CMakeLists.txt | 4 ++ maxscale-system-test/mxs872_roles.cpp | 60 +++++++++++++++++++++++++++ 2 files changed, 64 insertions(+) create mode 100644 maxscale-system-test/mxs872_roles.cpp diff --git a/maxscale-system-test/CMakeLists.txt b/maxscale-system-test/CMakeLists.txt index 65b4e7ca2..1740ba870 100644 --- a/maxscale-system-test/CMakeLists.txt +++ b/maxscale-system-test/CMakeLists.txt @@ -483,6 +483,10 @@ add_test_executable(mxs822_maxpasswd.cpp mxs822_maxpasswd maxpasswd LABELS maxsc # Do only SELECTS during time > wait_timeout and then do INSERT add_test_executable(mxs827_write_timeout.cpp mxs827_write_timeout mxs827_write_timeout LABELS readwritesplit REPL_BACKEND) +# MXS-872: MaxScale doesn't understand roles +# https://jira.mariadb.org/browse/MXS-872 +add_test_executable(mxs872_roles.cpp mxs872_roles replication LABELS REPL_BACKEND) + # Block and unblock first and second slaves and check that they are recovered add_test_executable(mxs874_slave_recovery.cpp mxs874_slave_recovery mxs874 LABELS readwritesplit REPL_BACKEND) diff --git a/maxscale-system-test/mxs872_roles.cpp b/maxscale-system-test/mxs872_roles.cpp new file mode 100644 index 000000000..23a583fea --- /dev/null +++ b/maxscale-system-test/mxs872_roles.cpp @@ -0,0 +1,60 @@ +/** + * MXS-872: MaxScale doesn't understand roles + * + * https://jira.mariadb.org/browse/MXS-872 + */ + +#include "testconnections.h" +#include + +using namespace std; + +int main(int argc, char** argv) +{ + TestConnections test(argc, argv); + + test.repl->connect(); + for (auto a : vector({"DROP DATABASE IF EXISTS my_db", + "CREATE DATABASE my_db", + "DROP ROLE IF EXISTS dba", + "CREATE ROLE dba", + "GRANT SELECT ON my_db.* TO dba", + "DROP USER IF EXISTS 'test'@'%'", + "DROP USER IF EXISTS 'test2'@'%'", + "CREATE USER 'test'@'%' IDENTIFIED BY 'test'", + "CREATE USER 'test2'@'%' IDENTIFIED BY 'test2'", + "GRANT dba TO 'test'@'%'", + "GRANT dba TO 'test2'@'%'", + "SET DEFAULT ROLE dba FOR 'test'@'%'"})) + { + test.try_query(test.repl->nodes[0], "%s", a.c_str()); + } + + // Wait for the users to replicate + test.repl->sync_slaves(); + + test.tprintf("Connect with a user that has a default role"); + MYSQL* conn = open_conn_db(test.maxscales->rwsplit_port[0], test.maxscales->IP[0], "my_db", "test", "test"); + test.assert(mysql_errno(conn) == 0, "Connection failed: %s", mysql_error(conn)); + char value[100] {}; + find_field(conn, "SELECT CURRENT_ROLE() AS role", "role", value); + test.assert(strcmp(value, "dba") == 0, "Current role should be 'dba' but is: %s", value); + mysql_close(conn); + + test.tprintf("Connect with a user that doesn't have a default role, expect failure"); + conn = open_conn_db(test.maxscales->rwsplit_port[0], test.maxscales->IP[0], "my_db", "test2", "test2"); + test.assert(mysql_errno(conn) != 0, "Connection should fail"); + mysql_close(conn); + + // Cleanup + for (auto a : vector({"DROP DATABASE IF EXISTS my_db", + "DROP ROLE IF EXISTS dba", + "DROP USER 'test'@'%'", + "DROP USER 'test2'@'%'"})) + { + execute_query_silent(test.repl->nodes[0], "%s", a.c_str()); + } + + test.repl->disconnect(); + return test.global_result; +} From b0187817649d49262490e5594aca0b762facdb1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Fri, 15 Jun 2018 16:04:07 +0300 Subject: [PATCH 7/8] MXS-872: Add support for roles The users query for the MySQLAuth now handles users with default roles. --- .../modules/authenticator/MySQLAuth/dbusers.c | 69 ++++++++++++++++++- 1 file changed, 67 insertions(+), 2 deletions(-) diff --git a/server/modules/authenticator/MySQLAuth/dbusers.c b/server/modules/authenticator/MySQLAuth/dbusers.c index 374620a6d..e1c6bf04a 100644 --- a/server/modules/authenticator/MySQLAuth/dbusers.c +++ b/server/modules/authenticator/MySQLAuth/dbusers.c @@ -50,14 +50,77 @@ FROM mysql.user AS u LEFT JOIN mysql.tables_priv AS t \ ON (u.user = t.user AND u.host = t.host) WHERE u.plugin IN ('', 'mysql_native_password') %s" + // Query used with MariaDB 10.1 and newer, supports roles +const char* mariadb_users_query = + // First, select all users + "SELECT t.user, t.host, t.db, t.select_priv, t.password FROM " + "( " + " SELECT u.user, u.host, d.db, u.select_priv, u.password AS password, u.is_role " + " FROM mysql.user AS u LEFT JOIN mysql.db AS d " + " ON (u.user = d.user AND u.host = d.host) " + " UNION " + " SELECT u.user, u.host, t.db, u.select_priv, u.password AS password, u.is_role " + " FROM mysql.user AS u LEFT JOIN mysql.tables_priv AS t " + " ON (u.user = t.user AND u.host = t.host) " + ") AS t " + // Discard any users that are roles + "WHERE t.is_role <> 'Y' %s " + "UNION " + // Then select all users again + "SELECT r.user, r.host, u.db, u.select_priv, t.password FROM " + "( " + " SELECT u.user, u.host, d.db, u.select_priv, u.password AS password, u.default_role " + " FROM mysql.user AS u LEFT JOIN mysql.db AS d " + " ON (u.user = d.user AND u.host = d.host) " + " UNION " + " SELECT u.user, u.host, t.db, u.select_priv, u.password AS password, u.default_role " + " FROM mysql.user AS u LEFT JOIN mysql.tables_priv AS t " + " ON (u.user = t.user AND u.host = t.host) " + ") AS t " + // Join it to the roles_mapping table to only have users with roles + "JOIN mysql.roles_mapping AS r " + "ON (r.user = t.user AND r.host = t.host) " + // Then join it into itself to get the privileges of the role with the name of the user + "JOIN " + "( " + " SELECT u.user, u.host, d.db, u.select_priv, u.password AS password, u.is_role " + " FROM mysql.user AS u LEFT JOIN mysql.db AS d " + " ON (u.user = d.user AND u.host = d.host) " + " UNION " + " SELECT u.user, u.host, t.db, u.select_priv, u.password AS password, u.is_role " + " FROM mysql.user AS u LEFT JOIN mysql.tables_priv AS t " + " ON (u.user = t.user AND u.host = t.host) " + ") AS u " + "ON (u.user = r.role AND u.is_role = 'Y') " + // We only care about users that have a default role assigned + "WHERE t.default_role = u.user %s;"; + static int get_users(SERV_LISTENER *listener, bool skip_local); static MYSQL *gw_mysql_init(void); static int gw_mysql_set_timeouts(MYSQL* handle); static char *mysql_format_user_entry(void *data); static bool get_hostname(DCB *dcb, char *client_hostname, size_t size); -static char* get_new_users_query(const char *server_version, bool include_root) +static char* get_mariadb_users_query(bool include_root) { + const char *root = include_root ? "" : " AND t.user NOT IN ('root')"; + + size_t n_bytes = snprintf(NULL, 0, mariadb_users_query, root, root); + char *rval = MXS_MALLOC(n_bytes + 1); + MXS_ABORT_IF_NULL(rval); + snprintf(rval, n_bytes + 1, mariadb_users_query, root, root); + + return rval; +} + +static char* get_users_query(const char *server_version, uint64_t version, bool include_root) +{ + if (version >= 100101) // 10.1.1 or newer, supports default roles + { + return get_mariadb_users_query(include_root); + } + + // Either an older MariaDB version or a MySQL variant, use the legacy query const char* password = strstr(server_version, "5.7.") || strstr(server_version, "8.0.") ? MYSQL57_PASSWORD : MYSQL_PASSWORD; const char *with_root = include_root ? "" : " AND u.user NOT IN ('root')"; @@ -738,7 +801,9 @@ int get_users_from_server(MYSQL *con, SERVER_REF *server_ref, SERVICE *service, mxs_mysql_set_server_version(con, server_ref->server); } - char *query = get_new_users_query(server_ref->server->version_string, service->enable_root); + char *query = get_users_query(server_ref->server->version_string, + server_ref->server->version, + service->enable_root); MYSQL_AUTH *instance = (MYSQL_AUTH*)listener->auth_instance; sqlite3* handle = get_handle(instance); bool anon_user = false; From 980caa5be5e876943ca5d93a5ec29c5e5db90422 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Wed, 20 Jun 2018 00:34:16 +0300 Subject: [PATCH 8/8] MXS-1926: Ignore server shutdown errors If the server sends a server shutdown error, it is safe for readwritesplit to ignore it. When the TCP connection is closed, the router error handling will discard the connection, optionally replacing it. --- .../routing/readwritesplit/readwritesplit.cc | 50 ++++++++++++------- 1 file changed, 33 insertions(+), 17 deletions(-) diff --git a/server/modules/routing/readwritesplit/readwritesplit.cc b/server/modules/routing/readwritesplit/readwritesplit.cc index f315cb6e1..0973693dc 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.cc +++ b/server/modules/routing/readwritesplit/readwritesplit.cc @@ -1123,6 +1123,22 @@ static json_t* diagnostics_json(const MXS_ROUTER *instance) return rval; } +static bool connection_was_killed(GWBUF* buffer) +{ + bool rval = false; + + if (mxs_mysql_is_err_packet(buffer)) + { + uint8_t buf[2]; + // First two bytes after the 0xff byte are the error code + gwbuf_copy_data(buffer, MYSQL_HEADER_LEN + 1, 2, buf); + uint16_t errcode = gw_mysql_get_byte2(buf); + rval = errcode == ER_CONNECTION_KILLED; + } + + return rval; +} + static void log_unexpected_response(SRWBackend& backend, GWBUF* buffer) { if (mxs_mysql_is_err_packet(buffer)) @@ -1135,18 +1151,9 @@ static void log_unexpected_response(SRWBackend& backend, GWBUF* buffer) uint16_t errcode = MYSQL_GET_ERRCODE(data); std::string errstr((char*)data + 7, (char*)data + 7 + len - 3); - if (errcode == ER_CONNECTION_KILLED) - { - MXS_INFO("Connection from '%s'@'%s' to '%s' was killed", - backend->dcb()->session->client_dcb->user, - backend->dcb()->session->client_dcb->remote, - backend->name()); - } - else - { - MXS_WARNING("Server '%s' sent an unexpected error: %hu, %s", - backend->name(), errcode, errstr.c_str()); - } + ss_dassert(errcode != ER_CONNECTION_KILLED); + MXS_WARNING("Server '%s' sent an unexpected error: %hu, %s", + backend->name(), errcode, errstr.c_str()); } else { @@ -1192,11 +1199,20 @@ static void clientReply(MXS_ROUTER *instance, if (backend->get_reply_state() == REPLY_STATE_DONE) { - /** If we receive an unexpected response from the server, the internal - * logic cannot handle this situation. Routing the reply straight to - * the client should be the safest thing to do at this point. */ - log_unexpected_response(backend, writebuf); - MXS_SESSION_ROUTE_REPLY(backend_dcb->session, writebuf); + if (connection_was_killed(writebuf)) + { + // The connection was killed, we can safely ignore it. When the TCP connection is + // closed, the router's error handling will sort it out. + gwbuf_free(writebuf); + } + else + { + /** If we receive an unexpected response from the server, the internal + * logic cannot handle this situation. Routing the reply straight to + * the client should be the safest thing to do at this point. */ + log_unexpected_response(backend, writebuf); + MXS_SESSION_ROUTE_REPLY(backend_dcb->session, writebuf); + } return; }