nginx: reduced CLI logging when reloading nginx (#8237)

* nginx: reduced CLI logging when reloading nginx

Hides the output of `nginx -s reload` from the CLI, moving it to
debug-level logging.

Additionally, fixes an issue where Certbot did not properly capture the
output of the nginx reload and restart commands.

Fixes #8231

* remove leftover debugging

* reorder CHANGELOG

* don't use bare asserts

certbot-nginx/certbot_nginx/_internal/configurator.py

@@ -1175,23 +1175,29 @@ def nginx_restart(nginx_ctl, nginx_conf, sleep_duration):
 
     """
     try:
-        proc = subprocess.Popen([nginx_ctl, "-c", nginx_conf, "-s", "reload"],
+        reload_output = "" # type: unicode
-                                env=util.env_no_snap_for_external_calls())
+        with tempfile.TemporaryFile() as out:
-        proc.communicate()
+            proc = subprocess.Popen([nginx_ctl, "-c", nginx_conf, "-s", "reload"],
+                                    env=util.env_no_snap_for_external_calls(),
+                                    stdout=out, stderr=out)
+            proc.communicate()
+            out.seek(0)
+            reload_output = out.read().decode("utf-8")
 
         if proc.returncode != 0:
-            # Maybe Nginx isn't running
+            logger.debug("nginx reload failed:\n%s", reload_output)
+            # Maybe Nginx isn't running - try start it
             # Write to temporary files instead of piping because of communication issues on Arch
             # https://github.com/certbot/certbot/issues/4324
             with tempfile.TemporaryFile() as out:
-                with tempfile.TemporaryFile() as err:
+                nginx_proc = subprocess.Popen([nginx_ctl, "-c", nginx_conf],
-                    nginx_proc = subprocess.Popen([nginx_ctl, "-c", nginx_conf],
+                    stdout=out, stderr=out, env=util.env_no_snap_for_external_calls())
-                        stdout=out, stderr=err, env=util.env_no_snap_for_external_calls())
+                nginx_proc.communicate()
-                    nginx_proc.communicate()
+                if nginx_proc.returncode != 0:
-                    if nginx_proc.returncode != 0:
+                    out.seek(0)
-                        # Enter recovery routine...
+                    # Enter recovery routine...
-                        raise errors.MisconfigurationError(
+                    raise errors.MisconfigurationError(
-                            "nginx restart failed:\n%s\n%s" % (out.read(), err.read()))
+                        "nginx restart failed:\n%s" % out.read().decode("utf-8"))
 
     except (OSError, ValueError):
         raise errors.MisconfigurationError("nginx restart failed")

certbot-nginx/tests/configurator_test.py

@@ -466,14 +466,18 @@ class NginxConfiguratorTest(util.NginxTest):
         mocked.communicate.return_value = ('', '')
         mocked.returncode = 0
         self.config.restart()
+        self.assertEqual(mocked.communicate.call_count, 1)
         mock_time.sleep.assert_called_once_with(0.1234)
 
     @mock.patch("certbot_nginx._internal.configurator.subprocess.Popen")
-    def test_nginx_restart_fail(self, mock_popen):
+    @mock.patch("certbot_nginx._internal.configurator.logger.debug")
+    def test_nginx_restart_fail(self, mock_log_debug, mock_popen):
         mocked = mock_popen()
         mocked.communicate.return_value = ('', '')
         mocked.returncode = 1
         self.assertRaises(errors.MisconfigurationError, self.config.restart)
+        self.assertEqual(mocked.communicate.call_count, 2)
+        mock_log_debug.assert_called_once_with("nginx reload failed:\n%s", "")
 
     @mock.patch("certbot_nginx._internal.configurator.subprocess.Popen")
     def test_no_nginx_start(self, mock_popen):

certbot/CHANGELOG.md

@@ -13,6 +13,7 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
 
 * Update the packaging instructions to promote usage of `python -m pytest` to test Certbot
   instead of the deprecated `python setup.py test` setuptools approach.
+* Reduced CLI logging when reloading nginx, if it is not running.
 
 ### Fixed