Re: [ovs-dev] [PATCH ovn v2] northd: Add logical flow to skip GARP with LLA
On Tue, Jun 6, 2023 at 11:37 PM Ihar Hrachyshka wrote:
> On Tue, May 30, 2023 at 6:00 AM Ales Musil wrote:
>
>> Skip GARP packet with link-local address being advertised
>> when "always_learn_from_arp_request=false", this should
>> prevent huge grow of MAC Binding table. To keep the option
>> consistent overwrite the previous MAC with LLA if it was
>> already stored in DB.
>>
>> Signed-off-by: Ales Musil
>> ---
>> v2: Remove leftover from previous tests.
>> ---
>> northd/northd.c | 11 +++
>> tests/ovn.at| 78 -
>> 2 files changed, 63 insertions(+), 26 deletions(-)
>>
>> diff --git a/northd/northd.c b/northd/northd.c
>> index a6eca916b..ff305fc67 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -11951,6 +11951,17 @@ build_neigh_learning_flows_for_lrouter(
>> ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 100, "nd_ns",
>>ds_cstr(actions));
>>
>> +if (!learn_from_arp_request) {
>> +/* Add flow to skip LLA only if we don't know it already. */
>>
>
> Sorry for pedantry, but this "only" made me confused for a bit. With it,
> the comment may be (mis?)read as saying that "if we don't know it already -
> then we skip, OTHERWISE - we don't skip." And while in effect it's true
> (combined with the other flow for nd_na slightly above), the "OTHERWISE"
> part is not implemented by this new flow.
>
> I think I would be slightly less confused if this comment said
>
> "Add flow to skip LLA if we don't know it already." (without `only`).
>
> Again, sorry for pedantry.
>
No worries, it sounds perfectly reasonable.
>
> ---
>
> I also wonder if moving this flow setup closer to the other flow to handle
> nd_na would help with reading the code (to have all the effects related to
> nd_na in a single place).
>
Done in v3.
>
>
>> +ds_clear(actions);
>> +ds_put_format(actions, REGBIT_LOOKUP_NEIGHBOR_RESULT
>> + " = lookup_nd(inport, ip6.src, nd.tll); "
>> + REGBIT_LOOKUP_NEIGHBOR_IP_RESULT
>> + " = lookup_nd_ip(inport, ip6.src); next;");
>> +ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 110,
>> + "nd_na && ip6.src == fe80::/10", ds_cstr(actions));
>> +}
>> +
>> /* For other packet types, we can skip neighbor learning.
>> * So set REGBIT_LOOKUP_NEIGHBOR_RESULT to 1. */
>> ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 0, "1",
>> diff --git a/tests/ovn.at b/tests/ovn.at
>> index 6f9fbbfd2..7a2658f40 100644
>> --- a/tests/ovn.at
>> +++ b/tests/ovn.at
>> @@ -5073,6 +5073,7 @@ AT_CLEANUP
>>
>> OVN_FOR_EACH_NORTHD([
>> AT_SETUP([IP relocation using GARP request])
>> +AT_SKIP_IF([test $HAVE_SCAPY = no])
>>
>
> Thank you!!
>
>
>> ovn_start
>>
>> # Logical network:
>> @@ -5172,7 +5173,9 @@ done
>> test_ip() {
>> # This packet has bad checksums but logical L3 routing doesn't check.
>> local inport=$1 src_mac=$2 dst_mac=$3 src_ip=$4 dst_ip=$5
>> -local
>> packet=${dst_mac}${src_mac}0800451c4011${src_ip}${dst_ip}00350008
>> +local packet=$(fmt_pkt "Ether(dst='${dst_mac}', src='${src_mac}')/ \
>> +IP(dst='${dst_ip}', src='${src_ip}')/ \
>> +UDP(sport=53, dport=4369)")
>> shift; shift; shift; shift; shift
>> hv=hv`vif_to_hv $inport`
>> as $hv ovs-appctl netdev-dummy/receive vif$inport $packet
>> @@ -5187,7 +5190,9 @@ test_ip() {
>> # Routing decrements TTL and updates source and dest MAC
>> # (and checksum).
>> out_lrp=`vif_to_lrp $outport`
>> -echo
>> f0${outport}ff0${out_lrp}0800451c"3f1101"00${src_ip}${dst_ip}00350008
>> +echo $(fmt_pkt "Ether(dst='f0:00:00:00:00:${outport}',
>> src='00:00:00:00:ff:${out_lrp}')/ \
>> +IP(src='${src_ip}', dst='${dst_ip}',
>> ttl=63)/ \
>> +UDP(sport=53, dport=4369)")
>> fi >> $outport.expected
>> done
>> }
>> @@ -5203,8 +5208,10 @@ test_ip() {
>> # SHA and REPLY_HA are each 12 hex digits.
>> # SPA and TPA are each 8 hex digits.
>> test_arp() {
>> -local inport=$1 sha=$2 spa=$3 tpa=$4 reply_ha=$5
>> -local
>> request=${sha}08060001080006040001${sha}${spa}${tpa}
>> +local inport=$1 sha=$2 spa=$3 tpa=$3
>> +local request=$(fmt_pkt "Ether(dst='ff:ff:ff:ff:ff:ff',
>> src='${sha}')/ \
>> + ARP(hwsrc='${sha}',
>> hwdst='ff:ff:ff:ff:ff:ff', psrc='${spa}', pdst='${tpa}')")
>> +
>> hv=hv`vif_to_hv $inport`
>> as $hv ovs-appctl netdev-dummy/receive vif$inport $request
>>
>> @@ -5217,53 +5224,72 @@ test_arp() {
>> echo $request >> $i$j$k.expected
>> fi
>> done
>> +}
>>
>> -# Expect to receive the reply, if any.
>> -if test X$reply_ha != X; then
>> -
Re: [ovs-dev] [PATCH ovn v2] northd: Add logical flow to skip GARP with LLA
On Tue, May 30, 2023 at 6:00 AM Ales Musil wrote:
> Skip GARP packet with link-local address being advertised
> when "always_learn_from_arp_request=false", this should
> prevent huge grow of MAC Binding table. To keep the option
> consistent overwrite the previous MAC with LLA if it was
> already stored in DB.
>
> Signed-off-by: Ales Musil
> ---
> v2: Remove leftover from previous tests.
> ---
> northd/northd.c | 11 +++
> tests/ovn.at| 78 -
> 2 files changed, 63 insertions(+), 26 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index a6eca916b..ff305fc67 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -11951,6 +11951,17 @@ build_neigh_learning_flows_for_lrouter(
> ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 100, "nd_ns",
>ds_cstr(actions));
>
> +if (!learn_from_arp_request) {
> +/* Add flow to skip LLA only if we don't know it already. */
>
Sorry for pedantry, but this "only" made me confused for a bit. With it,
the comment may be (mis?)read as saying that "if we don't know it already -
then we skip, OTHERWISE - we don't skip." And while in effect it's true
(combined with the other flow for nd_na slightly above), the "OTHERWISE"
part is not implemented by this new flow.
I think I would be slightly less confused if this comment said
"Add flow to skip LLA if we don't know it already." (without `only`).
Again, sorry for pedantry.
---
I also wonder if moving this flow setup closer to the other flow to handle
nd_na would help with reading the code (to have all the effects related to
nd_na in a single place).
> +ds_clear(actions);
> +ds_put_format(actions, REGBIT_LOOKUP_NEIGHBOR_RESULT
> + " = lookup_nd(inport, ip6.src, nd.tll); "
> + REGBIT_LOOKUP_NEIGHBOR_IP_RESULT
> + " = lookup_nd_ip(inport, ip6.src); next;");
> +ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 110,
> + "nd_na && ip6.src == fe80::/10", ds_cstr(actions));
> +}
> +
> /* For other packet types, we can skip neighbor learning.
> * So set REGBIT_LOOKUP_NEIGHBOR_RESULT to 1. */
> ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 0, "1",
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 6f9fbbfd2..7a2658f40 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -5073,6 +5073,7 @@ AT_CLEANUP
>
> OVN_FOR_EACH_NORTHD([
> AT_SETUP([IP relocation using GARP request])
> +AT_SKIP_IF([test $HAVE_SCAPY = no])
>
Thank you!!
> ovn_start
>
> # Logical network:
> @@ -5172,7 +5173,9 @@ done
> test_ip() {
> # This packet has bad checksums but logical L3 routing doesn't check.
> local inport=$1 src_mac=$2 dst_mac=$3 src_ip=$4 dst_ip=$5
> -local
> packet=${dst_mac}${src_mac}0800451c4011${src_ip}${dst_ip}00350008
> +local packet=$(fmt_pkt "Ether(dst='${dst_mac}', src='${src_mac}')/ \
> +IP(dst='${dst_ip}', src='${src_ip}')/ \
> +UDP(sport=53, dport=4369)")
> shift; shift; shift; shift; shift
> hv=hv`vif_to_hv $inport`
> as $hv ovs-appctl netdev-dummy/receive vif$inport $packet
> @@ -5187,7 +5190,9 @@ test_ip() {
> # Routing decrements TTL and updates source and dest MAC
> # (and checksum).
> out_lrp=`vif_to_lrp $outport`
> -echo
> f0${outport}ff0${out_lrp}0800451c"3f1101"00${src_ip}${dst_ip}00350008
> +echo $(fmt_pkt "Ether(dst='f0:00:00:00:00:${outport}',
> src='00:00:00:00:ff:${out_lrp}')/ \
> +IP(src='${src_ip}', dst='${dst_ip}', ttl=63)/
> \
> +UDP(sport=53, dport=4369)")
> fi >> $outport.expected
> done
> }
> @@ -5203,8 +5208,10 @@ test_ip() {
> # SHA and REPLY_HA are each 12 hex digits.
> # SPA and TPA are each 8 hex digits.
> test_arp() {
> -local inport=$1 sha=$2 spa=$3 tpa=$4 reply_ha=$5
> -local
> request=${sha}08060001080006040001${sha}${spa}${tpa}
> +local inport=$1 sha=$2 spa=$3 tpa=$3
> +local request=$(fmt_pkt "Ether(dst='ff:ff:ff:ff:ff:ff',
> src='${sha}')/ \
> + ARP(hwsrc='${sha}',
> hwdst='ff:ff:ff:ff:ff:ff', psrc='${spa}', pdst='${tpa}')")
> +
> hv=hv`vif_to_hv $inport`
> as $hv ovs-appctl netdev-dummy/receive vif$inport $request
>
> @@ -5217,53 +5224,72 @@ test_arp() {
> echo $request >> $i$j$k.expected
> fi
> done
> +}
>
> -# Expect to receive the reply, if any.
> -if test X$reply_ha != X; then
> -lrp=`vif_to_lrp $inport`
> -local
> reply=${sha}ff0${lrp}08060001080006040002${reply_ha}${tpa}${sha}${spa}
> -echo $reply >> $inport.expected
> -fi
> +test_na() {
> +local inport=$1 sha=$2 spa=$3
> +local request=$(fmt_pkt
Re: [ovs-dev] [PATCH ovn v2] northd: Add logical flow to skip GARP with LLA
On Tue, May 30, 2023 at 11:59 AM Ales Musil wrote:
> Skip GARP packet with link-local address being advertised
> when "always_learn_from_arp_request=false", this should
> prevent huge grow of MAC Binding table. To keep the option
> consistent overwrite the previous MAC with LLA if it was
> already stored in DB.
>
> Signed-off-by: Ales Musil
> ---
> v2: Remove leftover from previous tests.
> ---
> northd/northd.c | 11 +++
> tests/ovn.at| 78 -
> 2 files changed, 63 insertions(+), 26 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index a6eca916b..ff305fc67 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -11951,6 +11951,17 @@ build_neigh_learning_flows_for_lrouter(
> ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 100, "nd_ns",
>ds_cstr(actions));
>
> +if (!learn_from_arp_request) {
> +/* Add flow to skip LLA only if we don't know it already. */
> +ds_clear(actions);
> +ds_put_format(actions, REGBIT_LOOKUP_NEIGHBOR_RESULT
> + " = lookup_nd(inport, ip6.src, nd.tll); "
> + REGBIT_LOOKUP_NEIGHBOR_IP_RESULT
> + " = lookup_nd_ip(inport, ip6.src); next;");
> +ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 110,
> + "nd_na && ip6.src == fe80::/10", ds_cstr(actions));
> +}
> +
> /* For other packet types, we can skip neighbor learning.
> * So set REGBIT_LOOKUP_NEIGHBOR_RESULT to 1. */
> ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 0, "1",
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 6f9fbbfd2..7a2658f40 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -5073,6 +5073,7 @@ AT_CLEANUP
>
> OVN_FOR_EACH_NORTHD([
> AT_SETUP([IP relocation using GARP request])
> +AT_SKIP_IF([test $HAVE_SCAPY = no])
> ovn_start
>
> # Logical network:
> @@ -5172,7 +5173,9 @@ done
> test_ip() {
> # This packet has bad checksums but logical L3 routing doesn't check.
> local inport=$1 src_mac=$2 dst_mac=$3 src_ip=$4 dst_ip=$5
> -local
> packet=${dst_mac}${src_mac}0800451c4011${src_ip}${dst_ip}00350008
> +local packet=$(fmt_pkt "Ether(dst='${dst_mac}', src='${src_mac}')/ \
> +IP(dst='${dst_ip}', src='${src_ip}')/ \
> +UDP(sport=53, dport=4369)")
> shift; shift; shift; shift; shift
> hv=hv`vif_to_hv $inport`
> as $hv ovs-appctl netdev-dummy/receive vif$inport $packet
> @@ -5187,7 +5190,9 @@ test_ip() {
> # Routing decrements TTL and updates source and dest MAC
> # (and checksum).
> out_lrp=`vif_to_lrp $outport`
> -echo
> f0${outport}ff0${out_lrp}0800451c"3f1101"00${src_ip}${dst_ip}00350008
> +echo $(fmt_pkt "Ether(dst='f0:00:00:00:00:${outport}',
> src='00:00:00:00:ff:${out_lrp}')/ \
> +IP(src='${src_ip}', dst='${dst_ip}', ttl=63)/
> \
> +UDP(sport=53, dport=4369)")
> fi >> $outport.expected
> done
> }
> @@ -5203,8 +5208,10 @@ test_ip() {
> # SHA and REPLY_HA are each 12 hex digits.
> # SPA and TPA are each 8 hex digits.
> test_arp() {
> -local inport=$1 sha=$2 spa=$3 tpa=$4 reply_ha=$5
> -local
> request=${sha}08060001080006040001${sha}${spa}${tpa}
> +local inport=$1 sha=$2 spa=$3 tpa=$3
> +local request=$(fmt_pkt "Ether(dst='ff:ff:ff:ff:ff:ff',
> src='${sha}')/ \
> + ARP(hwsrc='${sha}',
> hwdst='ff:ff:ff:ff:ff:ff', psrc='${spa}', pdst='${tpa}')")
> +
> hv=hv`vif_to_hv $inport`
> as $hv ovs-appctl netdev-dummy/receive vif$inport $request
>
> @@ -5217,53 +5224,72 @@ test_arp() {
> echo $request >> $i$j$k.expected
> fi
> done
> +}
>
> -# Expect to receive the reply, if any.
> -if test X$reply_ha != X; then
> -lrp=`vif_to_lrp $inport`
> -local
> reply=${sha}ff0${lrp}08060001080006040002${reply_ha}${tpa}${sha}${spa}
> -echo $reply >> $inport.expected
> -fi
> +test_na() {
> +local inport=$1 sha=$2 spa=$3
> +local request=$(fmt_pkt "Ether(dst='ff:ff:ff:ff:ff:ff',
> src='${sha}')/ \
> + IPv6(dst='fe80::1', src='${spa}')/ \
> + ICMPv6ND_NA(tgt='${spa}')")
> +
> +hv=hv`vif_to_hv $inport`
> +as $hv ovs-appctl netdev-dummy/receive vif$inport $request
> +
> +# Expect to receive the broadcast ARP on the other logical switch
> ports if
> +# IP address is not configured to the switch patch port.
> +local i=`vif_to_ls $inport`
> +local j
> +for j in 1 2; do
> +if test $i$j != $inport; then
> +echo $request >> $i$j$k.expected
> +fi
> +done
> }
>
> -# lp11 send GARP request to announce ownership of 192.168.1.100.
> +# lp11
