Tests for pfctl

[paggas1_yandex.com]

Summary

We have created a test suite for pfctl and integrated it under
FreeBSD's source tree using kyua/ATF. The tests reside under
src/sbin/pfctl/tests.

We have copied the most important test cases from OpenBSD's
corresponding src/regress/sbin/pfctl, those that run pfctl on a test
input file and check correctness of its output. We have also added
some new tests using the same format.

The tests consist of a collection of input files (pf*.in) and
corresponding output files (pf*.ok). We run pfctl -nv on the input
files and check that the output matches the output files. If any
discrepancy is discovered during future development in the source
tree, we know that a regression bug has been introduced into the tree.

This form of testing makes it easy to add new tests. The way to do it
is to create a pf*.in and matching pf*.ok file, add them to
src/tests/sbin/pfctl, and update the run file to include the new
files.

The tests are put together using FreeBSD's recommended test suite
which uses kyua. The tests are installed under /usr/tests using a
structure of files named Kyuafile. That means that running 'kyua
test' under /usr/tests will find our tests and run them as part of the
complete test suite.

Test Plan

Since this is a test tool, there is no separate test plan. Running
the tool and proving its usefulness can provide adequate testing.

[kp]

It might also be worth seeing if you can add a couple of tests for ALTQ.
That's also interesting to consider because ALTQ is an optional feature and might not be available in the running kernel.
That'd mean you'd have to have test metadata to figure out if you can sensibly run the test on this system or not (presumably kyua understands the concept of a skipped test).

tests/sbin/pfctl/pf0015.in

1

Just commenting this out still leads to test failures on my machine.
It might be better to just remove this test (and the others like it).

[paggas1_yandex.com]

tests/sbin/pfctl: removed empty tests

[paggas1_yandex.com]

tests/sbin/pfctl: improved test descriptions

[paggas1_yandex.com]

Added a small utility that checks for ALTQ support in the kernel. Also improved integration with the build system.

[kp]

It'd also be good to include the changes required to hook up the sbin/pfctl/Kyuafile to the tests Kyuafile.
Your GitHub repo has a symlink to get recursive behaviour out of Kyua, but there's basically no other symlinks in the repo, so let's avoid that. I'd add a simple include Kyuafile, like ./contrib/atf/Kyuafile does.

tests/sbin/pfctl/pfctl_test.sh

32 ↗

(On Diff #30182)

Wouldn't this allow descriptions with spaces (and optionally multiline):

eval "pf${if}_head() { atf_set descr \"$(pf${i}_descr)\" ; }"

I also wonder if it wouldn't be better to put the description in a per-test file. pf1004.descr for example.

tests/sbin/pfctl/test_altq.c

3 ↗

(On Diff #30182)

This file isn't used yet, right? I'd leave that out of this, and add it in a subsequent patch.

[paggas1_yandex.com]

Den 2017-06-28 kl. 20:14, skrev kristof (Kristof Provost):

kristof added a comment.

It'd also be good to include the changes required to hook up the sbin/pfctl/Kyuafile to the tests Kyuafile.
Your GitHub repo has a symlink to get recursive behaviour out of Kyua, but there's basically no other symlinks in the repo, so let's avoid that. I'd add a simple include Kyuafile, like ./contrib/atf/Kyuafile does.

Done! I've changed the Kyuafiles now, and started using Makefiles
extensively. This also means that tests can be installed to /usr/tests
and run from there, or run from the source directory.

INLINE COMMENTS

pfctl_test.sh:32
+ atf_test_case "pf${i}"
+ eval "pf${i}_head () { atf_set descr $(pf${i}_descr) ; }"
+ eval "pf${i}_body () { \

Wouldn't this allow descriptions with spaces (and optionally multiline):

eval "pf${if}_head() { atf_set descr \"$(pf${i}_descr)\" ; }"

I also wonder if it wouldn't be better to put the description in a per-test file. pf1004.descr for example.

That is true, it was mostly a quick solution. I did the change above
and it works now.

test_altq.c:3
+
+#include <sys/types.h>
+#include <net/if.h>

This file isn't used yet, right? I'd leave that out of this, and add it in a subsequent patch.

I have removed it now.

I will upload the new diff to Phabricator now. The changes are also
visible on my GitHub repo :)

REPOSITORY

rS FreeBSD src repository

REVISION DETAIL

https://reviews.freebsd.org/D11322

EMAIL PREFERENCES

https://reviews.freebsd.org/settings/panel/emailpreferences/

To: paggas1_yandex.com, kristof
Cc: imp, GSoC Admins, GSoC Students

[paggas1_yandex.com]

Cleaned-up version, with improved test descriptions.

[kp]

In D11322#238488, @paggas1_yandex.com wrote:

Cleaned-up version, with improved test descriptions.

I think you may have posted the wrong diff here. This is the sys/netpfil/pf test set, not the sbin/pfctl test set.

[paggas1_yandex.com]

Uploaded correct diff.

[kp]

tests/sbin/pfctl/pfctl_test.sh

3 ↗

(On Diff #30583)

I'm not sure this is quite right. It's possible for the build output to end up elsewhere (MAKEOBJDIRPREFIX), in which case this wouldn't work.

tests/etc/rc.d/routing_test.sh seems to just find binaries in the path. It might be worth asking on freebsd-hackers@ or freebsd-current@ what the preferred way of handling this is.

28 ↗

(On Diff #30583)

This path is wrong. The file is not found. It should be pfctl_test_descr.sh

[kp]

This also appears to be missing the integration with the other tests. It can only be used by running it directly from the tests/sbin/pfctl directory.

tests/sbin/pfctl/files/pfctl_test_descr.sh

3 ↗

(On Diff #30583)

This is a stale comment. The descriptions already include spaces.

[paggas1_yandex.com]

Added tests/sbin/Kyuafile. 'make install' as root still fails because of missing directories under '/usr/tests'.

[paggas1_yandex.com]

Use pfctl from PATH, instead of custom location.

[paggas1_yandex.com]

Removed hardcoded binary path. Also moved tests under src/sbin/pfctl/tests.

[kp]

I think you're also missing the connection in the pfctl makefile. I've been looking at sbin/mdconfig as an example and it has this:

.if ${MK_TESTS} != "no"
SUBDIR+=    tests
.endif

sbin/pfctl/tests/Makefile

6 ↗

(On Diff #30692)

The mdconfig test lists TEST_METADATA.mdconfig_test+= required_user="root"

That might be a good idea for this test too.

[paggas1_yandex.com]

Fixed various issues.

[paggas1_yandex.com]

In D11322#234898, @kristof wrote:

It might also be worth seeing if you can add a couple of tests for ALTQ.
That's also interesting to consider because ALTQ is an optional feature and might not be available in the running kernel.
That'd mean you'd have to have test metadata to figure out if you can sensibly run the test on this system or not (presumably kyua understands the concept of a skipped test).

Pfctl appears to parse files containing ALTQ rules regardless of ALTQ kernel support. So I'm leaving it as it is for now.

In D11322#239521, @kristof wrote:

I think you're also missing the connection in the pfctl makefile. I've been looking at sbin/mdconfig as an example and it has this:

.if ${MK_TESTS} != "no"
SUBDIR+=    tests
.endif

I've added that now.

tests/sbin/pfctl/pf0015.in

1

I have removed them in the updated diff.

[paggas1_yandex.com]

In D11322#239521, @kristof wrote:

I think you're also missing the connection in the pfctl makefile. I've been looking at sbin/mdconfig as an example and it has this:

.if ${MK_TESTS} != "no"
SUBDIR+=    tests
.endif

sbin/pfctl/tests/Makefile

6 ↗

(On Diff #30692)

No need for root privileges to run the parser tests, so I'm leaving it as is for now.

[kp]

I ran into issues with 'install world' with this patch. I think you also want the following:

diff --git a/etc/mtree/BSD.tests.dist b/etc/mtree/BSD.tests.dist
index 56ecb47e4dc..30b04def22f 100644
--- a/etc/mtree/BSD.tests.dist
+++ b/etc/mtree/BSD.tests.dist
@@ -378,6 +378,10 @@
         ..
         mdconfig
         ..
+        pfctl
+            files
+            ..
+        ..
     ..
     secure
         lib

diff --git a/targets/pseudo/tests/Makefile.depend b/targets/pseudo/tests/Makefile.depend
index 0ce21a9461b..ba22364750f 100644
--- a/targets/pseudo/tests/Makefile.depend
+++ b/targets/pseudo/tests/Makefile.depend
@@ -185,6 +185,7 @@ DIRDEPS= \
       sbin/growfs/tests \
       sbin/ifconfig/tests \
       sbin/mdconfig/tests \
+      sbin/pfctl/tests \
       sbin/tests \
       secure/lib/tests \
       secure/libexec/tests \

Even with that I still see issues. It looks like the test files don't get installed (/usr/tests/sbin/pfctl/files only contains pfctl_test_descr.sh)

sbin/pfctl/tests/Makefile

6 ↗

(On Diff #30692)

Ah, my misunderstanding. I thought we actually set the rules and then read them back, but that's not what pfctl -n does. I didn't expect the "Don't actually set the rules" mode to also print them again, but it is rather useful for this test that it does.

[paggas1_yandex.com]

Added workaround so that the test files get installed correctly.

[paggas1_yandex.com]

New version.

[paggas1_yandex.com]

Reverted to previous version (last update was a mistake).