Consider logging options when using OVNdbsync

Previously, OVN db sync would erase an ACL if any unexpected property
appeared on it and not recreate it again. This happened because of the
order of deletion and creation of the ACLS: the new ACL was first
created and then deleted just the moment after that. This meant that
even crucial ACLs like the ones bounded to the pg_drop port group, which
are used to reject all the traffic by default on ML2/OVN environments,
would dissapear. The order of the ACL deletion and creation has been
inverted to avoid this.

Furthermore, security group logging was not supported on the
ovn_db_sync script, which would also cause the logging parameters to
dissapear. Now, the logging options are considered when doing a sync.

Conflicts:
  neutron/common/ovn/acl.py

Closes-Bug: #2107925
Change-Id: I00fa8332fdebc958ddb8f28c638670c75a70e0c5
Signed-off-by: Elvira Garcia <[email protected]>
(cherry picked from commit 1cf5b6d)

Author: elvgarrui

neutron/common/ovn/acl.py

@@ -148,7 +148,8 @@ def add_acls_for_drop_port_group(pg_name):
                "name": [],
                "severity": [],
                "direction": direction,
-               "match": '{} == @{} && ip'.format(p, pg_name)}
+               "match": '{} == @{} && ip'.format(p, pg_name),
+               "meter": []}
         acl_list.append(acl)
     return acl_list
 
@@ -167,6 +168,7 @@ def drop_all_ip_traffic_for_port(port):
                "severity": [],
                "direction": direction,
                "match": '{} == "{}" && ip'.format(p, port['id']),
+               "meter": [],
                "external_ids": {'neutron:lport': port['id']}}
         acl_list.append(acl)
     return acl_list
@@ -187,6 +189,7 @@ def add_sg_rule_acl_for_port_group(port_group, r, stateful, match):
            "severity": [],
            "direction": dir_map[r['direction']],
            "match": match,
+           "meter": [],
            ovn_const.OVN_SG_RULE_EXT_ID_KEY: r['id']}
     return acl
 

neutron/common/ovn/constants.py

@@ -259,7 +259,7 @@
 
 ACL_EXPECTED_COLUMNS_NBDB = (
     'external_ids', 'direction', 'log', 'priority',
-    'name', 'action', 'severity', 'match')
+    'name', 'action', 'severity', 'match', 'meter')
 
 # Resource types
 TYPE_NETWORKS = 'networks'

neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py

@@ -35,6 +35,7 @@
 from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb.extensions import qos \
     as ovn_qos
 from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb import ovn_client
+from neutron.services.logapi.drivers.ovn import driver as log_driver
 from neutron.services.segments import db as segments_db
 
 
@@ -82,6 +83,18 @@ def __init__(self, core_plugin, ovn_api, sb_ovn, mode, ovn_driver):
             self.segments_plugin = (
                 manager.NeutronManager.load_class_for_provider(
                     'neutron.service_plugins', 'segments')())
+        self.log_plugin = directory.get_plugin(plugin_constants.LOG_API)
+        if not self.log_plugin:
+            self.log_plugin = (
+                manager.NeutronManager.load_class_for_provider(
+                    'neutron.service_plugins', 'log')())
+            directory.add_plugin(plugin_constants.LOG_API, self.log_plugin)
+        for driver in self.log_plugin.driver_manager.drivers:
+            if driver.name == "ovn":
+                self.ovn_log_driver = driver
+        if not hasattr(self, 'ovn_log_driver'):
+            self.ovn_log_driver = log_driver.OVNDriver()
+            self.log_plugin.driver_manager.register_driver(self.ovn_log_driver)
 
     def stop(self):
         if utils.is_ovn_l3(self.l3_plugin):
@@ -233,6 +246,9 @@ def sync_port_groups(self, ctx):
 
     def _get_acls_from_port_groups(self):
         ovn_acls = []
+        # Options and label columns are only present for OVN >= 22.03.
+        # Furthermore label is a randint so it cannot be compared with any
+        # expected neutron value. They are added later on the ACL addition.
         acl_columns = (self.ovn_api._tables['ACL'].columns.keys() &
                        set(ovn_const.ACL_EXPECTED_COLUMNS_NBDB))
         acl_columns.discard('external_ids')
@@ -244,7 +260,16 @@ def _get_acls_from_port_groups(self):
                 acl_string['port_group'] = pg.name
                 if id_key in acl.external_ids:
                     acl_string[id_key] = acl.external_ids[id_key]
+                # This properties are present as lists of one item,
+                # converting them to string.
+                if acl_string['name']:
+                    acl_string['name'] = acl_string['name'][0]
+                if acl_string['meter']:
+                    acl_string['meter'] = acl_string['meter'][0]
+                if acl_string['severity']:
+                    acl_string['severity'] = acl_string['severity'][0]
                 ovn_acls.append(acl_string)
+
         return ovn_acls
 
     def sync_acls(self, ctx):
@@ -274,6 +299,10 @@ def sync_acls(self, ctx):
         neutron_default_acls = acl_utils.add_acls_for_drop_port_group(
             ovn_const.OVN_DROP_PORT_GROUP_NAME)
 
+        # Add logging options
+        self.ovn_log_driver.add_logging_options_to_acls(neutron_acls, ctx)
+        self.ovn_log_driver.add_logging_options_to_acls(neutron_default_acls,
+                                                        ctx)
         ovn_acls = self._get_acls_from_port_groups()
         # Sort the acls in the ovn database according to the security
         # group rule id for easy comparison in the future.
@@ -304,14 +333,24 @@ def sync_acls(self, ctx):
                 o_index += 1
             elif n_id == o_id:
                 if any(item not in na.items() for item in oa.items()):
+                    for item in oa.items():
+                        if item not in na.items():
+                            LOG.warning('Property %(item)s from OVN ACL not '
+                                        'found in Neutron ACL: %(n_acl)s',
+                                        {'item': item,
+                                         'n_acl': na})
                     add_acls.append(na)
                     remove_acls.append(oa)
                 n_index += 1
                 o_index += 1
             elif n_id > o_id:
+                LOG.warning('ACL should not be present in OVN, removing'
+                            '%(acl)s', {'acl': oa})
                 remove_acls.append(oa)
                 o_index += 1
             else:
+                LOG.warning('ACL should be present in OVN but is not, adding:'
+                            '%(acl)s', {'acl': na})
                 add_acls.append(na)
                 n_index += 1
 
@@ -351,15 +390,41 @@ def get_num_acls(ovn_acls):
         if (self.mode == ovn_const.OVN_DB_SYNC_MODE_REPAIR and
                 (num_acls_to_add or num_acls_to_remove)):
             one_time_pg_resync = True
+            with self.ovn_api.transaction(check_error=True) as txn:
+                for aclr in ovn_acls:
+                    LOG.warning('ACLs found in OVN NB DB but not in '
+                                'Neutron for port group %s',
+                                aclr['port_group'])
+                    txn.add(self.ovn_api.pg_acl_del(aclr['port_group'],
+                                                    aclr['direction'],
+                                                    aclr['priority'],
+                                                    aclr['match']))
+                for aclr in ovn_acls_from_ls:
+                    # Remove all the ACLs from any Logical Switch if they have
+                    # any. Elements are (lswitch_name, list_of_acls).
+                    if len(aclr[1]) > 0:
+                        LOG.warning('Removing ACLs from OVN from Logical '
+                                    'Switch %s', aclr[0])
+                        txn.add(self.ovn_api.acl_del(aclr[0]))
             while True:
                 try:
                     with self.ovn_api.transaction(check_error=True) as txn:
                         for acla in neutron_acls:
                             LOG.warning('ACL found in Neutron but not in '
                                         'OVN NB DB for port group %s',
                                         acla['port_group'])
-                            txn.add(self.ovn_api.pg_acl_add(
-                                **acla, may_exist=True))
+                            acl = txn.add(self.ovn_api.pg_acl_add(**acla,
+                                                               may_exist=True))
+                            # We need to do this now since label should be
+                            # random and not 0. We can use options as a way
+                            # to see if label is supported or not.
+                            if acla.get('log'):
+                                self.ovn_log_driver.add_label_related(acla,
+                                                                      ctx)
+                                txn.add(self.ovn_api.db_set('ACL', acl,
+                                    label=acla['label'],
+                                    options=acla['options']))
+
                 except idlutils.RowNotFound as row_err:
                     if row_err.msg.startswith("Cannot find Port_Group"):
                         if one_time_pg_resync:
@@ -377,23 +442,6 @@ def get_num_acls(ovn_acls):
                         raise
                 break
 
-            with self.ovn_api.transaction(check_error=True) as txn:
-                for aclr in ovn_acls:
-                    LOG.warning('ACLs found in OVN NB DB but not in '
-                                'Neutron for port group %s',
-                                aclr['port_group'])
-                    txn.add(self.ovn_api.pg_acl_del(aclr['port_group'],
-                                                    aclr['direction'],
-                                                    aclr['priority'],
-                                                    aclr['match']))
-                for aclr in ovn_acls_from_ls:
-                    # Remove all the ACLs from any Logical Switch if they have
-                    # any. Elements are (lswitch_name, list_of_acls).
-                    if len(aclr[1]) > 0:
-                        LOG.warning('Removing ACLs from OVN from Logical '
-                                    'Switch %s', aclr[0])
-                        txn.add(self.ovn_api.acl_del(aclr[0]))
-
         LOG.debug('OVN-NB Sync ACLs completed @ %s', str(datetime.now()))
 
     def _calculate_routes_differences(self, ovn_routes, db_routes):

neutron/services/logapi/drivers/ovn/driver.py

@@ -409,6 +409,80 @@ def resource_update(self, context, log_objs):
         with self.ovn_nb.transaction(check_error=True) as ovn_txn:
             self._update_log_objs(context, ovn_txn, log_objs)
 
+    def add_logging_options_to_acls(self, neutron_acls, context):
+        log_objs = self._get_logs(context)
+        for log_obj in log_objs:
+            pgs = self._pgs_from_log_obj(context, log_obj)
+            actions_enabled = self._acl_actions_enabled(log_obj)
+            self._set_neutron_acls_log(pgs, context, actions_enabled,
+                                       utils.ovn_name(log_obj.id),
+                                       neutron_acls)
+
+    # This function is a version of set_acls_log meant to change neutron
+    # defined acls, mostly thought for ovndbsync consistency check.
+    def _set_neutron_acls_log(self, pgs, context, actions_enabled, log_name,
+                              neutron_acls):
+        acl_changes, acl_visits = 0, 0
+        for pg in pgs:
+            meter_name = self.meter_name
+            if pg['name'] != ovn_const.OVN_DROP_PORT_GROUP_NAME:
+                sg = sg_obj.SecurityGroup.get_sg_by_id(context,
+                        pg['external_ids'][ovn_const.OVN_SG_EXT_ID_KEY])
+                if not sg:
+                    LOG.warning("Port Group %s is missing a corresponding "
+                                "security group, skipping its network log "
+                                "setting...", pg["name"])
+                    continue
+                if not sg.stateful:
+                    meter_name = meter_name + ("_stateless")
+            # We need to get the OVN ACL because UUID is not listed as a
+            # property on neutron defined ACLs (and it shouldn't), so we need
+            # to check which ACL is that UUID referring to, using match as
+            # differentiating value.
+            for acl in neutron_acls:
+                acl_visits += 1
+                # skip acls used by a different network log
+                n_acl_name = acl['name']
+                if n_acl_name and n_acl_name != log_name:
+                    continue
+                action = acl['action'] in actions_enabled
+                acl['log'] = action
+                acl['meter'] = meter_name
+                acl['name'] = log_name
+                acl['severity'] = "info"
+                if acl.get('options'):
+                    acl["options"] = {'log-related': "true"}
+                # label is not set because the actual number should not
+                # be compared or taken into account, we only need it to be
+                # different from 0.
+                acl_changes += 1
+        LOG.info("Set %d (out of %d visited) Neutron ACLs for network log %s",
+                 acl_changes, acl_visits, log_name)
+
+    def _get_all_log_pgs(self, ctx):
+        """Get all Port Group names associated to a Log Object.
+
+        :param log_plugin: Currently loaded log_plugging.
+        :param ctx: current running context information
+        """
+        log_objs = self._get_logs(ctx)
+        log_pgs = []
+        for log_obj in log_objs:
+            log_pgs.extend(self._pgs_from_log_obj(ctx, log_obj))
+        return log_pgs
+
+    def add_label_related(self, n_acl, ctx):
+        # Get acls to be able to check if label is present in OVN ACLs and
+        # also check old label value for ACL if it was already present.
+        acls = [acl for pg in self._get_all_log_pgs(ctx) for acl in pg["acls"]]
+        if not acls:
+            return
+        acl = self.ovn_nb.lookup("ACL", acls[0], default=None)
+        if not hasattr(acl, 'label'):
+            return
+        n_acl["label"] = secrets.SystemRandom().randrange(1, MAX_INT_LABEL)
+        n_acl["options"] = {'log-related': 'true'}
+
 
 def register(plugin_driver):
     """Register the driver."""

neutron/tests/functional/base.py

@@ -50,6 +50,7 @@
 from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb import worker
 from neutron.plugins.ml2.drivers import type_geneve  # noqa
 from neutron import service  # noqa
+from neutron.services.logapi.drivers.ovn import driver as log_driver
 from neutron.tests import base
 from neutron.tests.common import base as common_base
 from neutron.tests.common import helpers
@@ -175,6 +176,7 @@ def setUp(self, maintenance_worker=False, service_plugins=None):
         self.addCleanup(exts.PluginAwareExtensionManager.clear_instance)
         self.ovsdb_server_mgr = None
         self._service_plugins = service_plugins
+        log_driver.DRIVER = None
         super().setUp()
         self.test_log_dir = os.path.join(DEFAULT_LOG_DIR, self.id())
         base.setup_test_logging(
@@ -197,6 +199,11 @@ def setUp(self, maintenance_worker=False, service_plugins=None):
                 self.mech_driver.log_driver)
         self.mech_driver.log_driver.plugin_driver = self.mech_driver
         self.mech_driver.log_driver._log_plugin_property = None
+        for driver in self.log_plugin.driver_manager.drivers:
+            if driver.name == "ovn":
+                self.ovn_log_driver = driver
+        if not hasattr(self, 'ovn_log_driver'):
+            self.ovn_log_driver = log_driver.OVNDriver()
         self.ovn_northd_mgr = None
         self.maintenance_worker = maintenance_worker
         mock.patch(