BIND 10 #3210: Kea4 must not echo back client-id

[BIND 10 Development]

#3210: Kea4 must not echo back client-id
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:  tomek
                Type:  defect        |                       Status:
            Priority:  very high     |  reviewing
           Component:  dhcp4         |                    Milestone:
            Keywords:                |  Sprint-DHCP-20131016
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  0             |              Defect Severity:  Low
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by marcin):

 * owner:  UnAssigned => tomek


Comment:

 Reviewed commit: 4a08e3662e7720ac6c96b7d585084bbe4033c29b

 '''doc/guide/bind10-guide.xml'''
 Apparently, the RFC6842 says that the client id must be echoed back to the
 client IF client id has been included in the client's message. I suggest
 that the bind10-guide is updated accordingly. Otherwise, it gives an
 impression that client-id must be always included.


 '''src/bin/dhcp4/config_parser.cc'''
 commitGlobalOptions: why is this function called like this? It merely
 stores the new !''echo-client-id!'' option in the !CfgMgr.

 Question: do you think it makes sense to have a per-subnet configuration
 of this new option? For the current use cases I am sure it is ok to have
 the global setting but I can imagine that in the future we may want to
 support a number of devices which are not RFC6842 conformant for which the
 client-id echoing should be disabled while for other devices it should be
 still enabled.

 '''src/bin/dhcp4/tests/config_parser_unittest.cc'''
 The echoClientId test is insufficient because it doesn't check the value
 of !''echo-client-id!'' flag before applying the configuration. It may be
 the case that the parser doesn't parse this new option at all and thus
 this check:
 {{{
     EXPECT_FALSE(CfgMgr::instance().echoClientId());
 }}}
 will always pass because the parser simply wouldn't modify this value. I
 think you should either set this value in the !CfgMgr to true before
 applying first configuration or you should swap the configurations (so as
 the one which sets it goes first) and prior to applying this configuration
 perform ASSERT_FALSE.

 '''src/bin/dhcp4/dhcp4_test_utils.h'''
 checkClientId: After recent changes to this function you should extend the
 description of this function to say that the function checks presence of
 the client-id if specified so in the !CfgMgr and checks the absence in
 other case. Otherwise it looks like you always expect that client-id is
 present.

 '''src/bin/dhcp4/tests/dhcp4_srv_unittest.cc'''
 Dhcpv4SrvTest::discoverEchoClientId: The test description is a bit
 ambiguous. It may be unclear what it means that the !''echoing back
 client-id!'' is controllable.

 The checks in line 705 and 713 look like duplicates of what the
 !''checkClientId!'' function is already doing.

 The same comments apply to Dhcpv4SrvTest::requestEchoClientId

 '''src/lib/dhcpsrv/cfgmgr.h'''
 I fixed a few trivial issues and pushed changes to the branch.

 '''src/lib/dhcpsrv/tests/cfgmgr_unittest.cc'''
 CfgMgrTest::echoClientId: Just a thought. The test could also check that
 the flag can be set to TRUE after setting it to FALSE. However, I admit
 that it is not very likely to fail :-)


 We need a !ChangeLog for this because it has a user impact.

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