[kea-dev] Client classification design

[Tomek Mrugalski]

Suggested items that were addressed are removed for clarity.

On 22.10.2015 12:30, Marcin Siodelski wrote:
> "Our current (already written) implementation... " Suggest adding a
> timestamp because as the time goes by this will refer to the past
> situation.
Added Sep. 2015. It's late October already, but the classification code
hasn't changed in more than a year. It's somewhat likely that first new
code will be merged before end of Oct, so it would be a bit ambiguous.
Hence past month.

> G.3 - It would be clearer to say "DHCP Options returned to the
> client are the combination of global options, subnet specific options
> and those specific to the classes that the client belongs to"
>
> I also think for G.3 that it should say something about
> class-specific options taking precedence over global and subnet
> specific options.
Updated. There's a separate section that discusses this matter further.

> G.5 - This requirement is unclear as it stands. In particular 
> "expression" is ambiguous. If it was explained well enough, the
> second part of this require wouldn't be needed. Maybe add something
> that expression in general  consists of several conditions with bool
> logic applied in-between. Or something like that.
Reworded. If you're still unhappy, please provide a text.

> G.7. - I don't understand why the requirement is repeated in parens
> and what "can apply" means.
There was something lost here. There is no "can apply" on neither the
requirments or design pages.

> G.8 - It is going to be hard to measure "minimal dependencies". I 
> recommend you simply remove the part about minimal dependencies. I
> guess nobody would add dependencies that are not required by the
> library implementation.
I disagree. People may try to push this thing into lidhcpsrv, because
that's where it will be used first. I'd like to keep that requirement.

> G.10. - I suggest rewriting it to say that "The administrator must
> be able to define arbitrary number of classes". I also suggest
> removing the part about issuing warning etc. I don't think we should
> even consider this warning at this point, because it is hard to
> determine upfront what number of classes is already worth a warning.
> I'd leave it up to the administrator of a system.
The text says "we may consider". That doesn't mean we will do it. It
will be a good sanity check against people who would come up with ideas
like defining an unique class for each subscriber.

> E.1, E.2. - I suggest that you expand on "extracting option values".
> Is it to say: "Extracting full option payload as an input for
> expression"? As opposed to extracting individual option fields?
Added clarification.

> E.3. - I think this requirement could be clearer as to how the 
> expression would look like for the option with multiple fields,
> would this be comma separated format or something else. This may be
> considered implementation detail really but I rise this question
> because you used example of "MSFT" string which is normally used in
> the substring operation context. Simply don't know if this
> requirement equally applies to the case of equality operator.
The text is clear enough in my opinion. We should not deliberate any
further what a constant string means.

> E.5. I presume the equality operator is supported in the sense that
> you can compare the payload of the whole option to the string or
> hexstring. Correct?
It is supported in the sense that it compares two evaluated values (two
strings), no matter where those values (strings) came from.

> I am confused with the distinction between the "General" and 
> "Functionality" requirements. Although, it is not a big deal because
> the essence is in the requirements themselves but grouping into
> distinct sets may make requirements clearer or may make them actually
> harder to read.
Merged those two lists.

> I don't see anything mentioned explicitly about sub-options here. Do
> we need additional requirements to cover the classification based on 
> sub-options?
Not in phase 1.

> I see further on that you're thinking about using the output of
> toText() function. But there are some issues with this output being
> compared to a fixed string, which I mention below. There is also an
> issue that if someone creates a configuration file which would use
> the equality operator, the future change in the toString()/toText()
> implementation would make his configuration incompatible.
The alternative is to implement toString() for all option formats we
currently have in 1.0. Good luck with that. We'll cover the possible
configuration incompatibilities in the release notes.

> A couple of comments about the Design document follow.
> 
> "Configuration" There are three proposals but the design doesn't
> include the final decision which of them will be implemented. I also
> think that the examples should reflect the fact that we're planning
> to start with implementation of global classes, rather than classes
> nested within the subnet.
It has one syntax now that has class definitions on a global level
(those can optionally include options that would apply to all subnets).
Also, options defined on subnet level can be extended can be class
specific (i.e. reserved for specific class).

> "Parsing"
> 
> I am not sure I understand that: "This list is not complete. We will
> likely need a dedicated class for extracting information from
> Vendor-Identifying Vendor Class (124) and Vendor-Identifying
> Vendor-Specific Information (125) options."
> 
> What list does it refer to? Also, why would you need additional
> class for those options? Aren't you planning to introduce a virtual
> toString() function which will be implemented within existing class
> hierarchy?
Clarified.

> "Interaction with Option classes" I am trying to understand the code
> that will be introduced into Option class and derived classes. So,
> you're planning to evaluate using the string value carried within the
> option, e.g. "MSFT". In the requirements you also mentioned that
> evaluation using the hex representation is possible. So, technically
> you're going to implement the toString() function within Option
> class, returning the string OR hex string? Would this be controlled
> using some sort of parameter passed to toString()? I presume it would
> be something like toString(const bool use_hex) ?
You're reading too much into this. The requirement states:
"Constant expression specified as hexstring MAY be supported.". Note
that it's a MAY for phase 1, so we'll likely skip it anyway. This will
only apply to constant strings. Instead of "MSFT" it could be possible
to write 0x4D534654.

> "Expected Tickets"
> 
> About task #4. After implementing this ticket all instances of the 
> classes derived from Option would return toText()? So this change
> would simply be to implement the following function in the Option
> class?
> 
> std::string Option::toString() const { return (toText()); }
> 
> Note that some specialized options would return data in different 
> formats and they also include indentation and new line characters. 
> Comparing such output of toText() for equality wouldn't probably do
> any good. It should work fine for substring, though.
Yes, that's the plan. If you have better idea that could be implemented
in the time constraints we have, please speak up.

> Another related question. How this will satisfy the requirement to 
> compare option against hex string?
That's easy. All operations are done on strings (we do not try to invent
strong typed language here. We sort of have that in ISC DHCP and it's
more than enough). Hexstring will be converted to normal string during
Token parsing, e.g. the configuration has 0x4D534654, which will be
parsed as 4 characters long constant string.

> About #5. Are we actually going to use any Yacc or Bison for 1.0?
Yes. With only == operator we could possibly get away without it. With
substring (which takes 3 arguments) there's no sane way to do it without
proper grammar definition. You could try to come up with some sort of
boost-based regexp parser that would not be extensible and would have to
be thrown away in 2 months when the real complexity planned for 1.1
comes in. That doesn't fall into my definition of sane plan, so I
rejected it.

> About #15. I don't have such a strong opinion about the update to 
> Developer's Guide. I agree we should aim for this but the world
> wouldn't collapse if we don't address it for 1.0. The world will
> collapse if we don't update the User Guide properly.
Oh no. We won't skip that one. It seems that we're doing everything here
by the book. We surely cannot skip such an important step as
documentation. It should be easy to do anyway. At this stage the design
is detailed enough that it should be mostly a copy-paste type of work.

Tomek