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
-------------------------------------+-------------------------------------

Comment (by marcin):

 Replying to [comment:2 marcin]:
 > 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.

 One correction. The default value is true, so there is no reason to set it
 to true explicitly. However, it should be checked that the value IS true
 before applying configuration.

 >
 > '''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:3>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development