Merge "Fix cannot deny ipv6 access rules"

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.