From 6995a3c641eb9d47d29bae9551c165f26a849ef3 Mon Sep 17 00:00:00 2001 From: Pierre Goiffon Date: Wed, 20 Dec 2023 15:19:50 +0100 Subject: [PATCH] =?UTF-8?q?N=C2=B06889=20backup=20mysqldump=20call=20:=20r?= =?UTF-8?q?estore=20possibility=20to=20connect=20using=20socket=20protocol?= =?UTF-8?q?=20(#591)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With previous fix (N°6123) we forced to use the tcp protocol each time. This was blocking for users wanting to connect using the socket protocol on localhost. Now for localhost we will : - send both port and protocol arguments if the `db_host` config parameter does contain a port - don't send any of the port or protocol arguments if `db_host` doesn't contain a port --- core/cmdbsource.class.inc.php | 19 ++++--- setup/backup.class.inc.php | 49 ++++++++++++------- .../core/CMDBSource/CMDBSourceTest.php | 33 +++++++++++++ .../unitary-tests/setup/DBBackupTest.php | 49 ++++++++++++------- 4 files changed, 107 insertions(+), 43 deletions(-) diff --git a/core/cmdbsource.class.inc.php b/core/cmdbsource.class.inc.php index eab8fffb5..07a1767d7 100644 --- a/core/cmdbsource.class.inc.php +++ b/core/cmdbsource.class.inc.php @@ -134,6 +134,12 @@ class CMDBSource const ENUM_DB_VENDOR_MARIADB = 'MariaDB'; const ENUM_DB_VENDOR_PERCONA = 'Percona'; + /** + * @since 2.7.10 3.0.4 3.1.2 3.0.2 N°6889 constant creation + * @internal will be removed in a future version + */ + const MYSQL_DEFAULT_PORT = 3306; + /** * Error: 1205 SQLSTATE: HY000 (ER_LOCK_WAIT_TIMEOUT) * Message: Lock wait timeout exceeded; try restarting transaction @@ -319,16 +325,19 @@ class CMDBSource /** * @param string $sDbHost initial value ("p:domain:port" syntax) * @param string $sServer server variable to update - * @param int $iPort port variable to update + * @param int|null $iPort port variable to update, will return null if nothing is specified in $sDbHost + * + * @since 2.7.10 3.0.4 3.1.2 3.2.0 N°6889 will return null in $iPort if port isn't present in $sDbHost. Use {@see MYSQL_DEFAULT_PORT} if needed + * + * @link http://php.net/manual/en/mysqli.persistconns.php documentation for the "p:" prefix (persistent connexion) */ public static function InitServerAndPort($sDbHost, &$sServer, &$iPort) { $aConnectInfo = explode(':', $sDbHost); $bUsePersistentConnection = false; - if (strcasecmp($aConnectInfo[0], 'p') == 0) + if (strcasecmp($aConnectInfo[0], 'p') === 0) { - // we might have "p:" prefix to use persistent connections (see http://php.net/manual/en/mysqli.persistconns.php) $bUsePersistentConnection = true; $sServer = $aConnectInfo[0].':'.$aConnectInfo[1]; } @@ -346,10 +355,6 @@ class CMDBSource { $iPort = (int)($aConnectInfo[1]); } - else - { - $iPort = 3306; - } } /** diff --git a/setup/backup.class.inc.php b/setup/backup.class.inc.php index a7bb9ac70..d03650edc 100644 --- a/setup/backup.class.inc.php +++ b/setup/backup.class.inc.php @@ -305,9 +305,8 @@ class DBBackup // Store the results in a temporary file $sTmpFileName = self::EscapeShellArg($sBackupFileName); - $sPortOption = self::GetMysqliCliSingleOption('port', $this->iDBPort); + $sPortAndTransportOptions = self::GetMysqlCliPortAndTransportOptions($this->sDBHost, $this->iDBPort); $sTlsOptions = self::GetMysqlCliTlsOptions($this->oConfig); - $sProtocolOption = self::GetMysqlCliTransportOption($this->sDBHost); $sMysqlVersion = CMDBSource::GetDBVersion(); $bIsMysqlSupportUtf8mb4 = (version_compare($sMysqlVersion, self::MYSQL_VERSION_WITH_UTF8MB4_IN_PROGRAMS) === -1); @@ -326,10 +325,10 @@ EOF; chmod($sMySQLDumpCnfFile, 0600); file_put_contents($sMySQLDumpCnfFile, $sMySQLDumpCnf, LOCK_EX); - // Note: opt implicitely sets lock-tables... which cancels the benefit of single-transaction! + // Note: opt implicitly sets lock-tables... which cancels the benefit of single-transaction! // skip-lock-tables compensates and allows for writes during a backup - $sCommand = "$sMySQLDump --defaults-extra-file=\"$sMySQLDumpCnfFile\" --opt --skip-lock-tables --default-character-set=".$sMysqldumpCharset." --add-drop-database --single-transaction --host=$sHost $sPortOption $sProtocolOption --user=$sUser $sTlsOptions --result-file=$sTmpFileName $sDBName $sTables 2>&1"; - $sCommandDisplay = "$sMySQLDump --defaults-extra-file=\"$sMySQLDumpCnfFile\" --opt --skip-lock-tables --default-character-set=".$sMysqldumpCharset." --add-drop-database --single-transaction --host=$sHost $sPortOption $sProtocolOption --user=xxxxx $sTlsOptions --result-file=$sTmpFileName $sDBName $sTables"; + $sCommand = "$sMySQLDump --defaults-extra-file=\"$sMySQLDumpCnfFile\" --opt --skip-lock-tables --default-character-set=" . $sMysqldumpCharset . " --add-drop-database --single-transaction --host=$sHost $sPortAndTransportOptions --user=$sUser $sTlsOptions --result-file=$sTmpFileName $sDBName $sTables 2>&1"; + $sCommandDisplay = "$sMySQLDump --defaults-extra-file=\"$sMySQLDumpCnfFile\" --opt --skip-lock-tables --default-character-set=" . $sMysqldumpCharset . " --add-drop-database --single-transaction --host=$sHost $sPortAndTransportOptions --user=xxxxx $sTlsOptions --result-file=$sTmpFileName $sDBName $sTables"; // Now run the command for real $this->LogInfo("backup: generate data file with command: $sCommandDisplay"); @@ -523,25 +522,37 @@ EOF; } /** - * Define if we should force a transport option - * - * @param string $sHost + * @return string CLI options for port and protocol * - * @return string . - - * @since 2.7.9 3.0.4 3.1.1 N°6123 + * @since 2.7.9 3.0.4 3.1.1 N°6123 method creation + * @since 2.7.10 3.0.4 3.1.2 3.2.0 N°6889 rename method to return both port and transport options. Keep default socket connexion if we are on localhost with no port + * + * @link https://bugs.mysql.com/bug.php?id=55796 MySQL CLI tools will ignore `--port` option on localhost + * @link https://jira.mariadb.org/browse/MDEV-14974 Since 10.6.1 the MariaDB CLI tools will use the `--port` option on host=localhost */ - public static function GetMysqlCliTransportOption(string $sHost) + private static function GetMysqlCliPortAndTransportOptions(string $sHost, ?int $iPort): string { - $sTransportOptions = ''; - - /** N°6123 As we're using a --port option, if we use localhost as host, - * MariaDB > 10.6 will implicitly change its protocol from socket to tcp and throw a warning **/ - if($sHost === 'localhost'){ - $sTransportOptions = '--protocol=tcp'; + if (strtolower($sHost) === 'localhost') { + /** + * Since MariaDB 10.6.1 if we have host=localhost, and only the --port option we will get a warning + * To avoid this warning if we want to set --port option we must set --protocol=tcp + **/ + if (is_null($iPort)) { + // no port specified => no option to return, this will mean using socket protocol (unix socket) + return ''; + } + + $sPortOption = self::GetMysqliCliSingleOption('port', $iPort); + $sTransportOptions = ' --protocol=tcp'; + return $sPortOption . $sTransportOptions; } - return $sTransportOptions; + if (is_null($iPort)) { + $iPort = CMDBSource::MYSQL_DEFAULT_PORT; + } + $sPortOption = self::GetMysqliCliSingleOption('port', $iPort); + + return $sPortOption; } /** diff --git a/tests/php-unit-tests/unitary-tests/core/CMDBSource/CMDBSourceTest.php b/tests/php-unit-tests/unitary-tests/core/CMDBSource/CMDBSourceTest.php index 986a79a49..213a1168e 100644 --- a/tests/php-unit-tests/unitary-tests/core/CMDBSource/CMDBSourceTest.php +++ b/tests/php-unit-tests/unitary-tests/core/CMDBSource/CMDBSourceTest.php @@ -135,4 +135,37 @@ class CMDBSourceTest extends ItopTestCase $bIsTlsCnx = $this->InvokeNonPublicStaticMethod(CMDBSource::class, 'IsOpenedDbConnectionUsingTls',[$oMysqli]); $this->assertFalse($bIsTlsCnx); } + + /** + * @dataProvider InitServerAndPortProvider + * @since 2.7.10 3.0.4 3.1.2 3.2.0 N°6889 method creation to keep track of the behavior change (port will return null) + */ + public function testInitServerAndPort(string $sDbHost, string $sExpectedServer, ?int $iExpectedPort) + { + $sActualServer = null; + $iActualPort = null; + CMDBSource::InitServerAndPort($sDbHost, $sActualServer, $iActualPort); + + $this->assertNotNull($sActualServer); + $this->assertEquals($sExpectedServer, $sActualServer); + $this->assertEquals($iExpectedPort, $iActualPort); + } + + public function InitServerAndPortProvider() + { + return [ + 'localhost no port' => ['localhost', 'localhost', null], + 'localhost with port' => ['localhost:333306', 'localhost', 333306], + 'persistent localhost no port' => ['p:localhost', 'p:localhost', null], + 'persistent localhost with port' => ['p:localhost:333306', 'p:localhost', 333306], + 'ip no port' => ['192.168.1.10', '192.168.1.10', null], + 'ip with port' => ['192.168.1.10:333306', '192.168.1.10', 333306], + 'persistent ip no port' => ['p:192.168.1.10', 'p:192.168.1.10', null], + 'persistent ip with port' => ['p:192.168.1.10:333306', 'p:192.168.1.10', 333306], + 'domain no port' => ['dbserver.mycompany.com', 'dbserver.mycompany.com', null], + 'domain with port' => ['dbserver.mycompany.com:333306', 'dbserver.mycompany.com', 333306], + 'persistent domain no port' => ['p:dbserver.mycompany.com', 'p:dbserver.mycompany.com', null], + 'persistent domain with port' => ['p:dbserver.mycompany.com:333306', 'p:dbserver.mycompany.com', 333306], + ]; + } } diff --git a/tests/php-unit-tests/unitary-tests/setup/DBBackupTest.php b/tests/php-unit-tests/unitary-tests/setup/DBBackupTest.php index f0694967b..e86e6ee28 100644 --- a/tests/php-unit-tests/unitary-tests/setup/DBBackupTest.php +++ b/tests/php-unit-tests/unitary-tests/setup/DBBackupTest.php @@ -86,29 +86,44 @@ class DBBackupTest extends ItopTestCase } /** - * Host is localhost, we should be forced into tcp - * - * @return void + * @dataProvider GetMysqlCliPortAndTransportOptionsProvider + * @since 2.7.10 3.0.4 3.1.2 3.2.0 test for N°6123 and N°6889 */ - public function testGetMysqlCliTransportOptionWithLocalhost() + public function testGetMysqlCliPortAndTransportOptions(string $sDbHost, ?int $iPort, ?int $iExpectedPortValue, string $sExpectedProtocolCliOption) { - $sHost= 'localhost'; - $sTransport = DBBackup::GetMysqlCliTransportOption($sHost); + if (is_null($iExpectedPortValue)) { + $sExpectedPortCliOption = ''; + } else { + $sEscapedPortValue = \DBBackup::EscapeShellArg($iExpectedPortValue); + $sExpectedPortCliOption = ' --port=' . $sEscapedPortValue; + } - $this->assertStringStartsWith('--protocol=tcp', $sTransport); - $this->assertStringEndsWith('--protocol=tcp', $sTransport); + $sActualCliOptions = $this->InvokeNonPublicStaticMethod(DBBackup::class, 'GetMysqlCliPortAndTransportOptions', [$sDbHost, $iPort]); + $this->assertEquals($sExpectedPortCliOption . $sExpectedProtocolCliOption, $sActualCliOptions); } - /** - * Host is not localhost, we shouldn't be forced into tcp - * - * @return void - */ - public function testGetMysqlCliTransportOptionWithoutLocalhost() + public function GetMysqlCliPortAndTransportOptionsProvider() { - $sHost= '127.0.0.1'; - $sTransport = DBBackup::GetMysqlCliTransportOption($sHost); + $iTestPort = 333306; + $iDefaultPort = 3306; // cannot access \CMDBSource::MYSQL_DEFAULT_PORT in dataprovider :( - $this->assertEmpty($sTransport); + return [ + 'Localhost no port' => ['localhost', null, null, ''], + 'Localhost with port' => ['localhost', $iTestPort, $iTestPort, ' --protocol=tcp'], + + // we want both port and protocol for 127.0.0.1, because it is an ip address so using tcp/ip stack ! + '127.0.0.1 no port' => ['127.0.0.1', null, $iDefaultPort, ''], + '127.0.0.1 with port' => ['127.0.0.1', $iTestPort, $iTestPort, ''], + + 'IP no port' => ['192.168.1.15', null, $iDefaultPort, ''], + 'IP with port' => ['192.168.1.15', $iTestPort, $iTestPort, ''], + + 'DNS no port' => ['dbserver.mycompany.com', null, $iDefaultPort, ''], + 'DNS with port' => ['dbserver.mycompany.com', $iTestPort, $iTestPort, ''], + + 'Windows name no port' => ['dbserver', null, $iDefaultPort, ''], + 'Windows name with port' => ['dbserver', $iTestPort, $iTestPort, ''], + ]; } + }