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/metadata.py b/neutron/common/metadata.py index b17765b8864..d3ade12235c 100644 --- a/neutron/common/metadata.py +++ b/neutron/common/metadata.py @@ -33,7 +33,10 @@ PROXY_SERVICE_NAME = 'haproxy' PROXY_SERVICE_CMD = 'haproxy' -CONTENT_ENCODERS = ('gzip', 'deflate') +CONTENT_ENCODERS = { + 'gzip': b'\x1f\x8b\x08', + 'deflate': b'\x1f\x8b\x08' +} class InvalidUserOrGroupException(Exception): @@ -159,15 +162,21 @@ class MetadataProxyHandlerBaseSocketServer( metaclass=abc.ABCMeta): @staticmethod def _http_response(http_response, request): + headerlist = list(http_response.headers.items()) + # We detect if content is compressed by magic signature, + # when `content-encoding` is not present. + if not http_response.headers.get('content-encoding'): + if http_response.content[:3] == CONTENT_ENCODERS['gzip']: + headerlist.append(('content-encoding', 'gzip')) + _res = webob.Response( body=http_response.content, status=http_response.status_code, - content_type=http_response.headers['content-type'], - charset=http_response.encoding) + headerlist=headerlist) # The content of the response is decoded depending on the # "Context-Enconding" header, if present. The operation is limited to # ("gzip", "deflate"), as is in the ``webob.response.Response`` class. - if _res.content_encoding in CONTENT_ENCODERS: + if _res.content_encoding in CONTENT_ENCODERS.keys(): _res.decode_content() # NOTE(ralonsoh): there should be a better way to format the HTTP 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/mech_driver.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py index ec87494a1bc..52c0ddfcca6 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py @@ -655,6 +655,14 @@ def _get_common_ips(ip_addresses, ip_networks): common_ips.add(str(ip_address)) return common_ips + # NOTE(slaweq): We can safely ignore any CIDR larger than /32 (for + # IPv4) or /128 (for IPv6) in the allowed_address_pairs, since such + # CIDRs cannot be set as a Virtual IP in OVN. + # Only /32 and /128 CIDRs are allowed to be set as Virtual IPs in OVN. + address_pairs_to_check = [ + ip_net for ip_net in port_allowed_address_pairs_ip_addresses + if ip_net.size == 1] + for distributed_port in distributed_ports: distributed_port_ip_addresses = [ netaddr.IPAddress(fixed_ip['ip_address']) for fixed_ip in @@ -662,7 +670,7 @@ def _get_common_ips(ip_addresses, ip_networks): common_ips = _get_common_ips( distributed_port_ip_addresses, - port_allowed_address_pairs_ip_addresses) + address_pairs_to_check) if common_ips: err_msg = ( 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/agent/ovsdb/native/test_helpers.py b/neutron/tests/functional/agent/ovsdb/native/test_helpers.py index 3b365533df4..6bacde09282 100644 --- a/neutron/tests/functional/agent/ovsdb/native/test_helpers.py +++ b/neutron/tests/functional/agent/ovsdb/native/test_helpers.py @@ -14,7 +14,7 @@ # under the License. from neutron_lib import constants as const -from ovsdbapp.backend.ovs_idl import event +from ovsdbapp.backend.ovs_idl import event as idl_event from neutron.agent.common import ovs_lib from neutron.agent.ovsdb.native import helpers @@ -23,14 +23,21 @@ from neutron.tests.functional import base -class WaitOvsManagerEvent(event.WaitEvent): +class WaitOvsManagerEvent(idl_event.WaitEvent): event_name = 'WaitOvsManagerEvent' - def __init__(self, manager_target): + def __init__(self, manager_target, inactivity_probe=None, event=None): table = 'Manager' - events = (self.ROW_CREATE,) + events = (self.ROW_CREATE,) if not event else (event,) conditions = (('target', '=', manager_target),) super().__init__(events, table, conditions, timeout=10) + self.inactivity_probe = inactivity_probe + + def match_fn(self, event, row, old): + if (self.inactivity_probe is None or + self.inactivity_probe == row.inactivity_probe[0]): + return True + return False class EnableConnectionUriTestCase(base.BaseSudoTestCase): @@ -78,8 +85,13 @@ def test_add_manager_overwrites_existing_manager(self): ovsdb_cfg_connection = 'tcp:127.0.0.1:%s' % _port manager_connection = 'ptcp:%s:127.0.0.1' % _port + inactivity_probe = 10 + manager_event = WaitOvsManagerEvent( + manager_connection, inactivity_probe=inactivity_probe) + ovs.ovsdb.idl.notify_handler.watch_event(manager_event) helpers.enable_connection_uri(ovsdb_cfg_connection, - inactivity_probe=10) + inactivity_probe=inactivity_probe) + manager_event.wait() self.addCleanup(ovs.ovsdb.remove_manager(manager_connection).execute) # First call of enable_connection_uri cretes the manager # and the list returned by get_manager contains it: @@ -89,7 +101,13 @@ def test_add_manager_overwrites_existing_manager(self): # after 2nd call of enable_connection_uri with new value of # inactivity_probe will keep the original manager only the # inactivity_probe value is set: + inactivity_probe = 100 + manager_event = WaitOvsManagerEvent( + manager_connection, inactivity_probe=inactivity_probe, + event='update') + ovs.ovsdb.idl.notify_handler.watch_event(manager_event) helpers.enable_connection_uri(ovsdb_cfg_connection, - inactivity_probe=100) + inactivity_probe=inactivity_probe) + manager_event.wait() my_mans = ovs.ovsdb.get_manager().execute() self.assertIn(manager_connection, my_mans) 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): diff --git a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py index 7bd6e1bc386..27ac90d9d2b 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py +++ b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py @@ -3212,17 +3212,23 @@ def test_create_port_with_allowed_address_pairs(self): 'mac_address': port2['mac_address']}], port2['allowed_address_pairs']) - # Now test the same but giving a subnet as allowed address pair - self._make_port( + # Now test the same but giving a subnet as allowed address + # pair, this should be fine as we treat only /32 and /128 IPs + # in allowed_address_pairs as Virtual IPs, there is no block + # anything when bigger CIDR is set as that don't break metadata + new_port = self._make_port( self.fmt, network['network']['id'], allowed_address_pairs=[{'ip_address': '10.0.0.2/26'}], - expected_res_status=exc.HTTPBadRequest.code, - arg_list=('allowed_address_pairs',)) + arg_list=('allowed_address_pairs',))['port'] port3 = self._show('ports', port1['id'])['port'] self.assertEqual( [{'ip_address': '10.0.0.3', 'mac_address': port3['mac_address']}], port3['allowed_address_pairs']) + self.assertEqual( + [{'ip_address': '10.0.0.2/26', + 'mac_address': new_port['mac_address']}], + new_port['allowed_address_pairs']) class OVNMechanismDriverTestCase(MechDriverSetupBase,