BIND 10 #3035: Modify DHCP4 to generate NameChangeRequest
BIND 10 Development
do-not-reply at isc.org
Wed Nov 20 17:05:03 UTC 2013
#3035: Modify DHCP4 to generate NameChangeRequest
-------------------------------------+-------------------------------------
Reporter: marcin | Owner: tmark
Type: enhancement | Status:
Priority: medium | reviewing
Component: dhcp-ddns | Milestone:
Keywords: | Sprint-DHCP-20131204
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
-------------------------------------+-------------------------------------
Changes (by marcin):
* owner: marcin => tmark
Comment:
Replying to [comment:10 tmark]:
> This work took a lot of research into the RFCs and thought to get the
logic together to handle all permutations. I like the way
fdqn_unittests.cc is structured.
Thanks!
>
>
> fqdn_unittest.cc:
>
> Extraneous whitespace. (HA! I finally busted you on this!)
In fact, not only did I have whitespace there but also the part of the
comment was missing.
>
>
-----------------------------------------------------------------------------
> dhcp4_srv.h
>
> Method commentary for computeDhcid seems a bit thin.
> Also, it is lacking @throw.
Fixed.
>
>
-----------------------------------------------------------------------------
> dhcp4_srv.h
>
> Method commentary for processClientFqdnOption seems a bit thin, same for
> processHostnameOption. You may wish to refer back to the description of
> processClientName.
>
> Alternatively, you could split up commentary above processClientName and
move
> the FQDN option discussion to processClientFqdnOption and the hostname
option
> discussion to processHostnameOption.
Fixed. It is better to keep the extensive comment in the public function
because the documentation for the private functions does not show up in
doxygen.
>
>
-----------------------------------------------------------------------------
> dhcp4_srv.cc
>
> add an "@todo" tag at line 81, where it discusses constants being
replaced.
Added.
>
>
----------------------------------------------------------------------------
> dhcp4_srv.cc
>
> Dhcpv4Srv::createNameChangeRequests
>
> lease_expires_on is set to 0. Lease expires on is the actual date/time
on
> which the lease expires. It is essentially time the lease was granted +
valid_lft.
> One way would be to add logic to calculate to the NCR constructor.
Another
> would be to calculate it in this method and pass it in.
Ok. I am now passing this value to the constructor.
>
>
----------------------------------------------------------------------------
> dhcp4_srv.cc
>
> Dhcpv4Srv::createNameChangeRequests
>
> There is some behavior here that we may wish to make configurable. If
> nothing else, maybe add this to the commentary?
>
> 1. Generating a delete and then an add - D2's state machine is geared
to
> first try an add and if that comes back with "FQDN in use", it is
supposed to
> issue a second request which deletes and then adds it. This is actually
per
> RFC 4703. It would be less work on DHCP4 but still only 2 transactions
with
> the server.
>
> 2. We may also wish to make it configurable that we always update,
whether it
> is is renewal or not.
I added a comment to the constants that currently define the configured
behaviour of the server.
>
>
----------------------------------------------------------------------------
> dhcp4_srv.cc
>
> queueNameChangeRequset
>
> Please check blank lines or lack thereof against coding guidelines in
this
> method. You seem have blank lines were you shouldn't and not where you
> should ;).
Fixed.
>
>
-----------------------------------------------------------------------------
> dhcp4_srv.cc
>
> queueNameChangeRequset
>
> Suggestion, you could reduce the two debug log statements into one by
doing
> something like this:
>
> {{{
> LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL_DATA,
> DHCP4_QUEUE_NCR)
> .arg(chg_type == CHG_ADD ? "add" : "remove")
> .arg(ncr.toText());
> }}}
>
> where the message is this:
>
> {{{
> % DHCP4_QUEUE_NCR name change request to %1 DNS entry queued: %2
> }}}
>
> Makes for a bit less code to look at and one less log message.
Reduced as suggested.
>
>
---------------------------------------------------------------------------
> dhcp4_srv.cc
>
> processHostnameOption
>
> I'm not certain if hostname string from opt_hostname->readString() is
> guaranteed to not be empty, but what if it were?
You're right. The Hostname option should be at least 1 byte long, but the
option class itself doesn't check that. This is correct behaviour of the
option class because in general it may represent options which are empty.
So, this function should do a check for empty options. I updated the
getLabelCount function in OptionDataTypeUtil to return 0 if the specified
name string is empty, which I thought would be a valid behaviour of this
function comparing to throwing an exception.
>
>
> Also, is there a unit test for an unqualified host name? Suppose the
> packet comes in with a hostname of "mycomputer".
>
> I think you need to re-examine using dns::Name.getLabelCount(). From my
> limited testing of this method, I do not beleive it counts labels the
way
> you think it does. This method, OptionDataTypeUtil::getLabelCount(),
> uses dns::Name(std::string) constructor and then calls
Name.getLabelCount().
>
> That constructor will throw if given "", ".", or ".sometext".
Additionally,
> it seems always return the number of labels + 1. A quick bit of test
code
> produced this:
>
> {{{
> For [bs at mycomputer] the label count is:2
> For [mycomputer] the label count is:2
> For [mycomputer.] the label count is:2
> For [mycomputer.tmark] the label count is:3
> For [mycomputer.tmark.] the label count is:3
> For [mycomputer.tmark.net.] the label count is:4
> }}}
>
> Where the string in brackets was passed to dns::Name() ctor and then
getLabelCount was invoked on the instance created.
I believe the rest of the code in this function is actually correct. The
getLabelCount function counts the root label !''.!'' and the function is
accepting the hostname containing only a root label. When root label is
given, the whole hostname will get auto generated from the client's IP
address.
I remember that we had a discussion that the getLabelCount throws an
exception for the hostname containing root label only, but I have actually
checked that it is not the case. I have even added an additional unit test
for this.
>
>
----------------------------------------------------------------------------
>
> dhcp4_messages.mes
>
> In the log message description:
>
> {{{
> % DHCP4_NCR_CREATION_FAILED failed to generate name change requests for
DNS: %1
> This message indicates that server was unable to generate so called
> NameChangeRequests which should be sent to the b10-dhcp_ddns module to
create
> }}}
>
> I would remove the phrase "so called". In English it can have negative
connotations:
>
>
> so-called: Incorrectly or falsely termed
> Example "My so-called friends were gossiping about me again."
>
> I think you meant something more like "known as" or "referred to as".
Removed !''so called!''.
>
>
----------------------------------------------------------------------------
>
> Developer's Guide:
>
> Again you used the phrase "so called". Also in this line you are
missing the
> word "or" after the word "preference":
>
> {{{
> configured to obey client's preference to do FQDN processing in a
different way
> }}}
Fixed.
>
>
---------------------------------------------------------------------------
>
> For future reference, the !NameChangeSender has an internal send queue.
DHCP4
> will not need to it's own FIFO. Really all it has to do once it
instantiates
> its !NameChangeSender is call its !sendRequest() method which places the
NCR on the sender's internal queue. The sends are asynchronous. When the
current send finishes, the sender will dequeue and send the next one
automatically.
>
>
I think it is going to be a work of someone integrating the
!NameChangeSender with the DHCP code to remove the queue that I have
added. In any case, we will then have to think how to adjust unit tests
because they make use of the queue to check what NCRs have been created.
Thanks for the review.
--
Ticket URL: <http://bind10.isc.org/ticket/3035#comment:13>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list