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

[BIND 10 Development]

#3033: DHCP4 Configuration Parameter additions for DDNS.
-------------------------------------+-------------------------------------
            Reporter:  marcin        |                        Owner:  tomek
                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:  44            |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  4
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tmark):

 * hours:  16 => 4
 * owner:  tmark => tomek
 * totalhours:  40 => 44


Comment:

 General:

 allow-client-update flag has been removed altogether. In conversations
 with Marcin it was determined that it provided no functionality not
 provided by override-client-update.

 Replying to [comment:10 tomek]:
 > 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.

 Oops. Sorry.

 > ----------------
 > Man, check your calendar! This ticket took you so long, that you'll have
 > to update copyright years now... It's 2014 already! :P
 >

 No it just took you guys until 2014 to pull it off the review queue! ;)
 So this is will be annoying. Every time we touch a file we have to check
 that.
 I need a script for this.

 > ----------------
 > @todo tags must be used with triple slashes. Otherwise Doxygen will not
 > spot them and will not include them in the todo page.
 >
 Not sure which file(s) you are citing, several have this which are
 uninvolved
 with 3033 so I fixed all of them.

 > ----------------
 > 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?)

 There is already a ticket for exactly this:

 BIND 10 #3283: Add documentation to BIND10 guide for DHCP-DDNS

 I created it after Wednesday's Kea call per my action item.


 > ----------------
 > '''dhcp4.spec''' - dhcp-ddns does not have a description.
 >

 One thing I have noticed about the descriptions is that config logging
 includes them when it logs the configuration read or sent.  It makes those
 log outputs really big. I will add the description but we may wish to add
 a ticket to modify
 that logging output to be more terse or at lest drop the descriptions.
 It's issued from config code not Kea.

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

 I have no idea, I was just following suite here.  You guys started it ;).
 Look the other entries in this file.

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

 This was arbitrary, somewhat loosely based on the notion that DNS is port
 53.
 However a better choice would be somewhere in the private range: 49152
 through 65535.  How about 53001?

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

 ncr_protocol is NCR_UPD and NCR_TCP:  TCP is not yet supported but will
 be.
 Stephen wants the TCP in before the 1.0 release.


 ncr_format right now is only FMT_JSON:
 I don't see the harm in including it, especially since it's already there.


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

 I rephrased these slightly.

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

 Oops.  I thought I changed them all to hyphens already. In fact I did in
 all
 my test configs but forget to change the spec file.

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

 Actually I wish we used "_" because that way element names and variables
 that refer to them would match, but oh well.


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

 It is not the whole configuration it is only the Kea side of the
 configuration. D2 has its own spec file and parameters.  The values you
 mention are not yet in D2's configuration but they are planned for in the
 code just currently
 constants.


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

 I added v4/v6 to the description. As to supporting more than one value,
 that
 would imply some sort of server-selection scheme, assuming there is more
 than
 one and so forth.  This is something a bit beyond 1.0.

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

 Done.

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

 A couple points:  First, these types of tests are addressed in libdhcpsrv
 which defines both the parser and the classes involved.  There's no reason
 to replicate such testing in the server.  If you look at d2_client.cc and
 tests/d2_client_unittests.cc you will see tests and/or todos.

 Regarding the two enums, ncr-format and ncr-protocol, we discussed
 recently that there is no good, portable, way to create invalid values.
 For that matter I don't think code should defend against invalid enum
 values if the method parameter supplying the value is of that enum type
 (i.e. not an int or something).
 Others feel differently.

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

 I suppose it is odd but I made them functions because the enum types
 themselves ,NameChangeProtocol and NameChangeFormat, do not belong to
 specific classes; rather they belong to the namespace dhcp_ddns.

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

 Thank you, I think I will.


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

 Done.

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

 Added todo to ncr_io.h.

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

 I like this. Done.  Should have thought of it myself.

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

 Hmmmm. I guess I can shorten it.

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

 I have created a separate protected method called by the constructors to
 do
 the validation.  However to do dynamic updates as you would require adding
 setters which I am disinclined to do at this time.

 D2ClientConfig is a passive, data object and my intent was to deliberately
 not allow an instance containing invalid content to be created nor to
 expose members or setters.

 >
 > 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?

 Yes, so I can compare whether or not there was any actual change.

 >
 > '''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 hard coded to true.
 >
 > Sorry it took so long to do the review.
 >
 > +8 hours.

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