BIND 10 #3151: PD: Configuration parsers much be able to parse PD configuration

[BIND 10 Development]

#3151: PD: Configuration parsers much be able to parse PD configuration
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:  tomek
                Type:  enhancement   |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcpconf      |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130918
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  0             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tmark):

 * owner:  tmark => tomek


Comment:

 Replying to [comment:5 tomek]:
 > '''src/bin/dhcp6/tests/test_data_files_config.h.in'''
 > Unless there's something you'd like to share about your side project
 that involved breakthrough in time travel, I think you should update the
 header to the current year, which is 2013. ;)
 >

 I hate it when I give away my secret projects.  This has been corrected.


 > '''src/bin/dhcp6/config_parser.cc'''
 > !PdPoolParser class should have a class comment. It would be very
 useful, if that comment could have a piece of configuration that specific
 class is handling e.g.
 > {{{
 > {
 >   "prefix": "2001:db8:1:02::",
 >   "prefix-len": 72,
 >   "delegated-len": 88
 > },
 > }}}

 It would be useful, but why start a trend?  I added the class comment
 (thought I had done that...) which includes an example.

 >
 > PdPoolListParser::build() checks if the list of pool definitions is
 empty and throws if it is. It is perfectly valid configuration to have
 only address pools specified, without any PD pools. I'm not sure whether
 the code or the comment should be updated.
 >

 Actually what it is testing is that the ConstElementPtr itself is not
 empty.
 That is not the same condition as an empty list.


 > So what happens if there is address-only configuration? The parser is
 not created at all? It is created, but an empty list passed?

 If a subnet has no "pd_pools" entry, this parser will not be created. If
 it is created but passed an empty list, it will simply produce no pools.

 >
 > '''src/bin/dhcp6/tests/config_parser_unittest.cc'''
 >
 > Dhcp6ParserTest.pdPoolBasics: the test should confirm that the last
 address in a pool is is ok (in words checking if 'prefix-len' was
 processed correctly). You can calculate it manually or use
 lastAddrInPrefix(prefix, prefix_len).
 >

 Done.


 > I'd like to see four additional tests. First, checking that it is
 possible to delegate the whole prefix (i.e. prefix-len == delegated-len).
 >The second one would be to have adress only configuration (some
 addresses, no prefixes).
 >The third one would be the opposite (no address, some prefixes).
 >The fourth would be to have the whole subnet delegated as prefixes.
 >
 > It is likely that the second test is already there, so just a comment
 added that address-only test doubles as a test that verifies that a prefix
 can remain not-configured.
 >

 The first and fourth tests I rolled into one new test,
 "subnetAndPrefixDelegated".  I think this tests what you are after.

 The second test does exist, in several places, so I added a comment above
 TEST_F(Dhcp7ParserTest, poolPrefixLen).

 The third test you asked for is verified by pdPoolBasics, so I added a
 comment for that as well.

 Lastly, I added an NA pool to the pdPoolList test, which demonstrates that
 a subnet may be a combination of pool types.

 If you have specific tests that are not covered by these, how about
 sending me the config snippets and I can add tests for them.


 > '''src/lib/dhcpsrv/tests/test_libraries.h'''
 > You modified its content and replaced /home/thomson/... with
 > /Users/tmark/... I'm not sure if I like that change :)
 > In fact, why they test_libraries.h file is in the repo at all?

 It should not be there at all.  I pulled a fresh copy of trac3151 and it
 isn't there, nor will git let me "rm" it.

 >
 > The changes look good. The code builds and unit-tests pass on Ubuntu.
 >
 > One important thing is missing: BIND10 User's Guide is not updated.
 Although there is a separate ticket for doing a full User's Guide update
 with all the explanations, for now adding a boilerplate would be
 sufficient. Please add a section to doc/guide/bind10-guide.xml, with a
 simple PD config and a @todo tag.
 >


 I added a new section, 18.2.4 for this.  It is bare bones and marked with
 a todo.

 > Please make sure that you tell Włodek when this ticket is merged, so he
 can start updating Forge tests to cover configuration.


 Ok

-- 
Ticket URL: <https://bind10.isc.org/ticket/3151#comment:6>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development