Re: [PATCH 1/1] tests: shell: Updated nat_ftp tests

[Florian Westphal <fw@strlen.de>]

Andrii Melnychenko <a.melnychenko@vyos.io> wrote:
> Added DNAT and SNAT only tests.
> There was an issue with DNAT that was not covered by tests.
> DNAT misses setting up the `seqadj`, which leads to FTP failures.
> The fix for DNAT has already been proposed to the kernel.

Thanks, could you please send a v2 that splits the refactoring
from the new test case (i.e. two changes)?

> Acked-by: Florian Westphal <fw@strlen.de>

This should not be here unless you would be re-sending a v2
of a patch that is almost 100% the same as the one I acked before.

And I don't recall acking this change, so please don't add this
yourself.

> +	# flush and add FTP helper
> +	read -r -d '' str <<-EOF
>  	flush ruleset
>  	table ip6 ftp_helper_nat_test {
>  		ct helper ftp-standard {
>  			type "ftp" protocol tcp;
>  		}
> +	EOF
> +	ruleset+=$str$'\n'

I'd suggest to just use multiple nft -f invocations
instead of this.

> +	# add DNAT
> +	if [[ $add_dnat -ne 0 ]]; then
> +		read -r -d '' str <<-EOF
>  		chain PRE-dnat {
>  			type nat hook prerouting priority dstnat; policy accept;
>  			# Dnat the control connection, data connection will be automaticly NATed.
>  			ip6 daddr ${ip_rc} counter ip6 nexthdr tcp tcp dport 2121 counter dnat ip6 to [${ip_sr}]:21
>  		}
> +		EOF
> +		ruleset+=$str$'\n'
> +	fi

Just move this from reload_ruleset to a helper function.

> @@ -111,18 +125,51 @@ reload_ruleset()
>  			ip6 nexthdr tcp ct state established counter accept
>  			ip6 nexthdr tcp ct state related     counter log accept
>  		}
> +	EOF
> +	ruleset+=$str$'\n'
>  
> +	# add SNAT
> +	if [[ $add_snat -ne 0 ]]; then
> +		read -r -d '' str <<-EOF

Same here, just omit this.

> +reload_ruleset()
> +{
> +	reload_ruleset_base 1 1
> +}

Then this would be something like:

reload_ruleset_dnat()
{
	reload_ruleset
	load_dnat
}

reload_ruleset_snat()
{
	reload_ruleset
	load_snat
}

reload_ruleset_allnat
{
	reload_ruleset
	load_snat
	load_dnat
}

(or similar naming).  I find that easier to follow, esp. because this
allows a refactor patch before adding the _snat/_dnat tests.

The reload_ruleset -> reload_ruleset_base rename is also ok if you
prefer that.

> +
>  dd if=/dev/urandom of="$INFILE" bs=4096 count=1 2>/dev/null
>  chmod 755 $INFILE
> +
> +mkdir -p /var/run/vsftpd/empty/
> +cp $INFILE /var/run/vsftpd/empty/

I don't understand this change, how is that related?

> +reload_ruleset_dnat_only
> +
> +ip netns exec $S tcpdump -q --immediate-mode -Ui s_r -w ${PCAP} 2> /dev/null &
> +pid=$!
> +sleep 0.5
> +ip netns exec $C curl --no-progress-meter -P - --connect-timeout 5 ftp://[${ip_rc}]:2121/$(basename $INFILE) -o $OUTFILE
> +assert_pass "curl ftp active mode "

Not a requirement, but there is a lot of repitition
of this sequence now, albeit with small changes.

Perhaps this should be in a reuseable function first
before adding the new test cases to this script.

> +ip netns exec $S tcpdump -q --immediate-mode -Ui s_r -w ${PCAP} 2> /dev/null &
> +pid=$!
> +sleep 0.5
> +ip netns exec $C curl --no-progress-meter -P - --connect-timeout 5 ftp://[${ip_sr}]:21/$(basename $INFILE) -o $OUTFILE
> +assert_pass "curl ftp active mode "
> +
> +cmp "$INFILE" "$OUTFILE"
> +assert_pass "FTP Active mode: in and output files remain the same when FTP traffic passes through NAT."
> +
> +kill $pid; sync

I don't understand this 'sync' (I know its already there is source).
It seems its not needed?