BIND 10 #2013: DDNS config and initial request handling

[BIND 10 Development]

#2013: DDNS config and initial request handling
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120612
  medium                             |            Resolution:
                  Component:  DDNS   |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  0
Feature Depending on Ticket:  DDNS   |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:7 vorner]:

 Thanks for the review.

 > There's a lot of hardcoding of paths and environment variable handling.
 I believe a lot of this functionality is provided by the bind10_config. Is
 there a reason not to use it?
 >
 > I myself usually prefer to get remote spec files from the config manager
 by its name instead of the path, it looks like less fragile to me. Just
 for my curiosity, is there a reason you prefer to use the path?

 Hmm, there was actually no reason for these, I simply didn't have the
 idea (and, actually, it just follows what other b10- apps do).  But
 from a closer look based on the comment, I realized it was not that
 trivial.

 For the hardcoded path, SPECFILE_PATH doesn't seem to be easily
 derived from bind10_config.  For the test version, we need to have
 'src/bin/ddns' hardocded anyway, and for the installed version, it
 will be something like /usr/local/share/bind10-devel, which doesn't
 seem to be easily derived from bind10_config.  Also, bind10_config
 doesn't have anything about B10_FROM_BUILD, so SOCKET_FILE_PATH and
 AUTH_SPECFILE_PATH will be special cases anyway (I'll talk about the
 remote config issue next).  We could unify SOCKET_FILE_PATH into
 B10_FROM_SOURCE_LOCALSTATEDIR and avoid relying on FROM_BUILD for it,
 but I was not sure about it, and since simplifying just this one
 doesn't seem to be a big cleanup.

 As for the remote config, I tended to agree and was about to change
 it, but then I was not sure whether it causes startup dependency
 issue.  In order for ddns to get the auth spec/config by name, the
 config manager should know it, and for that auth should have already
 started, right?  So, depending on which starts first, ddns and auth,
 or on the initial overhead of these proposes, adding the remote config
 might fail (at least I was afraid about it - and, as usual, the config
 stuff is so complicated and cannot easily be sure).

 So, in the end, I didn't touch the ddns code on these point for now.
 As for bind10_config, it's probably better if we extend it so we can
 hide these cases in a single module, and then use it in other
 modules.  That would be a separate cleanup task, though.  As for the
 remote config, if my concern isn't real I have no problem with
 changing it, but we should surely clarify these in documentation
 somewhere.

 > In the `get_datasrc_client` function, I believe the
 `HARDCODED_DATASRC_CLASS` is not in scope in the except clause, is it?

 Actually it is, it's not C/C++ :-)  (Otherwise
 test_get_datasrc_client_fail should fail) But on looking at the code
 again, I noticed the try block can be much smaller.  So I updated the
 code that way, which should resolve your confusion anyway.

 > I don't think I fully understand the comment „it was either totally
 broken as an any valid DNS message“.

 I've revised that part so it will be more specific: "it was either
 broken wire format data for a valid DNS message (e.g. it's shorter
 than the fixed-length header)".

 > Does this contain a trick I don't see?
 > {{{#!python
 > has_response = False if result == UPDATE_DROP else True
 > }}}
 > Or is it just over-complicated way to write?:
 > {{{#!python
 > has_response = (result != UPDATE_DROP)
 > }}}

 It was over-complicated.  This seems to my usual bad practice; I
 remember I've been pointed out something like this several times.
 Anyway, fixed.

 > There are merge markers left in the libddns_messages.mes:
 > {{{
 > =======
 > >>>>>>> 644c706... [1458] cleanup: reorder log message files
 > }}}

 Oops, thanks for catching it.  Removed it.

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