Fix permissions with NFS-backed snapshots and backups

Fix snapshot issues that occur if cinder does not have regular (non-root)
write permission on an NFS share. This is done by following the volume
driver's model of creating files as root, followed by calling
_set_rw_permissions().

Fix backup issues that occur if cinder's group ID changes, which can
happen when migrating from bare metal to a containerized service. If the
share's group ownership and permission needs to be modified, then do it
recursively so that previously made backups remain accessible.

Closes-Bug: #1807760
Change-Id: I6c20c4825af0a365b6a20fb633c810c2f2fe48b0
(cherry picked from commit 1fb342cba8)
(cherry picked from commit 95f30ea6ed)
Conflicts:
	cinder/tests/unit/volume/drivers/test_remotefs.py

cinder/backup/drivers/nfs.py

@@ -88,12 +88,12 @@ class NFSBackupDriver(posix.PosixBackupDriver):
         current_mode = utils.get_file_mode(mount_path)
 
         if group_id != current_group_id:
-            cmd = ['chgrp', group_id, mount_path]
+            cmd = ['chgrp', '-R', group_id, mount_path]
             self._execute(*cmd, root_helper=self._root_helper,
                           run_as_root=True)
 
         if not (current_mode & stat.S_IWGRP):
-            cmd = ['chmod', 'g+w', mount_path]
+            cmd = ['chmod', '-R', 'g+w', mount_path]
             self._execute(*cmd, root_helper=self._root_helper,
                           run_as_root=True)
 

cinder/tests/unit/backup/drivers/test_backup_nfs.py

@@ -119,6 +119,7 @@ class BackupNFSShareTestCase(test.TestCase):
         if file_gid != FAKE_EGID:
             mock_execute_calls.append(
                 mock.call('chgrp',
+                          '-R',
                           FAKE_EGID,
                           path,
                           root_helper=driver._root_helper,
@@ -127,6 +128,7 @@ class BackupNFSShareTestCase(test.TestCase):
         if not (file_mode & stat.S_IWGRP):
             mock_execute_calls.append(
                 mock.call('chmod',
+                          '-R',
                           'g+w',
                           path,
                           root_helper=driver._root_helper,

cinder/tests/unit/volume/drivers/test_remotefs.py

@@ -102,7 +102,6 @@ class RemoteFsSnapDriverTestCase(test.TestCase):
         self._driver._write_info_file = mock.Mock()
         self._driver._img_commit = mock.Mock()
         self._driver._rebase_img = mock.Mock()
-        self._driver._ensure_share_writable = mock.Mock()
         self._driver._delete_stale_snapshot = mock.Mock()
         self._driver._delete_snapshot_online = mock.Mock()
 
@@ -615,6 +614,44 @@ class RemoteFsSnapDriverTestCase(test.TestCase):
                                                       'count=1024',
                                                       run_as_root=True)
 
+    @ddt.data(False, True)
+    @mock.patch('json.dump')
+    @mock.patch('cinder.volume.drivers.remotefs.open')
+    @mock.patch('os.path.exists')
+    def test_write_info_file(self,
+                             info_file_exists,
+                             mock_os_path_exists,
+                             mock_open,
+                             mock_json_dump):
+
+        mock_os_path_exists.return_value = info_file_exists
+        fake_info_path = '/path/to/info'
+        fake_snapshot_info = {'active': self._fake_snapshot_path}
+        self._driver._execute = mock.Mock()
+        self._driver._set_rw_permissions = mock.Mock()
+
+        self._driver._write_info_file(fake_info_path, fake_snapshot_info)
+
+        mock_open.assert_called_once_with(fake_info_path, 'w')
+        mock_json_dump.assert_called_once_with(
+            fake_snapshot_info, mock.ANY, indent=1, sort_keys=True)
+
+        if info_file_exists:
+            self._driver._execute.assert_not_called()
+            self._driver._set_rw_permissions.assert_not_called()
+        else:
+            self._driver._execute.assert_called_once_with(
+                'truncate', "-s0", fake_info_path,
+                run_as_root=self._driver._execute_as_root)
+            self._driver._set_rw_permissions.assert_called_once_with(
+                fake_info_path)
+
+        fake_snapshot_info.pop('active')
+        self.assertRaises(exception.RemoteFSException,
+                          self._driver._write_info_file,
+                          fake_info_path,
+                          fake_snapshot_info)
+
 
 class RemoteFSPoolMixinTestCase(test.TestCase):
     def setUp(self):

cinder/volume/drivers/remotefs.py

@@ -23,7 +23,6 @@ import math
 import os
 import re
 import shutil
-import tempfile
 import time
 
 from oslo_config import cfg
@@ -740,6 +739,11 @@ class RemoteFSSnapDriverBase(RemoteFSDriver):
             msg = _("'active' must be present when writing snap_info.")
             raise exception.RemoteFSException(msg)
 
+        if not os.path.exists(info_path):
+            self._execute('truncate', "-s0", info_path,
+                          run_as_root=self._execute_as_root)
+            self._set_rw_permissions(info_path)
+
         with open(info_path, 'w') as f:
             json.dump(snap_info, f, indent=1, sort_keys=True)
 
@@ -909,26 +913,6 @@ class RemoteFSSnapDriverBase(RemoteFSDriver):
     def _get_mount_point_base(self):
         return self.base
 
-    def _ensure_share_writable(self, path):
-        """Ensure that the Cinder user can write to the share.
-
-        If not, raise an exception.
-
-        :param path: path to test
-        :raises: RemoteFSException
-        :returns: None
-        """
-
-        prefix = '.cinder-write-test-' + str(os.getpid()) + '-'
-
-        try:
-            tempfile.NamedTemporaryFile(prefix=prefix, dir=path)
-        except OSError:
-            msg = _('Share at %(dir)s is not writable by the '
-                    'Cinder volume service. Snapshot operations will not be '
-                    'supported.') % {'dir': path}
-            raise exception.RemoteFSException(msg)
-
     def _copy_volume_to_image(self, context, volume, image_service,
                               image_meta):
         """Copy the volume to the specified image."""
@@ -1096,7 +1080,6 @@ class RemoteFSSnapDriverBase(RemoteFSDriver):
         self._validate_state(volume_status, acceptable_states)
 
         vol_path = self._local_volume_dir(snapshot.volume)
-        self._ensure_share_writable(vol_path)
 
         # Determine the true snapshot file for this snapshot
         # based on the .info file