[squid-dev] A new 'has' ACL

[Alex Rousskov]

On 05/02/2017 12:38 PM, Eduard Bagdasaryan wrote:
> after r14752 each transaction has its ALE created. 

When you say "each transaction", how do you define "transaction" in this
context?

AFAICT, r14752 deals with transactions originated by client requests and
broken client connections. Even if all those transactions are fully
covered, there are other transactions (e.g., ESI and Downloader
requests) that may or may not be covered (and seem to be completely
unrelated to r14752 changes!). Is there some kind of common interface
that essentially requires all transactions to have ALE now?


> Normally it is stored in
> ClientHttpRequest::al.  However, I see that many ACLFilledChecklist
> objects does not initialize their ACLFilledChecklist::al (assigning from
> the transaction ALE). That works for now in most cases, probably
> because there is the only ACLExternal which needs it and overrides
> requiresAle() to return true. On the other hand, the lack of proper
> ACLFilledChecklist::al initialization makes impossible correct using
> ALE component in our 'has' ACL.

Thank you for researching this. Does the following summary accurately
reflect your findings as far as the current code is concerned?

"The current ACL-driven code may feed an ACLFilledChecklist object
without ALE to an (ACLExternal) ACL that requires ALE."


> Evidently, if we fix this (e.g., with mandatory initializing
> ACLFilledChecklist::al in its constructor), we would not need to
> check ALE component either.

The question is whether we can "fix this" (i.e., guarantee correct ALE
presence when ALE is needed) _quickly_. I doubt that all
ACLFilledChecklist objects can easily reach the right ALE from their
constructing context. I am worried that fixing this will turn into a
serious, large project. Are you sure it will not?

If you are not sure, then I suggest keeping "has ALE" support. We can
easily make "has ALE" a no-op when/if the code is fixed to always have
access to ALE when ALE is needed.

I see no reason to resist supporting "has ALE" if Squid support for ALE
is currently broken and cannot be fixed easily/quickly. If you see flaws
in this logic, please point them out. I may be missing something.


Thank you,

Alex.


> On 01.05.2017 19:11, Alex Rousskov wrote:
>> On 04/30/2017 10:03 PM, Amos Jeffries wrote:
>>
>>> Is there an explicit need you have found for ALE to be on the
>>> component list? Since ALE is currently standing in as a "master
>>> transaction" object for most of the Squid code. It needs to be either
>>> created or provided/fetched from elsewhere whenever it is used.
>>> Having an ACL that bypasses that would defeat bug-finding of places
>>> where it is broken.
>> I would like to polish that question and correct its justification
>> logic: We can abuse admins as bug-finding guinea pigs only if we give
>> them tools to turn those bugs off. The "has" ACL is such a tool. Thus,
>> we need a "has" ACL component for any object that any ACL or ACL-driven
>> code uses (or should be using) conditionally. Hence, the question is:
>>
>> Does Squid ACL or ACL-driven code correctly use (or should be using) ALE
>> conditionally?
>>
>> If the answer is "yes", then ALE should be added to "has". If the answer
>> is "maybe", then ALE may be added to "has".
>>
>> Please note that it is not necessary to have a specific crash or warning
>> bug report for the answer to become "yes" or "maybe", justifying ALE
>> support in the "has" ACL. Squid code analysis is sufficient.
>>
>> I cannot answer that question without doing more research, but I am sure
>> Eduard can find the right answer.
>>
>>
>> Thank you,
>>
>> Alex.
>>
>> _______________________________________________
>> squid-dev mailing list
>> squid-dev at lists.squid-cache.org
>> http://lists.squid-cache.org/listinfo/squid-dev
> 
> _______________________________________________
> squid-dev mailing list
> squid-dev at lists.squid-cache.org
> http://lists.squid-cache.org/listinfo/squid-dev