From e2d8d39fa13fad175d60ec1affc1de280a396fb6 Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Hernandez Date: Wed, 6 Nov 2024 11:20:44 +0000 Subject: [PATCH] [NB] Add "if_exists" flag to ACL and Port_Group deletion commands Related-Bug: #2084977 Change-Id: I64fd656fa7ce48e44bcf2881e7a06697335fd09f --- ovsdbapp/schema/ovn_northbound/api.py | 11 ++++- ovsdbapp/schema/ovn_northbound/commands.py | 18 ++++---- ovsdbapp/schema/ovn_northbound/impl_idl.py | 10 +++-- .../schema/ovn_northbound/test_impl_idl.py | 43 +++++++++++++++++++ 4 files changed, 68 insertions(+), 14 deletions(-) diff --git a/ovsdbapp/schema/ovn_northbound/api.py b/ovsdbapp/schema/ovn_northbound/api.py index b8ca143c..e60f3646 100644 --- a/ovsdbapp/schema/ovn_northbound/api.py +++ b/ovsdbapp/schema/ovn_northbound/api.py @@ -128,7 +128,8 @@ class API(api.API, metaclass=abc.ABCMeta): """ @abc.abstractmethod - def acl_del(self, switch, direction=None, priority=None, match=None): + def acl_del(self, switch, direction=None, priority=None, match=None, + if_exists=False): """Remove ACLs from 'switch' If only switch is supplied, all the ACLs from the logical switch are @@ -144,6 +145,9 @@ class API(api.API, metaclass=abc.ABCMeta): :type priority: int :param match: The match rule :type match: string + :param if_exists: If True, don't fail if the parent logical switch + containing the ACL doesn't exist + :type if_exists: boolean :returns: :class:`Command` with no result """ @@ -189,7 +193,7 @@ class API(api.API, metaclass=abc.ABCMeta): @abc.abstractmethod def pg_acl_del(self, port_group, direction=None, priority=None, - match=None): + match=None, if_exists=False): """Remove ACLs from 'port_group' If only port_group is supplied, all the ACLs from the logical switch @@ -205,6 +209,9 @@ class API(api.API, metaclass=abc.ABCMeta): :type priority: int :param match: The match rule :type match: string + :param if_exists: If True, don't fail if the parent port group + containing the ACL doesn't exist + :type if_exists: boolean :returns: :class:`Command` with no result """ diff --git a/ovsdbapp/schema/ovn_northbound/commands.py b/ovsdbapp/schema/ovn_northbound/commands.py index 21cd9402..d9503d68 100644 --- a/ovsdbapp/schema/ovn_northbound/commands.py +++ b/ovsdbapp/schema/ovn_northbound/commands.py @@ -190,13 +190,14 @@ class PgAclAddCommand(_AclAddHelper): class _AclDelHelper(cmd.BaseCommand): def __init__(self, api, entity, direction=None, - priority=None, match=None): + priority=None, match=None, if_exists=False): if (priority is None) != (match is None): raise TypeError("Must specify priority and match together") if priority is not None and not direction: raise TypeError("Cannot specify priority/match without direction") super().__init__(api) self.entity = entity + self.if_exists = if_exists self.conditions = [] if direction: self.conditions.append(('direction', '=', direction)) @@ -206,7 +207,14 @@ class _AclDelHelper(cmd.BaseCommand): ('match', '=', match)] def run_idl(self, txn): - entity = self.api.lookup(self.lookup_table, self.entity) + try: + entity = self.api.lookup(self.lookup_table, self.entity) + except idlutils.RowNotFound as e: + if self.if_exists: + return + msg = "%s %s does not exist" % (self.lookup_table, self.entity) + raise RuntimeError(msg) from e + for acl in [a for a in entity.acls if idlutils.row_match(a, self.conditions)]: entity.delvalue('acls', acl) @@ -216,12 +224,6 @@ class _AclDelHelper(cmd.BaseCommand): class AclDelCommand(_AclDelHelper): lookup_table = 'Logical_Switch' - def __init__(self, api, switch, direction=None, - priority=None, match=None): - # NOTE: we're overriding the constructor here to not break any - # existing callers before we introduced Port Groups. - super().__init__(api, switch, direction, priority, match) - class PgAclDelCommand(_AclDelHelper): lookup_table = 'Port_Group' diff --git a/ovsdbapp/schema/ovn_northbound/impl_idl.py b/ovsdbapp/schema/ovn_northbound/impl_idl.py index d3e790a6..97ce0f07 100644 --- a/ovsdbapp/schema/ovn_northbound/impl_idl.py +++ b/ovsdbapp/schema/ovn_northbound/impl_idl.py @@ -61,8 +61,10 @@ class OvnNbApiIdlImpl(ovs_idl.Backend, api.API): return cmd.AclAddCommand(self, switch, direction, priority, match, action, log, may_exist, **external_ids) - def acl_del(self, switch, direction=None, priority=None, match=None): - return cmd.AclDelCommand(self, switch, direction, priority, match) + def acl_del(self, switch, direction=None, priority=None, match=None, + if_exists=False): + return cmd.AclDelCommand(self, switch, direction, priority, match, + if_exists) def acl_list(self, switch): return cmd.AclListCommand(self, switch) @@ -76,9 +78,9 @@ class OvnNbApiIdlImpl(ovs_idl.Backend, api.API): **external_ids) def pg_acl_del(self, port_group, direction=None, priority=None, - match=None): + match=None, if_exists=False): return cmd.PgAclDelCommand(self, port_group, direction, priority, - match) + match, if_exists) def pg_acl_list(self, port_group): return cmd.PgAclListCommand(self, port_group) diff --git a/ovsdbapp/tests/functional/schema/ovn_northbound/test_impl_idl.py b/ovsdbapp/tests/functional/schema/ovn_northbound/test_impl_idl.py index cd1dafaf..99891ed3 100644 --- a/ovsdbapp/tests/functional/schema/ovn_northbound/test_impl_idl.py +++ b/ovsdbapp/tests/functional/schema/ovn_northbound/test_impl_idl.py @@ -217,6 +217,27 @@ class TestAclOps(OvnNorthboundTest): self.assertRaises(TypeError, self.api.acl_del, self.switch.uuid, priority=0) + def test_acl_del_acl_not_present(self): + r1 = self._acl_add('lswitch', 'from-lport', 0, + 'output == "fake_port"', 'drop') + cmd = self.api.acl_del(self.switch.uuid, + 'from-lport', 0, 'output == "fake_port"') + cmd.execute(check_error=True) + self.assertNotIn(r1, self.switch.acls) + + # The acl_del command is idempotent. + cmd.execute(check_error=True) + self.assertNotIn(r1, self.switch.acls) + + def test_acl_del_if_exists_false(self): + cmd = self.api.acl_del('lswitch2', 'from-lport', 0, 'match') + self.assertRaises(RuntimeError, cmd.execute, check_error=True) + + def test_acl_del_if_exists_true(self): + self.assertIsNone( + self.api.acl_del('lswitch2', 'from-lport', 0, 'match', + if_exists=True).execute(check_error=True)) + def test_acl_list(self): r1 = self._acl_add('lswitch', 'from-lport', 0, 'output == "fake_port"', 'drop') @@ -255,6 +276,28 @@ class TestAclOps(OvnNorthboundTest): self.assertNotIn(r1.uuid, self.api.tables['ACL'].rows) self.assertEqual([], self.port_group.acls) + def test_pg_acl_del_acl_not_present(self): + r1 = self._acl_add('port_group', 'from-lport', 0, + 'output == "fake_port"', 'drop') + cmd = self.api.pg_acl_del(self.port_group.uuid) + cmd.execute(check_error=True) + self.assertNotIn(r1.uuid, self.api.tables['ACL'].rows) + self.assertEqual([], self.port_group.acls) + + # The pg_acl_del command is idempotent. + cmd.execute(check_error=True) + self.assertNotIn(r1.uuid, self.api.tables['ACL'].rows) + self.assertEqual([], self.port_group.acls) + + def test_pg_acl_del_if_exists_false(self): + cmd = self.api.pg_acl_del('port_group2') + self.assertRaises(RuntimeError, cmd.execute, check_error=True) + + def test_pg_acl_del_if_exists_true(self): + self.assertIsNone( + self.api.pg_acl_del('port_group2', + if_exists=True).execute(check_error=True)) + def test_pg_acl_list(self): r1 = self._acl_add('port_group', 'from-lport', 0, 'output == "fake_port"', 'drop')