From 7254ea5fb04de0699695abb0bcaf763c0dce0b70 Mon Sep 17 00:00:00 2001 From: Fan Jiang Date: Wed, 13 Jan 2016 14:05:22 -0500 Subject: [PATCH] enable config_test in configurator prepare --- .../letsencrypt_nginx/configurator.py | 31 ++++++--------- .../tests/configurator_test.py | 15 ++++--- .../letsencrypt_nginx/tests/util.py | 39 ++++++++++--------- letsencrypt/tests/cli_test.py | 3 +- 4 files changed, 42 insertions(+), 46 deletions(-) diff --git a/letsencrypt-nginx/letsencrypt_nginx/configurator.py b/letsencrypt-nginx/letsencrypt_nginx/configurator.py index 4a5a3ddcd..efa7e08b4 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/configurator.py +++ b/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. diff --git a/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py b/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py index f9af5183a..4fce33079 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py +++ b/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 diff --git a/letsencrypt-nginx/letsencrypt_nginx/tests/util.py b/letsencrypt-nginx/letsencrypt_nginx/tests/util.py index 3d70f7ac7..7a16e3738 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/tests/util.py +++ b/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) diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index 39c09dede..16ef5c093 100644 --- a/letsencrypt/tests/cli_test.py +++ b/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)