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 ` for further explanation. - **data\_dir**: The location of the Postgres data directory, either :ref:`existing ` 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 }, 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))