BIND 10 #3282: Implement use of DHDP-DDNS configuration parameters in b10-dhcp4

[BIND 10 Development]

#3282: Implement use of DHDP-DDNS configuration parameters in b10-dhcp4
-------------------------------------+-------------------------------------
            Reporter:  tmark         |                        Owner:  tmark
                Type:  enhancement   |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp-ddns     |                    Milestone:  DHCP-
            Keywords:  task 1.6.1    |  Kea0.9
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  24 hours      |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  40
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by marcin):

 * owner:  marcin => tmark


Comment:

 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.

 '''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?

 '''src/lib/dhcpsrv/cfgmgr.cc'''

 Please update the copyright header.

 Spurious new line after getD2ClientMgr function.

 '''src/lib/dhcpsrv/cfgmgr.h'''

 Please update a copyright header.

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

 Technically, a mask variable could be const.

 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.

 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.

 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.

 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.

 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?

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


 '''src/lib/dhcpsrv/d2_client.h'''
 analyzeFqdn: The function description could be extended to reference
 appropriate RFCs.

 Also, doxygen has its notation for marking output parameters:
 {{{
 /// @param [out] server_s  S Flag for the server's FQDN
 }}}

 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?

 The anaylyzeFqdn could have a throw statement informing that it throws
 !Unexpected exception if there is an invalid combination of flags
 provided.

 Why template parameters are named using all capital letters?
 Also, template parameters should be described in the functions
 documentation using !''tparam!'' tag.

 adjustFqdnFlags and adjustDomainName: the first argument to this function
 could be const as these functions are not meant to modify them.

 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.

 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.

 Do we want it to be unit tested?

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

 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.

 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.

-- 
Ticket URL: <http://bind10.isc.org/ticket/3282#comment:4>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development