From 7e980f1ae5acc0af8810e783d4f3c27c194451c8 Mon Sep 17 00:00:00 2001 From: Daniel Alvarez Date: Fri, 1 Jun 2018 09:45:57 +0200 Subject: [PATCH] Add Port Group ACL commands After introducing the concept of Port Groups in OVN, ACLs can be applied either to a Logical Switch or to a Port Group. This patch is adding the commands to support adding ACLs to a Port Group. Also it's removing the initial implementation of adding standalone ACLs to a Port Group. Change-Id: I7720cd7aa801fbc4be87e1c665e4549725576269 Signed-off-by: Daniel Alvarez --- ovsdbapp/schema/ovn_northbound/api.py | 83 +++++++----- ovsdbapp/schema/ovn_northbound/commands.py | 128 +++++++++++------- ovsdbapp/schema/ovn_northbound/impl_idl.py | 26 ++-- .../schema/ovn_northbound/fixtures.py | 6 + .../schema/ovn_northbound/test_impl_idl.py | 100 ++++++++------ 5 files changed, 216 insertions(+), 127 deletions(-) diff --git a/ovsdbapp/schema/ovn_northbound/api.py b/ovsdbapp/schema/ovn_northbound/api.py index 359fa3d6..d03edd2a 100644 --- a/ovsdbapp/schema/ovn_northbound/api.py +++ b/ovsdbapp/schema/ovn_northbound/api.py @@ -140,6 +140,56 @@ class API(api.API): :returns: :class:`Command` with RowView list result """ + @abc.abstractmethod + def pg_acl_add(self, port_group, direction, priority, match, action, + log=False): + """Add an ACL to 'port_group' + + :param port_group: The name or uuid of the port group + :type port_group: string or uuid.UUID + :param direction: The traffic direction to match + :type direction: 'from-lport' or 'to-lport' + :param priority: The priority field of the ACL + :type priority: int + :param match: The match rule + :type match: string + :param action: The action to take upon match + :type action: 'allow', 'allow-related', 'drop', or 'reject' + :param log: If True, enable packet logging for the ACL + :type log: boolean + :returns: :class:`Command` with RowView result + """ + + @abc.abstractmethod + def pg_acl_del(self, port_group, direction=None, priority=None, + match=None): + """Remove ACLs from 'port_group' + + If only port_group is supplied, all the ACLs from the logical switch + are deleted. If direction is also specified, then all the flows in + that direction will be deleted from the Port Group. If all the fields + are given, then only flows that match all fields will be deleted. + + :param port_group: The name or uuid of the port group + :type port_group: string or uuid.UUID + :param direction: The traffic direction to match + :type direction: 'from-lport' or 'to-lport' + :param priority: The priority field of the ACL + :type priority: int + :param match: The match rule + :type match: string + :returns: :class:`Command` with no result + """ + + @abc.abstractmethod + def pg_acl_list(self, port_group): + """Get the ACLs for 'port group' + + :param port_group: The name or uuid of the switch + :type port_group: string or uuid.UUID + :returns: :class:`Command` with RowView list result + """ + @abc.abstractmethod def qos_add(self, switch, direction, priority, match, rate=None, burst=None, dscp=None, may_exist=False, **columns): @@ -823,10 +873,10 @@ class API(api.API): """ @abc.abstractmethod - def pg_add(self, name, may_exist=False, **columns): + def pg_add(self, name=None, may_exist=False, **columns): """Create a port group - :param name: The name of the port group + :param name: The name of the port group (optional) :type name: string :param may_exist: If True, don't fail if the port group already exists @@ -878,32 +928,3 @@ class API(api.API): :type if_exists: boolean :returns: :class:`Command` with no result """ - - @abc.abstractmethod - def pg_add_acls(self, pg_id, acl): - """Add a list of ACL to a port group - - :param pg_id: The name or uuid of the port group - :type pg_id: string or uuid.UUID - :param acl: The ACL instance or UUID - :type acl: A list of ACL instance or string or uuid.UUID - :param acl: A list of :class:`Command` with an ACL instance - result or UUID - :type acl: A :class:`Command` with an ACL or string - r uuid.UUID - :returns: :class:`Command` with no result - """ - - @abc.abstractmethod - def pg_del_acls(self, pg_id, acl, if_exists=False): - """Delete a list of ACL from a port group - - :param pg_id: The name or uuid of the port group - :type pg_id: string or uuid.UUID - :type acl: A list of ACL instance or string or uuid.UUID - :param acl: A list of :class:`Command` with an ACL instance - result or UUID - :type if_exists: If True, don't fail if the ACL(s) 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 904293de..942e6900 100644 --- a/ovsdbapp/schema/ovn_northbound/commands.py +++ b/ovsdbapp/schema/ovn_northbound/commands.py @@ -81,11 +81,12 @@ class LsGetCommand(cmd.BaseGetRowCommand): table = 'Logical_Switch' -class AclAddCommand(cmd.AddCommand): +class _AclAddHelper(cmd.AddCommand): table_name = 'ACL' - def __init__(self, api, switch, direction, priority, match, action, - log=False, may_exist=False, **external_ids): + def __init__(self, api, entity, direction, priority, match, action, + log=False, may_exist=False, severity=None, name=None, + **external_ids): if direction not in ('from-lport', 'to-lport'): raise TypeError("direction must be either from-lport or to-lport") if not 0 <= priority <= const.ACL_PRIORITY_MAX: @@ -93,14 +94,16 @@ class AclAddCommand(cmd.AddCommand): const.ACL_PRIORITY_MAX)) if action not in ('allow', 'allow-related', 'drop', 'reject'): raise TypeError("action must be allow/allow-related/drop/reject") - super(AclAddCommand, self).__init__(api) - self.switch = switch + super(_AclAddHelper, self).__init__(api) + self.entity = entity self.direction = direction self.priority = priority self.match = match self.action = action self.log = log self.may_exist = may_exist + self.severity = severity + self.name = name self.external_ids = external_ids def acl_match(self, row): @@ -109,8 +112,8 @@ class AclAddCommand(cmd.AddCommand): self.match == row.match) def run_idl(self, txn): - ls = self.api.lookup('Logical_Switch', self.switch) - acls = [acl for acl in ls.acls if self.acl_match(acl)] + entity = self.api.lookup(self.lookup_table, self.entity) + acls = [acl for acl in entity.acls if self.acl_match(acl)] if acls: if self.may_exist: self.result = rowview.RowView(acls[0]) @@ -123,45 +126,87 @@ class AclAddCommand(cmd.AddCommand): acl.match = self.match acl.action = self.action acl.log = self.log - ls.addvalue('acls', acl) + acl.severity = self.severity + acl.name = self.name + entity.addvalue('acls', acl) for col, value in self.external_ids.items(): acl.setkey('external_ids', col, value) self.result = acl.uuid -class AclDelCommand(cmd.BaseCommand): - def __init__(self, api, switch, direction=None, +class AclAddCommand(_AclAddHelper): + lookup_table = 'Logical_Switch' + + def __init__(self, api, switch, direction, priority, match, action, + log=False, may_exist=False, severity=None, name=None, + **external_ids): + # NOTE: we're overriding the constructor here to not break any + # existing callers before we introduced Port Groups. + super(AclAddCommand, self).__init__(api, switch, direction, priority, + match, action, log, may_exist, + severity, name, **external_ids) + + +class PgAclAddCommand(_AclAddHelper): + lookup_table = 'Port_Group' + + +class _AclDelHelper(cmd.BaseCommand): + def __init__(self, api, entity, direction=None, priority=None, match=None): 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(AclDelCommand, self).__init__(api) - self.switch = switch + super(_AclDelHelper, self).__init__(api) + self.entity = entity self.conditions = [] if direction: self.conditions.append(('direction', '=', direction)) # priority can be 0 - if match: # and therefor prioroity due to the above check + if match: # and therefore priority due to the above check self.conditions += [('priority', '=', priority), ('match', '=', match)] def run_idl(self, txn): - ls = self.api.lookup('Logical_Switch', self.switch) - for acl in [a for a in ls.acls + entity = self.api.lookup(self.lookup_table, self.entity) + for acl in [a for a in entity.acls if idlutils.row_match(a, self.conditions)]: - ls.delvalue('acls', acl) + entity.delvalue('acls', acl) acl.delete() -class AclListCommand(cmd.BaseCommand): - def __init__(self, api, switch): - super(AclListCommand, self).__init__(api) - self.switch = switch +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(AclDelCommand, self).__init__(api, switch, direction, priority, + match) + + +class PgAclDelCommand(_AclDelHelper): + lookup_table = 'Port_Group' + + +class _AclListHelper(cmd.BaseCommand): + def __init__(self, api, entity): + super(_AclListHelper, self).__init__(api) + self.entity = entity def run_idl(self, txn): - ls = self.api.lookup('Logical_Switch', self.switch) - self.result = [rowview.RowView(acl) for acl in ls.acls] + entity = self.api.lookup(self.lookup_table, self.entity) + self.result = [rowview.RowView(acl) for acl in entity.acls] + + +class AclListCommand(_AclListHelper): + lookup_table = 'Logical_Switch' + + +class PgAclListCommand(_AclListHelper): + lookup_table = 'Port_Group' class QoSAddCommand(cmd.AddCommand): @@ -1173,7 +1218,7 @@ class PgAddCommand(cmd.AddCommand): pass pg = txn.insert(self.api._tables[self.table_name]) - pg.name = self.name + pg.name = self.name or "" self.set_columns(pg, **self.columns) self.result = pg.uuid @@ -1196,40 +1241,34 @@ class PgDelCommand(cmd.BaseCommand): raise RuntimeError('Port group %s does not exist' % self.name) -class _PgUpdateHelper(cmd.BaseCommand): +class _PgUpdatePortsHelper(cmd.BaseCommand): method = None - def __init__(self, api, port_group, lsp=None, acl=None, if_exists=False): - super(_PgUpdateHelper, self).__init__(api) + def __init__(self, api, port_group, lsp=None, if_exists=False): + super(_PgUpdatePortsHelper, self).__init__(api) self.port_group = port_group self.lsp = [] if lsp is None else self._listify(lsp) - self.acl = [] if acl is None else self._listify(acl) self.if_exists = if_exists def _listify(self, res): return res if isinstance(res, (list, tuple)) else [res] - def _fetch_resource(self, type_, uuid_): - table = 'Logical_Switch_Port' if type_ == 'ports' else 'ACL' - return self.api.lookup(table, uuid_) - - def _run_method(self, pg, column, resource): - if not resource: + def _run_method(self, pg, port): + if not port: return - if isinstance(resource, cmd.BaseCommand): - resource = resource.result - elif uuidutils.is_uuid_like(resource): + if isinstance(port, cmd.BaseCommand): + port = port.result + elif uuidutils.is_uuid_like(port): try: - resource = self._fetch_resource(column, resource) + port = self.api.lookup('Logical_Switch_Port', port) except idlutils.RowNotFound: if self.if_exists: return raise RuntimeError( - 'Resource %(res)s of type "%(type)s" does not exist' % - {'res': resource, 'type': column}) + 'Port %s does not exist' % port) - getattr(pg, self.method)(column, resource) + getattr(pg, self.method)('ports', port) def run_idl(self, txn): try: @@ -1239,15 +1278,12 @@ class _PgUpdateHelper(cmd.BaseCommand): self.port_group) for lsp in self.lsp: - self._run_method(pg, 'ports', lsp) - - for acl in self.acl: - self._run_method(pg, 'acls', acl) + self._run_method(pg, lsp) -class PgAddDataCommand(_PgUpdateHelper): +class PgAddPortCommand(_PgUpdatePortsHelper): method = 'addvalue' -class PgDelDataCommand(_PgUpdateHelper): +class PgDelPortCommand(_PgUpdatePortsHelper): method = 'delvalue' diff --git a/ovsdbapp/schema/ovn_northbound/impl_idl.py b/ovsdbapp/schema/ovn_northbound/impl_idl.py index 593e7bad..2597a099 100644 --- a/ovsdbapp/schema/ovn_northbound/impl_idl.py +++ b/ovsdbapp/schema/ovn_northbound/impl_idl.py @@ -64,6 +64,20 @@ class OvnNbApiIdlImpl(ovs_idl.Backend, api.API): def acl_list(self, switch): return cmd.AclListCommand(self, switch) + def pg_acl_add(self, port_group, direction, priority, match, action, + log=False, may_exist=False, **external_ids): + return cmd.PgAclAddCommand(self, port_group, direction, priority, + match, action, log, may_exist, + **external_ids) + + def pg_acl_del(self, port_group, direction=None, priority=None, + match=None): + return cmd.PgAclDelCommand(self, port_group, direction, priority, + match) + + def pg_acl_list(self, port_group): + return cmd.PgAclListCommand(self, port_group) + def qos_add(self, switch, direction, priority, match, rate=None, burst=None, dscp=None, may_exist=False, **columns): return cmd.QoSAddCommand(self, switch, direction, priority, match, @@ -261,20 +275,14 @@ class OvnNbApiIdlImpl(ovs_idl.Backend, api.API): def dns_set_external_ids(self, uuid, **external_ids): return cmd.DnsSetExternalIdsCommand(self, uuid, **external_ids) - def pg_add(self, name, may_exist=False, **columns): + def pg_add(self, name=None, may_exist=False, **columns): return cmd.PgAddCommand(self, name, may_exist=may_exist, **columns) def pg_del(self, name, if_exists=False): return cmd.PgDelCommand(self, name, if_exists=if_exists) def pg_add_ports(self, pg_id, lsp): - return cmd.PgAddDataCommand(self, pg_id, lsp=lsp) + return cmd.PgAddPortCommand(self, pg_id, lsp=lsp) def pg_del_ports(self, pg_id, lsp, if_exists=False): - return cmd.PgDelDataCommand(self, pg_id, lsp=lsp, if_exists=if_exists) - - def pg_add_acls(self, pg_id, acl): - return cmd.PgAddDataCommand(self, pg_id, acl=acl) - - def pg_del_acls(self, pg_id, acl, if_exists=False): - return cmd.PgDelDataCommand(self, pg_id, acl=acl, if_exists=if_exists) + return cmd.PgDelPortCommand(self, pg_id, lsp=lsp, if_exists=if_exists) diff --git a/ovsdbapp/tests/functional/schema/ovn_northbound/fixtures.py b/ovsdbapp/tests/functional/schema/ovn_northbound/fixtures.py index fdfbbfbf..32bd3c22 100644 --- a/ovsdbapp/tests/functional/schema/ovn_northbound/fixtures.py +++ b/ovsdbapp/tests/functional/schema/ovn_northbound/fixtures.py @@ -44,3 +44,9 @@ class DnsFixture(fixtures.ImplIdlFixture): create = 'dns_add' delete = 'dns_del' delete_args = {} + + +class PortGroupFixture(fixtures.ImplIdlFixture): + api = impl_idl.OvnNbApiIdlImpl + create = 'pg_add' + delete = 'pg_del' 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 6386da4a..2e945ea2 100644 --- a/ovsdbapp/tests/functional/schema/ovn_northbound/test_impl_idl.py +++ b/ovsdbapp/tests/functional/schema/ovn_northbound/test_impl_idl.py @@ -108,11 +108,19 @@ class TestAclOps(OvnNorthboundTest): def setUp(self): super(TestAclOps, self).setUp() self.switch = self.useFixture(fixtures.LogicalSwitchFixture()).obj + self.port_group = self.useFixture(fixtures.PortGroupFixture()).obj + + def _acl_add(self, entity, *args, **kwargs): + self.assertIn(entity, ['lswitch', 'port_group']) + if entity == 'lswitch': + cmd = self.api.acl_add(self.switch.uuid, *args, **kwargs) + resource = self.switch + else: + cmd = self.api.pg_acl_add(self.port_group.uuid, *args, **kwargs) + resource = self.port_group - def _acl_add(self, *args, **kwargs): - cmd = self.api.acl_add(self.switch.uuid, *args, **kwargs) aclrow = cmd.execute(check_error=True) - self.assertIn(aclrow._row, self.switch.acls) + self.assertIn(aclrow._row, resource.acls) self.assertEqual(cmd.direction, aclrow.direction) self.assertEqual(cmd.priority, aclrow.priority) self.assertEqual(cmd.match, aclrow.match) @@ -120,43 +128,50 @@ class TestAclOps(OvnNorthboundTest): return aclrow def test_acl_add(self): - self._acl_add('from-lport', 0, 'output == "fake_port" && ip', - 'drop') + self._acl_add('lswitch', 'from-lport', 0, + 'output == "fake_port" && ip', 'drop') def test_acl_add_exists(self): - args = ('from-lport', 0, 'output == "fake_port" && ip', 'drop') + args = ('lswitch', 'from-lport', 0, 'output == "fake_port" && ip', + 'drop') self._acl_add(*args) self.assertRaises(RuntimeError, self._acl_add, *args) def test_acl_add_may_exist(self): args = ('from-lport', 0, 'output == "fake_port" && ip', 'drop') - row = self._acl_add(*args) - row2 = self._acl_add(*args, may_exist=True) + row = self._acl_add('lswitch', *args) + row2 = self._acl_add('lswitch', *args, may_exist=True) self.assertEqual(row, row2) def test_acl_add_extids(self): external_ids = {'mykey': 'myvalue', 'yourkey': 'yourvalue'} - acl = self._acl_add('from-lport', 0, 'output == "fake_port" && ip', + acl = self._acl_add('lswitch', + 'from-lport', 0, 'output == "fake_port" && ip', 'drop', **external_ids) self.assertEqual(external_ids, acl.external_ids) def test_acl_del_all(self): - r1 = self._acl_add('from-lport', 0, 'output == "fake_port"', 'drop') + r1 = self._acl_add('lswitch', 'from-lport', 0, 'output == "fake_port"', + 'drop') self.api.acl_del(self.switch.uuid).execute(check_error=True) self.assertNotIn(r1.uuid, self.api.tables['ACL'].rows) self.assertEqual([], self.switch.acls) def test_acl_del_direction(self): - r1 = self._acl_add('from-lport', 0, 'output == "fake_port"', 'drop') - r2 = self._acl_add('to-lport', 0, 'output == "fake_port"', 'allow') + r1 = self._acl_add('lswitch', 'from-lport', 0, + 'output == "fake_port"', 'drop') + r2 = self._acl_add('lswitch', 'to-lport', 0, + 'output == "fake_port"', 'allow') self.api.acl_del(self.switch.uuid, 'from-lport').execute( check_error=True) self.assertNotIn(r1, self.switch.acls) self.assertIn(r2, self.switch.acls) def test_acl_del_direction_priority_match(self): - r1 = self._acl_add('from-lport', 0, 'output == "fake_port"', 'drop') - r2 = self._acl_add('from-lport', 1, 'output == "fake_port"', 'allow') + r1 = self._acl_add('lswitch', 'from-lport', 0, + 'output == "fake_port"', 'drop') + r2 = self._acl_add('lswitch', 'from-lport', 1, + 'output == "fake_port"', 'allow') cmd = self.api.acl_del(self.switch.uuid, 'from-lport', 0, 'output == "fake_port"') cmd.execute(check_error=True) @@ -172,12 +187,35 @@ class TestAclOps(OvnNorthboundTest): priority=0) def test_acl_list(self): - r1 = self._acl_add('from-lport', 0, 'output == "fake_port"', 'drop') - r2 = self._acl_add('from-lport', 1, 'output == "fake_port2"', 'allow') + r1 = self._acl_add('lswitch', 'from-lport', 0, + 'output == "fake_port"', 'drop') + r2 = self._acl_add('lswitch', 'from-lport', 1, + 'output == "fake_port2"', 'allow') acls = self.api.acl_list(self.switch.uuid).execute(check_error=True) self.assertIn(r1, acls) self.assertIn(r2, acls) + def test_pg_acl_add(self): + self._acl_add('port_group', 'from-lport', 0, + 'output == "fake_port" && ip', 'drop') + + def test_pg_acl_del_all(self): + r1 = self._acl_add('port_group', 'from-lport', 0, + 'output == "fake_port"', 'drop') + self.api.pg_acl_del(self.port_group.uuid).execute(check_error=True) + self.assertNotIn(r1.uuid, self.api.tables['ACL'].rows) + self.assertEqual([], self.port_group.acls) + + def test_pg_acl_list(self): + r1 = self._acl_add('port_group', 'from-lport', 0, + 'output == "fake_port"', 'drop') + r2 = self._acl_add('port_group', 'from-lport', 1, + 'output == "fake_port2"', 'allow') + acls = self.api.pg_acl_list(self.port_group.uuid).execute( + check_error=True) + self.assertIn(r1, acls) + self.assertIn(r2, acls) + class TestQoSOps(OvnNorthboundTest): def setUp(self): @@ -1325,25 +1363,13 @@ class TestPortGroup(OvnNorthboundTest): ('name', '=', self.pg_name)).execute(check_error=True) self.assertEqual([], row) - def test_port_group_ports_and_acls(self): + def test_port_group_ports(self): lsp_add_cmd = self.api.lsp_add(self.switch.uuid, 'testport') - acl_add_cmd_1 = self.api.acl_add( - self.switch.uuid, 'from-lport', 0, 'output == "fake_port"', - 'drop') - acl_add_cmd_2 = self.api.acl_add( - self.switch.uuid, 'from-lport', 0, 'output == "fake_port" && ip', - 'drop') with self.api.transaction(check_error=True) as txn: txn.add(lsp_add_cmd) - txn.add(acl_add_cmd_1) - txn.add(acl_add_cmd_2) txn.add(self.api.pg_add(self.pg_name)) - txn.add(self.api.pg_add_acls( - self.pg_name, [acl_add_cmd_1, acl_add_cmd_2])) port_uuid = lsp_add_cmd.result.uuid - acl_uuid_1 = acl_add_cmd_1.result.uuid - acl_uuid_2 = acl_add_cmd_2.result.uuid # Lets add the port using the UUID instead of a `Command` to # exercise the API @@ -1354,37 +1380,29 @@ class TestPortGroup(OvnNorthboundTest): ('name', '=', self.pg_name)).execute(check_error=True) self.assertIsNotNone(row) self.assertEqual(self.pg_name, row[0]['name']) - # Assert the port and ACLs were added from the Port Group + # Assert the port was added from the Port Group self.assertEqual([port_uuid], row[0]['ports']) - self.assertEqual(sorted([acl_uuid_1, acl_uuid_2]), - sorted(row[0]['acls'])) - # Delete an ACL and the Port from the Port Group + # Delete the Port from the Port Group with self.api.transaction(check_error=True) as txn: txn.add(self.api.pg_del_ports(self.pg_name, port_uuid)) - txn.add(self.api.pg_del_acls(self.pg_name, acl_uuid_1)) row = self.api.db_find( 'Port_Group', ('name', '=', self.pg_name)).execute(check_error=True) self.assertIsNotNone(row) self.assertEqual(self.pg_name, row[0]['name']) - # Assert the port and ACL were removed from the Port Group + # Assert the port was removed from the Port Group self.assertEqual([], row[0]['ports']) - self.assertEqual([acl_uuid_2], row[0]['acls']) - def test_pg_del_ports_and_acls_if_exists(self): + def test_pg_del_ports_if_exists(self): self.api.pg_add(self.pg_name).execute(check_error=True) non_existent_res = uuidutils.generate_uuid() # Assert that if if_exists is False (default) it will raise an error self.assertRaises(RuntimeError, self.api.pg_del_ports(self.pg_name, non_existent_res).execute, True) - self.assertRaises(RuntimeError, self.api.pg_del_acls(self.pg_name, - non_existent_res).execute, True) # Assert that if if_exists is True it won't raise an error self.api.pg_del_ports(self.pg_name, non_existent_res, if_exists=True).execute(check_error=True) - self.api.pg_del_acls(self.pg_name, non_existent_res, - if_exists=True).execute(check_error=True)