From 8a0abed1baf1dda31ced731bbc7bea4bb0355863 Mon Sep 17 00:00:00 2001
From: Rick Elrod <rick@elrod.me>
Date: Thu, 9 Jul 2020 22:54:06 -0500
Subject: [PATCH] [hostname] don't write in get_*() methods

Change:
- Hostname strategies' get_*() methods should never write to the
  filesystem. They are used in check_mode by default to determine if
  there is any work to be done.

Test Plan:
- New unit tests to ensure that (at least when in check_mode) the get
  methods don't ever call write.

Tickets:
- Fixes #66432

Signed-off-by: Rick Elrod <rick@elrod.me>
---
 .../66432_hostname_check_mode_writes.yml      |  2 ++
 lib/ansible/modules/hostname.py               | 36 ++++++-------------
 test/units/modules/test_hostname.py           | 35 ++++++++++++++++++
 3 files changed, 47 insertions(+), 26 deletions(-)
 create mode 100644 changelogs/fragments/66432_hostname_check_mode_writes.yml
 create mode 100644 test/units/modules/test_hostname.py

diff --git a/changelogs/fragments/66432_hostname_check_mode_writes.yml b/changelogs/fragments/66432_hostname_check_mode_writes.yml
new file mode 100644
index 00000000000..c5108468d6d
--- /dev/null
+++ b/changelogs/fragments/66432_hostname_check_mode_writes.yml
@@ -0,0 +1,2 @@
+bugfixes:
+  - hostname - No longer modifies system files in get_* methods consulted when in check_mode (https://github.com/ansible/ansible/issues/66432)
diff --git a/lib/ansible/modules/hostname.py b/lib/ansible/modules/hostname.py
index 75e7edb1839..2d55c903006 100644
--- a/lib/ansible/modules/hostname.py
+++ b/lib/ansible/modules/hostname.py
@@ -237,11 +237,8 @@ class DebianStrategy(GenericStrategy):
 
     def get_permanent_hostname(self):
         if not os.path.isfile(self.HOSTNAME_FILE):
-            try:
-                open(self.HOSTNAME_FILE, "a").write("")
-            except IOError as e:
-                self.module.fail_json(msg="failed to write file: %s" %
-                                          to_native(e), exception=traceback.format_exc())
+            return ''
+
         try:
             f = open(self.HOSTNAME_FILE)
             try:
@@ -273,11 +270,8 @@ class SLESStrategy(GenericStrategy):
 
     def get_permanent_hostname(self):
         if not os.path.isfile(self.HOSTNAME_FILE):
-            try:
-                open(self.HOSTNAME_FILE, "a").write("")
-            except IOError as e:
-                self.module.fail_json(msg="failed to write file: %s" %
-                                          to_native(e), exception=traceback.format_exc())
+            return ''
+
         try:
             f = open(self.HOSTNAME_FILE)
             try:
@@ -362,11 +356,8 @@ class AlpineStrategy(GenericStrategy):
 
     def get_permanent_hostname(self):
         if not os.path.isfile(self.HOSTNAME_FILE):
-            try:
-                open(self.HOSTNAME_FILE, "a").write("")
-            except IOError as e:
-                self.module.fail_json(msg="failed to write file: %s" %
-                                          to_native(e), exception=traceback.format_exc())
+            return ''
+
         try:
             f = open(self.HOSTNAME_FILE)
             try:
@@ -497,11 +488,8 @@ class OpenBSDStrategy(GenericStrategy):
 
     def get_permanent_hostname(self):
         if not os.path.isfile(self.HOSTNAME_FILE):
-            try:
-                open(self.HOSTNAME_FILE, "a").write("")
-            except IOError as e:
-                self.module.fail_json(msg="failed to write file: %s" %
-                                          to_native(e), exception=traceback.format_exc())
+            return ''
+
         try:
             f = open(self.HOSTNAME_FILE)
             try:
@@ -562,14 +550,10 @@ class FreeBSDStrategy(GenericStrategy):
     HOSTNAME_FILE = '/etc/rc.conf.d/hostname'
 
     def get_permanent_hostname(self):
-
         name = 'UNKNOWN'
         if not os.path.isfile(self.HOSTNAME_FILE):
-            try:
-                open(self.HOSTNAME_FILE, "a").write("hostname=temporarystub\n")
-            except IOError as e:
-                self.module.fail_json(msg="failed to write file: %s" %
-                                          to_native(e), exception=traceback.format_exc())
+            return ''
+
         try:
             try:
                 f = open(self.HOSTNAME_FILE, 'r')
diff --git a/test/units/modules/test_hostname.py b/test/units/modules/test_hostname.py
new file mode 100644
index 00000000000..ddfeef41bb7
--- /dev/null
+++ b/test/units/modules/test_hostname.py
@@ -0,0 +1,35 @@
+from __future__ import (absolute_import, division, print_function)
+__metaclass__ = type
+
+from units.compat.mock import patch, MagicMock, mock_open
+from ansible.module_utils import basic
+from ansible.module_utils.common._utils import get_all_subclasses
+from ansible.modules import hostname
+from units.modules.utils import ModuleTestCase, set_module_args
+from ansible.module_utils.six import PY2
+
+
+class TestHostname(ModuleTestCase):
+    @patch('os.path.isfile')
+    def test_stategy_get_never_writes_in_check_mode(self, isfile):
+        isfile.return_value = True
+
+        set_module_args({'name': 'fooname', '_ansible_check_mode': True})
+        subclasses = get_all_subclasses(hostname.GenericStrategy)
+        module = MagicMock()
+        for cls in subclasses:
+            instance = cls(module)
+
+            instance.module.run_command = MagicMock()
+            instance.module.run_command.return_value = (0, '', '')
+
+            m = mock_open()
+            builtins = 'builtins'
+            if PY2:
+                builtins = '__builtin__'
+            with patch('%s.open' % builtins, m):
+                instance.get_permanent_hostname()
+                instance.get_current_hostname()
+                self.assertFalse(
+                    m.return_value.write.called,
+                    msg='%s called write, should not have' % str(cls))