Skip to content

Commit fb605a1

Browse files
StormLiangMSStormLiangMS
committed
address comments
1 parent f7ffa6c commit fb605a1

File tree

3 files changed

+34
-70
lines changed

3 files changed

+34
-70
lines changed

‎src/sonic-bgpcfgd/bgpcfgd/main.py

+1-2
Original file line numberDiff line numberDiff line change
@@ -63,8 +63,7 @@ def do_work():
6363
StaticRouteMgr(common_objs, "CONFIG_DB", "STATIC_ROUTE"),
6464
# Route Advertisement Managers
6565
AdvertiseRouteMgr(common_objs, "STATE_DB", swsscommon.STATE_ADVERTISE_NETWORK_TABLE_NAME),
66-
# TBD, Change to schema name from swsscommon (swsscommon.APP_BGP_PROFILE_TABLE_NAME) when the submodule is advanced.
67-
RouteMapMgr(common_objs, "APPL_DB", "BGP_PROFILE_TABLE"),
66+
RouteMapMgr(common_objs, "APPL_DB", swsscommon.APP_BGP_PROFILE_TABLE_NAME),
6867
]
6968
runner = Runner(common_objs['cfg_mgr'])
7069
for mgr in managers:

‎src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py

+18-53
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,11 @@
33
from swsscommon import swsscommon
44
from .managers_rm import ROUTE_MAPS
55
import ipaddress
6-
from .log import log_info, log_err
6+
from .log import log_info, log_err, log_debug
77

88

99
class AdvertiseRouteMgr(Manager):
10-
"""This class Advertises routes when ADVERTISE_NETWORK_TABLE in STATE_DB is updated"""
10+
""" This class Advertises routes when ADVERTISE_NETWORK_TABLE in STATE_DB is updated """
1111

1212
def __init__(self, common_objs, db, table):
1313
"""
@@ -23,70 +23,38 @@ def __init__(self, common_objs, db, table):
2323
table,
2424
)
2525

26-
self.directory.subscribe(
27-
[
28-
("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"),
29-
],
30-
self.on_bgp_asn_change,
31-
)
26+
self.directory.subscribe([("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"),], self.on_bgp_asn_change)
3227
self.advertised_routes = dict()
3328

29+
3430
OP_DELETE = "DELETE"
3531
OP_ADD = "ADD"
3632

3733
def set_handler(self, key, data):
38-
log_info("AdvertiseRouteMgr:: set handler")
39-
if not self._set_handler_validate(key, data):
34+
log_debug("AdvertiseRouteMgr:: set handler")
35+
if not self.__set_handler_validate(key, data):
4036
return True
4137
vrf, ip_prefix = self.split_key(key)
4238
self.add_route_advertisement(vrf, ip_prefix, data)
4339

4440
return True
4541

4642
def del_handler(self, key):
47-
log_info("AdvertiseRouteMgr:: del handler")
48-
if not self._del_handler_validate(key):
49-
return
43+
log_debug("AdvertiseRouteMgr:: del handler")
5044
vrf, ip_prefix = self.split_key(key)
5145
self.remove_route_advertisement(vrf, ip_prefix)
5246

53-
def _ip_addr_validate(self, key):
54-
if key:
55-
_, ip_prefix = self.split_key(key)
56-
ip_prefix = ip_prefix.split("/")
57-
if len(ip_prefix) != 2:
58-
log_err("BGPAdvertiseRouteMgr:: No valid ip prefix for advertised route %s" % key)
59-
return False
60-
try:
61-
ip = ipaddress.ip_address(ip_prefix[0])
62-
if ip.version == 4 and int(ip_prefix[1]) not in range(0, 33):
63-
log_err(
64-
"BGPAdvertiseRouteMgr:: ipv4 prefix %s is illegal for advertised route %s" % (ip_prefix[1], key)
65-
)
66-
return False
67-
if ip.version == 6 and int(ip_prefix[1]) not in range(0, 129):
68-
log_err(
69-
"BGPAdvertiseRouteMgr:: ipv6 prefix %s is illegal for advertised route %s" % (ip_prefix[1], key)
70-
)
71-
return False
72-
except ValueError:
73-
log_err("BGPAdvertiseRouteMgr:: No valid ip %s for advertised route %s" % (ip_prefix[0], key))
74-
return False
75-
else:
76-
return False
77-
return True
78-
79-
def _set_handler_validate(self, key, data):
47+
def __set_handler_validate(self, key, data):
8048
if data:
8149
if ("profile" in data and data["profile"] in ROUTE_MAPS) or data == {"":""}:
82-
return self._ip_addr_validate(key)
50+
"""
51+
APP which config the data should be responsible to pass a valid IP prefix
52+
"""
53+
return True
8354

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

87-
def _del_handler_validate(self, key):
88-
return self._ip_addr_validate(key)
89-
9058
def add_route_advertisement(self, vrf, ip_prefix, data):
9159
if self.directory.path_exist("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"):
9260
if not self.advertised_routes.get(vrf, dict()):
@@ -110,9 +78,8 @@ def remove_route_advertisement(self, vrf, ip_prefix):
11078

11179
def advertise_route_commands(self, ip_prefix, vrf, op, data=None):
11280
is_ipv6 = TemplateFabric.is_ipv6(ip_prefix)
113-
bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"][
114-
"bgp_asn"
115-
]
81+
bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]["bgp_asn"]
82+
11683
cmd_list = []
11784
if vrf == "default":
11885
cmd_list.append("router bgp %s" % bgp_asn)
@@ -123,24 +90,22 @@ def advertise_route_commands(self, ip_prefix, vrf, op, data=None):
12390

12491
if data and "profile" in data:
12592
cmd_list.append(" network %s route-map %s" % (ip_prefix, "%s_RM" % data["profile"]))
126-
log_info(
93+
log_debug(
12794
"BGPAdvertiseRouteMgr:: Update bgp %s network %s with route-map %s"
12895
% (bgp_asn, vrf + "|" + ip_prefix, "%s_RM" % data["profile"])
12996
)
13097
else:
13198
cmd_list.append(" %snetwork %s" % ("no " if op == self.OP_DELETE else "", ip_prefix))
132-
log_info(
99+
log_debug(
133100
"BGPAdvertiseRouteMgr:: %sbgp %s network %s"
134101
% ("Remove " if op == self.OP_DELETE else "Update ", bgp_asn, vrf + "|" + ip_prefix)
135102
)
136103

137104
self.cfg_mgr.push_list(cmd_list)
138-
log_info("BGPAdvertiseRouteMgr::Done")
105+
log_debug("BGPAdvertiseRouteMgr::Done")
139106

140107
def bgp_network_import_check_commands(self, vrf, op):
141-
bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"][
142-
"bgp_asn"
143-
]
108+
bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]["bgp_asn"]
144109
cmd_list = []
145110
if vrf == "default":
146111
cmd_list.append("router bgp %s" % bgp_asn)

‎src/sonic-bgpcfgd/bgpcfgd/managers_rm.py

+15-15
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
from .manager import Manager
2-
from .log import log_info, log_err
2+
from .log import log_err, log_debug
33

44
ROUTE_MAPS = ["FROM_SDN_SLB_ROUTES"]
55

@@ -22,27 +22,27 @@ def __init__(self, common_objs, db, table):
2222
)
2323

2424
def set_handler(self, key, data):
25-
log_info("BGPRouteMapMgr:: set handler")
25+
log_debug("BGPRouteMapMgr:: set handler")
2626
"""Only need a name as the key, and community id as the data"""
27-
if not self._set_handler_validate(key, data):
27+
if not self.__set_handler_validate(key, data):
2828
return True
2929

30-
self._update_rm(key, data)
30+
self.__update_rm(key, data)
3131
return True
3232

3333
def del_handler(self, key):
34-
log_info("BGPRouteMapMgr:: del handler")
35-
if not self._del_handler_validate(key):
34+
log_debug("BGPRouteMapMgr:: del handler")
35+
if not self.__del_handler_validate(key):
3636
return
37-
self._remove_rm(key)
37+
self.__remove_rm(key)
3838

39-
def _remove_rm(self, rm):
39+
def __remove_rm(self, rm):
4040
cmds = ["no route-map %s permit 100" % ("%s_RM" % rm)]
41-
log_info("BGPRouteMapMgr:: remove route-map %s" % ("%s_RM" % rm))
41+
log_debug("BGPRouteMapMgr:: remove route-map %s" % ("%s_RM" % rm))
4242
self.cfg_mgr.push_list(cmds)
43-
log_info("BGPRouteMapMgr::Done")
43+
log_debug("BGPRouteMapMgr::Done")
4444

45-
def _set_handler_validate(self, key, data):
45+
def __set_handler_validate(self, key, data):
4646
if key not in ROUTE_MAPS:
4747
log_err("BGPRouteMapMgr:: Invalid key for route-map %s" % key)
4848
return False
@@ -65,14 +65,14 @@ def _set_handler_validate(self, key, data):
6565

6666
return True
6767

68-
def _del_handler_validate(self, key):
68+
def __del_handler_validate(self, key):
6969
if key not in ROUTE_MAPS:
7070
log_err("BGPRouteMapMgr:: Invalid key for route-map %s" % key)
7171
return False
7272
return True
7373

74-
def _update_rm(self, rm, data):
74+
def __update_rm(self, rm, data):
7575
cmds = ["route-map %s permit 100" % ("%s_RM" % rm), " set community %s" % data["community_id"]]
76-
log_info("BGPRouteMapMgr:: update route-map %s community %s" % ("%s_RM" % rm, data["community_id"]))
76+
log_debug("BGPRouteMapMgr:: update route-map %s community %s" % ("%s_RM" % rm, data["community_id"]))
7777
self.cfg_mgr.push_list(cmds)
78-
log_info("BGPRouteMapMgr::Done")
78+
log_debug("BGPRouteMapMgr::Done")

0 commit comments

Comments
 (0)