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

@@ -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):

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
+        return 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)

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},
+        itertools.product({'1.0', '2.9', '2.37', LATEST_MICROVERSION}, {4}),
-                          {utils.rand_ip()}),
+        itertools.product({'2.38', LATEST_MICROVERSION}, {6})
-        itertools.product({'2.37', LATEST_MICROVERSION},
-                          {utils.rand_ipv6_ip()})
     ))
     @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},
+        itertools.product({'1.0', '2.9', '2.37', LATEST_MICROVERSION}, {4}),
-                          {utils.rand_ip(network=True)}),
+        itertools.product({'2.38', LATEST_MICROVERSION}, {6})
-        itertools.product({'2.37', LATEST_MICROVERSION},
-                          {utils.rand_ipv6_ip(network=True)})
     ))
     @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(

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.