Skip to content

Commit

Permalink
[bgpcfgd] Remove unnecessary dependency for StaticRouteMgr (sonic-net…
Browse files Browse the repository at this point in the history 
…#8037)

Why I did it
Static route configuration should not depend on BGP_ASN. Remove the dependency on BGP_ASN for StaticRouteMgr.
Fix sonic-net#8027

How I did it
Check if BGP_ASN field before configuring static route redistribution and wait until BGP_ASN is available to enable static route redistribution.

How to verify it
Add unit test to cover the scenario and verify the functionality on a virtual switch.
  • Loading branch information
@shi-su
shi-su authored and Carl Keene committed Aug 7, 2021
1 parent 1bfb597 commit 0a88e53
Show file tree
Hide file tree
Showing 2 changed files with 122 additions and 21 deletions.
61 changes: 42 additions & 19 deletions src/sonic-bgpcfgd/bgpcfgd/managers_static_rt.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,14 @@ def __init__(self, common_objs, db, table):
"""
super(StaticRouteMgr, self).__init__(
common_objs,
[("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"),],
[],
db,
table,
)


self.directory.subscribe([("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"),], self.on_bgp_asn_change)
self.static_routes = {}
self.vrf_pending_redistribution = set()

OP_DELETE = 'DELETE'
OP_ADD = 'ADD'
Expand All @@ -47,15 +49,10 @@ def set_handler(self, key, data):

# Enable redistribution of static routes when it is the first one get set
if not self.static_routes.get(vrf, {}):
log_debug("Enabling static route redistribution")
bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]["bgp_asn"]
if vrf == 'default':
cmd_list.append("router bgp %s" % bgp_asn)
if self.directory.path_exist("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"):
cmd_list.extend(self.enable_redistribution_command(vrf))
else:
cmd_list.append("router bgp %s vrf %s" % (bgp_asn, vrf))
for af in ["ipv4", "ipv6"]:
cmd_list.append(" address-family %s" % af)
cmd_list.append(" redistribute static")
self.vrf_pending_redistribution.add(vrf)

if cmd_list:
self.cfg_mgr.push_list(cmd_list)
Expand All @@ -78,15 +75,9 @@ def del_handler(self, key):

# Disable redistribution of static routes when it is the last one to delete
if self.static_routes.get(vrf, {}).keys() == {ip_prefix}:
log_debug("Disabling static route redistribution")
bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]["bgp_asn"]
if vrf == 'default':
cmd_list.append("router bgp %s" % bgp_asn)
else:
cmd_list.append("router bgp %s vrf %s" % (bgp_asn, vrf))
for af in ["ipv4", "ipv6"]:
cmd_list.append(" address-family %s" % af)
cmd_list.append(" no redistribute static")
if self.directory.path_exist("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"):
cmd_list.extend(self.disable_redistribution_command(vrf))
self.vrf_pending_redistribution.discard(vrf)

if cmd_list:
self.cfg_mgr.push_list(cmd_list)
Expand Down Expand Up @@ -135,6 +126,38 @@ def generate_command(self, op, ip_nh, ip_prefix, vrf):
' vrf {}'.format(vrf) if vrf != 'default' else ''
)

def enable_redistribution_command(self, vrf):
log_debug("Enabling static route redistribution")
cmd_list = []
bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]["bgp_asn"]
if vrf == 'default':
cmd_list.append("router bgp %s" % bgp_asn)
else:
cmd_list.append("router bgp %s vrf %s" % (bgp_asn, vrf))
for af in ["ipv4", "ipv6"]:
cmd_list.append(" address-family %s" % af)
cmd_list.append(" redistribute static")
return cmd_list

def disable_redistribution_command(self, vrf):
log_debug("Disabling static route redistribution")
cmd_list = []
bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]["bgp_asn"]
if vrf == 'default':
cmd_list.append("router bgp %s" % bgp_asn)
else:
cmd_list.append("router bgp %s vrf %s" % (bgp_asn, vrf))
for af in ["ipv4", "ipv6"]:
cmd_list.append(" address-family %s" % af)
cmd_list.append(" no redistribute static")
return cmd_list

def on_bgp_asn_change(self):
if self.directory.path_exist("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"):
for vrf in self.vrf_pending_redistribution:
self.cfg_mgr.push_list(self.enable_redistribution_command(vrf))
self.vrf_pending_redistribution.clear()

class IpNextHop:
def __init__(self, af_id, blackhole, dst_ip, if_name, dist, vrf):
zero_ip = lambda af: '0.0.0.0' if af == socket.AF_INET else '::'
Expand Down
82 changes: 80 additions & 2 deletions src/sonic-bgpcfgd/tests/test_static_rt.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from collections import Counter
from swsscommon import swsscommon

def constructor():
def constructor(skip_bgp_asn=False):
cfg_mgr = MagicMock()

common_objs = {
Expand All @@ -17,7 +17,8 @@ def constructor():
}

mgr = StaticRouteMgr(common_objs, "CONFIG_DB", "STATIC_ROUTE")
mgr.directory.put("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost", {"bgp_asn": "65100"})
if not skip_bgp_asn:
mgr.directory.put("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost", {"bgp_asn": "65100"})
assert len(mgr.static_routes) == 0

return mgr
Expand Down Expand Up @@ -573,3 +574,80 @@ def test_set_invalid_ipaddr():
False,
[]
)

def test_set_del_no_bgp_asn():
mgr = constructor(skip_bgp_asn=True)
set_del_test(
mgr,
"SET",
("vrfRED|10.1.3.0/24", {
"nexthop": "10.0.0.57,10.0.0.59,10.0.0.61",
"ifname": "PortChannel0001,PortChannel0002,PortChannel0003",
"distance": "10,20,30",
"nexthop-vrf": "nh_vrf,,default",
"blackhole": "false,false,false",
}),
True,
[
"ip route 10.1.3.0/24 10.0.0.57 PortChannel0001 10 nexthop-vrf nh_vrf vrf vrfRED",
"ip route 10.1.3.0/24 10.0.0.59 PortChannel0002 20 vrf vrfRED",
"ip route 10.1.3.0/24 10.0.0.61 PortChannel0003 30 nexthop-vrf default vrf vrfRED",
]
)
set_del_test(
mgr,
"DEL",
("vrfRED|10.1.3.0/24",),
True,
[
"no ip route 10.1.3.0/24 10.0.0.57 PortChannel0001 10 nexthop-vrf nh_vrf vrf vrfRED",
"no ip route 10.1.3.0/24 10.0.0.59 PortChannel0002 20 vrf vrfRED",
"no ip route 10.1.3.0/24 10.0.0.61 PortChannel0003 30 nexthop-vrf default vrf vrfRED",
]
)

def test_set_del_bgp_asn_change():
mgr = constructor(skip_bgp_asn=True)
set_del_test(
mgr,
"SET",
("vrfRED|10.1.3.0/24", {
"nexthop": "10.0.0.57,10.0.0.59,10.0.0.61",
"ifname": "PortChannel0001,PortChannel0002,PortChannel0003",
"distance": "10,20,30",
"nexthop-vrf": "nh_vrf,,default",
"blackhole": "false,false,false",
}),
True,
[
"ip route 10.1.3.0/24 10.0.0.57 PortChannel0001 10 nexthop-vrf nh_vrf vrf vrfRED",
"ip route 10.1.3.0/24 10.0.0.59 PortChannel0002 20 vrf vrfRED",
"ip route 10.1.3.0/24 10.0.0.61 PortChannel0003 30 nexthop-vrf default vrf vrfRED",
]
)

assert mgr.vrf_pending_redistribution == {"vrfRED"}

expected_cmds = [
"router bgp 65100 vrf vrfRED",
" address-family ipv4",
" redistribute static",
" address-family ipv6",
" redistribute static"
]
def push_list(cmds):
set_del_test.push_list_called = True
assert Counter(cmds) == Counter(expected_cmds) # check if commands are expected (regardless of the order)
max_del_idx = -1
min_set_idx = len(cmds)
for idx in range(len(cmds)):
if cmds[idx].startswith('no ip') and idx > max_del_idx:
max_del_idx = idx
if cmds[idx].startswith('ip') and idx < min_set_idx:
min_set_idx = idx
assert max_del_idx < min_set_idx, "DEL command comes after SET command" # DEL commands should be done first
return True
mgr.cfg_mgr.push_list = push_list
mgr.directory.put("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost", {"bgp_asn": "65100"})

assert not mgr.vrf_pending_redistribution

0 comments on commit 0a88e53

Please sign in to comment.