BIND 10 #3036: Modify DHCP6 to generate NameChangeRequest

[BIND 10 Development]

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

Comment (by tomek):

 General comments:

 Please rename option6_client_fqdn.cc|h to option6_fqdn.cc|h. We try
 to keep the names as short as possible. Otherwise we will have a
 really long names like option6_authentication.cc or
 option6_client_identifier.cc

 This work really should be split into 2 tickets: option class and then
 follow-up ticket that uses that class. Keep that in mind when doing
 its v4 counter-part.

 '''option6_client_fqdn.h'''
 FLAG_S, FLAG_O FLAG_N are defined as enums. They should be const
 uint8_t as flags field can have more than one of those set. Enums are
 used when only one of the possible values is valid.


 I don't like the Option6ClientFqdnImpl class much. I understand the
 reasons, but similar result could be achieved with having just one
 class and domain_name_ defined as private (or protected for testing
 purposes). It is true that there is no need to include path to
 dns/labelsequence.h when including option6_client_fqdn.h header, but
 on the other hand you still need to link with dns library. So the
 bottom line is that the Option6ClientFqdn comment in last paragraph
 about changes being transparent is not really valid.


 Some of the methods in Option6ClientFqdn can be private (or at least
 protected), e.g. packDomainName().


 '''option6_client_fqdn.cc'''
 Option6CXlientFqdnImpl and its methods should be documented.

 This class allows construction of partial or even empty domain names.
 That is not really needed on the server side (Last paragraph in
 section 4.2 of RFC4704: "Servers SHOULD send the complete fully
 qualified domain name in Client FQDN options."). But I suppose it
 is ok, as we will hopefully soon use those options in perfdhcp and
 other projects.

 Why domain_name_ is a pointer to dns::Name, rather than object itself?
 Can dns::Name store empty domain-name? If it can, then I suggest
 modify the code to use dns::Name directly, rather than via pointer.

 checkFlags() You can't throw if MBZ field is non-zero. See last
 paragraph in section 4.1 of RFC4704. Server MUST ignore these
 bits. (just bits, not the whole option). If you really want to do
 something, printing a debug message is probably ok, though. But it
 could be an attack vector to clog server logs.

 Assignment operator, Option6ClientFqdnImpl() constructor: What will
 happen if you try to assign/copy an option without domain name?  :)

 parseWireData(): exception comment should say what's the option
 size (0) and what is the expected minimum (1).

 Option6ClientFqdn::Option6ClientFqdn(first, last):
 the data is kept twice in the object (once in Option.data_ and
 second time in Option6ClientFqdnImpl). Is there a need for such
 redundancy?

 getFlag(): That check is weird. Flag enum has only 3 values defined:
 1, 2 and 4. Any values in between are not correct. Will the compiler
 even allow casting uint8_t into Flag enum?

 setFlag(): the same as getFlag(). You are using enum as uint8_t here.
 I think the parameters should be uint8_t.

 Is there even a guarantee that enum values are coded on 8 bits? I
 don't think so. What about big endians? The S flag could be stored in
 memory as 0x01000000. Please get rid of the enum Flag and substitute
 it with uint8_t. You may add a comment that some combination of the
 flags are allowed, so enum is not appropriate here.

 What is wrong will clearing all bits (flag=0)? Why do you throw an
 exception? My understanding is that 000 sent by client is a valid
 combination (client requests server to perform PTR only).

 toText(): please reformat toText() to return a single line response.
 We have a debug code somewhere that prints out whole packets with
 all options. It will be confusing if some options take up more than
 one line (multi-line strings should be reserved for encapsulated
 options)

 '''alloc_engine.h'''
 When updating copyright year, please use 2012-2013, not 2013.

 '''alloc_engine.cc'''
 Please implement Lease::setHostname(const bool fwd, const bool rev,
        const string& name). That will help avoiding invalid
 combinations (e.g. fwd=true, rev=true, name="") are not allowed.
 Since the fields are public we can't really make sure of it, but
 having such a simple method with set up best practice here.

 '''option_definition.cc'''
 I see many cosmetic changes related to maximum line length.
 We have and agreemeing in bind10 chatroom that we typically try
 to keep the code under 80 columns, but up to 100 columns is considered
 ok. If you still use terminal that can't handle more than 80
 columns, please at least do not add extra << to the code as
 it (marginally) bloats the output. Exploit the preprocessor to
 do the work for you:
 {{{
 cout << "text1"
         "text2" << end;
 }}}

 '''dhcp6_srv.h'''
 I don't like the inclusion of d2/ncr_msg.h. Can you move it to
 dhcp6_srv.cc? After that change suddenly the number of dependencies
 for dhcp6 became much more complex. Keep in mind that while
 ddns will be used in typical environment, we will eventually
 need to address embedded/CPE market. It is realistic to expect
 that the server will handle FQDN option, but will not do any
 DDNS stuff. In fact, we should have something like
 --disable-dhcp-ddns parameter in configure that would disable
 all DDNS stuff (except handling client FQDN option).

 General question for dhcp6_srv modifications: what will the server
 do if there is a client that sends 5 IA_NA options?

 appendClientFqdn(): please make it clear whether it is fqdn
 send by the client or the server's response


 '''dhcp6_srv.cc''
 appendClientFqdn(): The first if seems wrong. Something
 is also wrong with the tests that didn't pick it up.

 createNameChangeRequests(): We will eventually support
 a case when there is more than one address in a given
 IA_NA. That is an uncommon, but valid case (e.g. during
 renumbering events). The code will eventually have to do
 {{{
   for (each IA_NA/IA_TA in the response) {
       for each (address in this IA) {
           generateNCR()
       }
   }
 }}}

 Please add an appropriate @todo in the code.

 The method must get an extra parameter: add, update (and possibly
 delete or skip). Right now you're generating ncr ADD for RENEWs and
 you don't generate anything for RELEASEs.

 createRemovalNameChangeRequest(): Third comment it wrong.
 You can't create DHCID out of hostname. What you trying to
 generate here will be used as AAAA, not DHCID.

 run(): Please update the comment near call to
 sendNameChangeRequests() to say that we call that method to
 send NCRs. You mentioned that it's a stub in at least 3 places.
 It will be tricky to update all those comments once we
 implement actual NCR transmissions.

 processClientFQDN(): The first if is technically correct,
 but looks odd.

 Please add a @todo somewhere in the processClientFqdn(). Ideally
 all uses of constants should be replaced by methods (e.g.
 FQDN_GENERATED_PARTIAL_NAME with generateFqdnPartialName(question)),
 but I don't insist on it. It can be done as part of the next ticket.

 ----
 Rest of the review will continue tomorrow.

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