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

[Alex Rousskov]

On 12/03/2015 10:33 PM, Amos Jeffries wrote:
> 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:
>>>> 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).

#2 does not attempt to unify how -i and the new flags operate. In #2,
everything continues to match as it does today, but the admins are
warned when the -n flags configuration is inconsistent with how they
actually work.


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


This is the last open question on this thread AFAICT: What to do about
backward compatibility in approach #1 that you favor?



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

AFAICT, the above will perform an rDNS lookup (to match against
.example.com) in both cases. We are still matching each acl data entry
in order, right? Did you mean to place the dstdomain_str line first?


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


The -n approach is overall better than _str suffix approach because the
former makes it easy to mix-and-match multiple flags and to support
flags with values such as -m=delimiters.



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

#2 issues a warning when -n is used on some but not all lines of a given
ACL foo. No other changes in #2.


>  -i flag operates per-line, and resets at the EOL. 

Yes (which, BTW, also breaks the "multiple lines are the same as one
combined line" design principle).


> If we remove per-line/value flags then a lot of configs are broken.

Neither #1 nor #2 change how -i works.

#1 makes -n work per line, as intended.

#2 does not change how -n works, but issues a warning if -n is used
inconsistently across multiple lines.



> 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


AFAICT this is the same as optimized #1 I have mentioned earlier (except
for the _suffix part which I think we should avoid as discussed above).
To illustrate this even better, you need to add more lines and values:

  acl foo dstdomain v1
  acl foo dstdomain -n v2a v2b
  acl foo dstdomain v3

is interpreted after #1 changes as

  acl foo1  dstdomain v1
  acl foo2a dstdomain -n v2a
  acl foo2b dstdomain -n v2b
  acl foo3  dstdomain v3
  acl foo anyof foo1 foo2a foo2b foo3

Does this match what you think we should do? If yes, what to do about
backward compatibility? Break it because -n is not that widely used?


Thank you,

Alex.