BIND 10 #3036: Modify DHCP6 to generate NameChangeRequest
BIND 10 Development
do-not-reply at isc.org
Wed Aug 14 09:19:14 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
-------------------------------------+-------------------------------------
Comment (by marcin):
Replying to [comment:5 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
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.
>
> 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.
>
> '''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.
>
>
> 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.
>
>
> 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.
>
>
> '''option6_client_fqdn.cc'''
> Option6CXlientFqdnImpl and its methods should be documented.
I added documentation.
>
> 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.
>
> 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.
It is a pointer because when I set it to NULL it marks that the domain
name is empty. Please read through the class functions. This should
clarify how this pointer is used.
>
> 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.
>
> 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.
>
> parseWireData(): exception comment should say what's the option
> size (0) and what is the expected minimum (1).
Ok.
>
> 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?
>
> 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.
>
> setFlag(): the same as getFlag(). You are using enum as uint8_t here.
> I think the parameters should be uint8_t.
See above.
>
> 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.
The big endian is not an issue, unless you copy values to the memory. This
is not the cases 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).
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.
>
> 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.
>
> '''alloc_engine.h'''
> When updating copyright year, please use 2012-2013, not 2013.
Yes. It was accidental.
>
> '''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
>
> '''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.
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.
>
> '''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.
>
> General question for dhcp6_srv modifications: what will the server
> do if there is a client that sends 5 IA_NA options?
Each IA_NA is processed. Thus, at least 5 !NameChangeRequests will be
generated.
>
> appendClientFqdn(): please make it clear whether it is fqdn
> send by the client or the server's response
It is now clearer.
>
>
> '''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.
Added.
>
> 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.
>
> 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.
It is not. I am converting hostname to the canonical wire format so as I
can generate DHCID using it (together with FQDN) a few lines down.
>
> 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.
Arguably, someone could tell that saying that we send NCRs while we
actually don't is also wrong. :-) But, I changed the comment. One or
another is fine by me.
>
> 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.
>
> 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.
>
> ----
> Rest of the review will continue tomorrow.
The other chunk of comments will be addressed separately.
--
Ticket URL: <http://bind10.isc.org/ticket/3036#comment:7>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list