From 56dba93c559bcf757eb3532f5d5bc1cbeced16cc Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Tue, 13 Aug 2024 08:11:28 +0200 Subject: [PATCH] Implement support of `log.mode`. (#3122) There was one oversight of #2781 - to influence external tools that Patroni could execute, we set global `umask` value based on permissions of the $PGDATA directory. As a result, it also influenced permissions of log files created by Patroni. To address the problem we implement two measures: 1. Make `log.mode` configurable. 2. If the value is not set - calculate permissions from the original value of the umask setting. --- docs/ENVIRONMENT.rst | 1 + docs/yaml_configuration.rst | 1 + patroni/config.py | 4 +-- patroni/file_perm.py | 14 ++++++++--- patroni/log.py | 49 +++++++++++++++++++++++++++++++------ patroni/validator.py | 1 + tests/test_config.py | 2 ++ tests/test_file_perm.py | 3 +++ tests/test_log.py | 5 +++- 9 files changed, 67 insertions(+), 13 deletions(-) diff --git a/docs/ENVIRONMENT.rst b/docs/ENVIRONMENT.rst index d6c40116..ce2a5df8 100644 --- a/docs/ENVIRONMENT.rst +++ b/docs/ENVIRONMENT.rst @@ -28,6 +28,7 @@ Log - **PATRONI\_LOG\_STATIC\_FIELDS**: add additional fields to the log. This option is only available when the log type is set to **json**. Example ``PATRONI_LOG_STATIC_FIELDS="{app: patroni}"`` - **PATRONI\_LOG\_MAX\_QUEUE\_SIZE**: Patroni is using two-step logging. Log records are written into the in-memory queue and there is a separate thread which pulls them from the queue and writes to stderr or file. The maximum size of the internal queue is limited by default by **1000** records, which is enough to keep logs for the past 1h20m. - **PATRONI\_LOG\_DIR**: Directory to write application logs to. The directory must exist and be writable by the user executing Patroni. If you set this env variable, the application will retain 4 25MB logs by default. You can tune those retention values with `PATRONI_LOG_FILE_NUM` and `PATRONI_LOG_FILE_SIZE` (see below). +- **PATRONI\_LOG\_MODE**: Permissions for log files (for example, ``0644``). If not specified, permissions will be set based on the current umask value. - **PATRONI\_LOG\_FILE\_NUM**: The number of application logs to retain. - **PATRONI\_LOG\_FILE\_SIZE**: Size of patroni.log file (in bytes) that triggers a log rolling. - **PATRONI\_LOG\_LOGGERS**: Redefine logging level per python module. Example ``PATRONI_LOG_LOGGERS="{patroni.postmaster: WARNING, urllib3: DEBUG}"`` diff --git a/docs/yaml_configuration.rst b/docs/yaml_configuration.rst index d90a43fc..d11e5368 100644 --- a/docs/yaml_configuration.rst +++ b/docs/yaml_configuration.rst @@ -29,6 +29,7 @@ Log - **static_fields**: add additional fields to the log. This option is only available when the log type is set to **json**. - **max\_queue\_size**: Patroni is using two-step logging. Log records are written into the in-memory queue and there is a separate thread which pulls them from the queue and writes to stderr or file. The maximum size of the internal queue is limited by default by **1000** records, which is enough to keep logs for the past 1h20m. - **dir**: Directory to write application logs to. The directory must exist and be writable by the user executing Patroni. If you set this value, the application will retain 4 25MB logs by default. You can tune those retention values with `file_num` and `file_size` (see below). +- **mode**: Permissions for log files (for example, ``0644``). If not specified, permissions will be set based on the current umask value. - **file\_num**: The number of application logs to retain. - **file\_size**: Size of patroni.log file (in bytes) that triggers a log rolling. - **loggers**: This section allows redefining logging level per python module diff --git a/patroni/config.py b/patroni/config.py index 6b46f4bb..19e50a35 100644 --- a/patroni/config.py +++ b/patroni/config.py @@ -535,7 +535,7 @@ class Config(object): _set_section_values('postgresql', ['listen', 'connect_address', 'proxy_address', 'config_dir', 'data_dir', 'pgpass', 'bin_dir']) _set_section_values('log', ['type', 'level', 'traceback_level', 'format', 'dateformat', 'static_fields', - 'max_queue_size', 'dir', 'file_size', 'file_num', 'loggers']) + 'max_queue_size', 'dir', 'mode', 'file_size', 'file_num', 'loggers']) _set_section_values('raft', ['data_dir', 'self_addr', 'partner_addrs', 'password', 'bind_addr']) for binary in ('pg_ctl', 'initdb', 'pg_controldata', 'pg_basebackup', 'postgres', 'pg_isready', 'pg_rewind'): @@ -552,7 +552,7 @@ class Config(object): ret[first][second] = value for first, params in (('restapi', ('request_queue_size',)), - ('log', ('max_queue_size', 'file_size', 'file_num'))): + ('log', ('max_queue_size', 'file_size', 'file_num', 'mode'))): for second in params: value = ret.get(first, {}).pop(second, None) if value: diff --git a/patroni/file_perm.py b/patroni/file_perm.py index ed9c4e67..62a13f80 100644 --- a/patroni/file_perm.py +++ b/patroni/file_perm.py @@ -39,19 +39,27 @@ class __FilePermissions: def __init__(self) -> None: """Create a :class:`__FilePermissions` object and set default permissions.""" self.__set_owner_permissions() - self.__set_umask() + self.__orig_umask = self.__set_umask() - def __set_umask(self) -> None: + def __set_umask(self) -> int: """Set umask value based on calculations. .. note:: Should only be called once either :meth:`__set_owner_permissions` or :meth:`__set_group_permissions` has been executed. + + :returns: the previous value of the umask or ``0022`` if umask call failed. """ try: - os.umask(self.__pg_mode_mask) + return os.umask(self.__pg_mode_mask) except Exception as e: logger.error('Can not set umask to %03o: %r', self.__pg_mode_mask, e) + return 0o22 + + @property + def orig_umask(self) -> int: + """Original umask value.""" + return self.__orig_umask def __set_owner_permissions(self) -> None: """Make directories/files accessible only by the owner.""" diff --git a/patroni/log.py b/patroni/log.py index 2f383b0c..2a896958 100644 --- a/patroni/log.py +++ b/patroni/log.py @@ -12,15 +12,49 @@ from logging.handlers import RotatingFileHandler from queue import Queue, Full from threading import Lock, Thread +from io import TextIOWrapper from typing import Any, Dict, List, Optional, Union, TYPE_CHECKING -from .utils import deep_compare +from .file_perm import pg_perm +from .utils import deep_compare, parse_int type_logformat = Union[List[Union[str, Dict[str, Any], Any]], str, Any] _LOGGER = logging.getLogger(__name__) +class PatroniFileHandler(RotatingFileHandler): + """Wrapper of :class:`RotatingFileHandler` to handle permissions of log files. """ + + def __init__(self, filename: str, mode: Optional[int]) -> None: + """Create a new :class:`PatroniFileHandler` instance. + + :param filename: basename for log files. + :param mode: permissions for log files. + """ + self.set_log_file_mode(mode) + super(PatroniFileHandler, self).__init__(filename) + + def set_log_file_mode(self, mode: Optional[int]) -> None: + """Set mode for Patroni log files. + + :param mode: permissions for log files. + + .. note:: + If *mode* is not specified, we calculate it from the `umask` value. + """ + self._log_file_mode = 0o666 & ~pg_perm.orig_umask if mode is None else mode + + def _open(self) -> TextIOWrapper: + """Open a new log file and assign permissions. + + :returns: the resulting stream. + """ + ret = super(PatroniFileHandler, self)._open() + os.chmod(self.baseFilename, self._log_file_mode) + return ret + + def debug_exception(self: logging.Logger, msg: object, *args: Any, **kwargs: Any) -> None: """Add full stack trace info to debug log messages and partial to others. @@ -423,15 +457,16 @@ class PatroniLogger(Thread): handler = self.log_handler if 'dir' in config: - if not isinstance(handler, RotatingFileHandler): - handler = RotatingFileHandler(os.path.join(config['dir'], __name__)) - + mode = parse_int(config.get('mode')) + if not isinstance(handler, PatroniFileHandler): + handler = PatroniFileHandler(os.path.join(config['dir'], __name__), mode) + handler.set_log_file_mode(mode) max_file_size = int(config.get('file_size', 25000000)) handler.maxBytes = max_file_size # pyright: ignore [reportAttributeAccessIssue] handler.backupCount = int(config.get('file_num', 4)) # we can't use `if not isinstance(handler, logging.StreamHandler)` below, - # because RotatingFileHandler is a child of StreamHandler!!! - elif handler is None or isinstance(handler, RotatingFileHandler): + # because RotatingFileHandler and PatroniFileHandler are children of StreamHandler!!! + elif handler is None or isinstance(handler, PatroniFileHandler): handler = logging.StreamHandler() is_new_handler = handler != self.log_handler @@ -454,7 +489,7 @@ class PatroniLogger(Thread): .. note:: It is used to remove different handlers that were configured previous to a reload in the configuration, - e.g. if we are switching from :class:`~logging.handlers.RotatingFileHandler` to + e.g. if we are switching from :class:`PatroniFileHandler` to class:`~logging.StreamHandler` and vice-versa. """ while True: diff --git a/patroni/validator.py b/patroni/validator.py index 233b05b7..d1473f11 100644 --- a/patroni/validator.py +++ b/patroni/validator.py @@ -991,6 +991,7 @@ schema = Schema({ Optional("dir"): str, Optional("file_num"): int, Optional("file_size"): int, + Optional("mode"): IntValidator(min=0, max=511, expected_type=int, raise_assert=True), Optional("loggers"): dict }, Optional("ctl"): { diff --git a/tests/test_config.py b/tests/test_config.py index 6cad071d..c45bf420 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -38,6 +38,7 @@ class TestConfig(unittest.TestCase): 'PATRONI_LOG_FORMAT': '["message", {"levelname": "level"}]', 'PATRONI_LOG_LOGGERS': 'patroni.postmaster: WARNING, urllib3: DEBUG', 'PATRONI_LOG_FILE_NUM': '5', + 'PATRONI_LOG_MODE': '0123', 'PATRONI_CITUS_DATABASE': 'citus', 'PATRONI_CITUS_GROUP': '0', 'PATRONI_CITUS_HOST': '0', @@ -81,6 +82,7 @@ class TestConfig(unittest.TestCase): 'PATRONI_POSTGRESQL_BIN_POSTGRES': 'sergtsop' }) config = Config('postgres0.yml') + self.assertEqual(config.local_configuration['log']['mode'], 0o123) with patch.object(Config, '_load_config_file', Mock(return_value={'restapi': {}})): with patch.object(Config, '_build_effective_configuration', Mock(side_effect=Exception)): config.reload_local_configuration() diff --git a/tests/test_file_perm.py b/tests/test_file_perm.py index 09af602b..4a845ebe 100644 --- a/tests/test_file_perm.py +++ b/tests/test_file_perm.py @@ -30,3 +30,6 @@ class TestFilePermissions(unittest.TestCase): def test_set_permissions_from_data_directory(self, mock_logger): pg_perm.set_permissions_from_data_directory('test') self.assertEqual(mock_logger.call_args[0][0], 'Can not check permissions on %s: %r') + + def test_orig_umask(self): + self.assertIsNotNone(pg_perm.orig_umask) diff --git a/tests/test_log.py b/tests/test_log.py index 3d4e79ab..165fd522 100644 --- a/tests/test_log.py +++ b/tests/test_log.py @@ -38,6 +38,7 @@ class TestPatroniLogger(unittest.TestCase): 'traceback_level': 'DEBUG', 'max_queue_size': 5, 'dir': 'foo', + 'mode': 0o600, 'file_size': 4096, 'file_num': 5, 'loggers': { @@ -50,7 +51,9 @@ class TestPatroniLogger(unittest.TestCase): os.environ[Config.PATRONI_CONFIG_VARIABLE] = yaml.dump(config, default_flow_style=False) logger = PatroniLogger() patroni_config = Config(None) - logger.reload_config(patroni_config['log']) + with patch('os.chmod') as mock_chmod: + logger.reload_config(patroni_config['log']) + self.assertEqual(mock_chmod.call_args[0][1], 0o600) _LOG.exception('test') logger.start()