Merge "Remove false-positive ACLs in OVN DB sync" into stable/2024.2

neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py

@@ -364,6 +364,33 @@ class OvnNbSynchronizer(OvnDbSynchronizer):
                 add_acls.append(na)
                 n_index += 1
 
+        # Check any ACLs we found to add against existing ACLs, ignoring the
+        # SG rule ID key. This eliminates any false-positives where the
+        # normalized cidr for two SG rules is the same value, since there
+        # will only be a single ACL that matches exactly with the SG rule ID.
+        if add_acls:
+            def copy_acl_rem_id_key(acl):
+                acl_copy = acl.copy()
+                del acl_copy[ovn_const.OVN_SG_RULE_EXT_ID_KEY]
+                return acl_copy
+
+            add_rem_acls = []
+            # Make a list of non-default rule ACLs (they have a security group
+            # rule id). See ovn_default_acls code/comment above for more info.
+            nd_ovn_acls = [copy_acl_rem_id_key(oa) for oa in ovn_acls
+                           if ovn_const.OVN_SG_RULE_EXT_ID_KEY in oa]
+            # We must copy here since we need to keep the original
+            # 'add_acl' intact for removal
+            for add_acl in add_acls:
+                add_acl_copy = copy_acl_rem_id_key(add_acl)
+                if add_acl_copy in nd_ovn_acls:
+                    add_rem_acls.append(add_acl)
+
+            # Remove any of the false-positive ACLs
+            LOG.warning('False-positive ACLs to remove: (%s)', add_rem_acls)
+            for add_rem in add_rem_acls:
+                add_acls.remove(add_rem)
+
         if n_index < neutron_num:
             # We didn't find the OVN ACLs matching the Neutron ACLs
             # in "ovn_acls" and we are just adding the pending Neutron ACLs.

neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py

@@ -1147,7 +1147,7 @@ class TestOvnNbSync(testlib_api.MySQLTestCaseMixin,
                 pass
         return acl_utils.filter_acl_dict(acl_to_compare, extra_fields)
 
-    def _validate_acls(self, should_match=True):
+    def _validate_acls(self, should_match=True, db_duplicate_port=None):
         # Get the neutron DB ACLs.
         db_acls = []
 
@@ -1195,6 +1195,19 @@ class TestOvnNbSync(testlib_api.MySQLTestCaseMixin,
         # Values taken out from list for comparison, since ACLs from OVN DB
         # have certain values on a list of just one object
         if should_match:
+            if db_duplicate_port:
+                # If we have a duplicate port, that indicates there are two
+                # DB entries that map to the same ACL. Remove the extra from
+                # our comparison.
+                dup_acl = None
+                for acl in db_acls:
+                    if (str(db_duplicate_port) in acl['match'] and
+                            acl not in plugin_acls):
+                        dup_acl = acl
+                        break
+                # There should have been a duplicate
+                self.assertIsNotNone(dup_acl)
+                db_acls.remove(dup_acl)
             for acl in plugin_acls:
                 if isinstance(acl['severity'], list) and acl['severity']:
                     acl['severity'] = acl['severity'][0]
@@ -1785,13 +1798,16 @@ class TestOvnNbSync(testlib_api.MySQLTestCaseMixin,
         nb_synchronizer.sync_fip_qos_policies(ctx)
         self._validate_qos_records()
 
-    def _create_security_group_rule(self, sg_id, direction, tcp_port):
+    def _create_security_group_rule(self, sg_id, direction, tcp_port,
+                                    remote_ip_prefix=None):
         data = {'security_group_rule': {'security_group_id': sg_id,
                                         'direction': direction,
                                         'protocol': constants.PROTO_NAME_TCP,
                                         'ethertype': constants.IPv4,
                                         'port_range_min': tcp_port,
                                         'port_range_max': tcp_port}}
+        if remote_ip_prefix:
+            data['security_group_rule']['remote_ip_prefix'] = remote_ip_prefix
         req = self.new_create_request('security-group-rules', data, self.fmt)
         res = req.get_response(self.api)
         sgr = self.deserialize(self.fmt, res)
@@ -1862,6 +1878,20 @@ class TestOvnNbSync(testlib_api.MySQLTestCaseMixin,
         self._test_sync_acls_helper(test_log=True,
                                     log_event=log_const.DROP_EVENT)
 
+    def test_sync_acls_overlapping_cidr(self):
+        data = {'security_group': {'name': 'sgdup'}}
+        sg_req = self.new_create_request('security-groups', data)
+        res = sg_req.get_response(self.api)
+        sg = self.deserialize(self.fmt, res)['security_group']
+
+        # Add SG rules that map to the same ACL due to normalizing the cidr
+        for ip_suffix in range(10, 12):
+            remote_ip_prefix = '192.168.0.' + str(ip_suffix) + '/24'
+            self._create_security_group_rule(
+                sg['id'], 'ingress', 9000, remote_ip_prefix=remote_ip_prefix)
+
+        self._validate_acls(db_duplicate_port=9000)
+
 
 class TestOvnSbSync(base.TestOVNFunctionalBase):