Thanks Lorenzo

I have one minor comment down below, but it's very small and can be fixed by whoever merges the patch:

Acked-by: Mark Michelson <[email protected]>

On 4/28/23 11:06, Lorenzo Bianconi wrote:
Fix IPv6 support for OVN mirroring configuring properly interface type
in OVS interface table

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2168119
Fixes: ba8aa26e44cb ("OVN Remote Port Mirroring: controller changes to create ovs 
mirrors")
Signed-off-by: Lorenzo Bianconi <[email protected]>
---
Changes since v1:
- fix ovs interface configuration in check_and_update_interface_table()
- cosmetics
---
  controller/mirror.c |  33 ++++++----
  tests/system-ovn.at | 146 ++++++++++++++++++++++++++++++++++++++++++++
  2 files changed, 166 insertions(+), 13 deletions(-)

diff --git a/controller/mirror.c b/controller/mirror.c
index 665736966..0e5885e9b 100644
--- a/controller/mirror.c
+++ b/controller/mirror.c
@@ -22,6 +22,7 @@
/* OVS includes. */
  #include "lib/vswitch-idl.h"
+#include "lib/socket-util.h"
  #include "include/openvswitch/shash.h"
  #include "openvswitch/vlog.h"
@@ -69,6 +70,7 @@ static void set_mirror_iface_options(struct ovsrec_interface *,
  static const struct ovsrec_port *get_iface_port(
      const struct ovsrec_interface *, const struct ovsrec_bridge *);
+char *get_mirror_tunnel_type(const struct sbrec_mirror *); void
  mirror_register_ovs_idl(struct ovsdb_idl *ovs_idl)
@@ -244,24 +246,26 @@ set_mirror_iface_options(struct ovsrec_interface *iface,
      smap_destroy(&options);
  }
+char *
+get_mirror_tunnel_type(const struct sbrec_mirror *sb_mirror)
+{
+    bool is_ipv6 = addr_is_ipv6(sb_mirror->sink);
+
+    return xasprintf(is_ipv6 ? "ip6%s" : "%s", sb_mirror->type);
+}
+
  static void
  check_and_update_interface_table(const struct sbrec_mirror *sb_mirror,
                                   const struct ovsrec_mirror *ovs_mirror)
  {
-    char *type;
-    struct ovsrec_interface *iface =
-                          ovs_mirror->output_port->interfaces[0];
-    struct smap *opts = &iface->options;
-    const char *erspan_ver = smap_get(opts, "erspan_ver");
-    if (erspan_ver) {
-        type = "erspan";
-    } else {
-        type = "gre";
-    }
-    if (strcmp(type, sb_mirror->type)) {
-        ovsrec_interface_set_type(iface, sb_mirror->type);
+    struct ovsrec_interface *iface = ovs_mirror->output_port->interfaces[0];
+    char *type = get_mirror_tunnel_type(sb_mirror);
+
+    if (strcmp(type, iface->type)) {
+        ovsrec_interface_set_type(iface, type);
      }
      set_mirror_iface_options(iface, sb_mirror);
+    free(type);
  }
static void
@@ -327,8 +331,11 @@ create_ovs_mirror(struct ovn_mirror *m, struct 
ovsdb_idl_txn *ovs_idl_txn,
      char *port_name = xasprintf("ovn-%s", m->name);
ovsrec_interface_set_name(iface, port_name);
-    ovsrec_interface_set_type(iface, m->sb_mirror->type);
+
+    char *type = get_mirror_tunnel_type(m->sb_mirror);
+    ovsrec_interface_set_type(iface, type);
      set_mirror_iface_options(iface, m->sb_mirror);
+    free(type);
struct ovsrec_port *port = ovsrec_port_insert(ovs_idl_txn);
      ovsrec_port_set_name(port, port_name);
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index b46f67636..d68fe5fe7 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -10667,3 +10667,149 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port 
patch-.*/d
  /connection dropped.*/d"])
  AT_CLEANUP
  ])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([ovn mirroring])
+AT_KEYWORDS([mirror])
+AT_SKIP_IF([test $HAVE_TCPDUMP = no])
+
+CHECK_CONNTRACK()
+CHECK_CONNTRACK_NAT()
+ovn_start
+OVS_TRAFFIC_VSWITCHD_START()
+ADD_BR([br-int])
+ADD_BR([br-mirror])
+
+# Set external-ids in br-int needed for ovn-controller
+ovs-vsctl \
+        -- set Open_vSwitch . external-ids:system-id=hv1 \
+        -- set Open_vSwitch . 
external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
+        -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
+        -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
+        -- set bridge br-int fail-mode=secure other-config:disable-in-band=true
+
+# Start ovn-controller
+start_daemon ovn-controller
+
+ovs-ofctl add-flow br-mirror action=normal
+
+ovn-nbctl create Logical_Router name=R1 options:chassis=hv1
+
+ovn-nbctl ls-add foo
+ovn-nbctl ls-add bar
+
+# Connect foo to R1
+ovn-nbctl lrp-add R1 foo 00:00:01:01:02:03 192.168.1.1/24 2001::1/64
+ovn-nbctl lsp-add foo rp-foo -- set Logical_Switch_Port rp-foo \
+    type=router options:router-port=foo addresses=\"00:00:01:01:02:03\"
+
+# Connect bar to R1
+ovn-nbctl lrp-add R1 bar 00:00:01:01:02:04 192.168.2.1/24 2002::1/64
+ovn-nbctl lsp-add bar rp-bar -- set Logical_Switch_Port rp-bar \
+    type=router options:router-port=bar addresses=\"00:00:01:01:02:04\"
+
+# Logical port 'foo1' in switch 'foo'.
+ADD_NAMESPACES(foo1)
+ADD_VETH(foo1, foo1, br-int, "2001::2/64", "f0:00:00:01:02:03", \
+         "2001::1", "nodad", "192.168.1.2/24", "192.168.1.1")
+ovn-nbctl lsp-add foo foo1 \
+-- lsp-set-addresses foo1 "f0:00:00:01:02:03 192.168.1.2 2001::2"
+
+# Logical port 'bar1' in switch 'bar'.
+ADD_NAMESPACES(bar1)
+ADD_VETH(bar1, bar1, br-int, "2002::2/64", "f0:00:00:01:02:05", \
+         "2002::1", "nodad", "192.168.2.2/24", "192.168.2.1")
+ovn-nbctl lsp-add bar bar1 \
+-- lsp-set-addresses bar1 "f0:00:00:01:02:05 192.168.2.2 2002::2"
+
+ovn-nbctl mirror-add mirror0 gre 1 to-lport 172.16.0.100
+ovn-nbctl lsp-attach-mirror bar1 mirror0
+
+ADD_NAMESPACES(mirror)
+ADD_VETH(mirror, mirror, br-mirror, "2003::b/64", "f0:00:00:01:07:06", \
+         "2003::1", "nodad", "172.16.0.100/24", "172.16.0.1")
+AT_CHECK([ip addr add 172.16.0.101/24 dev br-mirror])
+AT_CHECK([ip addr add 2003::a/64 dev br-mirror nodad])
+AT_CHECK([ip link set dev br-mirror up])
+
+NS_CHECK_EXEC([mirror], [tcpdump -l -c 3 -neei mirror proto GRE > gre_mirror4.pcap 
2>gre_mirror4_error &])
+OVS_WAIT_UNTIL([grep "listening" gre_mirror4_error])
+
+NS_CHECK_EXEC([foo1], [ping -q -c 3 -i 0.3 -w 2 192.168.2.2 | FORMAT_PING], \
+[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+OVS_WAIT_UNTIL([
+    n_pakets=$(grep "GRE" -c gre_mirror4.pcap)

s/n_pakets/n_packets/

This same misspelling is repeated throughout the test and should be fixed in those instances as well.

+    test "${n_pakets}" = "3"
+])
+
+killall tcpdump
+
+ovn-nbctl mirror-del mirror0
+ovn-nbctl mirror-add mirror1 gre 2 to-lport 2003::b
+ovn-nbctl lsp-attach-mirror bar1 mirror1
+
+NS_CHECK_EXEC([mirror], [tcpdump -l -neei mirror proto GRE > gre_mirror6.pcap 
2>gre_mirror6_error &])
+NS_CHECK_EXEC([bar1], [tcpdump -l -neei bar1 > bar1.pcap 2>bar1 &])
+OVS_WAIT_UNTIL([grep "listening" gre_mirror6_error])
+
+NS_CHECK_EXEC([foo1], [ping6 -q -c 3 -i 0.3 -w 2 2002::2 | FORMAT_PING], \
+[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+
+killall tcpdump
+
+ovn-nbctl mirror-del mirror1
+ovn-nbctl mirror-add mirror2 erspan 3 to-lport 172.16.0.100
+ovn-nbctl lsp-attach-mirror bar1 mirror2
+
+NS_CHECK_EXEC([mirror], [tcpdump -l -c 3 -neei mirror ip[[22:2]]=0x88be > 
erspan_mirror4.pcap 2>erspan_mirror4_error &])
+OVS_WAIT_UNTIL([grep "listening" erspan_mirror4_error])
+
+NS_CHECK_EXEC([foo1], [ping -q -c 3 -i 0.3 -w 2 192.168.2.2 | FORMAT_PING], \
+[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+OVS_WAIT_UNTIL([
+    n_pakets=$(grep "gre-proto-0x88be" -c erspan_mirror4.pcap)
+    test "${n_pakets}" = "3"
+])
+
+killall tcpdump
+
+ovn-nbctl mirror-del mirror2
+ovn-nbctl mirror-add mirror3 erspan 4 to-lport 2003::b
+ovn-nbctl lsp-attach-mirror bar1 mirror3
+
+NS_CHECK_EXEC([mirror], [tcpdump -l -c 3 -neei mirror ip6[[42:2]]=0x88be > 
erspan_mirror6.pcap 2>erspan_mirror6_error &])
+OVS_WAIT_UNTIL([grep "listening" erspan_mirror6_error])
+
+NS_CHECK_EXEC([foo1], [ping6 -q -c 3 -i 0.3 -w 2 2002::2 | FORMAT_PING], \
+[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+OVS_WAIT_UNTIL([
+    n_pakets=$(grep "gre-proto-0x88be" -c erspan_mirror6.pcap)
+    test "${n_pakets}" = "3"
+])
+
+killall tcpdump
+
+OVS_APP_EXIT_AND_WAIT([ovn-controller])
+
+as ovn-sb
+OVS_APP_EXIT_AND_WAIT([ovsdb-server])
+
+as ovn-nb
+OVS_APP_EXIT_AND_WAIT([ovsdb-server])
+
+as northd
+OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE])
+
+as
+OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
+/connection dropped.*/d"])
+AT_CLEANUP
+])

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to