Merge "Fix for LV mounting issue in docker containers"

manila/share/drivers/container/container_helper.py

@@ -39,10 +39,32 @@ class DockerExecHelper(driver.ExecuteMixin):
         LOG.debug("Starting container from image %s.", image_name)
         # (aovchinnikov): --privileged is required for both samba and
         # nfs-ganesha to actually allow access to shared folders.
+        #
+        # (aovchinnikov): To actually make docker container mount a
+        # logical volume created after container start-up to some location
+        # inside it, we must share entire /dev with it. While seemingly
+        # dangerous it is not and moreover this is apparently the only sane
+        # way to do it. The reason is when a logical volume gets created
+        # several new things appear in /dev: a new /dev/dm-X and a symlink
+        # in /dev/volume_group_name pointing to /dev/dm-X. But to be able
+        # to interact with /dev/dm-X, it must be already present inside
+        # the container's /dev i.e. it must have been -v shared during
+        # container start-up. So we should either precreate an unknown
+        # number of /dev/dm-Xs (one per LV), share them all and hope
+        # for the best or share the entire /dev and hope for the best.
+        #
+        # The risk of allowing a container having access to entire host's
+        # /dev is not as big as it seems: as long as actual share providers
+        # are invulnerable this does not pose any extra risks. If, however,
+        # share providers contain vulnerabilities then the driver does not
+        # provide any more possibilities for an exploitation than other
+        # first-party drivers.
+
         cmd = ["docker", "run", "-d", "-i", "-t", "--privileged",
-               "--name=%s" % name, '-v', "/tmp/shares:/shares", image_name]
-        result = self._inner_execute(cmd) or ['', 1]
-        if result[1] != '':
+               "-v", "/dev:/dev", "--name=%s" % name,
+               "-v", "/tmp/shares:/shares", image_name]
+        result = self._inner_execute(cmd) or ["", 1]
+        if result[1] != "":
             raise exception.ManilaException(
                 _("Container %s has failed to start.") % name)
         LOG.info(_LI("A container has been successfully started! Its id is "

manila/share/drivers/container/driver.py

@@ -126,6 +126,11 @@ class ContainerShareDriver(driver.ShareDriver, driver.ExecuteMixin):
             ["mkdir", "-m", "750", "/shares/%s" % share_name]
         )
         self.storage.provide_storage(share)
+        lv_device = self.storage._get_lv_device(share)
+        self.container.execute(
+            server_id,
+            ["mount", lv_device, "/shares/%s" % share_name]
+        )
         location = self._get_helper(share).create_share(server_id)
         return location
 
@@ -136,15 +141,30 @@ class ContainerShareDriver(driver.ShareDriver, driver.ExecuteMixin):
         server_id = self._get_container_name(share_server["id"])
         self._get_helper(share).delete_share(server_id)
 
-        self.storage.remove_storage(share)
+        self.container.execute(
+            server_id,
+            ["umount", "/shares/%s" % share.share_id]
+        )
         self.container.execute(
             server_id,
             ["rm", "-fR", "/shares/%s" % share.share_id]
         )
+
+        self.storage.remove_storage(share)
         LOG.debug("Deletion of share %s is completed!", share.share_id)
 
     def extend_share(self, share, new_size, share_server=None):
+        server_id = self._get_container_name(share_server["id"])
+        self.container.execute(
+            server_id,
+            ["umount", "/shares/%s" % share.share_id]
+        )
         self.storage.extend_share(share, new_size, share_server)
+        lv_device = self.storage._get_lv_device(share)
+        self.container.execute(
+            server_id,
+            ["mount", lv_device, "/shares/%s" % share.share_id]
+        )
 
     def ensure_share(self, context, share, share_server=None):
         pass

manila/share/drivers/container/storage_helper.py

@@ -77,24 +77,14 @@ class LVMHelper(driver.ExecuteMixin):
                       run_as_root=True)
         self._execute("mkfs.ext4", self._get_lv_device(share),
                       run_as_root=True)
-        self._execute("mount", self._get_lv_device(share),
-                      self._get_lv_folder(share), run_as_root=True)
-        self._execute("chmod", "-R", "750", self._get_lv_folder(share),
-                      run_as_root=True)
-        self._execute("chown", "nobody:nogroup", self._get_lv_folder(share),
-                      run_as_root=True)
 
     def remove_storage(self, share):
-        self._execute("umount", self._get_lv_device(share), run_as_root=True)
         self._execute("lvremove", "-f", "--autobackup", "n",
                       self._get_lv_device(share), run_as_root=True)
 
     def extend_share(self, share, new_size, share_server=None):
         lv_device = self._get_lv_device(share)
-        lv_folder = self._get_lv_folder(share)
-        self._execute("umount", lv_folder, run_as_root=True)
         cmd = ('lvextend', '-L', '%sG' % new_size, '-n', lv_device)
         self._execute(*cmd, run_as_root=True)
         self._execute("e2fsck", "-f", "-y", lv_device, run_as_root=True)
         self._execute('resize2fs', lv_device, run_as_root=True)
-        self._execute("mount", lv_device, lv_folder, run_as_root=True)

manila/tests/share/drivers/container/test_container_helper.py

@@ -37,8 +37,8 @@ class DockerExecHelperTestCase(test.TestCase):
         self.mock_object(self.DockerExecHelper, "_inner_execute",
                          mock.Mock(return_value=['fake_output', '']))
         uuid.uuid1 = mock.Mock(return_value='')
-        expected = ['docker', 'run', '-d', '-i', '-t', '--privileged',
-                    '--name=manila_cifs_docker_container', '-v',
+        expected = ['docker', 'run', '-d', '-i', '-t', '--privileged', '-v',
+                    '/dev:/dev', '--name=manila_cifs_docker_container', '-v',
                     '/tmp/shares:/shares', 'fake_image']
 
         self.DockerExecHelper.start_container()

manila/tests/share/drivers/container/test_driver.py

@@ -58,6 +58,14 @@ class ContainerShareDriverTestCase(test.TestCase):
         # Used only to test compatibility with share manager
         self.share_server = "fake_share_server"
 
+    def fake_exec_sync(self, *args, **kwargs):
+        kwargs['execute_arguments'].append(args)
+        try:
+            ret_val = kwargs['ret_val']
+        except KeyError:
+            ret_val = None
+        return ret_val
+
     def test__get_helper_ok(self):
         share = cont_fakes.fake_share(share_proto='CIFS')
         expected = protocol_helper.DockerCIFSHelper(None)
@@ -125,9 +133,21 @@ class ContainerShareDriverTestCase(test.TestCase):
 
     def test_extend_share(self):
         share = cont_fakes.fake_share()
+        actual_arguments = []
+        expected_arguments = [
+            ('manila_fake_server', ['umount', '/shares/fakeshareid']),
+            ('manila_fake_server',
+             ['mount', '/dev/manila_docker_volumes/fakeshareid',
+              '/shares/fakeshareid'])
+        ]
         self.mock_object(self._driver.storage, "extend_share")
+        self._driver.container.execute = functools.partial(
+            self.fake_exec_sync, execute_arguments=actual_arguments,
+            ret_val='')
 
-        self._driver.extend_share(share, 2, 'fake-server')
+        self._driver.extend_share(share, 2, {'id': 'fake-server'})
+
+        self.assertEqual(expected_arguments, actual_arguments)
 
     def test_ensure_share(self):
         # Does effectively nothing by design.

manila/tests/share/drivers/container/test_storage_helper.py

@@ -73,10 +73,6 @@ class LVMHelperTestCase(test.TestCase):
             ('lvcreate', '-p', 'rw', '-L', '1G', '-n', 'fakeshareid',
              'manila_docker_volumes'),
             ('mkfs.ext4', '/dev/manila_docker_volumes/fakeshareid'),
-            ('mount', '/dev/manila_docker_volumes/fakeshareid',
-             '/tmp/shares/fakeshareid'),
-            ('chmod', '-R', '750', '/tmp/shares/fakeshareid'),
-            ('chown', 'nobody:nogroup', '/tmp/shares/fakeshareid')
         ]
         self.LVMHelper._execute = functools.partial(
             self.fake_exec_sync, execute_arguments=actual_arguments,
@@ -89,7 +85,6 @@ class LVMHelperTestCase(test.TestCase):
     def test_remove_storage(self):
         actual_arguments = []
         expected_arguments = [
-            ('umount', '/dev/manila_docker_volumes/fakeshareid'),
             ('lvremove', '-f', '--autobackup', 'n',
              '/dev/manila_docker_volumes/fakeshareid')
         ]
@@ -104,13 +99,10 @@ class LVMHelperTestCase(test.TestCase):
     def test_extend_share(self):
         actual_arguments = []
         expected_arguments = [
-            ('umount', '/tmp/shares/fakeshareid'),
             ('lvextend', '-L', 'shareG', '-n',
              '/dev/manila_docker_volumes/fakeshareid'),
             ('e2fsck', '-f', '-y', '/dev/manila_docker_volumes/fakeshareid'),
             ('resize2fs', '/dev/manila_docker_volumes/fakeshareid'),
-            ('mount', '/dev/manila_docker_volumes/fakeshareid',
-             '/tmp/shares/fakeshareid')
         ]
         self.LVMHelper._execute = functools.partial(
             self.fake_exec_sync, execute_arguments=actual_arguments,

releasenotes/notes/lv-mounting-inside-containers-af8f84d1fab256d1.yaml

@@ -0,0 +1,3 @@
+---
+fixes:
+  - Makes docker containers actually mount logical volumes.