BIND 10 #2342: MySQL IPv6 lease database access functions

[BIND 10 Development]

#2342: MySQL IPv6 lease database access functions
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  tomek
  stephen                            |                Status:  reviewing
                       Type:  task   |             Milestone:  Sprint-
                   Priority:         |  DHCP-20121101
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  dhcpdb                             |           Sub-Project:  DHCP
                   Keywords:         |  Estimated Difficulty:  0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => tomek


Comment:

 comment:7 address in commit 89c8076272fb81a9e49b3bdbcd704103707db83f

 '''src/lib/dhcp/tests/Makefile.am'''
 > Make sure that the following lines are cleaned up before merging the
 code:
 Removed.

 >All !LeaseMgr-related files should be in libdhcpsrv_unittests, not
 libdhcp++_unittests.
 Moved.

 '''src/lib/dhcp/tests/mysql_lease_mgr_unittest.cc'''
 > You should add some sanity check as a first test that connects to the
 database and prints out extended error message with explanations that the
 test environment is broken if connection fails.
 A message is printed out with the first test stating that if it fails, the
 environment is broken.

 As an aside, I have extended the code to auto-reconnect - although this is
 mainly because the client will disconnect from the database if inactive
 for too long.

 > BasicLease6 test fails for me:
 This was due to the timezone problem (described above), now corrected.

 > !OpenDatabase test: I would check positive case (all parameters valid)
 first. It will be easier to debug. Now the information about failed
 connection does not give you much information.
 Done.

 > Also you need to precisely describe how to set up the environment. I can
 imagine that some test environment may take liberal approach and allow to
 connect to the database to anyone (or at least skip password
 requirements). That would make !OpenDatabase test to fail.
 Will do in the associated documentation.  Good point about the password.

 > I think we can leave the test as is, but we need some easy to run script
 that will set up the environment and will print out that MySQL access must
 be configure to require user and password.
 Agreed.

 > !CheckTimeConversion: This test does not check that the conversion was
 done properly, just that it is reversible. The code could mess up months
 with minutes and days with seconds and the test would still pass.
 > Please check boundary conditions. Verify that the expire time is
 properly calculated if...
 I'm a bit wary of doing that because of DST issues - setting a "struct tm"
 and converting to a time_t requires knowledge of whether DST is in effect
 at that time.  And the reverse conversion will use the local tables to
 calculate DST.  However, I've modified the test to check that the
 MYSQL_TIME produced is the one that I would calculate by adding cltt and
 the valid lifetime and converting using localtime_r.  The reverse
 conversion checks that it reproduces the original time_t value.

 > !CheckVersion: We should have defines for the current versions
 somewhere, probably in mysql_lease_mgr.h.
 Done.

 > printLease6() seems very useful. We could consider moving it to
 !LeaseMgr.
 Added it as lease6::toText() to fit it with other text conversion stuff.

 > compareLease6() - can this be implemented as an operator?
 Done.  Added both operator==() and operator!=().

 > I think we should defined two == operators:
 > one for comparing two Lease6Ptr objects (it should throw as this
 operation does not make much sense)
 I don't think we need this - if we want to compare two pointed-to leases
 for equality, we can dereference them before comparison.

 > one for comparing Lease6 structures (it will do what compareLease6()
 does.
 Done - see above.

 > Do we really need hwaddr in Lease6? I think we should remove it and the
 sooner we do it the better.
 Probably not. Remove.  (Err.. BTW, you were the one who wrote lease_mgr.h
 and first put it in the Lease6 structure!)

 > Please test that it works for IA_NA leases. You check IA_TA and IA_PD -
 the two types that we don't care about.
 Done.

 > Before the test begins, make sure you delete everything from the lease6
 tables. There could easily be a leftover e.g. from previous test that
 failed in the middle. Make sure you also clean up after the test.
 Done.  Added clearAll() to the test fixture class.  This is run both
 before the test and after it.

 > The test doesn't actually put anything in the database...
 Now checked - added reopen() to the test fixture class.

 > Before putting anything in the database, check that there are no leases
 present that you are about to add. Even if you add "DELETE * from Lease6"
 query at the beginning of each test, it is still useful that the backend
 does not return bogus leases.
 Done - see above with the clearAll() function.

 > Please add boundary checks. Verify that:
 > 2^32^-1 can be stored in iaid_, preferred and valid lifetimes.
 Checked.  Values near 2^32^-1 have been used to ensure that the different
 fields can be differentiated.

 > 128 byte long DUID can be stored and retrieved and that its content is
 intact
 Done.

 > that hardware address of 20 bytes can be stored and retrieved
 A few lines ago, you suggested that the Lease6 hardware address should be
 removed.  This has been done.

 > check if the backend refuses to accept larger values.
 For what - apart from prefix_len, for everything the maximum value is set
 by the data type.

 > check that CLTT is able to properly store February 29th on leap years
 > check that the backend will refuse dat Feb. 29th on regular year.
 The backend accepts data in time_t format and will convert it to the
 appropriate local time.  Similarly, when it retrieves data, the result is
 converted to time_t format.  So these checks really boil down to whether
 MySQL can handle leap years, which is out of scope for these checks.

 > check that prefix_len larger than 128 will be refused.
 > check that IPv4 address will be refused in lease6.
 These properly belong in the Lease6 class (and it is becoming a class,
 rather than a struct).  The database layer should assume that given a
 Lease6 object, the data in it is correct.  When it retrieved data, it
 should be that the Lease6 object being created from the data detects that
 it is incorrect.  Otherwise, knowledge of what constitutes valid data in
 Lease4/6 is split into several places.  A new ticket (#2405) has been
 created for it.

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