Merge pull request #5479 from c-po/T3871-ifname-store

T3871: bind interface names to hardware slots instead of hw-id

4 files changed, 221 insertions, 8 deletions

diff --git a/smoketest/scripts/cli/test_interfaces_bonding.py b/smoketest/scripts/cli/test_interfaces_bonding.py
index 3a3b1e5cf..683fcc947 100755
--- a/smoketest/scripts/cli/test_interfaces_bonding.py
+++ b/smoketest/scripts/cli/test_interfaces_bonding.py
@@ -24,6 +24,7 @@ from vyos.ifconfig import Section
 from vyos.ifconfig.interface import Interface
 from vyos.configsession import ConfigSessionError
 from vyos.utils.network import get_interface_config
+from vyos.utils.network import interface_exists
 from vyos.utils.file import read_file
 
 class BondingInterfaceTest(BasicInterfaceTest.TestCase):
@@ -134,6 +135,95 @@ class BondingInterfaceTest(BasicInterfaceTest.TestCase):
             state = Interface(remove_member).get_admin_state()
             self.assertEqual('up', state)
 
+    def test_bonding_bare_member_node_released(self):
+        # T3871: a member whose ethernet node exists but carries nothing at
+        # all - a plain "set interfaces ethernet eth1" and no more - is still
+        # a configured interface, not an empty one. When it is released from
+        # the bond it must be brought back up, not torn down as though the
+        # operator had deleted it.
+        member = self._members[0]
+        self.cli_set(['interfaces', 'ethernet', member])
+
+        for interface in self._interfaces:
+            for option in self._options.get(interface, []):
+                self.cli_set(self._base_path + [interface] + option.split())
+        self.cli_commit()
+
+        for interface in self._interfaces:
+            self.cli_delete(
+                self._base_path + [interface, 'member', 'interface', member])
+        self.cli_commit()
+
+        self.assertEqual(Interface(member).get_admin_state(), 'up')
+
+    def test_bonding_delete_and_readd_member(self):
+        # T3871: deleting a member's whole ethernet node while it is enslaved,
+        # then re-adding it, must leave the port present and usable - the
+        # hardware never went anywhere.
+        #
+        # A bond also owns its members' address and MTU
+        # (BondIf.get_inherit_bond_options()), and it goes on using this one
+        # after the node is gone. Putting the Kernel defaults back here left
+        # the member out of step with the bond it was still an active slave
+        # of: on a jumbo bond every frame the bond was sized for was silently
+        # discarded, and with the address reset the replies addressed to the
+        # bond were dropped by the port. Both have to wait until the bond has
+        # released it - which is asserted at the end.
+        if len(self._members) < 2:
+            self.skipTest('need a second member - the bond adopts the first '
+                          "member's address, so that one cannot show an "
+                          'address reset at all')
+
+        bond = self._interfaces[0]
+        # deliberately not _members[0]: the bond takes its address from that
+        # one, so it could not tell a reset apart from a no-op
+        member = self._members[-1]
+        own_mac = Interface(member).get_mac()
+        # any MTU the bond does not share with the Kernel default will do.
+        # Staying below it keeps this runnable on ports which cannot carry
+        # jumbo frames, which is every port the smoketests run on.
+        bond_mtu = '1400'
+
+        for interface in self._interfaces:
+            for option in self._options.get(interface, []):
+                self.cli_set(self._base_path + [interface] + option.split())
+            self.cli_set(self._base_path + [interface, 'mtu', bond_mtu])
+        self.cli_commit()
+
+        bond_mac = Interface(bond).get_mac()
+        self.assertNotEqual(own_mac, bond_mac,
+                            f'{member} was picked as the address donor - it '
+                            'cannot show an address reset')
+        self.assertEqual(Interface(member).get_mac(), bond_mac)
+
+        self.cli_delete(['interfaces', 'ethernet', member])
+        self.cli_commit()
+
+        self.assertTrue(interface_exists(member),
+                        f'{member} disappeared after its configuration was deleted')
+        self.assertEqual(read_file(f'/sys/class/net/{member}/mtu'), bond_mtu,
+                         f'{member} left out of step with the bond it is still in')
+        self.assertEqual(Interface(member).get_mac(), bond_mac,
+                         f'{member} no longer carries the address of the bond '
+                         'it is still a member of')
+
+        # released: now, and only now, the port goes back to what the Kernel
+        # gave it - which is what makes the skip above a deferral and not a
+        # permanent exemption
+        self.cli_delete(self._base_path + [bond, 'member', 'interface', member])
+        self.cli_commit()
+
+        self.assertEqual(Interface(member).get_mac(), own_mac,
+                         f'{member} did not get its own address back once the '
+                         'bond released it')
+        self.assertEqual(read_file(f'/sys/class/net/{member}/mtu'), '1500',
+                         f'{member} did not get the default MTU back once the '
+                         'bond released it')
+
+        self.cli_set(['interfaces', 'ethernet', member, 'description', 'readded'])
+        self.cli_commit()
+        self.assertEqual(read_file(f'/sys/class/net/{member}/ifalias'), 'readded')
+
     def test_bonding_append_member(self):
         # T9269: appending a member to a bond that already has one must not
         # disturb the existing member. leaf_node_changed() used to report the
diff --git a/smoketest/scripts/cli/test_interfaces_ethernet.py b/smoketest/scripts/cli/test_interfaces_ethernet.py
index 0d29d291a..473af051b 100755
--- a/smoketest/scripts/cli/test_interfaces_ethernet.py
+++ b/smoketest/scripts/cli/test_interfaces_ethernet.py
@@ -32,7 +32,10 @@ from vyos.ethtool import Ethtool
 from vyos.netlink import coalesce
 from vyos.frrender import mgmt_daemon
 from vyos.ifconfig import Section
+from vyos.ifconfig.interface import Interface
 from vyos.utils.file import read_file
+from vyos.utils.network import get_interface_config
+from vyos.utils.network import interface_exists
 from vyos.utils.network import is_intf_addr_assigned
 from vyos.utils.network import is_ipv6_link_local
 from vyos.utils.process import cmdl
@@ -52,10 +55,6 @@ class EthernetInterfaceTest(BasicInterfaceTest.TestCase):
             for tmp in Section.interfaces('ethernet', vlan=False):
                 cls._interfaces.append(tmp)
 
-        cls._macs = {}
-        for interface in cls._interfaces:
-            cls._macs[interface] = read_file(f'/sys/class/net/{interface}/address')
-
         # call base-classes classmethod
         super(EthernetInterfaceTest, cls).setUpClass()
 
@@ -67,7 +66,6 @@ class EthernetInterfaceTest(BasicInterfaceTest.TestCase):
             self.cli_delete(self._base_path + [interface])
             self.cli_set(self._base_path + [interface, 'duplex', 'auto'])
             self.cli_set(self._base_path + [interface, 'speed', 'auto'])
-            self.cli_set(self._base_path + [interface, 'hw-id', self._macs[interface]])
 
         self.cli_commit()
 
@@ -168,6 +166,43 @@ class EthernetInterfaceTest(BasicInterfaceTest.TestCase):
         # manually, else tearDown() will have problem in commit()
         self.cli_delete(self._base_path + [unknonw_interface])
 
+    def test_delete_and_readd_interface(self):
+        # T3871: deleting an ethernet interface removes its configuration, but
+        # the port itself is "eternal" - it must still be present afterwards,
+        # keep its name, and be configurable again. An unconfigured port is
+        # not a deleted one.
+        for interface in self._interfaces:
+            self.cli_set(self._base_path + [interface, 'description', 'before'])
+        self.cli_commit()
+
+        for interface in self._interfaces:
+            self.cli_delete(self._base_path + [interface])
+        self.cli_commit()
+
+        for interface in self._interfaces:
+            # an ethernet port belongs to the kernel - deleting its
+            # configuration returns it to the state the kernel gave it, it
+            # does not take the port away or leave it stranded admin-down
+            self.assertTrue(interface_exists(interface),
+                            f'{interface} disappeared after its configuration '
+                            'was deleted')
+            self.assertEqual(Interface(interface).get_admin_state(), 'up',
+                             f'{interface} was left admin-down after its '
+                             'configuration was deleted')
+            tmp = get_interface_config(interface)
+            self.assertEqual(tmp['mtu'], 1500,
+                             f'{interface} kept a stale MTU after its '
+                             'configuration was deleted')
+
+        # re-adding must work and take effect
+        for interface in self._interfaces:
+            self.cli_set(self._base_path + [interface, 'description', 'after'])
+        self.cli_commit()
+
+        for interface in self._interfaces:
+            self.assertEqual(
+                read_file(f'/sys/class/net/{interface}/ifalias'), 'after')
+
     def test_speed_duplex_verify(self):
         for interface in self._interfaces:
             self.cli_set(self._base_path + [interface, 'speed', '1000'])
diff --git a/smoketest/scripts/cli/test_interfaces_wireless_rescan.py b/smoketest/scripts/cli/test_interfaces_wireless_rescan.py
new file mode 100755
index 000000000..82dc146c9
--- /dev/null
+++ b/smoketest/scripts/cli/test_interfaces_wireless_rescan.py
@@ -0,0 +1,88 @@
+#!/usr/bin/env python3
+#
+# Copyright VyOS maintainers and contributors <maintainers@vyos.io>
+#
+# 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/>.
+
+import os
+import shutil
+import tempfile
+import unittest
+from glob import glob
+
+from base_vyostest_shim import VyOSUnitTestSHIM
+
+from vyos.configtree import ConfigTree
+from vyos.utils.file import read_file
+from vyos.utils.kernel import check_kmod
+from vyos.utils.network import interface_exists
+from vyos.utils.process import call
+from vyos.utils.process import rc_cmd
+
+rescan = '/usr/libexec/vyos/vyos-interface-rescan.py'
+rescan_log = '/var/log/vyatta/vyos-interface-rescan'
+
+class WirelessRescanTest(VyOSUnitTestSHIM.TestCase):
+    """T3871: a radio is not in the interface mapping file - every interface on
+    a phy carries that phy's address, so there is nothing there to anchor a
+    name to. The rescan has to find radios on the system instead. While it took
+    its whole list from the map, no radio was ever given a node and 'interfaces
+    wireless' stayed empty on a box which had one.
+    """
+
+    ifname = 'wlan99'
+
+    def setUp(self):
+        phys = sorted(os.path.basename(p)
+                      for p in glob('/sys/class/ieee80211/*'))
+        if not phys:
+            self.skipTest('no wireless phy on this system')
+        self.phy = phys[0]
+
+        # an interface of our own, so the test does not depend on which radios
+        # another testcase left behind
+        if interface_exists(self.ifname):
+            call(f'sudo iw dev {self.ifname} del')
+        if call(f'sudo iw phy {self.phy} interface add {self.ifname} '
+                'type managed') != 0:
+            self.skipTest(f'could not create an interface on {self.phy}')
+        self.addCleanup(call, f'sudo iw dev {self.ifname} del')
+
+    def test_a_radio_is_given_its_physical_device(self):
+        # a directory of our own, not /tmp itself - the rescan runs as root
+        # and fs.protected_regular refuses it an O_CREAT on a file it does not
+        # own in a world writable sticky directory
+        tmp_dir = tempfile.mkdtemp()
+        self.addCleanup(shutil.rmtree, tmp_dir)
+        tmp = os.path.join(tmp_dir, 'config.boot')
+        shutil.copy('/config/config.boot', tmp)
+
+        config = ConfigTree(read_file(tmp))
+        self.assertFalse(config.exists(['interfaces', 'wireless', self.ifname]))
+
+        rc, out = rc_cmd(f'sudo {rescan} {tmp}')
+        if rc != 0:
+            # it reports a failure to its own log rather than to stderr, so
+            # without this a failure here is just an exit code
+            _, log = rc_cmd(f'sudo tail -n 20 {rescan_log}')
+            self.fail(f'rescan exited {rc}: {out}\n{log}')
+
+        config = ConfigTree(read_file(tmp))
+        self.assertTrue(config.exists(['interfaces', 'wireless', self.ifname]))
+        self.assertEqual(
+            config.return_value(['interfaces', 'wireless', self.ifname,
+                                 'physical-device']), self.phy)
+
+if __name__ == '__main__':
+    check_kmod('mac80211_hwsim')
+    unittest.main(verbosity=2, failfast=VyOSUnitTestSHIM.TestCase.debug_on())
diff --git a/smoketest/scripts/cli/test_vpp.py b/smoketest/scripts/cli/test_vpp.py
index 9eb446615..87b901ffd 100755
--- a/smoketest/scripts/cli/test_vpp.py
+++ b/smoketest/scripts/cli/test_vpp.py
@@ -141,7 +141,7 @@ class TestVPP(VyOSUnitTestSHIM.TestCase):
         cls.cli_delete(cls, base_path)
         cls.cli_delete(cls, interfaces_path)
         # drop any pre-existing custom MAC so the MAC test baseline is the
-        # interface hardware address (hw-id)
+        # interface's own hardware address
         cls.cli_delete(cls, ['interfaces', 'ethernet', interface, 'mac'])
         # drop any pre-existing MTU so tests start from the default MTU
         cls.cli_delete(cls, ['interfaces', 'ethernet', interface, 'mtu'])
@@ -280,7 +280,7 @@ class TestVPP(VyOSUnitTestSHIM.TestCase):
             self.cli_discard()
 
         # A custom MAC address must reach the VPP dataplane, and removing it must
-        # restore the hardware address (hw-id). Rejection of drivers that cannot
+        # restore the hardware address. Rejection of drivers that cannot
         # change the MAC (e.g. vmxnet3) is not covered here, as the CI dataplane
         # NIC uses a supported driver.
         hw_mac = VPPControl().get_mac(interface)
@@ -289,7 +289,7 @@ class TestVPP(VyOSUnitTestSHIM.TestCase):
         self.cli_commit()
         self.assertEqual(VPPControl().get_mac(interface), mac)
 
-        # removing the custom MAC reverts to the hardware address (hw-id)
+        # removing the custom MAC reverts to the hardware address
         self.cli_delete(['interfaces', 'ethernet', interface, 'mac'])
         self.cli_commit()
         self.assertEqual(VPPControl().get_mac(interface), hw_mac)