Support Port Groups in networking-ovn

A new feature has been introduced in core OVN which allows to define
a group of ports and assign ACLs to those. This patch is making use
of the new feature if supported by the underlying OVS version.

As a result we'll be modelling Neutron Security Groups as OVN Port
Groups and we won't be adding one ACL per Security Group Rule per
port. Instead, just add one single ACL per Security Group. This will
also tackle the race conditions that we had for Address Sets as those
will just be used for Remote Security Groups and will be automatically
generated/deleted by core OVN in SB database upon Port Group creation/
deletion.

The major benefit of this patch is that we'll reduce the number of
ACL's dramatically, resulting in a performance leap as discussed at:
https://mail.openvswitch.org/pipermail/ovs-discuss/2018-February/046166.html

This patch will address the migration of old Security Groups being
modelled as Address Sets if the OVN schema supports the feature. This
migration will be performed from the OvnWorker which is holding a lock
on the IDL. This ensures that the migration happens from only one worker
in the cloud and after it's done, all the neutron-server instances are
ready to use Port Groups.

Co-Authored-By: Lucas Alvares Gomes <lucasagomes@gmail.com>
Change-Id: I35d5ec40c666e92b92b9d664e9615c6fecde595a
Signed-off-by: Daniel Alvarez <dalvarez@redhat.com>

networking_ovn/common/acl.py

@@ -57,12 +57,15 @@ def is_sg_enabled():
     return cfg.CONF.SECURITYGROUP.enable_security_group
 
 
-def acl_direction(r, port):
+def acl_direction(r, port=None, port_group=None):
     if r['direction'] == 'ingress':
         portdir = 'outport'
     else:
         portdir = 'inport'
-    return '%s == "%s"' % (portdir, port['id'])
+
+    if port:
+        return '%s == "%s"' % (portdir, port['id'])
+    return '%s == @%s' % (portdir, port_group)
 
 
 def acl_ethertype(r):
@@ -135,6 +138,55 @@ def acl_protocol_and_ports(r, icmp):
     return match
 
 
+def add_acls_for_drop_port_group(pg_name):
+    acl_list = []
+    for direction, p in (('from-lport', 'inport'),
+                         ('to-lport', 'outport')):
+        acl = {"port_group": pg_name,
+               "priority": ovn_const.ACL_PRIORITY_DROP,
+               "action": ovn_const.ACL_ACTION_DROP,
+               "log": False,
+               "name": [],
+               "severity": [],
+               "direction": direction,
+               "match": '%s == @%s && ip' % (p, pg_name)}
+        acl_list.append(acl)
+    return acl_list
+
+
+def add_acls_for_subnet_port_group(ovn, pg_name, subnet, ovn_dhcp=True):
+    # Allow DHCP requests for OVN native DHCP service, while responses are
+    # allowed in ovn-northd.
+    # Allow both DHCP requests and responses to pass for other DHCP services.
+    # We do this even if DHCP isn't enabled for the subnet
+    acl_list = []
+    if not ovn_dhcp:
+        acl = {"port_group": pg_name,
+               "priority": ovn_const.ACL_PRIORITY_ALLOW,
+               "action": ovn_const.ACL_ACTION_ALLOW,
+               "log": False,
+               "name": [],
+               "severity": [],
+               "direction": 'to-lport',
+               "match": ('outport == @%s && ip4 && ip4.src == %s && '
+                         'udp && udp.src == 67 && udp.dst == 68'
+                         ) % (pg_name, subnet['cidr'])}
+        acl_list.append(acl)
+    acl = {"port_group": pg_name,
+           "priority": ovn_const.ACL_PRIORITY_ALLOW,
+           "action": ovn_const.ACL_ACTION_ALLOW,
+           "log": False,
+           "name": [],
+           "severity": [],
+           "direction": 'from-lport',
+           "match": ('inport == @%s && ip4 && '
+                     'ip4.dst == {255.255.255.255, %s} && '
+                     'udp && udp.src == 68 && udp.dst == 67'
+                     ) % (pg_name, subnet['cidr'])}
+    acl_list.append(acl)
+    return acl_list
+
+
 def drop_all_ip_traffic_for_port(port):
     acl_list = []
     for direction, p in (('from-lport', 'inport'),
@@ -173,6 +225,23 @@ def add_sg_rule_acl_for_port(port, r, match):
     return acl
 
 
+def add_sg_rule_acl_for_port_group(port_group, r, match):
+    dir_map = {
+        'ingress': 'to-lport',
+        'egress': 'from-lport',
+    }
+    acl = {"port_group": port_group,
+           "priority": ovn_const.ACL_PRIORITY_ALLOW,
+           "action": ovn_const.ACL_ACTION_ALLOW_RELATED,
+           "log": False,
+           "name": [],
+           "severity": [],
+           "direction": dir_map[r['direction']],
+           "match": match,
+           ovn_const.OVN_SG_RULE_EXT_ID_KEY: r['id']}
+    return acl
+
+
 def add_acl_dhcp(port, subnet, ovn_dhcp=True):
     # Allow DHCP requests for OVN native DHCP service, while responses are
     # allowed in ovn-northd.
@@ -242,13 +311,20 @@ def _get_sg_from_cache(plugin, admin_context, sg_cache, sg_id):
         return sg
 
 
-def acl_remote_group_id(r, ip_version):
+def acl_remote_group_id(r, ip_version, ovn=None):
     if not r['remote_group_id']:
         return ''
 
     src_or_dst = 'src' if r['direction'] == 'ingress' else 'dst'
-    addrset_name = utils.ovn_addrset_name(r['remote_group_id'],
-                                          ip_version)
+    # Check if the remote group is modelled as a Port Group or not as the
+    # name of the Address Set differs.
+    if (ovn and ovn.is_port_groups_supported() and
+            not ovn.get_address_set(r['security_group_id'])):
+        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)
     return ' && %s.%s == $%s' % (ip_version, src_or_dst, addrset_name)
 
 
@@ -275,11 +351,41 @@ def _add_sg_rule_acl_for_port(port, r):
     return add_sg_rule_acl_for_port(port, r, match)
 
 
+def _add_sg_rule_acl_for_port_group(port_group, r, ovn):
+    # Update the match based on which direction this rule is for (ingress
+    # or egress).
+    match = acl_direction(r, port_group=port_group)
+
+    # 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, ovn)
+
+    # 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_group(port_group, r, match)
+
+
 def _acl_columns_name_severity_supported(nb_idl):
     columns = list(nb_idl._tables['ACL'].columns)
     return ('name' in columns) and ('severity' in columns)
 
 
+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)
+        txn.add(ovn.pg_acl_add(**acl))
+
+
 def update_acls_for_security_group(plugin,
                                    admin_context,
                                    ovn,
@@ -287,10 +393,33 @@ def update_acls_for_security_group(plugin,
                                    security_group_rule,
                                    sg_ports_cache=None,
                                    is_add_acl=True):
+
     # Skip ACLs if security groups aren't enabled
     if not is_sg_enabled():
         return
 
+    # 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,
@@ -309,8 +438,6 @@ def update_acls_for_security_group(plugin,
     acl_new_values_dict = {}
     update_port_list = []
 
-    # Check if ACL log name and severity supported or not
-    keep_name_severity = _acl_columns_name_severity_supported(ovn)
     # 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:

networking_ovn/common/constants.py

@@ -45,6 +45,7 @@ OVN_PORT_BINDING_PROFILE_PARAMS = [{'parent_name': six.string_types,
 OVN_ROUTER_PORT_OPTION_KEYS = ['router-port', 'nat-addresses']
 OVN_GATEWAY_CHASSIS_KEY = 'redirect-chassis'
 OVN_GATEWAY_NAT_ADDRESSES_KEY = 'nat-addresses'
+OVN_DROP_PORT_GROUP_NAME = 'neutron_pg_drop'
 
 OVN_PROVNET_PORT_NAME_PREFIX = 'provnet-'
 

networking_ovn/common/maintenance.py

@@ -125,7 +125,7 @@ class DBInconsistenciesPeriodics(object):
             },
             ovn_const.TYPE_SECURITY_GROUPS: {
                 'neutron_get': self._ovn_client._plugin.get_security_group,
-                'ovn_get': self._nb_idl.get_address_set,
+                'ovn_get': self._get_security_group,
                 'ovn_create': self._ovn_client.create_security_group,
                 'ovn_delete': self._ovn_client.delete_security_group,
             },
@@ -146,6 +146,10 @@ class DBInconsistenciesPeriodics(object):
             },
         }
 
+    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

networking_ovn/common/ovn_client.py

@@ -282,41 +282,59 @@ class OVNClient(object):
             # The lport_name *must* be neutron port['id'].  It must match the
             # iface-id set in the Interfaces table of the Open_vSwitch
             # database which nova sets to be the port ID.
-            txn.add(self._nb_idl.create_lswitch_port(
-                    lport_name=port['id'],
-                    lswitch_name=lswitch_name,
-                    addresses=port_info.addresses,
-                    external_ids=external_ids,
-                    parent_name=port_info.parent_name,
-                    tag=port_info.tag,
-                    enabled=port.get('admin_state_up'),
-                    options=port_info.options,
-                    type=port_info.type,
-                    port_security=port_info.port_security,
-                    dhcpv4_options=dhcpv4_options,
-                    dhcpv6_options=dhcpv6_options))
-
-            acls_new = ovn_acl.add_acls(self._plugin, admin_context,
-                                        port, sg_cache, subnet_cache,
-                                        self._nb_idl)
-            for acl in acls_new:
-                txn.add(self._nb_idl.add_acl(**acl))
-
+            port_cmd = txn.add(self._nb_idl.create_lswitch_port(
+                lport_name=port['id'],
+                lswitch_name=lswitch_name,
+                addresses=port_info.addresses,
+                external_ids=external_ids,
+                parent_name=port_info.parent_name,
+                tag=port_info.tag,
+                enabled=port.get('admin_state_up'),
+                options=port_info.options,
+                type=port_info.type,
+                port_security=port_info.port_security,
+                dhcpv4_options=dhcpv4_options,
+                dhcpv6_options=dhcpv6_options))
+
+            # 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 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 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 Port Groups:
+                acls_new = ovn_acl.add_acls(self._plugin, admin_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 self.is_dns_required_for_port(port):
                 self.add_txns_to_sync_port_dns_records(txn, port)
@@ -423,63 +441,84 @@ class OVNClient(object):
             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,
-                                            admin_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 is_allowed_ips_updated:
-                    # 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:
+            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, allow all traffic
+                elif (not new_sg_ids and
+                      not utils.is_port_security_enabled(port)):
+                    self._del_port_from_drop_port_group(port['id'], txn)
+            else:
+                # 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,
+                                                admin_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:
-                            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:
+                            if addresses[ip_version]:
                                 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))
+                                    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 is_allowed_ips_updated:
+                        # 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))
 
             if self.is_dns_required_for_port(port):
                 self.add_txns_to_sync_port_dns_records(
@@ -504,20 +543,22 @@ class OVNClient(object):
         with self._nb_idl.transaction(check_error=True) as txn:
             txn.add(self._nb_idl.delete_lswitch_port(
                 port_id, network_id))
-            txn.add(self._nb_idl.delete_acl(network_id, port_id))
-
-            addresses = utils.sort_ips_by_version(
-                utils.get_ovn_port_addresses(ovn_port))
-            sec_groups = self._get_lsp_backward_compat_sgs(
-                ovn_port, port_object=port_object, 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 not self._nb_idl.is_port_groups_supported():
+                txn.add(self._nb_idl.delete_acl(network_id, port_id))
+
+                addresses = utils.sort_ips_by_version(
+                    utils.get_ovn_port_addresses(ovn_port))
+                sec_groups = self._get_lsp_backward_compat_sgs(
+                    ovn_port, port_object=port_object, 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 port_object and self.is_dns_required_for_port(port_object):
                 self.add_txns_to_remove_port_dns_records(txn, port_object)
@@ -1441,8 +1482,29 @@ class OVNClient(object):
                 self.update_metadata_port(context, network['id'])
 
             self._add_subnet_dhcp_options(subnet, network)
+
+            if self._nb_idl.is_port_groups_supported():
+                with self._nb_idl.transaction(check_error=True) as txn:
+                    self._create_subnet_port_group(subnet, txn)
+
         db_rev.bump_revision(subnet, ovn_const.TYPE_SUBNETS)
 
+    def _create_subnet_port_group(self, subnet, txn):
+        pg_name = utils.ovn_port_group_name(subnet['id'])
+        if self._nb_idl.get_port_group(pg_name):
+            return
+        # Create subnet Port Group.
+        txn.add(self._nb_idl.pg_add(pg_name, acls=[]))
+        # Add ACLs to this Port Group.
+        acls = ovn_acl.add_acls_for_subnet_port_group(
+            self._nb_idl, pg_name, subnet)
+        for acl in acls:
+            txn.add(self._nb_idl.pg_acl_add(**acl))
+
+    def _delete_subnet_port_group(self, subnet_id, txn):
+        pg_name = utils.ovn_port_group_name(subnet_id)
+        txn.add(self._nb_idl.pg_del(pg_name, if_exists=True))
+
     def update_subnet(self, subnet, network):
         ovn_subnet = self._nb_idl.get_subnet_dhcp_options(
             subnet['id'])['subnet']
@@ -1457,8 +1519,12 @@ class OVNClient(object):
             txn.add(check_rev_cmd)
             if subnet['enable_dhcp'] and not ovn_subnet:
                 self._enable_subnet_dhcp_options(subnet, network, txn)
+                if self._nb_idl.is_port_groups_supported():
+                    self._create_subnet_port_group(subnet, txn)
             elif not subnet['enable_dhcp'] and ovn_subnet:
                 self._remove_subnet_dhcp_options(subnet['id'], txn)
+                if self._nb_idl.is_port_groups_supported():
+                    self._delete_subnet_port_group(subnet, txn)
             elif subnet['enable_dhcp'] and ovn_subnet:
                 self._update_subnet_dhcp_options(subnet, network, txn)
 
@@ -1468,22 +1534,69 @@ class OVNClient(object):
     def delete_subnet(self, subnet_id):
         with self._nb_idl.transaction(check_error=True) as txn:
             self._remove_subnet_dhcp_options(subnet_id, txn)
+            if self._nb_idl.is_port_groups_supported():
+                self._delete_subnet_port_group(subnet_id, txn)
         db_rev.delete_revision(subnet_id, ovn_const.TYPE_FLOATINGIPS)
 
     def create_security_group(self, 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:
-            for ip_version in ('ip4', 'ip6'):
-                name = utils.ovn_addrset_name(security_group['id'], ip_version)
-                ext_ids = {ovn_const.OVN_SG_EXT_ID_KEY: security_group['id']}
-                txn.add(self._nb_idl.create_address_set(
-                    name=name, external_ids=ext_ids))
+            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))
         db_rev.bump_revision(security_group, ovn_const.TYPE_SECURITY_GROUPS)
 
+    def create_default_drop_port_group(self, ports=None):
+        pg_name = ovn_const.OVN_DROP_PORT_GROUP_NAME
+        with self._nb_idl.transaction(check_error=True) as txn:
+            if not self._nb_idl.get_port_group(pg_name):
+                # If drop Port Group doesn't exist yet, create it.
+                txn.add(self._nb_idl.pg_add(pg_name, acls=[]))
+                # Add ACLs to this Port Group so that all traffic is dropped.
+                acls = ovn_acl.add_acls_for_drop_port_group(pg_name)
+                for acl in acls:
+                    txn.add(self._nb_idl.pg_acl_add(**acl))
+
+            if ports:
+                ports_ids = [port['id'] for port in ports]
+                # Add the ports to the default Port Group
+                txn.add(self._nb_idl.pg_add_ports(pg_name, ports_ids))
+
+    def _add_port_to_drop_port_group(self, port, txn):
+        self.create_default_drop_port_group()
+        txn.add(self._nb_idl.pg_add_ports(ovn_const.OVN_DROP_PORT_GROUP_NAME,
+                port))
+
+    def _del_port_from_drop_port_group(self, port, txn):
+        pg_name = ovn_const.OVN_DROP_PORT_GROUP_NAME
+        if self._nb_idl.get_port_group(pg_name):
+            txn.add(self._nb_idl.pg_del_ports(pg_name, port))
+
     def delete_security_group(self, security_group_id):
         with self._nb_idl.transaction(check_error=True) as txn:
-            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))
+            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))
         db_rev.delete_revision(security_group_id,
                                ovn_const.TYPE_SECURITY_GROUPS)
 

networking_ovn/common/utils.py

@@ -80,6 +80,21 @@ def ovn_addrset_name(sg_id, ip_version):
     return ('as-%s-%s' % (ip_version, sg_id)).replace('-', '_')
 
 
+def ovn_pg_addrset_name(sg_id, ip_version):
+    # The name of the address set for the given security group id modelled as a
+    # Port Group and ip version. The format is:
+    #   pg-<security group uuid>-<ip version>
+    # with all '-' replaced with '_'. This replacement is necessary
+    # because OVN doesn't support '-' in an address set name.
+    return ('pg-%s-%s' % (sg_id, ip_version)).replace('-', '_')
+
+
+def ovn_port_group_name(sg_id):
+    # The name of the port group for the given security group id.
+    # The format is: pg-<security group uuid>.
+    return ('pg-%s' % sg_id).replace('-', '_')
+
+
 def is_network_device_port(port):
     return port.get('device_owner', '').startswith(
         const.DEVICE_OWNER_PREFIXES)

networking_ovn/ovn_db_sync.py

@@ -86,6 +86,8 @@ class OvnNbSynchronizer(OvnDbSynchronizer):
         LOG.debug("Starting OVN-Northbound DB sync process")
 
         ctx = context.get_admin_context()
+        self.migrate_to_port_groups(ctx)
+
         self.sync_address_sets(ctx)
         self.sync_networks_ports_and_dhcp_opts(ctx)
         self.sync_port_dns_records(ctx)
@@ -936,6 +938,111 @@ class OvnNbSynchronizer(OvnDbSynchronizer):
                 txn.add(self.ovn_api.dns_set_records(ls_dns_record.uuid,
                                                      **dns_records))
 
+    def _delete_address_sets(self, ctx):
+        with self.ovn_api.transaction(check_error=True) as txn:
+            for sg in self.core_plugin.get_security_groups(ctx):
+                for ip_version in ['ip4', 'ip6']:
+                    txn.add(self.ovn_api.delete_address_set(
+                        utils.ovn_addrset_name(sg['id'], ip_version)))
+
+    def _delete_acls_from_lswitches(self, ctx):
+        with self.ovn_api.transaction(check_error=True) as txn:
+            for net in self.core_plugin.get_networks(ctx):
+                # Calling acl_del from ovsdbapp with no ACL will delete
+                # 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 = 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_subnets_port_group(self, ctx, db_ports):
+        filters = {'enable_dhcp': [1]}
+        db_subnets = {}
+        for subnet in self.core_plugin.get_subnets(ctx, filters=filters):
+            if subnet['ip_version'] == constants.IP_VERSION_6 and (
+                    subnet.get('ipv6_address_mode') == constants.IPV6_SLAAC):
+                continue
+            db_subnets[subnet['id']] = subnet
+
+        with self.ovn_api.transaction(check_error=True) as txn:
+            for subnet_id, subnet in db_subnets.items():
+                pg_name = utils.ovn_port_group_name(subnet_id)
+                if not self.ovn_api.get_port_group(pg_name):
+                    # If subnet Port Group doesn't exist yet, create it.
+                    txn.add(self.ovn_api.pg_add(pg_name, acls=[]))
+                    # Add ACLs to this Port Group.
+                    acls = acl_utils.add_acls_for_subnet_port_group(
+                        self.ovn_api, pg_name, subnet)
+                    for acl in acls:
+                        txn.add(self.ovn_api.pg_acl_add(**acl))
+            # Add ports to the relevant subnet Port Groups
+            for port in db_ports:
+                for ip in port['fixed_ips']:
+                    if ip['subnet_id'] in db_subnets:
+                        txn.add(self.ovn_api.pg_add_ports(
+                            utils.ovn_port_group_name(ip['subnet_id']),
+                                port['id']))
+
+    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:
+            for sg in self.core_plugin.get_security_groups(ctx):
+                pg_name = utils.ovn_port_group_name(sg['id'])
+                if self.ovn_api.get_port_group(pg_name):
+                    continue
+                ext_ids = {const.OVN_SG_EXT_ID_KEY: sg['id']}
+                txn.add(self.ovn_api.pg_add(
+                    name=pg_name, acls=[], external_ids=ext_ids))
+                acl_utils.add_acls_for_sg_port_group(self.ovn_api, sg, txn)
+            for port in db_ports:
+                for sg in port['security_groups']:
+                    txn.add(self.ovn_api.pg_add_ports(
+                        utils.ovn_port_group_name(sg), port['id']))
+
+    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 subnet to allow DHCP traffic.
+        #    and add the relevant ports to them.
+        # 3. Create a Port Group for every existing Neutron Security Group and
+        #    add all its Security Group Rules as ACLs to that Port Group.
+        # 4. Delete all existing Address Sets in NorthBound database which
+        #    correspond to a Neutron Security Group.
+        # 5. Delete all the ACLs in every Logical Switch (Neutron network).
+        if not self.ovn_api.is_port_groups_supported():
+            return
+
+        LOG.debug('Port Groups Migration task started')
+
+        # Ignore the floating ip ports with device_owner set to
+        # constants.DEVICE_OWNER_FLOATINGIP
+        db_ports = [port for port in
+                    self.core_plugin.get_ports(ctx) if not
+                    utils.is_lsp_ignored(port) and not
+                    utils.is_lsp_trusted(port) and
+                    utils.is_port_security_enabled(port)]
+
+        self._create_default_drop_port_group(db_ports)
+        self._create_subnets_port_group(ctx, db_ports)
+        self._create_sg_port_groups_and_acls(ctx, db_ports)
+        self._delete_address_sets(ctx)
+        self._delete_acls_from_lswitches(ctx)
+
+        LOG.debug('Port Groups Migration task finished')
+
 
 class OvnSbSynchronizer(OvnDbSynchronizer):
     """Synchronizer class for SB."""

networking_ovn/ovsdb/impl_idl_ovn.py

@@ -663,6 +663,17 @@ 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)
+
+        for pg in self._tables['Port_Group'].rows.values():
+            if pg.name == pg_name:
+                return pg
+
 
 class OvsdbSbOvnIdl(sb_impl_idl.OvnSbApiIdlImpl, Backend):
     def __init__(self, connection):

networking_ovn/tests/functional/test_maintenance.py

@@ -148,10 +148,15 @@ class _TestMaintenanceHelper(base.TestOVNFunctionalBase):
         return self.deserialize(self.fmt, res)['security_group']
 
     def _find_security_group_row_by_id(self, sg_id):
-        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
+        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
 
     def _create_security_group_rule(self, sg_id):
         data = {'security_group_rule': {'security_group_id': sg_id,

networking_ovn/tests/functional/test_ovn_db_resources.py

@@ -720,7 +720,24 @@ class TestNBDbResources(base.TestOVNFunctionalBase):
         # empty.
         self._verify_dhcp_option_row_for_port(p1['id'], {})
 
-    def _verify_port_acls(self, port_id, expected_acls):
+
+class TestPortSecurity(base.TestOVNFunctionalBase):
+
+    def _get_port_related_acls(self, port_id):
+        ovn_port = self.nb_api.lookup('Logical_Switch_Port', port_id)
+        port_acls = []
+        for pg in self.nb_api.tables['Port_Group'].rows.values():
+            for p in pg.ports:
+                if ovn_port.uuid != p.uuid:
+                    continue
+                for a in pg.acls:
+                    port_acls.append({'match': a.match,
+                                      'action': a.action,
+                                      'priority': a.priority,
+                                      'direction': a.direction})
+        return port_acls
+
+    def _get_port_related_acls_port_group_not_supported(self, port_id):
         port_acls = []
         for acl in self.nb_api.tables['ACL'].rows.values():
             ext_ids = getattr(acl, 'external_ids', {})
@@ -729,10 +746,19 @@ class TestNBDbResources(base.TestOVNFunctionalBase):
                                   'action': acl.action,
                                   'priority': acl.priority,
                                   'direction': acl.direction})
+        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)
         self.assertItemsEqual(expected_acls, port_acls)
 
-    def test_port_port_security(self):
+    @mock.patch('networking_ovn.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']
@@ -816,6 +842,88 @@ class TestNBDbResources(base.TestOVNFunctionalBase):
         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']
+        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('-', '_')
+        pg_name = utils.ovn_port_group_name(sg_id)
+        expected_acls_with_sg_ps_enabled = [
+            {'match': 'inport == @neutron_pg_drop && ip',
+             'action': 'drop',
+             'priority': 1001,
+             'direction': 'from-lport'},
+            {'match': 'outport == @neutron_pg_drop && ip',
+             'action': 'drop',
+             'priority': 1001,
+             'direction': 'to-lport'},
+            {'match': 'inport == @' + pg_name + ' && ip6',
+             'action': 'allow-related',
+             'priority': 1002,
+             'direction': 'from-lport'},
+            {'match': 'inport == @' + pg_name + ' && ip4',
+             'action': 'allow-related',
+             'priority': 1002,
+             'direction': 'from-lport'},
+            {'match': 'outport == @' + pg_name + ' && ip4 && '
+                      'ip4.src == $' + pg_name + '_ip4',
+             'action': 'allow-related',
+             'priority': 1002,
+             'direction': 'to-lport'},
+            {'match': 'outport == @' + pg_name + ' && ip6 && '
+                      'ip6.src == $' + pg_name + '_ip6',
+             '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 == @neutron_pg_drop && ip',
+             'action': 'drop',
+             'priority': 1001,
+             'direction': 'from-lport'},
+            {'match': 'outport == @neutron_pg_drop && 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)
+
 
 class TestDNSRecords(base.TestOVNFunctionalBase):
     _extension_drivers = ['port_security', 'dns']

networking_ovn/tests/functional/test_ovn_db_sync.py

@@ -12,6 +12,8 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import mock
+
 from neutron.services.segments import db as segments_db
 from neutron.tests.unit.api import test_extensions
 from neutron.tests.unit.extensions import test_extraroute
@@ -32,6 +34,10 @@ from networking_ovn import ovn_db_sync
 from networking_ovn.tests.functional import base
 
 
+# TODO(lucasagomes): Write functional tests for the ovn_db_sync.py script
+#                    using Port Groups
+@mock.patch('networking_ovn.ovsdb.impl_idl_ovn.OvsdbNbOvnIdl.'
+            'is_port_groups_supported', lambda *args: False)
 class TestOvnNbSync(base.TestOVNFunctionalBase):
 
     _extension_drivers = ['port_security', 'dns']

networking_ovn/tests/unit/common/test_maintenance.py

@@ -145,6 +145,7 @@ class TestDBInconsistenciesPeriodics(db_base.DBTestCase,
 
         if revision_number < 0:
             self.fake_ovn_client._nb_idl.get_address_set.return_value = None
+            self.fake_ovn_client._nb_idl.get_port_group.return_value = None
         else:
             self.fake_ovn_client._nb_idl.get_address_set.return_value = (
                 mock.sentinel.AddressSet)

networking_ovn/tests/unit/fakes.py

@@ -36,6 +36,7 @@ class FakeOvsdbNbOvnIdl(object):
         self.acl_table = FakeOvsdbTable.create_one_ovsdb_table()
         self.dhcp_options_table = FakeOvsdbTable.create_one_ovsdb_table()
         self.nat_table = FakeOvsdbTable.create_one_ovsdb_table()
+        self.port_group_table = FakeOvsdbTable.create_one_ovsdb_table()
         self._tables = {}
         self._tables['Logical_Switch'] = self.lswitch_table
         self._tables['Logical_Switch_Port'] = self.lsp_table
@@ -47,6 +48,7 @@ class FakeOvsdbNbOvnIdl(object):
         self._tables['Address_Set'] = self.addrset_table
         self._tables['DHCP_Options'] = self.dhcp_options_table
         self._tables['NAT'] = self.nat_table
+        self._tables['Port_Group'] = self.port_group_table
         self.transaction = mock.MagicMock()
         self.ls_add = mock.Mock()
         self.set_lswitch_ext_ids = mock.Mock()
@@ -118,6 +120,19 @@ 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()
+        self.pg_add = mock.Mock()
+        self.get_port_group = mock.Mock()
+        self.pg_add_ports = mock.Mock()
+        self.pg_del_ports = mock.Mock()
 
 
 class FakeOvsdbSbOvnIdl(object):

networking_ovn/tests/unit/ml2/test_mech_driver.py

@@ -2018,6 +2018,50 @@ 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
+        sg = self._create_sg('sg')
+
+        expected_pg_name = ovn_utils.ovn_port_group_name(sg['id'])
+        expected_pg_add_calls = [
+            mock.call(acls=[],
+                      external_ids={'neutron:security_group_id': sg['id']},
+                      name=expected_pg_name),
+        ]
+        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
+        sg = self._create_sg('sg')
+        self._delete('security-groups', sg['id'])
+
+        expected_pg_name = ovn_utils.ovn_port_group_name(sg['id'])
+        expected_pg_del_calls = [
+            mock.call(name=expected_pg_name),
+        ]
+        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
+        with self.network() as n, self.subnet(n):
+            sg = self._create_empty_sg('sg')
+            self._make_port(self.fmt, n['network']['id'],
+                            security_groups=[sg['id']])
+
+            # Assert the port has been added to the right security groups
+            expected_pg_name = ovn_utils.ovn_port_group_name(sg['id'])
+            expected_pg_add_ports_calls = [
+                mock.call('neutron_pg_drop', mock.ANY),
+                mock.call(expected_pg_name, mock.ANY)
+            ]
+            self.mech_driver._nb_ovn.pg_add_ports.assert_has_calls(
+                expected_pg_add_ports_calls)
+
+            # Assert add_acl() is not used anymore
+            self.assertFalse(self.mech_driver._nb_ovn.add_acl.called)
+
     def test_create_port_with_sg_default_rules(self):
         with self.network() as n, self.subnet(n):
             sg = self._create_sg('sg')