netpfil tests: Add functions for testing routing scenarios

[vegeta_tuxpowered.net]

Summary

Many pf tests use identical setup where one jail is a router and optionally another jail is a server. Add functions to create such jails for IPv6 and IPv4 and functions to perform tests on such setup. Add tests using those functions: scrub actions, routing table, tcp sequence number modulation.

Sponsored by: InnoGames GmbH

[melifaro]

Thank you for working on improving the testing infra!
I'd suggest looking into the already-existing native python functionality: https://github.com/freebsd/freebsd-src/blob/main/tests/examples/test_examples.py#L86
It would be nice to use something better than a shell for complex test scenarios - that would simplify the reusability and reduce the amount of code.
What do you think?

[kp]

There's certainly a lot of repetition in many of the pf (and other firewall) tests.

So far I've chosen to not do anything about that, because it makes each individual test case much easier to understand. Each test case fully describes the setup it operates in, and when someone tries to debug it (or understand it for any other reason) there's no need to go look at other files to work out what the setup actually is.

I'm not 100% committed to this approach, but at the very least I'd like to see examples of what the tests themselves end up looking like.

[melifaro]

In D38126#866273, @kp wrote:

There's certainly a lot of repetition in many of the pf (and other firewall) tests.

So far I've chosen to not do anything about that, because it makes each individual test case much easier to understand. Each test case fully describes the setup it operates in, and when someone tries to debug it (or understand it for any other reason) there's no need to go look at other files to work out what the setup actually is.

I'm not 100% committed to this approach, but at the very least I'd like to see examples of what the tests themselves end up looking like.

Sure!
https://github.com/freebsd/freebsd-src/blob/main/tests/sys/netlink/test_rtnl_ifaddr.py -- probably the most clear one

https://github.com/freebsd/freebsd-src/blob/main/tests/sys/net/routing/test_rtsock_multipath.py -- tests with high level of parametrisation
https://github.com/freebsd/freebsd-src/blob/main/tests/sys/netinet6/test_ip6_output.py -- 2-jails tests with the separate python handler

[vegeta_tuxpowered.net]

In D38126#866273, @kp wrote:

There's certainly a lot of repetition in many of the pf (and other firewall) tests.

So far I've chosen to not do anything about that, because it makes each individual test case much easier to understand. Each test case fully describes the setup it operates in, and when someone tries to debug it (or understand it for any other reason) there's no need to go look at other files to work out what the setup actually is.

I must say that I found the old code quite hard to understand, not due to repetition but due to jail and variable names not really reflecting what they do. That might work fine for just a single jail with a single epair interface, but once you get a router and a server it became a bit messy to my eyes.

I'm not 100% committed to this approach, but at the very least I'd like to see examples of what the tests themselves end up looking like.

The use case for those changes is in D38129. I thought splitting the huge amount of changes in tests I've ended up with while developing the OpenBSD-like scrub syntax into separate reviews would be a good idea, but I think it backfired. Should we merge those to reviews into one?

[kp]

In D38126#866296, @vegeta_tuxpowered.net wrote:

In D38126#866273, @kp wrote:

I'm not 100% committed to this approach, but at the very least I'd like to see examples of what the tests themselves end up looking like.

The use case for those changes is in D38129. I thought splitting the huge amount of changes in tests I've ended up with while developing the OpenBSD-like scrub syntax into separate reviews would be a good idea, but I think it backfired. Should we merge those to reviews into one?

That, along with the code changes themselves, are on my list, but I don't expect to get the serious time and concentration that'll require before March.

[vegeta_tuxpowered.net]

! In D38126#866510, @kp wrote:

! In D38126#866296, @vegeta_tuxpowered.net wrote:

The use case for those changes is in D38129. I thought splitting the huge amount of changes in tests I've ended up with while developing the OpenBSD-like scrub syntax into separate reviews would be a good idea, but I think it backfired. Should we merge those to reviews into one?

That, along with the code changes themselves, are on my list, but I don't expect to get the serious time and concentration that'll require before March.

If it's gonna take so long, I could in the meantime use the routing scenarios proposed in this review and pick tests for the current pf.conf syntax from D38129. Maybe with the amount of tests for all scrub and rtable functions I could convince you to the benefits of splitting creation of the testing environment into reusable functions :) D38129 would then be left only with tests for the new OpenBSD-compatible syntax and could be reviewed together with it later.

[melifaro]

In D38126#866510, @kp wrote:

In D38126#866296, @vegeta_tuxpowered.net wrote:

In D38126#866273, @kp wrote:

I'm not 100% committed to this approach, but at the very least I'd like to see examples of what the tests themselves end up looking like.

The use case for those changes is in D38129. I thought splitting the huge amount of changes in tests I've ended up with while developing the OpenBSD-like scrub syntax into separate reviews would be a good idea, but I think it backfired. Should we merge those to reviews into one?

That, along with the code changes themselves, are on my list, but I don't expect to get the serious time and concentration that'll require before March.

Would you be open to reviewing python re-implementation of some of the existing pf tests?

[kp]

In D38126#866663, @melifaro wrote:

Would you be open to reviewing python re-implementation of some of the existing pf tests?

I am skeptical that that'd be an improvement for most of the current pf tests, but open to being proved wrong. Maybe pick one that you think would improve the most? (i.e. one, to have something specific to discuss.)

[kp]

In D38126#866648, @vegeta_tuxpowered.net wrote:

If it's gonna take so long, I could in the meantime use the routing scenarios proposed in this review and pick tests for the current pf.conf syntax from D38129. Maybe with the amount of tests for all scrub and rtable functions I could convince you to the benefits of splitting creation of the testing environment into reusable functions :) D38129 would then be left only with tests for the new OpenBSD-compatible syntax and could be reviewed together with it later.

Yeah, that probably makes sense. It'd also be good to separate the "remove redundancy" and "adjust for new syntax" changes into separate commits.

[vegeta_tuxpowered.net]

I have updated the review to not only add functions for creating routing-based tests but also to make use of them by adding more tests for standard pf.conf syntax. This makes this review not related anymore to D38025.

[kp]

Can you point me at the review for the setup_router_dummy_ipv4, ping_server_check_reply, .. functions? I'm sure you posted that somewhere, but I can't seem to find it.

[kp]

I'm certainly seeing the argument for having setup_router_dummy_ipv6 and friends do the test setup.

tests/sys/netpfil/pf/scrub.sh

44

I'd also add a test here to check that we don't manipulate the mss if there's no rule to do so.

[vegeta_tuxpowered.net]

Re-add the missing part: the setup functions.

[vegeta_tuxpowered.net]

In D38126#869129, @kp wrote:

I'm certainly seeing the argument for having setup_router_dummy_ipv6 and friends do the test setup.

I've generated the diff wrongly. Now it's fixed.

[kp]

Yeah, this is a clear improvement.

I do think we need to document what the setup functions do, and what variables they make available. Perhaps in a comment block on top of them?

tests/sys/netpfil/pf/fragmentation.sh

343

(nit) reassembled

350

I'm not fond of the 0/1 thing to indicate expected failure or success. It's a little unclear what it's meant to be here.

Maybe --expect-failure, or even passing 'exit:0' instead?

360

We're not actually checking that pf removed the DF flag here.

Could we extend the test to check for this on the server side, or maybe even use ipfw (because I don't think pf can be convinced to do this) to drop packets with the DF flag set?

tests/sys/netpfil/pf/utils.subr

273 ↗

(On Diff #116038)

I think that needs another 4 spaces to indent it. It'll make it more obvious that it's a single command split over several lines.

[vegeta_tuxpowered.net]

Fixed spelling, indentation and the expected test exit code argument.