[kea-dev] Review of the DHCPv4 Forge tests failing

[Marcin Siodelski]

Hi All,

We're now at the beginning of the Kea 1.1 preparations. While the scope
of the 1.1 is being finalized the Kea team is taking time to do some bug
fixing. As part of this effort I started going through the failing
DHCPv4 system tests on Jenkins:

https://jenkins.isc.org/view/Forge/job/Kea4-conformance/lastCompletedBuild/testReport/

I share some observations below.


The following three tests:

v4.host.reservation.conflicts.renew-address-that-has-been-reserved-during-reconfiguration.aggregated
v4.host.reservation.conflicts.renew-address-which-reservation-changed-during-reconfigure.aggregated
v4.host.reservation.conflicts.rebind-address-which-reservation-changed-during-reconfigure.aggregated

seem to be failing because of an assumption that when the client having
a lease is NAKed because the lease has just become reserved for another
client, the NAKed lease is removed from the lease database and is
available for the client for which it is reserved. In fact, the lease
will be removed from the database when the client having this lease
would obtain another lease or the lease will expire. This issue has been
reported and described in ticket #3693 and the team never concluded what
the expected behavior on the server side should be. It may be a good
time to decide?


The following test:

v4.host.reservation.hostname-duplicated-entries.aggregated

apparently fails because the server allows the same hostname being
reserved for many clients. This may not be what we desire in general,
but how would you guarantee the uniqueness if you store reservations in
the SQL database? For the reservations in the config file we could try
to apply this restriction but I wouldn't give it high priority. Note
that you may also have two reservations for the same client - one based
on its client id, another one based on its hwaddr. They could naturally
use the same hostname, so do we really want this restriction?


There is a pool of v4.options.malformed.* tests which reveal that the
server doesn't validate some of the configuration information, like
content of configured options, pools etc. This should be addressed, but
I wonder if we need to test it with system tests? Perhaps testing on the
unit tests level should be enough? I rather perceived forge as a
framework for tests that require communication between a client and server.


The following test v4.decline.success

I don't understand what this test is supposed to do. What it seems to be
doing is to make a client obtain a lease. The client then declines a
lease and another client sends DHCPDISCOVER to the server. The test
expects that the server returns a DHCPOFFER, which it doesn't do, and
the test fails. The address pool is restricted to a single address,
which is declined, so the server has nothing to allocate for the other
client. The server which can't offer anything remains silent, which in
my opinion is expected behavior.


The following test:

v4.request.relay.initreboot.no-leases.aggregated

This test expects that the server doesn't respond to a DHCPREQUEST
because it doesn't have a record of the client. Current DHCPv4
implementation allows for cases when the client doesn't include a
requested address option. In this case, the server will allocate some
address from the pool if available.

The test's behavior seems wrong to me because the fact that the server
doesn't have a record of the client doesn't automatically mean that the
packet should be rejected. In fact, when it is a new allocation as part
of the 4-way exchange, the server must allocate/respond to the client
with the allocated address.

I also think that the test is not simulating the init-reboot state
because the client doesn't seem to include requested-ip address option.
If the requested-ip address option is included the server will allocate
the address if it is available, without requiring the client to do
DHCPDISCOVER. This is by Kea design.

There is a question if Kea should allocate an address if the client
neither includes requested IP address option nor ciaddr. Currently it
does. But, I can imagine the case when there are two servers operating
in the same network and the client sends a DHCPREQUEST without
specifying address it is requesting. In this case, both servers would
allocate addresses from the available pools. The client would likely
pick one of them and the other lease would be left for expiry. We may
consider modifying Kea to reject messages which do not contain
indication of the address that the client wants to obtain. This address
would likely be available on only one server in the network and this
would prevent the problem of allocating two leases as a result of
DHCPREQUEST. On the flip side though, the servers are usually processing
traffic through relays which should prevent delivery of client's packet
to both servers. So, maybe it is not a big deal, after all.


In general, I didn't see anything in the test failures that would
require our immediate attention. Though, some system tests are clearly
wrong (e.g. decline) and they should be corrected to not cause false
positives.

Marcin Siodelski
ISC