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

[Amos Jeffries]

On 4/12/2015 4:03 p.m., Alex Rousskov wrote:
> 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 defence of #2 you made the statement:
"
it matches the original squid.conf "multiple ACL lines with the same
name are the same as a single line" design.
"

That design was thrown away with the introduction of the allof ACL when
you insisted that each separate line in squid.conf was OR'd together.
Even though its single-line values were being AND'd together.

Which means for at least that ACL there is not single-line vs
multiple-line equivalence.


The current working model is actually that all config lines ("acl "
definitions included) are OR'd on a first-match basis. Each line is
sequential but independently operating on its set of per-line
values/parameters.


Of the two proposals #1 (with the implied changing of -n behaviour to be
per-line) is actually the best fit with the currently working
design/architectural model for squid.conf.


> 
>>> 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.

You cannot do that while unifying the model for how flags operate (to
the form of per-ACL).

The -i/-n are polar opposites in conceptual design.



> 
>>>     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.

In context of the up to 64GB of RAM the 1M values will use for their
value portion that 16M is trivial.

The whole ACLFlags object is not necessary in per-value/line entries. We
only use 3 bits today, and even allowing for extension the flag field is
4 bytes. Most of what is in ACLFlags seems to only be needed by the ACL
Prototype object used by the config parser/dumper logic.


> 
>>> 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?
> 


  acl foo dstdomain .example.com
  acl foo dstdomain 192.0.2.1

  acl foo dstdomain_str 192.0.2.3

  http_access deny foo

when looking up http://192.0.2.3/ the ACL only performs an exact match.
when looking up http://192.0.2.1/ the ACL performs exact-match, then
rDNS lookup and match.


Same as the below would do today:

  acl foo dstdomain .example.com
  acl foo dstdomain 192.0.2.1

  acl bar dstdomain -n 192.0.2.3

  http_access deny foo
  http_access deny bar


(bar currently needing to exist due to -n's per-ACL not per-line nature).


> 
>> 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.
> 

Then I must have completely misunderstood what your #2 proposal was about.
 -i flag operates per-line, and resets at the EOL. If we remove
per-line/value flags then a lot of configs are broken.

 (making) -n operate per-line and (not?) reset at the EOL.

> 
> 
>> 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.

We already have allof/anyof generating sub-trees of ACL nodes.

I am proposing option #3 be that we have the flag (or ACL type _suffix)
cause parser to generate a similar sub-tree of nodes to what anyof would
generate. BUT using only the specific named type of ACL that the line
was given.


So this:
 acl foo dstdomain 192.0.2.1
 acl foo dstdomain -n 192.0.2.3

or this:
 acl foo dstdomain 192.0.2.1
 acl foo dstdomain_str 192.0.2.3


both generates internally a tree identical to:

  acl foo:1 dstdomain 192.0.2.1
  acl foo:2 dstdomain -n 192.0.2.3
  acl foo anyof foo:1 foo:2

That way:
 * the ACLFlags can be on the node instead of the value. But have the
behaviour of being on the value(s).

 * we can leave -n as exceptional behaviour, but deprecate it in favour
of the _suffix ACL type.

 * we can avoid the outstanding problem with dstdomain vs dstdom_regex
needing different multiple *_access lines.


Amos