Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#1069 closed task (complete)

combine resolver ACL context with generic request context

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: Sprint-20110712
Component: resolver Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 3.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

This is a continuation from #999.

See the review discussion (e.g., the 2nd paragraph of
http://bind10.isc.org/ticket/999#comment:12)

Subtickets

Change History (12)

comment:1 Changed 9 years ago by stephen

  • Milestone changed from Next-Sprint-Proposed to Sprint-20110712

comment:2 Changed 9 years ago by jinmei

  • Owner set to jinmei
  • Status changed from new to accepted

comment:3 follow-up: Changed 9 years ago 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.

comment:4 Changed 9 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to reviewing

comment:5 Changed 9 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:6 in reply to: ↑ 3 ; follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

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).

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.

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

  • When we introduce a parameter for the TSIG key in RequestContext?, I suspect std::string for TSIG keys wouldn't be the best choice

OK, I just guessed at the time of writing it.

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?
         * No exceptions from \c loadCheck (therefore from whatever creator is
         * used) and from the actionLoader passed to constructor are not caught.
    
  • Maybe calling explicit FAIL() in getSockAddr as well as returning the dummy value, to explain why it failed?

With regards

comment:7 in reply to: ↑ 6 Changed 9 years ago by jinmei

Replying to 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().

comment:8 Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:9 follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to jinmei

At first, the code failed for me, but I think I removed an artefact of some experiments and pushed. It is OK otherwise. If the change is OK and it is as I think only forgotten experiment, please merge.

Thank you

comment:10 in reply to: ↑ 9 Changed 9 years ago by jinmei

Replying to vorner:

At first, the code failed for me, but I think I removed an artefact of some experiments and pushed. It is OK otherwise. If the change is OK and it is as I think only forgotten experiment, please merge.

Oops, it was an embarrassing leftover. Thanks for fixing it.

Now merged and pushed. Closing ticket.

comment:11 Changed 9 years ago by jinmei

  • Resolution set to complete
  • Status changed from reviewing to closed

comment:12 Changed 9 years ago by stephen

  • Estimated Difficulty changed from 0.0 to 3
Note: See TracTickets for help on using tickets.