Re: [ovs-dev] [PATCH ovn 7/7] ovn-northd.c: Support optionally disabling neighbor learning from ARP request/NS.
On Wed, Jul 29, 2020 at 10:42 AM Numan Siddique wrote: > > > On Thu, Jul 23, 2020 at 10:57 AM Han Zhou wrote: > >> Support a new logical router option "always_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. >> >> Reported-by: Girish Moodalbail >> Reported-at: >> https://mail.openvswitch.org/pipermail/ovs-discuss/2020-May/049995.html >> Signed-off-by: Han Zhou >> > > Hi Han, > > I just gave a quick look. I'll review properly tomorrow. > > I just have a few small comments now. I think it's good to add a few tests > in ovn-northd.at > to make sure that ovn-northd programs the flows as expected. > > Hi Numan This patch already updated the existing test case in ovn.at to cover the scenario when always_learn_from_arp_request is set to false (and also flip back and forth to verify the mac binding is updated for existed entries). It verifies the behavior e2e, so I think maybe it is not necessary to add a separate test case just to check the lflows. What do you think? I sent v2 which added a test case for the first patch (it was not covered in the v1). Please take a look: https://patchwork.ozlabs.org/project/openvswitch/list/?series=194194 Thanks, Han > Thanks > Numan > > --- >> northd/ovn-northd.8.xml | 109 >> >> northd/ovn-northd.c | 96 +++--- >> ovn-nb.xml | 27 >> tests/ovn.at| 18 >> 4 files changed, 217 insertions(+), 33 deletions(-) >> >> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml >> index 9f2c70f..30936ab 100644 >> --- a/northd/ovn-northd.8.xml >> +++ b/northd/ovn-northd.8.xml >> @@ -1537,58 +1537,126 @@ output; >> >>For each router port P that owns IP address >> A, >>which belongs to subnet S with prefix length >> L, >> - a priority-100 flow is added which matches >> - inport == P >> - arp.spa == S/L arp.op == >> 1 >> - (ARP request) with the >> - following actions: >> + if the option always_learn_from_arp_request is >> + true for this router, a priority-100 flow is >> added which >> + matches inport == P arp.spa == >> + S/L arp.op == 1 (ARP >> request) >> + with the following actions: >> + >> + >> + >> +reg9[2] = lookup_arp(inport, arp.spa, arp.sha); >> +next; >> + >> + >> + >> + If the option always_learn_from_arp_request is >> + false, the following two flows are added. >> + >> + >> + >> + A priority-110 flow is added which matches inport == >> + P arp.spa == S/L >> + arp.tpa == A arp.op == >> 1 >> + (ARP request) with the following actions: >> >> >> >> reg9[2] = lookup_arp(inport, arp.spa, arp.sha); >> +reg9[3] = 1; >> +next; >> + >> + >> + >> + A priority-100 flow is added which matches inport == >> + P arp.spa == S/L >> + arp.op == 1 (ARP request) with the following >> + actions: >> + >> + >> + >> +reg9[2] = lookup_arp(inport, arp.spa, arp.sha); >> +reg9[3] = lookup_arp_ip(inport, arp.spa); >> next; >> >> >> >>If the logical router port P is a distributed >> gateway >>router port, additional match >> - is_chassis_resident(cr-P) is added so >> that >> - the resident gateway chassis handles the neighbor lookup. >> + is_chassis_resident(cr-P) is added for >> all >> + these flows. >> >> >> >> >> >>A priority-100 flow which matches on ARP reply packets and >> applies >> - the actions: >> + the actions if the option >> always_learn_from_arp_request >> + is true: >> >> >> >> reg9[2] = lookup_arp(inport, arp.spa, arp.sha); >> next; >> >> + >> + >> + If the option always_learn_from_arp_request >> + is false, the above actions will be: >> + >> + >> + >> +reg9[2] = lookup_arp(inport, arp.spa, arp.sha); >> +reg9[3] = 1; >> +next; >> + >> + >> >> >> >> >>A priority-100 flow which matches on IPv6 Neighbor Discovery >> - advertisement packet and applies the actions: >> + advertisement packet and applies the actions if the option >> + always_learn_from_arp_request is >> true: >> >> >> >> reg9[2] = lookup_nd(inport, nd.target,
Re: [ovs-dev] [PATCH ovn 7/7] ovn-northd.c: Support optionally disabling neighbor learning from ARP request/NS.
On Thu, Jul 23, 2020 at 10:57 AM Han Zhou wrote: > Support a new logical router option "always_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. > > Reported-by: Girish Moodalbail > Reported-at: > https://mail.openvswitch.org/pipermail/ovs-discuss/2020-May/049995.html > Signed-off-by: Han Zhou > Hi Han, I just gave a quick look. I'll review properly tomorrow. I just have a few small comments now. I think it's good to add a few tests in ovn-northd.at to make sure that ovn-northd programs the flows as expected. Thanks Numan --- > northd/ovn-northd.8.xml | 109 > > northd/ovn-northd.c | 96 +++--- > ovn-nb.xml | 27 > tests/ovn.at| 18 > 4 files changed, 217 insertions(+), 33 deletions(-) > > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > index 9f2c70f..30936ab 100644 > --- a/northd/ovn-northd.8.xml > +++ b/northd/ovn-northd.8.xml > @@ -1537,58 +1537,126 @@ output; > >For each router port P that owns IP address > A, >which belongs to subnet S with prefix length > L, > - a priority-100 flow is added which matches > - inport == P > - arp.spa == S/L arp.op == > 1 > - (ARP request) with the > - following actions: > + if the option always_learn_from_arp_request is > + true for this router, a priority-100 flow is added > which > + matches inport == P arp.spa == > + S/L arp.op == 1 (ARP > request) > + with the following actions: > + > + > + > +reg9[2] = lookup_arp(inport, arp.spa, arp.sha); > +next; > + > + > + > + If the option always_learn_from_arp_request is > + false, the following two flows are added. > + > + > + > + A priority-110 flow is added which matches inport == > + P arp.spa == S/L > + arp.tpa == A arp.op == 1 > + (ARP request) with the following actions: > > > > reg9[2] = lookup_arp(inport, arp.spa, arp.sha); > +reg9[3] = 1; > +next; > + > + > + > + A priority-100 flow is added which matches inport == > + P arp.spa == S/L > + arp.op == 1 (ARP request) with the following > + actions: > + > + > + > +reg9[2] = lookup_arp(inport, arp.spa, arp.sha); > +reg9[3] = lookup_arp_ip(inport, arp.spa); > next; > > > >If the logical router port P is a distributed gateway >router port, additional match > - is_chassis_resident(cr-P) is added so > that > - the resident gateway chassis handles the neighbor lookup. > + is_chassis_resident(cr-P) is added for > all > + these flows. > > > > > >A priority-100 flow which matches on ARP reply packets and > applies > - the actions: > + the actions if the option > always_learn_from_arp_request > + is true: > > > > reg9[2] = lookup_arp(inport, arp.spa, arp.sha); > next; > > + > + > + If the option always_learn_from_arp_request > + is false, the above actions will be: > + > + > + > +reg9[2] = lookup_arp(inport, arp.spa, arp.sha); > +reg9[3] = 1; > +next; > + > + > > > > >A priority-100 flow which matches on IPv6 Neighbor Discovery > - advertisement packet and applies the actions: > + advertisement packet and applies the actions if the option > + always_learn_from_arp_request is true: > > > > reg9[2] = lookup_nd(inport, nd.target, nd.tll); > next; > > + > + > + If the option always_learn_from_arp_request > + is false, the above actions will be: > + > + > + > +reg9[2] = lookup_nd(inport, nd.target, nd.tll); > +reg9[3] = 1; > +next; > + > > > > >A priority-100 flow which matches on IPv6 Neighbor Discovery > - solicitation packet and applies the actions: > + solicitation packet and applies the actions if the option > + always_learn_from_arp_request is true: > + > + > + > +reg9[2] = lookup_nd(inport, ip6.src, nd.sll); > +next; > + > + > + > + If the option always_learn_from_arp_request > + is false, the above actions will be: > > >
Re: [ovs-dev] [PATCH ovn 7/7] ovn-northd.c: Support optionally disabling neighbor learning from ARP request/NS.
Bleep bloop. Greetings Han Zhou, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Line is 92 characters long (recommended limit is 79) #370 FILE: ovn-nb.xml:1862: Lines checked: 438, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn 7/7] ovn-northd.c: Support optionally disabling neighbor learning from ARP request/NS.
Support a new logical router option "always_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. Reported-by: Girish Moodalbail Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2020-May/049995.html Signed-off-by: Han Zhou --- northd/ovn-northd.8.xml | 109 northd/ovn-northd.c | 96 +++--- ovn-nb.xml | 27 tests/ovn.at| 18 4 files changed, 217 insertions(+), 33 deletions(-) diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml index 9f2c70f..30936ab 100644 --- a/northd/ovn-northd.8.xml +++ b/northd/ovn-northd.8.xml @@ -1537,58 +1537,126 @@ output; For each router port P that owns IP address A, which belongs to subnet S with prefix length L, - a priority-100 flow is added which matches - inport == P - arp.spa == S/L arp.op == 1 - (ARP request) with the - following actions: + if the option always_learn_from_arp_request is + true for this router, a priority-100 flow is added which + matches inport == P arp.spa == + S/L arp.op == 1 (ARP request) + with the following actions: + + + +reg9[2] = lookup_arp(inport, arp.spa, arp.sha); +next; + + + + If the option always_learn_from_arp_request is + false, the following two flows are added. + + + + A priority-110 flow is added which matches inport == + P arp.spa == S/L + arp.tpa == A arp.op == 1 + (ARP request) with the following actions: reg9[2] = lookup_arp(inport, arp.spa, arp.sha); +reg9[3] = 1; +next; + + + + A priority-100 flow is added which matches inport == + P arp.spa == S/L + arp.op == 1 (ARP request) with the following + actions: + + + +reg9[2] = lookup_arp(inport, arp.spa, arp.sha); +reg9[3] = lookup_arp_ip(inport, arp.spa); next; If the logical router port P is a distributed gateway router port, additional match - is_chassis_resident(cr-P) is added so that - the resident gateway chassis handles the neighbor lookup. + is_chassis_resident(cr-P) is added for all + these flows. A priority-100 flow which matches on ARP reply packets and applies - the actions: + the actions if the option always_learn_from_arp_request + is true: reg9[2] = lookup_arp(inport, arp.spa, arp.sha); next; + + + If the option always_learn_from_arp_request + is false, the above actions will be: + + + +reg9[2] = lookup_arp(inport, arp.spa, arp.sha); +reg9[3] = 1; +next; + + A priority-100 flow which matches on IPv6 Neighbor Discovery - advertisement packet and applies the actions: + advertisement packet and applies the actions if the option + always_learn_from_arp_request is true: reg9[2] = lookup_nd(inport, nd.target, nd.tll); next; + + + If the option always_learn_from_arp_request + is false, the above actions will be: + + + +reg9[2] = lookup_nd(inport, nd.target, nd.tll); +reg9[3] = 1; +next; + A priority-100 flow which matches on IPv6 Neighbor Discovery - solicitation packet and applies the actions: + solicitation packet and applies the actions if the option + always_learn_from_arp_request is true: + + + +reg9[2] = lookup_nd(inport, ip6.src, nd.sll); +next; + + + + If the option always_learn_from_arp_request + is false, the above actions will be: reg9[2] = lookup_nd(inport, ip6.src, nd.sll); +reg9[3] = lookup_nd_ip(inport, ip6.src); next; @@ -1604,21 +1672,28 @@ next; This table adds flows to learn the mac bindings from the ARP and - IPv6 Neighbor Solicitation/Advertisement packets if ARP/ND lookup - failed in the previous table. + IPv6 Neighbor Solicitation/Advertisement packets if it is needed + according to the lookup results from the previous stage. reg9[2] will be 1 if the lookup_arp/lookup_nd - in the previous table was successful, or if there
