Delete sg rule which remote is the deleted sg

Based on bug #2008712 if we have a security-group which
is the remote group of a 2nd security-group, the backend
never deletes the rule of the 2nd group which
remote_group_id is the original security-group.
By AFTER_DELETE event for each rule that has the
security_group_id as remote_group_id, we can make the
mech drivers do their work and delete these rules in the
backend.

Change-Id: I207ecf7954b06507e03cb16b502ceb6e2807e0e7
(cherry picked from commit 6358495)
Closes-Bug: #2008712

Author: elajkat

neutron/db/securitygroups_db.py

@@ -253,6 +253,12 @@ def delete_security_group(self, context, id):
             if sg['name'] == 'default' and not context.is_admin:
                 raise ext_sg.SecurityGroupCannotRemoveDefault()
 
+            # Check if there are rules with remote_group_id ponting to
+            # the security_group to be deleted
+            rules_ids_as_remote = self._get_security_group_rules_by_remote(
+                context=context, remote_id=id,
+            )
+
         self._registry_publish(resources.SECURITY_GROUP,
                                events.BEFORE_DELETE,
                                exc_cls=ext_sg.SecurityGroupInUse, id=id,
@@ -285,6 +291,20 @@ def delete_security_group(self, context, id):
                              context, resource_id=id, states=(sec_group,),
                              metadata={'security_group_rule_ids': sgr_ids,
                                        'name': sg['name']}))
+        for rule in rules_ids_as_remote:
+            registry.publish(
+                resources.SECURITY_GROUP_RULE,
+                events.AFTER_DELETE,
+                self,
+                payload=events.DBEventPayload(
+                    context,
+                    resource_id=rule['id'],
+                    metadata={'security_group_id': rule['security_group_id'],
+                              'remote_group_id': rule['remote_group_id'],
+                              'rule': rule
+                              }
+                )
+            )
 
     @db_api.retry_if_session_inactive()
     def update_security_group(self, context, id, security_group):
@@ -371,6 +391,23 @@ def _get_port_security_group_bindings(self, context,
             self._make_security_group_binding_dict,
             filters=filters, fields=fields)
 
+    def _get_security_group_rules_by_remote(self, context, remote_id):
+        return model_query.get_collection(
+            context, sg_models.SecurityGroupRule,
+            self._make_security_group_rule_dict,
+            filters={'remote_group_id': [remote_id]},
+            fields=['id',
+                    'remote_group_id',
+                    'security_group_id',
+                    'direction',
+                    'ethertype',
+                    'protocol',
+                    'port_range_min',
+                    'port_range_max',
+                    'normalized_cidr'
+                    ]
+        )
+
     @db_api.retry_if_session_inactive()
     def _delete_port_security_group_bindings(self, context, port_id):
         with db_api.CONTEXT_WRITER.using(context):

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

@@ -253,9 +253,6 @@ def subscribe(self):
             registry.subscribe(self._create_security_group,
                                resources.SECURITY_GROUP,
                                events.AFTER_CREATE)
-            registry.subscribe(self._delete_security_group_precommit,
-                               resources.SECURITY_GROUP,
-                               events.PRECOMMIT_DELETE)
             registry.subscribe(self._delete_security_group,
                                resources.SECURITY_GROUP,
                                events.AFTER_DELETE)
@@ -268,6 +265,9 @@ def subscribe(self):
             registry.subscribe(self._process_sg_rule_notification,
                                resources.SECURITY_GROUP_RULE,
                                events.BEFORE_DELETE)
+            registry.subscribe(self._process_sg_rule_after_del_notification,
+                               resources.SECURITY_GROUP_RULE,
+                               events.AFTER_DELETE)
 
     def _clean_hash_ring(self, *args, **kwargs):
         admin_context = n_context.get_admin_context()
@@ -384,14 +384,6 @@ def _create_security_group(self, resource, event, trigger, payload):
         self._ovn_client.create_security_group(context,
                                                security_group)
 
-    def _delete_security_group_precommit(self, resource, event, trigger,
-                                         payload):
-        context = n_context.get_admin_context()
-        security_group_id = payload.resource_id
-        for sg_rule in self._plugin.get_security_group_rules(
-                context, filters={'remote_group_id': [security_group_id]}):
-            self._ovn_client.delete_security_group_rule(context, sg_rule)
-
     def _delete_security_group(self, resource, event, trigger, payload):
         context = payload.context
         security_group_id = payload.resource_id
@@ -449,6 +441,12 @@ def _process_sg_rule_notification(
                 context,
                 sg_rule)
 
+    def _process_sg_rule_after_del_notification(
+            self, resource, event, trigger, payload):
+        context = payload.context
+        sg_rule = payload.metadata['rule']
+        self._ovn_client.delete_security_group_rule(context, sg_rule)
+
     def _sg_has_rules_with_same_normalized_cidr(self, sg_rule):
         compare_keys = [
             'ethertype', 'direction', 'protocol',

neutron/tests/unit/db/test_securitygroups_db.py

@@ -404,6 +404,38 @@ def test_security_group_precommit_and_after_delete_event(self):
             self.assertEqual([mock.ANY, mock.ANY],
                              payload.metadata.get('security_group_rule_ids'))
 
+    def test_security_group_rule_after_delete_event_for_remot_group(self):
+        sg1_dict = self.mixin.create_security_group(self.ctx, FAKE_SECGROUP)
+        sg2_dict = self.mixin.create_security_group(self.ctx, FAKE_SECGROUP)
+
+        fake_rule = copy.deepcopy(FAKE_SECGROUP_RULE)
+        fake_rule['security_group_rule']['security_group_id'] = sg1_dict['id']
+        fake_rule['security_group_rule']['remote_group_id'] = sg2_dict['id']
+        fake_rule['security_group_rule']['remote_ip_prefix'] = None
+        remote_rule = self.mixin.create_security_group_rule(
+            self.ctx, fake_rule)
+
+        with mock.patch.object(registry, "publish") as mock_publish:
+            self.mixin.delete_security_group(self.ctx, sg2_dict['id'])
+            mock_publish.assert_has_calls(
+                [mock.call('security_group', 'before_delete',
+                           mock.ANY, payload=mock.ANY),
+                 mock.call('security_group', 'precommit_delete',
+                           mock.ANY,
+                           payload=mock.ANY),
+                 mock.call('security_group', 'after_delete',
+                           mock.ANY,
+                           payload=mock.ANY),
+                 mock.call('security_group_rule', 'after_delete',
+                           mock.ANY,
+                           payload=mock.ANY)])
+            rule_payload = mock_publish.mock_calls[3][2]['payload']
+            self.assertEqual(remote_rule['id'], rule_payload.resource_id)
+            self.assertEqual(sg1_dict['id'],
+                             rule_payload.metadata['security_group_id'])
+            self.assertEqual(sg2_dict['id'],
+                             rule_payload.metadata['remote_group_id'])
+
     def test_security_group_rule_precommit_create_event_fail(self):
         registry.subscribe(fake_callback, resources.SECURITY_GROUP_RULE,
                            events.PRECOMMIT_CREATE)