Skip to content

Commit

Permalink
Fix: zero route may have empty nexthop (sonic-net#276)
Browse files Browse the repository at this point in the history
**- What I did**
Fix: zero route may have empty nexthop. Improve code robustness.

**- How I did it**

**- How to verify it**
Manual test:
1. manipulate ApplDB default route: remove nexthop field and value
2. recover
3. manipulate ApplDB default route: remove ifname field and value
4. recover

Also add unit test.
  • Loading branch information
qiluo-msft committed Feb 15, 2023
1 parent e60a64c commit 7147354
Show file tree
Hide file tree
Showing 4 changed files with 118 additions and 3 deletions.
5 changes: 4 additions & 1 deletion src/sonic_ax_impl/mibs/ietf/rfc1213.py
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,10 @@ def update_data(self):
ipn = ipaddress.ip_network(ipnstr)
ent = Namespace.dbs_get_all(self.db_conn, mibs.APPL_DB, routestr, blocking=False)
if ent:
nexthops = ent["nexthop"]
nexthops = ent.get("nexthop", None)
if nexthops is None:
mibs.logger.warning("Route has no nexthop: {} {}".format(routestr, str(ent)))
continue
for nh in nexthops.split(','):
# TODO: if ipn contains IP range, create more sub_id here
sub_id = ip2byte_tuple(ipn.network_address)
Expand Down
10 changes: 8 additions & 2 deletions src/sonic_ax_impl/mibs/ietf/rfc4292.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,14 @@ def update_data(self):
ent = db_conn.get_all(mibs.APPL_DB, route_str, blocking=False)
if not ent:
continue
nexthops = ent["nexthop"]
ifnames = ent["ifname"]
nexthops = ent.get("nexthop", None)
if nexthops is None:
mibs.logger.warning("Route has no nexthop: {} {}".format(route_str, str(ent)))
continue
ifnames = ent.get("ifname", None)
if ifnames is None:
mibs.logger.warning("Route has no ifname: {} {}".format(route_str, str(ent)))
continue
for nh, ifn in zip(nexthops.split(','), ifnames.split(',')):
## Ignore non front panel interfaces
## TODO: non front panel interfaces should not be in APPL_DB at very beginning
Expand Down
45 changes: 45 additions & 0 deletions tests/test_rfc1213.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import os
import sys
from unittest import TestCase

if sys.version_info.major == 3:
from unittest import mock
else:
import mock

modules_path = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
sys.path.insert(0, os.path.join(modules_path, 'src'))

from sonic_ax_impl.mibs.ietf.rfc1213 import NextHopUpdater

class TestNextHopUpdater(TestCase):

@mock.patch('sonic_ax_impl.mibs.Namespace.dbs_keys', mock.MagicMock(return_value=(["ROUTE_TABLE:0.0.0.0/0"])))
@mock.patch('sonic_ax_impl.mibs.Namespace.dbs_get_all', mock.MagicMock(return_value=({"nexthop": "10.0.0.1,10.0.0.3", "ifname": "Ethernet0,Ethernet4"})))
def test_NextHopUpdater_route_has_next_hop(self):
updater = NextHopUpdater()

with mock.patch('sonic_ax_impl.mibs.logger.warning') as mocked_warning:
updater.update_data()

# check warning
mocked_warning.assert_not_called()

self.assertTrue(len(updater.route_list) == 1)
self.assertTrue(updater.route_list[0] == (0,0,0,0))

@mock.patch('sonic_ax_impl.mibs.Namespace.dbs_keys', mock.MagicMock(return_value=(["ROUTE_TABLE:0.0.0.0/0"])))
@mock.patch('sonic_ax_impl.mibs.Namespace.dbs_get_all', mock.MagicMock(return_value=({"ifname": "Ethernet0,Ethernet4"})))
def test_NextHopUpdater_route_no_next_hop(self):
updater = NextHopUpdater()

with mock.patch('sonic_ax_impl.mibs.logger.warning') as mocked_warning:
updater.update_data()

# check warning
expected = [
mock.call("Route has no nexthop: ROUTE_TABLE:0.0.0.0/0 {'ifname': 'Ethernet0,Ethernet4'}")
]
mocked_warning.assert_has_calls(expected)

self.assertTrue(len(updater.route_list) == 0)
61 changes: 61 additions & 0 deletions tests/test_rfc4292.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import os
import sys
from unittest import TestCase

if sys.version_info.major == 3:
from unittest import mock
else:
import mock

modules_path = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
sys.path.insert(0, os.path.join(modules_path, 'src'))

from sonic_ax_impl.mibs.ietf.rfc4292 import RouteUpdater

class TestRouteUpdater(TestCase):

@mock.patch('sonic_py_common.multi_asic.get_all_namespaces', mock.MagicMock(return_value=({"front_ns": ['']})))
@mock.patch('swsscommon.swsscommon.SonicV2Connector.get_all', mock.MagicMock(return_value=({"nexthop": "10.0.0.1", "ifname": "Ethernet0"})))
def test_RouteUpdater_route_has_next_hop_and_iframe(self):
updater = RouteUpdater()

with mock.patch('sonic_ax_impl.mibs.logger.warning') as mocked_warning:
updater.update_data()

# check warning
mocked_warning.assert_not_called()

self.assertTrue(len(updater.route_dest_list) == 1)
self.assertTrue(updater.route_dest_list[0] == (0, 0, 0, 0, 0, 0, 0, 0, 0, 10, 0, 0, 1))

@mock.patch('sonic_py_common.multi_asic.get_all_namespaces', mock.MagicMock(return_value=({"front_ns": ['']})))
@mock.patch('swsscommon.swsscommon.SonicV2Connector.get_all', mock.MagicMock(return_value=({"ifname": "Ethernet0"})))
def test_RouteUpdater_route_no_next_hop(self):
updater = RouteUpdater()

with mock.patch('sonic_ax_impl.mibs.logger.warning') as mocked_warning:
updater.update_data()

# check warning
expected = [
mock.call("Route has no nexthop: ROUTE_TABLE:0.0.0.0/0 {'ifname': 'Ethernet0'}")
]
mocked_warning.assert_has_calls(expected)

self.assertTrue(len(updater.route_dest_list) == 0)

@mock.patch('sonic_py_common.multi_asic.get_all_namespaces', mock.MagicMock(return_value=({"front_ns": ['']})))
@mock.patch('swsscommon.swsscommon.SonicV2Connector.get_all', mock.MagicMock(return_value=({"nexthop": "10.0.0.1"})))
def test_RouteUpdater_route_no_iframe(self):
updater = RouteUpdater()

with mock.patch('sonic_ax_impl.mibs.logger.warning') as mocked_warning:
updater.update_data()

# check warning
expected = [
mock.call("Route has no ifname: ROUTE_TABLE:0.0.0.0/0 {'nexthop': '10.0.0.1'}")
]
mocked_warning.assert_has_calls(expected)

self.assertTrue(len(updater.route_dest_list) == 0)

0 comments on commit 7147354

Please sign in to comment.