BIND 10 #1069: combine resolver ACL context with generic request context

BIND 10 Development do-not-reply at isc.org
Fri Jul 1 00:37:29 UTC 2011


#1069: combine resolver ACL context with generic request context
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  accepted
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20110712
                  Component:         |            Resolution:
  resolver                           |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  0.0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 trac1069 is ready for review.

 The first two commits are the most non trivial ones.  They implement
 the load and match logic (partially incorporated from the separate
 implementation done in #999) for the generic RequestContext.

 Others are mostly straightforward cleanup or adjustment as a result
 of the first two changes.

 Some points that may warrant discussions:

 - I fully simplified RequestContext.  It currently only consists of
   a member for the remote IP address.  The rationale is documented
   in the commit log of d2dc7a3ef911a5ab83527753f351bc99440d60fe.
   Other background point is that I wanted to minimize dependency on
   other modules.  This is one reason why I used IPAddress instead of
   asiolink::IOAddress (and yet another reason specific to IOAddress is
   handling IOAddress is expensive).

 - When we introduce a parameter for the TSIG key in RequestContext, I
   suspect std::string for TSIG keys wouldn't be the best choice
   because it's a DNS name and should be compared in a case insensitive
   manner and as its binary form (there can be multiple textual
   representation even without taking into account the case).  Having
   (a reference to) dns::Name object might not be ideal either,
   however, in terms of efficiency and additional dependency.  I guess
   the best way is to introduce a lightweight unique IDs for TSIG keys
   (that would just be integers, probably) within the system add an
   interface to TSIGKey to get the ID, and use it for ACL.  But that's
   a matter of a separate task anyway.

 - I renamed some shortcut typedefs, e.g., from "ACL" to "RequestACL".
   While I see the rationale of using the shorter name - that's
   probably because they are in a separate namespace -, in practice
   such too generic names often cause problems when used with 'using
   namespace' especially when there are exactly the same names in a
   different (but close) namespace.  Also, it's not clear whether there
   can only be one ACL (or Check or..) for Request in the dns
   namespace.  If and when that's not the case, saying one variant as
   "ACL" etc, would become quite ambiguous.  I wanted to change the
   name of "getLoader" for the same reason, but to minimize unrelated
   changes I didn't touch it for now.  If the sense of the change makes
   sense, I'd propose changing it to, e.g., getRequestLoader.

 - The Client class introduced in #999 is now (currently) almost
   unused.  It's only used for constructing RequestContext (and for
   helping generate log output), and it could easily be done without
   the help of Client.  But as (I believe) commented in #999 I intended
   to have this class for a more generic purpose, and so I kept it at
   the moment.  If this approach looks against the YAGNI principle, I'd
   consider reverting it until we really need it.

 - As for where to define the context for resolver (and perhaps auth,
   too): as I thought it further while working on this ticket, I now
   feel a bit more strongly that it should better be outside the ACL
   module.  This is based on the dependency inversion principle (DIP): it's
   quite predictable we'll need to have more enhancements to this
   context (e.g. we'll soon need to support TSIG).  Right now the
   context belongs to the lower-level component (i.e., ACL), but
   because of that, when we extend it, we'll also need to change the
   higher level "client" (i.e., b10-resolver).  Such a design is
   generally considered fragile to moving targets, and in the sense of
   DIP it's better if the upper level client has the interface, which
   is changed only when the client sees the need.  That said, I realize
   this type of thing is often a matter of preference, too, and I also
   see one advantage in having the context in acl: it would be easier
   to share it with python.  So, in this branch, I left the
   RequestContext at the layer of acl.

 Finally, I'm not planning to have a changelog entry for this ticket
 as it's essentially an internal refactoring.

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


More information about the bind10-tickets mailing list