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

Thomas Markwalder tmark at isc.org
Tue Aug 11 17:31:17 UTC 2015


On 8/11/15 12:39 PM, Tomek Mrugalski wrote:
> 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 prefer NULL to then try convey a special business meaning.  That was
primary objection. 
Using both would be confusing.

>> 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.
I didn't say this was better,  I said it was an alternative.  But
correct me on this,
won't getLeases4() currently return an expired lease?   Perhaps it makes
no difference
and we just reuse it.  I'm not certain if we can end up in a situation
where there is
both an expired lease and an active lease for the same client.

Keep in mind, too that lease::state of expired may not ever exist.   We
won't be
setting lease state to expired automatically when a lease expires.  I'm
not sure the
reclamation process ever sets it to expired either.   
> 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.
I'm not sure a single query for this is possible, as mentioned above when
trying to reclaim expired leases, the query will have to look at the
"expire"
column whereas declined leases must be selected based on the lease::state.
Additionally, both queries need to be ordered in ascending order by "expire"
if we wish to process them oldest first.
> 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
Yes.  Again my primary issue was the special use of hwaddr/duid
attributes. 





More information about the kea-dev mailing list