Opened 7 years ago

Closed 6 years ago

#3035 closed enhancement (complete)

Modify DHCP4 to generate NameChangeRequest

Reported by: marcin Owned by: marcin
Priority: medium Milestone: Kea0.8
Component: ~dhcp-ddns(obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description (last modified by marcin)

DHCP4 server should be act upon FQDNs sent by clients. If requested, it should generate NameChangeRequests that would in turn result in sending DNS Updates.

Subtickets

Change History (15)

comment:1 Changed 7 years ago by marcin

  • Description modified (diff)

comment:2 Changed 7 years ago by tmark

This ticket corresponds to Task 2.3 in the D2 Development list.

comment:3 Changed 7 years ago by tomek

  • Milestone changed from Sprint-DHCP-20130821 to Sprint-DHCP-20130904

comment:4 Changed 7 years ago by marcin

  • Owner set to marcin
  • Status changed from new to assigned

comment:5 Changed 7 years ago by tomek

  • Milestone changed from Sprint-DHCP-20130904 to Sprint-DHCP-20130918

comment:7 Changed 7 years ago by marcin

  • Owner changed from marcin to UnAssigned
  • Status changed from assigned to reviewing

The both Client FQDN and Host Name options are now processed. The relevant configuration is described by the const values defined in dhcp4_srv.cc. These constants will be replaced by the actual configuration once DDNS configuration for DHCP is implemented.

Proposed ChangeLog:

670.	[func]		marcin
	b10-dhcp4: Server processes the DHCPv4 Client FQDN and Host Name
	options sent by a client and generates the response. as a result
	of processing, the server generates NameChangeRequests which
	represent changes to DNS mappings for a particular lease (addition
	or removal of DNS mappings).
	Currently all generated NameChangeRequests are dropped. Sending
	them to b10-dhcp-ddns will be implemented with the future tickets.
	(Trac #3035, git abcd)

comment:8 Changed 6 years ago by tomek

  • Milestone changed from Sprint-DHCP-20130918 to Sprint-DHCP-20131016

comment:9 Changed 6 years ago by tmark

  • Owner changed from UnAssigned to tmark

comment:10 follow-up: Changed 6 years ago by tmark

  • Owner changed from tmark to marcin

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.

fqdn_unittest.cc:

Extraneous whitespace. (HA! I finally busted you on this!)


dhcp4_srv.h

Method commentary for computeDhcid seems a bit thin.
Also, it is lacking @throw.


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.


dhcp4_srv.cc

add an "@todo" tag at line 81, where it discusses constants being replaced.


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.


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.

  1. We may also wish to make it configurable that we always update, whether it

is is renewal or not.


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


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.


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?

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


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


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

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.

comment:11 Changed 6 years ago by tmark

I neglected to mention that it passed unit tests passed valgrind, and the code passed cppcheck.

comment:12 Changed 6 years ago by tomek

  • Milestone changed from Sprint-DHCP-20131016 to Sprint-DHCP-20131204

comment:13 in reply to: ↑ 10 Changed 6 years ago by marcin

  • Owner changed from marcin to tmark

Replying to 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.

  1. 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@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.

comment:14 Changed 6 years ago by tmark

  • Owner changed from tmark to marcin

Only a minor comment change in fqdn_unittest.cc:

Change the comment above processHostname() to state that it "will return a NULL pointer". At the moment it claims to "throw" a NULL pointer.


I like the not_strict_expire_check flag. Very clever.

Adding the unit test for label counting was a good idea.

Merge in your changes please, I do not need to see this ticket again.

comment:15 Changed 6 years ago by marcin

  • Resolution set to complete
  • Status changed from reviewing to closed

Merged with commit f617e6af8cdf068320d14626ecbe14a73a6da22

Note: See TracTickets for help on using tickets.