> +tcpdump -nnr ${0##*/}.pcap src ${ip_rs} and dst ${ip_sr} 2>&1 |grep -q FTP
Why tcpdump check is needed:
If the snat rule doesn't work, does this test still pass? "cmp file"
pass doesn't mean the snat really happened.
Add tcpdump check to make sure NAT really happened.
> tcpdump_pid=$!
> sleep 2
> ...
> kill "$tcpdump_pid"
Sure, will do.
> So no files are created outside of /tmp.
Sure, got this principal
> +INFILE=$(mktemp -p /var/ftp/pub/)
> This directory might not be writeable.
Sure, vsftpd must support a custom file path. I will find the configuration.
> Is tcpdump a requirement? AFAICS the dumps are only used
> as a debug aid when something goes wrong?
tcpdump is widely used in our LNST test cases. for example check if
one packet got modified.
Is it bad to use in upstream tests? If you still feel strange, I can
remove the tcpdump check.
What I care about most is whether the ruleset in the test is
configured correctly.
One only needs to NAT the control connection — the data connection
will be NATed automatically.
Of course, if there's a better ruleset that can exercise more kernel
code, that would be even better.
Will send out the next version.
On Fri, Jun 6, 2025 at 9:49 PM Florian Westphal <fw@xxxxxxxxx> wrote:
>
> Yi Chen <yiche@xxxxxxxxxx> 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?
>
