From 25371478101ca233f52fb407fda2764fb8be0489 Mon Sep 17 00:00:00 2001 From: Henning Jacobs Date: Tue, 12 Jun 2018 14:08:38 +0200 Subject: [PATCH] #694 handle configuration error (#695) It is possible to change a lot of parameters in runtime (including `restapi.listen`) by updating Patroni config file and sending SIGHUP to Patroni process. If something was misconfigured it was throwing a weird exception and breaking `restapi` thread. This PR improves friendliness of error message and avoids breaking of `restapi`. --- patroni/api.py | 37 ++++++++++++++++++++++++------------- tests/test_api.py | 18 ++++++++++++++---- 2 files changed, 38 insertions(+), 17 deletions(-) diff --git a/patroni/api.py b/patroni/api.py index 4b7d835b..de121b2e 100644 --- a/patroni/api.py +++ b/patroni/api.py @@ -8,7 +8,8 @@ import dateutil.parser import datetime from patroni.postgresql import PostgresConnectionException, PostgresException, Postgresql -from patroni.utils import deep_compare, parse_bool, patch_config, Retry, RetryFailedError, parse_int, tzutc +from patroni.utils import deep_compare, parse_bool, patch_config, Retry, \ + RetryFailedError, parse_int, split_host_port, tzutc from six.moves.BaseHTTPServer import BaseHTTPRequestHandler, HTTPServer from six.moves.socketserver import ThreadingMixIn from threading import Thread @@ -473,6 +474,7 @@ class RestApiServer(ThreadingMixIn, HTTPServer, Thread): def __init__(self, patroni, config): self.patroni = patroni + self.__listen = None self.__initialize(config) self.__set_config_parameters(config) self.daemon = True @@ -507,18 +509,25 @@ class RestApiServer(ThreadingMixIn, HTTPServer, Thread): def __get_ssl_options(config): return {option: config[option] for option in ['certfile', 'keyfile'] if option in config} - def __set_connection_string(self, connect_address): - self.connection_string = '{0}://{1}/patroni'.format(self.__protocol, connect_address or self.__listen) - def __set_config_parameters(self, config): self.__auth_key = base64.b64encode(config['auth'].encode('utf-8')).decode('utf-8') if 'auth' in config else None - self.__set_connection_string(config.get('connect_address')) + self.connection_string = '{0}://{1}/patroni'.format(self.__protocol, + config.get('connect_address') or self.__listen) def __initialize(self, config): - self.__ssl_options = self.__get_ssl_options(config) + try: + host, port = split_host_port(config['listen'], None) + except Exception: + raise ValueError('Invalid "restapi" config: expected : for "listen", but got "{0}"' + .format(config['listen'])) + + if self.__listen is not None: # changing config in runtime + self.shutdown() + self.__listen = config['listen'] - host, port = config['listen'].rsplit(':', 1) - HTTPServer.__init__(self, (host, int(port)), RestApiHandler) + self.__ssl_options = self.__get_ssl_options(config) + + HTTPServer.__init__(self, (host, port), RestApiHandler) Thread.__init__(self, target=self.serve_forever) self._set_fd_cloexec(self.socket) @@ -530,11 +539,13 @@ class RestApiServer(ThreadingMixIn, HTTPServer, Thread): import ssl self.socket = ssl.wrap_socket(self.socket, server_side=True, **self.__ssl_options) self.__protocol = 'https' - self.__set_connection_string(config.get('connect_address')) + return True def reload_config(self, config): - self.__set_config_parameters(config) - if self.__listen != config['listen'] or self.__ssl_options != self.__get_ssl_options(config): - self.shutdown() - self.__initialize(config) + if 'listen' not in config: # changing config in runtime + raise ValueError('Can not find "restapi.listen" config') + + elif (self.__listen != config['listen'] or self.__ssl_options != self.__get_ssl_options(config)) \ + and self.__initialize(config): self.start() + self.__set_config_parameters(config) diff --git a/tests/test_api.py b/tests/test_api.py index d4f0d97f..29bcabb4 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -123,16 +123,14 @@ class MockRequest(object): class MockRestApiServer(RestApiServer): - def __init__(self, Handler, request): + def __init__(self, Handler, request, config=None): self.socket = 0 self.serve_forever = Mock() BaseHTTPServer.HTTPServer.__init__ = Mock() MockRestApiServer._BaseServer__is_shut_down = Mock() MockRestApiServer._BaseServer__shutdown_request = True - config = {'listen': '127.0.0.1:8008', 'auth': 'test:test'} + config = config or {'listen': '127.0.0.1:8008', 'auth': 'test:test', 'certfile': 'dumb'} super(MockRestApiServer, self).__init__(MockPatroni(), config) - config['certfile'] = 'dumb' - self.reload_config(config) Handler(MockRequest(request), ('0.0.0.0', 8080), self) @@ -380,3 +378,15 @@ class TestRestApiHandler(unittest.TestCase): post = 'POST /failover HTTP/1.0' + self._authorization + '\nContent-Length: ' MockRestApiServer(RestApiHandler, post + '14\n\n{"leader":"1"}') MockRestApiServer(RestApiHandler, post + '37\n\n{"candidate":"2","scheduled_at": "1"}') + + +@patch('ssl.wrap_socket', Mock(return_value=0)) +class TestRestApiServer(unittest.TestCase): + + def test_reload_config(self): + bad_config = {'listen': 'foo'} + self.assertRaises(ValueError, MockRestApiServer, None, '', bad_config) + srv = MockRestApiServer(lambda a1, a2, a3: None, '') + self.assertRaises(ValueError, srv.reload_config, bad_config) + self.assertRaises(ValueError, srv.reload_config, {}) + srv.reload_config({'listen': '127.0.0.2:8008'})