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

[Alex Rousskov]

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.

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


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.
    Simpler configuration interface.
    Less RAM.


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?


Thank you,

Alex.