BIND 10 #3036: Modify DHCP6 to generate NameChangeRequest

Tue Aug 13 13:10:03 UTC 2013

#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:

 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.

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

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

 '''dhcp6/dhcp6.dox'''
 Please udpate the copyright year to 2012-2013.

 Please also update doc/devel/mainpage.dox with a reference to
 dhcpv6DDNSIntegration.

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

 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.

 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?

 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.

 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.

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

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

 FqdnDhcpv6SrvTest class and its members should be documented.

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

 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.

 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.

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

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

 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.

 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.

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

 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.

 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:6>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development