Opened 4 years ago

Closed 4 years ago

#4259 closed enhancement (complete)

Consider adding configuration option to force DNS updates for clients not sending hostname

Reported by: marcin Owned by: tmark
Priority: medium Milestone: Kea1.1
Component: dhcp Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 24 Add Hours to Ticket: 0.5
Total Hours: 31.5 Internal?: no

Description

As highlighted on the Kea-users list: https://lists.isc.org/pipermail/kea-users/2016-January/000186.html some clients do not send hostname (or FQDN) option to the server. The server configured to send DNS updates currently expects the client to send one of those options to update DNS. Though, it may be desired to update DNS even for the clients which don't send Hostname (or FQDN) option, e.g. udhcp.

We should consider providing a configuration option to Kea to force DNS updates for clients which don't send their hostname.

Subtickets

Change History (10)

comment:1 Changed 4 years ago by tomek

  • Milestone changed from Kea-proposed to Kea1.1

As discussed on Kea-2016-02-03 call, moving this to Outstanding.

comment:2 Changed 4 years ago by tmark

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

comment:3 Changed 4 years ago by tmark

  • Add Hours to Ticket changed from 0 to 24
  • Estimated Difficulty changed from 0 to 24
  • Owner changed from tmark to Unassigned
  • Status changed from assigned to reviewing
  • Total Hours changed from 0 to 24

The parameter, replace-client-name, has been changed from a boolean to a list of modes:

  • "never" - never replace the name sent by the client, if they did not send one do not

generate one for them

  • "always" - always replace the name sent by the client, if they did not send one, generate one for them
  • "when_present" - replace the client name if they sent one, if they did not send one do not generate one for them
  • "when_not_present" - only generate the client name if they did not send one

Boolean values are still accepted, true equates to "when_present", false equates to "never"

I suggest the following for ChangeLog

10xx.   [func]      tmark
    The DDNS parameter, replace-client-name, has been changed from a boolean
    to list of modes, which provides greater flexibility in when the Kea 
    servers replace or supply DNS names for clients.  This is supported both
    kea-dhcp4 and kea-dhcp6.
    (Trac #4529, git TBD)

comment:4 Changed 4 years ago by tmark

  • Owner changed from Unassigned to tmark

comment:5 Changed 4 years ago by tmark

  • Owner changed from tmark to Unassigned

comment:6 Changed 4 years ago by marcin

  • Owner changed from Unassigned to marcin

comment:7 follow-up: Changed 4 years ago by marcin

  • Add Hours to Ticket changed from 24 to 3
  • Owner changed from marcin to tmark
  • Total Hours changed from 24 to 27

Reviewed commit fe77538dc446fd37a87d6a915a20b17f75f7cc4f

This built and passed tests on my OS-X.

doc/guide/dhcp4-srv.xml
In the following sentence: "The basic rules for constructing the FQDN that will be used for DNS entries are:". Shouldn't it be "The default rules.... " ?

OLD:
Replace the name the client sent. If the client sent no name, generate one for them.

NEW:
Replace the name the client sent. If the client sent no name, generate one for the client.

Same proposed change for "when_not_present" case.

I'd suggest that configuration parameter values, such as "when_not_present", "when_present" etc. use hyphen rather than underscore, so: "when-not-present", "when-present" etc. I think we generally follow the rule of using hyphens for parameter names. It would be good to be consistent with respect to values of parameters.

Note that for option names we already use hyphens, not underscores, and option names are values of parameters, so we already have a precedent.

Further reviewing the changes I noticed that you use upper case for these values. This is clearly not consistent with the documentation. But I'd still think that lower case and hyphens are what I'd prefer here, because it is generally more consistent with other parameters we already have.

doc/guide/dhcp6-srv.xml

OLD:
kea-dhcp6 can be configured to supply a portion or all of that name based upon what it receives from the client in the DHCP REQUEST.

NEW:
kea-dhcp6 can be configured to supply a portion or all of that name based upon what it receives from the client.

Note that there is no DHCP REQUEST in DHCPv6. There is a Request message.

src/bin/dhcp4/dhcp4.spec
Default value for "replace-client-name" is lower case, while throughout the code you use upper case.

src/bin/dhcp4/dhcp4_messages.mes
% DHCP4_SUPPLY_HOSTNAME %1: client did not send hostname option, will supply onefor them

"onefor" and "them" don't sound quite right.

src/bin/dhcp4/dhcp4_srv.cc
src/bin/dhcp4/tests/config_parser_unittest.cc

src/bin/dhcp4/tests/fqdn_unittest.cc
testReplaceClientNameMode: I suggest that a call to configure() is enclosed in ASSERT_NO_THROW or try-catch clause with ADD_FAILURE() << "bla bla bla". Otherwise the tests will terminate due to uncaught exception and following tests will not run.

Because there is quite a bit of conditional logic in this test, I think it might be good to add some more detailed error messages to explain the reason for failure. For example:

// Verify the contents (or lack thereof) of the hostname
if (exp_replacement_flag == NAME_REPLACED) {
    ASSERT_TRUE(hostname) << "Expected that the hostname supplied by the client"
        " will be replaced with a server supplied hostname, but no hostname is"
        " present";
    EXPECT_EQ(".", hostname->getValue());
} else {
   if (client_name_flag == CLIENT_NAME_PRESENT) {
       ASSERT_TRUE(hostname) << "Expected that client supplied hostname is preserved"
           " when the server is not configured to replace the hostname, but no hostname"
           " is found in the server's output";
       EXPECT_EQ("my.example.com.", hostname->getValue());
   } else {
       ASSERT_FALSE(hostname);
   }

or something like that.

processHostname and verifyNameChangeRequest should have doxygen documentation. I know that it doesn't strictly belong to this ticket but you touched them, so perhaps it is a right time to document their parameters?

src/bin/dhcp6/dhcp6.spec
Default value for "replace-client-name" is lower case, while throughout the code you use upper case.

src/bin/dhcp6/dhcp6_messages.mes
% DHCP6_DDNS_SUPPLY_FQDN %1: client did not send a FQDN option, will supply one for them

"them" doesn't sound right when referring to a client

src/bin/dhcp6/dhcp6_srv.cc
Unexpected termination of a comment in the following section:

// No FQDN so lease hostname comes from host reservation if one
if (ctx.host_) {
    ctx.hostname_ = ctx.host_->getHostname();
}

src/bin/dhcp6/tests/config_parser_unittest.cc
src/bin/dhcp6/tests/fqdn_unittest.cc
src/lib/dhcpsrv/d2_client_cfg.cc
src/lib/dhcpsrv/d2_client_cfg.h
I am not terribly excited with the stringToReplaceClientNameMode and replaceClientNameModeToString being defined outside of the D2ClientConfig class. I would rather make them static members of that class as they convert to/from values declared within the class. But, if you want to leave that, I won't object.

I think you may have cppcheck warnings with enum function arguments passed by const value, like this:

const ReplaceClientNameMode replace_client_name_mode

cppcheck wants enums either be passed by const reference or by non-const value. Personally I pass them by const reference because in C++11 enums may be represented by more complex types then integer, e.g strings in which case it makes sense to pass them by const reference.

src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc
Should there be a test case that verifies that invalid value of the "replace-client-name" parameter causes an error?

comment:8 in reply to: ↑ 7 Changed 4 years ago by tmark

  • Owner changed from tmark to marcin
  • Total Hours changed from 27 to 30

Replying to marcin:

Reviewed commit fe77538dc446fd37a87d6a915a20b17f75f7cc4f

This built and passed tests on my OS-X.

Ship it!

doc/guide/dhcp4-srv.xml
In the following sentence: "The basic rules for constructing the FQDN that will be used for DNS entries are:". Shouldn't it be "The default rules.... " ?

Got it.

OLD:
Replace the name the client sent. If the client sent no name, generate one for them.

NEW:
Replace the name the client sent. If the client sent no name, generate one for the client.

Same proposed change for "when_not_present" case.

Debatabled but I changed it.

I'd suggest that configuration parameter values, such as "when_not_present", "when_present" etc. use hyphen rather than underscore, so: "when-not-present", "when-present" etc. I think we generally follow the rule of using hyphens for parameter names. It would be good to be consistent with respect to values of parameters.

Note that for option names we already use hyphens, not underscores, and option names are values of parameters, so we already have a precedent.

I keep forgetting the hyphen not undescore. Sadly, I have to live with poor choice of hypenating everything so I've changed it.

Further reviewing the changes I noticed that you use upper case for these values. This is clearly not consistent with the documentation. But I'd still think that lower case and hyphens are what I'd prefer here, because it is generally more consistent with other parameters we already have.

The parameter value comparision is case insensitive, but I've made it consistently lower case.

doc/guide/dhcp6-srv.xml

OLD:
kea-dhcp6 can be configured to supply a portion or all of that name based upon what it receives from the client in the DHCP REQUEST.

NEW:
kea-dhcp6 can be configured to supply a portion or all of that name based upon what it receives from the client.

Note that there is no DHCP REQUEST in DHCPv6. There is a Request message.

Got it. Also fixed a couple of other spots.

src/bin/dhcp4/dhcp4.spec
Default value for "replace-client-name" is lower case, while throughout the code you use upper case.

Fixed above.

src/bin/dhcp4/dhcp4_messages.mes
% DHCP4_SUPPLY_HOSTNAME %1: client did not send hostname option, will supply onefor them

"onefor" and "them" don't sound quite right.

How's this?

% DHCP4_GENERATE_FQDN %1: client did not send a FQDN or hostname; FQDN will be be generated for the client

src/bin/dhcp4/dhcp4_srv.cc
src/bin/dhcp4/tests/config_parser_unittest.cc

src/bin/dhcp4/tests/fqdn_unittest.cc
testReplaceClientNameMode: I suggest that a call to configure() is enclosed in ASSERT_NO_THROW or try-catch clause with ADD_FAILURE() << "bla bla bla". Otherwise the tests will terminate due to uncaught exception and following tests will not run.

Because there is quite a bit of conditional logic in this test, I think it might be good to add some more detailed error messages to explain the reason for failure. For example:

// Verify the contents (or lack thereof) of the hostname
if (exp_replacement_flag == NAME_REPLACED) {
    ASSERT_TRUE(hostname) << "Expected that the hostname supplied by the client"
        " will be replaced with a server supplied hostname, but no hostname is"
        " present";
    EXPECT_EQ(".", hostname->getValue());
} else {
   if (client_name_flag == CLIENT_NAME_PRESENT) {
       ASSERT_TRUE(hostname) << "Expected that client supplied hostname is preserved"
           " when the server is not configured to replace the hostname, but no hostname"
           " is found in the server's output";
       EXPECT_EQ("my.example.com.", hostname->getValue());
   } else {
       ASSERT_FALSE(hostname);
   }

or something like that.

Done.

processHostname and verifyNameChangeRequest should have doxygen documentation. I know that it doesn't strictly belong to this ticket but you touched them, so perhaps it is a right time to document their parameters?

Sigh, yes, you're right. Done.

src/bin/dhcp6/dhcp6.spec
Default value for "replace-client-name" is lower case, while throughout the code you use upper case.

Right, got it already.

src/bin/dhcp6/dhcp6_messages.mes
% DHCP6_DDNS_SUPPLY_FQDN %1: client did not send a FQDN option, will supply one for them

"them" doesn't sound right when referring to a client

How this?

% DHCP6_DDNS_GENERATE_FQDN %1: client did not send a FQDN option; FQDN will be
generated for the client.

src/bin/dhcp6/dhcp6_srv.cc
Unexpected termination of a comment in the following section:

// No FQDN so lease hostname comes from host reservation if one
if (ctx.host_) {
    ctx.hostname_ = ctx.host_->getHostname();
}

Actually I think that was the intended end but I made it a little clearer for you.

src/bin/dhcp6/tests/config_parser_unittest.cc
src/bin/dhcp6/tests/fqdn_unittest.cc
src/lib/dhcpsrv/d2_client_cfg.cc
src/lib/dhcpsrv/d2_client_cfg.h
I am not terribly excited with the stringToReplaceClientNameMode and replaceClientNameModeToString being defined outside of the D2ClientConfig class. I would rather make them static members of that class as they convert to/from values declared within the class. But, if you want to leave that, I won't object.

Done.

I think you may have cppcheck warnings with enum function arguments passed by const value, like this:

const ReplaceClientNameMode replace_client_name_mode

cppcheck wants enums either be passed by const reference or by non-const value. Personally I pass them by const reference because in C++11 enums may be represented by more complex types then integer, e.g strings in which case it makes sense to pass them by const reference.

src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc

Done.

Should there be a test case that verifies that invalid value of the "replace-client-name" parameter causes an error?

Added it to dhcp_parsers_unittest.cc:ParseConfigTest.invalidD2Config

comment:9 follow-up: Changed 4 years ago by marcin

  • Add Hours to Ticket changed from 3 to 1
  • Owner changed from marcin to tmark
  • Total Hours changed from 30 to 31

Reviewed commit 773659ff2359640392a4e6b0dc07cdbe792d4db3

Two minor issues. After fixing them, please merge.

doc/guide/dhcp{4,6}-srv.xml
The following note": Boolean values will still be accepted but may eventually be deprecated. A value of true equates to when_present, false equates to never. still uses underscore.

src/bin/dhcp4/tests/fqdn_unittest.cc
OLD:
Invokes Dhcpsrv4::processHostname on the given packet
NEW:
Invokes Dhcpv4Srv::processHostname on the given packet

comment:10 in reply to: ↑ 9 Changed 4 years ago by tmark

  • Add Hours to Ticket changed from 1 to 0.5
  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 31 to 31.5

Replying to marcin:

Reviewed commit 773659ff2359640392a4e6b0dc07cdbe792d4db3

Two minor issues. After fixing them, please merge.

doc/guide/dhcp{4,6}-srv.xml
The following note": Boolean values will still be accepted but may eventually be deprecated. A value of true equates to when_present, false equates to never. still uses underscore.

Fixed

src/bin/dhcp4/tests/fqdn_unittest.cc
OLD:
Invokes Dhcpsrv4::processHostname on the given packet
NEW:
Invokes Dhcpv4Srv::processHostname on the given packet

Fixed

Changes merged with git 45e56d7aa0d4a6224a1a28941f6cb11575391222
Added ChangeLog entry 1104.

This ticket is complete.

Note: See TracTickets for help on using tickets.