[squid-dev] [RFC] Where do ACL flags belong?

[Alex Rousskov]

On 12/03/2015 06:10 PM, Amos Jeffries wrote:
> On 4/12/2015 1:02 p.m., Alex Rousskov wrote:
>> AFAICT, admins expect the following configuration to work as described
>> in the #comments:
>>
>>   acl foo dstdomain    v1 # lookup/conversion allowed
>>   acl foo dstdomain -n v2 # no lookup/conversion
>>
>>
>> However, Squid interprets the above as:
>>
>>   acl foo dstdomain -n v1 # no lookup/conversion
>>   acl foo dstdomain -n v2 # no lookup/conversion
>>
>> or, if you prefer:
>>
>>   acl foo dstdomain -n v1 v2 # no lookup/conversion


>> The only exception is the -i flag that is parsed/stored
>> specially for historical reasons; let's ignore that exception for now.


> Lets not. It used to be -i was the only one that existed for many years.
> So it was the authoritative pattern or how flags operated in Squid. AIUI
> this is where the admins expectation comes from.

Admin expectations differ as they come from different places. Some Squid
admins read our -n documentation and expect the above line-specific flag
behavior. I know I did and was surprised to find out that Squid does
something else.


> The -n flags which are breaking that pattern are the exception. Not the
> other way around.

Sorry for not being clear enough (I copied that phrase from a bug report
with more context): -i is exceptional from implementation point of view
because it does not use the general ACLFlags interface (where the new
flags should be added), and not because its interface is "broken".


>> I see two primary ways to fix this:
>>
>> 1. Move ACLFlags from the ACL object to individual data items inside the
>> ACLData object. This move will allow multiple acl lines to have
>> different flags.


> It will also work much better with the allof design you insisted on
> using. Where a single ACL contains a full AND-OR boolean tree of decisions.

Sorry, I do not see the relevance.


>> In summary, this is a tough choice between:
>>
>> #1: Supporting what the admins thought they are configuring.
>>     Better configuration interface?
>>     More similar to how -i is handled today.
>>
>> #2: Backward compatible (but with "Is your config broken?" warnings).
>>     A lot less development work.
> 
> No. You will have to fully re-implement regex parsing not to perform -i/+i.

No. We could leave -i/+i handling as is.


>>     Simpler configuration interface.
>>     Less RAM.


> Not much less RAM to matter. Each of the split ACLs will have more
> overhead in RAM usage

A simple implementation of #1 will add about 16 bytes to each ACL data
value item. For example, an ACL with 1M URLs will grow by 16MB. As I
mentioned, this can be optimized, but at the expense of more complex code.


>> I lean towards #2 because it is backward compatible and simpler (the
>> ACLs are already "too complex" for many admins). I would have been sure
>> that this is the best approach if -i was not handled differently already.
>>
>> How do you think this should be fixed?

> I prefer #1, because it is already familiar to admins and does not force
> us all to do a non-backward-compatible manual config update (those as
> you may see below that is not mandatory). We have enough config updates
> happening without this.

If we do #1, do you think we should ignore the possibility that some
Squids will be broken because Squid interpretation of the configuration
file will change? We can add warnings, but then removing those warnings
will require changing the config...


> I believe -i is a lot more widespread in historic usage than -n.

Yes, of course. I was not proposing to change how -i works.


> I also see no use for a flag that changes the behaviour for an entire
> ACL type. We would probably be better off making a new type (dstdomain_str).


Not sure what you mean. A single -n flag changes behavior of a named ACL
(today) or a single data item of a named ACL (if we implement #1).


> I know that has issues of its own as side effects on *access complexity.
> Allowing one "name" to be shared between the dstdomain, dstdom_regex,
> and dstdom_str types would simplify those.
>  OR, using a namespace sub-type ('_' maybe?) that implies the flagged
> behaviour difference. A renaming dstdom_regex to dstdomain_regex etc.
> under the new model.

Sorry, you lost me here completely. Can you give an example?


> AFAICS it should not be much more difficult to
> re-implement -n using #1 than to re-implement regex parsing and -i to
> implement #2.

True, but I do not think we should touch the -i interface. It works as
expected.



> Or if you are super worried about one boolean flag per ACL value

I am not.

> adn insist on #2

I am not. It is an RFC, and I did express my concerns about each approach.


> then auto-converting the lists of values between -n/+n
> flags down into sub-nodes of the ACL tree would let the flag actually be
> on the node rather than the value entry. Without requiring the manual
> config migrations.

Sorry, I do not follow, but perhaps this is not important for now.


Thank you,

Alex.