Re: [PATCH net-next v7 2/2] selftests: openvswitch: add pop_vlan test

[Aaron Conole <aconole@redhat.com>]

Minxi Hou <houminxi@gmail.com> writes:

> Add test_pop_vlan() to verify OVS kernel datapath pop_vlan action
> correctly strips 802.1Q VLAN tags from frames.
>
> Test structure:
> - Baseline: untagged forwarding validates basic connectivity.
> - Negative: forward without pop_vlan, tagged frame is invisible
>   to ns2 (no VLAN sub-interface), ping fails.
> - Positive: pop_vlan strips tag on forward path, push_vlan
>   restores tag on return path, ping succeeds.
>
> Use static ARP entries to avoid VLAN-tagged ARP complexity.
> Rely on ping success/failure for verification -- no tcpdump or
> pcap files needed.
>
> Signed-off-by: Minxi Hou <houminxi@gmail.com>
> ---
>  .../selftests/net/openvswitch/openvswitch.sh  | 73 +++++++++++++++++++
>  1 file changed, 73 insertions(+)
>
> diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> index b327d3061ed5..6d13ee8c2baf 100755
> --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
> +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> @@ -27,6 +27,7 @@ tests="
>  	upcall_interfaces			ovs: test the upcall interfaces
>  	tunnel_metadata				ovs: test extraction of tunnel metadata
>  	drop_reason				drop: test drop reasons are emitted
> +	pop_vlan				vlan: POP_VLAN action strips tag
>  	psample					psample: Sampling packets with psample"
>  
>  info() {
> @@ -830,6 +831,78 @@ test_tunnel_metadata() {
>  	return 0
>  }
>  
> +test_pop_vlan() {
> +	local sbx="test_pop_vlan"
> +	sbx_add "$sbx" || return $?
> +	ovs_add_dp "$sbx" vlandp || return 1
> +
> +	ovs_add_netns_and_veths "$sbx" vlandp \
> +		ns1 veth1 ns1veth 192.0.2.1/24 || return 1
> +	ovs_add_netns_and_veths "$sbx" vlandp \
> +		ns2 veth2 ns2veth 192.0.2.2/24 || return 1
> +
> +	# Baseline: untagged bidirectional forwarding
> +	ovs_add_flow "$sbx" vlandp \
> +		'in_port(1),eth(),eth_type(0x0806),arp()' '2' || return 1
> +	ovs_add_flow "$sbx" vlandp \
> +		'in_port(2),eth(),eth_type(0x0806),arp()' '1' || return 1
> +	ovs_add_flow "$sbx" vlandp \
> +		'in_port(1),eth(),eth_type(0x0800),ipv4()' '2' || return 1
> +	ovs_add_flow "$sbx" vlandp \
> +		'in_port(2),eth(),eth_type(0x0800),ipv4()' '1' || return 1
> +	ovs_sbx "$sbx" ip netns exec ns1 ping -c 3 -W 2 \
> +		192.0.2.2 || return 1
> +
> +	# VLAN topology: ns1 uses VLAN sub-interface, ns2 is plain
> +	ip -n ns1 link add link ns1veth name ns1veth.10 \
> +		type vlan id 10 || return 1
> +	on_exit "ip -n ns1 link del ns1veth.10 2>/dev/null"
> +	ip -n ns1 addr add 198.51.100.1/24 dev ns1veth.10 || return 1
> +	ip -n ns1 link set ns1veth.10 up || return 1
> +	ip -n ns2 addr add 198.51.100.2/24 dev ns2veth || return 1
> +
> +	ovs_del_flows "$sbx" vlandp
> +
> +	# Static ARP: avoids VLAN-tagged ARP complexity
> +	local ns1veth10mac ns2mac
> +	ns1veth10mac=$(ip -n ns1 link show ns1veth.10 \
> +		| awk '/link\/ether/ {print $2}')

We might add a check here (as a just in case):

  [ -z "$ns1veth10mac" ] && \
    { info "failed to get ns1veth10mac"; return 1; }

Same with the ns2mac.   The commands after will fail anyway (because the
empty string will be an issue for the syntax), so it's more like helping
to debug when things go wrong.

With that:

Reviewed-by: Aaron Conole <aconole@redhat.com>

> +	ns2mac=$(ip -n ns2 link show ns2veth \
> +		| awk '/link\/ether/ {print $2}')
> +	ip -n ns1 neigh replace 198.51.100.2 lladdr "$ns2mac" \
> +		dev ns1veth.10 nud permanent || return 1
> +	ip -n ns2 neigh replace 198.51.100.1 \
> +		lladdr "$ns1veth10mac" \
> +		dev ns2veth nud permanent || return 1
> +
> +	local vlan_match='in_port(1),eth(),eth_type(0x8100),'
> +	vlan_match+='vlan(vid=10),'
> +	vlan_match+='encap(eth_type(0x0800),'
> +	vlan_match+='ipv4(src=198.51.100.1,proto=1),icmp())'
> +
> +	# Negative: forward without pop_vlan -- tagged frame
> +	# is invisible to ns2 (no VLAN sub-interface), ping fails
> +	ovs_add_flow "$sbx" vlandp "$vlan_match" '2' || return 1
> +	ovs_sbx "$sbx" ip netns exec ns1 ping -I ns1veth.10 \
> +		-c 3 -W 1 198.51.100.2 >/dev/null 2>&1 \
> +		&& { info "FAIL: ping should fail without pop_vlan"
> +		     return 1; }
> +
> +	ovs_del_flows "$sbx" vlandp
> +
> +	# Positive: pop_vlan strips tag on forward path,
> +	# push_vlan restores tag on return path -- ping succeeds
> +	ovs_add_flow "$sbx" vlandp \
> +		"$vlan_match" 'pop_vlan,2' || return 1
> +	ovs_add_flow "$sbx" vlandp \
> +		'in_port(2),eth(),eth_type(0x0800),ipv4()' \
> +		'push_vlan(vid=10,pcp=0,tpid=0x8100),1' || return 1
> +	ovs_sbx "$sbx" ip netns exec ns1 ping -I ns1veth.10 \
> +		-c 3 -W 2 198.51.100.2 || return 1
> +
> +	return 0
> +}
> +
>  run_test() {
>  	(
>  	tname="$1"