BIND 10 #2957: DHCP-DDNS: Develop the DHCP DDNS configuration manager class

[BIND 10 Development]

#2957: DHCP-DDNS: Develop the DHCP DDNS configuration manager class
-------------------------------------+-------------------------------------
            Reporter:  stephen       |                        Owner:
                Type:  enhancement   |  stephen
            Priority:  medium        |                       Status:
           Component:  dhcp-ddns     |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130703
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tmark):

 * owner:  tmark => stephen


Comment:

 Replying to [comment:11 stephen]:


 In addition to the changes below, I replaced @TODO with @todo and
 corrected about a dozen doxygen issues.

 > Reviewed commit 971e77005e139347d5d4fedf0cc87bfb26b2685f
 >
 > '''src/bin/d2/d2_config.h'''[[BR]]
 > !DnsServerInfo constructor: spaces required around the "=" signs.
 >

 Got it.

 > '''src/bin/d2/d2_messages.mes'''[[BR]]
 > DCTL_ORDER_NO_ELEMENT: the description doesn't really give any
 information.  From what I can see, the error arises when the configuration
 being parsed does not contain the elements that were specified when the
 parse order was set up.  In other words, there the configuration is
 missing some elements expected by the program.  A better explanation would
 be something like:
 >
 >  "An error message output during a configuration update.  The program is
 expecting an item but has not found it in the new configuration.  This may
 mean that the BIND 10 configuration database is corrupt.
 >

 Well said, you're hired!
 >
 >
 > '''src/bin/d2/tests/d2_cfg_mgr_unittests.cc'''[[BR]]
 > >Various places in the file: in EXPECT_EQ, the expected value should be
 the first argument and the actual value the second. (Google Test's failure
 messages expect this order.)
 > This wasn't addressed but leave it for now - even if a test fails, you
 can work out what was meant.

 Sorry, somehow this got overlooked in with the other comments. I have
 corrected it.

 >
 > checKey(): brace should be on same line as "if".
 >

 Ugh. Got it.

 > tsigTest: add a "todo" to note that this should be expanded later.  All
 the test does is check that an empty list of keys is recognised, it
 doesn't check that a list comprising valid elements is parsed correctly.
 >

 Actually, I meant to delete this test. It is one I created early on and
 then neglected to remove.  TSIG parsing is well covered by several other
 tests.


 > '''src/bin/d2/tests/d_test_stubs.h'''[[BR]]
 > Rather than include an #if...#endif to conditionally output the text if
 fromJSON fails, stream the JSON text into an error message that will be
 output if a test fails, e.g.
 > {{{
 > EXPECT_TRUE(fromJSON(config_text)) << config_text;
 > }}}
 >
 > Similarly, rather than use conditional compilation for outputting the
 checkAnswer rcode, change the assertion used.  Rather than have the method
 return a bool, have it return the actual result: then replace
 > {{{
 > EXPECT_TRUE(checkAnswer(expected_result));
 > }}}
 > with
 > {{{
 > EXPECT_EQ(expected_result, checkAnswer());
 > }}}
 > The message associated with an assertion failure will include both the
 expected and actual values.


 I have changed these two to return ::testingAssertionResult.  This is much
 cleaner.  Wasn't aware of this feature.


 >
 > '''Other Issues'''[[BR]]
 > >> A FQDN ends with a ".". Where is this checked? As an aside, are the
 list of search domains normalized on input (i.e. adding a trailing "." if
 they don't have one?").
 > > Currently this not enforced and there's no normalization as you
 describe. This would fall into the todo category. FQDN entry validation
 should be enforced during configuration parsing.
 > I think we should follow the Postel dictum of "being liberal in what you
 accept".  As there is no default domain in the DDNS configuration, we have
 to assume that everything entered is a FQDN. In other words, assume that
 if someone entered "example.com" they really meant "example.com.".  We
 really need to ensure that this does not fall by the wayside - a missing
 "." can be [http://royal.pingdom.com/2009/10/13/sweden%E2%80%99s-internet-
 broken-by-dns-mistake really important].
 >

 In Marcin's review of 3007 and he suggested I use the dns::Name class for
 handling FQDNs.  I have looked at this briefly, and I don't disagree. It
 may be more appropriate here as well.  It bears more study.  In the
 interests of getting D2 on its feet, this is an area that for now I have
 glossed over and am using simple strings.  I have however, marked all this
 with @todos.


 > '''!ChangeLog'''[[BR]]
 > This needs a !ChangeLog entry.  Something like:
 > {{{
 > Added the configuration parsing module for the DHCP DDNS server.
 > }}}
 > ... would do.
 >


 xxxx        [func]      [tmark]
 Added the initial implementation of configuration parsing for
 the DCHP DDNS server.

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