BIND 10 #3282: Implement use of DHDP-DDNS configuration parameters in b10-dhcp4
BIND 10 Development
do-not-reply at isc.org
Wed Jan 29 14:56:08 UTC 2014
#3282: Implement use of DHDP-DDNS configuration parameters in b10-dhcp4
-------------------------------------+-------------------------------------
Reporter: tmark | Owner:
Type: enhancement | marcin
Priority: medium | Status:
Component: dhcp-ddns | reviewing
Keywords: task 1.6.1 | Milestone: DHCP-
Sensitive: 0 | Kea0.9
Sub-Project: DHCP | Resolution:
Estimated Difficulty: 24 hours | CVSS Scoring:
Total Hours: 50 | Defect Severity: N/A
| Feature Depending on Ticket:
| Add Hours to Ticket: 4
| Internal?: 0
-------------------------------------+-------------------------------------
Changes (by tmark):
* hours: 6 => 4
* owner: tmark => marcin
* totalhours: 46 => 50
Comment:
Replying to [comment:4 marcin]:
> Reviewed commit d38e7638f9ecafc4e0db3a6b9dcad7a808c49781
>
> '''src/bin/dhcp4/dhcp4_srv.cc'''
> Line 1339: So, there is a chance that server doesn't send NCR for
removal when configured not to do so? I was not aware of this capability.
>
I added this configuration option with the notion that no removal is
necessary on renewals as Per RFC 4703 section 5.3, D2 will remove any
entries for a given FQDN if they exist before adding them. However, I see
now that this test at 1339 is not that case, this is an outright release,
so it would not apply here. It should only check if DDNS is enabled.
Further, in looking at the code 839 which would be dealing with renewals,
I see it doens't apply there either.
Per our discussion on this, I have removed the remove-on-renewal flag.
> '''src/bin/dhcp4/tests/fqdn_unittest.cc'''
>
> generatedNameFromAddress: Does it make any sense to validate the
generation of the FQDN with the same function that is being (partially)
validated? Or I missed something?
>
No you didn't miss anything. The method generateFqdn is tested under in
libdhcpsrv unit testing. What we are validating here is not the name
generation method but that the NCR contains the generated name. In other
words, did the server follow the code path to populate the NCR with the
generated FDQN as expected?
The alternative would be to copy and paste the contents of
D2ClientMgr::generateFqdn() into
NameDhcp4SrvTest::generatedNameFromAddress() but then the two must be kept
in sync.
> '''src/lib/dhcpsrv/cfgmgr.cc'''
>
> Please update the copyright header.
I thought I did. Oh well. Done.
>
> Spurious new line after getD2ClientMgr function.
>
WHAT WAS I THINKING???? Got it.
> '''src/lib/dhcpsrv/cfgmgr.h'''
>
> Please update a copyright header.
De ja vu. I thought I did. Oh well. Done.
>
> '''src/lib/dhcpsrv/d2_client.cc'''
> analyzeFqdn: typos !''secion!'' in a commentary.
>
> The commentary opening a logic in this function is very descriptive. As
it refers to both RFC4702 and RFC4704 you may consider to mention that
cited sections (e.g. section 3.2, 3.3 and 3.4) belong to RFC4702, not to
RFC4704 but the latter one has corresponding sections which talk pretty
much about the same thing.
Done.
>
> Technically, a mask variable could be const.
Picky picky. Jeepers. In reality I could have just put the mask
expression
in the switch() but I found that less readable.
>
> I was going to say that this:
> {{{
> server_n = !server_s;
> }}}
> is disputable.
>
> After our dicussion on jabber I understand why it is so. I suggest that
the todo paragraph from line 200 is moved to this assignment so as it is
clear that we don't do reverse updates if forward update is delegated.
>
Ok, I moved the comments.
> I also suggest that this behavior is documented for the upcoming
release. I am not sure what DDNS documentation is planned for the release
but probably b10-guide or b10 developer's guide would be good places for
that.
>
There are two documentation tickets, 3158 (developer's guide),
3283 (bind10 guide)
> generateFqdn: I don't understand why you have to create a temporary
variable !''tmp!'' instead of this:
> {{{
> return (qualifyName(gen_name.str()));
> }}}
>
> This makes it more readable what you're !''qualifying!'' (it is a
generated name). The name for the temporary variable !''tmp!'' is not
meaningful. If you want to keep it, I suggest renaming.
>
I wanted to see if you were paying attention. Changed as suggested.
> qualifyName: There seem to be no reason to convert the STL string to a C
string to access its last element and the length. The std::string has
operator [] and length() function which should do what you need.
>
Oops. Done.
> Question: This function makes use of the getQualifyingSuffix() function.
The D2ClientConfig's constructor doesn't seem to validate this value,
which in turn may result in faulty hostname being generated. Is this
validated at the configuration parser level? SHouldn't it be validated in
the !D2ClientConfig::validateContents?
Stephen has suggested to me before that if we are given values that should
be FQDNs but happen to be missing the last ".", we should append it for
them. I suppose I took that same approach here. We could just append it
during parsing, if it's missing or we should proclaim an error.
Also what is valid and invalid? If my clients always have FQDNs then is
"" valid? Why should sysadmin fill it in if they don't need it? I
suppose if I try to use it I should get an exception though.
I think a lot of this is because we really don't have much in the way of
requirements for these. I am open to discussing what we think the rules
should be and then enforcing them.
>
> And, with respect to this todo:
> {{{
> // @todo perhaps more validation we should do yet?
> // Are there any invalid combinations of options we need to test
against?
> // Also do we care about validating contents if it's disabled?
> }}}
>
> This seems like a good place to validate the class members. It could be
utilized by the configuration parser to check that all these settings are
valid. And yes! The configuration should be validated, even if disabled.
If I am administrating the server, I will not enable the service before I
have it fully configured. When it is configured I can enable the service
with a single parameter change - this would be surprising if enabling DDNS
would result in configuration failure. :-)
>
Of course it's a good place we just don't have much in the way of
requirements for what the rules should be. I would be more than happy to
add rules.
The last part about whether is disabled or not is actually obsolete. This
method always gets called by the constructors.
>
> '''src/lib/dhcpsrv/d2_client.h'''
> analyzeFqdn: The function description could be extended to reference
appropriate RFCs.
Added.
>
> Also, doxygen has its notation for marking output parameters:
> {{{
> /// @param [out] server_s S Flag for the server's FQDN
> }}}
Ok, got it.
>
> Is analyzeFqdn meant to be called externally? Shouldn't it be moved to
protected scope? Do you think that there are some other functions that
could be moved to protected or private scope?
>
Yes, it is meant to be called externally.
> The anaylyzeFqdn could have a throw statement informing that it throws
!Unexpected exception if there is an invalid combination of flags
provided.
It should yes, I have added it. I also changed the type of exception to
be BadValue.
>
> Why template parameters are named using all capital letters?
Because nobody said to do it one way or another least this way they were
easy to spot.
I changed them based on general consensus on the internet.
> Also, template parameters should be described in the functions
documentation using !''tparam!'' tag.
I looked for examples in other code. Must have missed them.
>
> adjustFqdnFlags and adjustDomainName: the first argument to this
function could be const as these functions are not meant to modify them.
Do you have a special tool just for this? Fixed.
>
> The documentation should mention that the input E flag is ignored by
adjustFqdnFlags but also that it actually resets this flag in the response
(along with other flags). It would be better to preserve this flag but it
brings some additional complexity to the templated function so I am fine
with having appropriate comment.
The E-flag is only supported by v4, so I left it up the v4 code to
preserve it as it did before.
I updated the commentary accordingly.
>
> adjustDomainName: On reflection, I think I should have implemented
empty() function for !Option4ClientFqdn and !Option6ClientFqdn to avoid
returning the whole fqdn string to check that it is empty. But, this is
another story. We could probably do it when we will be merging both
classes.
>
> '''src/lib/dhcpsrv/dhcp_parsers.cc'''
> I understand the intent behind abbreviated configuration and I believe
it makes it convenient for an administrator to disable DDNS. This solution
(together with whole configuration) deserves documentation in b10-guide. I
see it is not yet documented.
>
Documentation forthcoming under aforementioned trac tickets.
> Do we want it to be unit tested?
It is unit tested here:
dhcp_parsers_unittest.cc TEST_F(ParseConfigTest, validDisabledD2Config)
>
> '''src/lib/dhcpsrv/tests/d2_client_unittest.cc'''
>
> Tests that verify the behavior of analyzeFqdn look ok, except that there
is no test case that checks it return Unexpected when invalid combination
of flags is used.
>
Very true. When I first wrote that it accepted FQDN objects which cannot
provide the invalid
combination. I have added the test:
TEST(D2ClientMgr, analyzeFqdnInvalidCombination)
> qualifyName: There are no negative unit tests for this function: empty
name or a name that consists of dots only etc. Should they be covered
here? The code doesn't protect against negative scenarios but the function
is in the public scope of the class so it may be used externally. If this
function is not meant to validate the input data it could be mentioned in
its documentation. In this case, it should be fine to leave a test as it
is.
>
> The same question refers to other functions in the public scope of this
class.
Again we lack of requirements as to what is valid and invalid. When we do
define rules we
should enforce them in the validContents() and such.
>
> adjustDomainNameV6: This comment is aside of the purpose of this test
but I just had a thought that since it is a V6 test it would be good to
create an instance of D2ClientConfig with V6 address of the b10-dhcp-ddns
module. It could even be a separate constructor test.
Now why didn't I think of that? OH WAIT I DID! please see
TEST_F(ParseConfigTest, validD2Config) in dhcp_parsers_unittest.cc line
764 or so.
--
Ticket URL: <http://bind10.isc.org/ticket/3282#comment:6>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list