Opened 11 months ago

Closed 7 months ago

#5087 closed enhancement (complete)

The `domain-search` option should accept CSV format

Reported by: jsumners Owned by: fdupont
Priority: medium Milestone: Kea1.2-final
Component: dhcp4 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: 2
Total Hours: 10 Internal?: no

Description

Currently, the domain-search option is a binary only option. I understand the actual option is more complicated than a list of strings. But that doesn't matter to people just trying to write the configuration file. DHCPD does not require binary input; it does the translation from a comma separated list of domains.

Subtickets

Change History (12)

comment:1 follow-up: Changed 9 months ago by fdupont

  • Milestone changed from Kea1.x to Kea-proposed

Submit it again to triage from Kea1.x (so should go to Kea1.2 or back to Kea1.x).

comment:2 in reply to: ↑ 1 ; follow-up: Changed 9 months ago by jsumners

Replying to fdupont:

Submit it again to triage from Kea1.x (so should go to Kea1.2 or back to Kea1.x).

Are you asking me to re-create this issue?

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

Replying to jsumners:

Replying to fdupont:

Submit it again to triage from Kea1.x (so should go to Kea1.2 or back to Kea1.x).

Are you asking me to re-create this issue?

=> no: each week we go through kea-proposed tickets (in general new ticket) to assign a milestone to them. Here #5087 was in Kea1.x so not in the current milestone (Kea1.2) so I've just given a chance to it to be considered for Kea1.2 or 1.3, i.e. likely better (so addressed sooner) than it was.

comment:4 Changed 9 months ago by hschempf

  • Milestone changed from Kea-proposed to Kea1.2-final

Per 9 Mar team meeting, accept this in 1.2-final

comment:5 Changed 8 months ago by fdupont

Note at least an unit test must use DNS compression.

comment:6 Changed 7 months ago by fdupont

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

comment:7 Changed 7 months ago by fdupont

  • Add Hours to Ticket changed from 0 to 8
  • Owner changed from fdupont to UnAssigned
  • Status changed from accepted to assigned

Harder than expected because the code did not handle compressed FQDN lists. BTW currently they are handled only for domain-search but by removing the code check it should work for all DHCPv4 options (ISC DHCP defines another compressed FQDN list). For DHCPv6 the RFC 3315 specifies a MUST NOT for compression.

Ready for review.

comment:8 Changed 7 months ago by tomek

  • Owner changed from UnAssigned to Unassigned
  • Status changed from assigned to reviewing

Since it's ready for review, moving to review state.

comment:9 Changed 7 months ago by tomek

  • Owner changed from Unassigned to tomek

comment:10 follow-up: Changed 7 months ago by tomek

  • Add Hours to Ticket changed from 8 to 2
  • Owner changed from tomek to fdupont
  • Total Hours changed from 0 to 10

option_definition.cc
Line 789 uses &data[0]. What would happen if truncated option
(just option code and length set to 0) was received? data
would be initialized to a vector of zero length. I added
a check. Please pull and review.

libdhcp++_unittest.cc
The test you added covers the functionality, but I think those
3 cases should be split to separate tests. I did the change,
please pull and review.

Your changes are good, but I think we need an example of a config
file. I updated doc/examples/kea4/multiple-options.json. Please
pull and review.

There is one outstanding problem here. I have split the tests and
the one that is dedicated to testing compressed FQDN is failing.
Perhaps I don't understand how this is supposed to work, but take
a look at LibDhcpTest?.fqdnListCompressed. It uses 26bytes long
compressed list. Yet when names->len() is called it returns 40,
as if the names were uncompressed. Is this ok? If it is, please
update test test. If it's not, please fix the code to actually
keep the compression. If it's a lot of work, you can simply disable
the check for now and add /@todo: in the code.

This requires a changelog. Here's my proposal:

12XX.	[func]		fdupont
	DHCPv4 domain-search option can now be defined using
	comma separated values.
	(Trac #5087, git tbd)

comment:11 in reply to: ↑ 10 Changed 7 months ago by fdupont

Replying to tomek:

option_definition.cc
Line 789 uses &data[0]. What would happen if truncated option
(just option code and length set to 0) was received? data
would be initialized to a vector of zero length. I added
a check. Please pull and review.

=> I agree (usually there is the kind of things I report during reviews :-).
BTW if the RFC 3646 is not explicit about empty domain-search options
ISC DHCP code rejects them (cf check_domain_name_list()
in the client code which mainly protects against injection of
junk values to shell scripts).

libdhcp++_unittest.cc
The test you added covers the functionality, but I think those
3 cases should be split to separate tests. I did the change,
please pull and review.

=> I am afraid that each test alone is harder to understand
(and see below).

Your changes are good, but I think we need an example of a config
file. I updated doc/examples/kea4/multiple-options.json. Please
pull and review.

=> I believed I did it but obviously I forgot it. Of course it should
be in examples!

There is one outstanding problem here. I have split the tests and
the one that is dedicated to testing compressed FQDN is failing.
Perhaps I don't understand how this is supposed to work, but take
a look at LibDhcpTest?.fqdnListCompressed. It uses 26bytes long
compressed list. Yet when names->len() is called it returns 40,
as if the names were uncompressed. Is this ok?

=> yes, the compressed test uses the same list than the first /
uncompressed one so the length must be the same
(and if the uncompression code is called directly a memcmp
should return 0).

If it is, please update test test.

=> done. I added an assert so if we change something the compiler
will remain us to update other tests.

=> I removed the getCode() == DHO_DOMAIN_SEARCH so
any DHCPv4 options (including user defined) which carry a
domain list will be uncompressed.

This requires a changelog. Here's my proposal:

12XX.	[func]		fdupont
	DHCPv4 domain-search option can now be defined using
	comma separated values.
	(Trac #5087, git tbd)

=> fine. IMHO there is nothing to say about compression handling
(it works automagically and BTW ISC DHCP fails to raise an error
if someone defines a DHCPv6 option with "Dc" (aka compressed
domain list) format (but the MA does!)).

comment:12 Changed 7 months ago by fdupont

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

Merged. Closing.

Note: See TracTickets for help on using tickets.