Opened 4 years ago

Closed 3 years ago

#5022 closed enhancement (complete)

Implement options support for pd-pool

Reported by: tomek Owned by: marcin
Priority: high Milestone: Kea1.2
Component: dhcp6 Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 24 Add Hours to Ticket: 2
Total Hours: 32 Internal?: no

Description

We need to provide the ability to define options on a PD-pool level. If it's convenient we may do so for all pool types, but to address the use case we're looking for, covering just the PD-pool is sufficient.

Subtickets

Change History (7)

comment:1 Changed 4 years ago by marcin

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

comment:2 Changed 3 years ago by marcin

  • Add Hours to Ticket changed from 0 to 24
  • Estimated Difficulty changed from 0 to 24
  • Owner changed from marcin to UnAssigned
  • Status changed from accepted to reviewing
  • Total Hours changed from 0 to 24

I have implemented the requested feature for DHCPv6 server. It supports specifying options for both address and prefix pools. There is one issue that I hope we can sort out by simply implementing #2280 (currently proposed ticket). This is a performance issue with searching for pools using address value.

Proposed !Changelog entry:

11XX.	[func]		marcin
	Extended DHCPv6 server to allow for specifying DHCP options
	on address and prefix pool levels.
	(Trac #5022, git abcd)

comment:3 Changed 3 years ago by tomek

  • Owner changed from UnAssigned to tomek

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

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

dhcp6.spec
Please provide item_description for the items you added.

On a related note we still need to decide what to do with .spec files.
In their current for they're unsuable. I think their format was
invented specifically for bind10. I'm not aware of any tool that
can load and process them in any meaningful way.

dhcp6_srv.cc
Todo in line 828: "This is has" requires rewording.

Also, in line 831: trie => tree

host_unittest.cc
I'm puzzled with the changes in Configuration 3. It does look
reasonable, but I don't see any tests that actually use it.
Did you forget to update testOverrideRequestedOptions() or perhaps
wanted to add a new test altogether? Or maybe the idea is to check
that the pool values can be overridden by host options? If that is
so, the test description should be updated.


Do you think it would be reasonable to update doc/examples/kea6/multiple-options.json?

It's an old config that demonstrates how to define more than one option in a subnet.
It used to be impressive couple years ago, but it would be nice to bring it up to date
with all the option related features we have: custom options, global/subnet/pool options.
But we don't have to do it in one go. Maybe adding pool options in this ticket would
be reasonable first step?


The code compiles and unit-tests pass on Ubuntu 16.04 x64.

Your ChangeLog? proposal looks ok.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 3 years ago by marcin

  • Owner changed from marcin to tomek

Replying to tomek:

dhcp6.spec
Please provide item_description for the items you added.

On a related note we still need to decide what to do with .spec files.
In their current for they're unsuable. I think their format was
invented specifically for bind10. I'm not aware of any tool that
can load and process them in any meaningful way.

I am not sure what to do. It is good to have a collection of all supported parameters in a single place, but as long as we don't use spec file in the configuration parser it is likely that the spec file will diverge from the actually supported parameters.

dhcp6_srv.cc
Todo in line 828: "This is has" requires rewording.

Fixed it.

Also, in line 831: trie => tree

Actually, I think that trie is correct, but I removed the entire todo, because ticket #2280 is under way and I'll surely forget to remove the todo when I merge #2280.

host_unittest.cc
I'm puzzled with the changes in Configuration 3. It does look
reasonable, but I don't see any tests that actually use it.
Did you forget to update testOverrideRequestedOptions() or perhaps
wanted to add a new test altogether? Or maybe the idea is to check
that the pool values can be overridden by host options? If that is
so, the test description should be updated.

Not sure what the problem is. The configuration 3 is used in the tests and the description of configuration 3 explains that it is used to check that host specific options take precedence. Could you have a look again?


Do you think it would be reasonable to update doc/examples/kea6/multiple-options.json?

It's an old config that demonstrates how to define more than one option in a subnet.
It used to be impressive couple years ago, but it would be nice to bring it up to date
with all the option related features we have: custom options, global/subnet/pool options.
But we don't have to do it in one go. Maybe adding pool options in this ticket would
be reasonable first step?

I added pool specific options and extended description slightly.


The code compiles and unit-tests pass on Ubuntu 16.04 x64.

Your ChangeLog? proposal looks ok.

Please re-review.

comment:6 in reply to: ↑ 5 Changed 3 years ago by tomek

  • Add Hours to Ticket changed from 6 to 2
  • Owner changed from tomek to marcin
  • Total Hours changed from 30 to 32

Thanks for your changes. They address all my comments, except this one.

Replying to marcin:

Replying to tomek:

host_unittest.cc
I'm puzzled with the changes in Configuration 3. It does look
reasonable, but I don't see any tests that actually use it.
Did you forget to update testOverrideRequestedOptions() or perhaps
wanted to add a new test altogether? Or maybe the idea is to check
that the pool values can be overridden by host options? If that is
so, the test description should be updated.

Not sure what the problem is. The configuration 3 is used in the tests and the description of configuration 3 explains that it is used to check that host specific options take precedence. Could you have a look again?

Sure. We are talking about src/bin/dhcp6/tests/host_unittest.cc:

There are 2 issues:

1) tests using testOverrideRequestedOptions (4 of them, starting around line 1220) mentioned
in their description "override subnet specfic options". I have updated those descriptions to
say "override subnet specific or pool specific options". Please pull. If you agree with my
changes, consider this issue addressed.

2) tests using testHostOnlyOptions. Let's take a look at the testHostOnlyOptions description around lines 506-507. It claims the client is expected to receive option if they're defined in host scope only. Looking at the actual code in line 944 and 950, it seems the code requests dns-servers and nis-servers option. Now, looking at the configuration 3, dns-servers is defined in host, pool and subnet, so the "only" part in the description and method names are incorrect. I didn't fix this, because I don't know whether you prefer to keep the code as is and fix the description or vice versa.

Anyway, that's a minor thing. If this description is still not clear or you consider this issue insignificant, please carry on and merge.

comment:7 Changed 3 years ago by marcin

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

Merged with commit e3b2785c79aedbb0c8af7468d61f6d61dafd2282.

Note: See TracTickets for help on using tickets.