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

Comment (by 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.

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

 >
 > '''ncr_unittests.cc'''
 > Please also test min (1), max (128) lengths of DUID when calculating
 > DHCID.

 Added.

 >
 > '''dhcp6/dhcp6.dox'''
 > Please udpate the copyright year to 2012-2013.
 >
 > Please also update doc/devel/mainpage.dox with a reference to
 > dhcpv6DDNSIntegration.

 Added.

 >
 > Please update section dhcp6-std in doc/guide/bind10-guide.xml.
 >
 > Second paragraph gives wrong RFC. It should be 4703, not 4307 (which
 > is about IKEv2).

 Fixed.

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

 >
 > bind10-dhcp6 => b10-dhcp6
 >
 > "Depending on the message type, a DHCPv6 server may generate 0, 1 or 2
 > NameChangeRequests during single message processing." Is this true?
 > How many NCRs will be generated if client sends REQUEST with 10
 > IA_NAs and server assigns them?

 I clarified the text a bit. That was an example picked for simplicity.

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

 >
 > The @todo starting with "The decision about not..." is confusing. I
 > don't understand what is left to do there. Do you mean writing the
 > house keeper code? There's no need to mention it here. I think it is
 > ok to remove the whole paragraph. If you want to keep it, please
 > rephrase it to make its point clearer.

 ok. Removed.

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

 >
 > Why do you need N,O,S flags redefined? See also previous comment about
 > migrating Option6ClientFqdn::Flags enum to uint8_t.

 I needed them because I had to initialize arrays of uint8_t in many tests.
 I wanted to use named values to initialize the bytes corresponding to
 flags field in those arrays. If I used enum some compilers would complain
 that I am using non-uint8_t value to initialize the pieces of array of
 uint8_t. That was a hack to avoid it. But, it is not a problem anymore
 because I changed the enum to uint8_t.

 >
 > FqdnDhcpv6SrvTest class and its members should be documented.

 I added some documentation.

 >
 > testFqdn(): != 0 is not required in the flag_{n,s,o} initialization.

 Actually, it doesn't need !''? true : false!''.


 >
 > serverAAAAUpdatePartialName(), serverAAAAUpdateNoName() and similar
 > should probably use the same constants that are used in the
 > Dhcpv6Srv::processClientFqdn(). It is ok to leave the code as it is
 > now, but you'll likely forget to remove/update it when
 > Dhcpv6Srv::processClientFqdn() is implemented using actual values.

 Fixed.

 >
 > createNameChangeRequests(): I like this test a lot and the fact that
 > it checks mulitple IA_NAs in a single message.


 >
 > FqdnDhcpv6SrvTest.processSolicit(). Interesting test. What about a
 > case when during ADVERTISE generation server reuses old expired lease?
 > I think it should then delete old entries, but I haven't given it
 > enough thought, so I'm not 100% convinced. If we don't do it, client
 > may get ADVERTISE and then try to resolve it and see who (what domain
 > name) was using that address before.

 It will delete old entries when the client actually gets the lease when
 sends REQUEST.

 >
 > '''option6_client_fqdn_unittest.cc'''
 > N,O,S flags are defined here third time.

 I explained that already.

 >
 > Option6ClientFqdnTest.assignment, .copyConstruct: Please extend the
 > tests to copy empty name.

 Added new tests.

 >
 > Please add a test that will check that the code throws an exception
 > if:
 > - any label is longer than 63 octets
 > - the whole domain name is longer than 255 bytes
 > See RFC4704, section 4.2, first paragraph.

 Added.

 >
 > Also see my comment about ignoring MBZ field. The code should not
 > throw. RFC4704 says that the server must ignore those bits, not the
 > whole option.

 That has been addressed already.

 >
 >
 > Finally, the code doesn't build on Ubuntu 13.04 x64.
 >
 > You include d2/ncr_msg.cc when building dhcp6, but you do not include
 > $(BOTAN_INCLUDE) in AM_CPPFLAGS in src/bin/dhcp6/Makefile.am
 >
 > The following fix makes the compilation possible, but it later fails
 linking.
 > I have not pursued it further as the libdhcp-ddns code on master makes
 > any fixes to the current linking issues obsolete.
 >
 > {{{
 > diff --git a/src/bin/dhcp6/Makefile.am b/src/bin/dhcp6/Makefile.am
 > index 6f2c694..1f00c66 100644
 > --- a/src/bin/dhcp6/Makefile.am
 > +++ b/src/bin/dhcp6/Makefile.am
 > @@ -3,7 +3,7 @@ SUBDIRS = . tests
 >  AM_CPPFLAGS = -I$(top_srcdir)/src/lib -I$(top_builddir)/src/lib
 >  AM_CPPFLAGS += -I$(top_srcdir)/src/bin -I$(top_builddir)/src/bin
 >  AM_CPPFLAGS += -I$(top_srcdir)/src/lib/cc -I$(top_builddir)/src/lib/cc
 > -AM_CPPFLAGS += $(BOOST_INCLUDES)
 > +AM_CPPFLAGS += $(BOOST_INCLUDES) $(BOTAN_INCLUDES)
 >
 >  AM_CXXFLAGS = $(B10_CXXFLAGS)
 >  if USE_CLANGPP
 > }}}

 Hopefully, it compiles now.

 >
 > We had a long discussion on bind10 chatroom today about using enums for
 > flags in FQDN option. The source of confusion comes from the fact that
 > the 'flag' word it is used in two contexts. The first one refers to the
 > whole bitfield called 'flags', which flag refers to only a single bit.
 > Both Tom and I would prefer to use uint8_t for both as more then one
 > flag is allowed to be set. And it seems wrong to do logical operations
 > on enums. However, if our arguments didn't convince you, please at
 > least rename the FLAG_N to BIT_N as this information is referred to as
 > 'N bit' in RFC4704. Also, please use plural form (flags) when you refer
 > to the whole bitfield. The distinction would then be easier to
 understand.
 >
 > Your ChangeLog proposal is ok, but a bit long. Also, D2 is now
 officially
 > called b10-dhcp-ddns, so please use that name.

 Yes. That will be changed.


 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.

 >
 > I have no more comments at this time. I would definitely like to see
 this
 > ticket again.

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