[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.

Author: dgsudharsan

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:
@@ -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:

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"])

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()

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

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,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"
     },

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": {

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()

tests/show_vrf_test.py

@@ -29,7 +29,7 @@ def test_vrf_show(self):
 Vrf101  Ethernet0.10
 Vrf102  PortChannel0002
         Vlan40
-        Eth32.10
+        Eth36.10
 Vrf103  Ethernet4
         Loopback0
         Po0002.101
@@ -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
@@ -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)
@@ -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

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')
 

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] <subinterface_name> <vid>
+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()

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"