BIND 10 #3295: Refactor DHCPv6 code which processes Client FDQN option.

[BIND 10 Development]

#3295: Refactor DHCPv6 code which processes Client FDQN option.
-------------------------------------+-------------------------------------
            Reporter:  marcin        |                        Owner:
                Type:  enhancement   |  marcin
            Priority:  high          |                       Status:
           Component:  dhcp-ddns     |  reviewing
            Keywords:                |                    Milestone:  DHCP-
           Sensitive:  0             |  Kea0.9-alpha
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  24            |                 CVSS Scoring:
         Total Hours:  40            |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  2
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tmark):

 * hours:  3 => 2
 * owner:  tmark => marcin
 * totalhours:  38 => 40


Comment:

 Replying to [comment:10 marcin]:
 > Replying to [comment:9 tmark]:
 > > This was some complicated work, nice job.
 > >
 > > cppcheck.sh was clean, I did NOT run the unittests through valgrind.
 >
 > Thank you QA team. Isn't it your responsibility to run valgrind as well?
 ;) Just kidding.
 >
 > >
 ----------------------------------------------------------------------------------------
 > >
 > > bin/dhcp6/dhcp6_src.cc
 > >
 > > Dhcp6Srv::createNameChangeRequests()
 > >
 > > At line 1056:
 > >
 > > {{{
 > >     // It is likely that client haven't included the FQDN option. In
 such case,
 > >     // FQDN option will be NULL. This is valid state, so we simply
 return.
 > > }}}
 > >
 > > If DDNS is enabled we are always going to include the FQDN option in
 the
 > > answer so it should be there.  Suppose DDNS is enabled but a
 programming error
 > > results in there being no FQDN option?  This test would mask such a
 condition.
 > >
 > > I would suggest only calling this method if DDNS is enabled and inside
 the method
 > > throw an exception if there is no FQDN option.
 >
 > It is not how the code is working now. The processClientFqdn will not
 append Client FQDN option  if there is no such an option in client's
 message. Client informs of its capability to process this option by
 sending it to the server (and adding its code in ORO). So, you can't
 expect that the FQDN option is always there.
 >

 As we discussed there I see now that there is a legitmate case where DDNS
 is
 enabled but the client's request did NOT include the FQDN. This implies
 that
 the client has no intererst if DDNS.  In such a case it would be valid for
 there to be no FQDN in the response.

 Please add a test at the beginning of the method to reutrn if
 FQDN_ENABLE_UDPAT E is false or a todo stating a test should be added.

 > >
 > >
 ------------------------------------------------------------------------------
 > > bin/dhcp6/dhcp6_src.cc
 > >
 > > Dhcp6Srv::createRemovalNameChangeRequests()
 > >
 > > Rather than do an explicit check and log message for an empty hostname
 at
 > > line 1138, couldn't you just let it process the conversion attempt at
 1153?
 > > writeFqdn should throw if passed an empty string and both tests guard
 against
 > > a host name corrupted by outside intervention in the db.
 > >
 >
 > I initially thought that checking for empty hostname is faster than
 forcing writeFqdn to thrown an exception. However, since this is a
 prgrammatic error it should be rare and thus it doesn't really matter.
 >
 > > Also, the log message at 1155 should probably include the lease
 address.
 >
 > Added.
 >
 > >
 > >
 ------------------------------------------------------------------------------
 > > bin/dhcp6/dhcp6_srv.cc
 > >
 > > Dhcp6Srv::generateFqdn()
 > >
 > > At line 2449 you call updateLease6 even if the lease is not found.  In
 other
 > > words, it is not protected by the lease not null test at 2446.
 Really, if
 > > the lease isn't found shouldn't you throw an exception?  It means
 something
 > > is rather wrong.
 >
 > What was I thinking?! Fixed.
 >
 > >
 > >
 ------------------------------------------------------------------------------
 > > lib/dhcpsrv/alloc_engine.h
 > >
 > > There is no commentary for  Lease6Collection::updateFqdnData.
 >
 > Yep. I am pretty sure I added this comment and I have no idea what
 happened to it. Added again.
 >

 You must have used a "disappearing font".  I hate it when that happens.


 > >
 > >
 ------------------------------------------------------------------------------
 > > lib/dhcpsrv/tests/lease_unittest.c
 > >
 > > TEST(Lease6, hasIdenticalFqdn)
 > >
 > > Curiously this test uses Lease4 instances to compare against.  Not
 quite
 > > what you had in mind is it?
 >
 > Yes, this test was copied from the V4 test. Fixed.
 >
 > >
 > >
 ------------------------------------------------------------------------------
 > > bin/dhcp6/tests/fqdn_unittest.cc
 > >
 > > Cosmetic but...at line 305 you use the conditional (ternary) operator
 which is great, I love them for this type of thing, but I prefer to
 enclose them in parens like so:
 > >
 > >
 > > {{{
 > >         Option6ClientFqdn::DomainNameType fqdn_type =
 (hostname.empty() ?
 > >             Option6ClientFqdn::PARTIAL : Option6ClientFqdn::FULL);
 > > }}}
 > >
 > > This way the reader's mind knows the start an expression. It's up to
 you.
 >
 > My editor (everybody knows what it is), does odd things in terms of
 aligning parameters in multiple lines, when doing this change. But I
 forced it to accept it. :)
 >
 > >
 > >
 ------------------------------------------------------------------------------

 If you agree with my comments, I do not need to see this ticket again.
 Please proceed.

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