From a1ba28cad3b445bd28644f0be55912a072859166 Mon Sep 17 00:00:00 2001 From: zhongjun Date: Sat, 29 Jul 2017 15:50:46 +0800 Subject: [PATCH] Fix cannot deny ipv6 access rules We cannot deny ipv6 access rules because we remove the ipv6 access rules from global delete rules, not just remove the ipv6 access rules from driver update_access interface parameters. So the ipv6 access rules cannot be deleted in db. Now, changed to only remove the ipv6 access rules from the driver update_access interface parameters(add_rules, delete_rules, access_rules_to_be_on_share). Closes-bug: 1707066 Depends-On: Ifea1799e1d2e3963fec7e90ce3f9cb47b9f02f4f Change-Id: Idd0014d898d5468922625e62f9e649926dc04e35 --- manila/share/access.py | 18 ++-- manila/tests/share/test_access.py | 86 +++++++++++-------- manila_tempest_tests/tests/api/test_rules.py | 25 +++--- ...ipv6-access-in-error-bce379ee310060f6.yaml | 7 ++ 4 files changed, 82 insertions(+), 54 deletions(-) create mode 100644 releasenotes/notes/bug-1707066-deny-ipv6-access-in-error-bce379ee310060f6.yaml diff --git a/manila/share/access.py b/manila/share/access.py index d8b2816a54..10628485ee 100644 --- a/manila/share/access.py +++ b/manila/share/access.py @@ -373,6 +373,13 @@ class ShareInstanceAccess(ShareInstanceAccessDatabaseMixin): rules_to_be_removed_from_db, share_server): driver_rule_updates = {} + share_protocol = share_instance['share_proto'].lower() + if (not self.driver.ipv6_implemented and + share_protocol == 'nfs'): + add_rules = self._filter_ipv6_rules(add_rules) + delete_rules = self._filter_ipv6_rules(delete_rules) + access_rules_to_be_on_share = self._filter_ipv6_rules( + access_rules_to_be_on_share) try: driver_rule_updates = self.driver.update_access( context, @@ -463,10 +470,10 @@ class ShareInstanceAccess(ShareInstanceAccessDatabaseMixin): return access_rules_to_be_on_share @staticmethod - def _filter_ipv6_rules(rules, share_instance_proto): + def _filter_ipv6_rules(rules): filtered = [] for rule in rules: - if rule['access_type'] == 'ip' and share_instance_proto == 'nfs': + if rule['access_type'] == 'ip': ip_version = ipaddress.ip_network( six.text_type(rule['access_to'])).version if 6 == ip_version: @@ -496,13 +503,6 @@ class ShareInstanceAccess(ShareInstanceAccessDatabaseMixin): access_rules_to_be_on_share = [ r for r in existing_rules_in_db if r['id'] not in delete_rule_ids ] - share = self.db.share_get(context, share_instance['share_id']) - si_proto = share['share_proto'].lower() - if not self.driver.ipv6_implemented: - add_rules = self._filter_ipv6_rules(add_rules, si_proto) - delete_rules = self._filter_ipv6_rules(delete_rules, si_proto) - access_rules_to_be_on_share = self._filter_ipv6_rules( - access_rules_to_be_on_share, si_proto) return access_rules_to_be_on_share, add_rules, delete_rules def _check_needs_refresh(self, context, share_instance_id): diff --git a/manila/tests/share/test_access.py b/manila/tests/share/test_access.py index 3234a01133..900a9da3af 100644 --- a/manila/tests/share/test_access.py +++ b/manila/tests/share/test_access.py @@ -13,10 +13,12 @@ # License for the specific language governing permissions and limitations # under the License. -import itertools - import ddt import mock +import random + +import itertools +import six from manila.common import constants from manila import context @@ -721,55 +723,71 @@ class ShareInstanceAccessTestCase(test.TestCase): self.assertEqual(states[0], rule_1['state']) self.assertEqual(states[-1], rule_4['state']) - @ddt.data(('nfs', True), ('cifs', False), ('ceph', False)) + @ddt.data(('nfs', True, False), ('nfs', False, True), + ('cifs', True, False), ('cifs', False, False), + ('cephx', True, False), ('cephx', False, False)) @ddt.unpack - def test__filter_ipv6_rules(self, proto, filtered): + def test__update_rules_through_share_driver(self, proto, + enable_ipv6, filtered): + self.driver.ipv6_implemented = enable_ipv6 + share_instance = {'share_proto': proto} + pass_rules, fail_rules = self._get_pass_rules_and_fail_rules() + pass_add_rules, fail_add_rules = self._get_pass_rules_and_fail_rules() + pass_delete_rules, fail_delete_rules = ( + self._get_pass_rules_and_fail_rules()) + test_rules = pass_rules + fail_rules + test_add_rules = pass_add_rules + fail_add_rules + test_delete_rules = pass_delete_rules + fail_delete_rules + + fake_expect_driver_update_rules = pass_rules + update_access_call = self.mock_object( + self.access_helper.driver, 'update_access', + mock.Mock(return_value=pass_rules)) + driver_update_rules = ( + self.access_helper._update_rules_through_share_driver( + self.context, share_instance=share_instance, + access_rules_to_be_on_share=test_rules, + add_rules=test_add_rules, + delete_rules=test_delete_rules, + rules_to_be_removed_from_db=test_rules, + share_server=None)) + + if filtered: + update_access_call.assert_called_once_with( + self.context, share_instance, + pass_rules, add_rules=pass_add_rules, + delete_rules=pass_delete_rules, share_server=None) + else: + update_access_call.assert_called_once_with( + self.context, share_instance, test_rules, + add_rules=test_add_rules, delete_rules=test_delete_rules, + share_server=None) + self.assertEqual(fake_expect_driver_update_rules, driver_update_rules) + + def _get_pass_rules_and_fail_rules(self): + random_value = six.text_type(random.randint(10, 32)) pass_rules = [ { 'access_type': 'ip', - 'access_to': '1.1.1.1' + 'access_to': '1.1.1.' + random_value, }, { 'access_type': 'ip', - 'access_to': '1.1.1.0/24' + 'access_to': '1.1.%s.0/24' % random_value, }, { 'access_type': 'user', - 'access_to': 'fake_user' + 'access_to': 'fake_user' + random_value, }, ] fail_rules = [ { 'access_type': 'ip', - 'access_to': '1001::1001' + 'access_to': '1001::' + random_value, }, { 'access_type': 'ip', - 'access_to': '1001::/64' + 'access_to': '%s::/64' % random_value, }, ] - test_rules = pass_rules + fail_rules - filtered_rules = self.access_helper._filter_ipv6_rules( - test_rules, proto) - if filtered: - self.assertEqual(pass_rules, filtered_rules) - else: - self.assertEqual(test_rules, filtered_rules) - - def test__get_rules_to_send_to_driver(self): - self.driver.ipv6_implemented = False - - share = db_utils.create_share(status=constants.STATUS_AVAILABLE) - share_instance = share['instance'] - db_utils.create_access(share_id=share['id'], access_to='1001::/64', - state=constants.ACCESS_STATE_ACTIVE) - self.mock_object( - self.access_helper, 'get_and_update_share_instance_access_rules', - mock.Mock(side_effect=self.access_helper. - get_and_update_share_instance_access_rules)) - - access_rules_to_be_on_share, add_rules, delete_rules = ( - self.access_helper._get_rules_to_send_to_driver( - self.context, share_instance)) - self.assertEqual([], add_rules) - self.assertEqual([], delete_rules) + return pass_rules, fail_rules diff --git a/manila_tempest_tests/tests/api/test_rules.py b/manila_tempest_tests/tests/api/test_rules.py index 229263ed7b..0b44a5c8e9 100644 --- a/manila_tempest_tests/tests/api/test_rules.py +++ b/manila_tempest_tests/tests/api/test_rules.py @@ -93,15 +93,17 @@ class ShareIpRulesForNFSTest(base.BaseSharesTest): @tc.attr(base.TAG_POSITIVE, base.TAG_BACKEND) @ddt.data(*itertools.chain( - itertools.product({'1.0', '2.9', '2.37', LATEST_MICROVERSION}, - {utils.rand_ip()}), - itertools.product({'2.37', LATEST_MICROVERSION}, - {utils.rand_ipv6_ip()}) + itertools.product({'1.0', '2.9', '2.37', LATEST_MICROVERSION}, {4}), + itertools.product({'2.38', LATEST_MICROVERSION}, {6}) )) @ddt.unpack def test_create_delete_access_rules_with_one_ip(self, version, - access_to): + ip_version): + if ip_version == 4: + access_to = utils.rand_ip() + else: + access_to = utils.rand_ipv6_ip() # create rule if utils.is_microversion_eq(version, '1.0'): rule = self.shares_client.create_access_rule( @@ -145,14 +147,15 @@ class ShareIpRulesForNFSTest(base.BaseSharesTest): @tc.attr(base.TAG_POSITIVE, base.TAG_BACKEND) @ddt.data(*itertools.chain( - itertools.product({'1.0', '2.9', '2.37', LATEST_MICROVERSION}, - {utils.rand_ip(network=True)}), - itertools.product({'2.37', LATEST_MICROVERSION}, - {utils.rand_ipv6_ip(network=True)}) + itertools.product({'1.0', '2.9', '2.37', LATEST_MICROVERSION}, {4}), + itertools.product({'2.38', LATEST_MICROVERSION}, {6}) )) @ddt.unpack - def test_create_delete_access_rule_with_cidr(self, version, access_to): - + def test_create_delete_access_rule_with_cidr(self, version, ip_version): + if ip_version == 4: + access_to = utils.rand_ip(network=True) + else: + access_to = utils.rand_ipv6_ip(network=True) # create rule if utils.is_microversion_eq(version, '1.0'): rule = self.shares_client.create_access_rule( diff --git a/releasenotes/notes/bug-1707066-deny-ipv6-access-in-error-bce379ee310060f6.yaml b/releasenotes/notes/bug-1707066-deny-ipv6-access-in-error-bce379ee310060f6.yaml new file mode 100644 index 0000000000..5462776aac --- /dev/null +++ b/releasenotes/notes/bug-1707066-deny-ipv6-access-in-error-bce379ee310060f6.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - The access-allow API accepts ipv6 rules and ignores them if the + configured backend does not support ipv6 access rules. + However, when the access-deny API is invoked to remove such + rules, they used to be stuck in "denying" state. This bug has been + fixed and ipv6 access rules can be denied successfully.