Merge ~larsks/cloud-init:lp/1644064 into cloud-init:master

[Lars Kellogg-Stedman]

util: teach write_file to default to existing file mode

On centos/fedora/rhel/derivatives, /etc/ssh/sshd_config has mode 0600,
but cloud-init unilaterally sets file modes to 0644 when no explicit
mode is passed to util.write_file. With this patch, write_file will
by default check if the target file already exists, and if so will use
the mode of that file by default unless an explicit mode is passed by
the caller.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:3cd3081fe697d9544b36ecd6552442663da2374a
https://jenkins.ubuntu.com/server/job/cloud-init-ci/221/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/221
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/221
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/221
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/221
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/221

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/221/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

This looks good, but there is one issue, in that you "fixed" /etc/ssh/sshd_config's permissions
by changing the behavior of write_file(filename, "foo").

That means all callers that do not pass 'mode' will now have (possibly) changed behavior.

The other issue here is that there is no way for the caller to say "use existing, but if new, use 0o755".

we could change the signature to include a 'reuse_existing_mode' (with open bikeshedding for a better name) or some other way indicate that the perms on the existing file should be used if it exists.

Lastly... atomic_helper.write_file currently has the same signature as util.write_file and i'd like to keep that, do lets plan on updating it as well.

review: Needs Fixing

[Lars Kellogg-Stedman (larsks)]

I've submitted an update to this patch that:

- introduces a new "copy_mode" argument to write_file (which defaults to False)
- updates the test to reflect the new parameter
- updates config/cc_set_passwords.py to pass copy_mode=True to write_file when writing sshd_config
- updates write_file in atomic_helper to behave the same way

> The other issue here is that there is no way for the caller to say "use existing, but if new, use 0o755".

This is now possible.

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:84c5efc58157da68ca19ea69ec220736ced38166
https://jenkins.ubuntu.com/server/job/cloud-init-ci/234/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/234/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/234/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/234/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/234/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/234/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/234/rebuild

review: Needs Fixing (continuous-integration)

[Lars Kellogg-Stedman (larsks)]

Aaaaand fixed the pep8 error.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:94ab1f5d96090a89c4a2be5851b3c2032f82caf5
https://jenkins.ubuntu.com/server/job/cloud-init-ci/248/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/248
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/248
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/248
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/248
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/248

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/248/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

The commit message now needs update to match the code changes.

The code changes and tests look good though. thank you.

review: Needs Fixing

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:4ac7f308c826090d3cb4ef8833227d07c23b6e1a
https://jenkins.ubuntu.com/server/job/cloud-init-ci/249/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/249
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/249
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/249
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/249
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/249

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/249/rebuild

review: Approve (continuous-integration)

[Lars Kellogg-Stedman (larsks)]

I've updated the commit message.

diff --git a/cloudinit/atomic_helper.py b/cloudinit/atomic_helper.py
index fb2df8d..587b994 100644
--- a/cloudinit/atomic_helper.py
+++ b/cloudinit/atomic_helper.py
@@ -2,13 +2,23 @@
 
 import json
 import os
+import stat
 import tempfile
 
 _DEF_PERMS = 0o644
 
 
-def write_file(filename, content, mode=_DEF_PERMS, omode="wb"):
+def write_file(filename, content, mode=_DEF_PERMS,
+               omode="wb", copy_mode=False):
     # open filename in mode 'omode', write content, set permissions to 'mode'
+
+    if copy_mode:
+        try:
+            file_stat = os.stat(filename)
+            mode = stat.S_IMODE(file_stat.st_mode)
+        except OSError:
+            pass
+
     tf = None
     try:
         tf = tempfile.NamedTemporaryFile(dir=os.path.dirname(filename),
diff --git a/cloudinit/config/cc_set_passwords.py b/cloudinit/config/cc_set_passwords.py
index eb0bdab..bb24d57 100755
--- a/cloudinit/config/cc_set_passwords.py
+++ b/cloudinit/config/cc_set_passwords.py
@@ -215,7 +215,8 @@ def handle(_name, cfg, cloud, log, args):
                                                      pw_auth))
 
         lines = [str(l) for l in new_lines]
-        util.write_file(ssh_util.DEF_SSHD_CFG, "\n".join(lines))
+        util.write_file(ssh_util.DEF_SSHD_CFG, "\n".join(lines),
+                        copy_mode=True)
 
         try:
             cmd = cloud.distro.init_cmd  # Default service
diff --git a/cloudinit/util.py b/cloudinit/util.py
index 17abdf8..6940850 100644
--- a/cloudinit/util.py
+++ b/cloudinit/util.py
@@ -1688,7 +1688,7 @@ def chmod(path, mode):
             os.chmod(path, real_mode)
 
 
-def write_file(filename, content, mode=0o644, omode="wb"):
+def write_file(filename, content, mode=0o644, omode="wb", copy_mode=False):
     """
     Writes a file with the given content and sets the file mode as specified.
     Resotres the SELinux context if possible.
@@ -1698,6 +1698,14 @@ def write_file(filename, content, mode=0o644, omode="wb"):
     @param mode: The filesystem mode to set on the file.
     @param omode: The open mode used when opening the file (w, wb, a, etc.)
     """
+
+    if copy_mode:
+        try:
+            file_stat = os.stat(filename)
+            mode = stat.S_IMODE(file_stat.st_mode)
+        except OSError:
+            pass
+
     ensure_dir(os.path.dirname(filename))
     if 'b' in omode.lower():
         content = encode_text(content)
diff --git a/tests/unittests/test_util.py b/tests/unittests/test_util.py
index ab74311..5d21b4b 100644
--- a/tests/unittests/test_util.py
+++ b/tests/unittests/test_util.py
@@ -103,8 +103,8 @@ class TestWriteFile(helpers.TestCase):
         self.assertTrue(os.path.isdir(dirname))
         self.assertTrue(os.path.isfile(path))
 
-    def test_custom_mode(self):
-        """Verify custom mode works properly."""
+    def test_explicit_mode(self):
+        """Verify explicit file mode works properly."""
         path = os.path.join(self.tmp, "NewFile.txt")
         contents = "Hey there"
 
@@ -115,6 +115,35 @@ class TestWriteFile(helpers.TestCase):
         file_stat = os.stat(path)
         self.assertEqual(0o666, stat.S_IMODE(file_stat.st_mode))
 
+    def test_copy_mode_no_existing(self):
+        """Verify that file is created with mode 0o644 if copy_mode
+        is true and there is no prior existing file."""
+        path = os.path.join(self.tmp, "NewFile.txt")
+        contents = "Hey there"
+
+        util.write_file(path, contents, copy_mode=True)
+
+        self.assertTrue(os.path.exists(path))
+        self.assertTrue(os.path.isfile(path))
+        file_stat = os.stat(path)
+        self.assertEqual(0o644, stat.S_IMODE(file_stat.st_mode))
+
+    def test_copy_mode_with_existing(self):
+        """Verify that file is created using mode of existing file
+        if copy_mode is true."""
+        path = os.path.join(self.tmp, "NewFile.txt")
+        contents = "Hey there"
+
+        open(path, 'w').close()
+        os.chmod(path, 0o666)
+
+        util.write_file(path, contents, copy_mode=True)
+
+        self.assertTrue(os.path.exists(path))
+        self.assertTrue(os.path.isfile(path))
+        file_stat = os.stat(path)
+        self.assertEqual(0o666, stat.S_IMODE(file_stat.st_mode))
+
     def test_custom_omode(self):
         """Verify custom omode works properly."""
         path = os.path.join(self.tmp, "NewFile.txt")