From dc4118c22c1ded44d56b7f0cd730594212892621 Mon Sep 17 00:00:00 2001 From: Ethan Gafford Date: Wed, 5 Aug 2015 14:23:45 -0400 Subject: [PATCH] Check ACLs before adding access for Manila share On investigation, Manila access calls are non-idempotent. For this reason, it is vital that we assess the presence or absence of pre-existing ACLs for any one instance before calling share.allow(..., ip, ...) Otherwise subsequent attempts to idempotently verify or add shares to a cluster will fail. Change-Id: Ib471876f896523856141434b343a743a0f357f84 Closes-bug: 1481849 --- sahara/service/shares.py | 19 +++- sahara/tests/unit/service/test_shares.py | 124 ++++++++++++++++++++++- 2 files changed, 141 insertions(+), 2 deletions(-) diff --git a/sahara/service/shares.py b/sahara/service/shares.py index 28a3a493..26723d37 100644 --- a/sahara/service/shares.py +++ b/sahara/service/shares.py @@ -153,7 +153,24 @@ class _ShareHandler(object): def allow_access_to_instance(self, instance, share_config): """Mounts a specific share to a specific instance.""" access_level = self._get_access_level(share_config) - self.share.allow('ip', instance.internal_ip, access_level) + ip_filter = lambda x: (x.access_type == 'ip' and + x.access_to == instance.internal_ip) + accesses = list(filter(ip_filter, self.share.access_list())) + if accesses: + access = accesses[0] + if access.access_level not in ('ro', 'rw'): + LOG.warning( + _LW("Unknown permission level %(access_level)s on share " + "id %(share_id)s for ip %(ip)s. Leaving pre-existing " + "permissions.").format( + access_level=access.access_level, + share_id=self.share.id, + ip=instance.internal_ip)) + elif access.access_level == 'ro' and access_level == 'rw': + self.share.deny(access.id) + self.share.allow('ip', instance.internal_ip, access_level) + else: + self.share.allow('ip', instance.internal_ip, access_level) @abc.abstractmethod def mount_to_instance(self, remote, share_info): diff --git a/sahara/tests/unit/service/test_shares.py b/sahara/tests/unit/service/test_shares.py index 64a42df7..cd4f873b 100644 --- a/sahara/tests/unit/service/test_shares.py +++ b/sahara/tests/unit/service/test_shares.py @@ -31,11 +31,14 @@ class _FakeShare(object): def __init__(self, id='12345678-1234-1234-1234-123456789012', share_proto='NFS', - export_location='192.168.122.1:/path'): + export_location='192.168.122.1:/path', + access_list=None): self.id = id self.share_proto = share_proto self.export_location = export_location self.allow = mock.Mock() + self.deny = mock.Mock() + self.access_list = mock.Mock(return_value=access_list or []) def _mock_node_group(ips, share_list): @@ -114,6 +117,7 @@ class TestShares(base.SaharaTestCase): for executor in namenode_executors: executor.assert_has_calls( + _setup_calls() + _expected_calls('/mnt/localpath', '192.168.122.1:/path', '-w')) for executor in datanode_executors: self.assertEqual(0, executor.call_count) @@ -183,6 +187,7 @@ class TestShares(base.SaharaTestCase): for executor in datanode_executors: executor.assert_has_calls( + _setup_calls() + _expected_calls('/mnt/somanylocalpaths', '192.168.122.1:/path', '-r')) self.assertEqual(4, executor.call_count) @@ -224,3 +229,120 @@ class TestShares(base.SaharaTestCase): with testtools.ExpectedException(exceptions.NotFoundException): shares.mount_shares(cluster) + + @mock.patch('sahara.context.set_current_instance_id') + @mock.patch('sahara.utils.openstack.manila.client') + def test_acl_exists_unexpected_type(self, f_manilaclient, f_context): + + share = _FakeShare(access_list=[mock.Mock( + access_level='wat', access_to=ip, access_type='ip') + for ip in _NAMENODE_IPS]) + f_manilaclient.return_value = mock.Mock( + shares=mock.Mock( + get=mock.Mock(return_value=share))) + + namenode_group, namenode_executors = _mock_node_group( + _NAMENODE_IPS, + [{ + 'id': '12345678-1234-1234-1234-123456789012', + 'access_level': 'rw', + 'path': '/mnt/localpath' + }]) + + datanode_group, datanode_executors = _mock_node_group( + _DATANODE_IPS, []) + + cluster = mock.Mock( + node_groups=[namenode_group, datanode_group], shares=[]) + + shares.mount_shares(cluster) + + self.assertEqual(0, share.allow.call_count) + + for executor in namenode_executors: + executor.assert_has_calls( + _setup_calls() + + _expected_calls('/mnt/localpath', '192.168.122.1:/path', '-w')) + + for executor in datanode_executors: + self.assertEqual(0, executor.call_count) + + @mock.patch('sahara.context.set_current_instance_id') + @mock.patch('sahara.utils.openstack.manila.client') + def test_acl_exists_no_recreate(self, f_manilaclient, f_context): + + share = _FakeShare(access_list=[mock.Mock( + access_level='rw', access_to=ip, access_type='ip') + for ip in _NAMENODE_IPS]) + f_manilaclient.return_value = mock.Mock( + shares=mock.Mock( + get=mock.Mock(return_value=share))) + + namenode_group, namenode_executors = _mock_node_group( + _NAMENODE_IPS, + [{ + 'id': '12345678-1234-1234-1234-123456789012', + 'access_level': 'ro', + 'path': '/mnt/localpath' + }]) + + datanode_group, datanode_executors = _mock_node_group( + _DATANODE_IPS, []) + + cluster = mock.Mock( + node_groups=[namenode_group, datanode_group], shares=[]) + + shares.mount_shares(cluster) + + self.assertEqual(0, share.allow.call_count) + + for executor in namenode_executors: + executor.assert_has_calls( + _setup_calls() + + _expected_calls('/mnt/localpath', '192.168.122.1:/path', '-r')) + + for executor in datanode_executors: + self.assertEqual(0, executor.call_count) + + @mock.patch('sahara.context.set_current_instance_id') + @mock.patch('sahara.utils.openstack.manila.client') + def test_acl_exists_recreate(self, f_manilaclient, f_context): + + share = _FakeShare(access_list=[mock.Mock( + access_level='ro', access_to=ip, access_type='ip', id="access_id") + for ip in _NAMENODE_IPS]) + f_manilaclient.return_value = mock.Mock( + shares=mock.Mock( + get=mock.Mock(return_value=share))) + + namenode_group, namenode_executors = _mock_node_group( + _NAMENODE_IPS, + [{ + 'id': '12345678-1234-1234-1234-123456789012', + 'access_level': 'rw', + 'path': '/mnt/localpath' + }]) + + datanode_group, datanode_executors = _mock_node_group( + _DATANODE_IPS, []) + + cluster = mock.Mock( + node_groups=[namenode_group, datanode_group], shares=[]) + + shares.mount_shares(cluster) + + namenode_denials = [mock.call('access_id') + for ip in _NAMENODE_IPS] + share.deny.assert_has_calls(namenode_denials) + + namenode_permissions = [mock.call('ip', ip, 'rw') + for ip in _NAMENODE_IPS] + share.allow.assert_has_calls(namenode_permissions, + any_order=True) + + for executor in namenode_executors: + executor.assert_has_calls( + _setup_calls() + + _expected_calls('/mnt/localpath', '192.168.122.1:/path', '-w')) + for executor in datanode_executors: + self.assertEqual(0, executor.call_count)