qos: T4284: support mirror and redirect on all interface types

2 files changed, 59 insertions, 52 deletions

diff --git a/python/vyos/configverify.py b/python/vyos/configverify.py
index 7f1258575..df2c5775a 100644
--- a/python/vyos/configverify.py
+++ b/python/vyos/configverify.py
@@ -178,31 +178,26 @@ def verify_eapol(config):
             if 'certificate' not in ca_cert:
                 raise ConfigError('Invalid CA certificate specified for EAPoL')
 
-def verify_mirror(config):
+def verify_mirror_redirect(config):
     """
     Common helper function used by interface implementations to perform
-    recurring validation of mirror interface configuration.
+    recurring validation of mirror and redirect interface configuration via tc(8)
 
     It makes no sense to mirror traffic back at yourself!
     """
+    if {'mirror', 'redirect'} <= set(config):
+        raise ConfigError('Mirror and redirect can not be enabled at the same time!')
+
     if 'mirror' in config:
         for direction, mirror_interface in config['mirror'].items():
             if mirror_interface == config['ifname']:
                 raise ConfigError(f'Can not mirror "{direction}" traffic back ' \
                                    'the originating interface!')
 
-def verify_redirect(config):
-    """
-    Common helper function used by interface implementations to perform
-    recurring validation of the redirect interface configuration.
-
-    It makes no sense to mirror and redirect traffic at the same time!
-    """
-    if {'mirror', 'redirect'} <= set(config):
-        raise ConfigError('Can not do both redirect and mirror')
-
     if dict_search('traffic_policy.in', config) != None:
-        raise ConfigError('Can not use ingress policy and redirect')
+        # XXX: support combination of limiting and redirect/mirror - this is an
+        # artificial limitation
+        raise ConfigError('Can not use ingress policy tigether with mirror or redirect!')
 
 def verify_authentication(config):
     """
@@ -328,7 +323,7 @@ def verify_vlan_config(config):
         verify_dhcpv6(vlan)
         verify_address(vlan)
         verify_vrf(vlan)
-        verify_redirect(vlan)
+        verify_mirror_redirect(vlan)
         verify_mtu_parent(vlan, config)
 
     # 802.1ad (Q-in-Q) VLANs
@@ -337,7 +332,7 @@ def verify_vlan_config(config):
         verify_dhcpv6(s_vlan)
         verify_address(s_vlan)
         verify_vrf(s_vlan)
-        verify_redirect(s_vlan)
+        verify_mirror_redirect(s_vlan)
         verify_mtu_parent(s_vlan, config)
 
         for c_vlan in s_vlan.get('vif_c', {}):
@@ -345,7 +340,7 @@ def verify_vlan_config(config):
             verify_dhcpv6(c_vlan)
             verify_address(c_vlan)
             verify_vrf(c_vlan)
-            verify_redirect(c_vlan)
+            verify_mirror_redirect(c_vlan)
             verify_mtu_parent(c_vlan, config)
             verify_mtu_parent(c_vlan, s_vlan)
 
diff --git a/python/vyos/ifconfig/interface.py b/python/vyos/ifconfig/interface.py
index 585a605e4..76164ca32 100755
--- a/python/vyos/ifconfig/interface.py
+++ b/python/vyos/ifconfig/interface.py
@@ -1294,48 +1294,60 @@ class Interface(Control):
             if os.path.isfile(config_file):
                 os.remove(config_file)
 
-    def set_mirror(self):
+    def set_mirror_redirect(self):
         # Please refer to the document for details
         #   - https://man7.org/linux/man-pages/man8/tc.8.html
         #   - https://man7.org/linux/man-pages/man8/tc-mirred.8.html
         # Depening if we are the source or the target interface of the port
         # mirror we need to setup some variables.
         source_if = self._config['ifname']
-        config = self._config.get('mirror', None)
 
+        mirror_config = None
+        if 'mirror' in self._config:
+            mirror_config = self._config['mirror']
         if 'is_mirror_intf' in self._config:
             source_if = next(iter(self._config['is_mirror_intf']))
-            config = self._config['is_mirror_intf'][source_if].get('mirror', None)
-
-        # Check configuration stored by old perl code before delete T3782/T4056
-        if not 'redirect' in self._config and not 'traffic_policy' in self._config:
-            # Please do not clear the 'set $? = 0 '. It's meant to force a return of 0
-            # Remove existing mirroring rules
-            delete_tc_cmd  = f'tc qdisc del dev {source_if} handle ffff: ingress 2> /dev/null;'
-            delete_tc_cmd += f'tc qdisc del dev {source_if} handle 1: root prio 2> /dev/null;'
-            delete_tc_cmd += 'set $?=0'
-            self._popen(delete_tc_cmd)
-
-        # Bail out early if nothing needs to be configured
-        if not config:
-            return
-
-        for direction, mirror_if in config.items():
-            if mirror_if not in interfaces():
-                continue
-
-            if direction == 'ingress':
-                handle = 'ffff: ingress'
-                parent = 'ffff:'
-            elif direction == 'egress':
-                handle = '1: root prio'
-                parent = '1:'
-
-            # Mirror egress 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 all prio 10 u32 match u32 0 0 flowid 1:1 action mirred egress mirror dev {mirror_if}'
-            self._popen(mirror_cmd)
+            mirror_config = self._config['is_mirror_intf'][source_if].get('mirror', None)
+
+        redirect_config = None
+
+        # clear existing ingess - ignore errors (e.g. "Error: Cannot find specified
+        # qdisc on specified device") - we simply cleanup all stuff here
+        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');
+
+        # Apply interface mirror policy
+        if mirror_config:
+            for direction, target_if in mirror_config.items():
+                if target_if not in interfaces():
+                    continue
+
+                if direction == 'ingress':
+                    handle = 'ffff: ingress'
+                    parent = 'ffff:'
+                elif direction == 'egress':
+                    handle = '1: root prio'
+                    parent = '1:'
+
+                # Mirror egress 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')
+
+        # Apply interface traffic redirection policy
+        elif 'redirect' in self._config:
+            _, err = self._popen(f'tc qdisc add dev {source_if} handle ffff: ingress')
+            if err: print(f'tc qdisc add for redirect failed!')
+
+            target_if = self._config['redirect']
+            _, 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')
 
     def set_xdp(self, state):
         """
@@ -1562,8 +1574,8 @@ class Interface(Control):
         # eXpress Data Path - highly experimental
         self.set_xdp('xdp' in config)
 
-        # configure port mirror
-        self.set_mirror()
+        # configure interface mirror or redirection target
+        self.set_mirror_redirect()
 
         # Enable/Disable of an interface must always be done at the end of the
         # derived class to make use of the ref-counting set_admin_state()
@@ -1723,5 +1735,5 @@ class VLANIf(Interface):
 
         return super().set_admin_state(state)
 
-    def set_mirror(self):
+    def set_mirror_redirect(self):
         return