From cff80d368480aae30c72f8d753bdfe92e2edd7d1 Mon Sep 17 00:00:00 2001 From: Sudharsan Dhamal Gopalarathnam Date: Mon, 12 Sep 2022 18:47:06 -0700 Subject: [PATCH] [subinterface]Added additional checks in portchannel and subinterface commands (#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. --- config/main.py | 19 ++++++++++++++----- tests/intfutil_test.py | 12 ++++++------ tests/ip_config_test.py | 20 ++++++++++---------- tests/loopback_action_test.py | 2 +- tests/mock_tables/appl_db.json | 6 +++--- tests/mock_tables/config_db.json | 6 +++--- tests/portchannel_test.py | 13 +++++++++++++ tests/static_routes_test.py | 10 +++++----- tests/subintf_test.py | 21 +++++++++++++++++++++ tests/vrf_input/config_db.json | 2 +- 10 files changed, 77 insertions(+), 34 deletions(-) diff --git a/config/main.py b/config/main.py index 335417731e3..7595be9e286 100644 --- a/config/main.py +++ b/config/main.py @@ -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: @@ -6766,23 +6774,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: diff --git a/tests/intfutil_test.py b/tests/intfutil_test.py index 081246a4881..2a13075919b 100644 --- a/tests/intfutil_test.py +++ b/tests/intfutil_test.py @@ -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" ) @@ -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) @@ -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"]) diff --git a/tests/ip_config_test.py b/tests/ip_config_test.py index c56b226c74d..fd6b4feb9fc 100644 --- a/tests/ip_config_test.py +++ b/tests/ip_config_test.py @@ -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) @@ -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() @@ -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) @@ -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() diff --git a/tests/loopback_action_test.py b/tests/loopback_action_test.py index 58942b0c4b3..b88d36973d1 100644 --- a/tests/loopback_action_test.py +++ b/tests/loopback_action_test.py @@ -7,7 +7,7 @@ show_ip_interfaces_loopback_action_output="""\ Interface Action --------------- -------- -Eth32.10 drop +Eth36.10 drop Ethernet0 forward PortChannel0001 drop Vlan3000 forward diff --git a/tests/mock_tables/appl_db.json b/tests/mock_tables/appl_db.json index cd00408b496..ab4e31282fe 100644 --- a/tests/mock_tables/appl_db.json +++ b/tests/mock_tables/appl_db.json @@ -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" @@ -202,7 +202,7 @@ "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" }, @@ -210,7 +210,7 @@ "family": "IPv4", "scope": "global" }, - "INTF_TABLE:Eth32.10|3210::12/126": { + "INTF_TABLE:Eth36.10|3210::12/126": { "family": "IPv6", "scope": "global" }, diff --git a/tests/mock_tables/config_db.json b/tests/mock_tables/config_db.json index 699ef155e0e..fcb16e8f2d9 100644 --- a/tests/mock_tables/config_db.json +++ b/tests/mock_tables/config_db.json @@ -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": { diff --git a/tests/portchannel_test.py b/tests/portchannel_test.py index 9b187f13d5c..bd30c73649e 100644 --- a/tests/portchannel_test.py +++ b/tests/portchannel_test.py @@ -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() diff --git a/tests/static_routes_test.py b/tests/static_routes_test.py index fc7371b344a..3fce727ee29 100644 --- a/tests/static_routes_test.py +++ b/tests/static_routes_test.py @@ -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') diff --git a/tests/subintf_test.py b/tests/subintf_test.py index 581ea49ce5d..c69d87572eb 100644 --- a/tests/subintf_test.py +++ b/tests/subintf_test.py @@ -7,6 +7,12 @@ import show.main as show from utilities_common.db import Db +SUB_INTF_ON_LAG_MEMBER_ERR="""\ +Usage: add [OPTIONS] +Try "add --help" for help. + +Error: Ethernet32 is configured as a member of portchannel. Cannot configure subinterface +""" class TestSubinterface(object): @classmethod @@ -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() diff --git a/tests/vrf_input/config_db.json b/tests/vrf_input/config_db.json index fe1cb2eb255..1746c14c4f4 100644 --- a/tests/vrf_input/config_db.json +++ b/tests/vrf_input/config_db.json @@ -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"