Re: [PATCH v2] tests: shell: Add a test case for FTP helper combined with NAT.

[Florian Westphal <fw@strlen.de>]

Yi Chen <yiche@redhat.com> wrote:
> This test verifies functionality of the FTP helper,
> for both passive, active FTP modes,
> and the functionality of the nf_nat_ftp module.

Thanks for this test case.

Some minor comments below.

> diff --git a/tests/shell/features/tcpdump.sh b/tests/shell/features/tcpdump.sh
> new file mode 100755
> index 00000000..70df9f68
> --- /dev/null
> +++ b/tests/shell/features/tcpdump.sh
> @@ -0,0 +1,4 @@
> +#!/bin/sh
> +
> +# check whether tcpdump is installed
> +tcpdump -h >/dev/null 2>&1

Is tcpdump a requirement? AFAICS the dumps are only used
as a debug aid when something goes wrong?

> +INFILE=$(mktemp -p /var/ftp/pub/)

This directory might not be writeable.

Can you use a /tmp/ directory?

I suggest to do:

WORKDIR=$(mktemp -d)
mkdir "$WORKDIR/pub"

... and then place all files there.

> +dd if=/dev/urandom of="$INFILE" bs=4096 count=1 2>/dev/null
> +chmod 755 $INFILE
> +assert_pass "Prepare the file for FTP transmission"

Including this one

... and this config:

> +cat > ./vsftpd.conf <<-EOF
> +anonymous_enable=YES
> +local_enable=YES
> +connect_from_port_20=YES
> +listen=NO
> +listen_ipv6=YES
> +pam_service_name=vsftpd
> +background=YES
> +EOF
> +ip netns exec $S vsftpd ./vsftpd.conf
> +sleep 1
> +ip netns exec $S ss -6ltnp | grep -q '*:21'
> +assert_pass "start vsftpd server"

So no files are created outside of /tmp.

> +# test passive mode
> +reload_ruleset
> +ip netns exec $S tcpdump -q --immediate-mode -Ui s_r -w ${0##*/}.pcap 2> /dev/null & sleep 2
> +ip netns exec $C curl -s --connect-timeout 5 ftp://[${ip_rc}]:2121${INFILE#/var/ftp} -o $OUTFILE
> +assert_pass "curl ftp passive mode "
> +
> +pkill tcpdump

Can you do this instead?:
> +ip netns exec $S tcpdump -q --immediate-mode -Ui s_r -w ${0##*/}.pcap 2> /dev/null &

tcpdump_pid=$!
sleep 2
...
kill "$tcpdump_pid"

?

pkill will zap all tcpdump instances.
Since tests are executed in parallel, it might zap other tcpdump
instances as well and not just the one spawned by this script.

> +tcpdump -nnr ${0##*/}.pcap src ${ip_rs} and dst ${ip_sr} 2>&1 |grep -q FTP

Not sure why the above is needed.  Isn't the 'cmp' enough to see
if the ftp xfer worked?

> +assert_pass "assert FTP traffic NATed"
> +
> +cmp "$INFILE" "$OUTFILE"

... because if there is a problem with the helper, then the cmp
ought to fail?