Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#4096 closed enhancement (complete)

Implement configuration parsers for class information statements

Reported by: sar Owned by: tmark
Priority: high Milestone: Kea1.0-beta
Component: configuration Version: git
Keywords: classification Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket: 4095
Estimated Difficulty: 0 Add Hours to Ticket:
Total Hours: 44 Internal?: no

Description (last modified by tmark)

Parse class statements form either v4 or v6 serves to create the class information structures from 4095

Subtickets

Change History (17)

comment:1 Changed 4 years ago by sar

  • Component changed from dhcp to configuration

comment:2 Changed 4 years ago by tmark

  • Owner set to tmark
  • Status changed from new to assigned

comment:3 Changed 4 years ago by tmark

  • Add Hours to Ticket changed from 0 to 30
  • Description modified (diff)
  • Owner changed from tmark to Unassigned
  • Status changed from assigned to reviewing

Ticket is ready for review:

I'm sure I have overlooked things but the work to this point needs reviewing. It is all
functional, unit tested with valgrind.

Both servers will parse a "client-classes" element which occurs within their module scope.
In other words, it is a peer of lease-database, subnet<n>, etc...

This actually involved a good deal of work spread out over several commits:

  • ClientClassDef originally stored options in a OptionCollection when fact it needed

to be a CfgOption. This has been corrected.

  • Created new parser classes and unit tests:

ExpressionParser
ClientClassDefParser
ClientClassDefListParser

  • Added copy constructors and equality tools to ClientClassDef and ClientClassDictionary These are needed for configuration comparison
  • Added client class dictionary to SrvConfig
  • Added support for a client-classes element to kea-dhcp4
  • Added support for a client-classes element to kea-dhcp6

Note that ExpressionParser needs to integrate the use of the Bison parser, once
that's available.

This ticket only allows the servers to parse and maintain a global list of client class
definitions. Use of that list is beyond the scope of this ticket.

Thought needs to be given to how "defined" classes and "automatic vendor" classes
are going to handled.

The server spec files have been updated to reflect the new elements. The admin guide
has not been updated.

comment:4 Changed 4 years ago by fdupont

  • Owner changed from Unassigned to fdupont

comment:5 Changed 4 years ago by fdupont

  • Owner changed from fdupont to tmark

dhcp4 and dhpc6 config_parser_unittest.cc: an class -> a class (fixed in the branch, don't forget to pull it).

same files: configuration -> configuration

dhcpsrv/Makefile.am: bad position for libkea-eval.la (libraries MUST be in the strict dependency order).
BTW if you maintain the library order change in src/lib/Makefile.am (which BTW seems right) we have to fix this in all places where libkea-eval.la is linked.

client_class_def.h: missing and extra spaces, spelling. And a point which should have to be revisited for 1.1: if we add a in_class token and logical operations the order of class definitions will become important so a map won't be enough (if of course we don't decide classes are ordered by their name vs. by their positions in the config file).

client_class_def_parser.cc: the parser was merged so you can (should!) call parseString(). BTW note the eval parser will put a position (a bit different but not enough) too in its error string when it fails.

In dhcpsrv/parser/* the @todo refers to the Bison parser. IMHO (and if you keep such comments) it is better to say the eval parser as it is possible we switch one day to flex/bison to parse config files (I expect a flex/bison JSON parser to be pretty fast).

In dhcpsrv/tests/* now we have the eval parser so which should use it (or at least take it into account).

I didn't find something bad in the code, please update the eval parser related things.

comment:6 follow-up: Changed 4 years ago by fdupont

I don't know how empty class names are handled. I suggest:

  • add an empty class name case in client_class_def_parser_unittest.cc
  • check manually what happens and fix if needed

comment:7 follow-up: Changed 4 years ago by fdupont

A question related to #4097 (and #4098). In ClientClassDictionary? all elements can be assumed to have a not null ClientClassDefPtr?. What to do when this is checked to be wrong at runtime? E.g., skip the entry, log an error, raise Unexpected? (I implemented the first but IMHO the last one is better if it is really an impossible condition).

comment:8 in reply to: ↑ 6 ; follow-up: Changed 4 years ago by tmark

Replying to fdupont:

I don't know how empty class names are handled. I suggest:

  • add an empty class name case in client_class_def_parser_unittest.cc
  • check manually what happens and fix if needed

ClientClassDef cannot be instantiated with an empty class name. Its constructor will throw if you attempt it.

comment:9 in reply to: ↑ 7 ; follow-up: Changed 4 years ago by tmark

Replying to fdupont:

A question related to #4097 (and #4098). In ClientClassDictionary? all elements can be assumed to have a not null ClientClassDefPtr?. What to do when this is checked to be wrong at runtime? E.g., skip the entry, log an error, raise Unexpected? (I implemented the first but IMHO the last one is better if it is really an impossible condition).

ClientClassDictionary does not permit entries with an empty class pointer.

comment:10 Changed 4 years ago by tmark

Pursuant to your last two questions, these things are already verified in the unit tests in both client_class_def_unittest.cc and client_class_def_parser_unittest.cc

comment:11 in reply to: ↑ 8 ; follow-up: Changed 4 years ago by fdupont

Replying to tmark:

Replying to fdupont:

I don't know how empty class names are handled. I suggest:

  • add an empty class name case in client_class_def_parser_unittest.cc
  • check manually what happens and fix if needed

ClientClassDef cannot be instantiated with an empty class name. Its constructor will throw if you attempt it.

=> I know but I'd like to know if the error message is helpful or not.

comment:12 in reply to: ↑ 9 Changed 4 years ago by fdupont

Replying to tmark:

Replying to fdupont:

A question related to #4097 (and #4098). In ClientClassDictionary? all elements can be assumed to have a not null ClientClassDefPtr?. What to do when this is checked to be wrong at runtime? E.g., skip the entry, log an error, raise Unexpected? (I implemented the first but IMHO the last one is better if it is really an impossible condition).

ClientClassDictionary does not permit entries with an empty class pointer.

=> so to raise Unexpected seems the best.

comment:13 in reply to: ↑ 11 Changed 4 years ago by tmark

Replying to fdupont:

Replying to tmark:

Replying to fdupont:

I don't know how empty class names are handled. I suggest:

  • add an empty class name case in client_class_def_parser_unittest.cc
  • check manually what happens and fix if needed

ClientClassDef cannot be instantiated with an empty class name. Its constructor will throw if you attempt it.

=> I know but I'd like to know if the error message is helpful or not.

Ok, I misundertood your comment. First, there are already tests for this and the error
message itself is explicit:

There was already a test for a class with no name at all, "noClassName", in that case you will get an error message that says "Missing parameter 'name'" following by configuration position info.

I have added second test, "blankClassName", which attempts to create a class with name = "".
That error message is "ClientClassDef? name cannot be empty, followed by configuration position info.

comment:14 Changed 4 years ago by tmark

  • Owner changed from tmark to fdupont

With the following the ticket is ready for re-review

Replying to fdupont:

dhcp4 and dhpc6 config_parser_unittest.cc: an class -> a class (fixed in the branch, don't forget to pull it).

same files: configuration -> configuration

Thanks.

dhcpsrv/Makefile.am: bad position for libkea-eval.la (libraries MUST be in the strict dependency order).
BTW if you maintain the library order change in src/lib/Makefile.am (which BTW seems right) we have to fix this in all places where libkea-eval.la is linked.

I moved eval up in the dhcpsrv/Makefile.am and dhcpsrv/tests/Makefile.am. I believe
they are now correct.

client_class_def.h: missing and extra spaces, spelling. And a point which should have to be revisited for 1.1: if we add a in_class token and logical operations the order of class definitions will become important so a map won't be enough (if of course we don't decide classes are ordered by their name vs. by their positions in the config file).

I think for now, the map will suffice. We can address alternate ordering of classes if/when it arises. Since the dictionary is class, it will be pretty painless to extend it.

client_class_def_parser.cc: the parser was merged so you can (should!) call parseString(). BTW note the eval parser will put a position (a bit different but not enough) too in its error string when it fails.

Use of the parser has been integrated, and unit tests revamped accordingly.

In dhcpsrv/parser/* the @todo refers to the Bison parser. IMHO (and if you keep such comments) it is better to say the eval parser as it is possible we switch one day to flex/bison to parse config files (I expect a flex/bison JSON parser to be pretty fast).

The todo has been removed.

In dhcpsrv/tests/* now we have the eval parser so which should use it (or at least take it into account).

Done see above.

I suggest the following ChangeLog?

1xxx.   [func]     [tmark]
	kea-dhcp4 and kea-dhcp6 configuration parsing now supports
	the "client-classes" element for defining client classes.  Note
	the classes are not yet used in classification.
	(Trac #4096    git TBD)

comment:15 Changed 4 years ago by fdupont

  • Owner changed from fdupont to tmark

client_class_def_parser.h: there were @todo's and references to Bison. I fixed it in the branch as they are comments (and pushed the commit).

client_class_def_parser_unittest.cc: spurious extra lines 129 and 162 (fixed too as it looked like a trivial typo).

BTW the blankClassName error message is ClientClassDef name cannot be empty (<string>:1:2) so IMHO can be improved...

Ready to merge. As it augments the syntax I agree it should get a ChangeLog, perhaps without the last/note statement?

comment:16 Changed 4 years ago by tmark

  • Add Hours to Ticket 30 deleted
  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 24 to 44

Ticket merged with git d21fd6925983eb20f82029e3866652398ea5e5fe
Added ChangeLog entry 1051.

Note that this ticket actually included the work called for in #4099 and #4100. Oh well, somebody had to do it.

Ticket is closed.

comment:17 Changed 4 years ago by tomek

  • Milestone changed from Kea1.0 to Kea1.0-beta

Milestone renamed

Note: See TracTickets for help on using tickets.