[kea-dev] Client classification design

[Thomas Markwalder]

On 10/15/15 1:34 PM, Tomek Mrugalski wrote:
> On 15.10.2015 19:06, Tomek Mrugalski wrote:
>> On 15.10.2015 05:09, Shawn Routhier wrote:
>>> 4) It would be quite useful to have some method to help the admin debug these things.  In ISC DHCP 
>>> the standard way is to insert a statement that will log something into the class definition 
>>> or use of class in a subnet.  This provides a way for the admin to see if they wrote the
>>> matching statements correctly for the given clients.
>> This will be taken case with a dedicated logger in the expressions
>> library. Added G.9 requirement to explicitly state that.
>>
>>> If we go with proposal 1 it would seem simple enough to have the class definitions be global
>>> while the use of the classes would be only per subnet in phase 1.  It would also seem that we
>>> would then be able to extend the global class definition to add option data in phase 2.
>>>
>>> To me it seems like it will be easier to expand something like proposal 1 into phase 2 rather
>>> than expanding proposal 2.  It also may be a bit easier for people to update their configuration
>>> files .
>> Ack. So it's now a tie:
>> Proposal 1: 1
>> Proposal 2: 1
> Oops, I forgot about Francis's vote. But we still have a tie, as he
> prefers proposal 3 :)
>
> Tomek
>
> _______________________________________________
> kea-dev mailing list
> kea-dev at lists.isc.org
> https://lists.isc.org/mailman/listinfo/kea-dev
First, let me commend Tomek for all the effort he has put into this and
harassment he has endured for it ;).

REQUIREMENTS:

You have two paragraphs which begin with "Client classification can be a
very complex feature. Due to scheduling constraints...". I don't think
you need em both ;)

The section called "Design Assumptions" should be called "Requirements"

G7. This is an implementation constraint, not a requirement.

G3. I think this should be expanded similar to what Stephen had in the
discussion document, where he stated the search path:

    "The search path for an option value is then:

    Subnet class-option-data matching the client class
    Subnet option-data
    Global class-option-data matching the client class
    Global option-data"

We also need to state what happens if a client matches more than one
class and they both specify the same option.  Even if we say it is
arbitrary.

G10. Requirements should be stated as positives not negatives.

    "The system MUST support an arbitrary number of class defintions."

====================================================================

Design:

Configuration:
--------------

Actually I like a hybrid of Proposal #2 and #3. 

Proposal 2 makes it convenient, clear,  and visually compact when
defining the classes themselves.  I imagine this  would end up covering
a good deal of the use cases.

I think we do want the ability to specify alternate or additional class
options for a given class on a per subnet basis, which both  options 1
and 3 provide.

I tried both #1 and #3  in a test configuration and #1 was a bit uglier
to read and write than #3, and I think #3 will be less effort to
implement. Here's what the hybrid might look like:


"Dhcp4": {

    # Using Proposal #2 to include options within the class definition

    "client-classes": [
     {
        "name": "MICROSOFT",
        "test": "vendor-class-identifier == 'MSFT'",
        "option-data": [
        {
            "name": "some-option"
            "data": 100
        },
        {
            "name": "another-option"
            "data": "XXX"
        }]
     }],


    # Using Proposal #3 to add more options within subnet

    "subnet4": [
    {  
        "subnet": "192.0.2.0/24",
        "pools": [ { "pool": "192.0.2.1 - 192.0.2.200" } ]

        "option-data": [
        {
            "name": "domain-name-servers",
            "data": "192.0.2.1, 192.0.2.2"
        },
        {
            "class": "MICROSOFT"
            "name": "third-option"
            "data": 777
        }]
    }],
} 

  I think for 1.0 we could probably limit it to Proposal #2 portion.

Evaluation:
------------


I believe the tokens should be stateless.
But rather than pass in a fixed number of tokens into evaluate(), I
believe we should pass in a "value" stack.  Each evaluate()
implemenation pops off as many values from the stack as it requires to
calculate its result.  If there are not enough on the stack that's an
error.  Otherwise it calculates the result and pushes onto the stack and
returns.  This allows an evaluate method to use an arbitrary number of
operands.  So some psuedo code to give you the gist:

// Evaluated values are stored as a stack of strings
typedef stl::stack<std::string> ValueStack;

// Method which evaluates an RPN stack:
bool evaluateRPNStack(RPN rpn) {
    ValueStack values;

    // iterate through all the tokens, eavluating each
    for each token on the RPN {
        token.evaluate(pkt, values);
    }

    // Last value pushed is the end result
    // Note if there's more than one that would also be an error
    bool res = values.pop() ? "true" : "false";
    return (res);
}


// Evaluate for string literal would look like this
void TokenString::evaluate (Pkt& pkt, ValueStack& values) {
    // Literals only push, nothing to pop
    values.push(toString());
}


// Evaluate for equality would like like this
void TokenEqual::evaluate (Pkt& pkt, ValueStack& values) {
    String op1 = values.pop();
    String opt2 = values.pop();
    if (op1 == op2)
        values.push("true");
    else
        values.push("false);
 
}