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:  tmark
                Type:  enhancement   |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp-ddns     |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130703
           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 stephen):

 * owner:  stephen => tmark


Comment:

 Reviewed commit 971e77005e139347d5d4fedf0cc87bfb26b2685f

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

 '''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.



 '''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.

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

 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.

 '''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.

 '''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].

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

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