vyos.ifconfig: T8358: clear qdiscs when deleting mirror CLI node

When removing the mirror CLI node to stop mirroring or redirecting traffic
to another interface, the egress configuration was not cleared. This caused
traffic to continue being sent out the SPAN port even after the node was
removed.

Fix by properly clearing all tc(8) qdiscs.

Update smoketests to verify nothing remains after mirror deletion.

6 files changed, 34 insertions, 19 deletions

diff --git a/python/vyos/ifconfig/interface.py b/python/vyos/ifconfig/interface.py
index 33b643e7e..a416de1ab 100644
--- a/python/vyos/ifconfig/interface.py
+++ b/python/vyos/ifconfig/interface.py
@@ -1617,7 +1617,7 @@ class Interface(Control):
         if 'netns' in self.config:
             return None
 
-        source_if = self.config['ifname']
+        source_if = self.ifname
 
         mirror_config = None
         if 'mirror' in self.config:
@@ -1631,8 +1631,8 @@ class Interface(Control):
         # clear existing ingess - ignore errors (e.g. "Error: Cannot find specified
         # qdisc on specified device") - we simply cleanup all stuff here
         if not 'qos' in self.config:
-            self._popen(f'tc qdisc del dev {source_if} parent ffff: 2>/dev/null');
-            self._popen(f'tc qdisc del dev {source_if} parent 1: 2>/dev/null');
+            self._popen(f'tc qdisc del dev {source_if} root 2>/dev/null')
+            self._popen(f'tc qdisc del dev {source_if} ingress 2>/dev/null')
 
         # Apply interface mirror policy
         if mirror_config:
@@ -1644,14 +1644,14 @@ class Interface(Control):
                     handle = '1: root prio'
                     parent = '1:'
 
-                # Mirror egress traffic
+                # Mirror traffic
                 mirror_cmd  = f'tc qdisc add dev {source_if} handle {handle}; '
                 # Export the mirrored traffic to the interface
                 mirror_cmd += f'tc filter add dev {source_if} parent {parent} protocol '\
                               f'all prio 10 u32 match u32 0 0 flowid 1:1 action mirred '\
                               f'egress mirror dev {target_if}'
                 _, err = self._popen(mirror_cmd)
-                if err: print('tc qdisc(filter for mirror port failed')
+                if err: print('tc filter for mirror port failed')
 
         # Apply interface traffic redirection policy
         elif 'redirect' in self.config:
@@ -1662,7 +1662,7 @@ class Interface(Control):
             _, err = self._popen(f'tc filter add dev {source_if} parent ffff: protocol '\
                                  f'all prio 10 u32 match u32 0 0 flowid 1:1 action mirred '\
                                  f'egress redirect dev {target_if}')
-            if err: print('tc filter add for redirect failed')
+            if err: print('tc filter for redirect failed')
 
     def set_per_client_thread(self, enable):
         """
diff --git a/smoketest/scripts/cli/base_interfaces_test.py b/smoketest/scripts/cli/base_interfaces_test.py
index 7e9c5b6a4..b2f700fce 100644
--- a/smoketest/scripts/cli/base_interfaces_test.py
+++ b/smoketest/scripts/cli/base_interfaces_test.py
@@ -13,6 +13,7 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import re
+import jmespath
 
 from json import loads
 from netifaces import ifaddresses # pylint: disable = no-name-in-module
@@ -121,9 +122,9 @@ def get_certificate_count(interface, cert_type):
     tmp = read_file(f'/run/wpa_supplicant/{interface}_{cert_type}.pem')
     return tmp.count(CERT_BEGIN)
 
-def is_mirrored_to(interface, mirror_if, qdisc):
+def is_mirrored_to(interface, mirror_if, qdisc) -> bool:
     """
-    Ask TC if we are mirroring traffic to a discrete interface.
+    Ask tc(8) if we are mirroring traffic to a specific interface.
 
     interface: source interface
     mirror_if: destination where we mirror our data to
@@ -132,12 +133,11 @@ def is_mirrored_to(interface, mirror_if, qdisc):
     if qdisc not in ['ffff', '1']:
         raise ValueError()
 
-    ret_val = False
-    tmp = cmd(f'tc -s -p filter ls dev {interface} parent {qdisc}: | grep mirred')
-    tmp = tmp.lower()
-    if mirror_if in tmp:
-        ret_val = True
-    return ret_val
+    tmp = loads(cmd(f'tc -json filter ls dev {interface} parent {qdisc}:'))
+    # the following syntax looks odd but we need to filter out the first
+    # result sets from tc which do not have "options.actions...".
+    tmp = jmespath.search("[?options.actions[0].kind=='mirred'].options.actions[0].{mirred_action: mirred_action, to_dev: to_dev} | [0]", tmp)
+    return bool(dict_search('mirred_action', tmp) == 'mirror' and dict_search('to_dev', tmp) == mirror_if)
 
 class BasicInterfaceTest:
     class TestCase(VyOSUnitTestSHIM.TestCase):
@@ -161,7 +161,7 @@ class BasicInterfaceTest:
         _test_addr = ['192.0.2.1/26', '192.0.2.255/31', '192.0.2.64/32',
                       '2001:db8:1::ffff/64', '2001:db8:101::1/112']
 
-        _mirror_interfaces = []
+        _mirror_interfaces = ['dum21354']
         # choose IPv6 minimum MTU value for tests - this must always work
         _mtu = '1280'
 
@@ -181,6 +181,7 @@ class BasicInterfaceTest:
             cls._test_ipv6_pd = cli_defined(cls._base_path + ['dhcpv6-options'], 'pd')
             cls._test_mtu = cli_defined(cls._base_path, 'mtu')
             cls._test_vrf = cli_defined(cls._base_path, 'vrf')
+            cls._test_mirror = cli_defined(cls._base_path, 'mirror')
 
             # Setup mirror interfaces for SPAN (Switch Port Analyzer)
             for span in cls._mirror_interfaces:
@@ -456,9 +457,13 @@ class BasicInterfaceTest:
                 self.cli_set(self._base_path + [interface, 'description', 'test_add_to_invalid_vrf'])
 
         def test_span_mirror(self):
-            if not self._mirror_interfaces:
+            if not self._test_mirror:
                 self.skipTest(MSG_TESTCASE_UNSUPPORTED)
 
+            for interface in self._interfaces:
+                for option in self._options.get(interface, []):
+                    self.cli_set(self._base_path + [interface] + option.split())
+
             # Check the two-way mirror rules of ingress and egress
             for mirror in self._mirror_interfaces:
                 for interface in self._interfaces:
@@ -473,6 +478,18 @@ class BasicInterfaceTest:
                     self.assertTrue(is_mirrored_to(interface, mirror, 'ffff'))
                     self.assertTrue(is_mirrored_to(interface, mirror, '1'))
 
+            # delete interface mirror - check that configuration from tc is removed
+            for mirror in self._mirror_interfaces:
+                for interface in self._interfaces:
+                    self.cli_delete(self._base_path + [interface, 'mirror'])
+            self.cli_commit()
+
+            # Verify config
+            for mirror in self._mirror_interfaces:
+                for interface in self._interfaces:
+                    self.assertFalse(is_mirrored_to(interface, mirror, 'ffff'))
+                    self.assertFalse(is_mirrored_to(interface, mirror, '1'))
+
         def test_interface_disable(self):
             # Check if description can be added to interface and
             # can be read back
diff --git a/smoketest/scripts/cli/test_interfaces_bonding.py b/smoketest/scripts/cli/test_interfaces_bonding.py
index a565168cf..a4afae5da 100755
--- a/smoketest/scripts/cli/test_interfaces_bonding.py
+++ b/smoketest/scripts/cli/test_interfaces_bonding.py
@@ -30,7 +30,6 @@ class BondingInterfaceTest(BasicInterfaceTest.TestCase):
     @classmethod
     def setUpClass(cls):
         cls._base_path = ['interfaces', 'bonding']
-        cls._mirror_interfaces = ['dum21354']
         cls._members = []
 
         # we need to filter out VLAN interfaces identified by a dot (.)
diff --git a/smoketest/scripts/cli/test_interfaces_bridge.py b/smoketest/scripts/cli/test_interfaces_bridge.py
index 1a67a9165..a41ad1482 100755
--- a/smoketest/scripts/cli/test_interfaces_bridge.py
+++ b/smoketest/scripts/cli/test_interfaces_bridge.py
@@ -35,7 +35,6 @@ class BridgeInterfaceTest(BasicInterfaceTest.TestCase):
     @classmethod
     def setUpClass(cls):
         cls._base_path = ['interfaces', 'bridge']
-        cls._mirror_interfaces = ['dum21354']
         cls._members = []
 
         # we need to filter out VLAN interfaces identified by a dot (.)
diff --git a/smoketest/scripts/cli/test_interfaces_dummy.py b/smoketest/scripts/cli/test_interfaces_dummy.py
index b19ab02dd..c011a411b 100755
--- a/smoketest/scripts/cli/test_interfaces_dummy.py
+++ b/smoketest/scripts/cli/test_interfaces_dummy.py
@@ -24,6 +24,7 @@ class DummyInterfaceTest(BasicInterfaceTest.TestCase):
     def setUpClass(cls):
         cls._base_path = ['interfaces', 'dummy']
         cls._interfaces = ['dum435', 'dum8677', 'dum0931', 'dum089']
+        cls._mirror_interfaces = ['eth0']
         # call base-classes classmethod
         super(DummyInterfaceTest, cls).setUpClass()
 
diff --git a/smoketest/scripts/cli/test_interfaces_ethernet.py b/smoketest/scripts/cli/test_interfaces_ethernet.py
index 7c8106521..7e0da2079 100755
--- a/smoketest/scripts/cli/test_interfaces_ethernet.py
+++ b/smoketest/scripts/cli/test_interfaces_ethernet.py
@@ -43,7 +43,6 @@ class EthernetInterfaceTest(BasicInterfaceTest.TestCase):
     @classmethod
     def setUpClass(cls):
         cls._base_path = ['interfaces', 'ethernet']
-        cls._mirror_interfaces = ['dum21354']
 
         # We only test on physical interfaces and not VLAN (sub-)interfaces
         if 'TEST_ETH' in os.environ: