BIND 10 #3035: Modify DHCP4 to generate NameChangeRequest

[BIND 10 Development]

#3035: Modify DHCP4 to generate NameChangeRequest
-------------------------------------+-------------------------------------
            Reporter:  marcin        |                        Owner:
                Type:  enhancement   |  marcin
            Priority:  medium        |                       Status:
           Component:  dhcp-ddns     |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20131016
         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 => marcin


Comment:

 This work took a lot of research into the RFCs and thought to get the
 logic together to handle all permutations.  I like the way
 fdqn_unittests.cc is structured.


 fqdn_unittest.cc:

 Extraneous whitespace. (HA! I finally busted you on this!)

 -----------------------------------------------------------------------------
 dhcp4_srv.h

 Method commentary for computeDhcid seems a bit thin.
 Also, it is lacking @throw.

 -----------------------------------------------------------------------------
 dhcp4_srv.h

 Method commentary for processClientFqdnOption seems a bit thin, same for
 processHostnameOption.  You may wish to refer back to the description of
 processClientName.

 Alternatively, you could split up commentary above processClientName and
 move
 the FQDN option discussion to processClientFqdnOption and the hostname
 option
 discussion to processHostnameOption.

 -----------------------------------------------------------------------------
 dhcp4_srv.cc

 add an "@todo" tag at line 81, where it discusses constants being
 replaced.

 ----------------------------------------------------------------------------
 dhcp4_srv.cc

 Dhcpv4Srv::createNameChangeRequests

 lease_expires_on is set to 0. Lease expires on is the actual date/time on
 which the lease expires.  It is essentially time the lease was granted +
 valid_lft.
 One way would be to add logic to calculate to the NCR constructor.
 Another
 would be to calculate it in this method and pass it in.

 ----------------------------------------------------------------------------
 dhcp4_srv.cc

 Dhcpv4Srv::createNameChangeRequests

 There is some behavior here that we may wish to make configurable.  If
 nothing else, maybe add this to the commentary?

 1. Generating a delete and then an add -  D2's state machine is geared to
 first try an add and if that comes back with "FQDN in use", it is supposed
 to
 issue a second request which deletes and then adds it.  This is actually
 per
 RFC 4703.  It would be less work on DHCP4 but still only 2 transactions
 with
 the server.

 2. We may also wish to make it configurable that we always update, whether
 it
 is is renewal or not.

 ----------------------------------------------------------------------------
 dhcp4_srv.cc

 queueNameChangeRequset

 Please check blank lines or lack thereof against coding guidelines in this
 method.  You seem have blank lines were you shouldn't and not where you
 should ;).

 -----------------------------------------------------------------------------
 dhcp4_srv.cc

 queueNameChangeRequset

 Suggestion, you could reduce the two debug log statements into one by
 doing
 something like this:

 {{{
     LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL_DATA,
                   DHCP4_QUEUE_NCR)
                   .arg(chg_type == CHG_ADD ? "add" : "remove")
                   .arg(ncr.toText());
 }}}

 where the message is this:

 {{{
 % DHCP4_QUEUE_NCR name change request to %1 DNS entry queued: %2
 }}}

 Makes for a bit less code to look at and one less log message.

 ---------------------------------------------------------------------------
 dhcp4_srv.cc

 processHostnameOption

 I'm not certain if hostname string from opt_hostname->readString() is
 guaranteed to not be empty, but what if it were?


 Also, is there a unit test for an unqualified host name?  Suppose the
 packet comes in with a hostname of "mycomputer".

 I think you need to re-examine using dns::Name.getLabelCount().  From my
 limited testing of this method, I do not beleive it counts labels the way
 you think it does.  This method, OptionDataTypeUtil::getLabelCount(),
 uses dns::Name(std::string) constructor and then calls
 Name.getLabelCount().

 That constructor will throw if given "", ".", or ".sometext".
 Additionally,
 it seems always return the number of labels + 1.  A quick bit of test code
 produced this:

 {{{
 For [bs at mycomputer] the label count is:2
 For [mycomputer] the label count is:2
 For [mycomputer.] the label count is:2
 For [mycomputer.tmark] the label count is:3
 For [mycomputer.tmark.] the label count is:3
 For [mycomputer.tmark.net.] the label count is:4
 }}}

 Where the string in brackets was passed to dns::Name() ctor and then
 getLabelCount was invoked on the instance created.

 ----------------------------------------------------------------------------

 dhcp4_messages.mes

 In the log message description:

 {{{
  % DHCP4_NCR_CREATION_FAILED failed to generate name change requests for
 DNS: %1
  This message indicates that server was unable to generate so called
  NameChangeRequests which should be sent to the b10-dhcp_ddns module to
 create
 }}}

 I would remove the phrase "so called". In English it can have negative
 connotations:


 so-called: Incorrectly or falsely termed
 Example  "My so-called friends were gossiping about me again."

 I think you meant something more like "known as" or "referred to as".

 ----------------------------------------------------------------------------

 Developer's Guide:

 Again you used the phrase "so called".  Also in this line you are missing
 the
 word "or" after the word "preference":

 {{{
 configured to obey client's preference to do FQDN processing in a
 different way
 }}}

 ---------------------------------------------------------------------------

 For future reference, the !NameChangeSender has an internal send queue.
 DHCP4
 will not need to it's own FIFO.  Really all it has to do once it
 instantiates
 its !NameChangeSender is call its !sendRequest() method which places the
 NCR on the sender's internal queue.  The sends are asynchronous.  When the
 current send finishes, the sender will dequeue and send the next one
 automatically.

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