[kea-dev] Client classification design

[Tomek Mrugalski]

Thanks a lot for your comments. See my responses inline.

On 14.10.2015 13:39, Marcin Siodelski wrote:
> On 13.10.2015 17:31, Tomek Mrugalski wrote:
>> Folks,
>> I took a first stab at the client classification design. See the
>> document here: http://kea.isc.org/wiki/ClientClassificationDesign
>>
>> For the reference, we have the requirements more or less described here:
>> http://kea.isc.org/wiki/ClientClassificationRequirements.
>>
>> We had some discussions about the syntax, but I did think about it a bit
>> more and came up with three alternative proposals. Please comment on
>> which one seems to be the best in your opinion (or reject all of them
>> and propose a fourth one). I tried to be objective and not push for any
>> specific one.
>>
>> Keep in mind that we're working under very tight schedule here and
>> client classification is a broad and complex topic. There's a lot we
>> could do, but due to the time constraints, we need to be very selective
>> for phase 1 (aka Kea 1.0).
>>
>> Some parts of the design will likely be moved to the requirements page,
>> but I want to keep it in the new doc for now, so you could review it
>> more easily as one chunk.
>>
>> It would be great if you could provide your comments by end of this week
>> (Oct. 16th), so we could start an implementation the next week.
>>
>> Thoughts? Comments? Tomatoes?
>>
> First of all, let me say that it is nice and comprehensive document. It
> does lack some of the details but given the amount of time you had to
> write it up, and the time constraints for this work overall, you've done
> pretty good job. The code snippets are valuable addition.
Thanks.

> It would be good to extend this document with some text describing what
> bits and pieces go where in the code structure. For example, are you
> going to create a separate library for client classification or put it
> into the libdhcpsrv. I feel the client classification is going to be a
> large feature and it actually deserves its own place in the code. The
> libdhcpsrv is already bloated. Also, the future implementation of the
> client and the relay may benefit from this.
Good idea. Added.

> It would be nice to have a list of all tasks to be performed to
> implement the limited client classification for 1.0. These tasks would
> be converted to actual tickets once the design is approved. For now it
> would serve to the reviewers as an indicator what exactly has to be done
> to meet our 1.0 goals.
Added a list of tasks. It's depressingly long. Currently has 16 tickets.
All of them are necessary for *basic* client classification to be usable.

> Apart from implementing new classes, what are the changes needed to
> Option class and classes derived from it? What are the requirements for
> new Option classes to be implemented in the future?
> 
> I presume that expressions will be converted to RPNs in the server
> configuration time. But then, where would those RPNs be held until the
> next configuration or server shutdown? Is this CfgMgr, SrvConfig class,
> Subnet class?
There's a new section that explains how the lib will be plugged into
existing code.

> It would be helpful to see some performance considerations here. For
> example: how are we going to mitigate string conversions? Is there any
> plan for mitigating the conversion of the whole option to string if only
> specific field is of interest?
Added, but it's likely not what you'd like to see. The bottom line is
that we can do some tweaks, but if you need high performance and lots of
classes, our recommendation will be to write dedicated hooks. (it's
possible and quite easy to do classification in hooks, even now).

> Example Use Cases -> Use case 1
> From what I have seen the following condition:
> match if substring (option vendor-class-identifier, 0, 9) = "PXEClient";
> is a typical one to check if the client is a PXE client (apart from
> other use scenarios for it). So, I think this should be implemented in
> phase1.
Updated to be phase 1.

> Design Assumptions -> General requirements
> 
> Some of the listed assumptions are not candidates for moving into the
> requirements document, e.g. G.1, G.4, G.6 as they are vague and
> unmeasurable. However, this is not to say that they are wrong as design
> assumptions.
Reorganized the whole thing. Former requirements page is now called
ClientClassificationDiscussion. I wrote the requirements page from
scratch. Most of the general requirements are there, except those you
mentioned. They stayed in the design page.

> Design Assumptions -> Expressions:
> 
> Extracting option values from the vendor-class-identifier (60) must be
> supported. In particular you should be able to do this:
> 
> match if substring (option vendor-class-identifier, 0, 9) = "PXEClient";
Added.

> Design Assumptions -> Functionality
> 
> There is more to this:
> F.3 It MUST be possible to override the options defined for a given
> subnet, with the options defined for a given class.
Due to text reorg, I couldn't find F.3 anymore. Anyway, I reworded
existing requirements and added an new one (F.3) that says explicitly
that if there's a generic option (for all clients) and there's another
option specific for a class, the server must use value specified for the
class, not the generic one.

> Note that F.3. is different than F.1. because F.1. says "extra" options.
> As for E.6 - yes I think it is required.
Ok, it's now phase 1.


> Configuration
> I am leaning towards proposal 2.
Acknowledged. There's one significant issue with proposal 2, though.

Currently the code execution order is as follows

1. classify packet
2. call selectSubnet (and use class information)

And that order must stay if we want to keep the ability to allow or
reject clients into subnet based on classes.

We could flip the order and do:

1. selectSubnet
2. classify packet based on the per subnet definition

It would be slightly better performance wise (need to evaluate only
those classes that are actually used in this subnet), but we would lose
the ability to select subnet based on class.

As a reminder, we need that ability to handle cable modems. Cable modems
should go to subnet X and all devices behind it should go to subnet Y.
It's essential for X and Y to not overlap, as cable users are not
supposed to be able to fiddle with their modem's configuration interface.

> Parsing
> 
> I think we really need to clarify on the need for substrings being
> extracted from the vendor-class-identifier options. Currently the design
> is based on the assumption that the only required operator is equality
> operator. It is explicitly said in this section.
It's now clarified that vendor-class-identifier and substring are required.

> Evaluation
> 
> I am slightly confused with the code examples. I realize that you're
> describing two proposals for retrieving the result of the evaluation,
> i.e. one is to store the result in the value_, another one return it.
> The evaluateToBool() example seems to be using the latter approach
> because it assigns the result of evaluate() to a value. However, the
> evaluareToBool() later obtains the value using the toString() method.

> bool evaluateToBool(const Expression& expr, Pkt6Ptr& pkt) {
>     TokenPtr prv1;
>     TokenPtr prv2;
>     TokenPtr prv3;
>
>     /// Let's evaluate all tokens in their RPN order
>     for (Expression::iterator current = expr.begin(); current !=
> exp.end(); ++current) {
>
>         string value = *current->evaluate(pkt, prv1, prv2, prv3);
>
>         /// Let's remember up to three last tokens
>         prv3 = prv2;
>         prv2 = prv1;
>         prv1 = *current;
>     }
>     return (value == "true");
> }
> 
> In order to determine whether it is better to store the evaluation
> result internally in the Token or return it, it would be good to have
> code snippet for both cases and see how the code looks like. However,
> from the given examples I tend to think that the Token should be
> stateless and the result of evaluation should be returned. This is your
> proposal 2.
Ok, updated the snipped. It's still not correct, as the parameters
passed to evaluate() would be token values, not tokens, but I don't
think it's that important. The point I was trying to make here is that
there will be a stack that the evaluation code will walk over,
evaluating tokens one at a time. Once the last token is evaluated, its
value determines the value of the whole expression.

Tomek