On 10/01/2017 12:49 a.m., Christos Tsantilas wrote:
> The helper protocol for external ACLs [1] defines three possible return
> values:
> OK - Success. ACL test matches.
> ERR - Success. ACL test fails to match.
> BH - Failure. The helper encountered a problem.
>
> The external acl helpers distributed with squid currently doesn't follow
> this definition. For example, upon connection error, ERR is returned:
>
> $ ext_ldap_group_acl ... -d
> ext_ldap_group_acl: WARNING: could not bind to binddn 'Can't contact
> LDAP server'
> ERR
>
> This is does not allow to distinguish "no match" and "error" either and
> therefore negative caches "ERR", also in the case of an error.
>
> Moreover there are multiple problems inside squid when trying to handle
> BH responses:
> - Squid-5 and squid-4 retries requests for BH responses but crashes
> after the maximum retry number (currently 2) is reached.
> - If an external acl helper return always BH (eg because the LDAP
> server is down) squid sends infinitely new request to the helper.
>
> This patch fixes the problems described above.
>
> This is a Measurement Factory project
>
Thank you for this long overdue fix.
+1. Though if possible I would like one extra change...
Please add the below method to class external_acl and use it to
de-duplicate the complex if-conditions in external_acl_cache_touch and
external_acl_cache_add about whether a response is non-cacheable:
bool
external_acl::maybeCacheable(const allow_t &result)
{
if (cache_size <= 0)
return false; // cache is disabled
if (result == ACCESS_DUNNO)
return false; // non-cacheable response
if ((result == ACCESS_ALLOWED ? ttl : negative_ttl) <= 0)
return false; // not caching this type of response
return true;
}
Could you also mention for the squid-dev record how much testing this
patch has had.
Cheers
Amos
_______________________________________________
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
