From 9cbbd8de53db18a5388d109174a464dcd96ab78b Mon Sep 17 00:00:00 2001 From: Jakub Libosvar Date: Fri, 5 Jun 2020 10:26:21 +0000 Subject: [PATCH] ovn: Remove is_port_groups_supported() code Port_Groups table has been introduced in OVN 2.10 and we've moved in master to newer version since. This patch removes all references to code branching between port_groups and address_sets, and also removes unneeded Address_Set commands and references. Change-Id: I592d31db9be76d9be202d79d942e15b1668e3c0e --- doc/source/admin/ovn/refarch/refarch.rst | 81 +++-- .../internals/ovn/acl_optimizations.rst | 186 ---------- .../contributor/internals/ovn/index.rst | 1 - neutron/common/ovn/acl.py | 178 +--------- .../ml2/drivers/ovn/mech_driver/ovsdb/api.py | 54 --- .../drivers/ovn/mech_driver/ovsdb/commands.py | 69 ---- .../ovn/mech_driver/ovsdb/impl_idl_ovn.py | 23 -- .../ovn/mech_driver/ovsdb/maintenance.py | 9 +- .../ovn/mech_driver/ovsdb/ovn_client.py | 210 ++---------- .../ovn/mech_driver/ovsdb/ovn_db_sync.py | 211 +----------- .../ovn/mech_driver/ovsdb/test_maintenance.py | 12 +- .../ovsdb/test_ovn_db_resources.py | 96 +----- .../ovn/mech_driver/ovsdb/test_ovn_db_sync.py | 145 ++------ neutron/tests/unit/common/ovn/test_acl.py | 106 +----- neutron/tests/unit/fake_resources.py | 10 - .../ovn/mech_driver/ovsdb/test_commands.py | 165 --------- .../ovn/mech_driver/ovsdb/test_maintenance.py | 26 +- .../ovn/mech_driver/ovsdb/test_ovn_db_sync.py | 131 +------- .../ovn/mech_driver/test_mech_driver.py | 317 +++++------------- 19 files changed, 255 insertions(+), 1775 deletions(-) delete mode 100644 doc/source/contributor/internals/ovn/acl_optimizations.rst diff --git a/doc/source/admin/ovn/refarch/refarch.rst b/doc/source/admin/ovn/refarch/refarch.rst index fcf1fbb7fa7..c5156383a6c 100644 --- a/doc/source/admin/ovn/refarch/refarch.rst +++ b/doc/source/admin/ovn/refarch/refarch.rst @@ -218,10 +218,23 @@ for the compute node. Security Groups/Rules --------------------- -Each security group will map to 2 Address_Sets in the OVN NB and SB -tables, one for ipv4 and another for ipv6, which will be used to hold ip -addresses for the ports that belong to the security group, so that rules -with remote_group_id can be efficiently applied. +When a Neutron Security Group is created, the equivalent Port Group in OVN +(pg- is created). This Port Group references Neutron SG id +in its external_ids column. + +When a Neutron Port is created, the equivalent Logical Port in OVN is added to +those Port Groups associated to the Neutron Security Groups this port belongs +to. + +When a Neutron Port is deleted, the associated Logical Port in OVN is deleted. +Since the schema includes a weak reference to the port, when the LSP gets +deleted, it is automatically deleted from any Port Group entry where it was +previously present. + +Every time a security group rule is created, instead of figuring out the ports +affected by its SG and inserting an ACL row which will be referenced by +different Logical Switches, we just reference it from the associated +Port Group. .. todo: add block with openstack security group rule example @@ -229,36 +242,52 @@ OVN operations ~~~~~~~~~~~~~~ #. Creating a security group will cause the OVN mechanism driver to create - 2 new entries in the Address Set table of the northbound DB: + a port group in the Port_Group table of the northbound DB: .. code-block:: console - _uuid : 9a9d01bd-4afc-4d12-853a-cd21b547911d - addresses : [] - external_ids : {"neutron:security_group_name"=default} - name : "as_ip4_90a78a43_b549_4bee_8822_21fcccab58dc" + _uuid : e96c5994-695d-4b9c-a17b-c7375ad281e2 + acls : [33c3c2d0-bc7b-421b-ace9-10884851521a, c22170ec-da5d-4a59-b118-f7f0e370ebc4] + external_ids : {"neutron:security_group_id"="ccbeffee-7b98-4b6f-adf7-d42027ca6447"} + name : pg_ccbeffee_7b98_4b6f_adf7_d42027ca6447 + ports : [] - _uuid : 27a91327-636e-4125-99f0-6f2937a3b6d8 - addresses : [] - external_ids : {"neutron:security_group_name"=default} - name : "as_ip6_90a78a43_b549_4bee_8822_21fcccab58dc" - - In the above entries, the address set name include the protocol (IPv4 - or IPv6, written as ip4 or ip6) and the UUID of the Openstack security - group, dashes translated to underscores. - -#. In turn, these new entries will be translated by the OVN northd daemon - into entries in the southbound DB: + And it also creates the default ACLs for egress traffic in the ACL table of + the northbound DB: .. code-block:: console - _uuid : 886d7b3a-e460-470f-8af2-7c7d88ce45d2 - addresses : [] - name : "as_ip4_90a78a43_b549_4bee_8822_21fcccab58dc" + _uuid : 33c3c2d0-bc7b-421b-ace9-10884851521a + action : allow-related + direction : from-lport + external_ids : {"neutron:security_group_rule_id"="655b0d7e-144e-4bd8-9243-10a261b91041"} + log : false + match : "inport == @pg_ccbeffee_7b98_4b6f_adf7_d42027ca6447 && ip4" + meter : [] + name : [] + priority : 1002 + severity : [] - _uuid : 355ddcba-941d-4f1c-b823-dc811cec59ca - addresses : [] - name : "as_ip6_90a78a43_b549_4bee_8822_21fcccab58dc" + _uuid : c22170ec-da5d-4a59-b118-f7f0e370ebc4 + action : allow-related + direction : from-lport + external_ids : {"neutron:security_group_rule_id"="a303a34f-5f19-494f-a9e2-e23f246bfcad"} + log : false + match : "inport == @pg_ccbeffee_7b98_4b6f_adf7_d42027ca6447 && ip6" + meter : [] + name : [] + priority : 1002 + severity : [] + +Ports with no security groups +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +When a port doesn't belong to any Security Group and port security is enabled, +we, by default, drop all the traffic to/from that port. In order to implement +this through Port Groups, we'll create a special Port Group with a fixed name +(``neutron_pg_drop``) which holds the ACLs to drop all the traffic. + +This PG is created automatically once before neutron-server forks into workers. Networks -------- diff --git a/doc/source/contributor/internals/ovn/acl_optimizations.rst b/doc/source/contributor/internals/ovn/acl_optimizations.rst deleted file mode 100644 index b70ec64b6b4..00000000000 --- a/doc/source/contributor/internals/ovn/acl_optimizations.rst +++ /dev/null @@ -1,186 +0,0 @@ -.. _acl_optimizations: - -======================================== -ACL Handling optimizations in ovn driver -======================================== - -This document presents the current problem with ACLs and the design changes -proposed to core OVN as well as the necessary modifications to be made to -ovn driver to improve their usage. - -Problem description -=================== - -There is basically two problems being addressed in this spec: - -1. While in Neutron, a ``Security Group Rule`` is tied to a -``Security Group``, in OVN ``ACLs`` are created per port. Therefore, -we'll typically have *many* more ACLs than Security Group Rules, resulting -in a performance hit as the number of ports grows. - -2. An ACL in OVN is applied to a ``Logical Switch``. As a result, -``ovn driver`` has to figure out which Logical Switches to apply the -generated ACLs per each Security Rule. - -Let's highlight both problems with an example: - -- Neutron Networks: NA, NB, NC -- Neutron Security Group: SG1 -- Number of Neutron Security Group Rules in SG1: 10 -- Neutron Ports in NA: 100 -- Neutron Ports in NB: 100 -- Neutron Ports in NC: 100 -- All ports belong to SG1 - -When we implement the above scenario in OVN, this is what we'll get: - -- OVN Logical Switches: NA, NB, NC -- Number of ACL rows in Northbound DB ACL table: 3000 (10 rules * 100 ports * - 3 networks) -- Number of elements in acl column on each Logical_Switch row: 1000 (10 rules - * 100 ports). - -And this is how, for example, the ACL match fields for the default Neutron -Security Group would look like:: - - outport == && ip4 && ip4.src == $as_ip4_ - outport == && ip4 && ip4.src == $as_ip4_ - outport == && ip4 && ip4.src == $as_ip4_ - ... - outport == && ip4 && ip4.src == $as_ip4_ - -As you can see, all of them look the same except for the outport field which -is clearly redundant and makes the NB database grow a lot at scale. -Also, ``ovn driver`` had to figure out for each rule in SG1 which Logical -Switches it had to apply the ACLs on (NA, NB and NC). This can be really costly -when the number of networks and port grows. - - -Proposed optimization -===================== - -In the OpenStack context, we'll be facing this scenario most of the time -where the majority of the ACLs will look the same except for the -outport/inport fields in the match column. It would make sense to be able to -substitute all those ACLs by a single one which references all the ports -affected by that SG rule:: - - outport == @port_group1 && ip4 && ip4.src == $port_group1_ip4 - - -Implementation Details -====================== - -Core OVN --------- - -There's a series of patches in Core OVN that will enable us to achieve this -optimization: - -https://github.com/openvswitch/ovs/commit/3d2848bafa93a2b483a4504c5de801454671dccf -https://github.com/openvswitch/ovs/commit/1beb60afd25a64f1779903b22b37ed3d9956d47c -https://github.com/openvswitch/ovs/commit/689829d53612a573f810271a01561f7b0948c8c8 - - -In summary, these patches are: - -- Adding a new entity called Port_Group which will hold a list of weak - references to the Logical Switch ports that belong to it. -- Automatically creating/updating two Address Sets (_ip4 and _ip6) in - Southbound database every time a new port is added to the group. -- Support adding a list of ACLs to a Port Group. As the SG rules may - span across different Logical Switches, we used to insert the ACLs in - all the Logical Switches where we have ports in within a SG. Figuring this - out is expensive and this new feature is a huge gain in terms of - performance when creating/deleting ports. - - -ovn driver ----------- - -In the OpenStack integration driver, the following changes are required to -accomplish this optimization: - -- When a Neutron Security Group is created, create the equivalent Port Group - in OVN (pg-), instead of creating a pair of Adress Sets - for IPv4 and IPv6. This Port Group will reference Neutron SG id in its - ``external_ids`` column. - -- When a Neutron Port is created, the equivalent Logical Port in OVN will be - added to those Port Groups associated to the Neutron Security Groups this - port belongs to. - -- When a Neutron Port is deleted, we'll delete the associated Logical Port in - OVN. Since the schema includes a weak reference to the port, when the LSP - gets deleted, it will also be automatically deleted from any Port Group - entry where it was previously present. - -- Instead of handling SG rules per port, we now need to handle them per SG - referencing the associated Port Group in the outport/inport fields. This - will be the biggest gain in terms of processing since we don't need to - iterate through all the ports anymore. For example: - -.. code-block:: python - - -def acl_direction(r, port): - +def acl_direction(r): - if r['direction'] == 'ingress': - portdir = 'outport' - else: - portdir = 'inport' - - return '%s == "%s"' % (portdir, port['id']) - + return '%s == "@%s"' % (portdir, utils.ovn_name(r['security_group_id']) - -- Every time a SG rule is created, instead of figuring out the ports affected - by its SG and inserting an ACL row which will be referrenced by different - Logical Switches, we will just reference it from the associated Port Group. - -- For Neutron remote security groups, we just need to reference the - automatically created Address_Set for that Port Group. - -As a bonus, we are tackling the race conditions that could happen in -Address_Sets right now when we're deleting and creating a port at the same -time. This is thanks to the fact that the Address_Sets in the SB table are -generated automatically by ovn-northd from the Port_Group contents and -Port Group is referencing actual Logical Switch Ports. More info at: -https://bugs.launchpad.net/networking-ovn/+bug/1611852 - - -Backwards compatibility considerations --------------------------------------- - -- If the schema doesn't include the ``Port_Group`` table, keep the old - behavior(Address Sets) for backwards compatibility. - -- If the schema supports Port Groups, then a migration task will be performed - from an OvnWorker. This way we'll ensure that it'll happen only once across - the cloud thanks the OVSDB lock. This will be done right at the beginning of - the ovn_db_sync process to make sure that when neutron-server starts, - everything is in place to work with Port Groups. This migration process will - perform the following steps: - - * Create the default drop Port Group and add all ports with port - security enabled to it. - * Create a Port Group for every existing Neutron Security Group and - add all its Security Group Rules as ACLs to that Port Group. - * Delete all existing Address Sets in NorthBound database which correspond to - a Neutron Security Group. - * Delete all the ACLs in every Logical Switch (Neutron network). - -We should eventually remove the backwards compatibility and migration path. At -that point we should require OVS >= 2.10 from neutron ovn driver. - -Special cases -------------- - -Ports with no security groups -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - -When a port doesn't belong to any Security Group and port security is enabled, -we, by default, drop all the traffic to/from that port. In order to implement -this through Port Groups, we'll create a special Port Group with a fixed name -(``neutron_pg_drop``) which holds the ACLs to drop all the traffic. - -This PG will be created automatically when we first need it, avoiding the need -to create it beforehand or during deployment. - diff --git a/doc/source/contributor/internals/ovn/index.rst b/doc/source/contributor/internals/ovn/index.rst index 066723fdf68..5a666a4027f 100644 --- a/doc/source/contributor/internals/ovn/index.rst +++ b/doc/source/contributor/internals/ovn/index.rst @@ -12,7 +12,6 @@ OVN Design Notes ovn_worker metadata_api database_consistency - acl_optimizations loadbalancer distributed_ovsdb_events l3_ha_rescheduling diff --git a/neutron/common/ovn/acl.py b/neutron/common/ovn/acl.py index a3a7457d39e..a0a3e47fac1 100644 --- a/neutron/common/ovn/acl.py +++ b/neutron/common/ovn/acl.py @@ -12,8 +12,6 @@ # under the License. # -import netaddr - from neutron_lib import constants as const from neutron_lib import exceptions as n_exceptions from oslo_config import cfg @@ -274,44 +272,17 @@ def _get_sg_from_cache(plugin, admin_context, sg_cache, sg_id): return sg -def acl_remote_group_id(r, ip_version, ovn=None): +def acl_remote_group_id(r, ip_version): if not r['remote_group_id']: return '' src_or_dst = 'src' if r['direction'] == const.INGRESS_DIRECTION else 'dst' - if (ovn and ovn.is_port_groups_supported()): - addrset_name = utils.ovn_pg_addrset_name(r['remote_group_id'], - ip_version) - else: - addrset_name = utils.ovn_addrset_name(r['remote_group_id'], - ip_version) + addrset_name = utils.ovn_pg_addrset_name(r['remote_group_id'], + ip_version) return ' && %s.%s == $%s' % (ip_version, src_or_dst, addrset_name) -def _add_sg_rule_acl_for_port(port, r): - # Update the match based on which direction this rule is for (ingress - # or egress). - match = acl_direction(r, port) - - # Update the match for IPv4 vs IPv6. - ip_match, ip_version, icmp = acl_ethertype(r) - match += ip_match - - # Update the match if an IPv4 or IPv6 prefix was specified. - match += acl_remote_ip_prefix(r, ip_version) - - # Update the match if remote group id was specified. - match += acl_remote_group_id(r, ip_version) - - # Update the match for the protocol (tcp, udp, icmp) and port/type - # range if specified. - match += acl_protocol_and_ports(r, icmp) - - # Finally, create the ACL entry for the direction specified. - return add_sg_rule_acl_for_port(port, r, match) - - -def _add_sg_rule_acl_for_port_group(port_group, r, ovn): +def _add_sg_rule_acl_for_port_group(port_group, r): # Update the match based on which direction this rule is for (ingress # or egress). match = acl_direction(r, port_group=port_group) @@ -324,7 +295,7 @@ def _add_sg_rule_acl_for_port_group(port_group, r, ovn): match += acl_remote_ip_prefix(r, ip_version) # Update the match if remote group id was specified. - match += acl_remote_group_id(r, ip_version, ovn) + match += acl_remote_group_id(r, ip_version) # Update the match for the protocol (tcp, udp, icmp) and port/type # range if specified. @@ -342,7 +313,7 @@ def _acl_columns_name_severity_supported(nb_idl): def add_acls_for_sg_port_group(ovn, security_group, txn): for r in security_group['security_group_rules']: acl = _add_sg_rule_acl_for_port_group( - utils.ovn_port_group_name(security_group['id']), r, ovn) + utils.ovn_port_group_name(security_group['id']), r) txn.add(ovn.pg_acl_add(**acl)) @@ -351,7 +322,6 @@ def update_acls_for_security_group(plugin, ovn, security_group_id, security_group_rule, - sg_ports_cache=None, is_add_acl=True): # Skip ACLs if security groups aren't enabled @@ -361,135 +331,19 @@ def update_acls_for_security_group(plugin, # Check if ACL log name and severity supported or not keep_name_severity = _acl_columns_name_severity_supported(ovn) - # If we're using a Port Group for this SG, just update it. - # Otherwise, keep the old behavior. - if (ovn.is_port_groups_supported() and - not ovn.get_address_set(security_group_id)): - acl = _add_sg_rule_acl_for_port_group( - utils.ovn_port_group_name(security_group_id), - security_group_rule, ovn) - # Remove ACL log name and severity if not supported - if is_add_acl: - if not keep_name_severity: - acl.pop('name') - acl.pop('severity') - ovn.pg_acl_add(**acl).execute(check_error=True) - else: - ovn.pg_acl_del(acl['port_group'], acl['direction'], - acl['priority'], acl['match']).execute( - check_error=True) - return - - # Get the security group ports. - sg_ports_cache = sg_ports_cache or {} - sg_ports = _get_sg_ports_from_cache(plugin, - admin_context, - sg_ports_cache, - security_group_id) - - # ACLs associated with a security group may span logical switches - sg_port_ids = [binding['port_id'] for binding in sg_ports] - sg_port_ids = list(set(sg_port_ids)) - port_list = plugin.get_ports(admin_context, - filters={'id': sg_port_ids}) - if not port_list: - return - - acl_new_values_dict = {} - update_port_list = [] - - # NOTE(lizk): We can directly locate the affected acl records, - # so no need to compare new acl values with existing acl objects. - for port in port_list: - # Skip trusted port - if utils.is_lsp_trusted(port): - continue - - update_port_list.append(port) - acl = _add_sg_rule_acl_for_port(port, security_group_rule) - # Remove lport and lswitch since we don't need them - acl.pop('lport') - acl.pop('lswitch') - # Remove ACL log name and severity if not supported, + acl = _add_sg_rule_acl_for_port_group( + utils.ovn_port_group_name(security_group_id), + security_group_rule) + # Remove ACL log name and severity if not supported + if is_add_acl: if not keep_name_severity: acl.pop('name') acl.pop('severity') - acl_new_values_dict[port['id']] = acl - - if not update_port_list: - return - lswitch_names = {p['network_id'] for p in update_port_list} - - ovn.update_acls(list(lswitch_names), - iter(update_port_list), - acl_new_values_dict, - need_compare=False, - is_add_acl=is_add_acl).execute(check_error=True) - - -def add_acls(plugin, admin_context, port, sg_cache, subnet_cache, ovn): - acl_list = [] - - # Skip ACLs if security groups aren't enabled - if not is_sg_enabled(): - return acl_list - - sec_groups = utils.get_lsp_security_groups(port) - if not sec_groups: - # If it is a trusted port or port security is disabled, allow all - # traffic. So don't add any ACLs. - if utils.is_lsp_trusted(port) or not utils.is_port_security_enabled( - port): - return acl_list - - # if port security is enabled, drop all traffic. - return drop_all_ip_traffic_for_port(port) - - # Drop all IP traffic to and from the logical port by default. - acl_list += drop_all_ip_traffic_for_port(port) - - # Add DHCP ACLs. - port_subnet_ids = set() - for ip in port['fixed_ips']: - if netaddr.IPNetwork(ip['ip_address']).version != 4: - continue - subnet = _get_subnet_from_cache(plugin, - admin_context, - subnet_cache, - ip['subnet_id']) - # Ignore duplicate DHCP ACLs for the subnet. - if subnet['id'] not in port_subnet_ids: - acl_list += add_acl_dhcp(port, subnet, True) - port_subnet_ids.add(subnet['id']) - - # We create an ACL entry for each rule on each security group applied - # to this port. - for sg_id in sec_groups: - sg = _get_sg_from_cache(plugin, - admin_context, - sg_cache, - sg_id) - for r in sg['security_group_rules']: - acl = _add_sg_rule_acl_for_port(port, r) - acl_list.append(acl) - - # Remove ACL log name and severity if not supported, - if not _acl_columns_name_severity_supported(ovn): - for acl in acl_list: - acl.pop('name') - acl.pop('severity') - - return acl_list - - -def acl_port_ips(port): - # Skip ACLs if security groups aren't enabled - if not is_sg_enabled(): - return {'ip4': [], 'ip6': []} - - ip_list = [x['ip_address'] for x in port.get('fixed_ips', [])] - ip_list.extend(utils.get_allowed_address_pairs_ip_addresses(port)) - return utils.sort_ips_by_version(ip_list) + ovn.pg_acl_add(**acl).execute(check_error=True) + else: + ovn.pg_acl_del(acl['port_group'], acl['direction'], + acl['priority'], acl['match']).execute( + check_error=True) def filter_acl_dict(acl, extra_fields=None): diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/api.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/api.py index bcf3363b7b4..6476d89afa2 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/api.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/api.py @@ -255,20 +255,6 @@ class API(api.API, metaclass=abc.ABCMeta): :returns: :class:`Command` with no result """ - @abc.abstractmethod - def create_address_set(self, name, may_exist=True, **columns): - """Create an address set - - :param name: The name of the address set - :type name: string - :param may_exist: Do not fail if address set already exists - :type may_exist: bool - :param columns: Dictionary of address set columns - Supported columns: external_ids, addresses - :type columns: dictionary - :returns: :class:`Command` with no result - """ - @abc.abstractmethod def delete_address_set(self, name, if_exists=True): """Delete an address set @@ -280,35 +266,6 @@ class API(api.API, metaclass=abc.ABCMeta): :returns: :class:`Command` with no result """ - @abc.abstractmethod - def update_address_set(self, name, addrs_add, addrs_remove, - if_exists=True): - """Updates addresses in an address set - - :param name: The name of the address set - :type name: string - :param addrs_add: The addresses to be added - :type addrs_add: [] - :param addrs_remove: The addresses to be removed - :type addrs_remove: [] - :param if_exists: Do not fail if the address set does not exist - :type if_exists: bool - :returns: :class:`Command` with no result - """ - - @abc.abstractmethod - def update_address_set_ext_ids(self, name, external_ids, if_exists=True): - """Update external IDs for an address set - - :param name: The name of the address set - :type name: string - :param external_ids: The external IDs for the address set - :type external_ids: dict - :param if_exists: Do not fail if the address set does not exist - :type if_exists: bool - :returns: :class:`Command` with no result - """ - @abc.abstractmethod def get_all_chassis_gateway_bindings(self, chassis_candidate_list=None): @@ -577,17 +534,6 @@ class API(api.API, metaclass=abc.ABCMeta): :returns: :class:`Command` with no result """ - @abc.abstractmethod - def get_address_set(self, addrset_id, ip_version='ip4'): - """Get a Address Set by its ID. - - :param addrset_id: The Address Set ID - :type addrset_id: string - :param ip_version: Either "ip4" or "ip6". Defaults to "ip4" - :type addr_name: string - :returns: The Address Set row or None - """ - @abc.abstractmethod def set_lswitch_port_to_virtual_type(self, lport_name, vip, virtual_parent, if_exists=True): diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py index 22c4d34993f..d0299d310e5 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py @@ -675,25 +675,6 @@ class DelStaticRouteCommand(command.BaseCommand): break -class AddAddrSetCommand(command.BaseCommand): - def __init__(self, api, name, may_exist, **columns): - super(AddAddrSetCommand, self).__init__(api) - self.name = name - self.columns = columns - self.may_exist = may_exist - - def run_idl(self, txn): - if self.may_exist: - addrset = idlutils.row_by_value(self.api.idl, 'Address_Set', - 'name', self.name, None) - if addrset: - return - row = txn.insert(self.api._tables['Address_Set']) - row.name = self.name - for col, val in self.columns.items(): - setattr(row, col, val) - - class DelAddrSetCommand(command.BaseCommand): def __init__(self, api, name, if_exists): super(DelAddrSetCommand, self).__init__(api) @@ -714,56 +695,6 @@ class DelAddrSetCommand(command.BaseCommand): self.api._tables['Address_Set'].rows[addrset.uuid].delete() -class UpdateAddrSetCommand(command.BaseCommand): - def __init__(self, api, name, addrs_add, addrs_remove, if_exists): - super(UpdateAddrSetCommand, self).__init__(api) - self.name = name - self.addrs_add = addrs_add - self.addrs_remove = addrs_remove - self.if_exists = if_exists - - def run_idl(self, txn): - try: - addrset = idlutils.row_by_value(self.api.idl, 'Address_Set', - 'name', self.name) - except idlutils.RowNotFound: - if self.if_exists: - return - msg = _("Address set %s does not exist. " - "Can't update addresses") % self.name - raise RuntimeError(msg) - - _updatevalues_in_list( - addrset, 'addresses', - new_values=self.addrs_add, - old_values=self.addrs_remove) - - -class UpdateAddrSetExtIdsCommand(command.BaseCommand): - def __init__(self, api, name, external_ids, if_exists): - super(UpdateAddrSetExtIdsCommand, self).__init__(api) - self.name = name - self.external_ids = external_ids - self.if_exists = if_exists - - def run_idl(self, txn): - try: - addrset = idlutils.row_by_value(self.api.idl, 'Address_Set', - 'name', self.name) - except idlutils.RowNotFound: - if self.if_exists: - return - msg = _("Address set %s does not exist. " - "Can't update external IDs") % self.name - raise RuntimeError(msg) - - addrset.verify('external_ids') - addrset_external_ids = getattr(addrset, 'external_ids', {}) - for ext_id_key, ext_id_value in self.external_ids.items(): - addrset_external_ids[ext_id_key] = ext_id_value - addrset.external_ids = addrset_external_ids - - class UpdateChassisExtIdsCommand(command.BaseCommand): def __init__(self, api, name, external_ids, if_exists): super(UpdateChassisExtIdsCommand, self).__init__(api) diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py index a0dd2fe489a..325ff0c0fce 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py @@ -373,21 +373,9 @@ class OvsdbNbOvnIdl(nb_impl_idl.OvnNbApiIdlImpl, Backend): return cmd.DelStaticRouteCommand(self, lrouter, ip_prefix, nexthop, if_exists) - def create_address_set(self, name, may_exist=True, **columns): - return cmd.AddAddrSetCommand(self, name, may_exist, **columns) - def delete_address_set(self, name, if_exists=True, **columns): return cmd.DelAddrSetCommand(self, name, if_exists) - def update_address_set(self, name, addrs_add, addrs_remove, - if_exists=True): - return cmd.UpdateAddrSetCommand(self, name, addrs_add, addrs_remove, - if_exists) - - def update_address_set_ext_ids(self, name, external_ids, if_exists=True): - return cmd.UpdateAddrSetExtIdsCommand(self, name, external_ids, - if_exists) - def _get_logical_router_port_gateway_chassis(self, lrp): """Get the list of chassis hosting this gateway port. @@ -657,14 +645,6 @@ class OvsdbNbOvnIdl(nb_impl_idl.OvnNbApiIdlImpl, Backend): nat['external_ip'] == external_ip): return nat - def get_address_set(self, addrset_id, ip_version='ip4'): - addr_name = utils.ovn_addrset_name(addrset_id, ip_version) - try: - return idlutils.row_by_value(self.idl, 'Address_Set', - 'name', addr_name) - except idlutils.RowNotFound: - return None - def check_revision_number(self, name, resource, resource_type, if_exists=True): return cmd.CheckRevisionNumberCommand( @@ -691,9 +671,6 @@ class OvsdbNbOvnIdl(nb_impl_idl.OvnNbApiIdlImpl, Backend): def delete_lrouter_ext_gw(self, lrouter_name, if_exists=True): return cmd.DeleteLRouterExtGwCommand(self, lrouter_name, if_exists) - def is_port_groups_supported(self): - return self.is_table_present('Port_Group') - def get_port_group(self, pg_name): if uuidutils.is_uuid_like(pg_name): pg_name = utils.ovn_port_group_name(pg_name) diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py index bb6d65efb2d..e613712c294 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py @@ -168,7 +168,7 @@ class DBInconsistenciesPeriodics(SchemaAwarePeriodicsBase): }, ovn_const.TYPE_SECURITY_GROUPS: { 'neutron_get': self._ovn_client._plugin.get_security_group, - 'ovn_get': self._get_security_group, + 'ovn_get': self._nb_idl.get_port_group, 'ovn_create': self._ovn_client.create_security_group, 'ovn_delete': self._ovn_client.delete_security_group, }, @@ -189,10 +189,6 @@ class DBInconsistenciesPeriodics(SchemaAwarePeriodicsBase): }, } - def _get_security_group(self, uuid): - return (self._nb_idl.get_address_set(uuid) or - self._nb_idl.get_port_group(uuid)) - @property def has_lock(self): return not self._idl.is_lock_contended @@ -287,8 +283,7 @@ class DBInconsistenciesPeriodics(SchemaAwarePeriodicsBase): # If Port Groups are not supported or we've already migrated, we don't # need to attempt to migrate again. - if (not self._nb_idl.is_port_groups_supported() or - not self._nb_idl.get_address_sets()): + if not self._nb_idl.get_address_sets(): raise periodics.NeverAgain() # Only the worker holding a valid lock within OVSDB will perform the diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py index f94ba8208a7..4e92a64567e 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py @@ -322,8 +322,6 @@ class OVNClient(object): utils.get_revision_number( port, ovn_const.TYPE_PORTS))} lswitch_name = utils.ovn_name(port['network_id']) - sg_cache = {} - subnet_cache = {} # It's possible to have a network created on one controller and then a # port created on a different controller quickly enough that the second @@ -392,45 +390,16 @@ class OVNClient(object): port_cmd = txn.add(self._nb_idl.create_lswitch_port( **kwargs)) - # Handle ACL's for this port. If we're not using Port Groups - # because either the schema doesn't support it or we didn't - # migrate old SGs from Address Sets to Port Groups, then we - # keep the old behavior. For those SGs this port belongs to - # that are modelled as a Port Group, we'll use it. sg_ids = utils.get_lsp_security_groups(port) - if self._nb_idl.is_port_groups_supported(): - # If this is not a trusted port or port security is enabled, - # add it to the default drop Port Group so that all traffic - # is dropped by default. - if not utils.is_lsp_trusted(port) or port_info.port_security: - self._add_port_to_drop_port_group(port_cmd, txn) - # For SGs modelled as OVN Port Groups, just add the port to - # its Port Group. - for sg in sg_ids: - txn.add(self._nb_idl.pg_add_ports( - utils.ovn_port_group_name(sg), port_cmd)) - else: - # SGs modelled as Address Sets: - acls_new = ovn_acl.add_acls(self._plugin, context, - port, sg_cache, subnet_cache, - self._nb_idl) - for acl in acls_new: - txn.add(self._nb_idl.add_acl(**acl)) - - if port.get('fixed_ips') and sg_ids: - addresses = ovn_acl.acl_port_ips(port) - # NOTE(rtheis): Fail port creation if the address set - # doesn't exist. This prevents ports from being created on - # any security groups out-of-sync between neutron and OVN. - for sg_id in sg_ids: - for ip_version in addresses: - if addresses[ip_version]: - txn.add(self._nb_idl.update_address_set( - name=utils.ovn_addrset_name(sg_id, - ip_version), - addrs_add=addresses[ip_version], - addrs_remove=None, - if_exists=False)) + # If this is not a trusted port or port security is enabled, + # add it to the default drop Port Group so that all traffic + # is dropped by default. + if not utils.is_lsp_trusted(port) or port_info.port_security: + self._add_port_to_drop_port_group(port_cmd, txn) + # Just add the port to its Port Group. + for sg in sg_ids: + txn.add(self._nb_idl.pg_add_ports( + utils.ovn_port_group_name(sg), port_cmd)) if self.is_dns_required_for_port(port): self.add_txns_to_sync_port_dns_records(txn, port) @@ -481,8 +450,6 @@ class OVNClient(object): ovn_const.OVN_REV_NUM_EXT_ID_KEY: str( utils.get_revision_number( port, ovn_const.TYPE_PORTS))} - sg_cache = {} - subnet_cache = {} check_rev_cmd = self._nb_idl.check_revision_number( port['id'], port, ovn_const.TYPE_PORTS) @@ -560,96 +527,22 @@ class OVNClient(object): detached_sg_ids = old_sg_ids - new_sg_ids attached_sg_ids = new_sg_ids - old_sg_ids - if self._nb_idl.is_port_groups_supported(): - for sg in detached_sg_ids: - txn.add(self._nb_idl.pg_del_ports( - utils.ovn_port_group_name(sg), port['id'])) - for sg in attached_sg_ids: - txn.add(self._nb_idl.pg_add_ports( - utils.ovn_port_group_name(sg), port['id'])) - if (not utils.is_lsp_trusted(port) and - utils.is_port_security_enabled(port)): - self._add_port_to_drop_port_group(port['id'], txn) - # If the port doesn't belong to any security group and - # port_security is disabled, or it's a trusted port, then - # allow all traffic. - elif ((not new_sg_ids and - not utils.is_port_security_enabled(port)) or - utils.is_lsp_trusted(port)): - self._del_port_from_drop_port_group(port['id'], txn) - else: - - old_fixed_ips = utils.remove_macs_from_lsp_addresses( - ovn_port.addresses) - new_fixed_ips = [x['ip_address'] for x in - port.get('fixed_ips', [])] - is_fixed_ips_updated = ( - sorted(old_fixed_ips) != sorted(new_fixed_ips)) - port_security_changed = ( - utils.is_port_security_enabled(port) != - bool(ovn_port.port_security)) - # Refresh ACLs for changed security groups or fixed IPs. - if (detached_sg_ids or attached_sg_ids or - is_fixed_ips_updated or port_security_changed): - # Note that update_acls will compare the port's ACLs to - # ensure only the necessary ACLs are added and deleted - # on the transaction. - acls_new = ovn_acl.add_acls(self._plugin, - context, - port, - sg_cache, - subnet_cache, - self._nb_idl) - txn.add(self._nb_idl.update_acls([port['network_id']], - [port], - {port['id']: acls_new}, - need_compare=True)) - - # Refresh address sets for changed security groups or fixed - # IPs. - if len(old_fixed_ips) != 0 or len(new_fixed_ips) != 0: - addresses = ovn_acl.acl_port_ips(port) - addresses_old = utils.sort_ips_by_version( - utils.get_ovn_port_addresses(ovn_port)) - # Add current addresses to attached security groups. - for sg_id in attached_sg_ids: - for ip_version in addresses: - if addresses[ip_version]: - txn.add(self._nb_idl.update_address_set( - name=utils.ovn_addrset_name(sg_id, - ip_version), - addrs_add=addresses[ip_version], - addrs_remove=None)) - # Remove old addresses from detached security groups. - for sg_id in detached_sg_ids: - for ip_version in addresses_old: - if addresses_old[ip_version]: - txn.add(self._nb_idl.update_address_set( - name=utils.ovn_addrset_name(sg_id, - ip_version), - addrs_add=None, - addrs_remove=addresses_old[ip_version])) - - if is_fixed_ips_updated or addr_pairs_diff.changed: - # We have refreshed address sets for attached and - # detached security groups, so now we only need to take - # care of unchanged security groups. - unchanged_sg_ids = new_sg_ids & old_sg_ids - for sg_id in unchanged_sg_ids: - for ip_version in addresses: - addr_add = ((set(addresses[ip_version]) - - set(addresses_old[ip_version])) or - None) - addr_remove = ( - (set(addresses_old[ip_version]) - - set(addresses[ip_version])) or None) - - if addr_add or addr_remove: - txn.add(self._nb_idl.update_address_set( - name=utils.ovn_addrset_name( - sg_id, ip_version), - addrs_add=addr_add, - addrs_remove=addr_remove)) + for sg in detached_sg_ids: + txn.add(self._nb_idl.pg_del_ports( + utils.ovn_port_group_name(sg), port['id'])) + for sg in attached_sg_ids: + txn.add(self._nb_idl.pg_add_ports( + utils.ovn_port_group_name(sg), port['id'])) + if (not utils.is_lsp_trusted(port) and + utils.is_port_security_enabled(port)): + self._add_port_to_drop_port_group(port['id'], txn) + # If the port doesn't belong to any security group and + # port_security is disabled, or it's a trusted port, then + # allow all traffic. + elif ((not new_sg_ids and + not utils.is_port_security_enabled(port)) or + utils.is_lsp_trusted(port)): + self._del_port_from_drop_port_group(port['id'], txn) self._qos_driver.update_port(txn, port, port_object) @@ -681,24 +574,6 @@ class OVNClient(object): txn.add(self._nb_idl.delete_lswitch_port( port_id, network_id)) - if not self._nb_idl.is_port_groups_supported(): - txn.add(self._nb_idl.delete_acl( - network_id, port_id, if_exists=True)) - - addresses = utils.sort_ips_by_version( - utils.get_ovn_port_addresses(ovn_port)) - sec_groups = utils.get_ovn_port_security_groups( - ovn_port, skip_trusted_port=False) - for sg_id in sec_groups: - for ip_version, addr_list in addresses.items(): - if not addr_list: - continue - txn.add(self._nb_idl.update_address_set( - name=utils.ovn_addrset_name(sg_id, ip_version), - addrs_add=None, - addrs_remove=addr_list, - if_exists=True)) - p_object = ({'id': port_id, 'network_id': network_id} if not port_object else port_object) self._qos_driver.delete_port(txn, p_object) @@ -2061,26 +1936,15 @@ class OVNClient(object): context, subnet_id, ovn_const.TYPE_SUBNETS) def create_security_group(self, context, security_group): - # If the OVN schema supports Port Groups, we'll model security groups - # as such. Otherwise, for backwards compatibility, we'll keep creating - # two Address Sets for each Neutron SG (one for IPv4 and one for - # IPv6). with self._nb_idl.transaction(check_error=True) as txn: ext_ids = {ovn_const.OVN_SG_EXT_ID_KEY: security_group['id']} - if self._nb_idl.is_port_groups_supported(): - name = utils.ovn_port_group_name(security_group['id']) - txn.add(self._nb_idl.pg_add( - name=name, acls=[], external_ids=ext_ids)) - # When a SG is created, it comes with some default rules, - # so we'll apply them to the Port Group. - ovn_acl.add_acls_for_sg_port_group(self._nb_idl, - security_group, txn) - else: - for ip_version in ('ip4', 'ip6'): - name = utils.ovn_addrset_name(security_group['id'], - ip_version) - txn.add(self._nb_idl.create_address_set( - name=name, external_ids=ext_ids)) + name = utils.ovn_port_group_name(security_group['id']) + txn.add(self._nb_idl.pg_add( + name=name, acls=[], external_ids=ext_ids)) + # When a SG is created, it comes with some default rules, + # so we'll apply them to the Port Group. + ovn_acl.add_acls_for_sg_port_group(self._nb_idl, + security_group, txn) db_rev.bump_revision( context, security_group, ovn_const.TYPE_SECURITY_GROUPS) @@ -2095,14 +1959,8 @@ class OVNClient(object): def delete_security_group(self, context, security_group_id): with self._nb_idl.transaction(check_error=True) as txn: - if self._nb_idl.is_port_groups_supported(): - name = utils.ovn_port_group_name(security_group_id) - txn.add(self._nb_idl.pg_del(name=name)) - else: - for ip_version in ('ip4', 'ip6'): - name = utils.ovn_addrset_name(security_group_id, - ip_version) - txn.add(self._nb_idl.delete_address_set(name=name)) + name = utils.ovn_port_group_name(security_group_id) + txn.add(self._nb_idl.pg_del(name=name)) db_rev.delete_revision(context, security_group_id, ovn_const.TYPE_SECURITY_GROUPS) diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py index 9c3110c7ebd..4cd14842f6e 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py @@ -86,7 +86,6 @@ class OvnNbSynchronizer(OvnDbSynchronizer): ctx = context.get_admin_context() - self.sync_address_sets(ctx) self.sync_port_groups(ctx) self.sync_networks_ports_and_dhcp_opts(ctx) self.sync_port_dns_records(ctx) @@ -118,24 +117,6 @@ class OvnNbSynchronizer(OvnDbSynchronizer): neutron_acls[port].remove(acl) nb_acls[port].remove(acl) - def compute_address_set_difference(self, neutron_sgs, nb_sgs): - neutron_sgs_name_set = set(neutron_sgs.keys()) - nb_sgs_name_set = set(nb_sgs.keys()) - sgnames_to_add = list(neutron_sgs_name_set - nb_sgs_name_set) - sgnames_to_delete = list(nb_sgs_name_set - neutron_sgs_name_set) - sgs_common = list(neutron_sgs_name_set & nb_sgs_name_set) - sgs_to_update = {} - for sg_name in sgs_common: - neutron_addr_set = set(neutron_sgs[sg_name]['addresses']) - nb_addr_set = set(nb_sgs[sg_name]['addresses']) - addrs_to_add = list(neutron_addr_set - nb_addr_set) - addrs_to_delete = list(nb_addr_set - neutron_addr_set) - if addrs_to_add or addrs_to_delete: - sgs_to_update[sg_name] = {'name': sg_name, - 'addrs_add': addrs_to_add, - 'addrs_remove': addrs_to_delete} - return sgnames_to_add, sgnames_to_delete, sgs_to_update - def get_acls(self, context): """create the list of ACLS in OVN. @@ -163,17 +144,12 @@ class OvnNbSynchronizer(OvnDbSynchronizer): acl_list_dict[key] = list([acl]) return acl_list_dict - def get_address_sets(self): - return self.ovn_api.get_address_sets() - def sync_port_groups(self, ctx): """Sync Port Groups between neutron and NB. @param ctx: neutron_lib.context @type ctx: object of type neutron_lib.context.Context """ - if not self.ovn_api.is_port_groups_supported(): - return neutron_sgs = {} neutron_pgs = set() @@ -234,68 +210,6 @@ class OvnNbSynchronizer(OvnDbSynchronizer): LOG.debug('Port-Group-SYNC: transaction finished @ %s', str(datetime.now())) - def sync_address_sets(self, ctx): - """Sync Address Sets between neutron and NB. - - @param ctx: neutron_lib.context - @type ctx: object of type neutron_lib.context.Context - @var db_ports: List of ports from neutron DB - """ - LOG.debug('Address-Set-SYNC: started @ %s', str(datetime.now())) - - sgnames_to_add = sgnames_to_delete = [] - sgs_to_update = {} - nb_sgs = self.get_address_sets() - - if self.ovn_api.is_port_groups_supported(): - # If Port Groups are supported, we just need to delete all Address - # Sets from NB database. - sgnames_to_delete = nb_sgs.keys() - else: - neutron_sgs = {} - with ctx.session.begin(subtransactions=True): - db_sgs = self.core_plugin.get_security_groups(ctx) - db_ports = self.core_plugin.get_ports(ctx) - - for sg in db_sgs: - for ip_version in ['ip4', 'ip6']: - name = utils.ovn_addrset_name(sg['id'], ip_version) - neutron_sgs[name] = { - 'name': name, 'addresses': [], - 'external_ids': { - ovn_const.OVN_SG_EXT_ID_KEY: sg['id']}} - - for port in db_ports: - sg_ids = utils.get_lsp_security_groups(port) - if port.get('fixed_ips') and sg_ids: - addresses = acl_utils.acl_port_ips(port) - for sg_id in sg_ids: - for ip_version in addresses: - name = utils.ovn_addrset_name(sg_id, ip_version) - neutron_sgs[name]['addresses'].extend( - addresses[ip_version]) - - sgnames_to_add, sgnames_to_delete, sgs_to_update = ( - self.compute_address_set_difference(neutron_sgs, nb_sgs)) - - LOG.debug('Address_Sets added %d, removed %d, updated %d', - len(sgnames_to_add), len(sgnames_to_delete), - len(sgs_to_update)) - - if self.mode == SYNC_MODE_REPAIR: - LOG.debug('Address-Set-SYNC: transaction started @ %s', - str(datetime.now())) - with self.ovn_api.transaction(check_error=True) as txn: - for sgname in sgnames_to_add: - sg = neutron_sgs[sgname] - txn.add(self.ovn_api.create_address_set(**sg)) - for sgname, sg in sgs_to_update.items(): - txn.add(self.ovn_api.update_address_set(**sg)) - for sgname in sgnames_to_delete: - txn.add(self.ovn_api.delete_address_set(name=sgname)) - LOG.debug('Address-Set-SYNC: transaction finished @ %s', - str(datetime.now())) - def _get_acls_from_port_groups(self): ovn_acls = [] port_groups = self.ovn_api.db_list_rows('Port_Group').execute() @@ -311,15 +225,20 @@ class OvnNbSynchronizer(OvnDbSynchronizer): ovn_acls.append(acl_string) return ovn_acls - def _sync_acls_port_groups(self, ctx): - # If Port Groups are supported, the ACLs in the system will equal - # the number of SG rules plus the default drop rules as OVN would - # allow all traffic by default if those are not added. + def sync_acls(self, ctx): + """Sync ACLs between neutron and NB. + + @param ctx: neutron_lib.context + @type ctx: object of type neutron_lib.context.Context + @return: Nothing + """ + LOG.debug('ACL-SYNC: started @ %s', str(datetime.now())) + neutron_acls = [] for sgr in self.core_plugin.get_security_group_rules(ctx): pg_name = utils.ovn_port_group_name(sgr['security_group_id']) neutron_acls.append(acl_utils._add_sg_rule_acl_for_port_group( - pg_name, sgr, self.ovn_api)) + pg_name, sgr)) neutron_acls += acl_utils.add_acls_for_drop_port_group( ovn_const.OVN_DROP_PORT_GROUP_NAME) @@ -373,88 +292,6 @@ class OvnNbSynchronizer(OvnDbSynchronizer): 'Switch %s', aclr[0]) txn.add(self.ovn_api.acl_del(aclr[0])) - def _sync_acls(self, ctx): - """Sync ACLs between neutron and NB when not using Port Groups. - - @param ctx: neutron_lib.context - @type ctx: object of type neutron_lib.context.Context - @var db_ports: List of ports from neutron DB - @var neutron_acls: neutron dictionary of port - vs list-of-acls - @var nb_acls: NB dictionary of port - vs list-of-acls - @var subnet_cache: cache for subnets - @return: Nothing - """ - db_ports = {} - for port in self.core_plugin.get_ports(ctx): - db_ports[port['id']] = port - - sg_cache = {} - subnet_cache = {} - neutron_acls = {} - for port_id, port in db_ports.items(): - if utils.get_lsp_security_groups(port): - acl_list = acl_utils.add_acls(self.core_plugin, - ctx, - port, - sg_cache, - subnet_cache, - self.ovn_api) - if port_id in neutron_acls: - neutron_acls[port_id].extend(acl_list) - else: - neutron_acls[port_id] = acl_list - - nb_acls = self.get_acls(ctx) - - self.remove_common_acls(neutron_acls, nb_acls) - - num_acls_to_add = len(list(itertools.chain(*neutron_acls.values()))) - num_acls_to_remove = len(list(itertools.chain(*nb_acls.values()))) - if 0 != num_acls_to_add or 0 != num_acls_to_remove: - LOG.warning('ACLs-to-be-added %(add)d ' - 'ACLs-to-be-removed %(remove)d', - {'add': num_acls_to_add, - 'remove': num_acls_to_remove}) - - if self.mode == SYNC_MODE_REPAIR: - with self.ovn_api.transaction(check_error=True) as txn: - for acla in list(itertools.chain(*neutron_acls.values())): - LOG.warning('ACL found in Neutron but not in ' - 'OVN DB for port %s', acla['lport']) - txn.add(self.ovn_api.add_acl(**acla)) - - with self.ovn_api.transaction(check_error=True) as txn: - for aclr in list(itertools.chain(*nb_acls.values())): - # Both lswitch and lport aren't needed within the ACL. - lswitchr = aclr.pop('lswitch').replace('neutron-', '') - lportr = aclr.pop('lport') - aclr_dict = {lportr: aclr} - LOG.warning('ACLs found in OVN DB but not in ' - 'Neutron for port %s', lportr) - txn.add(self.ovn_api.update_acls( - [lswitchr], - [lportr], - aclr_dict, - need_compare=False, - is_add_acl=False - )) - - def sync_acls(self, ctx): - """Sync ACLs between neutron and NB. - - @param ctx: neutron_lib.context - @type ctx: object of type neutron_lib.context.Context - @return: Nothing - """ - LOG.debug('ACL-SYNC: started @ %s', str(datetime.now())) - - if self.ovn_api.is_port_groups_supported(): - self._sync_acls_port_groups(ctx) - else: - self._sync_acls(ctx) - LOG.debug('ACL-SYNC: finished @ %s', str(datetime.now())) def _calculate_fips_differences(self, ovn_fips, db_fips): @@ -1126,20 +963,6 @@ class OvnNbSynchronizer(OvnDbSynchronizer): # all the ACLs belonging to that Logical Switch. txn.add(self.ovn_api.acl_del(utils.ovn_name(net['id']))) - def _create_default_drop_port_group(self, db_ports): - with self.ovn_api.transaction(check_error=True) as txn: - pg_name = ovn_const.OVN_DROP_PORT_GROUP_NAME - if not self.ovn_api.get_port_group(pg_name): - # If drop Port Group doesn't exist yet, create it. - txn.add(self.ovn_api.pg_add(pg_name, acls=[])) - # Add ACLs to this Port Group so that all traffic is dropped. - acls = acl_utils.add_acls_for_drop_port_group(pg_name) - for acl in acls: - txn.add(self.ovn_api.pg_acl_add(**acl)) - ports_ids = [port['id'] for port in db_ports] - # Add the ports to the default Port Group - txn.add(self.ovn_api.pg_add_ports(pg_name, ports_ids)) - def _create_sg_port_groups_and_acls(self, ctx, db_ports): # Create a Port Group per Neutron Security Group with self.ovn_api.transaction(check_error=True) as txn: @@ -1159,17 +982,14 @@ class OvnNbSynchronizer(OvnDbSynchronizer): def migrate_to_port_groups(self, ctx): # This routine is responsible for migrating the current Security # Groups and SG Rules to the new Port Groups implementation. - # 1. Create the default drop Port Group and add all ports with port - # security enabled to it. - # 2. Create a Port Group for every existing Neutron Security Group and + # 1. Create a Port Group for every existing Neutron Security Group and # add all its Security Group Rules as ACLs to that Port Group. - # 3. Delete all existing Address Sets in NorthBound database which + # 2. Delete all existing Address Sets in NorthBound database which # correspond to a Neutron Security Group. - # 4. Delete all the ACLs in every Logical Switch (Neutron network). + # 3. Delete all the ACLs in every Logical Switch (Neutron network). - # If Port Groups are not supported or we've already migrated, return - if (not self.ovn_api.is_port_groups_supported() or - not self.ovn_api.get_address_sets()): + # If we've already migrated, return + if not self.ovn_api.get_address_sets(): return LOG.debug('Port Groups Migration task started') @@ -1182,7 +1002,6 @@ class OvnNbSynchronizer(OvnDbSynchronizer): utils.is_lsp_trusted(port) and utils.is_port_security_enabled(port)] - self._create_default_drop_port_group(db_ports) self._create_sg_port_groups_and_acls(ctx, db_ports) self._delete_address_sets(ctx) self._delete_acls_from_lswitches(ctx) diff --git a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py index 4b2541ee907..d0b75e9c8b5 100644 --- a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py +++ b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py @@ -170,15 +170,9 @@ class _TestMaintenanceHelper(base.TestOVNFunctionalBase): return self.deserialize(self.fmt, res)['security_group'] def _find_security_group_row_by_id(self, sg_id): - if self.nb_api.is_port_groups_supported(): - for row in self.nb_api._tables['Port_Group'].rows.values(): - if row.name == utils.ovn_port_group_name(sg_id): - return row - else: - for row in self.nb_api._tables['Address_Set'].rows.values(): - if (row.external_ids.get( - ovn_const.OVN_SG_EXT_ID_KEY) == sg_id): - return row + for row in self.nb_api._tables['Port_Group'].rows.values(): + if row.name == utils.ovn_port_group_name(sg_id): + return row def _create_security_group_rule(self, sg_id): data = {'security_group_rule': {'security_group_id': sg_id, diff --git a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_resources.py b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_resources.py index 746fd7df09e..8c9fce7ed81 100644 --- a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_resources.py +++ b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_resources.py @@ -821,104 +821,10 @@ class TestPortSecurity(base.TestOVNFunctionalBase): return port_acls def _verify_port_acls(self, port_id, expected_acls): - if self.nb_api.is_port_groups_supported(): - port_acls = self._get_port_related_acls(port_id) - else: - port_acls = self._get_port_related_acls_port_group_not_supported( - port_id) + port_acls = self._get_port_related_acls(port_id) self.assertItemsEqual(expected_acls, port_acls) - @mock.patch('neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb.' - 'impl_idl_ovn.OvsdbNbOvnIdl.is_port_groups_supported', - lambda *args: False) - def test_port_security_port_group_not_supported(self): - n1 = self._make_network(self.fmt, 'n1', True) - res = self._create_subnet(self.fmt, n1['network']['id'], '10.0.0.0/24') - subnet = self.deserialize(self.fmt, res)['subnet'] - p = self._make_port(self.fmt, n1['network']['id'], - fixed_ips=[{'subnet_id': subnet['id']}]) - port_id = p['port']['id'] - sg_id = p['port']['security_groups'][0].replace('-', '_') - expected_acls_with_sg_ps_enabled = [ - {'match': 'inport == "' + str(port_id) + '" && ip', - 'action': 'drop', - 'priority': 1001, - 'direction': 'from-lport'}, - {'match': 'outport == "' + str(port_id) + '" && ip', - 'action': 'drop', - 'priority': 1001, - 'direction': 'to-lport'}, - {'match': 'inport == "' + str(port_id) + '" && ip4 && ip4.dst == ' - '{255.255.255.255, 10.0.0.0/24} && udp && udp.src == 68 ' - '&& udp.dst == 67', - 'action': 'allow', - 'priority': 1002, - 'direction': 'from-lport'}, - {'match': 'inport == "' + str(port_id) + '" && ip6', - 'action': 'allow-related', - 'priority': 1002, - 'direction': 'from-lport'}, - {'match': 'inport == "' + str(port_id) + '" && ip4', - 'action': 'allow-related', - 'priority': 1002, - 'direction': 'from-lport'}, - {'match': 'outport == "' + str(port_id) + '" && ip4 && ' - 'ip4.src == $as_ip4_' + str(sg_id), - 'action': 'allow-related', - 'priority': 1002, - 'direction': 'to-lport'}, - {'match': 'outport == "' + str(port_id) + '" && ip6 && ' - 'ip6.src == $as_ip6_' + str(sg_id), - 'action': 'allow-related', - 'priority': 1002, - 'direction': 'to-lport'}, - ] - self._verify_port_acls(port_id, expected_acls_with_sg_ps_enabled) - - # clear the security groups. - data = {'port': {'security_groups': []}} - port_req = self.new_update_request('ports', data, p['port']['id']) - port_req.get_response(self.api) - - # No security groups and port security enabled - > ACLs should be - # added to drop the packets. - expected_acls_with_no_sg_ps_enabled = [ - {'match': 'inport == "' + str(port_id) + '" && ip', - 'action': 'drop', - 'priority': 1001, - 'direction': 'from-lport'}, - {'match': 'outport == "' + str(port_id) + '" && ip', - 'action': 'drop', - 'priority': 1001, - 'direction': 'to-lport'}, - ] - self._verify_port_acls(port_id, expected_acls_with_no_sg_ps_enabled) - - # Disable port security - data = {'port': {'port_security_enabled': False}} - port_req = self.new_update_request('ports', data, p['port']['id']) - port_req.get_response(self.api) - # No security groups and port security disabled - > No ACLs should be - # added (allowing all the traffic). - self._verify_port_acls(port_id, []) - - # Enable port security again with no security groups - > ACLs should - # be added back to drop the packets. - data = {'port': {'port_security_enabled': True}} - port_req = self.new_update_request('ports', data, p['port']['id']) - port_req.get_response(self.api) - self._verify_port_acls(port_id, expected_acls_with_no_sg_ps_enabled) - - # Set security groups back - data = {'port': {'security_groups': p['port']['security_groups']}} - port_req = self.new_update_request('ports', data, p['port']['id']) - port_req.get_response(self.api) - self._verify_port_acls(port_id, expected_acls_with_sg_ps_enabled) - def test_port_security_port_group(self): - if not self.nb_api.is_port_groups_supported(): - self.skipTest('Port groups is not supported') - n1 = self._make_network(self.fmt, 'n1', True) res = self._create_subnet(self.fmt, n1['network']['id'], '10.0.0.0/24') subnet = self.deserialize(self.fmt, res)['subnet'] diff --git a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py index bd478730214..ba717a43a16 100644 --- a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py +++ b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py @@ -12,8 +12,6 @@ # License for the specific language governing permissions and limitations # under the License. -from unittest import mock - from neutron.common.ovn import acl as acl_utils from neutron.common.ovn import constants as ovn_const from neutron.common.ovn import utils @@ -60,9 +58,6 @@ class TestOvnNbSync(base.TestOVNFunctionalBase): self.delete_lrouter_routes = [] self.delete_lrouter_nats = [] self.delete_acls = [] - self.create_address_sets = [] - self.delete_address_sets = [] - self.update_address_sets = [] self.create_port_groups = [] self.delete_port_groups = [] self.expected_dhcp_options_rows = [] @@ -579,17 +574,6 @@ class TestOvnNbSync(base.TestOVNFunctionalBase): 'neutron-' + r1['id'])) self.delete_lrouters.append('neutron-' + r2['id']) - address_set_name = n1_prtr['port']['security_groups'][0] - self.create_address_sets.extend([('fake_sg', 'ip4'), - ('fake_sg', 'ip6')]) - self.delete_address_sets.append((address_set_name, 'ip6')) - address_adds = ['10.0.0.101', '10.0.0.102'] - address_dels = [] - for address in n1_prtr['port']['fixed_ips']: - address_dels.append(address['ip_address']) - self.update_address_sets.append((address_set_name, 'ip4', - address_adds, address_dels)) - self.create_port_groups.extend([{'name': 'pg1', 'acls': []}, {'name': 'pg2', 'acls': []}]) self.delete_port_groups.append( @@ -756,26 +740,10 @@ class TestOvnNbSync(base.TestOVNFunctionalBase): txn.add(self.nb_api.delete_acl(lswitch_name, lport_name, True)) - for name, ip_version in self.create_address_sets: - ovn_name = utils.ovn_addrset_name(name, ip_version) - external_ids = {ovn_const.OVN_SG_EXT_ID_KEY: name} - txn.add(self.nb_api.create_address_set( - ovn_name, True, external_ids=external_ids)) - - for name, ip_version in self.delete_address_sets: - ovn_name = utils.ovn_addrset_name(name, ip_version) - txn.add(self.nb_api.delete_address_set(ovn_name, True)) - - for name, ip_version, ip_adds, ip_dels in self.update_address_sets: - ovn_name = utils.ovn_addrset_name(name, ip_version) - txn.add(self.nb_api.update_address_set(ovn_name, - ip_adds, ip_dels, True)) - - if self.nb_api.is_port_groups_supported(): - for pg in self.create_port_groups: - txn.add(self.nb_api.pg_add(**pg)) - for pg in self.delete_port_groups: - txn.add(self.nb_api.pg_del(pg)) + for pg in self.create_port_groups: + txn.add(self.nb_api.pg_add(**pg)) + for pg in self.delete_port_groups: + txn.add(self.nb_api.pg_del(pg)) for lport_name in self.reset_lport_dhcpv4_options: txn.add(self.nb_api.set_lswitch_port(lport_name, True, @@ -1077,54 +1045,39 @@ class TestOvnNbSync(base.TestOVNFunctionalBase): def _validate_acls(self, should_match=True): # Get the neutron DB ACLs. db_acls = [] - sg_cache = {} - subnet_cache = {} _plugin_nb_ovn = self.mech_driver._nb_ovn - if not _plugin_nb_ovn.is_port_groups_supported(): - for db_port in self._list('ports')['ports']: - acls = acl_utils.add_acls(self.plugin, - context.get_admin_context(), - db_port, - sg_cache, - subnet_cache, - self.mech_driver._nb_ovn) - for acl in acls: - db_acls.append(acl_utils.filter_acl_dict(acl)) - else: - # ACLs due to SGs and default drop port group - for sg in self._list('security-groups')['security_groups']: - for sgr in sg['security_group_rules']: - acl = acl_utils._add_sg_rule_acl_for_port_group( - utils.ovn_port_group_name(sg['id']), sgr, - self.mech_driver._nb_ovn) - db_acls.append(TestOvnNbSync._build_acl_for_pgs(**acl)) - for acl in acl_utils.add_acls_for_drop_port_group( - ovn_const.OVN_DROP_PORT_GROUP_NAME): + # ACLs due to SGs and default drop port group + for sg in self._list('security-groups')['security_groups']: + for sgr in sg['security_group_rules']: + acl = acl_utils._add_sg_rule_acl_for_port_group( + utils.ovn_port_group_name(sg['id']), sgr) db_acls.append(TestOvnNbSync._build_acl_for_pgs(**acl)) + for acl in acl_utils.add_acls_for_drop_port_group( + ovn_const.OVN_DROP_PORT_GROUP_NAME): + db_acls.append(TestOvnNbSync._build_acl_for_pgs(**acl)) + # Get the list of ACLs stored in the OVN plugin IDL. plugin_acls = [] for row in _plugin_nb_ovn._tables['Logical_Switch'].rows.values(): for acl in getattr(row, 'acls', []): plugin_acls.append(self._build_acl_to_compare(acl)) - if self.nb_api.is_port_groups_supported(): - for row in _plugin_nb_ovn._tables['Port_Group'].rows.values(): - for acl in getattr(row, 'acls', []): - plugin_acls.append( - self._build_acl_to_compare( - acl, extra_fields=['external_ids'])) + for row in _plugin_nb_ovn._tables['Port_Group'].rows.values(): + for acl in getattr(row, 'acls', []): + plugin_acls.append( + self._build_acl_to_compare( + acl, extra_fields=['external_ids'])) # Get the list of ACLs stored in the OVN monitor IDL. monitor_acls = [] for row in self.nb_api.tables['Logical_Switch'].rows.values(): for acl in getattr(row, 'acls', []): monitor_acls.append(self._build_acl_to_compare(acl)) - if _plugin_nb_ovn.is_port_groups_supported(): - for row in self.nb_api.tables['Port_Group'].rows.values(): - for acl in getattr(row, 'acls', []): - monitor_acls.append(self._build_acl_to_compare(acl)) + for row in self.nb_api.tables['Port_Group'].rows.values(): + for acl in getattr(row, 'acls', []): + monitor_acls.append(self._build_acl_to_compare(acl)) if should_match: self.assertItemsEqual(db_acls, plugin_acls) @@ -1346,57 +1299,8 @@ class TestOvnNbSync(base.TestOVNFunctionalBase): AssertionError, self.assertItemsEqual, r_nats, monitor_nats) - def _validate_address_sets(self, should_match=True): - _plugin_nb_ovn = self.mech_driver._nb_ovn - if _plugin_nb_ovn.is_port_groups_supported(): - # If Port Groups are supported, no Address Sets are expected. - # This validation is still useful as we expect existing ones to - # be deleted after the sync. - db_sgs = [] - else: - db_ports = self._list('ports')['ports'] - sgs = self._list('security-groups')['security_groups'] - db_sgs = {} - for sg in sgs: - for ip_version in ['ip4', 'ip6']: - name = utils.ovn_addrset_name(sg['id'], ip_version) - db_sgs[name] = [] - - for port in db_ports: - sg_ids = utils.get_lsp_security_groups(port) - addresses = acl_utils.acl_port_ips(port) - for sg_id in sg_ids: - for ip_version in addresses: - name = utils.ovn_addrset_name(sg_id, ip_version) - db_sgs[name].extend(addresses[ip_version]) - - nb_address_sets = _plugin_nb_ovn.get_address_sets() - nb_sgs = {} - for nb_sgid, nb_values in nb_address_sets.items(): - nb_sgs[nb_sgid] = nb_values['addresses'] - - mn_sgs = {} - for row in self.nb_api.tables['Address_Set'].rows.values(): - mn_sgs[getattr(row, 'name')] = getattr(row, 'addresses') - - if should_match: - self.assertItemsEqual(nb_sgs, db_sgs) - self.assertItemsEqual(mn_sgs, db_sgs) - else: - # This condition is to cover the case when we use Port Groups - # and we completely deleted the NB DB. At this point, the expected - # number of Address Sets is 0 and the observed number in NB is - # also 0 so we can't have the asserts below as both will be empty. - if _plugin_nb_ovn.is_port_groups_supported() and nb_sgs: - self.assertRaises(AssertionError, self.assertItemsEqual, - nb_sgs, db_sgs) - self.assertRaises(AssertionError, self.assertItemsEqual, - mn_sgs, db_sgs) - def _validate_port_groups(self, should_match=True): _plugin_nb_ovn = self.mech_driver._nb_ovn - if not _plugin_nb_ovn.is_port_groups_supported(): - return db_pgs = [] for sg in self._list('security-groups')['security_groups']: @@ -1469,7 +1373,6 @@ class TestOvnNbSync(base.TestOVNFunctionalBase): self._validate_dhcp_opts(should_match=should_match) self._validate_acls(should_match=should_match) self._validate_routers_and_router_ports(should_match=should_match) - self._validate_address_sets(should_match=should_match) self._validate_port_groups(should_match=should_match) self._validate_dns_records(should_match=should_match) @@ -1624,9 +1527,3 @@ class TestOvnNbSyncOverSsl(TestOvnNbSync): class TestOvnSbSyncOverSsl(TestOvnSbSync): def get_ovsdb_server_protocol(self): return 'ssl' - - -@mock.patch('neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb.impl_idl_ovn.' - 'OvsdbNbOvnIdl.is_port_groups_supported', lambda *args: False) -class TestOvnNbSyncNoPgs(TestOvnNbSync): - pass diff --git a/neutron/tests/unit/common/ovn/test_acl.py b/neutron/tests/unit/common/ovn/test_acl.py index d1f60f7958f..d4dcd2d5df3 100644 --- a/neutron/tests/unit/common/ovn/test_acl.py +++ b/neutron/tests/unit/common/ovn/test_acl.py @@ -15,7 +15,6 @@ import copy from unittest import mock from neutron_lib import constants as const -from oslo_config import cfg from ovsdbapp.backend.ovs_idl import idlutils from neutron.common.ovn import acl as ovn_acl @@ -548,114 +547,15 @@ class TestACLs(base.BaseTestCase): ip_version = 'ip4' sg_id = sg_rule['security_group_id'] - addrset_name = ovn_utils.ovn_addrset_name(sg_id, ip_version) + pg_name = ovn_utils.ovn_pg_addrset_name(sg_id, ip_version) match = ovn_acl.acl_remote_group_id(sg_rule, ip_version) self.assertEqual('', match) sg_rule['remote_group_id'] = sg_id match = ovn_acl.acl_remote_group_id(sg_rule, ip_version) - self.assertEqual(' && ip4.src == $' + addrset_name, match) + self.assertEqual(' && ip4.src == $' + pg_name, match) sg_rule['direction'] = 'egress' match = ovn_acl.acl_remote_group_id(sg_rule, ip_version) - self.assertEqual(' && ip4.dst == $' + addrset_name, match) - - def _test_update_acls_for_security_group(self, use_cache=True): - sg = fakes.FakeSecurityGroup.create_one_security_group().info() - remote_sg = fakes.FakeSecurityGroup.create_one_security_group().info() - sg_rule = fakes.FakeSecurityGroupRule.create_one_security_group_rule({ - 'security_group_id': sg['id'], - 'remote_group_id': remote_sg['id'] - }).info() - port = fakes.FakePort.create_one_port({ - 'security_groups': [sg['id']] - }).info() - self.plugin.get_ports.return_value = [port] - if use_cache: - sg_ports_cache = {sg['id']: [{'port_id': port['id']}], - remote_sg['id']: []} - else: - sg_ports_cache = None - self.plugin._get_port_security_group_bindings.return_value = \ - [{'port_id': port['id']}] - - # Build ACL for validation. - expected_acl = ovn_acl._add_sg_rule_acl_for_port(port, sg_rule) - expected_acl.pop('lport') - expected_acl.pop('lswitch') - - # Validate ACLs when port has security groups. - ovn_acl.update_acls_for_security_group(self.plugin, - self.admin_context, - self.driver._nb_ovn, - sg['id'], - sg_rule, - sg_ports_cache=sg_ports_cache) - self.driver._nb_ovn.update_acls.assert_called_once_with( - [port['network_id']], - mock.ANY, - {port['id']: expected_acl}, - need_compare=False, - is_add_acl=True - ) - - def test_update_acls_for_security_group_cache(self): - self._test_update_acls_for_security_group(use_cache=True) - - def test_update_acls_for_security_group_no_cache(self): - self._test_update_acls_for_security_group(use_cache=False) - - def test_acl_port_ips(self): - port4 = fakes.FakePort.create_one_port({ - 'fixed_ips': [{'subnet_id': 'subnet-ipv4', - 'ip_address': '10.0.0.1'}], - }).info() - port46 = fakes.FakePort.create_one_port({ - 'fixed_ips': [{'subnet_id': 'subnet-ipv4', - 'ip_address': '10.0.0.2'}, - {'subnet_id': 'subnet-ipv6', - 'ip_address': 'fde3:d45:df72::1'}], - }).info() - port6 = fakes.FakePort.create_one_port({ - 'fixed_ips': [{'subnet_id': 'subnet-ipv6', - 'ip_address': '2001:db8::8'}], - }).info() - - addresses = ovn_acl.acl_port_ips(port4) - self.assertEqual({'ip4': [port4['fixed_ips'][0]['ip_address']], - 'ip6': []}, - addresses) - - addresses = ovn_acl.acl_port_ips(port46) - self.assertEqual({'ip4': [port46['fixed_ips'][0]['ip_address']], - 'ip6': [port46['fixed_ips'][1]['ip_address']]}, - addresses) - - addresses = ovn_acl.acl_port_ips(port6) - self.assertEqual({'ip4': [], - 'ip6': [port6['fixed_ips'][0]['ip_address']]}, - addresses) - - def test_sg_disabled(self): - sg = fakes.FakeSecurityGroup.create_one_security_group().info() - port = fakes.FakePort.create_one_port({ - 'security_groups': [sg['id']] - }).info() - - cfg.CONF.set_override('enable_security_group', 'False', - 'SECURITYGROUP') - acl_list = ovn_acl.add_acls(self.plugin, - self.admin_context, - port, {}, {}, self.driver._ovn) - self.assertEqual([], acl_list) - - ovn_acl.update_acls_for_security_group(self.plugin, - self.admin_context, - self.driver._ovn, - sg['id'], - None) - self.driver._ovn.update_acls.assert_not_called() - - addresses = ovn_acl.acl_port_ips(port) - self.assertEqual({'ip4': [], 'ip6': []}, addresses) + self.assertEqual(' && ip4.dst == $' + pg_name, match) diff --git a/neutron/tests/unit/fake_resources.py b/neutron/tests/unit/fake_resources.py index b711d68dc41..968ed061ee3 100644 --- a/neutron/tests/unit/fake_resources.py +++ b/neutron/tests/unit/fake_resources.py @@ -71,10 +71,6 @@ class FakeOvsdbNbOvnIdl(object): self.idl = mock.Mock() self.add_static_route = mock.Mock() self.delete_static_route = mock.Mock() - self.create_address_set = mock.Mock() - self.update_address_set_ext_ids = mock.Mock() - self.delete_address_set = mock.Mock() - self.update_address_set = mock.Mock() self.get_all_chassis_gateway_bindings = mock.Mock() self.get_chassis_gateways = mock.Mock() self.get_gateway_chassis_binding = mock.Mock() @@ -121,12 +117,6 @@ class FakeOvsdbNbOvnIdl(object): self.get_lrouter.return_value = None self.delete_lrouter_ext_gw = mock.Mock() self.delete_lrouter_ext_gw.return_value = None - self.is_port_groups_supported = mock.Mock() - # TODO(lucasagomes): Flip this return value to True at some point, - # port groups should be the default method used by networking-ovn - self.is_port_groups_supported.return_value = False - self.get_address_set = mock.Mock() - self.get_address_set.return_value = None self.pg_acl_add = mock.Mock() self.pg_acl_del = mock.Mock() self.pg_del = mock.Mock() diff --git a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_commands.py b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_commands.py index 65fcaa8ecc8..69715b9ce3f 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_commands.py +++ b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_commands.py @@ -936,171 +936,6 @@ class TestDelStaticRouteCommand(TestBaseCommand): self.assertEqual([mock.ANY], fake_lrouter.static_routes) -class TestAddAddrSetCommand(TestBaseCommand): - - def test_addrset_exists(self): - with mock.patch.object(idlutils, 'row_by_value', - return_value=mock.ANY): - cmd = commands.AddAddrSetCommand( - self.ovn_api, 'fake-addrset', may_exist=True) - cmd.run_idl(self.transaction) - self.transaction.insert.assert_not_called() - - def test_addrset_add_exists(self): - fake_addrset = fakes.FakeOvsdbRow.create_one_ovsdb_row() - self.ovn_api._tables['Address_Set'].rows[fake_addrset.uuid] = \ - fake_addrset - self.transaction.insert.return_value = fake_addrset - cmd = commands.AddAddrSetCommand( - self.ovn_api, fake_addrset.name, may_exist=False) - cmd.run_idl(self.transaction) - # NOTE(rtheis): Mocking the transaction allows this insert - # to succeed when it normally would fail due the duplicate name. - self.transaction.insert.assert_called_once_with( - self.ovn_api._tables['Address_Set']) - - def _test_addrset_add(self, may_exist=True): - with mock.patch.object(idlutils, 'row_by_value', - return_value=None): - fake_addrset = fakes.FakeOvsdbRow.create_one_ovsdb_row( - attrs={'foo': ''}) - self.transaction.insert.return_value = fake_addrset - cmd = commands.AddAddrSetCommand( - self.ovn_api, 'fake-addrset', may_exist=may_exist, - foo='bar') - cmd.run_idl(self.transaction) - self.transaction.insert.assert_called_once_with( - self.ovn_api._tables['Address_Set']) - self.assertEqual('fake-addrset', fake_addrset.name) - self.assertEqual('bar', fake_addrset.foo) - - def test_addrset_add_may_exist(self): - self._test_addrset_add(may_exist=True) - - def test_addrset_add_ignore_exists(self): - self._test_addrset_add(may_exist=False) - - -class TestDelAddrSetCommand(TestBaseCommand): - - def _test_addrset_del_no_exist(self, if_exists=True): - with mock.patch.object(idlutils, 'row_by_value', - side_effect=idlutils.RowNotFound): - cmd = commands.DelAddrSetCommand( - self.ovn_api, 'fake-addrset', if_exists=if_exists) - if if_exists: - cmd.run_idl(self.transaction) - else: - self.assertRaises(RuntimeError, cmd.run_idl, self.transaction) - - def test_addrset_no_exist_ignore(self): - self._test_addrset_del_no_exist(if_exists=True) - - def test_addrset_no_exist_fail(self): - self._test_addrset_del_no_exist(if_exists=False) - - def test_addrset_del(self): - fake_addrset = fakes.FakeOvsdbRow.create_one_ovsdb_row() - self.ovn_api._tables['Address_Set'].rows[fake_addrset.uuid] = \ - fake_addrset - with mock.patch.object(idlutils, 'row_by_value', - return_value=fake_addrset): - cmd = commands.DelAddrSetCommand( - self.ovn_api, fake_addrset.name, if_exists=True) - cmd.run_idl(self.transaction) - fake_addrset.delete.assert_called_once_with() - - -class TestUpdateAddrSetCommand(TestBaseCommand): - - def _test_addrset_update_no_exist(self, if_exists=True): - with mock.patch.object(idlutils, 'row_by_value', - side_effect=idlutils.RowNotFound): - cmd = commands.UpdateAddrSetCommand( - self.ovn_api, 'fake-addrset', - addrs_add=[], addrs_remove=[], - if_exists=if_exists) - if if_exists: - cmd.run_idl(self.transaction) - else: - self.assertRaises(RuntimeError, cmd.run_idl, self.transaction) - - def test_addrset_no_exist_ignore(self): - self._test_addrset_update_no_exist(if_exists=True) - - def test_addrset_no_exist_fail(self): - self._test_addrset_update_no_exist(if_exists=False) - - def _test_addrset_update(self, addrs_add=None, addrs_del=None): - save_address = '10.0.0.1' - initial_addresses = [save_address] - final_addresses = [save_address] - expected_addvalue_calls = [] - expected_delvalue_calls = [] - if addrs_add: - for addr_add in addrs_add: - final_addresses.append(addr_add) - expected_addvalue_calls.append( - mock.call('addresses', addr_add)) - if addrs_del: - for addr_del in addrs_del: - initial_addresses.append(addr_del) - expected_delvalue_calls.append( - mock.call('addresses', addr_del)) - fake_addrset = fakes.FakeOvsdbRow.create_one_ovsdb_row( - attrs={'addresses': initial_addresses}) - with mock.patch.object(idlutils, 'row_by_value', - return_value=fake_addrset): - cmd = commands.UpdateAddrSetCommand( - self.ovn_api, fake_addrset.name, - addrs_add=addrs_add, addrs_remove=addrs_del, - if_exists=True) - cmd.run_idl(self.transaction) - fake_addrset.addvalue.assert_has_calls(expected_addvalue_calls) - fake_addrset.delvalue.assert_has_calls(expected_delvalue_calls) - - def test_addrset_update_add(self): - self._test_addrset_update(addrs_add=['10.0.0.4']) - - def test_addrset_update_del(self): - self._test_addrset_update(addrs_del=['10.0.0.2']) - - -class TestUpdateAddrSetExtIdsCommand(TestBaseCommand): - def setUp(self): - super(TestUpdateAddrSetExtIdsCommand, self).setUp() - self.ext_ids = {ovn_const.OVN_SG_EXT_ID_KEY: 'default'} - - def _test_addrset_extids_update_no_exist(self, if_exists=True): - with mock.patch.object(idlutils, 'row_by_value', - side_effect=idlutils.RowNotFound): - cmd = commands.UpdateAddrSetExtIdsCommand( - self.ovn_api, 'fake-addrset', self.ext_ids, - if_exists=if_exists) - if if_exists: - cmd.run_idl(self.transaction) - else: - self.assertRaises(RuntimeError, cmd.run_idl, self.transaction) - - def test_addrset_no_exist_ignore(self): - self._test_addrset_extids_update_no_exist(if_exists=True) - - def test_addrset_no_exist_fail(self): - self._test_addrset_extids_update_no_exist(if_exists=False) - - def test_addrset_extids_update(self): - new_ext_ids = {ovn_const.OVN_SG_EXT_ID_KEY: 'default-new'} - fake_addrset = fakes.FakeOvsdbRow.create_one_ovsdb_row( - attrs={'external_ids': self.ext_ids}) - with mock.patch.object(idlutils, 'row_by_value', - return_value=fake_addrset): - cmd = commands.UpdateAddrSetExtIdsCommand( - self.ovn_api, fake_addrset.name, - new_ext_ids, if_exists=True) - cmd.run_idl(self.transaction) - self.assertEqual(new_ext_ids, fake_addrset.external_ids) - - class TestUpdateChassisExtIdsCommand(TestBaseCommand): def setUp(self): super(TestUpdateChassisExtIdsCommand, self).setUp() diff --git a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py index 10b8542f859..a3ebe4a3b5d 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py +++ b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py @@ -100,10 +100,8 @@ class TestDBInconsistenciesPeriodics(testlib_api.SqlTestCaseLight, self.periodic.check_for_inconsistencies() mock_fix_net.assert_called_once_with(mock.ANY, fake_row) - def _test_migrate_to_port_groups_helper(self, pg_supported, a_sets, - migration_expected, never_again): - self.fake_ovn_client._nb_idl.is_port_groups_supported.return_value = ( - pg_supported) + def _test_migrate_to_port_groups_helper(self, a_sets, migration_expected, + never_again): self.fake_ovn_client._nb_idl.get_address_sets.return_value = a_sets with mock.patch.object(ovn_db_sync.OvnNbSynchronizer, 'migrate_to_port_groups') as mtpg: @@ -118,24 +116,15 @@ class TestDBInconsistenciesPeriodics(testlib_api.SqlTestCaseLight, else: mtpg.assert_not_called() - def test_migrate_to_port_groups_port_groups_not_supported(self): - self._test_migrate_to_port_groups_helper(pg_supported=False, - a_sets=None, - migration_expected=False, - never_again=True) - def test_migrate_to_port_groups_not_needed(self): - self._test_migrate_to_port_groups_helper(pg_supported=True, - a_sets=None, + self._test_migrate_to_port_groups_helper(a_sets=None, migration_expected=False, never_again=True) def test_migrate_to_port_groups(self): - # Check normal migration path: if port groups are supported by the - # schema and the migration has to be done, it will take place and - # won't be attempted in the future. - self._test_migrate_to_port_groups_helper(pg_supported=True, - a_sets=['as1', 'as2'], + # Check normal migration path: if the migration has to be done, it will + # take place and won't be attempted in the future. + self._test_migrate_to_port_groups_helper(a_sets=['as1', 'as2'], migration_expected=True, never_again=True) @@ -145,8 +134,7 @@ class TestDBInconsistenciesPeriodics(testlib_api.SqlTestCaseLight, return_value=False)): # Check that if this worker doesn't have the lock, it won't # perform the migration and it will try again later. - self._test_migrate_to_port_groups_helper(pg_supported=True, - a_sets=['as1', 'as2'], + self._test_migrate_to_port_groups_helper(a_sets=['as1', 'as2'], migration_expected=False, never_again=False) diff --git a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py index 34e7811556a..b259ef4ec15 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py +++ b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py @@ -213,25 +213,6 @@ class TestOvnNbSyncML2(test_mech_driver.OVNMechanismDriverTestCase): 'match': 'outport == "p2n2" && ip4 && ' 'ip4.src == 10.0.0.0/24 && udp && ' 'udp.src == 67 && udp.dst == 68'}]} - self.address_sets_ovn = { - 'as_ip4_sg1': {'external_ids': {ovn_const.OVN_SG_EXT_ID_KEY: - 'all-tcp'}, - 'name': 'as_ip4_sg1', - 'addresses': ['10.0.0.4']}, - 'as_ip4_sg2': {'external_ids': {ovn_const.OVN_SG_EXT_ID_KEY: - 'all-tcpe'}, - 'name': 'as_ip4_sg2', - 'addresses': []}, - 'as_ip6_sg2': {'external_ids': {ovn_const.OVN_SG_EXT_ID_KEY: - 'all-tcpe'}, - 'name': 'as_ip6_sg2', - 'addresses': ['fd79:e1c:a55::816:eff:eff:ff2', - 'fd79:e1c:a55::816:eff:eff:ff3']}, - 'as_ip4_del': {'external_ids': {ovn_const.OVN_SG_EXT_ID_KEY: - 'all-delete'}, - 'name': 'as_ip4_delete', - 'addresses': ['10.0.0.4']}, - } self.routers = [{'id': 'r1', 'routes': [{'nexthop': '20.0.0.100', 'destination': '11.0.0.0/24'}, { @@ -392,9 +373,6 @@ class TestOvnNbSyncML2(test_mech_driver.OVNMechanismDriverTestCase): ovn_nb_synchronizer.get_acls.return_value = self.acls_ovn core_plugin.get_security_groups = mock.MagicMock( return_value=self.security_groups) - ovn_nb_synchronizer.get_address_sets = mock.Mock() - ovn_nb_synchronizer.get_address_sets.return_value =\ - self.address_sets_ovn get_port_groups = mock.MagicMock() get_port_groups.execute.return_value = self.port_groups_ovn ovn_api.db_list_rows.return_value = get_port_groups @@ -505,9 +483,6 @@ class TestOvnNbSyncML2(test_mech_driver.OVNMechanismDriverTestCase): 'port_id': 'p1n2'}, 'uuid': 'UUID6'}}} - ovn_api.create_address_set = mock.Mock() - ovn_api.delete_address_set = mock.Mock() - ovn_api.update_address_set = mock.Mock() ovn_nb_synchronizer._ovn_client._add_subnet_dhcp_options = mock.Mock() ovn_nb_synchronizer._ovn_client._get_ovn_dhcp_options = mock.Mock() ovn_nb_synchronizer._ovn_client._get_ovn_dhcp_options.side_effect = ( @@ -528,54 +503,17 @@ class TestOvnNbSyncML2(test_mech_driver.OVNMechanismDriverTestCase): add_static_route_list, del_static_route_list, add_snat_list, del_snat_list, add_floating_ip_list, del_floating_ip_list, - add_address_set_list, del_address_set_list, - update_address_set_list, add_subnet_dhcp_options_list, delete_dhcp_options_list, add_port_groups_list, - del_port_groups_list, - port_groups_supported=False): + del_port_groups_list): self._test_mocks_helper(ovn_nb_synchronizer) - core_plugin = ovn_nb_synchronizer.core_plugin ovn_api = ovn_nb_synchronizer.ovn_api - ovn_api.is_port_groups_supported.return_value = port_groups_supported mock.patch.object(impl_idl_ovn, 'get_connection').start() ovn_nb_synchronizer.do_sync() - if not ovn_api.is_port_groups_supported(): - get_security_group_calls = [mock.call(mock.ANY, sg['id']) - for sg in self.security_groups] - self.assertEqual(len(self.security_groups), - core_plugin.get_security_group.call_count) - core_plugin.get_security_group.assert_has_calls( - get_security_group_calls, any_order=True) - - create_address_set_calls = [mock.call(**a) - for a in add_address_set_list] - self.assertEqual( - len(add_address_set_list), - ovn_api.create_address_set.call_count) - ovn_api.create_address_set.assert_has_calls( - create_address_set_calls, any_order=True) - - del_address_set_calls = [mock.call(**d) - for d in del_address_set_list] - self.assertEqual( - len(del_address_set_list), - ovn_api.delete_address_set.call_count) - ovn_api.delete_address_set.assert_has_calls( - del_address_set_calls, any_order=True) - - update_address_set_calls = [mock.call(**u) - for u in update_address_set_list] - self.assertEqual( - len(update_address_set_list), - ovn_api.update_address_set.call_count) - ovn_api.update_address_set.assert_has_calls( - update_address_set_calls, any_order=True) - create_port_groups_calls = [mock.call(**a) for a in add_port_groups_list] self.assertEqual( @@ -740,7 +678,7 @@ class TestOvnNbSyncML2(test_mech_driver.OVNMechanismDriverTestCase): ovn_api.delete_dhcp_options.assert_has_calls( delete_dhcp_options_calls, any_order=True) - def _test_ovn_nb_sync_mode_repair_helper(self, port_groups_supported=True): + def test_ovn_nb_sync_mode_repair(self): create_network_list = [{'net': {'id': 'n2', 'mtu': 1450}, 'ext_ids': {}}] @@ -818,37 +756,11 @@ class TestOvnNbSyncML2(test_mech_driver.OVNMechanismDriverTestCase): update_router_port_list[0].update( {'networks': self.lrport_networks}) - if not port_groups_supported: - add_address_set_list = [ - {'external_ids': {ovn_const.OVN_SG_EXT_ID_KEY: 'sg1'}, - 'name': 'as_ip6_sg1', - 'addresses': ['fd79:e1c:a55::816:eff:eff:ff2']}] - del_address_set_list = [{'name': 'as_ip4_del'}] - update_address_set_list = [ - {'addrs_remove': [], - 'addrs_add': ['10.0.0.4'], - 'name': 'as_ip4_sg2'}, - {'addrs_remove': ['fd79:e1c:a55::816:eff:eff:ff3'], - 'addrs_add': [], - 'name': 'as_ip6_sg2'}] - # If Port Groups are not supported, we don't expect any of those - # to be created/deleted. - add_port_groups_list = [] - del_port_groups_list = [] - else: - add_port_groups_list = [ - {'external_ids': {ovn_const.OVN_SG_EXT_ID_KEY: 'sg2'}, - 'name': 'pg_sg2', - 'acls': []}] - del_port_groups_list = ['pg_unknown_del'] - # If using Port Groups, no Address Set shall be created/updated - # and all the existing ones have to be removed. - add_address_set_list = [] - update_address_set_list = [] - del_address_set_list = [{'name': 'as_ip4_sg1'}, - {'name': 'as_ip4_sg2'}, - {'name': 'as_ip6_sg2'}, - {'name': 'as_ip4_del'}] + add_port_groups_list = [ + {'external_ids': {ovn_const.OVN_SG_EXT_ID_KEY: 'sg2'}, + 'name': 'pg_sg2', + 'acls': []}] + del_port_groups_list = ['pg_unknown_del'] add_subnet_dhcp_options_list = [(self.subnets[2], self.networks[1]), (self.subnets[1], self.networks[0])] @@ -875,22 +787,12 @@ class TestOvnNbSyncML2(test_mech_driver.OVNMechanismDriverTestCase): del_snat_list, add_floating_ip_list, del_floating_ip_list, - add_address_set_list, - del_address_set_list, - update_address_set_list, add_subnet_dhcp_options_list, delete_dhcp_options_list, add_port_groups_list, - del_port_groups_list, - port_groups_supported) + del_port_groups_list) - def test_ovn_nb_sync_mode_repair_no_pgs(self): - self._test_ovn_nb_sync_mode_repair_helper(port_groups_supported=False) - - def test_ovn_nb_sync_mode_repair_pgs(self): - self._test_ovn_nb_sync_mode_repair_helper(port_groups_supported=True) - - def _test_ovn_nb_sync_mode_log_helper(self, port_groups_supported=True): + def test_ovn_nb_sync_mode_log(self): create_network_list = [] create_port_list = [] create_provnet_port_list = [] @@ -907,9 +809,6 @@ class TestOvnNbSyncML2(test_mech_driver.OVNMechanismDriverTestCase): del_snat_list = [] add_floating_ip_list = [] del_floating_ip_list = [] - add_address_set_list = [] - del_address_set_list = [] - update_address_set_list = [] add_subnet_dhcp_options_list = [] delete_dhcp_options_list = [] add_port_groups_list = [] @@ -936,20 +835,10 @@ class TestOvnNbSyncML2(test_mech_driver.OVNMechanismDriverTestCase): del_snat_list, add_floating_ip_list, del_floating_ip_list, - add_address_set_list, - del_address_set_list, - update_address_set_list, add_subnet_dhcp_options_list, delete_dhcp_options_list, add_port_groups_list, - del_port_groups_list, - port_groups_supported) - - def test_ovn_nb_sync_mode_log_pgs(self): - self._test_ovn_nb_sync_mode_log_helper(port_groups_supported=True) - - def test_ovn_nb_sync_mode_log_no_pgs(self): - self._test_ovn_nb_sync_mode_log_helper(port_groups_supported=False) + del_port_groups_list) class TestOvnSbSyncML2(test_mech_driver.OVNMechanismDriverTestCase): diff --git a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py index a08e9cf75f1..4e47564ff2e 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py +++ b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py @@ -159,28 +159,26 @@ class TestOVNMechanismDriver(test_plugin.Ml2PluginV2TestCase): resources.SECURITY_GROUP, events.AFTER_CREATE, {}, security_group=self.fake_sg, context=self.context) external_ids = {ovn_const.OVN_SG_EXT_ID_KEY: self.fake_sg['id']} - ip4_name = ovn_utils.ovn_addrset_name(self.fake_sg['id'], 'ip4') - ip6_name = ovn_utils.ovn_addrset_name(self.fake_sg['id'], 'ip6') - create_address_set_calls = [mock.call(name=name, - external_ids=external_ids) - for name in [ip4_name, ip6_name]] + pg_name = ovn_utils.ovn_port_group_name(self.fake_sg['id']) + + self.nb_ovn.pg_add.assert_called_once_with( + name=pg_name, acls=[], external_ids=external_ids) - self.nb_ovn.create_address_set.assert_has_calls( - create_address_set_calls, any_order=True) mock_bump.assert_called_once_with( mock.ANY, self.fake_sg, ovn_const.TYPE_SECURITY_GROUPS) - def test__delete_security_group(self): + @mock.patch.object(ovn_revision_numbers_db, 'delete_revision') + def test__delete_security_group(self, mock_del_rev): self.mech_driver._delete_security_group( resources.SECURITY_GROUP, events.AFTER_CREATE, {}, security_group_id=self.fake_sg['id'], context=self.context) - ip4_name = ovn_utils.ovn_addrset_name(self.fake_sg['id'], 'ip4') - ip6_name = ovn_utils.ovn_addrset_name(self.fake_sg['id'], 'ip6') - delete_address_set_calls = [mock.call(name=name) - for name in [ip4_name, ip6_name]] + pg_name = ovn_utils.ovn_port_group_name(self.fake_sg['id']) - self.nb_ovn.delete_address_set.assert_has_calls( - delete_address_set_calls, any_order=True) + self.nb_ovn.pg_del.assert_called_once_with( + name=pg_name) + + mock_del_rev.assert_called_once_with( + mock.ANY, self.fake_sg['id'], ovn_const.TYPE_SECURITY_GROUPS) @mock.patch.object(ovn_revision_numbers_db, 'bump_revision') def test__process_sg_rule_notifications_sgr_create(self, mock_bump): @@ -213,88 +211,6 @@ class TestOVNMechanismDriver(test_plugin.Ml2PluginV2TestCase): mock_delrev.assert_called_once_with( mock.ANY, rule['id'], ovn_const.TYPE_SECURITY_GROUP_RULES) - def test_add_acls_no_sec_group(self): - fake_port_no_sg = fakes.FakePort.create_one_port().info() - expected_acls = ovn_acl.drop_all_ip_traffic_for_port(fake_port_no_sg) - acls = ovn_acl.add_acls(self.mech_driver._plugin, - mock.Mock(), - fake_port_no_sg, - {}, {}, self.mech_driver._nb_ovn) - self.assertEqual(expected_acls, acls) - - def test_add_acls_no_sec_group_no_port_security(self): - fake_port_no_sg_no_ps = fakes.FakePort.create_one_port( - attrs={'port_security_enabled': False}).info() - acls = ovn_acl.add_acls(self.mech_driver._plugin, - mock.Mock(), - fake_port_no_sg_no_ps, - {}, {}, self.mech_driver._nb_ovn) - self.assertEqual([], acls) - - def _test_add_acls_with_sec_group_helper(self, native_dhcp=True): - fake_port_sg = fakes.FakePort.create_one_port( - attrs={'security_groups': [self.fake_sg['id']], - 'fixed_ips': [{'subnet_id': self.fake_subnet['id'], - 'ip_address': '10.10.10.20'}]} - ).info() - - expected_acls = [] - expected_acls += ovn_acl.drop_all_ip_traffic_for_port( - fake_port_sg) - expected_acls += ovn_acl.add_acl_dhcp( - fake_port_sg, self.fake_subnet, native_dhcp) - sg_rule_acl = ovn_acl.add_sg_rule_acl_for_port( - fake_port_sg, self.fake_sg_rule, - 'outport == "' + fake_port_sg['id'] + '" ' + - '&& ip4 && ip4.src == 0.0.0.0/0 ' + - '&& tcp && tcp.dst == 22') - expected_acls.append(sg_rule_acl) - - # Test with caches - acls = ovn_acl.add_acls(self.mech_driver._plugin, - mock.Mock(), - fake_port_sg, - self.sg_cache, - self.subnet_cache, - self.mech_driver._nb_ovn) - self.assertEqual(expected_acls, acls) - - # Test without caches - with mock.patch.object(db_base_plugin_v2.NeutronDbPluginV2, - 'get_subnet', return_value=self.fake_subnet), \ - mock.patch.object(securitygroups_db.SecurityGroupDbMixin, - 'get_security_group', - return_value=self.fake_sg): - acls = ovn_acl.add_acls(self.mech_driver._plugin, - mock.Mock(), - fake_port_sg, - {}, {}, self.mech_driver._nb_ovn) - self.assertEqual(expected_acls, acls) - - # Test with security groups disabled - with mock.patch.object(ovn_acl, 'is_sg_enabled', return_value=False): - acls = ovn_acl.add_acls(self.mech_driver._plugin, - mock.Mock(), - fake_port_sg, - self.sg_cache, - self.subnet_cache, - self.mech_driver._nb_ovn) - self.assertEqual([], acls) - - # Test with multiple fixed IPs on the same subnet. - fake_port_sg['fixed_ips'].append({'subnet_id': self.fake_subnet['id'], - 'ip_address': '10.10.10.21'}) - acls = ovn_acl.add_acls(self.mech_driver._plugin, - mock.Mock(), - fake_port_sg, - self.sg_cache, - self.subnet_cache, - self.mech_driver._nb_ovn) - self.assertEqual(expected_acls, acls) - - def test_add_acls_with_sec_group_native_dhcp_enabled(self): - self._test_add_acls_with_sec_group_helper() - def test_port_invalid_binding_profile(self): invalid_binding_profiles = [ {'tag': 0, @@ -694,8 +610,7 @@ class TestOVNMechanismDriver(test_plugin.Ml2PluginV2TestCase): **kwargs): self.assertEqual( 1, self.nb_ovn.create_lswitch_port.call_count) - self.assertEqual(2, self.nb_ovn.add_acl.call_count) - self.nb_ovn.update_address_set.assert_not_called() + self.assertFalse(self.nb_ovn.add_acl.called) def test_create_port_without_security_groups_no_ps(self): kwargs = {'security_groups': [], 'port_security_enabled': False} @@ -709,23 +624,6 @@ class TestOVNMechanismDriver(test_plugin.Ml2PluginV2TestCase): self.assertEqual( 1, self.nb_ovn.create_lswitch_port.call_count) self.nb_ovn.add_acl.assert_not_called() - self.nb_ovn.update_address_set.assert_not_called() - - def _test_create_port_with_security_groups_helper(self, - add_acl_call_count): - with self.network(set_context=True, tenant_id='test') as net1: - with self.subnet(network=net1) as subnet1: - with self.port(subnet=subnet1, - set_context=True, tenant_id='test'): - self.assertEqual( - 1, self.nb_ovn.create_lswitch_port.call_count) - self.assertEqual( - add_acl_call_count, self.nb_ovn.add_acl.call_count) - self.assertEqual( - 1, self.nb_ovn.update_address_set.call_count) - - def test_create_port_with_security_groups_native_dhcp_enabled(self): - self._test_create_port_with_security_groups_helper(7) def test_update_port_changed_security_groups(self): with self.network(set_context=True, tenant_id='test') as net1: @@ -741,29 +639,21 @@ class TestOVNMechanismDriver(test_plugin.Ml2PluginV2TestCase): # Remove the default security group. self.nb_ovn.set_lswitch_port.reset_mock() self.nb_ovn.update_acls.reset_mock() - self.nb_ovn.update_address_set.reset_mock() data = {'port': {'security_groups': []}} self._update('ports', port1['port']['id'], data) self.assertEqual( 1, self.nb_ovn.set_lswitch_port.call_count) - self.assertEqual( - 1, self.nb_ovn.update_acls.call_count) - self.assertEqual( - 1, self.nb_ovn.update_address_set.call_count) + self.assertFalse(self.nb_ovn.update_acls.called) + self.assertTrue(self.nb_ovn.pg_add_ports.called) # Add the default security group. self.nb_ovn.set_lswitch_port.reset_mock() self.nb_ovn.update_acls.reset_mock() - self.nb_ovn.update_address_set.reset_mock() fake_lsp.external_ids.pop(ovn_const.OVN_SG_IDS_EXT_ID_KEY) data = {'port': {'security_groups': [sg_id]}} self._update('ports', port1['port']['id'], data) - self.assertEqual( - 1, self.nb_ovn.set_lswitch_port.call_count) - self.assertEqual( - 1, self.nb_ovn.update_acls.call_count) - self.assertEqual( - 1, self.nb_ovn.update_address_set.call_count) + self.assertFalse(self.nb_ovn.update_acls.called) + self.assertTrue(self.nb_ovn.pg_add_ports.called) def test_update_port_unchanged_security_groups(self): with self.network(set_context=True, tenant_id='test') as net1: @@ -778,26 +668,20 @@ class TestOVNMechanismDriver(test_plugin.Ml2PluginV2TestCase): # Update the port name. self.nb_ovn.set_lswitch_port.reset_mock() self.nb_ovn.update_acls.reset_mock() - self.nb_ovn.update_address_set.reset_mock() data = {'port': {'name': 'rtheis'}} self._update('ports', port1['port']['id'], data) self.assertEqual( 1, self.nb_ovn.set_lswitch_port.call_count) self.nb_ovn.update_acls.assert_not_called() - self.nb_ovn.update_address_set.assert_not_called() # Update the port fixed IPs self.nb_ovn.set_lswitch_port.reset_mock() self.nb_ovn.update_acls.reset_mock() - self.nb_ovn.update_address_set.reset_mock() data = {'port': {'fixed_ips': []}} self._update('ports', port1['port']['id'], data) self.assertEqual( 1, self.nb_ovn.set_lswitch_port.call_count) - self.assertEqual( - 1, self.nb_ovn.update_acls.call_count) - self.assertEqual( - 1, self.nb_ovn.update_address_set.call_count) + self.assertFalse(self.nb_ovn.update_acls.called) def _test_update_port_vip(self, is_vip=True): kwargs = {} @@ -847,33 +731,9 @@ class TestOVNMechanismDriver(test_plugin.Ml2PluginV2TestCase): self.nb_ovn.lookup.return_value = fake_lsp self.nb_ovn.delete_lswitch_port.reset_mock() self.nb_ovn.delete_acl.reset_mock() - self.nb_ovn.update_address_set.reset_mock() self._delete('ports', port1['port']['id']) self.assertEqual( 1, self.nb_ovn.delete_lswitch_port.call_count) - self.assertEqual( - 1, self.nb_ovn.delete_acl.call_count) - self.nb_ovn.update_address_set.assert_not_called() - - def test_delete_port_with_security_groups(self): - with self.network(set_context=True, tenant_id='test') as net1: - with self.subnet(network=net1) as subnet1: - with self.port(subnet=subnet1, - set_context=True, tenant_id='test') as port1: - fake_lsp = ( - fakes.FakeOVNPort.from_neutron_port( - port1['port'])) - self.nb_ovn.lookup.return_value = fake_lsp - self.nb_ovn.delete_lswitch_port.reset_mock() - self.nb_ovn.delete_acl.reset_mock() - self.nb_ovn.update_address_set.reset_mock() - self._delete('ports', port1['port']['id']) - self.assertEqual( - 1, self.nb_ovn.delete_lswitch_port.call_count) - self.assertEqual( - 1, self.nb_ovn.delete_acl.call_count) - self.assertEqual( - 1, self.nb_ovn.update_address_set.call_count) @mock.patch.object(ovn_revision_numbers_db, 'delete_revision') @mock.patch.object(ovn_client.OVNClient, '_delete_port') @@ -2600,8 +2460,7 @@ class TestOVNMechanismDriverSecurityGroup( def _delete_sg_rule(self, rule_id): self._delete('security-group-rules', rule_id) - def test_create_security_group_with_port_group(self): - self.mech_driver._nb_ovn.is_port_groups_supported.return_value = True + def test_create_security_group(self): sg = self._create_sg('sg') expected_pg_name = ovn_utils.ovn_port_group_name(sg['id']) @@ -2613,8 +2472,7 @@ class TestOVNMechanismDriverSecurityGroup( self.mech_driver._nb_ovn.pg_add.assert_has_calls( expected_pg_add_calls) - def test_delete_security_group_with_port_group(self): - self.mech_driver._nb_ovn.is_port_groups_supported.return_value = True + def test_delete_security_group(self): sg = self._create_sg('sg') self._delete('security-groups', sg['id']) @@ -2625,8 +2483,7 @@ class TestOVNMechanismDriverSecurityGroup( self.mech_driver._nb_ovn.pg_del.assert_has_calls( expected_pg_del_calls) - def test_create_port_with_port_group(self): - self.mech_driver._nb_ovn.is_port_groups_supported.return_value = True + def test_create_port(self): with self.network() as n, self.subnet(n): sg = self._create_empty_sg('sg') self._make_port(self.fmt, n['network']['id'], @@ -2646,56 +2503,53 @@ class TestOVNMechanismDriverSecurityGroup( def test_create_port_with_sg_default_rules(self): with self.network() as n, self.subnet(n): + self.mech_driver._nb_ovn.pg_acl_add.reset_mock() sg = self._create_sg('sg') self._make_port(self.fmt, n['network']['id'], security_groups=[sg['id']]) - - # One DHCP rule, one IPv6 rule, one IPv4 rule and - # two default dropping rules. + # egress traffic for ipv4 and ipv6 is allowed by default self.assertEqual( - 5, self.mech_driver._nb_ovn.add_acl.call_count) + 2, self.mech_driver._nb_ovn.pg_acl_add.call_count) def test_create_port_with_empty_sg(self): with self.network() as n, self.subnet(n): + self.mech_driver._nb_ovn.pg_acl_add.reset_mock() sg = self._create_empty_sg('sg') + # Implicit egress rules for ipv4 and ipv6 + self.assertEqual(2, self.mech_driver._nb_ovn.pg_acl_add.call_count) + self.mech_driver._nb_ovn.pg_acl_add.reset_mock() + self.mech_driver._nb_ovn.pg_add.reset_mock() + self.mech_driver._nb_ovn.pg_add_ports.reset_mock() + self._make_port(self.fmt, n['network']['id'], security_groups=[sg['id']]) - # One DHCP rule and two default dropping rules. - self.assertEqual( - 3, self.mech_driver._nb_ovn.add_acl.call_count) - - def test_create_port_with_multi_sgs(self): - with self.network() as n, self.subnet(n): - sg1 = self._create_empty_sg('sg1') - sg2 = self._create_empty_sg('sg2') - self._create_sg_rule(sg1['id'], 'ingress', const.PROTO_NAME_TCP, - port_range_min=22, port_range_max=23) - self._create_sg_rule(sg2['id'], 'egress', const.PROTO_NAME_UDP, - remote_ip_prefix='0.0.0.0/0') - self._make_port(self.fmt, n['network']['id'], - security_groups=[sg1['id'], sg2['id']]) - - # One DHCP rule, one TCP rule, one UDP rule and - # two default dropping rules. - self.assertEqual( - 5, self.mech_driver._nb_ovn.add_acl.call_count) + self.assertFalse(self.mech_driver._nb_ovn.pg_acl_add.called) + self.assertFalse(self.mech_driver._nb_ovn.pg_add.called) + self.assertEqual(1, self.mech_driver._nb_ovn.pg_add_ports.called) def test_create_port_with_multi_sgs_duplicate_rules(self): with self.network() as n, self.subnet(n): + self.mech_driver._nb_ovn.pg_add.reset_mock() sg1 = self._create_empty_sg('sg1') sg2 = self._create_empty_sg('sg2') + self.assertEqual( + 2, self.mech_driver._nb_ovn.pg_add.call_count) + + self.mech_driver._nb_ovn.pg_acl_add.reset_mock() self._create_sg_rule(sg1['id'], 'ingress', const.PROTO_NAME_TCP, port_range_min=22, port_range_max=23, remote_ip_prefix='20.0.0.0/24') self._create_sg_rule(sg2['id'], 'ingress', const.PROTO_NAME_TCP, port_range_min=22, port_range_max=23, remote_ip_prefix='20.0.0.0/24') + self.assertEqual( + 2, self.mech_driver._nb_ovn.pg_acl_add.call_count) + self._make_port(self.fmt, n['network']['id'], security_groups=[sg1['id'], sg2['id']]) - - # One DHCP rule, two TCP rule and two default dropping rules. + # Default drop group, two security groups self.assertEqual( - 5, self.mech_driver._nb_ovn.add_acl.call_count) + 3, self.mech_driver._nb_ovn.pg_add_ports.call_count) @mock.patch('neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb.' 'ovn_client.OVNClient.is_external_ports_supported', @@ -2739,18 +2593,18 @@ class TestOVNMechanismDriverSecurityGroup( p = self._make_port(self.fmt, n['network']['id'], security_groups=[sg1['id']])['port'] - # One DHCP rule, one TCP rule and two default dropping rules. - self.assertEqual( - 4, self.mech_driver._nb_ovn.add_acl.call_count) sg2 = self._create_empty_sg('sg2') self._create_sg_rule(sg2['id'], 'egress', const.PROTO_NAME_UDP, remote_ip_prefix='30.0.0.0/24') data = {'port': {'security_groups': [sg1['id'], sg2['id']]}} + + self.mech_driver._nb_ovn.pg_add_ports.reset_mock() req = self.new_update_request('ports', data, p['id']) req.get_response(self.api) + self.assertEqual( - 1, self.mech_driver._nb_ovn.update_acls.call_count) + 2, self.mech_driver._nb_ovn.pg_add_ports.call_count) def test_update_sg_change_rule(self): with self.network() as n, self.subnet(n): @@ -2758,39 +2612,18 @@ class TestOVNMechanismDriverSecurityGroup( self._make_port(self.fmt, n['network']['id'], security_groups=[sg['id']]) - # One DHCP rule and two default dropping rules. - self.assertEqual( - 3, self.mech_driver._nb_ovn.add_acl.call_count) + self.mech_driver._nb_ovn.pg_acl_add.reset_mock() sg_r = self._create_sg_rule(sg['id'], 'ingress', const.PROTO_NAME_UDP, ethertype=const.IPv6) self.assertEqual( - 1, self.mech_driver._nb_ovn.update_acls.call_count) + 1, self.mech_driver._nb_ovn.pg_acl_add.call_count) + self.mech_driver._nb_ovn.pg_acl_del.reset_mock() self._delete_sg_rule(sg_r['id']) self.assertEqual( - 2, self.mech_driver._nb_ovn.update_acls.call_count) - - def test_update_sg_change_rule_unrelated_port(self): - with self.network() as n, self.subnet(n): - sg1 = self._create_empty_sg('sg1') - sg2 = self._create_empty_sg('sg2') - self._create_sg_rule(sg1['id'], 'ingress', const.PROTO_NAME_TCP, - remote_group_id=sg2['id']) - - self._make_port(self.fmt, n['network']['id'], - security_groups=[sg1['id']]) - # One DHCP rule, one TCP rule and two default dropping rules. - self.assertEqual( - 4, self.mech_driver._nb_ovn.add_acl.call_count) - - sg2_r = self._create_sg_rule(sg2['id'], 'egress', - const.PROTO_NAME_UDP) - self.mech_driver._nb_ovn.update_acls.assert_not_called() - - self._delete_sg_rule(sg2_r['id']) - self.mech_driver._nb_ovn.update_acls.assert_not_called() + 1, self.mech_driver._nb_ovn.pg_acl_del.call_count) def test_update_sg_duplicate_rule(self): with self.network() as n, self.subnet(n): @@ -2801,27 +2634,31 @@ class TestOVNMechanismDriverSecurityGroup( port_range_min=22, port_range_max=23) self._make_port(self.fmt, n['network']['id'], security_groups=[sg1['id'], sg2['id']]) - # One DHCP rule, one UDP rule and two default dropping rules. + # One default drop rule, two SGs self.assertEqual( - 4, self.mech_driver._nb_ovn.add_acl.call_count) + 3, self.mech_driver._nb_ovn.pg_add_ports.call_count) + self.mech_driver._nb_ovn.pg_acl_add.reset_mock() # Add a new duplicate rule to sg2. It's expected to be added. sg2_r = self._create_sg_rule(sg2['id'], 'ingress', const.PROTO_NAME_UDP, port_range_min=22, port_range_max=23) self.assertEqual( - 1, self.mech_driver._nb_ovn.update_acls.call_count) + 1, self.mech_driver._nb_ovn.pg_acl_add.call_count) + self.mech_driver._nb_ovn.pg_acl_del.reset_mock() # Delete the duplicate rule. It's expected to be deleted. self._delete_sg_rule(sg2_r['id']) self.assertEqual( - 2, self.mech_driver._nb_ovn.update_acls.call_count) + 1, self.mech_driver._nb_ovn.pg_acl_del.call_count) def test_update_sg_duplicate_rule_multi_ports(self): with self.network() as n, self.subnet(n): sg1 = self._create_empty_sg('sg1') sg2 = self._create_empty_sg('sg2') sg3 = self._create_empty_sg('sg3') + + self.mech_driver._nb_ovn.pg_acl_add.reset_mock() self._create_sg_rule(sg1['id'], 'ingress', const.PROTO_NAME_UDP, remote_group_id=sg3['id']) @@ -2834,34 +2671,56 @@ class TestOVNMechanismDriverSecurityGroup( security_groups=[sg1['id'], sg2['id']]) self._make_port(self.fmt, n['network']['id'], security_groups=[sg2['id'], sg3['id']]) - # Rules include 5 + 5 + 4 + + # No matter how many ports are there, there are two rules only self.assertEqual( - 14, self.mech_driver._nb_ovn.add_acl.call_count) + 2, self.mech_driver._nb_ovn.pg_acl_add.call_count) # Add a rule to sg1 duplicate with sg2. It's expected to be added. + self.mech_driver._nb_ovn.pg_acl_add.reset_mock() sg1_r = self._create_sg_rule(sg1['id'], 'egress', const.PROTO_NAME_TCP, port_range_min=60, port_range_max=70) self.assertEqual( - 1, self.mech_driver._nb_ovn.update_acls.call_count) + 1, self.mech_driver._nb_ovn.pg_acl_add.call_count) # Add a rule to sg2 duplicate with sg1 but not duplicate with sg3. # It's expected to be added as well. + self.mech_driver._nb_ovn.pg_acl_add.reset_mock() sg2_r = self._create_sg_rule(sg2['id'], 'ingress', const.PROTO_NAME_UDP, remote_group_id=sg3['id']) self.assertEqual( - 2, self.mech_driver._nb_ovn.update_acls.call_count) + 1, self.mech_driver._nb_ovn.pg_acl_add.call_count) # Delete the duplicate rule in sg1. It's expected to be deleted. + self.mech_driver._nb_ovn.pg_acl_del.reset_mock() self._delete_sg_rule(sg1_r['id']) self.assertEqual( - 3, self.mech_driver._nb_ovn.update_acls.call_count) + 1, self.mech_driver._nb_ovn.pg_acl_del.call_count) # Delete the duplicate rule in sg2. It's expected to be deleted. + self.mech_driver._nb_ovn.pg_acl_del.reset_mock() self._delete_sg_rule(sg2_r['id']) self.assertEqual( - 4, self.mech_driver._nb_ovn.update_acls.call_count) + 1, self.mech_driver._nb_ovn.pg_acl_del.call_count) + + def test_delete_port_with_security_groups_port_doesnt_remove_pg(self): + with self.network(set_context=True, tenant_id='test') as net1: + with self.subnet(network=net1): + sg = self._create_sg('sg') + port = self._make_port( + self.fmt, net1['network']['id'], + security_groups=[sg['id']])['port'] + fake_lsp = fakes.FakeOVNPort.from_neutron_port(port) + self.mech_driver._nb_ovn.lookup.return_value = fake_lsp + self.mech_driver._nb_ovn.delete_lswitch_port.reset_mock() + self.mech_driver._nb_ovn.delete_acl.reset_mock() + self._delete('ports', port['id']) + self.assertEqual( + 1, self.mech_driver._nb_ovn.delete_lswitch_port.call_count) + self.assertFalse(self.mech_driver._nb_ovn.pg_del.called) + self.assertFalse(self.mech_driver._nb_ovn.delete_acl.called) class TestOVNMechanismDriverMetadataPort(test_plugin.Ml2PluginV2TestCase):