From 7df245ee5bc02065852096086e64d19f4272d0c1 Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Hernandez Date: Tue, 5 Oct 2021 08:50:52 +0000 Subject: [PATCH] Delete log entries when SG or port is deleted NOTE: this patch is an ammend of [1]. When a SG or a port is deleted, the related log entry should be too. A log entry has the following fields: - log.resource_id = SG ID - log.target_id = port ID [1] was deleting all log entries, related or not with the SG ID deleted. This is because "get_logs_bound_sg" returned all log entries, including those ones without any "resource_id" or "target_id". Now this method can return only the log entries related to a port or a SG, excluding those ones without those two parameters populated. Closes-Bug: #1939558 [1]https://review.opendev.org/c/openstack/neutron/+/804237 Conflicts: neutron/tests/unit/services/logapi/test_logging_plugin.py Change-Id: Icb92327a06486e168ce064532d819347e6031cc1 (cherry picked from commit 41f78c678bcc164cd77ed7302bacf086d24ab28c) (cherry picked from commit 2c4ab468ae53533db236809afaf0dd5f593db4dc) (cherry picked from commit d5fd9f035ac38dde75e5cf89a062387d4d5512fc) (cherry picked from commit 7790cecfc8c324780f8e5031bc59cb3d1a3c5f7c) (cherry picked from commit 226367eed1148c6e555dcb8087f11b1f85a8eddc) --- neutron/services/logapi/common/db_api.py | 26 +++--- neutron/services/logapi/common/sg_callback.py | 2 +- neutron/services/logapi/logging_plugin.py | 22 +++-- .../services/logapi/common/test_db_api.py | 93 ++++++++++++------- .../services/logapi/test_logging_plugin.py | 22 ----- 5 files changed, 90 insertions(+), 75 deletions(-) diff --git a/neutron/services/logapi/common/db_api.py b/neutron/services/logapi/common/db_api.py index 2864a14fab9..d01a54b2b69 100644 --- a/neutron/services/logapi/common/db_api.py +++ b/neutron/services/logapi/common/db_api.py @@ -180,7 +180,8 @@ def get_logs_bound_port(context, port_id): return [log for log in logs if is_bound(log)] -def get_logs_bound_sg(context, sg_id, project_id=None): +def get_logs_bound_sg(context, sg_id=None, project_id=None, port_id=None, + exclusive=False): """Return a list of log_resources bound to a security group""" kwargs = { @@ -189,19 +190,22 @@ def get_logs_bound_sg(context, sg_id, project_id=None): if project_id: kwargs['project_id'] = project_id - log_objs = log_object.Log.get_objects( - context, **kwargs) - + log_objs = log_object.Log.get_objects(context, **kwargs) log_resources = [] for log_obj in log_objs: - if log_obj.resource_id == sg_id: - log_resources.append(log_obj) - elif log_obj.target_id: - port = port_objects.Port.get_object( - context, id=log_obj.target_id) - if sg_id in port.security_group_ids: + if sg_id: + if log_obj.resource_id == sg_id: log_resources.append(log_obj) - elif not log_obj.resource_id and not log_obj.target_id: + elif log_obj.target_id: + # NOTE: optimize this method just returning the SGs per port. + port = port_objects.Port.get_object( + context, id=log_obj.target_id) + if sg_id in port.security_group_ids: + log_resources.append(log_obj) + elif (not log_obj.resource_id and not log_obj.target_id and + not exclusive): + log_resources.append(log_obj) + elif port_id and log_obj.target_id and log_obj.target_id == port_id: log_resources.append(log_obj) return log_resources diff --git a/neutron/services/logapi/common/sg_callback.py b/neutron/services/logapi/common/sg_callback.py index f66189734cb..bf3b1fd5a0b 100644 --- a/neutron/services/logapi/common/sg_callback.py +++ b/neutron/services/logapi/common/sg_callback.py @@ -30,7 +30,7 @@ class SecurityGroupRuleCallBack(manager.ResourceCallBackBase): sg_id = kwargs.get('security_group_id') log_resources = db_api.get_logs_bound_sg( - context, sg_id, project_id=context.project_id) + context, sg_id=sg_id, project_id=context.project_id) if log_resources: self.resource_push_api( log_const.RESOURCE_UPDATE, context, log_resources) diff --git a/neutron/services/logapi/logging_plugin.py b/neutron/services/logapi/logging_plugin.py index ac24bdcfeac..c40862758f7 100644 --- a/neutron/services/logapi/logging_plugin.py +++ b/neutron/services/logapi/logging_plugin.py @@ -30,6 +30,7 @@ from neutron.services.logapi.common import validators from neutron.services.logapi.drivers import manager as driver_mgr +@registry.has_registry_receivers class LoggingPlugin(log_ext.LoggingPluginBase): """Implementation of the Neutron logging api plugin.""" @@ -43,23 +44,30 @@ class LoggingPlugin(log_ext.LoggingPluginBase): super(LoggingPlugin, self).__init__() self.driver_manager = driver_mgr.LoggingServiceDriverManager() self.validator_mgr = validators.ResourceValidateRequest.get_instance() - registry.subscribe( - self._clean_security_group_logs, - resources.SECURITY_GROUP, events.AFTER_DELETE) @property def supported_logging_types(self): # supported_logging_types are be dynamically loaded from log_drivers return self.driver_manager.supported_logging_types - def _clean_security_group_logs(self, resource, event, trigger, payload): - context = payload.context.elevated() - sg_id = payload.resource_id + def _clean_logs(self, context, sg_id=None, port_id=None): with db_api.CONTEXT_WRITER.using(context): - sg_logs = log_db_api.get_logs_bound_sg(context, sg_id) + sg_logs = log_db_api.get_logs_bound_sg( + context, sg_id=sg_id, port_id=port_id, exclusive=True) for log in sg_logs: self.delete_log(context, log['id']) + @registry.receives(resources.SECURITY_GROUP, [events.AFTER_DELETE]) + def _clean_logs_by_resource_id(self, resource, event, trigger, payload): + # log.resource_id == SG + self._clean_logs(payload.context.elevated(), sg_id=payload.resource_id) + + @registry.receives(resources.PORT, [events.AFTER_DELETE]) + def _clean_logs_by_target_id(self, resource, event, trigger, payload): + # log.target_id == port + self._clean_logs(payload.context.elevated(), + port_id=payload.resource_id) + @db_base_plugin_common.filter_fields @db_base_plugin_common.convert_result_to_dict def get_logs(self, context, filters=None, fields=None, sorts=None, diff --git a/neutron/tests/unit/services/logapi/common/test_db_api.py b/neutron/tests/unit/services/logapi/common/test_db_api.py index d75a60f9d16..f5938471156 100644 --- a/neutron/tests/unit/services/logapi/common/test_db_api.py +++ b/neutron/tests/unit/services/logapi/common/test_db_api.py @@ -16,6 +16,7 @@ import mock from neutron_lib import constants as const from neutron_lib import context +from neutron_lib.db import api as n_db_api from neutron_lib.services.logapi import constants as log_const from neutron_lib.utils import net as net_utils from oslo_utils import uuidutils @@ -27,21 +28,24 @@ from neutron.services.logapi.rpc import server as server_rpc from neutron.tests.unit.extensions import test_securitygroup as test_sg -def _create_log(tenant_id, resource_id=None, +def _create_log(context, project_id, resource_id=None, target_id=None, event='ALL', enabled=True,): log_data = { 'id': uuidutils.generate_uuid(), 'name': 'test', 'resource_type': 'security_group', - 'project_id': tenant_id, + 'project_id': project_id, 'event': event, 'enabled': enabled} if resource_id: log_data['resource_id'] = resource_id if target_id: log_data['target_id'] = target_id - return log_object.Log(**log_data) + with n_db_api.CONTEXT_WRITER.using(context): + _log_obj = log_object.Log(context, **log_data) + _log_obj.create() + return _log_obj class LoggingDBApiTestCase(test_sg.SecurityGroupDBTestCase): @@ -49,8 +53,8 @@ class LoggingDBApiTestCase(test_sg.SecurityGroupDBTestCase): def setUp(self): super(LoggingDBApiTestCase, self).setUp() self.context = context.get_admin_context() - self.sg_id, self.port_id, self.tenant_id = self._create_sg_and_port() - self.context.tenant_id = self.tenant_id + self.sg_id, self.port_id, self._tenant_id = self._create_sg_and_port() + self.context.tenant_id = self._tenant_id def _create_sg_and_port(self): with self.network() as network, \ @@ -67,45 +71,66 @@ class LoggingDBApiTestCase(test_sg.SecurityGroupDBTestCase): return sg_id, port_id, tenant_id def test_get_logs_bound_port(self): - log = _create_log(target_id=self.port_id, tenant_id=self.tenant_id) + log = _create_log(self.context, self._tenant_id, + target_id=self.port_id) with mock.patch.object(log_object.Log, 'get_objects', return_value=[log]): self.assertEqual( [log], db_api.get_logs_bound_port(self.context, self.port_id)) # Test get log objects with required resource type - calls = [mock.call(self.context, project_id=self.tenant_id, + calls = [mock.call(self.context, project_id=self._tenant_id, resource_type=log_const.SECURITY_GROUP, enabled=True)] log_object.Log.get_objects.assert_has_calls(calls) def test_get_logs_not_bound_port(self): fake_sg_id = uuidutils.generate_uuid() - log = _create_log(resource_id=fake_sg_id, tenant_id=self.tenant_id) + log = _create_log(self.context, self._tenant_id, + resource_id=fake_sg_id) with mock.patch.object(log_object.Log, 'get_objects', return_value=[log]): self.assertEqual( [], db_api.get_logs_bound_port(self.context, self.port_id)) # Test get log objects with required resource type - calls = [mock.call(self.context, project_id=self.tenant_id, + calls = [mock.call(self.context, project_id=self._tenant_id, resource_type=log_const.SECURITY_GROUP, enabled=True)] log_object.Log.get_objects.assert_has_calls(calls) def test_get_logs_bound_sg(self): - log = _create_log(resource_id=self.sg_id, tenant_id=self.tenant_id) - with mock.patch.object(log_object.Log, 'get_objects', - return_value=[log]): - self.assertEqual( - [log], db_api.get_logs_bound_sg( - self.context, self.sg_id, project_id=self.tenant_id)) + with self.network() as network, \ + self.subnet(network=network) as subnet, \ + self.port(subnet=subnet) as p1, \ + self.port(subnet=subnet, security_groups=[self.sg_id]) as p2: - # Test get log objects with required resource type - calls = [mock.call(self.context, project_id=self.tenant_id, - resource_type=log_const.SECURITY_GROUP, - enabled=True)] - log_object.Log.get_objects.assert_has_calls(calls) + log = _create_log(self.context, self._tenant_id) + log_sg = _create_log(self.context, self._tenant_id, + resource_id=self.sg_id) + log_port_no_sg = _create_log(self.context, self._tenant_id, + target_id=p1['port']['id']) + log_port_sg = _create_log(self.context, self._tenant_id, + target_id=p2['port']['id']) + self.assertEqual( + [log, log_sg, log_port_sg], + db_api.get_logs_bound_sg(self.context, sg_id=self.sg_id, + project_id=self._tenant_id)) + self.assertEqual( + [log_sg, log_port_sg], + db_api.get_logs_bound_sg(self.context, sg_id=self.sg_id, + project_id=self._tenant_id, + exclusive=True)) + self.assertEqual( + [log_port_no_sg], + db_api.get_logs_bound_sg( + self.context, project_id=self._tenant_id, + port_id=p1['port']['id'])) + self.assertEqual( + [log_port_sg], + db_api.get_logs_bound_sg( + self.context, project_id=self._tenant_id, + port_id=p2['port']['id'])) def test_get_logs_not_bound_sg(self): with self.network() as network, \ @@ -116,28 +141,28 @@ class LoggingDBApiTestCase(test_sg.SecurityGroupDBTestCase): self.fmt, network['network']['id'], security_groups=[sg2_id]) port2_id = self.deserialize(self.fmt, res)['port']['id'] - log = _create_log(target_id=port2_id, tenant_id=self.tenant_id) + log = _create_log(self.context, self._tenant_id, + target_id=port2_id) with mock.patch.object(log_object.Log, 'get_objects', return_value=[log]): self.assertEqual( [], db_api.get_logs_bound_sg( - self.context, self.sg_id, project_id=self.tenant_id)) + self.context, self.sg_id, project_id=self._tenant_id)) # Test get log objects with required resource type - calls = [mock.call(self.context, project_id=self.tenant_id, + calls = [mock.call(self.context, project_id=self._tenant_id, resource_type=log_const.SECURITY_GROUP, enabled=True)] log_object.Log.get_objects.assert_has_calls(calls) def test__get_ports_being_logged(self): - log1 = _create_log(target_id=self.port_id, - tenant_id=self.tenant_id) - log2 = _create_log(resource_id=self.sg_id, - tenant_id=self.tenant_id) - log3 = _create_log(target_id=self.port_id, - resource_id=self.tenant_id, - tenant_id=self.tenant_id) - log4 = _create_log(tenant_id=self.tenant_id) + log1 = _create_log(self.context, self._tenant_id, + target_id=self.port_id) + log2 = _create_log(self.context, self._tenant_id, + resource_id=self.sg_id) + log3 = _create_log(self.context, self._tenant_id, + target_id=self.port_id, resource_id=self.sg_id) + log4 = _create_log(self.context, self._tenant_id) with mock.patch.object( validators, 'validate_log_type_for_port', return_value=True): ports_log1 = db_api._get_ports_being_logged(self.context, log1) @@ -151,7 +176,7 @@ class LoggingDBApiTestCase(test_sg.SecurityGroupDBTestCase): self.assertEqual([self.port_id], ports_log4) def test__get_ports_being_logged_not_supported_log_type(self): - log = _create_log(tenant_id=self.tenant_id) + log = _create_log(self.context, self._tenant_id) with mock.patch.object( validators, 'validate_log_type_for_port', return_value=False): ports_log = db_api._get_ports_being_logged(self.context, log) @@ -189,7 +214,7 @@ class LoggingRpcCallbackTestCase(test_sg.SecurityGroupDBTestCase): security_groups=[sg_id]) ports_rest = self.deserialize(self.fmt, res) port_id = ports_rest['port']['id'] - log = _create_log(resource_id=sg_id, tenant_id=tenant_id) + log = _create_log(self.context, self._tenant_id, resource_id=sg_id) with mock.patch.object( server_rpc, 'get_rpc_method', @@ -261,7 +286,7 @@ class LoggingRpcCallbackTestCase(test_sg.SecurityGroupDBTestCase): ) ports_rest = self.deserialize(self.fmt, res) port_id = ports_rest['port']['id'] - log = _create_log(tenant_id=tenant_id) + log = _create_log(self.context, tenant_id) with mock.patch.object( log_object.Log, 'get_objects', return_value=[log]): with mock.patch.object( diff --git a/neutron/tests/unit/services/logapi/test_logging_plugin.py b/neutron/tests/unit/services/logapi/test_logging_plugin.py index 899d5559a89..c3f50c162a4 100644 --- a/neutron/tests/unit/services/logapi/test_logging_plugin.py +++ b/neutron/tests/unit/services/logapi/test_logging_plugin.py @@ -14,8 +14,6 @@ # under the License. import mock -from neutron_lib.callbacks import events -from neutron_lib.callbacks import resources from neutron_lib import context from neutron_lib.plugins import constants as plugin_const from neutron_lib.plugins import directory @@ -26,7 +24,6 @@ from neutron import manager from neutron.objects.logapi import logging_resource as log_object from neutron.objects import ports from neutron.objects import securitygroup as sg_object -from neutron.services.logapi.common import db_api as log_db_api from neutron.services.logapi.common import exceptions as log_exc from neutron.services.logapi.common import sg_validate from neutron.tests.unit.services.logapi import base @@ -70,25 +67,6 @@ class TestLoggingPlugin(base.BaseLogTestCase): new_callable=log_types).start() self.ctxt = context.Context('admin', 'fake_tenant') - def test__clean_security_group_logs(self): - logs = [ - {'id': uuidutils.generate_uuid()}, - {'id': uuidutils.generate_uuid()}] - sg_id = uuidutils.generate_uuid() - expected_delete_calls = [ - mock.call(mock.ANY, log['id']) for log in logs] - with mock.patch.object(log_db_api, 'get_logs_bound_sg', - return_value=logs) as mock_get_logs, \ - mock.patch.object( - self.log_plugin, 'delete_log') as mock_delete_log: - payload = mock.Mock( - context=self.ctxt, resource_id=sg_id) - self.log_plugin._clean_security_group_logs( - resources.SECURITY_GROUP, events.AFTER_DELETE, None, payload) - mock_get_logs.assert_called_once_with( - mock.ANY, sg_id) - mock_delete_log.assert_has_calls(expected_delete_calls) - def test_get_logs(self): with mock.patch.object(log_object.Log, 'get_objects')\ as get_objects_mock: