Merge pull request #1450 from c-po/bridge-fixes-equuleus

bridge: bugfixes for equuleus

6 files changed, 115 insertions, 97 deletions

diff --git a/python/vyos/configdict.py b/python/vyos/configdict.py
index 70e5cd889..466433c37 100644
--- a/python/vyos/configdict.py
+++ b/python/vyos/configdict.py
@@ -228,24 +228,10 @@ def is_member(conf, interface, intftype=None):
         for intf in conf.list_nodes(base):
             member = base + [intf, 'member', 'interface', interface]
             if conf.exists(member):
-                member_type = Section.section(interface)
-                # Check if it's a VLAN (QinQ) interface
-                interface = interface.split('.')
-                if len(interface) == 3:
-                    if conf.exists(['interfaces', member_type, interface[0], 'vif-s', interface[1], 'vif-c', interface[2]]):
-                        tmp = conf.get_config_dict(['interfaces', member_type, interface[0]],
-                                                   key_mangling=('-', '_'), get_first_key=True)
-                        ret_val.update({intf : tmp})
-                elif len(interface) == 2:
-                    if conf.exists(['interfaces', member_type, interface[0], 'vif', interface[1]]):
-                        tmp = conf.get_config_dict(['interfaces', member_type, interface[0]],
-                                                   key_mangling=('-', '_'), get_first_key=True)
-                        ret_val.update({intf : tmp})
-                else:
-                    if conf.exists(['interfaces', member_type, interface[0]]):
-                        tmp = conf.get_config_dict(['interfaces', member_type, interface[0]],
-                                                   key_mangling=('-', '_'), get_first_key=True)
-                        ret_val.update({intf : tmp})
+                tmp = conf.get_config_dict(member, key_mangling=('-', '_'),
+                                           get_first_key=True,
+                                           no_tag_node_value_mangle=True)
+                ret_val.update({intf : tmp})
 
     old_level = conf.set_level(old_level)
     return ret_val
diff --git a/python/vyos/configverify.py b/python/vyos/configverify.py
index 44342f289..356246053 100644
--- a/python/vyos/configverify.py
+++ b/python/vyos/configverify.py
@@ -196,10 +196,10 @@ def verify_address(config):
     of a bridge or bond.
     """
     if {'is_bridge_member', 'address'} <= set(config):
-        raise ConfigError(
-            'Cannot assign address to interface "{ifname}" as it is a '
-            'member of bridge "{is_bridge_member}"!'.format(**config))
-
+        interface = config['ifname']
+        bridge_name = next(iter(config['is_bridge_member']))
+        raise ConfigError(f'Cannot assign address to interface "{interface}" '
+                          f'as it is a member of bridge "{bridge_name}"!')
 
 def verify_bridge_delete(config):
     """
@@ -209,9 +209,9 @@ def verify_bridge_delete(config):
     """
     if 'is_bridge_member' in config:
         interface = config['ifname']
-        for bridge in config['is_bridge_member']:
-            raise ConfigError(f'Interface "{interface}" cannot be deleted as it '
-                              f'is a member of bridge "{bridge}"!')
+        bridge_name = next(iter(config['is_bridge_member']))
+        raise ConfigError(f'Interface "{interface}" cannot be deleted as it '
+                          f'is a member of bridge "{bridge_name}"!')
 
 def verify_interface_exists(ifname):
     """
diff --git a/python/vyos/ifconfig/bridge.py b/python/vyos/ifconfig/bridge.py
index ffd9c590f..eef02f21f 100644
--- a/python/vyos/ifconfig/bridge.py
+++ b/python/vyos/ifconfig/bridge.py
@@ -183,6 +183,11 @@ class BridgeIf(Interface):
         """
         self.set_interface('vlan_filter', state)
 
+        # VLAN of bridge parent interface is always 1
+        # VLAN 1 is the default VLAN for all unlabeled packets
+        cmd = f'bridge vlan add dev {self.ifname} vid 1 pvid untagged self'
+        self._cmd(cmd)
+
     def set_multicast_querier(self, enable):
         """
         Sets whether the bridge actively runs a multicast querier or not. When a
@@ -272,30 +277,6 @@ class BridgeIf(Interface):
         vlan_filter = '1' if 'enable_vlan' in config else '0'
         self.set_vlan_filter(vlan_filter)
 
-        ifname = config['ifname']
-        if int(vlan_filter):
-            add_vlan = []
-            cur_vlan_ids = get_vlan_ids(ifname)
-
-            tmp = dict_search('vif', config)
-            if tmp:
-                for vif, vif_config in tmp.items():
-                    add_vlan.append(vif)
-
-            # Remove redundant VLANs from the system
-            for vlan in list_diff(cur_vlan_ids, add_vlan):
-                cmd = f'bridge vlan del dev {ifname} vid {vlan} self'
-                self._cmd(cmd)
-
-            for vlan in add_vlan:
-                cmd = f'bridge vlan add dev {ifname} vid {vlan} 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:
             for interface, interface_config in tmp.items():
@@ -325,15 +306,13 @@ class BridgeIf(Interface):
 
                 # set bridge port path cost
                 if 'cost' in interface_config:
-                    value = interface_config.get('cost')
-                    lower.set_path_cost(value)
+                    lower.set_path_cost(interface_config['cost'])
 
                 # set bridge port path priority
                 if 'priority' in interface_config:
-                    value = interface_config.get('priority')
-                    lower.set_path_priority(value)
+                    lower.set_path_priority(interface_config['priority'])
 
-                if int(vlan_filter):
+                if 'enable_vlan' in config:
                     add_vlan = []
                     native_vlan_id = None
                     allowed_vlan_ids= []
@@ -363,6 +342,7 @@ class BridgeIf(Interface):
                     for vlan in allowed_vlan_ids:
                         cmd = f'bridge vlan add dev {interface} vid {vlan} master'
                         self._cmd(cmd)
+
                     # Setting native VLAN to system
                     if native_vlan_id:
                         cmd = f'bridge vlan add dev {interface} vid {native_vlan_id} pvid untagged master'
diff --git a/smoketest/scripts/cli/test_interfaces_bridge.py b/smoketest/scripts/cli/test_interfaces_bridge.py
index b4b91f226..386d37614 100755
--- a/smoketest/scripts/cli/test_interfaces_bridge.py
+++ b/smoketest/scripts/cli/test_interfaces_bridge.py
@@ -144,6 +144,51 @@ class BridgeInterfaceTest(BasicInterfaceTest.TestCase):
         super().test_vif_8021q_mtu_limits()
 
     def test_bridge_vlan_filter(self):
+        def _verify_members() -> None:
+            # check member interfaces are added on the bridge
+            for interface in self._interfaces:
+                bridge_members = []
+                for tmp in glob(f'/sys/class/net/{interface}/lower_*'):
+                    bridge_members.append(os.path.basename(tmp).replace('lower_', ''))
+
+                # We can not use assertListEqual() b/c the position of the interface
+                # names within the list is not fixed
+                self.assertEqual(len(self._members), len(bridge_members))
+                for member in self._members:
+                    self.assertIn(member, bridge_members)
+
+        def _check_vlan_filter() -> None:
+            for interface in self._interfaces:
+                tmp = cmd(f'bridge -j vlan show dev {interface}')
+                tmp = json.loads(tmp)
+                self.assertIsNotNone(tmp)
+
+                for interface_status in tmp:
+                    ifname = interface_status['ifname']
+                    for interface in self._members:
+                        vlan_success = 0;
+                        if interface == ifname:
+                            vlans_status = interface_status['vlans']
+                            for vlan_status in vlans_status:
+                                vlan_id = vlan_status['vlan']
+                                flag_num = 0
+                                if 'flags' in vlan_status:
+                                    flags = vlan_status['flags']
+                                    for flag in flags:
+                                        flag_num = flag_num +1
+                                if vlan_id == 2:
+                                    if flag_num == 0:
+                                        vlan_success = vlan_success + 1
+                                else:
+                                    for id in range(4,10):
+                                        if vlan_id == id:
+                                            if flag_num == 0:
+                                                vlan_success = vlan_success + 1
+                                    if vlan_id >= 101:
+                                        if flag_num == 2:
+                                            vlan_success = vlan_success + 1
+                            self.assertGreaterEqual(vlan_success, 7)
+
         vif_vlan = 2
         # Add member interface to bridge and set VLAN filter
         for interface in self._interfaces:
@@ -167,61 +212,50 @@ class BridgeInterfaceTest(BasicInterfaceTest.TestCase):
         # commit config
         self.cli_commit()
 
-        # Detect the vlan filter function
+        # Verify correct setting of VLAN filter function
         for interface in self._interfaces:
             tmp = read_file(f'/sys/class/net/{interface}/bridge/vlan_filtering')
             self.assertEqual(tmp, '1')
 
-        # Execute the program to obtain status information
-        json_data = cmd('bridge -j vlan show', shell=True)
-        vlan_filter_status = None
-        vlan_filter_status = json.loads(json_data)
-
-        if vlan_filter_status is not None:
-            for interface_status in vlan_filter_status:
-                ifname = interface_status['ifname']
-                for interface in self._members:
-                    vlan_success = 0;
-                    if interface == ifname:
-                        vlans_status = interface_status['vlans']
-                        for vlan_status in vlans_status:
-                            vlan_id = vlan_status['vlan']
-                            flag_num = 0
-                            if 'flags' in vlan_status:
-                                flags = vlan_status['flags']
-                                for flag in flags:
-                                    flag_num = flag_num +1
-                            if vlan_id == 2:
-                                if flag_num == 0:
-                                    vlan_success = vlan_success + 1
-                            else:
-                                for id in range(4,10):
-                                    if vlan_id == id:
-                                        if flag_num == 0:
-                                            vlan_success = vlan_success + 1
-                                if vlan_id >= 101:
-                                    if flag_num == 2:
-                                        vlan_success = vlan_success + 1
-                        if vlan_success >= 7:
-                            self.assertTrue(True)
-                        else:
-                            self.assertTrue(False)
+        # Execute the program to obtain status information and verify proper
+        # VLAN filter setup
+        _check_vlan_filter()
 
-        else:
-            self.assertTrue(False)
+        # check member interfaces are added on the bridge
+        _verify_members()
+
+        # change member interface description to trigger config update,
+        # VLANs must still exist (T4565)
+        for interface in self._interfaces:
+            for member in self._members:
+                self.cli_set(['interfaces', Section.section(member), member, 'description', f'foo {member}'])
+
+        # commit config
+        self.cli_commit()
 
         # check member interfaces are added on the bridge
+        _verify_members()
+
+        # Execute the program to obtain status information and verify proper
+        # VLAN filter setup
+        _check_vlan_filter()
+
+        # delete all members
+        for interface in self._interfaces:
+            self.cli_delete(self._base_path + [interface, 'member'])
+
+        # commit config
+        self.cli_commit()
+
+        # verify member interfaces are no longer assigned on the bridge
         for interface in self._interfaces:
             bridge_members = []
             for tmp in glob(f'/sys/class/net/{interface}/lower_*'):
                 bridge_members.append(os.path.basename(tmp).replace('lower_', ''))
 
+            self.assertNotEqual(len(self._members), len(bridge_members))
             for member in self._members:
-                self.assertIn(member, bridge_members)
-
-        # delete all members
-        for interface in self._interfaces:
-            self.cli_delete(self._base_path + [interface, 'member'])
+                self.assertNotIn(member, bridge_members)
 
 
     def test_bridge_vif_members(self):
diff --git a/src/conf_mode/interfaces-bridge.py b/src/conf_mode/interfaces-bridge.py
index 9ad39e080..f548a0e75 100755
--- a/src/conf_mode/interfaces-bridge.py
+++ b/src/conf_mode/interfaces-bridge.py
@@ -60,7 +60,7 @@ def get_config(config=None):
         else:
             bridge.update({'member': {'interface_remove': tmp }})
 
-    if dict_search('member.interface', bridge):
+    if dict_search('member.interface', bridge) != None:
         # XXX: T2665: we need a copy of the dict keys for iteration, else we will get:
         # RuntimeError: dictionary changed size during iteration
         for interface in list(bridge['member']['interface']):
@@ -102,6 +102,14 @@ def get_config(config=None):
             if 'enable_vlan' in bridge and tmp:
                 bridge['member']['interface'][interface].update({'has_vlan' : ''})
 
+    # delete empty dictionary keys - no need to run code paths if nothing is there to do
+    if 'member' in bridge:
+        if 'interface' in bridge['member'] and len(bridge['member']['interface']) == 0:
+            del bridge['member']['interface']
+
+        if len(bridge['member']) == 0:
+            del bridge['member']
+
     return bridge
 
 def verify(bridge):
diff --git a/src/conf_mode/interfaces-macsec.py b/src/conf_mode/interfaces-macsec.py
index 6ec34a961..5ae07dae0 100755
--- a/src/conf_mode/interfaces-macsec.py
+++ b/src/conf_mode/interfaces-macsec.py
@@ -84,6 +84,16 @@ def verify(macsec):
             raise ConfigError('Missing mandatory MACsec security '
                               'keys as encryption is enabled!')
 
+        cak_len = len(dict_search('security.mka.cak', macsec))
+
+        if dict_search('security.cipher', macsec) == 'gcm-aes-128' and cak_len != 32:
+            # gcm-aes-128 requires a 128bit long key - 32 characters (string) = 16byte = 128bit
+            raise ConfigError('gcm-aes-128 requires a 128bit long key!')
+
+        elif dict_search('security.cipher', macsec) == 'gcm-aes-256' and cak_len != 64:
+            # gcm-aes-128 requires a 128bit long key - 64 characters (string) = 32byte = 256bit
+            raise ConfigError('gcm-aes-128 requires a 256bit long key!')
+
     if 'source_interface' in macsec:
         # MACsec adds a 40 byte overhead (32 byte MACsec + 8 bytes VLAN 802.1ad
         # and 802.1q) - we need to check the underlaying MTU if our configured