[ovs-dev] [PATCH v2] ovn: Fix the issue in IPv6 Neigh Solicitation responder for router IPs

[Ben Pfaff]

On Fri, Aug 24, 2018 at 02:11:05PM -0700, Han Zhou wrote:
> On Fri, Aug 24, 2018 at 12:27 PM <nusiddiq at redhat.com> wrote:
> >
> > From: Numan Siddique <nusiddiq at redhat.com>
> >
> > Commit [1] added a new action 'nd_na_router' to set the router bit
> > in the 'flags' field of the Neighbour Adv packet for router IPs.
> > This action was used in the router pipeline. But the logical switch
> > pipeline also adds the Neighbour Adv flows for router IPs but with
> > 'nd_na' action (which the commit [1] didn't handle).
> >
> > This patch fixes this by changing the action to 'nd_na_router' for
> > router IPs.
> >
> > Without this patch, the IPv6 functionality is broken.
> >
> > [1] - "c9756229ed: ovn: Set proper Neighbour Adv flag when replying
> > for NS request for router IP"
> >
> > Signed-off-by: Numan Siddique <nusiddiq at redhat.com>
> > ---
> >  ovn/northd/ovn-northd.8.xml | 24 ++++++++++++++++++++++--
> >  ovn/northd/ovn-northd.c     |  4 +++-
> >  tests/ovn.at                | 15 ++++++++++++++-
> >  3 files changed, 39 insertions(+), 4 deletions(-)
> >
> > v1 -> v2
> > -------
> > Addressed the review comments from Han Zhou by using the nd_na_router
> > action for router IPs in the logical switch pipeline.
> >
> >
> > diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
> > index e5ff2b661..7352c6764 100644
> > --- a/ovn/northd/ovn-northd.8.xml
> > +++ b/ovn/northd/ovn-northd.8.xml
> > @@ -549,8 +549,8 @@ output;
> >          <p>
> >            Priority-50 flows that match IPv6 ND neighbor solicitations to
> >            each known IP address <var>A</var> (and <var>A</var>'s
> > -          solicited node address) of every logical switch port, and
> > -          respond with neighbor advertisements directly with
> > +          solicited node address) of every logical switch port except of
> type
> > +          router, and respond with neighbor advertisements directly with
> >            corresponding Ethernet address <var>E</var>:
> >          </p>
> >
> > @@ -566,6 +566,26 @@ nd_na {
> >  };
> >          </pre>
> >
> > +        <p>
> > +          Priority-50 flows that match IPv6 ND neighbor solicitations to
> > +          each known IP address <var>A</var> (and <var>A</var>'s
> > +          solicited node address) of logical switch port of type router,
> and
> > +          respond with neighbor advertisements directly with
> > +          corresponding Ethernet address <var>E</var>:
> > +        </p>
> > +
> > +        <pre>
> > +nd_na_router {
> > +    eth.src = <var>E</var>;
> > +    ip6.src = <var>A</var>;
> > +    nd.target = <var>A</var>;
> > +    nd.tll = <var>E</var>;
> > +    outport = inport;
> > +    flags.loopback = 1;
> > +    output;
> > +};
> > +        </pre>
> > +
> >          <p>
> >            These flows are omitted for logical ports (other than router
> ports or
> >            <code>localport</code> ports) that are down.
> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > index 1d020a739..72e25181d 100644
> > --- a/ovn/northd/ovn-northd.c
> > +++ b/ovn/northd/ovn-northd.c
> > @@ -4163,7 +4163,7 @@ build_lswitch_flows(struct hmap *datapaths, struct
> hmap *ports,
> >
> >                  ds_clear(&actions);
> >                  ds_put_format(&actions,
> > -                        "nd_na { "
> > +                        "%s { "
> >                          "eth.src = %s; "
> >                          "ip6.src = %s; "
> >                          "nd.target = %s; "
> > @@ -4172,6 +4172,8 @@ build_lswitch_flows(struct hmap *datapaths, struct
> hmap *ports,
> >                          "flags.loopback = 1; "
> >                          "output; "
> >                          "};",
> > +                        !strcmp(op->nbsp->type, "router") ?
> > +                            "nd_na_router" : "nd_na",
> >                          op->lsp_addrs[i].ea_s,
> >                          op->lsp_addrs[i].ipv6_addrs[j].addr_s,
> >                          op->lsp_addrs[i].ipv6_addrs[j].addr_s,
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index c5d054c21..e10a7f9ba 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -9530,7 +9530,7 @@ ovn-nbctl lr-add lr0_ip6
> >  ovn-nbctl lrp-add lr0_ip6 lrp0_ip6 00:00:00:00:af:01
> aef0:0:0:0:0:0:0:0/64
> >  ovn-nbctl lsp-add sw0_ip6 lrp0_ip6-attachment
> >  ovn-nbctl lsp-set-type lrp0_ip6-attachment router
> > -ovn-nbctl lsp-set-addresses lrp0_ip6-attachment 00:00:00:00:af:01
> > +ovn-nbctl lsp-set-addresses lrp0_ip6-attachment router
> >  ovn-nbctl lsp-set-options lrp0_ip6-attachment router-port=lrp0_ip6
> >  ovn-nbctl set logical_router_port lrp0_ip6
> ipv6_ra_configs:address_mode=slaac
> >
> > @@ -9563,6 +9563,19 @@ ovs-vsctl -- add-port br-int hv1-vif1 -- \
> >  ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
> >
> >  OVS_WAIT_UNTIL([test x`ovn-nbctl lsp-get-up sw0_ip6-port1` = xup])
> > +
> > +# There should be 2 Neighbor Advertisement flows for the router port
> > +# aef0:: ip address in logical switch pipeline with action nd_na_router.
> > +AT_CHECK([ovn-sbctl dump-flows sw0_ip6 | grep ls_in_arp_rsp | \
> > +grep "nd_na_router" | wc -l], [0], [2
> > +])
> > +
> > +# There should be 4 Neighbor Advertisement flows with action nd_na_router
> > +# in the router pipeline for the router lr0_ip6.
> > +AT_CHECK([ovn-sbctl dump-flows lr0_ip6 | grep nd_na_router | \
> > +wc -l], [0], [4
> > +])
> > +
> >  cr_uuid=`ovn-sbctl find port_binding logical_port=cr-ip6_public | grep
> _uuid | cut -f2 -d ":"`
> >
> >  # There is only one chassis.
> > --
> > 2.17.1
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 
> Acked-by: Han Zhou <hzhou8 at ebay.com>

Thanks, applied to master and branch-2.10.