From 71d9dfa86ee78907b73feb161b304394445f3bbb Mon Sep 17 00:00:00 2001
From: alexzorin <alex@zorin.id.au>
Date: Thu, 17 Sep 2020 05:22:15 +1000
Subject: [PATCH] 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/_internal/configurator.py   | 30 +++++++++++--------
 certbot-nginx/tests/configurator_test.py      |  6 +++-
 certbot/CHANGELOG.md                          |  1 +
 3 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/certbot-nginx/certbot_nginx/_internal/configurator.py b/certbot-nginx/certbot_nginx/_internal/configurator.py
index 3da38ff2d..7bd5d2e42 100644
--- a/certbot-nginx/certbot_nginx/_internal/configurator.py
+++ b/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"],
-                                env=util.env_no_snap_for_external_calls())
-        proc.communicate()
+        reload_output = "" # type: unicode
+        with tempfile.TemporaryFile() as out:
+            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],
-                        stdout=out, stderr=err, env=util.env_no_snap_for_external_calls())
-                    nginx_proc.communicate()
-                    if nginx_proc.returncode != 0:
-                        # Enter recovery routine...
-                        raise errors.MisconfigurationError(
-                            "nginx restart failed:\n%s\n%s" % (out.read(), err.read()))
+                nginx_proc = subprocess.Popen([nginx_ctl, "-c", nginx_conf],
+                    stdout=out, stderr=out, env=util.env_no_snap_for_external_calls())
+                nginx_proc.communicate()
+                if nginx_proc.returncode != 0:
+                    out.seek(0)
+                    # Enter recovery routine...
+                    raise errors.MisconfigurationError(
+                        "nginx restart failed:\n%s" % out.read().decode("utf-8"))
 
     except (OSError, ValueError):
         raise errors.MisconfigurationError("nginx restart failed")
diff --git a/certbot-nginx/tests/configurator_test.py b/certbot-nginx/tests/configurator_test.py
index 0a5adcd3f..e677ad471 100644
--- a/certbot-nginx/tests/configurator_test.py
+++ b/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):
diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md
index a0c8c5818..d4b7ac21b 100644
--- a/certbot/CHANGELOG.md
+++ b/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