[ovs-dev] [PATCH ovn v2] ovn-northd: Virtual port add ND/ARP responder flows for IPv6 VIPs.

Mohammad Heib mheib at redhat.com
Wed Nov 3 14:00:11 UTC 2021


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 <mheib at redhat.com> wrote:
>> From: Mohammad Heib <mheib at 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 <mheib at 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-mheib@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
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>


More information about the dev mailing list