[ovs-dev] [RFC ovn 6/6] ovn-northd.c: Support optionally disabling neighbor learning from ARP request/NS.

Han Zhou hzhou at ovn.org
Sun Jul 19 22:46:13 UTC 2020


On Thu, Jun 11, 2020 at 5:54 AM Mark Michelson <mmichels at redhat.com> wrote:
>
> On 6/10/20 3:00 PM, Han Zhou wrote:
> > Support a new logical router option "learn_from_arp_request" that
controls
> > behavior when handling ARP requests or IPv4 ND-NS packets.
> >
> > "true" - Always learn the MAC/IP binding and add a new MAC_Binding entry
> > (default behavior)
> >
> > "false" - If there is a MAC_binding for that IP and the MAC is
different, or,
> > if TPA of ARP request belongs to any router port on this router, then
> > update/add that MAC/IP binding. Otherwise, don't update/add entries.
>
> Hi Han,
>
> The "learn_from_arp_requests" option is not documented in this patch.
>
> The option values are misleading. As a user, I would expect that setting
> "learn_from_arp_requests" to false would mean *never* learning from ARP
> requests. Perhaps instead of "true/false" this could be a string such as
> "always/local_only" that better indicates what the option does.
>

Hi Mark,

Sorry for lacking documentation in the RFC.
This options is similar to the linux sysctl option:

arp_accept - BOOLEAN
	Define behavior for gratuitous ARP frames who's IP is not
	already present in the ARP table:
	0 - don't create new entries in the ARP table
	1 - create new entries in the ARP table

	Both replies and requests type gratuitous arp will trigger the
	ARP table to be updated, if this setting is on.

	If the ARP table already contains the IP address of the
	gratuitous arp frame, the arp table will be updated regardless
	if this setting is on or off.

Although the meaning of the new option is slightly different from the linux
arp_accept option, I think boolean is more convenient and I followed the
linux option convention. I am ok to change it to string but it could
introduce typos, and would require validations of the values - I'd avoid
that if possible. I'll document with details in the formal patch and let's
see if it is easy to understand or if you still think it is better to
change to string.

> >
> > Reported-by: Girish Moodalbail <gmoodalbail at nvidia.com>
> > Reported-at:
https://mail.openvswitch.org/pipermail/ovs-discuss/2020-May/049995.html
> > Signed-off-by: Han Zhou <hzhou at ovn.org>
> > ---
> >   northd/ovn-northd.c | 78
++++++++++++++++++++++++++++++++++++++++++++---------
> >   tests/ovn.at        | 18 +++++++++++++
> >   2 files changed, 84 insertions(+), 12 deletions(-)
> >
> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > index 9a4e884..8a1f490 100644
> > --- a/northd/ovn-northd.c
> > +++ b/northd/ovn-northd.c
> > @@ -219,6 +219,7 @@ enum ovn_stage {
> >   /* Register to store the result of check_pkt_larger action. */
> >   #define REGBIT_PKT_LARGER        "reg9[1]"
> >   #define REGBIT_LOOKUP_NEIGHBOR_RESULT "reg9[2]"
> > +#define REGBIT_LOOKUP_NEIGHBOR_IP_RESULT "reg9[3]"
> >
> >   /* Register for ECMP bucket selection. */
> >   #define REG_ECMP_GROUP_ID       "reg8[0..15]"
> > @@ -7964,18 +7965,33 @@ build_lrouter_flows(struct hmap *datapaths,
struct hmap *ports,
> >            * */
> >
> >           /* Flows for LOOKUP_NEIGHBOR. */
> > +        bool learn_from_arp_request = smap_get_bool(&od->nbr->options,
> > +
 "learn_from_arp_request",
> > +                                                    true);
> > +        ds_clear(&actions);
> > +        ds_put_format(&actions, REGBIT_LOOKUP_NEIGHBOR_RESULT
> > +                      " = lookup_arp(inport, arp.spa, arp.sha);
%snext;",
> > +                      learn_from_arp_request? "":
> > +                      REGBIT_LOOKUP_NEIGHBOR_IP_RESULT" = 1; ");
> >           ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 100,
> > -                      "arp.op == 2",
> > -                      REGBIT_LOOKUP_NEIGHBOR_RESULT" = "
> > -                      "lookup_arp(inport, arp.spa, arp.sha); next;");
> > +                      "arp.op == 2", ds_cstr(&actions));
> >
> > +        ds_clear(&actions);
> > +        ds_put_format(&actions, REGBIT_LOOKUP_NEIGHBOR_RESULT
> > +                      " = lookup_nd(inport, nd.target, nd.tll);
%snext;",
> > +                      learn_from_arp_request? "":
>
> Minor nitpick, but coding guidelines suggest putting a space on either
> side of the "?" and ":" operators.

Ack.

>
> > +                      REGBIT_LOOKUP_NEIGHBOR_IP_RESULT" = 1; ");
> >           ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 100,
"nd_na",
> > -                      REGBIT_LOOKUP_NEIGHBOR_RESULT" = "
> > -                      "lookup_nd(inport, nd.target, nd.tll); next;");
> > +                      ds_cstr(&actions));
> >
> > +        ds_clear(&actions);
> > +        ds_put_format(&actions, REGBIT_LOOKUP_NEIGHBOR_RESULT
> > +                      " = lookup_nd(inport, ip6.src, nd.sll); %snext;",
> > +                      learn_from_arp_request? "":
> > +                      REGBIT_LOOKUP_NEIGHBOR_IP_RESULT
> > +                      " = lookup_nd_ip(inport, ip6.src); ");
> >           ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 100,
"nd_ns",
> > -                      REGBIT_LOOKUP_NEIGHBOR_RESULT" = "
> > -                      "lookup_nd(inport, ip6.src, nd.sll); next;");
> > +                      ds_cstr(&actions));
> >
> >           /* For other packet types, we can skip neighbor learning.
> >            * So set REGBIT_LOOKUP_NEIGHBOR_RESULT to 1. */
> > @@ -7984,8 +8000,12 @@ build_lrouter_flows(struct hmap *datapaths,
struct hmap *ports,
> >
> >           /* Flows for LEARN_NEIGHBOR. */
> >           /* Skip Neighbor learning if not required. */
> > +        ds_clear(&match);
> > +        ds_put_format(&match, REGBIT_LOOKUP_NEIGHBOR_RESULT" == 1%s",
> > +                      learn_from_arp_request? "":
> > +                      " || "REGBIT_LOOKUP_NEIGHBOR_IP_RESULT" == 0");
> >           ovn_lflow_add(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 100,
> > -                      REGBIT_LOOKUP_NEIGHBOR_RESULT" == 1", "next;");
> > +                      ds_cstr(&match), "next;");
> >
> >           ovn_lflow_add(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 90,
> >                         "arp", "put_arp(inport, arp.spa, arp.sha);
next;");
> > @@ -8002,8 +8022,38 @@ build_lrouter_flows(struct hmap *datapaths,
struct hmap *ports,
> >               continue;
> >           }
> >
> > +        bool learn_from_arp_request =
smap_get_bool(&op->od->nbr->options,
> > +
 "learn_from_arp_request",
> > +                                                    true);
> > +
> >           /* Check if we need to learn mac-binding from ARP requests. */
> >           for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
> > +            if (!learn_from_arp_request) {
> > +                /* ARP request to this address should always get
learned,
> > +                 * so add a priority-110 flow to set
> > +                 * REGBIT_LOOKUP_NEIGHBOR_IP_RESULT to 1. */
> > +                ds_clear(&match);
> > +                ds_put_format(&match,
> > +                              "inport == %s && arp.spa == %s/%u && "
> > +                              "arp.tpa == %s && arp.op == 1",
> > +                              op->json_key,
> > +                              op->lrp_networks.ipv4_addrs[i].network_s,
> > +                              op->lrp_networks.ipv4_addrs[i].plen,
> > +                              op->lrp_networks.ipv4_addrs[i].addr_s);
> > +                if (op->od->l3dgw_port && op == op->od->l3dgw_port
> > +                    && op->od->l3redirect_port) {
> > +                    ds_put_format(&match, " &&
is_chassis_resident(%s)",
> > +                                  op->od->l3redirect_port->json_key);
> > +                }
> > +                const char *actions_s = REGBIT_LOOKUP_NEIGHBOR_RESULT
> > +                                  " = lookup_arp(inport, arp.spa,
arp.sha); "
> > +                                  REGBIT_LOOKUP_NEIGHBOR_IP_RESULT" =
1;"
> > +                                  " next;";
> > +                ovn_lflow_add_with_hint(lflows, op->od,
> > +                                        S_ROUTER_IN_LOOKUP_NEIGHBOR,
110,
> > +                                        ds_cstr(&match), actions_s,
> > +                                        &op->nbrp->header_);
> > +            }
> >               ds_clear(&match);
> >               ds_put_format(&match,
> >                             "inport == %s && arp.spa == %s/%u && arp.op
== 1",
> > @@ -8015,12 +8065,16 @@ build_lrouter_flows(struct hmap *datapaths,
struct hmap *ports,
> >                   ds_put_format(&match, " && is_chassis_resident(%s)",
> >                                 op->od->l3redirect_port->json_key);
> >               }
> > +            ds_clear(&actions);
> > +            ds_put_format(&actions, REGBIT_LOOKUP_NEIGHBOR_RESULT
> > +                          " = lookup_arp(inport, arp.spa, arp.sha);
%snext;",
> > +                          learn_from_arp_request? "":
> > +                          REGBIT_LOOKUP_NEIGHBOR_IP_RESULT
> > +                          " = lookup_arp_ip(inport, arp.spa); ");
> >               ovn_lflow_add_with_hint(lflows, op->od,
> >                                       S_ROUTER_IN_LOOKUP_NEIGHBOR, 100,
> > -                                    ds_cstr(&match),
> > -                                    REGBIT_LOOKUP_NEIGHBOR_RESULT" = "
> > -                                    "lookup_arp(inport, arp.spa,
arp.sha); "
> > -                                    "next;", &op->nbrp->header_);
> > +                                    ds_cstr(&match), ds_cstr(&actions),
> > +                                    &op->nbrp->header_);
> >           }
> >       }
> >
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 12849b4..c7e1cdc 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -3967,6 +3967,18 @@ ip_to_hex() {
> >   sha=f00000000011
> >   spa=`ip_to_hex 192 168 1 100`
> >   tpa=$spa
> > +
> > +# When learn_from_arp_request=false, the new mac-binding will not be
learned
> > +# through GARP request.
> > +ovn-nbctl --wait=hv set logical_router lr0
options:learn_from_arp_request=false
> > +
> > +test_arp 11 $sha $spa $tpa
> > +sleep 1
> > +AT_CHECK([ovn-sbctl find mac_binding ip="192.168.1.100"], [0], [])
> > +
> > +# When learn_from_arp_request=true, the new mac-binding will be
learned.
> > +ovn-nbctl --wait=hv set logical_router lr0
options:learn_from_arp_request=true
> > +
> >   test_arp 11 $sha $spa $tpa
> >   OVS_WAIT_UNTIL([test `ovn-sbctl find mac_binding ip="192.168.1.100" |
wc -l` -gt 0])
> >   ovn-nbctl --wait=hv sync
> > @@ -3981,6 +3993,12 @@ test_ip 21 $smac $dmac $sip $dip 11
> >
> >   # lp12 send GARP request to announce ownership of 192.168.1.100.
> >
> > +# Even when learn_from_arp_request=false, the existing mac-binding
should be
> > +# updated through GARP request.
> > +ovn-nbctl --wait=hv set logical_router lr0
options:learn_from_arp_request=false
> > +ovn-sbctl lflow-list
> > +as hv2 ovs-ofctl dump-flows br-int
> > +
> >   sha=f00000000012
> >   test_arp 12 $sha $spa $tpa
> >   OVS_WAIT_UNTIL([ovn-sbctl find mac_binding ip="192.168.1.100" | grep
f0:00:00:00:00:12])
> >
>


More information about the dev mailing list