[kea-dev] Client classification design

[Marcin Siodelski]

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.

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.

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.

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?

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?

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.


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.


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";


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.

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.


Configuration

I am leaning towards proposal 2.


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.


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.

I think the correct code is: (?)

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");
}

No?

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.

Marcin