[squid-dev] A new 'has' ACL

[Eduard Bagdasaryan]

On 02.05.2017 23:53, Alex Rousskov wrote:
> 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?
I have not found any ALE manipulations inside ESI and Downloader
code, so that is probably unrelated.
ClientHttpRequest is the object used in transactions, (including
Downloader and ESI code). This object owns its ALE, which is
created in the constructor. I have not found 'common interface'
you are talking about, but if all Squid transactions require
ClientHttpRequest object (which is probably true) then they surely
will have the corresponding ALE.
>> 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."

Yes.

>> 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?
I agree that it is possible large project, because I am not
sure that every ACL checking context have its ALE near at hand.
> If you are not sure, then I suggest keeping "has ALE" support.

... and have our new 'has' ACL broken from the very beginning too.
As I tried to explain earlier, how can we rely on
ACLFilledChecklist::hasAle(), if the ALE itself have not been provided to
ACLFilledChecklist before?

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


Eduard.