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

 Replying to [comment:7 marcin]:
 > I would avoid making option names too short. The mnemonic for option 39
 is OPTION_CLIENT_FQDN and the full option name is !''DHCPv6 Option Client
 FQDN!''. I wanted the class to reflect the actual option name. It
 precisely identifies the purpose of the class.
 There are no other options with similar names. But that was only a
 suggestion. Let's keep the current name.

 > > 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.
 > In fact, it should be 3 tickets: implementation of the option, changes
 to allocation engine to support FQDNs, changes to the DHCPv6 server to
 process the option. Optionally, I could add another ticket to add support
 to store FQDN information in MySQL. But, this will be implemented together
 with the relevant changes for DHCPv4.
 Good to know. I don't like large tickets, because besides doing the review
 is tedious, there's also another aspect. Technically valid comments are
 rejected on the grounds of making the ticket work too big or too bloated.
 Anyway, it's good that the next work is done in smaller chunks.

 > > '''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 told you on the chatroom, that use of enums does certain things more
 clear from the API point of you. Functions like getFlag() and setFlag()
 require that there is a single !''bit!'' specified (because you want to
 get a value of the bit N, or bit S or bit O etc.) Enums make it more clear
 to the caller that it is expected that only one of them is specified.
 Caller would be able to pass different values (e.g. 0x3) but it is not
 that easy because he would have to cast the value of 0x3 from int to
 enumeration type. This discourages invalid use of these two methods. This
 doesn't work like this when use uint8_t.
 >
 > Technically, both solutions (using enums and not using enums) work so it
 is just a discussion what is more/less clear from the interface point of
 you and we seem to have different opinions. I decided to do the change to
 avoid confusion in the use of constructor which takes uint8_t value to
 accept the combinations of flags. I also extended the description of
 setFlag and getFlag functions to clarify that only specific uint8_t values
 should be used.
 Thank you.

 > > 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.
 >
 > I disagree. The pimpl idiom is a common technique to separate the
 interface from the implementation. Not only it is common C++ programming
 technique, but also it is used in bind10 already in many places. I guess
 we both aren't happy with the fact that we need to link with libdns. If I
 do what you suggest, not only do I have to link with libdns but also I
 need to include a libdns header. Suppose, there are people who want to use
 libdhcp and make use of the !Option6ClientFqdn class. If they do, together
 with the option6_client_fqdn.h header they include a bunch of stuff from
 libdns. Who knows, maybe they even use functions from there  to do some
 specific things. When we write the common libraries we should really avoid
 misuse of our classes.
 Ok, I perhaps didn't express myself clearly. I understand that the
 dependency is needed (at least for now). I only commented on your
 statement that the future changes will be transparent. Regardless of the
 choice, they won't be. I didn't expect you to rewrite it with just one
 class.

 > > Some of the methods in Option6ClientFqdn can be private (or at least
 > > protected), e.g. packDomainName().
 >
 > No. This function is used in dhcp6_srv.cc and thus must be public.
 Ok, I missed that.

 > > '''option6_client_fqdn.cc'''
 > > Option6CXlientFqdnImpl and its methods should be documented.
 >
 > I added documentation.
 Thanks.

 > > 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.
 >
 > I agree it is not needed on the server side. But, the libdhcp is not a
 server-only library. So, I don't really understand the comment.
 It was just it - a comment, not a suggestion to do anything.



 > > 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.
 >
 > Ok. I shouldn't be throwing when I received an option with MBZ bits set,
 but I should throw if I am attempting to send option with MBZ bits set. I
 made a relevant change. Depending on the constructor used the exception is
 thrown or not.
 Good approach.

 > > Assignment operator, Option6ClientFqdnImpl() constructor: What will
 > > happen if you try to assign/copy an option without domain name?  :)
 >
 > Good catch. I added a check for this case. I also realized that the
 operator is not really used anywhere in the current code. So, technically
 I could remove the operator, but I left it, if it occurs to be needed in
 the future.
 I have mixed feelings about assignment operators. We are mostly accessing
 objects by smart pointers to them. It is a frequent mistake to assign one
 shared pointer to another rather then assign objects they point to.
 Anyway, let's keep the operator.

 > >
 > > 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?
 > Well, isn't it what you have been doing for other options too? Isn't it
 what we do for all options?
 >
 > The purpose of the specialized class is to parse the data and store them
 in the data structure which fits the format of the option. So, what is the
 actual problem?
 Ok, we had a discussion about that in chatroom and the consensus was to
 keep the code as it is.

 > > 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?
 > Yes, I would allow casting from 0x3 to enum. Please refer to the C++
 standard.
 Ok, I feel now more educated. But that enum behavior is counter-intuitive.
 However, since we're not in the language standardization business yet,
 I'll accept your explanation for now ;-)

 > > 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).
 >
 > You may have misunderstood the purpose of this method. There are three
 bits that you want to set or get: N, O, S. Each is identified by the value
 (mask) which is used to set this bit in the flags field: 0x1, 0x2, 0x4.
 Only those values makes sense. Specifying 0x0 doesn't make sense because
 it doesn't specify which bit you want to get/set.
 Ok.

 > > 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)
 >
 > Ok. I made a change to make it consistent. But, I am not very happy
 about the format of the text being printed. In my opinion it would be much
 easier to read the log if we made proper indentation. Sub-options could be
 more indented. We can have some offline discussion about it.
 Printing out whole messages will never be pretty. If we ever need detailed
 information about messages stored, I think it would be useful to consider
 having an pcap writer.

 > > '''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.
 >
 > I wish we followed this best practice everywhere in the code. I am not
 going to do it now, because it would bloat this ticket. This ticket is not
 really about changing how Allocation Engine manages internal data. New
 ticket: http://bind10.isc.org/ticket/3096
 Another ticket is fine. Thanks.

 > > '''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;
 > > }}}
 >
 > We should either work with all developers to document this informal
 agreement and make it formal or we should stick to 80.
 >
 > My terminal is doing good and it can absorb more than 80 chars but if I
 open two windows in emacs, each 80 chars wide, the code gets wrapped.
 It was intended as a joke. But I get your point. Nevertheless, the rule
 for having 80 chars was good for C code, when you had relatively short
 names and no namespaces or classes. With C++ 80 chars is not enough.

 > I will not expand it to over 80 lines because other developers don't
 really agree that it is allowed, and they also review my code.
 Yes, I'll add it to my TODO list.

 > > '''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).
 >
 > You tried to make me include the libdns header in the lbdhcp++ but you
 now suggest that linking dhcp6_srv with libdhcp_ddns is wrong. I
 understand the motiviation but that would require encapsulating the
 NameChangeRequests queue in the other class. If you really think that
 linking with libdhcp_ddns is a big issue, please confirm, I will file a
 ticket for it.
 No, it is fine as it is. I wrote my comment before I realized that
 libdhcp_ddns is already merged to master. I see that the d2/ncr_msg.h
 header is gone from dhcp6_srv.h. That's even better.

 > > 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.
 >
 > No. There are two types of !NameChangeRequest: for addition and for
 removal. The ones which remove DNS entries we also use for updating (by
 doing remove, add). Please have a look into renewIA_NA and releaseIA_NA.
 So during normal operation when client renews his address (and server is
 configured to do updates during RENEW) there will be a short periods of
 time when the name and address won't be resolving correctly?

 > > processClientFQDN(): The first if is technically correct,
 > > but looks odd.
 >
 > And, what is the problem? Returning existing empty pointer is faster
 than creating new empty pointer which will be copy constructed anyway.
 There's no problem. I sometimes make comments and they are just that -
 comments.

 > > 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.
 >
 > Constants are described in the header file.
 FQDN_GENERATE_PARTIAL_NAME is not defined in a header file, it is in
 dhcp6_srv.cc:112. Anyway, it doesn't make sense to do any clean-ups here
 as that part of the code will go away once #3034 is merged. I merely added
 @todo near the temporary constants definitions.

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