From 0ab34e42b2e81d9d1903b87efe51570fc326acb5 Mon Sep 17 00:00:00 2001 From: Csaba Henk Date: Mon, 6 Apr 2015 02:05:29 +0200 Subject: [PATCH] glusterfs_native: make {allow,deny}_access non-destructive With this patch the allow_access and deny_access methods of glusterfs_native will keep preexisting common names in the affected GlusterFS volume 'auth.ssl-allow' option. Also check in _setup_gluster_vol if 'auth.ssl-allow' is set to a non-empty value to avoid semantically problematic (and from Manila POV, useless) edge cases. Change-Id: I952049d694509a338c7f56b45c5ef0872c3e7d70 Closes-Bug: #1439198 --- manila/share/drivers/glusterfs_native.py | 66 +++++++++++++-- .../share/drivers/test_glusterfs_native.py | 81 +++++++++++++++++-- 2 files changed, 133 insertions(+), 14 deletions(-) diff --git a/manila/share/drivers/glusterfs_native.py b/manila/share/drivers/glusterfs_native.py index 5e8600ba22..4d94717b9f 100644 --- a/manila/share/drivers/glusterfs_native.py +++ b/manila/share/drivers/glusterfs_native.py @@ -42,6 +42,7 @@ from manila import exception from manila.i18n import _ from manila.i18n import _LE from manila.i18n import _LI +from manila.i18n import _LW from manila.share import driver from manila.share.drivers import glusterfs from manila import utils @@ -265,6 +266,22 @@ class GlusterfsNativeShareDriver(driver.ExecuteMixin, driver.ShareDriver): gluster_mgr = self._glustermanager(vol) + ssl_allow_opt = gluster_mgr.get_gluster_vol_option(AUTH_SSL_ALLOW) + if not ssl_allow_opt: + # Not having AUTH_SSL_ALLOW set is a problematic edge case. + # - In GlusterFS 3.6, it implies that access is allowed to + # noone, including intra-service access, which causes + # problems internally in GlusterFS + # - In GlusterFS 3.7, it implies that access control is + # disabled, which defeats the purpose of this driver -- + # so to avoid these possiblitiies, we throw an error in this case. + msg = (_("Option %(option)s is not defined on gluster volume. " + "Volume: %(volname)s") % + {'volname': gluster_mgr.volume, + 'option': AUTH_SSL_ALLOW}) + LOG.error(msg) + raise exception.GlusterfsException(msg) + for option, value in six.iteritems( {NFS_EXPORT_VOL: 'off', CLIENT_SSL: 'on', SERVER_SSL: 'on'} ): @@ -638,6 +655,7 @@ class GlusterfsNativeShareDriver(driver.ExecuteMixin, driver.ShareDriver): 'errno': operrno, 'errstr': operrstr}) + @utils.synchronized("glusterfs_native_access", external=False) def allow_access(self, context, share, access, share_server=None): """Allow access to a share using certs. @@ -650,17 +668,34 @@ class GlusterfsNativeShareDriver(driver.ExecuteMixin, driver.ShareDriver): exp_locn = share.get('export_location', None) gluster_mgr = self.gluster_used_vols_dict.get(exp_locn) + ssl_allow_opt = gluster_mgr.get_gluster_vol_option(AUTH_SSL_ALLOW) + # wrt. GlusterFS' parsing of auth.ssl-allow, please see code from + # https://github.com/gluster/glusterfs/blob/v3.6.2/ + # xlators/protocol/auth/login/src/login.c#L80 + # until end of gf_auth() function + ssl_allow = re.split('[ ,]', ssl_allow_opt) + access_to = access['access_to'] + if access_to in ssl_allow: + LOG.warn(_LW("Access to %(share)s at %(export)s is already " + "granted for %(access_to)s. GlusterFS volume " + "options might have been changed externally."), + {'share': share['id'], 'export': exp_locn, + 'access_to': access_to}) + return + + ssl_allow.append(access_to) + ssl_allow_opt = ','.join(ssl_allow) try: gluster_mgr.gluster_call( 'volume', 'set', gluster_mgr.volume, - AUTH_SSL_ALLOW, access['access_to']) + AUTH_SSL_ALLOW, ssl_allow_opt) except exception.ProcessExecutionError as exc: msg = (_("Error in gluster volume set during allow access. " "Volume: %(volname)s, Option: %(option)s, " "access_to: %(access_to)s, Error: %(error)s"), {'volname': gluster_mgr.volume, - 'option': AUTH_SSL_ALLOW, - 'access_to': access['access_to'], 'error': exc.stderr}) + 'option': AUTH_SSL_ALLOW, 'access_to': access_to, + 'error': exc.stderr}) LOG.error(msg) raise exception.GlusterfsException(msg) @@ -668,6 +703,7 @@ class GlusterfsNativeShareDriver(driver.ExecuteMixin, driver.ShareDriver): # set dynamically. self._restart_gluster_vol(gluster_mgr) + @utils.synchronized("glusterfs_native_access", external=False) def deny_access(self, context, share, access, share_server=None): """Deny access to a share that's using cert based auth. @@ -681,16 +717,30 @@ class GlusterfsNativeShareDriver(driver.ExecuteMixin, driver.ShareDriver): exp_locn = share.get('export_location', None) gluster_mgr = self.gluster_used_vols_dict.get(exp_locn) + ssl_allow_opt = gluster_mgr.get_gluster_vol_option(AUTH_SSL_ALLOW) + ssl_allow = re.split('[ ,]', ssl_allow_opt) + access_to = access['access_to'] + if access_to not in ssl_allow: + LOG.warn(_LW("Access to %(share)s at %(export)s is already " + "denied for %(access_to)s. GlusterFS volume " + "options might have been changed externally."), + {'share': share['id'], 'export': exp_locn, + 'access_to': access_to}) + return + + ssl_allow.remove(access_to) + ssl_allow_opt = ','.join(ssl_allow) try: gluster_mgr.gluster_call( - 'volume', 'reset', gluster_mgr.volume, - AUTH_SSL_ALLOW) + 'volume', 'set', gluster_mgr.volume, + AUTH_SSL_ALLOW, ssl_allow_opt) except exception.ProcessExecutionError as exc: - msg = (_("Error in gluster volume reset during deny access. " + msg = (_("Error in gluster volume set during deny access. " "Volume: %(volname)s, Option: %(option)s, " - "Error: %(error)s"), + "access_to: %(access_to)s, Error: %(error)s"), {'volname': gluster_mgr.volume, - 'option': AUTH_SSL_ALLOW, 'error': exc.stderr}) + 'option': AUTH_SSL_ALLOW, 'access_to': access_to, + 'error': exc.stderr}) LOG.error(msg) raise exception.GlusterfsException(msg) diff --git a/manila/tests/share/drivers/test_glusterfs_native.py b/manila/tests/share/drivers/test_glusterfs_native.py index 2de64d81a1..abeba849f6 100644 --- a/manila/tests/share/drivers/test_glusterfs_native.py +++ b/manila/tests/share/drivers/test_glusterfs_native.py @@ -222,8 +222,11 @@ class GlusterfsNativeShareDriverTestCase(test.TestCase): gmgr = glusterfs.GlusterManager gmgr1 = gmgr(self.glusterfs_target1, self._execute, None, None) self._driver._glustermanager = mock.Mock(return_value=gmgr1) + self.mock_object(gmgr1, 'get_gluster_vol_option', + mock.Mock(return_value='some.common.name')) ret = self._driver._setup_gluster_vol(gmgr1.volume) + gmgr1.get_gluster_vol_option.assert_called_once_with('auth.ssl-allow') gmgr1.gluster_call.has_calls( mock.call(*test_args[0]), mock.call(*test_args[1]), @@ -247,15 +250,33 @@ class GlusterfsNativeShareDriverTestCase(test.TestCase): gmgr = glusterfs.GlusterManager gmgr1 = gmgr(self.glusterfs_target1, self._execute, None, None) self._driver._glustermanager = mock.Mock(return_value=gmgr1) + self.mock_object(gmgr1, 'get_gluster_vol_option', + mock.Mock(return_value='some.common.name')) self.mock_object(gmgr1, 'gluster_call', mock.Mock(side_effect=raise_exception)) self.assertRaises(exception.GlusterfsException, self._driver._setup_gluster_vol, gmgr1.volume) + gmgr1.get_gluster_vol_option.assert_called_once_with('auth.ssl-allow') self.assertTrue( mock.call(*test_args[idx]) in gmgr1.gluster_call.call_args_list) self.assertFalse(self._driver._restart_gluster_vol.called) + def test_setup_gluster_vol_no_ssl_allow(self): + self._driver._restart_gluster_vol = mock.Mock() + + gmgr = glusterfs.GlusterManager + gmgr1 = gmgr(self.glusterfs_target1, self._execute, None, None) + self._driver._glustermanager = mock.Mock(return_value=gmgr1) + self.mock_object(gmgr1, 'get_gluster_vol_option', + mock.Mock(return_value=None)) + + self.assertRaises(exception.GlusterfsException, + self._driver._setup_gluster_vol, gmgr1.volume) + gmgr1.get_gluster_vol_option.assert_called_once_with('auth.ssl-allow') + self.assertFalse(gmgr1.gluster_call.called) + self.assertFalse(self._driver._restart_gluster_vol.called) + def test_restart_gluster_vol(self): gmgr = glusterfs.GlusterManager gmgr1 = gmgr(self.glusterfs_target1, self._execute, None, None) @@ -831,13 +852,33 @@ class GlusterfsNativeShareDriverTestCase(test.TestCase): access = {'access_type': 'cert', 'access_to': 'client.example.com'} gmgr = glusterfs.GlusterManager gmgr1 = gmgr(self.glusterfs_target1, self._execute, None, None) + self.mock_object(gmgr1, 'get_gluster_vol_option', + mock.Mock(return_value='some.common.name')) test_args = ('volume', 'set', 'gv1', 'auth.ssl-allow', - access['access_to']) + 'some.common.name,' + access['access_to']) self._driver.gluster_used_vols_dict = {self.glusterfs_target1: gmgr1} self._driver.allow_access(self._context, self.share1, access) + gmgr1.get_gluster_vol_option.assert_called_once_with('auth.ssl-allow') gmgr1.gluster_call.assert_called_once_with(*test_args) + self._driver._restart_gluster_vol.assert_called_once_with(gmgr1) + + def test_allow_access_with_share_having_access(self): + self._driver._restart_gluster_vol = mock.Mock() + access = {'access_type': 'cert', 'access_to': 'client.example.com'} + gmgr = glusterfs.GlusterManager + gmgr1 = gmgr(self.glusterfs_target1, self._execute, None, None) + self.mock_object( + gmgr1, 'get_gluster_vol_option', + mock.Mock(return_value='some.common.name,' + access['access_to'])) + + self._driver.gluster_used_vols_dict = {self.glusterfs_target1: gmgr1} + + self._driver.allow_access(self._context, self.share1, access) + gmgr1.get_gluster_vol_option.assert_called_once_with('auth.ssl-allow') + self.assertFalse(gmgr1.gluster_call.called) + self.assertFalse(self._driver._restart_gluster_vol.called) def test_allow_access_invalid_access_type(self): self._driver._restart_gluster_vol = mock.Mock() @@ -853,7 +894,7 @@ class GlusterfsNativeShareDriverTestCase(test.TestCase): def test_allow_access_excp(self): access = {'access_type': 'cert', 'access_to': 'client.example.com'} test_args = ('volume', 'set', 'gv1', 'auth.ssl-allow', - access['access_to']) + 'some.common.name,' + access['access_to']) def raise_exception(*args, **kwargs): if (args == test_args): @@ -863,6 +904,8 @@ class GlusterfsNativeShareDriverTestCase(test.TestCase): gmgr = glusterfs.GlusterManager gmgr1 = gmgr(self.glusterfs_target1, self._execute, None, None) + self.mock_object(gmgr1, 'get_gluster_vol_option', + mock.Mock(return_value='some.common.name')) self._driver.gluster_used_vols_dict = {self.glusterfs_target1: gmgr1} self.mock_object(gmgr1, 'gluster_call', @@ -871,22 +914,43 @@ class GlusterfsNativeShareDriverTestCase(test.TestCase): self.assertRaises(exception.GlusterfsException, self._driver.allow_access, self._context, self.share1, access) + gmgr1.get_gluster_vol_option.assert_called_once_with('auth.ssl-allow') gmgr1.gluster_call.assert_called_once_with(*test_args) self.assertFalse(self._driver._restart_gluster_vol.called) def test_deny_access(self): self._driver._restart_gluster_vol = mock.Mock() - access = {'access_type': 'cert', 'access_to': 'NotApplicable'} + access = {'access_type': 'cert', 'access_to': 'client.example.com'} gmgr = glusterfs.GlusterManager gmgr1 = gmgr(self.glusterfs_target1, self._execute, None, None) - test_args = ('volume', 'reset', 'gv1', 'auth.ssl-allow') + self.mock_object( + gmgr1, 'get_gluster_vol_option', + mock.Mock(return_value='some.common.name,' + access['access_to'])) + test_args = ('volume', 'set', 'gv1', 'auth.ssl-allow', + 'some.common.name') self._driver.gluster_used_vols_dict = {self.glusterfs_target1: gmgr1} self._driver.deny_access(self._context, self.share1, access) + gmgr1.get_gluster_vol_option.assert_called_once_with('auth.ssl-allow') gmgr1.gluster_call.assert_called_once_with(*test_args) self._driver._restart_gluster_vol.assert_called_once_with(gmgr1) + def test_deny_access_with_share_having_no_access(self): + self._driver._restart_gluster_vol = mock.Mock() + access = {'access_type': 'cert', 'access_to': 'client.example.com'} + gmgr = glusterfs.GlusterManager + gmgr1 = gmgr(self.glusterfs_target1, self._execute, None, None) + self.mock_object(gmgr1, 'get_gluster_vol_option', + mock.Mock(return_value='some.common.name')) + + self._driver.gluster_used_vols_dict = {self.glusterfs_target1: gmgr1} + + self._driver.deny_access(self._context, self.share1, access) + gmgr1.get_gluster_vol_option.assert_called_once_with('auth.ssl-allow') + self.assertFalse(gmgr1.gluster_call.called) + self.assertFalse(self._driver._restart_gluster_vol.called) + def test_deny_access_invalid_access_type(self): self._driver._restart_gluster_vol = mock.Mock() access = {'access_type': 'invalid', 'access_to': 'NotApplicable'} @@ -897,8 +961,9 @@ class GlusterfsNativeShareDriverTestCase(test.TestCase): self.assertFalse(self._driver._restart_gluster_vol.called) def test_deny_access_excp(self): - access = {'access_type': 'cert', 'access_to': 'NotApplicable'} - test_args = ('volume', 'reset', 'gv1', 'auth.ssl-allow') + access = {'access_type': 'cert', 'access_to': 'client.example.com'} + test_args = ('volume', 'set', 'gv1', 'auth.ssl-allow', + 'some.common.name') def raise_exception(*args, **kwargs): if (args == test_args): @@ -908,6 +973,9 @@ class GlusterfsNativeShareDriverTestCase(test.TestCase): gmgr = glusterfs.GlusterManager gmgr1 = gmgr(self.glusterfs_target1, self._execute, None, None) + self.mock_object( + gmgr1, 'get_gluster_vol_option', + mock.Mock(return_value='some.common.name,' + access['access_to'])) self._driver.gluster_used_vols_dict = {self.glusterfs_target1: gmgr1} self.mock_object(gmgr1, 'gluster_call', @@ -916,6 +984,7 @@ class GlusterfsNativeShareDriverTestCase(test.TestCase): self.assertRaises(exception.GlusterfsException, self._driver.deny_access, self._context, self.share1, access) + gmgr1.get_gluster_vol_option.assert_called_once_with('auth.ssl-allow') gmgr1.gluster_call.assert_called_once_with(*test_args) self.assertFalse(self._driver._restart_gluster_vol.called)