On Mon, Jan 18, 2016 at 06:38:33PM +0100, Jakub Hrozek wrote:
> On Tue, Jan 12, 2016 at 10:30:29AM +0100, Petr Cech wrote:
>> On 01/06/2016 02:19 PM, Jakub Hrozek wrote:
>>> On Wed, Jan 06, 2016 at 11:03:45AM +0100, Sumit Bose wrote:
>>>> On Wed, Jan 06, 2016 at 10:47:13AM +0100, Pavel Březina wrote:
>>>>> On 01/05/2016 05:33 PM, Jakub Hrozek wrote:
>>>>>> On Tue, Jan 05, 2016 at 02:12:51PM +0100, Pavel Březina wrote:
>>>>>>> On 12/14/2015 03:36 PM, Petr Cech wrote:
>>>>>>>> Hi all,
>>>>>>>>
>>>>>>>> there is patch for
https://fedorahosted.org/sssd/ticket/2791 attached.
>>>>>>>>
>>>>>>>> Result of patch:
>>>>>>>>
>>>>>>>> The message:
>>>>>>>> Dec 14 14:16:11 vm-058-166 sssd[be[uma.dev]]: dereference
processing
>>>>>>>> failed : Input/output error
>>>>>>>> is replaced by
>>>>>>>> Dec 14 15:29:26 vm-058-166 sssd[be[uma.dev]]: LDAP server
claims to
>>>>>>>> support deref, but deref search failed. Disabling deref
for further
>>>>>>>> requests. You can permanently disable deref by setting
>>>>>>>> ldap_deref_threshold to 0 in domain configuration.
>>>>>>>>
>>>>>>>> But I am little afraid of this patch. Why? I tested this
work on RHEL 6
>>>>>>>> how it is described in the ticket.
>>>>>>>> If I tried to set
>>>>>>>> ldap_deref_threshold = 0
>>>>>>>> it was still falling into bad case and printed I/O error.
So, if we
>>>>>>>> considered patch is right, there is question if advice in
new message is
>>>>>>>> valid for IPA 3.x.
>>>>>>>>
>>>>>>>> Regards
>>>>>>>>
>>>>>>>> Petr
>>>>>>>
>>>>>>> This threshold parameter applies for groups membership, not
for views where
>>>>>>> dereference is always used in current code.
>>>>>>>
>>>>>>> The question is, why does sdap_x_deref_search_send fail with
EIO?
>>>>>>> Dereference should be supported with IPA...
>>>>>>
>>>>>> Because the dereferenced attribute doesn't exist on the
server IIRC.
>>>>>
>>>>> Then the debug message is misleading since dereference is in fact
working.
>>>>> We should return different error code if possible (ENOENT?) and this
case
>>>>> should be handled in views code.
>>>>
>>>> Unfortunately there is no specific LDAP error code for this case, see
>>>> e.g. comments in a50b229c8ea1e22c9efa677760b94d8c48c3ec89. Different
>>>> versions of 389ds return LDAP_UNAVAILABLE_CRITICAL_EXTENSION or
>>>> LDAP_PROTOCOL_ERROR, not sure what OpenLDAP does.
>>>>
>>>> I think the error handling code should be moved to the callers because
>>>> only they know how to handle different error conditions.
>>>
>>> +1
>>>
>>> we can also translate the LDAP-specific error codes into sssd private
>>> error codes instead of overloading ENOENT/EIO/ENOSUP (ENOSUP is IMO the
>>> only one that really fits the problem..)
>>>
>>>>
>>>> As an alternative now options/flags can be added to
>>>> sdap_deref_search_send() as it was done for sdap_get_generic_ext_send()
>>>> some time ago, but I would prefer the other.
>>>
>>> No, I also prefer to let the low-level function be dumb and just return
>>> an error which the caller deals with.
>>
>> Hi,
>>
>> thanks for all comments.
>>
>> I found processing of LDAP_UNAVAILABLE_CRITICAL_EXTENSION at
>> sdap_get_generic_op_finished() function. There is no processing of
>> LDAP_PROTOCOL_ERROR. So I added it.
>>
>> Now, the error codes for LDAP_RES_SEARCH_RESULT are ENOTSUP for our case and
>> EIO for the others.
>>
>> Next processing of those EIO and ENOTSUP is in sdap_deref_search_done(),
>> it is added by 4709ff46db0dbe073aef061b796d2fd7adeaf18f commit. I am not
>> sure, after Pavel Brezina comment, if the debug message for ENOTSUP case is
>> right here.
>>
>> Attached patch is applicable for master and sssd-1-12 too.
>
> I'm sorry, but this is not the right thing to do. If the request errors
> out with ENOTSUP, we print a log message, too, just different, but on
> top of that we also disable dereference..
>
> I don't think the problem can be solved at the lowest sdap level, but at
> the callers, similar to how Pavel solved the 'size exceeded' error in
> commit 1f1b41931d299d6356ac205b75b402adb2cc9234. We should add a flag to
> sdap_deref_search_state that would default to zero but
> would allow the caller to suppress the warnings. In
> sdap_deref_search_send() I think we can always leave the flag to zero, but
> in sdap_deref_search_with_filter_send() we should expose the flag so
> that the caller can set the suppress warnings flag. This would be
> evaluated in sdap_deref_search_done() and if the flag is set, then the
> sss_log() function calls would not be called.
>
> I hope this is doable in a couple of hours. Feel free to ping me if you
> need some help.
Petr won't be around for a couple of days and our downstream requested
this fix..so I took over (sorry Petr..) and attached is my version of
the patch.
The patch silences the syslog message (thx Jakub for giving access
testing machine).
Code looks good to me.
CI passed: