Merge branch 'master' into prep4prod

bootstrap/_rpm_common.sh

@@ -40,6 +40,7 @@ if ! $tool install -y \
        augeas-libs \
        openssl-devel \
        libffi-devel \
+       redhat-rpm-config \
        ca-certificates
 then
     echo "Could not install additional dependencies. Aborting bootstrap!"

letsencrypt-apache/letsencrypt_apache/configurator.py

@@ -7,7 +7,6 @@ import os
 import re
 import shutil
 import socket
-import subprocess
 import time
 
 import zope.interface
@@ -95,13 +94,11 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
             help="Path to the Apache 'a2enmod' binary.")
         add("dismod", default=constants.CLI_DEFAULTS["dismod"],
             help="Path to the Apache 'a2enmod' binary.")
-        add("init-script", default=constants.CLI_DEFAULTS["init_script"],
-            help="Path to the Apache init script (used for server "
-            "reload).")
         add("le-vhost-ext", default=constants.CLI_DEFAULTS["le_vhost_ext"],
             help="SSL vhost configuration extension.")
         add("server-root", default=constants.CLI_DEFAULTS["server_root"],
             help="Apache server root directory.")
+        le_util.add_deprecated_argument(add, "init-script", 1)
 
     def __init__(self, *args, **kwargs):
         """Initialize an Apache Configurator.
@@ -140,8 +137,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
 
         """
         # Verify Apache is installed
-        for exe in (self.conf("ctl"), self.conf("enmod"),
-                    self.conf("dismod"), self.conf("init-script")):
+        for exe in (self.conf("ctl"), self.conf("enmod"), self.conf("dismod")):
             if not le_util.exe_exists(exe):
                 raise errors.NoInstallationError
 
@@ -1186,16 +1182,25 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         le_util.run_script([self.conf("enmod"), mod_name])
 
     def restart(self):
-        """Reloads apache server.
+        """Runs a config test and reloads the Apache server.
 
-        .. todo:: This function will be converted to using reload
-
-        :raises .errors.MisconfigurationError: If unable to reload due
-            to a configuration problem, or if the reload subprocess
-            cannot be run.
+        :raises .errors.MisconfigurationError: If either the config test
+            or reload fails.
 
         """
-        return apache_reload(self.conf("init-script"))
+        self.config_test()
+        self._reload()
+
+    def _reload(self):
+        """Reloads the Apache server.
+
+        :raises .errors.MisconfigurationError: If reload fails
+
+        """
+        try:
+            le_util.run_script([self.conf("ctl"), "-k", "graceful"])
+        except errors.SubprocessError as err:
+            raise errors.MisconfigurationError(str(err))
 
     def config_test(self):  # pylint: disable=no-self-use
         """Check the configuration of Apache for errors.
@@ -1317,44 +1322,6 @@ def _get_mod_deps(mod_name):
     return deps.get(mod_name, [])
 
 
-def apache_reload(apache_init_script):
-    """Reloads the Apache Server.
-
-    :param str apache_init_script: Path to the Apache init script.
-
-    .. todo:: Try to use reload instead. (This caused timing problems before)
-
-    .. todo:: On failure, this should be a recovery_routine call with another
-       reload.  This will confuse and inhibit developers from testing code
-       though.  This change should happen after
-       the ApacheConfigurator has been thoroughly tested.  The function will
-       need to be moved into the class again.  Perhaps
-       this version can live on... for testing purposes.
-
-    :raises .errors.MisconfigurationError: If unable to reload due to a
-        configuration problem, or if the reload subprocess cannot be run.
-
-    """
-    try:
-        proc = subprocess.Popen([apache_init_script, "reload"],
-                                stdout=subprocess.PIPE,
-                                stderr=subprocess.PIPE)
-
-    except (OSError, ValueError):
-        logger.fatal(
-            "Unable to reload the Apache process with %s", apache_init_script)
-        raise errors.MisconfigurationError(
-            "Unable to reload Apache process with %s" % apache_init_script)
-
-    stdout, stderr = proc.communicate()
-
-    if proc.returncode != 0:
-        # Enter recovery routine...
-        logger.error("Apache Reload Failed!\n%s\n%s", stdout, stderr)
-        raise errors.MisconfigurationError(
-            "Error while reloading Apache:\n%s\n%s" % (stdout, stderr))
-
-
 def get_file_path(vhost_path):
     """Get file path from augeas_vhost_path.
 

letsencrypt-apache/letsencrypt_apache/constants.py

@@ -7,7 +7,6 @@ CLI_DEFAULTS = dict(
     ctl="apache2ctl",
     enmod="a2enmod",
     dismod="a2dismod",
-    init_script="/etc/init.d/apache2",
     le_vhost_ext="-le-ssl.conf",
 )
 """CLI defaults."""

letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py

@@ -563,24 +563,13 @@ class TwoVhost80Test(util.ApacheTest):
         mock_script.side_effect = errors.SubprocessError("Can't find program")
         self.assertRaises(errors.PluginError, self.config.get_version)
 
-    @mock.patch("letsencrypt_apache.configurator.subprocess.Popen")
-    def test_restart(self, mock_popen):
-        """These will be changed soon enough with reload."""
-        mock_popen().returncode = 0
-        mock_popen().communicate.return_value = ("", "")
-
+    @mock.patch("letsencrypt_apache.configurator.le_util.run_script")
+    def test_restart(self, _):
         self.config.restart()
 
-    @mock.patch("letsencrypt_apache.configurator.subprocess.Popen")
-    def test_restart_bad_process(self, mock_popen):
-        mock_popen.side_effect = OSError
-
-        self.assertRaises(errors.MisconfigurationError, self.config.restart)
-
-    @mock.patch("letsencrypt_apache.configurator.subprocess.Popen")
-    def test_restart_failure(self, mock_popen):
-        mock_popen().communicate.return_value = ("", "")
-        mock_popen().returncode = 1
+    @mock.patch("letsencrypt_apache.configurator.le_util.run_script")
+    def test_restart_bad_process(self, mock_run_script):
+        mock_run_script.side_effect = [None, errors.SubprocessError]
 
         self.assertRaises(errors.MisconfigurationError, self.config.restart)
 

letsencrypt-apache/letsencrypt_apache/tests/util.py

@@ -75,11 +75,7 @@ def get_apache_configurator(
         in_progress_dir=os.path.join(backups, "IN_PROGRESS"),
         work_dir=work_dir)
 
-    with mock.patch("letsencrypt_apache.configurator."
-                    "subprocess.Popen") as mock_popen:
-        # This indicates config_test passes
-        mock_popen().communicate.return_value = ("Fine output", "No problems")
-        mock_popen().returncode = 0
+    with mock.patch("letsencrypt_apache.configurator.le_util.run_script"):
         with mock.patch("letsencrypt_apache.configurator.le_util."
                         "exe_exists") as mock_exe_exists:
             mock_exe_exists.return_value = True

letsencrypt/le_util.py

@@ -1,12 +1,14 @@
 """Utilities for all Let's Encrypt."""
+import argparse
 import collections
 import errno
 import logging
 import os
 import platform
 import re
-import subprocess
 import stat
+import subprocess
+import sys
 
 from letsencrypt import errors
 
@@ -255,3 +257,26 @@ def safe_email(email):
     else:
         logger.warn("Invalid email address: %s.", email)
         return False
+
+
+def add_deprecated_argument(add_argument, argument_name, nargs):
+    """Adds a deprecated argument with the name argument_name.
+
+    Deprecated arguments are not shown in the help. If they are used on
+    the command line, a warning is shown stating that the argument is
+    deprecated and no other action is taken.
+
+    :param callable add_argument: Function that adds arguments to an
+        argument parser/group.
+    :param str argument_name: Name of deprecated argument.
+    :param nargs: Value for nargs when adding the argument to argparse.
+
+    """
+    class ShowWarning(argparse.Action):
+        """Action to log a warning when an argument is used."""
+        def __call__(self, unused1, unused2, unused3, option_string=None):
+            sys.stderr.write(
+                "Use of {0} is deprecated.\n".format(option_string))
+
+    add_argument(argument_name, action=ShowWarning,
+                 help=argparse.SUPPRESS, nargs=nargs)

letsencrypt/tests/le_util_test.py

@@ -1,8 +1,10 @@
 """Tests for letsencrypt.le_util."""
+import argparse
 import errno
 import os
 import shutil
 import stat
+import StringIO
 import tempfile
 import unittest
 
@@ -284,5 +286,42 @@ class SafeEmailTest(unittest.TestCase):
             self.assertFalse(self._call(addr), "%s failed." % addr)
 
 
+class AddDeprecatedArgumentTest(unittest.TestCase):
+    """Test add_deprecated_argument."""
+    def setUp(self):
+        self.parser = argparse.ArgumentParser()
+
+    def _call(self, argument_name, nargs):
+        from letsencrypt.le_util import add_deprecated_argument
+
+        add_deprecated_argument(self.parser.add_argument, argument_name, nargs)
+
+    def test_warning_no_arg(self):
+        self._call("--old-option", 0)
+        stderr = self._get_argparse_warnings(["--old-option"])
+        self.assertTrue("--old-option is deprecated" in stderr)
+
+    def test_warning_with_arg(self):
+        self._call("--old-option", 1)
+        stderr = self._get_argparse_warnings(["--old-option", "42"])
+        self.assertTrue("--old-option is deprecated" in stderr)
+
+    def _get_argparse_warnings(self, args):
+        stderr = StringIO.StringIO()
+        with mock.patch("letsencrypt.le_util.sys.stderr", new=stderr):
+            self.parser.parse_args(args)
+        return stderr.getvalue()
+
+    def test_help(self):
+        self._call("--old-option", 2)
+        stdout = StringIO.StringIO()
+        with mock.patch("letsencrypt.le_util.sys.stdout", new=stdout):
+            try:
+                self.parser.parse_args(["-h"])
+            except SystemExit:
+                pass
+        self.assertTrue("--old-option" not in stdout.getvalue())
+
+
 if __name__ == "__main__":
     unittest.main()  # pragma: no cover