Skip to content

Commit

Permalink
[subinterface]Added additional checks in portchannel and subinterface…
Browse files Browse the repository at this point in the history
… commands (sonic-net#2345)

*Added additional checks in subinterface and portchannel commands so they don't conflict. Without the checks, a subinterface could be created on a portchannel member and vice versa which will lead to SAI failure followed by orchagent crash.
  • Loading branch information
dgsudharsan authored and preetham-singh committed Nov 18, 2022
1 parent a220f68 commit ecb9ea7
Show file tree
Hide file tree
Showing 11 changed files with 82 additions and 39 deletions.
19 changes: 14 additions & 5 deletions config/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -2113,6 +2113,14 @@ def add_portchannel_member(ctx, portchannel_name, port_name):
ctx.fail(" {} has ip address configured".format(port_name))
return

for key in db.get_keys('VLAN_SUB_INTERFACE'):
if type(key) == tuple:
continue
intf = key.split(VLAN_SUB_INTERFACE_SEPARATOR)[0]
parent_intf = get_intf_longname(intf)
if parent_intf == port_name:
ctx.fail(" {} has subinterfaces configured".format(port_name))

# Dont allow a port to be member of port channel if it is configured as a VLAN member
for k,v in db.get_table('VLAN_MEMBER'):
if v == port_name:
Expand Down Expand Up @@ -6762,23 +6770,24 @@ def add_subinterface(ctx, subinterface_name, vid):

config_db = ctx.obj['db']
port_dict = config_db.get_table(intf_table_name)
parent_intf = get_intf_longname(interface_alias)
if interface_alias is not None:
if not port_dict:
ctx.fail("{} parent interface not found. {} table none".format(interface_alias, intf_table_name))
if get_intf_longname(interface_alias) not in port_dict.keys():
if parent_intf not in port_dict.keys():
ctx.fail("{} parent interface not found".format(subinterface_name))

# Validate if parent is portchannel member
portchannel_member_table = config_db.get_table('PORTCHANNEL_MEMBER')
if interface_is_in_portchannel(portchannel_member_table, interface_alias):
if interface_is_in_portchannel(portchannel_member_table, parent_intf):
ctx.fail("{} is configured as a member of portchannel. Cannot configure subinterface"
.format(interface_alias))
.format(parent_intf))

# Validate if parent is vlan member
vlan_member_table = config_db.get_table('VLAN_MEMBER')
if interface_is_in_vlan(vlan_member_table, interface_alias):
if interface_is_in_vlan(vlan_member_table, parent_intf):
ctx.fail("{} is configured as a member of vlan. Cannot configure subinterface"
.format(interface_alias))
.format(parent_intf))

sub_intfs = [k for k,v in config_db.get_table('VLAN_SUB_INTERFACE').items() if type(k) != tuple]
if subinterface_name in sub_intfs:
Expand Down
12 changes: 6 additions & 6 deletions tests/intfutil_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ def test_subintf_status(self):
expected_output = (
"Sub port interface Speed MTU Vlan Admin Type\n"
"-------------------- ------- ----- ------ ------- --------------------\n"
" Eth32.10 40G 9100 100 up 802.1q-encapsulation\n"
" Eth36.10 10M 9100 100 up 802.1q-encapsulation\n"
" Ethernet0.10 25G 9100 10 up 802.1q-encapsulation\n"
" Po0001.10 40G 9100 100 up 802.1q-encapsulation"
)
Expand Down Expand Up @@ -248,10 +248,10 @@ def test_single_subintf_status(self):
expected_output = (
"Sub port interface Speed MTU Vlan Admin Type\n"
"-------------------- ------- ----- ------ ------- --------------------\n"
" Eth32.10 40G 9100 100 up 802.1q-encapsulation"
" Eth36.10 10M 9100 100 up 802.1q-encapsulation"
)
# Test 'intfutil status Eth32.10'
output = subprocess.check_output('intfutil -c status -i Eth32.10', stderr=subprocess.STDOUT, shell=True, text=True)
# Test 'intfutil status Eth36.10'
output = subprocess.check_output('intfutil -c status -i Eth36.10', stderr=subprocess.STDOUT, shell=True, text=True)
print(output, file=sys.stderr)
self.assertEqual(output.strip(), expected_output)

Expand All @@ -272,9 +272,9 @@ def test_single_subintf_status_verbose(self):
expected_output = "Command: intfutil -c status -i Ethernet0.10"
self.assertEqual(result.output.split('\n')[0], expected_output)

result = self.runner.invoke(show.cli.commands["subinterfaces"].commands["status"], ["Eth32.10", "--verbose"])
result = self.runner.invoke(show.cli.commands["subinterfaces"].commands["status"], ["Eth36.10", "--verbose"])
print(result.output, file=sys.stderr)
expected_output = "Command: intfutil -c status -i Eth32.10"
expected_output = "Command: intfutil -c status -i Eth36.10"
self.assertEqual(result.output.split('\n')[0], expected_output)

result = self.runner.invoke(show.cli.commands["subinterfaces"].commands["status"], ["Po0001.10", "--verbose"])
Expand Down
20 changes: 10 additions & 10 deletions tests/ip_config_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,11 @@ def test_add_del_interface_valid_ipv4(self):
assert result.exit_code == 0
assert ('Ethernet0.10', '10.11.10.1/24') in db.cfgdb.get_table('VLAN_SUB_INTERFACE')

# config int ip add Eth32.10 32.11.10.1/24
result = runner.invoke(config.config.commands["interface"].commands["ip"].commands["add"], ["Eth32.10", "32.11.10.1/24"], obj=obj)
# config int ip add Eth36.10 32.11.10.1/24
result = runner.invoke(config.config.commands["interface"].commands["ip"].commands["add"], ["Eth36.10", "32.11.10.1/24"], obj=obj)
print(result.exit_code, result.output)
assert result.exit_code == 0
assert ('Eth32.10', '32.11.10.1/24') in db.cfgdb.get_table('VLAN_SUB_INTERFACE')
assert ('Eth36.10', '32.11.10.1/24') in db.cfgdb.get_table('VLAN_SUB_INTERFACE')

# config int ip remove Ethernet64 10.10.10.1/24
result = runner.invoke(config.config.commands["interface"].commands["ip"].commands["remove"], ["Ethernet64", "10.10.10.1/24"], obj=obj)
Expand All @@ -72,11 +72,11 @@ def test_add_del_interface_valid_ipv4(self):
assert result.exit_code != 0
assert ('Ethernet0.10', '10.11.10.1/24') not in db.cfgdb.get_table('VLAN_SUB_INTERFACE')

# config int ip remove Eth32.10 32.11.10.1/24
result = runner.invoke(config.config.commands["interface"].commands["ip"].commands["remove"], ["Eth32.10", "32.11.10.1/24"], obj=obj)
# config int ip remove Eth36.10 32.11.10.1/24
result = runner.invoke(config.config.commands["interface"].commands["ip"].commands["remove"], ["Eth36.10", "32.11.10.1/24"], obj=obj)
print(result.exit_code, result.output)
assert result.exit_code != 0
assert ('Eth32.10', '32.11.10.1/24') not in db.cfgdb.get_table('VLAN_SUB_INTERFACE')
assert ('Eth36.10', '32.11.10.1/24') not in db.cfgdb.get_table('VLAN_SUB_INTERFACE')

def test_add_interface_invalid_ipv4(self):
db = Db()
Expand Down Expand Up @@ -129,10 +129,10 @@ def test_add_del_interface_valid_ipv6(self):
assert result.exit_code == 0
assert ('Ethernet0.10', '1010:1db8:11a3:19d7:1f34:8a2e:17a0:765d/34') in db.cfgdb.get_table('VLAN_SUB_INTERFACE')

result = runner.invoke(config.config.commands["interface"].commands["ip"].commands["add"], ["Eth32.10", "3210:1db8:11a3:19d7:1f34:8a2e:17a0:765d/34"], obj=obj)
result = runner.invoke(config.config.commands["interface"].commands["ip"].commands["add"], ["Eth36.10", "3210:1db8:11a3:19d7:1f34:8a2e:17a0:765d/34"], obj=obj)
print(result.exit_code, result.output)
assert result.exit_code == 0
assert ('Eth32.10', '3210:1db8:11a3:19d7:1f34:8a2e:17a0:765d/34') in db.cfgdb.get_table('VLAN_SUB_INTERFACE')
assert ('Eth36.10', '3210:1db8:11a3:19d7:1f34:8a2e:17a0:765d/34') in db.cfgdb.get_table('VLAN_SUB_INTERFACE')

# config int ip remove Ethernet72 2001:1db8:11a3:19d7:1f34:8a2e:17a0:765d/34
result = runner.invoke(config.config.commands["interface"].commands["ip"].commands["remove"], ["Ethernet72", "2001:1db8:11a3:19d7:1f34:8a2e:17a0:765d/34"], obj=obj)
Expand All @@ -145,10 +145,10 @@ def test_add_del_interface_valid_ipv6(self):
assert result.exit_code != 0
assert ('Ethernet0.10', '1010:1db8:11a3:19d7:1f34:8a2e:17a0:765d/34') not in db.cfgdb.get_table('VLAN_SUB_INTERFACE')

result = runner.invoke(config.config.commands["interface"].commands["ip"].commands["remove"], ["Eth32.10", "3210:1db8:11a3:19d7:1f34:8a2e:17a0:765d/34"], obj=obj)
result = runner.invoke(config.config.commands["interface"].commands["ip"].commands["remove"], ["Eth36.10", "3210:1db8:11a3:19d7:1f34:8a2e:17a0:765d/34"], obj=obj)
print(result.exit_code, result.output)
assert result.exit_code != 0
assert ('Eth32.10', '3210:1db8:11a3:19d7:1f34:8a2e:17a0:765d/34') not in db.cfgdb.get_table('VLAN_SUB_INTERFACE')
assert ('Eth36.10', '3210:1db8:11a3:19d7:1f34:8a2e:17a0:765d/34') not in db.cfgdb.get_table('VLAN_SUB_INTERFACE')

def test_del_interface_case_sensitive_ipv6(self):
db = Db()
Expand Down
2 changes: 1 addition & 1 deletion tests/loopback_action_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
show_ip_interfaces_loopback_action_output="""\
Interface Action
--------------- --------
Eth32.10 drop
Eth36.10 drop
Ethernet0 forward
PortChannel0001 drop
Vlan3000 forward
Expand Down
6 changes: 3 additions & 3 deletions tests/mock_tables/appl_db.json
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@
"admin_status": "up",
"vlan": "10"
},
"INTF_TABLE:Eth32.10": {
"INTF_TABLE:Eth36.10": {
"admin_status": "up",
"vrf_name": "Vrf1",
"vlan": "100"
Expand All @@ -202,15 +202,15 @@
"family": "IPv4",
"scope": "global"
},
"INTF_TABLE:Eth32.10|32.10.11.12/24": {
"INTF_TABLE:Eth36.10|32.10.11.12/24": {
"family": "IPv4",
"scope": "global"
},
"INTF_TABLE:Po0001.10|10.10.11.12/24": {
"family": "IPv4",
"scope": "global"
},
"INTF_TABLE:Eth32.10|3210::12/126": {
"INTF_TABLE:Eth36.10|3210::12/126": {
"family": "IPv6",
"scope": "global"
},
Expand Down
6 changes: 3 additions & 3 deletions tests/mock_tables/config_db.json
Original file line number Diff line number Diff line change
Expand Up @@ -376,16 +376,16 @@
"VLAN_SUB_INTERFACE|Ethernet0.10|10.11.12.13/24": {
"NULL" : "NULL"
},
"VLAN_SUB_INTERFACE|Eth32.10": {
"VLAN_SUB_INTERFACE|Eth36.10": {
"admin_status": "up",
"loopback_action": "drop",
"vrf_name": "Vrf1",
"vlan": "100"
},
"VLAN_SUB_INTERFACE|Eth32.10|32.10.11.12/24": {
"VLAN_SUB_INTERFACE|Eth36.10|32.10.11.12/24": {
"NULL" : "NULL"
},
"VLAN_SUB_INTERFACE|Eth32.10|3210::12/126": {
"VLAN_SUB_INTERFACE|Eth36.10|3210::12/126": {
"NULL" : "NULL"
},
"VLAN_SUB_INTERFACE|Po0001.10": {
Expand Down
13 changes: 13 additions & 0 deletions tests/portchannel_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,19 @@ def test_add_portchannel_member_which_has_ipaddress(self):
assert result.exit_code != 0
assert "Error: Ethernet0 has ip address configured" in result.output

def test_add_portchannel_member_which_has_subintf(self):
runner = CliRunner()
db = Db()
obj = {'db':db.cfgdb}

# add a portchannel member with port which has ip-address
result = runner.invoke(config.config.commands["portchannel"].commands["member"].commands["add"], ["PortChannel1001", "Ethernet36"], obj=obj)
print(result.exit_code)
print(result.output)
assert result.exit_code != 0
print(result.output)
assert "Error: Ethernet36 has subinterfaces configured" in result.output

def test_add_portchannel_member_which_is_member_of_vlan(self):
runner = CliRunner()
db = Db()
Expand Down
10 changes: 5 additions & 5 deletions tests/show_vrf_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ def test_vrf_show(self):
Vrf101 Ethernet0.10
Vrf102 PortChannel0002
Vlan40
Eth32.10
Eth36.10
Vrf103 Ethernet4
Loopback0
Po0002.101
Expand All @@ -53,7 +53,7 @@ def test_vrf_bind_unbind(self):
Vrf101 Ethernet0.10
Vrf102 PortChannel0002
Vlan40
Eth32.10
Eth36.10
Vrf103 Ethernet4
Loopback0
Po0002.101
Expand Down Expand Up @@ -86,10 +86,10 @@ def test_vrf_bind_unbind(self):
assert result.exit_code == 0
assert 'PortChannel002' not in db.cfgdb.get_table('PORTCHANNEL_INTERFACE')

result = runner.invoke(config.config.commands["interface"].commands["vrf"].commands["unbind"], ["Eth32.10"], obj=obj)
result = runner.invoke(config.config.commands["interface"].commands["vrf"].commands["unbind"], ["Eth36.10"], obj=obj)
print(result.exit_code, result.output)
assert result.exit_code == 0
assert ('vrf_name', 'Vrf102') not in db.cfgdb.get_table('VLAN_SUB_INTERFACE')['Eth32.10']
assert ('vrf_name', 'Vrf102') not in db.cfgdb.get_table('VLAN_SUB_INTERFACE')['Eth36.10']

result = runner.invoke(config.config.commands["interface"].commands["vrf"].commands["unbind"], ["Ethernet0.10"], obj=obj)
print(result.exit_code, result.output)
Expand All @@ -114,7 +114,7 @@ def test_vrf_bind_unbind(self):
Vrf101 Ethernet0.10
Vrf102 PortChannel0002
Vlan40
Eth32.10
Eth36.10
Vrf103 Ethernet4
Loopback0
Po0002.101
Expand Down
10 changes: 5 additions & 5 deletions tests/static_routes_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -403,16 +403,16 @@ def test_static_route_nexthop_subinterface(self):
print(result.exit_code, result.output)
assert not ('2.2.3.5/32') in db.cfgdb.get_table('STATIC_ROUTE')

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

# config route del prefix 2.2.3.5/32 nexthop dev Eth32.10
# config route del prefix 2.2.3.5/32 nexthop dev Eth36.10
result = runner.invoke(config.config.commands["route"].commands["del"], \
["prefix", "2.2.3.5/32", "nexthop", "dev", "Eth32.10"], obj=obj)
["prefix", "2.2.3.5/32", "nexthop", "dev", "Eth36.10"], obj=obj)
print(result.exit_code, result.output)
assert not ('2.2.3.5/32') in db.cfgdb.get_table('STATIC_ROUTE')

Expand Down
21 changes: 21 additions & 0 deletions tests/subintf_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,12 @@
import show.main as show
from utilities_common.db import Db

SUB_INTF_ON_LAG_MEMBER_ERR="""\
Usage: add [OPTIONS] <subinterface_name> <vid>
Try "add --help" for help.
Error: Ethernet32 is configured as a member of portchannel. Cannot configure subinterface
"""

class TestSubinterface(object):
@classmethod
Expand Down Expand Up @@ -141,6 +147,21 @@ def test_invalid_subintf_creation(self):
print(result.exit_code, result.output)
assert result.exit_code != 0

def test_subintf_creation_on_lag_member(self):
runner = CliRunner()
db = Db()
obj = {'db':db.cfgdb}

result = runner.invoke(config.config.commands["subinterface"].commands["add"], ["Ethernet32.10"], obj=obj)
print(result.exit_code, result.output)
assert result.exit_code != 0
assert(result.output == SUB_INTF_ON_LAG_MEMBER_ERR)

result = runner.invoke(config.config.commands["subinterface"].commands["add"], ["Eth32.20"], obj=obj)
print(result.exit_code, result.output)
assert result.exit_code != 0
assert(result.output == SUB_INTF_ON_LAG_MEMBER_ERR)

def test_subintf_vrf_bind_unbind(self):
runner = CliRunner()
db = Db()
Expand Down
2 changes: 1 addition & 1 deletion tests/vrf_input/config_db.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"vrf_name": "Vrf101",
"admin_status": "up"
},
"VLAN_SUB_INTERFACE|Eth32.10": {
"VLAN_SUB_INTERFACE|Eth36.10": {
"vrf_name": "Vrf102",
"admin_status": "up",
"vlan": "100"
Expand Down

0 comments on commit ecb9ea7

Please sign in to comment.