BIND 10 #3033: DHCP4 Configuration Parameter additions for DDNS.

[BIND 10 Development]

#3033: DHCP4 Configuration Parameter additions for DDNS.
-------------------------------------+-------------------------------------
            Reporter:  marcin        |                        Owner:  tmark
                Type:  enhancement   |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp-ddns     |                    Milestone:  DHCP-
            Keywords:                |  Kea1.0-alpha
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  0             |              Defect Severity:  N/A
         Total Hours:  40            |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  16
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tomek):

 * owner:  tomek => tmark
 * totalhours:  32 => 40


Comment:

 The code didn't compile for me on Ubuntu 13.10 x64 with g++ 4.8.1:
 {{{
 cfgmgr_unittest.cc: In member function ‘virtual void
 {anonymous}::CfgMgrTest_d2ClientConfig_Test::TestBody()’:
 cfgmgr_unittest.cc:675:13: error: unused variable ‘cfg_mgr’ [-Werror
 =unused-variable]
      CfgMgr& cfg_mgr = CfgMgr::instance();
              ^
 cc1plus: all warnings being treated as errors
 make[7]: *** [libdhcpsrv_unittests-cfgmgr_unittest.o] Błąd 1
 }}}
 I pushed a (trivial) fix. Please update.
 ----------------
 Man, check your calendar! This ticket took you so long, that you'll have
 to update copyright years now... It's 2014 already! :P

 ----------------
 @todo tags must be used with triple slashes. Otherwise Doxygen will not
 spot them and will not include them in the todo page.

 ----------------
 There is no use visible documentation for those changes. Is there
 a separate ticket for this? In general I would prefer to add a very
 simple (even a stub) section to the BIND10 Guide in this ticket and
 then write a proper documentation in a separate ticket. I'm afraid
 that we'll add the code and then Stephen may say start deferring the
 documentation ticket. Or we may simply forget some of the options that
 were added. If you don't believe me, take a look at prefix
 delegation docs. (Which docs, you may ask?)
 ----------------
 '''dhcp4.spec''' - dhcp-ddns does not have a description.

 enable-updates - An honest question: why is "false" in item_default
 capitalized, but in item_optional it is not?

 server port 5301 - I'm just curious. Why this particular one?

 What are the allowed values for ncr_protocol and ncr_format?
 Unless we support more than one, I'm not sure if those need to be
 exposed to users.

 item_descriptions for remove-on-renew and always-include-fqdn should
 end up with a question mark.

 Some parameters uses underscore (e.g. ncr_protocol, ncr_format) while
 others (remove-on-renew, always-include-fqdn) using a dash. Please
 be consistent with the naming. Looking at the rest of the spec file,
 it seems that a dash is more common. As a side note, we weren't that
 consistent (there's "interface_name" for example). As a proof that it
 is really needed, see your very own comment in dhcp_parser.h:909.

 Since this is something that user will have to type in, it's better to
 use dash (single keystroke) rather than underscore (2 keystrokes -
 shift + the key).

 If this is the whole DDNS configuration, then there are some
 parameters missing. In particular:
 - transmission timeout
 - max retries count

 server-ip should be a list of addresses eventually. It is ok to have
 it as a single address for now, though. The description should be clear
 that this may be either IPv4 or IPv6 address. (DHCP family and the
 protocol
 over which the updates are done are completely unrelated things).

 '''src/bin/dhcp4/tests/config_arser_unittest.cc'''
 Dhcp4ParserTest.d2ClientConfig. Can you update the sample config to
 use non-default values?

 There are tests missing for bogus values (e.g. setting ncr-protocol
 to AppleTalk or IP-over-carrier-pigeons) and for logic (like the
 example you gave: ALLOW_CLIENT_UPDATE can only be true if both
 OVERRIDE_NO_UPDATE and OVERRIDE_CLIENT_UPDATE are false). That's ok
 if you want to do this in a follow up ticket, but please add
 appropriate @todo for such tests.

 '''src/lib/dhcp_ddns/ncr_io.cc'''
 stringtoNcrProtocol() - it is somewhat odd that this is not a static
 member of some class. It is ok as it is now, just looks a bit...
 detached. That's just a comment, no need to do anything with it.

 Stephen would probably say that there should be else between ifs, but
 I like the elegance and simplicity of the current code. And this part
 of the code will never be performance critical, so please leave it as
 it is.

 The function should use uppercase conversion, so "Udp", "udp", "UDP"
 should all work.

 We may consider adding a @todo for 3rd option, besides TCP and UDP. In
 Dibbler it is called "ANY". Dibbler configured to ANY starts with UDP,
 but if there is not response it then falls over to slower, but more
 reliable TCP. If you think that's a good idea, please add a @todo
 somewhere in the code.

 ncrProtocolToString() - not handled values could return a numeric value
 (like. UNKNOWN(58)).

 '''src/lib/dhcpsrv/cfgmgr.h'''
 isDhcpDdnsEnabled() - I think the name is too long. It is in dhcpsrv
 directory in isc::dhcp namespace, so it is obvious that it's about
 DHCP. Therefore isDdnsEnabled() or even ddnsEnabled() sounds easier.

 '''src/lib/dhcpsrv/d2_client.cc'''
 D2ClientConfig constructor: please move the validation part to a
 separate method. Over time we may grow a capability to dynamically
 update the configuration rather than tossing it all and recreating
 from scratch.

 D2ClientMgr::setD2ClientConfig()

 '''src/lib/dhcpsrv/d2_client.h'''
 The comment for D2ClientMgr class (line 237) is missing
 a closing bracket.

 For the server IP related methods, please make it explicit in the
 comments that the address can be v4 or v6.

 Why do you need == operator for? That's an honest question. I'm
 just curious. To check whether the configuration has changed?

 '''src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc'''
 Please add a case in ParseConfigTest.validD2Config a case with most
 of the values set to false. The current code would pass if everything
 was hardcoded to true.

 Sorry it took so long to do the review.

 +8 hours.

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