[kea-dev] Call for comments about the Decline design

Tomek Mrugalski tomasz at isc.org
Tue Aug 11 16:39:25 UTC 2015


On 11/08/15 18:00, Thomas Markwalder wrote:
> The decline design now states that declined leases will be marked as
> declined by setting the lease attribute, state, to indicate declined AND by
> overwriting the the hardware address/duid with a special value.
> 
> This doesn't make a lot of sense.  One of the primary reasons for creating
> and using lease::state was that it allows us to preserve who declined the
> address.  You may argue that is would be in the logs but that is true only
> if the logging level emits the log message and only if the logs
> themselves go back far enough in time to contain it.
I think it is explained in the design why the client information
is removed. But may it wasn't clear, so let me try to make that point
again. Once client declines a lease, it is no longer associated with it.
Decline means that the client is essentially saying "i don't know who is
using this address, it's not me". Keeping client details with the lease
would be an attempt at keeping part of the lease history within the lease.

Why would you want to keep it anyway? I think you are trying to
half-solve a problem that's really out of scope for now. I presume
you're trying to keep that info, so you could later query the database
and see how many leases a given client declined,
maybe as a way to defend against too many declines from a single client.
Declining many addresses by a single client is one of many, many
different attack vectors. Solving this particular case just for decline
would not solve the general problem at all.  There's no point in putting
stale information into your database on purpose in a futile attempt to
fix this. What we'll do eventually is to implement client-based rate
limiting. It will cover much more than just Decline.

Also, some time in the future, we'll have context-based metrics. Right
now we keep per-subnet information about assigned addresses, but I hope
to extend this, so one day it will be possible to keep the statistics
on per pool, per client-class and per host basis. This would be roughly
matching the functionality of billing-class in isc dhcp.

> In addition it isn't good practice to store the same information twice. It
> ought to be one or the other, not both.
I'm ok with just using state column. But what will you put in the hwaddr
or duid columns? Keeping client's hwaddr and/or DUID is inappropriate
for two reasons, both already mentioned:
- after a lease is declined, it no longer belongs to or is associated
with the client in any way. Also, there's no expectation that the lease
will ever be assigned again to a given client.
- we select a lease based on client's hwaddr or DUID in many places in
the code. If we keep those two fields in the declined leases, all of
those places would have to be updated.

To answer my own question, initially I thought that we do have NON NULL
constraint for hwaddr and DUID fields, but upon actually checking it, we
don't. Ok, so instead of keeping special values there, we can just set
hwaddr/DUID to NULL. Would that address your objection regarding data
redundancy?

There may be places in the code that assume both of those fields are
non-null, but that should be easy to spot and fix.

> I think we can reduce the impact to the getLeases() queries by simply
> modifying them to exclude declined leases (i.e. those leases whose state indicates
> declined).  This would not alter the API at all.  Assuming the query
> does the comparison by address first, testing the state value won't cost much at all.
I must to say no. You'd then need a dedicated query for obtaining
declined leases, so you'd actually make the API more complex and less
intuitive to use.

> An alternative is to allow the queries to return them, along with the active
> and expired leases they can return now, and let the caller cope with them.
Then you have a much bigger problem in doing this check in every place
getLeases4() is called. That's A LOT of places. getLeases4() is used in
.cc files in 193 places. Some of them would require update and and some
would not. But you'd have to check them all to be sure. And that's just
for v4. Do you want to review and potentially update ALL of them? That's
really a lot of effort.

It is much better to follow the reality: Once lease is declined, it is
no longer associated with the client.

> Both declined and expired reclamations will require a new specialized
> queries anyway. If we in the future, we find a need to create a getLeases()
> variant which accepts a state parameter we can do so then.
Yes, but that could be a single query for both use cases that requires
specific value of the state column. It would be state==expired when
dealing with lease reclaimation or state==declined when recovering
expired states.

To wrap this up, would the following change address your concerns?

- Remove special purpose hwaddr/duid
- Use state=declined only

The benefits of that approach are:
- no data redundancy (single condition for declined: state==declined)
- database reflects reality
- much less code to update

Tomek


More information about the kea-dev mailing list