[squid-dev] [PATCH] External ACL helpers error handling & caching

[Amos Jeffries]

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