From cb360f089c0ae8eaa1b732b221033cba2bf29af9 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Sat, 22 Jul 2017 09:46:05 +0200 Subject: [PATCH] Restart postgres after custom bootstrap if hba_file is defined in configuration (#482) In addition to that always use absolute paths to config files. Fixes https://github.com/zalando/patroni/issues/481 --- patroni/postgresql.py | 53 +++++++++++++++++++++++----------------- tests/test_postgresql.py | 37 ++++++++++++++++------------ 2 files changed, 52 insertions(+), 38 deletions(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 02f203df..7b37b39c 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -102,7 +102,7 @@ class Postgresql(object): self._bin_dir = config.get('bin_dir') or '' self._database = config.get('database', 'postgres') self._data_dir = config['data_dir'] - self._config_dir = config.get('config_dir') or self._data_dir + self._config_dir = os.path.abspath(config.get('config_dir') or self._data_dir) self._pending_restart = False self._running_custom_bootstrap = False self.__thread_ident = current_thread().ident @@ -170,9 +170,9 @@ class Postgresql(object): configuration = [os.path.basename(self._postgresql_conf)] if 'custom_conf' not in self.config: configuration.append(os.path.basename(self._postgresql_base_conf)) - if not self.config['parameters'].get('hba_file'): + if not self._server_parameters.get('hba_file'): configuration.append('pg_hba.conf') - if not self.config['parameters'].get('ident_file'): + if not self._server_parameters.get('ident_file'): configuration.append('pg_ident.conf') return configuration @@ -226,8 +226,12 @@ class Postgresql(object): parameters['synchronous_standby_names'] = self._synchronous_standby_names if self._major_version >= 90600 and parameters['wal_level'] == 'hot_standby': parameters['wal_level'] = 'replica' - return {k: v for k, v in parameters.items() if not self._major_version or - self._major_version >= self.CMDLINE_OPTIONS.get(k, (0, 1, 90100))[2]} + ret = {k: v for k, v in parameters.items() if not self._major_version or + self._major_version >= self.CMDLINE_OPTIONS.get(k, (0, 1, 90100))[2]} + for k in ('hba_file', 'ident_file'): + if k in ret: + ret[k] = os.path.join(self._config_dir, ret[k]) + return ret def resolve_connection_addresses(self): port = self._server_parameters['port'] @@ -335,7 +339,7 @@ class Postgresql(object): conf_changed = True break - if not config['parameters'].get('hba_file') and config.get('pg_hba'): + if not server_parameters.get('hba_file') and config.get('pg_hba'): hba_changed = self.config.get('pg_hba', []) != config['pg_hba'] self.config = config @@ -519,7 +523,7 @@ class Postgresql(object): if pwfile: os.remove(pwfile) if ret: - if not self.config['parameters'].get('hba_file') and not self.config.get('pg_hba'): + if not self._server_parameters.get('hba_file') and not self.config.get('pg_hba'): self.write_pg_hba(config.get('pg_hba', [])) self._major_version = self.get_major_version() self._server_parameters = self.get_server_parameters(self.config) @@ -1085,7 +1089,6 @@ class Postgresql(object): with open(self._postgresql_conf, 'w') as f: f.write(self._CONFIG_WARNING_HEADER) f.write("include '{0}'\n\n".format(self.config.get('custom_conf') or self._postgresql_base_conf_name)) - f.write("data_directory = '{0}'\n".format(self._data_dir)) for name, value in sorted(self._server_parameters.items()): if not self._running_custom_bootstrap or name != 'hba_file': f.write("{0} = '{1}'\n".format(name, value)) @@ -1130,7 +1133,7 @@ class Postgresql(object): for address, t in addresses.items(): f.write('{0}\t{1}\t{2}\t{3}\ttrust\n'.format(t, self._database, self._superuser.get('username') or 'all', address)) - elif not self.config['parameters'].get('hba_file') and self.config.get('pg_hba'): + elif not self._server_parameters.get('hba_file') and self.config.get('pg_hba'): with open(self._pg_hba_conf, 'w') as f: f.write(self._CONFIG_WARNING_HEADER) for line in self.config['pg_hba']: @@ -1538,20 +1541,6 @@ $$""".format(name, ' '.join(options)), name, password, password) try: self.create_or_update_role(self._superuser['username'], self._superuser['password'], ['SUPERUSER']) - # We were doing a custom bootstrap instead of running initdb, therefore we opened trust - # access from certain addresses to be able to reach cluster and change password - if self._running_custom_bootstrap: - self._running_custom_bootstrap = False - # If we don't have custom configuration for pg_hba.conf we need to restore original file - if not self.config.get('pg_hba'): - os.unlink(self._pg_hba_conf) - self.restore_configuration_files() - self._write_postgresql_conf() - self._replace_pg_hba() - self.reload() - time.sleep(1) # give a time to postgres to "reload" configuration files - self.close_connection() # close connection to reconnect with a new password - task.complete(self.run_bootstrap_post_init(config)) if task.result: self.create_or_update_role(self._replication['username'], @@ -1559,6 +1548,24 @@ $$""".format(name, ' '.join(options)), name, password, password) for name, value in (config.get('users') or {}).items(): if name not in (self._superuser.get('username'), self._replication['username']): self.create_or_update_role(name, value['password'], value.get('options', [])) + + # We were doing a custom bootstrap instead of running initdb, therefore we opened trust + # access from certain addresses to be able to reach cluster and change password + if self._running_custom_bootstrap: + self._running_custom_bootstrap = False + # If we don't have custom configuration for pg_hba.conf we need to restore original file + if not self.config.get('pg_hba'): + os.unlink(self._pg_hba_conf) + self.restore_configuration_files() + self._write_postgresql_conf() + if self._server_parameters.get('hba_file') and \ + self._server_parameters['hba_file'] != self._pg_hba_conf: + self.restart() + else: + self._replace_pg_hba() + self.reload() + time.sleep(1) # give a time to postgres to "reload" configuration files + self.close_connection() # close connection to reconnect with a new password except Exception: logger.exception('post_bootstrap') task.complete(False) diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 0dd82344..4b1342df 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -280,7 +280,7 @@ class TestPostgresql(unittest.TestCase): self.assertFalse(self.p.stop()) mock_get_pid.return_value = 123 with patch('os.kill', Mock(side_effect=[OSError(errno.ESRCH, ''), OSError, None])),\ - patch('psutil.Process', Mock(side_effect=psutil.NoSuchProcess(123))): + patch('psutil.Process', Mock(side_effect=psutil.NoSuchProcess(123))): self.assertTrue(self.p.stop()) self.assertFalse(self.p.stop()) self.p.stop_safepoint_reached.clear() @@ -542,7 +542,7 @@ class TestPostgresql(unittest.TestCase): patch('os.unlink', Mock()),\ patch.object(Postgresql, 'save_configuration_files', Mock()),\ patch.object(Postgresql, 'restore_configuration_files', Mock()),\ - patch.object(Postgresql, 'write_recovery_conf', Mock()): + patch.object(Postgresql, 'write_recovery_conf', Mock()): with self.assertRaises(Exception) as e: self.p.bootstrap(config) self.assertEqual(str(e.exception), '42') @@ -554,24 +554,31 @@ class TestPostgresql(unittest.TestCase): self.assertEqual(str(e.exception), '42') @patch('time.sleep', Mock()) - @patch.object(Postgresql, 'run_bootstrap_post_init', Mock(side_effect=Exception)) + @patch('os.unlink', Mock()) + @patch.object(Postgresql, 'run_bootstrap_post_init', Mock(return_value=True)) + @patch.object(Postgresql, '_custom_bootstrap', Mock(return_value=True)) + @patch.object(Postgresql, 'start', Mock(return_value=True)) def test_post_bootstrap(self): config = {'method': 'foo', 'foo': {'command': 'bar'}} - with patch('subprocess.call', Mock(return_value=0)), \ - patch('subprocess.Popen', Mock(side_effect=Exception("42"))), \ - patch('os.path.isfile', Mock(return_value=True)),\ - patch('os.unlink', Mock()), \ - patch.object(Postgresql, 'save_configuration_files', Mock()), \ - patch.object(Postgresql, 'restore_configuration_files', Mock()), \ - patch.object(Postgresql, 'write_recovery_conf', Mock()): - with self.assertRaises(Exception) as e: - self.p.bootstrap(config) - self.assertEqual(str(e.exception), '42') + self.p.bootstrap(config) + + task = CriticalTask() + with patch.object(Postgresql, 'create_or_update_role', Mock(side_effect=Exception)): + self.p.post_bootstrap({}, task) + self.assertFalse(task.result) self.p.config.pop('pg_hba') - task = CriticalTask() self.p.post_bootstrap({}, task) - self.assertFalse(task.result) + self.assertTrue(task.result) + + self.p.bootstrap(config) + self.p.set_state('stopped') + self.p.reload_config({'authentication': {'superuser': {'username': 'p', 'password': 'p'}, + 'replication': {'username': 'r', 'password': 'r'}}, + 'listen': '*', 'retry_timeout': 10, 'parameters': {'hba_file': 'foo'}}) + with patch.object(Postgresql, 'restart', Mock()) as mock_restart: + self.p.post_bootstrap({}, task) + mock_restart.assert_called_once() def test_run_bootstrap_post_init(self): with patch('subprocess.call', Mock(return_value=1)):