From 8c7f3b61f75368f05369785f7931b5134a7e93fa Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Hernandez Date: Sun, 5 Mar 2023 22:12:55 +0100 Subject: [PATCH 1/4] [OVS] Allow custom ethertype traffic in the ingress table This patch is a partial revert of [1], reinstantiating the code merged in [2]. This patch is the complementary to [1]: the traffic with custom ethertypes is allowed in the ingress processing tables, same as [1] is allowing all traffic from the virtual machine ports in this host to leave the node. Both, this patch and [1], are bypassing the OVS firewall just for the traffic with the configured allowed ethertypes and just for/to the local ports and MAC addresses. Any other traffic not coming from a local port or with destination a local port, will be blocked as is now. [1]https://review.opendev.org/c/openstack/neutron/+/678021 [2]https://review.opendev.org/c/openstack/neutron/+/668224/ Conflicts: doc/source/admin/config-ovsfwdriver.rst neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py Closes-Bug: #2009221 Related-Bug: #1832758 Change-Id: Ib8340d9430b946a446edf80886c49fbac729073c (cherry picked from commit 008277b8c12d99438951a308b278203fa7a7c3ef) (cherry picked from commit 5026d805fe01aaf237081c606f1d1bf87bbff6d4) --- doc/source/admin/config-ovsfwdriver.rst | 12 ++++++++++ .../linux/openvswitch_firewall/firewall.py | 19 ++++++++++++++++ .../openvswitch_firewall/test_firewall.py | 22 +++++++++++++++++++ 3 files changed, 53 insertions(+) diff --git a/doc/source/admin/config-ovsfwdriver.rst b/doc/source/admin/config-ovsfwdriver.rst index e915700b462..061c77cea6c 100644 --- a/doc/source/admin/config-ovsfwdriver.rst +++ b/doc/source/admin/config-ovsfwdriver.rst @@ -88,6 +88,18 @@ not true and there may be slight differences between those drivers. | (please check [3]_ for details) | | rule. | +----------------------------------------+-----------------------+-----------------------+ + +Permitted ethertypes +~~~~~~~~~~~~~~~~~~~~ + +The OVS Firewall blocks traffic that does not have either the IPv4 or IPv6 +ethertypes at present. This is a behavior change compared to the +"iptables_hybrid" firewall, which only operates on IP packets and thus does +not address other ethertypes. With the configuration option +``permitted_ethertypes`` it is possible to define a set of allowed ethertypes. +Any traffic with these allowed ethertypes with destination to a local port or +generated from a local port and MAC address, will be allowed. + References ~~~~~~~~~~ diff --git a/neutron/agent/linux/openvswitch_firewall/firewall.py b/neutron/agent/linux/openvswitch_firewall/firewall.py index b57658b2321..992d1754fbf 100644 --- a/neutron/agent/linux/openvswitch_firewall/firewall.py +++ b/neutron/agent/linux/openvswitch_firewall/firewall.py @@ -1371,6 +1371,25 @@ def _initialize_ingress(self, port): actions='output:{:d}'.format(port.ofport) ) + # Allow custom ethertypes + for permitted_ethertype in self.permitted_ethertypes: + if permitted_ethertype[:2] == '0x': + try: + hex_ethertype = hex(int(permitted_ethertype, base=16)) + self._add_flow( + table=ovs_consts.BASE_INGRESS_TABLE, + priority=100, + dl_type=hex_ethertype, + reg_port=port.ofport, + actions='output:{:d}'.format(port.ofport) + ) + continue + except ValueError: + pass + LOG.warning('Custom ethertype %(permitted_ethertype)s is not ' + 'a hexadecimal number.', + {'permitted_ethertype': permitted_ethertype}) + self._initialize_ingress_ipv6_icmp(port) # DHCP offers diff --git a/neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py b/neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py index 0976b2f646d..4824ca3c9f8 100644 --- a/neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py +++ b/neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py @@ -30,6 +30,7 @@ from neutron.agent.linux.openvswitch_firewall import exceptions from neutron.agent.linux.openvswitch_firewall import firewall as ovsfw from neutron.conf.agent import securitygroups_rpc +from neutron.conf.plugins.ml2.drivers import ovs_conf from neutron.plugins.ml2.drivers.openvswitch.agent.common import constants \ as ovs_consts from neutron.plugins.ml2.drivers.openvswitch.agent.openflow.native \ @@ -514,6 +515,7 @@ def __init__(self, name, port, mac): class TestOVSFirewallDriver(base.BaseTestCase): def setUp(self): super(TestOVSFirewallDriver, self).setUp() + ovs_conf.register_ovs_agent_opts(cfg=cfg.CONF) mock_bridge = mock.patch.object( ovs_lib, 'OVSBridge', autospec=True).start() securitygroups_rpc.register_securitygroups_opts() @@ -840,6 +842,26 @@ def test_initialize_port_flows_vlan_dvr_conntrack_direct_vlan(self): return_value={"vlan1": "br-vlan1"}): self.firewall.initialize_port_flows(port) + def test_initialize_port_flows_permitted_ethertypes(self): + self.firewall.permitted_ethertypes = ['0x1234', '0x5678'] + port_dict = {'device': 'port-id', + 'security_groups': [1]} + of_port = create_ofport(port_dict, + network_type=constants.TYPE_VLAN, + physical_network='vlan1') + self.firewall.sg_port_map.ports[of_port.id] = of_port + port = self.firewall.get_or_create_ofport(port_dict) + with mock.patch.object(self.firewall, '_add_flow') as mock_add_flow: + self.firewall.initialize_port_flows(port) + + calls = [mock.call(table=ovs_consts.BASE_INGRESS_TABLE, + priority=100, dl_type='0x1234', + reg_port=1, actions='output:1'), + mock.call(table=ovs_consts.BASE_INGRESS_TABLE, + priority=100, dl_type='0x5678', + reg_port=1, actions='output:1')] + mock_add_flow.assert_has_calls(calls, any_order=True) + def test_delete_all_port_flows(self): port_dict = { 'device': 'port-id', From fec0286e26b5c0b02f1ebdc8d3d481f73fbe4aec Mon Sep 17 00:00:00 2001 From: Thomas Bachman Date: Wed, 23 Sep 2020 14:34:36 +0000 Subject: [PATCH 2/4] Fix default value for MTUs, when not provided When networks are created using REST APIs, if the MTU isn't specified in the request, then a default value of 0 is used. Some use cases, such as the auto-allocated-topology workflow, call the plugin directly to create networks, bypassing the layer that inserts this default value. Commit 68625686a40b3eb75502c8116f23d2297e288ca1 introduced a different default value at the DB layer, defined by a constant in neutron-lib. If the maximum MTU size has been configured lower than this constant, then the user receives an exception, even though they didn't provide a value for MTU. This patch changes the default value used in the DB layer, so that it's consistent with the workflow seen via REST APIs. Change-Id: Ica21e891cd2559942abb0ab2b12132e7f6cdd835 Closes-Bug: #1896933 (cherry picked from commit f759915ab0c30ebba6d3d970943b596b5c245599) --- neutron/db/db_base_plugin_v2.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/neutron/db/db_base_plugin_v2.py b/neutron/db/db_base_plugin_v2.py index 0b56b6cce88..492939bdca8 100644 --- a/neutron/db/db_base_plugin_v2.py +++ b/neutron/db/db_base_plugin_v2.py @@ -407,7 +407,7 @@ def create_network_db(self, context, network): args = {'tenant_id': project_id, 'id': n.get('id') or uuidutils.generate_uuid(), 'name': n['name'], - 'mtu': n.get('mtu', constants.DEFAULT_NETWORK_MTU), + 'mtu': n.get('mtu', 0), 'admin_state_up': n['admin_state_up'], 'status': n.get('status', constants.NET_STATUS_ACTIVE), 'description': n.get('description')} From 9150206a0744616cdc64313b62b718e7a2688d25 Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Hernandez Date: Wed, 15 Mar 2023 06:29:39 +0100 Subject: [PATCH 3/4] [OVN] Explicitly define the fixed IPs for the metadata port The metadata port fixed IPs depend on the subnets "enabled_dhcp" flag. If the subnet has this flag disabled, the metadata port doesn't receive an IP on the subnet CIDR. The method ``create_metadata_port`` should explicitly define what fixed IPs should request the metadata port during the creating depending on the subnets "enabled_dhcp" flag. Closes-Bug: #2011724 Change-Id: If362fab20ac03f8b62471b60c031f9349171ce93 (cherry picked from commit 9704dca84ea3ad21ecf9730eea03692daeddf382) --- .../ovn/mech_driver/ovsdb/ovn_client.py | 27 +++++--- .../ovn/mech_driver/ovsdb/test_ovn_client.py | 68 +++++++++++++++++++ 2 files changed, 86 insertions(+), 9 deletions(-) create mode 100644 neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_client.py 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 d319e7d5cc7..3281a869470 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 @@ -2377,17 +2377,26 @@ def _find_metadata_port_ip(self, context, subnet): return fixed_ip['ip_address'] def create_metadata_port(self, context, network): - if ovn_conf.is_ovn_metadata_enabled(): - metadata_port = self._find_metadata_port(context, network['id']) - if not metadata_port: - # Create a neutron port for DHCP/metadata services - port = {'port': - {'network_id': network['id'], + if not ovn_conf.is_ovn_metadata_enabled(): + return + + if self._find_metadata_port(context, network['id']): + return + + # Create a neutron port for DHCP/metadata services + filters = {'network_id': [network['id']]} + subnets = self._plugin.get_subnets(context, filters=filters) + fixed_ips = [{'subnet_id': s['id']} + for s in subnets if s['enable_dhcp']] + port = {'port': {'network_id': network['id'], 'tenant_id': network['project_id'], 'device_owner': const.DEVICE_OWNER_DISTRIBUTED, - 'device_id': 'ovnmeta-%s' % network['id']}} - # TODO(boden): rehome create_port into neutron-lib - p_utils.create_port(self._plugin, context, port) + 'device_id': 'ovnmeta-%s' % network['id'], + 'fixed_ips': fixed_ips, + } + } + # TODO(boden): rehome create_port into neutron-lib + p_utils.create_port(self._plugin, context, port) def update_metadata_port(self, context, network_id, subnet=None): """Update metadata port. diff --git a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_client.py b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_client.py new file mode 100644 index 00000000000..6a9b5a0938a --- /dev/null +++ b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_client.py @@ -0,0 +1,68 @@ +# Copyright 2023 Red Hat, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +from neutron_lib import constants + +from neutron.conf.plugins.ml2.drivers.ovn import ovn_conf as ovn_config +from neutron.tests.functional import base + + +class TestOVNClient(base.TestOVNFunctionalBase): + + def test_create_metadata_port(self): + def check_metadata_port(enable_dhcp): + ports = self.plugin.get_ports( + self.context, filters={'network_id': [network['id']]}) + self.assertEqual(1, len(ports)) + if enable_dhcp: + self.assertEqual(1, len(ports[0]['fixed_ips'])) + else: + self.assertEqual(0, len(ports[0]['fixed_ips'])) + return ports + + ovn_config.cfg.CONF.set_override('ovn_metadata_enabled', True, + group='ovn') + ovn_client = self.mech_driver._ovn_client + for enable_dhcp in (True, False): + network_args = {'tenant_id': 'project_1', + 'name': 'test_net_1', + 'admin_state_up': True, + 'shared': False, + 'status': constants.NET_STATUS_ACTIVE} + network = self.plugin.create_network(self.context, + {'network': network_args}) + subnet_args = {'tenant_id': 'project_1', + 'name': 'test_snet_1', + 'network_id': network['id'], + 'ip_version': constants.IP_VERSION_4, + 'cidr': '10.210.10.0/28', + 'enable_dhcp': enable_dhcp, + 'gateway_ip': constants.ATTR_NOT_SPECIFIED, + 'allocation_pools': constants.ATTR_NOT_SPECIFIED, + 'dns_nameservers': constants.ATTR_NOT_SPECIFIED, + 'host_routes': constants.ATTR_NOT_SPECIFIED} + self.plugin.create_subnet(self.context, {'subnet': subnet_args}) + + # The metadata port has been created during the network creation. + ports = check_metadata_port(enable_dhcp) + + # Force the deletion and creation the metadata port. + self.plugin.delete_port(self.context, ports[0]['id']) + ovn_client.create_metadata_port(self.context, network) + check_metadata_port(enable_dhcp) + + # Call again the "create_metadata_port" method as is idempotent + # because it checks first if the metadata port exists. + ovn_client.create_metadata_port(self.context, network) + check_metadata_port(enable_dhcp) From 85321a61d39f8a0e60a442e33ece810162f310f6 Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Date: Fri, 24 Mar 2023 15:50:30 +0000 Subject: [PATCH 4/4] Revert "Ensure vlan network traffic is not centralized" This reverts commit f83a97dea21c4d60cb2da16984fffcfb231fc239. Reason for revert: As part of the reverted commit, the redirect-type=bridged flag was enabled by default. However this have the side effect of also decentralizing N/S traffic for geneve tenant networks, breaking the VM connectivity on them when it must be centralized, i.e., when no FIPs are associated to the VMs. A new fix will be provided ASAP. Change-Id: I258cc439c70cfeae5b638ddd8e650dc2bf403c31 --- neutron/common/ovn/constants.py | 2 -- .../drivers/ovn/mech_driver/ovsdb/maintenance.py | 5 ++++- .../drivers/ovn/mech_driver/ovsdb/ovn_client.py | 16 ++++------------ .../notes/bug-2003455-dff0d0f00b5a18e2.yaml | 9 --------- 4 files changed, 8 insertions(+), 24 deletions(-) delete mode 100644 releasenotes/notes/bug-2003455-dff0d0f00b5a18e2.yaml diff --git a/neutron/common/ovn/constants.py b/neutron/common/ovn/constants.py index 8ac8bfc91af..93e9035c7d7 100644 --- a/neutron/common/ovn/constants.py +++ b/neutron/common/ovn/constants.py @@ -372,8 +372,6 @@ LSP_OPTIONS_MCAST_FLOOD = 'mcast_flood' LRP_OPTIONS_RESIDE_REDIR_CH = 'reside-on-redirect-chassis' -LRP_OPTIONS_REDIRECT_TYPE = 'redirect-type' -BRIDGE_REDIRECT_TYPE = "bridged" # Port Binding types PB_TYPE_VIRTUAL = 'virtual' diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py index a25e75bcb4b..a6c1f1c628e 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py @@ -831,7 +831,10 @@ def check_vlan_distributed_ports(self): # Get router ports belonging to VLAN networks vlan_nets = self._ovn_client._plugin.get_networks( context, {pnet.NETWORK_TYPE: [n_const.TYPE_VLAN]}) - vlan_net_ids = [vn['id'] for vn in vlan_nets] + # FIXME(ltomasbo): Once Bugzilla 2162756 is fixed the + # is_provider_network check should be removed + vlan_net_ids = [vn['id'] for vn in vlan_nets + if not utils.is_provider_network(vn)] router_ports = self._ovn_client._plugin.get_ports( context, {'network_id': vlan_net_ids, 'device_owner': n_const.ROUTER_PORT_OWNERS}) 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 d319e7d5cc7..a62324135b7 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 @@ -1554,29 +1554,21 @@ def _gen_router_port_options(self, port, network=None): if network is None: network = self._plugin.get_network(admin_context, port['network_id']) - # For VLAN type networks we need to set the # "reside-on-redirect-chassis" option so the routing for this # logical router port is centralized in the chassis hosting the # distributed gateway port. # https://github.com/openvswitch/ovs/commit/85706c34d53d4810f54bec1de662392a3c06a996 + # FIXME(ltomasbo): Once Bugzilla 2162756 is fixed the + # is_provider_network check should be removed if network.get(pnet.NETWORK_TYPE) == const.TYPE_VLAN: options[ovn_const.LRP_OPTIONS_RESIDE_REDIR_CH] = ( - 'false' if ovn_conf.is_ovn_distributed_floating_ip() + 'false' if (ovn_conf.is_ovn_distributed_floating_ip() and + not utils.is_provider_network(network)) else 'true') is_gw_port = const.DEVICE_OWNER_ROUTER_GW == port.get( 'device_owner') - - # NOTE(ltomasbo): For VLAN type networks connected through the gateway - # port there is a need to set the redirect-type option to bridge to - # ensure traffic is not centralized through the controller. - # For geneve based tenant networks it won't have any effect as it only - # applies to network with a localnet associated to it - if is_gw_port and ovn_conf.is_ovn_distributed_floating_ip(): - options[ovn_const.LRP_OPTIONS_REDIRECT_TYPE] = ( - ovn_const.BRIDGE_REDIRECT_TYPE) - if is_gw_port and ovn_conf.is_ovn_emit_need_to_frag_enabled(): try: router_ports = self._get_router_ports(admin_context, diff --git a/releasenotes/notes/bug-2003455-dff0d0f00b5a18e2.yaml b/releasenotes/notes/bug-2003455-dff0d0f00b5a18e2.yaml deleted file mode 100644 index c17fe4338da..00000000000 --- a/releasenotes/notes/bug-2003455-dff0d0f00b5a18e2.yaml +++ /dev/null @@ -1,9 +0,0 @@ ---- -fixes: - - | - [`bug 2003455 `_] - Previous commit (https://review.opendev.org/c/openstack/neutron/+/871252) - added a workaround to avoid vlan provider networks traffic to be tunneled - to the compute nodes but it was still centralized. Now the traffic is - distributed thanks to using the "redirect-type" flag on the ovn gateway - port.