#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

@@ -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 <HOST>:<PORT> 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)

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'})