Skip to content

Commit 5da469f

Browse files
Zuulopenstack-gerrit
Zuul
authored and
openstack-gerrit
committed
Merge "Log warning about port forwardings that won't work properly" into stable/2023.1
2 parents 0dd3cf9 + bd6595d commit 5da469f

File tree

7 files changed

+200
-4
lines changed

7 files changed

+200
-4
lines changed

‎neutron/common/ovn/utils.py‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -679,6 +679,15 @@ def is_gateway_chassis_invalid(chassis_name, gw_chassis,
679679

680680

681681
def is_provider_network(network):
682+
"""Check if given network is provider network
683+
:param network: (str, dict) it can be given as network object or as string
684+
with network ID only. In the latter case, network object
685+
will be loaded from the database
686+
"""
687+
if isinstance(network, str):
688+
ctx = n_context.get_admin_context()
689+
plugin = directory.get_plugin()
690+
network = plugin.get_network(ctx, network)
682691
return network.get(provider_net.PHYSICAL_NETWORK, False)
683692

684693

‎neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -971,6 +971,41 @@ def create_router_extra_attributes_registers(self):
971971

972972
raise periodics.NeverAgain()
973973

974+
# TODO(slaweq): Remove this in the E cycle (C+2 as it will be next SLURP)
975+
@periodics.periodic(spacing=600, run_immediately=True)
976+
def add_gw_port_info_to_logical_router_port(self):
977+
"""Add info if LRP is connecting internal subnet or ext gateway."""
978+
cmds = []
979+
context = n_context.get_admin_context()
980+
for router in self._ovn_client._l3_plugin.get_routers(context):
981+
ext_gw_networks = [
982+
ext_gw['network_id'] for ext_gw in router['external_gateways']]
983+
rtr_name = 'neutron-{}'.format(router['id'])
984+
ovn_lr = self._nb_idl.get_lrouter(rtr_name)
985+
for lrp in ovn_lr.ports:
986+
if ovn_const.OVN_ROUTER_IS_EXT_GW in lrp.external_ids:
987+
continue
988+
ovn_network_name = lrp.external_ids.get(
989+
ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY)
990+
if not ovn_network_name:
991+
continue
992+
network_id = ovn_network_name.replace('neutron-', '')
993+
if not network_id:
994+
continue
995+
is_ext_gw = str(network_id in ext_gw_networks)
996+
external_ids = lrp.external_ids
997+
external_ids[ovn_const.OVN_ROUTER_IS_EXT_GW] = is_ext_gw
998+
cmds.append(
999+
self._nb_idl.update_lrouter_port(
1000+
name=lrp.name,
1001+
external_ids=external_ids))
1002+
1003+
if cmds:
1004+
with self._nb_idl.transaction(check_error=True) as txn:
1005+
for cmd in cmds:
1006+
txn.add(cmd)
1007+
raise periodics.NeverAgain()
1008+
9741009
@periodics.periodic(spacing=600, run_immediately=True)
9751010
def check_router_default_route_empty_dst_ip(self):
9761011
"""Check routers with default route with empty dst-ip (LP: #2002993).

‎neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1537,7 +1537,10 @@ def _gen_router_port_ext_ids(self, port):
15371537
ovn_const.OVN_SUBNET_EXT_IDS_KEY:
15381538
' '.join(utils.get_port_subnet_ids(port)),
15391539
ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY:
1540-
utils.ovn_name(port['network_id'])}
1540+
utils.ovn_name(port['network_id']),
1541+
ovn_const.OVN_ROUTER_IS_EXT_GW:
1542+
str(const.DEVICE_OWNER_ROUTER_GW == port.get('device_owner')),
1543+
}
15411544

15421545
router_id = port.get('device_id')
15431546
if router_id:

‎neutron/services/portforwarding/drivers/ovn/driver.py‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,14 @@
1717
from neutron_lib.plugins import constants as plugin_constants
1818
from neutron_lib.plugins import directory
1919
from oslo_log import log
20+
from oslo_utils import strutils
2021
from ovsdbapp.backend.ovs_idl import idlutils
2122
from ovsdbapp import constants as ovsdbapp_const
2223

2324
from neutron.common.ovn import constants as ovn_const
2425
from neutron.common.ovn import exceptions as ovn_exc
2526
from neutron.common.ovn import utils as ovn_utils
27+
from neutron.conf.plugins.ml2.drivers.ovn import ovn_conf
2628
from neutron.db import ovn_revision_numbers_db as db_rev
2729
from neutron import manager
2830
from neutron.objects import port_forwarding as port_forwarding_obj
@@ -129,7 +131,41 @@ def _port_forwarding_created(self, ovn_txn, nb_ovn, pf_obj,
129131
"Switch %s failed as it is not found",
130132
lb_name, ls_name)
131133

134+
def _validate_router_networks(self, nb_ovn, router_id):
135+
if not ovn_conf.is_ovn_distributed_floating_ip():
136+
return
137+
rtr_name = 'neutron-{}'.format(router_id)
138+
ovn_lr = nb_ovn.get_lrouter(rtr_name)
139+
if not ovn_lr:
140+
return
141+
for lrouter_port in ovn_lr.ports:
142+
is_ext_gw = strutils.bool_from_string(
143+
lrouter_port.external_ids.get(ovn_const.OVN_ROUTER_IS_EXT_GW))
144+
if is_ext_gw:
145+
# NOTE(slaweq): This is external gateway port of the router and
146+
# this not needs to be checked
147+
continue
148+
ovn_network_name = lrouter_port.external_ids.get(
149+
ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY)
150+
if not ovn_network_name:
151+
continue
152+
network_id = ovn_network_name.replace('neutron-', '')
153+
if not network_id:
154+
continue
155+
if ovn_utils.is_provider_network(network_id):
156+
LOG.warning("Port forwarding configured in the router "
157+
"%(router_id)s will not work properly as "
158+
"distributed floating IPs are enabled "
159+
"and at least one provider network "
160+
"(%(network_id)s) is connected to that router. "
161+
"See bug https://launchpad.net/bugs/2028846 for "
162+
"more details.", {
163+
'router_id': router_id,
164+
'network_id': network_id})
165+
return
166+
132167
def port_forwarding_created(self, ovn_txn, nb_ovn, pf_obj):
168+
self._validate_router_networks(nb_ovn, pf_obj.router_id)
133169
pf_objs = pf_obj.unroll_port_ranges()
134170
is_range = len(pf_objs) > 1
135171
for pf_obj in pf_objs:

‎neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -937,6 +937,70 @@ def test_update_port_virtual_type(self, *args):
937937
('type', constants.LSP_TYPE_VIRTUAL))]
938938
nb_idl.db_set.assert_has_calls(expected_calls)
939939

940+
def test_add_gw_port_info_to_logical_router_port(self):
941+
nb_idl = self.fake_ovn_client._nb_idl
942+
ext_net_id = uuidutils.generate_uuid()
943+
internal_net_id = uuidutils.generate_uuid()
944+
routers_db = [{
945+
'id': uuidutils.generate_uuid(),
946+
'external_gateways': [{'network_id': ext_net_id}]}]
947+
ext_gw_lrp = fakes.FakeOvsdbRow.create_one_ovsdb_row(
948+
attrs={'external_ids': {
949+
constants.OVN_NETWORK_NAME_EXT_ID_KEY:
950+
'neutron-{}'.format(ext_net_id)}})
951+
internal_net_lrp = fakes.FakeOvsdbRow.create_one_ovsdb_row(
952+
attrs={'external_ids': {
953+
constants.OVN_NETWORK_NAME_EXT_ID_KEY:
954+
'neutron-{}'.format(internal_net_id)}})
955+
fake_router = fakes.FakeOvsdbRow.create_one_ovsdb_row(
956+
attrs={
957+
'ports': [ext_gw_lrp, internal_net_lrp]})
958+
959+
expected_new_ext_gw_lrp_ids = ext_gw_lrp.external_ids
960+
expected_new_ext_gw_lrp_ids[constants.OVN_ROUTER_IS_EXT_GW] = 'True'
961+
expected_new_internal_lrp_ids = internal_net_lrp.external_ids
962+
expected_new_internal_lrp_ids[constants.OVN_ROUTER_IS_EXT_GW] = 'False'
963+
964+
self.fake_ovn_client._l3_plugin.get_routers.return_value = routers_db
965+
nb_idl.get_lrouter.return_value = fake_router
966+
self.assertRaises(
967+
periodics.NeverAgain,
968+
self.periodic.add_gw_port_info_to_logical_router_port)
969+
nb_idl.update_lrouter_port.assert_has_calls([
970+
mock.call(name=ext_gw_lrp.name,
971+
external_ids=expected_new_ext_gw_lrp_ids),
972+
mock.call(name=internal_net_lrp.name,
973+
external_ids=expected_new_internal_lrp_ids)],
974+
any_order=True)
975+
976+
def test_add_gw_port_info_to_logical_router_port_no_action_needed(self):
977+
nb_idl = self.fake_ovn_client._nb_idl
978+
ext_net_id = uuidutils.generate_uuid()
979+
internal_net_id = uuidutils.generate_uuid()
980+
routers_db = [{
981+
'id': uuidutils.generate_uuid(),
982+
'external_gateways': [{'network_id': ext_net_id}]}]
983+
ext_gw_lrp = fakes.FakeOvsdbRow.create_one_ovsdb_row(
984+
attrs={'external_ids': {
985+
constants.OVN_NETWORK_NAME_EXT_ID_KEY:
986+
'neutron-{}'.format(ext_net_id),
987+
constants.OVN_ROUTER_IS_EXT_GW: 'True'}})
988+
internal_net_lrp = fakes.FakeOvsdbRow.create_one_ovsdb_row(
989+
attrs={'external_ids': {
990+
constants.OVN_NETWORK_NAME_EXT_ID_KEY:
991+
'neutron-{}'.format(internal_net_id),
992+
constants.OVN_ROUTER_IS_EXT_GW: 'False'}})
993+
fake_router = fakes.FakeOvsdbRow.create_one_ovsdb_row(
994+
attrs={
995+
'ports': [ext_gw_lrp, internal_net_lrp]})
996+
997+
self.fake_ovn_client._l3_plugin.get_routers.return_value = routers_db
998+
nb_idl.get_lrouter.return_value = fake_router
999+
self.assertRaises(
1000+
periodics.NeverAgain,
1001+
self.periodic.add_gw_port_info_to_logical_router_port)
1002+
nb_idl.update_lrouter_port.assert_not_called()
1003+
9401004
def test_check_router_default_route_empty_dst_ip(self):
9411005
nb_idl = self.fake_ovn_client._nb_idl
9421006
route0 = fakes.FakeOvsdbRow.create_one_ovsdb_row(

‎neutron/tests/unit/services/ovn_l3/test_plugin.py‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,8 @@ def setUp(self):
9393
ovn_const.OVN_SUBNET_EXT_IDS_KEY: 'subnet-id',
9494
ovn_const.OVN_REV_NUM_EXT_ID_KEY: '1',
9595
ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY:
96-
utils.ovn_name(self.fake_network['id'])}}
96+
utils.ovn_name(self.fake_network['id']),
97+
ovn_const.OVN_ROUTER_IS_EXT_GW: 'False'}}
9798
self.fake_router_ports = [self.fake_router_port]
9899
self.fake_subnet = {'id': 'subnet-id',
99100
'ip_version': 4,
@@ -149,7 +150,8 @@ def setUp(self):
149150
ovn_const.OVN_SUBNET_EXT_IDS_KEY: 'ext-subnet-id',
150151
ovn_const.OVN_REV_NUM_EXT_ID_KEY: '1',
151152
ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY:
152-
utils.ovn_name(self.fake_network['id'])},
153+
utils.ovn_name(self.fake_network['id']),
154+
ovn_const.OVN_ROUTER_IS_EXT_GW: 'True'},
153155
'gateway_chassis': ['hv1'],
154156
'options': {}}
155157
self.fake_floating_ip_attrs = {'floating_ip_address': '192.168.0.10',
@@ -404,7 +406,8 @@ def test_remove_router_interface_update_lrouter_port(self):
404406
ovn_const.OVN_SUBNET_EXT_IDS_KEY: 'subnet-id',
405407
ovn_const.OVN_REV_NUM_EXT_ID_KEY: '1',
406408
ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY:
407-
utils.ovn_name(self.fake_network['id'])})
409+
utils.ovn_name(self.fake_network['id']),
410+
ovn_const.OVN_ROUTER_IS_EXT_GW: 'False'})
408411

409412
def test_remove_router_interface_router_not_found(self):
410413
router_id = 'router-id'
@@ -1631,6 +1634,8 @@ def test_add_router_interface_need_to_frag_enabled(
16311634
fake_router_port_assert['options'] = {
16321635
ovn_const.OVN_ROUTER_PORT_GW_MTU_OPTION:
16331636
str(prov_net['mtu'])}
1637+
fake_router_port_assert['external_ids'][
1638+
ovn_const.OVN_ROUTER_IS_EXT_GW] = 'True'
16341639

16351640
self.l3_inst._nb_ovn.add_lrouter_port.assert_called_once_with(
16361641
**fake_router_port_assert)
@@ -1678,6 +1683,8 @@ def test_add_router_interface_need_to_frag_enabled_then_remove(
16781683
fake_router_port_assert = self.fake_router_port_assert
16791684
fake_router_port_assert['gateway_chassis'] = mock.ANY
16801685
fake_router_port_assert['options'] = {}
1686+
fake_router_port_assert['external_ids'][
1687+
ovn_const.OVN_ROUTER_IS_EXT_GW] = 'True'
16811688

16821689
self.l3_inst._nb_ovn.add_lrouter_port.assert_called_once_with(
16831690
**fake_router_port_assert)

‎neutron/tests/unit/services/portforwarding/drivers/ovn/test_driver.py‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -450,6 +450,48 @@ def test_port_forwarding_deleted(self, m_info):
450450
self.l3_plugin._nb_ovn.lb_del.assert_called_once_with(
451451
exp_lb_name, exp_vip, if_exists=mock.ANY)
452452

453+
@mock.patch.object(port_forwarding.LOG, 'warning')
454+
def test__validate_router_networks_provider_networks(self, mock_warning):
455+
lr_ports = [
456+
mock.MagicMock(external_ids={
457+
ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY: 'neutron-ext-net-id',
458+
ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: 'True'}),
459+
mock.MagicMock(external_ids={
460+
ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY: 'neutron-net-id',
461+
ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: 'False'})]
462+
lr_mock = mock.MagicMock(ports=lr_ports)
463+
ovn_nb_mock = mock.Mock()
464+
ovn_nb_mock.get_lrouter.return_value = lr_mock
465+
with mock.patch.object(
466+
ovn_conf, 'is_ovn_distributed_floating_ip',
467+
return_value=True), mock.patch.object(
468+
ovn_utils, 'is_provider_network',
469+
return_value=True):
470+
self.handler._validate_router_networks(
471+
ovn_nb_mock, 'router-id')
472+
self.assertEqual(1, mock_warning.call_count)
473+
474+
@mock.patch.object(port_forwarding.LOG, 'warning')
475+
def test__validate_router_networks_tunnel_networks(self, mock_warning):
476+
lr_ports = [
477+
mock.MagicMock(external_ids={
478+
ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY: 'neutron-ext-net-id',
479+
ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: 'True'}),
480+
mock.MagicMock(external_ids={
481+
ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY: 'neutron-net-id',
482+
ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: 'False'})]
483+
lr_mock = mock.MagicMock(ports=lr_ports)
484+
ovn_nb_mock = mock.Mock()
485+
ovn_nb_mock.get_lrouter.return_value = lr_mock
486+
with mock.patch.object(
487+
ovn_conf, 'is_ovn_distributed_floating_ip',
488+
return_value=True), mock.patch.object(
489+
ovn_utils, 'is_provider_network',
490+
return_value=False):
491+
self.handler._validate_router_networks(
492+
ovn_nb_mock, 'router-id')
493+
mock_warning.assert_not_called()
494+
453495

454496
class TestOVNPortForwarding(TestOVNPortForwardingBase):
455497
def setUp(self):

0 commit comments

Comments
 (0)