Re: [PATCH] AND and OR ACLs

On 12/22/2012 12:10 AM, Alex Rousskov wrote:
> On 12/21/2012 02:21 PM, Tsantilas Christos wrote:
>> On 12/21/2012 08:58 PM, Alex Rousskov wrote:
>>> Christos,
>>>
>>> I wonder if we can avoid code duplication by moving existing AND and
>>> OR logic from Checklist into a new ACL node type and then _always_ using
>>> that node type to wrap _all_ ACL rules? It would be kind of the opposite
>>> of what you have done: You are wrapping existing nodes into Checklist
>>> rule when an AND/OR ACL is found. This solution would wrap ALL rules
>>> into an AND or OR ACL node while Checklist will always check just _one_
>>> ACL node (usually AND or OR).
>>>
>>> In other words, this solution would automatically transfer
>>>
>>> http_access allow a1 a2 a3
>>> http_access allow b1 b2 b3
>>>
>>> into
>>>
>>> acl autoA all-of a1 a2 a3
>>> acl autoB all-of b1 b2 b3
>>> acl autoAB any-of autoA autoB
>>> http_access allow autoAB
>>>
>>> We would need to store the allow/deny keyword with the and/or node to
>>> make this work, but I did not show that detail in the above example in
>>> hope to avoid further confusion.
>>>
>>> Do you see what I am getting at?
>>
>> I think yes...
>> I don't now how easy is to implement it but it may worth it if we decide
>> that the any-of and all-of needed (Personally I am seeing many advantages).
>>
>> The only concerns I have is that I prefer a one-to-one interpretation of
>> squid.conf acls and acls related C++ structure. Interpreting the
>> http_access rules to an other scheme may confuse development, more bugs,
>> more difficult to solve problems etc...
>
> Yes, if we want to implement AND/OR grouping logic in one class, then we
> would have to use that class for all AND/OR groups, both on the ACL node
> level (no keywords) and on the ACL rules level (with keywords). This may
> be a little confusing/complex indeed.
>
> Another option is to duplicate AND/OR processing for each of those two
> levels. I am not against that, at least as a starting point, if no
> better solution is found. Once implemented, we can see exactly what code
> got duplicated and decide whether we can somehow reduce that duplication.
>
> Yet another option is to place level-agnostic, "pure" AND/OR processing
> into some abstract class that can handle any level (ACL node or ACL
> rule), but I would not do that because it would probably end up being
> even more confusing/complex.

The last two options maybe can work... The only restriction we have is
the required development time and the available budget. The last option
is the best but requires many changes in the current checklist code.

The major problems have to do with the the way the squid-acls code
decides if the check is finished or an asynchronous call is in progress
because these decisions may taken by acls not the checklist itself.

I think with the following will not have any problem:

 - modify a little the ACLCheckList::matchNodes method

 - Implement a method
      bool ACLChecklist::matchFirstOfAclList(ACLList *head, bool fast)

 - Make ACLChecklist::matchAclList and Checklist::matchFirstOfAclList
public methods

 - Use the above methods to implement AND and OR acls.

I believe with the above we have the minimum duplication code and will
work.

We are modifying a little the ACLChecklist class, but an
matchFirstOfAclList method looks that can be included in such class...

>
>
> Cheers,
>
> Alex.
>
>
Received on Wed Dec 26 2012 - 10:28:31 MST