Repository: zalando/patroni
Pull request #2678 (2023-05-15 08:31:39)

Validate-config fixes
- fix --validate-config not to error out if bin_dir is an empty string in the yaml config
- mention bin_dir optionality in the docs
- validate bin_dir even if it is not in the yaml config (add optional default value for Optional config params in validator)
- make rewind user optional

coveralls: ## Pull Request Test Coverage Report for [Build 4978239619](https://coveralls.io/builds/59941121)

* **11** of **11**   **(100.0%)**  changed or added relevant lines in **1** file are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage increased (+**0.02%**) to **99.849%**

---



|  Totals | [![Coverage Status](https://coveralls.io/builds/59941121/badge)](https://coveralls.io/builds/59941121) |
| :-- | --: |
| Change from base [Build 4959503774](https://coveralls.io/builds/59900387): |  0.02% |
| Covered Lines: | 12568 |
| Relevant Lines: | 12587 |

---
##### 💛  - [Coveralls](https://coveralls.io)

CyberDem0n: :+1:
hughcapet: 👍 

Patch:
diff --git a/docs/yaml_configuration.rst b/docs/yaml_configuration.rst
index a5b32a20..51f769ca 100644
--- a/docs/yaml_configuration.rst
+++ b/docs/yaml_configuration.rst
@@ -262,7 +262,7 @@ PostgreSQL
       own config item. See :ref:`custom replica creation methods documentation <custom_replica_creation>` for further explanation.
    -  **data\_dir**: The location of the Postgres data directory, either :ref:`existing <existing_data>` or to be initialized by Patroni.
    -  **config\_dir**: The location of the Postgres configuration directory, defaults to the data directory. Must be writable by Patroni.
-   -  **bin\_dir**: Path to PostgreSQL binaries (pg_ctl, pg_rewind, pg_basebackup, postgres). The default value is an empty string meaning that PATH environment variable will be used to find the executables.
+   -  **bin\_dir**: (optional) Path to PostgreSQL binaries (pg_ctl, pg_rewind, pg_basebackup, postgres). If not provided or is an empty string, PATH environment variable will be used to find the executables.
    -  **listen**: IP address + port that Postgres listens to; must be accessible from other nodes in the cluster, if you're using streaming replication. Multiple comma-separated addresses are permitted, as long as the port component is appended after to the last one with a colon, i.e. ``listen: 127.0.0.1,127.0.0.2:5432``. Patroni will use the first address from this list to establish local connections to the PostgreSQL node.
    -  **use\_unix\_socket**: specifies that Patroni should prefer to use unix sockets to connect to the cluster. Default value is ``false``. If ``unix_socket_directories`` is defined, Patroni will use the first suitable value from it to connect to the cluster and fallback to tcp if nothing is suitable. If ``unix_socket_directories`` is not specified in ``postgresql.parameters``, Patroni will assume that the default value should be used and omit ``host`` from the connection parameters.
    -  **use\_unix\_socket\_repl**: specifies that Patroni should prefer to use unix sockets for replication user cluster connection. Default value is ``false``. If ``unix_socket_directories`` is defined, Patroni will use the first suitable value from it to connect to the cluster and fallback to tcp if nothing is suitable. If ``unix_socket_directories`` is not specified in ``postgresql.parameters``, Patroni will assume that the default value should be used and omit ``host`` from the connection parameters.
diff --git a/patroni/validator.py b/patroni/validator.py
index e867ce58..3d30ca2c 100644
--- a/patroni/validator.py
+++ b/patroni/validator.py
@@ -341,14 +341,17 @@ class Optional(object):
     """Mark a configuration option as optional.
 
     :ivar name: name of the configuration option.
+    :ivar default: value to set if the configuration option is not explicitly provided
     """
 
-    def __init__(self, name: str) -> None:
+    def __init__(self, name: str, default: OptionalType[Any] = None) -> None:
         """Create an :class:`Optional` object.
 
         :param name: name of the configuration option.
+        :param default: value to set if the configuration option is not explicitly provided
         """
         self.name = name
+        self.default = default
 
 
 class Directory(object):
@@ -370,14 +373,27 @@ class Directory(object):
         self.contains = contains
         self.contains_executable = contains_executable
 
+    def _check_executables(self, path: OptionalType[str] = None) -> Iterator[Result]:
+        """Check that all executables from contains_executable list exist within the given directory or within PATH.
+
+        :param path: optional path to the base directory against which executables will be validated.
+                     If not provided, check within PATH.
+        :rtype: Iterator[:class:`Result`] objects with the error message containing the name of the executable,
+                if any check fails.
+        """
+        for program in self.contains_executable or []:
+            if not shutil.which(program, path=path):
+                yield Result(False, f"does not contain '{program}' in '{(path or '$PATH')}'")
+
     def validate(self, name: str) -> Iterator[Result]:
         """Check if the expected paths and executables can be found under *name* directory.
 
         :param name: path to the base directory against which paths and executables will be validated.
+                     Check against PATH if name is not provided.
         :rtype: Iterator[:class:`Result`] objects with the error message related to the failure, if any check fails.
         """
         if not name:
-            yield Result(False, "is an empty string")
+            yield from self._check_executables()
         elif not os.path.exists(name):
             yield Result(False, "Directory '{}' does not exist.".format(name))
         elif not os.path.isdir(name):
@@ -387,10 +403,7 @@ class Directory(object):
                 for path in self.contains:
                     if not os.path.exists(os.path.join(name, path)):
                         yield Result(False, "'{}' does not contain '{}'".format(name, path))
-            if self.contains_executable:
-                for program in self.contains_executable:
-                    if not shutil.which(program, path=name):
-                        yield Result(False, "'{}' does not contain '{}'".format(name, program))
+            yield from self._check_executables(path=name)
 
 
 class Schema(object):
@@ -472,8 +485,7 @@ class Schema(object):
         * It must contain a ``bind.host`` entry which value should be valid as per function ``validate_host``;
         * It must contain a ``bind.port`` entry which value should be an :class:`int` instance;
         * It must contain a ``aliases`` entry which value should be a :class:`list` of :class:`str` instances;
-        * It may optionally contain a ``data_directory`` entry. If not given it will assume the value
-            ``/var/lib/myapp``;
+        * It may optionally contain a ``data_directory`` entry, with a value which should be a string;
         * It must contain at least one of ``log_to_file`` or ``log_to_db``, with a value which should be a
             :class:`bool` instance;
         * It must contain a ``version`` entry which value should be either an :class:`int` or a :class:`float`
@@ -583,9 +595,11 @@ class Schema(object):
             for d in self._data_key(key):
                 if d not in self.data and not isinstance(key, Optional):
                     yield Result(False, "is not defined.", path=d)
-                elif d not in self.data and isinstance(key, Optional):
+                elif d not in self.data and isinstance(key, Optional) and key.default is None:
                     continue
                 else:
+                    if d not in self.data and isinstance(key, Optional):
+                        self.data[d] = key.default
                     validator = self.validator[key]
                     if isinstance(key, Or) and isinstance(self.validator[key], Case):
                         validator = self.validator[key]._schema[d]
@@ -807,11 +821,11 @@ schema = Schema({
         "authentication": {
             "replication": userattributes,
             "superuser": userattributes,
-            "rewind": userattributes
+            Optional("rewind"): userattributes
         },
         "data_dir": validate_data_dir,
-        Optional("bin_dir"): Directory(contains_executable=["pg_ctl", "initdb", "pg_controldata", "pg_basebackup",
-                                                            "postgres", "pg_isready"]),
+        Optional("bin_dir", ""): Directory(contains_executable=["pg_ctl", "initdb", "pg_controldata", "pg_basebackup",
+                                                                "postgres", "pg_isready"]),
         Optional("parameters"): {
             Optional("unix_socket_directories"): str
         },


Test patch:
diff --git a/tests/test_validator.py b/tests/test_validator.py
index df017500..87ea78b7 100644
--- a/tests/test_validator.py
+++ b/tests/test_validator.py
@@ -7,7 +7,7 @@ import unittest
 from io import StringIO
 from mock import Mock, patch, mock_open
 from patroni.dcs import dcs_modules
-from patroni.validator import schema
+from patroni.validator import schema, Directory, Schema
 
 available_dcs = [m.split(".")[-1] for m in dcs_modules()]
 config = {
@@ -92,6 +92,16 @@ config = {
     }
 }
 
+config_2 = {
+    "some_dir": "very_interesting_dir"
+}
+
+schema2 = Schema({
+    "some_dir": Directory(contains=["very_interesting_subdir", "another_interesting_subdir"])
+})
+
+required_binaries = ["pg_ctl", "initdb", "pg_controldata", "pg_basebackup", "postgres", "pg_isready"]
+
 directories = []
 files = []
 binaries = []
@@ -190,6 +200,14 @@ class TestValidator(unittest.TestCase):
         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))
 
+    def test_bin_dir_is_empty_string_excutables_in_path(self, mock_out, mock_err):
+        binaries.extend(required_binaries)
+        c = copy.deepcopy(config)
+        c["postgresql"]["bin_dir"] = ""
+        errors = schema(c)
+        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"))
     def test_data_dir_contains_pg_version(self, mock_out, mock_err):
         directories.append(config["postgresql"]["data_dir"])
@@ -197,14 +215,11 @@ class TestValidator(unittest.TestCase):
         directories.append(os.path.join(config["postgresql"]["data_dir"], "pg_wal"))
         files.append(os.path.join(config["postgresql"]["data_dir"], "global", "pg_control"))
         files.append(os.path.join(config["postgresql"]["data_dir"], "PG_VERSION"))
-        binaries.append(os.path.join(config["postgresql"]["bin_dir"], "pg_ctl"))
-        binaries.append(os.path.join(config["postgresql"]["bin_dir"], "initdb"))
-        binaries.append(os.path.join(config["postgresql"]["bin_dir"], "pg_controldata"))
-        binaries.append(os.path.join(config["postgresql"]["bin_dir"], "pg_basebackup"))
-        binaries.append(os.path.join(config["postgresql"]["bin_dir"], "postgres"))
-        binaries.append(os.path.join(config["postgresql"]["bin_dir"], "pg_isready"))
+        binaries.extend(required_binaries)
+        c = copy.deepcopy(config)
+        c["postgresql"]["bin_dir"] = ""  # to cover postgres --version call from PATH
         with patch('patroni.validator.open', mock_open(read_data='12')):
-            errors = schema(config)
+            errors = schema(c)
         output = "\n".join(errors)
         self.assertEqual(['raft.bind_addr', 'raft.self_addr'], parse_output(output))
 
@@ -215,10 +230,10 @@ class TestValidator(unittest.TestCase):
         directories.append(os.path.join(config["postgresql"]["data_dir"], "pg_wal"))
         files.append(os.path.join(config["postgresql"]["data_dir"], "global", "pg_control"))
         files.append(os.path.join(config["postgresql"]["data_dir"], "PG_VERSION"))
+        binaries.extend([os.path.join(config["postgresql"]["bin_dir"], i) for i in required_binaries])
         c = copy.deepcopy(config)
         c["etcd"]["hosts"] = []
         c["postgresql"]["listen"] = '127.0.0.2,*:543'
-        del c["postgresql"]["bin_dir"]
         with patch('patroni.validator.open', mock_open(read_data='11')):
             errors = schema(c)
         output = "\n".join(errors)
@@ -227,18 +242,19 @@ class TestValidator(unittest.TestCase):
 
     @patch('subprocess.check_output', Mock(return_value=b"postgres (PostgreSQL) 12.1"))
     def test_pg_wal_doesnt_exist(self, mock_out, mock_err):
+        binaries.extend([os.path.join(config["postgresql"]["bin_dir"], i) for i in required_binaries])
         directories.append(config["postgresql"]["data_dir"])
         directories.append(config["postgresql"]["bin_dir"])
         files.append(os.path.join(config["postgresql"]["data_dir"], "global", "pg_control"))
         files.append(os.path.join(config["postgresql"]["data_dir"], "PG_VERSION"))
         c = copy.deepcopy(config)
-        del c["postgresql"]["bin_dir"]
         with patch('patroni.validator.open', mock_open(read_data='11')):
             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):
+        binaries.extend(required_binaries)
         directories.append(config["postgresql"]["data_dir"])
         directories.append(config["postgresql"]["bin_dir"])
         c = copy.deepcopy(config)
@@ -248,5 +264,11 @@ class TestValidator(unittest.TestCase):
         c["postgresql"]["bin_dir"] = ""
         errors = schema(c)
         output = "\n".join(errors)
-        self.assertEqual(['kubernetes', 'postgresql.bin_dir', 'postgresql.data_dir',
+        self.assertEqual(['kubernetes', 'postgresql.data_dir',
                           'postgresql.pg_hba', 'raft.bind_addr', 'raft.self_addr'], parse_output(output))
+
+    def test_directory_contains(self, mock_out, mock_err):
+        directories.extend([config_2["some_dir"], os.path.join(config_2["some_dir"], "very_interesting_subdir")])
+        errors = schema2(config_2)
+        output = "\n".join(errors)
+        self.assertEqual(['some_dir'], parse_output(output))