| Message ID | 20220627095652.1018006-1-amusil@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] northd.c: Add flow to skip put_nd action if ip6.src or nd.sll is 0 | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On 6/27/22 11:56, Ales Musil wrote: > The ip6.src or nd.sll does not have to be always set. > According to rfc4861: > > Source Address > Either an address assigned to the interface from > which this message is sent or (if Duplicate Address > Detection is in progress [ADDRCONF]) the > unspecified address. > > Source link-layer address > The link-layer address for the sender. MUST NOT be > included when the source IP address is the > unspecified address. Otherwise, on link layers > that have addresses this option MUST be included in > multicast solicitations and SHOULD be included in > unicast solicitations. > > Add rule that avoids adding MAC binding is either of those > is 0. This is continuation after discussion during review on > 80187a803 (ovn-northd: Add flow to use eth.src if nd.tll is 0 > in put_nd() action.) > > Signed-off-by: Ales Musil <amusil@redhat.com> > --- Hi Ales, Thanks for the follow up! > northd/northd.c | 3 +++ > northd/ovn-northd.8.xml | 6 ++++++ > tests/ovn-northd.at | 3 +++ > 3 files changed, 12 insertions(+) > > diff --git a/northd/northd.c b/northd/northd.c > index 6997c280c..6634edb0f 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -11022,6 +11022,9 @@ build_neigh_learning_flows_for_lrouter( > ovn_lflow_add(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 100, > ds_cstr(match), "next;"); > > + ovn_lflow_add(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 95, > + "nd_ns && (ip6.src == 0 || nd.sll == 0)", "next;"); > + Nit: I'd move this lower, I think the flows are grouped per IP family right now. > ovn_lflow_metered(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 90, > "arp", "put_arp(inport, arp.spa, arp.sha); next;", > copp_meter_get(COPP_ARP, od->nbr->copp, > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > index 59c584710..5df74a410 100644 > --- a/northd/ovn-northd.8.xml > +++ b/northd/ovn-northd.8.xml > @@ -2332,6 +2332,12 @@ next; > to learn the neighbor. > </li> > > + <li> > + A priority-95 flow with the match <code>nd_ns && > + (ip6.src == 0 || nd.sll == 0)</code> and applies the action > + <code>next;</code> > + </li> > + > <li> > A priority-90 flow with the match <code>arp</code> and > applies the action > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 033b58b8c..fe97bedad 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -6707,6 +6707,7 @@ AT_CHECK([ovn-sbctl dump-flows DR | grep -e lr_in_unsnat -e lr_out_snat -e lr_in > > AT_CLEANUP > > +OVN_FOR_EACH_NORTHD([ This is unrelated. > AT_SETUP([LR NB Static_MAC_Binding table]) > ovn_start > > @@ -6730,6 +6731,7 @@ ovn-nbctl --may-exist static-mac-binding-add lr0-p0 192.168.10.100 00:00:22:33:5 > wait_row_count Static_MAC_Binding 1 logical_port=lr0-p0 ip=192.168.10.100 mac="00\:00\:22\:33\:55\:66" > > AT_CLEANUP > +]) This too. > > OVN_FOR_EACH_NORTHD([ > AT_SETUP([LR neighbor lookup and learning flows]) > @@ -6751,6 +6753,7 @@ AT_CHECK([cat lrflows | grep -e lr_in_lookup_neighbor -e lr_in_learn_neighbor | > table=2 (lr_in_learn_neighbor), priority=90 , match=(nd_na), action=(put_nd(inport, nd.target, nd.tll); next;) > table=2 (lr_in_learn_neighbor), priority=90 , match=(nd_ns), action=(put_nd(inport, ip6.src, nd.sll); next;) > table=2 (lr_in_learn_neighbor), priority=95 , match=(nd_na && nd.tll == 0), action=(put_nd(inport, nd.target, eth.src); next;) > + table=2 (lr_in_learn_neighbor), priority=95 , match=(nd_ns && (ip6.src == 0 || nd.sll == 0)), action=(next;) > ]) > > AT_CLEANUP Thanks, Dumitru
On Tue, Jun 28, 2022 at 4:41 PM Dumitru Ceara <dceara@redhat.com> wrote: > On 6/27/22 11:56, Ales Musil wrote: > > The ip6.src or nd.sll does not have to be always set. > > According to rfc4861: > > > > Source Address > > Either an address assigned to the interface from > > which this message is sent or (if Duplicate Address > > Detection is in progress [ADDRCONF]) the > > unspecified address. > > > > Source link-layer address > > The link-layer address for the sender. MUST NOT be > > included when the source IP address is the > > unspecified address. Otherwise, on link layers > > that have addresses this option MUST be included in > > multicast solicitations and SHOULD be included in > > unicast solicitations. > > > > Add rule that avoids adding MAC binding is either of those > > is 0. This is continuation after discussion during review on > > 80187a803 (ovn-northd: Add flow to use eth.src if nd.tll is 0 > > in put_nd() action.) > > > > Signed-off-by: Ales Musil <amusil@redhat.com> > > --- > > Hi Ales, > > Thanks for the follow up! > > > northd/northd.c | 3 +++ > > northd/ovn-northd.8.xml | 6 ++++++ > > tests/ovn-northd.at | 3 +++ > > 3 files changed, 12 insertions(+) > > > > diff --git a/northd/northd.c b/northd/northd.c > > index 6997c280c..6634edb0f 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -11022,6 +11022,9 @@ build_neigh_learning_flows_for_lrouter( > > ovn_lflow_add(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 100, > > ds_cstr(match), "next;"); > > > > + ovn_lflow_add(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 95, > > + "nd_ns && (ip6.src == 0 || nd.sll == 0)", > "next;"); > > + > > Nit: I'd move this lower, I think the flows are grouped per IP family > right now. > Moved in v2. > > ovn_lflow_metered(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 90, > > "arp", "put_arp(inport, arp.spa, arp.sha); > next;", > > copp_meter_get(COPP_ARP, od->nbr->copp, > > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > > index 59c584710..5df74a410 100644 > > --- a/northd/ovn-northd.8.xml > > +++ b/northd/ovn-northd.8.xml > > @@ -2332,6 +2332,12 @@ next; > > to learn the neighbor. > > </li> > > > > + <li> > > + A priority-95 flow with the match <code>nd_ns && > > + (ip6.src == 0 || nd.sll == 0)</code> and applies the action > > + <code>next;</code> > > + </li> > > + > > <li> > > A priority-90 flow with the match <code>arp</code> and > > applies the action > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index 033b58b8c..fe97bedad 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -6707,6 +6707,7 @@ AT_CHECK([ovn-sbctl dump-flows DR | grep -e > lr_in_unsnat -e lr_out_snat -e lr_in > > > > AT_CLEANUP > > > > +OVN_FOR_EACH_NORTHD([ > > This is unrelated. > I've moved this change to a separate commit. > > > AT_SETUP([LR NB Static_MAC_Binding table]) > > ovn_start > > > > @@ -6730,6 +6731,7 @@ ovn-nbctl --may-exist static-mac-binding-add > lr0-p0 192.168.10.100 00:00:22:33:5 > > wait_row_count Static_MAC_Binding 1 logical_port=lr0-p0 > ip=192.168.10.100 mac="00\:00\:22\:33\:55\:66" > > > > AT_CLEANUP > > +]) > > This too. > > > > > OVN_FOR_EACH_NORTHD([ > > AT_SETUP([LR neighbor lookup and learning flows]) > > @@ -6751,6 +6753,7 @@ AT_CHECK([cat lrflows | grep -e > lr_in_lookup_neighbor -e lr_in_learn_neighbor | > > table=2 (lr_in_learn_neighbor), priority=90 , match=(nd_na), > action=(put_nd(inport, nd.target, nd.tll); next;) > > table=2 (lr_in_learn_neighbor), priority=90 , match=(nd_ns), > action=(put_nd(inport, ip6.src, nd.sll); next;) > > table=2 (lr_in_learn_neighbor), priority=95 , match=(nd_na && > nd.tll == 0), action=(put_nd(inport, nd.target, eth.src); next;) > > + table=2 (lr_in_learn_neighbor), priority=95 , match=(nd_ns && > (ip6.src == 0 || nd.sll == 0)), action=(next;) > > ]) > > > > AT_CLEANUP > > Thanks, > Dumitru > > Thanks, Ales
diff --git a/northd/northd.c b/northd/northd.c index 6997c280c..6634edb0f 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -11022,6 +11022,9 @@ build_neigh_learning_flows_for_lrouter( ovn_lflow_add(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 100, ds_cstr(match), "next;"); + ovn_lflow_add(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 95, + "nd_ns && (ip6.src == 0 || nd.sll == 0)", "next;"); + ovn_lflow_metered(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 90, "arp", "put_arp(inport, arp.spa, arp.sha); next;", copp_meter_get(COPP_ARP, od->nbr->copp, diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml index 59c584710..5df74a410 100644 --- a/northd/ovn-northd.8.xml +++ b/northd/ovn-northd.8.xml @@ -2332,6 +2332,12 @@ next; to learn the neighbor. </li> + <li> + A priority-95 flow with the match <code>nd_ns && + (ip6.src == 0 || nd.sll == 0)</code> and applies the action + <code>next;</code> + </li> + <li> A priority-90 flow with the match <code>arp</code> and applies the action diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 033b58b8c..fe97bedad 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -6707,6 +6707,7 @@ AT_CHECK([ovn-sbctl dump-flows DR | grep -e lr_in_unsnat -e lr_out_snat -e lr_in AT_CLEANUP +OVN_FOR_EACH_NORTHD([ AT_SETUP([LR NB Static_MAC_Binding table]) ovn_start @@ -6730,6 +6731,7 @@ ovn-nbctl --may-exist static-mac-binding-add lr0-p0 192.168.10.100 00:00:22:33:5 wait_row_count Static_MAC_Binding 1 logical_port=lr0-p0 ip=192.168.10.100 mac="00\:00\:22\:33\:55\:66" AT_CLEANUP +]) OVN_FOR_EACH_NORTHD([ AT_SETUP([LR neighbor lookup and learning flows]) @@ -6751,6 +6753,7 @@ AT_CHECK([cat lrflows | grep -e lr_in_lookup_neighbor -e lr_in_learn_neighbor | table=2 (lr_in_learn_neighbor), priority=90 , match=(nd_na), action=(put_nd(inport, nd.target, nd.tll); next;) table=2 (lr_in_learn_neighbor), priority=90 , match=(nd_ns), action=(put_nd(inport, ip6.src, nd.sll); next;) table=2 (lr_in_learn_neighbor), priority=95 , match=(nd_na && nd.tll == 0), action=(put_nd(inport, nd.target, eth.src); next;) + table=2 (lr_in_learn_neighbor), priority=95 , match=(nd_ns && (ip6.src == 0 || nd.sll == 0)), action=(next;) ]) AT_CLEANUP
The ip6.src or nd.sll does not have to be always set. According to rfc4861: Source Address Either an address assigned to the interface from which this message is sent or (if Duplicate Address Detection is in progress [ADDRCONF]) the unspecified address. Source link-layer address The link-layer address for the sender. MUST NOT be included when the source IP address is the unspecified address. Otherwise, on link layers that have addresses this option MUST be included in multicast solicitations and SHOULD be included in unicast solicitations. Add rule that avoids adding MAC binding is either of those is 0. This is continuation after discussion during review on 80187a803 (ovn-northd: Add flow to use eth.src if nd.tll is 0 in put_nd() action.) Signed-off-by: Ales Musil <amusil@redhat.com> --- northd/northd.c | 3 +++ northd/ovn-northd.8.xml | 6 ++++++ tests/ovn-northd.at | 3 +++ 3 files changed, 12 insertions(+)