BIND 10 #1512: implement zone section processing in DDNS

[BIND 10 Development]

#1512: implement zone section processing in DDNS
-------------------------------------+-------------------------------------
                   Reporter:  jelte  |                 Owner:  jinmei
                       Type:  task   |                Status:  reviewing
                   Priority:         |             Milestone:
  medium                             |  Sprint-20120529
                  Component:  DDNS   |            Resolution:
                   Keywords:         |             Sensitive:  0
            Defect Severity:  N/A    |           Sub-Project:  DNS
Feature Depending on Ticket:  DDNS   |  Estimated Difficulty:  6
        Add Hours to Ticket:  0      |           Total Hours:  0
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:11 vorner]:

 Thanks for the check and review.

 > First, the distcheck doesn't work, it seems it is missing a file needed
 for tests or some environment variable is not set:
 > {{{
 > Running test: session_tests.py
 > EEEE
 > ======================================================================
 > ERROR: test_broken_request (__main__.SessionTest)
 > ----------------------------------------------------------------------
 > Traceback (most recent call last):
 >   File
 "/home/vorner/work/bind10/bind10-devel-20120405/_build/../src/lib/python/isc/ddns/tests/session_tests.py",
 line 61, in setUp
 >     WRITE_ZONE_DB_CONFIG)
 > isc.datasrc.Error: Failed to create DataSourceClient of type
 sqlite3:dlopen failed for
 /home/vorner/work/bind10/bind10-devel-20120405/_inst/libexec/bind10-devel/backends/sqlite3_ds.so:
 /home/vorner/work/bind10/bind10-devel-20120405/_inst/libexec/bind10-devel/backends/sqlite3_ds.so:
 cannot open shared object file: No such file or directory
 [...]
 > }}}

 Fixed at 40f1d42 (ignore the change to configure.ac, which was an
 accident and canceled in the following commit).

 > The `test_hash` ‒ as the hashes returned by it are deterministic, we
 could find a pair of names that return different values and check it.

 Hmm, I'm not sure if we can safely assume that - it depends on the
 underlying boost hash implementation.  But I've added a case for
 different hashes based on my local environment anyway.  If it turns
 out we were naive via buildbot or other developer's reports, we can
 then disable it.

 > `LIBDDNS_UPDATE_FORWARD_FAIL` ‒ „This is not necessarily mean invalid,“
 ‒ maybe the „mean“ is extra?

 Ah, right, removed "mean".

 > The description of `LIBDDNS_UPDATE_NOTAUTH` is confusing. Does have or
 does not have authority?

 You're right, too: it should be "doesn't have authority", and the
 following sentence should say "this is an unexpected event" (not
 "not").  Fixed these.

 > What is the motivation to have classes for formatting data
 (`ZoneFormatter` and `ClientFormatter`)? If it is performance, is it
 really faster to allocate the new python object than to do the actual
 conversion?

 Good point, it was for possible performance benefit, but I was aware
 the overhead of creating (and subsequently destroying) the additional
 python object might make the attempt moot.

 One possible way to even eliminate this overhead would be to
 pre-create the formatter object, and dynamically set the format
 parameter when it's possibly needed in a log message:

 {{{#!python
 class ClientFormatter:
     # ...
     def set(self, addr):
         self.__addr = addr
         return self

 # And, in the application:

 # Define this either module-wide or (if pythread safety is needed) per
 # session class:
 client_formatter = ClientFormatter(None)
 #...
                 logger.debug(logger.DBGLVL_TRACE_BASIC,
 LIBDDNS_UPDATE_ERROR,
                              client_formatter.set(self.__client_addr),
                              ZoneFormatter(e.zname, e.zclass), e)
 }}}

 This way we can ensure the overhead is basically a few number of
 pointer passing, function calls, and reference manipulations unless
 the log is actually needed to be dumped.

 But you might think any of these are just premature.  In that case I'm
 also okay with always converting them to string.

 > The `req_data` parameter is not handled in `UpdateSession.__init__`.

 Yeah I know, and it was intentional.  I've added a note on this in
 9bc7701.

 > Should the `return UPDATE_ERROR, None, None` be inside the `except`?
 This looks like it is common for both successful and unsuccessful code
 flow, but the successful one returns before that, I think this might be
 confusing.

 Right, I saw the possible confusion, but I placed it there as I had an
 expectation that we'd add some other error exceptions, and then return
 UPDATE_ERROR commonly:

 {{{#!python
         except UpdateError as e:
             # do something
         except SomeOtherError as e:
             # do some other thing
     # in all cases we return UPDATE_ERROR
     return UPDATE_ERROR, None, None
 }}}

 But we can defer such cleanups unless and until we know we need them.
 So, for now, I've simply moved it to the except clause.

 > In the session tests, what is the purpose of the `toText()` calls?
 Shouldn't the objects be directly comparable?
 > {{{#!python
 >       self.assertEqual(Opcode.UPDATE().to_text(),
 msg.get_opcode().to_text())
 >       self.assertEqual(expected_rcode.to_text(),
 msg.get_rcode().to_text())
 > }}}

 They are, but when the test fails the output is less informative.  If
 we change the code to:

 {{{#!python
         #self.assertEqual(Opcode.UPDATE().to_text(),
 msg.get_opcode().to_text())
         self.assertEqual(Opcode.NOTIFY(), msg.get_opcode())
 }}}

 we'll see:

 {{{
 AssertionError: <pydnspp.Opcode object at 0x1018bbd50> != <pydnspp.Opcode
 object at 0x1018bbd10>
 }}}

 On the other hand, I admit it's counter intuitive.  I've added a
 simple comment about the intent, but I'm also okay with removing
 to_text().

 > The comment is missing an „s“:
 > `secondaries: a list of 2-element tuple`

 To "tuple"?  That's right, added.

 > Would it be better to use `set` instead of dict here? Or is it expected
 to contain something else than `True` values?
 > {{{#!python
 >     self.__secondaries = {}
 >         for (zname, zclass) in secondaries:
 >             self.__secondaries[(zname, zclass)] = True
 > }}}

 No, it should have been a set.  Thanks for pointing it out.  Somehow I
 often forget the existence of this builtin type.

 > How will the zone config handle different classes if it can hold only
 one data source? Is it expected to hold multiple zone configs then?

 This is one of the open points, and I'd defer it at least until we fix
 how we handle multiple data sources.  In general, I intended to keep
 the `UpdateSession` class independent from these details and
 encapsulate any open issues in `ZoneConfig`.

 > It'll look up a zone twice (once to check it contains the zone and
 another time when the handling will happen). Is it OK?

 Do you mean that we now do find_zone() in `ZoneConfig` and we'll
 subsequently have to do get_updater() (which is not yet included in
 this branch)?  I realized it could be a waste, and I could have call
 get_updater() from the beginning.  But I was not sure if that was
 correct.  Depending on the post search processing we may reject the
 request even before starting the transaction.  For example, in BIND 9
 we check the generic query ACL (not the update ACL) before starting
 the update transaction and if it fails we reject the request.  Since a
 transaction is relatively expensive, we may want to delay it as much
 as possible.

 Perhaps the best way is to add some interface to just know the
 existence of a zone with some abstract form of "zone ID" when found,
 which the caller can subsequently use as a shortcut for more specific
 actions (get updater, finder, iterator, etc).

 That will be far beyond of this branch, and for this branch I don't
 have a strong opinion.  On one hand, the overhead of doing the search
 twice wouldn't be of much concern for our initial implementation; but
 that also applies to the overhead of starting an update transaction
 possibly too early.

 If you have a specific suggestion, I'm okay with adopting it.

 > Are there no tests for the zone config? Or is it expected to be really
 short-term?

 I (more or less implicitly) thought its feature is well covered in
 session tests, but I was probably lazy.  I've added dedicated tests
 so we can cover a bit unusual cases too.  It was indeed expected to be
 short-term (even if we still keep the same name of class the detail
 will be heavily changed), but in retrospect it was not really a
 justifiable reason for not writing tests.  So, thanks for raising
 this.

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