BIND 10 #3231: server-id for Kea4 does not work properly

[BIND 10 Development]

#3231: server-id for Kea4 does not work properly
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:  tmark
                Type:  enhancement   |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp4         |                    Milestone:  DHCP-
            Keywords:                |  Kea1.0-alpha
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  0             |              Defect Severity:  N/A
         Total Hours:  4             |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  4
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by marcin):

 * owner:  marcin => tmark


Comment:

 Replying to [comment:6 tmark]:
 > I think this is terrific solution. Good job.

 You're too kind :). Thanks!

 >
 > General:
 >
 > * Since server id file is now obsolete, this should perhaps be mentioned
 in the !ChangeLog?  Otherwise anyone running b10-dhcp4 now will have this
 file and it will be orphaned.  Either that or we have to come up with a
 way for it to get wiped out?

 How about this?
 {{{
 7XX.    [bug]           marcin
         b10-dhcp4: Different server identifiers are used for the packets
         being sent through different interfaces. The server uses IPv4
 address
         assigned to the particular interface as a server identifier. This
         guarantees that the unicast packet sent by a relay or a client, to
         the address being a server identifier, will reach the server.
         The file holding server identifier is unused for b10-dhcpv4 and
         may be removed for existing installations. For new installations
 it
         is not created.
         (Trac #3231, git abc)
 }}}

 >
 >
 -------------------------------------------------------------------------------
 >
 > src/bin/dhcp4/dhcp4_src.h
 >
 > Why are the methods adjustIfaceData, adjustRemoteAddr, and
 appendServerId declared static?  I saw comments on other existing static
 methods that say it is because they are not dependent upon server state.
 I don't agree with this approach. It makes it impossible to override them
 in deriving test fixtures.
 >
 > As a general rule I try to limit statics to tasks relegated to singleton
 start up, those uninteresting in terms of inheritance, or unaffected if
 there were multiple instances of an object.  Mind you this is my own
 philosophy, I know there are others.

 If we really want to make a function overrideable in the child class (say
 test fixture class) we should make it virtual. But, we don't do that for
 the functions we don't plan to override. In other words, if you want to
 override a certain function in the test fixture class, you have to modify
 its declaration anyway. So, I disagree with a point about overriding.

 The method being static is similar to making methods const. If we don't
 plan the certain function to modify the class state, we make it const.
 Although, making it non-const would still work for such function. By
 declaring a function static we can use this function (even unit test it)
 without making a new instance of the object which may slightly simplify
 the unit test code. Plus, we make API clearer saying: !''this function
 does not depend on object state!''.

 I wouldn't sacrifice my life for making object-independent functions
 static, but I also don't see a sufficient reason not to make it static.
 So, it is a judgment call. :)


 >
 >
 -------------------------------------------------------------------------------
 >
 > src/bin/dhcp4/dhcp4_src.cc
 >
 > Dhcpv4Srv::adjustIfaceData()
 >
 > *At line 1157, response->getHops() is used to determine which port value
 is used in the response.  In trac3203 (not yet merged),  tomek changed
 this decision to be based upon query->getRemotePort().  One of you appears
 to be wrong.  You may wish to discuss this with him.  Additionally,
 adjustRemoteAddr uses non-zero value of giaddr to detect relay messages,
 so even with this we are using two different ways to check for "relayed-
 ness".
 >

 I agree it should be consistent, so I followed your suggestion and created
 a new Pkt4 function to check whether the message is relayed or not. This
 function checks both Hops and Giaddr value. If the combination of these
 values is wrong it throws an exception. This exception will propagate to
 the !Dhcpv4Srv main loop and the packet will be discarded, which is a
 right behavior I suppose.

 >
 ------------------------------------------------------------------------------
 >
 > Code passes cppcheck.sh, unit tests pass valgrind.

 Thank you QA Team! :) I really appreciate your efforts here!

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