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:
                Type:  enhancement   |  marcin
            Priority:  medium        |                       Status:
           Component:  dhcp4         |  reviewing
            Keywords:                |                    Milestone:  DHCP-
           Sensitive:  0             |  Kea1.0-alpha
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  4             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  4
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tmark):

 * owner:  tmark => marcin


Comment:

 Replying to [comment:7 marcin]:
 > 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)
 > }}}
 >

 I think this is sufficient.

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

 As I said, simply my own philosophy.

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

 Looks good.  This is much cleaner.

 >
 > >
 ------------------------------------------------------------------------------
 > >
 > > Code passes cppcheck.sh, unit tests pass valgrind.
 >
 > Thank you QA Team! :) I really appreciate your efforts here!
 >

 Wait til you get my bill!


 Changes look good.  Please merge.

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