On Thu, Sep 29, 2022 at 7:32 AM Lorenzo Bianconi <
[email protected]> wrote:
>
> Fix regression in ipv6 prefix delegation running in gw router mode
> introduce by the following commit '04292cc2dc2c ("controller: fix
> potential segmentation violation when removing ports")'.
>
Thanks Lorenzo for the fix! Please see some minor comments below:
> Fixes: 04292cc2dc2c ("controller: fix potential segmentation violation
when removing ports")
> Reoported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129244
> Reoported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129247
> Signed-off-by: Lorenzo Bianconi <[email protected]>
> ---
> controller/local_data.c | 9 ++-
> tests/system-ovn.at | 156 +++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 162 insertions(+), 3 deletions(-)
>
> diff --git a/controller/local_data.c b/controller/local_data.c
> index 9eee568d1..54acc41e6 100644
> --- a/controller/local_data.c
> +++ b/controller/local_data.c
> @@ -115,14 +115,19 @@ local_datapath_destroy(struct local_datapath *ld)
> free(ld);
> }
>
> -/* Checks if pb is a patch port and the peer datapath should be added to
local
> - * datapaths. */
> +/* Checks if pb is running on local gw router or pb is a patch port
> + * and the peer datapath should be added to local datapaths. */
> bool
> need_add_patch_peer_to_local(
Could you update the function name to "need_add_peer_to_local", since it is
not only checking for "patch" ports now?
> struct ovsdb_idl_index *sbrec_port_binding_by_name,
> const struct sbrec_port_binding *pb,
> const struct sbrec_chassis *chassis)
> {
> + /* This port is running on local gw router. */
> + if (!strcmp(pb->type, "l3gateway") && pb->chassis == chassis) {
> + return true;
> + }
> +
> /* If it is not a patch port, no peer to add. */
> if (strcmp(pb->type, "patch")) {
> return false;
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index 75f986d4d..4b20e3808 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -5272,7 +5272,7 @@ AT_CLEANUP
> ])
>
> OVN_FOR_EACH_NORTHD([
> -AT_SETUP([IPv6 prefix delegation])
> +AT_SETUP([IPv6 prefix delegation - distributed router with gw port])
These two test cases are almost identical except for just two lines. It
would be better to wrap with a function and be called by the two test cases
providing a parameter "IS_GATEWAY_ROUTER", to avoid maintaining redundant
code.
Thanks,
Han
> AT_SKIP_IF([test $HAVE_DHCPD = no])
> AT_SKIP_IF([test $HAVE_TCPDUMP = no])
> AT_KEYWORDS([ovn-ipv6-prefix_d])
> @@ -5427,6 +5427,160 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d
> AT_CLEANUP
> ])
>
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([IPv6 prefix delegation - gw router])
> +AT_SKIP_IF([test $HAVE_DHCPD = no])
> +AT_SKIP_IF([test $HAVE_TCPDUMP = no])
> +AT_KEYWORDS([ovn-ipv6-prefix_d])
> +
> +ovn_start
> +OVS_TRAFFIC_VSWITCHD_START()
> +
> +ADD_BR([br-int])
> +ADD_BR([br-ext])
> +
> +ovs-ofctl add-flow br-ext action=normal
> +# Set external-ids in br-int needed for ovn-controller
> +ovs-vsctl \
> + -- set Open_vSwitch . external-ids:system-id=hv1 \
> + -- set Open_vSwitch .
external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
> + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
> + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
> + -- set bridge br-int fail-mode=secure
other-config:disable-in-band=true
> +
> +# Start ovn-controller
> +start_daemon ovn-controller
> +
> +ovn-nbctl create Logical_Router name=R1 options:chassis=hv1
> +ovn-nbctl ls-add sw0
> +ovn-nbctl ls-add sw1
> +ovn-nbctl ls-add public
> +
> +ovn-nbctl lrp-add R1 rp-sw0 00:00:01:01:02:03 192.168.1.1/24
> +ovn-nbctl lrp-add R1 rp-sw1 00:00:03:01:02:03 192.168.2.1/24
> +ovn-nbctl lrp-add R1 rp-public 00:00:02:01:02:03 172.16.1.1/24
> +
> +ovn-nbctl lsp-add sw0 sw0-rp -- set Logical_Switch_Port sw0-rp \
> + type=router options:router-port=rp-sw0 \
> + -- lsp-set-addresses sw0-rp router
> +ovn-nbctl lsp-add sw1 sw1-rp -- set Logical_Switch_Port sw1-rp \
> + type=router options:router-port=rp-sw1 \
> + -- lsp-set-addresses sw1-rp router
> +
> +ovn-nbctl lsp-add public public-rp -- set Logical_Switch_Port public-rp \
> + type=router options:router-port=rp-public \
> + -- lsp-set-addresses public-rp router
> +
> +ADD_NAMESPACES(sw01)
> +ADD_VETH(sw01, sw01, br-int, "192.168.1.2/24", "f0:00:00:01:02:03", \
> + "192.168.1.1")
> +ovn-nbctl lsp-add sw0 sw01 \
> + -- lsp-set-addresses sw01 "f0:00:00:01:02:03 192.168.1.2"
> +
> +ADD_NAMESPACES(sw11)
> +ADD_VETH(sw11, sw11, br-int, "192.168.2.2/24", "f0:00:00:02:02:03", \
> + "192.168.2.1")
> +ovn-nbctl lsp-add sw1 sw11 \
> + -- lsp-set-addresses sw11 "f0:00:00:02:02:03 192.168.2.2"
> +
> +ADD_NAMESPACES(server)
> +ADD_VETH(s1, server, br-ext, "2001:1db8:3333::2/64",
"f0:00:00:01:02:05", \
> + "2001:1db8:3333::1")
> +
> +OVS_WAIT_UNTIL([test "$(ip netns exec server ip a | grep
2001:1db8:3333::2 | grep tentative)" = ""])
> +OVS_WAIT_UNTIL([test "$(ip netns exec server ip a | grep fe80 | grep
tentative)" = ""])
> +
> +AT_CHECK([ovs-vsctl set Open_vSwitch .
external-ids:ovn-bridge-mappings=phynet:br-ext])
> +ovn-nbctl lsp-add public public1 \
> + -- lsp-set-addresses public1 unknown \
> + -- lsp-set-type public1 localnet \
> + -- lsp-set-options public1 network_name=phynet
> +
> +ovn-nbctl set logical_router_port rp-public
options:prefix_delegation=true
> +ovn-nbctl set logical_router_port rp-public options:prefix=true
> +ovn-nbctl set logical_router_port rp-sw0 options:prefix=true
> +ovn-nbctl set logical_router_port rp-sw1 options:prefix=true
> +
> +OVN_POPULATE_ARP
> +
> +ovn-nbctl --wait=hv sync
> +
> +cat > /etc/dhcp/dhcpd.conf <<EOF
> +option dhcp-rebinding-time 15;
> +option dhcp-renewal-time 10;
> +option dhcp6.unicast 2001:1db8:3333::1;
> +subnet6 2001:1db8:3333::/64 {
> + prefix6 2001:1db8:3333:100:: 2001:1db8:3333:111:: /96;
> +}
> +EOF
> +rm -f /var/lib/dhcp/dhcpd6.leases
> +touch /var/lib/dhcp/dhcpd6.leases
> +chown root:dhcpd /var/lib/dhcp /var/lib/dhcp/dhcpd6.leases
> +chmod 775 /var/lib/dhcp
> +chmod 664 /var/lib/dhcp/dhcpd6.leases
> +
> +NETNS_DAEMONIZE([server], [dhcpd -6 -f s1 > dhcpd.log 2>&1], [dhcpd.pid])
> +ovn-nbctl --wait=hv sync
> +
> +OVS_WAIT_WHILE([test "$(ovn-nbctl get logical_router_port rp-public
ipv6_prefix | cut -c4-15)" = ""])
> +OVS_WAIT_WHILE([test "$(ovn-nbctl get logical_router_port rp-sw0
ipv6_prefix | cut -c4-15)" = ""])
> +OVS_WAIT_WHILE([test "$(ovn-nbctl get logical_router_port rp-sw1
ipv6_prefix | cut -c4-15)" = ""])
> +
> +AT_CHECK([ovn-nbctl get logical_router_port rp-public ipv6_prefix | cut
-c3-16], [0], [dnl
> +[2001:1db8:3333]
> +])
> +AT_CHECK([ovn-nbctl get logical_router_port rp-sw0 ipv6_prefix | cut
-c3-16], [0], [dnl
> +[2001:1db8:3333]
> +])
> +AT_CHECK([ovn-nbctl get logical_router_port rp-sw1 ipv6_prefix | cut
-c3-16], [0], [dnl
> +[2001:1db8:3333]
> +])
> +
> +prefix=$(ovn-nbctl list logical_router_port rp-public | awk -F/
'/ipv6_prefix/{print substr($1,25,9)}' | sed 's/://g')
> +ovn-nbctl set logical_router_port rp-sw0 options:prefix=false
> +ovn-nbctl set logical_router_port rp-sw1 options:prefix=false
> +
> +# Renew message
> +NS_CHECK_EXEC([server], [tcpdump -c 1 -nni s1 ip6[[48:1]]=0x05 and
ip6[[113:4]]=0x${prefix} > renew.pcap &])
> +# Reply message with Status OK
> +NS_CHECK_EXEC([server], [tcpdump -c 1 -nni s1 ip6[[48:1]]=0x07 and
ip6[[81:4]]=0x${prefix} > reply.pcap &])
> +
> +OVS_WAIT_UNTIL([
> + total_pkts=$(cat renew.pcap | wc -l)
> + test "${total_pkts}" = "1"
> +])
> +
> +OVS_WAIT_UNTIL([
> + total_pkts=$(cat reply.pcap | wc -l)
> + test "${total_pkts}" = "1"
> +])
> +
> +kill $(pidof tcpdump)
> +
> +ovn-nbctl set logical_router_port rp-sw0 options:prefix=false
> +ovn-nbctl clear logical_router_port rp-sw0 ipv6_prefix
> +OVS_WAIT_WHILE([test "$(ovn-nbctl get logical_router_port rp-sw0
ipv6_prefix | cut -c3-16)" = "[2001:1db8:3333]"])
> +AT_CHECK([ovn-nbctl get logical_router_port rp-sw0 ipv6_prefix | cut
-c3-16], [0], [dnl
> +[]
> +])
> +
> +kill $(pidof ovn-controller)
> +
> +as ovn-sb
> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> +
> +as ovn-nb
> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> +
> +as northd
> +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE])
> +
> +as
> +OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d
> +/.*terminating with signal 15.*/d"])
> +AT_CLEANUP
> +])
> +
> # Tests that when an established connection sends TCP reset,
> # the conntrack entry is not in established state.
> OVN_FOR_EACH_NORTHD([
> --
> 2.37.3
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
