From 0a40a6422489029b9543c759866d13a5267ed6a0 Mon Sep 17 00:00:00 2001
From: Alexey Ovchinnikov <aovchinnikov@mirantis.com>
Date: Wed, 17 Aug 2016 14:55:54 +0300
Subject: [PATCH] Fix for LV mounting issue in docker containers
Docker containers were previously started with insufficient
privileges to successfully mount a LV created on host.
This change fixes this problem.
Closes-Bug: 1613675
Change-Id: I63f3480ace3be70100a245570d7f3579b333e972
---
.../drivers/container/container_helper.py | 28 +++++++++++++++++--
manila/share/drivers/container/driver.py | 22 ++++++++++++++-
.../share/drivers/container/storage_helper.py | 10 -------
.../container/test_container_helper.py | 4 +--
.../share/drivers/container/test_driver.py | 22 ++++++++++++++-
.../drivers/container/test_storage_helper.py | 8 ------
...ng-inside-containers-af8f84d1fab256d1.yaml | 3 ++
7 files changed, 72 insertions(+), 25 deletions(-)
create mode 100644 releasenotes/notes/lv-mounting-inside-containers-af8f84d1fab256d1.yaml
diff --git a/manila/share/drivers/container/container_helper.py b/manila/share/drivers/container/container_helper.py
index eba03aaa11..80f19ec334 100644
--- a/manila/share/drivers/container/container_helper.py
+++ b/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 "
diff --git a/manila/share/drivers/container/driver.py b/manila/share/drivers/container/driver.py
index 931cb08fac..629859621d 100644
--- a/manila/share/drivers/container/driver.py
+++ b/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
diff --git a/manila/share/drivers/container/storage_helper.py b/manila/share/drivers/container/storage_helper.py
index 2e9e658003..a787514ca4 100644
--- a/manila/share/drivers/container/storage_helper.py
+++ b/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)
diff --git a/manila/tests/share/drivers/container/test_container_helper.py b/manila/tests/share/drivers/container/test_container_helper.py
index e7f7f281b9..1999f0ce0b 100644
--- a/manila/tests/share/drivers/container/test_container_helper.py
+++ b/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()
diff --git a/manila/tests/share/drivers/container/test_driver.py b/manila/tests/share/drivers/container/test_driver.py
index fb91434c7c..31b5365450 100644
--- a/manila/tests/share/drivers/container/test_driver.py
+++ b/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.
diff --git a/manila/tests/share/drivers/container/test_storage_helper.py b/manila/tests/share/drivers/container/test_storage_helper.py
index f6cb6a64ec..02521f57fe 100644
--- a/manila/tests/share/drivers/container/test_storage_helper.py
+++ b/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,
diff --git a/releasenotes/notes/lv-mounting-inside-containers-af8f84d1fab256d1.yaml b/releasenotes/notes/lv-mounting-inside-containers-af8f84d1fab256d1.yaml
new file mode 100644
index 0000000000..ee0c814aff
--- /dev/null
+++ b/releasenotes/notes/lv-mounting-inside-containers-af8f84d1fab256d1.yaml
@@ -0,0 +1,3 @@
+---
+fixes:
+ - Makes docker containers actually mount logical volumes.