Add support for custom nginx error filename

This allows for custom error output filenames to be configured, which is
useful when using a shared nginx directory across servers.

Author: tdgroot

nginx_config_reloader/__init__.py

@@ -3,8 +3,8 @@
 import argparse
 import fnmatch
 import logging
-import logging.handlers
 import os
+import re
 import shutil
 import signal
 import subprocess
@@ -32,6 +32,7 @@
     MAIN_CONFIG_DIR,
     NGINX,
     NGINX_PID_FILE,
+    SYNC_IGNORE_FILES,
     UNPRIVILEGED_GID,
     UNPRIVILEGED_UID,
     WATCH_IGNORE_FILES,
@@ -46,11 +47,12 @@ class NginxConfigReloader(FileSystemEventHandler):
     def __init__(
         self,
         logger=None,
-        no_magento_config=False,
-        no_custom_config=False,
-        dir_to_watch=DIR_TO_WATCH,
-        magento2_flag=None,
-        use_systemd=False,
+        no_magento_config: bool = False,
+        no_custom_config: bool = False,
+        dir_to_watch: str = DIR_TO_WATCH,
+        magento2_flag: str | None = None,
+        use_systemd: bool = False,
+        error_file: str = ERROR_FILE,
     ):
         """Constructor called by ProcessEvent
 
@@ -59,6 +61,7 @@ def __init__(
         :param bool no_custom_config: True if we should not copy custom configuration
         :param str dir_to_watch: The directory to watch
         :param str magento2_flag: Magento 2 flag location
+        :param str error_file: File name for error output file
         """
         if not logger:
             self.logger = logging
@@ -76,6 +79,8 @@ def __init__(
         self.dirty = False
         self.applying = False
         self._on_config_reload = Signal()
+        # @TODO(timon): validate the input here
+        self.error_file = error_file
 
     def on_deleted(self, event):
         """Triggered by inotify on removal of file or removal of dir
@@ -113,7 +118,8 @@ def handle_event(self, event):
             return
 
         basename = os.path.basename(event.src_path)
-        if not any(fnmatch.fnmatch(basename, pat) for pat in WATCH_IGNORE_FILES):
+        ignore_files = list(WATCH_IGNORE_FILES) + [self.error_file]
+        if not any(fnmatch.fnmatch(basename, pat) for pat in ignore_files):
             self.logger.debug(
                 f"{event.event_type.upper()} detected on {event.src_path}"
             )
@@ -153,37 +159,38 @@ def check_no_forbidden_config_directives_are_present(self):
                         True    if forbidden config directives are present
                         False   if check couldn't find any forbidden config flags
         """
-        if os.path.isdir(self.dir_to_watch):
-            for rules in FORBIDDEN_CONFIG_REGEX:
-                try:
-                    # error file may contain messages that match a forbidden config pattern
-                    # then validation could fail while the actual config is correct.
-                    # we'll exclude the error file from searching for patterns,
-                    # NOTE: exclusion of error_file requires to ensure the
-                    # file is removed before moving it to nginx conf dir
-                    # @TODO: use Python to search for forbidden configs instead
-                    # of spawning external procs. Will have better testing
-                    # and even may consume less system resources
-                    check_external_resources = (
-                        "[ $(grep -r --exclude={} -P '{}' '{}' | wc -l) -lt 1 ]".format(
-                            ERROR_FILE, rules[0], self.dir_to_watch
-                        )
-                    )
-                    subprocess.check_output(check_external_resources, shell=True)
-                except subprocess.CalledProcessError:
-                    error = f"Unable to load config: {rules[1]}"
-                    self.logger.error(error)
-                    self.write_error_file(error)
-                    return True
+        if not os.path.isdir(self.dir_to_watch):
             return False
 
+        for pattern, message in FORBIDDEN_CONFIG_REGEX:
+            try:
+                # error file may contain messages that match a forbidden config pattern
+                # then validation could fail while the actual config is correct.
+                # we'll exclude the error file from searching for patterns,
+                # NOTE: exclusion of error_file requires to ensure the
+                # file is removed before moving it to nginx conf dir
+                # @TODO: use Python to search for forbidden configs instead
+                # of spawning external procs. Will have better testing
+                # and even may consume less system resources
+                check_external_resources = "[ $(grep -r --exclude={} --exclude={} -P '{}' '{}' | wc -l) -lt 1 ]".format(
+                    ERROR_FILE, self.error_file, pattern, self.dir_to_watch
+                )
+                subprocess.check_output(check_external_resources, shell=True)
+            except subprocess.CalledProcessError:
+                error = f"Unable to load config: {message}"
+                self.logger.error(error)
+                self.write_error_file(error)
+                return True
+
+        return False
+
     def remove_error_file(self):
         """Try removing the error file. Return True on success or False on errors
         :rtype: bool
         """
         removed = False
         try:
-            os.unlink(os.path.join(self.dir_to_watch, ERROR_FILE))
+            os.unlink(os.path.join(self.dir_to_watch, self.error_file))
             removed = True
         except OSError:
             pass
@@ -282,7 +289,8 @@ def install_new_custom_config_dir(self):
         if os.path.exists(CUSTOM_CONFIG_DIR):
             shutil.move(CUSTOM_CONFIG_DIR, BACKUP_CONFIG_DIR)
         os.mkdir(CUSTOM_CONFIG_DIR)
-        safe_copy_files(self.dir_to_watch, CUSTOM_CONFIG_DIR)
+        ignore_files = list(SYNC_IGNORE_FILES) + [self.error_file]
+        safe_copy_files(self.dir_to_watch, CUSTOM_CONFIG_DIR, ignore_files)
 
     def restore_old_custom_config_dir(self):
         shutil.rmtree(CUSTOM_CONFIG_DIR)
@@ -308,7 +316,7 @@ def get_nginx_pid(self):
             return None
 
     def write_error_file(self, error):
-        with open(os.path.join(self.dir_to_watch, ERROR_FILE), "w") as f:
+        with open(os.path.join(self.dir_to_watch, self.error_file), "w") as f:
             f.write(error)
 
     @property
@@ -366,6 +374,7 @@ def wait_loop(
     recursive_watch=False,
     use_systemd=False,
     no_dbus=False,
+    error_file: str = ERROR_FILE,
 ):
     """Main event loop
 
@@ -382,6 +391,7 @@ def wait_loop(
     :param bool recursive_watch: True if we should watch the dir recursively
     :param use_systemd: True if we should reload nginx using systemd instead of process signal
     :param bool no_dbus: True if we should not use DBus
+    :param str error_file: Error file to write error output to
     :return None:
     """
     dir_to_watch = os.path.abspath(dir_to_watch)
@@ -392,6 +402,7 @@ def wait_loop(
         no_custom_config=no_custom_config,
         dir_to_watch=dir_to_watch,
         use_systemd=use_systemd,
+        error_file=error_file,
     )
 
     if not no_dbus:
@@ -474,6 +485,11 @@ def parse_nginx_config_reloader_arguments():
         help="Disable DBus interface",
         default=False,
     )
+    parser.add_argument(
+        "--error-file",
+        help="File name for error output",
+        default=ERROR_FILE,
+    )
     return parser.parse_args()
 
 
@@ -491,6 +507,11 @@ def main():
     args = parse_nginx_config_reloader_arguments()
     log = get_logger()
 
+    error_file_pattern = re.compile(r"[a-zA-Z0-9_]+")
+    if not error_file_pattern.fullmatch(args.error_file):
+        log.error(f"Invalid error file name provided: {args.error_file}")
+        return 1
+
     if args.monitor:
         # Track changed files in the nginx config dir and reload on change
         wait_loop(
@@ -501,6 +522,7 @@ def main():
             recursive_watch=args.recursivewatch,
             use_systemd=args.use_systemd,
             no_dbus=args.no_dbus,
+            error_file=args.error_file,
         )
         # should never return
         return 1
@@ -512,6 +534,7 @@ def main():
             no_custom_config=args.nocustomconfig,
             dir_to_watch=args.watchdir,
             use_systemd=args.use_systemd,
+            error_file=args.error_file,
         ).apply_new_config()
         return 0
 

nginx_config_reloader/copy_files.py

@@ -7,7 +7,10 @@
 logger = logging.getLogger(__name__)
 
 
-def safe_copy_files(src, dest):
+def safe_copy_files(src, dest, ignore_files: list[str] | None = None):
+    if not ignore_files:
+        ignore_files = list(SYNC_IGNORE_FILES)
+
     cmd = [
         # Adding a / at the end copies contents of the dir and not the dir itself
         # This is achieved with `os.path.join(x, '')`, which ensures a trailing slash
@@ -22,7 +25,7 @@ def safe_copy_files(src, dest):
         # Dirs default to 0755 to read. Remove setuid bits. Remove executability for others
         '--chmod="D755,-s,Fo-wx"',
     ]
-    cmd.extend([f'--exclude="{pattern}"' for pattern in SYNC_IGNORE_FILES])
+    cmd.extend([f'--exclude="{pattern}"' for pattern in ignore_files])
     cmd = " ".join(cmd)
     # shell=True to ensure globs are not escaped
     check_output(cmd, shell=True, stderr=STDOUT)

tests/test_assert_forbidden_statements_in_config.py

@@ -4,7 +4,11 @@
 
 import pytest
 
-from nginx_config_reloader import FORBIDDEN_CONFIG_REGEX, NginxConfigReloader
+from nginx_config_reloader import (
+    ERROR_FILE,
+    FORBIDDEN_CONFIG_REGEX,
+    NginxConfigReloader,
+)
 from tests.testcase import TestCase
 
 # Skip marker for tests that require grep -P (PCRE support, not available on macOS)
@@ -32,6 +36,22 @@ def test_assert_no_includes_in_config_does_not_check_config_if_no_dir_to_watch(
 
         self.assertFalse(self.check_output.called)
 
+    def test_check_no_forbidden_config_excludes_default_and_custom_error_files(self):
+        reloader = NginxConfigReloader(
+            dir_to_watch="/tmp/nginx", error_file="custom_error_output"
+        )
+
+        reloader.check_no_forbidden_config_directives_are_present()
+
+        self.assertEqual(
+            len(self.check_output.call_args_list), len(FORBIDDEN_CONFIG_REGEX)
+        )
+        for call_args in self.check_output.call_args_list:
+            command = call_args.args[0]
+            self.assertIn(f"--exclude={ERROR_FILE}", command)
+            self.assertIn("--exclude=custom_error_output", command)
+            self.assertIn("'/tmp/nginx'", command)
+
     @requires_pcre_grep
     def test_include_prevention_legal_includes(self):
         TEST_CASES = [

tests/test_main.py

@@ -2,6 +2,7 @@
 from tempfile import mkdtemp
 from unittest.mock import Mock
 
+import nginx_config_reloader
 from nginx_config_reloader import main
 from tests.testcase import TestCase
 
@@ -21,6 +22,7 @@ def setUp(self):
             recursivewatch=False,
             use_systemd=False,
             no_dbus=False,
+            error_file="custom_error_output",
         )
         self.get_logger = self.set_up_context_manager_patch(
             "nginx_config_reloader.get_logger"
@@ -54,6 +56,7 @@ def test_main_reloads_config_once_if_monitor_mode_not_specified(self):
             no_custom_config=self.parse_nginx_config_reloader_arguments.return_value.nocustomconfig,
             dir_to_watch=self.parse_nginx_config_reloader_arguments.return_value.watchdir,
             use_systemd=self.parse_nginx_config_reloader_arguments.return_value.use_systemd,
+            error_file=self.parse_nginx_config_reloader_arguments.return_value.error_file,
         )
         self.reloader.return_value.apply_new_config.assert_called_once_with()
 
@@ -80,6 +83,7 @@ def test_main_watches_the_config_dir_if_monitor_specified(self):
             recursive_watch=self.parse_nginx_config_reloader_arguments.return_value.recursivewatch,
             use_systemd=self.parse_nginx_config_reloader_arguments.return_value.use_systemd,
             no_dbus=self.parse_nginx_config_reloader_arguments.return_value.no_dbus,
+            error_file=self.parse_nginx_config_reloader_arguments.return_value.error_file,
         )
 
     def test_main_watches_the_config_dir_if_monitor_mode_is_specified_and_includes_allowed(
@@ -98,6 +102,7 @@ def test_main_watches_the_config_dir_if_monitor_mode_is_specified_and_includes_a
             recursive_watch=self.parse_nginx_config_reloader_arguments.return_value.recursivewatch,
             use_systemd=self.parse_nginx_config_reloader_arguments.return_value.use_systemd,
             no_dbus=self.parse_nginx_config_reloader_arguments.return_value.no_dbus,
+            error_file=self.parse_nginx_config_reloader_arguments.return_value.error_file,
         )
 
     def test_main_does_not_reload_the_config_once_if_monitor_mode_is_specified(self):
@@ -128,4 +133,26 @@ def test_main_passes_no_dbus_to_wait_loop(self):
             recursive_watch=self.parse_nginx_config_reloader_arguments.return_value.recursivewatch,
             use_systemd=self.parse_nginx_config_reloader_arguments.return_value.use_systemd,
             no_dbus=True,
+            error_file=self.parse_nginx_config_reloader_arguments.return_value.error_file,
         )
+
+    def test_main_rejects_invalid_error_file_name(self):
+        self.parse_nginx_config_reloader_arguments.return_value.error_file = "bad/name"
+
+        ret = main()
+
+        self.assertEqual(1, ret)
+        self.get_logger.return_value.error.assert_called_once_with(
+            "Invalid error file name provided: bad/name"
+        )
+        self.assertFalse(self.wait_loop.called)
+        self.assertFalse(self.reloader.called)
+
+    def test_main_accepts_default_error_file_name(self):
+        self.parse_nginx_config_reloader_arguments.return_value.error_file = (
+            nginx_config_reloader.ERROR_FILE
+        )
+
+        ret = main()
+
+        self.assertEqual(0, ret)