Send kea-dev mailing list submissions to
[email protected]
To subscribe or unsubscribe via the World Wide Web, visit
https://lists.isc.org/mailman/listinfo/kea-dev
or, via email, send a message with subject or body 'help' to
[email protected]
You can reach the person managing the list at
[email protected]
When replying, please edit your Subject line so it is more specific
than "Re: Contents of kea-dev digest..."
Today's Topics:
1. Re: Call for comments about the Decline design
(Thomas Markwalder)
2. Re: Call for comments about the Decline design (Tomek Mrugalski)
3. Re: Call for comments about the Decline design
(Thomas Markwalder)
4. Re: Call for comments about the Decline design (Tomek Mrugalski)
5. Re: Call for comments about the Decline design
(Thomas Markwalder)
----------------------------------------------------------------------
Message: 1
Date: Tue, 11 Aug 2015 12:00:26 -0400
From: Thomas Markwalder <[email protected]>
To: Marcin Siodelski <[email protected]>, Tomek Mrugalski
<[email protected]>, "[email protected]" <[email protected]>
Subject: Re: [kea-dev] Call for comments about the Decline design
Message-ID: <[email protected]>
Content-Type: text/plain; charset=windows-1252
On 7/28/15 7:06 AM, Thomas Markwalder wrote:
> On 7/28/15 3:38 AM, Marcin Siodelski wrote:
>> On 17.07.2015 17:01, Thomas Markwalder wrote:
>>> On 7/13/15 10:54 AM, Tomek Mrugalski wrote:
>>>> Hi everyone,
>>>> As part of the Kea 1.0 preparation, I wrote a short document about our
>>>> intended design for Decline support. It is described here:
>>>>
>>>> http://kea.isc.org/wiki/DeclineDesign
>>>>
>>>> The major idea is to use special hardware address or duid values to
>>>> indicate a declined address and keep it in the regular lease database.
>>>> With this approach, the amount of work is greatly reduced, there is
>>>> almost no performance degradation and this approach is proven
>>>> (implemented years ago in Dibbler) to work well.
>>>>
>>>> I'd like to hear your opinions on this proposal. We plan to conclude the
>>>> design discussions around end of July.
>>>>
>>>> Tomek
>>>>
>>>> _______________________________________________
>>>> kea-dev mailing list
>>>> [email protected]
>>>> https://lists.isc.org/mailman/listinfo/kea-dev
>>> I am not crazy about special purposing HWADDR/DUID to indicate a that lease
>>> has been declined. I get that it is "less expensive" in the short term
>>> from a development standpoint. I understand the appeal of that rationale but
>>> if that is the only motivation it isn't enough.
>>>
>>> From an analysis standpoint HWADDR/DUID are values that identify DHCP
>>> clients not a indicator of lease state. We all know that proper API design
>>> frowns upon multi-meaning parameters and the reasons why.
>>>
>>> We must also ask ourselves if there are any other circumstances under
>>> which we might persist a lease other than when it has been granted or
>>> declined? If so, would we need to retain the actual client values for
>>> HWADDR/DUID or would we define more special values? If we think there
>>> is a reasonable possibility of other cases where we would persist leases,
>>> then we should design for that now. A more extensible approach would be
>>> to add a state or status column.
>>>
>>> I realize that would leave us with a question of what values would we then
>>> store in these columns when a lease is declined. Again, from an analysis
>>> standpoint the most meaningful values would be those of the declining
>>> client.
>>> Suppose we did retain the value(s) from the declining client, what are the
>>> ramifications there? It is already conceivable that a returning client
>>> could
>>> find a previous but expired lease right? We must be checking for that
>>> now right?
>>> Bear in mind too, that using the "extra" effort it takes to test a flag or
>>> state column is no more than the effort the it takes to to test for a
>>> special
>>> value in HWADDR/DUID.
>>>
>>> Similarly, using the viewpoint of "don't change the schema" if we can
>>> squeeze
>>> it in somewhere else is short-sighted thinking. Not just in this matter
>>> but in
>>> any other. Eventually short-cutting the solution to "save time" ALWAYS
>>> comes
>>> back to haunt you. It may be faster now but we will feel the pain
>>> later. We
>>> have schema-upgrading now built into the project so we should not be afraid
>>> to change it. We should strive to do it right the first time.
>>>
>>> My gut instinct is that lease should have a state or status column and we
>>> should use it to indicate that a lease has been declined.
>>> ----------------------------------------------------------
>>>
>>> Regarding declined address processing via expiration - Should we really call
>>> both the decline_recycle and expiration_reclaim hooks? Seems to me that it
>>> should only call the decline_recycle hook. From a business logic
>>> standpoint it
>>> wasn't an active lease so it cannot "expire" and therefore cannot be
>>> reclaimed from
>>> expiration. If we call both then hooks developers may have to implement
>>> logic
>>> in their callouts to distinguish between the two business cases.
>>>
>>> What about the skip flag? How/where does this fit in?
>>>
>>> ----------------------------------------------------------
>>>
>>> Regarding the optional command to get "all declined", there needs to be
>>> some guard
>>> against how many we will return, if the number of leases is large it
>>> could be an issue
>>>
>>> ----------------------------------------------------------
>>>
>>>
>> Given all the comments for the lease expiration design and the decline
>> design, I would like to propose that we actually implement the
>> additional column 'state' for the Kea 1.0.
>>
>> The lease expiration, as it is described now, marks the lease as
>> 'reclaimed' by simply removing it from the lease database. This
>> minimizes changes to the lease database backends etc. However, I am
>> quite confident (after some discussions I had so far) that at some point
>> we will want 'address affinity'. In that case, removing the lease on
>> expiration is not a right approach.
>>
>> Since we need to add new functions and indexes to retrieve expired
>> leases, *now* seems to be the best time to add the new 'state' field if
>> we want to go down this path.
>>
>> The reason I bring it in this thread is that at this point we may also
>> consider if this field can be used for declined addresses.
>>
>> The big question is how to encode the state of the lease. I'd be in
>> favor of encoding a single state on an individual bit, in which case it
>> would be possible to encode multiple states with a single INT value,
>> i.e. the lease could be in multiple states simultaneously.
>>
>> I am seeking for an answer whether we want this or not before I further
>> work on the lease expiration design, as almost all further updates are
>> going to be based on this decision.
>>
>> Marcin
> A bitmask as you suggest, is probably the most flexible. And I believe
> that the time to add the new field is now, assuming that is the consensus.
>
> Thomas
>
>
>
Tomek:
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.
In addition it isn't good practice to store the same information twice. It
ought to be one or the other, not both.
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.
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.
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.
Thomas
------------------------------
Message: 2
Date: Tue, 11 Aug 2015 18:39:25 +0200
From: Tomek Mrugalski <[email protected]>
To: Thomas Markwalder <[email protected]>, Marcin Siodelski
<[email protected]>, "[email protected]" <[email protected]>
Subject: Re: [kea-dev] Call for comments about the Decline design
Message-ID: <[email protected]>
Content-Type: text/plain; charset=windows-1252
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
------------------------------
Message: 3
Date: Tue, 11 Aug 2015 13:31:17 -0400
From: Thomas Markwalder <[email protected]>
To: Tomek Mrugalski <[email protected]>, Marcin Siodelski
<[email protected]>, "[email protected]" <[email protected]>
Subject: Re: [kea-dev] Call for comments about the Decline design
Message-ID: <[email protected]>
Content-Type: text/plain; charset=windows-1252
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.
------------------------------
Message: 4
Date: Wed, 12 Aug 2015 13:43:18 +0200
From: Tomek Mrugalski <[email protected]>
To: Thomas Markwalder <[email protected]>, Marcin Siodelski
<[email protected]>, "[email protected]" <[email protected]>
Subject: Re: [kea-dev] Call for comments about the Decline design
Message-ID: <[email protected]>
Content-Type: text/plain; charset=windows-1252
On 11.08.2015 19:31, Thomas Markwalder wrote:
> On 8/11/15 12:39 PM, Tomek Mrugalski wrote:
>> 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.
Decline design updated. Special duids and hwaddr are gone. Unless you
(or other folks) raise other issues, I consider this design done.
Tomek
------------------------------
Message: 5
Date: Wed, 12 Aug 2015 07:44:20 -0400
From: Thomas Markwalder <[email protected]>
To: Tomek Mrugalski <[email protected]>, Marcin Siodelski
<[email protected]>, "[email protected]" <[email protected]>
Subject: Re: [kea-dev] Call for comments about the Decline design
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8
On 8/12/15 7:43 AM, Tomek Mrugalski wrote:
> On 11.08.2015 19:31, Thomas Markwalder wrote:
>> On 8/11/15 12:39 PM, Tomek Mrugalski wrote:
>>> 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.
> Decline design updated. Special duids and hwaddr are gone. Unless you
> (or other folks) raise other issues, I consider this design done.
>
> Tomek
>
You'll need to update the ticket, it's in the review queue.
------------------------------
_______________________________________________
kea-dev mailing list
[email protected]
https://lists.isc.org/mailman/listinfo/kea-dev
End of kea-dev Digest, Vol 17, Issue 11
***************************************
