Opened 6 years ago

Closed 5 years ago

#3399 closed enhancement (complete)

Modify Kea4 to read JSON file

Reported by: tomek Owned by: tomek
Priority: medium Milestone: Kea0.9
Component: dhcp4 Version:
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: 2
Total Hours: 40 Internal?: no

Description (last modified by tomek)

Currently Kea4 receives configuration over control session from BIND10 framework. This task is about having the ability to get configuration from a different source, i.e. read it from a JSON file.

For testing purposes, it would be beneficial if the old capability were still available, but disabled. Compilation option seems reasonable here.

In the long run, it would be good to have the capability to support multiple configuration backends, even if we decide to always support only one. We know that OEMs and power users have their own configuration backends, e.g. LDAP.

Subtickets

Change History (8)

comment:1 Changed 6 years ago by tomek

  • Component changed from Unclassified to dhcp4
  • Description modified (diff)

comment:2 Changed 5 years ago by tomek

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

comment:3 Changed 5 years ago by tomek

  • Owner changed from tomek to UnAssigned
  • Status changed from assigned to reviewing
  • Total Hours changed from 0 to 24

The code is now ready for review. Note that is follows exactly the same path
as #3400 did.

7XX.	[func]*		tomek
	b10-dhcp4: New parameter added to configure: --with-kea-config.
	It allows selecting configuration backend and accepts one of two
	values: BUNDY, which uses Bundy (former BIND10) framework as Kea
	0.8 did, or JSON, which reads configuration from a JSON file.
	(Trac #3399, git TBD)

comment:4 Changed 5 years ago by marcin

  • Owner changed from UnAssigned to marcin

comment:5 follow-up: Changed 5 years ago by marcin

  • Add Hours to Ticket changed from 0 to 6
  • Owner changed from marcin to tomek
  • Total Hours changed from 24 to 30

Reviewed commit af2b1fb41c4af86b8c1624952ffb468f8b966267ESC

ChangeLog?
I wonder if we should continue using ''b10-dhcp4'' as we are going to rename it soon. Maybe DHCPv4 server instead?

Also, suggest that it says: ''New parameter added to configure.ac'' to indicate that it is a parameter of the configuration script. Now it is unclear where this new parameter has been added.

doc/examples/kea4/several-subnets.json
doc/examples/kea4/single-subnet.json

Both json files were foo-bared, because you copied IPv6 configuration and basically left the descriptions refer to IPv6 parameters. For example:

  • there is no preferred lifetime in the DHCPv4 config
  • you reduced the number of subnets comparing to IPv6 subnets
  • etc.

I fixed them but you may want to have a look whether my changes are complete and generally ok.

src/bin/dhcp4/ctrl_dhcp4_srv.cc

Throughout the file you use the server_ static object directly. In processCommand you call getInstance(). Wouldn't it be better to call getInstance() everywhere?

In this file there appear to be 4 occurrences of the word ''callback''. I think it is relevant for bundy_controller but not necessarily for kea_controller where the processCommand is triggered from init or a signal handler (in the future). In this case, the use of ''callback'' is a little bit misleading.

processCommand: In the following error handling statement:

    if (!srv) {
        ConstElementPtr no_srv = isc::config::createAnswer(1,
          "Server object not initialized, can't process command '" +
          command + "'.");
        return (no_srv);
    }

it would be useful to print arguments apart from the command. The DEBUG message at the beginning of the method prints a command and argument but it will not be logged unless DEBUG mode is enabled.

The last answer should also have arguments included.

Also, it should be ''Server object not initialized: can't process command... ''

processConfig: suggest adding the debug message at the beginnig of the function (just like for the processCommand).

A nit: I think that instead of doing this:

        return (isc::config::createAnswer(1, "Failed to process configuration:"
                                          + string(ex.what())));

you could use the ostringstream just like you do for other function exit points. Maybe just declare a single stream in the function scope and use it for all answers being created.

What should happen with a D2 instance started at line 131 when sockets fail to open at line 144? Shouldn't it be stopped or actually reverted to the state that it was in before the reconfiguration?

src/bin/dhcp4/ctrl_dhcp4_srv.h
The class description should be updated to say that this class can serve multiple configuration backends, no just to establish the connection with the message queue. Also, is this true:

''... while Dhcpv4Srv should be used in embedded environments.''?

I thought that in embedded environments you will rather use the file based configuration, instead of BIND10 infrastructure, but you'll still need !ControlledDhcpv4Srv class for this.

return tag in init function is invalid. This function doesn't return anything.

processCommand: It would be useful to document the names of supported commands.

processConfig: Is the use of ''callback'' intentional? It is not really a callback for file based config parsers.

getInstance: Instead of putting a note tag, wouldn't it be better to use return tag and list possible values returned by this function with an indication when is returned what?

src/bin/dhcp4/dhcp4.dox
I fixed some typos in this file.

src/bin/dhcp4/kea_controller.cc
I wonder if you need to include:

  • dhcp/iface_mgr.h
  • util/buffer.h
  • cassert.
  • iostream
  • vector

And whether you need using namespace isc::util?

ControlledDhcpv4Srv::init: It is unclear to me why you return when the configuration result returned is NULL and you don't thrown an exception, as in the case of other errors.

src/bin/dhcp4/main.cc

Is command line option ''-s'' used for anything really? Currently, the standalone mode is turned on in compilation time. Also, it seems I don't need to use any option other than ''-c'' to use file based configuration. So, should ''-s'' be removed?

src/bin/dhcp4/tests/bundy_controller_unittest.cc

There are not tests for Bundy whatsoever. Is it because we will not support this backend or we don't have time for them? I believe it is fine to not spend time on this but perhaps some little commentary should be added to explain why this file is not taken care of.

src/bin/dhcp4/tests/kea_controller_unittest.cc

JSONFileBackendTest should have some documentation. This will be useful if we ever take the class out of the anonymous namespace into the header file with qualified namespaces.

The TEST_FILE is removed in the destructor. The writeFile function takes the file name as a parameter. So, since you always use the same file name why you just don't remove the file_name parameter from the writeFile function declaration and not always use TEST_FILE? This would guarantee that the appropriate file is always removed.

Since, you create the .json file for the test, I would recommend that you add .json file extensions into the list of CLEAN_FILES. This is just in case, the removal doesn't work for some reason - tests terminate in some unusual way and destructor is not called.

Incomplete comment in Line: 172:

    // And configure it using config without

DISABLED_loadAllConfigs: Does it need a ticket or not just yet? Also todo should be preceded by three slashes (not two).

comment:6 in reply to: ↑ 5 ; follow-up: Changed 5 years ago by tomek

  • Add Hours to Ticket changed from 6 to 4
  • Owner changed from tomek to marcin
  • Total Hours changed from 30 to 36

Replying to marcin:

Reviewed commit af2b1fb41c4af86b8c1624952ffb468f8b966267ESC

ChangeLog?
I wonder if we should continue using ''b10-dhcp4'' as we are going to rename it soon. Maybe DHCPv4 server instead?

Good point. Updated this entry (and the previous one).

Also, suggest that it says: ''New parameter added to configure.ac'' to indicate that it is a parameter of the configuration script. Now it is unclear where this new parameter has been added.

Updated.

doc/examples/kea4/several-subnets.json
doc/examples/kea4/single-subnet.json

Both json files were foo-bared, because you copied IPv6 configuration and basically left the descriptions refer to IPv6 parameters. For example:

  • there is no preferred lifetime in the DHCPv4 config
  • you reduced the number of subnets comparing to IPv6 subnets
  • etc.

I fixed them but you may want to have a look whether my changes are complete and generally ok.

Thanks. I've checked your changes and they look ok.

src/bin/dhcp4/ctrl_dhcp4_srv.cc

Throughout the file you use the server_ static object directly. In processCommand you call getInstance(). Wouldn't it be better to call getInstance() everywhere?

Updated to getInstance() where possible. In constructor and destructor, it has to be server_.

In this file there appear to be 4 occurrences of the word ''callback''. I think it is relevant for bundy_controller but not necessarily for kea_controller where the processCommand is triggered from init or a signal handler (in the future). In this case, the use of ''callback'' is a little bit misleading.

Updated all but one instance. In one place the function is called as a callback from IfaceMgr? when there's some data to be received over a socket.

processCommand: In the following error handling statement:

    if (!srv) {
        ConstElementPtr no_srv = isc::config::createAnswer(1,
          "Server object not initialized, can't process command '" +
          command + "'.");
        return (no_srv);
    }

it would be useful to print arguments apart from the command. The DEBUG message at the beginning of the method prints a command and argument but it will not be logged unless DEBUG mode is enabled.

Done.

The last answer should also have arguments included.

I'm not sure which one you have in mind. I've added printing arguments in several places. If that is not sufficient, please be more specific.

Also, it should be ''Server object not initialized: can't process command... ''

Coma was ok as it formed a full sentence, albeit one that is not easy to read. Updated.

processConfig: suggest adding the debug message at the beginnig of the function (just like for the processCommand).

Added.

A nit: I think that instead of doing this:

        return (isc::config::createAnswer(1, "Failed to process configuration:"
                                          + string(ex.what())));

you could use the ostringstream just like you do for other function exit points. Maybe just declare a single stream in the function scope and use it for all answers being created.

Done.

What should happen with a D2 instance started at line 131 when sockets fail to open at line 144? Shouldn't it be stopped or actually reverted to the state that it was in before the reconfiguration?

I don't really know, but that's beyond adding configuration read. Created ticket #3455 for this.

src/bin/dhcp4/ctrl_dhcp4_srv.h
The class description should be updated to say that this class can serve multiple configuration backends, no just to establish the connection with the message queue. Also, is this true:

''... while Dhcpv4Srv should be used in embedded environments.''?

Not anymore. Updated the text.

I thought that in embedded environments you will rather use the file based configuration, instead of BIND10 infrastructure, but you'll still need !ControlledDhcpv4Srv class for this.

You are right. I think this comment was just a leftover from time when we did't have configuration backends.

return tag in init function is invalid. This function doesn't return anything.

Fixed.

processCommand: It would be useful to document the names of supported commands.

I was a bit hesitant to add it, because we'll forget to update it. But added it anyway.

processConfig: Is the use of ''callback'' intentional? It is not really a callback for file based config parsers.

No, it isn't. Update.

getInstance: Instead of putting a note tag, wouldn't it be better to use return tag and list possible values returned by this function with an indication when is returned what?

It would be. Done as suggested.

src/bin/dhcp4/dhcp4.dox
I fixed some typos in this file.

Thanks.

src/bin/dhcp4/kea_controller.cc
I wonder if you need to include:

  • dhcp/iface_mgr.h
  • util/buffer.h
  • cassert.
  • iostream
  • vector

And whether you need using namespace isc::util?

Those and a lot of others. Removed.

ControlledDhcpv4Srv::init: It is unclear to me why you return when the configuration result returned is NULL and you don't thrown an exception, as in the case of other errors.

Updated. It was a leftover from time when I planned init() to actually return something. The
idea was that in some cases returning false means that the configuration failed, but it ok
to continue (e.g. specified file was missing, but the server was told that the config may
be created at a later time). Anyway, I decided to scrap the idea.

src/bin/dhcp4/main.cc

Is command line option ''-s'' used for anything really? Currently, the standalone mode is turned on in compilation time. Also, it seems I don't need to use any option other than ''-c'' to use file based configuration. So, should ''-s'' be removed?

This is a sad and convoluted story. Stand-alone mode was added, so the DHCP server could be
started without the whole framework. That used to be useful mostly for testing purposes.
The only place where it was used were python tests. Those tests are going away, along with
the framework. So it doesn't make sense to keep the stand-alone option. I've removed it
from both v4 and v6.

src/bin/dhcp4/tests/bundy_controller_unittest.cc

There are not tests for Bundy whatsoever. Is it because we will not support this backend or we don't have time for them? I believe it is fine to not spend time on this but perhaps some little commentary should be added to explain why this file is not taken care of.

Added comment. I don't want to expand it much further, because honestly speaking, we haven't
decided what's the fate of Bundy backend will be. Perhaps we'll keep it (just the bundy_controller.cc
file, while the framework will go). Another alternative is for it to be merged into Bundy repo
and maintained there (by Bundy team). The third alternative is that we could get rid of it
altogether once Forge tests are updated and Kea backend is fully functional. Finally, the
last possible way forward is that we could decide that there's a value of maintaining Bundy
framework after all and develop those tests. For the time being, it doesn't matter, so I
just added a small comment there.

src/bin/dhcp4/tests/kea_controller_unittest.cc

JSONFileBackendTest should have some documentation. This will be useful if we ever take the class out of the anonymous namespace into the header file with qualified namespaces.

Added basic docum

The TEST_FILE is removed in the destructor. The writeFile function takes the file name as a parameter. So, since you always use the same file name why you just don't remove the file_name parameter from the writeFile function declaration and not always use TEST_FILE? This would guarantee that the appropriate file is always removed.

Good suggested. Done.

Since, you create the .json file for the test, I would recommend that you add .json file extensions into the list of CLEAN_FILES. This is just in case, the removal doesn't work for some reason - tests terminate in some unusual way and destructor is not called.

Added.

Incomplete comment in Line: 172:

    // And configure it using config without

Fixed.

DISABLED_loadAllConfigs: Does it need a ticket or not just yet? Also todo should be preceded by three slashes (not two).

No, not yet. We are producing new tickets in depressing quantities. Updated the comment to three slashes.

comment:7 in reply to: ↑ 6 Changed 5 years ago by marcin

  • Add Hours to Ticket changed from 4 to 2
  • Owner changed from marcin to tomek
  • Total Hours changed from 36 to 38

Reviewed commit fe739cf85387e170c935cb9cfb92155a0efd31e8

Replying to tomek:

Replying to marcin:

Reviewed commit af2b1fb41c4af86b8c1624952ffb468f8b966267ESC

The last answer should also have arguments included.

I'm not sure which one you have in mind. I've added printing arguments in several places. If that is not sufficient, please be more specific.

Line 98, the following bit of code:

    } catch (const Exception& ex) {
        return (isc::config::createAnswer(1, "Error while processing command '"
                                          + command + "':" + ex.what()));
    }

It should print args.


What should happen with a D2 instance started at line 131 when sockets fail to open at line 144? Shouldn't it be stopped or actually reverted to the state that it was in before the reconfiguration?

I don't really know, but that's beyond adding configuration read. Created ticket #3455 for this.

Agreed it is out of scope. But, it should be resolved at some point. Thanks for submitting the ticket.

processCommand: It would be useful to document the names of supported commands.

I was a bit hesitant to add it, because we'll forget to update it. But added it anyway.

Thanks. IMHO, it brings a lot of value if you can read that from the doxygen instead of looking at the code, how the function can be used.

src/bin/dhcp4/tests/bundy_controller_unittest.cc

There are not tests for Bundy whatsoever. Is it because we will not support this backend or we don't have time for them? I believe it is fine to not spend time on this but perhaps some little commentary should be added to explain why this file is not taken care of.

Added comment. I don't want to expand it much further, because honestly speaking, we haven't
decided what's the fate of Bundy backend will be. Perhaps we'll keep it (just the bundy_controller.cc
file, while the framework will go). Another alternative is for it to be merged into Bundy repo
and maintained there (by Bundy team). The third alternative is that we could get rid of it
altogether once Forge tests are updated and Kea backend is fully functional. Finally, the
last possible way forward is that we could decide that there's a value of maintaining Bundy
framework after all and develop those tests. For the time being, it doesn't matter, so I
just added a small comment there.

Fair enough.

DISABLED_loadAllConfigs: Does it need a ticket or not just yet? Also todo should be preceded by three slashes (not two).

No, not yet. We are producing new tickets in depressing quantities. Updated the comment to three slashes.

Well, on one hand it is bad, on the other hand the fact that we create more and more tickets is a sign of a big progress that we're making. Only dead projects have no new tickets.

But, I am fine with a TODO.

I guess, the comment about the args printing is sufficiently minor to tell that I don't need to see this ticket again.

comment:8 Changed 5 years ago by tomek

  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 38 to 40

Thanks for the review. Exception log updated, changes merged and pushed, closing ticket.

Note: See TracTickets for help on using tickets.