Opened 5 years ago

Closed 3 years ago

#3770 closed enhancement (complete)

Check Kea's configuration without starting the server

Reported by: nicolas.chaigneau Owned by: fdupont
Priority: medium Milestone: Kea1.2
Component: remote-management 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: 1
Total Hours: 7 Internal?: no

Description (last modified by tomek)

Is it possible to check that Kea’s configuration is valid, without actually trying to start the server ?

(if not, would it be possible to add this, perhaps with a new command line parameter to kea-dhcp4 for instance ?)

I think of this because, from my experience, it is very easy to botch the configuration. (For instance, a comma not removed at the end of a list. I’m doing this quite often.) That’s not really an issue in development: you notice it fails to start, the message displayed allows to precisely locate the issue. You just fix the configuration, and start again.

But in production, Kea will be run on several nodes of a cluster. The server will be restarted through the cluster.

If the configuration is incorrect, the cluster will not be happy, and it will be more difficult to figure out what went wrong and restore a nominal cluster situation.

So it would be really useful to be able to check a configuration, before actually try to start or reconfigure the server with this configuration.

Subtickets

Change History (19)

comment:1 Changed 5 years ago by hschempf

  • Milestone changed from Kea-proposed to Kea1.1

comment:2 Changed 4 years ago by tomek

  • Milestone changed from Kea1.1 to DHCP Outstanding Tasks

comment:3 Changed 4 years ago by tomek

  • Milestone changed from DHCP Outstanding Tasks to Outstanding Tasks

Milestone renamed

comment:4 Changed 4 years ago by tomek

  • Description modified (diff)
  • Milestone changed from Outstanding Tasks to Kea1.2

This request seems similar to what we have envisioned for test-config command. Let's move it to Kea1.2 milestone for evaluation.

comment:5 Changed 4 years ago by tomek

  • Component changed from Unclassified to configuration

comment:6 Changed 4 years ago by tomek

This proposal is partially overlapping with #3406. Both tickets should be done together.

comment:7 Changed 3 years ago by tomek

  • Component changed from configuration to remote-management

comment:8 Changed 3 years ago by fdupont

There are at least 3 levels:

  • JSON syntax error (e.g. extra comma)
  • DHCPv4/DHCPv6/DDNS syntax error (e.g. typo in a keyword)
  • semantic error (e.g. out of range value)

The first 2 are pretty easy now we have flex/bison. Note as the second includes the first IMHO it should be enough to propose the second only/at the first time.
The standard option name for this kind of feature is -t.

comment:9 Changed 3 years ago by fdupont

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

comment:10 Changed 3 years ago by fdupont

Did kea-dhcp6.

comment:11 Changed 3 years ago by fdupont

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

Did kea-dhcp4 too. D2 doesn't use a flex/bison parser too so I stop here.
Ready for review...

comment:12 Changed 3 years ago by tomek

  • Owner changed from UnAssigned to tomek

comment:13 Changed 3 years ago by fdupont

Possible dependency on #5110 i.e. there are 3 possible way to provide this feature to D2:

  • do it in this ticket
  • do it in #5110
  • open a followup ticket

Note the same question will apply to the agent too (but if with D2 the second option is not the best for the agent it will be).

comment:14 follow-up: Changed 3 years ago by tomek

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

Thanks for implemeting this ticket. Sorry I couldn't review it
earlier, but as you know I was busy with Milestone 1 work.

This code has been in review for quite a while. In particular, DHCPv4
and DHCPv6 configuration code has changed significantly. I decided
to rebase it to latest master. See trac3770_rebase branch.

dhcp6_srv_unittest.cc
Why did you remove "csv-format": false around line 2032?

src/bin/dhcp{4,6}/main.cc
The changes you did are a step towards the right solution, but they're
not sufficient. They cover only the bison phase, so would skip the
most important part: logic checking. The logic is the core reason why
people want to sanity check their configuration.

I thought it should be easy to do, so I went on and implemented those
checks. Please pull and review.

src/bin/dhcp{4,6}/dhcp{4,6}_process_tests.sh.in
I think the unit-test as written is not flexible enough. I refactored
it slightly and added one extra config file. As a result, we now
have 3 unit-tests rather than one. Please pull and review.

dhcp{4,6}-srv.xml
I think the description should extended. As written, it may suggest
that the server expect -t config.json, rather than -t -c config.json.
Furthermore, it should be clear what kind of checks are and not done.
I have proposed a text. Please pull and review.


You brought a good topic of D2 and agent sanity checking. I think
we should have separate tickets for them. Please create them in
kea-proposed. We will evaluate and assign on the next call as needed.

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

This change requires a changelog entry. If you don't have any better
text, here's my proposal:

12xx.	[func]		fdupont,tomek
	Command line option -t implementated for DHCPv4 and DHCPv6.  It
	allows configuration sanity checking. Note that not all parameters
	are completely checked. In particular, traffic sockets are not
	opened, control channel socket is not opened and hook libraries
	are not loaded.
	(Trac #3770, git tbd)
Last edited 3 years ago by tomek (previous) (diff)

comment:15 in reply to: ↑ 14 Changed 3 years ago by fdupont

Replying to tomek:

See trac3770_rebase branch.

=> got it.

dhcp6_srv_unittest.cc
Why did you remove "csv-format": false around line 2032?

=> I can't see this change: the only difference in this file is
the #5093 i.e. DUID length...

src/bin/dhcp{4,6}/main.cc
I thought it should be easy to do, so I went on and implemented those
checks. Please pull and review.

=> reading them they seem good.

src/bin/dhcp{4,6}/dhcp{4,6}_process_tests.sh.in
I think the unit-test as written is not flexible enough. I refactored
it slightly and added one extra config file. As a result, we now
have 3 unit-tests rather than one. Please pull and review.

=> as we now check more I agree about more tests.

dhcp{4,6}-srv.xml
I think the description should extended. As written, it may suggest
that the server expect -t config.json, rather than -t -c config.json.

=> ok for the -t file (vs -t -c file) change. BTW current text is
{{{Check the syntax of the configuration file and report the first
error if any.}}}. I can't see your proposal and the syntax of
no longer applies.

Furthermore, it should be clear what kind of checks are and not done.
I have proposed a text. Please pull and review.

=> can't get it?

You brought a good topic of D2 and agent sanity checking. I think
we should have separate tickets for them. Please create them in
kea-proposed. We will evaluate and assign on the next call as needed.

=> today there is only a change for kea-dhcp-ddns.xml?

This change requires a changelog entry. If you don't have any better
text, here's my proposal:

12xx.	[func]		fdupont,tomek
	Command line option -t implementated for DHCPv4 and DHCPv6.  It
	allows configuration sanity checking. Note that not all parameters
	are completely checked. In particular, traffic sockets are not
	opened, control channel socket is not opened and hook libraries
	are not loaded.
	(Trac #3770, git tbd)

=> looks good.

So we have 3 things:

  • the -t file
  • improve the doc
  • D2 and agent child ticket (#5152)

comment:16 Changed 3 years ago by fdupont

BTW now the logging is enabled we have a lot of spurious things logged on standard output. Of course interesting things (e.g. errors) are on the standard error so we have only to explain this point in the doc...
Note I built and ran tests on my macOS with success.

comment:17 follow-up: Changed 3 years ago by fdupont

  • Owner changed from fdupont to tomek

Looks good. Ready for a final review?

comment:18 in reply to: ↑ 17 ; follow-up: Changed 3 years ago by tomek

  • Add Hours to Ticket changed from 6 to 1
  • Owner changed from tomek to fdupont
  • Total Hours changed from 6 to 7

Replying to fdupont:

Looks good. Ready for a final review?

I pulled 74e2c1624f270e77bf0cf655b297460e61b8fc28. It built ok, but unit-tests failed. That was easy to fix (use -t config, rather than -t -c config). Also, User's Guide needed update.

With the fixed I did, the code builds and unit-tests pass on OS X 10.11.5.

Done both. Please pull and review. If the change is ok, please merge.

comment:19 in reply to: ↑ 18 Changed 3 years ago by fdupont

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

Replying to tomek:

Replying to fdupont:

Looks good. Ready for a final review?

I pulled 74e2c1624f270e77bf0cf655b297460e61b8fc28. It built ok, but unit-tests failed. That was easy to fix (use -t config, rather than -t -c config).

=> oops, I checked the command directly, ran unit tests command and forgot the shell tests.

Also, User's Guide needed update.

=> I agree. BTW #2688 should change where logs are sent so I put a note in this ticket.

With the fixed I did, the code builds and unit-tests pass on OS X 10.11.5.

Done both. Please pull and review. If the change is ok, please merge.

=> Merged. Closing.

Note: See TracTickets for help on using tickets.