Merge pull request #2165 from twstrike/config_test_in_nginx_prepare

enable config_test in configurator prepare

letsencrypt-nginx/letsencrypt_nginx/configurator.py

@@ -5,7 +5,6 @@ import re
 import shutil
 import socket
 import subprocess
-import sys
 import time
 
 import OpenSSL
@@ -106,11 +105,18 @@ class NginxConfigurator(common.Plugin):
 
     # This is called in determine_authenticator and determine_installer
     def prepare(self):
-        """Prepare the authenticator/installer."""
+        """Prepare the authenticator/installer.
+
+        :raises .errors.NoInstallationError: If Nginx ctl cannot be found
+        :raises .errors.MisconfigurationError: If Nginx is misconfigured
+        """
         # Verify Nginx is installed
         if not le_util.exe_exists(self.conf('ctl')):
             raise errors.NoInstallationError
 
+        # Make sure configuration is valid
+        self.config_test()
+
         self.parser = parser.NginxParser(
             self.conf('server-root'), self.mod_ssl_conf)
 
@@ -409,26 +415,13 @@ class NginxConfigurator(common.Plugin):
     def config_test(self):  # pylint: disable=no-self-use
         """Check the configuration of Nginx for errors.
 
-        :returns: Success
-        :rtype: bool
+        :raises .errors.MisconfigurationError: If config_test fails
 
         """
         try:
-            proc = subprocess.Popen(
-                [self.conf('ctl'), "-c", self.nginx_conf, "-t"],
-                stdout=subprocess.PIPE,
-                stderr=subprocess.PIPE)
-            stdout, stderr = proc.communicate()
-        except (OSError, ValueError):
-            logger.fatal("Unable to run nginx config test")
-            sys.exit(1)
-
-        if proc.returncode != 0:
-            # Enter recovery routine...
-            logger.error("Config test failed\n%s\n%s", stdout, stderr)
-            return False
-
-        return True
+            le_util.run_script([self.conf('ctl'), "-c", self.nginx_conf, "-t"])
+        except errors.SubprocessError as err:
+            raise errors.MisconfigurationError(str(err))
 
     def _verify_setup(self):
         """Verify the setup to ensure safe operating environment.

letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py

@@ -54,6 +54,7 @@ class NginxConfiguratorTest(util.NginxTest):
         mock_exe_exists.return_value = True
 
         self.config.version = None
+        self.config.config_test = mock.Mock()
         self.config.prepare()
         self.assertEquals((1, 6, 2), self.config.version)
 
@@ -361,12 +362,14 @@ class NginxConfiguratorTest(util.NginxTest):
         mock_popen.side_effect = OSError("Can't find program")
         self.assertRaises(errors.MisconfigurationError, self.config.restart)
 
-    @mock.patch("letsencrypt_nginx.configurator.subprocess.Popen")
-    def test_config_test(self, mock_popen):
-        mocked = mock_popen()
-        mocked.communicate.return_value = ('', '')
-        mocked.returncode = 0
-        self.assertTrue(self.config.config_test())
+    @mock.patch("letsencrypt.le_util.run_script")
+    def test_config_test(self, _):
+        self.config.config_test()
+
+    @mock.patch("letsencrypt.le_util.run_script")
+    def test_config_test_bad_process(self, mock_run_script):
+        mock_run_script.side_effect = errors.SubprocessError
+        self.assertRaises(errors.MisconfigurationError, self.config.config_test)
 
     def test_get_snakeoil_paths(self):
         # pylint: disable=protected-access

letsencrypt-nginx/letsencrypt_nginx/tests/util.py

@@ -49,25 +49,26 @@ def get_nginx_configurator(
 
     backups = os.path.join(work_dir, "backups")
 
-    with mock.patch("letsencrypt_nginx.configurator.le_util."
-                    "exe_exists") as mock_exe_exists:
-        mock_exe_exists.return_value = True
-
-        config = configurator.NginxConfigurator(
-            config=mock.MagicMock(
-                nginx_server_root=config_path,
-                le_vhost_ext="-le-ssl.conf",
-                config_dir=config_dir,
-                work_dir=work_dir,
-                backup_dir=backups,
-                temp_checkpoint_dir=os.path.join(work_dir, "temp_checkpoints"),
-                in_progress_dir=os.path.join(backups, "IN_PROGRESS"),
-                server="https://acme-server.org:443/new",
-                tls_sni_01_port=5001,
-            ),
-            name="nginx",
-            version=version)
-        config.prepare()
+    with mock.patch("letsencrypt_nginx.configurator.NginxConfigurator."
+                    "config_test"):
+        with mock.patch("letsencrypt_nginx.configurator.le_util."
+                        "exe_exists") as mock_exe_exists:
+            mock_exe_exists.return_value = True
+            config = configurator.NginxConfigurator(
+                config=mock.MagicMock(
+                    nginx_server_root=config_path,
+                    le_vhost_ext="-le-ssl.conf",
+                    config_dir=config_dir,
+                    work_dir=work_dir,
+                    backup_dir=backups,
+                    temp_checkpoint_dir=os.path.join(work_dir, "temp_checkpoints"),
+                    in_progress_dir=os.path.join(backups, "IN_PROGRESS"),
+                    server="https://acme-server.org:443/new",
+                    tls_sni_01_port=5001,
+                ),
+                name="nginx",
+                version=version)
+            config.prepare()
 
     # Provide general config utility.
     nsconfig = configuration.NamespaceConfig(config.config)

letsencrypt/tests/cli_test.py

@@ -202,8 +202,7 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
             # (we can only do that if letsencrypt-nginx is actually present)
             ret, _, _, _ = self._call(args)
             self.assertTrue("The nginx plugin is not working" in ret)
-            self.assertTrue("Could not find configuration root" in ret)
-            self.assertTrue("NoInstallationError" in ret)
+            self.assertTrue("MisconfigurationError" in ret)
 
         args = ["certonly", "--webroot"]
         ret, _, _, _ = self._call(args)