Re: [PATCH v3] selftests/net/openvswitch: add ICMPv6 echo type match test

[Aaron Conole <aconole@redhat.com>]

Minxi Hou <houminxi@gmail.com> writes:

> Register OVS_KEY_ATTR_ICMPV6 in the flow key parser so that
> icmpv6(type=...) can be used in flow specifications. Without this
> registration the parser silently drops the token and the kernel
> rejects the flow with EINVAL because the expected ICMPv6 key
> attribute is missing.
>
> While here, add convert_int() to the ovs_key_ipv6 and ovs_key_icmp
> fields_map entries so that specifying a field value produces the
> correct wildcard mask. The IPv6 flow label uses convert_int(20) to
> produce a 20-bit mask (0x000FFFFF), matching the kernel constraint in
> flow_netlink.c that rejects masks with bits 20-31 set; byte-wide
> fields use convert_int(8). The ipv4 counterpart already does this via
> convert_int(); the ipv6 and icmp classes were simply missing the fifth
> tuple element. Existing callers that pass empty parentheses are
> unaffected because convert_int("") returns (0, 0).
>
> Add test_icmpv6 exercising the ICMPv6 echo flow key. The test uses
> static neighbour entries to bypass NDP, then verifies in three steps:
> install icmpv6(type=128) and icmpv6(type=129) flows and confirm ping
> works, remove the flows and confirm ping fails, reinstall and confirm
> recovery.
>
> Signed-off-by: Minxi Hou <houminxi@gmail.com>
> ---

Same as other, I think this should have had the 'net-next' specifier.

Also, I see that dev@openvswitch.org and Ilya weren't CC'd.  Next
version should do that.

>  .../selftests/net/openvswitch/openvswitch.sh  | 63 +++++++++++++++++++
>  .../selftests/net/openvswitch/ovs-dpctl.py    | 26 +++++---
>  2 files changed, 82 insertions(+), 7 deletions(-)
>
> diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> index 2954245129a2..2de01137bb50 100755
> --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
> +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> @@ -32,6 +32,7 @@ tests="
>  	dec_ttl					ttl: dec_ttl decrements IP TTL
>  	flow_set				flow-set: Flow modify
>  	action_set				set: SET action rewrites fields
> +	icmpv6					icmpv6: ICMPv6 echo type match
>  	psample					psample: Sampling packets with psample"
>  
>  info() {
> @@ -443,6 +444,68 @@ test_action_set() {
>  	return 0
>  }
>  

Please document this test case

> +test_icmpv6() {
> +	sbx_add "test_icmpv6" || return $?
> +	ovs_add_dp "test_icmpv6" icmpv6 || return 1
> +
> +	info "create namespaces"
> +	for ns in client server; do
> +		ovs_add_netns_and_veths "test_icmpv6" "icmpv6" \
> +			"$ns" "${ns:0:1}0" "${ns:0:1}1" || return 1
> +	done
> +

===

> +	ip netns exec client ip addr add fd00::1/64 dev c1 nodad
> +	ip netns exec client ip link set c1 up
> +	ip netns exec server ip addr add fd00::2/64 dev s1 nodad
> +	ip netns exec server ip link set s1 up
> +
> +	local cl_mac sl_mac
> +	cl_mac=$(ip netns exec client \
> +		ip link show c1 | awk '/link\/ether/ {print $2}')
> +	[ -z "$cl_mac" ] && \
> +		{ info "failed to get c1 hwaddr"; return 1; }
> +	sl_mac=$(ip netns exec server \
> +		ip link show s1 | awk '/link\/ether/ {print $2}')
> +	[ -z "$sl_mac" ] && \
> +		{ info "failed to get s1 hwaddr"; return 1; }
> +	ip netns exec client \
> +		ip -6 neigh add fd00::2 lladdr "$sl_mac" dev c1
> +	ip netns exec server \
> +		ip -6 neigh add fd00::1 lladdr "$cl_mac" dev s1

===

Should there be some error detection / bailing here?  I think we should
do the same as in the vlan case and set the unreachability detection to
'permanent' to prevent possible issues with racy neighbor discovery.

> +
> +	ovs_add_flow "test_icmpv6" icmpv6 \
> +	  'in_port(1),eth(),eth_type(0x86dd),ipv6(proto=58),icmpv6(type=128)' \
> +	  '2' || return 1
> +	ovs_add_flow "test_icmpv6" icmpv6 \
> +	  'in_port(2),eth(),eth_type(0x86dd),ipv6(proto=58),icmpv6(type=129)' \
> +	  '1' || return 1
> +
> +	info "verify ICMPv6 echo with type-specific flows"
> +	ovs_sbx "test_icmpv6" ip netns exec client \
> +		ping -6 -c 1 -W 2 fd00::2 || return 1
> +
> +	ovs_del_flows "test_icmpv6" icmpv6
> +
> +	info "verify ping fails without echo flows"
> +	ovs_sbx "test_icmpv6" ip netns exec client \
> +		ping -6 -c 1 -W 2 fd00::2 >/dev/null 2>&1 \
> +		&& { info "FAIL: ping should fail without flows"
> +		     return 1; }
> +
> +	ovs_add_flow "test_icmpv6" icmpv6 \
> +	  'in_port(1),eth(),eth_type(0x86dd),ipv6(proto=58),icmpv6(type=128)' \
> +	  '2' || return 1
> +	ovs_add_flow "test_icmpv6" icmpv6 \
> +	  'in_port(2),eth(),eth_type(0x86dd),ipv6(proto=58),icmpv6(type=129)' \
> +	  '1' || return 1
> +
> +	info "verify connectivity restored"
> +	ovs_sbx "test_icmpv6" ip netns exec client \
> +		ping -6 -c 1 -W 2 fd00::2 || return 1
> +
> +	return 0
> +}
> +
>  # psample test
>  # - use psample to observe packets
>  test_psample() {
> diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> index e1ecfad2c03e..f3edd198223f 100644
> --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> @@ -1255,11 +1255,16 @@ class ovskey(nla):
>                  lambda x: ipaddress.IPv6Address(x).packed if x else 0,
>                  convert_ipv6,
>              ),
> -            ("label", "label", "%d", lambda x: int(x) if x else 0),
> -            ("proto", "proto", "%d", lambda x: int(x) if x else 0),
> -            ("tclass", "tclass", "%d", lambda x: int(x) if x else 0),
> -            ("hlimit", "hlimit", "%d", lambda x: int(x) if x else 0),
> -            ("frag", "frag", "%d", lambda x: int(x) if x else 0),
> +            ("label", "label", "%d", lambda x: int(x) if x else 0,
> +                convert_int(20)),
> +            ("proto", "proto", "%d", lambda x: int(x) if x else 0,
> +                convert_int(8)),
> +            ("tclass", "tclass", "%d", lambda x: int(x) if x else 0,
> +                convert_int(8)),
> +            ("hlimit", "hlimit", "%d", lambda x: int(x) if x else 0,
> +                convert_int(8)),
> +            ("frag", "frag", "%d", lambda x: int(x) if x else 0,
> +                convert_int(8)),
>          )
>  
>          def __init__(
> @@ -1344,8 +1349,10 @@ class ovskey(nla):
>          )
>  
>          fields_map = (
> -            ("type", "type", "%d", lambda x: int(x) if x else 0),
> -            ("code", "code", "%d", lambda x: int(x) if x else 0),
> +            ("type", "type", "%d", lambda x: int(x) if x else 0,
> +                convert_int(8)),
> +            ("code", "code", "%d", lambda x: int(x) if x else 0,
> +                convert_int(8)),
>          )
>  
>          def __init__(
> @@ -1982,6 +1989,11 @@ class ovskey(nla):
>                  "icmp",
>                  ovskey.ovs_key_icmp,
>              ),
> +            (
> +                "OVS_KEY_ATTR_ICMPV6",
> +                "icmpv6",
> +                ovskey.ovs_key_icmpv6,
> +            ),
>              (
>                  "OVS_KEY_ATTR_TCP_FLAGS",
>                  "tcp_flags",
>
> base-commit: cef9d6804030793cf8b8796fd6936197d065dd3e