Hi Numan,

thank you for reviewing the patch.
On 11/3/21 1:57 AM, Numan Siddique wrote:
On Wed, Sep 22, 2021 at 1:58 PM <mh...@redhat.com> wrote:
From: Mohammad Heib <mh...@redhat.com>

currently ovn-northd only handle virtual ports with VIP IPv4 and ignores
virtual ports with VIP IPv6.

This patch adds support for virtual ports with VIP IPv6 by adding
lflows to the lsp_in_arp_rsp logical switch pipeline.
Those lflows handle Neighbor Solicitations and Neighbor Advertisement requests
that target the virtual port VIPs and bind the virtual port to the desired VIF.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2003091
Fixes: 054f4c85c413 ("Add a new logical switch port type - 'virtual'")
Signed-off-by: Mohammad Heib <mh...@redhat.com>
Hi Mohammad,

Thanks for the patch.

I didn't do a thorough review.  I have just a few comments.

You need to update the documentation in northd/ovn-northd.8.xml about
the new flows added.
i added some changes <https://patchwork.ozlabs.org/project/ovn/patch/20211103133616.433626-1-mh...@redhat.com/> to the ovn-northd.8.xml but didn't found a way to test my changes or rebuild those docs
so hope my changes will not break the docs.

PSB for a couple of  small nit comments.

Thanks
Numan

---
  northd/northd.c | 80 ++++++++++++++++++++++++++++++++++++++-----------
  tests/ovn.at    | 66 ++++++++++++++++++++++++++++++++--------
  2 files changed, 116 insertions(+), 30 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index cf2467fe1..b934819fc 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -7382,16 +7382,28 @@ build_lswitch_arp_nd_responder_known_ips(struct 
ovn_port *op,
               *  - ARP reply from the virtual ip which belongs to a logical
               *    port of type 'virtual' and bind that port.
               * */
-            ovs_be32 ip;
+            struct in6_addr ip;
+
              const char *virtual_ip = smap_get(&op->nbsp->options,
                                                "virtual-ip");
              const char *virtual_parents = smap_get(&op->nbsp->options,
                                                     "virtual-parents");
-            if (!virtual_ip || !virtual_parents ||
-                !ip_parse(virtual_ip, &ip)) {
+            if (!virtual_ip || !virtual_parents) {
                  return;
              }

+            bool is_ipv4 = strchr(virtual_ip, '.') ? true : false;
+            if (is_ipv4) {
+                ovs_be32 ipv4;
+                if (!ip_parse(virtual_ip, &ipv4)) {
+                     return;
+                }
+            } else{
+                if (!ipv6_parse(virtual_ip, &ip)) {
+                     return;
+                }
+            }
+
              char *tokstr = xstrdup(virtual_parents);
              char *save_ptr = NULL;
              char *vparent;
@@ -7404,13 +7416,31 @@ build_lswitch_arp_nd_responder_known_ips(struct 
ovn_port *op,
                      continue;
                  }

-                ds_clear(match);
-                ds_put_format(match, "inport == \"%s\" && "
-                              "((arp.op == 1 && arp.spa == %s && "
-                              "arp.tpa == %s) || (arp.op == 2 && "
-                              "arp.spa == %s))",
-                              vparent, virtual_ip, virtual_ip,
-                              virtual_ip);
+                if (is_ipv4) {
+                    ds_clear(match);
+                    ds_put_format(match, "inport == \"%s\" && "
+                            "((arp.op == 1 && arp.spa == %s && "
+                            "arp.tpa == %s) || (arp.op == 2 && "
+                            "arp.spa == %s))",
+                            vparent, virtual_ip, virtual_ip,
+                            virtual_ip);
+                } else{
+                    struct ipv6_netaddr na;
+                    /* Find VIP multicast group */
+                    in6_addr_solicited_node(&na.sn_addr, &ip);
+                    inet_ntop(AF_INET6, &na.sn_addr, na.sn_addr_s, sizeof 
na.sn_addr_s);
+
+                    ds_clear(match);
+                    ds_put_format(match, "inport == \"%s\" && "
+                            "((nd_ns && ip6.dst == {%s, %s} && nd.target == %s) 
||"
+                            "(nd_na && nd.target == %s))",
+                            vparent,
+                            virtual_ip,
+                            na.sn_addr_s,
+                            virtual_ip,
+                            virtual_ip);
+                }
+
                  ds_clear(actions);
                  ds_put_format(actions,
                      "bind_vport(%s, inport); "
@@ -11030,17 +11060,29 @@ build_arp_resolve_flows_for_lrouter_port(
           * 00:00:00:00:00:00 and advance to next table so that ARP is
           * resolved by router pipeline using the arp{} action.
           * The MAC_Binding entry for the virtual ip might be invalid. */
-        ovs_be32 ip;

          const char *vip = smap_get(&op->nbsp->options,
                                     "virtual-ip");
          const char *virtual_parents = smap_get(&op->nbsp->options,
                                                 "virtual-parents");
-        if (!vip || !virtual_parents ||
-            !ip_parse(vip, &ip) || !op->sb) {
+
+        if (!vip || !virtual_parents || !op->sb) {
              return;
          }

+        bool is_ipv4 = strchr(vip, '.') ? true : false;
+        if (is_ipv4) {
+            ovs_be32 ipv4;
+            if (!ip_parse(vip, &ipv4)) {
+                 return;
+            }
+        } else{
+            struct in6_addr ipv6;
+            if (!ipv6_parse(vip, &ipv6)) {
+                 return;
+            }
+        }
+
          if (!op->sb->virtual_parent || !op->sb->virtual_parent[0] ||
              !op->sb->chassis) {
              /* The virtual port is not claimed yet. */
@@ -11054,8 +11096,10 @@ build_arp_resolve_flows_for_lrouter_port(
                  if (find_lrp_member_ip(peer, vip)) {
                      ds_clear(match);
                      ds_put_format(match, "outport == %s && "
-                                  REG_NEXT_HOP_IPV4 " == %s",
-                                  peer->json_key, vip);
+                                  "%s == %s",
+                                  peer->json_key,
+                                  is_ipv4? REG_NEXT_HOP_IPV4 : 
REG_NEXT_HOP_IPV6,
Can you please add space before and after the '?' and ':'.


+                                  vip);

                      const char *arp_actions =
                                    "eth.dst = 00:00:00:00:00:00; next;";
@@ -11093,8 +11137,10 @@ build_arp_resolve_flows_for_lrouter_port(

                      ds_clear(match);
                      ds_put_format(match, "outport == %s && "
-                                  REG_NEXT_HOP_IPV4 " == %s",
-                                  peer->json_key, vip);
+                                  "%s == %s",
+                                  peer->json_key,
+                                  is_ipv4? REG_NEXT_HOP_IPV4 : 
REG_NEXT_HOP_IPV6,
Same as above.
Done,
Thank you

Thanks
Numan

+                                  vip);

                      ds_clear(actions);
                      ds_put_format(actions, "eth.dst = %s; next;", ea_s);
diff --git a/tests/ovn.at b/tests/ovn.at
index 172b5c713..e2d4a592d 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -17831,6 +17831,11 @@ AT_SETUP([virtual ports])
  AT_KEYWORDS([virtual ports])
  ovn_start

+send_nd_ns() {
+    local hv=$1 inport=$2 eth_src=$3 eth_dst=$4 ip6_src=$5 ip6_dst=$6 tgt_ip=$7
+    local 
request=${eth_dst}${eth_src}86dd6000000000203aff${ip6_src}${ip6_dst}870005c900000000${tgt_ip}0101${eth_src}
+    as hv$hv ovs-appctl netdev-dummy/receive hv${hv}-vif$inport $request
+}
  send_garp() {
      local hv=$1 inport=$2 eth_src=$3 eth_dst=$4 spa=$5 tpa=$6
      local 
request=${eth_dst}${eth_src}08060001080006040001${eth_src}${spa}${eth_dst}${tpa}
@@ -17899,43 +17904,52 @@ check ovn-nbctl lsp-set-type sw0-vir virtual
  check ovn-nbctl set logical_switch_port sw0-vir options:virtual-ip=10.0.0.10
  check ovn-nbctl set logical_switch_port sw0-vir 
options:virtual-parents=sw0-p1,sw0-p2,sw0-p3

+
+check ovn-nbctl lsp-add sw0 sw0-vir6
+check ovn-nbctl lsp-set-addresses sw0-vir6 "50:54:00:00:00:11 1000::61d1"
+check ovn-nbctl lsp-set-port-security sw0-vir6 "50:54:00:00:00:11 1000::61d1"
+check ovn-nbctl lsp-set-type sw0-vir6 virtual
+check ovn-nbctl set logical_switch_port sw0-vir6 options:virtual-ip=1000::61d1
+check ovn-nbctl set logical_switch_port sw0-vir6 
options:virtual-parents=sw0-p1,sw0-p2,sw0-p3
+
  check ovn-nbctl lsp-add sw0 sw0-p1
-check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3"
-check ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3 
10.0.0.10"
+check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3 1000::3"
+check ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3 10.0.0.10 
1000::3"

  check ovn-nbctl lsp-add sw0 sw0-p2
-check ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:04 10.0.0.4"
-check ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:04 10.0.0.4 
10.0.0.10"
+check ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:04 10.0.0.4 1000::4"
+check ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:04 10.0.0.4 10.0.0.10 
1000::4"

  check ovn-nbctl lsp-add sw0 sw0-p3
-check ovn-nbctl lsp-set-addresses sw0-p3 "50:54:00:00:00:05 10.0.0.5"
-check ovn-nbctl lsp-set-port-security sw0-p3 "50:54:00:00:00:05 10.0.0.5 
10.0.0.10"
+check ovn-nbctl lsp-set-addresses sw0-p3 "50:54:00:00:00:05 10.0.0.5 1000::5"
+check ovn-nbctl lsp-set-port-security sw0-p3 "50:54:00:00:00:05 10.0.0.5 10.0.0.10 
1000::5"

  # Create the second logical switch with one port
  check ovn-nbctl ls-add sw1
  check ovn-nbctl lsp-add sw1 sw1-p1
-check ovn-nbctl lsp-set-addresses sw1-p1 "40:54:00:00:00:03 20.0.0.3"
-check ovn-nbctl lsp-set-port-security sw1-p1 "40:54:00:00:00:03 20.0.0.3"
+check ovn-nbctl lsp-set-addresses sw1-p1 "40:54:00:00:00:03 20.0.0.3 2000::3"
+check ovn-nbctl lsp-set-port-security sw1-p1 "40:54:00:00:00:03 20.0.0.3 
2000::3"

  # Create a logical router and attach both logical switches
  check ovn-nbctl lr-add lr0
-check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24
+check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24 1000::a/64
  check ovn-nbctl lsp-add sw0 sw0-lr0
  check ovn-nbctl lsp-set-type sw0-lr0 router
  check ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:ff:01
  check ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0

-check ovn-nbctl lrp-add lr0 lr0-sw1 00:00:00:00:ff:02 20.0.0.1/24
+check ovn-nbctl lrp-add lr0 lr0-sw1 00:00:00:00:ff:02 20.0.0.1/24 2000::a/64
  check ovn-nbctl lsp-add sw1 sw1-lr0
  check ovn-nbctl lsp-set-type sw1-lr0 router
  check ovn-nbctl lsp-set-addresses sw1-lr0 00:00:00:00:ff:02
  check ovn-nbctl lsp-set-options sw1-lr0 router-port=lr0-sw1

-# Add an ACL that matches on sw0-vir being bound locally.
+# Add an ACL that matches on sw0-vir/sw0-vir6 being bound locally.
  check ovn-nbctl acl-add sw0 to-lport 1000 'is_chassis_resident("sw0-vir") && 
ip' allow
+check ovn-nbctl acl-add sw0 to-lport 1000 'is_chassis_resident("sw0-vir6") && 
ip' allow

  check ovn-nbctl ls-add public
-check ovn-nbctl lrp-add lr0 lr0-public 00:00:20:20:12:13 172.168.0.100/24
+check ovn-nbctl lrp-add lr0 lr0-public 00:00:20:20:12:13 172.168.0.100/24 
2001:db8::1/64
  check ovn-nbctl lsp-add public public-lr0
  check ovn-nbctl lsp-set-type public-lr0 router
  check ovn-nbctl lsp-set-addresses public-lr0 router
@@ -17951,13 +17965,14 @@ check ovn-nbctl lsp-set-options ln-public 
network_name=public
  check ovn-nbctl --wait=hv lrp-set-gateway-chassis lr0-public hv1 20

  check ovn-nbctl lr-nat-add lr0 dnat_and_snat 172.168.0.50 10.0.0.10 sw0-vir 
10:54:00:00:00:10
+check ovn-nbctl lr-nat-add lr0 dnat_and_snat 2001:db8::61d1 1000::61d1 
sw0-vir6 20:01:db:86:d1:10

  OVN_POPULATE_ARP

  wait_for_ports_up
  ovn-nbctl --wait=hv sync

-# Check that logical flows are added for sw0-vir in lsp_in_arp_rsp pipeline
+# Check that logical flows are added for sw0-vir/sw0vir6 in lsp_in_arp_rsp 
pipeline
  # with bind_vport action.

  ovn-sbctl dump-flows sw0 > sw0-flows
@@ -17965,8 +17980,11 @@ AT_CAPTURE_FILE([sw0-flows])

  AT_CHECK([grep ls_in_arp_rsp sw0-flows | grep bind_vport | sed 
's/table=../table=??/' | sort], [0], [dnl
    table=??(ls_in_arp_rsp      ), priority=100  , match=(inport == "sw0-p1" && ((arp.op == 1 && arp.spa 
== 10.0.0.10 && arp.tpa == 10.0.0.10) || (arp.op == 2 && arp.spa == 10.0.0.10))), 
action=(bind_vport("sw0-vir", inport); next;)
+  table=??(ls_in_arp_rsp      ), priority=100  , match=(inport == "sw0-p1" && ((nd_ns && ip6.dst == 
{1000::61d1, ff02::1:ff00:61d1} && nd.target == 1000::61d1) ||(nd_na && nd.target == 1000::61d1))), 
action=(bind_vport("sw0-vir6", inport); next;)
    table=??(ls_in_arp_rsp      ), priority=100  , match=(inport == "sw0-p2" && ((arp.op == 1 && arp.spa 
== 10.0.0.10 && arp.tpa == 10.0.0.10) || (arp.op == 2 && arp.spa == 10.0.0.10))), 
action=(bind_vport("sw0-vir", inport); next;)
+  table=??(ls_in_arp_rsp      ), priority=100  , match=(inport == "sw0-p2" && ((nd_ns && ip6.dst == 
{1000::61d1, ff02::1:ff00:61d1} && nd.target == 1000::61d1) ||(nd_na && nd.target == 1000::61d1))), 
action=(bind_vport("sw0-vir6", inport); next;)
    table=??(ls_in_arp_rsp      ), priority=100  , match=(inport == "sw0-p3" && ((arp.op == 1 && arp.spa 
== 10.0.0.10 && arp.tpa == 10.0.0.10) || (arp.op == 2 && arp.spa == 10.0.0.10))), 
action=(bind_vport("sw0-vir", inport); next;)
+  table=??(ls_in_arp_rsp      ), priority=100  , match=(inport == "sw0-p3" && ((nd_ns && ip6.dst == 
{1000::61d1, ff02::1:ff00:61d1} && nd.target == 1000::61d1) ||(nd_na && nd.target == 1000::61d1))), 
action=(bind_vport("sw0-vir6", inport); next;)
  ])

  ovn-sbctl dump-flows lr0 > lr0-flows
@@ -17978,6 +17996,10 @@ AT_CHECK([grep lr_in_arp_resolve lr0-flows | grep "reg0 == 
10.0.0.10" | sed 's/t
    table=??(lr_in_arp_resolve  ), priority=100  , match=(outport == "lr0-sw0" 
&& reg0 == 10.0.0.10), action=(eth.dst = 00:00:00:00:00:00; next;)
  ])

+AT_CHECK([grep lr_in_arp_resolve lr0-flows | grep "xxreg0 == 1000::61d1" | sed 
's/table=../table=??/'], [0], [dnl
+  table=??(lr_in_arp_resolve  ), priority=100  , match=(outport == "lr0-sw0" 
&& xxreg0 == 1000::61d1), action=(eth.dst = 00:00:00:00:00:00; next;)
+])
+
  hv1_ch_uuid=`ovn-sbctl --bare --columns _uuid find chassis name="hv1"`
  hv2_ch_uuid=`ovn-sbctl --bare --columns _uuid find chassis name="hv2"`

@@ -17986,6 +18008,8 @@ logical_port=sw0-vir) = x], [0], [])

  AT_CHECK([test x$(ovn-sbctl --bare --columns virtual_parent find port_binding 
\
  logical_port=sw0-vir) = x])
+AT_CHECK([test x$(ovn-sbctl --bare --columns virtual_parent find port_binding \
+logical_port=sw0-vir6) = x])

  # Try to bind sw0-vir directly to an OVS port. This should be ignored by
  # ovn-controller.
@@ -18036,11 +18060,22 @@ spa=$(ip_to_hex 10 0 0 10)
  tpa=$(ip_to_hex 10 0 0 10)
  send_garp 1 1 $eth_src $eth_dst $spa $tpa

+eth_dst=3333ff0061d1
+ipv6_src=10000000000000000000000000000003
+ipv6_dst=ff0200000000000000000001ff0061d1
+ipv6_tgt=100000000000000000000000000061d1
+send_nd_ns 1 1 $eth_src $eth_dst $ipv6_src $ipv6_dst $ipv6_tgt
+
  wait_row_count Port_Binding 1 logical_port=sw0-vir chassis=$hv1_ch_uuid
  check_row_count Port_Binding 1 logical_port=sw0-vir virtual_parent=sw0-p1
  wait_for_ports_up sw0-vir
  check ovn-nbctl --wait=hv sync

+wait_row_count Port_Binding 1 logical_port=sw0-vir6 chassis=$hv1_ch_uuid
+check_row_count Port_Binding 1 logical_port=sw0-vir6 virtual_parent=sw0-p1
+wait_for_ports_up sw0-vir6
+check ovn-nbctl --wait=hv sync
+
  # There should be an arp resolve flow to resolve the virtual_ip with the
  # sw0-p1's MAC.
  ovn-sbctl dump-flows lr0 > lr0-flows2
@@ -18049,6 +18084,10 @@ AT_CHECK([grep lr_in_arp_resolve lr0-flows2 | grep "reg0 == 
10.0.0.10" | sed 's/
    table=??(lr_in_arp_resolve  ), priority=100  , match=(outport == "lr0-sw0" 
&& reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:03; next;)
  ])

+AT_CHECK([grep lr_in_arp_resolve lr0-flows2 | grep "xxreg0 == 1000::61d1" | 
sed 's/table=../table=??/'], [0], [dnl
+  table=??(lr_in_arp_resolve  ), priority=100  , match=(outport == "lr0-sw0" 
&& xxreg0 == 1000::61d1), action=(eth.dst = 50:54:00:00:00:03; next;)
+])
+
  # hv1 should add the flow for the ACL with is_chassis_redirect check for 
sw0-vir and
  # arp responder flow in lr0 pipeline.
  check_virtual_offlows_present hv1
@@ -18321,6 +18360,7 @@ wait_row_count nb:Logical_Switch_Port 1 up=false 
name=sw0-vir

  # Clear virtual_ip column of sw0-vir. There should be no bind_vport flows.
  ovn-nbctl --wait=hv remove logical_switch_port sw0-vir options virtual-ip
+ovn-nbctl --wait=hv remove logical_switch_port sw0-vir6 options virtual-ip

  ovn-sbctl dump-flows sw0 > sw0-flows2
  AT_CAPTURE_FILE([sw0-flows2])
--
2.27.0

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to