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

@@ -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}"``

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

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:

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."""

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:

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"): {

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()

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)

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()