Re: [PATCH] AND and OR ACLs v15

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
Received on Tue May 14 2013 - 07:34:52 MDT