Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bgpcfgd] ECMP overlay VxLan with BGP support #10716

Merged
merged 13 commits into from
May 19, 2022
3 changes: 3 additions & 0 deletions src/sonic-bgpcfgd/bgpcfgd/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
from .managers_intf import InterfaceMgr
from .managers_setsrc import ZebraSetSrc
from .managers_static_rt import StaticRouteMgr
from .managers_rm import RouteMapMgr
from .runner import Runner, signal_handler
from .template import TemplateFabric
from .utils import read_constants
Expand Down Expand Up @@ -62,6 +63,8 @@ def do_work():
StaticRouteMgr(common_objs, "CONFIG_DB", "STATIC_ROUTE"),
# Route Advertisement Managers
AdvertiseRouteMgr(common_objs, "STATE_DB", swsscommon.STATE_ADVERTISE_NETWORK_TABLE_NAME),
# TBD, Change to schema name from swsscommon (swsscommon.APP_BGP_PROFILE_TABLE_NAME) when the submodule is advanced.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix TBD before merge

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked, the code is already advanced in the master branch. Will make sure which is advanced before cherry pick to 202012 branch.

RouteMapMgr(common_objs, "APPL_DB", "BGP_PROFILE_TABLE"),
]
runner = Runner(common_objs['cfg_mgr'])
for mgr in managers:
Expand Down
128 changes: 95 additions & 33 deletions src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
from .manager import Manager
from .template import TemplateFabric
from swsscommon import swsscommon
from .managers_rm import ROUTE_MAPS
import ipaddress
from .log import log_info, log_err


class AdvertiseRouteMgr(Manager):
""" This class Advertises routes when ADVERTISE_NETWORK_TABLE in STATE_DB is updated """
"""This class Advertises routes when ADVERTISE_NETWORK_TABLE in STATE_DB is updated"""

def __init__(self, common_objs, db, table):
"""
Initialize the object
Expand All @@ -18,82 +22,140 @@ def __init__(self, common_objs, db, table):
db,
table,
)

self.directory.subscribe([("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"),], self.on_bgp_asn_change)
self.advertised_routes = dict()


OP_DELETE = 'DELETE'
OP_ADD = 'ADD'
self.directory.subscribe(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we retain the original code as-is if its not related to this PR. This will help in reviewing just the changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, will keep original code as much as possible. Only change is to use " instead of ' to keep the style consistent.

[
("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"),
],
self.on_bgp_asn_change,
)
self.advertised_routes = dict()

OP_DELETE = "DELETE"
OP_ADD = "ADD"

def set_handler(self, key, data):
log_info("AdvertiseRouteMgr:: set handler")
if not self._set_handler_validate(key, data):
return True
vrf, ip_prefix = self.split_key(key)
self.add_route_advertisement(vrf, ip_prefix)
self.add_route_advertisement(vrf, ip_prefix, data)

return True


def del_handler(self, key):
log_info("AdvertiseRouteMgr:: del handler")
if not self._del_handler_validate(key):
return
vrf, ip_prefix = self.split_key(key)
self.remove_route_advertisement(vrf, ip_prefix)

def _ip_addr_validate(self, key):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this mandatory? I mean APP_DB entries shall be already validated by the producer. So may be this is redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good one, add one comment in the code.

if key:
_, ip_prefix = self.split_key(key)
ip_prefix = ip_prefix.split("/")
if len(ip_prefix) != 2:
log_err("BGPAdvertiseRouteMgr:: No valid ip prefix for advertised route %s" % key)
return False
try:
ip = ipaddress.ip_address(ip_prefix[0])
if ip.version == 4 and int(ip_prefix[1]) not in range(0, 33):
log_err(
"BGPAdvertiseRouteMgr:: ipv4 prefix %s is illegal for advertised route %s" % (ip_prefix[1], key)
)
return False
if ip.version == 6 and int(ip_prefix[1]) not in range(0, 129):
log_err(
"BGPAdvertiseRouteMgr:: ipv6 prefix %s is illegal for advertised route %s" % (ip_prefix[1], key)
)
return False
except ValueError:
log_err("BGPAdvertiseRouteMgr:: No valid ip %s for advertised route %s" % (ip_prefix[0], key))
return False
else:
return False
return True

def _set_handler_validate(self, key, data):
if data:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If data is not present, we need to advertise without a community string (for backward compatibility). Can you please confirm.?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@prsunny confirmed, the check after the 'or' is for the case which doesn't have the community string.
if ("profile" in data and data["profile"] in ROUTE_MAPS) or data == {"":""}:

if ("profile" in data and data["profile"] in ROUTE_MAPS) or data == {"":""}:
return self._ip_addr_validate(key)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, this could be just returning 'true' here. IP prefix should already be validated, and we may just be adding extra delay here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.


log_err("BGPAdvertiseRouteMgr:: Invalid data %s for advertised route %s" % (data, key))
return False

def add_route_advertisement(self, vrf, ip_prefix):
def _del_handler_validate(self, key):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be removed as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

return self._ip_addr_validate(key)

def add_route_advertisement(self, vrf, ip_prefix, data):
if self.directory.path_exist("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"):
if not self.advertised_routes.get(vrf, set()):
if not self.advertised_routes.get(vrf, dict()):
self.bgp_network_import_check_commands(vrf, self.OP_ADD)
self.advertise_route_commands(ip_prefix, vrf, self.OP_ADD)

self.advertised_routes.setdefault(vrf, set()).add(ip_prefix)
self.advertise_route_commands(ip_prefix, vrf, self.OP_ADD, data)

self.advertised_routes.setdefault(vrf, dict()).update({ip_prefix: data})

def remove_route_advertisement(self, vrf, ip_prefix):
self.advertised_routes.setdefault(vrf, set()).discard(ip_prefix)
if not self.advertised_routes.get(vrf, set()):
if ip_prefix not in self.advertised_routes.get(vrf, dict()):
log_info("BGPAdvertiseRouteMgr:: %s|%s does not exist" % (vrf, ip_prefix))
return
self.advertised_routes.get(vrf, dict()).pop(ip_prefix)
if not self.advertised_routes.get(vrf, dict()):
self.advertised_routes.pop(vrf, None)

if self.directory.path_exist("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"):
if not self.advertised_routes.get(vrf, set()):
if not self.advertised_routes.get(vrf, dict()):
self.bgp_network_import_check_commands(vrf, self.OP_DELETE)
self.advertise_route_commands(ip_prefix, vrf, self.OP_DELETE)


def advertise_route_commands(self, ip_prefix, vrf, op):
def advertise_route_commands(self, ip_prefix, vrf, op, data=None):
is_ipv6 = TemplateFabric.is_ipv6(ip_prefix)
bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]["bgp_asn"]

bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"][
"bgp_asn"
]
cmd_list = []
if vrf == 'default':
if vrf == "default":
cmd_list.append("router bgp %s" % bgp_asn)
else:
cmd_list.append("router bgp %s vrf %s" % (bgp_asn, vrf))

cmd_list.append(" address-family %s unicast" % ("ipv6" if is_ipv6 else "ipv4"))
cmd_list.append(" %snetwork %s" % ('no ' if op == self.OP_DELETE else '', ip_prefix))

self.cfg_mgr.push_list(cmd_list)
if data and "profile" in data:
cmd_list.append(" network %s route-map %s" % (ip_prefix, "%s_RM" % data["profile"]))
log_info(
"BGPAdvertiseRouteMgr:: Update bgp %s network %s with route-map %s"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on scale, there could be significant number of route updates. So please reduce the log-level and ensure it won't show up in regular flow unless explicitly enabled

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good one, change to debug.

% (bgp_asn, vrf + "|" + ip_prefix, "%s_RM" % data["profile"])
)
else:
cmd_list.append(" %snetwork %s" % ("no " if op == self.OP_DELETE else "", ip_prefix))
log_info(
"BGPAdvertiseRouteMgr:: %sbgp %s network %s"
% ("Remove " if op == self.OP_DELETE else "Update ", bgp_asn, vrf + "|" + ip_prefix)
)

self.cfg_mgr.push_list(cmd_list)
log_info("BGPAdvertiseRouteMgr::Done")

def bgp_network_import_check_commands(self, vrf, op):
bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]["bgp_asn"]
bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"][
"bgp_asn"
]
cmd_list = []
if vrf == 'default':
if vrf == "default":
cmd_list.append("router bgp %s" % bgp_asn)
else:
cmd_list.append("router bgp %s vrf %s" % (bgp_asn, vrf))
cmd_list.append(" %sbgp network import-check" % ('' if op == self.OP_DELETE else 'no '))
cmd_list.append(" %sbgp network import-check" % ("" if op == self.OP_DELETE else "no "))

self.cfg_mgr.push_list(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, ip_prefixes in self.advertised_routes.items():
self.bgp_network_import_check_commands(vrf, self.OP_ADD)
for ip_prefix in ip_prefixes:
self.add_route_advertisement(vrf, ip_prefix)

self.add_route_advertisement(vrf, ip_prefix, ip_prefixes[ip_prefix])

@staticmethod
def split_key(key):
Expand All @@ -102,7 +164,7 @@ def split_key(key):
:param key: key to split
:return: vrf name extracted from the key, ip prefix extracted from the key
"""
if '|' not in key:
return 'default', key
if "|" not in key:
return "default", key
else:
return tuple(key.split('|', 1))
return tuple(key.split("|", 1))
78 changes: 78 additions & 0 deletions src/sonic-bgpcfgd/bgpcfgd/managers_rm.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
from .manager import Manager
from .log import log_info, log_err

ROUTE_MAPS = ["FROM_SDN_SLB_ROUTES"]


class RouteMapMgr(Manager):
"""This class add route-map when BGP_PROFILE_TABLE in APPL_DB is updated"""

def __init__(self, common_objs, db, table):
"""
Initialize the object
:param common_objs: common object dictionary
:param db: name of the db
:param table: name of the table in the db
"""
super(RouteMapMgr, self).__init__(
common_objs,
[],
db,
table,
)

def set_handler(self, key, data):
log_info("BGPRouteMapMgr:: set handler")
"""Only need a name as the key, and community id as the data"""
if not self._set_handler_validate(key, data):
return True

self._update_rm(key, data)
return True

def del_handler(self, key):
log_info("BGPRouteMapMgr:: del handler")
if not self._del_handler_validate(key):
return
self._remove_rm(key)

def _remove_rm(self, rm):
cmds = ["no route-map %s permit 100" % ("%s_RM" % rm)]
log_info("BGPRouteMapMgr:: remove route-map %s" % ("%s_RM" % rm))
self.cfg_mgr.push_list(cmds)
log_info("BGPRouteMapMgr::Done")

def _set_handler_validate(self, key, data):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets use consistent formatting for names. Can you check if its "__" vs "_" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, changed all internal function to "__"

if key not in ROUTE_MAPS:
log_err("BGPRouteMapMgr:: Invalid key for route-map %s" % key)
return False

if not data:
log_err("BGPRouteMapMgr:: data is None")
return False
community_ids = data["community_id"].split(":")
try:
if (
len(community_ids) != 2
or int(community_ids[0]) not in range(0, 65536)
or int(community_ids[1]) not in range(0, 65536)
):
log_err("BGPRouteMapMgr:: data %s doesn't include valid community id %s" % (data, community_ids))
return False
except ValueError:
log_err("BGPRouteMapMgr:: data %s includes illegal input" % (data))
return False

return True

def _del_handler_validate(self, key):
if key not in ROUTE_MAPS:
log_err("BGPRouteMapMgr:: Invalid key for route-map %s" % key)
return False
return True

def _update_rm(self, rm, data):
cmds = ["route-map %s permit 100" % ("%s_RM" % rm), " set community %s" % data["community_id"]]
log_info("BGPRouteMapMgr:: update route-map %s community %s" % ("%s_RM" % rm, data["community_id"]))
self.cfg_mgr.push_list(cmds)
log_info("BGPRouteMapMgr::Done")
Loading