Repository: zalando/patroni (python)

Issue:
'patroni --validate-config' exists 0 upon invalid configuration
`patroni --validate-config` exists 0 upon invalid configuration:
```
$ patroni --validate-config postgres0.yml
restapi.connect_address 127.0.0.1:8008 didn't pass validation: 'must not contain "127.0.0.1", "0.0.0.0", "*", "::1", "localhost"'
etcd.host 127.0.0.1:2379 didn't pass validation: '127.0.0.1:2379 is not reachable'
postgresql.connect_address 127.0.0.1:5432 didn't pass validation: 'must not contain "127.0.0.1", "0.0.0.0", "*", "::1", "localhost"'
$ echo $?
0
```
This is misleading since one cannot rely on exit code to detect an error; also errors are printed to stdout whereas stderr would be more appropriate.
I'd suggest to:
* exit 1 upon invalid configuration
* print errors to stderr

and I'm willing to prepare a PR for this.

**Environment**
- Patroni version: master at b8a6387236d619345c2f72f2f8e858dc7a2bd791
- PostgreSQL version: n/a
- DCS (and its version): n/a

Pull request:
Improvements to 'patroni --validate-config'
Fix #2345 

Patch:
diff --git a/docs/SETTINGS.rst b/docs/SETTINGS.rst
index 9712f445..205f80dd 100644
--- a/docs/SETTINGS.rst
+++ b/docs/SETTINGS.rst
@@ -131,7 +131,7 @@ Most of the parameters are optional, but you have to specify one of the **host**
 -  **checks**: (optional) list of Consul health checks used for the session. By default an empty list is used.
 -  **register\_service**: (optional) whether or not to register a service with the name defined by the scope parameter and the tag master, replica or standby-leader depending on the node's role. Defaults to **false**.
 -  **service\_tags**: (optional) additional static tags to add to the Consul service apart from the role (``master``/``replica``/``standby-leader``).  By default an empty list is used.
--  **service\_check\_interval**: (optional) how often to perform health check against registered url.
+-  **service\_check\_interval**: (optional) how often to perform health check against registered url. Defaults to '5s'.
 -  **service\_check\_tls\_server\_name**: (optional) overide SNI host when connecting via TLS, see also `consul agent check API reference <https://www.consul.io/api-docs/agent/check#tlsservername>`__.
 
 The ``token`` needs to have the following ACL permissions:
diff --git a/docs/replica_bootstrap.rst b/docs/replica_bootstrap.rst
index a9ed7848..1524f7f2 100644
--- a/docs/replica_bootstrap.rst
+++ b/docs/replica_bootstrap.rst
@@ -212,4 +212,9 @@ in a patroni configuration:
 Note, that these options will be applied only once during cluster bootstrap,
 and the only way to change them afterwards is through DCS.
 
+Patroni expects to find `postgresql.conf` or `postgresql.conf.backup` in PGDATA
+of the remote master and will not start if it does not find it after a
+basebackup. If the remote master keeps its `postgresql.conf` elsewhere, it is
+your responsibility to copy it to PGDATA.
+
 If you use replication slots on the standby cluster, you must also create the corresponding replication slot on the primary cluster.  It will not be done automatically by the standby cluster implementation.  You can use Patroni's permanent replication slots feature on the primary cluster to maintain a replication slot with the same name as ``primary_slot_name``, or its default value if ``primary_slot_name`` is not provided.
diff --git a/patroni/config.py b/patroni/config.py
index 5996f68e..0ae9726a 100644
--- a/patroni/config.py
+++ b/patroni/config.py
@@ -102,9 +102,9 @@ class Config(object):
             config_env = os.environ.pop(self.PATRONI_CONFIG_VARIABLE, None)
             self._local_configuration = config_env and yaml.safe_load(config_env) or self.__environment_configuration
         if validator:
-            error = validator(self._local_configuration)
-            if error:
-                raise ConfigParseError(error)
+            errors = validator(self._local_configuration)
+            if errors:
+                raise ConfigParseError("\n".join(errors))
 
         self.__effective_configuration = self._build_effective_configuration({}, self._local_configuration)
         self._data_dir = self.__effective_configuration.get('postgresql', {}).get('data_dir', "")
diff --git a/patroni/daemon.py b/patroni/daemon.py
index 5bb99990..ca118acf 100644
--- a/patroni/daemon.py
+++ b/patroni/daemon.py
@@ -1,3 +1,5 @@
+from __future__ import print_function
+
 import abc
 import os
 import signal
@@ -83,16 +85,18 @@ def abstract_main(cls, validator=None):
                         help='Patroni may also read the configuration from the {0} environment variable'
                         .format(Config.PATRONI_CONFIG_VARIABLE))
     args = parser.parse_args()
+    validate_config = validator and args.validate_config
     try:
-        if validator and args.validate_config:
+        if validate_config:
             Config(args.configfile, validator=validator)
             sys.exit()
 
         config = Config(args.configfile)
     except ConfigParseError as e:
         if e.value:
-            print(e.value)
-        parser.print_help()
+            print(e.value, file=sys.stderr)
+        if not validate_config:
+            parser.print_help()
         sys.exit(1)
 
     controller = cls(config)
diff --git a/patroni/validator.py b/patroni/validator.py
index 6d2a060c..595d4355 100644
--- a/patroni/validator.py
+++ b/patroni/validator.py
@@ -178,9 +178,11 @@ class Schema(object):
         self.validator = validator
 
     def __call__(self, data):
+        errors = []
         for i in self.validate(data):
             if not i.status:
-                print(i)
+                errors.append(str(i))
+        return errors
 
     def validate(self, data):
         self.data = data


Test patch:
diff --git a/tests/test_validator.py b/tests/test_validator.py
index a04403ce..830b415e 100644
--- a/tests/test_validator.py
+++ b/tests/test_validator.py
@@ -141,14 +141,14 @@ class TestValidator(unittest.TestCase):
         del directories[:]
 
     def test_empty_config(self, mock_out, mock_err):
-        schema({})
-        output = mock_out.getvalue()
+        errors = schema({})
+        output = "\n".join(errors)
         expected = list(sorted(['name', 'postgresql', 'restapi', 'scope'] + available_dcs))
         self.assertEqual(expected, parse_output(output))
 
     def test_complete_config(self, mock_out, mock_err):
-        schema(config)
-        output = mock_out.getvalue()
+        errors = schema(config)
+        output = "\n".join(errors)
         self.assertEqual(['postgresql.bin_dir', 'raft.bind_addr', 'raft.self_addr'], parse_output(output))
 
     def test_bin_dir_is_file(self, mock_out, mock_err):
@@ -158,8 +158,8 @@ class TestValidator(unittest.TestCase):
         c["restapi"]["connect_address"] = 'False:blabla'
         c["etcd"]["hosts"] = ["127.0.0.1:2379", "1244.0.0.1:2379", "127.0.0.1:invalidport"]
         c["kubernetes"]["pod_ip"] = "127.0.0.1111"
-        schema(c)
-        output = mock_out.getvalue()
+        errors = schema(c)
+        output = "\n".join(errors)
         self.assertEqual(['etcd.hosts.1', 'etcd.hosts.2', 'kubernetes.pod_ip', 'postgresql.bin_dir',
                           'postgresql.data_dir', 'raft.bind_addr', 'raft.self_addr',
                           'restapi.connect_address'], parse_output(output))
@@ -176,8 +176,8 @@ class TestValidator(unittest.TestCase):
         c["etcd"]["host"] = "127.0.0.1:237"
         c["postgresql"]["listen"] = "127.0.0.1:5432"
         with patch('patroni.validator.open', mock_open(read_data='9')):
-            schema(c)
-        output = mock_out.getvalue()
+            errors = schema(c)
+        output = "\n".join(errors)
         self.assertEqual(['consul.host', 'etcd.host', 'postgresql.bin_dir', 'postgresql.data_dir', 'postgresql.listen',
                           'raft.bind_addr', 'raft.self_addr', 'restapi.connect_address'], parse_output(output))
 
@@ -195,8 +195,8 @@ class TestValidator(unittest.TestCase):
         files.append(os.path.join(config["postgresql"]["bin_dir"], "postgres"))
         files.append(os.path.join(config["postgresql"]["bin_dir"], "pg_isready"))
         with patch('patroni.validator.open', mock_open(read_data='12')):
-            schema(config)
-        output = mock_out.getvalue()
+            errors = schema(config)
+        output = "\n".join(errors)
         self.assertEqual(['raft.bind_addr', 'raft.self_addr'], parse_output(output))
 
     @patch('subprocess.check_output', Mock(return_value=b"postgres (PostgreSQL) 12.1"))
@@ -210,8 +210,8 @@ class TestValidator(unittest.TestCase):
         c["etcd"]["hosts"] = []
         del c["postgresql"]["bin_dir"]
         with patch('patroni.validator.open', mock_open(read_data='11')):
-            schema(c)
-        output = mock_out.getvalue()
+            errors = schema(c)
+        output = "\n".join(errors)
         self.assertEqual(['etcd.hosts', 'postgresql.data_dir',
                           'raft.bind_addr', 'raft.self_addr'], parse_output(output))
 
@@ -224,8 +224,8 @@ class TestValidator(unittest.TestCase):
         c = copy.deepcopy(config)
         del c["postgresql"]["bin_dir"]
         with patch('patroni.validator.open', mock_open(read_data='11')):
-            schema(c)
-        output = mock_out.getvalue()
+            errors = schema(c)
+        output = "\n".join(errors)
         self.assertEqual(['postgresql.data_dir', 'raft.bind_addr', 'raft.self_addr'], parse_output(output))
 
     def test_data_dir_is_empty_string(self, mock_out, mock_err):
@@ -236,7 +236,7 @@ class TestValidator(unittest.TestCase):
         c["postgresql"]["pg_hba"] = ""
         c["postgresql"]["data_dir"] = ""
         c["postgresql"]["bin_dir"] = ""
-        schema(c)
-        output = mock_out.getvalue()
+        errors = schema(c)
+        output = "\n".join(errors)
         self.assertEqual(['kubernetes', 'postgresql.bin_dir', 'postgresql.data_dir',
                           'postgresql.pg_hba', 'raft.bind_addr', 'raft.self_addr'], parse_output(output))