From 63584957203ec9f5ba165177978213c3909f81f0 Mon Sep 17 00:00:00 2001 From: elajkat Date: Fri, 10 Mar 2023 13:29:48 +0100 Subject: [PATCH] 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 Closes-Bug: #2008712 --- neutron/db/securitygroups_db.py | 37 +++++++++++++++++++ .../drivers/ovn/mech_driver/mech_driver.py | 20 +++++----- .../tests/unit/db/test_securitygroups_db.py | 32 ++++++++++++++++ 3 files changed, 78 insertions(+), 11 deletions(-) diff --git a/neutron/db/securitygroups_db.py b/neutron/db/securitygroups_db.py index f65b1d8fc3e..57cf316ee07 100644 --- a/neutron/db/securitygroups_db.py +++ b/neutron/db/securitygroups_db.py @@ -251,6 +251,12 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase, 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, @@ -279,6 +285,20 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase, 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): @@ -365,6 +385,23 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase, 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): diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py index ef16d06227d..1ffb4059b51 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py @@ -265,9 +265,6 @@ class OVNMechanismDriver(api.MechanismDriver): 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) @@ -280,6 +277,9 @@ class OVNMechanismDriver(api.MechanismDriver): 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() @@ -396,14 +396,6 @@ class OVNMechanismDriver(api.MechanismDriver): 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 @@ -461,6 +453,12 @@ class OVNMechanismDriver(api.MechanismDriver): 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', diff --git a/neutron/tests/unit/db/test_securitygroups_db.py b/neutron/tests/unit/db/test_securitygroups_db.py index 593272027b7..9036a11e9d1 100644 --- a/neutron/tests/unit/db/test_securitygroups_db.py +++ b/neutron/tests/unit/db/test_securitygroups_db.py @@ -404,6 +404,38 @@ class SecurityGroupDbMixinTestCase(testlib_api.SqlTestCase): 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)