ping_tests: Introduce a script to test ping

[jlduran]

Summary

This script is largely inspired by injection.py, but it has been
extended to test most of ping.c.

This commit simply replaces the current injection.py with pinger.py.

It will provide the basis for series of fixes.

There is one test that is wrongly passing. Let's mark it as XXX for
now, it will be fixed in future commits.

Test Plan

The idea is just to get a review of the python/Scapy script itself and to use it as a drop-in replacement for injection.py, for now.

Some of the bugs have been simplified here:

https://github.com/jlduran/ping-bugs

[melifaro]

Thank you for working on this!
Any thoughts of using native python interface instead of the shell wrappers?
Just in case, the examples can be found in tests/sys/net/routing/test_rtsock_multipath.py or tests/sys/netlink/test_rtnl_iface.py

[jlduran]

In D37876#860539, @melifaro wrote:

Any thoughts of using native python interface instead of the shell wrappers?

That looks nice!, but do you mean using pytest instead of atf-sh(3), or instead of using subprocess.run to execute shell commands?
If the latter, given a separation between kernel and userland tests is likely desired, duplication of the tools/helpers is what we should do initially?
For example, instead of:

import subprocess

subprocess.run(["sysctl", "net.inet.ip.process_options=0"], check=True)

let's change it to a native python interface, using tests/atf_python/sys/net/tools.py:

from atf_python.sys.net.tools import ToolsHelper

ToolsHelper.set_sysctl("net.inet.ip.process_options", 0)

Should we duplicate tools.py initially, and place it under a common test directory for userland? Given downstream projects may consume userland- or kernel-only code from FreeBSD.
I was planning on stealing common code from tests/sys for a future VNET jails integration, so the placement of vnet.subr was going to raise the same question.

[melifaro]

In D37876#860550, @jlduran_gmail.com wrote:

In D37876#860539, @melifaro wrote:

Any thoughts of using native python interface instead of the shell wrappers?

That looks nice!, but do you mean using pytest instead of atf-sh(3), or instead of using subprocess.run to execute shell commands?

Pytest. There is a wrapper that automatically wraps pytest tests into kyua interface, allowing to run them in a standard system test way via kuya.

If the latter, given a separation between kernel and userland tests is likely desired, duplication of the tools/helpers is what we should do initially?

In’s not exactly kernel/userland but more like just as include, so all of the files are good to use in any tests.

For example, instead of:

import subprocess

subprocess.run(["sysctl", "net.inet.ip.process_options=0"], check=True)

let's change it to a native python interface, using tests/atf_python/sys/net/tools.py:

from atf_python.sys.net.tools import ToolsHelper

ToolsHelper.set_sysctl("net.inet.ip.process_options", 0)

Absolutely.

Should we duplicate tools.py initially, and place it under a common test directory for userland? Given downstream projects may consume userland- or kernel-only code from FreeBSD.

I’d suggest using (and altering) the existing tools.py

I was planning on stealing common code from tests/sys for a future VNET jails integration, so the placement of vnet.subr was going to raise the same question.

I’d vote for just re-using it. Happy to review/accept any changes required

[asomers]

I think this looks really great. I assume you have a few extra test cases that you're planning to add to ping_test.sh? Personally, I don't have strong opinions about whether to use atf-sh or pytest, as long as you can get the cleanup and metadata correct. And don't forget that you'll need to add injection.py to ObsoleteFiles.mk, or else rename pinger.py to injection.py.

sbin/ping/tests/ping_test.sh

262

Explicitly deleting files is unnecessary, because Kyua uses a per-testcase tempdir.

sbin/ping/tests/pinger.py

2

s/python/python3/

28

Specifying the iface on the command line will fail if an interface named tun0 already exists for some reason. Better to do a plain ifconfig tun create and use whatever ifconfig returns.

40

Probably not necessary to test the Evil bit.

75

Does Scapy have definitions for these magic numbers?

164

@melifaro note that this line only works because atf-sh runs each test case in its own temporary directory. I don't think the pytest Kyua wrapper does that.

[melifaro]

sbin/ping/tests/pinger.py

164

atf-sh is just an utility wrapper that implements kyua/atf protocol.
All isolation/priv drops are done by kyua itself.
So it’s the same for pytest wrapper and in fact nearly all tests (SingleVnetTest) use that.

[jlduran]

In D37876#860561, @asomers wrote:

I assume you have a few extra test cases that you're planning to add to ping_test.sh?

Yes! Absolutely, I currently have more than 10 tests using this script.

And don't forget that you'll need to add injection.py to ObsoleteFiles.mk, or else rename pinger.py to injection.py.

Thank you, I was completely unaware of this step.
There it goes my ping-themed series of tests... I even had the jails named BRL, after the Ballistic Research Lab, :-(
I'll think it over, but it is likely I won't pollute ObsoleteFiles.mk out of sheer vanity and rename the script. However, it can do more than just inject packets.

sbin/ping/tests/ping_test.sh

262

OK, I'll remove it, if a test fails, it complains about it having "Files left in work directory after failure". I thought it was a common courtesy to remove them anyways.
I'll also remove the std.out and std.out.filtered files that I plan on use.

sbin/ping/tests/pinger.py

28

Yes, If you don't mind, I had this planned for the next commit, where I put these tests inside a VNET jail each, and extend tests/sys/common/vnet.subr to create tun interfaces:
tun=$(vnet_mktun) and pass --iface $tun. But if you prefer, I can squash all in one commit for this revision.

40

I'll remove it. I only plan to expose a minor bug using the Don't Fragment bit.
The original idea was to keep parity with scapy:
https://github.com/secdev/scapy/blob/9473f77d8b548c8e478e52838bdd4c12f5d4f4ff/scapy/layers/inet.py#L531

75

Not that I'm aware. In fact, this line was taken from scapy:
https://github.com/secdev/scapy/blob/9473f77d8b548c8e478e52838bdd4c12f5d4f4ff/scapy/layers/inet.py#L951

164

Thank you! I'll study those pytests!
At the moment, I fail to see the advantage of one over the other, but this may be the product of my ignorance on the subject.

[jlduran]

Address first round of suggestions.

sbin/ping/tests/ping_test.sh

262

I can't edit comments. For self reference: 96b90524d155bdd2d3f78a0a50cf250164bfbf6c

[jlduran]

sbin/ping/tests/pinger.py

164

For self reference: D31084 (should become a wiki!)

[jlduran]

• Add an ObsoleteFiles.inc entry (TODO: fix date upon commit)
• Rely on tests/sys/common/vnet.subr for VNET jail and tun interface creation
• Add a vnet_mktun() function to vnet.subr
• Imprison tests (test can now run simultaneously while using the same IP address)
• PEP 8 lint the script using black/flake8 (maximum-line-length 79 characters)

[asomers]

Ahh, you're correct about the cleanup directory. I misremembered that.

sbin/ping/tests/pinger.py

28

Why bother with vnet? Is that really necessary for these tests?

75

Lame. It would be good to fix that some day, but no need to do it now.

[jlduran]

@asomers I can drop the jailing part at your request, of course, although I personally prefer to have these tests jailed.
I am also playing with the native python interface, it'll take me a while, since I already had everything set up for atf-sh. I stopped short of getting started with IPv6 tests, and now I'm grateful, as I am currently lured by the possibility of using parameterized pytests.

sbin/ping/tests/pinger.py

28

My original thought was, VNET jails are cheap, and allow us to remove the "exclusivity clause" from the Makefile, maybe to allow running these tests in parallel while using the same IP address in all of them.

75

After your suggestion, I tried to essentially create a table from <netinet/ip_icmp.h>, but given most of those codes are already deprecated, and we are even missing the names for ICMP codes 37 and 38, I gave up.

[asomers]

ObsoleteFiles.inc

56 ↗

(On Diff #114580)

These paths should be relative to /, not /usr/src. So in this case, usr/tests/sbin/ping/injection.py

sbin/ping/tests/ping_test.sh

166

Since you're using vnet, you really could go back to hardcoding tun0, if that would make things easier. But it's probably best not to, in case future developers want to disable the vnet part for some reason. It's more robust this way.

sbin/ping/tests/pinger.py

28

Ahh, removing the "exclusivity clause" is a definite benefit. That makes sense.

[jlduran]

• Use the right path for the obsoleted file

[jlduran]

ObsoleteFiles.inc

56 ↗

(On Diff #114580)

Thank you! That makes sense.

[jlduran]

Abandoned in favor of D38053, which uses a native Python testing infrastructure. Thank you!