On 14/05/2013 12:11 p.m., Alex Rousskov wrote:
On 05/01/2013 09:57 PM, Alex Rousskov wrote:
On 05/01/2013 07:28 PM, Amos Jeffries wrote:
On 21/12/2012 6:28 a.m., Tsantilas Christos wrote:
- The new acls converted to Checklists in order to computed.
The issue I had with the earlier version was that each ACL test involved
allocating a whole new ACLFilledChecklist object and initializing it.
What I had been evisioning when we discussed this earlier was a sleek
little design where the ACL AND/OR type was a pre-constructed tree with
the node match() function doing the logic of whether to walk down
sub-ACL A or sub-ACL B to produce its own result. The whole thing using
1 ACLFilledChecklist across the entire process in the same way that 1
ACLFilledChecklist is used today for the whole set of *_access lines no
matter how long or complex they are.
Great, this matches what I am working on.
The attached patch updates ACL handling to support the following:
* Expressiveness: Two new boolean ACLs (all-of and any-of) that allow
admins to group ACLs as needed, to express complex conditions more
naturally, with fewer squid.conf lines. Conditions such as "(a or b) and
(c or d)" are easily expressed now. Explicit groups of ACLs of different
types can now be configured, named, and used in any ACL expression.
* Correctness and performance: When a slow ACL (that has suspended
checks to wait for an async lookup) is ready to resume checking, Squid
resumes checking from that ACL, instead of rechecking all ACLs for the
same action (or the same squid.conf directive) again.
* Internals: Store ACL-related configurations as an expression tree,
streamlining the code and clearing the way for future math-style/natural
ACL conditions support. The usual boolean operators (and, or, and not)
form intermediate nodes while good old configurable ACLs become tree
leaves. The new all-of and any-of ACLs use the boolean operators (and
also become intermediate nodes, of course).
I have also attached an annotated and sanitized debugging log showing
how the new ACL tree traversal looks in cache.log.
In retrospect, I do not think it was possible to add AND/OR ACLs
correctly without most of the changes in this patch. I am sure there
will be more major ACL-related improvements in the foreseeable future,
and this patch is another step to make those improvements possible.
FWIW, I do not think I would be able to implement this without the
previous cleanup step (r12176).
I hope these changes address all concerns expressed so far.
HTH,
Alex.
P.S. While I started with pieces of his earlier patch, Christos is not
responsible for the bugs in the attached patch.
Audit nits:
in src/acl/BoolOps.h:
* copy-paste typo on AllOf documentation "conditions expressed on a
single http_access line are ORed." is not true. Conditions expressed on
a *single* http_access line are supposed to be ANDed.
in src/acl/Checklist.cc:
* s/curernt/current/
in src/acl/Makefile.am:
* please add the new files in alphabetical listing order.
in src/acl/Tree.c:
* Acl::Tree::treeDump() needs to produce "allow" / "deny" actions in
lower case to assist the project of making the config parser
case-sensitive for these types of things.
Please also run the scripts/source-maintenance.sh script over the branch
for this patch. If not now, then during commit to trunk. There are quite
a few formatting quirks in there.
Code looks great now. I'll give it a +0.5, but not a clear +1 because ...
Rant: (don't let this hold you up, just voicing my dissatisfaction with
the design decision).
I'm still stuck on muliple all-of lines ORing together for the AND acl
type. It really is much simpler to think of and describe "acl X foo ..."
as X being a test of a set of values with operation foo applied to the
whole set.
In your example log; the good1 line #1 scan reaches "matches: checked:
extBad1 = 0" (good1 AND operation has failed) then *keeps going* on
good1 line #2.
When one would expect from the configured type name that *all of* the
tests in good1 to match it comes as a small oddity that extBad1 can be
false and the good1 still produce a success/true match.
In abstract I think we should define ACL types as each performing a
*single* operation on a set of values. With this patch all-of is
applying a much more complex multi-type calculation which is not safe to
describe as an AND test. I can't put my finger on a good example case
but I'm sure this will come back to bite us (or a client) when we get to
adding tests like this with large sets of values:
acl magic all-of A
acl magic all-of B
acl voodoo all-of magic !B
when A=true, B=false ==> voodoo is true.
I don't think we should be forcing this level of mathematical knowledge
on people just to write ACLs.
Amos
