From 5ca43c1a3d578e6b7146d0ec9cc9b6f6fad4ecf0 Mon Sep 17 00:00:00 2001 From: Feike Steenbergen Date: Wed, 20 Apr 2016 10:54:55 +0200 Subject: [PATCH 1/3] Remove pg_hba injection and filtering Previously we explicitly injected a replication record into pg_hba.conf. This doesn't allow users to explicitly write their configurations. This change will just write the lines specified by the user. --- patroni/postgresql.py | 6 +----- postgres0.yml | 4 ++-- postgres1.yml | 4 ++-- postgres2.yml | 4 ++-- tests/test_postgresql.py | 12 +++++++++--- 5 files changed, 16 insertions(+), 14 deletions(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 909a120e..a75bc6a3 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -446,11 +446,7 @@ class Postgresql(object): def write_pg_hba(self): with open(os.path.join(self.data_dir, 'pg_hba.conf'), 'a') as f: - f.write('\nhost replication {username} {network} md5\n'.format(**self.replication)) - for line in self.config.get('pg_hba', []): - if line.split()[0].strip() == 'hostssl' and self.server_parameters.get('ssl', 'off').lower() != 'on': - continue - f.write(line + '\n') + f.write('\n{}\n'.format('\n'.join(self.config.get('pg_hba', [])))) @staticmethod def primary_conninfo(leader_url): diff --git a/postgres0.yml b/postgres0.yml index c02429b4..0bbdd34e 100644 --- a/postgres0.yml +++ b/postgres0.yml @@ -56,12 +56,12 @@ postgresql: username: postgres password: zalando pg_hba: + - host replication replicator 127.0.0.1/32 md5 - host all all 0.0.0.0/0 md5 - - hostssl all all 0.0.0.0/0 md5 + # - hostssl all all 0.0.0.0/0 md5 replication: username: replicator password: rep-pass - network: 127.0.0.1/32 superuser: username: postgres password: zalando diff --git a/postgres1.yml b/postgres1.yml index 1b3ef72a..85529b36 100644 --- a/postgres1.yml +++ b/postgres1.yml @@ -56,12 +56,12 @@ postgresql: username: postgres password: zalando pg_hba: + - host replication replicator 127.0.0.1/32 md5 - host all all 0.0.0.0/0 md5 - - hostssl all all 0.0.0.0/0 md5 + # - hostssl all all 0.0.0.0/0 md5 replication: username: replicator password: rep-pass - network: 127.0.0.1/32 superuser: username: postgres password: zalando diff --git a/postgres2.yml b/postgres2.yml index db0d7c61..be0615ce 100644 --- a/postgres2.yml +++ b/postgres2.yml @@ -56,12 +56,12 @@ postgresql: username: postgres password: zalando pg_hba: + - host replication replicator 127.0.0.1/32 md5 - host all all 0.0.0.0/0 md5 - - hostssl all all 0.0.0.0/0 md5 + # - hostssl all all 0.0.0.0/0 md5 replication: username: replicator password: rep-pass - network: 127.0.0.1/32 superuser: username: postgres password: zalando diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 8bd563b9..43d830ea 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -164,13 +164,14 @@ class TestPostgresql(unittest.TestCase): def setUp(self): self.p = Postgresql({'name': 'test0', 'scope': 'batman', 'data_dir': 'data/test0', 'listen': '127.0.0.1, *:5432', 'connect_address': '127.0.0.2:5432', - 'pg_hba': ['hostssl all all 0.0.0.0/0 md5', 'host all all 0.0.0.0/0 md5'], + 'pg_hba': ['host replication replicator 127.0.0.1/32 md5', + 'hostssl all all 0.0.0.0/0 md5', + 'host all all 0.0.0.0/0 md5'], 'superuser': {'username': 'test', 'password': 'test'}, 'admin': {'username': 'admin', 'password': 'admin'}, 'pg_rewind': {'username': 'admin', 'password': 'admin'}, 'replication': {'username': 'replicator', - 'password': 'rep-pass', - 'network': '127.0.0.1/32'}, + 'password': 'rep-pass'}, 'parameters': {'foo': 'bar'}, 'recovery_conf': {'foo': 'bar'}, 'callbacks': {'on_start': 'true', 'on_stop': 'true', 'on_restart': 'true', 'on_role_change': 'true', @@ -205,6 +206,11 @@ class TestPostgresql(unittest.TestCase): self.assertTrue(self.p.initialize()) self.assertTrue(os.path.exists(os.path.join(self.p.data_dir, 'pg_hba.conf'))) + with open(os.path.join(self.p.data_dir, 'pg_hba.conf')) as f: + lines = f.readlines() + assert 'host replication replicator 127.0.0.1/32 md5\n' in lines + assert 'host all all 0.0.0.0/0 md5\n' in lines + @patch('os.path.exists', Mock(return_value=True)) @patch('os.unlink', Mock()) def test_delete_trigger_file(self): From b727626f19c7696ec70e38f399300290328225aa Mon Sep 17 00:00:00 2001 From: Feike Steenbergen Date: Wed, 20 Apr 2016 16:44:02 +0200 Subject: [PATCH 2/3] Update README.rst --- README.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/README.rst b/README.rst index d8068b46..b1ddbd7e 100644 --- a/README.rst +++ b/README.rst @@ -91,6 +91,7 @@ For an example file, see ``postgres0.yml``. Regarding settings: - *pg\_hba*: list of lines which should be added to pg\_hba.conf. - *- host all all 0.0.0.0/0 md5*. + - *- host replication replicator 127.0.0.1/32 md5* # A line like this is required for replication - *replication*: - *username*: replication username; user will be created during initialization. From c7ebe4aa63852e1b33ce495f749a05f8c986d3e9 Mon Sep 17 00:00:00 2001 From: Feike Steenbergen Date: Fri, 22 Apr 2016 08:35:53 +0200 Subject: [PATCH 3/3] Unify Patroni and Patronictl configuration A Patroni configuration should be enough for Patronictl The previous dcs_api url style is still supported. To remove duplicate magic variables a DCS_DEFAULTS was introduced. Some behaviour has changed: If you do not specify a DCS at all (not in configuration, not on commandline, not in environment), it will not default to etcd://localhost:4001 More test coverage for patronictl --- patroni/ctl.py | 51 +++++---------- tests/test_ctl.py | 162 ++++++++++++++++++++++++++-------------------- 2 files changed, 107 insertions(+), 106 deletions(-) diff --git a/patroni/ctl.py b/patroni/ctl.py index 3e382ce0..94541b0b 100644 --- a/patroni/ctl.py +++ b/patroni/ctl.py @@ -24,6 +24,10 @@ from six.moves.urllib_parse import urlparse CONFIG_DIR_PATH = click.get_app_dir('patroni') CONFIG_FILE_PATH = os.path.join(CONFIG_DIR_PATH, 'patronictl.yaml') LOGLEVEL = 'WARNING' +DCS_DEFAULTS = {'zookeeper': {'port': 2181, 'template': "zookeeper:\n hosts: ['{host}:{port}']"}, + 'exhibitor': {'port': 8181, 'template': "zookeeper:\n exhibitor:\n hosts: [{host}]\n port: {port}"}, + 'consul': {'port': 8500, 'template': "consul:\n host: '{host}:{port}'"}, + 'etcd': {'port': 4001, 'template': "etcd:\n host: '{host}:{port}'"}} class PatroniCtlException(ClickException): @@ -31,42 +35,22 @@ class PatroniCtlException(ClickException): def parse_dcs(dcs): - """ - Break up the provided dcs string - >>> parse_dcs('localhost') == {'etcd': {'host': 'localhost:4001'}} - True - >>> parse_dcs('localhost:8500') == {'consul': {'host': 'localhost:8500'}} - True - >>> parse_dcs('zookeeper://localhost') == {'zookeeper': {'hosts': ['localhost:2181']}} - True - >>> parse_dcs('exhibitor://localhost') == {'zookeeper': {'exhibitor': {'hosts': ['localhost'], 'port': 8181}}} - True - """ - - if not dcs: - return {} + if dcs is None: + return None parsed = urlparse(dcs) scheme = parsed.scheme if scheme == '' and parsed.netloc == '': parsed = urlparse('//' + dcs) + port = int(parsed.port) if parsed.port else None if scheme == '': - default_schemes = {'2181': 'zookeeper', '8181': 'exhibitor', '8500': 'consul'} - scheme = default_schemes.get(str(parsed.port), 'etcd') + scheme = ([k for k, v in DCS_DEFAULTS.items() if v['port'] == port] or ['etcd'])[0] + elif scheme not in DCS_DEFAULTS: + raise PatroniCtlException('Unknown dcs scheme: {}'.format(scheme)) - port = parsed.port - if port is None: - default_ports = {'consul': 8500, 'zookeeper': 2181, 'exhibitor': 8181} - port = default_ports.get(str(scheme), 4001) - - config = {'host': '{0}:{1}'.format(parsed.hostname, port)} - if scheme == 'exhibitor': - config = {scheme: {'port': int(port), 'hosts': [str(parsed.hostname)]}} - scheme = 'zookeeper' - elif scheme == 'zookeeper': - config['hosts'] = [config.pop('host')] - return {scheme: config} + dcs_info = DCS_DEFAULTS[scheme] + return yaml.load(dcs_info['template'].format(host=parsed.hostname or 'localhost', port=port or dcs_info['port'])) def load_config(path, dcs): @@ -78,10 +62,7 @@ def load_config(path, dcs): except (IOError, yaml.YAMLError): logging.exception('Could not load configuration file') - if dcs: - config['dcs'] = parse_dcs(dcs) - else: - config['dcs'] = parse_dcs(config.get('dcs_api')) + config.update(parse_dcs(dcs) or parse_dcs(config.get('dcs_api')) or {}) return config @@ -112,10 +93,10 @@ def ctl(ctx): def get_dcs(config, scope): - dcs_config = config.get('dcs', {}) - dcs_config[list(dcs_config.keys())[0]]['scope'] = scope + for k in set(DCS_DEFAULTS.keys()) & set(config.keys()): + config[k].setdefault('scope', scope) try: - return Patroni.get_dcs(scope, dcs_config) + return Patroni.get_dcs(scope, config) except PatroniException as e: raise PatroniCtlException(str(e)) diff --git a/tests/test_ctl.py b/tests/test_ctl.py index 8586c7d1..2e8c16ef 100644 --- a/tests/test_ctl.py +++ b/tests/test_ctl.py @@ -6,7 +6,7 @@ import unittest from click.testing import CliRunner from mock import patch, Mock -from patroni.ctl import ctl, members, store_config, load_config, output_members, post_patroni, get_dcs, \ +from patroni.ctl import ctl, members, store_config, load_config, output_members, post_patroni, get_dcs, parse_dcs, \ wait_for_leader, get_all_members, get_any_member, get_cursor, query_member, configure, PatroniCtlException from patroni.etcd import Etcd, Client from psycopg2 import OperationalError @@ -40,6 +40,8 @@ def test_rw_config(): load_config(CONFIG_FILE_PATH, None) load_config(CONFIG_FILE_PATH, '0.0.0.0') + store_config({'dcs_api': None}, CONFIG_FILE_PATH) + load_config(CONFIG_FILE_PATH, None) @patch('patroni.ctl.load_config', Mock(return_value={'dcs': {'etcd': {'host': 'localhost:4001'}}})) class TestCtl(unittest.TestCase): @@ -62,6 +64,17 @@ class TestCtl(unittest.TestCase): self.assertIsNotNone(get_cursor(get_cluster_initialized_with_leader(), role='any')) + def test_parse_dcs(self): + assert parse_dcs(None) is None + assert parse_dcs('localhost') == {'etcd': {'host': 'localhost:4001'}} + assert parse_dcs('') == {'etcd': {'host': 'localhost:4001'}} + assert parse_dcs('localhost:8500') == {'consul': {'host': 'localhost:8500'}} + assert parse_dcs('zookeeper://localhost') == {'zookeeper': {'hosts': ['localhost:2181']}} + assert parse_dcs('exhibitor://dummy') == {'zookeeper': {'exhibitor': {'hosts': ['dummy'], 'port': 8181}}} + assert parse_dcs('consul://localhost') == {'consul': {'host': 'localhost:8500'}} + self.assertRaises(PatroniCtlException, parse_dcs, 'invalid://test') + + def test_output_members(self): cluster = get_cluster_initialized_with_leader() self.assertIsNone(output_members(cluster, name='abc', fmt='pretty')) @@ -72,67 +85,70 @@ class TestCtl(unittest.TestCase): @patch('patroni.etcd.Etcd.get_etcd_client', Mock(return_value=None)) @patch('patroni.ctl.post_patroni', Mock(return_value=MockResponse())) def test_failover(self): - result = self.runner.invoke(ctl, ['failover', 'dummy'], input='''leader\nother\n\ny''') - assert 'leader' in result.output - - result = self.runner.invoke(ctl, ['failover', 'dummy'], input='''leader\nother\n2100-01-01T12:23:00\ny''') - assert result.exit_code == 0 - - result = self.runner.invoke(ctl, ['failover', 'dummy'], input='''leader\nother\n2030-01-01T12:23:00\ny''') - assert result.exit_code == 0 - - # Aborting failover,as we anser NO to the confirmation - result = self.runner.invoke(ctl, ['failover', 'dummy'], input='''leader\nother\n\nN''') - assert result.exit_code == 1 - - # Target and source are equal - result = self.runner.invoke(ctl, ['failover', 'dummy'], input='''leader\nleader\n\ny''') - assert result.exit_code == 1 - - # Reality is not part of this cluster - result = self.runner.invoke(ctl, ['failover', 'dummy'], input='''leader\nReality\n\ny''') - assert result.exit_code == 1 - - result = self.runner.invoke(ctl, ['failover', 'dummy', '--force']) - assert 'Member' in result.output - - result = self.runner.invoke(ctl, ['failover', 'dummy', '--force', '--scheduled', '2015-01-01T12:00:00+01:00']) - assert result.exit_code == 0 - - # Invalid timestamp - result = self.runner.invoke(ctl, ['failover', 'dummy', '--force', '--scheduled', 'invalid']) - assert result.exit_code != 0 - - # Invalid timestamp - result = self.runner.invoke(ctl, ['failover', 'dummy', '--force', '--scheduled', '2115-02-30T12:00:00+01:00']) - assert result.exit_code != 0 - - # Specifying wrong leader - result = self.runner.invoke(ctl, ['failover', 'dummy'], input='dummy') - assert result.exit_code == 1 - - with patch('patroni.etcd.Etcd.get_cluster', Mock(return_value=get_cluster_initialized_with_only_leader())): - # No members available + with patch('patroni.ctl.get_dcs', Mock(return_value=self.e)): result = self.runner.invoke(ctl, ['failover', 'dummy'], input='''leader\nother\n\ny''') + assert 'leader' in result.output + + result = self.runner.invoke(ctl, ['failover', 'dummy'], input='''leader\nother\n2100-01-01T12:23:00\ny''') + assert result.exit_code == 0 + + result = self.runner.invoke(ctl, ['failover', 'dummy'], input='''leader\nother\n2030-01-01T12:23:00\ny''') + assert result.exit_code == 0 + + # Aborting failover,as we anser NO to the confirmation + result = self.runner.invoke(ctl, ['failover', 'dummy'], input='''leader\nother\n\nN''') assert result.exit_code == 1 - with patch('patroni.etcd.Etcd.get_cluster', Mock(return_value=get_cluster_initialized_without_leader())): - # No master available - result = self.runner.invoke(ctl, ['failover', 'dummy'], input='''leader\nother\n\ny''') + # Target and source are equal + result = self.runner.invoke(ctl, ['failover', 'dummy'], input='''leader\nleader\n\ny''') assert result.exit_code == 1 - with patch('patroni.ctl.post_patroni', Mock(side_effect=Exception)): - # Non-responding patroni - result = self.runner.invoke(ctl, ['failover', 'dummy'], input='''leader\nother\n\ny''') - assert 'falling back to DCS' in result.output + # Reality is not part of this cluster + result = self.runner.invoke(ctl, ['failover', 'dummy'], input='''leader\nReality\n\ny''') + assert result.exit_code == 1 - with patch('patroni.ctl.post_patroni') as mocked: - mocked.return_value.status_code = 500 - result = self.runner.invoke(ctl, ['failover', 'dummy'], input='''leader\nother\n\ny''') - assert 'Failover failed' in result.output + result = self.runner.invoke(ctl, ['failover', 'dummy', '--force']) + assert 'Member' in result.output + + result = self.runner.invoke(ctl, ['failover', 'dummy', '--force', '--scheduled', '2015-01-01T12:00:00+01:00']) + assert result.exit_code == 0 + + # Invalid timestamp + result = self.runner.invoke(ctl, ['failover', 'dummy', '--force', '--scheduled', 'invalid']) + assert result.exit_code != 0 + + # Invalid timestamp + result = self.runner.invoke(ctl, ['failover', 'dummy', '--force', '--scheduled', '2115-02-30T12:00:00+01:00']) + assert result.exit_code != 0 + + # Specifying wrong leader + result = self.runner.invoke(ctl, ['failover', 'dummy'], input='dummy') + assert result.exit_code == 1 + + with patch('patroni.etcd.Etcd.get_cluster', Mock(return_value=get_cluster_initialized_with_only_leader())): + # No members available + result = self.runner.invoke(ctl, ['failover', 'dummy'], input='''leader\nother\n\ny''') + assert result.exit_code == 1 + + with patch('patroni.etcd.Etcd.get_cluster', Mock(return_value=get_cluster_initialized_without_leader())): + # No master available + result = self.runner.invoke(ctl, ['failover', 'dummy'], input='''leader\nother\n\ny''') + assert result.exit_code == 1 + + with patch('patroni.ctl.post_patroni', Mock(side_effect=Exception)): + # Non-responding patroni + result = self.runner.invoke(ctl, ['failover', 'dummy'], input='''leader\nother\n\ny''') + assert 'falling back to DCS' in result.output + + with patch('patroni.ctl.post_patroni') as mocked: + mocked.return_value.status_code = 500 + result = self.runner.invoke(ctl, ['failover', 'dummy'], input='''leader\nother\n\ny''') + assert 'Failover failed' in result.output def test_get_dcs(self): - self.assertRaises(PatroniCtlException, get_dcs, {'dcs': {'dummy': {}}}, 'dummy') + self.assertRaises(PatroniCtlException, get_dcs, {'dummy': {}}, 'dummy') + with patch('patroni.Patroni.get_dcs', Mock(return_value=self.e)): + assert get_dcs({'etcd': {'host':'none'}}, 'dummy').client_path('') == '/service/test/' @patch('psycopg2.connect', psycopg2_connect) @patch('patroni.ctl.query_member', Mock(return_value=([['mock column']], None))) @@ -206,24 +222,26 @@ class TestCtl(unittest.TestCase): @patch('patroni.etcd.Etcd.get_etcd_client', Mock(return_value=None)) @patch('requests.post', requests_get) def test_restart_reinit(self): - result = self.runner.invoke(ctl, ['restart', 'alpha'], input='y') - assert result.exit_code == 0 - - result = self.runner.invoke(ctl, ['reinit', 'alpha'], input='y') - assert result.exit_code == 1 - - # Aborted restart - result = self.runner.invoke(ctl, ['restart', 'alpha'], input='N') - assert result.exit_code == 1 - - # Not a member - result = self.runner.invoke(ctl, ['restart', 'alpha', 'dummy', '--any'], input='y') - assert result.exit_code == 1 - - with patch('requests.post', Mock(return_value=MockResponse())): + with patch('patroni.ctl.get_dcs', Mock(return_value=self.e)): result = self.runner.invoke(ctl, ['restart', 'alpha'], input='y') + assert 'restart failed for' in result.output assert result.exit_code == 0 + result = self.runner.invoke(ctl, ['reinit', 'alpha'], input='y') + assert result.exit_code == 1 + + # Aborted restart + result = self.runner.invoke(ctl, ['restart', 'alpha'], input='N') + assert result.exit_code == 1 + + # Not a member + result = self.runner.invoke(ctl, ['restart', 'alpha', 'dummy', '--any'], input='y') + assert result.exit_code == 1 + + with patch('requests.post', Mock(return_value=MockResponse())): + result = self.runner.invoke(ctl, ['restart', 'alpha'], input='y') + assert result.exit_code == 0 + @patch('patroni.etcd.Etcd.get_cluster', Mock(return_value=get_cluster_initialized_with_leader())) @patch.object(etcd.Client, 'delete', Mock(side_effect=etcd.EtcdException)) def test_remove(self): @@ -286,8 +304,10 @@ class TestCtl(unittest.TestCase): @patch('patroni.etcd.Etcd.get_cluster', Mock(return_value=get_cluster_initialized_with_leader())) @patch('patroni.etcd.Etcd.get_etcd_client', Mock(return_value=None)) def test_members(self): - result = self.runner.invoke(members, ['alpha']) - assert result.exit_code == 0 + with patch('patroni.ctl.get_dcs', Mock(return_value=self.e)): + result = self.runner.invoke(members, ['alpha']) + assert '127.0.0.1' in result.output + assert result.exit_code == 0 def test_configure(self): result = self.runner.invoke(configure, ['--dcs', 'abc', '-c', 'dummy', '-n', 'bla'])