Logging setup before argument parsing (#4446)

Second part of #4443. Built on #4444. Fixes #3148.

This fixes an old problem with code logging messages before logging has been set up. How this works is explained in the docstring of certbot.log.pre_arg_setup.

* add memory handler

* Add exit_with_log_path

* add new_except_hook

* pre_arg_parse_setup++ and remove old except_hook

* Rewrite post_arg_setup

* test restricted permissions

* move changes to main

* Use .name of NamedTemporaryFile

* use better assertions

* set exc_info in except_hook

* Make post_arg_setup more robust

* final cleanup

* Add TempHandler

* undo main_test changes

* improve documentation

* use decorators instead of with for mock.patch

* add inline comment about logging.shutdown

certbot/cli.py

@@ -19,6 +19,7 @@ import certbot
 from certbot import constants
 from certbot import crypto_util
 from certbot import errors
+from certbot import hooks
 from certbot import interfaces
 from certbot import util
 
@@ -565,6 +566,11 @@ class HelpfulArgumentParser(object):
         if parsed_args.must_staple:
             parsed_args.staple = True
 
+        if parsed_args.validate_hooks:
+            hooks.validate_hooks(parsed_args)
+
+        possible_deprecation_warning(parsed_args)
+
         return parsed_args
 
     def set_test_server(self, parsed_args):

certbot/log.py

@@ -1,15 +1,29 @@
-"""Logging utilities for Certbot."""
+"""Logging utilities for Certbot.
+
+The best way to use this module is through `pre_arg_parse_setup` and
+`post_arg_parse_setup`. `pre_arg_parse_setup` configures a minimal
+terminal logger and ensures a detailed log is written to a secure
+temporary file if Certbot exits before `post_arg_parse_setup` is called.
+`post_arg_parse_setup` relies on the parsed command line arguments and
+does the full logging setup with terminal and rotating file handling as
+configured by the user. Any logged messages before
+`post_arg_parse_setup` is called are sent to the rotating file handler.
+Special care is taken by both methods to ensure all errors are logged
+and properly flushed before program exit.
+
+"""
 from __future__ import print_function
 import functools
 import logging
+import logging.handlers
 import os
 import sys
+import tempfile
 import time
 import traceback
 
 from acme import messages
 
-from certbot import cli
 from certbot import constants
 from certbot import errors
 from certbot import util
@@ -23,36 +37,85 @@ logger = logging.getLogger(__name__)
 
 
 def pre_arg_parse_setup():
-    """Ensures fatal exceptions are logged and reported to the user."""
+    """Setup logging before command line arguments are parsed.
-    sys.excepthook = functools.partial(except_hook, config=None)
+
+    Terminal logging is setup using
+    `certbot.constants.QUIET_LOGGING_LEVEL` so Certbot is as quiet as
+    possible. File logging is setup so that logging messages are
+    buffered in memory. If Certbot exits before `post_arg_parse_setup`
+    is called, these buffered messages are written to a temporary file.
+    If Certbot doesn't exit, `post_arg_parse_setup` writes the messages
+    to the normal log files.
+
+    This function also sets `logging.shutdown` to be called on program
+    exit which automatically flushes logging handlers and
+    `sys.excepthook` to properly log/display fatal exceptions.
+
+    """
+    temp_handler = TempHandler()
+    temp_handler.setFormatter(logging.Formatter(FILE_FMT))
+    temp_handler.setLevel(logging.DEBUG)
+    memory_handler = MemoryHandler(temp_handler)
+
+    stream_handler = ColoredStreamHandler()
+    stream_handler.setFormatter(logging.Formatter(CLI_FMT))
+    stream_handler.setLevel(constants.QUIET_LOGGING_LEVEL)
+
+    root_logger = logging.getLogger()
+    root_logger.setLevel(logging.DEBUG)  # send all records to handlers
+    root_logger.addHandler(memory_handler)
+    root_logger.addHandler(stream_handler)
+
+    # logging.shutdown will flush the memory handler because flush() and
+    # close() are explicitly called
+    util.atexit_register(logging.shutdown)
+    sys.excepthook = functools.partial(
+        except_hook, debug='--debug' in sys.argv, log_path=temp_handler.path)
 
 
 def post_arg_parse_setup(config):
     """Setup logging after command line arguments are parsed.
 
+    This function assumes `pre_arg_parse_setup` was called earlier and
+    the root logging configuration has not been modified. A rotating
+    file logging handler is created and the buffered log messages are
+    sent to that handler. Terminal logging output is set to the level
+    requested by the user.
+
     :param certbot.interface.IConfig config: Configuration object
 
     """
     file_handler, file_path = setup_log_file_handler(
         config, 'letsencrypt.log', FILE_FMT)
+    logs_dir = os.path.dirname(file_path)
+
+    root_logger = logging.getLogger()
+    memory_handler = stderr_handler = None
+    for handler in root_logger.handlers:
+        if isinstance(handler, ColoredStreamHandler):
+            stderr_handler = handler
+        elif isinstance(handler, MemoryHandler):
+            memory_handler = handler
+    msg = 'Previously configured logging handlers have been removed!'
+    assert memory_handler is not None and stderr_handler is not None, msg
+
+    root_logger.addHandler(file_handler)
+    root_logger.removeHandler(memory_handler)
+    temp_handler = memory_handler.target
+    memory_handler.setTarget(file_handler)
+    memory_handler.close()
+    temp_handler.delete_and_close()
 
     if config.quiet:
         level = constants.QUIET_LOGGING_LEVEL
     else:
         level = -config.verbose_count * 10
-    stderr_handler = ColoredStreamHandler()
-    stderr_handler.setFormatter(logging.Formatter(CLI_FMT))
     stderr_handler.setLevel(level)
-
-    root_logger = logging.getLogger()
-    root_logger.setLevel(logging.DEBUG)  # send all records to handlers
-    root_logger.addHandler(stderr_handler)
-    root_logger.addHandler(file_handler)
-
     logger.debug('Root logging level set at %d', level)
     logger.info('Saving debug log to %s', file_path)
 
-    sys.excepthook = functools.partial(except_hook, config=config)
+    sys.excepthook = functools.partial(
+        except_hook, debug=config.debug, log_path=logs_dir)
 
 
 def setup_log_file_handler(config, logfile, fmt):
@@ -91,17 +154,17 @@ class ColoredStreamHandler(logging.StreamHandler):
     """Sends colored logging output to a stream.
 
     If the specified stream is not a tty, the class works like the
-    standard logging.StreamHandler. Default red_level is logging.WARNING.
+    standard `logging.StreamHandler`. Default red_level is
+    `logging.WARNING`.
 
     :ivar bool colored: True if output should be colored
     :ivar bool red_level: The level at which to output
 
     """
-
     def __init__(self, stream=None):
-        if sys.version_info < (2, 7):
+        # logging handlers use old style classes in Python 2.6 so
-            # pragma: no cover
+        # super() cannot be used
-            # pylint: disable=non-parent-init-called
+        if sys.version_info < (2, 7):  # pragma: no cover
             logging.StreamHandler.__init__(self, stream)
         else:
             super(ColoredStreamHandler, self).__init__(stream)
@@ -127,54 +190,132 @@ class ColoredStreamHandler(logging.StreamHandler):
             return out
 
 
-def except_hook(exc_type, exc_value, trace, config):
+class MemoryHandler(logging.handlers.MemoryHandler):
-    """Logs exceptions and reports them to the user.
+    """Buffers logging messages in memory until the buffer is flushed.
 
-    Config is used to determine how to display exceptions to the user. In
+    This differs from `logging.handlers.MemoryHandler` in that flushing
-    general, if config.debug is True, then the full exception and traceback is
+    only happens when it is done explicitly by calling flush() or
-    shown to the user, otherwise it is suppressed. If config itself is None,
+    close().
-    then the traceback and exception is attempted to be written to a logfile.
-    If this is successful, the traceback is suppressed, otherwise it is shown
-    to the user. sys.exit is always called with a nonzero status.
 
     """
-    tb_str = "".join(traceback.format_exception(exc_type, exc_value, trace))
+    def __init__(self, target=None):
-    logger.debug("Exiting abnormally:%s%s", os.linesep, tb_str)
+        # capacity doesn't matter because should_flush() is overridden
+        capacity = float('inf')
+        # logging handlers use old style classes in Python 2.6 so
+        # super() cannot be used
+        if sys.version_info < (2, 7):  # pragma: no cover
+            logging.handlers.MemoryHandler.__init__(
+                self, capacity, target=target)
+        else:
+            super(MemoryHandler, self).__init__(capacity, target=target)
 
-    if issubclass(exc_type, Exception) and (config is None or not config.debug):
+    def shouldFlush(self, record):
-        if config is None:
+        """Should the buffer be automatically flushed?
-            logfile = "certbot.log"
-            try:
-                with open(logfile, "w") as logfd:
-                    traceback.print_exception(
-                        exc_type, exc_value, trace, file=logfd)
-                assert "--debug" not in sys.argv  # config is None if this explodes
-            except:  # pylint: disable=bare-except
-                sys.exit(tb_str)
-            if "--debug" in sys.argv:
-                sys.exit(tb_str)
 
+        :param logging.LogRecord record: log record to be considered
+
+        :returns: False because the buffer should never be auto-flushed
+        :rtype: bool
+
+        """
+        return False
+
+
+class TempHandler(logging.StreamHandler):
+    """Safely logs messages to a temporary file.
+
+    The file is created with permissions 600.
+
+    :ivar str path: file system path to the temporary log file
+
+    """
+    def __init__(self):
+        stream = tempfile.NamedTemporaryFile('w', delete=False)
+        # logging handlers use old style classes in Python 2.6 so
+        # super() cannot be used
+        if sys.version_info < (2, 7):  # pragma: no cover
+            logging.StreamHandler.__init__(self, stream)
+        else:
+            super(TempHandler, self).__init__(stream)
+        self.path = stream.name
+
+    def delete_and_close(self):
+        """Close the handler and delete the temporary log file."""
+        self._close(delete=True)
+
+    def close(self):
+        """Close the handler and the temporary log file."""
+        self._close(delete=False)
+
+    def _close(self, delete):
+        """Close the handler and the temporary log file.
+
+        :param bool delete: True if the log file should be deleted
+
+        """
+        self.acquire()
+        try:
+            # StreamHandler.close() doesn't close the stream to allow a
+            # stream like stderr to be used
+            self.stream.close()
+            if delete:
+                os.remove(self.path)
+            if sys.version_info < (2, 7):  # pragma: no cover
+                logging.StreamHandler.close(self)
+            else:
+                super(TempHandler, self).close()
+        finally:
+            self.release()
+
+
+def except_hook(exc_type, exc_value, trace, debug, log_path):
+    """Logs fatal exceptions and reports them to the user.
+
+    If debug is True, the full exception and traceback is shown to the
+    user, otherwise, it is suppressed. sys.exit is always called with a
+    nonzero status.
+
+    :param type exc_type: type of the raised exception
+    :param BaseException exc_value: raised exception
+    :param traceback trace: traceback of where the exception was raised
+    :param bool debug: True if the traceback should be shown to the user
+    :param str log_path: path to file or directory containing the log
+
+    """
+    exc_info = (exc_type, exc_value, trace)
+    # constants.QUIET_LOGGING_LEVEL or higher should be used to
+    # display message the user, otherwise, a lower level like
+    # logger.DEBUG should be used
+    if debug or not issubclass(exc_type, Exception):
+        assert constants.QUIET_LOGGING_LEVEL <= logging.ERROR
+        logger.error('Exiting abnormally:', exc_info=exc_info)
+    else:
+        logger.debug('Exiting abnormally:', exc_info=exc_info)
         if issubclass(exc_type, errors.Error):
             sys.exit(exc_value)
+        print('An unexpected error occurred:', file=sys.stderr)
+        if messages.is_acme_error(exc_value):
+            # Remove the ACME error prefix from the exception
+            _, _, exc_str = str(exc_value).partition(':: ')
+            print(exc_str, file=sys.stderr)
         else:
-            # Here we're passing a client or ACME error out to the client at the shell
+            traceback.print_exception(exc_type, exc_value, None)
-            # Tell the user a bit about what happened, without overwhelming
+    exit_with_log_path(log_path)
-            # them with a full traceback
+
-            err = traceback.format_exception_only(exc_type, exc_value)[0]
+
-            # Typical error from the ACME module:
+def exit_with_log_path(log_path):
-            # acme.messages.Error: urn:ietf:params:acme:error:malformed :: The
+    """Print a message about the log location and exit.
-            # request message was malformed :: Error creating new registration
+
-            # :: Validation of contact mailto:none@longrandomstring.biz failed:
+    The message is printed to stderr and the program will exit with a
-            # Server failure at resolver
+    nonzero status.
-            if (messages.is_acme_error(err) and ":: " in err and
+
-                 config.verbose_count <= cli.flag_default("verbose_count")):
+    :param str log_path: path to file or directory containing the log
-                # prune ACME error code, we have a human description
+
-                _code, _sep, err = err.partition(":: ")
+    """
-            msg = "An unexpected error occurred:\n" + err + "Please see the "
+    msg = 'Please see the '
-            if config is None:
+    if os.path.isdir(log_path):
-                msg += "logfile '{0}' for more details.".format(logfile)
+        msg += 'logfiles in {0} '.format(log_path)
-            else:
-                msg += "logfiles in {0} for more details.".format(config.logs_dir)
-            sys.exit(msg)
     else:
-        sys.exit(tb_str)
+        msg += "logfile '{0}' ".format(log_path)
+    msg += 'for more details.'
+    sys.exit(msg)

certbot/main.py

@@ -714,37 +714,24 @@ def set_displayer(config):
                                              config.force_interactive)
     zope.component.provideUtility(displayer)
 
-def _post_logging_setup(config, plugins, cli_args):
-    """Perform any setup or configuration tasks that require a logger."""
-
-    # This needs logging, but would otherwise be in HelpfulArgumentParser
-    if config.validate_hooks:
-        hooks.validate_hooks(config)
-
-    cli.possible_deprecation_warning(config)
-
-    logger.debug("certbot version: %s", certbot.__version__)
-    # do not log `config`, as it contains sensitive data (e.g. revoke --key)!
-    logger.debug("Arguments: %r", cli_args)
-    logger.debug("Discovered plugins: %r", plugins)
-
 
 def main(cli_args=sys.argv[1:]):
     """Command line argument parsing and main script execution."""
     log.pre_arg_parse_setup()
+
     plugins = plugins_disco.PluginsRegistry.find_all()
+    logger.debug("certbot version: %s", certbot.__version__)
+    # do not log `config`, as it contains sensitive data (e.g. revoke --key)!
+    logger.debug("Arguments: %r", cli_args)
+    logger.debug("Discovered plugins: %r", plugins)
 
     # note: arg parser internally handles --help (and exits afterwards)
     args = cli.prepare_and_parse_args(plugins, cli_args)
     config = configuration.NamespaceConfig(args)
     zope.component.provideUtility(config)
 
-    make_or_verify_needed_dirs(config)
-
     log.post_arg_parse_setup(config)
-
+    make_or_verify_needed_dirs(config)
-    _post_logging_setup(config, plugins, cli_args)
-
     set_displayer(config)
 
     # Reporter

certbot/tests/log_test.py

@@ -1,6 +1,5 @@
 """Tests for certbot.log."""
 import logging
-import traceback
 import logging.handlers
 import os
 import sys
@@ -26,13 +25,34 @@ class PreArgParseSetupTest(unittest.TestCase):
         from certbot.log import pre_arg_parse_setup
         return pre_arg_parse_setup(*args, **kwargs)
 
-    def test_it(self):
+    @mock.patch('certbot.log.sys')
-        with mock.patch('certbot.log.except_hook') as mock_except_hook:
+    @mock.patch('certbot.log.except_hook')
-            with mock.patch('certbot.log.sys') as mock_sys:
+    @mock.patch('certbot.log.logging.getLogger')
-                self._call()
+    @mock.patch('certbot.log.util.atexit_register')
+    def test_it(self, mock_register, mock_get, mock_except_hook, mock_sys):
+        mock_sys.argv = ['--debug']
+        mock_sys.version_info = sys.version_info
+        self._call()
 
+        mock_register.assert_called_once_with(logging.shutdown)
         mock_sys.excepthook(1, 2, 3)
-        mock_except_hook.assert_called_once_with(1, 2, 3, config=None)
+        mock_except_hook.assert_called_once_with(
+            1, 2, 3, debug=True, log_path=mock.ANY)
+
+        mock_root_logger = mock_get()
+        mock_root_logger.setLevel.assert_called_once_with(logging.DEBUG)
+        self.assertEqual(mock_root_logger.addHandler.call_count, 2)
+
+        MemoryHandler = logging.handlers.MemoryHandler
+        memory_handler = None
+        for call in mock_root_logger.addHandler.call_args_list:
+            handler = call[0][0]
+            if memory_handler is None and isinstance(handler, MemoryHandler):
+                memory_handler = handler
+            else:
+                self.assertTrue(isinstance(handler, logging.StreamHandler))
+        self.assertTrue(
+            isinstance(memory_handler.target, logging.StreamHandler))
 
 
 class PostArgParseSetupTest(test_util.TempDirTestCase):
@@ -46,9 +66,24 @@ class PostArgParseSetupTest(test_util.TempDirTestCase):
     def setUp(self):
         super(PostArgParseSetupTest, self).setUp()
         self.config = mock.MagicMock(
-            logs_dir=self.tempdir, quiet=False,
+            debug=False, logs_dir=self.tempdir, quiet=False,
             verbose_count=constants.CLI_DEFAULTS['verbose_count'])
-        self.root_logger = mock.MagicMock()
+        self.devnull = open(os.devnull, 'w')
+
+        from certbot.log import ColoredStreamHandler
+        self.stream_handler = ColoredStreamHandler(six.StringIO())
+        from certbot.log import MemoryHandler, TempHandler
+        self.temp_handler = TempHandler()
+        self.temp_path = self.temp_handler.path
+        self.memory_handler = MemoryHandler(self.temp_handler)
+        self.root_logger = mock.MagicMock(
+            handlers=[self.memory_handler, self.stream_handler])
+
+    def tearDown(self):
+        self.memory_handler.close()
+        self.stream_handler.close()
+        self.temp_handler.close()
+        super(PostArgParseSetupTest, self).tearDown()
 
     def test_common(self):
         with mock.patch('certbot.log.logging.getLogger') as mock_get_logger:
@@ -58,19 +93,26 @@ class PostArgParseSetupTest(test_util.TempDirTestCase):
                     mock_sys.version_info = sys.version_info
                     self._call(self.config)
 
-        self.assertEqual(self.root_logger.addHandler.call_count, 2)
+        self.root_logger.removeHandler.assert_called_once_with(
+            self.memory_handler)
+        self.assertTrue(self.root_logger.addHandler.called)
         self.assertTrue(os.path.exists(os.path.join(
             self.config.logs_dir, 'letsencrypt.log')))
+        self.assertFalse(os.path.exists(self.temp_path))
         mock_sys.excepthook(1, 2, 3)
-        mock_except_hook.assert_called_once_with(1, 2, 3, config=self.config)
+        mock_except_hook.assert_called_once_with(
+            1, 2, 3, debug=self.config.debug, log_path=self.tempdir)
 
-        stderr_handler = self.root_logger.addHandler.call_args_list[0][0][0]
+        level = self.stream_handler.level
-        level = stderr_handler.level
         if self.config.quiet:
             self.assertEqual(level, constants.QUIET_LOGGING_LEVEL)
         else:
             self.assertEqual(level, -self.config.verbose_count * 10)
 
+    def test_debug(self):
+        self.config.debug = True
+        self.test_common()
+
     def test_quiet(self):
         self.config.quiet = True
         self.test_common()
@@ -107,22 +149,25 @@ class SetupLogFileHandlerTest(test_util.TempDirTestCase):
 
         expected_path = os.path.join(self.config.logs_dir, log_file)
         self.assertEqual(log_path, expected_path)
+        handler.close()
 
 
 class ColoredStreamHandlerTest(unittest.TestCase):
-    """Tests for certbot.log."""
+    """Tests for certbot.log.ColoredStreamHandler"""
 
     def setUp(self):
-        from certbot import log
-
         self.stream = six.StringIO()
         self.stream.isatty = lambda: True
-        self.handler = log.ColoredStreamHandler(self.stream)
-
         self.logger = logging.getLogger()
         self.logger.setLevel(logging.DEBUG)
+
+        from certbot.log import ColoredStreamHandler
+        self.handler = ColoredStreamHandler(self.stream)
         self.logger.addHandler(self.handler)
 
+    def tearDown(self):
+        self.handler.close()
+
     def test_format(self):
         msg = 'I did a thing'
         self.logger.debug(msg)
@@ -139,6 +184,75 @@ class ColoredStreamHandlerTest(unittest.TestCase):
                                               util.ANSI_SGR_RESET))
 
 
+class MemoryHandlerTest(unittest.TestCase):
+    """Tests for certbot.log.MemoryHandler"""
+    def setUp(self):
+        self.logger = logging.getLogger(__name__)
+        self.logger.setLevel(logging.DEBUG)
+        self.msg = 'hi there'
+        self.stream = six.StringIO()
+
+        self.stream_handler = logging.StreamHandler(self.stream)
+        from certbot.log import MemoryHandler
+        self.handler = MemoryHandler(self.stream_handler)
+        self.logger.addHandler(self.handler)
+
+    def tearDown(self):
+        self.handler.close()
+        self.stream_handler.close()
+
+    def test_flush(self):
+        self._test_log_debug()
+        self.handler.flush()
+        self.assertEqual(self.stream.getvalue(), self.msg + '\n')
+
+    def test_not_flushed(self):
+        # By default, logging.ERROR messages and higher are flushed
+        self.logger.critical(self.msg)
+        self.assertEqual(self.stream.getvalue(), '')
+
+    def test_target_reset(self):
+        self._test_log_debug()
+
+        new_stream = six.StringIO()
+        new_stream_handler = logging.StreamHandler(new_stream)
+        self.handler.setTarget(new_stream_handler)
+        self.handler.flush()
+        self.assertEqual(self.stream.getvalue(), '')
+        self.assertEqual(new_stream.getvalue(), self.msg + '\n')
+        new_stream_handler.close()
+
+    def _test_log_debug(self):
+        self.logger.debug(self.msg)
+
+
+class TempHandlerTest(unittest.TestCase):
+    """Tests for certbot.log.TempHandler."""
+    def setUp(self):
+        self.closed = False
+        from certbot.log import TempHandler
+        self.handler = TempHandler()
+
+    def tearDown(self):
+        if not self.closed:
+            self.handler.delete_and_close()
+
+    def test_permissions(self):
+        self.assertTrue(
+            util.check_permissions(self.handler.path, 0o600, os.getuid()))
+
+    def test_delete(self):
+        self.handler.delete_and_close()
+        self.closed = True
+        self.assertFalse(os.path.exists(self.handler.path))
+
+    def test_no_delete(self):
+        self.handler.close()
+        self.closed = True
+        self.assertTrue(os.path.exists(self.handler.path))
+        os.remove(self.handler.path)
+
+
 class ExceptHookTest(unittest.TestCase):
     """Tests for certbot.log.except_hook."""
     @classmethod
@@ -146,53 +260,114 @@ class ExceptHookTest(unittest.TestCase):
         from certbot.log import except_hook
         return except_hook(*args, **kwargs)
 
-    @mock.patch('certbot.log.sys')
+    def setUp(self):
-    def test_except_hook(self, mock_sys):
+        self.error_msg = 'test error message'
-        config = mock.MagicMock()
+        self.log_path = 'foo.log'
-        mock_open = mock.mock_open()
 
-        with mock.patch('certbot.log.open', mock_open, create=True):
+    def test_base_exception(self):
-            exception = Exception('detail')
+        exc_type = KeyboardInterrupt
-            config.verbose_count = 1
+        mock_logger, output = self._test_common(exc_type, debug=False)
-            self._call(
+        self._assert_exception_logged(mock_logger.error, exc_type)
-                Exception, exc_value=exception, trace=None, config=None)
+        self._assert_logfile_output(output)
-            mock_open().write.assert_any_call(''.join(
-                traceback.format_exception_only(Exception, exception)))
-            error_msg = mock_sys.exit.call_args_list[0][0][0]
-            self.assertTrue('unexpected error' in error_msg)
 
-        with mock.patch('certbot.log.open', mock_open, create=True):
+    def test_debug(self):
-            mock_open.side_effect = [KeyboardInterrupt]
+        exc_type = ValueError
-            error = errors.Error('detail')
+        mock_logger, output = self._test_common(exc_type, debug=True)
-            self._call(
+        self._assert_exception_logged(mock_logger.error, exc_type)
-                errors.Error, exc_value=error, trace=None, config=None)
+        self._assert_logfile_output(output)
-            # assert_any_call used because sys.exit doesn't exit in cli.py
-            mock_sys.exit.assert_any_call(''.join(
-                traceback.format_exception_only(errors.Error, error)))
 
-        bad_typ = messages.ERROR_PREFIX + 'triffid'
+    def test_custom_error(self):
-        exception = messages.Error(detail='alpha', typ=bad_typ, title='beta')
+        exc_type = errors.PluginError
-        config = mock.MagicMock(debug=False, verbose_count=-3)
+        mock_logger, output = self._test_common(exc_type, debug=False)
-        self._call(
+        self._assert_exception_logged(mock_logger.debug, exc_type)
-            messages.Error, exc_value=exception, trace=None, config=config)
+        self._assert_quiet_output(mock_logger, output)
-        error_msg = mock_sys.exit.call_args_list[-1][0][0]
-        self.assertTrue('unexpected error' in error_msg)
-        self.assertTrue('acme:error' not in error_msg)
-        self.assertTrue('alpha' in error_msg)
-        self.assertTrue('beta' in error_msg)
-        config = mock.MagicMock(debug=False, verbose_count=1)
-        self._call(
-            messages.Error, exc_value=exception, trace=None, config=config)
-        error_msg = mock_sys.exit.call_args_list[-1][0][0]
-        self.assertTrue('unexpected error' in error_msg)
-        self.assertTrue('acme:error' in error_msg)
-        self.assertTrue('alpha' in error_msg)
 
-        interrupt = KeyboardInterrupt('detail')
+    def test_acme_error(self):
-        self._call(
+        # Get an arbitrary error code
-            KeyboardInterrupt, exc_value=interrupt, trace=None, config=None)
+        acme_code = next(six.iterkeys(messages.ERROR_CODES))
-        mock_sys.exit.assert_called_with(''.join(
+
-            traceback.format_exception_only(KeyboardInterrupt, interrupt)))
+        def get_acme_error(msg):
+            """Wraps ACME errors so the constructor takes only a msg."""
+            return messages.Error.with_code(acme_code, detail=msg)
+
+        mock_logger, output = self._test_common(get_acme_error, debug=False)
+        self._assert_exception_logged(mock_logger.debug, messages.Error)
+        self._assert_quiet_output(mock_logger, output)
+        self.assertFalse(messages.ERROR_PREFIX in output)
+
+    def test_other_error(self):
+        exc_type = ValueError
+        mock_logger, output = self._test_common(exc_type, debug=False)
+        self._assert_exception_logged(mock_logger.debug, exc_type)
+        self._assert_quiet_output(mock_logger, output)
+
+    def _test_common(self, error_type, debug):
+        """Returns the mocked logger and stderr output."""
+        mock_err = six.StringIO()
+        try:
+            raise error_type(self.error_msg)
+        except BaseException:
+            exc_info = sys.exc_info()
+            with mock.patch('certbot.log.logger') as mock_logger:
+                with mock.patch('certbot.log.sys.stderr', mock_err):
+                    try:
+                        # pylint: disable=star-args
+                        self._call(
+                            *exc_info, debug=debug, log_path=self.log_path)
+                    except SystemExit as exit_err:
+                        mock_err.write(str(exit_err))
+                    else:  # pragma: no cover
+                        self.fail('SystemExit not raised.')
+
+        output = mock_err.getvalue()
+        return mock_logger, output
+
+    def _assert_exception_logged(self, log_func, exc_type):
+        self.assertTrue(log_func.called)
+        call_kwargs = log_func.call_args[1]
+        self.assertTrue('exc_info' in call_kwargs)
+
+        actual_exc_info = call_kwargs['exc_info']
+        expected_exc_info = (exc_type, mock.ANY, mock.ANY)
+        self.assertEqual(actual_exc_info, expected_exc_info)
+
+    def _assert_logfile_output(self, output):
+        self.assertTrue('Please see the logfile' in output)
+        self.assertTrue(self.log_path in output)
+
+    def _assert_quiet_output(self, mock_logger, output):
+        self.assertFalse(mock_logger.exception.called)
+        self.assertTrue(mock_logger.debug.called)
+        self.assertTrue(self.error_msg in output)
+
+
+class ExitWithLogPathTest(test_util.TempDirTestCase):
+    """Tests for certbot.log.exit_with_log_path."""
+    @classmethod
+    def _call(cls, *args, **kwargs):
+        from certbot.log import exit_with_log_path
+        return exit_with_log_path(*args, **kwargs)
+
+    def test_log_file(self):
+        log_file = os.path.join(self.tempdir, 'test.log')
+        open(log_file, 'w').close()
+
+        err_str = self._test_common(log_file)
+        self.assertTrue('logfiles' not in err_str)
+        self.assertTrue(log_file in err_str)
+
+    def test_log_dir(self):
+        err_str = self._test_common(self.tempdir)
+        self.assertTrue('logfiles' in err_str)
+        self.assertTrue(self.tempdir in err_str)
+
+    def _test_common(self, *args, **kwargs):
+        try:
+            self._call(*args, **kwargs)
+        except SystemExit as err:
+            return str(err)
+        else:  # pragma: no cover
+            self.fail('SystemExit was not raised.')
 
 
 if __name__ == "__main__":