BIND 10 #2326: Allocation engine v6: release

[BIND 10 Development]

#2326: Allocation engine v6: release
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:
                Type:  task          |  stephen
            Priority:  medium        |                       Status:
           Component:  dhcp6         |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130103
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tomek):

 * owner:  tomek => stephen


Comment:

 Replying to [comment:6 stephen]:
 > Reviewed commit 6ce0831748423ab48611785f29ee999beeaf8322
 >
 > '''src/bin/dhcp6/dhcp6_messages.mes'''[[BR]]
 > I've made some changes here and pushed the result.  However:
 Thank you.

 > DHCP6_DB_ERROR_LEASE6_WITHOUT_DUID: if the workaround is to remove the
 lease entry from the database, why does the software not do that
 regardless?
 Because the reason for this is unknown. Just wiping it out would
 potentially mask a problem. I think there could be 2 likely cases that
 could result to that condition:
 1. Someone modified the record to hold empty DUID (BTW do we have tests
 that try to read record with duid field set to NULL?)
 2. a backend has a bug that returns null DUIDs.

 If we had removed the entry and the reason was 1, we would be essentially
 saying "that's ok that you're messing with the database, we can recover
 from that, carry on as you please". This is potentially dangerous
 situation. Logging a warning message is warranted here in my opinion. And
 I hope is it annoying to the user.

 > In summary, the name "DHCP6_LEASE_WITHOUT_DUID" is suggested.
 Agree. Changed.


 > '''src/bin/dhcp6/dhcp6_srv.cc'''[[BR]]
 > Line 715: "This should not happen. We have checked this before."  This
 seems to indicate a software failure and I would suggest that a message be
 logged at this point (with at least WARNING severity).
 Added.

 > Line 735: the "default" clause does not need a "break" - it could just
 drop through. (Leave the "default" clause there though with a comment to
 note that the remaining cases have been considered.)
 But it needs something after the label. Added semicolon.

 > Line 780: suggest that this be a DEBUG message - a malicious client
 could trigger a flood of warning messages by sending release messages for
 random addresses. (It is assumed that by default, WARNING messages are
 enabled and DEBUG messages disabled.)
 I disagree. This is a (serious) violation of RFC and we should log it.
 Nobody would notice a log on DEBUG level. I've lowered message severity to
 INFO. Does it look acceptable?

 > Line 828: correction - s/necessary if/necessary to check if/
 Done.

 > Line 833: looking at the code, a failure here is not a database error
 (as indicated by the message for DHCP6_RELEASE_FAIL).  A database error
 would trigger an exception: deleteLease() returning false indicates that
 there was no lease with the given address in the database.  As getLease6
 has established that the address is present, the implication is that
 something else has deleted the lease in the meantime.
 Ok. Updated explanation of the message. So what would the mysql backend
 do, if it has access to the database in read-only mode? Return false or
 throw?

 > '''src/bin/dhcp6/dhcp6_srv.h'''[[BR]]
 > Header for releaseIA_NA does not quite parse.  How about:
 > {{{
 > As RFC 3315 requires that a single status code be sent for the whole
 message,
 > this method may update the passed general_status: it is set to SUCCESS
 when
 > message processing begins, but may be updated to some error code if the
 > release process fails.
 > }}}
 Updated.

 > '''src/bin/dhcp6/tests/dhcp6_srv_unittest.cc'''[[BR]]
 > !Dhcpv6SrvTest(s): instead of using a scope_ptr, why not declare
 !NamedDhcpv6Srv as an automatic variable?
 I don't like it much, because I can't control creation/destruction moment
 with automatic variable. If I try:

 ASSERT_NO_THROW(Dhcpv6Srv srv(0));

 The srv variable is valid only within ASSERT_NO_THROW scope and is
 destroyed immediately afterwards. But ASSERT_NO_THROW() macro does not
 have much sense anyway. Any unexpected throws will abort the test anyway.

 > Line 1245: "check that the packets originating from local addresses can
 be" what?
 Comment rewritten.

 > !ReleaseReject: the last statement before case 1 adds a server ID to the
 renew packet with the comment that it is mandatory in a RENEW.  Is there a
 test for the case where it is absent?
 It is tested indirectly. First call in processRelease() (or any other
 processX method) is to call sanityCheck(). This method is checked. But I
 agree that dedicated tests could be developed for that. Added @todo in the
 code as this is a broad topic, not just a single test. If you multiply
 absent/present/than one instance by forbidden/optional/mandatory by number
 of messages, you'll get quite a number of cases to cover. And such
 negative cases could be more subtle than that, e.g. renew with empty IA,
 renew without IA at all.

 Changes pushed. Please confirm that all is ok (or point out new issues).

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