Add end to end tests for dhclient

[jmg]

Summary

This adds end to end tests using ISC dhcp server to verify dhclient functionality.

It also tests that when an interface has pcp set on it (vlan 0, required when bypassing many ISP's gateways to talk directly to the ONT), this was added in https://cgit.freebsd.org/src/commit/sbin/dhclient?id=abf5bff71d38da3c797a3b6decb426c375cc0f8f

Test Plan

Run the tests:

sbin/dhclient/pcp:normal  ->  passed  [0.153s]
sbin/dhclient/pcp:pcp  ->  passed  [0.181s]

Results file id is usr_tests.20220512-004313-063623
Results saved to /root/.kyua/store/results.usr_tests.20220512-004313-063623.db

2/2 passed (0 failed)

[franco_opnsense.org]

LGTM, thanks!

[kp]

Mostly LGTM too.

A few stylistic remarks.

sbin/dhclient/tests/pcp.sh

4

We no longer add this, unless we're MFC-ing (I think).

65

atf_set require.progs dhcpcd in normal_head should have the same effect, and that saves us a few lines.

If we want more complex checks (than we currently have here) we do have to do them in the body, but then I'd create a 'dhcp_init' function which does vnet_init and whatever checks we want.

92

Just dhcpd? We've just done a which dhcpcd to find it, so it doesn't seem worth having the variable for it.

98

Would it be worth having a 1 address pool for this? Just in case dhcpd ever decides to change how it picks the addresses to hand out. A remote possibility, I grant you.

194

Brace on the next line, if only to be consistent with the rest of this file. (But also with the other tests we have).

[markj]

sbin/dhclient/tests/pcp.sh

49

I think this cleanup is not necessary? kyua will handle it.

98

Why ifconfig -a instead of ifconfig ${epair}b?

157

Should we set -x or wrap these commands with atf_check, so as to fail (more) cleanly if one of the commands fails?

[emaste]

If you upload again for changes, please include full context e.g. git diff -U99999 .... or git show -U99999 ...

sbin/dhclient/tests/pcp.sh

34

These are regular kyua tests and these notes are just a quick reminder for running these tests in isolation, yeah?

[jmg]

Thanks, will address a few of the comments.

sbin/dhclient/tests/pcp.sh

4

Then we need to update atf-sh(1), because it explicitly says to add this:

EXAMPLES
     Scripts using atf-sh(3) should start with:

           #! /usr/bin/env atf-sh

34

Correct. It's notes for me to remember how to run tests directly, as I always forget how to run them. And I did update our kyua docs to list the kyua report command, since there wasn't a good example of this.

49

Theoretically, there is/should be a check to make sure that the test fully cleaned up after itself, except that it doesn't seem to be run, and oddly, runs before AND after cleanup when a failure happens. This would help catch failures to call cleanup functions, like the vnet_cleanup if we warned/errored on this.

65

As was pointed out to me, the issue w/ using require.progs, is that it doesn't provide an explanation on HOW to get the missing program. It'd be nice if require.progs had a little bit of text to explain how/what is needed to obtain the program.

92

Habit of writing scripts to allow people to override the program in more central place.

98

Good question, I'll make that change. Wasn't thinking.

98

Yeah, just habit of needing more than one address.

157

We can't set -x, because that will cause a failure, I tried.

I wasn't quite sure how to handle the setup commands myself. Debugging atf-sh scripts like this, even IF I had atf_check for everything is a PITA. It'd be nice if there was the ability to launch a shell after each command so you could inspect state, or a way to set a breakpoint. To me, it seemed like if the setup commands failed, hopefully either another test caught those failures earlier, or there's something seriously wrong.

[jmg]

sbin/dhclient/tests/pcp.sh

4

Oh, sorry, you were referring to the $FreeBSD$ line, not the #! line, oops. Yeah, I'll drop it.

[jmg]

Addresses a few of the comments.

sbin/dhclient/pcp:normal  ->  passed  [0.163s]
sbin/dhclient/pcp:pcp  ->  passed  [0.190s]

Results file id is usr_tests.20220513-230531-587326
Results saved to /root/.kyua/store/results.usr_tests.20220513-230531-587326.db

2/2 passed (0 failed)

[jmg]

New patch has addressed a few of the comments.

[kp]

sbin/dhclient/tests/pcp.sh

4

I was, yes ;)

Although you also don't need the shebang line. It gets added by the makefiles, so I think you'll now end up with two. Not the worst thing in the world, but a little untidy.

[jmg]

sbin/dhclient/tests/pcp.sh

4

Yeah, this can be fixed when the docs are properly fix to tell people to ignore this.

Problems of atf-sh docs saying one thing, and our testing docs not being details enough w/ back references to say you don't need to do it. (it may well say it SOMEWHERE, but clearly way too buried for me to find it.) Probably expanding tests(7), or build(7) to document ATF_TESTS_SH and what it does.