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

[Amos Jeffries]

On 5/12/2015 8:42 a.m., Alex Rousskov wrote:
> On 12/04/2015 06:26 AM, Amos Jeffries wrote:
>> On 4/12/2015 7:51 p.m., Alex Rousskov wrote:
>>> 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?
> 
> 
>> Yes.
> 
> 
>>> If yes, what to do about
>>> backward compatibility? Break it because -n is not that widely used?
>>
>> Yes. 
> 
> Noted.
> 
> 
>> With a +n flag (akin to the +i, re-enabling what -n disabled) we
>> can follow -i/+i pattern with an auto-reset at the end of each line but
>> add a warning if the +n is omitted on the next one.
> 
> I am not against adding +n support, but I do not recommend forcing users
> to add +n, just like we do not force them to add +i.

Then do you thik -n is used rarely enough to warrant breaking BC silently?
Because having warnings about the line change with no way to silence
them in the case that it was intentional is not a good option.


> 
> Now for flags in-between acl values on the same line:
> 
>   acl old dstname foo -n bar -x baz
> 
> I think we should prohibit general flags between same-line values

Agree. Seems reasonable to at least require different lines for
different permutations of flags.

> (-i
> support preserved for backward compatibility, but I would probably warn
> about it anyway). Such support does not really add much value AFAICT,
> but the resulting ACL lines become more and more difficult for a human
> to interpret correctly because each flag "area" overlaps subsequent
> flags on the same line. KISS.


Maybe we should deprecate +i and warn admins to split the line where the
+i is?
If you agree, that change and WARNING can go in immediately.

> 
> General flags in front of lines loaded from an acl parameter file should
> be supported.
> 

You mean in the data file or the squid.conf?

If you mean the data file, that sounds like its moving the goalpposts of
this proposal. It can be planned to allow for future addition, but not
in scope for coding yet.

> 
> To protect admins from typos and such, we need to prohibit non-quoted
> acl values that
> 
> a) start with a dash followed by a single character (-x and -x=foo)
> 
> and possibly (for future flags)
> 
> b) start with two dashes followed by one or more characters (--long and
> --long=foo)
> 

Nod.

> Same for those starting with a + sign.
> 
> We can start with deprecation warnings (except for +n which will be
> mis-interpreted in old configurations read by newer Squid, I guess).
> 

I can't think of any ACL which would need its values to start with '+'
today. The n flag is only valid for ACLs handling DNS labels where '+'
is already invalid for starting a value.
 I'm struggling to see any other ACL that would have a value starting
with '+' either. So no need for an exception there I think.


> 
> Would you recommend any changes to the above approach?
> 

Agreed on approach. See above for possible redux.

> 
>> The internal implementation of that could be a node listing values to
>> check pre-rDNS and a node listing values only checked post-rDNS.
> 
> Most likely, this fix will not be done using multiple ACL nodes because
> each ACL class object is meant to combine everything about a single ACL
> named foo. We could add more "fake" or "implicit" ACL objects to handle
> this, but I think it would be better to change ACLData instead. That's
> how I would start anyway.
> 

Hmm. Okay I will shelve that idea until the code hits audit and I get a
better view of what you are thinking.

> 
>> Could this be something completed quickly enough for Squid-4?
> 
> IMO, this change can always go into Squid-4 because it is essentially a
> bug fix. If Factory does the work, and accounting for current overload
> and upcoming holidays, we can probably deliver #1 patch for review by
> the end of January 2016, but this is an estimate and not a promise. Do
> you want Factory to work on this?
> 

I assumed that since this proposal was coming from you it was a Factory
intention to do anyway. Just design details and timing to organize.

Well the size of patch diff is also a factor on the port. If it is going
to do hundreds of lines of logic change the best thing to do for
stability is procrastinate or incremental addition.

January sounds like it would be just scraping into the beta cycle. But I
do request that back-compat warning happen within the next few weeks, so
we can at least start to see if the assumptions are correct.

Amos