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

[Amos Jeffries]

On 4/12/2015 1:02 p.m., Alex Rousskov wrote:
> Hello,
> 
>     We are writing code to add a new ACL flag (-m) that converts opaque
> string value comparison into a CSV list membership test. The patch is
> going through internal polishing and will be posted soon. That work
> exposed a design bug in ACL flags handling. We are not going to alter -m
> code to address this bug because that -m code is almost done, but I
> wanted to start this discussion about what to do next.
> 
> 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
> 
> 
> This happens because we store one ACLFlags object per ACL object and
> update that flags object as we parse multiple acl lines with the same
> aclname. 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.

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


> 
> The situation is further complicated by the fact that different flags
> interpretation for different ACL parameter/data items is tricky. For
> example, if -n disallows DNS lookups, what does it really mean that we
> allow them for comparison with v1 but disallow them for comparison with
> v2? Does the lookup we do before comparing with v1 violates the lookup
> prohibition for v2?
> 
> In a sense, some flags like "no slow DNS lookup" are better to view in a
> "whole ACL" scope while others like "case insensitive" or "no DNS/IP
> conversion" are better applied to individual ACL data/parameter values.
> 
> 
> 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.
> 
> This approach requires serious coding. It changes how the existing
> configurations are interpreted, but it also supports existing and future
> configurations the way most admins expect (even though their
> expectations do not match what actually happens right now!).
> 
> [ N.B. This approach can be further optimized to group same-flag data
> items inside an ACLData object so that we do not have to store so many
> [refcounted] ACLFlags object pointers. Even more code, more complexity,
> but less RAM in some cases. We can discuss that optimization separately
> if #1 is picked as the overall direction. ]
> 

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.


> 
> 2. Continue to use one ACLFlags object for all acl lines with the same
> name. Add ACLFlags equality comparison operator. Warn the admin if they
> attempt to use different flags for different lines. Admins would have to
> split the misleading ACL into several ACLs with different names to avoid
> the warning.
> 
> This approach is much easier to implement and it matches the original
> squid.conf "multiple ACL lines with the same name are the same as a
> single line" design. However, it forces admins to write less natural(?)
> configurations and rewrite existing configurations to get rid of the
> warning.
> 
> 
> 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.

>     Simpler configuration interface.
>     Less RAM.
> 

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

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

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


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

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.


Although that said; 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. All that is required is that the ACL parser remembers the
-n/+n value for the duration of a single line like -i/+i does today in
order to set the per-value flag correctly on those values. Then the DNS
ACL checks with the flag from it new location before doing lookups.


Or if you are super worried about one boolean flag per ACL value adn
insist on #2 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.

Amos