From 6ef9d235d2f9ba764f96d32741fffc28c80076f2 Mon Sep 17 00:00:00 2001 From: Slawek Kaplonski Date: Mon, 4 Jul 2022 11:09:23 +0200 Subject: [PATCH 1/2] Use common wait_until_ha_router_has_state method everywhere In the L3 functional tests framework module there is already helper method called wait_until_ha_router_has_state which should be used to wait for desired HA router's state. This method has proper debug logging added so debugging issues in CI is easier when it's used. It is also decorated with unstable_test decorator to skip tests when router will fail to transition to desired state (see related bug for details). In some tests this method wasn't used so we couldn't benefit from the logging and unstable_test decorator there. Now it should be unifed and used everywhere in the same way. Related-Bug: #1956958 Change-Id: I9d79b123bb20ded327208d84a14d4f8d2e505087 (cherry picked from commit e39011c73396af68fe5002cdbe4c6b3fe8b7cf23) --- neutron/tests/functional/agent/l3/framework.py | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/neutron/tests/functional/agent/l3/framework.py b/neutron/tests/functional/agent/l3/framework.py index 45de9ac8278..c30c44cbf49 100644 --- a/neutron/tests/functional/agent/l3/framework.py +++ b/neutron/tests/functional/agent/l3/framework.py @@ -213,7 +213,7 @@ def assert_num_of_conntrack_rules(n): n, len([line for line in out.strip().split('\n') if line])) if ha: - common_utils.wait_until_true(lambda: router.ha_state == 'primary') + self.wait_until_ha_router_has_state(router, 'primary') with self.assert_max_execution_time(100): assert_num_of_conntrack_rules(0) @@ -334,7 +334,7 @@ def _router_lifecycle(self, enable_ha, ip_version=constants.IP_VERSION_4, router.process() if enable_ha: - common_utils.wait_until_true(lambda: router.ha_state == 'primary') + self.wait_until_ha_router_has_state(router, 'primary') # Keepalived notifies of a state transition when it starts, # not when it ends. Thus, we have to wait until keepalived finishes @@ -669,29 +669,25 @@ def _get_primary_and_backup_routers(self, router1, router2, check_external_device=True): try: - common_utils.wait_until_true( - lambda: router1.ha_state == 'primary') + self.wait_until_ha_router_has_state(router1, 'primary') if check_external_device: common_utils.wait_until_true( lambda: self._check_external_device(router1)) primary_router = router1 backup_router = router2 except common_utils.WaitTimeout: - common_utils.wait_until_true( - lambda: router2.ha_state == 'primary') + self.wait_until_ha_router_has_state(router2, 'primary') if check_external_device: common_utils.wait_until_true( lambda: self._check_external_device(router2)) primary_router = router2 backup_router = router1 - common_utils.wait_until_true( - lambda: primary_router.ha_state == 'primary') + self.wait_until_ha_router_has_state(primary_router, 'primary') if check_external_device: common_utils.wait_until_true( lambda: self._check_external_device(primary_router)) - common_utils.wait_until_true( - lambda: backup_router.ha_state == 'backup') + self.wait_until_ha_router_has_state(backup_router, 'backup') LOG.debug("Found primary router %s and backup router %s", primary_router.router_id, backup_router.router_id) From a70cfffef35f4ad90754b6a9c73766fc3584b2d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elvira=20Garc=C3=ADa?= Date: Thu, 19 Jan 2023 14:48:23 +0100 Subject: [PATCH 2/2] [OVN] Allow logging all traffic related to an ACL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before this patch, we would only get logged the client to server side of the communication. The OVN allow-related ACL option was implemented [0] so as to be able to log also the packets that are going from server to client. This patch implements the addition of that feature in Neutron and needs OVN version 22.03 or updated 21.12. [0] https://patchwork.ozlabs.org/project/ovn/patch/20220201141118.1846390-1-mmichels@redhat.com/ Closes-Bug: #2003706 Change-Id: I72d061c333f53e07f6feedec032e2c0b06a61248 Signed-off-by: Elvira García (cherry picked from commit f7e31b4c0533687622f8f2644c802574e31536f7) --- neutron/services/logapi/drivers/ovn/driver.py | 40 +++++++++++++------ .../logapi/drivers/ovn/test_driver.py | 10 +++++ .../logapi/drivers/ovn/test_driver.py | 5 ++- ...-related-traffic-ovn-96b304ab744de13e.yaml | 6 +++ 4 files changed, 48 insertions(+), 13 deletions(-) create mode 100644 releasenotes/notes/sgl-log-related-traffic-ovn-96b304ab744de13e.yaml diff --git a/neutron/services/logapi/drivers/ovn/driver.py b/neutron/services/logapi/drivers/ovn/driver.py index 98ed3f29ec9..5bea32a1ec2 100644 --- a/neutron/services/logapi/drivers/ovn/driver.py +++ b/neutron/services/logapi/drivers/ovn/driver.py @@ -11,6 +11,7 @@ # under the License. from collections import namedtuple +import random from neutron_lib.api.definitions import portbindings from neutron_lib.callbacks import resources @@ -38,6 +39,7 @@ log_cfg.register_log_driver_opts() +MAX_INT_LABEL = 2**32 SUPPORTED_LOGGING_TYPES = [log_const.SECURITY_GROUP] @@ -169,13 +171,20 @@ def _remove_acls_log(self, pgs, ovn_txn, log_name=None): if log_name: if acl.name and acl.name[0] != log_name: continue + columns = { + 'log': False, + 'meter': [], + 'name': [], + 'severity': [] + } + # TODO(egarciar): There wont be a need to check if label exists + # once minimum version for OVN is >= 22.03 + if hasattr(acl, 'label'): + columns['label'] = 0 + ovn_txn.add(self.ovn_nb.db_remove( + "ACL", acl_uuid, 'options', 'log-related')) ovn_txn.add(self.ovn_nb.db_set( - "ACL", acl_uuid, - ("log", False), - ("meter", []), - ("name", []), - ("severity", []) - )) + "ACL", acl_uuid, *columns.items())) acl_changes += 1 msg = "Cleared %d, Not found %d (out of %d visited) ACLs" if log_name: @@ -191,13 +200,20 @@ def _set_acls_log(self, pgs, ovn_txn, actions_enabled, log_name): # skip acls used by a different network log if acl.name and acl.name[0] != log_name: continue + columns = { + 'log': acl.action in actions_enabled, + 'meter': self.meter_name, + 'name': log_name, + 'severity': "info" + } + # TODO(egarciar): There wont be a need to check if label exists + # once minimum version for OVN is >= 22.03 + if hasattr(acl, "label"): + # Label needs to be an unsigned 32 bit number and not 0. + columns["label"] = random.randrange(1, MAX_INT_LABEL) + columns["options"] = {'log-related': "true"} ovn_txn.add(self.ovn_nb.db_set( - "ACL", acl_uuid, - ("log", acl.action in actions_enabled), - ("meter", self.meter_name), - ("name", log_name), - ("severity", "info") - )) + "ACL", acl_uuid, *columns.items())) acl_changes += 1 LOG.info("Set %d (out of %d visited) ACLs for network log %s", acl_changes, acl_visits, log_name) diff --git a/neutron/tests/functional/services/logapi/drivers/ovn/test_driver.py b/neutron/tests/functional/services/logapi/drivers/ovn/test_driver.py index 03f1d365d62..57c7978b8fc 100644 --- a/neutron/tests/functional/services/logapi/drivers/ovn/test_driver.py +++ b/neutron/tests/functional/services/logapi/drivers/ovn/test_driver.py @@ -151,6 +151,16 @@ def _check_acl_log(self, sgr, is_enabled=True): acl = self._find_security_group_rule_row_by_id(sgr) self.assertIsNotNone(acl) self.assertEqual(is_enabled, acl.log) + if hasattr(acl, "label"): + # Here we compare if there is a name because the log can be + # disabled but disabling a log would not take out the properties + # attached to it. + if acl.name: + self.assertNotEqual(0, acl.label) + self.assertEqual("true", acl.options.get("log-related")) + else: + self.assertEqual(0, acl.label) + self.assertIsNone(acl.options.get("log-related")) return acl def _check_acl_log_drop(self, is_enabled=True): diff --git a/neutron/tests/unit/services/logapi/drivers/ovn/test_driver.py b/neutron/tests/unit/services/logapi/drivers/ovn/test_driver.py index 1afa735c10a..35c2ec5765a 100644 --- a/neutron/tests/unit/services/logapi/drivers/ovn/test_driver.py +++ b/neutron/tests/unit/services/logapi/drivers/ovn/test_driver.py @@ -278,7 +278,10 @@ def test__remove_acls_log(self, m_info): self.assertEqual(len(pg_dict["acls"]), info_args[1]) self.assertEqual(len(pg_dict["acls"]) - 2, info_args[2]) self.assertEqual(len(pg_dict["acls"]), info_args[3]) - self.assertEqual(len(pg_dict["acls"]), self._nb_ovn.db_set.call_count) + self.assertEqual(len(pg_dict["acls"]), + self._nb_ovn.db_set.call_count) + self.assertEqual(len(pg_dict["acls"]), + self._nb_ovn.db_remove.call_count) @mock.patch.object(ovn_driver.LOG, 'info') def test__remove_acls_log_missing_acls(self, m_info): diff --git a/releasenotes/notes/sgl-log-related-traffic-ovn-96b304ab744de13e.yaml b/releasenotes/notes/sgl-log-related-traffic-ovn-96b304ab744de13e.yaml new file mode 100644 index 00000000000..b3ea740aa54 --- /dev/null +++ b/releasenotes/notes/sgl-log-related-traffic-ovn-96b304ab744de13e.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + Neutron can record full connection using log-related feature introduced in + OVN 21.12. + For more info see `bug LP#`