BIND 10 #3036: Modify DHCP6 to generate NameChangeRequest

[BIND 10 Development]

#3036: Modify DHCP6 to generate NameChangeRequest
-------------------------------------+-------------------------------------
            Reporter:  marcin        |                        Owner:
                Type:  enhancement   |  marcin
            Priority:  medium        |                       Status:
           Component:  dhcp-ddns     |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130821
         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 tomek):

 * owner:  tomek => marcin


Comment:

 Replying to [comment:8 marcin]:
 > Replying to [comment:6 tomek]:
 > > Many of the hacks done in Makefiles (like including d2/ncr_msg.cc in
 > > dhcp6) are no longer necessary, because libdhcp_ddns is already merged
 > > to master. Please merge master into trac3036 and clean up the
 > > makefiles.
 >
 > I merged master to trac3036 and cleaned up the Makefiles.
 Thanks.

 > > '''ncr_msg.cc|h'''
 > >
 > > fromDUID(): Changes look ok, but there's one thing that requires a bit
 > > though (or at least extra comment in the text). RFC4701, Section 3.5
 > > says that FQDN should be in canonical format, as defined in RFC4034,
 > > Section 6.2. That section talks about converting FQDN to lowercase. Do
 > > we do that? If we do, we should make a brief comment about it and
 > > point to specific place where this conversion happens. If we don't, we
 > > should extend the code. I'm not saying that this is the best place. It
 > > isn't. Probably somewhere in dns/labelsequence would be better (or in
 > > places that use that class).
 >
 > That is a good point. The isc::dns::Name class is capable of doing it
 but by default it is disabled. I made a change in the DHCPv6 srv code to
 force downcase.
 Are there any unit-tests that check that?

 > > Second paragraph in dhcpv6DDNSIntegration should be moved to
 > > doc/guide/bind10-guide.xml to section dhcp6-limit. Also, the comment
 > > about DNS Update not being supported needs to be removed there.
 >
 > I suggest that we keep this in mind and do this change when we actually
 support DNS updates. Currently it is only stub implementation. The doxygen
 section regarding the DDNS will be evolving with almost every ticket that
 I am working on right now. I prefer not to spread the information
 everywhere until we really support DDNS.

 Ok.

 > > The list "(no update, reverse only update, both reverse and
 > > forward update)" is not complete. Also forward only is
 > > possible. Although client may request both, server can be configured
 > > to do only forward updates.
 >
 > Ok. I was under the impression that in most cases server does reverse.
 And it can delegate the forward to the client but still does reverse.
 Isn't it the case?
 That's the typical case, I agree. However, in some networks the opposite
 (server doing only AAAA) is also valid. Sysadmins may configure the server
 to do only forward updates, e.g. because they don't care about reverse.

 > > "The DHCPv6 Client FQDN Option is comprises "NOS" flags" =>
 > > "The DHCPv6 Client FQDN Option contains "NOS" flags
 > >
 > > '''dhcp6_srv_unittest.cc'''
 > > This code added around 700 lines of new code to the file. For hooks, I
 > > did split them into separate file (see hooks_unittest.cc). You may
 > > consider doing the same. It is ok if you prefer to do it as a separate
 > > ticket, though.
 >
 > I don't want to bloat this ticket anymore.
 Then please create a separate ticket for that (or at least add a @todo in
 the test code).

 > > testFqdn(): != 0 is not required in the flag_{n,s,o} initialization.
 >
 > Actually, it doesn't need !''? true : false!''.
 True.

 > > Finally, the code doesn't build on Ubuntu 13.04 x64.
 > Hopefully, it compiles now.
 Nope. Still failed compilation. But the fix was easy. I pushed my changes.
 It compiles now.
 Please pull.

 > I don't understand the point about too long entry. Anything shorter
 would be vague and would not reflect what the new code is actually doing.
 Plus, this is not the final implementation and I don't want to make
 impression that we are doing any DNS updates at this time. So the last
 sentence is required.
 Again, it was only a comment. Let it keep that way.

 I have one more minor comment, which probably doesn't require any code
 changes, just a bit of explanation.
 Option6ClientFqdn::packDomainName() creates labels object from
 dereferenced domain_name_. What if the name is empty?

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