Merge ~chad.smith/cloud-init:tests-fix-root-os-access-leak into cloud-init:master

[Chad Smith]

resizefs: Drop check for read-only device file, do not warn on overlayroot.

As root user, os.access(<path>, os.W_OK) will always return True so that
path will never get executed. Also avoid a warning if the root is
overlayroot, which is the common case on a MAAS booted 'ephemeral' system.

resizefs: Drop check and tests for read-only device file

As root user, os.access(<path>, os.W_OK) will always return True so that path will never get executed as Scott pointed out. Also, on a real read-only iscsi devpath, resizefs doesn't perform an operation on a filesystem it recognizes as read-only. It may emit a warning, but not cause a traceback.

[Scott Moser (smoser)]

So did this just expose an *actual* bug ?

  "root user still sees read-only files as writable so the unittests would fail"

If root user sees read-only files as writable, and cloud-init runs as root,
would we ever take that path ?

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

PASSED: Continuous integration, rev:675996d25dc2f7a101e75e6562d7d7e712e76f07
https://jenkins.ubuntu.com/server/job/cloud-init-ci/300/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

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

review: Approve (continuous-integration)

[Chad Smith (chad.smith)]

I'm not quite sure what we should do here. You are right that root user will likely never see os.access(W_OK) as False because root's a privileged user so the perms don't matter.

Also, it seems that in containers we fail to resize up at cc_resizefs line 197 on ENOENT so generally trying to account for is_container checks below in the os.access and stat.S_ISBLK
conditionals feel like wasted logic. We could just check mode values instead of os.access like this:

os.stat(devpath).st_mode & (stat.S_IROTH | stat.S_IRGRP| stat.S_IRUSR)
I know there must be a better way here.

But ultimately, do you know if there are device files that would be read only for root?

1367b2b... by Chad Smith on 2017-09-14

drop 'not os.access(os.W_OK)' check from resizefs because os.access as root will always return writable and resizefs of read-only devices will perform no resize operation and exit

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

PASSED: Continuous integration, rev:1367b2b911d61ae95c2c635259b2f2b85c8eb541
https://jenkins.ubuntu.com/server/job/cloud-init-ci/301/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

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

review: Approve (continuous-integration)

e19de87... by Chad Smith on 2017-09-15

log a message on overlayroot devpath and return False. Add unit test

[Scott Moser (smoser)]

Approve.
I've re-worded commit message some and am pushing after a tox

review: Approve

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

PASSED: Continuous integration, rev:e19de87fdae9fb58d39ac415b73fa37e6b6e1e83
https://jenkins.ubuntu.com/server/job/cloud-init-ci/312/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

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

review: Approve (continuous-integration)

diff --git a/cloudinit/config/cc_resizefs.py b/cloudinit/config/cc_resizefs.py
index f42b6a6..f774baa 100644
--- a/cloudinit/config/cc_resizefs.py
+++ b/cloudinit/config/cc_resizefs.py
@@ -192,6 +192,10 @@ def is_device_path_writable_block(devpath, info, log):
             return False
         log.debug("Converted /dev/root to '%s' per kernel cmdline", devpath)
 
+    if devpath == 'overlayroot':
+        log.debug("Not attempting to resize devpath '%s': %s", devpath, info)
+        return False
+
     try:
         statret = os.stat(devpath)
     except OSError as exc:
@@ -205,14 +209,6 @@ def is_device_path_writable_block(devpath, info, log):
             raise exc
         return False
 
-    if not os.access(devpath, os.W_OK):
-        if container:
-            log.debug("'%s' not writable in container. cannot resize: %s",
-                      devpath, info)
-        else:
-            log.warn("'%s' not writable. cannot resize: %s", devpath, info)
-        return
-
     if not stat.S_ISBLK(statret.st_mode) and not stat.S_ISCHR(statret.st_mode):
         if container:
             log.debug("device '%s' not a block device in container."
diff --git a/tests/unittests/test_handler/test_handler_resizefs.py b/tests/unittests/test_handler/test_handler_resizefs.py
index 76dddbf..3e5d436 100644
--- a/tests/unittests/test_handler/test_handler_resizefs.py
+++ b/tests/unittests/test_handler/test_handler_resizefs.py
@@ -166,6 +166,18 @@ class TestIsDevicePathWritableBlock(CiTestCase):
 
     with_logs = True
 
+    def test_is_device_path_writable_block_false_on_overlayroot(self):
+        """When devpath is overlayroot (on MAAS), is_dev_writable is False."""
+        info = 'does not matter'
+        is_writable = wrap_and_call(
+            'cloudinit.config.cc_resizefs.util',
+            {'is_container': {'return_value': False}},
+            is_device_path_writable_block, 'overlayroot', info, LOG)
+        self.assertFalse(is_writable)
+        self.assertIn(
+            "Not attempting to resize devpath 'overlayroot'",
+            self.logs.getvalue())
+
     def test_is_device_path_writable_block_warns_missing_cmdline_root(self):
         """When root does not exist isn't in the cmdline, log warning."""
         info = 'does not matter'
@@ -178,24 +190,24 @@ class TestIsDevicePathWritableBlock(CiTestCase):
         exists_mock_path = 'cloudinit.config.cc_resizefs.os.path.exists'
         with mock.patch(exists_mock_path) as m_exists:
             m_exists.return_value = False
-            is_valid = wrap_and_call(
+            is_writable = wrap_and_call(
                 'cloudinit.config.cc_resizefs.util',
                 {'is_container': {'return_value': False},
                  'get_mount_info': {'side_effect': fake_mount_info},
                  'get_cmdline': {'return_value': 'BOOT_IMAGE=/vmlinuz.efi'}},
                 is_device_path_writable_block, '/dev/root', info, LOG)
-        self.assertFalse(is_valid)
+        self.assertFalse(is_writable)
         logs = self.logs.getvalue()
         self.assertIn("WARNING: Unable to find device '/dev/root'", logs)
 
     def test_is_device_path_writable_block_does_not_exist(self):
         """When devpath does not exist, a warning is logged."""
         info = 'dev=/I/dont/exist mnt_point=/ path=/dev/none'
-        is_valid = wrap_and_call(
+        is_writable = wrap_and_call(
             'cloudinit.config.cc_resizefs.util',
             {'is_container': {'return_value': False}},
             is_device_path_writable_block, '/I/dont/exist', info, LOG)
-        self.assertFalse(is_valid)
+        self.assertFalse(is_writable)
         self.assertIn(
             "WARNING: Device '/I/dont/exist' did not exist."
             ' cannot resize: %s' % info,
@@ -204,11 +216,11 @@ class TestIsDevicePathWritableBlock(CiTestCase):
     def test_is_device_path_writable_block_does_not_exist_in_container(self):
         """When devpath does not exist in a container, log a debug message."""
         info = 'dev=/I/dont/exist mnt_point=/ path=/dev/none'
-        is_valid = wrap_and_call(
+        is_writable = wrap_and_call(
             'cloudinit.config.cc_resizefs.util',
             {'is_container': {'return_value': True}},
             is_device_path_writable_block, '/I/dont/exist', info, LOG)
-        self.assertFalse(is_valid)
+        self.assertFalse(is_writable)
         self.assertIn(
             "DEBUG: Device '/I/dont/exist' did not exist in container."
             ' cannot resize: %s' % info,
@@ -226,57 +238,17 @@ class TestIsDevicePathWritableBlock(CiTestCase):
         self.assertEqual(
             'Something unexpected', str(context_manager.exception))
 
-    def test_is_device_path_writable_block_readonly(self):
-        """When root device is readonly, emit a warning and return False."""
-        fake_devpath = self.tmp_path('dev/readonly')
-        util.write_file(fake_devpath, '', mode=0o400)  # read-only
-        info = 'dev=/dev/root mnt_point=/ path={0}'.format(fake_devpath)
-
-        exists_mock_path = 'cloudinit.config.cc_resizefs.os.path.exists'
-        with mock.patch(exists_mock_path) as m_exists:
-            m_exists.return_value = False
-            is_valid = wrap_and_call(
-                'cloudinit.config.cc_resizefs.util',
-                {'is_container': {'return_value': False},
-                 'rootdev_from_cmdline': {'return_value': fake_devpath}},
-                is_device_path_writable_block, '/dev/root', info, LOG)
-        self.assertFalse(is_valid)
-        logs = self.logs.getvalue()
-        self.assertIn(
-            "Converted /dev/root to '{0}' per kernel cmdline".format(
-                fake_devpath),
-            logs)
-        self.assertIn(
-            "WARNING: '{0}' not writable. cannot resize".format(fake_devpath),
-            logs)
-
-    def test_is_device_path_writable_block_readonly_in_container(self):
-        """When root device is readonly, emit debug log and return False."""
-        fake_devpath = self.tmp_path('dev/readonly')
-        util.write_file(fake_devpath, '', mode=0o400)  # read-only
-        info = 'dev=/dev/root mnt_point=/ path={0}'.format(fake_devpath)
-
-        is_valid = wrap_and_call(
-            'cloudinit.config.cc_resizefs.util',
-            {'is_container': {'return_value': True}},
-            is_device_path_writable_block, fake_devpath, info, LOG)
-        self.assertFalse(is_valid)
-        self.assertIn(
-            "DEBUG: '{0}' not writable in container. cannot resize".format(
-                fake_devpath),
-            self.logs.getvalue())
-
     def test_is_device_path_writable_block_non_block(self):
         """When device is not a block device, emit warning return False."""
         fake_devpath = self.tmp_path('dev/readwrite')
         util.write_file(fake_devpath, '', mode=0o600)  # read-write
         info = 'dev=/dev/root mnt_point=/ path={0}'.format(fake_devpath)
 
-        is_valid = wrap_and_call(
+        is_writable = wrap_and_call(
             'cloudinit.config.cc_resizefs.util',
             {'is_container': {'return_value': False}},
             is_device_path_writable_block, fake_devpath, info, LOG)
-        self.assertFalse(is_valid)
+        self.assertFalse(is_writable)
         self.assertIn(
             "WARNING: device '{0}' not a block device. cannot resize".format(
                 fake_devpath),
@@ -288,11 +260,11 @@ class TestIsDevicePathWritableBlock(CiTestCase):
         util.write_file(fake_devpath, '', mode=0o600)  # read-write
         info = 'dev=/dev/root mnt_point=/ path={0}'.format(fake_devpath)
 
-        is_valid = wrap_and_call(
+        is_writable = wrap_and_call(
             'cloudinit.config.cc_resizefs.util',
             {'is_container': {'return_value': True}},
             is_device_path_writable_block, fake_devpath, info, LOG)
-        self.assertFalse(is_valid)
+        self.assertFalse(is_writable)
         self.assertIn(
             "DEBUG: device '{0}' not a block device in container."
             ' cannot resize'.format(fake_devpath),