pf tests: Do not handle ipfw presence

[igoro]

Summary

Initially, it was added to cover a conflicting case of ipfw and pf used
together. But there are more drawbacks than benefits:

• A half of these tests are always skipped. That leads to misunderstanding, while the test suite strives to avoid ambiguous situations.
• Handling enabled ipfw on the test level is tedious, error-prone, and less maintainable.
• CI and similar parties already know how to deal with ipfw for the test suite, like making it open by default. Extra complexity is not needed.

In addition, ipfw+pf use cases are not officially supported.

[markj]

I don't quite understand the implications of the change. Why do these tests care about whether ipfw is loaded? Will their behaviour change depending on that?

[igoro]

In D46655#1063383, @markj wrote:

I don't quite understand the implications of the change. Why do these tests care about whether ipfw is loaded? Will their behaviour change depending on that?

It's started with divert-to.sh, the if_enc.sh simply repeated it. The fix of pf divert-to exposed a deeper problem when ipfw and pf are used at the same time. The logic of ipdivert was based on ipfw, and pf code tried to mimic ipfw and used the same mbuf tag. It was a conflict of interests between ipfw and pf, which did not allow pf to work as expected. As a result, pf got its own mbuf tag, ipdivert learned to work with two tags.

So, the initial intention of the divert-to.sh tests was to cover the original defect of pf itself. And, ipfwon/ipfwoff tests were added with the idea to also cover that ipfw+pf use case (what actually tests ipdivert). As the final outcome, we have 6, kind of, always skipped pf tests. From the comments I collected so far I've got the feeling that it only adds annoyance instead of value. And the FreeBSD Test Suite does not support something like matrix in GitHub Actions, to declare and test different combinations (by the way, I had some thinking about that, probably, something will come up in the future if it's found practical, but I'm not sure for now). In addition, the ipfw+pf usage is not officially supported. That's why your recent comment (https://reviews.freebsd.org/D46653#1063244) made me re-think it.

I hope it sheds some light.

[kp]

I'm happy with this.
It's been the default assumption in the pf tests anyway. That is, we assume that ipfw is not loaded or at least does not interfere with the pf tests.
Given that ipfw by default blocks all traffic (that's why the CI tests set net.inet.ip.fw.default_to_accept=1) that's usually a reasonable assumption.

[markj]

In D46655#1063387, @igoro wrote:

In D46655#1063383, @markj wrote:

I don't quite understand the implications of the change. Why do these tests care about whether ipfw is loaded? Will their behaviour change depending on that?

It's started with divert-to.sh, the if_enc.sh simply repeated it. The fix of pf divert-to exposed a deeper problem when ipfw and pf are used at the same time. The logic of ipdivert was based on ipfw, and pf code tried to mimic ipfw and used the same mbuf tag. It was a conflict of interests between ipfw and pf, which did not allow pf to work as expected. As a result, pf got its own mbuf tag, ipdivert learned to work with two tags.

Ok, so these restrictions came from that now-fixed problem.

So, the initial intention of the divert-to.sh tests was to cover the original defect of pf itself. And, ipfwon/ipfwoff tests were added with the idea to also cover that ipfw+pf use case (what actually tests ipdivert). As the final outcome, we have 6, kind of, always skipped pf tests. From the comments I collected so far I've got the feeling that it only adds annoyance instead of value. And the FreeBSD Test Suite does not support something like matrix in GitHub Actions, to declare and test different combinations (by the way, I had some thinking about that, probably, something will come up in the future if it's found practical, but I'm not sure for now). In addition, the ipfw+pf usage is not officially supported. That's why your recent comment (https://reviews.freebsd.org/D46653#1063244) made me re-think it.

I hope it sheds some light.

Thanks for the explanation. I would encourage you to include some of these details in the commit log message.

Approved.