bridge: T3137: Let VLAN aware bridge approach the behavior of professional equipment

According to the consensus, the specific behavior of a VLAN aware bridge should conform
to the behavior of professional equipment. This commit makes a significant change to the
behavior of VLAN aware bridge, and has the following behaviors:

1. Disable `vif 1` configuration
2. When the VLAN aware bridge is enabled, the parent interface is always VLAN 1
3. When `native-vlan` is not configured, the default behavior of the device is `native-vlan 1`
4. The VLAN ids forwarded by the bridge are determined by `vif`
5. It has an `enable-vlan` node to enable VLAN awareness
6. VLAN configuration is allowed only when VLAN aware bridge is activated

6 files changed, 204 insertions, 170 deletions

diff --git a/interface-definitions/interfaces-bridge.xml.in b/interface-definitions/interfaces-bridge.xml.in
index c32c0ca32..e940e6685 100644
--- a/interface-definitions/interfaces-bridge.xml.in
+++ b/interface-definitions/interfaces-bridge.xml.in
@@ -86,6 +86,12 @@
           #include <include/interface-ipv6-options.xml.i>
           #include <include/interface-mac.xml.i>
           #include <include/interface-mirror.xml.i>
+          <leafNode name="enable-vlan">
+            <properties>
+              <help>Enable VLAN aware bridge</help>
+              <valueless/>
+            </properties>
+          </leafNode>
           <leafNode name="max-age">
             <properties>
               <help>Interval at which neighbor bridges are removed</help>
@@ -196,7 +202,6 @@
               <valueless/>
             </properties>
           </leafNode>
-          #include <include/vif-s.xml.i>
           #include <include/vif.xml.i>
         </children>
       </tagNode>
diff --git a/python/vyos/ifconfig/bridge.py b/python/vyos/ifconfig/bridge.py
index 76520f2ba..87af0c9df 100644
--- a/python/vyos/ifconfig/bridge.py
+++ b/python/vyos/ifconfig/bridge.py
@@ -274,20 +274,31 @@ class BridgeIf(Interface):
         for member in (tmp or []):
             if member in interfaces():
                 self.del_port(member)
-        vlan_filter = 0
 
-        vlan_del = set()
-        vlan_add = set()
+        # enable/disable Vlan Filter
+        vlan_filter = 1 if 'enable_vlan' in config else 0
+        self.set_vlan_filter(str(vlan_filter))
+
+        if vlan_filter:
+            del_ifname_vlan_ids = get_vlan_ids(ifname)
+            # Delete all VLAN IDS configured in the interface first
+            for vlan in del_ifname_vlan_ids:
+                cmd = f'bridge vlan del dev {ifname} vid {vlan} self'
+                self._cmd(cmd)
+                
+            tmp = dict_search('vif', config)
+            if tmp:
+                for vif, vif_config in tmp.items():
+                    cmd = f'bridge vlan add dev {ifname} vid {vif} self'
+                    self._cmd(cmd)
+            
+            # VLAN of bridge parent interface is always 1
+            # VLAN 1 is the default VLAN for all unlabeled packets
+            cmd = f'bridge vlan add dev {ifname} vid 1 pvid untagged self'
+            self._cmd(cmd)
 
         tmp = dict_search('member.interface', config)
         if tmp:
-            if self.get_vlan_filter():
-                bridge_vlan_ids = get_vlan_ids(ifname)
-                # Delete VLAN ID for the bridge
-                if 1 in bridge_vlan_ids:
-                    bridge_vlan_ids.remove(1)
-                for vlan in bridge_vlan_ids:
-                    vlan_del.add(str(vlan))
 
             for interface, interface_config in tmp.items():
                 # if interface does yet not exist bail out early and
@@ -315,62 +326,27 @@ class BridgeIf(Interface):
                     value = interface_config.get('priority')
                     lower.set_path_priority(value)
 
-                tmp = dict_search('native_vlan_removed', interface_config)
-
-                for vlan_id in (tmp or []):
-                    cmd = f'bridge vlan del dev {interface} vid {vlan_id}'
-                    self._cmd(cmd)
-                    cmd = f'bridge vlan add dev {interface} vid 1 pvid untagged master'
-                    self._cmd(cmd)
-                    vlan_del.add(vlan_id)
-                    vlan_add.add(1)
-
-                tmp = dict_search('allowed_vlan_removed', interface_config)
-
-
-                for vlan_id in (tmp or []):
-                    cmd = f'bridge vlan del dev {interface} vid {vlan_id}'
-                    self._cmd(cmd)
-                    vlan_del.add(vlan_id)
-
-                if 'native_vlan' in interface_config:
-                    vlan_filter = 1
-                    cmd = f'bridge vlan del dev {interface} vid 1'
-                    self._cmd(cmd)
-                    vlan_id = interface_config['native_vlan']
-                    if int(vlan_id) != 1:
-                        if 1 in vlan_add:
-                            vlan_add.remove(1)
-                        vlan_del.add(1)
-                    cmd = f'bridge vlan add dev {interface} vid {vlan_id} pvid untagged master'
-                    self._cmd(cmd)
-                    vlan_add.add(vlan_id)
-                    if vlan_id in vlan_del:
-                        vlan_del.remove(vlan_id)
-
-                if 'allowed_vlan' in interface_config:
-                    vlan_filter = 1
-                    if 'native_vlan' not in interface_config:
-                        cmd = f'bridge vlan del dev {interface} vid 1'
+                if vlan_filter:
+                    
+                    del_member_vlan_ids = get_vlan_ids(interface)
+                    # Delete all VLAN IDS configured in the interface first
+                    for vlan in del_member_vlan_ids:
+                        cmd = f'bridge vlan del dev {interface} vid {vlan} master'
                         self._cmd(cmd)
-                        vlan_del.add(1)
-                    for vlan in interface_config['allowed_vlan']:
-                        cmd = f'bridge vlan add dev {interface} vid {vlan} master'
+                    
+                    if 'native_vlan' in interface_config:
+                        vlan_id = interface_config['native_vlan']
+                        cmd = f'bridge vlan add dev {interface} vid {vlan_id} pvid untagged master'
+                        self._cmd(cmd)
+                    else:
+                        # VLAN 1 is the default VLAN for all unlabeled packets
+                        cmd = f'bridge vlan add dev {interface} vid 1 pvid untagged master'
                         self._cmd(cmd)
-                        vlan_add.add(vlan)
-                        if vlan in vlan_del:
-                            vlan_del.remove(vlan)
-
-        for vlan in vlan_del:
-            cmd = f'bridge vlan del dev {ifname} vid {vlan} self'
-            self._cmd(cmd)
-
-        for vlan in vlan_add:
-            cmd = f'bridge vlan add dev {ifname} vid {vlan} self'
-            self._cmd(cmd)
 
-        # enable/disable Vlan Filter
-        self.set_vlan_filter(vlan_filter)
+                    if 'allowed_vlan' in interface_config:
+                        for vlan in interface_config['allowed_vlan']:
+                            cmd = f'bridge vlan add dev {interface} vid {vlan} master'
+                            self._cmd(cmd)
 
 
         # Enable/Disable of an interface must always be done at the end of the
diff --git a/python/vyos/ifconfig/interface.py b/python/vyos/ifconfig/interface.py
index 1561d340e..ffe55648c 100644
--- a/python/vyos/ifconfig/interface.py
+++ b/python/vyos/ifconfig/interface.py
@@ -900,49 +900,29 @@ class Interface(Control):
             if 'priority' in bridge_config:
                 self.set_path_cost(bridge_config['priority'])
 
-            vlan_filter = 0
-            vlan_add = set()
-
             del_ifname_vlan_ids = get_vlan_ids(ifname)
-            bridge_vlan_filter = Section.klass(bridge)(bridge, create=True).get_vlan_filter()
+            bridge_vlan_filter = int(Section.klass(bridge)(bridge, create=True).get_vlan_filter())
 
             if bridge_vlan_filter:
-                if 1 in del_ifname_vlan_ids:
-                    del_ifname_vlan_ids.remove(1)
-                vlan_filter = 1
-
-            for vlan in del_ifname_vlan_ids:
-                cmd = f'bridge vlan del dev {ifname} vid {vlan}'
-                self._cmd(cmd)
-
-            if 'native_vlan' in bridge_config:
-                vlan_filter = 1
-                cmd = f'bridge vlan del dev {self.ifname} vid 1'
-                self._cmd(cmd)
-                vlan_id = bridge_config['native_vlan']
-                cmd = f'bridge vlan add dev {self.ifname} vid {vlan_id} pvid untagged master'
-                self._cmd(cmd)
-                vlan_add.add(vlan_id)
-
-            if 'allowed_vlan' in bridge_config:
-                vlan_filter = 1
-                if 'native_vlan' not in bridge_config:
-                    cmd = f'bridge vlan del dev {self.ifname} vid 1'
-                    self._cmd(cmd)
-                for vlan in bridge_config['allowed_vlan']:
-                    cmd = f'bridge vlan add dev {self.ifname} vid {vlan} master'
+
+                # Delete all VLAN IDS configured in the interface first
+                for vlan in del_ifname_vlan_ids:
+                    cmd = f'bridge vlan del dev {ifname} vid {vlan} master'
                     self._cmd(cmd)
-                    vlan_add.add(vlan)
 
-            if vlan_filter:
-                # Setting VLAN ID for the bridge
-                for vlan in vlan_add:
-                    cmd = f'bridge vlan add dev {bridge} vid {vlan} self'
+                if 'native_vlan' in bridge_config:
+                    vlan_id = bridge_config['native_vlan']
+                    cmd = f'bridge vlan add dev {ifname} vid {vlan_id} pvid untagged master'
+                    self._cmd(cmd)
+                else:
+                    # VLAN 1 is the default VLAN for all unlabeled packets
+                    cmd = f'bridge vlan add dev {ifname} vid 1 pvid untagged master'
                     self._cmd(cmd)
 
-                # enable/disable Vlan Filter
-                # When the VLAN aware option is not detected, the setting of `bridge` should not be overwritten
-                Section.klass(bridge)(bridge, create=True).set_vlan_filter(vlan_filter)
+                if 'allowed_vlan' in bridge_config:
+                    for vlan in bridge_config['allowed_vlan']:
+                        cmd = f'bridge vlan add dev {ifname} vid {vlan} master'
+                        self._cmd(cmd)
 
     def set_dhcp(self, enable):
         """
diff --git a/smoketest/scripts/cli/test_interfaces_bridge.py b/smoketest/scripts/cli/test_interfaces_bridge.py
index 464226b6f..c1e98cc67 100755
--- a/smoketest/scripts/cli/test_interfaces_bridge.py
+++ b/smoketest/scripts/cli/test_interfaces_bridge.py
@@ -25,14 +25,13 @@ from netifaces import interfaces
 from vyos.ifconfig import Section
 from vyos.util import cmd
 from vyos.util import read_file
+from vyos.validate import is_intf_addr_assigned
 
 class BridgeInterfaceTest(BasicInterfaceTest.BaseTest):
     def setUp(self):
         self._test_ip = True
         self._test_ipv6 = True
         self._test_ipv6_pd = True
-        self._test_vlan = True
-        self._test_qinq = True
         self._base_path = ['interfaces', 'bridge']
         self._mirror_interfaces = ['dum21354']
         self._members = []
@@ -52,6 +51,12 @@ class BridgeInterfaceTest(BasicInterfaceTest.BaseTest):
         self._interfaces = list(self._options)
 
         super().setUp()
+    
+    def tearDown(self):
+        for intf in self._interfaces:
+            self.session.delete(self._base_path + [intf])
+        
+        super().tearDown()
 
     def test_add_remove_bridge_member(self):
         # Add member interfaces to bridge and set STP cost/priority
@@ -88,11 +93,15 @@ class BridgeInterfaceTest(BasicInterfaceTest.BaseTest):
         self.session.commit()
 
     def test_bridge_vlan_filter(self):
+        
+        vif_vlan = 2
         # Add member interface to bridge and set VLAN filter
         for interface in self._interfaces:
             base = self._base_path + [interface]
-            self.session.set(base + ['vif', '1', 'address', '192.0.2.1/24'])
-            self.session.set(base + ['vif', '2', 'address', '192.0.3.1/24'])
+            self.session.set(base + ['enable-vlan'])
+            self.session.set(base + ['address', '192.0.2.1/24'])
+            self.session.set(base + ['vif', str(vif_vlan), 'address', '192.0.3.1/24'])
+            self.session.set(base + ['vif', str(vif_vlan), 'mtu', self._mtu])
 
             vlan_id = 101
             allowed_vlan = 2
@@ -159,7 +168,13 @@ class BridgeInterfaceTest(BasicInterfaceTest.BaseTest):
 
             for member in self._members:
                 self.assertIn(member, bridge_members)
-
+        
+        # Check VLAN sub interface
+        for intf in self._interfaces:
+            vif = f'{intf}.{vif_vlan}'
+            tmp = read_file(f'/sys/class/net/{vif}/mtu')
+            self.assertEqual(tmp, self._mtu)
+        
         # delete all members
         for interface in self._interfaces:
             self.session.delete(self._base_path + [interface, 'member'])
diff --git a/src/conf_mode/interfaces-bridge.py b/src/conf_mode/interfaces-bridge.py
index 7af3e3d7c..d5bcfec4f 100755
--- a/src/conf_mode/interfaces-bridge.py
+++ b/src/conf_mode/interfaces-bridge.py
@@ -41,26 +41,6 @@ from vyos import ConfigError
 from vyos import airbag
 airbag.enable()
 
-def helper_check_removed_vlan(conf,bridge,key,key_mangling):
-    key_update = re.sub(key_mangling[0], key_mangling[1], key)
-    if dict_search('member.interface', bridge):
-        for interface in bridge['member']['interface']:
-            tmp = leaf_node_changed(conf, ['member', 'interface',interface,key])
-            if tmp:
-                if 'member' in bridge:
-                    if 'interface' in bridge['member']:
-                        if interface in bridge['member']['interface']:
-                            bridge['member']['interface'][interface].update({f'{key_update}_removed': tmp })
-                        else:
-                            bridge['member']['interface'].update({interface: {f'{key_update}_removed': tmp }})
-                    else:
-                        bridge['member'].update({ 'interface': {interface: {f'{key_update}_removed': tmp }}})
-                else:
-                    bridge.update({'member': { 'interface': {interface: {f'{key_update}_removed': tmp }}}})
-                    
-    return bridge
-    
-
 def get_config(config=None):
     """
     Retrive CLI config as dictionary. Dictionary can never be empty, as at least the
@@ -80,12 +60,6 @@ def get_config(config=None):
             bridge['member'].update({'interface_remove': tmp })
         else:
             bridge.update({'member': {'interface_remove': tmp }})
-        
-            
-    # determine which members vlan have been removed
-    
-    bridge = helper_check_removed_vlan(conf,bridge,'native-vlan',('-', '_'))
-    bridge = helper_check_removed_vlan(conf,bridge,'allowed-vlan',('-', '_'))
 
     if dict_search('member.interface', bridge):
         # XXX: T2665: we need a copy of the dict keys for iteration, else we will get:
@@ -99,7 +73,7 @@ def get_config(config=None):
         # the default dictionary is not properly paged into the dict (see T2665)
         # thus we will ammend it ourself
         default_member_values = defaults(base + ['member', 'interface'])
-        vlan_aware = False 
+        vlan_aware = True if 'enable_vlan' in bridge else False
         for interface,interface_config in bridge['member']['interface'].items():
             bridge['member']['interface'][interface] = dict_merge(
                     default_member_values, bridge['member']['interface'][interface])
@@ -120,19 +94,11 @@ def get_config(config=None):
             # Bridge members must not have an assigned address
             tmp = has_address_configured(conf, interface)
             if tmp: bridge['member']['interface'][interface].update({'has_address' : ''})
-            
+
             # VLAN-aware bridge members must not have VLAN interface configuration
-            if 'native_vlan' in interface_config:
-                vlan_aware = True
-            
-            if 'allowed_vlan' in interface_config:
-                vlan_aware = True
-            
-            
-            if vlan_aware:
-                tmp = has_vlan_subinterface_configured(conf,interface)
-                if tmp:
-                    if tmp: bridge['member']['interface'][interface].update({'has_vlan' : ''})
+            tmp = has_vlan_subinterface_configured(conf,interface)
+            if vlan_aware and tmp:
+                bridge['member']['interface'][interface].update({'has_vlan' : ''})
 
     return bridge
 
@@ -142,8 +108,10 @@ def verify(bridge):
 
     verify_dhcpv6(bridge)
     verify_vrf(bridge)
+
+    vlan_aware = True if 'enable_vlan' in bridge else False
     
-    vlan_aware = False 
+    ifname = bridge['ifname']
 
     if dict_search('member.interface', bridge):
         for interface, interface_config in bridge['member']['interface'].items():
@@ -166,31 +134,37 @@ def verify(bridge):
 
             if 'has_address' in interface_config:
                 raise ConfigError(error_msg + 'it has an address assigned!')
-            
-            if 'has_vlan' in interface_config:
-                raise ConfigError(error_msg + 'it has an VLAN subinterface assigned!')
-            
-            # VLAN-aware bridge members must not have VLAN interface configuration
-            if 'native_vlan' in interface_config:
-                vlan_aware = True
-            
-            if 'allowed_vlan' in interface_config:
-                vlan_aware = True
-            
-            if vlan_aware and 'wlan' in interface:
-                raise ConfigError(error_msg + 'VLAN aware cannot be set!')
-            
-            if 'allowed_vlan' in interface_config:
-                for vlan in interface_config['allowed_vlan']:
-                    if re.search('[0-9]{1,4}-[0-9]{1,4}', vlan):
-                        vlan_range = vlan.split('-')
-                        if int(vlan_range[0]) <1 and int(vlan_range[0])>4094:
-                            raise ConfigError('VLAN ID must be between 1 and 4094')
-                        if int(vlan_range[1]) <1 and int(vlan_range[1])>4094:
-                            raise ConfigError('VLAN ID must be between 1 and 4094')
-                    else:
-                        if int(vlan) <1 and int(vlan)>4094:
-                            raise ConfigError('VLAN ID must be between 1 and 4094')
+
+            if vlan_aware:
+                if 'has_vlan' in interface_config:
+                    raise ConfigError(error_msg + 'it has an VLAN subinterface assigned!')
+
+                if 'wlan' in interface:
+                    raise ConfigError(error_msg + 'VLAN aware cannot be set!')
+
+                if 'allowed_vlan' in interface_config:
+                    for vlan in interface_config['allowed_vlan']:
+                        if re.search('[0-9]{1,4}-[0-9]{1,4}', vlan):
+                            vlan_range = vlan.split('-')
+                            if int(vlan_range[0]) <1 and int(vlan_range[0])>4094:
+                                raise ConfigError('VLAN ID must be between 1 and 4094')
+                            if int(vlan_range[1]) <1 and int(vlan_range[1])>4094:
+                                raise ConfigError('VLAN ID must be between 1 and 4094')
+                        else:
+                            if int(vlan) <1 and int(vlan)>4094:
+                                raise ConfigError('VLAN ID must be between 1 and 4094')
+            else:
+                if 'allowed_vlan' in interface_config:
+                    raise ConfigError(f'You must first activate "enable-vlan" of {ifname} bridge to use "allowed-vlan"')
+                if 'native_vlan' in interface_config:
+                    raise ConfigError(f'You must first activate "enable-vlan" of {ifname} bridge to use "native-vlan"')
+    
+    if vlan_aware:
+        if dict_search('vif.1', bridge):
+            raise ConfigError(f'VLAN 1 sub interface cannot be set for VLAN aware bridge {ifname}, and VLAN 1 is always the parent interface')
+    else:
+        if dict_search('vif', bridge):
+            raise ConfigError(f'You must first activate "enable-vlan" of {ifname} bridge to use "vif"')
 
     return None
 
diff --git a/src/migration-scripts/interfaces/18-to-19 b/src/migration-scripts/interfaces/18-to-19
new file mode 100755
index 000000000..86b2343b9
--- /dev/null
+++ b/src/migration-scripts/interfaces/18-to-19
@@ -0,0 +1,84 @@
+#!/usr/bin/env python3
+#
+# Copyright (C) 2020 VyOS maintainers and contributors
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License version 2 or later as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# VLAN 1 command line migration for bridge aware
+# https://phabricator.vyos.net/T3137
+
+import sys
+from vyos.configtree import ConfigTree
+
+if __name__ == '__main__':
+    if (len(sys.argv) < 1):
+        print("Must specify file name!")
+        sys.exit(1)
+
+    file_name = sys.argv[1]
+
+    with open(file_name, 'r') as f:
+        config_file = f.read()
+
+    config = ConfigTree(config_file)
+    base = ['interfaces', 'bridge']
+    if not config.exists(base):
+        # Nothing to do
+        sys.exit(0)
+
+    vlan_add = set()
+
+    for interface in config.list_nodes(base):
+        vif_1_old_base = base + [interface, 'vif', '1']
+        if config.exists(vif_1_old_base):
+            address_base = vif_1_old_base + ['address']
+            if config.exists(address_base):
+                address = config.return_values(address_base)
+                for addr in address:
+                    config.set(base + [interface, 'address'],addr,False)
+            config.delete(vif_1_old_base)
+
+        # Get all VLANs
+        member_base = base + [interface, 'member', 'interface']
+        if config.exists(member_base):
+            for mem_intf in config.list_nodes(member_base):
+                native_vlan_base = member_base + [mem_intf,'native-vlan']
+                allowed_vlan_base = member_base + [mem_intf,'allowed-vlan']
+                if config.exists(native_vlan_base):
+                    vlan = config.return_values(native_vlan_base)[0]
+                    vlan_add.add(vlan)
+
+                if config.exists(allowed_vlan_base):
+                    vlan_ranges = config.return_values(allowed_vlan_base)
+                    for vlan_range in vlan_ranges:
+                        vlan_data = vlan_range.split('-')
+                        for vlan in range(int(vlan_data[0]),int(vlan_data[1])+1):
+                            vlan_add.add(vlan)
+
+        # Start configuration
+        if len(vlan_add):
+            for vlan in vlan_add:
+                if int(vlan) != 1:
+                    config.set(base + [interface, 'vif', vlan])
+            config.set(base + [interface, 'enable-vlan'])
+        
+        
+        config.set_tag(base + [interface])
+
+
+    try:
+        with open(file_name, 'w') as f:
+            f.write(config.to_string())
+    except OSError as e:
+        print("Failed to save the modified config: {}".format(e))
+        sys.exit(1)