Skip to content

[config]Static routes to config_db #1534

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.

Sign up for GitHub

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

Merged
prsunny merged 19 commits into from
May 13, 2021
Merged

[config]Static routes to config_db #1534

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
47696b9
Write static routes to config_db
d-dashkov Mar 30, 2021
7de1946
Update route name for db
d-dashkov Mar 30, 2021
eb87991
Resolve conflicts
d-dashkov Mar 30, 2021
93220d9
Tab replaced with spaces
d-dashkov Mar 30, 2021
cab6c7a
Renamed entry and initial UT
d-dashkov Mar 31, 2021
fbff2e4
Added VRF prefix check and renamed VRF entry
d-dashkov Mar 31, 2021
1a02681
Depricated frr cfg
d-dashkov Apr 6, 2021
6392385
Updated UT
d-dashkov Apr 6, 2021
91723c8
Initial ECMP
d-dashkov Apr 12, 2021
091082b
Fix tests
d-dashkov Apr 12, 2021
bc26317
Update doc
d-dashkov Apr 12, 2021
605f455
Add comments
d-dashkov Apr 12, 2021
5e41d96
Merge branch 'sroute_to_configdb' of https://github.com/d-dashkov/son…
d-dashkov Apr 13, 2021
e4b14b0
Add ip validation
d-dashkov Apr 14, 2021
11d74e9
Del entire route at once
d-dashkov Apr 16, 2021
847399f
Add check on delete for ifname
d-dashkov Apr 20, 2021
319a3ea
Update entry
d-dashkov Apr 28, 2021
bc14735
Fix command with interface
d-dashkov Apr 28, 2021
fb6d0eb
Update configuration with "ifname" as "null"
d-dashkov May 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
286 changes: 198 additions & 88 deletions config/main.py
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import subprocess
import sys
import time
import itertools

from socket import AF_INET, AF_INET6
from minigraph import parse_device_desc_xml
Expand Down Expand Up @@ -783,6 +784,66 @@ def validate_mirror_session_config(config_db, session_name, dst_port, src_port,

return True

def cli_sroute_to_config(ctx, command_str, strict_nh = True):
if len(command_str) < 2 or len(command_str) > 9:
ctx.fail("argument is not in pattern prefix [vrf <vrf_name>] <A.B.C.D/M> nexthop <[vrf <vrf_name>] <A.B.C.D>>|<dev <dev_name>>!")
if "prefix" not in command_str:
ctx.fail("argument is incomplete, prefix not found!")
if "nexthop" not in command_str and strict_nh:
ctx.fail("argument is incomplete, nexthop not found!")

nexthop_str = None
config_entry = {}
vrf_name = ""

if "nexthop" in command_str:
idx = command_str.index("nexthop")
prefix_str = command_str[:idx]
nexthop_str = command_str[idx:]
else:
prefix_str = command_str[:]

if prefix_str:
if 'prefix' in prefix_str and 'vrf' in prefix_str:
# prefix_str: ['prefix', 'vrf', Vrf-name, ip]
vrf_name = prefix_str[2]
ip_prefix = prefix_str[3]
elif 'prefix' in prefix_str:
# prefix_str: ['prefix', ip]
ip_prefix = prefix_str[1]
else:
ctx.fail("prefix is not in pattern!")

if nexthop_str:
if 'nexthop' in nexthop_str and 'vrf' in nexthop_str:
# nexthop_str: ['nexthop', 'vrf', Vrf-name, ip]
config_entry["nexthop"] = nexthop_str[3]
config_entry["nexthop-vrf"] = nexthop_str[2]
elif 'nexthop' in nexthop_str and 'dev' in nexthop_str:
# nexthop_str: ['nexthop', 'dev', ifname]
config_entry["ifname"] = nexthop_str[2]
elif 'nexthop' in nexthop_str:
# nexthop_str: ['nexthop', ip]
config_entry["nexthop"] = nexthop_str[1]
else:
ctx.fail("nexthop is not in pattern!")

try:
ipaddress.ip_network(ip_prefix)
if 'nexthop' in config_entry:
nh = config_entry['nexthop'].split(',')
for ip in nh:
ipaddress.ip_address(ip)
except ValueError:
ctx.fail("ip address is not valid.")

if not vrf_name == "":
key = vrf_name + "|" + ip_prefix
else:
key = ip_prefix

return key, config_entry

def update_sonic_environment():
"""Prepare sonic environment variable using SONiC environment template file.
"""
Expand Down Expand Up @@ -3153,111 +3214,160 @@ def del_vrf_vni_map(ctx, vrfname):
@click.pass_context
def route(ctx):
"""route-related configuration tasks"""
pass
config_db = ConfigDBConnector()
config_db.connect()
ctx.obj = {}
ctx.obj['config_db'] = config_db
Copy link
Copy Markdown
Contributor

@arlakshm arlakshm Apr 29, 2021

Choose a reason for hiding this comment

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

can you add the support for multi asic ?
Similar to how it is done for interface command https://github.com/Azure/sonic-utilities/blob/08337aa7637b290bb8407c38b2a5dbe3e8383b3e/config/main.py#L2359

Copy link
Copy Markdown
Contributor

@prsunny prsunny May 4, 2021

Choose a reason for hiding this comment

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

@arlakshm, can we add multiasic as another enhancement PR?

Copy link
Copy Markdown
Contributor

@arlakshm arlakshm May 4, 2021

Choose a reason for hiding this comment

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

@prsunny, Sure. Can we create an issue or task to track this?

Copy link
Copy Markdown
Contributor

@prsunny prsunny May 11, 2021

Choose a reason for hiding this comment

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

Created issue - #1608


@route.command('add', context_settings={"ignore_unknown_options":True})
@click.argument('command_str', metavar='prefix [vrf <vrf_name>] <A.B.C.D/M> nexthop <[vrf <vrf_name>] <A.B.C.D>>|<dev <dev_name>>', nargs=-1, type=click.Path())
@click.pass_context
def add_route(ctx, command_str):
"""Add route command"""
if len(command_str) < 4 or len(command_str) > 9:
ctx.fail("argument is not in pattern prefix [vrf <vrf_name>] <A.B.C.D/M> nexthop <[vrf <vrf_name>] <A.B.C.D>>|<dev <dev_name>>!")
if "prefix" not in command_str:
ctx.fail("argument is incomplete, prefix not found!")
if "nexthop" not in command_str:
ctx.fail("argument is incomplete, nexthop not found!")
for i in range(0, len(command_str)):
if "nexthop" == command_str[i]:
prefix_str = command_str[:i]
nexthop_str = command_str[i:]
vrf_name = ""
cmd = 'sudo vtysh -c "configure terminal" -c "ip route'
if prefix_str:
if len(prefix_str) == 2:
prefix_mask = prefix_str[1]
cmd += ' {}'.format(prefix_mask)
elif len(prefix_str) == 4:
vrf_name = prefix_str[2]
prefix_mask = prefix_str[3]
cmd += ' {}'.format(prefix_mask)
config_db = ctx.obj['config_db']
key, route = cli_sroute_to_config(ctx, command_str)
Copy link
Copy Markdown
Contributor

@prsunny prsunny Apr 12, 2021

Choose a reason for hiding this comment

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

Now that we are writing to DB, lets check the correctness of key (like a valid ip prefix)


# If defined intf name, check if it belongs to interface
if 'ifname' in route:
Copy link
Copy Markdown
Contributor

@prsunny prsunny Apr 23, 2021

Choose a reason for hiding this comment

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

As commented by @arlakshm for PR #1535 , please check if ifname belongs to VLAN_INTERFACE, INTERFACE and PORTCHANNEL_INTERFACE. If not return failure as "IP interface doesn't exist"

if (not route['ifname'] in config_db.get_keys('VLAN_INTERFACE') and
not route['ifname'] in config_db.get_keys('INTERFACE') and
not route['ifname'] in config_db.get_keys('PORTCHANNEL_INTERFACE') and
not route['ifname'] == 'null'):
ctx.fail('interface {} doesn`t exist'.format(route['ifname']))

entry_counter = 1
if 'nexthop' in route:
entry_counter = len(route['nexthop'].split(','))

# Alignment in case the command contains several nexthop ip
for i in range(entry_counter):
if 'nexthop-vrf' in route:
if i > 0:
vrf = route['nexthop-vrf'].split(',')[0]
route['nexthop-vrf'] += ',' + vrf
else:
ctx.fail("prefix is not in pattern!")
if nexthop_str:
if len(nexthop_str) == 2:
ip = nexthop_str[1]
if vrf_name == "":
cmd += ' {}'.format(ip)
else:
cmd += ' {} vrf {}'.format(ip, vrf_name)
elif len(nexthop_str) == 3:
dev_name = nexthop_str[2]
if vrf_name == "":
cmd += ' {}'.format(dev_name)
route['nexthop-vrf'] = ''

if not 'nexthop' in route:
route['nexthop'] = ''

if 'ifname' in route:
if i > 0:
route['ifname'] += ','
else:
route['ifname'] = ''

# Set default values for distance and blackhole because the command doesn't have such an option
if 'distance' in route:
route['distance'] += ',0'
else:
route['distance'] = '0'

if 'blackhole' in route:
Copy link
Copy Markdown
Contributor

@prsunny prsunny May 5, 2021

Choose a reason for hiding this comment

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

We would need to handle one scenario for blackhole routes. If the user configure with "ifname" as "null", could you please add the attribute 'blackhole' as true.

route['blackhole'] += ',false'
else:
# If the user configure with "ifname" as "null", set 'blackhole' attribute as true.
if 'ifname' in route and route['ifname'] == 'null':
Copy link
Copy Markdown
Contributor

@shi-su shi-su May 7, 2021

Choose a reason for hiding this comment

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

Just want to confirm. Does this mean that it is necessary to confirm the route is completely removed before setting it to blackhole? If a route already exists in Config_DB and then we configure it to blackhole, the command would simply get ignored? If this is the case, could you please add logic to notify the caller that the command is getting ignored (e.g., fail the command with an error message)?

Copy link
Copy Markdown
Contributor Author

@d-dashkov d-dashkov May 11, 2021

Choose a reason for hiding this comment

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

Yes, it will be ignored if some entries already exist in the db. @prsunny can this suggestion will be add in #1608?

route['blackhole'] = 'true'
else:
cmd += ' {} vrf {}'.format(dev_name, vrf_name)
elif len(nexthop_str) == 4:
vrf_name_dst = nexthop_str[2]
ip = nexthop_str[3]
if vrf_name == "":
cmd += ' {} nexthop-vrf {}'.format(ip, vrf_name_dst)
route['blackhole'] = 'false'

# Check if exist entry with key
keys = config_db.get_keys('STATIC_ROUTE')
if key in keys:
# If exist update current entry
current_entry = config_db.get_entry('STATIC_ROUTE', key)

for entry in ['nexthop', 'nexthop-vrf', 'ifname', 'distance', 'blackhole']:
if not entry in current_entry:
current_entry[entry] = ''
if entry in route:
current_entry[entry] += ',' + route[entry]
else:
cmd += ' {} vrf {} nexthop-vrf {}'.format(ip, vrf_name, vrf_name_dst)
else:
ctx.fail("nexthop is not in pattern!")
cmd += '"'
clicommon.run_command(cmd)
current_entry[entry] += ','

config_db.set_entry("STATIC_ROUTE", key, current_entry)
else:
config_db.set_entry("STATIC_ROUTE", key, route)

@route.command('del', context_settings={"ignore_unknown_options":True})
@click.argument('command_str', metavar='prefix [vrf <vrf_name>] <A.B.C.D/M> nexthop <[vrf <vrf_name>] <A.B.C.D>>|<dev <dev_name>>', nargs=-1, type=click.Path())
@click.pass_context
def del_route(ctx, command_str):
"""Del route command"""
if len(command_str) < 4 or len(command_str) > 9:
ctx.fail("argument is not in pattern prefix [vrf <vrf_name>] <A.B.C.D/M> nexthop <[vrf <vrf_name>] <A.B.C.D>>|<dev <dev_name>>!")
if "prefix" not in command_str:
ctx.fail("argument is incomplete, prefix not found!")
if "nexthop" not in command_str:
ctx.fail("argument is incomplete, nexthop not found!")
for i in range(0, len(command_str)):
if "nexthop" == command_str[i]:
prefix_str = command_str[:i]
nexthop_str = command_str[i:]
vrf_name = ""
cmd = 'sudo vtysh -c "configure terminal" -c "no ip route'
if prefix_str:
if len(prefix_str) == 2:
prefix_mask = prefix_str[1]
cmd += ' {}'.format(prefix_mask)
elif len(prefix_str) == 4:
vrf_name = prefix_str[2]
prefix_mask = prefix_str[3]
cmd += ' {}'.format(prefix_mask)
else:
ctx.fail("prefix is not in pattern!")
if nexthop_str:
if len(nexthop_str) == 2:
ip = nexthop_str[1]
if vrf_name == "":
cmd += ' {}'.format(ip)
else:
cmd += ' {} vrf {}'.format(ip, vrf_name)
elif len(nexthop_str) == 3:
dev_name = nexthop_str[2]
if vrf_name == "":
cmd += ' {}'.format(dev_name)
else:
cmd += ' {} vrf {}'.format(dev_name, vrf_name)
elif len(nexthop_str) == 4:
vrf_name_dst = nexthop_str[2]
ip = nexthop_str[3]
if vrf_name == "":
cmd += ' {} nexthop-vrf {}'.format(ip, vrf_name_dst)
config_db = ctx.obj['config_db']
key, route = cli_sroute_to_config(ctx, command_str, strict_nh=False)
keys = config_db.get_keys('STATIC_ROUTE')
prefix_tuple = tuple(key.split('|'))
if not key in keys and not prefix_tuple in keys:
ctx.fail('Route {} doesnt exist'.format(key))
else:
# If not defined nexthop or intf name remove entire route
if not 'nexthop' in route and not 'ifname' in route:
config_db.set_entry("STATIC_ROUTE", key, None)
return

current_entry = config_db.get_entry('STATIC_ROUTE', key)

nh = ['']
nh_vrf = ['']
ifname = ['']
distance = ['']
blackhole = ['']
if 'nexthop' in current_entry:
nh = current_entry['nexthop'].split(',')
if 'nexthop-vrf' in current_entry:
nh_vrf = current_entry['nexthop-vrf'].split(',')
if 'ifname' in current_entry:
ifname = current_entry['ifname'].split(',')
if 'distance' in current_entry:
distance = current_entry['distance'].split(',')
if 'blackhole' in current_entry:
blackhole = current_entry['blackhole'].split(',')

# Zip data from config_db into tuples
# {'nexthop': '10.0.0.2,20.0.0.2', 'vrf-nexthop': ',Vrf-RED', 'ifname': ','}
# [('10.0.0.2', '', ''), ('20.0.0.2', 'Vrf-RED', '')]
nh_zip = list(itertools.zip_longest(nh, nh_vrf, ifname, fillvalue=''))
Copy link
Copy Markdown
Contributor

@prsunny prsunny Apr 12, 2021

Choose a reason for hiding this comment

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

can you add some comments to understand the logic in this section?

cli_tuple = ()

# Create tuple from CLI argument
# config route add prefix 1.4.3.4/32 nexthop vrf Vrf-RED 20.0.0.2
# ('20.0.0.2', 'Vrf-RED', '')
for entry in ['nexthop', 'nexthop-vrf', 'ifname']:
if entry in route:
cli_tuple += (route[entry],)
else:
cmd += ' {} vrf {} nexthop-vrf {}'.format(ip, vrf_name, vrf_name_dst)
cli_tuple += ('',)

if cli_tuple in nh_zip:
# If cli tuple is in config_db find its index and delete from lists
idx = nh_zip.index(cli_tuple)
if len(nh) - 1 >= idx:
del nh[idx]
if len(nh_vrf) - 1 >= idx:
del nh_vrf[idx]
if len(ifname) - 1 >= idx:
del ifname[idx]
if len(distance) - 1 >= idx:
del distance[idx]
if len(blackhole) - 1 >= idx:
del blackhole[idx]
else:
ctx.fail("nexthop is not in pattern!")
cmd += '"'
clicommon.run_command(cmd)
ctx.fail('Not found {} in {}'.format(cli_tuple, key))

if (len(nh) == 0 or (len(nh) == 1 and nh[0] == '')) and \
(len(ifname) == 0 or (len(ifname) == 1 and ifname[0] == '')):
# If there are no nexthop and ifname fields in the current record, delete it
config_db.set_entry("STATIC_ROUTE", key, None)
Copy link
Copy Markdown
Contributor

@prsunny prsunny Apr 12, 2021

Choose a reason for hiding this comment

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

if user just deletes the route without mentioning any nexthops, then the route is expected to be fully deleted.

Copy link
Copy Markdown
Contributor Author

@d-dashkov d-dashkov Apr 13, 2021

Choose a reason for hiding this comment

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

If the user tries to delete a route without nexthop (for example, 'сonfig del config route del prefix 1.2.3.4/32'), he will receive an error: 'argument is not in pattern'. So if the user has previously added ECMP routes, he must remove them one by one.
This condition checks if the record still has any nexthop, if so, it just updates this record, if it doesn't have the nexthop, it removes the key. Is this the correct behavior or should user be able to delete the entire entry at once?

Copy link
Copy Markdown
Contributor

@prsunny prsunny Apr 14, 2021

Choose a reason for hiding this comment

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

IMO, user should be able to delete the entire route at once.

else:
# Otherwise it still has ECMP nexthop or ifname fields, so compose it from the lists into db
current_entry['nexthop'] = ','.join((str(e)) for e in nh)
current_entry['nexthop-vrf'] = ','.join((str(e)) for e in nh_vrf)
current_entry['ifname'] = ','.join((str(e)) for e in ifname)
current_entry['distance'] = ','.join((str(e)) for e in distance)
current_entry['blackhole'] = ','.join((str(e)) for e in blackhole)
config_db.set_entry("STATIC_ROUTE", key, current_entry)

#
# 'acl' group ('config acl ...')
Expand Down
Loading