#5351 closed enhancement (complete)

Add "comment" property to configuration blocks

Reported by: jsumners Owned by: fdupont
Priority: medium Milestone: Kea1.4
Component: configuration Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 3
Total Hours: 19 Internal?: no

Description

As discussed in the email thread https://lists.isc.org/pipermail/kea-users/2017-August/001116.html , it would be nice to have a property, likely named "comment", that can be added to any configuration block which would be for the configuration writer to read and the parser to ignore. This would make it possible to have configuration files that are valid JSON from start to finish.

Subtickets

Change History (31)

comment:1 Changed 17 months ago by fdupont

It is a design related question:

  • for properties which belongs to objects we already have user contexts so it should be enough to generalize them (i.e. to accept them in more syntax elements)
  • JSON is not supposed to be written directly as the comma discussion showed so it is not important to have comments in the syntax

Note there is a medium solution: generalize user contexts *and* parse "comment" properties as "user-context": { "comment": xxx } ...

comment:2 follow-up: Changed 17 months ago by jsumners

What do you mean "JSON is not supposed to be written directly"? How else do you expect to write your configuration?

comment:3 in reply to: ↑ 2 Changed 17 months ago by fdupont

Replying to jsumners:

What do you mean "JSON is not supposed to be written directly"? How else do you expect to write your configuration?

=> JSON was designed to be produced by tools and not manually. This is why it has no comments, no trailing commas, etc. To summary it is far to be user friendly because this was not a design goal...

comment:4 Changed 17 months ago by jsumners

This ticket isn't about the design goals of JSON as a format. Nor is it a knock against JSON being chosen as the configuration format, or the ability to added standard comments to that configuration. This ticket is about adding a feature to Kea's configuration parser so that valid JSON can be written and understood by a human.

comment:5 Changed 17 months ago by tomek

There is some progress made with regards to this problem. #5350 introduces user context for subnets. Once it's merged (it's under review right now, so merge should happen in next couple days), contexts will be supported in subnets and pools. This is definitely not everywhere, but it's a step in the right direction.

comment:6 Changed 17 months ago by tomek

  • Component changed from Unclassified to configuration
  • Milestone changed from Kea-proposed to Kea 1.4

Moving this ticket to 1.4 as discussed on 2017-08-31 meeting.

comment:7 Changed 16 months ago by tomek

  • Milestone changed from Kea 1.4 to Kea1.4

Milestone renamed

comment:8 Changed 16 months ago by fdupont

To summary 3 things to do:

  • add user context everywhere
  • make comment an implicit user context
  • save (and get) user context in host databases

For the last item I suggest to make provision for options too

comment:9 Changed 15 months ago by fdupont

We need to store a JSON value in databases. I looked at standard binary encodings for JSON. Two are good, UBJSON and CBOR. IMHO we should use CBOR (RFC 7049).

comment:10 Changed 15 months ago by jsumners

All three supported databases natively support JSON:

Why would you not use the native db types? Doing so makes it a helluva lot more difficult to work with the database.

comment:11 Changed 15 months ago by fdupont

I believe we should split this ticket into a design ticket and implementation tickets.
This will make new ideas which need some work (e.g. the two previous comments) to get their own space.
For instance a tool which imports and exports JSON host reservations to/from database.

comment:12 Changed 14 months ago by fdupont

  • Owner set to fdupont
  • Status changed from new to accepted

comment:13 Changed 14 months ago by fdupont

Did the hardest phase:

  • hacked parser, toElement and prettyPrint to get a nice handling of user-context/comment
  • create a new base class to share code
  • applied it to shared-networks (they were valid only for subnets and pools, shared-networks were the obvious next point)

Note I didn't update the doc about the last change. In theory we can add it to anything which is a map with a reasonable number of entries in the syntax, and extend to d2 and agent...

comment:14 Changed 14 months ago by jsumners

Excellent.

comment:15 Changed 14 months ago by fdupont

Found a bug in SrvConfig::toElement() which outputs subnets in shared networks twice. Commit 353dcb74d0..b46747b23e on trac5351 branch. Note get_config_unittest.cc must be regenerated.

comment:16 Changed 14 months ago by fdupont

Previous fix lost reservations: new fix b46747b23e..b6a202af89

comment:17 Changed 14 months ago by fdupont

  • Owner changed from fdupont to UnAssigned
  • Status changed from accepted to reviewing

Done at the exception of host reservations (which can be stored in databases so require more work).
Supported today:

  • global scope
  • shared network (forgotten :-)
  • subnet (since 1.3)
  • pools (all kinds, since 1.3)
  • client classes
  • option def
  • option data

So at the exception of host reservations all list of maps which can get a high number of entries.

Even if it is not finished (cf host reservations) it is ready for a review...

comment:18 Changed 14 months ago by fdupont

Added interfaces config, d2 client, loggers, DHCPv6 server id, control socket (but d2 and agent syntax were not yet updated), and host reservations including stored in database.

Ready for review.

comment:19 Changed 14 months ago by tomek

  • Owner changed from UnAssigned to tomek

comment:20 follow-ups: Changed 13 months ago by tomek

  • Add Hours to Ticket changed from 0 to 12
  • Owner changed from tomek to fdupont
  • Total Hours changed from 0 to 12

Whew! Finally got through the changes.

First of all, thanks a lot for writing this code. It's a huge added
functionality. Thanks for doing that. Second, this is the last time
I'm reviewing a change that is so huge. The diff is over 26000 lines.
That's insane. Even after removing the autogenerated code (parsers,
get-config unittests, etc.) the diff is 6000 lines long. This
could have been easily split into smaller tickets.

I have corrected couple things in many places. Please pull and review.

There is some more work needed as pointed out below.

doc/examples/kea{4,6}/comments.json
bin/dhcp{4,6}/tests/parser_unittest.cc
I did review all of the changes and agree with great majority of them.
One thing I disagree is the part that aggregates multiple instances
of "comments" into an array. Multiple instances of the same name are
not allowed in JSON, so we should not encourage it. Therefore I
think we should not make any special code to do aggregations like
this. In fact, if you remove the code, you will be able to handle
comments.json the same way as other configs.

After that the comments.json should be uncommented in ParserTest?.file
in both v4 and v6 versions.

user_context_utils.cc
I don't like the fact that this code sometimes throws exceptions even
in positive path. But since I don't have any better solution to
propose, let's keep the code as is.

cc/user_context.h
What do you need empty contructors and destructors for?

The class description was too brief. I wrote a text. Please pull
and review.

cc/data.h
Please use camel notation for method names: combineSet instead of
combine_set.

dhcpsrv/srv_config.cc
This kind of change (skipping subnets that are part of shared
networks) should definitely be mentioned in the ChangeLog?. In fact,
it probably would be better as a separate ticket on its own.

The new code definitely needs some comments. I tried to put some
comments, but please check that I understood your intentions
correctly. If I understand correctly what the code is trying to do,
there's a bug. The first (around line 248) skips if a subnet
belongs to shared network. The code continues on to 251 only
if the subnet is not part of a network. Yet the adds it to the
sn_list. Why? I thought sn_list stands for shared networks list?

The same confusion applies to v6. If I misunderstood, please explain
what exactly is going on here. The best way to explain is to write
comments.

dhcpsrv/tests/*.cc
The tests for cfg_duid, cfg_iface, cfg_option_def, cfg_option,
client_class_def, d2_client, shared_networks_list_parser check if a
comment can be stored, but do not check if a general user-context
(with content other than comment) can be stored.

dhcpsrv/tests/generic_host_data-source_unittest.cc
I think the testUserContext should be split into v4 and v6 versions.

There should be a test that checks the backend will reject inserts
with user context being larger than 8K.

dhcpsrv/tests/generic_host_data-source_unittest.h

dhcpsrv/tests/host_unittest.cc
Thanks for writing the unparse test. It uses a comment, which is a
special case. This (or similar) test should also check whether other
information can be stored and retrieved properly in user context.

dhcpsrv/tests/{pgsql,mysql}_host_data_source_unittest.cc
There must be a test that checks user context with content other
than comment.

dhcpsrv/mysql_host_data_source.cc
is no a JSON => it not a JSON

Fixed that. Please pull.

What would happen if the user context was bigger than 8K? The
insert would truncate it, right? There should be a check somewhere
around line 1770.

Not really related to this ticket, but the @todo in line 1806 about
client-class can be removed, right?

dhcpsrv/pgsql_host_data_source.cc
There should be a check in insert queries for user-context being over
8K large.

dhcp6/dhcp6_parser.yy
Why did you use socket_user_context for control_socket_param (see line
1719), instead of using user_context directly?

dhcp6/tests/config_parser_unittest.cc
The config used in Dhcp6ParserTest.comments should follow what
other tests do - put the config in PARSER_CONFIGS table rather
than in the body.

The #if 0 chunk of code at the end should be removed. I did that.

I don't understand why you changed the tests to use
ControlledDhcpv6Srv rather than Dhcpv6Srv as it used to be.
I reverted the change and tests pass just fine.

Please pull and review.

dhcp4/dhcp4_parser.yy
Why did you use control_socket_user_context and control_socket_comment
instead of user_context and comment?

dhcp{4,6}/tests/shared_network_unittest.cc
The Dhcpv{4,6}SharedNetworkTest?.parse should have some additional
comments. (why using configure from utils is not ok? What's going on
in the test?)

dhcp4/tests/confg_parser_unittest.cc
The same comments as for v6:

  • please move the config to PARSER_CONFIGS
  • why do you need ControlledDhcpv4Srv instead of Dhcpv4Srv?
  • please remove the #if 0 code at the end

shared/dtabase/scripts/*
I disagree with the versioning number. This is incompatible change,
so the version should be bumped up to 6.0 for mysql and to 4.0 for
postgres. Also, note that there is a script 3.2 on master for pgsql
and Marcin told me that this upgrade has been delivered to a customer,
so the upgrade script should be 3.2 to 4.0 (or to 3.3 if you have
good arguments why major version should not be bumped).


This change absolutely needs a changelog entry. Here's my proposal:

13XX.	[func]		fdupont
	User-context and comments are now supported in many new scopes:
	global, shared network, subnet, host, duid, option, option
	defintion, client-class, d2-client, duid and interfaces.
	(Trac #5351, git tbd)

If you use this text, please make sure it didn't skip any scopes.


Ok, that's it. Once again, thank you for implementing this
functionality. The user context will be super useful for so many
things, including the Radius code that we're about to start
developing.

comment:21 in reply to: ↑ 20 ; follow-up: Changed 13 months ago by fdupont

Replying to tomek:

One thing I disagree is the part that aggregates multiple instances
of "comments" into an array. Multiple instances of the same name are
not allowed in JSON, so we should not encourage it. Therefore I
think we should not make any special code to do aggregations like
this. In fact, if you remove the code, you will be able to handle
comments.json the same way as other configs.

=> IMHO we need the ability to stack comments, i.e., instead
to have a new comment overwrites the previous one (this is
the behavior of the parser on multiple instances of the same name)
a new comment is appended to the previous one.

Now comments are syntactic sugar (BTW this means it will be never
possible to parse comments.json with a not modified parser) so
there are many ways to implement them:

  • a possibility is to always make them an array
  • another is to happen a counter to the name so one gets comment1, comment2, etc, entries in a similar way we already do for subnet IDs.
  • we can split user-context and comment(s).

It is a matter of taste between code to handle comments and
the easy of use: at the opposite of the current code you have
comments (better with an 's'?) must appear once and are an array
of strings (there is a parser rule of exactly this). A bit harder
to use but this extreme proposal should not require syntactic sugar
at all.

comment:22 in reply to: ↑ 20 Changed 13 months ago by fdupont

Replying to tomek:

user_context_utils.cc
I don't like the fact that this code sometimes throws exceptions even
in positive path. But since I don't have any better solution to
propose, let's keep the code as is.

=> IMHO you have this concern because you consider exceptions
as error case handlers and not as a generic programming tool.

Now if you reread (when you'll have free time :-) this piece of code

you should look at this as a really fun thing...

Note if we step back on the comment as an auto-stacked item of
user-context this code will go away. (not a problem: the fun was
to write it (again)).

comment:23 in reply to: ↑ 20 Changed 13 months ago by fdupont

Replying to tomek:

cc/user_context.h
What do you need empty contructors and destructors for?

=> it was an attempt to avoid a strange bug with options.
IMHO they are automagically added by the compiler so
we can remove them and cross fingers (oops, rebuild and
check whether it still work)

comment:25 in reply to: ↑ 20 Changed 13 months ago by fdupont

Replying to tomek:

dhcpsrv/tests/*.cc
The tests for cfg_duid, cfg_iface, cfg_option_def, cfg_option,
client_class_def, d2_client, shared_networks_list_parser check if a
comment can be stored, but do not check if a general user-context
(with content other than comment) can be stored.

=> I leave this and further comments to after a decision is taken
about the point we disagree so we have to discuss and decide:

  • comment vs comments
  • comments inside user-context vs its own element (with both, i.e., I propose to keep the new user-context code without the combine_set/combineSet and to clone it in a new comment one)
  • auto-stack/syntactic sugar vs optional array of strings

Take care that these choices are *not* independent.

With the most drastic options it should give a similar amount of
code but simpler so quicker to review.

comment:26 in reply to: ↑ 24 Changed 13 months ago by fdupont

Replying to fdupont:

Replying to tomek:

dhcpsrv/srv_config.cc
This kind of change (skipping subnets that are part of shared
networks) should definitely be mentioned in the ChangeLog?. In fact,
it probably would be better as a separate ticket on its own.

=> fully agree. I took care to have the bug fix in separated commits
so it should be easy. And it will be a good occasion to add C++ code
comments to explain the logic which was reworked 3 times so
as you complained is now far to be clear.

comment:27 in reply to: ↑ 21 Changed 13 months ago by tomek

Replying to fdupont:

=> IMHO we need the ability to stack comments, i.e., instead
to have a new comment overwrites the previous one (this is
the behavior of the parser on multiple instances of the same name)
a new comment is appended to the previous one.

Why do you need this for? People can put multi-line comments if they really want to.

comment:28 Changed 13 months ago by fdupont

Decision was taken to keep comment (without 's') as an entry in user context (so using syntactic sugar) but as a JSON string without magic stack or array.

comment:29 in reply to: ↑ 20 Changed 13 months ago by fdupont

  • Add Hours to Ticket changed from 12 to 16
  • Owner changed from fdupont to tomek
  • Total Hours changed from 12 to 16

Replying to tomek:

Whew! Finally got through the changes.

First of all, thanks a lot for writing this code. It's a huge added
functionality. Thanks for doing that. Second, this is the last time
I'm reviewing a change that is so huge. The diff is over 26000 lines.
That's insane. Even after removing the autogenerated code (parsers,
get-config unittests, etc.) the diff is 6000 lines long. This
could have been easily split into smaller tickets.

=> there is one child ticket (#5452) and 4 to come:

  • recode user context utils without exceptions (I know a good way to do this: it is in an annex of by PhD document)
  • handle buffer overflows in database interface
  • add unit test for duplicate detected by parsers (BTW there is a ticket about adding more duplicate checks

so it will be recycled/spread)

  • use the user context to track inheritance (it is possible there is a ticket about this from user complains).

There is some more work needed as pointed out below.

doc/examples/kea{4,6}/comments.json
bin/dhcp{4,6}/tests/parser_unittest.cc
I did review all of the changes and agree with great majority of them.
One thing I disagree is the part that aggregates multiple instances
of "comments" into an array.

=> drop the multiple entries idea but kept the syntactic sugar
for comment / user-context.

After that the comments.json should be uncommented in ParserTest?.file
in both v4 and v6 versions.

=> I didn't expect it worked well but in fact it works...

user_context_utils.cc
I don't like the fact that this code sometimes throws exceptions even
in positive path. But since I don't have any better solution to
propose, let's keep the code as is.

=> first ticket to come.

cc/user_context.h
What do you need empty contructors and destructors for?

=> removed them

The class description was too brief. I wrote a text. Please pull
and review.

=> I even added a short statement for the case the class is embedded
(vs derived).

cc/data.h
Please use camel notation for method names: combineSet instead of
combine_set.

=> removed combine and combine_set (but kept prettyPrint feature
for comment).

dhcpsrv/srv_config.cc
This kind of change (skipping subnets that are part of shared
networks) should definitely be mentioned in the ChangeLog?.
...

=> moved to #5452

dhcpsrv/tests/*.cc
The tests for cfg_duid, cfg_iface, cfg_option_def, cfg_option,
client_class_def, d2_client, shared_networks_list_parser check if a
comment can be stored, but do not check if a general user-context
(with content other than comment) can be stored.

=> I added a lot of comment and user-context tests,
please ask for the few I missed...

dhcpsrv/tests/generic_host_data-source_unittest.cc
I think the testUserContext should be split into v4 and v6 versions.

=> I didn't yet touch DB tests (not critical and take a long time
to run (20s for mysql per test step)).

There should be a test that checks the backend will reject inserts
with user context being larger than 8K.

=> IMHO it should be done for all the buffers which can
overflow. I think you agree that to put a large value (8K)
is not the proper solution...

What would happen if the user context was bigger than 8K? The
insert would truncate it, right? There should be a check somewhere
around line 1770.

=> line 1770 is about formatted option data.

Not really related to this ticket, but the @todo in line 1806 about
client-class can be removed, right?

=> removed

dhcp6/dhcp6_parser.yy
Why did you use socket_user_context for control_socket_param (see line
1719), instead of using user_context directly?

=> I thought it was not possible to add directly a user context in it
but finally there is no good reason to make a particular case so
now it is handled by the same rules.

dhcp6/tests/config_parser_unittest.cc
The config used in Dhcp6ParserTest.comments should follow what
other tests do - put the config in PARSER_CONFIGS table rather
than in the body.

=> I saw you did this. Note that the controlled DHCPv6 parser
is required or the control socket stuff makes the test to crash.

The #if 0 chunk of code at the end should be removed. I did that.

=> it was a reminder the logging user context should be checked
also in DHCPvX servers.

I don't understand why you changed the tests to use
ControlledDhcpv6Srv rather than Dhcpv6Srv as it used to be.
I reverted the change and tests pass just fine.

=> it crashed in openCommandSocket() referencing a null shared pointer
so I retablished the code.

dhcp{4,6}/tests/shared_network_unittest.cc
The Dhcpv{4,6}SharedNetworkTest?.parse should have some additional
comments. (why using configure from utils is not ok? What's going on
in the test?)

=> I should explain why (better than to add a comment saying
it is required even it worked well :-).

shared/dtabase/scripts/*
I disagree with the versioning number.

=> I asked about this to avoid exactly this problem.
I moved both schemas to next major with minor = 0.


This change absolutely needs a changelog entry. Here's my proposal:

13XX.	[func]		fdupont
	User-context and comment are now supported in many new scopes:
	global, shared network, subnet, host, duid, option, option
	definition, client-class, d2-client, duid and interfaces.
	(Trac #5351, git tbd)

If you use this text, please make sure it didn't skip any scopes.

=> it skipped and DHCPv6 has one more.

comment:30 follow-up: Changed 13 months ago by tomek

  • Add Hours to Ticket changed from 16 to 3
  • Owner changed from tomek to fdupont
  • Total Hours changed from 16 to 19

I have reviewed your changes and they're good. Please merge.

I don't think we need special code to check whether a comment is already present or not, simply call ptr->set("comment", ...); This is a minor thing and we can leave it as it is or handle that at some other occasion.

The code builds and unit-tests pass on Ubuntu 17.04 x64.

Please merge.

comment:31 in reply to: ↑ 30 Changed 13 months ago by fdupont

Replying to tomek:

I don't think we need special code to check whether a comment is already present or not, simply call ptr->set("comment", ...); This is a minor thing and we can leave it as it is or handle that at some other occasion.

=> I am opening a ticket for this point.

comment:32 Changed 13 months ago by fdupont

  • Resolution set to complete
  • Status changed from reviewing to closed

Merged. I added a second ChangeLog entry for #5452 as it was merged too. I opened a new ticket (#5492) for Cassandra ports so we have a few children...

Note: See TracTickets for help on using tickets.