[ovs-dev] pinctrl.c: Send GARP only on chassis atached to l3gw

The GARP was sent even on chassis that were not serving
as l3gw for the specified router. This could lead to race
on the physical network when multiple chassis send the same
ARP but the physical interfaces have different port numbers
on the external bridge.

Add check to filter out port_bindings for l3gw that
are sitting on different chassis.

Signed-off-by: Ales Musil <amusil@redhat.com>
Reported-at: https://bugzilla.redhat.com/2062580
---
 controller/pinctrl.c |   2 +-
 tests/ovn.at         | 103 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 104 insertions(+), 1 deletion(-)

Thanks Ales, looks good to me.

Acked-by: Mark Michelson <mmichels@redhat.com>

On 4/14/22 07:03, Ales Musil wrote:
> The GARP was sent even on chassis that were not serving
> as l3gw for the specified router. This could lead to race
> on the physical network when multiple chassis send the same
> ARP but the physical interfaces have different port numbers
> on the external bridge.
> 
> Add check to filter out port_bindings for l3gw that
> are sitting on different chassis.
> 
> Signed-off-by: Ales Musil <amusil@redhat.com>
> Reported-at: https://bugzilla.redhat.com/2062580
> ---
>   controller/pinctrl.c |   2 +-
>   tests/ovn.at         | 103 +++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 104 insertions(+), 1 deletion(-)
> 
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index 25b37ee88..42d1c2a46 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -5554,7 +5554,7 @@ get_localnet_vifs_l3gwports(
>           sbrec_port_binding_index_set_datapath(target, ld->datapath);
>           SBREC_PORT_BINDING_FOR_EACH_EQUAL (pb, target,
>                                              sbrec_port_binding_by_datapath) {
> -            if (!strcmp(pb->type, "l3gateway")
> +            if ((!strcmp(pb->type, "l3gateway") && pb->chassis == chassis)
>                   || !strcmp(pb->type, "patch")) {
>                   sset_add(local_l3gw_ports, pb->logical_port);
>               }
> diff --git a/tests/ovn.at b/tests/ovn.at
> index f9551b843..d6376efcd 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -8707,6 +8707,109 @@ OVN_CLEANUP([hv1])
>   AT_CLEANUP
>   ])
>   
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([send gratuitous arp for l3gateway only on selected chassis])
> +ovn_start
> +
> +# Create logical switch
> +ovn-nbctl ls-add ls0
> +# Create gateway router
> +ovn-nbctl lr-add lr0
> +# Add router port to gateway router
> +ovn-nbctl lrp-add lr0 lr0-ls0 f0:00:00:00:00:01 192.168.0.1/24
> +ovn-nbctl lsp-add ls0 ls0-lr0 -- set Logical_Switch_Port ls0-lr0 \
> +    type=router options:router-port=lr0-ls0 addresses='"f0:00:00:00:00:01"'
> +
> +# Create a localnet port.
> +ovn-nbctl lsp-add ls0 ln_port
> +ovn-nbctl lsp-set-addresses ln_port unknown
> +ovn-nbctl lsp-set-type ln_port localnet
> +ovn-nbctl --wait=hv lsp-set-options ln_port network_name=physnet1
> +
> +# Prepare packets
> +touch empty_expected
> +echo "fffffffffffff0000000000108060001080006040001f00000000001c0a80001000000000000c0a80001" > arp_expected
> +
> +net_add n1
> +sim_add hv1
> +as hv1
> +ovs-vsctl \
> +    -- add-br br-phys \
> +    -- add-br br-eth0
> +
> +ovn_attach n1 br-phys 192.168.0.10
> +
> +AT_CHECK([ovs-vsctl set Open_vSwitch . external-ids:ovn-bridge-mappings=physnet1:br-eth0])
> +AT_CHECK([ovs-vsctl add-port br-eth0 snoopvif -- set Interface snoopvif options:tx_pcap=hv1/snoopvif-tx.pcap options:rxq_pcap=hv1/snoopvif-rx.pcap])
> +
> +sim_add hv2
> +as hv2
> +ovs-vsctl \
> +    -- add-br br-phys \
> +    -- add-br br-eth0
> +
> +ovn_attach n1 br-phys 192.168.0.20
> +
> +AT_CHECK([ovs-vsctl set Open_vSwitch . external-ids:ovn-bridge-mappings=physnet1:br-eth0])
> +AT_CHECK([ovs-vsctl add-port br-eth0 snoopvif -- set Interface snoopvif options:tx_pcap=hv2/snoopvif-tx.pcap options:rxq_pcap=hv2/snoopvif-rx.pcap])
> +
> +ovn-sbctl dump-flows > sbflows
> +AT_CAPTURE_FILE([sbflows])
> +
> +# Wait until the patch ports are created in hv1 and hv2 to connect br-int to br-eth0
> +AT_CHECK([ovn-nbctl set logical_router lr0 options:chassis=hv1])
> +OVS_WAIT_UNTIL([test 1 = `as hv1 ovs-vsctl show | \
> +grep "Port patch-br-int-to-ln_port" | wc -l`])
> +AT_CHECK([ovn-nbctl set logical_router lr0 options:chassis=hv2])
> +OVS_WAIT_UNTIL([test 1 = `as hv2 ovs-vsctl show | \
> +grep "Port patch-br-int-to-ln_port" | wc -l`])
> +
> +# Temporarily remove lr0 chassis
> +AT_CHECK([ovn-nbctl remove logical_router lr0 options chassis])
> +
> +hv1_uuid=$(ovn-sbctl --bare --columns _uuid list chassis hv1)
> +AT_CHECK([ovn-nbctl set logical_router lr0 options:chassis=hv1])
> +OVS_WAIT_UNTIL([
> +    ls0_lr0=$(ovn-sbctl --bare --columns chassis list port_binding ls0-lr0)
> +    test "$ls0_lr0" = $hv1_uuid
> +])
> +
> +OVN_CHECK_PACKETS([hv1/snoopvif-tx.pcap], [arp_expected])
> +OVN_CHECK_PACKETS([hv2/snoopvif-tx.pcap], [empty_expected])
> +
> +# Temporarily remove lr0 chassis
> +AT_CHECK([ovn-nbctl remove logical_router lr0 options chassis])
> +
> +reset_pcap_file() {
> +    local hv=$1
> +    local iface=$2
> +    local pcap_file=$3
> +    as $hv
> +    ovs-vsctl -- set Interface $iface options:tx_pcap=dummy-tx.pcap \
> +options:rxq_pcap=dummy-rx.pcap
> +    rm -f ${pcap_file}*.pcap
> +    ovs-vsctl -- set Interface $iface options:tx_pcap=${pcap_file}-tx.pcap \
> +options:rxq_pcap=${pcap_file}-rx.pcap
> +}
> +
> +reset_pcap_file hv1 snoopvif hv1/snoopvif
> +reset_pcap_file hv2 snoopvif hv2/snoopvif
> +
> +hv2_uuid=$(ovn-sbctl --bare --columns _uuid list chassis hv2)
> +AT_CHECK([ovn-nbctl set logical_router lr0 options:chassis=hv2])
> +OVS_WAIT_UNTIL([
> +    ls0_lr0=$(ovn-sbctl --bare --columns chassis list port_binding ls0-lr0)
> +    test "$ls0_lr0" = $hv2_uuid
> +])
> +
> +OVN_CHECK_PACKETS([hv2/snoopvif-tx.pcap], [arp_expected])
> +OVN_CHECK_PACKETS([hv1/snoopvif-tx.pcap], [empty_expected])
> +
> +OVN_CLEANUP([hv1],[hv2])
> +
> +AT_CLEANUP
> +])
> +
>   OVN_FOR_EACH_NORTHD([
>   AT_SETUP([send gratuitous arp with nat-addresses router in localnet])
>   ovn_start
>