Re: [PATCH] AND and OR ACLs

On 12/21/2012 11:05 AM, Amos Jeffries wrote:
> On 21/12/2012 6:28 a.m., Tsantilas Christos wrote:
>> Patch Description
>> ===================
>>
>> This patch adds support for two new ACL types: one-of and all-of (as
>> agreed on the "or ACLs" squid-dev thread). Each new ACL type will take a
>> list of optionally negated ACL names as parameters. All ACL name
>> parameters must be already defined.
>>
>> acl AorB one-of a b
>> acl CorNotD one-of c !b
>> acl EandNeitherAnorB all-of e !AorB
>>
>> This is a Measurement Factory project
>>
>>
>> Comments
>> =========
>>
>> Unfortunately this is a patch we are not happy with. Alex have many
>> concerns, but I believe that it is not so bad.
>>
>> In the followings I am trying to explain the patch.
>>
>> New acls interpretation
>> ------------------------
>>
>> The patch is implemented as follows:
>> - The new acls converted to Checklists in order to computed.
>> - The "any-of" acl converted as follows:
>> acl any-of acl1 acl2 acl3
>> To:
>> Checklist allow acl1
>> Checklist allow acl2
>> Checklist allow acl3
>> Checklist deny all
>>
>> - The "all-of" acl converted as follows.
>> acl "all-of" acl1 acl2 acl3
>> To:
>> Checklist allow acl1 acl2 acl3
>> Checklist deny all
>>
>> The above looks trivial to implement it but there are problems when
>> someone tries to develop it, mostly because of the current acls check
>> scheme used in squid.
>
> I really have a big issue with designing it this way as well. The
> overheads of setting up many Checklists are way out of proportion
> compared to the problem avoided...

It creates a new ACLFilledChecklist option for each group-acl check.
There are many drawbacks. Also the "run fast check first and then a slow
check" is problematic too.

However there is not easy solution, if someone does not want to change
the current Checklist.cc code and does not want to have repeated code in
many places (eg copy a little modified code from checklist.cc to Group.cc )

Other ideas are welcome...

>
>> Squid acls checking
>> --------------------
>>
>> This is what we are doing currently in squid. Assume the following
>> access list:
>> checklist allow acl11 acl12 acl13 acl14
>> checklist allow acl21 acl22 acl23
>> ....
>>
>> When we are checking the checklist we are doing the following:
>> 1) Do a fast check for the acl11, acl12 etc
>> 2) Assume that the acl13 requires an async lookup. We are scheduling
>> the lookup. The fast acl check fails.
>> 3) After the async lookup for the acl13 finished:
>> - we are checking again all the access line.
>> - when the acl13 checked for second time and depending the acl we
>> are getting the value with a fast lookup from a cached value in
>> checklist object or from an other cache.
>>
>>
>> What this patch does
>> -----------------------
>>
>> Assume that the acl13 acl in the above example is an "one-of" or an
>> "any-of" acl.
>> When we are checking the acl13:
>> a) create the acl13::checklist which interprets the acl13 acl
>> b) copy all cached values from current checklist to acl13::checklist
>> c) To a fast lookup to acl13::checklist.
>> d) if it fails schedule slow checklist lookup for the
>> acl13::checklist
>>
>>
>> Comments,concerns etc
>> ----------------------
>>
>> In the above example when the async lookup for the acl13 finishes do not
>> start the checklist from the beginning of the current line, but start
>> the check from the acl13 and continue with the next acls.
>>
>> This is (and depending the implementation) will help us to implement a
>> little better "any-of" and "all-of" acls.
>>
>> My question is, is this any reason why squid is checking from the
>> beginning the access line when an acl needs lookup?
>> Is n't it add a performance overhead?
>
> It is an artifact of the ACL processing simplification Alex added a
> while back. Yes it is a performance overhead.

Yep also there is a reference in his comment....

>
>> Also for example for the source domain acl, we are getting the domain
>> always from the fqdncache cache. Is n't it a little slow? What if the
>> value did not found even in the second lookup, for example because
>> expired in cache, or it is replaced by other value?
>
> If the value has changed the effect of running the whole line again is
> no worse than processing the request slightly later. I think this is a
> very minor issue and one we can optimize out later as a pure performance

OK

> issue. IMO design these ACLs to operate as nodes in the ACL test tree
> which operate on the one Checklist same as all other ACLs.

Are you reffered to any-of, and all-of acls?
Well not so easy...

>
> ON teh code itself, please use the word "Logic" or something similar
> instead of "Group" in relation to this feature. We will be adding a
> "group" type ACL shortly for authentication purposes and having two will
> add confusion. Also, as discussed other logic operations are possibly
> going to be added if needed, not just set operations.

Lets see first if the patch is acceptable (or can be acceptable) and I
will make any required changes ...

>
>
> Amos
>
Received on Fri Dec 21 2012 - 16:52:52 MST