BIND 10 #999: Integrate ACLs into b10-resolver

[BIND 10 Development]

#999: Integrate ACLs into b10-resolver
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  vorner                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20110628
                  Component:         |            Resolution:
  Unclassified                       |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  4.0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:12 vorner]:

 Replying to the specific code comments:

 > And, finally, some comments about the code:
 >  * In the `io_endpoint_unittest`, you introduced shared pointers.
 Wouldn't `auto_ptr` be enough? But if you like shared pointers more, I
 think it is OK in test, there's no performance problem in it.

 auto_ptr is not very friendly with STL containers (which often
 internally involve object copies).  So, in general, I'd avoid using
 auto_ptr with STLs.  It might happen to be safe in this specific case
 (I've not examined that possibility), but even if so, as you said I
 don't think it worth taking the risk in this context.

 >  * `EXPECT_THROW(IPAddress ipaddr(sa), isc::BadValue);` ‒ this doesn't
 need the (unused) variable ipaddr, it should be enough to have
 `EXPECT_THROW(IPAddress(sa), isc::BadValue);`

 Unfortunately my clang++/g++ don't work at least for my usual compiler.
 Strangely it requests the default constructor in this case:
 {{{
 ip_check_unittest.cc:207: error: no matching function for call to
 'isc::acl::IPAddress::IPAddress()'
 }}}

 >  * Should the `IPCheck<Client>::matches` really be in
 `server_common/client.{cc,h}`?

 Where else do you think it should belong?  As long as Client is
 defined in server_common, it should be at least somewhere we can
 reasonably refer to the Client's definition.

 But, considering we'll merge this and #769 quite soon and it's likely
 to use the definitions in lib/acl at that point.  So, in this case,
 this question will disappear.

 >  * Should we test localhost addresses as well? (and in the following
 test) They are default, so to check that they are default at the right
 place.
 > {{{#!C++
 > TEST_F(ResolverConfig, defaultQueryACL) {
 >     // If no configuration is loaded, the default ACL should reject
 everything.
 > }}}

 I don't understand these comments...in the first sentence are you
 suggesting we should explicitly test 127.0.0.1 and ::1 instead of
 192.0.2.1, etc?  We could do so, but in this context I don't see
 an essential difference between these cases.

 As for the default, if you mean the "default for b10-resolver" (that
 comes from the spec file), unfortunately it's mostly impossible to
 test in our current framework, because (as far as I know) there's no
 easy way to incorporate the default config values to the test cases
 (it's not specific to the ACL).  I think we should have a framework to
 test these (and I agree it would especially important for ACLs because
 if the expected default is not actually loaded due to a bug that would
 be a much more significant effect than other similar bugs), but I'm
 afraid that's beyond the scope of this ticket.

 >  * The name `compoundQueryAcl` might be confusing, considering that we
 have compound ACL checks. This ACL contains multiple entries, but none of
 them is compound in that sense. Would be `longerAcl` or `multiEntryACL` be
 acceptable?

 Okay.  I've changed it to multiEntryACL.

 >  * You enable debug logging in tests. But should we spam the outputs
 with the debug logging?

 I believe we should, because otherwise we could overlook regression in
 producing log messages (we in fact had such a bug, and since it was at
 the DEBUG level we didn't notice that until after merging it to
 master).  Actually, I believe we should do this for other cases, too,
 and created a general ticket for that (don't remember the number right
 now).  The increased noise level could be an issue as you hinted, so
 we may have to address that part while enabling debug logging in
 tests.  But I still believe it's important to enable as many logs as
 possible in our tests.

 > And, do we plan to support checking the receiving IP address and the
 „from“ port in future (surely not in this ticket)?

 Perhaps, especially for the receiving (destination) address.  But it's
 a future work, and will only be an actual task when we see a clear
 need for them.

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