BIND 10 #3083: Changes to allocation engine to propagate the information about the FQDNs

[BIND 10 Development]

#3083: Changes to allocation engine to propagate the information about the FQDNs
-------------------------------------+-------------------------------------
            Reporter:  marcin        |                        Owner:  tmark
                Type:  enhancement   |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp-ddns     |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130904
           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 marcin):

 * owner:  marcin => tmark


Comment:

 Replying to [comment:6 tmark]:
 > Changes here are straight forward.  A couple of questions and some
 misspellings.  Unit tests run cleanly with valgrind, cppcheck looks clean
 too.

 Thanks!

 Before I corrected the spelling, I merged master to trac3083 to resolve
 the conflicts with recent changes to the Allocation Engine to implement
 hooks. Therefore, when you pull from the branch you will observe many
 commits being added to it.

 Proposed !ChangeLog entry:
 {{{
 6XX.    [func]*         marcin
         libdhcpsrv: Implemented changes to lease allocation engine to
         propagate information about client's FQDN.
         (Trac #3083, git abcdefg)

 }}}

 Other responses below:

 >
 >
 ---------------------------------------------------------------------------
 > alloc_engine.h
 >     /// engine. In particular, the DHCP server should be able to check
 whether
 >     /// existing lease was returned, or new lease was allocated. When
 existing
 >     /// lease was returned, server should check whether the FQDN has
 changed
 >     /// between the allocation of the old and new lease. If so, server
 should
 >     /// perform appropriate DNS update. If not, server may choose to not
 >     /// perform the update. The information about the old lease is
 returned via
 >     /// @c old_lease parameter. If NULL value is returned, it is an
 indication
 >
 > General question, should any of the above behavior be configurable?

 The server logic which is described here will be implemented with the
 #3035. This will also include some stub configuration for the server
 behaviour.

 To answer your question... I believe it doesn't need to be configurable
 whether the server updates the existing lease or it doesn't when nothing
 has changed in the notion of the client's FQDN. Why would server do that
 when it has sufficient amount of information that nothing has changed? If
 we ever decide that it has to, implementation of the additional parameter
 that turns the "always update mode" will be straightforward. Probably as
 simple as addition of the one !''if!'' statement (and the parameter in the
 config parser of course).

 The general idea behind returning the instance of the old lease is that
 server may make any decision by comparing the two leases. In particular,
 it may remove the old DNS record when it detects changes in FQDN. Any
 changes to the server's behaviour should be transparent to the allocation
 engine, which should blindly pass the FQDN data to and from the database.

 >
 >
 ---------------------------------------------------------------------------
 > alloc_engine.h
 >
 > Misspellings   "responisibility" and "responibility" in comments for
 > fwd_dns_update and rev_dns_update parameters.

 Thanks. I corrected those, plus I made some other small corrections in
 comments. I also modified the style of the new parameters descriptions
 (second line aligned with the start of the description of the first line),
 to make it consistent with existing parameters' descriptions.

 >
 >
 ---------------------------------------------------------------------------
 >
 > memfile_lease_mgr.cc
 >
 > Memfile_LeaesMgr::updateLease4 and updateLease6 both warn that the
 pointers are not validated.  Is this a performance consideration?  Almost
 everywhere else
 > in our codebase it seems like we check.  The code will crash if they are
 > invalid.  What would Stephen say?

 I agree. I simply followed the rule for all other functions in either
 Memfile or MySQL backends, where lease pointers are not sanity-checked. It
 doesn't make sense to make two functions throw exceptions when there are
 10 other functions which don't. Since backends are in the libdhcpsrv,
 which could theoretically be used by the external code I think it may be a
 good idea to sanity-check arguments of public functions in backends. I
 created a ticket for this: #3120.

 >
 >
 ---------------------------------------------------------------------------

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