On Fri, Apr 28, 2023 at 7:20 AM Lorenzo Bianconi <
[email protected]> wrote:
>
> > On Tue, Apr 25, 2023 at 3:56 PM Lorenzo Bianconi <
> > [email protected]> wrote:
> >
> > > Fix IPv6 support for OVN mirroring configuring properly interface type
> > > in OVS interface table
> > >
> > > Fixes: ba8aa26e44cb ("OVN Remote Port Mirroring: controller changes to
> > > create ovs mirrors")
> > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2168119
> > > Signed-off-by: Lorenzo Bianconi <[email protected]>
> > >
> >
> > Hi Lorenzo,
> >
> > I have a few minor comments, please see below.
>
> Hi Ales,
>
> thx for the review.
>
> >
> >
> > > ---
> > > controller/mirror.c | 39 ++++++++----
> > > tests/system-ovn.at | 146
++++++++++++++++++++++++++++++++++++++++++++
> > > 2 files changed, 173 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/controller/mirror.c b/controller/mirror.c
> > > index 665736966..8553385b2 100644
> > > --- a/controller/mirror.c
> > > +++ b/controller/mirror.c
> > > @@ -69,6 +69,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 *);
> > >
> > > +const char *get_mirror_tunnel_type(const struct sbrec_mirror *);
> > >
> > > void
> > > mirror_register_ovs_idl(struct ovsdb_idl *ovs_idl)
> > > @@ -244,21 +245,33 @@ set_mirror_iface_options(struct ovsrec_interface
> > > *iface,
> > > smap_destroy(&options);
> > > }
> > >
> > > +const char *
> > > +get_mirror_tunnel_type(const struct sbrec_mirror *sb_mirror)
> > > +{
> > > + struct in6_addr remote_addr;
> > > + bool ipv6 = ipv6_parse(sb_mirror->sink, &remote_addr);
> > > +
> > > + if (!strcmp(sb_mirror->type, "erspan")) {
> > > + if (ipv6) {
> > > + return "ip6erspan";
> > > + } else {
> > > + return "erspan";
> > > + }
> > > + } if (ipv6) {
> > > + return "ip6gre";
> > > + }
> > > +
> > > + return "gre";
> > > +}
> > > +
> > >
> >
> > Just a suggestion, what do you think about reducing the function to the
> > following?
> >
> > static 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);
> > }
>
> ack, it is just to avoid freeing the string in the caller, but that's ok.
>
> >
> >
> > > 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)) {
> > > + struct ovsrec_interface *iface =
> > > ovs_mirror->output_port->interfaces[0];
> > > + const char *type = get_mirror_tunnel_type(sb_mirror);
> > > +
> > > + if (strcmp(type, iface->type)) {
> > > ovsrec_interface_set_type(iface, sb_mirror->type);
> > >
> >
> > AFAICT this should set the type to "type" instead of "sb_mirror->type".
>
> ack, right. I will fix it.
>
Good catch! Do we know why the test case didn't catch it? Does this suggest
a fix needed in the test case, too?
Thanks,
Han
> >
> >
> > > }
> > > set_mirror_iface_options(iface, sb_mirror);
> > > @@ -327,7 +340,9 @@ 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);
> > > +
> > > + const char *type = get_mirror_tunnel_type(m->sb_mirror);
> > > + ovsrec_interface_set_type(iface, type);
> > > set_mirror_iface_options(iface, m->sb_mirror);
> > >
> > > struct ovsrec_port *port = ovsrec_port_insert(ovs_idl_txn);
> > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> > > index b46f67636..a3e7ad0aa 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([ovnnat])
> > >
> >
> > This keyword is not right please remove it.
>
> ack, right. I will fix it.
>
> Regards,
> Lorenzo
>
> >
> >
> > > +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)
> > > + 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
> > > +])
> > > --
> > > 2.40.0
> > >
> > > _______________________________________________
> > > dev mailing list
> > > [email protected]
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> > >
> > Thanks,
> > Ales
> >
> > --
> >
> > Ales Musil
> >
> > Senior Software Engineer - OVN Core
> >
> > Red Hat EMEA <https://www.redhat.com>
> >
> > [email protected] IM: amusil
> > <https://red.ht/sig>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
