nginx,apache: CLI logging changes

- Add "Successfully deployed ..." message using display_util
- Remove IReporter usage and replace with display_util
- Standardize "... could not find a VirtualHost ..." error

This changes also bumps the version of certbot required by certbot-nginx
and certbot-apache to take use of the new display_util function.

certbot-apache/certbot_apache/_internal/configurator.py

@@ -29,6 +29,7 @@ from certbot import util
 from certbot.achallenges import KeyAuthorizationAnnotatedChallenge  # pylint: disable=unused-import
 from certbot.compat import filesystem
 from certbot.compat import os
+from certbot.display import util as display_util
 from certbot.plugins import common
 from certbot.plugins.enhancements import AutoHSTSEnhancement
 from certbot.plugins.util import path_surgery
@@ -484,6 +485,8 @@ class ApacheConfigurator(common.Installer):
         vhosts = self.choose_vhosts(domain)
         for vhost in vhosts:
             self._deploy_cert(vhost, cert_path, key_path, chain_path, fullchain_path)
+            display_util.notify("Successfully deployed certificate for {} to {}"
+                                .format(domain, vhost.filep))
 
     def choose_vhosts(self, domain, create_if_no_ssl=True):
         """
@@ -522,6 +525,20 @@ class ApacheConfigurator(common.Installer):
 
         return list(matched)
 
+    def _raise_no_suitable_vhost_error(self, target_name):
+        # type: (str) -> None
+        """
+        Notifies the user that Certbot could not find a vhost to secure
+        and raises an error.
+        :param str target_name: The server name that could not be mapped
+        :raises errors.PluginError: Raised unconditionally
+        """
+        raise errors.PluginError(
+            "Certbot could not find a VirtualHost for {0} in the Apache "
+            "configuration. Please create a VirtualHost with a ServerName "
+            "matching {0} and try again.".format(target_name)
+        )
+
     def _in_wildcard_scope(self, name, domain):
         """
         Helper method for _vhosts_for_wildcard() that makes sure that the domain
@@ -559,12 +576,7 @@ class ApacheConfigurator(common.Installer):
         dialog_output = display_ops.select_vhost_multiple(list(dialog_input))
 
         if not dialog_output:
-            logger.error(
-                "No vhost exists with servername or alias for domain %s. "
-                "No vhost was selected. Please specify ServerName or ServerAlias "
-                "in the Apache config.",
-                domain)
-            raise errors.PluginError("No vhost selected")
+            self._raise_no_suitable_vhost_error(domain)
 
         # Make sure we create SSL vhosts for the ones that are HTTP only
         # if requested.
@@ -688,12 +700,7 @@ class ApacheConfigurator(common.Installer):
         # Select a vhost from a list
         vhost = display_ops.select_vhost(target_name, self.vhosts)
         if vhost is None:
-            logger.error(
-                "No vhost exists with servername or alias of %s. "
-                "No vhost was selected. Please specify ServerName or ServerAlias "
-                "in the Apache config.",
-                target_name)
-            raise errors.PluginError("No vhost selected")
+            self._raise_no_suitable_vhost_error(target_name)
         if temp:
             return vhost
         if not vhost.ssl:
@@ -1498,12 +1505,12 @@ class ApacheConfigurator(common.Installer):
             raise errors.PluginError("Unable to write/read in make_vhost_ssl")
 
         if sift:
-            reporter = zope.component.getUtility(interfaces.IReporter)
-            reporter.add_message(
-                "Some rewrite rules copied from {0} were disabled in the "
-                "vhost for your HTTPS site located at {1} because they have "
-                "the potential to create redirection loops.".format(
-                    vhost.filep, ssl_fp), reporter.MEDIUM_PRIORITY)
+            display_util.notify(
+                "Some rewrite rules copied from {src_path} were disabled in the "
+                "vhost for your HTTPS site located at {dest_path} because they have "
+                "the potential to create redirection loops."
+                .format(src_path=vhost.filep, dest_path=ssl_fp)
+            )
         self.parser.aug.set("/augeas/files%s/mtime" % (self._escape(ssl_fp)), "0")
         self.parser.aug.set("/augeas/files%s/mtime" % (self._escape(vhost.filep)), "0")
 

certbot-apache/local-oldest-requirements.txt

@@ -1,3 +1,3 @@
 # Remember to update setup.py to match the package versions below.
 acme[dev]==0.29.0
-certbot[dev]==1.6.0
+-e certbot[dev]

certbot-apache/setup.py

@@ -11,7 +11,7 @@ version = '1.11.0.dev0'
 # acme/certbot version.
 install_requires = [
     'acme>=0.29.0',
-    'certbot>=1.6.0',
+    'certbot>=1.10.0.dev0',
     'python-augeas',
     'setuptools',
     'zope.component',

certbot-apache/tests/centos6_test.py

@@ -1,6 +1,12 @@
 """Test for certbot_apache._internal.configurator for CentOS 6 overrides"""
 import unittest
 
+try:
+    import mock
+except ImportError: # pragma: no cover
+    from unittest import mock # type: ignore
+import six  # pylint: disable=unused-import  # six is used in mock.patch()
+
 from certbot.compat import os
 from certbot.errors import MisconfigurationError
 from certbot_apache._internal import obj
@@ -9,6 +15,7 @@ from certbot_apache._internal import parser
 import util
 
 
+
 def get_vh_truth(temp_dir, config_name):
     """Return the ground truth for the specified directory."""
     prefix = os.path.join(
@@ -65,7 +72,8 @@ class CentOS6Tests(util.ApacheTest):
                 raise Exception("Missed: %s" % vhost)  # pragma: no cover
         self.assertEqual(found, 2)
 
-    def test_loadmod_default(self):
+    @mock.patch("certbot_apache._internal.configurator.display_util.notify")
+    def test_loadmod_default(self, unused_mock_notify):
         ssl_loadmods = self.config.parser.find_dir(
             "LoadModule", "ssl_module", exclude=False)
         self.assertEqual(len(ssl_loadmods), 1)
@@ -95,7 +103,8 @@ class CentOS6Tests(util.ApacheTest):
             ifmod_args = self.config.parser.get_all_args(lm[:-17])
             self.assertTrue("!mod_ssl.c" in ifmod_args)
 
-    def test_loadmod_multiple(self):
+    @mock.patch("certbot_apache._internal.configurator.display_util.notify")
+    def test_loadmod_multiple(self, unused_mock_notify):
         sslmod_args = ["ssl_module", "modules/mod_ssl.so"]
         # Adds another LoadModule to main httpd.conf in addtition to ssl.conf
         self.config.parser.add_dir(self.config.parser.loc["default"], "LoadModule",
@@ -115,7 +124,8 @@ class CentOS6Tests(util.ApacheTest):
         for mod in post_loadmods:
             self.assertTrue(self.config.parser.not_modssl_ifmodule(mod))  #pylint: disable=no-member
 
-    def test_loadmod_rootconf_exists(self):
+    @mock.patch("certbot_apache._internal.configurator.display_util.notify")
+    def test_loadmod_rootconf_exists(self, unused_mock_notify):
         sslmod_args = ["ssl_module", "modules/mod_ssl.so"]
         rootconf_ifmod = self.config.parser.get_ifmod(
             parser.get_aug_path(self.config.parser.loc["default"]),
@@ -142,7 +152,8 @@ class CentOS6Tests(util.ApacheTest):
             self.config.parser.get_all_args(mods[0][:-7]),
             sslmod_args)
 
-    def test_neg_loadmod_already_on_path(self):
+    @mock.patch("certbot_apache._internal.configurator.display_util.notify")
+    def test_neg_loadmod_already_on_path(self, unused_mock_notify):
         loadmod_args = ["ssl_module", "modules/mod_ssl.so"]
         ifmod = self.config.parser.get_ifmod(
             self.vh_truth[1].path, "!mod_ssl.c", beginning=True)
@@ -185,7 +196,8 @@ class CentOS6Tests(util.ApacheTest):
         # Make sure that none was changed
         self.assertEqual(pre_matches, post_matches)
 
-    def test_loadmod_not_found(self):
+    @mock.patch("certbot_apache._internal.configurator.display_util.notify")
+    def test_loadmod_not_found(self, unused_mock_notify):
         # Remove all existing LoadModule ssl_module... directives
         orig_loadmods = self.config.parser.find_dir("LoadModule",
                                                     "ssl_module",

certbot-apache/tests/configurator_test.py

@@ -339,7 +339,8 @@ class MultipleVhostsTest(util.ApacheTest):
         vhosts = self.config._non_default_vhosts(self.config.vhosts)
         self.assertEqual(len(vhosts), 10)
 
-    def test_deploy_cert_enable_new_vhost(self):
+    @mock.patch('certbot_apache._internal.configurator.display_util.notify')
+    def test_deploy_cert_enable_new_vhost(self, unused_mock_notify):
         # Create
         ssl_vhost = self.config.make_vhost_ssl(self.vh_truth[0])
         self.config.parser.modules["ssl_module"] = None
@@ -377,7 +378,8 @@ class MultipleVhostsTest(util.ApacheTest):
                 self.fail("Include shouldn't be added, as patched find_dir 'finds' existing one") \
                     # pragma: no cover
 
-    def test_deploy_cert(self):
+    @mock.patch('certbot_apache._internal.configurator.display_util.notify')
+    def test_deploy_cert(self, unused_mock_notify):
         self.config.parser.modules["ssl_module"] = None
         self.config.parser.modules["mod_ssl.c"] = None
         self.config.parser.modules["socache_shmcb_module"] = None
@@ -1292,7 +1294,8 @@ class MultipleVhostsTest(util.ApacheTest):
             os.path.basename(inc_path) in self.config.parser.existing_paths[
                 os.path.dirname(inc_path)])
 
-    def test_deploy_cert_not_parsed_path(self):
+    @mock.patch('certbot_apache._internal.configurator.display_util.notify')
+    def test_deploy_cert_not_parsed_path(self, unused_mock_notify):
         # Make sure that we add include to root config for vhosts when
         # handle-sites is false
         self.config.parser.modules["ssl_module"] = None
@@ -1388,7 +1391,8 @@ class MultipleVhostsTest(util.ApacheTest):
             self.assertEqual(vhs[0], self.vh_truth[7])
 
 
-    def test_deploy_cert_wildcard(self):
+    @mock.patch('certbot_apache._internal.configurator.display_util.notify')
+    def test_deploy_cert_wildcard(self, unused_mock_notify):
         # pylint: disable=protected-access
         mock_choose_vhosts = mock.MagicMock()
         mock_choose_vhosts.return_value = [self.vh_truth[7]]
@@ -1608,8 +1612,8 @@ class MultiVhostsTest(util.ApacheTest):
         self.assertEqual(self.config._get_new_vh_path(without_index, both),
                          with_index_2[0])
 
-    @certbot_util.patch_get_utility()
-    def test_make_vhost_ssl_with_existing_rewrite_rule(self, mock_get_utility):
+    @mock.patch("certbot_apache._internal.configurator.display_util.notify")
+    def test_make_vhost_ssl_with_existing_rewrite_rule(self, mock_notify):
         self.config.parser.modules["rewrite_module"] = None
 
         ssl_vhost = self.config.make_vhost_ssl(self.vh_truth[4])
@@ -1625,11 +1629,11 @@ class MultiVhostsTest(util.ApacheTest):
                                     "\"http://new.example.com/docs/$1\"  [R,L]")
         self.assertTrue(commented_rewrite_rule in conf_text)
         self.assertTrue(uncommented_rewrite_rule in conf_text)
-        mock_get_utility().add_message.assert_called_once_with(mock.ANY,
-                                                               mock.ANY)
+        self.assertEqual(mock_notify.call_count, 1)
+        self.assertTrue("Some rewrite rules" in mock_notify.call_args[0][0])
 
-    @certbot_util.patch_get_utility()
-    def test_make_vhost_ssl_with_existing_rewrite_conds(self, mock_get_utility):
+    @mock.patch("certbot_apache._internal.configurator.display_util.notify")
+    def test_make_vhost_ssl_with_existing_rewrite_conds(self, mock_notify):
         self.config.parser.modules["rewrite_module"] = None
 
         ssl_vhost = self.config.make_vhost_ssl(self.vh_truth[3])
@@ -1654,8 +1658,8 @@ class MultiVhostsTest(util.ApacheTest):
         self.assertTrue(commented_cond1 in conf_line_set)
         self.assertTrue(commented_cond2 in conf_line_set)
         self.assertTrue(commented_rewrite_rule in conf_line_set)
-        mock_get_utility().add_message.assert_called_once_with(mock.ANY,
-                                                               mock.ANY)
+        self.assertEqual(mock_notify.call_count, 1)
+        self.assertTrue("Some rewrite rules" in mock_notify.call_args[0][0])
 
 
 class InstallSslOptionsConfTest(util.ApacheTest):

certbot-nginx/certbot_nginx/_internal/configurator.py

@@ -21,6 +21,7 @@ from certbot import crypto_util
 from certbot import errors
 from certbot import interfaces
 from certbot import util
+from certbot.display import util as display_util
 from certbot.compat import os
 from certbot.plugins import common
 from certbot_nginx._internal import constants
@@ -231,6 +232,8 @@ class NginxConfigurator(common.Installer):
         vhosts = self.choose_vhosts(domain, create_if_no_match=True)
         for vhost in vhosts:
             self._deploy_cert(vhost, cert_path, key_path, chain_path, fullchain_path)
+            display_util.notify("Successfully deployed certificate for {} to {}"
+                                .format(domain, vhost.filep))
 
     def _deploy_cert(self, vhost, cert_path, key_path, chain_path, fullchain_path):  # pylint: disable=unused-argument
         """

certbot-nginx/local-oldest-requirements.txt

@@ -1,3 +1,3 @@
 # Remember to update setup.py to match the package versions below.
 acme[dev]==1.6.0
-certbot[dev]==1.6.0
+-e certbot[dev]

certbot-nginx/setup.py

@@ -11,7 +11,7 @@ version = '1.11.0.dev0'
 # acme/certbot version.
 install_requires = [
     'acme>=1.4.0',
-    'certbot>=1.6.0',
+    'certbot>=1.10.0.dev0',
     'PyOpenSSL',
     'pyparsing>=1.5.5',  # Python3 support
     'setuptools',

certbot-nginx/tests/configurator_test.py

@@ -31,6 +31,10 @@ class NginxConfiguratorTest(util.NginxTest):
         self.config = self.get_nginx_configurator(
             self.config_path, self.config_dir, self.work_dir, self.logs_dir)
 
+        patch = mock.patch('certbot_nginx._internal.configurator.display_util.notify')
+        self.mock_notify = patch.start()
+        self.addCleanup(patch.stop)
+
     @mock.patch("certbot_nginx._internal.configurator.util.exe_exists")
     def test_prepare_no_install(self, mock_exe_exists):
         mock_exe_exists.return_value = False