Opened 6 years ago

Closed 5 years ago

#3401 closed enhancement (complete)

Modify D2 to read configuration from a JSON file

Reported by: tomek Owned by: tmark
Priority: medium Milestone: Kea0.9
Component: ~dhcp-ddns(obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: Add Hours to Ticket: 3
Total Hours: 32 Internal?: no

Description

Currently D2 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

Attachments (1)

full_cfg.json (4.1 KB) - added by tmark 5 years ago.
JSON configuration fie used for system testing D2

Download all attachments as: .zip

Change History (9)

comment:1 Changed 5 years ago by tmark

  • Component changed from DDNS to dhcp-ddns

comment:2 Changed 5 years ago by tmark

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

Changed 5 years ago by tmark

JSON configuration fie used for system testing D2

comment:3 Changed 5 years ago by tmark

  • Add Hours to Ticket changed from 0 to 20
  • Estimated Difficulty 0 deleted
  • Owner changed from tmark to UnAssigned
  • Status changed from assigned to reviewing
  • Total Hours changed from 0 to 20

The configuration switch --with-kea-config, now selects between
two versions of D2Controller:

  1. One which must run as a BUNDY module and is implemented in

bundy_d2_controller.(h/cc)

All of the BIND10 support was extracted from DControllerBase and moved
into this version of D2Controller.

This controller is tested in tests/bundy_d2_controller_unittests.cc

  1. One that runs as a stand alone executable which must be supplied

with a configuration file via the command line and is implemented in
d2_controller.(h/cc).

This version of D2Controller is nearly identical the original.
DControllerBase supports configuration from file.

This controller is tested in tests/d2_controller_unittests.cc

DControllerBase now inherits from Daemon which keeps it in step with
K4 and K6.

The stand-alone mode flag has been removed from all controllers.

I verified that unit tests pass with both controllers and valgrind
reports them as clean.

In addition I conducted some basic systems tests with both controllers.
First I tested D2 running as a BIND10 module. It started, stopped,
reconfigured normally. With K4 and K6 also running as BIND10 modules, D2
received and processed NCRs correctly.

Then using a json text configuration file (see attached file full_cfg.json),
I was able to launch D2 stand-alone and it received and processed requests from
K4 and K6 (running under BIND10) correctly.

(Note that I have not verified full_cfg.json with either K4 or K6)

7XX.	[func]		tmark
	b10-dhcp-ddns: New parameter added to configure: --with-kea-config.
	It allows selecting the 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 text file.
	(Trac #3401, git TBD)

This ticket also addresses 3404.

comment:4 Changed 5 years ago by marcin

  • Owner changed from UnAssigned to marcin

comment:5 Changed 5 years ago by marcin

  • Add Hours to Ticket changed from 20 to 4
  • Owner changed from marcin to tmark
  • Total Hours changed from 20 to 24

It seems to me that we'll have to processes to use the same class hierachy. The use of Deamon in the DControllerBase is ''artificial''. But, it doesn't mean that you have to change anything. Just an observation.

Currently the Developer's guide for the DHCPv4 and DHCPv6 server's contains the section describing the --with-kea-config. I am not sure that the full text has to be repeated for each module. But, since there is a section about the switchable configuration backend (including motivation etc.) maybe the D2-specific text should refer to the section about configuration backends.

My cppcheck doesn't like this:

src/bin/d2/tests/d_test_stubs.h:450: check_fail: Function parameter 'module_name' should be passed by reference. (performance,passedByValue)

ChangeLog
Suggest that you don't use ''b10-dhcp-ddns'' because we're going to rename it soon. Maybe dhcp-ddns?

Also, I suggest replacing ''configure'' with ''configure.ac'' so as it is clear where exactly this parameter has been added.

src/bin/d2/d2_controller.cc
D2Controller::instance: I think that the comment in line 33 is not relevant for the file-based configuration: ''The base class must have access to the singleton in order to use it within BIND10 static function callbacks.''

I think this is more appropriate for the Bundy controller where the reconfiguration is signalled through the callback invocation.

src/bin/d2/d_controller.cc
Why is the getConfigFileName needed? Can't you just use the getConfigFile() method declared in the Daemon? Like this:

    try {
        std::string config_file = getConfigFile();
        if (config_file.empty()) {
    ...

src/bin/d2/d_controller.h
Question: configHandler and commandHandler are currently unused in the d_controller.cc and d2_controller.cc. Were they left in the base class because they will be used to update configuration when the hangup signal is received? Since there is a new configFromFile function, maybe not. So, why are these functions left in the base class?

Apparently not related to this ticket but, the following comment:

/// @brief DControllerBase launch exit status values.  Upon service shutdown
/// normal or otherwise, the Controller's launch method will return one of
/// these values.

is hanging in the air and there are no values defined in this header that it talks about.

Why values returned by the getAppName and getBinName are const? They are returned by value so they are copy constructed anyway. The copy doesn't need to be const. Or, they were meant to be returned as const reference?

src/bin/d2/tests/d2_test.py
Are you planning to write any additional tests for the server startup and reconfiguration (using SIGHUP) with the ticket that implements reconfiguration? The current unit tests cover some negative test scenarios but it would be useful to have some tests that actually start the process and make sure it gets configured?

comment:6 Changed 5 years ago by tmark

  • Owner changed from tmark to marcin
  • Total Hours changed from 24 to 28

Replying to marcin:

It seems to me that we'll have to processes to use the same class hierachy. The use of Deamon in the DControllerBase is ''artificial''. But, it doesn't mean that you have to change anything. Just an observation.

It is artificial, but I did it to at least try to keep them pointed in the
same general direction.

Currently the Developer's guide for the DHCPv4 and DHCPv6 server's contains the section describing the --with-kea-config. I am not sure that the full text has to be repeated for each module. But, since there is a section about the switchable configuration backend (including motivation etc.) maybe the D2-specific text should refer to the section about configuration backends.

I have added discussion about the switch, as well as discussion and a few affected diagrams.

My cppcheck doesn't like this:

src/bin/d2/tests/d_test_stubs.h:450: check_fail: Function parameter 'module_name' should be passed by reference. (performance,passedByValue)

Interesting. I have changed it but my cppcheck doesn't catch this.

ChangeLog
Suggest that you don't use ''b10-dhcp-ddns'' because we're going to rename it soon. Maybe dhcp-ddns?

Also, I suggest replacing ''configure'' with ''configure.ac'' so as it is clear where exactly this parameter has been added.

How this:

    DHCP-DDNS: New parameter added to configure.ac: --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.

src/bin/d2/d2_controller.cc
D2Controller::instance: I think that the comment in line 33 is not relevant for the file-based configuration: ''The base class must have access to the singleton in order to use it within BIND10 static function callbacks.''

I have removed it.

I think this is more appropriate for the Bundy controller where the reconfiguration is signalled through the callback invocation.

src/bin/d2/d_controller.cc
Why is the getConfigFileName needed? Can't you just use the getConfigFile() method declared in the Daemon? Like this:

    try {
        std::string config_file = getConfigFile();
        if (config_file.empty()) {
    ...

It isn't necessary. I preceded my inheriting from Daemon and so I simply
altered it's implementation. BTW, the Daemon method should have been called
getConfigFileName() as it returns the name of the file, not a open file stream
file description. Just my two cents.

I have removed the wrapper method.

src/bin/d2/d_controller.h
Question: configHandler and commandHandler are currently unused in the d_controller.cc and d2_controller.cc. Were they left in the base class because they will be used to update configuration when the hangup signal is received? Since there is a new configFromFile function, maybe not. So, why are these functions left in the base class?

Mostly I left them as a matter of convenience. I have removed them.

Apparently not related to this ticket but, the following comment:

/// @brief DControllerBase launch exit status values.  Upon service shutdown
/// normal or otherwise, the Controller's launch method will return one of
/// these values.

is hanging in the air and there are no values defined in this header that it talks about.

How odd. I deleted the comment. Maybe it was cut and paste error.

Why values returned by the getAppName and getBinName are const? They are returned by value so they are copy constructed anyway. The copy doesn't need to be const. Or, they were meant to be returned as const reference?

I just got const happy.

src/bin/d2/tests/d2_test.py
Are you planning to write any additional tests for the server startup and reconfiguration (using SIGHUP) with the ticket that implements reconfiguration? The current unit tests cover some negative test scenarios but it would be useful to have some tests that actually start the process and make sure it gets configured?

I was not planning to add any python tests as we are doing away with them.

I am planning to follow your lead and add shell tests for testing signals once
I do 3407.

As for valid config file testing, the D2ControllerTest.launchNormalShutdown,
does test a valid D2 file via the launch method. If you look at D2's main()
you will see it does virtually nothing other than pass the command line
arguments into controller's launch() method.

comment:7 Changed 5 years ago by marcin

  • Add Hours to Ticket changed from 4 to 1
  • Owner changed from marcin to tmark
  • Total Hours changed from 28 to 29

Reviewed commit 2c0ab179a50a47b7dcc38a9714a86faf7078a39a

Changes are good, except that the images still contain removed getConfigFileName function.

As for the name of the Daemon::getConfigFile, I just tried to keep the name short. I think that goes along with your preference to keep names short? Ok, I am teasing.... :)

I fixed a couple of typos and formatting issues in the d2.dox, so please pull my changes from the branch.

If you concur that the images have to be corrected, please do and merge the code. I don't have to see it again.

comment:8 Changed 5 years ago by tmark

  • Add Hours to Ticket changed from 1 to 3
  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 29 to 32

Changes merged with git 004f6cec32e7454961ef0ca9744028b8d0751e4d.

I overlooked added the static ModuleSession? handlers to the BUNDY version of D2Controller
after removing them from DControllerBase. This was corrected after the merge (but prior to
its push) with git 8e69209caafc81041229f3d9601599f3d98fc86e.

ChangeLog entry 791 added.

Note: See TracTickets for help on using tickets.