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.

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

[VRF]Adding CLI checks to ensure Vrf is valid in interface bind and static route commands #2333

Merged
merged 1 commit into from
Aug 29, 2022
Merged

[VRF]Adding CLI checks to ensure Vrf is valid in interface bind and static route commands #2333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 21 additions & 0 deletions config/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,19 @@ def get_interface_ipaddresses(config_db, interface_name):

return ipaddresses

def is_vrf_exists(config_db, vrf_name):
"""Check if VRF exists
"""
keys = config_db.get_keys("VRF")
if vrf_name in keys:
return True
elif vrf_name == "mgmt":
entry = config_db.get_entry("MGMT_VRF_CONFIG", "vrf_global")
if entry and entry.get("mgmtVrfEnabled") == "true":
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we prevent from using non-existing VRF in the INTERFACE table during config load? this seems to address only in the Click CLI.

If we can do the YANG level validation for all north bound interfaces (CLick, config load..etc), it would be an one time check in the back-end rather than doing it on all front-end (e.g this check in Click), what is your thought?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @venkatmahalingam
Your comment is applicable for any table in config_db.json.
This PR is focussed on only the Click CLI. I believe we have a separate discussion on using yang for validation which is the work in progress and would take sometime.
For now It is expected of the end user to use Click CLI to make any incremental configuration changes.
Let me know if the changes look good.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any update we do with config-db should be YANG validated is what the expectation but the infra is not ready yet, we can merge this code for now.

return True

return False

def is_interface_bind_to_vrf(config_db, interface_name):
"""Get interface if bind to vrf or not
"""
Expand Down Expand Up @@ -986,6 +999,7 @@ def cli_sroute_to_config(ctx, command_str, strict_nh = True):
nexthop_str = None
config_entry = {}
vrf_name = ""
config_db = ctx.obj['config_db']

if "nexthop" in command_str:
idx = command_str.index("nexthop")
Expand All @@ -998,6 +1012,8 @@ def cli_sroute_to_config(ctx, command_str, strict_nh = True):
if 'prefix' in prefix_str and 'vrf' in prefix_str:
# prefix_str: ['prefix', 'vrf', Vrf-name, ip]
vrf_name = prefix_str[2]
if not is_vrf_exists(config_db, vrf_name):
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have check to ensure vrf is removed only after all interface bindings are removed? Otherwise this is only a check in the add-path.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is only a check in the add path.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In fact the current vrf removal logic removes all these bindings implicitly using the function del_interface_bind_to_vrf

ctx.fail("VRF %s does not exist!"%(vrf_name))
ip_prefix = prefix_str[3]
elif 'prefix' in prefix_str:
# prefix_str: ['prefix', ip]
Expand All @@ -1009,6 +1025,8 @@ def cli_sroute_to_config(ctx, command_str, strict_nh = True):
if 'nexthop' in nexthop_str and 'vrf' in nexthop_str:
# nexthop_str: ['nexthop', 'vrf', Vrf-name, ip]
config_entry["nexthop"] = nexthop_str[3]
if not is_vrf_exists(config_db, nexthop_str[2]):
ctx.fail("VRF %s does not exist!"%(nexthop_str[2]))
config_entry["nexthop-vrf"] = nexthop_str[2]
elif 'nexthop' in nexthop_str and 'dev' in nexthop_str:
# nexthop_str: ['nexthop', 'dev', ifname]
Expand Down Expand Up @@ -4883,6 +4901,9 @@ def bind(ctx, interface_name, vrf_name):
if interface_name is None:
ctx.fail("'interface_name' is None!")

if not is_vrf_exists(config_db, vrf_name):
ctx.fail("VRF %s does not exist!"%(vrf_name))

table_name = get_interface_table_name(interface_name)
if table_name == "":
ctx.fail("'interface_name' is not valid. Valid names [Ethernet/PortChannel/Vlan/Loopback]")
Expand Down
35 changes: 35 additions & 0 deletions tests/ip_config_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,20 @@

ERROR_MSG = "Error: IP address is not valid"

INVALID_VRF_MSG ="""\
Usage: bind [OPTIONS] <interface_name> <vrf_name>
Try "bind --help" for help.

Error: VRF Vrf2 does not exist!
"""

INVALID_MGMT_VRF_MSG ="""\
Usage: bind [OPTIONS] <interface_name> <vrf_name>
Try "bind --help" for help.

Error: VRF mgmt does not exist!
"""

class TestConfigIP(object):
@classmethod
def setup_class(cls):
Expand Down Expand Up @@ -190,6 +204,27 @@ def test_intf_vrf_bind_unbind(self):
print(result.exit_code, result.output)
assert result.exit_code == 0

def test_intf_unknown_vrf_bind(self):
runner = CliRunner()
db = Db()
obj = {'config_db':db.cfgdb, 'namespace':db.db.namespace}

result = runner.invoke(config.config.commands["interface"].commands["vrf"].commands["bind"], ["Ethernet64", "Vrf2"], obj=obj)
print(result.exit_code, result.output)
assert result.exit_code != 0
assert result.output == INVALID_VRF_MSG

result = runner.invoke(config.config.commands["interface"].commands["vrf"].commands["bind"], ["Ethernet64", "mgmt"], obj=obj)
print(result.exit_code, result.output)
assert result.exit_code != 0
assert result.output == INVALID_MGMT_VRF_MSG

result = runner.invoke(config.config.commands["vrf"].commands["add"], ["mgmt"], obj=obj)
print(result.exit_code, result.output)
result = runner.invoke(config.config.commands["interface"].commands["vrf"].commands["bind"], ["Ethernet64", "mgmt"], obj=obj)
print(result.exit_code, result.output)
assert result.exit_code == 0

@classmethod
def teardown_class(cls):
os.environ['UTILITIES_UNIT_TESTING'] = "0"
Expand Down
13 changes: 13 additions & 0 deletions tests/static_routes_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ def test_vrf_static_route(self):
obj = {'config_db':db.cfgdb}

# config route add prefix vrf Vrf-BLUE 2.2.3.4/32 nexthop 30.0.0.6
result = runner.invoke(config.config.commands["vrf"].commands["add"], ["Vrf-BLUE"], obj=obj)
print(result.exit_code, result.output)
result = runner.invoke(config.config.commands["route"].commands["add"], \
["prefix", "vrf", "Vrf-BLUE", "2.2.3.4/32", "nexthop", "30.0.0.6"], obj=obj)
print(result.exit_code, result.output)
Expand All @@ -111,9 +113,12 @@ def test_dest_vrf_static_route(self):
obj = {'config_db':db.cfgdb}

# config route add prefix 3.2.3.4/32 nexthop vrf Vrf-RED 30.0.0.6
result = runner.invoke(config.config.commands["vrf"].commands["add"], ["Vrf-RED"], obj=obj)
print(result.exit_code, result.output)
result = runner.invoke(config.config.commands["route"].commands["add"], \
["prefix", "3.2.3.4/32", "nexthop", "vrf", "Vrf-RED", "30.0.0.6"], obj=obj)
print(result.exit_code, result.output)
print(db.cfgdb.get_table('STATIC_ROUTE'))
assert ('3.2.3.4/32') in db.cfgdb.get_table('STATIC_ROUTE')
assert db.cfgdb.get_entry('STATIC_ROUTE', '3.2.3.4/32') == {'nexthop': '30.0.0.6', 'nexthop-vrf': 'Vrf-RED', 'blackhole': 'false', 'distance': '0', 'ifname': ''}

Expand All @@ -129,6 +134,8 @@ def test_multiple_nexthops_with_vrf_static_route(self):
obj = {'config_db':db.cfgdb}

''' Add '''
result = runner.invoke(config.config.commands["vrf"].commands["add"], ["Vrf-RED"], obj=obj)
print(result.exit_code, result.output)
# config route add prefix 6.2.3.4/32 nexthop vrf Vrf-RED "30.0.0.6,30.0.0.7"
result = runner.invoke(config.config.commands["route"].commands["add"], \
["prefix", "6.2.3.4/32", "nexthop", "vrf", "Vrf-RED", "30.0.0.6,30.0.0.7"], obj=obj)
Expand Down Expand Up @@ -251,13 +258,17 @@ def test_static_route_ECMP_nexthop_with_vrf(self):
obj = {'config_db':db.cfgdb}

''' Add '''
result = runner.invoke(config.config.commands["vrf"].commands["add"], ["Vrf-RED"], obj=obj)
print(result.exit_code, result.output)
# config route add prefix 11.2.3.4/32 nexthop vrf Vrf-RED 30.0.0.5
result = runner.invoke(config.config.commands["route"].commands["add"], \
["prefix", "11.2.3.4/32", "nexthop", "vrf", "Vrf-RED", "30.0.0.5"], obj=obj)
print(result.exit_code, result.output)
assert ('11.2.3.4/32') in db.cfgdb.get_table('STATIC_ROUTE')
assert db.cfgdb.get_entry('STATIC_ROUTE', '11.2.3.4/32') == {'nexthop': '30.0.0.5', 'nexthop-vrf': 'Vrf-RED', 'blackhole': 'false', 'distance': '0', 'ifname': ''}

result = runner.invoke(config.config.commands["vrf"].commands["add"], ["Vrf-BLUE"], obj=obj)
print(result.exit_code, result.output)
# config route add prefix 11.2.3.4/32 nexthop vrf Vrf-BLUE 30.0.0.6
result = runner.invoke(config.config.commands["route"].commands["add"], \
["prefix", "11.2.3.4/32", "nexthop", "vrf", "Vrf-BLUE", "30.0.0.6"], obj=obj)
Expand Down Expand Up @@ -292,6 +303,8 @@ def test_static_route_ECMP_mixed_nextfop(self):
assert ('12.2.3.4/32') in db.cfgdb.get_table('STATIC_ROUTE')
assert db.cfgdb.get_entry('STATIC_ROUTE', '12.2.3.4/32') == {'nexthop': '30.0.0.6', 'blackhole': 'false', 'distance': '0', 'ifname': '', 'nexthop-vrf': ''}

result = runner.invoke(config.config.commands["vrf"].commands["add"], ["Vrf-RED"], obj=obj)
print(result.exit_code, result.output)
# config route add prefix 12.2.3.4/32 nexthop vrf Vrf-RED 30.0.0.7
result = runner.invoke(config.config.commands["route"].commands["add"], \
["prefix", "12.2.3.4/32", "nexthop", "vrf", "Vrf-RED", "30.0.0.7"], obj=obj)
Expand Down