Merge pull request #5036 from c-po/bond-mirror

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

9 files changed, 48 insertions, 61 deletions

diff --git a/python/vyos/configdict.py b/python/vyos/configdict.py
index e1d05c384..b16c3cf1d 100644
--- a/python/vyos/configdict.py
+++ b/python/vyos/configdict.py
@@ -472,7 +472,7 @@ def get_interface_dict(config, base, ifname='', recursive_defaults=True, with_pk
 
     # Check if QoS policy applied on this interface - See ifconfig.interface.set_mirror_redirect()
     if config.exists(['qos', 'interface', ifname]):
-        dict.update({'traffic_policy': {}})
+        dict.update({'qos': {}})
 
     address = leaf_node_changed(config, base + [ifname, 'address'])
     if address: dict.update({'address_old' : address})
@@ -531,7 +531,7 @@ def get_interface_dict(config, base, ifname='', recursive_defaults=True, with_pk
         dict['vif'][vif].update({'ifname' : f'{ifname}.{vif}'})
 
         if config.exists(['qos', 'interface', f'{ifname}.{vif}']):
-            dict['vif'][vif].update({'traffic_policy': {}})
+            dict['vif'][vif].update({'qos': {}})
 
         if 'deleted' not in dict:
             address = leaf_node_changed(config, base + [ifname, 'vif', vif, 'address'])
@@ -558,7 +558,7 @@ def get_interface_dict(config, base, ifname='', recursive_defaults=True, with_pk
         dict['vif_s'][vif_s].update({'ifname' : f'{ifname}.{vif_s}'})
 
         if config.exists(['qos', 'interface', f'{ifname}.{vif_s}']):
-            dict['vif_s'][vif_s].update({'traffic_policy': {}})
+            dict['vif_s'][vif_s].update({'qos': {}})
 
         if 'deleted' not in dict:
             address = leaf_node_changed(config, base + [ifname, 'vif-s', vif_s, 'address'])
@@ -586,7 +586,7 @@ def get_interface_dict(config, base, ifname='', recursive_defaults=True, with_pk
             dict['vif_s'][vif_s]['vif_c'][vif_c].update({'ifname' : f'{ifname}.{vif_s}.{vif_c}'})
 
             if config.exists(['qos', 'interface', f'{ifname}.{vif_s}.{vif_c}']):
-                dict['vif_s'][vif_s]['vif_c'][vif_c].update({'traffic_policy': {}})
+                dict['vif_s'][vif_s]['vif_c'][vif_c].update({'qos': {}})
 
             if 'deleted' not in dict:
                 address = leaf_node_changed(config, base + [ifname, 'vif-s', vif_s, 'vif-c', vif_c, 'address'])
diff --git a/python/vyos/configverify.py b/python/vyos/configverify.py
index 995c4ee83..177c4f9b0 100644
--- a/python/vyos/configverify.py
+++ b/python/vyos/configverify.py
@@ -197,10 +197,10 @@ def verify_mirror_redirect(config):
             raise ConfigError(f'Requested redirect interface "{redirect_ifname}" '\
                                'does not exist!')
 
-    if ('mirror' in config or 'redirect' in config) and dict_search('traffic_policy.in', config) is not None:
+    if 'qos' in config and ('mirror' in config or 'redirect' in config):
         # XXX: support combination of limiting and redirect/mirror - this is an
         # artificial limitation
-        raise ConfigError('Can not use ingress policy together with mirror or redirect!')
+        raise ConfigError('Can not use QoS together with mirror/redirect!')
 
 def verify_authentication(config):
     """
diff --git a/python/vyos/ifconfig/interface.py b/python/vyos/ifconfig/interface.py
index f03d0cb68..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:
@@ -1630,9 +1630,9 @@ 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 'traffic_policy' 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');
+        if not 'qos' in self.config:
+            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/python/vyos/utils/network.py b/python/vyos/utils/network.py
index 54dc0b2d9..217428a21 100644
--- a/python/vyos/utils/network.py
+++ b/python/vyos/utils/network.py
@@ -14,6 +14,8 @@
 # License along with this library.  If not, see <http://www.gnu.org/licenses/>.
 
 import hashlib
+
+from json import loads
 from socket import AF_INET
 from socket import AF_INET6
 from vyos.utils.process import cmd
@@ -107,8 +109,6 @@ def gen_mac(name: str, addr: str, ident: str) -> str:
     return ":".join(f"{x:02x}" for x in b)
 
 def get_netns_all() -> list:
-    from json import loads
-    from vyos.utils.process import cmd
     tmp = loads(cmd('ip --json netns ls'))
     return [ netns['name'] for netns in tmp ]
 
@@ -118,14 +118,12 @@ def get_vrf_members(vrf: str) -> list:
     :param vrf: str
     :return: list
     """
-    import json
-    from vyos.utils.process import cmd
     interfaces = []
     try:
         if not interface_exists(vrf):
             raise ValueError(f'VRF "{vrf}" does not exist!')
         output = cmd(f'ip --json --brief link show vrf {vrf}')
-        answer = json.loads(output)
+        answer = loads(output)
         for data in answer:
             if 'ifname' in data:
                 # Skip PIM interfaces which appears in VRF
@@ -166,8 +164,6 @@ def get_interface_config(interface):
     """
     if not interface_exists(interface):
         return None
-    from json import loads
-    from vyos.utils.process import cmd
     tmp = loads(cmd(f'ip --detail --json link show dev {interface}'))[0]
     return tmp
 
@@ -177,8 +173,6 @@ def get_interface_address(interface):
     """
     if not interface_exists(interface):
         return None
-    from json import loads
-    from vyos.utils.process import cmd
     tmp = loads(cmd(f'ip --detail --json addr show dev {interface}'))[0]
     return tmp
 
@@ -186,9 +180,6 @@ def get_interface_namespace(interface: str):
     """
        Returns wich netns the interface belongs to
     """
-    from json import loads
-    from vyos.utils.process import cmd
-
     # Bail out early if netns does not exist
     tmp = cmd(f'ip --json netns ls')
     if not tmp: return None
@@ -216,14 +207,13 @@ def is_ipv6_tentative(iface: str, ipv6_address: str) -> bool:
     Returns:
         bool: True if the IPv6 address is tentative, False otherwise.
     """
-    import json
     from vyos.utils.process import rc_cmd
 
     rc, out = rc_cmd(f'ip -6 --json address show dev {iface}')
     if rc:
         return False
 
-    data = json.loads(out)
+    data = loads(out)
     for addr_info in data[0]['addr_info']:
         if (
             addr_info.get('local') == ipv6_address and
@@ -235,9 +225,7 @@ def is_ipv6_tentative(iface: str, ipv6_address: str) -> bool:
 def is_wwan_connected(interface):
     """ Determine if a given WWAN interface, e.g. wwan0 is connected to the
     carrier network or not """
-    import json
     from vyos.utils.dict import dict_search
-    from vyos.utils.process import cmd
     from vyos.utils.process import is_systemd_service_active
 
     if not interface.startswith('wwan'):
@@ -251,7 +239,7 @@ def is_wwan_connected(interface):
     modem = interface.lstrip('wwan')
 
     tmp = cmd(f'mmcli --modem {modem} --output-json')
-    tmp = json.loads(tmp)
+    tmp = loads(tmp)
 
     # return True/False if interface is in connected state
     return dict_search('modem.generic.state', tmp) == 'connected'
@@ -260,15 +248,11 @@ def get_bridge_fdb(interface):
     """ Returns the forwarding database entries for a given interface """
     if not interface_exists(interface):
         return None
-    from json import loads
-    from vyos.utils.process import cmd
     tmp = loads(cmd(f'bridge -j fdb show dev {interface}'))
     return tmp
 
 def get_all_vrfs():
     """ Return a dictionary of all system wide known VRF instances """
-    from json import loads
-    from vyos.utils.process import cmd
     tmp = loads(cmd('ip --json vrf list'))
     # Result is of type [{"name":"red","table":1000},{"name":"blue","table":2000}]
     # so we will re-arrange it to a more nicer representation:
@@ -287,7 +271,6 @@ def interface_list() -> list:
     """
     return Section.interfaces()
 
-
 def vrf_list() -> list:
     """
     Get list of VRFs in system
@@ -432,7 +415,6 @@ def is_intf_addr_assigned(ifname: str, addr: str, netns: str=None) -> bool:
     It can check both a single IP address (e.g. 192.0.2.1 or a assigned CIDR
     address 192.0.2.1/24.
     """
-    import json
     import jmespath
 
     from vyos.utils.process import rc_cmd
@@ -441,7 +423,7 @@ def is_intf_addr_assigned(ifname: str, addr: str, netns: str=None) -> bool:
     netns_cmd = f'ip netns exec {netns}' if netns else ''
     rc, out = rc_cmd(f'{netns_cmd} ip --json address show dev {ifname}')
     if rc == 0:
-        json_out = json.loads(out)
+        json_out = loads(out)
         addresses = jmespath.search("[].addr_info[].{family: family, address: local, prefixlen: prefixlen}", json_out)
         for address_info in addresses:
             family = address_info['family']
@@ -471,7 +453,6 @@ def is_wireguard_key_pair(private_key: str, public_key:str) -> bool:
     :return: If public/private keys are keypair returns True else False
     :rtype: bool
     """
-    from vyos.utils.process import cmd
     gen_public_key = cmd('wg pubkey', input=private_key)
     if gen_public_key == public_key:
         return True
@@ -488,8 +469,6 @@ def get_wireguard_peers(ifname: str) -> list:
     """
     if not interface_exists(ifname):
         return []
-
-    from vyos.utils.process import cmd
     peers = cmd(f'wg show {ifname} peers')
     return peers.splitlines()
 
@@ -557,9 +536,6 @@ def is_afi_configured(interface: str, afi):
 
 def get_vxlan_vlan_tunnels(interface: str) -> list:
     """ Return a list of strings with VLAN IDs configured in the Kernel """
-    from json import loads
-    from vyos.utils.process import cmd
-
     if not interface.startswith('vxlan'):
         raise ValueError('Only applicable for VXLAN interfaces!')
 
@@ -600,9 +576,6 @@ def get_vxlan_vlan_tunnels(interface: str) -> list:
 
 def get_vxlan_vni_filter(interface: str) -> list:
     """ Return a list of strings with VNIs configured in the Kernel"""
-    from json import loads
-    from vyos.utils.process import cmd
-
     if not interface.startswith('vxlan'):
         raise ValueError('Only applicable for VXLAN interfaces!')
 
@@ -673,9 +646,8 @@ def get_nft_vrf_zone_mapping() -> dict:
               {'interface': 'eth2', 'vrf_tableid': 1000},
               {'interface': 'blue', 'vrf_tableid': 2000}]
     """
-    from json import loads
     from jmespath import search
-    from vyos.utils.process import cmd
+
     output = []
     tmp = loads(cmd('sudo nft -j list table inet vrf_zones'))
     # {'nftables': [{'metainfo': {'json_schema_version': 1,
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: