Skip to content

Commit

Permalink
[bgpcfgd] Redistribute static routes (#7492)
Browse files Browse the repository at this point in the history 
Why I did it
Enable redistribution of static routes

How I did it
Enable redistribution of static routes when the first route is added to STATIC_ROUTE table of Config_DB and disable the redistribution when the last route is removed from STATIC_ROUTE table.
  • Loading branch information
@shi-su @qiluo-msft
shi-su authored and qiluo-msft committed Jun 16, 2021
1 parent fa321f1 commit 15bc3c3
Show file tree
Hide file tree
Showing 2 changed files with 133 additions and 20 deletions.
27 changes: 26 additions & 1 deletion src/sonic-bgpcfgd/bgpcfgd/managers_static_rt.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
from .manager import Manager
from .template import TemplateFabric
import socket
from swsscommon import swsscommon

class StaticRouteMgr(Manager):
""" This class updates static routes when STATIC_ROUTE table is updated """
Expand All @@ -15,7 +16,7 @@ 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,
)
Expand Down Expand Up @@ -44,6 +45,18 @@ def set_handler(self, key, data):
log_crit("Got an exception %s: Traceback: %s" % (str(exc), traceback.format_exc()))
return False

# 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)
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")

if cmd_list:
self.cfg_mgr.push_list(cmd_list)
log_debug("Static route {} is scheduled for updates".format(key))
Expand All @@ -63,6 +76,18 @@ def del_handler(self, key):
cur_nh_set = self.static_routes.get(vrf, {}).get(ip_prefix, IpNextHopSet(is_ipv6))
cmd_list = self.static_route_commands(ip_nh_set, cur_nh_set, ip_prefix, vrf)

# 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 cmd_list:
self.cfg_mgr.push_list(cmd_list)
log_debug("Static route {} is scheduled for updates".format(key))
Expand Down
126 changes: 107 additions & 19 deletions src/sonic-bgpcfgd/tests/test_static_rt.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from bgpcfgd.template import TemplateFabric
from bgpcfgd.managers_static_rt import StaticRouteMgr
from collections import Counter
from swsscommon import swsscommon

def constructor():
cfg_mgr = MagicMock()
Expand All @@ -16,6 +17,7 @@ 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"})
assert len(mgr.static_routes) == 0

return mgr
Expand All @@ -28,9 +30,9 @@ def push_list(cmds):
max_del_idx = -1
min_set_idx = len(cmds)
for idx in range(len(cmds)):
if cmds[idx].startswith('no') and idx > max_del_idx:
if cmds[idx].startswith('no ip') and idx > max_del_idx:
max_del_idx = idx
if not cmds[idx].startswith('no') and idx < min_set_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
Expand Down Expand Up @@ -59,7 +61,12 @@ def test_set():
}),
True,
[
"ip route 10.1.0.0/24 10.0.0.57"
"ip route 10.1.0.0/24 10.0.0.57",
"router bgp 65100",
" address-family ipv4",
" redistribute static",
" address-family ipv6",
" redistribute static"
]
)

Expand All @@ -77,7 +84,12 @@ def test_set_nhvrf():
}),
True,
[
"ip route 10.1.1.0/24 10.0.0.57 PortChannel0001 10 nexthop-vrf nh_vrf"
"ip route 10.1.1.0/24 10.0.0.57 PortChannel0001 10 nexthop-vrf nh_vrf",
"router bgp 65100",
" address-family ipv4",
" redistribute static",
" address-family ipv6",
" redistribute static"
]
)

Expand All @@ -95,7 +107,12 @@ def test_set_blackhole():
}),
True,
[
"ip route 10.1.2.0/24 blackhole 10"
"ip route 10.1.2.0/24 blackhole 10",
"router bgp 65100",
" address-family ipv4",
" redistribute static",
" address-family ipv6",
" redistribute static"
]
)

Expand All @@ -113,7 +130,12 @@ def test_set_vrf():
}),
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.57 PortChannel0001 10 nexthop-vrf nh_vrf vrf vrfRED",
"router bgp 65100 vrf vrfRED",
" address-family ipv4",
" redistribute static",
" address-family ipv6",
" redistribute static"
]
)

Expand All @@ -131,7 +153,12 @@ def test_set_ipv6():
}),
True,
[
"ipv6 route fc00:10::/64 fc00::72 PortChannel0001 10"
"ipv6 route fc00:10::/64 fc00::72 PortChannel0001 10",
"router bgp 65100",
" address-family ipv4",
" redistribute static",
" address-family ipv6",
" redistribute static"
]
)

Expand All @@ -150,7 +177,12 @@ def test_set_nh_only():
[
"ip route 10.1.3.0/24 10.0.0.57 10 nexthop-vrf nh_vrf vrf vrfRED",
"ip route 10.1.3.0/24 10.0.0.59 20 vrf vrfRED",
"ip route 10.1.3.0/24 10.0.0.61 30 nexthop-vrf default vrf vrfRED"
"ip route 10.1.3.0/24 10.0.0.61 30 nexthop-vrf default vrf vrfRED",
"router bgp 65100 vrf vrfRED",
" address-family ipv4",
" redistribute static",
" address-family ipv6",
" redistribute static"
]
)

Expand All @@ -169,7 +201,12 @@ def test_set_ifname_only():
[
"ip route 10.1.3.0/24 PortChannel0001 10 nexthop-vrf nh_vrf vrf vrfRED",
"ip route 10.1.3.0/24 PortChannel0002 20 vrf vrfRED",
"ip route 10.1.3.0/24 PortChannel0003 30 nexthop-vrf default vrf vrfRED"
"ip route 10.1.3.0/24 PortChannel0003 30 nexthop-vrf default vrf vrfRED",
"router bgp 65100 vrf vrfRED",
" address-family ipv4",
" redistribute static",
" address-family ipv6",
" redistribute static"
]
)

Expand All @@ -189,7 +226,12 @@ def test_set_with_empty_ifname():
[
"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 20 vrf vrfRED",
"ip route 10.1.3.0/24 10.0.0.61 PortChannel0003 30 nexthop-vrf default vrf vrfRED"
"ip route 10.1.3.0/24 10.0.0.61 PortChannel0003 30 nexthop-vrf default vrf vrfRED",
"router bgp 65100 vrf vrfRED",
" address-family ipv4",
" redistribute static",
" address-family ipv6",
" redistribute static"
]
)

Expand All @@ -209,7 +251,12 @@ def test_set_with_empty_nh():
[
"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 PortChannel0002 20 vrf vrfRED",
"ip route 10.1.3.0/24 PortChannel0003 30 nexthop-vrf default vrf vrfRED"
"ip route 10.1.3.0/24 PortChannel0003 30 nexthop-vrf default vrf vrfRED",
"router bgp 65100 vrf vrfRED",
" address-family ipv4",
" redistribute static",
" address-family ipv6",
" redistribute static"
]
)

Expand All @@ -229,7 +276,12 @@ def test_set_del():
[
"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"
"ip route 10.1.3.0/24 10.0.0.61 PortChannel0003 30 nexthop-vrf default vrf vrfRED",
"router bgp 65100 vrf vrfRED",
" address-family ipv4",
" redistribute static",
" address-family ipv6",
" redistribute static"
]
)
set_del_test(
Expand All @@ -240,7 +292,12 @@ def test_set_del():
[
"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"
"no ip route 10.1.3.0/24 10.0.0.61 PortChannel0003 30 nexthop-vrf default vrf vrfRED",
"router bgp 65100 vrf vrfRED",
" address-family ipv4",
" no redistribute static",
" address-family ipv6",
" no redistribute static"
]
)
set_del_test(
Expand All @@ -257,7 +314,12 @@ def test_set_del():
[
"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"
"ip route 10.1.3.0/24 10.0.0.61 PortChannel0003 30 nexthop-vrf default vrf vrfRED",
"router bgp 65100 vrf vrfRED",
" address-family ipv4",
" redistribute static",
" address-family ipv6",
" redistribute static"
]
)

Expand All @@ -277,7 +339,12 @@ def test_set_same_route():
[
"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"
"ip route 10.1.3.0/24 10.0.0.61 PortChannel0003 30 nexthop-vrf default vrf vrfRED",
"router bgp 65100 vrf vrfRED",
" address-family ipv4",
" redistribute static",
" address-family ipv6",
" redistribute static"
]
)
set_del_test(
Expand Down Expand Up @@ -317,7 +384,12 @@ def test_set_add_del_nh():
[
"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"
"ip route 10.1.3.0/24 10.0.0.61 PortChannel0003 30 nexthop-vrf default vrf vrfRED",
"router bgp 65100 vrf vrfRED",
" address-family ipv4",
" redistribute static",
" address-family ipv6",
" redistribute static"
]
)
set_del_test(
Expand Down Expand Up @@ -368,7 +440,12 @@ def test_set_add_del_nh_ethernet():
[
"ip route 20.1.3.0/24 20.0.0.57 Ethernet4 10 nexthop-vrf default",
"ip route 20.1.3.0/24 20.0.0.59 Ethernet8 20",
"ip route 20.1.3.0/24 20.0.0.61 Ethernet12 30 nexthop-vrf default"
"ip route 20.1.3.0/24 20.0.0.61 Ethernet12 30 nexthop-vrf default",
"router bgp 65100",
" address-family ipv4",
" redistribute static",
" address-family ipv6",
" redistribute static"
]
)
set_del_test(
Expand Down Expand Up @@ -416,7 +493,12 @@ def test_set_no_action(mocked_log_debug):
}),
True,
[
"ip route 10.1.1.0/24 blackhole"
"ip route 10.1.1.0/24 blackhole",
"router bgp 65100",
" address-family ipv4",
" redistribute static",
" address-family ipv6",
" redistribute static"
]
)

Expand Down Expand Up @@ -470,7 +552,13 @@ def test_set_invalid_blackhole(mocked_log_err):
"blackhole": "false",
}),
True,
[]
[
"router bgp 65100",
" address-family ipv4",
" redistribute static",
" address-family ipv6",
" redistribute static"
]
)
mocked_log_err.assert_called_with("Mandatory attribute not found for nexthop")

Expand Down

0 comments on commit 15bc3c3

Please sign in to comment.