diff --git a/neutron/agent/ovn/agent/ovn_neutron_agent.py b/neutron/agent/ovn/agent/ovn_neutron_agent.py index ffebdc4ba91..9be7b0060c8 100644 --- a/neutron/agent/ovn/agent/ovn_neutron_agent.py +++ b/neutron/agent/ovn/agent/ovn_neutron_agent.py @@ -58,9 +58,8 @@ def __init__(self, conf): self._chassis = None self._chassis_id = None self._ovn_bridge = None - self.ext_manager_api = ext_mgr.OVNAgentExtensionAPI() - self.ext_manager = ext_mgr.OVNAgentExtensionManager(self._conf) - self.ext_manager.initialize(None, 'ovn', self) + self.ext_manager_api = None + self.ext_manager = None def __getitem__(self, name): """Return the named extension objet from ``self.ext_manager``""" @@ -159,7 +158,22 @@ def update_neutron_sb_cfg_key(self): 'Chassis_Private', self.chassis, ('external_ids', external_ids)).execute(check_error=True) + def _initialize_ext_manager(self): + """Initialize the externsion manager and the extension manager API. + + This method must be called once, outside the ``__init__`` method and + at the beginning of the ``start`` method. + """ + if not self.ext_manager: + self.ext_manager_api = ext_mgr.OVNAgentExtensionAPI() + self.ext_manager = ext_mgr.OVNAgentExtensionManager(self._conf) + self.ext_manager.initialize(None, 'ovn', self) + def start(self): + # This must be the first operation in the `start` method. + self._initialize_ext_manager() + + # Extension manager configuration. self.ext_manager_api.ovs_idl = self._load_ovs_idl() self.load_config() # Before executing "_load_sb_idl", is is needed to execute diff --git a/neutron/common/ovn/extensions.py b/neutron/common/ovn/extensions.py index afd6bf4f900..f2b15634096 100644 --- a/neutron/common/ovn/extensions.py +++ b/neutron/common/ovn/extensions.py @@ -70,6 +70,7 @@ from neutron_lib.api.definitions import qinq from neutron_lib.api.definitions import qos from neutron_lib.api.definitions import qos_bw_limit_direction +from neutron_lib.api.definitions import qos_bw_minimum_ingress from neutron_lib.api.definitions import qos_default from neutron_lib.api.definitions import qos_gateway_ip from neutron_lib.api.definitions import qos_rule_type_details @@ -171,6 +172,7 @@ qinq.ALIAS, qos.ALIAS, qos_bw_limit_direction.ALIAS, + qos_bw_minimum_ingress.ALIAS, qos_default.ALIAS, qos_rule_type_details.ALIAS, qos_rule_type_filter.ALIAS, diff --git a/neutron/pecan_wsgi/hooks/policy_enforcement.py b/neutron/pecan_wsgi/hooks/policy_enforcement.py index a6b469edc40..cba9d1573ad 100644 --- a/neutron/pecan_wsgi/hooks/policy_enforcement.py +++ b/neutron/pecan_wsgi/hooks/policy_enforcement.py @@ -16,6 +16,7 @@ from neutron_lib import constants as const from oslo_log import log as logging from oslo_policy import policy as oslo_policy +from oslo_serialization import jsonutils from oslo_utils import excutils from pecan import hooks import webob @@ -190,6 +191,14 @@ def after(self, state): # we have to set the status_code here to prevent the catch_errors # middleware from turning this into a 500. state.response.status_code = 404 + # replace the original body on NotFound body + error_message = { + 'type': 'HTTPNotFound', + 'message': 'The resource could not be found.', + 'detail': '' + } + state.response.text = jsonutils.dumps(error_message) + state.response.content_type = 'application/json' return if is_single: diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py index 0265c9768d1..f62f91ee95a 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py @@ -307,7 +307,7 @@ def update_lsp_host_info(self, context, db_port, up=True): if up: if not port_up: LOG.warning('Logical_Switch_Port %s host information not ' - 'updated, the port state is down') + 'updated, the port state is down', db_port.id) return if not db_port.port_bindings: @@ -333,7 +333,7 @@ def update_lsp_host_info(self, context, db_port, up=True): else: if port_up: LOG.warning('Logical_Switch_Port %s host information not ' - 'removed, the port state is up') + 'removed, the port state is up', db_port.id) return cmd.append( diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py index 9745ed183bb..a88d1c76a06 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py @@ -354,6 +354,33 @@ def sync_acls(self, ctx): add_acls.append(na) n_index += 1 + # Check any ACLs we found to add against existing ACLs, ignoring the + # SG rule ID key. This eliminates any false-positives where the + # normalized cidr for two SG rules is the same value, since there + # will only be a single ACL that matches exactly with the SG rule ID. + if add_acls: + def copy_acl_rem_id_key(acl): + acl_copy = acl.copy() + del acl_copy[ovn_const.OVN_SG_RULE_EXT_ID_KEY] + return acl_copy + + add_rem_acls = [] + # Make a list of non-default rule ACLs (they have a security group + # rule id). See ovn_default_acls code/comment above for more info. + nd_ovn_acls = [copy_acl_rem_id_key(oa) for oa in ovn_acls + if ovn_const.OVN_SG_RULE_EXT_ID_KEY in oa] + # We must copy here since we need to keep the original + # 'add_acl' intact for removal + for add_acl in add_acls: + add_acl_copy = copy_acl_rem_id_key(add_acl) + if add_acl_copy in nd_ovn_acls: + add_rem_acls.append(add_acl) + + # Remove any of the false-positive ACLs + LOG.warning('False-positive ACLs to remove: (%s)', add_rem_acls) + for add_rem in add_rem_acls: + add_acls.remove(add_rem) + if n_index < neutron_num: # We didn't find the OVN ACLs matching the Neutron ACLs # in "ovn_acls" and we are just adding the pending Neutron ACLs. diff --git a/neutron/services/logapi/drivers/ovn/driver.py b/neutron/services/logapi/drivers/ovn/driver.py index dc0a025b5ed..8871dda6c80 100644 --- a/neutron/services/logapi/drivers/ovn/driver.py +++ b/neutron/services/logapi/drivers/ovn/driver.py @@ -158,6 +158,11 @@ def _set_acls_log(self, pgs, context, ovn_txn, actions_enabled, log_name): for pg in pgs: meter_name = self.meter_name if pg["name"] != ovn_const.OVN_DROP_PORT_GROUP_NAME: + if ovn_const.OVN_SG_EXT_ID_KEY not in pg["external_ids"]: + LOG.info("Port Group %s is not part of any security " + "group, skipping its network log " + "setting...", pg["name"]) + continue sg = sg_obj.SecurityGroup.get_sg_by_id( context, pg["external_ids"][ovn_const.OVN_SG_EXT_ID_KEY]) if not sg: @@ -426,6 +431,11 @@ def _set_neutron_acls_log(self, pgs, context, actions_enabled, log_name, for pg in pgs: meter_name = self.meter_name if pg['name'] != ovn_const.OVN_DROP_PORT_GROUP_NAME: + if ovn_const.OVN_SG_EXT_ID_KEY not in pg["external_ids"]: + LOG.info("Port Group %s is not part of any security " + "group, skipping its network log " + "setting...", pg["name"]) + continue sg = sg_obj.SecurityGroup.get_sg_by_id(context, pg['external_ids'][ovn_const.OVN_SG_EXT_ID_KEY]) if not sg: diff --git a/neutron/tests/functional/agent/ovn/agent/test_ovn_neutron_agent.py b/neutron/tests/functional/agent/ovn/agent/test_ovn_neutron_agent.py index d25ebd57cf6..7211c8f9ee8 100644 --- a/neutron/tests/functional/agent/ovn/agent/test_ovn_neutron_agent.py +++ b/neutron/tests/functional/agent/ovn/agent/test_ovn_neutron_agent.py @@ -23,6 +23,7 @@ from neutron.agent.ovn.agent import ovsdb as agent_ovsdb from neutron.agent.ovn.metadata import agent as metadata_agent from neutron.agent.ovn.metadata import server_socket +from neutron.agent.ovsdb import impl_idl from neutron.common.ovn import constants as ovn_const from neutron.common import utils as n_utils from neutron.tests.common import net_helpers @@ -46,6 +47,7 @@ def setUp(self, extensions, **kwargs): self.mock_chassis_name = mock.patch.object( agent_ovsdb, 'get_own_chassis_name', return_value=self.chassis_name).start() + self.ovs_idl_events = [] with mock.patch.object(metadata_agent.MetadataAgent, '_get_own_chassis_name', return_value=self.chassis_name): @@ -57,6 +59,21 @@ def _check_loaded_and_started_extensions(self, ovn_agent): self.assertEqual(EXTENSION_NAMES.get(_ext), loaded_ext.name) self.assertTrue(loaded_ext.is_started) + def _create_ovs_idl(self, ovn_agent): + for extension in ovn_agent.ext_manager: + self.ovs_idl_events += extension.obj.ovs_idl_events + self.ovs_idl_events = [e(ovn_agent) for e in + set(self.ovs_idl_events)] + ovsdb = impl_idl.api_factory() + ovsdb.idl.notify_handler.watch_events(self.ovs_idl_events) + + ovn_agent.ext_manager_api.ovs_idl = ovsdb + return ovsdb + + def _clear_events_ovs_idl(self): + self.ovn_agent.ovs_idl.idl_monitor.notify_handler.unwatch_events( + self.ovs_idl_events) + def _start_ovn_neutron_agent(self): conf = self.useFixture(fixture_config.Config()).conf conf.set_override('extensions', ','.join(self.extensions), @@ -75,7 +92,11 @@ def _start_ovn_neutron_agent(self): # Once eventlet is completely removed, this mock can be deleted. with mock.patch.object(ovn_neutron_agent.OVNNeutronAgent, 'wait'), \ mock.patch.object(server_socket.UnixDomainMetadataProxy, - 'wait'): + 'wait'), \ + mock.patch.object(ovn_neutron_agent.OVNNeutronAgent, + '_load_ovs_idl') as mock_load_ovs_idl: + agt._initialize_ext_manager() + mock_load_ovs_idl.return_value = self._create_ovs_idl(agt) agt.start() external_ids = agt.sb_idl.db_get( 'Chassis_Private', agt.chassis, 'external_ids').execute( @@ -85,8 +106,7 @@ def _start_ovn_neutron_agent(self): '0') self._check_loaded_and_started_extensions(agt) - - self.addCleanup(agt.ext_manager_api.ovs_idl.ovsdb_connection.stop) + self.addCleanup(self._clear_events_ovs_idl) if agt.ext_manager_api.sb_idl: self.addCleanup(agt.ext_manager_api.sb_idl.ovsdb_connection.stop) if agt.ext_manager_api.nb_idl: diff --git a/neutron/tests/functional/pecan_wsgi/test_hooks.py b/neutron/tests/functional/pecan_wsgi/test_hooks.py index 9f43659aaef..ec091e3c066 100644 --- a/neutron/tests/functional/pecan_wsgi/test_hooks.py +++ b/neutron/tests/functional/pecan_wsgi/test_hooks.py @@ -288,6 +288,12 @@ def test_after_on_get_not_found(self): headers={'X-Project-Id': 'tenid'}, expect_errors=True) self.assertEqual(404, response.status_int) + self.assertEqual( + { + 'type': 'HTTPNotFound', + 'message': 'The resource could not be found.', + 'detail': '' + }, jsonutils.loads(response.body)) self.assertEqual(1, self.mock_plugin.get_meh.call_count) def test_after_on_get_excludes_admin_attribute(self): diff --git a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py index 5140361b099..6fb51367be3 100644 --- a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py +++ b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py @@ -1146,7 +1146,7 @@ def _build_acl_to_compare(self, acl, extra_fields=None): pass return acl_utils.filter_acl_dict(acl_to_compare, extra_fields) - def _validate_acls(self, should_match=True): + def _validate_acls(self, should_match=True, db_duplicate_port=None): # Get the neutron DB ACLs. db_acls = [] @@ -1194,6 +1194,19 @@ def _validate_acls(self, should_match=True): # Values taken out from list for comparison, since ACLs from OVN DB # have certain values on a list of just one object if should_match: + if db_duplicate_port: + # If we have a duplicate port, that indicates there are two + # DB entries that map to the same ACL. Remove the extra from + # our comparison. + dup_acl = None + for acl in db_acls: + if (str(db_duplicate_port) in acl['match'] and + acl not in plugin_acls): + dup_acl = acl + break + # There should have been a duplicate + self.assertIsNotNone(dup_acl) + db_acls.remove(dup_acl) for acl in plugin_acls: if isinstance(acl['severity'], list) and acl['severity']: acl['severity'] = acl['severity'][0] @@ -1786,13 +1799,16 @@ def test_sync_fip_qos_policies(self): nb_synchronizer.sync_fip_qos_policies(ctx) self._validate_qos_records() - def _create_security_group_rule(self, sg_id, direction, tcp_port): + def _create_security_group_rule(self, sg_id, direction, tcp_port, + remote_ip_prefix=None): data = {'security_group_rule': {'security_group_id': sg_id, 'direction': direction, 'protocol': constants.PROTO_NAME_TCP, 'ethertype': constants.IPv4, 'port_range_min': tcp_port, 'port_range_max': tcp_port}} + if remote_ip_prefix: + data['security_group_rule']['remote_ip_prefix'] = remote_ip_prefix req = self.new_create_request('security-group-rules', data, self.fmt) res = req.get_response(self.api) sgr = self.deserialize(self.fmt, res) @@ -1863,6 +1879,20 @@ def test_sync_acls_with_logging(self): self._test_sync_acls_helper(test_log=True, log_event=log_const.DROP_EVENT) + def test_sync_acls_overlapping_cidr(self): + data = {'security_group': {'name': 'sgdup'}} + sg_req = self.new_create_request('security-groups', data) + res = sg_req.get_response(self.api) + sg = self.deserialize(self.fmt, res)['security_group'] + + # Add SG rules that map to the same ACL due to normalizing the cidr + for ip_suffix in range(10, 12): + remote_ip_prefix = '192.168.0.' + str(ip_suffix) + '/24' + self._create_security_group_rule( + sg['id'], 'ingress', 9000, remote_ip_prefix=remote_ip_prefix) + + self._validate_acls(db_duplicate_port=9000) + class TestOvnSbSync(base.TestOVNFunctionalBase):