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

[BIND 10 Development]

#1069: combine resolver ACL context with generic request context
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       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):

 Replying to [comment:6 vorner]:

 > > - 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).
 >
 > Well, the motivation for having the members there was to minimise need
 to change it (and therefore the interface) later. Also, we're planning to
 allow users plugging in their own ACL checks, so it would be nice if they
 were provided with the information they need.

 Point taken.  But in this case I suspect it's a premature preparation
 before seeing the real need for it.  As noted for the TSIG case, we
 can only guess until we really need and try to implement and use it,
 at which point it often requires an interface change anyway.
 Meanwhile, leaving the unused members for unseen future extension
 makes the structure fragile, e.g., via referencing a non initialized
 member accidentally.

 Also, if the intent is to open up the possibility of extended use for
 external plugins, I guess we'd need a bit higher level abstraction
 than exposing bare members directly (although I don't have a concrete
 idea for that right now).

 > However, I don't mind leaving this decision (eg. what members to put
 there) to later time.

 Okay, so I guess we've agreed at least on what to do in the scope of
 this ticket.

 > Anyway, few minor things:
 >  * Is this needed? There's a typedef for the same in the isc::acl:
 > {{{
 > typedef isc::acl::ACL<isc::acl::dns::RequestContext> QueryACL;
 > }}}
 >  * Is there a double negative?

 Removed it.  I thought the shortcut still helps keep lines shorter,
 but on second look it doesn't seem to be so advantageous.

 > {{{
 >      * No exceptions from \c loadCheck (therefore from whatever creator
 is
 >      * used) and from the actionLoader passed to constructor are not
 caught.
 > }}}

 I believe it was originally written by you and I don't think I touched
 it:-)  But, yes, this sentence doesn't seem to correct, and I fixed it
 in this branch.

 >  * Maybe calling explicit `FAIL()` in getSockAddr as well as returning
 the dummy value, to explain why it failed?

 FAIL() doesn't work because it needs to return a gtest object.  We
 could use ADD_FAILURE(), but, on second thought, I now think it's even
 better to throw an exception so that the calling test will surely
 fail; now this function is a semi public utility, we cannot reliably
 assume how the caller will use it.  So I changed the code that way.

 BTW: do you have any opinion about renaming dns::getLoader()?  If you
 don't have a particular reason to keep the current name, I'd suggest
 changing it to getRequestLoader().

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